Support arbitrary* media sources, and improve default device selection #561
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWidespread API changes make many props accept Signals via a new Signal.from; publish pipeline switches from media/device to source-based APIs (Device/Camera/Microphone/Screen); audio/video encoding and catalog wiring updated to use sources; various cleanups and two CI workflows removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant HangPublish
participant DeviceMgr as DeviceManager
participant Source as Source.Camera/Screen/Microphone
participant Broadcast
participant PubAudio as Publish.Audio
participant PubVideo as Publish.Video
User->>HangPublish: set source("camera"|"screen"|undefined)
HangPublish->>DeviceMgr: select device (if applicable)
DeviceMgr-->>HangPublish: selected MediaDeviceInfo
HangPublish->>Source: instantiate Camera/Microphone/Screen
note over Source: getUserMedia / getDisplayMedia → tracks
Source-->>HangPublish: stream { video?, audio? }
HangPublish->>Broadcast: set video.source / audio.source (Signals)
Broadcast->>PubAudio: audio.source (reactive)
Broadcast->>PubVideo: video.source (reactive)
par Audio path
PubAudio->>PubAudio: read enabled + source
PubAudio->>PubAudio: encoder configured from source.getSettings()
PubAudio->>Broadcast: publish audio groups
and Video path
PubVideo->>PubVideo: process frames via VideoTrackProcessor(source)
PubVideo->>Broadcast: publish video groups
end
sequenceDiagram
autonumber
participant App
participant Component
participant Signals as Signal/Effect
App->>Component: prop (value or Signal<T>)
Component->>Signals: Signal.from(prop)
Signals-->>Component: Signal<T> (identity if already Signal)
Component->>Signals: Effect.event(...) usage
Component->>Signals: Effect.reload() (reschedule)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
js/hang/src/publish/audio/captions.ts (1)
10-15: Remove or wire thetranscribeprop
transcribeis declared inCaptionsPropsbut never read in the implementation. Either:
- Drop the
transcribe?: booleanline from js/hang/src/publish/audio/captions.ts (and update the demo in js/hang-demo/src/publish.html)- Or add a runtime check (e.g.
if (!props.transcribe) return;) where transcription is triggered.js/hang/src/connection.ts (1)
83-96: Based on inspection of the Moq library’s Connection implementations,closed()is defined only in the rootjs/moq/src/connection.tsand wraps the underlying WebTransport.closed promise, which always resolves on normal close (never rejects) and only rejects on internal promise errors (not on normal disconnect). As such, clean closures currently bypass your retry logic. Keeping the existing catch-only retry path is insufficient.Trigger retry on clean closes
After thePromise.race, detect whenconnection.closed()wins without error and schedule the backoff retry in the same block as the error path, without retrying on effect cancellation:- await Promise.race([cancel, connection.closed()]); + const canceled = await Promise.race([ + cancel.then(() => true), + connection.closed().then(() => false), + ]); + if (!canceled && this.reload) { + this.status.set("disconnected"); + const tick = this.#tick.peek() + 1; + effect.timer(() => this.#tick.set(prev => Math.max(prev, tick)), this.#delay); + this.#delay = Math.min(this.#delay * 2, this.maxDelay); + }This ensures clean closes re-trigger the effect while still skipping retry when the effect is canceled.
js/hang/src/publish/audio/index.ts (4)
117-121: Avoid passing undefined sampleRate into AudioContextOnly set sampleRate if present to prevent odd UA behavior.
-const context = new AudioContext({ - latencyHint: "interactive", - sampleRate: settings.sampleRate, -}); +const contextOptions: AudioContextOptions = { latencyHint: "interactive" }; +if (settings.sampleRate) contextOptions.sampleRate = settings.sampleRate; +const context = new AudioContext(contextOptions);
148-156: Clear #worklet and #gain on teardown to prevent stale referencesWithout clearing, #runEncoder/#runGain may see a closed node.
effect.spawn(async () => { @@ effect.set(this.#worklet, worklet); @@ effect.set(this.#gain, gain); }); +// Ensure downstream effects stop when this effect tears down +effect.cleanup(() => { + worklet.port.onmessage = null as unknown as (ev: MessageEvent) => void; + effect.set(this.#gain, undefined); + effect.set(this.#worklet, undefined); +});
239-247: AudioDataInit includes no “transfer” field; this won’t type-checkAlso guard for empty/mismatched channel arrays.
-const frame = new AudioData({ +if (channels.length === 0 || channels.length !== settings.channelCount) { + // Mismatch between capture and configured channel count; drop frame. + return; +} +const frame = new AudioData({ format: "f32-planar", sampleRate: worklet.context.sampleRate, numberOfFrames: channels[0].length, numberOfChannels: channels.length, timestamp: (1_000_000 * data.timestamp) / worklet.context.sampleRate, data: joined, - transfer: [joined.buffer], });
218-228: Flush encoder before closing to avoid dropping final framesEffect cleanups are sync—spawn a flush.
-effect.cleanup(() => encoder.close()); +effect.cleanup(() => { + // Best-effort flush then close + Promise.resolve(encoder.flush()).catch(() => {}).finally(() => encoder.close()); +});js/hang/src/publish/chat/typing.ts (1)
29-40: Unset catalog on cleanup to avoid stale availabilityWhen disabled,
catalogshould go undefined so watchers don’t think the track is still present.this.#signals.effect((effect) => { const enabled = effect.get(this.enabled); if (!enabled) return; broadcast.insertTrack(this.#track.consume()); effect.cleanup(() => broadcast.removeTrack(this.#track.name)); this.catalog.set({ name: this.#track.name, priority: u8(this.#track.priority), }); + effect.cleanup(() => this.catalog.set(undefined)); });js/hang/src/publish/video/polyfill.ts (1)
27-33: Stop the stream when the track ends to avoid leaksCurrently the loop will keep enqueuing frames after the track stops. Close the stream on track end and guard in the pull loop.
- return new ReadableStream<VideoFrame>({ - async start() { + return new ReadableStream<VideoFrame>({ + async start(controller) { video = document.createElement("video") as HTMLVideoElement; video.srcObject = new MediaStream([track]); + track.addEventListener("ended", () => controller.close(), { once: true }); await Promise.all([ video.play(), new Promise((r) => { video.onloadedmetadata = r; }), ]); @@ - async pull(controller) { + async pull(controller) { while (true) { + // If the track is no longer live, close the stream. + if (track.readyState !== "live") { + controller.close(); + return; + } const now = performance.now(); if (now - last < 1000 / frameRate) { await new Promise((r) => requestAnimationFrame(r)); continue; }Also applies to: 49-61
js/hang/src/watch/video/detection.ts (1)
31-35: Enabled toggle doesn’t gate subscription; can leak CPU and stay subscribedSecond effect does not check
enabled, and the first effect never clears#catalog. Once enabled, detection may continue reading after disable. Gate the second effect onenabledand clear#catalogon cleanup.this.#signals.effect((effect) => { - if (!effect.get(this.enabled)) return; - this.#catalog.set(effect.get(catalog)?.detection); + if (!effect.get(this.enabled)) return; + this.#catalog.set(effect.get(catalog)?.detection); + effect.cleanup(() => this.#catalog.set(undefined)); }); @@ - this.#signals.effect((effect) => { + this.#signals.effect((effect) => { + if (!effect.get(this.enabled)) return; const catalog = effect.get(this.#catalog); if (!catalog) return; const broadcast = effect.get(this.broadcast); if (!broadcast) return;Also applies to: 36-57
js/hang/src/publish/chat/message.ts (1)
29-37: Catalog not cleared when disabled.After disabling, catalog retains the last value, advertising a non-existent track.
this.#signals.effect((effect) => { const enabled = effect.get(this.enabled); if (!enabled) return; broadcast.insertTrack(this.#track.consume()); effect.cleanup(() => broadcast.removeTrack(this.#track.name)); - this.catalog.set({ name: this.#track.name, priority: u8(this.#track.priority) }); + this.catalog.set({ name: this.#track.name, priority: u8(this.#track.priority) }); + effect.cleanup(() => this.catalog.set(undefined)); });js/hang/src/publish/video/detection.ts (1)
49-51: Catalog should clear on cleanup.Avoid exposing a stale detection catalog after disable.
this.#catalog.set({ track: { name: this.#track.name, priority: Catalog.u8(this.#track.priority) }, }); + effect.cleanup(() => this.#catalog.set(undefined));js/hang/src/watch/video/renderer.ts (1)
30-36: Scheduler doesn’t react to canvas/paused/enabled changes; can throw when canvas becomes undefined.
Because #schedule uses peek and isn’t subscribed to dependencies, rAF may remain scheduled after props change. If canvas is removed mid-frame, #render throws. Also, rAF keeps running even when source.enabled is false.Apply these diffs to make scheduling reactive and robust:
@@ this.#signals.effect((effect) => { const canvas = effect.get(this.canvas); this.#ctx.set(canvas?.getContext("2d") ?? undefined); }); - this.#signals.effect(this.#schedule.bind(this)); + // (Re)schedule on relevant changes. + this.#signals.effect((effect) => { + effect.get(this.canvas); + effect.get(this.paused); + effect.get(this.source.enabled); + this.#schedule(); + }); this.#signals.effect(this.#runEnabled.bind(this)); @@ #schedule() { const ctx = this.#ctx.peek(); const paused = this.paused.peek(); + const enabled = this.source.enabled.peek(); - if (ctx && !paused) { + if (ctx && !paused && enabled) { if (!this.#animate) { this.#animate = requestAnimationFrame(this.#render.bind(this)); } } else { if (this.#animate) { cancelAnimationFrame(this.#animate); this.#animate = undefined; } } } @@ #render() { // Schedule the next render. this.#animate = undefined; this.#schedule(); const ctx = this.#ctx.peek(); - if (!ctx) { - throw new Error("scheduled without a canvas"); - } + if (!ctx || !this.source.enabled.peek()) { + // Canvas detached or source disabled; nothing to draw. + return; + }Also applies to: 88-104, 105-113
js/hang/src/meet/room.ts (1)
108-111: Avoid tearing down locals when announcements stream ends or filter changes.
In finally you call this.close(), which also clears locals and fires callbacks. On name/filter changes this is surprising.Apply this diff to only clear remotes/active here and leave locals intact:
- } finally { - this.close(); - } + } finally { + // Only clear remotes/active; keep locals. + for (const [n, watch] of this.remotes) { + watch.close(); + this.#onRemote?.(n, undefined); + } + for (const n of this.active.keys()) { + this.#onActive?.(n, undefined); + } + this.remotes = new Map(); + this.active = new Map(); + }If you prefer, extract this into a private helper for readability.
js/hang/src/watch/video/index.ts (1)
100-101: Clear frames on disable/teardown of the decoding effect to prevent stale redraws.
When enabled flips to false, the last frame persists and the renderer can keep scheduling. Clear buffered frames in the effect cleanup.Apply:
- effect.cleanup(() => decoder.close()); + effect.cleanup(() => { + decoder.close(); + this.frame.set((prev) => { + prev?.close(); + return undefined; + }); + this.#next?.close(); + this.#next = undefined; + });js/hang/src/watch/broadcast.ts (1)
119-130: Avoid stale #broadcast handle on cleanup.
When the effect re-runs (name/enabled/active changes), #broadcast keeps pointing at the old (closed) consumer. Restore it to undefined on cleanup.Apply:
- const broadcast = conn.consume(name); - effect.cleanup(() => broadcast.close()); - effect.set(this.#broadcast, broadcast); + const broadcast = conn.consume(name); + effect.set(this.#broadcast, broadcast, undefined); + effect.cleanup(() => broadcast.close());js/hang/src/publish/video/index.ts (1)
171-196: Guard against missing frameRate and zero dimensions.
getSettings().frameRate can be undefined on some browsers; width/height could be 0 briefly. Prevent NaN/INF bitrate.static async #bestEncoderConfig(settings: VideoTrackSettings, frame: VideoFrame): Promise<VideoEncoderConfig> { - const width = frame.codedWidth; - const height = frame.codedHeight; - const framerate = settings.frameRate; + const width = Math.max(1, frame.codedWidth); + const height = Math.max(1, frame.codedHeight); + const framerate = settings.frameRate ?? 30; @@ - const framerateFactor = 30.0 + (framerate - 30) / 2; + const framerateFactor = 30.0 + (framerate - 30) / 2;
🧹 Nitpick comments (24)
js/moq-clock/src/main.ts (1)
3-4: Prefer ts-expect-error over ts-ignore (safer suppression).ts-expect-error will fail the build if the error disappears, keeping the suppression honest.
Apply:
-// @ts-ignore Deno import. +// @ts-expect-error External TypeScript can't resolve Deno's jsr: scheme.js/signals/src/index.ts (2)
27-33: Signal.from is a solid ergonomics win.Minor: instanceof can fail across duplicated bundles; if this ever happens, consider duck-typing (peek/subscribe) fallback.
398-406: Fix log message to match the renamed API (event vs eventListener).Prevents confusion during debugging.
Apply:
- console.warn("Effect.eventListener called when closed, ignoring"); + console.warn("Effect.event called when closed, ignoring");js/hang/src/publish/audio/index.ts (2)
104-110: Remove noisy console logs or gate behind a debug flagThese will spam in production and re-fire on every reactive tick.
- console.log("running audio source", enabled); + // debug: console.log("running audio source", enabled); @@ - console.log("running audio source", source); + // debug: console.log("running audio source", source);
18-22: Unused AudioConstraints typeIt appears unused after the source migration—remove to reduce surface area.
-export type AudioConstraints = Omit< - MediaTrackConstraints, - "aspectRatio" | "backgroundBlur" | "displaySurface" | "facingMode" | "frameRate" | "height" | "width" ->; +// (removed: constraints moved to Source-based API)js/hang/src/meet/element.ts (1)
112-114: Rename callback param to reflect new semantics (track, not media)Purely readability; reduces confusion post-migration.
-const cleanup = broadcast.video.source.subscribe((media) => { - video.srcObject = media ? new MediaStream([media]) : null; +const cleanup = broadcast.video.source.subscribe((track) => { + video.srcObject = track ? new MediaStream([track]) : null; });js/hang/src/watch/chat/typing.ts (1)
31-34: Optional: clear catalog on disable to avoid stale pointersHarmless either way; adds symmetry with active reset.
this.#signals.effect((effect) => { if (!effect.get(this.enabled)) return; this.#catalog.set(effect.get(catalog)?.chat?.typing); + effect.cleanup(() => this.#catalog.set(undefined)); });js/hang/src/watch/chat/message.ts (2)
62-64: Fix dequal-avoidance mismatchComment says to avoid the dequal check but code doesn’t. Set via function.
- // Use a function to avoid the dequal check. - this.#latest.set(frame); + // Use a function to avoid the dequal check. + this.#latest.set(() => frame);
33-36: Optional: clear cached catalog on disable to avoid stale readsPrevents consumers from seeing a non-existent track after disabling.
this.#signals.effect((effect) => { if (!effect.get(this.enabled)) return; this.#catalog.set(effect.get(catalog)?.chat?.message); + effect.cleanup(() => this.#catalog.set(undefined)); });js/hang/src/watch/audio/index.ts (2)
83-85: Comment contradicts code pathWe only create AudioContext when enabled; update comment.
- // NOTE: We still create an AudioContext even when muted. - // This way we can process the audio for visualizations. + // NOTE: We only create an AudioContext when enabled. + // If visualizations while muted are desired, move the gating to the decoder only.
153-155: Unify cancellation check (minor)Prefer strict undefined to match other modules and avoid future falsey pitfalls.
- if (!frame) break; + if (frame === undefined) break;js/hang/src/watch/audio/speaking.ts (1)
47-55: Harden read loop against subscription errors.Wrap the read loop to avoid unhandled rejections from sub.readBool() and ensure state resets on error.
- effect.spawn(async (cancel) => { - for (;;) { - const speaking = await Promise.race([sub.readBool(), cancel]); - if (speaking === undefined) break; - - this.#active.set(speaking); - } - }); + effect.spawn(async (cancel) => { + try { + for (;;) { + const speaking = await Promise.race([sub.readBool(), cancel]); + if (speaking === undefined) break; + this.#active.set(speaking); + } + } catch { + // Optional: surface via a logger if available. + } finally { + this.#active.set(undefined); + } + });js/hang/src/watch/audio/captions.ts (1)
47-55: Guard against read errors and ensure state reset.Mirror the speaking.ts suggestion to avoid leaking stale text and to prevent unhandled rejections.
- effect.spawn(async (cancel) => { - for (;;) { - const frame = await Promise.race([sub.readString(), cancel]); - if (frame === undefined) break; // don't treat "" as EOS - this.#text.set(frame); - } - }); - effect.cleanup(() => this.#text.set(undefined)); + effect.spawn(async (cancel) => { + try { + for (;;) { + const frame = await Promise.race([sub.readString(), cancel]); + if (frame === undefined) break; // don't treat "" as EOS + this.#text.set(frame); + } + } catch { + // Optional: log + } finally { + this.#text.set(undefined); + } + });js/hang/src/publish/video/detection.ts (2)
86-89: Avoid double-closing the track.Track is already closed via signals.cleanup; closing again here risks double-close errors if not idempotent.
close() { this.signals.close(); - this.#track.close(); }
61-81: Optional: cancel in-flight detect on disable.Consider checking an effect-bound cancellation flag before/after api.detect to minimize wasted work on rapid toggles.
js/hang/src/publish/broadcast.ts (1)
11-23: reload prop appears unused in publisher.
It’s defined in BroadcastProps but not referenced. Drop it or wire it; otherwise it’s confusing.js/hang/src/publish/source/camera.ts (2)
49-50: Use debug-level logging.
Console noise in hot paths; prefer console.debug.- console.log("requesting camera", device); + console.debug("requesting camera", device);
20-22: flip is unused here.
You expose flip but never read it. If video flipping is handled downstream, consider removing from Camera or wire it into constraints/metadata.Also applies to: 30-30
js/hang/src/publish/element.ts (3)
166-171: Avoid double-driving Screen.enabled.
You both bind screen.enabled via an effect (Lines 143-147) and set it here. Prefer the binding effect only to prevent oscillation.- if (this.#audio instanceof Source.Screen) { - // Enable the screenshare capture if either audio or video are enabled. - this.#audio.enabled.set(audio || !!this.#video?.enabled.peek()); - } else { + if (!(this.#audio instanceof Source.Screen)) { this.#audio?.enabled.set(audio); } @@ - if (this.#video instanceof Source.Screen) { - // Only disable the screenshare capture if both audio and video are disabled. - this.#video.enabled.set(video || !!this.#audio?.enabled.peek()); - } else { + if (!(this.#video instanceof Source.Screen)) { this.#video?.enabled.set(video); }Also applies to: 181-186
291-318: UI still labeled “Device:” while the API is source-based.
Optional rename to “Source:” for clarity.
323-349: Status logic: “Connected” branch is effectively unreachable.
The prior branches cover all audio/video combinations; consider removing or reordering if you intend to show “Connected” without media.js/hang/src/publish/source/microphone.ts (3)
39-45: Add a debug breadcrumb when requesting the micParity with camera source helps trace selection.
#runStream(effect: Effect): void { const enabled = effect.get(this.enabled); if (!enabled) return; const device = effect.get(this.device); if (!device) return; + console.debug("requesting microphone", device);
75-81: Add a permission-request debug log (parity with camera)This improves diagnosability around first-time permission prompts.
if (!devices.length) { - // Request permissions and try again. + // Request permissions and try again. + console.debug("requesting permissions");
109-116: Broaden default device heuristics for cross-platform labelsFallback to common mic labels improves default picks on Windows/macOS/mobile when alias/groupId is missing.
- if (!defaultDevice) { - // Look for default or communications device - defaultDevice = devices.find((d) => { - const label = d.label.toLowerCase(); - return label.includes("default") || label.includes("communications"); - }); - } + if (!defaultDevice) { + // Look for common built-ins or system-preferred devices + defaultDevice = devices.find((d) => { + const label = d.label.toLowerCase(); + return ( + label.includes("default") || + label.includes("communications") || + label.includes("built-in") || + label.includes("internal") || + label.includes("headset") || + label.includes("usb") || + label.includes("bluetooth") + ); + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (36)
js/hang/src/connection.ts(2 hunks)js/hang/src/meet/element.ts(1 hunks)js/hang/src/meet/room.ts(2 hunks)js/hang/src/preview/member.ts(2 hunks)js/hang/src/preview/room.ts(2 hunks)js/hang/src/publish/audio/captions.ts(3 hunks)js/hang/src/publish/audio/index.ts(8 hunks)js/hang/src/publish/audio/speaking.ts(3 hunks)js/hang/src/publish/broadcast.ts(5 hunks)js/hang/src/publish/chat/message.ts(2 hunks)js/hang/src/publish/chat/typing.ts(2 hunks)js/hang/src/publish/element.ts(10 hunks)js/hang/src/publish/index.ts(1 hunks)js/hang/src/publish/location.ts(3 hunks)js/hang/src/publish/preview.ts(2 hunks)js/hang/src/publish/source/camera.ts(1 hunks)js/hang/src/publish/source/index.ts(1 hunks)js/hang/src/publish/source/microphone.ts(1 hunks)js/hang/src/publish/source/screen.ts(1 hunks)js/hang/src/publish/video/detection.ts(3 hunks)js/hang/src/publish/video/index.ts(8 hunks)js/hang/src/publish/video/polyfill.ts(1 hunks)js/hang/src/watch/audio/captions.ts(2 hunks)js/hang/src/watch/audio/emitter.ts(2 hunks)js/hang/src/watch/audio/index.ts(2 hunks)js/hang/src/watch/audio/speaking.ts(2 hunks)js/hang/src/watch/broadcast.ts(2 hunks)js/hang/src/watch/chat/message.ts(2 hunks)js/hang/src/watch/chat/typing.ts(2 hunks)js/hang/src/watch/location.ts(3 hunks)js/hang/src/watch/preview.ts(2 hunks)js/hang/src/watch/video/detection.ts(2 hunks)js/hang/src/watch/video/index.ts(2 hunks)js/hang/src/watch/video/renderer.ts(2 hunks)js/moq-clock/src/main.ts(1 hunks)js/signals/src/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/meet/element.tsjs/hang/src/publish/source/index.tsjs/hang/src/connection.tsjs/hang/src/watch/location.tsjs/hang/src/watch/audio/captions.tsjs/hang/src/publish/index.tsjs/hang/src/watch/preview.tsjs/hang/src/publish/video/polyfill.tsjs/hang/src/publish/chat/message.tsjs/hang/src/publish/location.tsjs/hang/src/watch/chat/message.tsjs/hang/src/watch/audio/index.tsjs/hang/src/meet/room.tsjs/hang/src/watch/audio/emitter.tsjs/hang/src/watch/video/renderer.tsjs/hang/src/publish/preview.tsjs/hang/src/watch/video/detection.tsjs/hang/src/publish/source/screen.tsjs/hang/src/preview/member.tsjs/hang/src/preview/room.tsjs/hang/src/publish/source/camera.tsjs/hang/src/publish/chat/typing.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/watch/chat/typing.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/watch/broadcast.tsjs/hang/src/watch/video/index.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/watch/audio/speaking.tsjs/hang/src/publish/broadcast.tsjs/hang/src/publish/video/index.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/video/detection.tsjs/hang/src/publish/element.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/meet/element.tsjs/hang/src/publish/source/index.tsjs/hang/src/connection.tsjs/hang/src/watch/location.tsjs/hang/src/watch/audio/captions.tsjs/hang/src/publish/index.tsjs/moq-clock/src/main.tsjs/hang/src/watch/preview.tsjs/hang/src/publish/video/polyfill.tsjs/hang/src/publish/chat/message.tsjs/hang/src/publish/location.tsjs/hang/src/watch/chat/message.tsjs/hang/src/watch/audio/index.tsjs/hang/src/meet/room.tsjs/hang/src/watch/audio/emitter.tsjs/hang/src/watch/video/renderer.tsjs/hang/src/publish/preview.tsjs/hang/src/watch/video/detection.tsjs/hang/src/publish/source/screen.tsjs/hang/src/preview/member.tsjs/hang/src/preview/room.tsjs/hang/src/publish/source/camera.tsjs/signals/src/index.tsjs/hang/src/publish/chat/typing.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/watch/chat/typing.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/watch/broadcast.tsjs/hang/src/watch/video/index.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/watch/audio/speaking.tsjs/hang/src/publish/broadcast.tsjs/hang/src/publish/video/index.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/video/detection.tsjs/hang/src/publish/element.ts
🧬 Code graph analysis (31)
js/hang/src/connection.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/watch/location.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/watch/audio/captions.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/index.ts (2)
js/hang/src/publish/audio/index.ts (1)
Source(16-16)js/hang/src/publish/video/index.ts (1)
Source(13-13)
js/hang/src/watch/preview.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/video/polyfill.ts (1)
js/hang/src/publish/video/index.ts (1)
VideoStreamTrack(19-23)
js/hang/src/publish/chat/message.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/location.ts (2)
js/signals/src/index.ts (1)
Signal(19-83)js/hang/src/catalog/location.ts (1)
Position(42-42)
js/hang/src/watch/chat/message.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/watch/audio/index.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/meet/room.ts (2)
js/moq/src/path.ts (1)
Valid(31-31)js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/watch/audio/emitter.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/watch/video/renderer.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/preview.ts (2)
js/signals/src/index.ts (1)
Signal(19-83)js/hang/src/preview/info.ts (1)
Info(13-13)
js/hang/src/watch/video/detection.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/source/screen.ts (6)
js/signals/src/index.ts (3)
Signal(19-83)Effect(88-447)effect(303-313)js/hang/src/publish/video/index.ts (4)
VideoConstraints(37-43)VideoStreamTrack(19-23)effect(88-168)effect(334-369)js/hang/src/publish/audio/index.ts (6)
AudioConstraints(18-21)AudioStreamTrack(24-28)effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/hang/src/publish/source/camera.ts (3)
effect(42-72)effect(74-134)effect(136-154)js/hang/src/publish/source/microphone.ts (3)
effect(39-65)effect(67-125)effect(127-145)js/hang/src/publish/element.ts (4)
video(174-176)video(178-187)audio(159-161)audio(163-172)
js/hang/src/preview/member.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/preview/room.ts (2)
js/moq/src/path.ts (1)
Valid(31-31)js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/source/camera.ts (3)
js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)Effect(88-447)effect(303-313)js/hang/src/publish/video/index.ts (4)
VideoConstraints(37-43)VideoStreamTrack(19-23)effect(88-168)effect(334-369)js/hang/src/publish/source/microphone.ts (3)
effect(39-65)effect(67-125)effect(127-145)
js/hang/src/publish/chat/typing.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/audio/captions.ts (3)
js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/index.ts (5)
Audio(59-279)effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/hang/src/publish/audio/speaking.ts (1)
effect(31-111)
js/hang/src/watch/chat/typing.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/audio/speaking.ts (4)
js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/captions.ts (1)
effect(40-131)js/hang/src/publish/audio/index.ts (4)
effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/hang/src/publish/element.ts (2)
source(107-109)source(111-157)
js/hang/src/watch/broadcast.ts (2)
js/signals/src/index.ts (1)
Signal(19-83)js/moq/src/path.ts (1)
Valid(31-31)
js/hang/src/watch/video/index.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/source/microphone.ts (3)
js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)Effect(88-447)effect(303-313)js/hang/src/publish/audio/index.ts (6)
AudioConstraints(18-21)AudioStreamTrack(24-28)effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/hang/src/publish/source/camera.ts (3)
effect(42-72)effect(74-134)effect(136-154)
js/hang/src/watch/audio/speaking.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/broadcast.ts (8)
js/hang/src/watch/broadcast.ts (1)
BroadcastProps(12-29)js/signals/src/index.ts (1)
Signal(19-83)js/hang/src/publish/audio/index.ts (2)
AudioProps(45-57)Audio(59-279)js/hang/src/watch/audio/index.ts (2)
AudioProps(13-25)Audio(29-212)js/hang/src/watch/video/index.ts (2)
VideoProps(11-14)Video(17-145)js/hang/src/publish/video/index.ts (2)
VideoProps(45-50)Video(52-381)js/hang/src/publish/location.ts (2)
LocationProps(6-15)Location(17-77)js/hang/src/publish/preview.ts (1)
Preview(10-48)
js/hang/src/publish/video/index.ts (6)
js/hang/src/publish/audio/index.ts (5)
Source(16-16)effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)effect(303-313)Effect(88-447)js/hang/src/publish/source/camera.ts (3)
effect(42-72)effect(74-134)effect(136-154)js/hang/src/publish/source/microphone.ts (3)
effect(39-65)effect(67-125)effect(127-145)js/hang/src/publish/element.ts (2)
source(107-109)source(111-157)js/hang/src/publish/video/polyfill.ts (1)
VideoTrackProcessor(6-63)
js/hang/src/publish/video/detection.ts (1)
js/signals/src/index.ts (1)
Signal(19-83)
js/hang/src/publish/element.ts (7)
js/signals/src/index.ts (1)
Signal(19-83)js/hang/src/publish/audio/index.ts (5)
Source(16-16)effect(102-156)effect(158-171)effect(173-252)effect(254-272)js/hang/src/publish/video/index.ts (3)
Source(13-13)effect(88-168)effect(334-369)js/hang/src/publish/source/camera.ts (4)
Camera(11-159)effect(42-72)effect(74-134)effect(136-154)js/hang/src/publish/source/screen.ts (2)
Screen(11-76)effect(28-71)js/hang/src/publish/source/microphone.ts (4)
Microphone(10-150)effect(39-65)effect(67-125)effect(127-145)js/hang/src/meet/element.ts (4)
name(89-118)name(120-153)name(174-176)name(178-180)
🔇 Additional comments (66)
js/hang/src/publish/preview.ts (2)
6-8: Prop types expansion looks good.Allowing boolean | Signal and Info | Signal<Info | undefined> aligns with the new reactive API.
20-22: Correct use of Signal.from for normalization.This ensures callers can pass either raw values or Signals without extra wrappers.
js/hang/src/publish/audio/captions.ts (2)
35-36: Good: normalized enabled via Signal.from.Keeps the public surface flexible and consistent with other modules.
41-46: Switch to audio.source and local enabled gate looks correct.Correctly short-circuits when disabled or no track; MediaStreamAudioSourceNode creation from the track is right.
Also applies to: 90-93
js/signals/src/index.ts (2)
231-234: spawn now swallows rejections with a log — good.Prevents the scheduler from hanging on rejected promises while preserving diagnostics.
409-412: reload() helper is a nice addition.Explicit rescheduling without poking a signal is useful.
js/hang/src/connection.ts (2)
6-7: URL prop now accepts Signal — good.Matches the reactive props pattern used elsewhere.
39-44: Correct normalization with Signal.from.Keeps this.url reactive without forcing callers to wrap.
js/hang/src/preview/member.ts (2)
7-8: Prop now accepts Signal: LGTMThe widened type is consistent with the new signal-driven API.
19-20: Use Signal.from for enabled: LGTMThis preserves existing defaults and reuses provided signals when present.
js/hang/src/publish/audio/index.ts (2)
16-16: Source alias introduced: LGTMAlias keeps the public surface aligned with video’s Source.
86-95: Signal.from adoption and new source prop: LGTMConstructor wiring matches the new Source-based API.
js/hang/src/publish/index.ts (1)
6-6: Expose Source namespace: LGTMKeeps the new Camera/Microphone/Screen sources discoverable from publish/.
js/hang/src/watch/chat/typing.ts (1)
8-9: Signal.from for enabled: LGTMMatches the broader signalable-props pattern.
Also applies to: 28-29
js/hang/src/watch/chat/message.ts (2)
8-9: API: Prop now accepts Signal — looks goodBroadens inputs without breaking existing boolean callers.
30-31: Use Signal.from for normalization — goodKeeps the field a Signal while allowing reactive callers.
js/hang/src/publish/chat/typing.ts (2)
7-8: API: Prop now accepts Signal<boolean) — goodConsistent with the broader Signal.from migration.
26-27: Normalization via Signal.from — goodKeeps internal type stable.
js/hang/src/watch/audio/index.ts (2)
15-16: API: enabled accepts Signal — goodNon-breaking and enables reactive toggling.
62-63: Signal.from usage — goodKeeps internal contract as Signal.
js/hang/src/publish/video/polyfill.ts (1)
1-1: Type rename to VideoStreamTrack — looks correctMatches the new interface and keeps runtime unchanged.
Also applies to: 6-6
js/hang/src/watch/video/detection.ts (2)
9-10: API: enabled accepts Signal — goodKeeps call sites flexible without breaking booleans.
28-29: Normalization via Signal.from — goodConsistent with other modules.
js/hang/src/watch/audio/speaking.ts (2)
6-6: Prop typing widening matches Signal.from usage.Accepting boolean | Signal is consistent with the new reactive API.
28-28: Good normalization with Signal.from.This keeps external API flexible while ensuring an internal Signal.
js/hang/src/publish/chat/message.ts (2)
7-7: Prop typing widening is consistent.boolean | Signal aligns with the codebase-wide pattern.
26-26: Signal.from usage is appropriate.Normalization avoids boilerplate for callers passing Signals.
js/hang/src/publish/source/index.ts (1)
1-3:Sourcere-export verified. Top-leveljs/hang/src/publish/index.tsincludesexport * as Source from "./source".js/hang/src/watch/audio/captions.ts (2)
6-6: Prop typing widening matches new API.
28-28: Signal.from is the right call.js/hang/src/publish/video/detection.ts (1)
25-37: Minor: track cleanup placement is good.Track producer lifecycle is tied to the effect; insert/remove are balanced.
js/hang/src/watch/preview.ts (2)
8-9: API: enabled now accepts boolean | Signal — good widening with backward compatibility.
No behavior regressions spotted; callers can pass either a literal or a Signal.
24-24: Use Signal.from for normalization — correct.
Keeps external Signals unwrapped and preserves defaults.js/hang/src/meet/room.ts (1)
8-9: API: name now accepts Path.Valid | Signal — good normalization via Signal.from.
Constructor default remains Moq.Path.empty(); behavior matches previous semantics.Also applies to: 39-40
js/hang/src/watch/video/index.ts (1)
12-13: API: enabled now accepts boolean | Signal — good; constructor uses Signal.from correctly.
No gating regressions observed.Also applies to: 46-47
js/hang/src/preview/room.ts (2)
8-10: API: name/enabled now accept Signal — correct use of Signal.from with sensible defaults.
Looks consistent with the broader migration.Also applies to: 24-26
45-49: Confirm announced(name) supports undefined.
You pass possibly undefined name to conn.announced(name). If the API doesn’t accept undefined as “all”, guard accordingly.If needed:
- const name = effect.get(this.name); - const announced = conn.announced(name); + const name = effect.get(this.name); + const announced = conn.announced(name ?? undefined /* all broadcasts */);Or default to Moq.Path.empty() if that’s the intended “room root”.
js/hang/src/watch/location.ts (3)
7-7: LGTM! Signal-based prop normalization correctly implemented.The change from
enabled?: booleantoenabled?: boolean | Signal<boolean>aligns with the PR's migration to reactive inputs.
29-29: LGTM! Proper Signal.from initialization.The change from
new Signal(...)toSignal.from(...)correctly handles both plain boolean values and existing Signals, maintaining backward compatibility while enabling reactive inputs.
120-120: LGTM! Consistent Signal.from usage in LocationPeer.The LocationPeer constructor now properly normalizes the handle parameter using Signal.from, consistent with the Location class changes.
js/hang/src/publish/audio/speaking.ts (4)
10-10: LGTM! Reactive prop signature correctly updated.The SpeakingProps.enabled type change to
boolean | Signal<boolean>follows the established pattern across the codebase.
27-27: LGTM! Proper Signal normalization.Using
Signal.from()correctly handles both plain boolean values and existing Signal instances.
32-36: LGTM! Clean extraction of reactive values and source-based media handling.The code properly extracts the enabled state and migrates from
this.audio.mediatothis.audio.source, aligning with the PR's shift to source-driven media pipeline.
81-81: LGTM! MediaStream constructed from new source variable.The MediaStream construction now uses the
sourcevariable extracted fromthis.audio.source, completing the migration from media-based to source-based audio handling.js/hang/src/publish/location.ts (3)
8-14: LGTM! Comprehensive reactive prop interface.All three LocationProps fields now properly support both plain values and Signal types:
enabled:boolean | Signal<boolean>current:Catalog.Position | Signal<Catalog.Position | undefined>handle:string | Signal<string | undefined>This provides full reactive capability while maintaining backward compatibility.
36-38: LGTM! Consistent Signal.from initialization pattern.All three properties are properly initialized using
Signal.from(), ensuring correct handling of both plain values and existing Signal instances with appropriate defaults.
92-92: LGTM! LocationPeer handle initialization updated.The LocationPeer constructor now uses
Signal.from()for handle initialization, maintaining consistency with the Location class changes.js/hang/src/watch/audio/emitter.ts (2)
8-10: LGTM! Complete reactive prop support for AudioEmitter.All AudioEmitterProps now support both plain values and Signal types:
volume?: number | Signal<number>muted?: boolean | Signal<boolean>paused?: boolean | Signal<boolean>This enables full reactive control over audio playback parameters.
33-35: LGTM! Proper Signal.from initialization with logical defaults.All three properties are correctly initialized:
volume: defaults to 0.5muted: defaults to falsepaused: defaults to muted state when not explicitly setThe initialization pattern correctly uses
Signal.from()to handle both plain values and Signal inputs.js/hang/src/publish/source/screen.ts (5)
5-9: LGTM! Comprehensive reactive ScreenProps interface.The ScreenProps interface properly supports Signal inputs for all properties:
enabled:boolean | Signal<boolean>video:VideoConstraints | boolean | Signal<VideoConstraints | boolean | undefined>audio:AudioConstraints | boolean | Signal<AudioConstraints | boolean | undefined>This follows the established pattern across other Source classes.
21-23: LGTM! Proper Signal normalization with appropriate defaults.All properties are correctly initialized using
Signal.from():
enabled: defaults to false when not providedvideoandaudio: accept undefined when not providedThis maintains consistency with other Source implementations.
36-43: Flag browser compatibility concerns with CaptureController.The Chrome-only CaptureController usage should be documented or guarded more explicitly for cross-browser compatibility.
Consider adding a more explicit browser compatibility check or documentation about Chrome-only features:
// TODO Expose these to the application. - // @ts-expect-error Chrome only let controller: CaptureController | undefined; - // @ts-expect-error Chrome only - if (typeof self.CaptureController !== "undefined") { - // @ts-expect-error Chrome only + // Note: CaptureController is Chrome-only, gracefully degrade on other browsers + if (typeof globalThis.CaptureController !== "undefined") { controller = new CaptureController(); controller.setFocusBehavior("no-focus-change"); }
46-61: LGTM! Proper async media capture with cancellation.The screen capture implementation correctly:
- Uses
Promise.race()with cancel signal for proper cleanup- Handles getDisplayMedia failure gracefully
- Passes video/audio constraints from reactive signals
- Uses Chrome-specific options for enhanced UX
64-69: LGTM! Proper track extraction and cleanup.The implementation correctly:
- Extracts video and audio tracks with proper typing
- Registers cleanup handlers to stop tracks on effect cleanup
- Sets the stream signal with proper structure
js/hang/src/watch/broadcast.ts (3)
15-21: Prop types accept Signals — good alignment with the new reactive API.
The widened types for enabled/name/reload are consistent with the rest of hang and with Signal.from usage.
62-64: Constructor now uses Signal.from — good.
Prevents double-wrapping and keeps reactive props flexible.
109-116: Confirm announcement name filtering.
You ignore updates where update.name !== "". Is this suffix check intentional for your naming scheme? If announcements can legitimately carry non-empty suffixes, this will suppress state changes.js/hang/src/publish/broadcast.ts (1)
51-53: Constructor Signal.from usage — good.
Consistent with watch-side API and other publish modules.Also applies to: 59-59
js/hang/src/publish/source/camera.ts (1)
136-154: Default device selection flow — solid.
Validates selected device availability and falls back to a sensible default.js/hang/src/publish/video/index.ts (3)
13-23: Typed VideoStreamTrack alias and Source = VideoStreamTrack — good.
Matches polyfill signature and simplifies interop with Source.* modules.
46-50: Prop and state refactor to source-driven video — good.
Signal-based enabled/flip/source and a fixed #track align with the new architecture.Also applies to: 56-59, 63-64, 80-83
371-380: Closing #track — good.
Ensures producer resources are released.js/hang/src/publish/source/microphone.ts (4)
29-37: Solid effect wiring for device reactivityGood separation: devicechange listener, device enumeration, selection, and stream acquisition in distinct effects. Aligns with hang-layer media logic guideline.
10-23: API surface looks good; signals are exposed read-only where appropriate
devicesviaGetteravoids external mutation;enabled/device/constraintsaccept raw or Signal viaSignal.from. Nice.
147-149: Graceful teardown
close()delegates toEffect.close(); prior cleanups (track.stop, listeners) will run. LGTM.
46-50: Could you share lines 40–60 ofjs/hang/src/publish/source/microphone.tsso I can confirm the merge order ofdeviceIdvs. spreadconstraints?
| export interface AudioStreamTrack extends MediaStreamTrack { | ||
| kind: "audio"; | ||
| clone(): AudioTrack; | ||
| clone(): AudioStreamTrack; | ||
| getSettings(): AudioTrackSettings; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getSettings typing is too strong; sampleRate can be undefined in practice
Your code already guards sampleRate later; the interface should reflect that to avoid unsoundness.
Apply:
-export interface AudioStreamTrack extends MediaStreamTrack {
+export interface AudioStreamTrack extends MediaStreamTrack {
kind: "audio";
- clone(): AudioStreamTrack;
- getSettings(): AudioTrackSettings;
+ clone(): AudioStreamTrack;
+ getSettings(): AudioTrackSettings;
}
@@
-export interface AudioTrackSettings {
+export interface AudioTrackSettings {
deviceId: string;
groupId: string;
autoGainControl: boolean;
channelCount: number;
echoCancellation: boolean;
noiseSuppression: boolean;
- sampleRate: number;
+ sampleRate?: number;
sampleSize: number;
}Also applies to: 32-42
🤖 Prompt for AI Agents
In js/hang/src/publish/audio/index.ts around lines 24-28 (and similarly lines
32-42), the current AudioStreamTrack.getSettings typing requires sampleRate but
in practice sampleRate can be undefined; update the
AudioTrackSettings/getSettings return type so sampleRate is optional or typed as
number | undefined and propagate that change to the other similar
interfaces/occurrences noted (lines 32-42); then keep the runtime guards as-is
where code already checks for undefined.
| let group: Moq.GroupProducer = this.#track.appendGroup(); | ||
| effect.cleanup(() => group.close()); | ||
|
|
||
| let groupTimestamp = 0; | ||
|
|
||
| const encoder = new AudioEncoder({ | ||
| output: (frame) => { | ||
| if (frame.type !== "key") { | ||
| throw new Error("only key frames are supported"); | ||
| } | ||
|
|
||
| if (!this.#group || frame.timestamp - this.#groupTimestamp >= 1000 * this.maxLatency) { | ||
| this.#group?.close(); | ||
| this.#group = this.#track.appendGroup(); | ||
| this.#groupTimestamp = frame.timestamp; | ||
| if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { | ||
| group.close(); | ||
| group = this.#track.appendGroup(); | ||
| groupTimestamp = frame.timestamp; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
First-frame grouping can close an empty group if timestamps start > 0
Initialize from the first encoded frame to avoid an immediate rotate.
-let group: Moq.GroupProducer = this.#track.appendGroup();
+let group: Moq.GroupProducer = this.#track.appendGroup();
effect.cleanup(() => group.close());
-let groupTimestamp = 0;
+let groupTimestamp: number | undefined = undefined;
@@
- if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) {
+ if (groupTimestamp === undefined) {
+ groupTimestamp = frame.timestamp;
+ } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) {
group.close();
group = this.#track.appendGroup();
groupTimestamp = frame.timestamp;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let group: Moq.GroupProducer = this.#track.appendGroup(); | |
| effect.cleanup(() => group.close()); | |
| let groupTimestamp = 0; | |
| const encoder = new AudioEncoder({ | |
| output: (frame) => { | |
| if (frame.type !== "key") { | |
| throw new Error("only key frames are supported"); | |
| } | |
| if (!this.#group || frame.timestamp - this.#groupTimestamp >= 1000 * this.maxLatency) { | |
| this.#group?.close(); | |
| this.#group = this.#track.appendGroup(); | |
| this.#groupTimestamp = frame.timestamp; | |
| if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { | |
| group.close(); | |
| group = this.#track.appendGroup(); | |
| groupTimestamp = frame.timestamp; | |
| } | |
| let group: Moq.GroupProducer = this.#track.appendGroup(); | |
| effect.cleanup(() => group.close()); | |
| let groupTimestamp: number | undefined = undefined; | |
| const encoder = new AudioEncoder({ | |
| output: (frame) => { | |
| if (frame.type !== "key") { | |
| throw new Error("only key frames are supported"); | |
| } | |
| if (groupTimestamp === undefined) { | |
| // First encoded frame: set the baseline timestamp without rotating | |
| groupTimestamp = frame.timestamp; | |
| } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { | |
| // Latency exceeded: close and reopen the group | |
| group.close(); | |
| group = this.#track.appendGroup(); | |
| groupTimestamp = frame.timestamp; | |
| } | |
| // …rest of output handling… | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In js/hang/src/publish/audio/index.ts around lines 194 to 209, the grouping
logic can immediately close an empty group when the first encoded frame has
timestamp > 0; change initialization so groupTimestamp is set from the timestamp
of the first processed frame (or only perform the timestamp-based rotate after
the first frame has been appended), i.e., defer the initial groupTimestamp
assignment until the encoder output handler sees the first frame and then set
groupTimestamp = frame.timestamp before any rotate check, ensuring you do not
close a newly created but still-empty group.
| this.device = new Signal(props?.device); | ||
| this.user = Signal.from(props?.user); | ||
|
|
||
| this.#broadcast.insertTrack(this.#catalog.consume()); |
There was a problem hiding this comment.
Track insertion without symmetric removal in close().
You insert the catalog track once and never remove/close it when the publisher closes.
Apply:
this.#broadcast.insertTrack(this.#catalog.consume());And in close():
close() {
this.signals.close();
this.audio.close();
this.video.close();
this.location.close();
this.chat.close();
+ this.preview.close();
+ // Remove and close the catalog track.
+ this.#broadcast.removeTrack(this.#catalog.name);
+ this.#catalog.close();
+ // Optionally close the broadcast producer to release resources.
+ this.#broadcast.close();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.#broadcast.insertTrack(this.#catalog.consume()); | |
| close() { | |
| this.signals.close(); | |
| this.audio.close(); | |
| this.video.close(); | |
| this.location.close(); | |
| this.chat.close(); | |
| this.preview.close(); | |
| // Remove and close the catalog track. | |
| this.#broadcast.removeTrack(this.#catalog.name); | |
| this.#catalog.close(); | |
| // Optionally close the broadcast producer to release resources. | |
| this.#broadcast.close(); | |
| } |
🤖 Prompt for AI Agents
js/hang/src/publish/broadcast.ts around line 61, you insert a catalog track via
this.#broadcast.insertTrack(this.#catalog.consume()) but never remove or close
it on publisher shutdown; instead capture the consumed track in a field (e.g.
this.#publishedTrack = this.#catalog.consume()), pass that to insertTrack, and
in close() perform the symmetric cleanup: check for existence, call
this.#broadcast.removeTrack(this.#publishedTrack) (or the broadcast API’s
equivalent), and then close or release the track (e.g.
this.#publishedTrack.close() or this.#catalog.release(this.#publishedTrack))
with null/ready checks and idempotence so repeated close() calls are safe.
js/hang/src/publish/element.ts
Outdated
| this.#video?.close(); | ||
| this.#audio?.close(); | ||
|
|
There was a problem hiding this comment.
Clear broadcast sources when switching/clearing source.
When closing previous Source.* instances, also clear broadcast.video.source and broadcast.audio.source to avoid stale, stopped tracks keeping the publisher “enabled”.
this.#video?.close();
this.#audio?.close();
+ // Clear published sources immediately; effects below will repopulate.
+ this.broadcast.video.source.set(undefined);
+ this.broadcast.audio.source.set(undefined);
@@
this.#source.set(source);Also applies to: 156-156
🤖 Prompt for AI Agents
In js/hang/src/publish/element.ts around lines 114-116 (and also at line 156),
the code closes previous this.#video and this.#audio but does not clear
broadcast.video.source and broadcast.audio.source, which can leave stopped
tracks attached and keep the publisher "enabled"; after calling
this.#video?.close() and this.#audio?.close(), set broadcast.video.source = null
and broadcast.audio.source = null (guarding for existence of broadcast and its
video/audio subobjects) so any stale/closed tracks are removed; apply the same
change at the other location noted (line 156).
| video.signals.effect((effect) => { | ||
| const stream = effect.get(video.stream); | ||
| effect.set(this.broadcast.video.source, stream); | ||
| }); | ||
|
|
||
| const audio = new Source.Microphone({ enabled: this.broadcast.audio.enabled }); | ||
| audio.signals.effect((effect) => { | ||
| const stream = effect.get(audio.stream); | ||
| effect.set(this.broadcast.audio.source, stream); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Make Effect.set reversible to avoid stale values on cleanup.
Without a cleanup value, broadcast..source remains set after Source. is closed.
- video.signals.effect((effect) => {
+ video.signals.effect((effect) => {
const stream = effect.get(video.stream);
- effect.set(this.broadcast.video.source, stream);
+ effect.set(this.broadcast.video.source, stream, undefined);
});
@@
- audio.signals.effect((effect) => {
+ audio.signals.effect((effect) => {
const stream = effect.get(audio.stream);
- effect.set(this.broadcast.audio.source, stream);
+ effect.set(this.broadcast.audio.source, stream, undefined);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| video.signals.effect((effect) => { | |
| const stream = effect.get(video.stream); | |
| effect.set(this.broadcast.video.source, stream); | |
| }); | |
| const audio = new Source.Microphone({ enabled: this.broadcast.audio.enabled }); | |
| audio.signals.effect((effect) => { | |
| const stream = effect.get(audio.stream); | |
| effect.set(this.broadcast.audio.source, stream); | |
| }); | |
| video.signals.effect((effect) => { | |
| const stream = effect.get(video.stream); | |
| effect.set(this.broadcast.video.source, stream, undefined); | |
| }); | |
| const audio = new Source.Microphone({ enabled: this.broadcast.audio.enabled }); | |
| audio.signals.effect((effect) => { | |
| const stream = effect.get(audio.stream); | |
| effect.set(this.broadcast.audio.source, stream, undefined); | |
| }); |
🤖 Prompt for AI Agents
In js/hang/src/publish/element.ts around lines 119-129, the Effect.set calls for
video and audio currently assign the stream but do not provide a
cleanup/restoration value, leaving broadcast.video.source and
broadcast.audio.source stale after the Source is closed; update both effect.set
usages to make them reversible by passing a cleanup value or callback that
restores the previous value (or sets undefined) when the effect is
disposed/stream closed so the broadcast.*.source is cleared on cleanup.
| effect.spawn(async (cancel) => { | ||
| const stream = await Promise.race([ | ||
| navigator.mediaDevices.getUserMedia({ audio: finalConstraints }).catch(() => undefined), | ||
| cancel, | ||
| ]); | ||
| if (!stream) return; | ||
|
|
||
| const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined; | ||
| if (!track) return; | ||
|
|
||
| effect.cleanup(() => track.stop()); | ||
| effect.set(this.stream, track, undefined); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix hot-mic/resource leak when cancel wins getUserMedia race
getUserMedia cannot be aborted. If cancel resolves first, the eventual stream (when it resolves) is never stopped, briefly leaving the mic active. Stop late-resolving streams after cancel.
- effect.spawn(async (cancel) => {
- const stream = await Promise.race([
- navigator.mediaDevices.getUserMedia({ audio: finalConstraints }).catch(() => undefined),
- cancel,
- ]);
- if (!stream) return;
-
- const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined;
- if (!track) return;
-
- effect.cleanup(() => track.stop());
- effect.set(this.stream, track, undefined);
- });
+ effect.spawn(async (cancel) => {
+ const req = navigator.mediaDevices
+ .getUserMedia({ audio: finalConstraints })
+ .catch(() => undefined as MediaStream | undefined);
+ const CANCELLED = {};
+ const result = await Promise.race([req, cancel.then(() => CANCELLED)]);
+ if (result === CANCELLED) {
+ // Ensure any late-resolving stream is immediately stopped.
+ req.then((s) => s?.getTracks().forEach((t) => t.stop())).catch(() => {});
+ return;
+ }
+ const stream = result as MediaStream | undefined;
+ if (!stream) return;
+
+ const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined;
+ if (!track) return;
+
+ effect.cleanup(() => track.stop());
+ effect.set(this.stream, track, undefined);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| effect.spawn(async (cancel) => { | |
| const stream = await Promise.race([ | |
| navigator.mediaDevices.getUserMedia({ audio: finalConstraints }).catch(() => undefined), | |
| cancel, | |
| ]); | |
| if (!stream) return; | |
| const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined; | |
| if (!track) return; | |
| effect.cleanup(() => track.stop()); | |
| effect.set(this.stream, track, undefined); | |
| }); | |
| } | |
| effect.spawn(async (cancel) => { | |
| const req = navigator.mediaDevices | |
| .getUserMedia({ audio: finalConstraints }) | |
| .catch(() => undefined as MediaStream | undefined); | |
| const CANCELLED = {}; | |
| const result = await Promise.race([req, cancel.then(() => CANCELLED)]); | |
| if (result === CANCELLED) { | |
| // Ensure any late-resolving stream is immediately stopped. | |
| req.then((s) => s?.getTracks().forEach((t) => t.stop())).catch(() => {}); | |
| return; | |
| } | |
| const stream = result as MediaStream | undefined; | |
| if (!stream) return; | |
| const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined; | |
| if (!track) return; | |
| effect.cleanup(() => track.stop()); | |
| effect.set(this.stream, track, undefined); | |
| }); |
| #run(effect: Effect): void { | ||
| if (!effect.get(this.enabled)) return; | ||
| if (!effect.get(this.video.enabled)) return; | ||
| const enabled = effect.get(this.enabled); | ||
| if (!enabled) return; | ||
|
|
There was a problem hiding this comment.
Regression: removal of video.enabled gating can stall detection.
If video is disabled (no frames), process() returns early and no next timeout is scheduled; with no dependency on video.enabled, the effect won’t re-run, so detection may never resume when video re-enables.
Two-part fix:
- Gate on both detection and video enabled.
- Always reschedule when no frame is available.
- #run(effect: Effect): void {
- const enabled = effect.get(this.enabled);
- if (!enabled) return;
+ #run(effect: Effect): void {
+ const enabled = effect.get(this.enabled);
+ // Also depend on video.enabled so the effect re-runs when video toggles.
+ const videoEnabled = (this.video as any).enabled ? effect.get((this.video as any).enabled) : true;
+ if (!enabled || !videoEnabled) return;- const process = async () => {
- const frame = this.video.frame.peek();
- if (!frame) return;
+ const process = async () => {
+ const frame = this.video.frame.peek();
+ if (!frame) {
+ // Poll again later until frames are available.
+ timeout = setTimeout(process, this.#interval);
+ return;
+ }If Video.enabled is not a Signal in your current type, keep the reschedule change and omit the gating; otherwise, include both.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #run(effect: Effect): void { | |
| if (!effect.get(this.enabled)) return; | |
| if (!effect.get(this.video.enabled)) return; | |
| const enabled = effect.get(this.enabled); | |
| if (!enabled) return; | |
| #run(effect: Effect): void { | |
| const enabled = effect.get(this.enabled); | |
| // Also depend on video.enabled so the effect re-runs when video toggles. | |
| const videoEnabled = (this.video as any).enabled | |
| ? effect.get((this.video as any).enabled) | |
| : true; | |
| if (!enabled || !videoEnabled) return; | |
| let timeout: ReturnType<typeof setTimeout>; | |
| const process = async () => { | |
| const frame = this.video.frame.peek(); | |
| if (!frame) { | |
| // Poll again later until frames are available. | |
| timeout = setTimeout(process, this.#interval); | |
| return; | |
| } | |
| // …existing detection logic… | |
| timeout = setTimeout(process, this.#interval); | |
| }; | |
| process(); | |
| } |
🤖 Prompt for AI Agents
In js/hang/src/publish/video/detection.ts around lines 41 to 44, the effect
currently only gates on detection.enabled and returns early without scheduling
when video frames are absent; this can stall detection when video is later
re-enabled. Update the effect to check both detection.enabled and video.enabled
(if video.enabled is a Signal in your types) before returning, and ensure that
when no frame is available you still schedule the next timeout/reschedule call
so the effect will re-run; if video.enabled is not a Signal in your types, skip
the extra gate but still always reschedule when no frame is found.
| #runEncoder(effect: Effect): void { | ||
| if (!effect.get(this.enabled)) return; | ||
| const enabled = effect.get(this.enabled); | ||
| if (!enabled) return; | ||
|
|
||
| const media = effect.get(this.media); | ||
| if (!media) return; | ||
| const source = effect.get(this.source); | ||
| if (!source) return; | ||
|
|
||
| const track = effect.get(this.#track); | ||
| if (!track) return; | ||
| // Insert the track into the broadcast. | ||
| this.broadcast.insertTrack(this.#track.consume()); | ||
| effect.cleanup(() => this.broadcast.removeTrack(this.#track.name)); | ||
|
|
||
| const settings = media.getSettings() as VideoTrackSettings; | ||
| const processor = VideoTrackProcessor(media); | ||
| const settings = source.getSettings() as VideoTrackSettings; | ||
| const processor = VideoTrackProcessor(source); | ||
| const reader = processor.getReader(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure frame Signal is cleared on effect cleanup.
Close any last held VideoFrame when the encoder effect tears down to avoid retaining a frame until Video.close() is called.
const reader = processor.getReader();
effect.cleanup(() => reader.cancel());
+ effect.cleanup(() =>
+ this.frame.set((prev) => {
+ prev?.close();
+ return undefined;
+ }),
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In js/hang/src/publish/video/index.ts around lines 88 to 101, the encoder effect
currently creates a VideoTrackProcessor and reader but never closes any
last-held VideoFrame on teardown; update the effect to track the most recently
acquired VideoFrame (e.g., lastFrame variable) and in effect.cleanup() check if
lastFrame is set, call lastFrame.close(), and null it so the frame Signal is
cleared; also ensure any reader/processor resources are released as appropriate
(e.g., cancel/release reader) so no VideoFrame is retained until Video.close().
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
js/signals/src/dom.ts (2)
75-96: Bug: removing a DocumentFragment throws; track child nodes for array renders
parent.appendChild(node)withnodeas aDocumentFragmentmoves its children; the fragment itself is never a child ofparent. The cleanup then tries toremoveChild(node), which will throw. Capture the fragment’s children and remove those instead.Apply this diff:
- } else if (Array.isArray(e)) { - node = document.createDocumentFragment(); - for (const child of e) { - render(effect, node, child); - } + } else if (Array.isArray(e)) { + const fragment = document.createDocumentFragment(); + for (const child of e) { + render(effect, fragment, child); + } + const nodes = Array.from(fragment.childNodes); + parent.appendChild(fragment); + effect.cleanup(() => { + for (const n of nodes) { + if (n.parentNode === parent) parent.removeChild(n); + } + }); + return; } @@ - parent.appendChild(node); - effect.cleanup(() => { - try { - parent.removeChild(node); - } catch (e) { - console.log("cleanup failed", parent, node); - throw e; - } - }); + parent.appendChild(node); + effect.cleanup(() => { + if (node.parentNode === parent) parent.removeChild(node); + });
3-10: Provide effect-scoped event-listener helper to replace removedeventsoptionThere are no remaining call-sites using the dropped
eventsproperty (verified via ripgrep), but its removal also drops support forcapture,passive,onceand prevents multiple listeners. To restore that functionality, add a lightweight helper in js/signals/src/dom.ts:export function on( effect: Effect, target: EventTarget, type: string, listener: EventListenerOrEventListenerObject, options?: AddEventListenerOptions | boolean ) { target.addEventListener(type, listener, options); effect.cleanup(() => target.removeEventListener(type, listener, options)); }js/hang/src/publish/audio/index.ts (1)
115-121: Don’t pass an undefinedsampleRatetoAudioContextoptions.Create options conditionally to avoid invalid init objects and align with optional typing.
- const context = new AudioContext({ - latencyHint: "interactive", - sampleRate: settings.sampleRate, - }); + const context = new AudioContext({ + latencyHint: "interactive", + ...(settings.sampleRate ? { sampleRate: settings.sampleRate } : {}), + });js/hang-demo/src/publish.html (1)
70-76: Docs/API mismatch: attribute and JS API should usesource(notdevice).The PR moved to a source-centric API. Update the snippet to match.
- Either modify HTML attributes like <code class="language-html"><hang-publish device="camera" /></code> - or access the element's Javascript API: + Either modify HTML attributes like <code class="language-html"><hang-publish source="camera" /></code> + or access the element's Javascript API: <pre><code class="language-typescript">const publish = document.getElementById("publish"); -publish.lib.device = "camera";</code></pre> +publish.lib.source = "camera";</code></pre>
♻️ Duplicate comments (6)
js/hang/src/publish/audio/index.ts (2)
31-42: MakesampleRateoptional in settings (Firefox returns undefined).Type is too strong; keeps forcing non-existent values.
-// This is a fork of the MediaTrackSettings interface with properties required for audio or vidfeo. +// This is a fork of the MediaTrackSettings interface with properties required for audio or video. export interface AudioTrackSettings { deviceId: string; groupId: string; autoGainControl: boolean; channelCount: number; echoCancellation: boolean; noiseSuppression: boolean; - sampleRate: number; + sampleRate?: number; sampleSize: number; }
192-207: Avoid closing an empty first group when timestamps start > 0.Initialize
groupTimestampfrom the first frame.- let group: Moq.GroupProducer = this.#track.appendGroup(); + let group: Moq.GroupProducer = this.#track.appendGroup(); effect.cleanup(() => group.close()); - let groupTimestamp = 0; + let groupTimestamp: number | undefined = undefined; @@ - if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { + if (groupTimestamp === undefined) { + groupTimestamp = frame.timestamp; + } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { group.close(); group = this.#track.appendGroup(); groupTimestamp = frame.timestamp; }js/hang/src/publish/source/microphone.ts (1)
43-55: Fix hot-mic/resource leak when cancel winsgetUserMediarace.- effect.spawn(async (cancel) => { - const stream = await Promise.race([ - navigator.mediaDevices.getUserMedia({ audio: finalConstraints }).catch(() => undefined), - cancel, - ]); - if (!stream) return; - - const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined; - if (!track) return; - - effect.cleanup(() => track.stop()); - effect.set(this.stream, track, undefined); - }); + effect.spawn(async (cancel) => { + const req = navigator.mediaDevices + .getUserMedia({ audio: finalConstraints }) + .catch(() => undefined as MediaStream | undefined); + const CANCELLED = Symbol("cancelled"); + const result = await Promise.race([req, cancel.then(() => CANCELLED)]); + if (result === CANCELLED) { + req.then((s) => s?.getTracks().forEach((t) => t.stop())).catch(() => {}); + return; + } + const stream = result as MediaStream | undefined; + if (!stream) return; + + const track = stream.getAudioTracks()[0] as AudioStreamTrack | undefined; + if (!track) { + stream.getTracks().forEach((t) => t.stop()); + return; + } + + effect.cleanup(() => track.stop()); + effect.set(this.stream, track, undefined); + });js/hang/src/publish/element.ts (3)
114-116: Clear broadcast.*.source when switching/clearing to avoid stale, stopped tracks.
Closing wrappers doesn’t clear broadcast.video.source/audio.source, leaving publisher “enabled” and UI states wrong until new sources arrive.this.#audio.peek()?.close(); this.#video.peek()?.close(); + // Clear published sources immediately; effects below will repopulate. + this.broadcast.video.source.set(undefined); + this.broadcast.audio.source.set(undefined); @@ } else { - this.#video.set(undefined); - this.#audio.set(undefined); + this.#video.set(undefined); + this.#audio.set(undefined); + // Also clear published sources when deselecting. + this.broadcast.video.source.set(undefined); + this.broadcast.audio.source.set(undefined); }Also applies to: 152-154
119-129: Make Effect.set reversible for camera/mic streams.
Without a cleanup value, broadcast.*.source can remain set after the Source is closed.- video.signals.effect((effect) => { + video.signals.effect((effect) => { const stream = effect.get(video.stream); - effect.set(this.broadcast.video.source, stream); + effect.set(this.broadcast.video.source, stream, undefined); }); @@ - audio.signals.effect((effect) => { + audio.signals.effect((effect) => { const stream = effect.get(audio.stream); - effect.set(this.broadcast.audio.source, stream); + effect.set(this.broadcast.audio.source, stream, undefined); });
135-141: Screen wiring should clear on cleanup; don’t early-return.
Returning early prevents clearing sources when the stream ends; also add cleanup value.- screen.signals.effect((effect) => { + screen.signals.effect((effect) => { const stream = effect.get(screen.stream); - if (!stream) return; - - effect.set(this.broadcast.video.source, stream.video); - effect.set(this.broadcast.audio.source, stream.audio); + effect.set(this.broadcast.video.source, stream?.video, undefined); + effect.set(this.broadcast.audio.source, stream?.audio, undefined); });
🧹 Nitpick comments (7)
js/signals/src/dom.ts (1)
47-56: Simplify children append; no need to branch or manually create Text nodes
appendaccepts bothNodeandstring. This reduces code and DOM allocations.- // Append children - if (children) { - children.forEach((child) => { - if (typeof child === "string") { - element.appendChild(document.createTextNode(child)); - } else { - element.appendChild(child); - } - }); - } + // Append children + if (children.length) element.append(...children);js/hang/src/publish/audio/index.ts (1)
227-233: Clean uponmessagehandler to prevent leaks if the worklet outlives the effect.- worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => { + worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => { const channels = data.channels.slice(0, settings.channelCount); ... encoder.encode(frame); frame.close(); }; + effect.cleanup(() => { + worklet.port.onmessage = null; + });js/hang/src/publish/source/device.ts (1)
103-106: Nit: typo in comment (“preferrence”).- // NOTE: The default device might change, and with no (valid) preferrence, we should switch to it. + // NOTE: The default device might change, and with no (valid) preference, we should switch to it.js/hang/src/publish/source/camera.ts (1)
16-18:flipis unused. Implement or drop to avoid dead API.Options:
- Wire into downstream preview via CSS transform (outside this module), or
- Add a
MediaStreamTrackProcessor/Generatorpath to apply mirroring, or- Remove
flipfrom this class for now.js/hang/src/publish/element.ts (3)
8-8: Also observe a “source” attribute (keep “device” for back-compat).
Allows declarative use via while preserving existing “device”.-const OBSERVED = ["url", "name", "device", "audio", "video", "controls", "captions"] as const; +const OBSERVED = ["url", "name", "device", "source", "audio", "video", "controls", "captions"] as const; @@ - } else if (name === "device") { - if (newValue === "camera" || newValue === "screen" || newValue === null) { - this.source = newValue ?? undefined; - } else { - throw new Error(`Invalid device: ${newValue}`); - } + } else if (name === "device" || name === "source") { + if (newValue === "camera" || newValue === "screen" || newValue === null) { + this.source = newValue ?? undefined; + } else { + throw new Error(`Invalid ${name}: ${newValue}`); + }Also applies to: 71-77
277-281: Rename UI label to “Source:” (source-centric API).
Minor clarity tweak matching the new model.- "Device:", + "Source:",
499-521: Tidy unreachable “connected” branch in status.
Given preceding conditions, the finalelse if (status === "connected")never runs.- } else if (status === "connected") { - container.textContent = "🟢 Connected"; - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
js/hang-demo/src/publish.html(1 hunks)js/hang/src/publish/audio/index.ts(8 hunks)js/hang/src/publish/element.ts(7 hunks)js/hang/src/publish/source/camera.ts(1 hunks)js/hang/src/publish/source/device.ts(1 hunks)js/hang/src/publish/source/index.ts(1 hunks)js/hang/src/publish/source/microphone.ts(1 hunks)js/signals/src/dom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang/src/publish/source/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/signals/src/dom.tsjs/hang-demo/src/publish.htmljs/hang/src/publish/source/microphone.tsjs/hang/src/publish/source/device.tsjs/hang/src/publish/element.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/source/camera.ts
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/source/microphone.tsjs/hang/src/publish/source/device.tsjs/hang/src/publish/element.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/source/camera.ts
🧬 Code graph analysis (5)
js/hang/src/publish/source/microphone.ts (4)
js/signals/src/index.ts (3)
Signal(19-83)Effect(88-447)effect(303-313)js/hang/src/publish/source/device.ts (4)
DeviceProps(3-5)Device(7-111)effect(40-85)effect(87-106)js/hang/src/publish/audio/index.ts (6)
AudioConstraints(18-21)AudioStreamTrack(24-28)effect(102-154)effect(156-169)effect(171-250)effect(252-270)js/hang/src/publish/source/camera.ts (1)
effect(31-61)
js/hang/src/publish/source/device.ts (3)
js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)Effect(88-447)effect(303-313)js/hang/src/publish/source/camera.ts (1)
effect(31-61)js/hang/src/publish/source/microphone.ts (1)
effect(30-56)
js/hang/src/publish/element.ts (9)
js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/index.ts (5)
Source(16-16)effect(102-154)effect(156-169)effect(171-250)effect(252-270)js/hang/src/publish/video/index.ts (3)
Source(13-13)effect(88-168)effect(334-369)js/hang/src/publish/source/camera.ts (2)
Camera(12-67)effect(31-61)js/hang/src/publish/source/screen.ts (2)
Screen(11-76)effect(28-71)js/hang/src/publish/source/microphone.ts (2)
Microphone(11-62)effect(30-56)js/hang/src/publish/preview.ts (1)
preview(41-43)js/hang/src/meet/element.ts (4)
name(89-118)name(120-153)name(174-176)name(178-180)js/hang/src/watch/element.ts (6)
name(140-142)name(144-146)parent(276-294)parent(296-349)parent(351-378)parent(380-400)
js/hang/src/publish/audio/index.ts (8)
js/hang/src/publish/video/index.ts (4)
Source(13-13)effect(88-168)effect(334-369)settings(171-297)js/signals/src/index.ts (2)
Signal(19-83)effect(303-313)js/hang/src/publish/audio/speaking.ts (2)
Speaking(14-117)effect(31-111)js/hang/src/publish/audio/captions.ts (2)
Captions(17-136)effect(40-131)js/hang/src/publish/element.ts (2)
source(107-109)source(111-157)js/hang/src/publish/source/microphone.ts (1)
effect(30-56)js/hang/src/publish/source/screen.ts (1)
effect(28-71)js/moq/src/group.ts (1)
GroupProducer(8-71)
js/hang/src/publish/source/camera.ts (3)
js/signals/src/index.ts (3)
Signal(19-83)Effect(88-447)effect(303-313)js/hang/src/publish/video/index.ts (4)
VideoConstraints(37-43)VideoStreamTrack(19-23)effect(88-168)effect(334-369)js/hang/src/publish/source/microphone.ts (1)
effect(30-56)
🔇 Additional comments (7)
js/hang/src/publish/element.ts (7)
5-6: LGTM: Source-centric imports.
11-12: LGTM: Local SourceType alias.
21-24: LGTM: Internal state signals for active source wrappers.
35-41: LGTM: Auto-enable publish when any track exists.
46-58: LGTM: Preview binds to video source with proper cleanup.
64-66: LGTM: Early-return on unchanged attributes.
143-147: LGTM: Bind screen.enabled to audio||video with explicit cleanup.
| <hang-publish url="%VITE_RELAY_URL%" name="me" controls captions> | ||
| <!-- It's optional to provide a video element to preview the outgoing media. --> | ||
| <video style="max-width: 100%; height: auto; border-radius: 4px; margin: 0 auto;" muted autoplay></video> | ||
| </hang-publish> |
There was a problem hiding this comment.
Example code below references #publish but the element lacks an id.
Add id="publish" so document.getElementById("publish") works.
- <hang-publish url="%VITE_RELAY_URL%" name="me" controls captions>
+ <hang-publish id="publish" url="%VITE_RELAY_URL%" name="me" controls captions>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <hang-publish url="%VITE_RELAY_URL%" name="me" controls captions> | |
| <!-- It's optional to provide a video element to preview the outgoing media. --> | |
| <video style="max-width: 100%; height: auto; border-radius: 4px; margin: 0 auto;" muted autoplay></video> | |
| </hang-publish> | |
| <hang-publish id="publish" url="%VITE_RELAY_URL%" name="me" controls captions> | |
| <!-- It's optional to provide a video element to preview the outgoing media. --> | |
| <video style="max-width: 100%; height: auto; border-radius: 4px; margin: 0 auto;" muted autoplay></video> | |
| </hang-publish> |
🤖 Prompt for AI Agents
In js/hang-demo/src/publish.html around lines 25 to 28, the <hang-publish>
element is referenced by document.getElementById("publish") but lacks an id
attribute; add id="publish" to the <hang-publish> tag so the DOM lookup
succeeds, ensuring any existing code that calls
document.getElementById("publish") will correctly find the element.
| if (!devices.length) { | ||
| console.warn(`no ${this.kind} devices found`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid stale state when no devices are found.
Currently returns early without clearing lists; downstream may keep using removed devices.
- if (!devices.length) {
- console.warn(`no ${this.kind} devices found`);
- return;
- }
+ if (!devices.length) {
+ console.warn(`no ${this.kind} devices found`);
+ effect.set(this.#devices, [], []);
+ effect.set(this.#default, undefined, undefined);
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!devices.length) { | |
| console.warn(`no ${this.kind} devices found`); | |
| return; | |
| } | |
| if (!devices.length) { | |
| console.warn(`no ${this.kind} devices found`); | |
| effect.set(this.#devices, [], []); | |
| effect.set(this.#default, undefined, undefined); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In js/hang/src/publish/source/device.ts around lines 46 to 49, the code returns
early when no devices are found but leaves internal state intact causing
downstream code to use stale device lists; fix by clearing the relevant stored
state before returning (e.g., reset this.devices to an empty array, clear any
device maps/sets, nullify selected device indices or related UI state, and
emit/update any change notifications), then return as before.
And remove Claude because it's not very good at PRs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/hang/src/publish/audio/index.ts (2)
146-153: Make #worklet and #gain signals reversible on teardown.Prevent stale references by reverting to undefined on cleanup.
- effect.set(this.#worklet, worklet); + effect.set(this.#worklet, worklet, undefined); @@ - effect.set(this.#gain, gain); + effect.set(this.#gain, gain, undefined);
273-277: Close Speaking on teardown to release its worker and track.
Audio.close()should also callthis.speaking.close().close() { this.#signals.close(); this.captions.close(); + this.speaking.close(); this.#track.close(); }
♻️ Duplicate comments (6)
js/hang/src/publish/source/device.ts (1)
46-49: Clear state when no devices are found to prevent stale selections.If enumeration returns zero devices,
available/defaultremain from a previous run. Reset both before returning.- if (!devices.length) { - console.warn(`no ${this.kind} devices found`); - return; - } + if (!devices.length) { + console.warn(`no ${this.kind} devices found`); + effect.set(this.#devices, [], []); + effect.set(this.#default, undefined, undefined); + return; + }js/hang/src/publish/audio/index.ts (2)
24-42: Relax AudioTrackSettings and getSettings typing; sampleRate can be undefined.Firefox (and sometimes others) omit
sampleRate. The code already guards at runtime; types should reflect that to avoid unsoundness.export interface AudioStreamTrack extends MediaStreamTrack { kind: "audio"; - clone(): AudioStreamTrack; - getSettings(): AudioTrackSettings; + clone(): AudioStreamTrack; + getSettings(): AudioTrackSettings; } @@ // MediaTrackSettings can represent both audio and video, which means a LOT of possibly undefined properties. -// This is a fork of the MediaTrackSettings interface with properties required for audio or vidfeo. +// This is a fork of the MediaTrackSettings interface with properties required for audio or video. export interface AudioTrackSettings { deviceId: string; groupId: string; autoGainControl: boolean; channelCount: number; echoCancellation: boolean; noiseSuppression: boolean; - sampleRate: number; + sampleRate?: number; sampleSize: number; }
192-207: Fix first-frame group rotation to avoid closing an empty group.Initialize timestamp from the first frame before applying the rotate threshold.
- let group: Moq.GroupProducer = this.#track.appendGroup(); - effect.cleanup(() => group.close()); - - let groupTimestamp = 0; + let group: Moq.GroupProducer = this.#track.appendGroup(); + effect.cleanup(() => group.close()); + + let groupTimestamp: number | undefined = undefined; @@ - if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { + if (groupTimestamp === undefined) { + groupTimestamp = frame.timestamp; + } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { group.close(); group = this.#track.appendGroup(); groupTimestamp = frame.timestamp; }js/hang/src/publish/element.ts (3)
123-125: Clear broadcast sources when switching to a new source.After closing the previous Source.* instances, immediately clear
broadcast.video.sourceandbroadcast.audio.sourceto avoid stale tracks keeping the publisher “enabled.”this.#audio.peek()?.close(); this.#video.peek()?.close(); + // Clear published sources immediately; effects below will repopulate. + this.broadcast.video.source.set(undefined); + this.broadcast.audio.source.set(undefined);
128-137: Make camera wiring reversible on cleanup.Ensure
broadcast.*.sourcereverts when the effect disposes.- video.signals.effect((effect) => { + video.signals.effect((effect) => { const stream = effect.get(video.stream); - effect.set(this.broadcast.video.source, stream); + effect.set(this.broadcast.video.source, stream, undefined); }); @@ - audio.signals.effect((effect) => { + audio.signals.effect((effect) => { const stream = effect.get(audio.stream); - effect.set(this.broadcast.audio.source, stream); + effect.set(this.broadcast.audio.source, stream, undefined); });
145-151: Screen wiring: avoid early return and revert on cleanup.Don’t return early when
streamis absent; always set sources (to values or undefined) so teardown clears them.- screen.signals.effect((effect) => { - const stream = effect.get(screen.stream); - if (!stream) return; - - effect.set(this.broadcast.video.source, stream.video); - effect.set(this.broadcast.audio.source, stream.audio); - }); + screen.signals.effect((effect) => { + const stream = effect.get(screen.stream); + effect.set(this.broadcast.video.source, stream?.video, undefined); + effect.set(this.broadcast.audio.source, stream?.audio, undefined); + });
🧹 Nitpick comments (6)
js/hang/src/publish/source/device.ts (1)
80-81: Gate noisy debug logs (optional).Consider gating
console.debugbehind a DEV flag to avoid log spam in production.js/hang/src/publish/audio/captions.ts (2)
50-57: Revert catalog on cleanup to avoid stale UI/state.
this.catalogstays set after teardown. Make it reversible.- effect.set(this.catalog, catalog); + effect.set(this.catalog, catalog, undefined);
70-78: Fix log prefix for clarity.This is captions, not VAD. Adjust the error message.
- console.error("VAD worker error:", data.message); + console.error("Captions worker error:", data.message);js/hang/src/publish/audio/index.ts (2)
115-123: Avoid passing undefined sampleRate to AudioContext options.Construct options conditionally to prevent an undefined numeric field.
- const context = new AudioContext({ - latencyHint: "interactive", - sampleRate: settings.sampleRate, - }); + const ctxOpts: AudioContextOptions = { latencyHint: "interactive" }; + if (settings.sampleRate !== undefined) ctxOpts.sampleRate = settings.sampleRate; + const context = new AudioContext(ctxOpts);
31-31: Typo in comment (“vidfeo”).Minor comment fix.
-// This is a fork of the MediaTrackSettings interface with properties required for audio or vidfeo. +// This is a fork of the MediaTrackSettings interface with properties required for audio or video.js/hang/src/publish/element.ts (1)
521-521: UI copy: say “Select Source” (not “Device”).Aligns with the new source-based model.
- container.textContent = "🟡 Select Device"; + container.textContent = "🟡 Select Source";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.github/workflows/claude-code-review.yml(0 hunks).github/workflows/claude-dispatch.yml(0 hunks)js/hang-demo/src/publish.html(2 hunks)js/hang/src/publish/audio/captions.ts(4 hunks)js/hang/src/publish/audio/index.ts(9 hunks)js/hang/src/publish/audio/speaking.ts(4 hunks)js/hang/src/publish/element.ts(7 hunks)js/hang/src/publish/source/camera.ts(1 hunks)js/hang/src/publish/source/device.ts(1 hunks)js/hang/src/publish/source/microphone.ts(1 hunks)js/hang/src/watch/audio/index.ts(3 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/claude-dispatch.yml
- .github/workflows/claude-code-review.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- js/hang/src/watch/audio/index.ts
- js/hang/src/publish/source/camera.ts
- js/hang-demo/src/publish.html
- js/hang/src/publish/audio/speaking.ts
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/element.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/source/device.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/element.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/source/device.ts
🧬 Code graph analysis (5)
js/hang/src/publish/element.ts (7)
js/signals/src/index.ts (2)
Signal(19-83)Effect(88-447)js/hang/src/publish/audio/index.ts (5)
Source(16-16)effect(102-154)effect(156-169)effect(171-251)effect(253-271)js/hang/src/publish/video/index.ts (3)
Source(13-13)effect(88-168)effect(334-369)js/hang/src/publish/source/camera.ts (2)
Camera(11-68)effect(28-62)js/hang/src/publish/source/screen.ts (2)
Screen(11-76)effect(28-71)js/hang/src/publish/source/microphone.ts (2)
Microphone(11-66)effect(30-60)js/hang/src/meet/element.ts (4)
name(89-118)name(120-153)name(174-176)name(178-180)
js/hang/src/publish/audio/captions.ts (2)
js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/index.ts (5)
Audio(59-278)effect(102-154)effect(156-169)effect(171-251)effect(253-271)
js/hang/src/publish/source/microphone.ts (4)
js/signals/src/index.ts (3)
Signal(19-83)Effect(88-447)effect(303-313)js/hang/src/publish/source/device.ts (4)
DeviceProps(3-5)Device(7-111)effect(40-85)effect(87-106)js/hang/src/publish/audio/index.ts (6)
AudioConstraints(18-21)AudioStreamTrack(24-28)effect(102-154)effect(156-169)effect(171-251)effect(253-271)js/hang/src/publish/source/camera.ts (1)
effect(28-62)
js/hang/src/publish/audio/index.ts (7)
js/hang/src/publish/video/index.ts (3)
Source(13-13)effect(88-168)effect(334-369)js/signals/src/index.ts (2)
Signal(19-83)effect(303-313)js/hang/src/publish/audio/speaking.ts (2)
Speaking(14-118)effect(31-112)js/hang/src/publish/audio/captions.ts (2)
Captions(17-137)effect(40-132)js/hang/src/publish/source/microphone.ts (1)
effect(30-60)js/hang/src/publish/source/screen.ts (1)
effect(28-71)js/moq/src/group.ts (1)
GroupProducer(8-71)
js/hang/src/publish/source/device.ts (4)
js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)Effect(88-447)effect(303-313)js/hang/src/publish/source/camera.ts (1)
effect(28-62)js/hang/src/publish/source/microphone.ts (1)
effect(30-60)js/hang/src/publish/element.ts (2)
device(108-110)device(112-114)
🔇 Additional comments (1)
js/hang/src/publish/source/microphone.ts (1)
43-59: getUserMedia race is handled; hot-mic mitigated.Pre-registering cleanup on the pending
mediapromise ensures late streams are stopped when the effect cancels. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang/src/publish/audio/index.ts (1)
180-191: Possible runtime crash if channelCount is missing; derive from worklet as a reliable fallbackFirefox can omit channelCount; configuring AudioEncoder with undefined channels will throw. Use worklet.channelCount (always present) as the primary source; also base bitrate on the resolved channel count.
-const config = { - // TODO get codec and description from decoderConfig - codec: "opus", - // Firefox doesn't provide the sampleRate in the settings. - sampleRate: u53(settings.sampleRate ?? worklet?.context.sampleRate), - numberOfChannels: u53(settings.channelCount), - // TODO configurable - bitrate: u53(settings.channelCount * 32_000), -}; +const resolvedChannels = worklet.channelCount ?? settings.channelCount ?? 1; +const resolvedRate = settings.sampleRate ?? worklet.context.sampleRate; +const config = { + // TODO get codec and description from decoderConfig + codec: "opus", + sampleRate: u53(resolvedRate), + numberOfChannels: u53(resolvedChannels), + // TODO configurable + bitrate: u53(resolvedChannels * 32_000), +};Additionally, ensure the frame path slices using the same resolvedChannels:
-worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => { - const channels = data.channels.slice(0, settings.channelCount); +worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => { + const limit = worklet.channelCount ?? settings.channelCount ?? data.channels.length; + const channels = data.channels.slice(0, limit); const joinedLength = channels.reduce((a, b) => a + b.length, 0); // ... const frame = new AudioData({ format: "f32-planar", - sampleRate: worklet.context.sampleRate, - numberOfFrames: channels[0].length, - numberOfChannels: channels.length, + sampleRate: worklet.context.sampleRate, + numberOfFrames: channels[0].length, + numberOfChannels: channels.length, timestamp: (1_000_000 * data.timestamp) / worklet.context.sampleRate, data: joined, transfer: [joined.buffer], });
♻️ Duplicate comments (2)
js/hang/src/publish/audio/index.ts (2)
192-207: Avoid closing an empty first group when timestamps start > 0Initialize groupTimestamp from the first encoded frame before rotating.
-let group: Moq.GroupProducer = this.#track.appendGroup(); +let group: Moq.GroupProducer = this.#track.appendGroup(); effect.cleanup(() => group.close()); -let groupTimestamp = 0; +let groupTimestamp: number | undefined = undefined; @@ - if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { + if (groupTimestamp === undefined) { + groupTimestamp = frame.timestamp; + } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { group.close(); group = this.#track.appendGroup(); groupTimestamp = frame.timestamp; }
24-28: getSettings typing is too strict; sampleRate can be undefined in browsers (e.g., Firefox)Make sampleRate optional in AudioTrackSettings and keep the runtime fallback you already use later.
export interface AudioTrackSettings { deviceId: string; groupId: string; autoGainControl: boolean; channelCount: number; echoCancellation: boolean; noiseSuppression: boolean; - sampleRate: number; + sampleRate?: number; sampleSize: number; }Nit: fix the comment typo “vidfeo” → “video”.
-// This is a fork of the MediaTrackSettings interface with properties required for audio or vidfeo. +// This is a fork of the MediaTrackSettings interface with properties required for audio or video.
🧹 Nitpick comments (1)
js/hang/src/publish/audio/index.ts (1)
209-214: Propagate encoder errors to the track, not just the current groupAlign with video: aborting the track helps downstream react correctly.
error: (err) => { group.abort(err); + this.#track.abort(err); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
js/hang/src/publish/audio/index.ts(9 hunks)js/hang/src/publish/source/camera.ts(1 hunks)js/hang/src/publish/source/device.ts(1 hunks)js/hang/src/publish/source/microphone.ts(1 hunks)js/hang/src/watch/audio/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- js/hang/src/watch/audio/index.ts
- js/hang/src/publish/source/microphone.ts
- js/hang/src/publish/source/device.ts
- js/hang/src/publish/source/camera.ts
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/audio/index.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/audio/index.ts
🧬 Code graph analysis (1)
js/hang/src/publish/audio/index.ts (7)
js/hang/src/publish/video/index.ts (4)
Source(13-13)effect(88-168)effect(334-369)settings(171-297)js/signals/src/index.ts (2)
Signal(19-83)effect(303-313)js/hang/src/publish/audio/speaking.ts (2)
Speaking(14-118)effect(31-112)js/hang/src/publish/audio/captions.ts (2)
Captions(17-137)effect(40-132)js/hang/src/publish/source/microphone.ts (1)
effect(30-63)js/hang/src/publish/source/screen.ts (1)
effect(28-71)js/moq/src/group.ts (1)
GroupProducer(8-71)
🔇 Additional comments (5)
js/hang/src/publish/audio/index.ts (5)
16-17: Source alias parity looks goodMatches the video module’s Source typing.
46-51: Props now accept Signals—LGTMThe Signal.from surface is consistent and backwards-friendly defaults are sensible.
69-70: Exposing source as Signal is appropriateEnables reactive rewiring across the pipeline.
88-94: Constructor Signal wrapping—LGTMGood consolidation via Signal.from; sane defaults.
250-252: Cleanup handler—LGTMClearing onmessage avoids leaks.
| #runSource(effect: Effect): void { | ||
| const enabled = effect.get(this.enabled); | ||
| const media = effect.get(this.media); | ||
| if (!enabled || !media) return; | ||
| if (!enabled) return; | ||
|
|
||
| const settings = media.getSettings(); | ||
| if (!settings) { | ||
| throw new Error("track has no settings"); | ||
| } | ||
| const source = effect.get(this.source); | ||
| if (!source) return; | ||
|
|
||
| // Insert the track into the broadcast. | ||
| this.broadcast.insertTrack(this.#track.consume()); | ||
| effect.cleanup(() => this.broadcast.removeTrack(this.#track.name)); | ||
|
|
||
| const settings = source.getSettings(); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Track insertion in #runSource diverges from video; confirm intent + minor AudioContext nit
-
In video, insert happens in #runEncoder after config is known. Here it’s in #runSource. If intentional, fine; otherwise consider moving for parity so the track only appears when the encoder is ready.
-
Optional: only set sampleRate on AudioContext when defined to avoid passing an explicit undefined.
-const context = new AudioContext({
- latencyHint: "interactive",
- sampleRate: settings.sampleRate,
-});
+const ctxOpts: AudioContextOptions = { latencyHint: "interactive" };
+if (settings.sampleRate !== undefined) ctxOpts.sampleRate = settings.sampleRate;
+const context = new AudioContext(ctxOpts);- Recommend explicit channel behavior on the capture worklet node (prevents implicit up/downmixing).
const worklet = new AudioWorkletNode(context, "capture", {
numberOfInputs: 1,
numberOfOutputs: 0,
- channelCount: settings.channelCount,
+ channelCount: settings.channelCount,
+ channelCountMode: "explicit",
+ channelInterpretation: "discrete",
});Also applies to: 121-123
🏁 Script executed:
#!/bin/bash
# Ensure the file exists
echo "File exists:"
ls -l js/hang/src/publish/audio/index.ts
# Show the runSource method context
echo -e "\n--- runSource context ---"
rg -n "runSource" -C5 js/hang/src/publish/audio/index.ts
# Show the runEncoder method context
echo -e "\n--- runEncoder context ---"
rg -n "runEncoder" -C5 js/hang/src/publish/audio/index.ts
# Show AudioContext instantiation
echo -e "\n--- AudioContext instantiation ---"
rg -n "new AudioContext" -C5 js/hang/src/publish/audio/index.ts
# Show AudioWorkletNode instantiation
echo -e "\n--- AudioWorkletNode instantiation ---"
rg -n "new AudioWorkletNode" -C5 js/hang/src/publish/audio/index.tsLength of output: 2859
🏁 Script executed:
#!/bin/bash
# Show full #runEncoder implementation to verify track insertion timing
sed -n '165,260p' js/hang/src/publish/audio/index.tsLength of output: 2771
Confirm track insertion timing & refine AudioContext/WorkletNode options
- Confirm if inserting the track in
#runSource(line 102) instead of in#runEncoderis intentional—video shows insertion after encoder configuration. - Optional: only set
sampleRateonAudioContextwhen defined to avoid passingundefined:
-const context = new AudioContext({
- latencyHint: "interactive",
- sampleRate: settings.sampleRate,
-});
+const ctxOpts: AudioContextOptions = { latencyHint: "interactive" };
+if (settings.sampleRate !== undefined) ctxOpts.sampleRate = settings.sampleRate;
+const context = new AudioContext(ctxOpts);- Essential: explicitly set
channelCountModeandchannelInterpretationon the capture worklet to prevent implicit mixing:
const worklet = new AudioWorkletNode(context, "capture", {
numberOfInputs: 1,
numberOfOutputs: 0,
- channelCount: settings.channelCount,
+ channelCount: settings.channelCount,
+ channelCountMode: "explicit",
+ channelInterpretation: "discrete",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #runSource(effect: Effect): void { | |
| const enabled = effect.get(this.enabled); | |
| const media = effect.get(this.media); | |
| if (!enabled || !media) return; | |
| if (!enabled) return; | |
| const settings = media.getSettings(); | |
| if (!settings) { | |
| throw new Error("track has no settings"); | |
| } | |
| const source = effect.get(this.source); | |
| if (!source) return; | |
| // Insert the track into the broadcast. | |
| this.broadcast.insertTrack(this.#track.consume()); | |
| effect.cleanup(() => this.broadcast.removeTrack(this.#track.name)); | |
| const settings = source.getSettings(); | |
| // … previous lines … | |
| const settings = source.getSettings(); | |
| // Build AudioContext options, only adding sampleRate if defined | |
| const ctxOpts: AudioContextOptions = { latencyHint: "interactive" }; | |
| if (settings.sampleRate !== undefined) { | |
| ctxOpts.sampleRate = settings.sampleRate; | |
| } | |
| const context = new AudioContext(ctxOpts); | |
| // Set up the capture worklet with explicit channel handling | |
| - const worklet = new AudioWorkletNode(context, "capture", { | |
| - numberOfInputs: 1, | |
| - numberOfOutputs: 0, | |
| - channelCount: settings.channelCount, | |
| const worklet = new AudioWorkletNode(context, "capture", { | |
| numberOfInputs: 1, | |
| numberOfOutputs: 0, | |
| channelCount: settings.channelCount, | |
| channelCountMode: "explicit", | |
| channelInterpretation: "discrete", | |
| }); | |
| // … following lines … |
🤖 Prompt for AI Agents
In js/hang/src/publish/audio/index.ts around lines 102–114, confirm whether
insertTrack should occur here in #runSource or after encoder setup in
#runEncoder (move insertTrack to #runEncoder to match video timing if insertion
should happen post-encoder). When constructing the AudioContext only pass
sampleRate if it is defined (avoid passing undefined). Also explicitly set the
capture WorkletNode's channelCountMode to "explicit" and channelInterpretation
to "discrete" to prevent implicit channel mixing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang/src/publish/audio/index.ts (1)
113-121: Avoid passing undefined sampleRate and prevent implicit channel mixing.- const context = new AudioContext({ - latencyHint: "interactive", - sampleRate: settings.sampleRate, - }); + const ctxOpts: AudioContextOptions = { latencyHint: "interactive" }; + if (settings.sampleRate !== undefined) ctxOpts.sampleRate = settings.sampleRate; + const context = new AudioContext(ctxOpts); @@ - const worklet = new AudioWorkletNode(context, "capture", { + const worklet = new AudioWorkletNode(context, "capture", { numberOfInputs: 1, numberOfOutputs: 0, channelCount: settings.channelCount, + channelCountMode: "explicit", + channelInterpretation: "discrete", }); - - effect.set(this.#worklet, worklet); + + effect.set(this.#worklet, worklet, undefined); @@ - // Only set the gain after the worklet is registered. - effect.set(this.#gain, gain); + // Only set the gain after the worklet is registered. + effect.set(this.#gain, gain, undefined);Also applies to: 133-141
♻️ Duplicate comments (5)
js/hang/src/publish/source/device.ts (1)
46-49: Clear state when no devices are found to prevent stale selections.Returning early leaves available/default/selected unchanged. Reset them before returning.
- if (!devices.length) { - console.warn(`no ${this.kind} devices found`); - return; - } + if (!devices.length) { + console.warn(`no ${this.kind} devices found`); + effect.set(this.#devices, [], []); + effect.set(this.#default, undefined, undefined); + effect.set(this.#selected, undefined, undefined); + return; + }js/hang/src/publish/element.ts (3)
123-125: Clear published sources when switching/clearing to avoid stale tracks.this.#audio.peek()?.close(); this.#video.peek()?.close(); + this.broadcast.video.source.set(undefined); + this.broadcast.audio.source.set(undefined); @@ } else { this.#video.set(undefined); this.#audio.set(undefined); + this.broadcast.video.source.set(undefined); + this.broadcast.audio.source.set(undefined); }Also applies to: 161-163
128-137: Make Effect.set reversible for camera wiring.Without cleanup values, stale streams persist after teardown.
- const stream = effect.get(video.stream); - effect.set(this.broadcast.video.source, stream); + const stream = effect.get(video.stream); + effect.set(this.broadcast.video.source, stream, undefined); @@ - const stream = effect.get(audio.stream); - effect.set(this.broadcast.audio.source, stream); + const stream = effect.get(audio.stream); + effect.set(this.broadcast.audio.source, stream, undefined);
144-151: Screen wiring should revert on cleanup and when stream disappears.- const stream = effect.get(screen.stream); - if (!stream) return; - - effect.set(this.broadcast.video.source, stream.video); - effect.set(this.broadcast.audio.source, stream.audio); + const stream = effect.get(screen.stream); + effect.set(this.broadcast.video.source, stream?.video, undefined); + effect.set(this.broadcast.audio.source, stream?.audio, undefined);js/hang/src/publish/audio/index.ts (1)
185-199: First-frame grouping rotates immediately if timestamps start > 0.Initialize from the first encoded frame to avoid closing an empty group.
- let group: Moq.GroupProducer = this.#track.appendGroup(); - effect.cleanup(() => group.close()); - - let groupTimestamp = 0; + let group: Moq.GroupProducer = this.#track.appendGroup(); + effect.cleanup(() => group.close()); + + let groupTimestamp: number | undefined = undefined; @@ - if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { + if (groupTimestamp === undefined) { + groupTimestamp = frame.timestamp; + } else if (frame.timestamp - groupTimestamp >= 1000 * this.maxLatency) { group.close(); group = this.#track.appendGroup(); groupTimestamp = frame.timestamp; }
🧹 Nitpick comments (5)
js/hang/src/publish/source/device.ts (1)
63-78: Provide a final fallback default device.If no alias/label match is found, fall back to the first device to reduce “no selection” cases.
} } console.debug("all devices", devices); console.debug("default device", defaultDevice); + if (!defaultDevice) { + defaultDevice = devices[0]; + }js/hang/src/publish/element.ts (2)
76-77: Message should mention “source” too.- throw new Error(`Invalid device: ${newValue}`); + throw new Error(`Invalid source/device: ${newValue}`);
364-368: Caret direction is inverted (▲ when open, ▼ when closed).- caret.innerText = v ? "▼" : "▲"; + caret.innerText = v ? "▲" : "▼";Also applies to: 446-450
js/hang/src/publish/audio/index.ts (1)
248-255: Align audio and video track insertion timing
Audio inserts in runCatalog (js/hang/src/publish/audio/index.ts:253-254) while video inserts in runEncoder (js/hang/src/publish/video/index.ts:96-97); confirm this divergence is intentional, and if not, consider unifying the insertion point to prevent audio frames appearing before video.js/hang/src/publish/audio/speaking.ts (1)
47-48: Make catalog reversible on cleanup.Prevents stale catalog after teardown.
- effect.set(this.catalog, catalog); + effect.set(this.catalog, catalog, undefined);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
js/hang/src/publish/audio/captions.ts(4 hunks)js/hang/src/publish/audio/index.ts(11 hunks)js/hang/src/publish/audio/speaking.ts(4 hunks)js/hang/src/publish/element.ts(8 hunks)js/hang/src/publish/source/device.ts(1 hunks)js/hang/src/publish/video/detection.ts(3 hunks)js/hang/src/watch/audio/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- js/hang/src/publish/video/detection.ts
- js/hang/src/publish/audio/captions.ts
- js/hang/src/watch/audio/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/source/device.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/element.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/source/device.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/element.ts
🧬 Code graph analysis (4)
js/hang/src/publish/source/device.ts (3)
js/signals/src/index.ts (4)
Signal(19-83)Getter(10-13)Effect(88-447)effect(303-313)js/hang/src/publish/source/microphone.ts (1)
effect(30-63)js/hang/src/publish/source/camera.ts (1)
effect(28-65)
js/hang/src/publish/audio/speaking.ts (5)
js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/index.ts (4)
effect(104-147)effect(149-162)effect(164-246)effect(248-270)js/hang/src/publish/audio/captions.ts (1)
effect(40-128)js/hang/src/publish/source/microphone.ts (1)
effect(30-63)js/hang/src/publish/element.ts (2)
source(116-118)source(120-166)
js/hang/src/publish/audio/index.ts (8)
js/hang/src/publish/video/index.ts (4)
Source(13-13)effect(88-168)effect(334-369)settings(171-297)js/signals/src/index.ts (3)
Signal(19-83)effect(303-313)Effect(88-447)js/hang/src/publish/audio/speaking.ts (2)
Speaking(14-114)effect(31-108)js/hang/src/publish/audio/captions.ts (2)
Captions(17-133)effect(40-128)js/hang/src/publish/source/microphone.ts (1)
effect(30-63)js/hang/src/publish/source/camera.ts (1)
effect(28-65)js/hang/src/publish/source/screen.ts (1)
effect(28-71)js/moq/src/group.ts (1)
GroupProducer(8-71)
js/hang/src/publish/element.ts (6)
js/signals/src/index.ts (2)
Signal(19-83)Effect(88-447)js/hang/src/publish/source/camera.ts (2)
Camera(11-71)effect(28-65)js/hang/src/publish/source/screen.ts (2)
Screen(11-76)effect(28-71)js/hang/src/publish/source/microphone.ts (2)
Microphone(11-69)effect(30-63)js/hang/src/publish/source/device.ts (2)
effect(40-85)effect(87-106)js/hang/src/meet/element.ts (4)
name(89-118)name(120-153)name(174-176)name(178-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (2)
js/hang/src/publish/audio/index.ts (1)
26-30: Relax sampleRate to optional in AudioTrackSettings
Firefox can omit sampleRate; existing call sites (including AudioContext instantiation and worklet fallback) already tolerate undefined.js/hang/src/publish/audio/speaking.ts (1)
25-33: LGTM: props-to-signal migration and gating are correct.
Summary by CodeRabbit
New Features
Improvements
Chores