feat: add audio sharing toggle for video call screen share#3317
feat: add audio sharing toggle for video call screen share#3317AnatharEridan wants to merge 2 commits into
Conversation
Made-with: Cursor
|
|
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (2)
WalkthroughThis change extends screen-sharing IPC payloads to include an optional Changes
Sequence DiagramsequenceDiagram
participant Dialog as screenSharePicker.tsx
participant Main as ScreenSharingRequestTracker\n(Main)
participant Relay as videoCallWindow/ipc.ts\n(Main)
participant Preload as preload/index.ts
participant Bridge as preload/jitsiBridge.ts
participant Jitsi as Jitsi Bridge
Dialog->>Main: send selection { sourceId, shareAudio }
Main->>Main: normalize payload (legacy or object)\nextract sourceId & shareAudio
Main->>Relay: forward payload (string|null or object)
Relay->>Preload: deliver payload to preload
Preload->>Preload: discriminate payload\nextract sourceId & shareAudio
Preload->>Bridge: invoke handler with sourceId, shareAudio
Bridge->>Jitsi: success(sourceId, sourceType, shareAudio)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screenSharing/screenSharePicker.tsx (1)
124-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
shareAudioon each picker open to avoid accidental audio sharing.Line 124-127 resets response flags but keeps prior
shareAudiostate. If it was checked previously, the next share can unexpectedly include system audio.Suggested fix
useEffect(() => { if (visible) { responseSentRef.current = false; wasVisibleRef.current = true; + setShareAudio(false); } else if (wasVisibleRef.current) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screenSharing/screenSharePicker.tsx` around lines 124 - 127, When the picker becomes visible reset the audio-sharing toggle so previous state doesn't leak into a new share: inside the visible branch that currently sets responseSentRef.current = false and wasVisibleRef.current = true, also reset the shareAudio state (e.g., call setShareAudio(false) or its equivalent) so shareAudio is cleared on each open; update the block handling visible (and any related init code that runs when visible becomes true) to explicitly set shareAudio to the default off value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/screenSharing/ScreenSharingRequestTracker.ts`:
- Line 129: The single-line conditional call to cb with a ternary is
misformatted; update the call in ScreenSharingRequestTracker.ts (the cb
invocation using shareAudio and selectedSource) to use a multiline expression so
it satisfies prettier/eslint: call cb( ... ) with the conditional split across
lines and the returned objects each on their own lines (i.e., place the opening
parenthesis on the cb line, then on separate indented lines put the conditional
start, the true object ({ video: selectedSource, audio: 'loopback' } as any) on
its own line, the colon, the false object ({ video: selectedSource }) on its own
line, and close the parenthesis) to match project prettier rules.
In `@src/videoCallWindow/ipc.ts`:
- Around line 303-306: The ipcMain.once callback signature in the ipcMain.once
call is split across multiple lines causing prettier/eslint errors; rewrite the
arrow function parameter list and opening so it is inline (preserve the types)
e.g. change the multiline "(_event, payload: string | null |
ScreenSharingSelectionPayload) => {" form to a single-line signature for the
ipcMain.once callback (keep the same parameter names _event and payload and the
ScreenSharingSelectionPayload union type) so eslint/prettier no longer flags the
formatting.
In `@src/videoCallWindow/preload/index.ts`:
- Around line 18-21: The IPC listener's parameter list is split across multiple
lines causing a Prettier lint error; collapse the parameters into a single
inline parameter list so the arrow function reads like (_event, payload: string
| null | ScreenSharingSelectionPayload) => { ... } — locate the anonymous
listener using the parameter names _event and payload (and the type
ScreenSharingSelectionPayload) in src/videoCallWindow/preload/index.ts and
reformat the parameter line to be on one line to satisfy Prettier.
In `@src/videoCallWindow/preload/jitsiBridge.ts`:
- Around line 411-414: The IPC listener signature in jitsiBridge.ts for the
anonymous callback (_event, payload: string | null |
ScreenSharingSelectionPayload) is split across lines and violates prettier;
reformat the function parameter list to conform to eslint/prettier (e.g.
collapse the parameters and their type annotation onto a single line or adjust
wrapping to match project prettier rules) so the arrow function signature is
syntactically identical but properly formatted within the listener registration
in the jitsiBridge.ts file.
---
Outside diff comments:
In `@src/screenSharing/screenSharePicker.tsx`:
- Around line 124-127: When the picker becomes visible reset the audio-sharing
toggle so previous state doesn't leak into a new share: inside the visible
branch that currently sets responseSentRef.current = false and
wasVisibleRef.current = true, also reset the shareAudio state (e.g., call
setShareAudio(false) or its equivalent) so shareAudio is cleared on each open;
update the block handling visible (and any related init code that runs when
visible becomes true) to explicitly set shareAudio to the default off value.
🪄 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: 677891bc-81df-4649-bb18-bf27b6de239c
📒 Files selected for processing (6)
src/ipc/channels.tssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/screenSharePicker.tsxsrc/videoCallWindow/ipc.tssrc/videoCallWindow/preload/index.tssrc/videoCallWindow/preload/jitsiBridge.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ipc/channels.tssrc/videoCallWindow/preload/jitsiBridge.tssrc/videoCallWindow/ipc.tssrc/videoCallWindow/preload/index.tssrc/screenSharing/screenSharePicker.tsxsrc/screenSharing/ScreenSharingRequestTracker.ts
🪛 ESLint
src/videoCallWindow/preload/jitsiBridge.ts
[error] 411-414: Replace ⏎··········_event,⏎··········payload:·string·|·null·|·ScreenSharingSelectionPayload⏎········ with _event,·payload:·string·|·null·|·ScreenSharingSelectionPayload
(prettier/prettier)
src/videoCallWindow/ipc.ts
[error] 303-306: Replace ⏎········_event,⏎········payload:·string·|·null·|·ScreenSharingSelectionPayload⏎······ with _event,·payload:·string·|·null·|·ScreenSharingSelectionPayload
(prettier/prettier)
src/videoCallWindow/preload/index.ts
[error] 18-21: Replace ⏎··········_event,⏎··········payload:·string·|·null·|·ScreenSharingSelectionPayload⏎········ with _event,·payload:·string·|·null·|·ScreenSharingSelectionPayload
(prettier/prettier)
src/screenSharing/ScreenSharingRequestTracker.ts
[error] 129-129: Replace shareAudio·?·({·video:·selectedSource,·audio:·'loopback'·}·as·any)·:·{·video:·selectedSource·} with ⏎··········shareAudio⏎············?·({·video:·selectedSource,·audio:·'loopback'·}·as·any)⏎············:·{·video:·selectedSource·}⏎········
(prettier/prettier)
🔇 Additional comments (6)
src/videoCallWindow/ipc.ts (1)
307-311: Nice backward-compatible relay handling.Forwarding the union payload unchanged here keeps compatibility with both legacy (
string | null) and structured responses.src/screenSharing/screenSharePicker.tsx (1)
414-423: Good accessible toggle implementation.The checkbox/label pairing with
id+htmlForis clean and keeps the new option keyboard/screen-reader friendly.src/ipc/channels.ts (1)
37-45: Channel typing update looks correct.This keeps the old payload shape working while formally supporting
{ sourceId, shareAudio }.src/videoCallWindow/preload/index.ts (1)
22-27: Good union payload handling in preload.The object-vs-primitive branching is straightforward and keeps existing callers stable.
src/videoCallWindow/preload/jitsiBridge.ts (1)
417-424: Share-audio mapping tosuccessCbis implemented well.Defaulting
shareAudiotofalsefor legacy payloads and forwarding it only when present is correct.Also applies to: 436-436
src/screenSharing/ScreenSharingRequestTracker.ts (1)
99-106: Good backward-compatible payload parsing and audio gating.The object/legacy normalization plus conditional
audio: 'loopback'inclusion matches the intended behavior cleanly.Also applies to: 129-129
Made-with: Cursor
|
Hi @jeanfbrito could you please take a look |
2 participants
Summary
This PR adds an explicit
Share system audiooption to the internal video call screen picker.When enabled, the selected screen-sharing source is returned with loopback audio so audio can be included in the shared stream.
Changes
Share system audiocheckbox to the internal screen picker UI.shareAudio.audio: 'loopback'in display media callback when audio sharing is enabled.Test plan
yarn build)Share system audiois visibleSummary by CodeRabbit