fix: allow screen sharing without media devices - #40744
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
WalkthroughAdds end-to-end screen sharing: new localization and type contracts, call and WebRTC processor support for a screen-share track, relaxed negotiation/session input-track handling, provider and hooks to toggle and sync screen-share track, and OngoingCall UI to render remote screen-share video. ChangesScreen Sharing Feature
Sequence Diagram(s)sequenceDiagram
participant Call as ClientMediaCall
participant Processor as MediaCallWebRTCProcessor
participant Transceiver as VideoTransceiver
Call->>Processor: setScreenShareTrack(track)
Processor->>Processor: validate track.kind === 'video'
Processor->>Processor: loadScreenShareTrack()
Processor->>Transceiver: setTrack / updateDirection
Processor->>Call: emit streamChange
sequenceDiagram
participant UI as OngoingCall
participant Provider as MediaCallViewProvider
participant Controls as useMediaSessionControls
participant Call as ClientMediaCall
UI->>Provider: onToggleScreenShare()
Provider->>Provider: getDisplayMedia()
Provider->>Controls: setScreenShareTrack(track)
Controls->>Call: setScreenShareTrack(track)
Call->>Provider: streamChange -> context update
Provider->>UI: render remote video
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui-voip/src/providers/MediaCallViewProvider.tsx (1)
212-237: ⚡ Quick winGuard
onToggleScreenShareagainst re-entrancy.
screenShareTrackRef.currentis only set aftergetDisplayMediaresolves. If the toggle is invoked twice before the first prompt resolves, twogetDisplayMediarequests run; the second overwrites the ref, orphaning the first track so it keeps capturing the screen with no way to stop it. A simple in-flight flag avoids the orphaned capture.♻️ Proposed guard
+ const screenShareBusyRef = useRef(false); + const onToggleScreenShare = useCallback(async () => { if (screenShareTrackRef.current) { await stopScreenShare(); return; } + if (screenShareBusyRef.current) { + return; + } + screenShareBusyRef.current = true; try { if (!navigator.mediaDevices.getDisplayMedia) { return; } const stream = await navigator.mediaDevices.getDisplayMedia({ video: true }); const [track] = stream.getVideoTracks(); if (!track) { return; } screenShareTrackRef.current = track; track.addEventListener('ended', () => { void stopScreenShare(); }); await controls.setScreenShareTrack(track); } catch (error) { console.error('Media Call - Error sharing screen', error); + } finally { + screenShareBusyRef.current = false; } }, [controls, stopScreenShare]);🤖 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 `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx` around lines 212 - 237, onToggleScreenShare can be re-entered while navigator.mediaDevices.getDisplayMedia is pending, allowing two concurrent requests that orphan the first track; introduce an in-flight guard (e.g., a local ref like screenShareInFlightRef) checked at the start of onToggleScreenShare and set true before awaiting getDisplayMedia then set false in finally so a second call returns early; ensure the guard is cleared on both success and error and keep existing logic that sets screenShareTrackRef.current and calls stopScreenShare unchanged.
🤖 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 `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx`:
- Around line 212-237: onToggleScreenShare can be re-entered while
navigator.mediaDevices.getDisplayMedia is pending, allowing two concurrent
requests that orphan the first track; introduce an in-flight guard (e.g., a
local ref like screenShareInFlightRef) checked at the start of
onToggleScreenShare and set true before awaiting getDisplayMedia then set false
in finally so a second call returns early; ensure the guard is cleared on both
success and error and keep existing logic that sets screenShareTrackRef.current
and calls stopScreenShare unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 993b7255-c7d5-4504-8b65-5e8bf132f1c7
⛔ Files ignored due to path filters (1)
packages/ui-voip/src/views/MediaCallWidget/__snapshots__/MediaCallWidget.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
packages/i18n/src/locales/en.i18n.jsonpackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.tspackages/media-signaling/src/lib/Call.tspackages/media-signaling/src/lib/NegotiationManager.tspackages/media-signaling/src/lib/Session.tspackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/context/MediaCallViewContext.tspackages/ui-voip/src/context/definitions.d.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/providers/useMediaSession.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsx
💤 Files with no reviewable changes (1)
- packages/media-signaling/src/lib/NegotiationManager.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/ui-voip/src/context/definitions.d.tspackages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/media-signaling/src/lib/Call.tspackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/useMediaSession.ts
🧠 Learnings (6)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-voip/src/context/definitions.d.tspackages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/media-signaling/src/lib/Call.tspackages/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/useMediaSession.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-voip/src/context/definitions.d.tspackages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/media-signaling/src/lib/Call.tspackages/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/useMediaSession.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/ui-voip/src/context/definitions.d.tspackages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/ui-voip/src/providers/useMediaSessionInstance.tspackages/ui-voip/src/providers/useMediaSessionControls.tspackages/media-signaling/src/lib/Call.tspackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsxpackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/useMediaSession.ts
📚 Learning: 2026-02-26T19:22:29.385Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:29.385Z
Learning: For TSX files in the UI VOIP package, ensure that when a media session state is 'unavailable', the voiceCall action is excluded from the actions object passed to CallHistoryActions so it does not render in the menu. This filtering should occur upstream (before getItems is called) to avoid tooltips or UI hints for unavailable actions. If there are multiple actions with availability states, implement a centralized helper to filter actions based on session state.
Applied to files:
packages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsx
📚 Learning: 2026-05-05T12:34:29.042Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 40331
File: packages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx:69-69
Timestamp: 2026-05-05T12:34:29.042Z
Learning: In Rocket.Chat’s `packages/ui-voip` UI (e.g., media/call widgets), voice/media calls are only supported in Direct Message (DM) rooms. Rocket.Chat models a DM as a “room” with exactly two participants, so handlers like `onClickDirectMessage` are the correct destination—even when the UI text/element says “Open in room” (e.g., on the shared screen card/`StreamCard`). During review, don’t flag a “DM vs room” mismatch for these cases; they intentionally map to the same destination.
Applied to files:
packages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
packages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsxpackages/ui-voip/src/providers/MockedMediaCallProvider.tsxpackages/ui-voip/src/providers/MediaCallViewProvider.tsx
🔇 Additional comments (23)
packages/i18n/src/locales/en.i18n.json (1)
4875-4875: LGTM!Also applies to: 7116-7116
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
22-22: LGTM!Also applies to: 115-115
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
37-37: LGTM!Also applies to: 54-61
packages/ui-voip/src/context/MediaCallViewContext.ts (1)
15-15: LGTM!Also applies to: 31-32, 44-44
packages/ui-voip/src/context/definitions.d.ts (1)
31-32: LGTM!packages/media-signaling/src/lib/Call.ts (3)
189-190: LGTM!Also applies to: 232-232
475-492: LGTM!
1302-1302: LGTM!packages/media-signaling/src/lib/services/webrtc/Processor.ts (8)
25-26: LGTM!Also applies to: 54-54
84-95: LGTM!
107-107: LGTM!Also applies to: 331-334
200-211: 💤 Low valueVerify intent:
updateScreenShareDirectionBeforeNegotiationis called twice for offers.When processing a remote offer, the screen share direction is updated once before
peer.setRemoteDescription(line 200) and again immediately after (lines 205-207). Typically, setting direction before applying the remote description is sufficient. Confirm this double invocation is intentional (e.g., to handle transceivers created by the remote offer).
346-362: LGTM!
446-473: LGTM!
498-500: LGTM!Also applies to: 554-556
720-723: LGTM!packages/media-signaling/src/lib/Session.ts (1)
406-427: LGTM!packages/ui-voip/src/providers/MediaCallViewProvider.tsx (1)
116-134: LGTM!packages/ui-voip/src/providers/MockedMediaCallProvider.tsx (1)
79-79: LGTM!Also applies to: 148-149, 162-162, 179-179
packages/ui-voip/src/providers/useMediaSessionControls.ts (1)
43-43: LGTM!Also applies to: 67-78, 113-113
packages/ui-voip/src/views/MediaCallWidget/OngoingCall.tsx (1)
78-84: LGTM!Also applies to: 100-107
packages/ui-voip/src/providers/useMediaSession.ts (1)
141-142: ConfirmmainCall.getLocalMediaStream/getRemoteMediaStreamreturn a stream wrapper withhasVideo()
@rocket.chat/media-signaling’sIClientMediaCallexposesgetLocalMediaStream(tag?)/getRemoteMediaStream(tag?)returningIMediaStreamWrapper | null, andIMediaStreamWrapperdefineshasVideo(): boolean(implemented byMediaStreamWrapper). This makesBoolean(localMediaStream?.hasVideo())/Boolean(remoteMediaStream?.hasVideo())correct (including 167-168 and 201-202).packages/ui-voip/src/providers/useMediaSessionInstance.ts (1)
88-90: ⚡ Quick winNo unhandled promise rejection here—
MediaSignalTransport/writeStreamarevoid
MediaSignalTransportis defined as(signal: T) => void, andServerContext.writeStream(viauseWriteStream) also returnsvoid, sovoid this.sendSignal(signal)doesn’t convert a rejectable Promise into an unhandled rejection. If you still want diagnostics, guard against synchronous throws (e.g.,try/catch) rather than adding a.catch(...)on a Promise.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
/label state: WAS assured |
|
Please add the 'state: WAS assured' label and assign the required milestone/project to this PR. I don't have permissions to modify them. |
coderabbitai
Bot
removed
the
type: feature
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui-voip/src/providers/MediaCallViewProvider.tsx">
<violation number="1" location="packages/ui-voip/src/providers/MediaCallViewProvider.tsx:246">
P2: Race condition in catch cleanup: `screenShareTrackRef.current` (shared mutable ref) is read in the catch block instead of the specific track from the failing attempt. If a concurrent `stopScreenShare` + new share starts while an older `setScreenShareTrack` is pending, the older catch will stop the newer active track and call `setScreenShareTrack(null)` on it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| try { | ||
| await controls.setScreenShareTrack(null); | ||
| } catch (resetError) { | ||
| console.error('Media Call - Error clearing failed screen share track', resetError); |
There was a problem hiding this comment.
P2: Race condition in catch cleanup: screenShareTrackRef.current (shared mutable ref) is read in the catch block instead of the specific track from the failing attempt. If a concurrent stopScreenShare + new share starts while an older setScreenShareTrack is pending, the older catch will stop the newer active track and call setScreenShareTrack(null) on it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui-voip/src/providers/MediaCallViewProvider.tsx, line 246:
<comment>Race condition in catch cleanup: `screenShareTrackRef.current` (shared mutable ref) is read in the catch block instead of the specific track from the failing attempt. If a concurrent `stopScreenShare` + new share starts while an older `setScreenShareTrack` is pending, the older catch will stop the newer active track and call `setScreenShareTrack(null)` on it.</comment>
<file context>
@@ -232,6 +236,16 @@ const MediaCallViewProvider = ({ children }: MediaCallViewProviderProps) => {
+ try {
+ await controls.setScreenShareTrack(null);
+ } catch (resetError) {
+ console.error('Media Call - Error clearing failed screen share track', resetError);
+ }
+ }
</file context>
e702a9d to
0a28e48
Compare
1 participant
This PR allows a client to share their screen even when they do not have an available microphone or camera.
Previously, the media call flow depended on obtaining an audio input track before the client could fully participate in the WebRTC session. Because of that, a user without mic/camera could receive another user's audio/video or screen share, but could not start sharing their own screen.
This change makes screen sharing independent from the local audio input track by:
Issue(s)
close #1745
Related to the issue where users without camera/microphone cannot share their screen during a media call.
Steps to test or reproduce
Further comments
The fix keeps audio input and screen sharing as separate media concerns. A missing audio input track should no longer prevent the client from negotiating or sending a screen share video track.
I also removed the behavior that ended calls with
input-errorwhen no local audio input was available, since that prevented screen-share-only participation.Summary by CodeRabbit