feat: 017 — configurable display spanning - #14
Merged
Conversation
Add Displays submenu to tray listing all connected monitors with checkmarks.
Toggling a monitor shows/hides a per-display overlay window within 1s.
Selection persists in settings.yaml and survives restart.
- domain: DisplayConfig { enabled_indices: Vec<usize> } added to Settings;
to_yaml/parse_yaml updated (comma-separated format); Settings no longer Copy
- overlay_manager: OverlayManager manages per-display WebviewWindow lifecycle
and per-window PigHitTester + polling thread; window ops dispatched to main thread
- tray: Displays submenu above focus list; toggle handler persists + applies config
- ui_bridge: update_pig_rects injects WebviewWindow to route rects per-window
- tauri.conf.json: "main" window renamed to "overlay-0"
- capabilities: "main" → "overlay-*" glob
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds multi-display overlay support: a DisplayConfig in Settings, per-monitor overlay windows managed by a new OverlayManager, tray "Displays" submenu to toggle monitors (persisted to settings.yaml), and plumbing so per-window pig hit-testing and rect updates route to the appropriate overlay. ChangesMulti-Display Overlay Architecture
Sequence DiagramsequenceDiagram
participant User as User
participant Tray as Tray Menu
participant Handler as Toggle Handler
participant State as DisplayConfigState
participant Writer as Settings Writer
participant AppMain as App Main Thread
participant OverlayMgr as OverlayManager
User->>Tray: click monitor checkbox
Tray->>Handler: dispatch DISPLAY_PREFIX event
Handler->>State: lock & toggle index
State->>State: update enabled_indices
Handler->>Writer: persist DisplayConfig to settings.yaml
Writer->>Writer: read/parse settings.yaml -> write updated settings
Handler->>AppMain: run_on_main_thread(apply overlays)
AppMain->>OverlayMgr: apply(monitors, config)
OverlayMgr->>OverlayMgr: for each monitor index
alt enabled
OverlayMgr->>OverlayMgr: ensure_shown -> create/show overlay-{idx}
OverlayMgr->>OverlayMgr: spawn hit-test polling thread
else disabled
OverlayMgr->>OverlayMgr: destroy/hide overlay-{idx}
end
AppMain->>Tray: rebuild_tray_menu()
Tray->>User: updated menu checkmarks
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/storage/src/settings_writer.rs (1)
45-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
preserves_all_keystest should explicitly verifydisplays.This test currently uses
DisplayConfig::default()and never assertsdisplays, so a regression could slip through undetected.Proposed test hardening
let original = Settings { @@ - displays: DisplayConfig::default(), + displays: DisplayConfig { + enabled_indices: vec![0, 2], + }, }; @@ assert!(final_settings.alerts.system_notifications); assert!(final_settings.widget.always_on_top); + assert_eq!(final_settings.displays.enabled_indices, vec![0, 2]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/storage/src/settings_writer.rs` around lines 45 - 71, The preserves_all_keys test doesn't assert the displays field so regressions can slip through; update the test (preserves_all_keys) to initialize original.displays with a non-default DisplayConfig (or at least inspect the default) and add assertions after reloading that final_settings.displays equals the original displays (compare relevant DisplayConfig fields or implement PartialEq check) to ensure write_settings/Settings::parse_yaml preserve the displays value; reference Settings, DisplayConfig, write_settings, and Settings::parse_yaml when locating the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/domain/src/settings.rs`:
- Around line 127-134: The match arm handling ("displays", "enabled") currently
skips assigning settings.displays.enabled_indices when the parsed indices vector
is empty, preventing an intentionally empty selection from being persisted;
change the logic so parsed Vec<usize> (built by the split/trim/parse/filter_map
sequence) is always assigned to settings.displays.enabled_indices regardless of
emptiness (i.e., remove the if !indices.is_empty() guard) so that an explicit
empty "enabled:" round-trips correctly.
In `@src-tauri/src/app/mod.rs`:
- Around line 99-108: available_monitors() is being silently ignored via
unwrap_or_default(), losing the error; change to capture its Result, log any Err
with context (e.g., using tracing::error or eprintln!) and then proceed to map
the Ok iterator into MonitorInfo (the mapping code that constructs MonitorInfo {
name: ..., size: ..., position: ... } can remain). In short: replace the
unwrap_or_default() call with a match or map_err that logs the error, then use
the Ok value (or an empty iterator) to build the Vec<MonitorInfo>.
In `@src-tauri/src/app/overlay_manager.rs`:
- Around line 53-62: Currently hidden overlays are never torn down so their
poller loops keep running; change the teardown logic so that when an overlay is
not enabled or its monitor no longer exists you fully close/destroy the webview
rather than just hiding it. In the loop in overlay_manager.rs that builds label
= format!("overlay-{idx}") (and the similar cleanup block later), replace the
hide-only branch that calls app.get_webview_window(&label).hide() with logic
that obtains the window and calls the API to close/destroy the window (so the
poller exits), and additionally add a cleanup pass that enumerates existing
overlay windows (matching "overlay-{n}") and closes any whose index is >=
monitors.len() or not present in config.enabled_indices; ensure this uses the
same ensure_shown/teardown patterns so associated resources/pollers are cleaned
up.
In `@src-tauri/src/app/tray.rs`:
- Around line 213-218: The toggle logic for display indices should validate that
idx is within the current MonitorsState bounds before mutating
config.enabled_indices: check that idx < monitors_state.0.len() and only then
push/sort or retain/remove; if idx is out of bounds simply ignore the add (or
remove if present) to avoid persisting stale indices—update the block around
enabled_indices handling (the idx variable and MonitorsState usage) to perform
this guard and keep OverlayManager::apply() semantics unchanged.
In `@src-tauri/src/ui_bridge/mod.rs`:
- Around line 12-17: The ui_bridge currently depends directly on
app::overlay_manager by wrapping OverlayManager in PigHitState and referencing
PigRect; extract a trait in the domain layer (e.g., trait RectUpdater with fn
update_rects(&self, label: &str, rects: Vec<PigRect>)) and move PigRect into the
domain if it isn't already, then change PigHitState to hold a boxed trait object
(e.g., Box<dyn RectUpdater + Send + Sync>) or a generic parameter instead of
OverlayManager, implement the trait for app::overlay_manager::OverlayManager,
and update any code that constructs PigHitState to pass the OverlayManager via
the trait so ui_bridge no longer imports OverlayManager directly.
---
Outside diff comments:
In `@crates/storage/src/settings_writer.rs`:
- Around line 45-71: The preserves_all_keys test doesn't assert the displays
field so regressions can slip through; update the test (preserves_all_keys) to
initialize original.displays with a non-default DisplayConfig (or at least
inspect the default) and add assertions after reloading that
final_settings.displays equals the original displays (compare relevant
DisplayConfig fields or implement PartialEq check) to ensure
write_settings/Settings::parse_yaml preserve the displays value; reference
Settings, DisplayConfig, write_settings, and Settings::parse_yaml when locating
the changes.
🪄 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: f9e59439-fb61-4875-80cf-a63e7388fff9
📒 Files selected for processing (12)
crates/commands/src/caps.rscrates/commands/src/lib.rscrates/domain/src/lib.rscrates/domain/src/settings.rscrates/storage/src/settings_writer.rssrc-tauri/capabilities/default.jsonsrc-tauri/src/app/menu.rssrc-tauri/src/app/mod.rssrc-tauri/src/app/overlay_manager.rssrc-tauri/src/app/tray.rssrc-tauri/src/ui_bridge/mod.rssrc-tauri/tauri.conf.json
- domain: add PigRect + RectUpdater trait; move PigRect out of pig_hittest so ui_bridge depends on domain, not on app internals - overlay_manager: implement RectUpdater; destroy (window.close) disabled overlays instead of hide so poller threads exit cleanly; add stale-window cleanup for indices beyond current monitor count; expose OverlayManagerState - tray: use OverlayManagerState for apply; guard idx < monitors.len() before toggling to prevent stale config entries from panicking - mod.rs: log available_monitors() error instead of swallowing with unwrap_or_default; wire Arc<dyn RectUpdater> into PigHitState - settings.rs: remove !is_empty() guard so explicit empty enabled: round-trips - settings_writer test: assert displays field survives write/read cycle
This was referenced May 3, 2026
Merged
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
WebviewWindow(overlay-0,overlay-1, …) with independentPigHitTester+ polling threadsettings.yamlunderdisplays.enabledand survives restart"main"window renamed to"overlay-0"throughout; capabilities useoverlay-*globTest plan
~/.adhd-ranch/settings.yamlupdates with correctdisplays.enabledvalue on toggletask checkgreenCloses #11
Summary by CodeRabbit
New Features
Bug Fixes