Overlay floating control [Improvements to #892]#907
Overlay floating control [Improvements to #892]#907yujonglee merged 7 commits intofastrepl:overlay-floating-controlfrom
Conversation
WalkthroughThis update introduces new commands and state management for handling "fake window bounds" in the Windows plugin, centralizes and improves overlay and audio state synchronization, and refactors UI components to support richer recording and audio controls. Permissions and documentation are updated to reflect the new commands, and platform-specific window handling is improved, especially for macOS. Changes
Sequence Diagram(s)Overlay/Fake Window Bounds Update FlowsequenceDiagram
participant App as App/Frontend
participant Tauri as Tauri Plugin
participant State as FakeWindowBounds State
App->>Tauri: set_fake_window_bounds(name, bounds)
Tauri->>State: update_bounds(name, bounds)
State-->>Tauri: State updated
Tauri-->>App: Success/Failure
App->>Tauri: remove_fake_window(name)
Tauri->>State: remove_bounds(name)
State-->>Tauri: State updated
Tauri-->>App: Success/Failure
Audio Mute State SynchronizationsequenceDiagram
participant WindowA as Window A
participant WindowB as Window B
participant Tauri as Tauri Event Bus
WindowA->>Tauri: Emit audio-mic-state-changed (muted/unmuted)
Tauri-->>WindowB: Receive audio-mic-state-changed
WindowB->>WindowB: Refetch mic mute state, update UI
Poem
✨ 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 (
|
|
@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: 5
🔭 Outside diff range comments (1)
apps/desktop/src/routes/app.control.tsx (1)
210-224: 🛠️ Refactor suggestion
⚠️ Potential issue
useEffectcleanup fires on every drag-state change – leaks + flicker riskThe effect that installs the
mousemove/mouseuphandlers is re-executed wheneverisDraggingordragOffsetchange.
That means its cleanup — includingwindowsCommands.removeFakeWindow("control")— runs many times during a single drag cycle.
Side-effects:
- The fake window is destroyed/re-created repeatedly → possible flicker / CPU overhead.
- Listeners are detached/attached unnecessarily.
- If
removeFakeWindowthrows, the user will get an error for every state change.Move the
removeFakeWindowcall to an unmount-only effect and keep the pointer-event listeners in their ownuseEffectthat depends onisDraggingonly when truly needed.-useEffect(() => { +// 1️⃣ install / remove global listeners +useEffect(() => { ... - window.removeEventListener("mouseup", handleMouseUp); - windowsCommands.removeFakeWindow("control"); + window.removeEventListener("mouseup", handleMouseUp); }, []); + +// 2️⃣ run once on unmount to clean up fake window +useEffect(() => { + return () => { + windowsCommands.removeFakeWindow("control"); + }; +}, []);
🧹 Nitpick comments (16)
apps/desktop/index.html (1)
11-16: LGTM! Transparent backgrounds support overlay functionality.The inline CSS styles correctly set transparent backgrounds for overlay functionality. The
!importantdeclarations ensure these styles override any conflicting styles, which is likely necessary for proper overlay rendering.Consider moving these styles to an external CSS file if more overlay-specific styles are added in the future to maintain better separation of concerns.
apps/desktop/src/routes/__root.tsx (1)
70-81: LGTM! Debug event listener properly implemented.The debug event listener is well-implemented with proper cleanup in the useEffect hook. The logging format with "[Control Debug]" prefix provides clear identification of debug messages from the control window.
Consider adding error handling around the event listener for production robustness:
listen<string>("debug", (event) => { console.log(`[Control Debug] ${event.payload}`); -}).then((fn) => { +}).then((fn) => { unlisten = fn; +}).catch((error) => { + console.error("[Control Debug] Failed to setup debug listener:", error); });plugins/windows/permissions/autogenerated/reference.md (1)
31-82: Consider maintaining alphabetical order in the permission table.The new permission entries are complete and well-documented. However, they appear to be inserted at the beginning of the table, which breaks the alphabetical ordering pattern used elsewhere in the documentation.
Consider placing these entries in alphabetical order within the table for better organization and consistency.
apps/desktop/src/components/editor-area/note-header/listen-button.tsx (2)
448-456: Remove noisy console logs before production
console.log("[Main Window] Received …is helpful while developing but easily spams the dev-tools for every audio toggle across windows.- console.log(`[Main Window] Received mic state change:`, payload);Consider guarding with
if (import.meta.env.DEV)or removing.
476-482: Duplicate logic – extract helper to DRY mutationsBoth toggle mutations share identical code except the getter/setter & event name. A small helper would reduce duplication:
function makeToggle( getter: () => Promise<boolean>, setter: (v: boolean) => Promise<void>, eventName: string, ) { return useMutation(async () => { const current = await getter(); await setter(!current); await emit(eventName, { muted: !current }); return !current; }); }This keeps the two mutations to one-liners and future-proofs against copy-paste errors.
plugins/windows/src/overlay.rs (4)
13-17: Remove deadOverlayStateto avoid warnings & confusion
OverlayStateis no longer referenced after the introduction ofFakeWindowBounds. Keeping it around will trigger an “unused type” warning and makes it harder for new contributors to grasp the active state-handling path.-#[derive(Default)] -pub struct OverlayState { - pub bounds: Arc<RwLock<HashMap<String, HashMap<String, OverlayBound>>>>, -}
34-47:last_focus_stateis never reset when all bounds are removedWhen
map.get(..)returnsNonethe cursor-ignore flag is restored, butlast_focus_statestays untouched.
If the window happened to be focused in a previous iteration the local flag remainstrue, which silences the focus-restore logic once new bounds arrive.Consider resetting
last_focus_statealongsidelast_ignore_statehere to keep the finite-state-machine consistent.
56-66: Potential focus desync on cursor-position/scale-factor errorsIn this early-continue branch the cursor-ignore flag is corrected, but the focus flag again stays stale.
Same recommendation as above: resetlast_focus_state = falseto guarantee the next successful iteration can triggerset_focus().
34-37: Usetokio::time::intervalfor cleaner, drift-free pollingManually sleeping inside an infinite loop accumulates drift over time and is harder to read.
interval(Duration::from_millis(100))produces the same 10 Hz cadence without drift and automatically yields between ticks.packages/utils/src/stores/ongoing-session.ts (2)
86-91: Ensurecleanup()idempotencyAfter invoking the unlisten callback you should null-out the reference so that:
- A second
cleanup()call does not throw.- A new listener can be safely attached later.
if (sessionEventUnlisten) { sessionEventUnlisten(); + set((state) => + mutate(state, (draft) => { + draft.sessionEventUnlisten = undefined; + }) + ); }
42-81: Handle.listen()rejection
.listen()returns aPromise<UnlistenFn>. Network / IPC issues can make this promise reject.
Add a.catch()to log and surface the problem, otherwise the store will silently miss all future events.plugins/windows/src/commands.rs (3)
1-2: Remove unusedHashMapimport
std::collections::HashMapis no longer used directly in this module; keeping it will emit an “unused import” warning.-use std::collections::HashMap;
161-198: Four public commands wrap the same helpers – consider consolidation
window_set_overlay_bounds/window_remove_overlay_boundsandset_fake_window_bounds/remove_fake_windowexpose identical behaviour under two naming schemes.
Unless both names are truly required for API compatibility, keeping only one pair reduces maintenance overhead and the permission surface.
123-137: Hold write-lock only as long as necessaryThe
println!debug statements run while the write lock is held, slightly extending the critical section. Move the prints outside the locked scope to shorten contention if these commands become hot.apps/desktop/src/routes/app.control.tsx (2)
50-71: Guard against state-update-after-unmount ininitializeStateIf the component unmounts before the async
getState()/ audio queries resolve, the subsequentset*calls will warn (Can't perform a React state update on an unmounted component).
Add anisMountedflag:useEffect(() => { let mounted = true; (async () => { try { const currentState = await listenerCommands.getState(); if (mounted) setRecordingStatus(currentState as RecordingStatus); ... } finally { // any finally work } })(); return () => { mounted = false }; }, []);
491-510: Add accessibility label toIconButtonButtons are rendered with only an SVG – screen-reader users have no context. Re-emit the
tooltipasaria-labeland hide decorative icons:-<button - onClick={handleClick} - disabled={disabled} - className="p-2 ..." - title={tooltip} -> +<button + onClick={handleClick} + disabled={disabled} + className="p-2 ..." + title={tooltip} + aria-label={tooltip} +>Also consider adding
role="img" aria-hidden="true"on the SVGs for complete semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/desktop/index.html(1 hunks)apps/desktop/src/components/editor-area/note-header/listen-button.tsx(4 hunks)apps/desktop/src/routes/__root.tsx(2 hunks)apps/desktop/src/routes/app.control.tsx(5 hunks)packages/utils/src/stores/ongoing-session.ts(3 hunks)plugins/windows/build.rs(1 hunks)plugins/windows/js/bindings.gen.ts(1 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/reference.md(2 hunks)plugins/windows/permissions/default.toml(1 hunks)plugins/windows/permissions/schemas/schema.json(2 hunks)plugins/windows/src/commands.rs(3 hunks)plugins/windows/src/ext.rs(2 hunks)plugins/windows/src/lib.rs(3 hunks)plugins/windows/src/overlay.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugins/windows/src/overlay.rs (1)
plugins/windows/js/bindings.gen.ts (1)
OverlayBound(112-117)
plugins/windows/js/bindings.gen.ts (2)
plugins/windows/src/events.rs (2)
window(9-9)window(22-22)plugins/db/js/bindings.gen.ts (1)
Result(187-189)
plugins/windows/src/commands.rs (2)
plugins/windows/js/bindings.gen.ts (2)
HyprWindow(96-105)OverlayBound(112-117)plugins/windows/src/ext.rs (1)
label(94-96)
🔇 Additional comments (16)
plugins/windows/build.rs (1)
15-16: LGTM! Command additions follow established patterns.The new commands for fake window bounds functionality are properly integrated into the COMMANDS array and follow the existing naming convention. These align well with the overlay improvements mentioned in the PR objectives.
plugins/windows/permissions/default.toml (1)
14-17: LGTM! Permissions properly secure new functionality.The new permissions correctly correspond to the fake window bounds and overlay functionality, following the established "allow-" naming convention. This ensures proper security for the new commands.
apps/desktop/src/routes/__root.tsx (1)
8-8: LGTM! Import addition supports debug event handling.The import of
listenfrom Tauri API is correctly added to support the new debug event functionality.plugins/windows/permissions/autogenerated/commands/set_fake_window_bounds.toml (1)
1-14: LGTM! Standard permission configuration file structure.The permission configuration follows Tauri's standard format with proper allow/deny entries, clear descriptions, and correct schema reference. The autogenerated nature ensures consistency with the plugin's command structure.
plugins/windows/permissions/autogenerated/commands/remove_fake_window.toml (1)
1-14: LGTM! Consistent permission configuration structure.The permission configuration for
remove_fake_windowfollows the same proper structure as theset_fake_window_boundspermissions, ensuring consistency across the plugin's permission model.plugins/windows/src/lib.rs (3)
11-11: LGTM! Proper type exports for the new functionality.The public exports of
FakeWindowBoundsandOverlayBoundprovide necessary type access for external consumers of the plugin.
75-78: LGTM! Proper state management setup.The initialization of
FakeWindowBoundsmanaged state follows the established pattern in the plugin setup. The default initialization and state management are correctly implemented.
51-52:Details
✅ Verification successful
Verify that the new commands are properly implemented.
The new commands
set_fake_window_boundsandremove_fake_windoware registered in the plugin's command list. Ensure these commands are implemented in the commands module.
🏁 Script executed:
#!/bin/bash # Description: Verify that the new commands are implemented in the commands module # Expected: Find implementations of both commands echo "Searching for set_fake_window_bounds implementation:" ast-grep --pattern 'pub fn set_fake_window_bounds($_) { $$$ }' echo -e "\nSearching for remove_fake_window implementation:" ast-grep --pattern 'pub fn remove_fake_window($_) { $$$ }'Length of output: 357
#!/bin/bash # Find commands module files under plugins/windows/src echo "Searching for commands module files:" find plugins/windows/src -type f \( -name "commands.rs" -o -name "mod.rs" \) | grep -E "commands\.rs|commands/mod\.rs" || true echo -e "\nSearching for a commands directory and listing its Rust files:" if [ -d plugins/windows/src/commands ]; then ls plugins/windows/src/commands/*.rs || echo "No .rs files in plugins/windows/src/commands/" else echo "No commands directory found at plugins/windows/src/commands/" fi
#!/bin/bash # List all files under the plugin to locate the commands module echo "=== All files in plugins/windows/src ===" find plugins/windows/src -type f # Look for any commands module declarations echo -e "\n=== Searching for `mod commands` declarations ===" grep -R "mod commands" -n plugins/windows/src || echo "No `mod commands` declaration found." # Locate commands.rs or commands directory echo -e "\n=== Locating commands.rs or commands directory ===" find plugins/windows/src -type f \( -name "commands.rs" -o -name "mod.rs" \) | grep -E "commands\.rs|commands/mod\.rs" || true if [ -d plugins/windows/src/commands ]; then echo -e "\n=== Listing .rs files in plugins/windows/src/commands ===" ls plugins/windows/src/commands/*.rs || echo "No .rs files in plugins/windows/src/commands/" else echo -e "\nNo commands/ subdirectory under plugins/windows/src" fi # Search entire plugin for command implementations echo -e "\n=== Searching for `set_fake_window_bounds` in codebase ===" grep -R "set_fake_window_bounds" -n plugins/windows/src || echo "set_fake_window_bounds not found." echo -e "\n=== Searching for `remove_fake_window` in codebase ===" grep -R "remove_fake_window" -n plugins/windows/src || echo "remove_fake_window not found."
🏁 Script executed:
#!/bin/bash # List all files under the Windows plugin to locate where commands might live echo "=== All files in plugins/windows/src ===" find plugins/windows/src -type f # Search for any mentions or definitions of the new commands echo -e "\n=== Searching for set_fake_window_bounds ===" grep -R -n "set_fake_window_bounds" plugins/windows/src || echo "No occurrences of set_fake_window_bounds found" echo -e "\n=== Searching for remove_fake_window ===" grep -R -n "remove_fake_window" plugins/windows/src || echo "No occurrences of remove_fake_window found"Length of output: 902
Commands Implementations Verified
Both
set_fake_window_boundsandremove_fake_windoware defined inplugins/windows/src/commands.rs:
pub async fn set_fake_window_bounds(...)at line 181pub async fn remove_fake_window(...)at line 192No further action required.
plugins/windows/permissions/autogenerated/reference.md (1)
17-20: LGTM! Complete default permission list updates.The default permission list correctly includes all four new permissions for the fake window bounds functionality, maintaining consistency with the plugin's permission model.
plugins/windows/permissions/schemas/schema.json (2)
297-320: New permission variants are correctly added and follow the existing patternThe four new
PermissionKindconstants (allow|deny-remove-fake-window,allow|deny-set-fake-window-bounds) are declared consistently with the rest of the schema (description + markdownDescription + const).
I don’t see any duplication, typos, or ordering issues that would break theoneOfvalidation.Looks good!
478-482: Default permission set updated – double-check downstream docs/guides
defaultnow advertises the two newallow-*fake-window*permissions.
Please confirm the generated docs (e.g.reference.md) and end-user examples reflect the same list so copy/paste users don’t get out-of-sync examples.plugins/windows/js/bindings.gen.ts (1)
69-77: Generated file edited – ensure regeneration stays deterministic
setFakeWindowBounds/removeFakeWindowlook correct, but remember this file is auto-generated bytauri-specta.
If the Rust side changes again, a full regeneration will overwrite manual tweaks (e.g. comment style, import spacing).
Unless you patched the generator, keep manual edits out of generated artefacts.plugins/windows/src/ext.rs (3)
356-386: Builder chain split improves readability – tiny portability tweakGood call extracting the builder into a mutable variable and applying platform specific modifiers.
On non-macOS you now apply.decorations(false)after.transparent(true). Some Wayland/X11 WMs ignore transparency if decorations are disabled first; to be safe, call.decorations(false)before.transparent(true)(no-op on macOS).
387-388:spawn_overlay_listenerstill called – verify it now consumesFakeWindowBoundsSince the state holder moved from
OverlayState→FakeWindowBounds, confirmspawn_overlay_listenersignature was updated correspondingly; otherwise this will compile but the listener will ignore fake-window updates.
399-421:⚠️ Potential issue
unsafeCocoa block – addobjc!nil checks for robustnessInside the
unsafeblock you cast the raw pointer and immediately callstandardWindowButton_.
Ifns_windowwere unexpectedlynil, the Obj-C message send would seg-fault. You already guard the outerOption, but addingif ns_window.is_null() { return; }costs nothing.- let ns_window: id = ns_window as *mut _ as id; + let ns_window: id = ns_window as *mut _ as id; + if ns_window.is_null() { + return; + }Likewise a try-catch style macro (
objc_exception!) would avoid panics when Apple changes API names.Likely an incorrect or invalid review comment.
plugins/windows/src/overlay.rs (1)
95-101:last_focus_statemirrors an assumption, not the real focus
window.is_focused()is queried each pass, butlast_focus_stateis updated only by our own calls, not by the actual focus state.
If another app steals focus the flag may be wrong andset_focus()will never fire again.
You can close the gap by always syncinglast_focus_state = focusedafter the check.
| // Listen for audio state changes from other windows | ||
| 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]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory-leak & race in event listener cleanup
listen returns Promise<UnlistenFn>.
Storing the promise and later calling .then(unlisten => unlisten()) inside the cleanup works, but if the promise rejects (e.g. window closed) the .catch() branch is swallowed and the listener may linger.
A safer pattern is:
useEffect(() => {
- const unsub = listen<Payload>("evt", handler);
+ let unlisten: (() => void) | undefined;
+ const unsubPromise = listen<Payload>("evt", handler)
+ .then(fn => (unlisten = fn))
+ .catch(console.error);
return () => {
- unsub.then(u => u()).catch(console.error);
+ if (unlisten) unlisten();
};
}, []);This guarantees that the actual unlisten function is called or the error is surfaced.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/components/editor-area/note-header/listen-button.tsx around
lines 446 to 463, the cleanup function for event listeners uses .then() on
promises returned by listen(), but does not handle promise rejections, risking
unhandled errors and lingering listeners. To fix this, convert the cleanup
function to async, await the promises from listen(), and call the unlisten
functions inside a try-catch block to properly handle any errors during
unsubscription.
| mutationFn: async () => { | ||
| const newMuted = !isMicMuted; | ||
| await listenerCommands.setMicMuted(newMuted); | ||
| // Emit event to synchronize with other windows | ||
| await emit("audio-mic-state-changed", { muted: newMuted }); | ||
| return newMuted; | ||
| }, | ||
| onSuccess: () => refetchMicMuted(), |
There was a problem hiding this comment.
Undefined state can flip mic unintentionally
isMicMuted can be undefined until the first query resolves.
!undefined evaluates to true, so the very first click may always mute, regardless of the actual device state.
- const newMuted = !isMicMuted;
+ if (isMicMuted === undefined) {
+ return; // or disable the button until loaded
+ }
+ const newMuted = !isMicMuted;The same applies to toggleSpeakerMuted.
Also consider passing disabled={isMicMuted === undefined} to <AudioControlButton> to prevent premature clicks.
📝 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.
| mutationFn: async () => { | |
| const newMuted = !isMicMuted; | |
| await listenerCommands.setMicMuted(newMuted); | |
| // Emit event to synchronize with other windows | |
| await emit("audio-mic-state-changed", { muted: newMuted }); | |
| return newMuted; | |
| }, | |
| onSuccess: () => refetchMicMuted(), | |
| mutationFn: async () => { | |
| if (isMicMuted === undefined) { | |
| return; // or disable the button until loaded | |
| } | |
| const newMuted = !isMicMuted; | |
| await listenerCommands.setMicMuted(newMuted); | |
| // Emit event to synchronize with other windows | |
| await emit("audio-mic-state-changed", { muted: newMuted }); | |
| return newMuted; | |
| }, | |
| onSuccess: () => refetchMicMuted(), |
🤖 Prompt for AI Agents
In apps/desktop/src/components/editor-area/note-header/listen-button.tsx around
lines 465 to 472, the isMicMuted state can be undefined initially, causing the
toggle logic to incorrectly flip the mic state on the first click. To fix this,
add a guard to check if isMicMuted is undefined before toggling and prevent the
mutation if so. Also, pass disabled={isMicMuted === undefined} to the
<AudioControlButton> component to disable the button until the state is
resolved. Apply the same fix for toggleSpeakerMuted to handle its undefined
state similarly.
| // Set up global session event listener | ||
| listenerEvents.sessionEvent.listen(({ payload }) => { | ||
| if (payload.type === "audioAmplitude") { | ||
| set((state) => | ||
| mutate(state, (draft) => { | ||
| draft.amplitude = { | ||
| mic: payload.mic, | ||
| speaker: payload.speaker, | ||
| }; | ||
| }) | ||
| ); | ||
| } else if (payload.type === "running_active") { | ||
| set((state) => | ||
| mutate(state, (draft) => { | ||
| draft.status = "running_active"; | ||
| draft.loading = false; | ||
| }) | ||
| ); | ||
| } else if (payload.type === "running_paused") { | ||
| set((state) => | ||
| mutate(state, (draft) => { | ||
| draft.status = "running_paused"; | ||
| draft.loading = false; | ||
| }) | ||
| ); | ||
| } else if (payload.type === "inactive") { | ||
| set((state) => | ||
| mutate(state, (draft) => { | ||
| draft.status = "inactive"; | ||
| draft.loading = false; | ||
| }) | ||
| ); | ||
| } | ||
| }).then((unlisten) => { | ||
| set((state) => | ||
| mutate(state, (draft) => { | ||
| draft.sessionEventUnlisten = unlisten; | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Event listener is registered for every store instance ⇒ memory-leak & duplicate updates
listenerEvents.sessionEvent.listen() is executed during store creation. If createOngoingSessionStore() is ever called twice (e.g. in tests, storybook, hot-reload), you will end up with N listeners all mutating the same state.
Guard against multiple registrations or move the listener outside the factory, e.g.:
-let unlistenPromise: Promise<() => void> | undefined;
+let unlistenPromise: Promise<() => void> | undefined;
export const createOngoingSessionStore = (…) => {
return createStore<State & Actions>((set, get) => {
- listenerEvents.sessionEvent.listen(…)
+ if (!unlistenPromise) {
+ unlistenPromise = listenerEvents.sessionEvent.listen(…);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/utils/src/stores/ongoing-session.ts between lines 42 and 81, the
session event listener is registered inside the store creation function, causing
multiple listeners to accumulate if the store is created multiple times, leading
to memory leaks and duplicate state updates. To fix this, move the listener
registration outside the store factory function so it only registers once, or
add a guard to ensure the listener is only registered a single time regardless
of how many store instances are created.
| // Listen for session events | ||
| const unsubscribeSession = listenerEvents.sessionEvent.listen(({ payload }) => { | ||
| console.log(`[Control Bar] Session event:`, payload); | ||
|
|
||
| if (payload.type === "inactive" || payload.type === "running_active" || payload.type === "running_paused") { | ||
| setRecordingStatus(payload.type); | ||
| setRecordingLoading(false); | ||
| } | ||
| }); | ||
|
|
||
| // Listen for audio state changes from other windows | ||
| const unsubscribeMicState = listen<{ muted: boolean }>("audio-mic-state-changed", ({ payload }) => { | ||
| console.log(`[Control Bar] Received mic state change:`, payload); | ||
| setMicMuted(payload.muted); | ||
| }); | ||
|
|
||
| const unsubscribeSpeakerState = listen<{ muted: boolean }>("audio-speaker-state-changed", ({ payload }) => { | ||
| console.log(`[Control Bar] Received speaker state change:`, payload); | ||
| setSpeakerMuted(payload.muted); | ||
| }); | ||
|
|
||
| return () => { | ||
| unsubscribeSession.then(unlisten => unlisten()); | ||
| unsubscribeMicState.then(unlisten => unlisten()); | ||
| unsubscribeSpeakerState.then(unlisten => unlisten()); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Promise-based unlisten pattern is error-prone
listenerEvents.sessionEvent.listen() returns Promise<UnlistenFn>.
Storing the promise in unsubscribeSession and then calling
unsubscribeSession.then(unlisten => unlisten()) inside cleanup:
- Loses type-safety (TS thinks
unsubscribeSessionis a promise, not a fn). - Makes it easy to miss awaited errors.
- Doubles the
.thenceremony for every listener.
Prefer await inside the mounting phase, keep the unlisten function directly:
useEffect(() => {
let unlistenSession: () => void;
(async () => {
unlistenSession = await listenerEvents.sessionEvent.listen(({ payload }) => {
...
});
})();
return () => {
unlistenSession?.();
};
}, []);🤖 Prompt for AI Agents
In apps/desktop/src/routes/app.control.tsx around lines 75 to 101, the current
pattern stores promises returned by listenerEvents.sessionEvent.listen() and
listen() calls, then calls .then() on them in the cleanup function, which loses
type safety and complicates error handling. Refactor the useEffect to use an
async IIFE that awaits each listen call and stores the unlisten functions
directly in variables. Then, in the cleanup function, call these unlisten
functions directly without using .then(), ensuring proper type safety and
simpler error handling.
| // 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, | ||
| }; | ||
| setPosition(newPosition); | ||
| // Update bounds immediately during drag for smooth interaction | ||
| setTimeout(updateOverlayBounds, 0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateOverlayBounds is invoked on every mouse-move – throttle it
Inside the drag handler we call
setTimeout(updateOverlayBounds, 0) for each mousemove.
On a typical drag this fires hundreds of async RPC calls per second to setFakeWindowBounds, overwhelming the Tauri bridge and wasting battery.
Replace with a throttled / requestAnimationFrame approach so the call runs at most once per frame:
- setTimeout(updateOverlayBounds, 0);
+ if (!updateOverlayBoundsPending.current) {
+ updateOverlayBoundsPending.current = true;
+ requestAnimationFrame(() => {
+ updateOverlayBounds();
+ updateOverlayBoundsPending.current = false;
+ });
+ }const updateOverlayBoundsPending = useRef(false);🤖 Prompt for AI Agents
In apps/desktop/src/routes/app.control.tsx around lines 196 to 207, the
updateOverlayBounds function is called via setTimeout on every mousemove event
during dragging, causing excessive asynchronous calls that overwhelm the Tauri
bridge. To fix this, implement a throttling mechanism using a useRef boolean
flag (e.g., updateOverlayBoundsPending) to ensure updateOverlayBounds is only
called once per animation frame. Replace the setTimeout call with a
requestAnimationFrame callback that checks and resets this flag, preventing
multiple invocations within the same frame.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Style