[032] Preferences window#34
Merged
archae0pteryx merged 2 commits intomainfrom May 4, 2026
Merged
Conversation
…rlay control - Add on-demand Preferences window (Cmd-, / tray → Settings…) - Sections: General (caps), Widget, Displays, Alerts, Debug - Auto-sizes to content via ResizeObserver + Tauri setSize - Reopens cleanly after close: destroys stale label, creates fresh window - Simplify tray: replace Settings submenu with single Settings… item - Add Tauri commands: get/update settings, get monitors, get/set debug overlay, toggle/query devtools - Debug overlay off by default; toggled via Settings > Debug > Debug Overlay using backend app.emit for reliable cross-window event delivery - DebugOverlayState managed in app; devtools toggle uses is_devtools_open
📝 WalkthroughWalkthroughThis PR adds a Tauri "Preferences" (Settings) window opened from Cmd-, or the tray "Settings…" item. It defines backend state and commands for getting/updating settings and monitors, persists settings to disk, applies runtime changes (e.g., always_on_top to overlays) and rebuilds the tray, and implements a React frontend (types, API wrappers, hook, component, entry HTML/entrypoint, styles) plus Vite wiring. ChangesPreferences Window Implementation
Sequence DiagramsequenceDiagram
actor User
participant Menu as Menu/Tray
participant Backend as Tauri Backend
participant Frontend as Settings React App
participant Storage as Settings File
User->>Menu: Click "Preferences…" or "Settings…"
Menu->>Backend: open_settings_window
Backend->>Backend: create/focus settings webview
Backend->>Frontend: load settings window assets
Frontend->>Backend: get_settings()
Backend->>Storage: read settings.yaml
Backend->>Frontend: return Settings
Frontend->>Backend: get_monitors()
Backend->>Frontend: return MonitorInfo[]
Frontend->>Backend: get_debug_overlay()
Backend->>Frontend: return bool
User->>Frontend: Edit a setting (e.g., always_on_top)
Frontend->>Backend: update_settings(modified_settings)
Backend->>Storage: persist settings.yaml
Backend->>Backend: apply always_on_top to overlay-N windows
Backend->>Backend: rebuild_tray_menu
Backend->>Frontend: confirm update
User->>Frontend: Toggle Debug Overlay
Frontend->>Backend: set_debug_overlay(enabled)
Backend->>Frontend: emit debug-overlay-toggle event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@issues/032-preferences-window.md`:
- Line 20: Update the spec so the tray behavior and acceptance criteria match
the implemented flow: replace the old "Settings submenu / Open Preferences…"
wording and any mention that "Displays stay in the tray (complex, per-monitor)"
with the new single tray entry labeled "Settings…" and move the Displays section
into the Preferences window; update the Acceptance criteria and any checklist
items (headings or sections named "Displays", "Settings submenu", and
"Acceptance criteria") to reflect that Displays are configured inside
Preferences rather than via the tray, and ensure all references and examples
throughout the document consistently use the new "Settings…" tray entry and
Preferences-based Displays workflow.
In `@src-tauri/src/ui_bridge/mod.rs`:
- Around line 157-173: The update_settings command currently calls the concrete
write_settings function directly, coupling ui_bridge to the storage
implementation; refactor update_settings to accept an injected storage trait
object (e.g., a SettingsPersister) via State (in addition to or replacing
SettingsPathState) and call its persist method instead of write_settings. Locate
the update_settings function and replace the direct
write_settings(&path_state.0, &settings) call with something like
persister.persist_settings(&path, &settings) (use the trait name
SettingsPersister and method name persist_settings or save_settings
consistently), and ensure the injected State type (e.g., State<'_, dyn
SettingsPersister> or a wrapper SettingsStorageState) is used to get the
concrete persister; keep the existing lock/update of SettingsState and error
mapping logic but surface storage errors via map_err as before.
- Around line 237-243: The command set_debug_overlay currently emits
"debug-overlay-toggle" regardless of whether state.0.lock() succeeded; change it
so the event is emitted only after successfully acquiring the lock and updating
the value (move the app.emit call inside the Ok branch that sets *v = enabled),
and in the Err/poisoned-lock case emit the actual stored value instead of the
requested enabled (or skip emit and log the error) so frontend cannot desync
from the backend state; locate the lock call state.0.lock(), the mutation *v =
enabled, and the app.emit("debug-overlay-toggle", ...) to implement this.
- Around line 186-190: The DisplayConfigState lock is currently being silently
ignored on poison; update the block that uses
app.try_state::<crate::app::DisplayConfigState>() / display_state.0.lock() to
handle a poisoned mutex the same way as the SettingsState path—either propagate
a Tauri error (return Err(...)) or at minimum log the failure with context—so
that a poisoned lock doesn't leave DisplayConfigState stale while SettingsState
was updated; refer to the existing SettingsState lock handling for the exact
error propagation/logging pattern and apply it to the DisplayConfigState branch.
In `@src/components/SettingsWindow.tsx`:
- Around line 167-169: The prop type mismatch: ToggleRow calls onChange(v:
boolean) but the handler onToggleDevtools is declared as () => void; either
update onToggleDevtools to accept a boolean parameter (e.g., onToggleDevtools(v:
boolean)) and use it to set state explicitly, or wrap the existing no-arg
handler in a short adapter when passing to ToggleRow (e.g., onChange={(v) =>
onToggleDevtools()}), making the intent explicit; locate the usage around
ToggleRow, the devtoolsOpen state, and the onToggleDevtools handler to apply the
change so the prop signature matches ToggleRow's onChange(v: boolean).
In `@src/hooks/useSettingsWindow.ts`:
- Around line 31-44: The callbacks update, setDebugOverlayEnabled, and
toggleDevtoolsOpen currently perform optimistic local state updates then call
updateSettings, setDebugOverlay, and toggleDevtools without handling failures;
change each to perform the state update optimistically but catch rejection to
either rollback the local state (restore previous value from a captured prev
variable) or surface a user-visible error (e.g., trigger a toast/notification)
so the UI does not remain inconsistent — specifically modify update to capture
prevSettings before setSettings and restore it on updateSettings failure, modify
setDebugOverlayEnabled to capture prev debug state from setDebugOverlayState and
restore on setDebugOverlay failure (or show toast), and modify
toggleDevtoolsOpen to capture previous devtools open state from setDevtoolsOpen
and restore on toggleDevtools failure (or show toast).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc0a8ef8-9df8-435a-9ac7-d85e527d7aba
📒 Files selected for processing (17)
issues/032-preferences-window.mdsettings.htmlsrc-tauri/src/app/menu.rssrc-tauri/src/app/mod.rssrc-tauri/src/app/tray.rssrc-tauri/src/ui_bridge/mod.rssrc/api/debug.tssrc/api/monitors.tssrc/api/settings.tssrc/components/SettingsWindow.tsxsrc/hooks/useDebugOverlay.tssrc/hooks/useSettingsWindow.tssrc/settings.tsxsrc/styles.csssrc/types/monitor.tssrc/types/settings.tsvite.config.ts
Comment on lines
+157
to
+173
| #[tauri::command] | ||
| pub fn update_settings( | ||
| settings: Settings, | ||
| app: AppHandle<Wry>, | ||
| state: State<'_, SettingsState>, | ||
| path_state: State<'_, SettingsPathState>, | ||
| ) -> Result<(), String> { | ||
| let old_displays = { | ||
| let Ok(mut s) = state.0.lock() else { | ||
| return Err("settings lock poisoned".to_string()); | ||
| }; | ||
| let old = s.displays.clone(); | ||
| *s = settings.clone(); | ||
| old | ||
| }; | ||
|
|
||
| write_settings(&path_state.0, &settings).map_err(|e| format!("persist settings: {e}"))?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Direct storage call bypasses trait interface.
write_settings is called directly rather than through an injected trait, which couples this command to the concrete storage implementation. As per coding guidelines, ui_bridge/ should depend on storage via traits.
Consider injecting a storage trait (e.g., SettingsPersister) as state to decouple this layer from the concrete implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/ui_bridge/mod.rs` around lines 157 - 173, The update_settings
command currently calls the concrete write_settings function directly, coupling
ui_bridge to the storage implementation; refactor update_settings to accept an
injected storage trait object (e.g., a SettingsPersister) via State (in addition
to or replacing SettingsPathState) and call its persist method instead of
write_settings. Locate the update_settings function and replace the direct
write_settings(&path_state.0, &settings) call with something like
persister.persist_settings(&path, &settings) (use the trait name
SettingsPersister and method name persist_settings or save_settings
consistently), and ensure the injected State type (e.g., State<'_, dyn
SettingsPersister> or a wrapper SettingsStorageState) is used to get the
concrete persister; keep the existing lock/update of SettingsState and error
mapping logic but surface storage errors via map_err as before.
Comment on lines
+31
to
+44
| const update = useCallback((next: Settings) => { | ||
| setSettings(next); | ||
| updateSettings(next).catch(console.error); | ||
| }, []); | ||
|
|
||
| const setDebugOverlayEnabled = useCallback((enabled: boolean) => { | ||
| setDebugOverlayState(enabled); | ||
| setDebugOverlay(enabled).catch(console.error); | ||
| }, []); | ||
|
|
||
| const toggleDevtoolsOpen = useCallback(() => { | ||
| setDevtoolsOpen((prev) => !prev); | ||
| toggleDevtools().catch(console.error); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optimistic updates without rollback on failure.
The callbacks update local state immediately, then persist asynchronously. If the backend call fails, the UI will show stale/incorrect state until the next mount. This is especially notable for toggleDevtoolsOpen where the actual devtools state may differ from displayed state.
For a settings window, this is typically acceptable UX (user can retry), but consider whether a toast/notification on failure would be helpful for user feedback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useSettingsWindow.ts` around lines 31 - 44, The callbacks update,
setDebugOverlayEnabled, and toggleDevtoolsOpen currently perform optimistic
local state updates then call updateSettings, setDebugOverlay, and
toggleDevtools without handling failures; change each to perform the state
update optimistically but catch rejection to either rollback the local state
(restore previous value from a captured prev variable) or surface a user-visible
error (e.g., trigger a toast/notification) so the UI does not remain
inconsistent — specifically modify update to capture prevSettings before
setSettings and restore it on updateSettings failure, modify
setDebugOverlayEnabled to capture prev debug state from setDebugOverlayState and
restore on setDebugOverlay failure (or show toast), and modify
toggleDevtoolsOpen to capture previous devtools open state from setDevtoolsOpen
and restore on toggleDevtools failure (or show toast).
- Log DisplayConfigState lock failure instead of silently ignoring (#3) - set_debug_overlay: only emit event when lock succeeds, not on failure (#4) - SettingsWindow: wrap onToggleDevtools in arrow to explicitly drop the boolean arg from ToggleRow onChange (#5) - Update 032 issue spec to match implemented tray/display behavior (#1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/ui_bridge/mod.rs`:
- Around line 245-256: toggle_devtools (and similarly get_devtools_open) only
target the single window "overlay-0"; either make the behavior explicit by
adding a short comment above toggle_devtools stating this is intentionally
limited to the first overlay for debugging, or change the implementation to
allow multi-overlay control by iterating over all overlay windows (reuse the
iteration pattern used around lines 179-183) or by accepting a window identifier
parameter and calling app.get_webview_window(window_id) so the caller can target
a specific overlay; update toggle_devtools and get_devtools_open accordingly and
keep the logic for opening/closing devtools
(win.is_devtools_open()/open_devtools()/close_devtools()) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2fd6a9e-a2ad-48c0-af2e-cbe04d8a1b4e
📒 Files selected for processing (3)
issues/032-preferences-window.mdsrc-tauri/src/ui_bridge/mod.rssrc/components/SettingsWindow.tsx
Comment on lines
+245
to
+256
| #[tauri::command] | ||
| pub fn toggle_devtools(app: AppHandle<Wry>) { | ||
| if let Some(win) = app.get_webview_window("overlay-0") { | ||
| #[cfg(debug_assertions)] | ||
| if win.is_devtools_open() { | ||
| win.close_devtools(); | ||
| } else { | ||
| win.open_devtools(); | ||
| } | ||
| let _ = win; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
DevTools toggle targets only overlay-0.
toggle_devtools and get_devtools_open only operate on the first overlay window. If this is intentional for debugging purposes, consider adding a brief comment. If multi-monitor devtools control is needed later, this would require iteration similar to lines 179-183.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/ui_bridge/mod.rs` around lines 245 - 256, toggle_devtools (and
similarly get_devtools_open) only target the single window "overlay-0"; either
make the behavior explicit by adding a short comment above toggle_devtools
stating this is intentionally limited to the first overlay for debugging, or
change the implementation to allow multi-overlay control by iterating over all
overlay windows (reuse the iteration pattern used around lines 179-183) or by
accepting a window identifier parameter and calling
app.get_webview_window(window_id) so the caller can target a specific overlay;
update toggle_devtools and get_devtools_open accordingly and keep the logic for
opening/closing devtools
(win.is_devtools_open()/open_devtools()/close_devtools()) intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cmd-,or tray → Settings… — created on demand, destroyed on close, reopens cleanlyapp.emitfrom Rust for reliable cross-window deliveryResizeObserver+getBoundingClientRect().height(includes padding); window fits content exactlyNew Tauri commands
get_settings,update_settings,get_monitors,get_debug_overlay,set_debug_overlay,toggle_devtools,get_devtools_openTest plan
Cmd-,opens Preferences windowsettings.yamltask checkgreenSummary by CodeRabbit
New Features
Documentation