feat: show screen picker on originating popout window and queue concurrent requests#3399
Conversation
…rrent requests Screen share requests from popout windows (webapp window.open) share the server webview session, so the per-session display-media handler always showed the picker on the main window and rejected concurrent requests. - Resolve the originating window from request.frame; popout-originated requests open a dedicated picker window parented to the popout - Queue concurrent requests FIFO in ScreenSharingRequestTracker instead of rejecting them, matching Chrome's one-at-a-time behavior - Add cancelAll() teardown so closing the video call window settles in-flight requests and closes popout picker windows - Thread originWindow through picker providers; portal (Linux) path unchanged SSGA-31
WalkthroughThis PR adds queued and cancelable screen-sharing requests, a standalone Electron picker window with HTML and React entrypoints, and origin-aware routing through server-view and video-call IPC flows. ChangesScreen Picker Window and Queued Request Tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant IPC as videoCallWindow/ipc.ts
participant Resolver as resolveStandaloneOriginWindow
participant Picker as requestViaPickerWindow
participant Window as ScreenPickerWindow
participant Tracker as ScreenSharingRequestTracker
Webview->>IPC: display-media request
IPC->>Resolver: resolve request frame
Resolver-->>IPC: origin window or null
IPC->>Picker: route foreign-origin request
Picker->>Tracker: createRequest with validity and completion handlers
Picker->>Window: open picker with channel parameters
Window-->>Picker: selection, denial, or close
Picker->>Tracker: cancel or complete request
Tracker-->>IPC: display-media result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Linux installer download |
macOS installer download |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screenSharing/ScreenSharingRequestTracker.ts (1)
46-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
cleanup()does not settle the active entry before clearing it.
cleanup()setsthis.activeEntry = nullwithout calling the active entry's callback oronDonehook. Ifcleanup()is invoked while a request is active (e.g., by external code or a future caller), the active request's callback will never fire, leaving the caller hanging indefinitely. Compare withcancelAll()(line 244), which correctly settles the active entry viacancelActiveEntrybefore draining the queue.The test at
ScreenSharingRequestTracker.main.spec.tslines 510-526 also does not verify the active entry's callback aftercleanup(), masking this gap.🔧 Proposed fix: settle active entry in cleanup()
cleanup(): void { + if (this.activeEntry && !this.activeEntry.settled) { + this.activeEntry.settled = true; + this.activeEntry.cb({ video: false } as any); + this.activeEntry.options?.onDone?.(); + } + if (this.activeListener) { ipcMain.removeListener(this.responseChannel, this.activeListener); this.activeListener = null; }🤖 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/ScreenSharingRequestTracker.ts` around lines 46 - 70, `cleanup()` in `ScreenSharingRequestTracker` clears `this.activeEntry` without settling the in-flight request, so update it to reuse the same active-entry settlement path used by `cancelAll()`/`cancelActiveEntry` before nulling state. Ensure the active entry callback and `onDone` are invoked when `cleanup()` runs, then drain the remaining queue as it does today. Also update the `cleanup()` spec in `ScreenSharingRequestTracker.main.spec.ts` to assert the active request is settled, not just the queued entries.
🧹 Nitpick comments (2)
src/screenSharing/ScreenSharingRequestTracker.ts (1)
95-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
finishActiverelies on the caller having already calledmarkComplete().
finishActivedoes not callmarkComplete()itself, depending on every call site to do it first. All current call sites (listener lines 144, timeout line 191) do callmarkComplete()beforefinishActive, so this is correct today. However, it's fragile — a future call site that forgetsmarkComplete()would leaveisPendingstale andactiveEntrydangling. Consider callingmarkComplete()insidefinishActive(or documenting the precondition) to make the contract self-enforcing.🤖 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/ScreenSharingRequestTracker.ts` around lines 95 - 99, `finishActive` in `ScreenSharingRequestTracker` currently assumes callers have already invoked `markComplete()`, which makes the completion contract fragile. Update `finishActive(entry: QueueEntry)` to enforce that contract itself by calling `markComplete()` before setting `settled`, invoking `onDone`, and advancing with `processNext()`, or otherwise add a clear precondition near `finishActive` and its call sites (`listener` and timeout handling) if you keep the current behavior.src/screenSharing/screenPickerWindow.ts (1)
60-67: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider blocking all navigation instead of allowing
file://URLs.The picker window should only ever display its initial HTML. Allowing
file://navigation means the window could navigate to any local file, which would execute withnodeIntegration: trueif it contains scripts. Blocking all navigation is safer since there is no legitimate navigation use case for this window.🔒 Proposed: block all navigation
screenPickerWindow.webContents.on( 'will-navigate', - (event: Event, url: string) => { - if (!url.startsWith('file://')) { - event.preventDefault(); - } + (event: Event) => { + event.preventDefault(); } );🤖 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/screenPickerWindow.ts` around lines 60 - 67, The screen picker window navigation guard in screenPickerWindow.webContents currently allows file:// URLs, which leaves an unsafe local-file navigation path open. Update the will-navigate handler in screenPickerWindow so it prevents every navigation attempt unconditionally, rather than checking the URL prefix, since this window should only ever show its initial HTML. Keep the fix scoped to the existing screenPickerWindow setup and its webContents event listener.
🤖 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 `@src/public/screen-picker-window.html`:
- Line 5: The current Content Security Policy in screen-picker-window HTML only
constrains script loading, so strengthen the policy by adding a restrictive
default-src alongside the existing script-src. Update the meta CSP tag in
screen-picker-window.html so default-src is explicitly set to a safe local-only
baseline, keeping the current script restriction intact and covering other
resource types as well.
In `@src/screenSharing/screenPickerWindow.ts`:
- Around line 14-22: openScreenPickerWindow currently calls parent.getBounds()
without verifying the BrowserWindow is still alive, which can throw if the
window is destroyed between the validation in startRequest and this callback.
Add a parent.isDestroyed() guard at the start of openScreenPickerWindow, before
any use of parent.getBounds(), and exit early or avoid creating the picker
window when the parent is gone. Keep the check alongside the existing parent
bounds logic so the unsafe access is prevented in this function.
- Around line 36-38: The screen picker window is still created with Node-enabled
renderer settings, which leaves the UI with unnecessary Node access. Update the
BrowserWindow configuration in screenPickerWindow so it uses a preload script
with contextBridge for the picker IPC actions, then switch webPreferences to
contextIsolation: true and nodeIntegration: false. Keep the existing picker flow
intact by moving any renderer-side Node usage behind the preload bridge.
---
Outside diff comments:
In `@src/screenSharing/ScreenSharingRequestTracker.ts`:
- Around line 46-70: `cleanup()` in `ScreenSharingRequestTracker` clears
`this.activeEntry` without settling the in-flight request, so update it to reuse
the same active-entry settlement path used by `cancelAll()`/`cancelActiveEntry`
before nulling state. Ensure the active entry callback and `onDone` are invoked
when `cleanup()` runs, then drain the remaining queue as it does today. Also
update the `cleanup()` spec in `ScreenSharingRequestTracker.main.spec.ts` to
assert the active request is settled, not just the queued entries.
---
Nitpick comments:
In `@src/screenSharing/screenPickerWindow.ts`:
- Around line 60-67: The screen picker window navigation guard in
screenPickerWindow.webContents currently allows file:// URLs, which leaves an
unsafe local-file navigation path open. Update the will-navigate handler in
screenPickerWindow so it prevents every navigation attempt unconditionally,
rather than checking the URL prefix, since this window should only ever show its
initial HTML. Keep the fix scoped to the existing screenPickerWindow setup and
its webContents event listener.
In `@src/screenSharing/ScreenSharingRequestTracker.ts`:
- Around line 95-99: `finishActive` in `ScreenSharingRequestTracker` currently
assumes callers have already invoked `markComplete()`, which makes the
completion contract fragile. Update `finishActive(entry: QueueEntry)` to enforce
that contract itself by calling `markComplete()` before setting `settled`,
invoking `onDone`, and advancing with `processNext()`, or otherwise add a clear
precondition near `finishActive` and its call sites (`listener` and timeout
handling) if you keep the current behavior.
🪄 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: 828116c1-8a81-4f36-8637-c4b3172c7215
📒 Files selected for processing (14)
rollup.config.mjssrc/public/screen-picker-window.htmlsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/popoutPickerRequest.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/serverViewScreenSharing.tssrc/videoCallWindow/ipc.tssrc/videoCallWindow/main/ipc.main.spec.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Build pull request artifacts / build (ubuntu-latest, linux): feat: show screen picker on originating popout window and queue concurrent requests
Conclusion: failure
##[group]Run SNAP_FILE=$(find dist/ -maxdepth 1 -name 'rocketchat-*.snap' -print -quit)
�[36;1mSNAP_FILE=$(find dist/ -maxdepth 1 -name 'rocketchat-*.snap' -print -quit)�[0m
�[36;1mif [ -z "$SNAP_FILE" ]; then�[0m
�[36;1m echo "::error::Snap file not found in dist/"�[0m
GitHub Actions: Build pull request artifacts / 0_build (ubuntu-latest, linux).txt: feat: show screen picker on originating popout window and queue concurrent requests
Conclusion: failure
##[group]Run SNAP_FILE=$(find dist/ -maxdepth 1 -name 'rocketchat-*.snap' -print -quit)
�[36;1mSNAP_FILE=$(find dist/ -maxdepth 1 -name 'rocketchat-*.snap' -print -quit)�[0m
�[36;1mif [ -z "$SNAP_FILE" ]; then�[0m
�[36;1m echo "::error::Snap file not found in dist/"�[0m
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.ts
🔇 Additional comments (12)
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
82-97: LGTM!Also applies to: 99-117, 119-138, 140-162, 164-189, 191-216, 220-274, 528-578
src/screenSharing/screen-picker-window.tsx (2)
21-37: LGTM!Also applies to: 39-54, 56-68, 70-93, 95-112
1-2: 📐 Maintainability & Code Quality
Themeshas no root export The deep type import is the available declaration, so there’s nothing to switch in@rocket.chat/fuselage’s public barrel.> Likely an incorrect or invalid review comment.rollup.config.mjs (1)
270-303: LGTM!src/screenSharing/popoutPickerRequest.ts (1)
14-42: LGTM!The bidirectional cleanup design is sound: the picker
closedevent cancels the tracker handle, andonDonecloses the picker window. Thehandle?.cancel()call in theclosedlistener is safe becausecreateRequestreturns synchronously before any asyncclosedevent can fire, sohandleis always assigned by the time the listener executes.src/screenSharing/main/resolveStandaloneOriginWindow.main.spec.ts (1)
1-90: LGTM!Test coverage is thorough — null/undefined frames, missing
fromFrameresults, webview guest exclusion viahostWebContents, and the standalone-window happy path are all covered with correct mock assertions.src/screenSharing/screenPicker/types.ts (1)
1-2: LGTM!The optional
originWindow?: BrowserWindowparameter is correctly added to the interface, and the JSDoc clearly explains its purpose.PortalPickerProvideromitting the parameter remains valid TypeScript since optional params can be dropped in implementations.Also applies to: 23-30
src/screenSharing/screenPicker/providers/InternalPickerProvider.ts (1)
1-2: LGTM!The
handleRequestFntype,setHandleRequestHandlersignature, andhandleDisplayMediaRequestall correctly thread the optionaloriginWindowthrough to the main-process handler.Also applies to: 23-25, 34-37, 50-55
src/screenSharing/screenPicker/providers/PortalPickerProvider.ts (1)
13-14: LGTM!The added comments clearly explain why
originWindowis irrelevant for the OS-level portal dialog. The signature omitting the parameter is valid TypeScript for an optional interface param.src/screenSharing/serverViewScreenSharing.ts (1)
1-10: LGTM!
resolveStandaloneOriginWindowcorrectly returnsnullfor webview guests (viahostWebContentscheck) and returns the owningBrowserWindowfor standalone windows.createServerViewPickerHandlerproperly routes throughrequestViaPickerWindowwhen a validoriginWindowexists and falls back to the root-window path otherwise. ThehandleServerViewDisplayMediaRequestsignature correctly threadsoriginWindowthrough to the provider.Also applies to: 22-63, 92-92, 117-123, 164-179
src/videoCallWindow/ipc.ts (1)
21-29: LGTM!The
cancelAll()replacements in cleanup/close/open-screen-picker paths are correct — they settle both active and queued requests and invokeonDoneto close any orphaned popout picker windows. ThecreateInternalPickerHandlercorrectly routes popout-originated requests (validoriginWindow≠videoCallWindow) throughrequestViaPickerWindowwhile preserving the call-window picker path for in-call requests. ThesetupDisplayMediaHandlerorigin resolution and shared-session routing logic (fromCallWindowcheck → server-view vs call-window picker) is sound.Also applies to: 85-90, 215-218, 265-283, 347-380, 789-790, 829-830, 1194-1199
src/videoCallWindow/main/ipc.main.spec.ts (1)
162-168: LGTM!The
cancelAll = jest.fn()addition correctly matches the new tracker API used by the IPC teardown paths.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (2)
99-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression test for cancellation during in-flight async validation.
None of the FIFO/queue tests exercise
handle.cancel()(or anisStillValidfailure) whiledesktopCapturer.getSources()is still pending for the active entry — this is exactly the scenario behind the double-settlement race flagged inScreenSharingRequestTracker.ts(lines 126-176). Suggest adding a test that callscancel()between issuing the listener response and resolvinggetSourcesMock, assertingcbis invoked only once.🤖 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/ScreenSharingRequestTracker.main.spec.ts` around lines 99 - 138, Add a regression test in ScreenSharingRequestTracker.main.spec.ts that covers cancelling the active request while desktopCapturer.getSources() is still pending in ScreenSharingRequestTracker.createRequest/handleResponse flow. Reuse the existing ScreenSharingRequestTracker, getSourcesMock, and getRegisteredListener setup, trigger the listener, call handle.cancel() (or force isStillValid to fail) before resolving getSourcesMock, then resolve the promise and assert the callback is invoked only once and the request is not double-settled.
510-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap:
cleanup()behavior for an active (non-queued) entry isn't asserted.This test only checks that queued entries are drained; it doesn't assert what happens to the already-active entry's
cb/onDonewhencleanup()runs mid-request. This matches the gap flagged inScreenSharingRequestTracker.ts(cleanup()lines 58-70) — once that's fixed, add an assertion here that the active entry'scb/onDoneare also invoked.🤖 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/ScreenSharingRequestTracker.main.spec.ts` around lines 510 - 525, The cleanup test in ScreenSharingRequestTracker.main.spec.ts only verifies queued requests are drained, but it misses the active entry handled by ScreenSharingRequestTracker.cleanup(). Update this spec to assert that when cleanup runs mid-request, the currently active request’s cb and onDone are also invoked, using the existing tracker.createRequest, cleanup, queuedCb, and queuedOnDone setup to distinguish active versus queued behavior.src/screenSharing/ScreenSharingRequestTracker.ts (1)
95-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the repeated settle-and-advance pattern;
cancelActiveEntry/processNextcoupling is implicit.The
entry.cb({ video: false } as any); this.finishActive(entry);(orentry.options?.onDone?.()) pattern is repeated ~5 times acrossfinishActive,cancelActiveEntry,processNext, and the listener/timeout bodies. Also,cancelActiveEntry()does not itself callprocessNext()— every caller must remember to call it separately (correctly done today incancel(), intentionally skipped incancelAll()), which is an easy invariant to break in future changes (as the critical bug above illustrates). Extracting a singlesettle(entry, result)helper, and making the active-vs-cancel-all advancement explicit (e.g. a parameter), would reduce the chance of another regression like the one flagged above.Also applies to: 200-242
🤖 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/ScreenSharingRequestTracker.ts` around lines 95 - 124, The settle-and-advance logic is duplicated across finishActive, cancelActiveEntry, and processNext, and the implicit requirement to call processNext after cancelActiveEntry is easy to miss. Refactor ScreenSharingRequestTracker by extracting a single helper for settling a QueueEntry (invoked from finishActive, cancelActiveEntry, and the validity-check path in processNext) and make the “advance to next” behavior explicit via a parameter or separate helper. Keep the existing semantics for cancel() versus cancelAll(), but ensure the helper consistently handles entry.settled, entry.cb({ video: false } as any), and entry.options?.onDone?.() in one place.
🤖 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 `@src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts`:
- Around line 99-138: Add a regression test in
ScreenSharingRequestTracker.main.spec.ts that covers cancelling the active
request while desktopCapturer.getSources() is still pending in
ScreenSharingRequestTracker.createRequest/handleResponse flow. Reuse the
existing ScreenSharingRequestTracker, getSourcesMock, and getRegisteredListener
setup, trigger the listener, call handle.cancel() (or force isStillValid to
fail) before resolving getSourcesMock, then resolve the promise and assert the
callback is invoked only once and the request is not double-settled.
- Around line 510-525: The cleanup test in
ScreenSharingRequestTracker.main.spec.ts only verifies queued requests are
drained, but it misses the active entry handled by
ScreenSharingRequestTracker.cleanup(). Update this spec to assert that when
cleanup runs mid-request, the currently active request’s cb and onDone are also
invoked, using the existing tracker.createRequest, cleanup, queuedCb, and
queuedOnDone setup to distinguish active versus queued behavior.
In `@src/screenSharing/ScreenSharingRequestTracker.ts`:
- Around line 95-124: The settle-and-advance logic is duplicated across
finishActive, cancelActiveEntry, and processNext, and the implicit requirement
to call processNext after cancelActiveEntry is easy to miss. Refactor
ScreenSharingRequestTracker by extracting a single helper for settling a
QueueEntry (invoked from finishActive, cancelActiveEntry, and the validity-check
path in processNext) and make the “advance to next” behavior explicit via a
parameter or separate helper. Keep the existing semantics for cancel() versus
cancelAll(), but ensure the helper consistently handles entry.settled,
entry.cb({ video: false } as any), and entry.options?.onDone?.() in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 828116c1-8a81-4f36-8637-c4b3172c7215
📒 Files selected for processing (14)
rollup.config.mjssrc/public/screen-picker-window.htmlsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/popoutPickerRequest.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/serverViewScreenSharing.tssrc/videoCallWindow/ipc.tssrc/videoCallWindow/main/ipc.main.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.tssrc/screenSharing/ScreenSharingRequestTracker.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.tssrc/screenSharing/ScreenSharingRequestTracker.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/screenSharing/popoutPickerRequest.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/screen-picker-window.tsxsrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/videoCallWindow/ipc.tssrc/screenSharing/ScreenSharingRequestTracker.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/screenSharing/main/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
🔇 Additional comments (29)
src/screenSharing/serverViewScreenSharing.ts (6)
1-10: LGTM!
33-44: LGTM!
resolveStandaloneOriginWindowcorrectly distinguishes a standalone popout (nohostWebContents) from a webview guest (hashostWebContents), consistent with Electron's documentedhostWebContentssemantics.
92-92: LGTM!
117-123: LGTM!
164-179: LGTM!
46-73: 🩺 Stability & AvailabilityHandle root-window lookup failures by settling the queued request. If
getRootWindow()resolves tonull/destroyed or rejects here, the active FIFO entry can stay pending and block later requests; use the tracker’s cancel/settle path instead of only logging.src/videoCallWindow/ipc.ts (6)
21-29: LGTM!
85-90: LGTM!
215-218: LGTM!Replacing
cleanup()withcancelAll()in the window-cleanup,closed, andclosepaths correctly drains both the active entry and the queue so a popout-parented picker window isn't orphaned.Also applies to: 789-790, 829-830
264-296: LGTM!Correctly falls back to
cb({ video: false })when the video call window is unavailable, unlike the analogous fallback path inserverViewScreenSharing.ts.
347-380: LGTM!Origin/
popoutOrigin/fromCallWindowrouting correctly separates in-call requests, main-server-webview requests (shared session), and genuine popout requests.
1194-1199: 🩺 Stability & Availability
cancelAll()may reject an active popout picker request. This handler clears the shared tracker before claimingvideo-call-window/screen-sharing-source-responded, so an in-flightrequestViaPickerWindowcould be force-closed ifopen-screen-pickerraces with it. If that preemption is intentional, keep it; otherwise queue this request behind the active one.src/videoCallWindow/main/ipc.main.spec.ts (1)
162-169: LGTM!src/screenSharing/ScreenSharingRequestTracker.ts (4)
8-39: LGTM!
185-194: LGTM!Also applies to: 244-258
58-70: 🩺 Stability & AvailabilityEnsure
cleanup()settles an in-flight request. If this path can run whileactiveEntryis set, settle it before clearing state; otherwise thegetDisplayMedia()promise can hang.
126-176: 🩺 Stability & AvailabilityPotential stale callback after async source lookup
src/screenSharing/ScreenSharingRequestTracker.ts:126-176—entry.settledis only checked beforeawait desktopCapturer.getSources(). If cancellation settles the entry while that call is pending, this branch can still invokeentry.cb(...)andfinishActive(entry)for the same request. Re-checkentry.settledafter the await, or defermarkComplete()until settlement.src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
82-274: LGTM!src/public/screen-picker-window.html (1)
1-33: LGTM!src/screenSharing/screenPickerWindow.ts (2)
1-35: LGTM!Also applies to: 40-75
36-39: 🔒 Security & Privacy
nodeIntegration: true+contextIsolation: falseshould stay limited to trusted local content. If this window can ever be reached by untrusted content, switch to a preload bridge withcontextIsolation: true.src/screenSharing/screen-picker-window.tsx (2)
39-54: 🎯 Functional CorrectnessVerify
PaletteStyleTag'sselectorprop against the installed Fuselage version's typings.As per coding guidelines, library props should be verified against official docs/
.d.tsfiles rather than assumed. Please confirmselectoris a valid prop forPaletteStyleTagin@rocket.chat/fuselage@0.78.0(checknode_modules/@rocket.chat/fuselage/dist/components/PaletteStyleTag).
As per coding guidelines: "Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification."Source: Coding guidelines
1-38: LGTM!Also applies to: 56-113
src/screenSharing/popoutPickerRequest.ts (1)
1-43: LGTM!rollup.config.mjs (1)
270-303: LGTM!src/screenSharing/main/resolveStandaloneOriginWindow.main.spec.ts (1)
1-90: LGTM!src/screenSharing/screenPicker/types.ts (1)
1-2: LGTM!Also applies to: 23-30
src/screenSharing/screenPicker/providers/InternalPickerProvider.ts (1)
1-2: LGTM!Also applies to: 23-25, 34-37, 50-55
src/screenSharing/screenPicker/providers/PortalPickerProvider.ts (1)
13-14: LGTM!
- CSP: add default-src 'self' baseline (img-src data: for source thumbnails, style-src 'unsafe-inline' for injected styles) - will-navigate: deny all navigation instead of allowing file://
…nt races
Electron's setDisplayMediaRequestHandler callback only accepts null as
a deny (DisplayMediaDeviceChosen in electron_browser_context.cc);
{video: false} and {} both throw a TypeError, surfacing as unhandled
rejections in main and leaving the renderer request hanging until
Chromium times out. Replace all 19 deny sites with cb(null), type
DisplayMediaCallback as nullable, and drop the as-any casts that hid
the invalid shape (boundary casts only where Electron's raw callback,
whose d.ts omits null, crosses into the typed interface).
Tracker settlement fixes from review verification:
- markComplete moved into finishActive: tracker no longer looks idle
while desktopCapturer.getSources validation is in flight, so
concurrent requests queue instead of draining the queue early or
starting a second active request
- entry.settled re-checked after the await: cancellation during
validation can no longer double-settle the callback
- cleanup() settles the active entry before clearing it
Adds regression tests for cancel-during-validation, queue-during-
validation, and cleanup mid-request.
Aligns the provider specs introduced on master (#3393) with this branch's display-media changes: deny shape is callback(null) and the internal picker handler now receives an optional originWindow.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test verifying
originWindowis forwarded to the handler.The current test only covers
originWindow = undefined. Since threadingoriginWindowthrough providers is a key change in this PR, a test with an actualBrowserWindowwould verify the forwarding path.✨ Suggested additional test
it('forwards request to provided handler', () => { const callback = jest.fn() as jest.MockedFunction<DisplayMediaCallback>; const handle = jest.fn((cb: DisplayMediaCallback) => cb(null)); provider.setHandleRequestHandler(handle); provider.handleDisplayMediaRequest(callback); expect(handle).toHaveBeenCalledWith(callback, undefined); expect(callback).toHaveBeenCalledWith(null); }); + + it('forwards originWindow to provided handler', () => { + const callback = jest.fn() as jest.MockedFunction<DisplayMediaCallback>; + const handle = jest.fn(); + const mockWindow = {} as BrowserWindow; + + provider.setHandleRequestHandler(handle); + provider.handleDisplayMediaRequest(callback, mockWindow); + + expect(handle).toHaveBeenCalledWith(callback, mockWindow); + });🤖 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/screenPicker/__tests__/InternalPickerProvider.spec.ts` around lines 19 - 28, Add a test alongside the existing “forwards request to provided handler” case that creates or mocks a BrowserWindow, invokes handleDisplayMediaRequest with that originWindow, and asserts the registered handler receives both the callback and the same BrowserWindow instance.
🤖 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 `@src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts`:
- Line 12: Rename the test case describing the missing handler in
InternalPickerProvider tests so it states that the callback is invoked with
null, matching the existing assertion and current behavior.
In `@src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.ts`:
- Line 22: Rename the tests in PortalPickerProvider.spec.ts at the cases around
“falls back to video false when no sources are selected” and the corresponding
second case so their descriptions state that video falls back to null, matching
the current assertions.
---
Nitpick comments:
In `@src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts`:
- Around line 19-28: Add a test alongside the existing “forwards request to
provided handler” case that creates or mocks a BrowserWindow, invokes
handleDisplayMediaRequest with that originWindow, and asserts the registered
handler receives both the callback and the same BrowserWindow instance.
🪄 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: 02b98d6f-1358-47f3-9202-068d756c61a1
📒 Files selected for processing (11)
src/public/screen-picker-window.htmlsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/serverViewScreenSharing.tssrc/videoCallWindow/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/public/screen-picker-window.html
- src/screenSharing/screenPicker/types.ts
- src/screenSharing/screenPicker/providers/InternalPickerProvider.ts
- src/videoCallWindow/ipc.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CodeRabbit / Review
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: build (macos-latest, mac)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (windows-latest, windows)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; only create custom components when Fuselage does not provide what is needed.
Import UI components from@rocket.chat/fuselage.
CheckTheme.d.tsfor valid color tokens before using Fuselage theme colors.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs likeprocess.getuid(),process.getgid(),process.geteuid(), andprocess.getegid().
Only mock platform-specific APIs when defensive coding is not possible.
Use TypeScript strict mode.
Use React functional components with hooks.
Redux actions must follow the Flux Standard Action (FSA) pattern.
Use camelCase for file names and PascalCase for components.
**/*.{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.
Files:
src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/serverViewScreenSharing.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfor main process tests.Main-process specs use
*.main.spec.ts.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.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.ts
🔇 Additional comments (7)
src/screenSharing/ScreenSharingRequestTracker.ts (1)
57-77: LGTM!Also applies to: 103-103, 109-155, 164-189, 206-206, 251-251, 270-270
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
151-151: LGTM!Also applies to: 161-161, 185-185, 217-255, 273-273, 314-344, 380-380, 395-395, 410-410, 422-430, 471-471, 495-501, 532-532, 597-613, 625-625, 641-641
src/screenSharing/screenPickerWindow.ts (1)
60-62: LGTM!src/screenSharing/screenPicker/providers/PortalPickerProvider.ts (1)
13-14: LGTM!Also applies to: 31-39
src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts (1)
30-60: LGTM!src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.ts (1)
31-41: LGTM!Also applies to: 55-58
src/screenSharing/serverViewScreenSharing.ts (1)
117-133: LGTM!Also applies to: 173-203
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: 2
🧹 Nitpick comments (1)
src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test verifying
originWindowis forwarded to the handler.The current test only covers
originWindow = undefined. Since threadingoriginWindowthrough providers is a key change in this PR, a test with an actualBrowserWindowwould verify the forwarding path.✨ Suggested additional test
it('forwards request to provided handler', () => { const callback = jest.fn() as jest.MockedFunction<DisplayMediaCallback>; const handle = jest.fn((cb: DisplayMediaCallback) => cb(null)); provider.setHandleRequestHandler(handle); provider.handleDisplayMediaRequest(callback); expect(handle).toHaveBeenCalledWith(callback, undefined); expect(callback).toHaveBeenCalledWith(null); }); + + it('forwards originWindow to provided handler', () => { + const callback = jest.fn() as jest.MockedFunction<DisplayMediaCallback>; + const handle = jest.fn(); + const mockWindow = {} as BrowserWindow; + + provider.setHandleRequestHandler(handle); + provider.handleDisplayMediaRequest(callback, mockWindow); + + expect(handle).toHaveBeenCalledWith(callback, mockWindow); + });🤖 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/screenPicker/__tests__/InternalPickerProvider.spec.ts` around lines 19 - 28, Add a test alongside the existing “forwards request to provided handler” case that creates or mocks a BrowserWindow, invokes handleDisplayMediaRequest with that originWindow, and asserts the registered handler receives both the callback and the same BrowserWindow instance.
🤖 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 `@src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts`:
- Line 12: Rename the test case describing the missing handler in
InternalPickerProvider tests so it states that the callback is invoked with
null, matching the existing assertion and current behavior.
In `@src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.ts`:
- Line 22: Rename the tests in PortalPickerProvider.spec.ts at the cases around
“falls back to video false when no sources are selected” and the corresponding
second case so their descriptions state that video falls back to null, matching
the current assertions.
---
Nitpick comments:
In `@src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts`:
- Around line 19-28: Add a test alongside the existing “forwards request to
provided handler” case that creates or mocks a BrowserWindow, invokes
handleDisplayMediaRequest with that originWindow, and asserts the registered
handler receives both the callback and the same BrowserWindow instance.
🪄 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: 02b98d6f-1358-47f3-9202-068d756c61a1
📒 Files selected for processing (11)
src/public/screen-picker-window.htmlsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenPicker/providers/InternalPickerProvider.tssrc/screenSharing/screenPicker/providers/PortalPickerProvider.tssrc/screenSharing/screenPicker/types.tssrc/screenSharing/screenPickerWindow.tssrc/screenSharing/serverViewScreenSharing.tssrc/videoCallWindow/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/public/screen-picker-window.html
- src/screenSharing/screenPicker/types.ts
- src/screenSharing/screenPicker/providers/InternalPickerProvider.ts
- src/videoCallWindow/ipc.ts
📜 Review details
🔇 Additional comments (7)
src/screenSharing/ScreenSharingRequestTracker.ts (1)
57-77: LGTM!Also applies to: 103-103, 109-155, 164-189, 206-206, 251-251, 270-270
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
151-151: LGTM!Also applies to: 161-161, 185-185, 217-255, 273-273, 314-344, 380-380, 395-395, 410-410, 422-430, 471-471, 495-501, 532-532, 597-613, 625-625, 641-641
src/screenSharing/screenPickerWindow.ts (1)
60-62: LGTM!src/screenSharing/screenPicker/providers/PortalPickerProvider.ts (1)
13-14: LGTM!Also applies to: 31-39
src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts (1)
30-60: LGTM!src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.ts (1)
31-41: LGTM!Also applies to: 55-58
src/screenSharing/serverViewScreenSharing.ts (1)
117-133: LGTM!Also applies to: 173-203
🛑 Comments failed to post (2)
src/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.ts (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update stale test name to match
nullassertion.The test name says "invokes callback with false" but the assertion at line 16 checks for
null. This is left over from the oldcallback({ video: false } as any)behavior.✏️ Proposed fix
- it('invokes callback with false when handler is missing', () => { + it('invokes callback with null when handler is missing', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('invokes callback with null when handler is missing', () => {🤖 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/screenPicker/__tests__/InternalPickerProvider.spec.ts` at line 12, Rename the test case describing the missing handler in InternalPickerProvider tests so it states that the callback is invoked with null, matching the existing assertion and current behavior.src/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.ts (1)
22-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update stale test names to match
nullassertions.Both test names reference "false" but the assertions check for
null, left over from the oldcallback({ video: false } as any)behavior.✏️ Proposed fix
- it('falls back to video false when no sources are selected', async () => { + it('calls callback with null when no sources are selected', async () => {- it('returns false on desktop capturer error', async () => { + it('calls callback with null on desktop capturer error', async () => {Also applies to: 43-43
🤖 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/screenPicker/__tests__/PortalPickerProvider.spec.ts` at line 22, Rename the tests in PortalPickerProvider.spec.ts at the cases around “falls back to video false when no sources are selected” and the corresponding second case so their descriptions state that video falls back to null, matching the current assertions.
1 participant
Summary
SSGA-31
Screen share requests from popout windows (created by the webapp via
window.open) share the server webview's session, so the desktop client always showed the screen picker on the main window regardless of which window originated thegetDisplayMediarequest. Concurrent requests were rejected outright (Electron overlaid pickers before that guard existed), unlike Chrome, which queues them and shows them one by one.This PR shows the picker on the window that originated the request and queues concurrent requests FIFO.
Changes
setDisplayMediaRequestHandlernow resolves the originating window fromrequest.frame(resolveStandaloneOriginWindow). Requests from the server webview keep the existing root-window picker; requests from a standalone popout window open a picker on that popout.screenPickerWindow.ts(main) +screen-picker-window.tsx(renderer entry) +screen-picker-window.html+ rollup entry. A fixed-size childBrowserWindowparented to the popout, reusing the existingScreenSharePickercomponent; IPC channel names passed via query params. All navigation andwindow.opendenied; CSP locked todefault-src 'self'.ScreenSharingRequestTrackerqueues concurrent requests instead of answering{video: false}. Each request gets acancel()handle; queued requests whose popout closed are skipped (isStillValid); the 60s timeout starts when a picker is shown, not when enqueued.ScreenSharingRequestTracker.cancelAll()settles the active request and drains the queue (closing any popout picker window) without starting the next one. All fourvideoCallScreenSharingTrackerreset sites invideoCallWindow/ipc.tsuse it — the previous silentcleanup()would leave a popout's picker window orphaned and itsgetDisplayMediapromise hanging when the call window closed.handleDisplayMediaRequestgains an optionaloriginWindowparameter, threaded throughInternalPickerProvider. The Linux portal picker path is unchanged (OS-level dialog, origin-agnostic).blob:/about:) get the same origin routing via the sharedpopoutPickerRequest.tshelper.callback(null). Electron only acceptsnullas a deny (DisplayMediaDeviceChoseninelectron_browser_context.cc); the previous{video: false}shape — pre-existing on master and hidden byas anycasts — throws aTypeErrorat runtime, surfacing as an unhandled rejection in main and leaving the renderer'sgetDisplayMediapromise hanging until Chromium times out.DisplayMediaCallbackis now typed nullable, with casts only at the boundary where Electron's raw callback (whose.d.tsomitsnull) crosses into it. Surfaced by live QA of the cancel flow.desktopCapturer.getSourcesvalidation is in flight (concurrent requests queue instead of draining the queue or double-starting), cancellation during validation can no longer double-settle a callback, andcleanup()settles the active entry instead of silently dropping it. Regression tests cover cancel-during-validation, queue-during-validation, and cleanup mid-request.Validation
yarn lint,npx tsc --noEmit: cleanyarn test: 70 suites, 1148 passed (new specs for queue behavior,cancelAll, and origin resolution)yarn build: compilesManually QA'd against a live server: picker on originating popout, FIFO queueing, and cancel/deny paths (no unhandled rejections; webapp receives
NotAllowedError).Manual test steps
Summary by CodeRabbit
nullinstead of{ video: false }.