feat(desktop): stop recording overlay and stable in-progress bounds#1704
feat(desktop): stop recording overlay and stable in-progress bounds#1704richiemcilroy merged 2 commits intomainfrom
Conversation
richiemcilroy
force-pushed
the
feat/desktop-recording-ux
branch
2 times, most recently
from
7d5ed28 to
7c59aaf
Compare
richiemcilroy
force-pushed
the
feat/desktop-recording-ux
branch
from
7c59aaf to
5dc076c
Compare
Paragon SummaryThis pull request review analyzed 3 files and found no issues. The review examined code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. Paragon did not detect any problems in the current diff. Proceed with merge after your normal checks. This PR adds a full-window stop recording control to the desktop app’s main window and makes the in-progress recording window geometry sync more reliable, so the overlay’s hit targets stay aligned with layout and resizing. Overall, it improves recording control accessibility and stability during active sessions. Key changes:
Confidence score: 5/5
3 files reviewed, 0 comments |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
|
@greptileai please review the PR :) |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| if (key === lastInteractiveBoundsKey) return; | ||
|
|
||
| lastInteractiveBoundsKey = key; | ||
| void commands.setFakeWindowBounds(FAKE_WINDOW_BOUNDS_NAME, { |
There was a problem hiding this comment.
void commands.setFakeWindowBounds(...) (and the removeFakeWindow call above) will surface as unhandled promise rejections if the backend fails. Since this runs frequently, it’s probably worth attaching a .catch(...) (even just console.error) so failures don’t spam/tear down the webview.
| setInterval, | ||
| ); | ||
|
|
||
| createTimer(syncInteractiveAreaBounds, 250, setInterval); |
There was a problem hiding this comment.
The 250ms polling loop calls getBoundingClientRect() continuously, which can force layout. If you want the same stability without the steady cost, a ResizeObserver on interactiveAreaRef() + rAF-throttled sync (and/or window.visualViewport events) can replace the interval in most cases.
| onMount(() => { | ||
| const onResize = () => syncInteractiveAreaBounds(); | ||
| window.addEventListener("resize", onResize); | ||
| onCleanup(() => window.removeEventListener("resize", onResize)); | ||
| requestAnimationFrame(() => syncInteractiveAreaBounds()); | ||
| setTimeout(() => syncInteractiveAreaBounds(), 150); | ||
| }); |
There was a problem hiding this comment.
requestAnimationFrame / setTimeout here aren't cleaned up; if this route unmounts quickly you can still call syncInteractiveAreaBounds() after cleanup.
| onMount(() => { | |
| const onResize = () => syncInteractiveAreaBounds(); | |
| window.addEventListener("resize", onResize); | |
| onCleanup(() => window.removeEventListener("resize", onResize)); | |
| requestAnimationFrame(() => syncInteractiveAreaBounds()); | |
| setTimeout(() => syncInteractiveAreaBounds(), 150); | |
| }); | |
| onMount(() => { | |
| const onResize = () => syncInteractiveAreaBounds(); | |
| window.addEventListener("resize", onResize); | |
| const rafId = requestAnimationFrame(() => syncInteractiveAreaBounds()); | |
| const timeoutId = window.setTimeout(() => syncInteractiveAreaBounds(), 150); | |
| onCleanup(() => { | |
| window.removeEventListener("resize", onResize); | |
| cancelAnimationFrame(rafId); | |
| clearTimeout(timeoutId); | |
| }); | |
| }); |
| const stopRecording = createMutation(() => ({ | ||
| mutationFn: async () => { | ||
| try { | ||
| await commands.stopRecording(); | ||
| } catch (error) { | ||
| await dialog.message( | ||
| `Failed to stop recording: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }`, | ||
| { title: "Stop Recording", kind: "error" }, | ||
| ); | ||
| } | ||
| }, | ||
| })); |
There was a problem hiding this comment.
If you rethrow after showing the dialog, the mutation's error state stays accurate (useful for telemetry / future UI) instead of always resolving successfully.
| const stopRecording = createMutation(() => ({ | |
| mutationFn: async () => { | |
| try { | |
| await commands.stopRecording(); | |
| } catch (error) { | |
| await dialog.message( | |
| `Failed to stop recording: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| { title: "Stop Recording", kind: "error" }, | |
| ); | |
| } | |
| }, | |
| })); | |
| const stopRecording = createMutation(() => ({ | |
| mutationFn: async () => { | |
| try { | |
| await commands.stopRecording(); | |
| } catch (error) { | |
| await dialog.message( | |
| `Failed to stop recording: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| { title: "Stop Recording", kind: "error" }, | |
| ); | |
| throw error; | |
| } | |
| }, | |
| })); |
| </Show> | ||
| </div> | ||
| <Show when={isActivelyRecording()}> | ||
| <div class="absolute inset-0 z-10 flex flex-col justify-end bg-gray-1/80 px-6 pb-8 backdrop-blur-sm"> |
There was a problem hiding this comment.
This overlay uses z-10; RecoveryToast doesn’t set a z-index and will likely render underneath it while recording. If the toast needs to stay actionable, consider bumping its z-index (or adjusting the overlay layering) so it isn’t blocked.
| const width = interactiveBounds.width ?? 0; | ||
| const height = interactiveBounds.height ?? 0; | ||
| const rect = element.getBoundingClientRect(); | ||
| if (rect.width === 0 || rect.height === 0) return; |
There was a problem hiding this comment.
If the interactive area stays mounted but becomes non-visible (0x0 rect), we keep the last fake window bounds. Clearing the fake window here too might avoid stale hit targets.
| if (rect.width === 0 || rect.height === 0) return; | |
| const rect = element.getBoundingClientRect(); | |
| if (rect.width === 0 || rect.height === 0) { | |
| if (lastInteractiveBoundsKey !== "") { | |
| lastInteractiveBoundsKey = ""; | |
| void commands.removeFakeWindow(FAKE_WINDOW_BOUNDS_NAME); | |
| } | |
| return; | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </div> | ||
| <Show when={isActivelyRecording()}> | ||
| <div class="absolute inset-0 z-10 flex flex-col justify-end bg-gray-1/80 px-6 pb-8 backdrop-blur-sm"> | ||
| <div class="pointer-events-auto"> |
There was a problem hiding this comment.
pointer-events-auto class
The parent div has no pointer-events-none class, so it already captures all pointer events by default (Tailwind's base is pointer-events-auto). The inner wrapper's pointer-events-auto class has no effect. The typical pattern would be to mark the overlay pointer-events-none so it's transparent to clicks, and only apply pointer-events-auto to the interactive button area — but since the design intent here is to block all background interaction while recording, the outer div is correct as-is; the inner class is just redundant.
| <div class="pointer-events-auto"> | |
| <div> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
Line: 2015
Comment:
**Redundant `pointer-events-auto` class**
The parent `div` has no `pointer-events-none` class, so it already captures all pointer events by default (Tailwind's base is `pointer-events-auto`). The inner wrapper's `pointer-events-auto` class has no effect. The typical pattern would be to mark the overlay `pointer-events-none` so it's transparent to clicks, and only apply `pointer-events-auto` to the interactive button area — but since the design intent here is to block all background interaction while recording, the outer div is correct as-is; the inner class is just redundant.
```suggestion
<div>
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Adds a full-window stop control on the main window while a recording is active. Replaces bounds primitives for the in-progress fake window with explicit geometry sync so hit targets track layout and resize more reliably.
Greptile Summary
This PR introduces two focused improvements to the desktop recording experience: a full-window stop overlay on the main window (shown while
currentRecording.data?.status === \"recording\") and a more reliable bounds-sync mechanism for the in-progress recording fake window.Main changes:
new-main/index.tsx): A newisActivelyRecordingderived signal gates a semi-transparentabsolute inset-0overlay with a single "Stop Recording" button that calls the samecommands.stopRecording()IPC command used elsewhere. Error handling is correct — failures surface as a dialog rather than swallowing silently. The backend (handle_recording_endinrecording.rs) already hides the in-progress window directly, so the multi-window teardown works correctly when stop is triggered here.in-progress-recording.tsx):createElementBounds(which internally polled viagetBoundingClientRectreactively) is replaced with an explicit, multi-trigger approach: two reactivecreateEffects schedule aqueueMicrotaskon ref/state/panel changes, anonMountresize listener,requestAnimationFrame+setTimeout(150)on mount, and acreateTimer(250ms)polling fallback. AlastInteractiveBoundsKeystring deduplication gate prevents redundant IPC calls when bounds haven't changed.Confidence Score: 5/5
Safe to merge — the backend correctly handles multi-window teardown on stop, and the bounds-sync refactor is functionally equivalent to the removed primitive with improved coverage.
No P0 or P1 issues found. The only finding is a redundant Tailwind class (P2 style). The initial concern about the in-progress window not reacting to an external stop is mitigated by the Rust backend, which directly calls window.hide() on CapWindowId::RecordingControls inside handle_recording_end for all stop paths.
No files require special attention.
Important Files Changed
pointer-events-autoclass on the inner button wrapper.Sequence Diagram
sequenceDiagram participant MW as Main Window participant IP as In-Progress Window participant BE as Rust Backend Note over MW: isActivelyRecording() = true<br/>(status === "recording") MW->>MW: Show stop overlay MW->>BE: commands.stopRecording() BE->>BE: handle_recording_end() BE->>IP: window.hide() [CapWindowId::RecordingControls] BE->>BE: emit CurrentRecordingChanged BE->>BE: emit RecordingStopped BE->>MW: currentRecordingChanged event MW->>MW: Invalidate currentRecording query MW->>MW: isActivelyRecording() = false → overlay hides BE->>MW: window.unminimize()Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(desktop): stop recording overlay an..." | Re-trigger Greptile