fix: allow screen sharing without media devices - #41682
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 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughThe PR allows media calls to continue without an input audio track. It adds screen-sharing controls and session state for local and remote streams. The development script now builds selected packages before starting Meteor. ChangesVoIP media flow
Development setup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant Processor
participant RTCPeerConnection
Session->>Processor: createAnswer without input audio
Processor->>RTCPeerConnection: add audio transceiver
Processor->>RTCPeerConnection: set audio direction
sequenceDiagram
participant MediaCallViewContext
participant MediaCallViewProvider
participant CallControls
MediaCallViewContext->>MediaCallViewProvider: onToggleScreenShare()
MediaCallViewProvider->>CallControls: toggleScreenSharing()
CallControls-->>MediaCallViewProvider: Promise<void>
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🤖 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 `@packages/media-signaling/src/lib/services/webrtc/Processor.ts`:
- Around line 489-495: The issue is that getDesiredAudioDirection() returns
recvonly when this.inputTrack is null, and then updateRemoteHeld() incorrectly
infers a remote hold condition from the absence of send capability in the
direction, marking the call as remotely held even when the peer is actively
sending audio. Fix this by updating the remote hold inference logic in
updateRemoteHeld() to skip the direction-based inference when this.inputTrack is
null, or by introducing an independent remote hold signal that does not rely on
the presence or absence of local send capability. The outcome must preserve the
distinction between a mic-less local state and an actual remote hold condition.
In `@packages/ui-voip/src/providers/MediaCallViewProvider.tsx`:
- Around line 114-117: Update the device-acquisition catch block in
MediaCallViewProvider to invoke startCall(true) after logging the error, using
the micless fallback so calls can proceed when microphone access fails.
🪄 Autofix
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 Plus
Run ID: eee7ac96-36d7-45c1-9a95-97aefefd5f74
⛔ 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 (9)
package.jsonpackages/i18n/src/locales/en.i18n.jsonpackages/media-signaling/src/definition/call/IClientMediaCall.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/useMediaSession.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (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/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/useMediaSession.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsx
🧠 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/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/useMediaSession.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/lib/services/webrtc/Processor.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/ui-voip/src/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/useMediaSession.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/lib/services/webrtc/Processor.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/context/MediaCallViewContext.tspackages/media-signaling/src/lib/Session.tspackages/ui-voip/src/providers/useMediaSession.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/lib/services/webrtc/Processor.tspackages/ui-voip/src/providers/MediaCallViewProvider.tsx
📚 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/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/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/providers/MediaCallViewProvider.tsx
🔇 Additional comments (11)
package.json (1)
27-27: LGTM!packages/i18n/src/locales/en.i18n.json (1)
7451-7452: LGTM!packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
110-111: LGTM!packages/media-signaling/src/lib/Session.ts (1)
564-565: LGTM!Also applies to: 567-569
packages/media-signaling/src/lib/services/webrtc/Processor.ts (2)
339-343: LGTM!Also applies to: 354-355, 576-577
473-474: 🎯 Functional CorrectnessNo change needed for late microphone attachment.
MediaStreamWrapper.setTrack()callssyncTrackChange(), which usesRTCRtpSender.replaceTrack()orRTCPeerConnection.addTrack()and registers the transceiver; this path renegotiates direction changes without callingupdateAudioDirectionWithoutNegotiation()directly.> Likely an incorrect or invalid review comment.packages/ui-voip/src/context/definitions.d.ts (1)
32-33: LGTM!packages/ui-voip/src/context/MediaCallViewContext.ts (1)
22-22: LGTM!Also applies to: 46-47, 60-60
packages/ui-voip/src/providers/MediaCallViewProvider.tsx (1)
131-132: LGTM!Also applies to: 204-206, 253-253
packages/ui-voip/src/providers/useMediaSession.ts (2)
20-21: LGTM!Also applies to: 160-161
176-180: 🎯 Functional CorrectnessNo change needed for inactive screen-share streams.
After stop,
screenShareRemote.stop()removes and clears the video track wrapper, sogetMediaStream('screen-share')?.hasVideo()returns false for stopped streams.> 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 1 file (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/media-signaling/src/lib/services/webrtc/Processor.ts">
<violation number="1" location="packages/media-signaling/src/lib/services/webrtc/Processor.ts:643">
P3: This guard correctly prevents micless calls from being falsely inferred as remote-held, but it also disables remote-held detection entirely once the local input track is absent. Because inputTrack can be cleared mid-call via setInputTrack(null), a call that was correctly marked as remote-held while the mic was present will keep _remoteHeld (exposed as remoteHeld in Call.ts) frozen at true even after the remote resumes, since updateRemoteHeld now returns before re-evaluating the audio transceivers. If stale held state is a concern, consider only skipping when there is genuinely nothing to infer, or explicitly resetting held state when the track is removed.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return; | ||
| } | ||
|
|
||
| if (!this.inputTrack) { |
There was a problem hiding this comment.
P3: This guard correctly prevents micless calls from being falsely inferred as remote-held, but it also disables remote-held detection entirely once the local input track is absent. Because inputTrack can be cleared mid-call via setInputTrack(null), a call that was correctly marked as remote-held while the mic was present will keep _remoteHeld (exposed as remoteHeld in Call.ts) frozen at true even after the remote resumes, since updateRemoteHeld now returns before re-evaluating the audio transceivers. If stale held state is a concern, consider only skipping when there is genuinely nothing to infer, or explicitly resetting held state when the track is removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/media-signaling/src/lib/services/webrtc/Processor.ts, line 643:
<comment>This guard correctly prevents micless calls from being falsely inferred as remote-held, but it also disables remote-held detection entirely once the local input track is absent. Because inputTrack can be cleared mid-call via setInputTrack(null), a call that was correctly marked as remote-held while the mic was present will keep _remoteHeld (exposed as remoteHeld in Call.ts) frozen at true even after the remote resumes, since updateRemoteHeld now returns before re-evaluating the audio transceivers. If stale held state is a concern, consider only skipping when there is genuinely nothing to infer, or explicitly resetting held state when the track is removed.</comment>
<file context>
@@ -640,6 +640,10 @@ export class MediaCallWebRTCProcessor implements IWebRTCProcessor {
return;
}
+ if (!this.inputTrack) {
+ return;
+ }
</file context>
|
Hello @Akanshagore! I'm sorry to inform you, this issue was already fixed here #41416, so I'll be closing this PR. But, I have a few tips for next time, if you're intersted:
This is just an overview, the last 3 items I included just FYI. That's why it's important to open PR's as draft first, and only converting them to ready for review when they're actually ready. Those items would make you PR invalid for merge. I greatly appreciate the effort though, thanks for contributing and making the platform better! 🚀 |
3 participants
Proposed changes (including videos or screenshots)
This PR allows users to share their screen even when a microphone or camera is not available.
Previously, screen sharing depended on having a local audio input track available. Users without media devices could receive calls and view shared screens, but they could not start their own screen sharing.
This change:
Issue(s)
Fixes #1745
Steps to test or reproduce
Use two clients:
Start a media call from Client A to Client B.
Accept the call from Client B.
From Client B, start screen sharing.
Verify that Client A can see Client B's shared screen.
Stop screen sharing and verify that the call continues normally.
Further comments
This implementation keeps audio input and screen sharing as separate media concerns. Missing microphone/camera devices should no longer prevent users from participating in calls or sharing their screen.
Summary by CodeRabbit
New Features
Bug Fixes