fix: keep screen picker sources stable under Electron 42 macOS capture stack - #3414
Conversation
…e stack Electron 42's macOS ScreenCaptureKit backend bounds getSources() at ~3s (upstream hang/crash fixes), returning an empty array or empty thumbnails when enumerations run back-to-back. The picker's 3s polling plus the post-selection re-enumeration turned those empty results into a blank "No windows found" list and denied valid share attempts. - Split the desktop capturer cache into per-type buckets: screen-only enumeration is fast (~700ms) and reliable; window enumeration is paced with a 4s post-completion cooldown (cold-start chain bypasses it once) - Never overwrite a non-empty bucket with an empty enumeration result; keep the bucket stale so it retries on the next opportunity - Merge thumbnails by source id so a source arriving with an empty thumbnail keeps its last good preview instead of being dropped - Validate the selected source against the cache (the same list the picker rendered from) instead of re-enumerating on Share; fall back to one direct enumeration only when the cache is empty - Keep the previous source list when a renderer fetch fails Measured on macOS with Electron 42.5.0: tight-loop getSources returned 0 sources in ~75% of calls; screen-only calls never failed; alternating per-type calls with 4s gaps returned complete results every round. Documented in docs/KNOWN_ISSUES.md with upstream refs (electron/electron#51128, electron/electron#50960) and the macOS 15+ useSystemPicker follow-up.
WalkthroughChangesThe screen-sharing capture cache now separates screen and window sources, coordinates refreshes, preserves thumbnails, and serves cached results. Request validation uses the cache before enumeration, picker errors preserve existing sources, tests cover the new behavior, and known-issue documentation is expanded. Screen sharing source caching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScreenSharingRequestTracker
participant desktopCapturerCache
participant ElectronDesktopCapturer
ScreenSharingRequestTracker->>desktopCapturerCache: getCachedSources()
desktopCapturerCache-->>ScreenSharingRequestTracker: cached sources or empty result
alt cache is empty
ScreenSharingRequestTracker->>ElectronDesktopCapturer: getSources()
ElectronDesktopCapturer-->>ScreenSharingRequestTracker: enumerated source
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/screenSharing/main/desktopCapturerCache.main.spec.ts (2)
58-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd mock resets to
beforeEachfor test isolation.
getSourcesMock/handleMockare never reset between tests. Several tests set persistent mocks (mockResolvedValue,mockImplementation) rather than one-shot variants, so a future test that forgets to reconfigure the mock before its first assertion could silently inherit behavior from a previous test.♻️ Suggested addition
beforeEach(() => { jest.useFakeTimers(); clearDesktopCapturerCache(); + getSourcesMock.mockReset(); + handleMock.mockClear(); });🤖 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/screenSharing/main/desktopCapturerCache.main.spec.ts` around lines 58 - 69, Reset getSourcesMock and handleMock in the beforeEach setup alongside the fake timer and cache initialization, using the appropriate Jest mock-reset behavior so persistent implementations and resolved values cannot leak between tests.
340-348: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't actually verify "screens first" ordering.
toHaveBeenCalledWithonly checks that a matching call occurred at some point, not that it was the first call. SinceprewarmDesktopCapturerCachealso eventually callsgetSources({types:['window']}), this test would still pass even if the screens/windows order were swapped.♻️ Suggested fix
- expect(getSourcesMock).toHaveBeenCalledWith({ types: ['screen'] }); + expect(getSourcesMock).toHaveBeenNthCalledWith(1, { types: ['screen'] });🤖 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/screenSharing/main/desktopCapturerCache.main.spec.ts` around lines 340 - 348, Strengthen the prewarmDesktopCapturerCache test by asserting the screens request was the first getSourcesMock invocation, using the mock’s call-order assertion rather than only matching call arguments. Keep validation that the first call uses { types: ['screen'] } and preserve the existing asynchronous setup.
🤖 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 `@docs/KNOWN_ISSUES.md`:
- Around line 5-15: Update the timing claims in the documented Electron
ScreenCaptureKit issue to include a reproducible benchmark or log reference with
sample size and invocation details, including the ~24s, ~700ms, ~75%, and
thumbnail observations. If no measurement source is available, remove the
unsubstantiated numeric claims and describe only the directly observable
behaviors while preserving the confirmed failure symptoms.
---
Nitpick comments:
In `@src/screenSharing/main/desktopCapturerCache.main.spec.ts`:
- Around line 58-69: Reset getSourcesMock and handleMock in the beforeEach setup
alongside the fake timer and cache initialization, using the appropriate Jest
mock-reset behavior so persistent implementations and resolved values cannot
leak between tests.
- Around line 340-348: Strengthen the prewarmDesktopCapturerCache test by
asserting the screens request was the first getSourcesMock invocation, using the
mock’s call-order assertion rather than only matching call arguments. Keep
validation that the first call uses { types: ['screen'] } and preserve the
existing asynchronous setup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92297ff8-5cbd-49d0-b274-5078bda4d566
📒 Files selected for processing (6)
docs/KNOWN_ISSUES.mdsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/desktopCapturerCache.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/screenSharePicker.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/screenSharing/screenSharePicker.tsxsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/desktopCapturerCache.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/screenSharing/screenSharePicker.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,mdx}: Avoid subjective descriptors and use measurable descriptions.
Never invent metrics; use only numbers from actual logs, error messages, or documented sources.
PR descriptions should use straightforward language and focus on what changed and why.
Files:
docs/KNOWN_ISSUES.md
🔇 Additional comments (13)
src/screenSharing/ScreenSharingRequestTracker.ts (1)
4-4: LGTM!Also applies to: 161-181
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
5-5: LGTM!Also applies to: 17-30, 61-63, 359-410, 433-433
src/screenSharing/screenSharePicker.tsx (1)
89-90: LGTM!docs/KNOWN_ISSUES.md (1)
47-47: LGTM!Also applies to: 69-69
src/screenSharing/desktopCapturerCache.ts (5)
5-34: LGTM!
111-165: LGTM!
36-96: 🎯 Functional CorrectnessPartial
getSources()results need an explicit decision. The cache already drops any id missing from a non-empty enumeration (window really closedis covered by a spec), so this is intentional ifdesktopCapturer.getSources()always returns a complete set. If ScreenCaptureKit can return incomplete-but-nonempty results, those still-open sources will be evicted immediately.
166-192: 🗄️ Data Integrity & IntegrationMerged cache response is fine here — current callers request
types: ['window', 'screen'], so returning the combined cache matches the existing usage.> Likely an incorrect or invalid review comment.
98-109: 🗄️ Data Integrity & IntegrationGlobal single-flight is intentional here.
scheduleRefreshserializes all refreshes, and the existing tests cover that no two enumerations run concurrently; no bucket-specific promise is needed.> Likely an incorrect or invalid review comment.src/screenSharing/main/desktopCapturerCache.main.spec.ts (4)
6-53: LGTM!
71-338: LGTM!
350-402: LGTM!
404-453: LGTM!
| - Status: Confirmed (Electron 42.5.0, macOS, hardware measurement 2026-07-14). | ||
| - Symptom: Screen picker intermittently shows "No windows found" / "No screens found" | ||
| seconds after listing sources; screen share denied with "selected source no longer | ||
| available" right after the user picks a valid source. | ||
| - Root cause: Electron's macOS ScreenCaptureKit rewrite made `getSources()` unbounded-slow | ||
| (~24s observed under Electron 41.9), then upstream bounded it at ~3s returning whatever | ||
| arrived (hang fix, electron/electron#51128 lineage; DCHECK crash fix electron/electron#50960 | ||
| shipped in 42.0.1). Under Electron 42.5.0, back-to-back calls return `[]` ~75% of the time | ||
| and mostly-empty thumbnails otherwise. Combined `types: ['window','screen']` calls always | ||
| hit the 3s cap; `['screen']`-only completes in ~700ms and never flakes; paced (≥4s gap) | ||
| alternating per-type calls never return empty. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/KNOWN_ISSUES.md (lines 1-120) =="
cat -n docs/KNOWN_ISSUES.md | sed -n '1,120p'
echo
echo "== search for distinctive benchmark phrases =="
rg -n --hidden --no-ignore-vcs -S \
"Electron 42.5.0|selected source no longer available|No windows found|No screens found|mostly-empty thumbnails|never flake|24s|700ms|75%" \
.
echo
echo "== search for links or measurement references in docs/KNOWN_ISSUES.md nearby =="
rg -n --hidden --no-ignore-vcs -S "measurement|benchmark|log|logfile|source|Electron 41.9|Electron 42.5.0|electron/electron#" docs/KNOWN_ISSUES.mdRepository: RocketChat/Rocket.Chat.Electron
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact phrases in repository =="
rg -n --hidden --no-ignore-vcs -S \
"24s observed under Electron 41.9|~24s|~700ms|~75%|mostly-empty thumbnails|never flakes|3s cap|hardware measurement 2026-07-14" \
. || true
echo
echo "== measurement/log/source references near docs =="
rg -n --hidden --no-ignore-vcs -S \
"benchmark|measurement|sample size|hardware|log|logs|reproduc|Electron 42.5.0|Electron 41.9|desktopCapturer.getSources\(\)" \
docs src .github . || true
echo
echo "== candidate external references in docs files =="
rg -n --hidden --no-ignore-vcs -S \
"electron/electron#51128|electron/electron#50960|ScreenCaptureKit|getSources|Screen picker|No windows found|No screens found" \
docs/*.md docs/**/*.md 2>/dev/null || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 50388
Make the timing claims reproducible
docs/KNOWN_ISSUES.md:5-15 uses ~24s, ~700ms, ~75%, and “mostly-empty thumbnails” without a sample size, command, or linked measurement source. Link the benchmark/log or rewrite this section in terms of observable behavior.
🤖 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 `@docs/KNOWN_ISSUES.md` around lines 5 - 15, Update the timing claims in the
documented Electron ScreenCaptureKit issue to include a reproducible benchmark
or log reference with sample size and invocation details, including the ~24s,
~700ms, ~75%, and thumbnail observations. If no measurement source is available,
remove the unsubstantiated numeric claims and describe only the directly
observable behaviors while preserving the confirmed failure symptoms.
Source: Coding guidelines
…icker, sidebar tooltip) (#3417) * fix: #3244 sidebar tooltip null in shortcut formatting (#3326) - index.tsx: order <= 9 -> order < 9 (off-by-one, server #10 showed shortcut) - ServerButton.tsx: conditional shortcut string in tooltip (null rendered as '(^+null)') (cherry picked from commit e405a6a) * fix: keep screen picker sources stable under Electron 42 macOS capture stack (#3414) Electron 42's macOS ScreenCaptureKit backend bounds getSources() at ~3s (upstream hang/crash fixes), returning an empty array or empty thumbnails when enumerations run back-to-back. The picker's 3s polling plus the post-selection re-enumeration turned those empty results into a blank "No windows found" list and denied valid share attempts. - Split the desktop capturer cache into per-type buckets: screen-only enumeration is fast (~700ms) and reliable; window enumeration is paced with a 4s post-completion cooldown (cold-start chain bypasses it once) - Never overwrite a non-empty bucket with an empty enumeration result; keep the bucket stale so it retries on the next opportunity - Merge thumbnails by source id so a source arriving with an empty thumbnail keeps its last good preview instead of being dropped - Validate the selected source against the cache (the same list the picker rendered from) instead of re-enumerating on Share; fall back to one direct enumeration only when the cache is empty - Keep the previous source list when a renderer fetch fails Measured on macOS with Electron 42.5.0: tight-loop getSources returned 0 sources in ~75% of calls; screen-only calls never failed; alternating per-type calls with 4s gaps returned complete results every round. Documented in docs/KNOWN_ISSUES.md with upstream refs (electron/electron#51128, electron/electron#50960) and the macOS 15+ useSystemPicker follow-up. (cherry picked from commit dddd2cc) * fix: externalize dependency subpath imports in rollup bundles (#3411) rollup externals matched module ids by exact name only, so subpath entrypoints like `react-dom/client` (used by createRoot in rootWindow.ts, log-viewer-window.tsx, and screenSharePickerMount.tsx) did not match the `react-dom` external and got bundled as build-time-NODE_ENV ReactDOM, while `react` stayed external and resolved to a different (dev) copy from the asar at runtime — mixing incompatible React internals and crashing the renderer (SUP-1072, ReferenceError: exports is not defined). makeExternal() now matches `id === moduleName || id.startsWith(moduleName + '/')` so subpath imports are externalized alongside their base package. scripts/check-bundle-externals.mjs guards the build against regressing back to bundled React internals, wired into `yarn build`. (backported to 4.15.x) Adapted from master commit 0dafe39: 4.15.4 has no `screen-picker-window.js` rollup entry (the screen picker mounts inside the video-call-window bundle via screenSharePickerMount.tsx, not a separate window/entry point in this release), so that entry and its assertions were omitted from check-bundle-externals.mjs. All other bundle configs and the check script are ported as-is. * fix: correct auto-merge artifact in ScreenSharingRequestTracker backport The cherry-pick of dddd2cc (#3414) auto-merged with references to master's entry/finishActive queue API, which doesn't exist in 4.15.4's flat cb/markComplete structure — this failed typecheck. Replaced with 4.15.4's existing cb(...) calling convention, matching the fallback path directly below it in the same function. Also fixed the corresponding cherry-picked spec assertion: it expected cb to be called with `null` (master's deny convention), but 4.15.4's DisplayMediaCallback deny convention is `{ video: false }` throughout this file — updated the assertion to match. * fix: remove leftover merge-conflict marker fragment from KNOWN_ISSUES.md The docs/KNOWN_ISSUES.md merge during the backport left a trailing `>>>>>>> dddd2cc (...)` conflict-marker remnant appended to the last line of the file. Removed; no other content was affected.
1 participant
What
On macOS, the screen picker could intermittently show "No windows found" / "No screens found" seconds after listing sources, and clicking Share on a visible source could be silently denied with
selected source no longer available. This PR makes the picker's source pipeline resilient to the newdesktopCapturer.getSources()behavior in Electron 42.Why
Electron 42's macOS capture backend (ScreenCaptureKit) bounds
getSources()at ~3s — upstream fixes for an indefinite hang and a DCHECK crash (electron/electron#51128, electron/electron#50960 — the latter shipped in 42.0.1). Instead of blocking until enumeration completes (~24s observed under Electron 41.9 on the same machine), calls now return whatever arrived within ~3s.Measured on macOS with Electron 42.5.0 (scripts in
docs/KNOWN_ISSUES.mdentry):['window','screen'], back-to-back['screen']only, back-to-back['screen']/['window'], 4s gapsThe app's previous design assumed
getSources()was reliable: the cache overwrote itself with empty results (picker blanks), the picker polled every 3s (continuous back-to-back enumeration), and the post-Share validation re-enumerated and denied the share when it hit an empty result.How
desktopCapturerCache.ts): screens and windows enumerate separately — screens are cheap and reliable, windows are paced with a 4s post-completion cooldown (single-flight preserved; the one-time cold-start chain bypasses the cooldown so the first picker open populates both tabs promptly). IPC contract unchanged.ScreenSharingRequestTracker.ts) — the same list the picker rendered from — instead of a fresh enumeration; a single direct enumeration remains only as the empty-cache fallback.screenSharePicker.tsx).Windows/Linux X11 use the same cache path with fast native backends — the pacing is a no-op there in practice. The Linux Wayland portal path doesn't use this pipeline and is untouched.
Testing
desktopCapturerCache.main.spec.ts(21 tests): empty-result preservation, thumbnail merge, cooldown gating, cold-start bypass, single-flight, merged accessor.ScreenSharingRequestTracker.main.spec.ts(36 tests): cache-based validation, deny on genuinely-missing source, empty-cache fallback.npx tsc --noEmitandyarn lintclean.Follow-up
Adopt
setDisplayMediaRequestHandler(..., { useSystemPicker: true })(native SCContentSharingPicker) on macOS 15+, which removesgetSources()from the flow entirely — tracked separately; experimental API with an open audio caveat (electron/electron#44685).Summary by CodeRabbit