[bjowe]

Adds in-call participant video: publish the local camera via WHIP and consume remote video via WHEP (H264/VP8 over SMB), with self-preview, per-tile video grid, source pinning, return-feed and fullscreen controls, plus the video-enabled toggle in production setup.

Closes #692

Companion PR (backend): Eyevinn/intercom-manager#315

[birme]

Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES

Reviewed the full +4752/−261 diff across 69 files with the branch checked out. Feature-scoped work that mirrors the existing audio patterns well, with good coverage of the new pure logic. Two concrete media-lifecycle bugs should be fixed before shipping a camera feature:

High

1. Camera capture is never stopped on call exit (privacy/hardware leak). production-line.tsx:149 destructures only const [inputVideoStream] = useVideoInput({...}), discarding the third tuple element — the reset() that stops the camera tracks. useVideoInput.reset (use-video-input.ts:67-72) is the only place videoInput.getTracks().forEach(t => t.stop()) runs, and it's never called. The effect cleanup (:62-64) only sets aborted = true; REMOVE_CALL (global-state-reducer.ts:123) doesn't stop mediaStreamVideoInput; and rtcPeerConnection.close() doesn't stop local getUserMedia tracks. Net: camera stays live + recording indicator on after leaving a video line, until the tab closes. Capture and call the video reset on exit/unmount.
2. Frame-monitor setIntervals leak on unmount. attachShowWhenReady (video-element-factory.ts:470-490) starts a 500ms interval per remote video element, cleared only by stopFrameMonitor, which is called only from removeThisElement on track ended/removetrack. useVideoElements cleanup (use-video-elements.ts:11-24) nulls srcObject but doesn't call stopFrameMonitor, nor does the useRtcConnection teardown. On abrupt teardown/navigation the interval + rVFC loop keep running against detached elements. Call stopFrameMonitor for every element in cleanUpVideo.

Medium

3. checkbox.tsx:76-87 — input made readOnly + tabIndex={-1} with toggle delegated to wrapper onClick, synthesizing a fake ChangeEvent via as unknown as. Removes keyboard focus/space-to-toggle from a shared primitive (a11y regression + scope creep). Prefer a native disabled prop.
4. PC held in useState (never recreated) but stream refs are in the effect deps — a stream-ref change would close() the PC and the re-run no-ops behind signalingState === "closed" guards, dead-ending the call. Latent today (no device-switch re-acquire); add a guard/comment.
5. No test asserting camera tracks stop / frame monitors clear on exit — exactly where the leaks live.

Nits

• Raw console.warn/error in prod paths vs the project logger util; video-element-factory.ts builds ~540 lines of tile chrome via imperative createElement+inline styles vs Emotion styled() used elsewhere.

Notes: no user-input URL construction (all via backend API_URL), no new bundle secrets, reducer ERROR change well-contained, CI green. Both High items are small localized fixes.

[birme]

This PR looks like part of a multi-repo/multi-PR dependency stack together with Eyevinn/intercom-manager#315 (backend video support). Holding off on automated review/merge here — this needs a human (or the relevant implementation agent) to assess the whole stack together, not a per-PR pass.

[bjowe]

#692 covers both High items — I'd written the same two fixes independently and landed on the same
shape, so nothing further from me there.

Two things still open:

main has a broken lockfile. #650 bumped react to ^19.2.7 but left react-dom at
^19.2.3, which resolves to 19.2.4, and React 19 refuses to run when the two differ — 15 of 24
test files fail on a pristine main. This branch is green because it predates the bump, but
inherits the breakage as soon as the conflict below is resolved. Opened separately as #NNN.

This PR currently conflicts with main, in calls-page.tsx against #693. When resolving:
#693's !isGuest guard belongs on the add-call JoinProduction, but the new VideoLineJoinCard
block should stay unguarded — pendingVideoRefs comes from the same validated-refs path as
pendingCallRefs, so a guest arriving on a share link still needs to join the line they were
invited to. Worth a second opinion if that reading is wrong.

