fix(linux): allow full-screen capture by position if IDs do not match#267
fix(linux): allow full-screen capture by position if IDs do not match#267webadderall merged 1 commit intomainfrom
Conversation
…position if IDs do not match (fixes #265)
📝 WalkthroughWalkthroughUpdated Linux screen-source matching logic in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/register/sources.ts (1)
99-131:⚠️ Potential issue | 🟠 MajorIndex-based fallback can mis-pair screens on multi-monitor Linux setups.
Two concerns in the new mapping when more than one display is present:
Ordering mismatch.
displaysis sorted bybounds.x,bounds.y,id(lines 90–97), butelectronScreenSourcesByIndexpreserves whatever orderdesktopCapturer.getSourcesreturns. Electron does not guarantee that order corresponds to on-screen layout, soelectronScreenSourcesByIndex[index]is not actually "by position" as the PR description states — it's purely ordinal. On a 2+ monitor X11 setup with mismatcheddisplay_ids, this can bind the wrong capture source to the wrong display (e.g., recording the left monitor when the user picked the right one), which is worse than the current "not available" error because it fails silently.Possible double-assignment. When counts are equal but only some
display_ids line up, a display whose ID did match will consume one source viaelectronScreenSourcesByDisplayId, while another display whose ID didn't match will fall back toelectronScreenSourcesByIndex[index]— which may point to the same already-claimed source. Two screen entries then share onesource.id, and recording one effectively records the other.For the single-monitor case (the main scenario in
#265) this logic is fine. For the multi-monitor case, consider matching by actual on-screen geometry instead, e.g. parsing the X11 source id (screen:<xrandr-output-id>:<index>/ bounds), or pairing remaining sources to remaining displays after removing those already matched bydisplay_id, and only falling back when exactly one of each is left.♻️ Sketch: consume sources left-to-right and skip ones already matched by display_id
- const electronScreenSourcesByDisplayId = new Map( - electronSources - .filter((source) => source.id.startsWith("screen:")) - .map((source) => [String(source.display_id ?? ""), source] as const), - ); - // On Linux, desktopCapturer display_id values may not match screen.getAllDisplays() IDs. - // Keep an ordered list so we can fall back to position-based matching. - const electronScreenSourcesByIndex = electronSources.filter((source) => - source.id.startsWith("screen:"), - ); + const electronScreenSources = electronSources.filter((source) => + source.id.startsWith("screen:"), + ); + const electronScreenSourcesByDisplayId = new Map( + electronScreenSources.map( + (source) => [String(source.display_id ?? ""), source] as const, + ), + ); + const claimedSourceIds = new Set<string>(); + for (const display of displays) { + const s = electronScreenSourcesByDisplayId.get(String(display.id)); + if (s) claimedSourceIds.add(s.id); + } + const unmatchedSources = electronScreenSources.filter( + (s) => !claimedSourceIds.has(s.id), + ); + const unmatchedDisplayCount = displays.filter( + (d) => !electronScreenSourcesByDisplayId.has(String(d.id)), + ).length; + const canFallbackByIndex = + unmatchedSources.length === unmatchedDisplayCount && unmatchedDisplayCount > 0; + let fallbackCursor = 0; const screenSources = displays.map((display, index) => { const displayId = String(display.id); - const matchedSource = - electronScreenSourcesByDisplayId.get(displayId) ?? - (electronScreenSourcesByIndex.length === displays.length - ? electronScreenSourcesByIndex[index] - : undefined); + const matchedSource = + electronScreenSourcesByDisplayId.get(displayId) ?? + (canFallbackByIndex ? unmatchedSources[fallbackCursor++] : undefined);Note: this still uses ordinal pairing for the leftover set; if you want genuine position-based matching, compare
display.boundsagainst source-provided geometry (where available) rather than relying on array order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/sources.ts` around lines 99 - 131, The current fallback uses electronScreenSourcesByIndex[index] which can mis-pair and double-assign sources; change the logic in the screenSources construction so you first build electronScreenSourcesByDisplayId, then compute two lists of remainingDisplays and remainingSources (filter out displays already matched and sources already used), and pair those leftovers one-to-one (consuming sources left-to-right and skipping any already-matched source ids) only when counts align; reference the variables electronScreenSourcesByDisplayId, electronScreenSourcesByIndex, screenSources, displays and matchedSource and ensure the fallback never returns a source id that was already assigned to another display (alternatively parse X11 source ids/geometry for precise matching if available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@electron/ipc/register/sources.ts`:
- Around line 99-131: The current fallback uses
electronScreenSourcesByIndex[index] which can mis-pair and double-assign
sources; change the logic in the screenSources construction so you first build
electronScreenSourcesByDisplayId, then compute two lists of remainingDisplays
and remainingSources (filter out displays already matched and sources already
used), and pair those leftovers one-to-one (consuming sources left-to-right and
skipping any already-matched source ids) only when counts align; reference the
variables electronScreenSourcesByDisplayId, electronScreenSourcesByIndex,
screenSources, displays and matchedSource and ensure the fallback never returns
a source id that was already assigned to another display (alternatively parse
X11 source ids/geometry for precise matching if available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2832c8cb-1d49-4fae-b9eb-23823de6303f
📒 Files selected for processing (1)
electron/ipc/register/sources.ts
1 participant
Summary
Fixes Linux full-screen capture failing with 'Selected display is not available for browser capture on this system'.
Root cause
On Linux/X11, Electron's desktopCapturer returns screen sources whose display_id values do not match those from screen.getAllDisplays(). This caused all displays to be assigned fallback IDs, which are rejected by the recording logic.
Fix
If the number of Electron screen sources matches the number of displays, pair them by position as a fallback. This allows full-screen capture to work on Linux/X11 even when IDs do not match.
Closes #265
Summary by CodeRabbit
Bug Fixes