Overlay floating control [More Improvements] #911
Conversation
## Walkthrough
This set of changes enhances error handling, state management, and UI clarity across several components. It introduces custom hooks for consent and audio mute state management with event subscriptions, refines window positioning and event handling in the desktop app, adds explicit loading and error states to session store actions, updates macOS dependencies, improves platform-specific window logic, and refines overlay event management with controlled async task handling.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| apps/desktop/src/components/editor-area/note-header/listen-button.tsx | Added new hooks `useConsentManagement` and `useAudioControls` for managing user consent and microphone/speaker mute states with Tauri event subscriptions and error-logging toggle mutations. |
| apps/desktop/src/routes/app.control.tsx | Improved position persistence, event handling, debounced bounds updates, user interaction tracking, UI grouping, accessibility, error handling, and removed debug code and screenshot button. |
| packages/utils/src/stores/ongoing-session.ts | Introduced explicit loading state and error handling for `stop`, `pause`, and `resume` session actions, using immutable state updates. |
| plugins/windows/Cargo.toml | Added `objc2`, `objc2-app-kit`, and `objc2-foundation` as macOS-specific dependencies; added `once_cell` to general dependencies. |
| plugins/windows/src/commands.rs | Added missing newline at end of file; no code logic changes. |
| plugins/windows/src/ext.rs | Refactored `close` and `show` methods for platform-specific handling, improved error handling, updated macOS window button manipulation using `objc2`, and managed overlay listener join handle. |
| plugins/windows/src/overlay.rs | Changed `spawn_overlay_listener` to return a join handle, improved error logging for cursor events, refined focus logic, and updated state reset conditions. |
| plugins/windows/src/lib.rs | Added static global for overlay join handle management with functions to set and abort the stored join handle, enabling controlled cancellation and replacement of the overlay async task. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UI (app.control.tsx)
participant Store (ongoing-session.ts)
participant AudioControls (listen-button.tsx)
participant Overlay (overlay.rs)
participant WindowExt (ext.rs)
User->>UI (app.control.tsx): Drag/Move/Interact with control
UI (app.control.tsx)->>UI (app.control.tsx): Update position state, debounce bounds update
UI (app.control.tsx)->>UI (app.control.tsx): Persist position to localStorage
User->>AudioControls (listen-button.tsx): Toggle mic/speaker mute
AudioControls (listen-button.tsx)->>AudioControls (listen-button.tsx): Call mutation with onError logging
User->>Store (ongoing-session.ts): Stop/Pause/Resume session
Store (ongoing-session.ts)->>Store (ongoing-session.ts): Set loading=true, call async command
Store (ongoing-session.ts)->>Store (ongoing-session.ts): On success: update state, loading=false
Store (ongoing-session.ts)->>Store (ongoing-session.ts): On error: log error, loading=false
UI (app.control.tsx)->>WindowExt (ext.rs): Show/Close window
WindowExt (ext.rs)->>Overlay (overlay.rs): Spawn overlay listener, handle join handle
Overlay (overlay.rs)->>Overlay (overlay.rs): Manage cursor/focus, log errorsPossibly related PRs
Poem
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
plugins/windows/src/ext.rs (1)
363-380: 🛠️ Refactor suggestionWindow created at full-screen size defeats the purpose of a “floating control”
inner_size(window_width, window_height)sets the Control window to the entire primary monitor.
On HiDPI displays the overlay will cover everything and prevent clicks, contradicting the “toolbar-sized” UX you achieve later with CSS.- .inner_size(window_width, window_height) + // Give the window just enough room for the toolbar + .inner_size( /* ≈ 220 × 80 logical px is sufficient */ 220.0, 80.0)If you really need the invisible full-screen window for hit-testing, consider using
set_fake_window_boundsonly and keep the real OS window minimal & click-through.
🧹 Nitpick comments (4)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
487-489: Good addition of error handling for speaker mute functionality.The error callback provides useful debugging information when the speaker mute operation fails.
For production, consider enhancing error handling to provide user feedback:
onError: (error) => { console.error('Failed to toggle speaker mute:', error); + // Consider showing user notification or toast message },plugins/windows/src/ext.rs (1)
398-437: Unsafe Objective-C calls: missing null-checks & memory-safety guarantees
ns_window as *mut AnyObjectis assumed non-null and subsequently dereferenced; ifns_window()returns0(can happen when window hasn’t finished initialisation) this will cause UB.Add an early return:
if let Ok(ns_window) = window.ns_window() { - unsafe { - let ns_window = ns_window as *mut AnyObject; - let ns_window = &*ns_window; + unsafe { + if ns_window.is_null() { + return; + } + let ns_window: &AnyObject = &*ns_window.cast::<AnyObject>();Also prefer
objc2::rc::Id::<objc2_app_kit::NSButton>::retain_autoreleased()helpers instead of raw pointers.apps/desktop/src/routes/app.control.tsx (2)
461-475: RepeatedsetTimeoutinside ref callback can cause layout jankEvery remount schedules a timer even when position hasn’t changed.
Prefer reading the DOM rect synchronously and compare before scheduling bounds updates, or throttle withrequestAnimationFrame.
498-509: Minor accessibility nit – buttons visually red but no ARIA stateMute buttons change colour but do not expose
aria-pressed.
Addaria-pressed={micMuted}/aria-pressed={speakerMuted}to help screen-reader users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx(2 hunks)apps/desktop/src/routes/app.control.tsx(8 hunks)packages/utils/src/stores/ongoing-session.ts(3 hunks)plugins/windows/Cargo.toml(1 hunks)plugins/windows/src/commands.rs(0 hunks)plugins/windows/src/ext.rs(5 hunks)plugins/windows/src/overlay.rs(3 hunks)
💤 Files with no reviewable changes (1)
- plugins/windows/src/commands.rs
🔇 Additional comments (15)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
473-475: Good addition of error handling for microphone mute functionality.The error callback provides useful debugging information when the microphone mute operation fails.
plugins/windows/src/overlay.rs (5)
26-26: Excellent improvement to return JoinHandle for task lifecycle management.Returning the
JoinHandleallows the caller to properly manage the spawned task, including cleanup and cancellation when needed.
62-64: Good improvement replacing silent error handling with explicit logging.The explicit error handling with warning logs is much better than the previous
.ok()approach that silently ignored failures.
93-95: Consistent error handling pattern applied.Good consistency in replacing silent error handling with explicit warning logs.
100-110: Enhanced focus management logic with proper state tracking.The refined focus management prevents redundant focus calls and provides better state tracking for hover interactions.
112-112: Correct return of JoinHandle.The function now properly returns the spawned task handle instead of discarding it.
packages/utils/src/stores/ongoing-session.ts (8)
133-137: Good addition of loading state management for stop operation.Setting the loading state before the async operation provides the UI with feedback capability.
150-156: Excellent error handling for stop operation.The error handling properly logs failures and resets the loading state to prevent UI from being stuck in a loading state.
162-166: Good loading state management for pause operation.Consistent with the stop operation's loading state pattern.
169-174: Improved state management using mutate for pause operation.The use of
mutatefunction for immutable state updates is more consistent with the store's pattern.
184-190: Proper error handling for pause operation.Consistent error handling pattern with logging and loading state reset.
194-198: Loading state management for resume operation.Consistent pattern applied to resume operation.
201-206: Consistent state management for resume operation.Good use of
mutatefunction for immutable updates.
207-213: Complete error handling pattern for resume operation.All session control operations now have consistent error handling with proper loading state management.
plugins/windows/src/ext.rs (1)
439-447: Race-condition between listener abort and window destruction
join_handle.abort()executes after theCloseRequestedevent fires; if the window is destroyed without firing that event (see first comment) the handle survives.
Tie the lifetime toWindowEvent::Destroyedor move the abort logic intoclose()for robustness.
|
@plyght Also please resolve the merge conflict! I think w're almost there. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/windows/src/lib.rs (1)
21-21: Consider alternatives to global state.While the global static variable achieves the goal of managing overlay task lifecycle, global state can make testing and reasoning about code more difficult. Consider if this could be managed through the existing state management system or dependency injection.
However, given this appears to be a plugin system where lifecycle management across modules is needed, this approach may be justified.
📜 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 (5)
apps/desktop/src/routes/app.control.tsx(8 hunks)plugins/windows/Cargo.toml(2 hunks)plugins/windows/src/ext.rs(5 hunks)plugins/windows/src/lib.rs(1 hunks)plugins/windows/src/overlay.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/windows/Cargo.toml
- plugins/windows/src/overlay.rs
- plugins/windows/src/ext.rs
- apps/desktop/src/routes/app.control.tsx
🔇 Additional comments (1)
plugins/windows/src/lib.rs (1)
16-17: LGTM! Necessary imports for synchronization.The added imports for
MutexandLazyare appropriate for the new global state management functionality.
| 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
Add documentation and improve error handling.
Similar to set_overlay_join_handle, this function needs documentation and better error handling.
Apply this improvement:
+/// Aborts the current overlay join handle if one exists.
+///
+/// # Errors
+///
+/// Logs an error if the mutex lock fails.
pub fn abort_overlay_join_handle() {
- if let Ok(mut guard) = OVERLAY_JOIN_HANDLE.lock() {
+ match OVERLAY_JOIN_HANDLE.lock() {
+ Ok(mut guard) => {
if let Some(handle) = guard.take() {
handle.abort();
}
+ }
+ Err(e) => {
+ eprintln!("Failed to acquire overlay join handle lock: {}", e);
+ }
}
}📝 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.
| pub fn abort_overlay_join_handle() { | |
| if let Ok(mut guard) = OVERLAY_JOIN_HANDLE.lock() { | |
| if let Some(handle) = guard.take() { | |
| handle.abort(); | |
| } | |
| } | |
| } | |
| /// Aborts the current overlay join handle if one exists. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Logs an error if the mutex lock fails. | |
| pub fn abort_overlay_join_handle() { | |
| match OVERLAY_JOIN_HANDLE.lock() { | |
| Ok(mut guard) => { | |
| if let Some(handle) = guard.take() { | |
| handle.abort(); | |
| } | |
| } | |
| Err(e) => { | |
| eprintln!("Failed to acquire overlay join handle lock: {}", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugins/windows/src/lib.rs around lines 32 to 38, the function
abort_overlay_join_handle lacks documentation and does not handle errors when
locking the OVERLAY_JOIN_HANDLE mutex. Add a doc comment explaining the
function's purpose and behavior. Improve error handling by logging or otherwise
handling the case when the mutex lock fails, instead of silently ignoring it.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add documentation.
The function logic for replacing overlay handles is sound, but there are opportunities for improvement:
- Silent error handling: Lock failures are silently ignored, which could mask threading issues or deadlocks.
- Missing documentation: As a public API function, it should be documented.
Consider this improvement:
+/// Sets a new overlay join handle, aborting any existing handle.
+///
+/// # Errors
+///
+/// Logs an error if the mutex lock fails.
pub fn set_overlay_join_handle(handle: tokio::task::JoinHandle<()>) {
- if let Ok(mut guard) = OVERLAY_JOIN_HANDLE.lock() {
+ match OVERLAY_JOIN_HANDLE.lock() {
+ Ok(mut guard) => {
if let Some(old_handle) = guard.take() {
old_handle.abort();
}
*guard = Some(handle);
+ }
+ Err(e) => {
+ eprintln!("Failed to acquire overlay join handle lock: {}", e);
+ }
}
}📝 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.
| 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); | |
| } | |
| } | |
| /// Sets a new overlay join handle, aborting any existing handle. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Logs an error if the mutex lock fails. | |
| pub fn set_overlay_join_handle(handle: tokio::task::JoinHandle<()>) { | |
| match OVERLAY_JOIN_HANDLE.lock() { | |
| Ok(mut guard) => { | |
| if let Some(old_handle) = guard.take() { | |
| old_handle.abort(); | |
| } | |
| *guard = Some(handle); | |
| } | |
| Err(e) => { | |
| eprintln!("Failed to acquire overlay join handle lock: {}", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugins/windows/src/lib.rs around lines 23 to 30, the function
set_overlay_join_handle silently ignores errors when locking the mutex, which
can hide potential threading issues. Modify the code to handle the lock error
explicitly, such as logging or returning a Result to indicate failure.
Additionally, add proper Rust doc comments above the function to describe its
purpose, parameters, and behavior as it is part of the public API.
plyght
force-pushed
the
overlay-floating-control
branch
from
917598f to
c0da74f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
326-339: useConsentManagement hook implementation is correct but appears unused.The hook logic correctly manages consent state based on onboarding status, but it's not being used anywhere in the component. This could indicate incomplete implementation or dead code.
Is this hook intended for future use, or should it be integrated with the existing component logic? I can help implement the integration if needed.
📜 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(3 hunks)apps/desktop/src/routes/app.control.tsx(8 hunks)packages/utils/src/stores/ongoing-session.ts(3 hunks)plugins/windows/Cargo.toml(2 hunks)plugins/windows/src/commands.rs(1 hunks)plugins/windows/src/ext.rs(5 hunks)plugins/windows/src/lib.rs(2 hunks)plugins/windows/src/overlay.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/windows/src/commands.rs
- plugins/windows/Cargo.toml
- plugins/windows/src/overlay.rs
- plugins/windows/src/lib.rs
- packages/utils/src/stores/ongoing-session.ts
- plugins/windows/src/ext.rs
- apps/desktop/src/routes/app.control.tsx
🔇 Additional comments (1)
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (1)
4-4: Import additions look appropriate.The new imports for
useState, useEffectand Tauri'slisten, emitare correctly added to support the new hooks functionality.Also applies to: 17-17
| function useConsentManagement(isOnboarding: boolean) { | ||
| const ongoingSessionStore = useOngoingSession((s) => ({ | ||
| hasShownConsent: s.hasShownConsent, | ||
| setHasShownConsent: s.setHasShownConsent, | ||
| })); | ||
|
|
||
| const effectiveHasShownConsent = isOnboarding ? true : ongoingSessionStore.hasShownConsent; | ||
| const showConsent = !effectiveHasShownConsent && !isOnboarding; | ||
|
|
||
| return { | ||
| showConsent, | ||
| setHasShownConsent: ongoingSessionStore.setHasShownConsent, | ||
| }; | ||
| } | ||
|
|
||
| function useAudioControls() { | ||
| const { data: isMicMuted, refetch: refetchMicMuted } = useQuery({ | ||
| queryKey: ["mic-muted"], | ||
| queryFn: () => listenerCommands.getMicMuted(), | ||
| }); | ||
|
|
||
| const { data: isSpeakerMuted, refetch: refetchSpeakerMuted } = useQuery({ | ||
| queryKey: ["speaker-muted"], | ||
| queryFn: () => listenerCommands.getSpeakerMuted(), | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribeMicState = listen<{ muted: boolean }>("audio-mic-state-changed", ({ payload }) => { | ||
| console.log(`[Main Window] Received mic state change:`, payload); | ||
| refetchMicMuted(); | ||
| }); | ||
|
|
||
| const unsubscribeSpeakerState = listen<{ muted: boolean }>("audio-speaker-state-changed", ({ payload }) => { | ||
| console.log(`[Main Window] Received speaker state change:`, payload); | ||
| refetchSpeakerMuted(); | ||
| }); | ||
|
|
||
| return () => { | ||
| unsubscribeMicState.then(unlisten => unlisten()); | ||
| unsubscribeSpeakerState.then(unlisten => unlisten()); | ||
| }; | ||
| }, [refetchMicMuted, refetchSpeakerMuted]); | ||
|
|
||
| const toggleMicMuted = useMutation({ | ||
| mutationFn: async () => { | ||
| const newMuted = !isMicMuted; | ||
| await listenerCommands.setMicMuted(newMuted); | ||
| await emit("audio-mic-state-changed", { muted: newMuted }); | ||
| return newMuted; | ||
| }, | ||
| onSuccess: () => refetchMicMuted(), | ||
| onError: (error) => { | ||
| console.error('Failed to toggle microphone mute:', error); | ||
| }, | ||
| }); | ||
|
|
||
| const toggleSpeakerMuted = useMutation({ | ||
| mutationFn: async () => { | ||
| const newMuted = !isSpeakerMuted; | ||
| await listenerCommands.setSpeakerMuted(newMuted); | ||
| await emit("audio-speaker-state-changed", { muted: newMuted }); | ||
| return newMuted; | ||
| }, | ||
| onSuccess: () => refetchSpeakerMuted(), | ||
| onError: (error) => { | ||
| console.error('Failed to toggle speaker mute:', error); | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| isMicMuted, | ||
| isSpeakerMuted, | ||
| toggleMicMuted, | ||
| toggleSpeakerMuted, | ||
| refetchSpeakerMuted, | ||
| refetchMicMuted, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider integration with existing functionality or removal.
Both hooks provide functionality that either duplicates existing code or isn't being used. The useAudioControls hook overlaps with the audio control logic in the RecordingControls component (lines 243-254). Consider either:
- Integrating these hooks with the existing component
- Removing them if they're not needed
- Documenting their intended future use
Let me check if these hooks are used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for usage of the new hooks across the codebase
echo "Searching for useConsentManagement usage..."
rg "useConsentManagement" --type tsx --type ts
echo "Searching for useAudioControls usage..."
rg "useAudioControls" --type tsx --type tsLength of output: 331
Let’s retry the search across all TypeScript/TSX files with glob patterns:
🏁 Script executed:
#!/bin/bash
# Search across .ts and .tsx files for hook usage
rg "useConsentManagement" -g '*.ts' -g '*.tsx' -n
rg "useAudioControls" -g '*.ts' -g '*.tsx' -nLength of output: 328
Review and consolidate unused hooks in listen-button.tsx
The useConsentManagement and useAudioControls hooks in
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (lines 326–403) are never referenced elsewhere in the codebase. At the same time, the RecordingControls component in
apps/desktop/src/components/editor-area/note-header/recording-controls.tsx (around lines 243–254) already implements similar mic/speaker mute logic.
Consider one of the following actions:
- Integrate the new hooks into the existing
RecordingControlscomponent so that the shared logic lives in one place. - Remove
useConsentManagementanduseAudioControlsif they’re not needed today. - Document their intended future use if you plan to expose this functionality elsewhere.
🤖 Prompt for AI Agents
In apps/desktop/src/components/editor-area/note-header/listen-button.tsx around
lines 326 to 403, the hooks useConsentManagement and useAudioControls are
defined but never used anywhere else in the codebase, and their functionality
overlaps with existing logic in the RecordingControls component in
apps/desktop/src/components/editor-area/note-header/recording-controls.tsx
around lines 243 to 254. To fix this, either integrate these hooks into the
RecordingControls component to consolidate audio control logic, or remove these
hooks entirely if they are not currently needed, or add clear documentation
about their intended future use if you plan to keep them for upcoming features.
fixes #653
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores