fix: cast screen-share stop preserves camera/mic, auto-focuses the share, and ignores native-stop#613
fix: cast screen-share stop preserves camera/mic, auto-focuses the share, and ignores native-stop#613cyaiox wants to merge 2 commits into
Conversation
…are, and ignores native-stop
- stopScreenShare now uses the symmetric setScreenShareEnabled(false) so the
{ audio: true } ScreenShareAudio track is torn down together with the video
and the camera/microphone publications are left untouched
- ParticipantGrid auto-focuses the first non-presentation screen share; a later
participant sharing no longer steals focus from an already-spotlighted tile
- a browser-native "Stop sharing" ends the track (fires `ended`) and is treated
as an intentional stop, so the "screen share failed" toast only surfaces on a
genuine unexpected death (publication dropped without the media track ending)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for CI Build 27837837411Coverage increased (+5.3%) to 93.584%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions5 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #613
PR: #613 — fix: cast screen-share stop preserves camera/mic, auto-focuses the share, and ignores native-stop
Branch: fix/cast-screen-share-native-stop → master
Files changed: 4 (+329 −29)
CI: ✅ All checks passing (lint, test, audit, Vercel preview)
Git conventions (ADR-6): ✅ PR title follows fix: <summary>, branch follows fix/<summary>
Security: ✅ No issues found — no secrets, no XSS vectors (React escapes all participant identity/name rendering), no unsafe DOM manipulation, spotlight logic cannot be externally manipulated without valid LiveKit track permissions.
Consumer impact: None — all changes are internal hooks and components with no public API surface modifications.
Core changes — all look correct
1. Symmetric teardown (useScreenShare.ts) — Replacing unpublishTrack with setScreenShareEnabled(false) is the right fix. The previous path only removed the screen-share video track, orphaning the ScreenShareAudio companion. setScreenShareEnabled(false) is the proper inverse of the setScreenShareEnabled(true, { audio: true }) used to start, and LiveKit handles stopping both tracks and their MediaStreamTracks while leaving camera/mic untouched.
2. Native stop detection (useScreenShare.ts:66-72) — The ended event listener on the MediaStreamTrack is a clean way to distinguish a browser-native stop (which fires ended) from a genuine unexpected death (publication dropped without ended). The { once: true } option prevents listener accumulation. The comment block is excellent — it explains the why, not just the what.
3. Auto-focus screen share (ParticipantGrid.tsx:63-75) — The spotlight latch via autoExpandedRef is well-designed: presentation bots keep priority (presentationTrack ?? screenShareTrack), a second participant sharing never steals focus, and the latch re-arms only when no spotlight source remains. The conditional clear on line 73 correctly preserves a manual user selection.
4. Test coverage — Thorough regression tests for both hooks: stop-never-touches-camera/mic, native-stop suppresses toast, ParticipantGrid first-share focus / no-steal / presentation priority / focus-release / render branches. The styled-component mock factory in ParticipantGrid.spec.tsx (converting $ transient props to data-* attributes) is a pragmatic pattern for testing layout state without the CSS-in-JS runtime.
Findings Summary
- P0 Critical: 0
- P1 Major: 0
- P2 Minor: 4
P2 — Minor (non-blocking)
-
[Style] Variable
tshadowing inParticipantGrid.tsx—.find(t => ...)and.some(t => ...)on lines 64-65, 73 shadow the outertfromuseCastTranslation(). This is a pre-existing pattern in the file, but the PR adds another instance (line 65). If someone later adds at('key')call inside one of those lambdas, they'd get a runtime error. Consider renaming the lambda parameter totrortrackin a follow-up. -
[Robustness] No explicit cleanup of the
endedlistener on unmount (useScreenShare.ts:66-72) — If the hook's component unmounts while screen-sharing, the listener persists on theMediaStreamTrack. Mitigated by{ once: true }and eventual GC when the track is stopped by LiveKit's room disconnect. Very low risk, but storing a ref and callingremoveEventListenerin a cleanup effect would be more defensive. -
[Robustness] Rapid double-start of
startScreenSharecould attach duplicateendedlisteners to successiveMediaStreamTrackinstances. In practice, LiveKit'ssetScreenShareEnabled(true)while already enabled is idempotent, making this edge case unlikely. A guard (if (hasLocalScreenShare) return) at the top ofstartScreenSharewould eliminate it. -
[Style]
ParticipantTileuses an inline anonymous type for its props (line 222-227). Extracting aParticipantTilePropsinterface would be consistent withParticipantGridPropsalready being in a.typesfile.
Verdict: ✅ APPROVE
Clean, well-motivated fix with excellent test coverage. The symmetric teardown correctly resolves the camera/mic disruption bug, the native-stop detection avoids false error toasts, and the auto-focus logic is carefully designed. All P2 items are suggestions for future hardening — none are blocking.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack
| const presentationTrack = finalTracks.find(t => isPresentationBot(t.participant)) | ||
| if (presentationTrack && !autoExpandedRef.current) { | ||
| setExpandedTrackSid(presentationTrack.participant.sid + presentationTrack.source) | ||
| const screenShareTrack = finalTracks.find(t => t.source === Track.Source.ScreenShare && !isPresentationBot(t.participant)) |
There was a problem hiding this comment.
[P2 — Style] The lambda parameter t shadows the outer t from useCastTranslation(). This is a pre-existing pattern in the file (line 64, 73, 123-124 all use t), but it could cause a subtle bug if someone later needs to call the translation function inside one of these callbacks. Consider renaming to tr or track in a follow-up cleanup.
| intentionalStopRef.current = true | ||
| }, | ||
| { once: true } | ||
| ) |
There was a problem hiding this comment.
[P2 — Robustness] The { once: true } option handles the common case well, but if the component unmounts while sharing (before ended fires), this listener stays attached. Since it only writes to a ref, no state update occurs — it's benign. For full rigor, you could store the track in a ref and call removeEventListener in a cleanup function returned from an effect, but this is low-priority given the risk level.
Make the ParticipantGrid spotlight a true ranking (presentation > screen share > cameras) rather than first-engaged-wins: - a presentation that starts while a screen share is already spotlighted now preempts it (detected via the hadPresentationRef appearance edge) - when that presentation ends, the latch re-arms so a still-active screen share reclaims the spotlight in the same render pass Unchanged: a second participant's share never steals focus from an already spotlighted share, manual tile selection is respected, and the spotlight releases when no presentation or share remains.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #613
PR: #613 — fix: cast screen-share stop preserves camera/mic, auto-focuses the share, and ignores native-stop
Branch: fix/cast-screen-share-native-stop → master
Files changed: 4 (+364 −28)
CI: ✅ All checks passing (lint, test, audit, Vercel preview deployed)
Coverage: ✅ Patch coverage 100% (16/16 lines). Overall coverage increased +5.3% to 93.6%.
Git conventions (ADR-6): ✅ PR title fix: <summary>, branch fix/<summary>
Core Changes — All Correct
1. Symmetric teardown (useScreenShare.ts:91-95) — Replacing the conditional unpublishTrack / setScreenShareEnabled(false) path with a single setScreenShareEnabled(false) is the correct fix. This is the proper inverse of setScreenShareEnabled(true, { audio: true }) used at start. LiveKit's implementation removes both the screen-share video and the ScreenShareAudio companion track while leaving camera/mic publications untouched. The previous unpublishTrack path only removed the video track, orphaning the audio companion — which is the root cause of the camera/mic disruption bug.
2. Native stop detection (useScreenShare.ts:66-78) — Clean approach. The ended event on MediaStreamTrack fires when the browser's native "Stop sharing" bar is used, but does NOT fire on a genuine anomaly (publication dropped server-side). The { once: true } option prevents listener accumulation. The comment block is excellent — it explains the distinction between native stop and unexpected death.
3. Auto-focus spotlight (ParticipantGrid.tsx:63-86) — The two-ref state machine correctly implements the priority system. I traced all five scenarios:
- Screen share starts → spotlighted ✓
- Second participant shares → no focus steal ✓
- Presentation starts while share is spotlighted → preempts ✓
- Presentation ends, share still active → share reclaims spotlight (latch re-arms) ✓
- All spotlight sources end → collapses back to grid ✓
4. Test coverage — Thorough. ParticipantGrid.spec.tsx covers empty state, local-only filtering, single/multi participant, auto-focus, no-steal-focus, presentation priority, presentation preemption, focus-release, overflow cards, avatar fallback, initializing state, speaking/muted indicators, and local mirror. The styled-component mock factory converting $-prefixed transient props to data-* attributes is a pragmatic testing pattern. useScreenShare.spec.ts correctly asserts that setCameraEnabled/setMicrophoneEnabled are never called during stop.
Security Review
- Secrets & credentials: No hardcoded secrets ✅
- XSS: All participant name/identity rendering goes through React's escaping.
getDisplayNameoutput is rendered as React children, not dangerously set. ✅ - Sensitive data exposure: Sentry captures only track stats (width, height, frameRate, durationMs, connectionState) — no PII. ✅
- Event listener safety:
{ once: true }prevents accumulation; the listener only flips a boolean ref. ✅ - Dependencies: No new dependencies added. ✅
- No security issues found.
Consumer Impact
All changes are internal hooks and components (useScreenShare, ParticipantGrid). No public API surface modified, no exported types changed. No downstream consumer impact.
Findings Summary
- P0 Critical: 0
- P1 Major: 0
- P2 Minor: 3
P2 — Minor (non-blocking)
-
[Style] Variable
tshadowing —ParticipantGrid.tsx:72,84:.find(t => ...)and.some(t => ...)shadow the outertfromuseCastTranslation(). Pre-existing pattern, but the PR extends it (line 72). If someone later addst('key')inside one of those lambdas, they'd reference the track instead of the translation function. Consider renaming totrortrackin a follow-up. -
[Robustness] No explicit cleanup of
endedlistener on unmount —useScreenShare.ts:72-78: If the component unmounts while screen-sharing, the listener persists until the track is GC'd. Mitigated by{ once: true }and LiveKit's disconnect stopping the track. Very low risk, but storing a ref and callingremoveEventListenerin a cleanup return would be more defensive. -
[Style] Inline anonymous type for
ParticipantTileprops —ParticipantGrid.tsx:222-227: TheParticipantTilefunction uses inline prop types whileParticipantGridPropsis already extracted to a.typesfile. ExtractingParticipantTilePropswould be consistent.
Verdict: ✅ APPROVE
Clean, well-motivated fix with excellent test coverage. The symmetric teardown correctly resolves the camera/mic disruption, the native-stop detection avoids false error toasts, and the spotlight logic handles all priority/preemption scenarios correctly. All P2 items are future-hardening suggestions — none blocking.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack
| const screenShareTrack = finalTracks.find(t => t.source === Track.Source.ScreenShare && !isPresentationBot(t.participant)) | ||
| const focusTrack = presentationTrack ?? screenShareTrack | ||
|
|
||
| if (presentationTrack && !hadPresentationRef.current) { |
There was a problem hiding this comment.
[P2 · Style] The lambda parameter t here shadows the outer t from useCastTranslation(). Same on line 84 (.some(t => ...)). Pre-existing pattern, but if someone later adds a t('key') call inside one of these lambdas it would reference the track. Consider renaming to tr or track in a follow-up.
| intentionalStopRef.current = true | ||
| }, | ||
| { once: true } | ||
| ) |
There was a problem hiding this comment.
[P2 · Robustness] The ended listener has no explicit cleanup on unmount. If the hook's component unmounts while sharing, the listener persists until the track is GC'd. Mitigated by { once: true } and LiveKit's disconnect stopping the track. Very low risk — storing a ref and removeEventListener in a cleanup effect would be the more defensive option.
3 participants
Summary
Fixes a bug where stopping a screen share in cast could disrupt the streamer's own camera/mic, plus two related UX improvements.
stopScreenSharenow usessetScreenShareEnabled(false)(the inverse of thesetScreenShareEnabled(true, { audio: true })used to start). LiveKit removes both the screen-share video and theScreenShareAudiotrack and leaves the camera/microphone publications untouched. The previousunpublishTrackpath only removed the video, orphaning the screen-audio track.ParticipantGridspotlights the first non-presentation screen share. A later participant sharing does not steal focus from an already-spotlighted tile (presentation bots keep priority).ended; that's now treated as an intentional stop, so the "screen share failed / Retry" toast only surfaces on a genuine unexpected death (the publication dropping without the media track ending).Test plan
npm test— full suite green (2201 passing)setCameraEnabled/setMicrophoneEnabled; a native-stopendedevent suppresses the toast;ParticipantGridfirst-share focus, no-steal-focus, presentation priority, focus-release, and render branches (ParticipantGrid.tsxat 97.91% stmts / 100% funcs)npm run lint/lint:pkg/prettier --checkclean