Fix selected recording target highlight#719
Open
chbndrhnns wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds source highlight overlay support across native visible-bounds data, a shared Electron overlay controller, IPC bridging for show/clear actions, and launch UI hover/selection flows that drive the highlight lifecycle. ChangesSource Highlight Overlay Feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
electron/native/ScreenCaptureKitWindowList.swift (1)
92-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd focused coverage for the rectangle visibility math.
subtractandunionRectnow drive user-visible highlight bounds; fixture tests for touching edges, full occlusion, nested occluders, and disjoint visible regions would make this safer to evolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/ScreenCaptureKitWindowList.swift` around lines 92 - 156, Add focused tests for the rectangle visibility math in subtract and unionRect so the highlight bounds behavior is covered. Create fixture cases for touching edges, full occlusion, nested occluders, and disjoint visible regions, and assert the returned WindowRect pieces and merged bounds match expectations. Keep the tests targeted around the existing subtract(_:from:) and unionRect(_:) helpers so future changes to ScreenCaptureKitWindowList remain safe.electron/sourceHighlightController.test.ts (1)
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the HUD passthrough reassertion in the clear-path test.
clear()is also responsible for callingreassertHudOverlayMousePassthrough(), and that lifecycle side effect is part of what this PR is adding. The current test only checksclose(), so that regression would still pass.Suggested test tweak
it("clears the persistent recording outline explicitly", async () => { const highlightWindow = createMockHighlightWindow(); + const reassertHudOverlayMousePassthrough = vi.fn(); const controller = createSourceHighlightController({ createWindow: vi.fn(() => highlightWindow), - reassertHudOverlayMousePassthrough: vi.fn(), + reassertHudOverlayMousePassthrough, }); await controller.show({ x: 0, y: 0, width: 200, height: 100 }); controller.clear(); expect(highlightWindow.close).toHaveBeenCalledTimes(1); + expect(reassertHudOverlayMousePassthrough).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/sourceHighlightController.test.ts` around lines 105 - 116, Update the source highlight controller clear-path test to also verify the HUD passthrough reassertion side effect. In the `createSourceHighlightController` setup, keep a spy for `reassertHudOverlayMousePassthrough`, then in the `clear()` flow assert that this callback is invoked in addition to `highlightWindow.close()` so the `clear` lifecycle in `sourceHighlightController` is fully covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/register/sources.ts`:
- Around line 53-65: getVisibleWindowHighlightBounds currently returns only the
largest display intersection, which collapses multi-display windows to a single
monitor. Update this helper to preserve all positive intersections from
getScreen().getAllDisplays() by combining them into one cross-display highlight
shape instead of sorting and selecting intersections[0]. Keep the logic
localized in getVisibleWindowHighlightBounds and ensure the controller can
receive the full visible region across displays.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 191-199: The initial getSelectedSource() handling in LaunchWindow
should clear any existing overlay when the returned source is falsy, not only on
Linux. Update the then() branch around syncSelectedSource and
showSelectedSourceHighlight so that it mirrors the subscription logic and calls
window.electronAPI.clearSourceHighlight?.() whenever source is null/undefined,
regardless of platform, while still showing the highlight only when source
exists and canShowSourceHighlight is true.
In `@src/components/launch/popovers/SourcePopover.tsx`:
- Around line 79-81: The hover flow in SourcePopover’s showSourceHighlight calls
is treating any resolved preload response as success, but the contract can
return { success: false } and that case must be handled. Update the
highlight/restore paths around showSourceHighlight and restoreSourceHighlight to
check the returned success flag and avoid clearing or leaving stale UI state
when highlighting fails, so the previous border/overlay is not left visible. Use
the existing SourcePopover hover handlers and the
window.electronAPI?.showSourceHighlight?. calls as the anchor points for the
fix.
In `@src/hooks/useScreenRecorder.ts`:
- Line 1522: The recording source highlight is being re-shown too early in the
start-recording flow, so it gets cleared again by the recording-start IPC
transition. Update the relevant start paths in useScreenRecorder (the
selectedSource/platform highlight handling in both flows noted in the review) to
re-show the overlay only after the recording-start IPC has settled, or adjust
electron/ipc/register/recording.ts so it does not clear the highlight on that
transition.
---
Nitpick comments:
In `@electron/native/ScreenCaptureKitWindowList.swift`:
- Around line 92-156: Add focused tests for the rectangle visibility math in
subtract and unionRect so the highlight bounds behavior is covered. Create
fixture cases for touching edges, full occlusion, nested occluders, and disjoint
visible regions, and assert the returned WindowRect pieces and merged bounds
match expectations. Keep the tests targeted around the existing
subtract(_:from:) and unionRect(_:) helpers so future changes to
ScreenCaptureKitWindowList remain safe.
In `@electron/sourceHighlightController.test.ts`:
- Around line 105-116: Update the source highlight controller clear-path test to
also verify the HUD passthrough reassertion side effect. In the
`createSourceHighlightController` setup, keep a spy for
`reassertHudOverlayMousePassthrough`, then in the `clear()` flow assert that
this callback is invoked in addition to `highlightWindow.close()` so the `clear`
lifecycle in `sourceHighlightController` is fully covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e15ff3f-390e-4aed-93f1-b16c81dd2eb7
📒 Files selected for processing (22)
electron/electron-env.d.tselectron/ipc/cursor/bounds.tselectron/ipc/paths/binaries.tselectron/ipc/register/recording.tselectron/ipc/register/sources.tselectron/ipc/types.tselectron/main.tselectron/native/ScreenCaptureKitWindowList.swiftelectron/native/bin/darwin-arm64/recordly-window-listelectron/native/bin/darwin-x64/recordly-window-listelectron/preload.tselectron/sourceHighlight.tselectron/sourceHighlightController.test.tselectron/sourceHighlightController.tssrc/components/launch/LaunchWindow.tsxsrc/components/launch/SourceSelector.tsxsrc/components/launch/hooks/useLaunchWindowActions.tssrc/components/launch/popovers/SourcePopover.tsxsrc/components/launch/popovers/sourcePreviewRequestGate.test.tssrc/components/launch/popovers/sourcePreviewRequestGate.tssrc/components/video-editor/VideoPlayback.tsxsrc/hooks/useScreenRecorder.ts
💤 Files with no reviewable changes (1)
- src/components/video-editor/VideoPlayback.tsx
| webcamStartTime.current === null | ||
| ? 0 | ||
| : webcamStartTime.current - mainStartedAt; | ||
| showRecordingSourceHighlight(selectedSource, platform); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Re-show the highlight after the recording-start clear.
These calls happen before setRecordingState(true). Per the PR stack context, electron/ipc/register/recording.ts clears the source highlight when recording starts, so both flows can tear the overlay down immediately after you show it. Move the re-show until after the recording-start IPC settles, or stop clearing on that transition.
Also applies to: 1589-1597, 1929-1934
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useScreenRecorder.ts` at line 1522, The recording source highlight
is being re-shown too early in the start-recording flow, so it gets cleared
again by the recording-start IPC transition. Update the relevant start paths in
useScreenRecorder (the selectedSource/platform highlight handling in both flows
noted in the review) to re-show the overlay only after the recording-start IPC
has settled, or adjust electron/ipc/register/recording.ts so it does not clear
the highlight on that transition.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/native/ScreenCaptureKitWindowListGeometry.test.ts (1)
120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrict empty-output assertion can fail on benign swiftc warnings.
Asserting combined
stdout+stderrequals""makes any compiler warning (e.g. deprecations) a hard failure, conflating "no diagnostics" with "compiles successfully". Consider gating on exit status and only surfacing output when the status is non-zero.♻️ Optional: assert on status, surface output on failure
- expect([compile.stdout, compile.stderr].filter(Boolean).join("\n")).toBe(""); - expect(compile.status).toBe(0); + expect( + compile.status, + [compile.stdout, compile.stderr].filter(Boolean).join("\n"), + ).toBe(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/ScreenCaptureKitWindowListGeometry.test.ts` at line 120, The test in ScreenCaptureKitWindowListGeometry.test.ts is asserting that combined swiftc stdout/stderr is exactly empty, which makes harmless compiler warnings fail the test. Update the check around the compile result to rely on the process exit status as the success condition, and only inspect or print the captured output when the compile fails; use the existing compile result object in the test to locate this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@electron/native/ScreenCaptureKitWindowListGeometry.test.ts`:
- Line 120: The test in ScreenCaptureKitWindowListGeometry.test.ts is asserting
that combined swiftc stdout/stderr is exactly empty, which makes harmless
compiler warnings fail the test. Update the check around the compile result to
rely on the process exit status as the success condition, and only inspect or
print the captured output when the compile fails; use the existing compile
result object in the test to locate this assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71145349-b236-46f4-81cd-0d2e5edf3f1b
📒 Files selected for processing (5)
electron/ipc/register/sources.tselectron/native/ScreenCaptureKitWindowListGeometry.test.tselectron/sourceHighlightController.test.tssrc/components/launch/LaunchWindow.tsxsrc/components/launch/popovers/SourcePopover.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/launch/popovers/SourcePopover.tsx
- electron/sourceHighlightController.test.ts
- src/components/launch/LaunchWindow.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #718
Summary:
Verification:
Summary by CodeRabbit