Fix disabled rules, window geometry validation, and privacy logging#5
Conversation
- config: clamp persisted window geometry and Activity splitter height on load; drop absurd/non-finite values rather than passing them to Slint (avoids local DoS via tampered fade.toml). - config: a disabled AppRule now suppresses bucket inheritance instead of falling through, so unchecking an app in an enabled bucket — or assigning a disabled Ungrouped rule into one — actually exempts it. - main: search-filtered group/app/unassigned models now carry stable orig_index fields; Slint passes those to mutating callbacks so delete/move/toggle/icon/timeout edits target the visible row instead of the filtered display position. - main: icon picker on an in-bucket app inherits bucket.enabled when creating its AppRule, instead of hard-coding enabled=true (which silently promoted apps inside disabled buckets to managed). - main: Restore Defaults now also calls autostart::set_auto_start(false) so the OS startup entry is removed, matching the reset UI state. - main: validate the Activity splitter height callback input (finite + clamped) before persisting. - monitor: drop the window title from the info-level minimize/close log line — titles can leak document/URL/chat metadata to the on-disk Windows release log. Full title remains available at debug level and in the in-memory Activity log.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness and privacy improvements. Key changes include making disabled app rules authoritative exclusions in process resolution, adding range clamping for window geometry and UI state in the configuration, and ensuring UI callbacks remain stable during searches by tracking original indices. Additionally, window titles are moved to debug logs to prevent sensitive information leakage. Feedback was provided regarding a performance optimization in resolve_process to use eq_ignore_ascii_case for string comparisons, which avoids unnecessary allocations within the loop.
| timeout_mins: rule.timeout_mins, | ||
| action: rule.action.clone(), | ||
| }); | ||
| if rule.process.to_lowercase() == process_lower { |
There was a problem hiding this comment.
Using to_lowercase() inside a loop creates a new String allocation on every iteration. Since process_lower is already computed outside the loop, you can use eq_ignore_ascii_case to perform a more efficient comparison without extra allocations. While to_lowercase() is more robust for full Unicode, process names on Windows are typically ASCII-compatible, making eq_ignore_ascii_case a significant performance win in this polling path.
| if rule.process.to_lowercase() == process_lower { | |
| if rule.process.eq_ignore_ascii_case(process) { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 921a6d8602
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| return None; |
There was a problem hiding this comment.
Returning early with None for a disabled app rule changes the effective-rule semantics, but Monitor::poll_once still builds currently_managed by blindly unioning enabled buckets and enabled app rules (see src/monitor.rs step 0). In a bucket-enabled + app-disabled case, the process is now unmanaged by resolve_process yet still tracked as managed, so re-enabling that app later is not detected as “newly managed” and the idle grace-period reset is skipped; already-idle windows can be minimized/closed immediately after the user enables them. Please update managed-set construction to use the same resolution logic as resolve_process.
Useful? React with 👍 / 👎.
Addresses review feedback on PR #5: - monitor: build currently_managed via Config::resolve_process so a disabled AppRule that excludes its process is reflected in the managed set too. Without this, a bucket-enabled + app-disabled process was still tracked as managed; re-enabling the AppRule later wasn't detected as "newly managed" and the idle grace period was skipped, so an already-idle window could be minimized/closed immediately on toggle. - config: switch resolve_process comparisons to eq_ignore_ascii_case; process names are ASCII (.exe), and this drops a per-iteration String allocation on the hot polling path.
|
Addressed both review comments in ca76437:
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca76437819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeout_mins: rule.timeout_mins, | ||
| action: rule.action.clone(), | ||
| }); | ||
| if rule.process.eq_ignore_ascii_case(process) { |
There was a problem hiding this comment.
resolve_process switched from Unicode-aware lowercasing to eq_ignore_ascii_case, which only folds ASCII characters. On Windows, executable names can contain non-ASCII letters, so rules like "Åpp.exe" vs "åpp.exe" no longer match and the process can be treated as unmanaged (or miss a disabled-rule exclusion) depending on casing. This is a functional regression for internationalized process names introduced by this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces several stability and privacy enhancements, including stable indexing for UI elements to maintain correct behavior during searches, and sanitization of window geometry and activity splitter height in the configuration. It also improves privacy by moving window titles from info-level to debug-level logs and ensures the Windows auto-start entry is properly unregistered when restoring defaults. Feedback indicates that replacing to_lowercase() with eq_ignore_ascii_case() for process name matching is a regression for Unicode-named processes in non-English locales.
| timeout_mins: rule.timeout_mins, | ||
| action: rule.action.clone(), | ||
| }); | ||
| if rule.process.eq_ignore_ascii_case(process) { |
There was a problem hiding this comment.
While eq_ignore_ascii_case is faster than to_lowercase(), it only handles ASCII characters. Process names on Windows can contain Unicode characters (e.g., "Música.exe"). Using eq_ignore_ascii_case will fail to match these correctly if the case differs on a non-ASCII character, whereas the previous implementation was Unicode-aware. This is a regression in correctness for non-English locales.
| } | ||
| for proc in &bucket.processes { | ||
| if proc.to_lowercase() == process_lower { | ||
| if proc.eq_ignore_ascii_case(process) { |
There was a problem hiding this comment.
Similar to the change in app_rule resolution, using eq_ignore_ascii_case here for bucket process matching is a regression for Unicode process names. It will fail to match non-ASCII characters correctly if their case differs between the configuration and the running process.
Reverts the eq_ignore_ascii_case optimization on review feedback from both Codex (P2) and Gemini: Windows executable names can contain non-ASCII letters (e.g. "Música.exe", "Åpp.exe"), and ASCII-only case folding regressed matching for those processes. to_lowercase costs a few extra allocations per poll but only runs O(rules) per cycle, which is well below the polling-loop budget.
|
Reverted the @codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness and UI stability improvements. Key changes include refining the resolve_process logic to make disabled AppRules authoritative, adding input validation for window geometry and activity height, and updating UI models to track original indices for stable interaction during search and filtering. Additionally, the pull request enhances logging privacy by moving sensitive window titles to debug logs and fixes a bug where restore_defaults failed to disable Windows auto-start. I have no feedback to provide.
2 participants
Summary
This PR hardens configuration validation, fixes rule resolution semantics, and improves privacy in logging. It addresses three distinct issues:
Disabled app rules now act as explicit exclusions — A disabled
AppRulefor a process prevents fallback to bucket inheritance, fixing a bug where unchecking an app inside a bucket silently did nothing.Window geometry and UI state are now validated on load — Absurd persisted values (e.g., from a tampered config) that could make the settings window unusable or crash the renderer are dropped, falling back to defaults.
Activity logging no longer leaks window titles — Window titles (which may contain sensitive data like document names or URLs) are now logged only at debug level; info-level logs omit them.
Key Changes
config.rsresolve_process()now checks if a matchingAppRuleexists (enabled or disabled) before considering bucket inheritance. A disabled rule returnsNone, preventing the process from falling through to a bucket.clamp_ranges()validates window geometry (window_width,window_height,window_x,window_y) and activity splitter height (activity_current_height), dropping out-of-range values to prevent UI breakage.test_disabled_rule_suppresses_bucket_inheritance(),test_clamp_drops_absurd_geometry(),test_clamp_preserves_sane_geometry().ui/main.slintorig-app-indexandorig-indexfields toGroupAppModel,GroupModel, andUnassignedRuleModelto preserve original indices across search filtering.g-idx,a-idx) with struct fields (group.orig-index,app.orig-app-index) so callbacks dispatch the correct unfiltered indices, making search-stable filtering work correctly.src/main.rsbuild_groups()now tracks original bucket indices and app indices via.enumerate()before filtering, storing them in the model.build_unassigned_rules()pre-computes the unfiltered unassigned list, then filters and maps with enumerated indices to ensureorig_indexmatches what callbacks reconstruct.on_set_activity_current_height()now clamps the value to80.0..=800.0and validates it is finite before persisting.on_restore_defaults()now callsautostart::set_auto_start(false)to unregister the Windows startup entry when restoring defaults, preventing a mismatch between config and OS state.enabledstate when creating a new customizedAppRule, preventing a disabled bucket from being silently promoted to managed.src/monitor.rsAction::MinimizeandAction::Closelogging now omit the window title from info-level logs (moved to debug level) to avoid leaking sensitive activity into the Windows release log. The full title is still recorded in the in-memory Activity log for the UI.Implementation Details
orig-indexfields are populated during model construction and remain stable across search filtering, allowing the UI to dispatch callbacks with the correct unfiltered indices.MIN_W..=MAX_W, etc.) and drops invalid values rather than clamping them, ensuring the window falls back to its preferred size/position.https://claude.ai/code/session_01UmuM86GuBpcCaPzAQDawXZ