Overlay floating control #908
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
plugins/windows/src/commands.rs (1)
153-194: 🛠️ Refactor suggestionConsolidate duplicate overlay and fake window bounds commands.
The commands
window_set_overlay_bounds/set_fake_window_boundsandwindow_remove_overlay_bounds/remove_fake_windowhave identical implementations, only differing in their command names. This duplication could lead to confusion and maintenance issues.Consider consolidating these into a single set of commands or clearly documenting the distinction between "overlay bounds" and "fake window bounds" if they serve different purposes. If they are truly the same, remove the duplicate commands:
-#[tauri::command] -#[specta::specta] -pub async fn set_fake_window_bounds( - window: tauri::Window, - name: String, - bounds: OverlayBound, - state: tauri::State<'_, FakeWindowBounds>, -) -> Result<(), String> { - update_bounds(&window, &state, name, bounds).await -} - -#[tauri::command] -#[specta::specta] -pub async fn remove_fake_window( - window: tauri::Window, - name: String, - state: tauri::State<'_, FakeWindowBounds>, -) -> Result<(), String> { - remove_bounds(&window, &state, name).await -}
🧹 Nitpick comments (10)
apps/desktop/index.html (1)
11-16: Inline style for transparent background is correct
The inline CSS forces thehtml,body, and#rootelements to be transparent, matching the transparent draggable control bar requirements.Consider moving these rules into a shared stylesheet or a CSS module to improve maintainability and avoid inline styles.
apps/desktop/src/routes/__root.tsx (1)
70-81: Consider adding error handling and event source filtering.The debug event listener implementation is correct and follows the established pattern in the file. However, consider these improvements:
- Add error handling for the
listenpromise- Filter events by source to ensure only control window events are processed
- Consider using a proper logging solution instead of
console.logfor production// Listen for debug events from control window useEffect(() => { let unlisten: (() => void) | undefined; - listen<string>("debug", (event) => { - console.log(`[Control Debug] ${event.payload}`); - }).then((fn) => { - unlisten = fn; - }); + listen<string>("debug", (event) => { + // Filter to only process events from control window if needed + console.log(`[Control Debug] ${event.payload}`); + }) + .then((fn) => { + unlisten = fn; + }) + .catch((error) => { + console.error("Failed to set up debug event listener:", error); + }); return () => unlisten?.(); }, []);apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
446-462: Remove debug console logs and consider error handling.The event listener implementation is solid, but consider these improvements:
- Remove console.log statements for production
- Add error handling for event listener setup
- const unsubscribeMicState = listen<{ muted: boolean }>("audio-mic-state-changed", ({ payload }) => { - console.log(`[Main Window] Received mic state change:`, payload); - refetchMicMuted(); - }); + const unsubscribeMicState = listen<{ muted: boolean }>("audio-mic-state-changed", ({ payload }) => { + refetchMicMuted(); + }); - const unsubscribeSpeakerState = listen<{ muted: boolean }>("audio-speaker-state-changed", ({ payload }) => { - console.log(`[Main Window] Received speaker state change:`, payload); - refetchSpeakerMuted(); - }); + const unsubscribeSpeakerState = listen<{ muted: boolean }>("audio-speaker-state-changed", ({ payload }) => { + refetchSpeakerMuted(); + });plugins/windows/js/bindings.gen.ts (1)
43-54: Runtime-validateboundsto guard against invalid rectanglesBecause this file is generated, adding logic here is undesirable, but the calling code should still validate that
width,height≥ 0 and the numbers are finite before invoking these commands. Passing NaN or negative sizes could crash or leave the native overlay logic in an undefined state.packages/utils/src/stores/ongoing-session.ts (1)
92-97: Reset the abortedenhanceControllerreferenceAfter
abort()theAbortControlleris no longer usable. Clearing it prevents accidental re-use and signals to callers that enhancement is no longer active.if (enhanceController) { enhanceController.abort(); -} + set({ enhanceController: null }); +}plugins/windows/src/ext.rs (1)
452-465: Document the platform-specific decoration behavior.The window builder applies decorations on macOS but not on Windows, which could lead to inconsistent UI across platforms.
Add a comment explaining why decorations are handled differently:
#[cfg(target_os = "macos")] { + // macOS uses native window decorations with custom traffic light positioning builder = builder .decorations(true) .hidden_title(true) .theme(Some(tauri::Theme::Light)) .traffic_light_position(tauri::LogicalPosition::new(12.0, 20.0)) .title_bar_style(tauri::TitleBarStyle::Overlay); } #[cfg(target_os = "windows")] { + // Windows uses custom decorations for consistent styling builder = builder.decorations(false); }plugins/windows/src/overlay.rs (2)
30-36: Consider making the polling frequency configurable.The hardcoded polling frequency of 10Hz might not be suitable for all use cases. Consider making this configurable through the plugin initialization or as a parameter to the function.
-pub fn spawn_overlay_listener(app: AppHandle, window: WebviewWindow) { +pub fn spawn_overlay_listener(app: AppHandle, window: WebviewWindow, polling_hz: Option<u32>) { window.set_ignore_cursor_events(true).ok(); + let polling_hz = polling_hz.unwrap_or(10); tokio::spawn(async move { let state = app.state::<FakeWindowBounds>(); let mut last_ignore_state = true; let mut last_focus_state = false; loop { - // Reduced polling frequency from 20Hz to 10Hz - sleep(Duration::from_millis(1000 / 10)).await; + sleep(Duration::from_millis(1000 / polling_hz)).await;
70-84: Optimize bounds checking with early exit.The bounds checking loop continues even after finding a match. You can break earlier to improve performance.
The current implementation is already optimized with an early
breakstatement on line 82 when a match is found. The logic is correct and efficient.apps/desktop/src/routes/app.control.tsx (2)
205-223: Extract drag constraints calculation for better maintainability.The clamping logic is duplicated and could be extracted into a utility function.
+ const clampPosition = (x: number, y: number) => { + const toolbarWidth = toolbarRef.current?.getBoundingClientRect().width || 200; + const toolbarHeight = toolbarRef.current?.getBoundingClientRect().height || 60; + return { + x: Math.max(0, Math.min(window.innerWidth - toolbarWidth, x)), + y: Math.max(0, Math.min(window.innerHeight - toolbarHeight, y)) + }; + }; + const handleMouseMove = (e: MouseEvent) => { if (isDragging) { - // Get toolbar dimensions for clamping - const toolbarWidth = toolbarRef.current?.getBoundingClientRect().width || 200; - const toolbarHeight = toolbarRef.current?.getBoundingClientRect().height || 60; - - // Clamp position to keep toolbar on screen - const clampedX = Math.max(0, Math.min(window.innerWidth - toolbarWidth, e.clientX - dragOffset.x)); - const clampedY = Math.max(0, Math.min(window.innerHeight - toolbarHeight, e.clientY - dragOffset.y)); - - const newPosition = { - x: clampedX, - y: clampedY, - }; + const newPosition = clampPosition(e.clientX - dragOffset.x, e.clientY - dragOffset.y); setPosition(newPosition); // Update bounds immediately during drag for smooth interaction setTimeout(updateOverlayBounds, 0); } };
300-301: Consider using a more robust session ID generation.The current timestamp-based session ID might not be unique enough in edge cases.
- const newSessionId = `control-session-${Date.now()}`; + const newSessionId = `control-session-${Date.now()}-${Math.random().toString(36).substring(2, 9)}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
apps/desktop/index.html(1 hunks)apps/desktop/src/components/editor-area/note-header/listen-button.tsx(4 hunks)apps/desktop/src/routeTree.gen.ts(13 hunks)apps/desktop/src/routes/__root.tsx(2 hunks)apps/desktop/src/routes/app.control.tsx(1 hunks)packages/utils/src/stores/ongoing-session.ts(2 hunks)plugins/listener/src/fsm.rs(2 hunks)plugins/windows/Cargo.toml(1 hunks)plugins/windows/build.rs(2 hunks)plugins/windows/js/bindings.gen.ts(3 hunks)plugins/windows/permissions/autogenerated/commands/remove_fake_window.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/set_fake_window_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_close.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_remove_overlay_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_set_overlay_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/reference.md(4 hunks)plugins/windows/permissions/default.toml(1 hunks)plugins/windows/permissions/schemas/schema.json(4 hunks)plugins/windows/src/commands.rs(3 hunks)plugins/windows/src/ext.rs(9 hunks)plugins/windows/src/lib.rs(4 hunks)plugins/windows/src/overlay.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugins/windows/src/ext.rs (3)
plugins/windows/src/overlay.rs (2)
app(30-30)spawn_overlay_listener(26-101)plugins/windows/js/bindings.gen.ts (1)
HyprWindow(76-76)plugins/windows/src/commands.rs (2)
window_show(5-11)window_close(15-21)
apps/desktop/src/routes/app.control.tsx (2)
plugins/windows/src/ext.rs (1)
position(186-223)plugins/listener/js/bindings.gen.ts (2)
setMicMuted(34-36)setSpeakerMuted(40-42)
🔇 Additional comments (29)
plugins/windows/Cargo.toml (1)
31-31: Dependency addition looks good
Thetokiocrate with thetimefeature is correctly added under[dependencies]to support the new async overlay management tasks.plugins/windows/permissions/default.toml (1)
14-17: New permissions added to default set
The overlay‐bounds and fake‐window permissions align with the new window and fake‐window commands exposed by the plugin.plugins/windows/permissions/autogenerated/commands/window_set_overlay_bounds.toml (1)
1-14: Generated permission file is valid
The schema path and both allow/deny entries correctly reference thewindow_set_overlay_boundscommand.plugins/windows/permissions/autogenerated/commands/window_remove_overlay_bounds.toml (1)
1-14: Generated permission file is valid
The schema path and both allow/deny entries correctly reference thewindow_remove_overlay_boundscommand.plugins/windows/permissions/autogenerated/commands/set_fake_window_bounds.toml (1)
1-14: LGTM! Well-structured permission configuration.The TOML file follows the standard Tauri plugin permission pattern with:
- Proper auto-generation warning
- Valid schema reference
- Consistent allow/deny permission definitions
- Clear descriptions and identifiers
plugins/windows/permissions/autogenerated/commands/remove_fake_window.toml (1)
1-14: LGTM! Consistent with established permission patterns.The file correctly follows the same structure as other permission files in the plugin, maintaining consistency across the codebase.
plugins/windows/permissions/autogenerated/commands/window_close.toml (1)
1-14: LGTM! Maintains consistency across permission files.The file structure is identical to the other permission files, ensuring a consistent permission management approach across all commands.
plugins/windows/build.rs (1)
3-3: LGTM! New commands properly integrated.The new window management commands are well-named and follow consistent patterns with existing commands. The additions support the overlay control functionality described in the PR objectives.
Also applies to: 13-16
plugins/listener/src/fsm.rs (2)
419-419: Good integration of control window visibility with session state.The entry action properly shows the control window when the session becomes active, using appropriate error handling for the window operation.
Also applies to: 473-479
488-491: Consistent window visibility management.The extended enter_inactive action correctly hides the control window alongside existing teardown logic, maintaining proper state synchronization.
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (3)
3-3: LGTM! Event imports support cross-window synchronization.The added event handling imports enable proper communication between windows for audio state management.
231-232: Good audio state synchronization implementation.The emit calls after setting audio states ensure all windows stay synchronized with the current mute states.
Also applies to: 242-242, 246-247
465-471: Excellent mutation implementation with event synchronization.The mutations correctly emit events after state changes and handle the new muted values properly. The pattern ensures all windows receive updates when audio states change.
Also applies to: 476-482
plugins/windows/src/lib.rs (3)
5-5: Good module organization and public API design.The overlay module is properly integrated and the selective public exports (
FakeWindowBounds,OverlayBound) provide a clean API for consumers.Also applies to: 10-11
39-39: Commands properly registered with specta builder.The new window and overlay management commands are correctly integrated into the command collection, enabling type-safe JavaScript bindings.
Also applies to: 49-52
65-78: Improved state management structure.The separate initialization blocks for each managed state improve code clarity and make the different state types more explicit. Each state is properly initialized with its default values.
plugins/windows/permissions/autogenerated/reference.md (2)
17-21: Consider whetherallow-window-closeshould be in the default allow-listThe new default permission block lists the four overlay / fake-window items, but omits the newly-added
allow-window-closepermission.
If closing arbitrary windows is supposed to be allowed by default (as is the case for destroy, show, hide, etc.), the omission is likely accidental and will force every consumer to add it manually.Would you double-check that the exclusion is intentional? If it was missed, add it here and to
default.toml.
86-107: 👍 Permission entries look consistent with the new commandThe
allow-window-close/deny-window-closerows are present, correctly named and follow the existing phrasing style. No issues spotted.apps/desktop/src/routeTree.gen.ts (1)
65-70: Generated route entry looks correctThe
/app/controlroute is wired identically to the other child routes and the manifest entries are all synchronised. Nothing to change here.plugins/windows/permissions/schemas/schema.json (2)
297-320: LGTM! Schema properly defines new window management permissions.The new permission constants for window close and bounds management operations are well-structured and follow the established naming conventions.
478-481: Verify that the default permissions align with security requirements.The default permission set now includes all the new window management permissions. This grants broad access by default, which may not align with the principle of least privilege.
Consider whether all these permissions should be included by default, or if some should require explicit opt-in for better security.
plugins/windows/src/commands.rs (2)
15-21: LGTM! Window close command properly implemented.The command follows the established pattern and includes proper error handling.
122-151: Consider extracting common bounds management logic.The
update_boundsandremove_boundshelper functions contain the core logic for managing window bounds state. These are well-implemented with proper concurrent access handling.plugins/windows/src/ext.rs (4)
28-30: LGTM! Control window variant properly integrated.The new Control variant is consistently added across all relevant methods with appropriate values.
Also applies to: 43-43, 149-149, 168-169, 182-183
294-440: Excellent refactoring of the show method!The refactored implementation is much cleaner and more maintainable. The match expression clearly shows the configuration for each window type, and the Control window setup with overlay functionality is comprehensive.
501-503: LGTM! Window close properly exposed in the trait.The implementation correctly delegates to the new close method.
395-431:❓ Verification inconclusive
Verify the safety of macOS-specific NSWindow manipulations.
The unsafe Objective-C runtime calls modify window button visibility and title bar transparency. While this appears necessary for the desired UI, it requires careful testing.
🏁 Script executed:
#!/bin/bash # Search for other uses of these NSWindow methods to ensure consistency ast-grep --pattern 'standardWindowButton_' ast-grep --pattern 'setTitlebarAppearsTransparent' ast-grep --pattern 'setMovableByWindowBackground' # Check if there are any related safety comments or documentation rg -A 5 -B 5 "NSWindow|standardWindowButton|setTitlebarAppearsTransparent|setMovableByWindowBackground"Length of output: 3629
Verify macOS NSWindow unsafe calls in ext.rs
No other occurrences of
standardWindowButton_,setTitlebarAppearsTransparent, orsetMovableByWindowBackgroundwere found outside this block. Because these Objective-C calls run inside anunsafeblock and directly manipulate the native window, please:
- Test on all supported macOS versions (Intel & Apple Silicon) to confirm buttons are hidden and title bar transparency behaves as expected.
- Consider guarding selectors with runtime or OS-version checks to avoid crashes if APIs change or become unavailable.
- Document the intent and add fallbacks (e.g., skip on unsupported versions) in case
msg_send!returns a null or errors out.Locations to review:
- File:
plugins/windows/src/ext.rs
Lines: 395–431apps/desktop/src/routes/app.control.tsx (2)
237-237: Ensure cleanup of fake window bounds on unmount.Good practice to clean up the fake window bounds when the component unmounts.
641-642:❓ Verification inconclusive
Verify the window type exists before showing.
The code assumes a "settings" window type exists. Consider adding error handling.
🏁 Script executed:
#!/bin/bash # Description: Verify that the "settings" window type is defined in the Windows plugin # Search for window type definitions ast-grep --pattern 'enum $_WindowType { $$$ }' # Also search for "settings" window references rg -A 3 'type.*settings' --type rustLength of output: 89
To locate the source of
windowShowand confirm that"settings"is a valid type, let’s search in the TypeScript codebase for the import, its definition, and anyWindowTypedeclarations:
🏁 Script executed:
#!/bin/bash # Find where windowsCommands is imported rg -n "import.*windowsCommands" -t ts* # Locate the definition of windowShow rg -n "windowShow" -t ts* -A3 # Search for any WindowType or similar union/type declarations rg -n "WindowType" -t ts* -A3 # Check for literal "settings" in union types or arrays rg -n "settings" -t ts* -A2Length of output: 248
Let’s locate where
windowsCommandsandwindowShoware defined and verify in your Tauri config that a"settings"window label exists:
🏁 Script executed:
#!/bin/bash # 1. Find the Tauri config file rg -l 'tauri.conf.json' # 2. If found, list its window labels to confirm "settings" is declared rg -n '"label"' -R src-tauri/tauri.conf.json -A2 -B2 # 3. Locate where windowsCommands is imported/defined in the desktop app rg -n "windowsCommands" -R apps/desktop/src -A3 -B3 # 4. Find all uses of windowShow to see other window types in use rg -n "windowShow" -R apps/desktop/src -A2 -B2Length of output: 1422
Let’s correct the search and locate the actual files before verifying the
"settings"window label:
🏁 Script executed:
#!/bin/bash # 1. Locate the Tauri configuration file find . -type f -name "tauri.conf.json" # 2. If found, dump window labels to check for "settings" CONFIG=$(find . -type f -name "tauri.conf.json" | head -n1) if [ -n "$CONFIG" ]; then echo "Found config at $CONFIG" grep -n '"label"' "$CONFIG" -C2 else echo "⚠️ tauri.conf.json not found" fi # 3. Find where windowsCommands is defined find apps/desktop/src -type f -iname "*windowscommands*.ts" # 4. List all windowShow calls and their arguments grep -R -n "windowShow" apps/desktop/srcLength of output: 2841
Verify “settings” window is defined and add error handling
I couldn’t locate a
label: "settings"entry in your Tauri config (apps/desktop/src-tauri/tauri.conf.json), so please manually confirm that a window withlabel: "settings"exists. To guard against typos or missing windows, wrap your call in atry/catch:- await windowsCommands.windowShow({ type: "settings" }); + try { + await windowsCommands.windowShow({ type: "settings" }); + } catch (err) { + console.error("Failed to open settings window:", err); + // e.g. show a fallback notification or silently fail + }This prevents unhandled promise rejections if the
"settings"type isn’t configured.
plugins/windows/src/overlay.rs
Outdated
| let focused = window.is_focused().unwrap_or(false); | ||
| if !ignore && !focused && !last_focus_state { | ||
| window.set_focus().ok(); | ||
| last_focus_state = true; | ||
| } else if ignore && last_focus_state { | ||
| last_focus_state = false; | ||
| } |
There was a problem hiding this comment.
Potential race condition in focus state management.
The focus state logic might have a race condition where last_focus_state is set to true but the window fails to gain focus. Consider checking the actual focus state after attempting to set focus.
let focused = window.is_focused().unwrap_or(false);
if !ignore && !focused && !last_focus_state {
- window.set_focus().ok();
- last_focus_state = true;
+ if window.set_focus().is_ok() {
+ // Verify focus was actually gained
+ if window.is_focused().unwrap_or(false) {
+ last_focus_state = true;
+ }
+ }
} else if ignore && last_focus_state {
last_focus_state = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let focused = window.is_focused().unwrap_or(false); | |
| if !ignore && !focused && !last_focus_state { | |
| window.set_focus().ok(); | |
| last_focus_state = true; | |
| } else if ignore && last_focus_state { | |
| last_focus_state = false; | |
| } | |
| let focused = window.is_focused().unwrap_or(false); | |
| if !ignore && !focused && !last_focus_state { | |
| - window.set_focus().ok(); | |
| - last_focus_state = true; | |
| + if window.set_focus().is_ok() { | |
| + // Verify focus was actually gained | |
| + if window.is_focused().unwrap_or(false) { | |
| + last_focus_state = true; | |
| + } | |
| + } | |
| } else if ignore && last_focus_state { | |
| last_focus_state = false; | |
| } |
🤖 Prompt for AI Agents
In plugins/windows/src/overlay.rs around lines 92 to 98, the code sets
last_focus_state to true immediately after calling window.set_focus() without
verifying if the window actually gained focus, which can cause a race condition.
To fix this, after calling window.set_focus(), check the window's actual focus
state using window.is_focused() and only update last_focus_state to true if the
window is confirmed to be focused.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/windows/src/ext.rs (1)
226-250: 🛠️ Refactor suggestionRefactor to eliminate redundant Control window checks.
The method checks for the Control window type twice (lines 227 and 233), making the logic harder to follow and potentially error-prone.
Apply this refactor to eliminate the redundant check:
- fn close(&self, app: &AppHandle<tauri::Wry>) -> Result<(), crate::Error> { - if !cfg!(target_os = "macos") || *self != HyprWindow::Control { - if let Some(window) = self.get(app) { - let _ = window.close(); - } - } - - if *self == HyprWindow::Control { - #[cfg(target_os = "macos")] - { - use tauri_nspanel::ManagerExt; - if let Ok(panel) = app.get_webview_panel(&HyprWindow::Control.label()) { - let _ = app.run_on_main_thread({ - let panel = panel.clone(); - move || { - panel.set_released_when_closed(true); - panel.close(); - } - }); - } - } - } - - Ok(()) - } + fn close(&self, app: &AppHandle<tauri::Wry>) -> Result<(), crate::Error> { + match self { + HyprWindow::Control => { + #[cfg(target_os = "macos")] + { + use tauri_nspanel::ManagerExt; + if let Ok(panel) = app.get_webview_panel(&self.label()) { + let _ = app.run_on_main_thread({ + let panel = panel.clone(); + move || { + panel.set_released_when_closed(true); + panel.close(); + } + }); + } + } + #[cfg(not(target_os = "macos"))] + { + if let Some(window) = self.get(app) { + let _ = window.close(); + } + } + } + _ => { + if let Some(window) = self.get(app) { + let _ = window.close(); + } + } + } + Ok(()) + }
🧹 Nitpick comments (1)
plugins/windows/src/ext.rs (1)
356-436:❓ Verification inconclusive
Review Control window configuration for potential issues.
The Control window has complex configuration with several considerations:
- Full monitor size: The window covers the entire monitor (lines 372-375) but is transparent, which could interfere with user interactions.
- Platform-specific behavior: Different configurations for macOS vs other platforms.
- Security consideration:
content_protected(true)(line 371) may prevent screen sharing/recording of this window.Consider these improvements:
- Document why full monitor size is needed for a 100x100 control
- Verify that transparent overlay doesn't interfere with underlying applications
- Consider if
content_protected(true)is intentional for a control window
🏁 Script executed:
#!/bin/bash # Description: Check for documentation about Control window behavior and overlay requirements # Expected: Find documentation explaining the full-screen transparent overlay approach echo "Searching for Control window documentation..." rg -i -A 5 -B 5 "control.*window|overlay.*control|floating.*control" echo "Searching for overlay-related functionality..." rg -i "overlay" --type rustLength of output: 2677
Document Control window overlay usage and assess interaction
It appears there’s no in-code documentation explaining why the Control window spans the full monitor (transparent overlay) or how it interacts with the overlay listener. Please verify and document:
- Update or add comments describing the rationale for sizing the window to the full monitor (plugins/windows/src/ext.rs, lines 356–436) and how
spawn_overlay_listener(plugins/windows/src/overlay.rs) drives the 100×100 control region.- Test on all target platforms that the transparent overlay does not unintentionally block or intercept input to underlying applications.
- Confirm whether
content_protected(true)is required for this overlay; if it’s not intentional for a control panel, consider disabling it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src/routes/__root.tsx(2 hunks)packages/utils/src/stores/ongoing-session.ts(1 hunks)plugins/listener/src/fsm.rs(1 hunks)plugins/windows/Cargo.toml(1 hunks)plugins/windows/src/ext.rs(10 hunks)plugins/windows/src/lib.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/stores/ongoing-session.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/windows/Cargo.toml
- apps/desktop/src/routes/__root.tsx
- plugins/listener/src/fsm.rs
- plugins/windows/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (7)
plugins/windows/src/ext.rs (7)
29-30: LGTM: Control variant addition is consistent.The new
Controlvariant follows the established pattern of other enum variants with proper serde renaming.
44-44: LGTM: Display implementation is consistent.The Display implementation for
Controlfollows the same pattern as other variants.
150-150: LGTM: Title assignment is appropriate.The "Control" title is appropriate for this window type.
275-293: LGTM: Analytics tracking for main window is well-implemented.The analytics tracking is properly implemented with async execution and error handling.
463-463: LGTM: Platform-specific decoration handling is appropriate.The conditional decoration handling (true for macOS, false for Windows) aligns with platform UI guidelines.
Also applies to: 470-473
483-483: LGTM: Window close method integration is clean.The trait method and implementation properly delegate to the instance method.
Also applies to: 564-566
434-434:✅ Verification successful
Verify overlay listener functionality.
The code spawns an overlay listener for the Control window. Ensure this function exists and handles the overlay behavior correctly.
🏁 Script executed:
#!/bin/bash # Description: Verify the overlay listener implementation # Expected: Find the spawn_overlay_listener function and its implementation echo "Searching for overlay listener implementation..." rg -A 20 "spawn_overlay_listener" echo "Searching for overlay-related modules..." fd overlay.rsLength of output: 2785
Overlay listener verified
The
spawn_overlay_listenerfunction is implemented inplugins/windows/src/overlay.rsand correctly handles togglingignore_cursor_eventsin a background task with a 10 Hz polling loop. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
259-267: Consider disabling buttons during mutation to prevent multiple clicks.The AudioControlButton components don't account for mutation loading states, potentially allowing users to rapidly click and create race conditions.
Consider passing the mutation loading state to disable buttons during execution:
<AudioControlButton isMuted={ongoingSessionMuted.micMuted} onClick={() => toggleMicMuted.mutate()} + disabled={toggleMicMuted.isPending} type="mic" /> <AudioControlButton isMuted={ongoingSessionMuted.speakerMuted} onClick={() => toggleSpeakerMuted.mutate()} + disabled={toggleSpeakerMuted.isPending} type="speaker" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(1 hunks)apps/desktop/src/routes/app.control.tsx(1 hunks)packages/utils/src/stores/ongoing-session.ts(3 hunks)plugins/listener/js/bindings.gen.ts(1 hunks)plugins/listener/src/events.rs(1 hunks)plugins/listener/src/fsm.rs(3 hunks)plugins/windows/build.rs(1 hunks)plugins/windows/permissions/autogenerated/reference.md(5 hunks)plugins/windows/permissions/default.toml(2 hunks)plugins/windows/permissions/schemas/schema.json(4 hunks)plugins/windows/src/lib.rs(4 hunks)plugins/windows/src/overlay.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/listener/src/events.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/windows/permissions/default.toml
- plugins/windows/build.rs
- packages/utils/src/stores/ongoing-session.ts
- plugins/windows/src/lib.rs
- plugins/listener/src/fsm.rs
- plugins/windows/permissions/autogenerated/reference.md
- plugins/windows/src/overlay.rs
- plugins/windows/permissions/schemas/schema.json
- apps/desktop/src/routes/app.control.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugins/listener/js/bindings.gen.ts (1)
plugins/db/js/bindings.gen.ts (1)
Word(164-164)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
packages/utils/src/contexts/ongoing-session.tsx (1)
useOngoingSession(32-46)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (2)
plugins/listener/js/bindings.gen.ts (1)
75-75: LGTM! SessionEvent type properly extended with mute state variants.The new
micMutedandspeakerMutedevent variants follow the existing pattern and are correctly typed. Since this is a generated file, the changes indicate proper backend implementation of the mute state event system.apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
242-245: LGTM! Direct store access simplifies state management.The direct access to mute states from the ongoing session store is cleaner than the previous hook approach and aligns well with the new event-driven architecture.
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces a floating meeting indicator as a draggable, always-on-top control bar within the desktop application. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ControlBar (React)
participant ListenerPlugin
participant WindowsPlugin
participant AppState
User->>ControlBar: Drag/move, click record/mute/settings
ControlBar->>ListenerPlugin: Query/set recording/mute state
ListenerPlugin-->>ControlBar: Emit session/mute events
ControlBar->>AppState: Update UI state
ControlBar->>WindowsPlugin: setFakeWindowBounds/removeFakeWindow
WindowsPlugin-->>ControlBar: Overlay/fake window state updated
ControlBar->>AppState: Show/hide settings popup, update preferences
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
plugins/windows/permissions/default.toml (1)
16-17: Missing autogenerated permission files for fake window bounds commands.The permissions
allow-set-fake-window-boundsandallow-remove-fake-windoware referenced here, but the corresponding autogenerated TOML files (set_fake_window_bounds.tomlandremove_fake_window.toml) are missing fromplugins/windows/permissions/autogenerated/commands/. This creates inconsistency in the permission system.Please ensure these autogenerated files are created and committed:
#!/bin/bash # Check for missing autogenerated permission files echo "Checking for autogenerated permission files..." ls -la plugins/windows/permissions/autogenerated/commands/ | grep -E "(set_fake_window_bounds|remove_fake_window)"packages/utils/src/stores/ongoing-session.ts (3)
146-157: Missing error handling forpauseSession- duplicate issueThe past review comment about missing
.catchblock forlistenerCommands.pauseSession()remains unaddressed. This can lead to unhandled rejections and unreliable store state.
160-162: Missing error handling forresumeSession- duplicate issueThe past review comment about missing
.catchblock forlistenerCommands.resumeSession()remains unaddressed.
130-141: Missing error handling forstopSession- duplicate issueThe past review comment about missing error handling for
listenerCommands.stopSession()remains unaddressed, which can cause UI and backend to become out-of-sync.apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
242-254: Add error handling and loading states to mutation hooks.The mutation hooks lack error handling and loading state management, which could lead to poor user experience if the mute commands fail or take time to execute.
Consider adding error handling and loading states:
const toggleMicMuted = useMutation({ mutationFn: () => listenerCommands.setMicMuted(!ongoingSessionMuted.micMuted), + onError: (error) => { + console.error('Failed to toggle microphone mute:', error); + // Consider showing a toast notification + }, }); const toggleSpeakerMuted = useMutation({ mutationFn: () => listenerCommands.setSpeakerMuted(!ongoingSessionMuted.speakerMuted), + onError: (error) => { + console.error('Failed to toggle speaker mute:', error); + // Consider showing a toast notification + }, });plugins/windows/src/overlay.rs (2)
50-60: Improve error handling for window operations.The error handling silently ignores failures which could hide important issues. Consider logging errors for better debugging.
let (Ok(window_position), Ok(mouse_position), Ok(scale_factor)) = ( window.outer_position(), window.cursor_position(), window.scale_factor(), ) else { if !last_ignore_state { - let _ = window.set_ignore_cursor_events(true); + if let Err(e) = window.set_ignore_cursor_events(true) { + log::warn!("Failed to set ignore cursor events: {}", e); + } last_ignore_state = true; } continue; };
85-91: Potential race condition in focus state management.The focus state logic might have a race condition where
last_focus_stateis set to true but the window fails to gain focus. Consider checking the actual focus state after attempting to set focus.let focused = window.is_focused().unwrap_or(false); if !ignore && !focused && !last_focus_state { - window.set_focus().ok(); - last_focus_state = true; + if window.set_focus().is_ok() { + // Verify focus was actually gained + if window.is_focused().unwrap_or(false) { + last_focus_state = true; + } + } } else if ignore && last_focus_state { last_focus_state = false; }apps/desktop/src/routes/app.control.tsx (2)
90-93: Duplicate of previous review: cleanup still doesn’t await the unsubscribe promises → possible leak.
505-513: Duplicate of previous review:IconButtonstill lacksaria-labelfor screen-reader accessibility.plugins/windows/src/ext.rs (1)
198-222: Duplicate of earlier review:closecontains two overlapping checks for the
Controlvariant, making the flow harder to read. Please consolidate.
🧹 Nitpick comments (6)
apps/desktop/index.html (1)
11-16: Add transparent backgrounds for root elements
This inline style ensures the window background is fully transparent, which is required for the overlay control. However, using!importantinline may complicate future theming and overrides.Consider moving these rules into your main stylesheet (e.g.,
src/global.css) and document why the transparency override is necessary.plugins/listener/src/fsm.rs (2)
482-485: Appropriate control window hiding on session inactive state.Hiding the control window when the session becomes inactive provides good UX by removing the overlay when it's not needed. The error handling uses
let _ = ...which ignores potential errors - this might be acceptable for window operations but consider logging errors if window control is critical.Consider adding error logging for better debugging:
- let _ = self.app.window_hide(HyprWindow::Control); + if let Err(e) = self.app.window_hide(HyprWindow::Control) { + tracing::warn!("Failed to hide control window: {:?}", e); + }
507-510: Appropriate control window showing on session active state.Showing the control window when the session becomes active provides good UX by making the overlay available when needed. Same consideration about error handling as with the hide operation.
Consider adding error logging for consistency:
- let _ = self.app.window_show(HyprWindow::Control); + if let Err(e) = self.app.window_show(HyprWindow::Control) { + tracing::warn!("Failed to show control window: {:?}", e); + }apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
258-268: Pass loading states to AudioControlButton for better UX.The audio control buttons should indicate when they're processing a mutation to prevent users from clicking repeatedly and provide visual feedback.
Pass the loading states to the buttons:
<AudioControlButton isMuted={ongoingSessionMuted.micMuted} onClick={() => toggleMicMuted.mutate()} + disabled={toggleMicMuted.isPending} type="mic" /> <AudioControlButton isMuted={ongoingSessionMuted.speakerMuted} onClick={() => toggleSpeakerMuted.mutate()} + disabled={toggleSpeakerMuted.isPending} type="speaker" />plugins/windows/src/overlay.rs (2)
30-30: Consider making the polling interval configurable.The hardcoded 100ms polling interval (1000/10) might not be optimal for all use cases. High frequency polling could impact performance, while lower frequency might make the UI feel less responsive.
Consider making this configurable:
+const OVERLAY_POLLING_INTERVAL_MS: u64 = 100; // 10 Hz pub fn spawn_overlay_listener(app: AppHandle, window: WebviewWindow) { // ... existing code ... loop { - sleep(Duration::from_millis(1000 / 10)).await; + sleep(Duration::from_millis(OVERLAY_POLLING_INTERVAL_MS)).await;
64-78: Consider extracting boundary check logic for better readability.The boundary checking logic could be extracted into a helper method to improve code clarity and reusability.
Extract the logic into a helper method:
+impl OverlayBound { + /// Check if a point is within the bounds, accounting for window position and scale + pub fn contains_point(&self, point: (f64, f64), window_pos: (f64, f64), scale: f64) -> bool { + let x_min = window_pos.0 + self.x * scale; + let x_max = window_pos.0 + (self.x + self.width) * scale; + let y_min = window_pos.1 + self.y * scale; + let y_max = window_pos.1 + (self.y + self.height) * scale; + + point.0 >= x_min && point.0 <= x_max && point.1 >= y_min && point.1 <= y_max + } +} // In the loop: for (_name, bounds) in windows.iter() { - let x_min = (window_position.x as f64) + bounds.x * scale_factor; - let x_max = (window_position.x as f64) + (bounds.x + bounds.width) * scale_factor; - let y_min = (window_position.y as f64) + bounds.y * scale_factor; - let y_max = (window_position.y as f64) + (bounds.y + bounds.height) * scale_factor; - - if mouse_position.x >= x_min - && mouse_position.x <= x_max - && mouse_position.y >= y_min - && mouse_position.y <= y_max - { + if bounds.contains_point( + (mouse_position.x, mouse_position.y), + (window_position.x as f64, window_position.y as f64), + scale_factor + ) { ignore = false; break; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
apps/desktop/index.html(1 hunks)apps/desktop/src/components/editor-area/note-header/listen-button.tsx(1 hunks)apps/desktop/src/routeTree.gen.ts(13 hunks)apps/desktop/src/routes/__root.tsx(2 hunks)apps/desktop/src/routes/app.control.tsx(1 hunks)packages/utils/src/stores/ongoing-session.ts(3 hunks)plugins/listener/js/bindings.gen.ts(1 hunks)plugins/listener/src/events.rs(1 hunks)plugins/listener/src/fsm.rs(3 hunks)plugins/windows/Cargo.toml(1 hunks)plugins/windows/build.rs(2 hunks)plugins/windows/js/bindings.gen.ts(3 hunks)plugins/windows/permissions/autogenerated/commands/remove_fake_window.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/set_fake_window_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_close.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_remove_overlay_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/commands/window_set_overlay_bounds.toml(1 hunks)plugins/windows/permissions/autogenerated/reference.md(5 hunks)plugins/windows/permissions/default.toml(1 hunks)plugins/windows/permissions/schemas/schema.json(4 hunks)plugins/windows/src/commands.rs(3 hunks)plugins/windows/src/ext.rs(8 hunks)plugins/windows/src/lib.rs(3 hunks)plugins/windows/src/overlay.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugins/listener/src/fsm.rs (2)
plugins/listener/js/bindings.gen.ts (1)
SessionEvent(75-75)plugins/windows/js/bindings.gen.ts (1)
HyprWindow(73-73)
plugins/listener/js/bindings.gen.ts (1)
plugins/db/js/bindings.gen.ts (1)
Word(164-164)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
packages/utils/src/contexts/ongoing-session.tsx (1)
useOngoingSession(32-46)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (27)
plugins/windows/Cargo.toml (1)
32-32:✅ Verification successful
Verify Tokio feature set includes async runtime
You’ve addedtokiowith thetimefeature enabled. Ensure that the default runtime features (rt,rt-multi-thread, etc.) remain available for spawning tasks, or explicitly enable them if needed.Run:
🏁 Script executed:
#!/bin/bash # Search for runtime-dependent tokio usages rg -n 'tokio::spawn' -A2 plugins/windows/srcLength of output: 271
Default Tokio runtime features remain enabled
Specifyingfeatures = ["time"]without settingdefault-features = falseleaves Cargo’s default features on, sort,rt-multi-thread, etc., are still enabled. Verified use oftokio::spawninplugins/windows/src/overlay.rs:24works as expected—no changes required.plugins/windows/permissions/autogenerated/commands/remove_fake_window.toml (1)
1-13: Auto-generated permissions config approved
This file correctly follows the structure of other command permission definitions and appears consistent with the schema. No manual edits are required.plugins/windows/build.rs (1)
3-3:❓ Verification inconclusive
Validate new command registrations
You’ve added five new commands (window_close,window_set_overlay_bounds,window_remove_overlay_bounds,set_fake_window_bounds,remove_fake_window) to theCOMMANDSarray. Ensure each entry matches its implementation incommands.rs, the generated JS bindings, and the corresponding permission files.Run:
Also applies to: 11-14
🏁 Script executed:
#!/bin/bash # Verify that each command string has a matching Rust function rg -nE 'pub fn (window_close|window_set_overlay_bounds|window_remove_overlay_bounds|set_fake_window_bounds|remove_fake_window)\b' -A2 plugins/windows/srcLength of output: 313
Let’s re-run the function lookup using
grepinstead ofrg:
🏁 Script executed:
#!/bin/bash # Verify that each NEW command string has a matching Rust function in plugins/windows/src grep -R -nE '^[[:space:]]*pub fn (window_close|window_set_overlay_bounds|window_remove_overlay_bounds|set_fake_window_bounds|remove_fake_window)\b' plugins/windows/srcLength of output: 169
Let’s locate your
commands.rsand check for any implementation of these names:
🏁 Script executed:
#!/bin/bash # List all Rust source files under plugins/windows/src find plugins/windows/src -type f -name '*.rs' # Search everywhere in plugins/windows for the new command names grep -R -nE 'window_close|window_set_overlay_bounds|window_remove_overlay_bounds|set_fake_window_bounds|remove_fake_window' plugins/windows/src # Specifically look inside commands.rs for any function or attribute usage grep -R -n '\#\[.*command\]' plugins/windows/src/commands.rs && \ grep -R -nE 'fn (window_close|window_set_overlay_bounds|window_remove_overlay_bounds|set_fake_window_bounds|remove_fake_window)' plugins/windows/src/commands.rsLength of output: 2134
Rust command functions validated
All five new commands are implemented and registered:
- plugins/windows/src/commands.rs
- line 15:
pub async fn window_close- line 144:
pub async fn window_set_overlay_bounds- line 155:
pub async fn window_remove_overlay_bounds- line 165:
pub async fn set_fake_window_bounds- line 176:
pub async fn remove_fake_window- plugins/windows/src/lib.rs
- lines 51, 60–63: registrations via
commands::…Please manually verify that:
- Your generated JS/TS bindings (e.g. in the plugin façade or
dist/index.d.ts) expose these exact command names.- Any permission definitions (e.g. in
tauri.conf.jsonallowlist or plugin-specific JSON) include all five commands.plugins/listener/src/events.rs (1)
22-25: Add mute state event variants
TheMicMutedandSpeakerMutedvariants withserde(rename)attributes are correctly defined and align with the new listener API contract.plugins/windows/permissions/autogenerated/commands/window_remove_overlay_bounds.toml (1)
1-14: LGTM! Well-structured permission configuration.The TOML file follows the established pattern for permission configurations with proper schema reference, clear identifiers, and appropriate command mappings.
apps/desktop/src/routes/__root.tsx (2)
6-6: LGTM! Proper import addition.The import of
listenfrom the Tauri API is correctly placed alphabetically with other Tauri imports.
71-82: LGTM! Well-implemented debug event listener.The useEffect hook follows React best practices:
- Proper cleanup with unlisten function
- Consistent pattern with other event listeners in the file
- Appropriate TypeScript typing for string payload
- Console logging is suitable for debug events
plugins/windows/permissions/autogenerated/commands/window_close.toml (1)
1-14: LGTM! Consistent permission configuration.The TOML file properly defines allow and deny permissions for the
window_closecommand, following the established pattern and naming conventions.plugins/windows/permissions/default.toml (2)
5-6: LGTM! Proper positioning of window close permission.The
allow-window-closepermission is appropriately positioned and the existingallow-window-hidepermission is correctly maintained.
14-15: LGTM! Overlay bounds permissions added correctly.The overlay bounds management permissions are properly added with consistent naming conventions.
plugins/windows/permissions/autogenerated/commands/set_fake_window_bounds.toml (1)
1-14: LGTM! Autogenerated permission configuration follows correct structure.The TOML permission file correctly defines both allow and deny permissions for the
set_fake_window_boundscommand with proper schema reference and standard formatting.plugins/windows/permissions/autogenerated/commands/window_set_overlay_bounds.toml (1)
1-14: LGTM! Autogenerated permission configuration follows correct structure.The TOML permission file correctly defines both allow and deny permissions for the
window_set_overlay_boundscommand with proper schema reference and consistent formatting.plugins/listener/src/fsm.rs (2)
406-406: Good integration of mute state event emission.The mute event emission after updating the watch channel ensures UI components can react to mute state changes in the correct order.
413-413: Good integration of speaker mute state event emission.Consistent with the mic mute pattern, properly emitting the event after updating the watch channel.
plugins/listener/js/bindings.gen.ts (1)
75-75: LGTM! Generated TypeScript types correctly reflect backend event structure.The
SessionEventtype union has been properly extended withmicMutedandspeakerMutedvariants that match the RustSessionEventenum, providing proper type safety for frontend mute state handling.plugins/windows/permissions/autogenerated/reference.md (2)
8-21: LGTM! Documentation properly reflects new plugin capabilities.The default permission list has been correctly updated to include the new window and overlay management permissions. The additions are consistent with the expanded plugin functionality.
32-108: LGTM! Permission table entries are well-structured.The new permission table entries follow the established format and provide clear descriptions for each allow/deny variant of the new commands. The structure is consistent with existing entries.
packages/utils/src/stores/ongoing-session.ts (2)
14-16: LGTM! Mute state properties properly added.The addition of
micMutedandspeakerMutedboolean properties to both the State type and initial state is correctly implemented and aligns with the new audio control functionality.Also applies to: 34-36
78-110: LGTM! Event handling for mute states is well-implemented.The new event handlers for session status changes and mute state updates are correctly structured using the mutative pattern. The logic properly updates the respective state properties based on the payload values.
plugins/windows/src/lib.rs (3)
5-5: LGTM! Clean overlay module integration.The overlay module import and re-exports are properly structured, making the necessary types (
FakeWindowBounds,OverlayBound) available to the plugin consumers.Also applies to: 10-11
51-51: LGTM! Command list properly updated for overlay functionality.The addition of new commands (
window_close,window_set_overlay_bounds,window_remove_overlay_bounds,set_fake_window_bounds,remove_fake_window) aligns well with the overlay control feature requirements. The removal ofwindow_resize_defaultappears intentional as part of the refactoring.Also applies to: 60-63
76-84: LGTM! Proper managed state setup for overlay functionality.The separate management of
ManagedStateandFakeWindowBoundsstates follows Tauri best practices. The block scoping ensures proper initialization of both states.plugins/windows/permissions/schemas/schema.json (3)
297-332: LGTM! New permission constants properly defined.The addition of permission constants for
remove_fake_window,set_fake_window_bounds, andwindow_closecommands follows the established schema pattern with proper allow/deny variants and descriptive documentation.
417-464: LGTM! Overlay permission constants well-structured.The permission definitions for
window_remove_overlay_boundsandwindow_set_overlay_boundsare correctly implemented with consistent descriptions and markdown formatting.
478-481: LGTM! Default permission description accurately updated.The default permission description and markdown have been properly updated to include all the new permissions, providing a comprehensive overview of the expanded plugin capabilities.
plugins/windows/js/bindings.gen.ts (1)
1-138: Generated file - no review needed.This is an auto-generated file (as indicated by the header comments). Changes should be made to the source files, not this generated output.
apps/desktop/src/routeTree.gen.ts (1)
1-396: Generated file - no review needed.This is an auto-generated file by TanStack Router (as indicated by the header comments). Changes should be made to the route files, not this generated output.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/utils/src/stores/ongoing-session.ts (1)
120-126:⚠️ Potential issueEvent listener leak when
startSessionfails
IfstartSessionrejects you reset the store withset(initialState)but never
call theunlistenfunction captured above.
The task keeps running and will dispatch events to a dead store instance.Add clean-up before resetting:
listenerCommands.startSession(sessionId) .then(() => { set({ status: "running_active", loading: false }); }) .catch((error) => { console.error(error); - set(initialState); + get().sessionEventUnlisten?.(); + set(initialState); });
♻️ Duplicate comments (6)
packages/utils/src/stores/ongoing-session.ts (1)
68-118:⚠️ Potential issueMissing
.catch()on thelisten()promise – still prone to unhandled-rejection leaks
The promise returned bylistenerEvents.sessionEvent.listenis not guarded with a.catch().
If the channel cannot be established (e.g. plugin fails to load) the rejection will escape the
store constructor and leave the UI in an unknown state.This has been called out in earlier reviews and is still unresolved.
listenerEvents.sessionEvent .listen(({ payload }) => { /* … */ }) .then((unlisten) => { /* store */ }) + .catch((err) => { + console.error("[OngoingSession] Failed to register sessionEvent listener", err); + set((state) => + mutate(state, (draft) => { + draft.loading = false; + }), + ); + });apps/desktop/src/routes/app.control.tsx (3)
134-138: Debug logging should be gated to development builds
console.logspam will hit end-users in production.
Wrap withif (import.meta.env.DEV)or remove entirely.
128-131: 🛠️ Refactor suggestionUnsubscribe promises not awaited – potential memory leak
You still fire‐and-forget theunlistenpromises. If they reject, the error is
silently swallowed and listeners may linger.Same feedback was provided earlier.
return () => { - unsubscribeSession.then(unlisten => unlisten()); + void unsubscribeSession.then(fn => fn()).catch(console.error); };
469-473:⚠️ Potential issue
alwaysOnToptoggle still does not affect the real window
The handler updates local state/storage but never invokes
windowsCommands.window_set_floating(or equivalent).
Users will assume the checkbox actually pins the overlay.This was highlighted in a previous review.
const toggleAlwaysOnTop = () => { const newValue = !alwaysOnTop; setAlwaysOnTop(newValue); localStorage.setItem("alwaysOnTop", newValue.toString()); + windowsCommands.window_set_floating("control", newValue).catch(console.error); };plugins/windows/src/ext.rs (2)
337-356:⚠️ Potential issueCritical issue: Control window size conflicts with UI expectations.
The Control window is configured with full monitor dimensions (
window_width,window_heightfrom monitor size), which contradicts the past review feedback indicating the UI expects a much smaller 200×80 viewport. This will cause layout and positioning issues.Based on the previous review comments, update the window size:
let window_width = (monitor.size().width as f64) / monitor.scale_factor(); let window_height = (monitor.size().height as f64) / monitor.scale_factor(); let mut builder = WebviewWindow::builder(app, self.label(), WebviewUrl::App("/app/control".into())) // ... other config ... - .inner_size(window_width, window_height) + .inner_size(200.0, 80.0) // ... rest of config ...
371-413:⚠️ Potential issueReplace unsafe macOS code with Tauri's built-in window customization.
The unsafe Objective-C code for hiding window buttons and making the title bar transparent should be replaced with Tauri's built-in configuration options to eliminate safety risks and deprecated API usage.
Remove the entire unsafe block (lines 373-412) and configure the window appearance in your
tauri.conf.json:{ "tauri": { "windows": [ { "label": "control", "titleBarStyle": "Overlay", "hiddenTitle": true, "decorations": false } ] } }This eliminates:
- Unsafe raw pointer manipulation
- Deprecated APIs (
#[allow(deprecated, unexpected_cfgs)])- App Store compatibility issues
- Memory safety concerns
🧹 Nitpick comments (5)
packages/utils/src/stores/ongoing-session.ts (1)
78-111: Preferswitchor early-return over longelse-ifladder
The mutation block is becoming hard to follow and will grow every time you add
another event type. Consider aswitch(payload.type)with separate helpers to
improve readability and reduce accidental fall-through.plugins/windows/src/overlay.rs (1)
66-76: Possible coordinate-space mismatch on HiDPI displays
window.outer_position()returns logical physical pixels, whereas the bounds
coming from the front-end are already logical CSS pixels. Multiplying by
scale_factora second time may shift the hit-test by the devicePixelRatio.Verify on a Retina display:
bounds.x // logical px window.outer_position().x // logical px scale_factor // 2.0If both sides are logical, drop the
* scale_factor.apps/desktop/src/routes/app.control.tsx (2)
145-218: High-volume debugemit()calls degrade IPC throughput
updateOverlayBoundsemits 10+ debug events every invocation and is called
frequently (drag, resize, popup toggle). On slower machines the IPC channel is
easily saturated.Remove or guard the debug emits behind a development flag.
475-614: Monolithic component – consider extracting sub-components & hooks
app.control.tsxis >600 LOC, maintains a dozen pieces of state, and registers
many global listeners. Splitting into:
useFloatingPositionhookuseRecordingControlshook<Toolbar />,<SettingsPopup />,<DragHandle />would reduce cognitive load, enable targeted unit tests, and minimise re-render
scope.plugins/windows/src/ext.rs (1)
198-231: Close method structure improved but still has room for simplification.The match-based approach is cleaner than the previous if-statement redundancy. However, the platform-specific logic could be further simplified by extracting the macOS panel handling into a separate method.
Consider extracting the macOS panel closing logic:
fn close(&self, app: &AppHandle<tauri::Wry>) -> Result<(), crate::Error> { match self { HyprWindow::Control => { crate::abort_overlay_join_handle(); - - #[cfg(target_os = "macos")] - { - use tauri_nspanel::ManagerExt; - if let Ok(panel) = app.get_webview_panel(&HyprWindow::Control.label()) { - app.run_on_main_thread({ - let panel = panel.clone(); - move || { - panel.set_released_when_closed(true); - panel.close(); - } - }).map_err(|e| tracing::warn!("Failed to run panel close on main thread: {}", e)).ok(); - } - } - #[cfg(not(target_os = "macos"))] - { - if let Some(window) = self.get(app) { - let _ = window.close(); - } - } + self.close_control_window(app)?; } _ => { if let Some(window) = self.get(app) { let _ = window.close(); } } } Ok(()) } #[cfg(target_os = "macos")] fn close_control_window(&self, app: &AppHandle<tauri::Wry>) -> Result<(), crate::Error> { // macOS panel logic here } #[cfg(not(target_os = "macos"))] fn close_control_window(&self, app: &AppHandle<tauri::Wry>) -> Result<(), crate::Error> { // regular window logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(6 hunks)apps/desktop/src/routes/app.control.tsx(1 hunks)packages/utils/src/stores/ongoing-session.ts(6 hunks)plugins/windows/Cargo.toml(2 hunks)plugins/windows/src/commands.rs(3 hunks)plugins/windows/src/ext.rs(9 hunks)plugins/windows/src/lib.rs(4 hunks)plugins/windows/src/overlay.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/windows/Cargo.toml
- apps/desktop/src/components/editor-area/note-header/listen-button.tsx
- plugins/windows/src/commands.rs
🔇 Additional comments (5)
plugins/windows/src/ext.rs (5)
29-30: LGTM: Control variant addition is well-integrated.The new Control variant is properly integrated across the enum implementation including serialization, display, and title methods.
Also applies to: 44-44, 150-150
256-274: LGTM: Analytics integration is well-implemented.The analytics event emission for Main window show is properly structured with async execution and error handling.
460-463: LGTM: Platform-specific decoration handling is appropriate.The conditional decoration disabling on Windows while enabling on macOS aligns with platform conventions.
473-473: LGTM: Window close method integration is clean.The new
window_closemethod is properly integrated into the trait and implementation, maintaining consistency with the existing API pattern.Also applies to: 553-555
415-423:✅ Verification successful
Verify overlay listener task management.
The overlay listener spawning and cleanup logic needs verification to ensure proper task lifecycle management and prevent resource leaks.
Please verify the overlay listener implementation:
🏁 Script executed:
#!/bin/bash # Description: Check overlay listener implementation and task management # Expected: Find overlay module with proper task handling echo "Searching for overlay listener implementation..." rg -A 10 -B 5 "spawn_overlay_listener|abort_overlay_join_handle|set_overlay_join_handle" echo "Checking overlay module structure..." fd -e rs | xargs rg -l "overlay" | head -3 | while read file; do echo "=== $file ===" rg -A 15 -B 5 "spawn.*overlay|abort.*overlay|JoinHandle" "$file" || true doneLength of output: 9085
Overlay listener task lifecycle properly managed
We’ve confirmed that:
- In
plugins/windows/src/overlay.rs,spawn_overlay_listenerreturns atokio::task::JoinHandle<()>for the ongoing loop.- In
plugins/windows/src/lib.rs,set_overlay_join_handleaborts any existing handle before storing the new one in theOVERLAY_JOIN_HANDLEmutex.- In
plugins/windows/src/ext.rs,abort_overlay_join_handleis invoked both before spawning a new listener and on the window’sCloseRequestedevent.Given this, the overlay listener is correctly cleaned up with no resource leaks detected.
plugins/windows/src/lib.rs
Outdated
| static OVERLAY_JOIN_HANDLE: Lazy<Mutex<Option<tokio::task::JoinHandle<()>>>> = Lazy::new(|| Mutex::new(None)); | ||
|
|
||
| pub fn set_overlay_join_handle(handle: tokio::task::JoinHandle<()>) { | ||
| if let Ok(mut guard) = OVERLAY_JOIN_HANDLE.lock() { | ||
| if let Some(old_handle) = guard.take() { | ||
| old_handle.abort(); | ||
| } | ||
| *guard = Some(handle); | ||
| } | ||
| } | ||
|
|
||
| pub fn abort_overlay_join_handle() { | ||
| if let Ok(mut guard) = OVERLAY_JOIN_HANDLE.lock() { | ||
| if let Some(handle) = guard.take() { | ||
| handle.abort(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
OVERLAY_JOIN_HANDLE never cleared on application shutdown
You store the task handle globally but nothing calls abort_overlay_join_handle
when the plugin is unloaded or the Tauri app quits. The Tokio task will survive
until process exit, potentially holding window handles and other resources.
Hook into Tauri’s on_window_event / on_exit or the plugin on_cleanup
callback to abort the task:
.setup(move |app, _| {
// …
app.listen_global("tauri://close-requested", |_| {
abort_overlay_join_handle();
});
Ok(())
})🤖 Prompt for AI Agents
In plugins/windows/src/lib.rs around lines 22 to 39, the global Tokio task
handle stored in OVERLAY_JOIN_HANDLE is never cleared or aborted on application
shutdown, which can cause resource leaks. To fix this, hook into Tauri's
lifecycle events such as on_window_event, on_exit, or the plugin's on_cleanup
callback, and call abort_overlay_join_handle() to abort the task when the plugin
unloads or the app quits. For example, add a setup closure that listens for the
"tauri://close-requested" event and calls abort_overlay_join_handle() to ensure
proper cleanup.
plugins/windows/src/overlay.rs
Outdated
| pub fn spawn_overlay_listener(app: AppHandle, window: WebviewWindow) -> tokio::task::JoinHandle<()> { | ||
| window.set_ignore_cursor_events(true).ok(); | ||
|
|
||
| tokio::spawn(async move { | ||
| let state = app.state::<FakeWindowBounds>(); | ||
| let mut last_ignore_state = true; | ||
| let mut last_focus_state = false; | ||
|
|
||
| loop { | ||
| sleep(Duration::from_millis(1000 / 10)).await; | ||
|
|
||
| let map = state.0.read().await; | ||
|
|
||
| let Some(windows) = map.get(window.label()) else { | ||
| if !last_ignore_state { | ||
| window.set_ignore_cursor_events(true).ok(); | ||
| last_ignore_state = true; | ||
| } | ||
| continue; | ||
| }; | ||
|
|
||
| if windows.is_empty() { | ||
| if !last_ignore_state { | ||
| window.set_ignore_cursor_events(true).ok(); | ||
| last_ignore_state = true; | ||
| } | ||
| continue; | ||
| }; | ||
|
|
||
| let (Ok(window_position), Ok(mouse_position), Ok(scale_factor)) = ( | ||
| window.outer_position(), | ||
| window.cursor_position(), | ||
| window.scale_factor(), | ||
| ) else { | ||
| if !last_ignore_state { | ||
| if let Err(e) = window.set_ignore_cursor_events(true) { | ||
| tracing::warn!("Failed to set ignore cursor events: {}", e); | ||
| } | ||
| last_ignore_state = true; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
UI calls from background Tokio task may violate Tauri threading guarantees
window.set_ignore_cursor_events, window.set_focus, window.is_focused,
etc. are executed from a spawned Tokio task (likely a non-UI thread).
While Wry APIs are Send + Sync, on macOS some window operations must occur on
the main thread. Random crashes have been observed under heavy cursor traffic.
Move the cursor loop onto the main thread with app.run_on_main_thread or send
events back to the UI thread.
Enforce main-thread context for all window API calls
Tauri requires certain window operations (e.g. set_ignore_cursor_events, set_focus, querying focus state) to run on the UI/main thread—especially on macOS. Calling these from a background Tokio task can lead to race conditions and random crashes under heavy cursor activity.
Please refactor the overlay listener in plugins/windows/src/overlay.rs so that every invocation of window.* runs on the main thread. For example, wrap calls in app.run_on_main_thread or emit a custom event and handle it in your UI-side code.
Locations to update:
- spawn_overlay_listener loop (lines 21–61)
Suggested pattern:
// Instead of calling directly inside the async loop:
window.set_ignore_cursor_events(true).ok();
// Use:
let label = window.label().to_string();
app.run_on_main_thread(move || {
if let Some(window) = tauri::AppHandle::get_window(&app, &label) {
window.set_ignore_cursor_events(true).ok();
}
});This guarantees all window manipulations occur on the main thread and prevents intermittent crashes.
🤖 Prompt for AI Agents
In plugins/windows/src/overlay.rs from lines 21 to 61, all calls to window
methods such as set_ignore_cursor_events, set_focus, and is_focused are made
inside a Tokio background task, which can cause crashes on macOS due to Tauri's
main-thread requirement for UI operations. Refactor the code so that every
window method call is wrapped inside app.run_on_main_thread, passing a closure
that retrieves the window by label and performs the operation there. This
ensures all window API calls execute on the main thread, preventing race
conditions and crashes.
Resolves #653, continuing #892 and #907.
Summary by CodeRabbit
New Features
Improvements
Documentation