Unrelated, noticed while in there: useAudioInput has the same shape the camera bug had —
resetAudioInput is only wired to the device-switch paths, never to call exit, so the mic track
outlives the call. Pre-existing on main, not this PR.

[birme]

Code Review — video support (WHIP/WHEP, H264) — Needs Changes

Reviewed by daily-backlog-pr (Phase 3). A large, well-structured video feature that correctly routes all state through the reducer (reusing UPDATE_CALL/extending DEVICES_UPDATED, no ad-hoc action types) and all REST calls through the API object — no blocking architectural or WebRTC violations. But several substantial new modules ship with no tests, there is a camera-track resource leak, and a block of pin-styling code is dead. Per project rules this cluster warrants Needs Changes. CI (build/lint/prettier/tests/e2e) is green — these are correctness/coverage findings, not formatting.

Blocking

• None.

Warnings

• No tests for the largest new modules. video-element-factory.ts (543 lines: tile factory, draggable PiP, fullscreen, rVFC stall monitor) has no test file; use-video-source-pin.ts (retry/backoff-on-HTTP-425, latest-wins ref) has no renderHook test; pin-data-channel.ts sendPinnedEndpoint (data-channel message shape — a correctness-sensitive SMB contract) is untested; use-video-tile-grid.ts and use-shadow-rtc-connection.ts (a full second WHIP/WHEP session lifecycle: peer connection, heartbeat, teardown) are untested. These pure/factory/hook units are the highest-value test targets.
• Camera-track leak on input change — use-video-input.ts:45-65. When videoInputId changes, the effect cleanup only sets aborted = true and never stops the previously acquired stream's tracks; setVideoInput(stream) replaces the reference. Switching cameras mid-call leaves the old camera active (indicator light stays on) and the old track detached but unstopped. Cleanup should stop the prior stream's tracks (only the explicit reset() on exit does today).
• Dead pin-styling code — video-element-factory.ts:394-403. updateVideoTileContainerPinned queries [data-tile-options-btn], but that attribute is never set on any element created in createVideoTileContainer, so the function always early-returns and container.dataset.pinned is never set. Pin reordering still works via tile.style.order, so the effect is cosmetic — but this code cannot run as written.
• Duplicated sibling-line polling — production-line.tsx:626-644. Reimplements participant polling as a raw window.setInterval(... API.fetchProductionLine ...) every 2s with .catch(() => {}), duplicating use-line-polling.ts and silently swallowing all errors.

Suggestions

• Use the project logger (logger.red/logger.yellow) instead of console.warn/console.error (use-rtc-connection.ts:84, use-video-input.ts:52).
• use-video-input.ts:54-59 — the ERROR dispatch omits callId, so a per-camera failure shows a global banner rather than a call-scoped one.
• production-line.tsx:620,631,655 — inline import("./types.ts").TParticipant/TLine refs; use the existing named import.
• ice-gathering.ts waitForIceGathering changed from reject-on-timeout to resolve-on-timeout (5s→8s). This is a shared function, so it also alters the audio-only flow (a timeout now proceeds to PATCH with partial candidates instead of tearing down). Defensible robustness improvement, but validate against SMB in a real environment.

