ix the black camera bubble in Clips desktop - #3369
Conversation
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
PR #3369 adds a WebRTC playback watchdog for the Clips camera bubble, delays declaring WebRTC active until decoded frames exist, falls back to the established canvas pump when rendering stalls, and enriches native recording append diagnostics with track/timestamp context. The overall direction is sound: it addresses the reported WebKit autoplay/rendering failure without removing the proven fallback, uses a bounded retry loop, and adds focused predicate tests. The native logging also preserves realtime behavior by continuing to skip backpressured samples.
Risk assessment: Standard (browser media/state behavior plus native recording diagnostics).
Key Findings
- 🟡 MEDIUM: A late
playingevent can switch the bubble back to WebRTC after the popover has selected the canvas fallback, hiding the working fallback. - 🟡 MEDIUM: A queued
ontrackcallback from a torn-down peer can overwrite the current receiver’s video and watchdog state during renegotiation. - 🟢 LOW: Append bookkeeping is committed before
appendSampleBuffersucceeds, so failure diagnostics count the rejected sample as successfully appended.
🧪 Browser testing: Will run after this review (PR touches UI code)
| @@ -272,17 +399,16 @@ export function Bubble() { | |||
| const incomingStream = ev.streams[0]; | |||
| if (!incomingStream) return; | |||
| videoEl.srcObject = incomingStream; | |||
There was a problem hiding this comment.
🟡 Stale WebRTC ontrack can revive a torn-down peer
The ontrack callback only checks the component-level stopped flag. During renegotiation, teardownPeer() can be followed by a new peer while an already-queued callback from the old peer still runs; it then replaces video.srcObject and updates trackArrivedAtRef with the stale stream. Gate the callback with the current peer/handshake identity before mutating playback state.
Additional Info
Found by 2 of 3 review agents.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental Code Review Summary
This incremental review examined the latest PR head and the added playback-path guard. The previously reported late-WebRTC-claim issue is fixed: fallbackRequestedRef now prevents a delayed playing event from reclaiming the surface after the canvas pump is requested, and a fresh track explicitly re-arms the watchdog. The stale-peer ontrack race remains present and is intentionally not reposted because its existing review comment is still open.
No genuinely new actionable issues were identified in the latest changes. The append-statistics accuracy concern from the prior review remains in the diff, but it was already identified previously and is not reposted here.
Risk assessment: Standard (desktop media state/fallback behavior and native recording diagnostics). Added predicate coverage is appropriate and targeted validation reported by reviewers passed for the TypeScript/Vitest portions.
🧪 Browser testing: Will run after this review (PR touches UI code)

The camera bubble now fixes itself. On some Macs the bubble opened as a black circle with a white play triangle, and only started showing the camera after you clicked it. It came back every time the bubble reopened. The bubble now keeps trying to start the video on its own, and if that keeps failing it switches to the older frame-streaming path, which always works. Either way the user sees their camera without clicking anything.
The bubble no longer claims success when nothing is on screen. It used to report that the camera was live as soon as the connection was made, even when no video was actually playing. That also hid the backup path that would have saved it. It now only reports live video once real frames are on screen, and asks for the backup path when they never arrive.
Better logging for a failed recording. A recording in the same bug report died halfway through and could not be saved. The error told us nothing useful, so we could not find the cause. Recording failures now say which audio or video track broke, at what point in the recording, and whether its timing had drifted. Nothing about recording behaviour changed, only what we can see when it goes wrong.