fix(linux): route X11 capture through real sources#613
Merged
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughExtracts Linux/Wayland portal source logic into a new sourceMapping module (constant + heuristics) and integrates it into Electron IPC sources, main display handler, and the browser-capture hook, with tests validating platform-specific screen ID routing and portal-sentinel synthesis. ChangesLinux Portal Source Mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/hooks/useScreenRecorder.test.ts (1)
211-238: ⚡ Quick winAdd X11/Wayland-aware coverage for portal decision logic.
These tests validate ID combinations only. Please add a case that protects Linux/X11 from portal routing when source resolution should stay on Electron IDs.
🤖 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.test.ts` around lines 211 - 238, Add a unit test for shouldUseLinuxPortalCapture that ensures a live Electron/X11/Wayland source stays on Electron capture even when selectedSourceId points at the Linux portal: call shouldUseLinuxPortalCapture with browserCaptureSourceId: "screen:42:0" and selectedSourceId: "screen:linux-portal" and assert it returns false so portal routing is not applied to live Electron IDs.electron/ipc/register/sourceMapping.test.ts (1)
41-91: ⚡ Quick winAdd regression cases for X11-unmatched mapping and stale fallback IDs.
The suite currently doesn’t lock in two key behaviors from this PR scope:
- unmatched Linux/X11 should avoid sentinel routing, and
- stale
screen:fallback:*should recover correctly on Wayland.🤖 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 41 - 91, Add two regression tests for shouldUseSyntheticLinuxPortalSource: one asserting that when platform is "linux", env indicates X11 (e.g. XDG_SESSION_TYPE: "x11") and sourceId is a sentinel/mapping-unmatched value (e.g. LINUX_PORTAL_SCREEN_SOURCE_ID or any non-concrete id), the function returns false (avoid sentinel routing for X11); and another asserting that when platform is "linux", env indicates Wayland (WAYLAND_DISPLAY present) and sourceId is a stale fallback like "screen:fallback:0", the function returns true (recover to synthetic path on Wayland). Use the existing test helper/constant names shouldUseSyntheticLinuxPortalSource and LINUX_PORTAL_SCREEN_SOURCE_ID to locate where to add these cases and match the style of surrounding tests.
🤖 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 48-50: The current check rejects any non-sentinel sourceId,
blocking persisted "screen:fallback:*" IDs; update the condition in the source
validation (the if using sourceId and LINUX_PORTAL_SCREEN_SOURCE_ID) to allow
IDs that start with the "screen:fallback:" prefix (i.e., treat them as eligible
for Wayland synthetic recovery) while still rejecting other non-sentinel values;
refer to the sourceId variable and LINUX_PORTAL_SCREEN_SOURCE_ID constant and
add a prefix check for "screen:fallback:" to permit those stale IDs.
- Around line 28-30: getScreenSourceIdForDisplay() currently returns the
sentinel LINUX_PORTAL_SCREEN_SOURCE_ID for any unmatched Linux display
regardless of session type; change it to only return
LINUX_PORTAL_SCREEN_SOURCE_ID when running on Linux AND the session is Wayland
(e.g., XDG_SESSION_TYPE === 'wayland' or an existing isWaylandSession() helper),
otherwise fall back to the previous non-portal behavior (return a real
desktopCapturer source id or null/undefined) so X11 paths do not receive the
portal sentinel. Ensure you update the logic around
getScreenSourceIdForDisplay(), reference LINUX_PORTAL_SCREEN_SOURCE_ID, and add
or reuse a small session-type check function to gate the portal sentinel.
---
Nitpick comments:
In `@electron/ipc/register/sourceMapping.test.ts`:
- Around line 41-91: Add two regression tests for
shouldUseSyntheticLinuxPortalSource: one asserting that when platform is
"linux", env indicates X11 (e.g. XDG_SESSION_TYPE: "x11") and sourceId is a
sentinel/mapping-unmatched value (e.g. LINUX_PORTAL_SCREEN_SOURCE_ID or any
non-concrete id), the function returns false (avoid sentinel routing for X11);
and another asserting that when platform is "linux", env indicates Wayland
(WAYLAND_DISPLAY present) and sourceId is a stale fallback like
"screen:fallback:0", the function returns true (recover to synthetic path on
Wayland). Use the existing test helper/constant names
shouldUseSyntheticLinuxPortalSource and LINUX_PORTAL_SCREEN_SOURCE_ID to locate
where to add these cases and match the style of surrounding tests.
In `@src/hooks/useScreenRecorder.test.ts`:
- Around line 211-238: Add a unit test for shouldUseLinuxPortalCapture that
ensures a live Electron/X11/Wayland source stays on Electron capture even when
selectedSourceId points at the Linux portal: call shouldUseLinuxPortalCapture
with browserCaptureSourceId: "screen:42:0" and selectedSourceId:
"screen:linux-portal" and assert it returns false so portal routing is not
applied to live Electron IDs.
🪄 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: 76e59c7c-30be-4f64-9218-79594fdebdc2
📒 Files selected for processing (6)
electron/ipc/register/sourceMapping.test.tselectron/ipc/register/sourceMapping.tselectron/ipc/register/sources.tselectron/main.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.ts
Contributor
|
Actionable comments posted: 0 |
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
desktopCapturer.getSources()source IDs instead of the syntheticscreen:0:0idscreen:fallback:*selections from failing immediately by resolving them back to the Linux portal path when neededWhy
A Fedora/X11 user reported that video source capture could not start. The previous Linux handler always used a synthetic portal id for
screen:linux-portal, which is useful for Wayland portal behavior but risky on X11 where Electron can provide real desktop capturer source IDs.Validation
npm run test -- electron/ipc/register/sourceMapping.test.ts src/hooks/useScreenRecorder.test.tsnpx biome check --formatter-enabled=false electron/main.ts electron/ipc/register/sourceMapping.ts electron/ipc/register/sourceMapping.test.ts electron/ipc/register/sources.ts src/hooks/useScreenRecorder.ts src/hooks/useScreenRecorder.test.tsnpx tsc --noEmitnpm run smoke:electron-main-cjsNotes
Summary by CodeRabbit
Improvements
Tests