Cross-repo coupling (hard dependency on intercom-manager #315)

Non-functional in isolation: requires #315's new routes (PATCH .../whep-source, PATCH /session/:id/video-source), the new hasVideo (required)/isWhepReceiver/videoEnabled/whepSourceSessionId response fields (the auto-pin filter p.hasVideo && !p.isWhepReceiver silently no-ops if hasVideo is absent), the HTTP 425 not-ready contract (retry only triggers on 425), and SMB forwarding H264 + honoring PinnedEndpointsChanged. Do not merge ahead of #315 — treat as an atomic pair.

Moving the board item back to Ready for these changes.

[birme]

Code Review

Verdict: Needs Changes

Summary: A large, generally well-structured frontend video feature. The core architectural invariants hold: all REST goes through src/api/api.ts, global state flows through globalReducer/UPDATE_CALL, no inline fetch, no jsdom, no yarn, no SDP string-munging or index-based media access, no hardcoded secrets. There are no hard Blocking violations. However, there is a cluster of Warnings — most notably a large set of new pure functions and custom hooks shipped with no tests, an accessibility/typing regression in checkbox.tsx, and a module-level mutable singleton — and per the review rules 3+ Warnings with no Blocking still means Needs Changes. Several places also lean on a backend contract that #315 must honor (see Domain Note).

Blocking

None.

Warnings

• src/components/production-line/pin-data-channel.ts:2 — New pure function sendPinnedEndpoint (data-channel PinnedEndpointsChanged serialization, a WHIP/WHEP-domain primitive) ships with no unit test. Pure functions are the highest-value test target in this codebase and this one encodes an SMB message contract. Add a unit test.
• src/components/production-line/use-video-source-pin.ts, use-video-tile-grid.ts, use-shadow-rtc-connection.ts, src/hooks/use-video-device-labels.ts — Four new custom hooks, none with renderHook tests, including two large/complex ones (use-video-tile-grid.ts 241 lines of imperative DOM reconciliation; use-shadow-rtc-connection.ts 185 lines duplicating RTC negotiation). use-video-input.ts and use-video-elements.ts were correctly tested — the rest should match that bar.
• src/components/production-line/video-element-factory.ts:1 — 550-line factory (frame-stall monitor, fullscreen, PiP, pin overlay) with no test, while the sibling audio-element-factory.ts has a test. At minimum the pure/observable exports should be covered.
• src/components/checkbox/checkbox.tsx:70-73 — toggle() fabricates a synthetic event with as unknown as React.ChangeEvent<HTMLInputElement> (type cast without a justifying comment, and fragile — it only works because consumers read e.target.checked). Compounding this, the real <input> is readOnly with tabIndex={-1} (line 82-83) and the toggle is wired to the wrapping div's onClick, so the control is not keyboard-focusable and has no role/key handler — an accessibility regression versus a native checkbox.
• src/hooks/use-video-device-labels.ts:5 — warmedUpInSession is a module-level mutable boolean only reset inside the .catch. After one successful warm-up the effect never re-runs, so the video device list will not refresh when a camera is plugged in/out mid-session, and the module-level state makes the hook stateful across unmounts/remounts and hard to test deterministically. Prefer a ref or state keyed to the hook/provider.
• src/components/production-line/use-shadow-rtc-connection.ts:68-69 — parseInt(productionId, 10) / parseInt(lineId, 10) are forwarded to API.offerAudioSession with no isNaN guard. This is the exact parseInt-without-NaN-check pattern the project flags; a malformed id silently forwards NaN to the backend.
• Silent error swallowing: src/components/production-line/use-rtc-connection.ts:182 (videoElement.play().catch(() => {}) in the unmute handler) and src/components/production-line/use-video-source-pin.ts:41 (.catch(() => {}) on setLineWhepSource). These hide failures with no logging. At least route through logger so pin/playback failures are diagnosable.
• src/components/production-line/types.ts:32 / src/api/api.ts:50 — TParticipant.hasVideo is typed as required boolean, but it crosses an untyped fetch→JSON boundary and is a new field that only backend #315 provides. If #315 omits it on any path, every p.hasVideo is undefined (falsy), so computeVideoTileLabels and the stale-tile logic silently drop/mislabel tiles with no type error. Consider typing it optional until #315 is confirmed to always return it (note isWhepReceiver? right above is already optional — the two are inconsistent).

Suggestions

• src/components/production-line/use-rtc-connection.ts:328 — callId added to the noStreamError effect deps but not read in the body; drop it.
• src/components/production-line/ice-gathering.ts:30 — Behavior changed from reject-on-timeout to resolve-on-timeout (5s→8s), now also affecting the audio path. Reasonable (half-trickle), but add a one-line comment that resolve-on-timeout is intentional.
• src/components/production-line/use-rtc-connection.ts:49-61 — applyVideoSenderParameters is fire-and-forget; a short comment noting the intentional non-await would help.
• src/components/production-line/video-options-modal.tsx — Named *-modal.tsx but renders a custom dropdown, not the unified Modal. It's genuinely a popover, so the rule doesn't strictly apply — consider renaming to *-dialogue.tsx.
• Pinning has three parallel mechanisms (setLineWhepSource, setSessionVideoSource with 425-retry, and the PinnedEndpointsChanged data-channel message). A short architecture comment tying them together would reduce divergence risk.

Domain Note

This change is squarely in the WHIP/WHEP session-lifecycle, video routing, and data-channel-pinning domain. It assumes several backend contracts from the companion PR Eyevinn/intercom-manager#315 that cannot be validated from the frontend alone:

• New response fields: TParticipant.hasVideo / isWhepReceiver, TLine.videoEnabled / whepSourceSessionId.
• New endpoints: PATCH production/{id}/line/{id}/whep-source, PATCH session/{id}/video-source, plus videoEnabled on POST .../line.
• use-video-source-pin.ts retries only on HTTP 425 Too Early — if #315 signals "WHEP source not ready" with any other status, the retry loop is dead code.
• The SMB data-channel message shape { type: "PinnedEndpointsChanged", pinnedEndpoints: [...] }.

Recommend consulting the intercom-expert agent to validate the pinning/WHEP-source lifecycle and the 425 retry handshake against #315 before merge.

---

Automated review by the daily-backlog-pr pipeline (code-reviewer). This PR is the frontend half of a cross-repo video-support stack with intercom-manager#315 — see the note on that PR about coordinated merge.

[birme]

Update from today's daily-backlog-pr run: the backend companion Eyevinn/intercom-manager#315 ("feat: add video support for intercom lines (WHIP/WHEP, H264)") has since merged. The reason this PR was held (an open cross-repo dependency stack needing joint assessment) has therefore partly cleared — the backend half it depends on is now in main.

This run is still not auto-reviewing/merging this one: it's a large, human-authored video-support feature, and a maintainer should confirm the frontend half matches the merged backend contract (and whether the still-draft intercom-manager#407 WHIP/WHEP SMB-neighbour-group work is a further prerequisite) before it lands. Over to a human to reassess now that #315 is merged.

[birme]

Code Review

Verdict: LGTM

Summary: Re-review of the current head after the author's follow-up commits ("address review feedback" 2026-10-20 and "stale video tile bug fix" 2026-10-22). All concerns from the three prior Needs-Changes rounds are resolved.

Blocking

None.

Warnings

None.

Notes

• Missing tests from the prior reviews now all exist: pin-data-channel.test.ts, use-video-source-pin.test.ts, use-video-tile-grid.test.ts + .lifecycle.test.ts, use-shadow-rtc-connection.test.ts (stubs RTCPeerConnection, asserts srcObject), use-video-device-labels.test.ts, plus use-video-input/use-video-elements/video-element-factory/match-video-tile-labels/pinnable-participants/fullscreen-overlay-utils/ice-gathering. No jsdom env override, no waitForTimeout.
• checkbox.tsx a11y/typing regression fixed (real <label> wrapper, :focus-visible/:disabled, !!checked, eslint-disable removed).
• No module-level mutable singleton in the new code. REST goes through src/api/api.ts + handleFetchRequest (setLineWhepSource, setSessionVideoSource). No new dependencies, yarn.lock untouched. use-shadow-rtc-connection.ts passes backend SDP through without string munging or media[n] indexing and guards a missing answer SDP; parseInt uses radix 10.

Backend companion Eyevinn/intercom-manager#315 is already merged; CI is green (lint, prettier, builds, tests, e2e).

Optional (non-blocking) suggestions: a short lifecycle-invariant comment atop use-video-tile-grid.ts, and shouldForwardProp/$disabled for the disabled prop on the styled.label in checkbox.tsx.