Revert HUD passthrough and Linux fallback changes#656
Merged
Conversation
Contributor
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoves ChangesFeature removal: nativeCaptureUnavailable, webcam HUD sync, and source mapping simplification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/launch/hooks/useLaunchHudInteractionState.ts (1)
42-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the HUD interactive while a popover is open.
On Line 45 and Line 84, the ignore-mouse timers still arm even when
openIdis non-null. That lets an already-open HUD menu/popover flip the overlay back to click-through mid-interaction.Suggested fix
if (isInteractive) { isMouseOverHudRef.current = true; if (timeoutRef.current) clearTimeout(timeoutRef.current); window.electronAPI?.hudOverlaySetIgnoreMouse?.(false); - } else { + } else if (openId === null) { isMouseOverHudRef.current = false; if (timeoutRef.current) clearTimeout(timeoutRef.current); timeoutRef.current = setTimeout(() => { if ( + openId === null && !isHudDraggingRef.current && !isWebcamPreviewDraggingRef.current && !webcamPreviewDragStartRef.current && !isMouseOverHudRef.current ) { window.electronAPI?.hudOverlaySetIgnoreMouse?.(true); } }, 300); } }; window.addEventListener("mouseover", handleMouseOver); return () => window.removeEventListener("mouseover", handleMouseOver); - }, [isHudDraggingRef, isWebcamPreviewDraggingRef, webcamPreviewDragStartRef]); + }, [openId, isHudDraggingRef, isWebcamPreviewDraggingRef, webcamPreviewDragStartRef]); ... timeoutRef.current = setTimeout(() => { if ( + openId === null && !isHudDraggingRef.current && !isWebcamPreviewDraggingRef.current && !webcamPreviewDragStartRef.current && !isMouseOverHudRef.current ) { window.electronAPI?.hudOverlaySetIgnoreMouse?.(true); } }, 300); }, - [isHudDraggingRef, isWebcamPreviewDraggingRef, webcamPreviewDragStartRef], + [openId, isHudDraggingRef, isWebcamPreviewDraggingRef, webcamPreviewDragStartRef], );Also applies to: 84-93
🤖 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/components/launch/hooks/useLaunchHudInteractionState.ts` around lines 42 - 55, The timer callback that sets window.electronAPI.hudOverlaySetIgnoreMouse is firing even when a HUD popover/menu is open; update the logic in the hook (the blocks using isMouseOverHudRef, timeoutRef and setTimeout) to first check that openId is null/undefined before arming or executing the timeout callback — i.e., include a guard referencing openId (from the hook state/params) both before scheduling timeoutRef.current = setTimeout(...) and inside the timeout callback so that when openId is non-null the overlay is not switched to ignore-mouse; apply the same change to the second occurrence that uses isHudDraggingRef, isWebcamPreviewDraggingRef and webcamPreviewDragStartRef so popovers remain interactive.
🤖 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 127-129: Restore the Linux portal sentinel mapping when returning
screen rows from register/sources.ts: instead of emitting matchedSource?.id or
`screen:fallback:${displayId}` directly, map portal-like identifiers back to the
sentinel "screen:linux-portal" (or call the previous helper such as
normalizeScreenId or mapSourceToSentinel) so that after explicit selection
useScreenRecorder.ts still sees the portal-safe id; ensure both the
matchedSource present branch and the fallback branch normalize to the sentinel
when appropriate and keep the displayName unchanged.
---
Outside diff comments:
In `@src/components/launch/hooks/useLaunchHudInteractionState.ts`:
- Around line 42-55: The timer callback that sets
window.electronAPI.hudOverlaySetIgnoreMouse is firing even when a HUD
popover/menu is open; update the logic in the hook (the blocks using
isMouseOverHudRef, timeoutRef and setTimeout) to first check that openId is
null/undefined before arming or executing the timeout callback — i.e., include a
guard referencing openId (from the hook state/params) both before scheduling
timeoutRef.current = setTimeout(...) and inside the timeout callback so that
when openId is non-null the overlay is not switched to ignore-mouse; apply the
same change to the second occurrence that uses isHudDraggingRef,
isWebcamPreviewDraggingRef and webcamPreviewDragStartRef so popovers remain
interactive.
🪄 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: bddacc97-8290-4cb9-b3b1-d3556cbaedc5
📒 Files selected for processing (18)
electron/electron-env.d.tselectron/hudOverlayBounds.test.tselectron/hudOverlayBounds.tselectron/ipc/register/project.tselectron/ipc/register/settings.tselectron/ipc/register/sourceMapping.test.tselectron/ipc/register/sourceMapping.tselectron/ipc/register/sources.tselectron/ipc/types.tselectron/main.tselectron/preload.tselectron/windows.tssrc/components/launch/LaunchWindow.tsxsrc/components/launch/hooks/useLaunchHudInteractionState.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.ts
💤 Files with no reviewable changes (9)
- electron/ipc/register/sourceMapping.ts
- electron/ipc/register/sourceMapping.test.ts
- electron/ipc/types.ts
- src/components/launch/LaunchWindow.tsx
- electron/hudOverlayBounds.ts
- electron/hudOverlayBounds.test.ts
- src/components/video-editor/SettingsPanel.tsx
- electron/preload.ts
- src/hooks/useScreenRecorder.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/ipc/register/sourceMapping.test.ts (1)
19-49: ⚡ Quick winCover the
WAYLAND_DISPLAY-only Wayland path.
isLikelyLinuxWaylandSession()has a distinct fallback branch for environments whereXDG_SESSION_TYPEis unset, but this suite never exercises it. A regression there would silently flip those Linux sessions back toscreen:fallback:*.Suggested test
describe("getScreenSourceIdForDisplay", () => { it("keeps the live Electron screen source when one is available", () => { expect( getScreenSourceIdForDisplay({ displayId: "42", matchedSourceId: "screen:42:0", platform: "linux", }), ).toBe("screen:42:0"); }); it("routes unmatched Linux Wayland screens through the portal sentinel", () => { expect( getScreenSourceIdForDisplay({ displayId: "42", env: { XDG_SESSION_TYPE: "wayland", WAYLAND_DISPLAY: "wayland-0" }, matchedSourceId: null, platform: "linux", }), ).toBe(LINUX_PORTAL_SCREEN_SOURCE_ID); }); + + it("treats WAYLAND_DISPLAY without XDG_SESSION_TYPE as Wayland", () => { + expect( + getScreenSourceIdForDisplay({ + displayId: "42", + env: { WAYLAND_DISPLAY: "wayland-0" }, + matchedSourceId: null, + platform: "linux", + }), + ).toBe(LINUX_PORTAL_SCREEN_SOURCE_ID); + }); it("keeps unmatched Linux X11 screens on the explicit fallback id", () => { expect( getScreenSourceIdForDisplay({ displayId: "42",🤖 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/ipc/register/sourceMapping.test.ts` around lines 19 - 49, Add a test that exercises the codepath where XDG_SESSION_TYPE is unset but WAYLAND_DISPLAY is present: call getScreenSourceIdForDisplay with platform "linux", displayId "42", env: { WAYLAND_DISPLAY: "wayland-0" } (no XDG_SESSION_TYPE), and matchedSourceId null/undefined, and assert it returns LINUX_PORTAL_SCREEN_SOURCE_ID so the isLikelyLinuxWaylandSession fallback is 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/sourceMapping.ts`:
- Around line 1-35: The code removed handling for legacy "screen:fallback:*"
ids; restore a small normalizer so legacy persisted Wayland selections are
treated as the portal alias. In getScreenSourceIdForDisplay check if
matchedSourceId startsWith("screen:fallback:") and map it to
LINUX_PORTAL_SCREEN_SOURCE_ID (or otherwise ensure the returned id signals the
portal branch) before the platform/env checks; also ensure the caller/handler in
electron/main.ts continues to branch on that normalized portal id rather than
running desktopCapturer.getSources() for legacy "screen:fallback:*" values.
---
Nitpick comments:
In `@electron/ipc/register/sourceMapping.test.ts`:
- Around line 19-49: Add a test that exercises the codepath where
XDG_SESSION_TYPE is unset but WAYLAND_DISPLAY is present: call
getScreenSourceIdForDisplay with platform "linux", displayId "42", env: {
WAYLAND_DISPLAY: "wayland-0" } (no XDG_SESSION_TYPE), and matchedSourceId
null/undefined, and assert it returns LINUX_PORTAL_SCREEN_SOURCE_ID so the
isLikelyLinuxWaylandSession fallback is 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: 85320dc6-54c1-446d-9660-93787061e6e1
📒 Files selected for processing (3)
electron/ipc/register/sourceMapping.test.tselectron/ipc/register/sourceMapping.tselectron/ipc/register/sources.ts
✅ Files skipped from review due to trivial changes (1)
- electron/ipc/register/sources.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/ipc/register/sourceMapping.test.ts (1)
19-49: ⚡ Quick winCover the
WAYLAND_DISPLAY-only Wayland path.
isLikelyLinuxWaylandSession()has a distinct fallback branch for environments whereXDG_SESSION_TYPEis unset, but this suite never exercises it. A regression there would silently flip those Linux sessions back toscreen:fallback:*.Suggested test
describe("getScreenSourceIdForDisplay", () => { it("keeps the live Electron screen source when one is available", () => { expect( getScreenSourceIdForDisplay({ displayId: "42", matchedSourceId: "screen:42:0", platform: "linux", }), ).toBe("screen:42:0"); }); it("routes unmatched Linux Wayland screens through the portal sentinel", () => { expect( getScreenSourceIdForDisplay({ displayId: "42", env: { XDG_SESSION_TYPE: "wayland", WAYLAND_DISPLAY: "wayland-0" }, matchedSourceId: null, platform: "linux", }), ).toBe(LINUX_PORTAL_SCREEN_SOURCE_ID); }); + + it("treats WAYLAND_DISPLAY without XDG_SESSION_TYPE as Wayland", () => { + expect( + getScreenSourceIdForDisplay({ + displayId: "42", + env: { WAYLAND_DISPLAY: "wayland-0" }, + matchedSourceId: null, + platform: "linux", + }), + ).toBe(LINUX_PORTAL_SCREEN_SOURCE_ID); + }); it("keeps unmatched Linux X11 screens on the explicit fallback id", () => { expect( getScreenSourceIdForDisplay({ displayId: "42",🤖 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/ipc/register/sourceMapping.test.ts` around lines 19 - 49, Add a test that exercises the codepath where XDG_SESSION_TYPE is unset but WAYLAND_DISPLAY is present: call getScreenSourceIdForDisplay with platform "linux", displayId "42", env: { WAYLAND_DISPLAY: "wayland-0" } (no XDG_SESSION_TYPE), and matchedSourceId null/undefined, and assert it returns LINUX_PORTAL_SCREEN_SOURCE_ID so the isLikelyLinuxWaylandSession fallback is 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/sourceMapping.ts`:
- Around line 1-35: The code removed handling for legacy "screen:fallback:*"
ids; restore a small normalizer so legacy persisted Wayland selections are
treated as the portal alias. In getScreenSourceIdForDisplay check if
matchedSourceId startsWith("screen:fallback:") and map it to
LINUX_PORTAL_SCREEN_SOURCE_ID (or otherwise ensure the returned id signals the
portal branch) before the platform/env checks; also ensure the caller/handler in
electron/main.ts continues to branch on that normalized portal id rather than
running desktopCapturer.getSources() for legacy "screen:fallback:*" values.
---
Nitpick comments:
In `@electron/ipc/register/sourceMapping.test.ts`:
- Around line 19-49: Add a test that exercises the codepath where
XDG_SESSION_TYPE is unset but WAYLAND_DISPLAY is present: call
getScreenSourceIdForDisplay with platform "linux", displayId "42", env: {
WAYLAND_DISPLAY: "wayland-0" } (no XDG_SESSION_TYPE), and matchedSourceId
null/undefined, and assert it returns LINUX_PORTAL_SCREEN_SOURCE_ID so the
isLikelyLinuxWaylandSession fallback is 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: 85320dc6-54c1-446d-9660-93787061e6e1
📒 Files selected for processing (3)
electron/ipc/register/sourceMapping.test.tselectron/ipc/register/sourceMapping.tselectron/ipc/register/sources.ts
✅ Files skipped from review due to trivial changes (1)
- electron/ipc/register/sources.ts
🛑 Comments failed to post (1)
electron/ipc/register/sourceMapping.ts (1)
1-35:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
screen:fallback:*as a Linux/Wayland portal alias.Removing the fallback-id predicate here drops the compatibility path for persisted Wayland selections.
electron/main.tsnow only takes the single-portal branch forscreen:linux-portalor an empty source id, so an older session saved asscreen:fallback:42will go back throughdesktopCapturer.getSources()and reintroduce the extra portal flow this revert is trying to avoid. Please keep a small normalizer/predicate for legacyscreen:fallback:*ids and continue branching on it in the handler.🤖 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/ipc/register/sourceMapping.ts` around lines 1 - 35, The code removed handling for legacy "screen:fallback:*" ids; restore a small normalizer so legacy persisted Wayland selections are treated as the portal alias. In getScreenSourceIdForDisplay check if matchedSourceId startsWith("screen:fallback:") and map it to LINUX_PORTAL_SCREEN_SOURCE_ID (or otherwise ensure the returned id signals the portal branch) before the platform/env checks; also ensure the caller/handler in electron/main.ts continues to branch on that normalized portal id rather than running desktopCapturer.getSources() for legacy "screen:fallback:*" values.
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.
Summary
Validation
Summary by CodeRabbit
Improvements
Removed
Tests