Revamp chat typing and audio speaking. #559
Conversation
WalkthroughRefactors framing from Container to a new Frame module with varint timestamps; introduces typed read/write helpers (string/json/bool) across moq; splits Chat into Message/Typing for publish and watch; replaces many container wrappers with direct TrackProducer/TrackConsumer usage and adds Zod-backed read/write flows. Signals DOM render API and Effect gains timeout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant FrameProducer as Frame.Producer
participant Track as Moq.TrackProducer
participant Group as Moq.GroupProducer
App->>FrameProducer: encode(data, ts, keyframe)
alt keyframe
FrameProducer->>Track: close current group (if any)
FrameProducer->>Track: open new group
end
FrameProducer->>Group: writeFrame(encode(data, ts))
note right of Group: Timestamp varint + payload
sequenceDiagram
autonumber
participant Publish as Chat.Publish
participant Message as Chat.Message
participant Typing as Chat.Typing
participant Broadcast as Moq.BroadcastProducer
Publish->>Message: new(broadcast, props.message)
Publish->>Typing: new(broadcast, props.typing)
rect rgba(200,235,255,0.3)
Message->>Broadcast: insert track (enabled)
Typing->>Broadcast: insert track (enabled)
Message->>Broadcast: writeString(latest)
Typing->>Broadcast: writeBool(active)
end
Publish-->>Publish: compute catalog { message, typing }
sequenceDiagram
autonumber
participant Watch as Preview
participant Catalog
participant Broadcast as Moq.BroadcastConsumer
participant Track as Moq.TrackConsumer
participant Z as Zod
Watch->>Catalog: get preview track
alt track available
Watch->>Broadcast: subscribe(name, priority)
Broadcast-->>Track: TrackConsumer
Watch->>Z: read(Track, InfoSchema)
Z-->>Watch: Info | undefined
Watch-->>Watch: set preview signal
else no track
Watch-->>Watch: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
|
Claude finished @kixelated's task —— View job Code Review Complete 👍Reviewing the "Revamp chat typing and audio speaking" PR for bugs and security issues. Todo List:
✅ No significant bugs or security issues found. The PR shows a solid architectural refactoring with proper security practices: Security Analysis:
Code Quality:
Architecture:
The refactoring maintains security best practices while improving code organization. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
js/hang/src/preview/member.ts (1)
44-47: Don’t close externally-owned broadcast in close().Member didn’t create the BroadcastConsumer; closing it here can tear down other consumers.
close() { - this.signals.close(); - this.broadcast.close(); + this.signals.close(); }js/moq/README.md (1)
69-71: Update README.md subscribe example and remove hard tabs
- Verified that
BroadcastConsumer.subscribe(track: string, priority: number): TrackConsumeris a synchronous API. The README example should dropawaitand pass a priority argument (e.g.0).- Lines 73–83 in the code block use hard tabs (markdownlint MD010). Replace all tabs with spaces in this snippet (or disable MD010 for the block).
File: js/moq/README.md (L69–71, L73–83)
Proposed diff:
- // Subscribe to a specific track - const track = await broadcast.subscribe("chat"); + // Subscribe to a specific track + const track = broadcast.subscribe("chat", 0);Be sure to convert any remaining tabs in the snippet to spaces.
js/hang/src/watch/location.ts (1)
86-105: Per-frame parse failures currently abort the entire consumer loop.A single invalid frame throws and exits; skip bad frames instead.
Apply:
async function runConsumer( consumer: Moq.TrackConsumer, location: Signal<Catalog.Position | undefined>, cancel: Promise<void>, ) { try { for (;;) { - const position = await Promise.race([Zod.read(consumer, Catalog.PositionSchema), cancel]); - if (!position) break; + let position: Catalog.Position | undefined; + try { + position = await Promise.race([Zod.read(consumer, Catalog.PositionSchema), cancel]); + } catch (e) { + console.warn("invalid location frame; skipping", e); + continue; + } + if (position === undefined) break; location.set(position); } location.set(undefined); } catch (err) {js/moq-clock/src/main.ts (1)
1-204: Update remainingnextFramereferences toreadFrameThe repository-wide rename of
nextFrame→readFrameis not yet complete. Please update the following locations:
- js/hang/src/frame.ts (around line 62)
// Before const next = await this.#track.nextFrame(); // After const next = await this.#track.readFrame();- js/moq/src/track.ts
• Lines 185–187: update comment and helper reference• Lines 197–201: the// Before // Update the state needed for the nextFrame() helper. // After // Update the state needed for the readFrame() helper.nextFrame()method definition remains; either rename it toreadFrame()(and adjust callers) or remove it if deprecated.- js/moq/src/track.ts (around line 235)
// Before const next = await this.nextFrame(); // After const next = await this.readFrame();- js/hang/src/watch/video/index.ts (around line 114)
// Before const next = await Promise.race([sub.nextFrame(), cancel]); // After const next = await Promise.race([sub.readFrame(), cancel]);After these edits, rerun a global search for
nextFrame(to confirm no remaining references.js/moq/src/util/watch.ts (2)
41-45: FinalizationRegistry: guard usage to avoid crashes in environments that lack it.Register only when the registry exists; otherwise older runtimes can throw at module load.
Apply this diff:
- if (DEV) { + if (DEV && WatchProducer.finalizer) { const debugInfo = new Error("Created here").stack ?? "No stack"; - WatchProducer.finalizer.register(this, debugInfo, this); + WatchProducer.finalizer.register(this, debugInfo, this); }Additionally, update the static definition (outside this hunk) to make the registry optional:
// near line 33 private static finalizer?: FinalizationRegistry<string> = typeof FinalizationRegistry !== "undefined" ? new FinalizationRegistry<string>((debugInfo) => { console.warn(`WatchProducer was garbage collected without being closed:\n${debugInfo}`); }) : undefined;
143-147: Consumer finalizer: guard register and make registry optional.- if (DEV) { + if (DEV && WatchConsumer.finalizer) { const debugInfo = new Error("Created here").stack ?? "No stack"; - WatchConsumer.finalizer.register(this, debugInfo, this); + WatchConsumer.finalizer.register(this, debugInfo, this); }Also mirror the producer’s optional static definition (outside this hunk):
// near the WatchConsumer.finalizer declaration private static finalizer?: FinalizationRegistry<string> = typeof FinalizationRegistry !== "undefined" ? new FinalizationRegistry<string>((debugInfo) => { console.warn(`WatchConsumer was garbage collected without being closed:\n${debugInfo}`); }) : undefined;
🧹 Nitpick comments (33)
js/hang/src/watch/audio/speaking.ts (1)
47-55: Good migration to direct readBool; add lightweight error guard in loopLoop + cancel handling looks correct. To avoid noisy effect-level error logs if sub.readBool() rejects, guard locally.
effect.spawn(async (cancel) => { for (;;) { - const speaking = await Promise.race([sub.readBool(), cancel]); + let speaking: boolean | undefined; + try { + speaking = await Promise.race([sub.readBool(), cancel]); + } catch (e) { + console.warn("speaking read error", e); + break; + } if (speaking === undefined) break; this.#active.set(speaking); } });js/hang/src/publish/location.ts (1)
96-127: Sanitize handle before embedding in track nameIf handle can be user-provided, path injection or invalid names are possible (slashes, .., control chars). Sanitize or constrain to a safe charset.
- const track = new Moq.TrackProducer(`peer/${handle}/location.json`, 0); + const safe = handle.replace(/[^a-zA-Z0-9._-]/g, "_"); + const track = new Moq.TrackProducer(`peer/${safe}/location.json`, 0);If a stricter policy is desired, validate and bail out instead of coercing.
Would you like me to add a small helper (e.g., to utils/path.ts) and unit tests to centralize this sanitization?
js/signals/src/dom.ts (1)
111-121: setClass is fine; optional: support SVGElementIf you plan to call setClass on SVG elements, broaden the element type to Element & { classList: DOMTokenList }.
-export function setClass(effect: Effect, element: HTMLElement, ...classNames: string[]) { +export function setClass(effect: Effect, element: Element & { classList: DOMTokenList }, ...classNames: string[]) {js/hang/src/publish/video/index.ts (1)
138-140: Decouple keyframe logic and note writeFrame is synchronousI looked into the
GroupProducerimplementation injs/moq/src/group.ts—itswriteFrame(frame: Uint8Array)method is a plain, synchronous call that returnsvoid(no promise) and simply appends the frame to an in-memory buffer. This means there’s no built-in backpressure or async boundary to await—if your pipeline needs to throttle or block on downstream consumers, it must do so before or after this call.Locations of interest:
js/hang/src/publish/video/index.tsat lines 138–140js/hang/src/publish/audio/index.tsat lines 208–209Suggested optional refactor to embed keyframe metadata (requires extending
Frame.encodeAPI):- const buffer = Frame.encode(frame, frame.timestamp); + // Propagate keyFrame flag so consumers needn’t assume object ID 0 is always a keyframe + const buffer = Frame.encode(frame, frame.timestamp /*, { keyFrame: frame.isKey } */); group?.writeFrame(buffer);Note:
GroupProducer.writeFrameis synchronous (no promise), so backpressure won’t be enforced here—ensure upstream write logic handles pacing if needed.js/hang/src/watch/video/index.ts (1)
117-123: Align VideoTrackConsumer API with AudioTrackConsumer
Video currently usesnextFrame(), whereas audio and catalog consumers usereadFrame(). For consistency and future maintainability, consider unifying these method names or clearly documenting the distinction:• Video consumer calls
– js/hang/src/frame.ts:62 →this.#track.nextFrame()
– js/hang/src/watch/video/index.ts:114 →sub.nextFrame()
• Audio consumer calls
– js/hang/src/watch/audio/index.ts:153 →sub.readFrame()
• Catalog uses
– js/hang/src/catalog/root.ts:42 →track.readFrame()Options:
- Rename (or alias)
nextFrame()toreadFrame()in the VideoTrackConsumer so all tracks share the same API surface.- If keeping both, add a note in the video code explaining why it uses
nextFrame()and thatframe === 0denotes a key frame (e.g.// key frame when frameId === 0).- There’s no
readObject()yet—if you’d like to expose both frame ID and payload in one call in the future, you could introduce areadObject()method.js/hang/src/publish/audio/captions.ts (2)
69-75: Rename misleading log label ("VAD worker error").This worker is captions/transcription; the log label is confusing.
Apply this change:
- console.error("VAD worker error:", data.message); + console.error("Captions worker error:", data.message);
131-134: Close the track on teardown to avoid resource leaks.
Captions.close()doesn't close#track, unlike Detection which does. Add a close to ensure underlying resources are released when the component is disposed.Apply this diff:
close() { - this.signals.close(); + this.signals.close(); + this.#track.close(); }js/hang/src/publish/video/detection.ts (1)
36-36: Avoid double-closing the track.
signals.cleanup(() => this.#track.close())plusthis.#track.close()inclose()can callclose()twice. Make one the single source of truth (prefer relying onsignals.cleanup).Apply this diff:
close() { this.signals.close(); - this.#track.close(); }Also applies to: 86-89
js/hang/src/watch/audio/captions.ts (1)
51-52: Optional: normalize empty caption toundefined.If the UI treats “no caption” as absence rather than empty string, normalize here.
- this.#text.set(result); + this.#text.set(result === "" ? undefined : result);Confirm whether the UI expects empty string or
undefinedto clear captions; adjust accordingly.js/hang/src/preview/member.ts (2)
29-39: Add error handling around Zod.read loop to avoid unhandled rejections.Mirror watch/preview.ts and log parse failures.
- effect.spawn(async (cancel) => { - try { - for (;;) { - const frame = await Promise.race([Zod.read(track, Preview.InfoSchema), cancel]); - if (!frame) break; - - this.info.set(frame); - } - } finally { - this.info.set(undefined); - } - }); + effect.spawn(async (cancel) => { + try { + for (;;) { + const info = await Promise.race([Zod.read(track, Preview.InfoSchema), cancel]); + if (!info) break; + this.info.set(info); + } + } catch (error) { + console.warn("Failed to parse preview JSON:", error); + } finally { + this.info.set(undefined); + } + });
15-22: Make Effect instance private for consistency with other classes.Other modules use #signals; align naming and encapsulation.
- signals = new Effect(); + #signals = new Effect(); @@ - this.signals.effect((effect) => { + this.#signals.effect((effect) => { @@ - this.signals.close(); + this.#signals.close();Also applies to: 45-45
js/hang/src/watch/video/detection.ts (1)
48-53: Add try/catch around Zod.read to surface parse errors.Aligns with preview/watch handling and avoids silent task rejection.
- effect.spawn(async (cancel) => { - for (;;) { - const frame = await Promise.race([Zod.read(track, Catalog.DetectionObjectsSchema), cancel]); - if (!frame) break; - - // Use a function to avoid the dequal check. - this.objects.set(() => frame); - } - }); + effect.spawn(async (cancel) => { + try { + for (;;) { + const frame = await Promise.race([Zod.read(track, Catalog.DetectionObjectsSchema), cancel]); + if (!frame) break; + // Use a function to avoid the dequal check. + this.objects.set(() => frame); + } + } catch (error) { + console.warn("Failed to parse detection JSON:", error); + } + });js/hang/src/publish/preview.ts (1)
1-4: Validate and encode with Zod to catch malformed preview payloads.Align publisher with watch-side Zod.read by validating before write.
Apply:
-import * as Moq from "@kixelated/moq"; +import * as Moq from "@kixelated/moq"; +import { write as ZodWrite } from "@kixelated/moq/zod"; +import { InfoSchema } from "../preview"; - #publish(preview: Info) { - this.#track.writeJson(preview); - } + #publish(preview: Info) { + ZodWrite(this.#track, preview, InfoSchema); + }Also applies to: 41-43
js/hang/src/watch/location.ts (1)
42-51: Race noted in TODO: initialize from catalog.initial only once per stream.Consider moving initial assignment into the consumer start (emit initial before first update) to avoid stale overwrites when catalogs churn.
js/hang/src/watch/chat/index.ts (2)
16-18: Expose a readonly catalog getter for consistency and reuse.Message/Typing expose catalog; Chat should too.
Apply:
-import { Effect, Signal } from "@kixelated/signals"; +import { Effect, type Getter, Signal } from "@kixelated/signals"; @@ - #catalog = new Signal<Catalog.Chat | undefined>(undefined); + #catalog = new Signal<Catalog.Chat | undefined>(undefined); + readonly catalog: Getter<Catalog.Chat | undefined> = this.#catalog;
27-37: Minor: avoid churn by no-op setting when both tracks absent.Current effect returns early when both undefined (good); consider also clearing catalog explicitly to undefined for clarity (functional behavior already achieved via Effect.set cleanup).
js/hang/src/watch/chat/typing.ts (1)
5-9: Tweak comment: this class manages typing state, not “downloading the chat.”Minor doc nit to avoid confusion.
export interface TypingProps { - // Whether to start downloading the chat. - // Defaults to false so you can make sure everything is ready before starting. + // Whether to start watching the typing indicator. + // Defaults to false so you can make sure everything is ready before starting. enabled?: boolean; }js/moq-clock/src/main.ts (2)
163-170: Use readString() for the base frame too.You write the base with writeString; read it with readString for consistency and less boilerplate.
- const baseFrame = await group.readFrame(); - if (!baseFrame) { + const base = await group.readString(); + if (!base) { console.warn("❌ No base frame found"); continue; } - - const base = new TextDecoder().decode(baseFrame);
173-185: Guard against invalid second frames.parseInt can produce NaN if a bad frame slips through. Cheap validation avoids downstream weirdness.
- const seconds = parseInt(frame, 10); + const seconds = parseInt(frame, 10); + if (Number.isNaN(seconds)) { + console.warn("⚠️ Invalid seconds frame:", frame); + continue; + }js/hang/src/publish/chat/index.ts (1)
25-33: Only publish a catalog object when there’s something to publish.Currently you always set { message, typing } even if both are undefined. Align with the watch-side behavior and reduce noise.
this.#signals.effect((effect) => { const message = effect.get(this.message.catalog); const typing = effect.get(this.typing.catalog); - - this.#catalog.set({ - message, - typing, - }); + const next = message || typing ? { message, typing } : undefined; + this.#catalog.set(next); });js/moq/src/group.ts (3)
33-39: Avoid per-call TextEncoder allocations.Instantiate once and reuse to reduce GC pressure on hot paths.
- writeString(str: string) { - this.writeFrame(new TextEncoder().encode(str)); - } + writeString(str: string) { + this.writeFrame(TE.encode(str)); + }Add at top-level (outside the selected range):
// Reuse encoders/decoders to avoid allocations const TE = new TextEncoder(); const TD = new TextDecoder();
106-114: Same for readString/readJson; reuse a single TextDecoder.- async readString(): Promise<string | undefined> { - const frame = await this.readFrame(); - return frame ? new TextDecoder().decode(frame) : undefined; - } + async readString(): Promise<string | undefined> { + const frame = await this.readFrame(); + return frame ? TD.decode(frame) : undefined; + }
116-119: Guard zero-length frames in readBool.Defensive check avoids false negatives if a producer sends an empty frame.
- async readBool(): Promise<boolean | undefined> { - const frame = await this.readFrame(); - return frame ? frame[0] === 1 : undefined; - } + async readBool(): Promise<boolean | undefined> { + const frame = await this.readFrame(); + if (!frame || frame.length === 0) return undefined; + return frame[0] === 1; + }js/hang/src/publish/chat/typing.ts (1)
29-40: Unset catalog on disable to avoid stale metadataWithout clearing
catalog, watchers may keep subscribing to a removed track afterenabledflips false. Add a cleanup to set it back toundefined.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/watch/chat/message.ts (1)
61-63: Align code with comment (avoid dequal check)The comment says to avoid the dequal check, but the code doesn’t. Use a function setter.
- // Use a function to avoid the dequal check. - this.#latest.set(frame); + // Use a function to avoid the dequal check. + this.#latest.set(() => frame);js/hang/src/publish/chat/message.ts (2)
29-37: Unset catalog on disable to avoid stale metadataMirror the Typing publisher: clear the catalog on cleanup so watchers don’t chase a removed track.
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));
39-45: Remove redundant nullish coalescing
latestisSignal<string>; it’s never undefined here.const latest = effect.get(this.latest); - this.#track.writeString(latest ?? ""); + this.#track.writeString(latest);js/hang/src/frame.ts (4)
8-17: Simplify encode buffer sizing and slicingAvoid redundant ternaries and use
byteLengthdirectly for both variants.-export function encode(source: Uint8Array | Source, timestamp: number): Uint8Array { - const data = new Uint8Array(8 + (source instanceof Uint8Array ? source.byteLength : source.byteLength)); - const size = setVint53(data, timestamp).byteLength; - if (source instanceof Uint8Array) { - data.set(source, size); - } else { - source.copyTo(data.subarray(size)); - } - return data.subarray(0, (source instanceof Uint8Array ? source.byteLength : source.byteLength) + size); -} +export function encode(source: Uint8Array | Source, timestamp: number): Uint8Array { + const len = source.byteLength; + const data = new Uint8Array(8 + len); + const size = setVint53(data, timestamp).byteLength; + if (source instanceof Uint8Array) { + data.set(source, size); + } else { + source.copyTo(data.subarray(size)); + } + return data.subarray(0, len + size); +}
84-88: Use unsigned read for 2-byte varint branch
getUint16is clearer/safer thangetInt16here.- v = view.getInt16(0) & 0x3fff; + v = view.getUint16(0) & 0x3fff;
48-51: Close group before trackClose the active group first to flush/cleanup before closing the parent track.
- this.#track.close(); - this.#group?.close(); + this.#group?.close(); + this.#track.close();
41-42: Non-null assertion clarifies invariantsAfter the keyframe/non-keyframe checks,
#groupmust exist. Optional chaining hides real failures.- this.#group?.writeFrame(encode(data, timestamp)); + this.#group!.writeFrame(encode(data, timestamp));js/moq/src/util/watch.ts (1)
22-22: Rename looks good; improve DEV detection and drop ts-ignore.Avoid the ts-ignore and support both Vite and Node by safely checking
import.metaandprocess.env.Apply this diff:
-// @ts-ignore - Some environments don't recognize import.meta.env -const DEV = typeof import.meta.env !== "undefined" && import.meta.env?.MODE !== "production"; +const DEV = + (typeof import.meta !== "undefined" && (import.meta as any).env?.MODE !== "production") || + (typeof process !== "undefined" && process.env?.NODE_ENV !== "production");js/moq/package.json (1)
25-27: Peer dep on Zod v4: confirm necessity and consider optional peer to avoid noise for non-users.Zod 4 is stable; the range
^4.0.0is appropriate. If only consumers of@kixelated/moq/zodneed Zod, mark it optional to prevent peer warnings for others."peerDependencies": { "zod": "^4.0.0" }, + "peerDependenciesMeta": { + "zod": { + "optional": true + } + },Reference: Zod 4 is stable and introduces new APIs like
z.strictObject, which this repo is beginning to use. (zod.dev, v4.zod.dev)
📜 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 ignored due to path filters (2)
js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlrs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
js/hang/src/catalog/chat.ts(1 hunks)js/hang/src/catalog/root.ts(1 hunks)js/hang/src/container/bool.ts(0 hunks)js/hang/src/container/frame.ts(0 hunks)js/hang/src/container/index.ts(0 hunks)js/hang/src/container/position.ts(0 hunks)js/hang/src/container/vint.ts(0 hunks)js/hang/src/frame.ts(1 hunks)js/hang/src/index.ts(1 hunks)js/hang/src/preview/info.ts(1 hunks)js/hang/src/preview/member.ts(2 hunks)js/hang/src/publish/audio/captions.ts(1 hunks)js/hang/src/publish/audio/index.ts(2 hunks)js/hang/src/publish/audio/speaking.ts(3 hunks)js/hang/src/publish/chat.ts(0 hunks)js/hang/src/publish/chat/index.ts(1 hunks)js/hang/src/publish/chat/message.ts(1 hunks)js/hang/src/publish/chat/typing.ts(1 hunks)js/hang/src/publish/location.ts(3 hunks)js/hang/src/publish/preview.ts(1 hunks)js/hang/src/publish/video/detection.ts(1 hunks)js/hang/src/publish/video/index.ts(2 hunks)js/hang/src/watch/audio/captions.ts(1 hunks)js/hang/src/watch/audio/index.ts(2 hunks)js/hang/src/watch/audio/speaking.ts(1 hunks)js/hang/src/watch/broadcast.ts(3 hunks)js/hang/src/watch/chat.ts(0 hunks)js/hang/src/watch/chat/index.ts(1 hunks)js/hang/src/watch/chat/message.ts(1 hunks)js/hang/src/watch/chat/typing.ts(1 hunks)js/hang/src/watch/location.ts(4 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/moq-clock/src/main.ts(4 hunks)js/moq/README.md(2 hunks)js/moq/package.json(2 hunks)js/moq/src/group.ts(2 hunks)js/moq/src/ietf/publisher.ts(1 hunks)js/moq/src/lite/publisher.ts(1 hunks)js/moq/src/track.test.ts(1 hunks)js/moq/src/track.ts(4 hunks)js/moq/src/util/watch.ts(6 hunks)js/moq/src/zod.ts(1 hunks)js/signals/src/dom.ts(1 hunks)js/signals/src/index.ts(13 hunks)rs/justfile(0 hunks)
💤 Files with no reviewable changes (8)
- js/hang/src/container/index.ts
- js/hang/src/container/frame.ts
- js/hang/src/container/vint.ts
- js/hang/src/watch/chat.ts
- js/hang/src/container/position.ts
- rs/justfile
- js/hang/src/container/bool.ts
- js/hang/src/publish/chat.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/catalog/chat.tsjs/hang/src/preview/info.tsjs/hang/src/index.tsjs/hang/src/watch/audio/index.tsjs/hang/src/catalog/root.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/video/detection.tsjs/hang/src/watch/audio/captions.tsjs/hang/src/watch/audio/speaking.tsjs/hang/src/watch/video/index.tsjs/hang/src/watch/chat/index.tsjs/hang/src/publish/video/index.tsjs/hang/src/watch/broadcast.tsjs/hang/src/preview/member.tsjs/hang/src/publish/chat/typing.tsjs/hang/src/watch/location.tsjs/hang/src/publish/location.tsjs/hang/src/publish/audio/index.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/publish/chat/message.tsjs/hang/src/frame.tsjs/hang/src/watch/chat/message.tsjs/hang/src/watch/chat/typing.tsjs/hang/src/watch/video/detection.tsjs/hang/src/publish/chat/index.tsjs/hang/src/watch/preview.tsjs/hang/src/publish/preview.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/catalog/chat.tsjs/hang/src/preview/info.tsjs/hang/src/index.tsjs/hang/src/watch/audio/index.tsjs/hang/src/catalog/root.tsjs/hang/src/publish/audio/captions.tsjs/hang/src/publish/video/detection.tsjs/moq/src/util/watch.tsjs/hang/src/watch/audio/captions.tsjs/moq/package.jsonjs/moq/src/lite/publisher.tsjs/moq/src/group.tsjs/hang/src/watch/audio/speaking.tsjs/moq/src/ietf/publisher.tsjs/hang/src/watch/video/index.tsjs/moq/src/zod.tsjs/hang/src/watch/chat/index.tsjs/moq/README.mdjs/hang/src/publish/video/index.tsjs/hang/src/watch/broadcast.tsjs/moq/src/track.tsjs/hang/src/preview/member.tsjs/hang/src/publish/chat/typing.tsjs/hang/src/watch/location.tsjs/hang/src/publish/location.tsjs/signals/src/dom.tsjs/hang/src/publish/audio/index.tsjs/signals/src/index.tsjs/hang/src/publish/audio/speaking.tsjs/hang/src/publish/chat/message.tsjs/moq-clock/src/main.tsjs/hang/src/frame.tsjs/hang/src/watch/chat/message.tsjs/hang/src/watch/chat/typing.tsjs/moq/src/track.test.tsjs/hang/src/watch/video/detection.tsjs/hang/src/publish/chat/index.tsjs/hang/src/watch/preview.tsjs/hang/src/publish/preview.ts
🧬 Code graph analysis (24)
js/hang/src/catalog/chat.ts (1)
js/hang/src/catalog/track.ts (1)
TrackSchema(4-7)
js/hang/src/watch/audio/index.ts (1)
js/moq/src/lite/publisher.ts (2)
sub(172-194)sub(203-226)
js/hang/src/watch/audio/captions.ts (1)
js/moq/src/lite/publisher.ts (2)
sub(172-194)sub(203-226)
js/moq/src/lite/publisher.ts (2)
js/moq/src/ietf/connection.ts (1)
stream(246-255)js/moq/src/lite/connection.ts (3)
stream(125-140)stream(142-158)stream(179-187)
js/hang/src/watch/audio/speaking.ts (1)
js/moq/src/lite/publisher.ts (2)
sub(172-194)sub(203-226)
js/hang/src/watch/video/index.ts (2)
js/moq/src/ietf/object.ts (1)
Frame(49-96)js/moq/src/stream.ts (1)
next(412-416)
js/moq/src/zod.ts (2)
js/moq/src/track.ts (2)
TrackConsumer(143-322)TrackProducer(9-136)js/moq/src/group.ts (2)
GroupConsumer(78-147)GroupProducer(8-71)
js/hang/src/watch/chat/index.ts (3)
js/hang/src/watch/chat/message.ts (2)
MessageProps(5-9)Message(11-70)js/hang/src/watch/chat/typing.ts (2)
TypingProps(5-9)Typing(11-66)js/signals/src/index.ts (2)
Signal(19-76)Effect(81-433)
js/hang/src/publish/video/index.ts (1)
js/moq/src/ietf/object.ts (1)
Frame(49-96)
js/hang/src/watch/broadcast.ts (1)
js/hang/src/watch/preview.ts (1)
Preview(11-54)
js/moq/src/track.ts (2)
rs/hang/src/catalog/root.rs (1)
next(210-230)rs/hang/src/model/location.rs (1)
next(40-60)
js/hang/src/preview/member.ts (2)
js/hang/src/publish/preview.ts (1)
Preview(10-48)js/hang/src/watch/preview.ts (1)
Preview(11-54)
js/hang/src/publish/chat/typing.ts (4)
js/hang/src/watch/chat/typing.ts (2)
TypingProps(5-9)Typing(11-66)js/moq/src/broadcast.ts (1)
BroadcastProducer(17-113)js/signals/src/index.ts (2)
Signal(19-76)Effect(81-433)js/hang/src/catalog/track.ts (1)
Track(9-9)
js/hang/src/watch/location.ts (2)
js/moq/src/track.ts (1)
TrackConsumer(143-322)js/hang/src/catalog/location.ts (1)
Position(42-42)
js/hang/src/publish/location.ts (2)
js/hang/src/watch/location.ts (2)
LocationPeer(107-158)effect(140-153)js/moq/src/track.ts (1)
TrackProducer(9-136)
js/signals/src/dom.ts (1)
js/signals/src/index.ts (2)
effect(294-304)Effect(81-433)
js/hang/src/publish/audio/speaking.ts (3)
js/hang/src/publish/audio/captions.ts (1)
effect(39-129)js/hang/src/publish/video/index.ts (3)
effect(91-103)effect(105-183)effect(349-387)js/hang/src/watch/audio/speaking.ts (1)
effect(32-56)
js/hang/src/publish/chat/message.ts (3)
js/hang/src/watch/chat/message.ts (2)
MessageProps(5-9)Message(11-70)js/moq/src/broadcast.ts (1)
BroadcastProducer(17-113)js/signals/src/index.ts (2)
Signal(19-76)Effect(81-433)
js/hang/src/watch/chat/message.ts (7)
js/hang/src/publish/chat/message.ts (2)
MessageProps(6-8)Message(10-52)js/signals/src/index.ts (3)
Signal(19-76)Getter(10-13)Effect(81-433)js/moq/src/broadcast.ts (1)
BroadcastConsumer(122-180)js/hang/src/catalog/track.ts (1)
Track(9-9)js/hang/src/watch/broadcast.ts (3)
catalog(146-163)effect(81-117)effect(119-130)js/hang/src/watch/audio/captions.ts (1)
effect(32-55)js/hang/src/watch/audio/speaking.ts (1)
effect(32-56)
js/hang/src/watch/chat/typing.ts (6)
js/hang/src/publish/chat/typing.ts (2)
TypingProps(6-8)Typing(10-55)js/signals/src/index.ts (3)
Signal(19-76)Getter(10-13)Effect(81-433)js/moq/src/broadcast.ts (1)
BroadcastConsumer(122-180)js/hang/src/watch/broadcast.ts (3)
catalog(146-163)effect(81-117)effect(119-130)js/hang/src/watch/audio/captions.ts (1)
effect(32-55)js/hang/src/watch/audio/speaking.ts (1)
effect(32-56)
js/hang/src/watch/video/detection.ts (1)
js/hang/src/publish/video/detection-worker.ts (1)
frame(35-86)
js/hang/src/publish/chat/index.ts (4)
js/hang/src/watch/chat/index.ts (2)
ChatProps(7-10)Chat(12-45)js/hang/src/publish/chat/message.ts (2)
MessageProps(6-8)Message(10-52)js/hang/src/publish/chat/typing.ts (2)
TypingProps(6-8)Typing(10-55)js/hang/src/catalog/chat.ts (1)
Chat(9-9)
js/hang/src/watch/preview.ts (4)
js/hang/src/publish/preview.ts (2)
PreviewProps(5-8)Preview(10-48)js/moq/src/broadcast.ts (1)
BroadcastConsumer(122-180)js/hang/src/preview/info.ts (2)
Info(13-13)InfoSchema(3-11)js/hang/src/connection.ts (1)
effect(56-98)
js/hang/src/publish/preview.ts (2)
js/hang/src/meet/room.ts (1)
preview(47-49)js/hang/src/preview/info.ts (1)
Info(13-13)
🪛 markdownlint-cli2 (0.17.2)
js/moq/README.md
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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 (35)
js/signals/src/index.ts (2)
8-8: LGTM: Consistent environment flag standardizationThe rename from
devtoDEVmaintains consistent development-mode gating behavior while aligning with the broader codebase standardization effort. All conditional checks have been properly updated.Also applies to: 100-100, 107-107, 145-145, 185-185, 207-207, 227-227, 240-240, 257-257, 281-281, 296-296, 309-309, 390-390, 403-403, 429-429
254-277: LGTM: Well-designed timeout method with proper cleanupThe new
timeoutmethod provides a clean API for running effects with automatic cleanup after a delay. The implementation correctly:
- Creates a nested effect for the provided function
- Schedules cleanup via
setTimeout- Ensures proper resource cleanup on both timeout and parent effect disposal
- Guards against usage on closed effects with appropriate DEV warnings
The method complements the existing
timermethod by focusing on effect lifecycle management rather than simple function execution.js/moq/src/track.ts (4)
69-73: LGTM: Consistent method namingThe rename from
appendFrametowriteFrameimproves API consistency with the write-oriented naming pattern used throughout the codebase (e.g.,writeString,writeJson,writeBool).
75-91: LGTM: Useful convenience methodsThe new
writeString,writeJson, andwriteBoolmethods provide convenient wrappers that follow a consistent pattern: create a group, delegate to the group's write method, then close the group. This aligns well with the Frame-based I/O approach mentioned in the AI summary.
189-189: LGTM: Consistent frame reading methodThe internal calls now use
readFrame()instead ofnextFrame()for frame advancement, which aligns with the new read-oriented API naming convention.Also applies to: 210-210, 230-230
234-258: LGTM: Comprehensive read helper methodsThe new read methods provide a clean, type-safe API for consuming different data types:
readFrame(): Returns raw bytesreadString(): Uses TextDecoder for proper string conversionreadJson(): Handles JSON parsing with proper error propagationreadBool(): Validates single-byte boolean format (0 or 1) with appropriate error handlingThe boolean validation is particularly well-implemented, ensuring data integrity by throwing an error for invalid boolean frames.
js/hang/src/publish/audio/speaking.ts (1)
24-53: Direct TrackProducer usage is clean and consistentNice removal of the Bool wrapper, correct use of insert/remove and writeBool, and proper lifecycle cleanup.
Also applies to: 113-116
js/hang/src/publish/location.ts (2)
25-65: Location updates via writeJson: looks goodTrack lifecycle, catalog publication (with cleanup back to undefined), and writeJson on position changes all look correct.
83-88: Producer type change ripple-check completeA full-text search for
producer.updateandproducer.write(Json|JSON|Position)across all.tsfiles yielded no occurrences, confirming that downstream code no longer calls the removed methods and is using the newSignal<Moq.TrackProducer>API as intended. No further changes are required here.js/moq/src/lite/publisher.ts (1)
210-216: readFrame swap is correct and matches new APIGraceful break on undefined and race with stream.closed() mirrors the rest of the pipeline.
js/hang/src/publish/video/index.ts (1)
5-5: Switch to Frame module import looks correct.
Matches the PR’s direction to centralize framing under js/hang/src/frame.ts and keeps media logic within the hang layer as per guidelines.js/moq/src/ietf/publisher.ts (1)
162-169: Good migration to readFrame(); I/O loop remains robust.
Using group.readFrame() aligns with the new typed read helpers and preserves end-of-group signaling via undefined payload.js/hang/src/index.ts (1)
7-7: No migration shim needed — Frame export is additive and non-breaking
A ripgrep scan for any remainingContainer.*usages injs/(excludingdist/build) returned no results, confirming there are no existing calls toContainer.encodeFrameorContainer.decodeFrame. ExportingFramesimply adds a new surface—it doesn’t remove or alter any existing API. Accordingly, the suggested temporary shim isn’t necessary. Feel free to proceed without the shim, and document the newFrameexport in the release notes as an additive enhancement.js/hang/src/catalog/root.ts (1)
42-45: readFrame() + direct decode is correct and tighter.
This matches the new read* helpers and avoids the extra .data unwrap.js/hang/src/watch/video/index.ts (1)
4-4: Import switch to Frame is aligned with the new API.
Keeps media logic in the hang layer and matches other modules.js/hang/src/publish/audio/captions.ts (1)
59-59: Good migration to typed I/O (writeString).Direct string write removes needless TextEncoder work and matches the new API.
js/hang/src/publish/video/detection.ts (1)
76-76: Switch towriteJson(result)looks good.Cleaner and allows typed consumption on the watch side.
js/moq/src/track.test.ts (1)
94-109: LGTM: tests updated toreadFrame()API.Covers cloned group readers and independent consumption semantics correctly.
js/hang/src/publish/audio/index.ts (1)
5-5: Frame module migration looks correct.
Frame.encode(frame, frame.timestamp)and subsequentwriteFramealign with the new framing path.Also applies to: 208-210
js/hang/src/watch/audio/index.ts (2)
153-157: API migration to readFrame + Frame.decode looks correct.Matches the new Frame-based I/O path and moq-lite publisher usage.
156-162: ✅ Confirmed timestamp units are in microseconds
- In js/hang/src/frame.ts,
Frame.decodesimply returns the number that was originally passed toFrame.encode(viagetVint53)—there’s no hidden unit conversion there.- In js/hang/src/publish/audio/index.ts, the raw sample-count timestamp from the AudioWorklet is explicitly converted to microseconds by
before being fed intotimestamp: (1_000_000 * data.timestamp) / worklet.context.sampleRateFrame.encode.- In the video pipeline (js/hang/src/publish/video/index.ts), the WebCodecs-provided
VideoFrame.timestamp(already in µs) is passed directly toFrame.encode.No mismatches detected—the decoded timestamps match WebCodecs’ expected microsecond units.
js/moq/README.md (1)
51-57: Docs update to writeString is clear and aligned with new helpers.js/hang/src/preview/info.ts (2)
10-11: Adding typing?: boolean is fine and backward compatible.
3-14: No lingering PreviewSchema references found – removal is safeI’ve searched the entire repository for any occurrences of
PreviewSchema(both withinjs/and across all files) and found none. You can safely finalize the removal of thePreviewSchemaalias without impacting the build.js/hang/src/watch/video/detection.ts (1)
2-2: Zod-based read integration looks good.js/hang/src/publish/preview.ts (1)
23-29: Track lifecycle insertion/removal is correct.Insert on enable, remove on cleanup. Close() will trigger removal via Effect cleanup.
js/hang/src/watch/location.ts (1)
62-73: Good switch to TrackConsumer + Zod.read with proper cleanup.Subscription, spawn, and teardown are wired correctly.
js/hang/src/watch/broadcast.ts (1)
8-9: PreviewWatch→Preview rename integration looks correct.Imports, field type, construction, and close path all align with the new Preview API.
Also applies to: 47-48, 70-71
js/hang/src/watch/chat/index.ts (1)
24-26: Decomposition into Message/Typing subcomponents is clean.Composition matches publish-side structure and keeps responsibilities isolated.
js/hang/src/watch/preview.ts (1)
32-44: Nice simplification with Zod.read and direct subscribe to preview.json.Cleaner and safer than manual JSON.parse, and it decouples preview bootstrap from the catalog nicely.
js/moq-clock/src/main.ts (1)
97-107: Good adoption of typed I/O helpers (writeString/readString/readFrame).The publish/subscribe paths align with the new Group/Track APIs and keep the example readable.
Also applies to: 162-179
js/hang/src/publish/chat/index.ts (1)
12-24: Solid composition and catalog merge.Clear separation of concerns and mirrors the watch-side structure neatly.
js/moq/src/group.ts (1)
101-104: Rename looks good; behavior preserved.readFrame maintains the same semantics as the old nextFrame, with a clearer name.
js/hang/src/catalog/chat.ts (1)
5-6: No staleChatSchema.trackreferences detected; change is safe to mergeI searched for any occurrences of a single
trackfield inChatSchemaor related imports and found none. All remaining.trackusages are in thewatch/*modules (video and audio), which are unrelated to the chat schema.• Ran targeted ripgrep for
catalog.chat.trackandChatSchema.*track– no matches
• Broad.tracksearch underjs/hang/srconly surfacedwatch/videoandwatch/audiousagesThe
z.strictObjectrefactor onChatSchemacan be applied confidently without breaking existing code.js/moq/package.json (1)
13-15: Packaging script correctly rewrites all subpath exports (including./zod)The
rewritePathfunction in js/scripts/package.ts
– replaces^./srcwith.and converts*.ts(x)?to.jsfor every string value inpkg.exports(lines 11–23)
– therefore"./zod": "./src/zod.ts"will become"./zod": "./zod.js"automaticallyNo further action required.
| async decode(): Promise<{ data: Uint8Array; timestamp: number; keyframe: boolean } | undefined> { | ||
| const next = await this.#track.nextFrame(); | ||
| if (!next) return undefined; | ||
|
|
||
| const { timestamp, data } = decode(next.data); | ||
| return { data, timestamp, keyframe: next.frame === 0 }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use readFrame() (API rename) instead of nextFrame()
The Track API rename in this PR implies nextFrame() → readFrame(). Using the old name will break at compile/runtime.
Apply this change:
- const next = await this.#track.nextFrame();
+ const next = await this.#track.readFrame();Run to confirm the API across the repo:
🏁 Script executed:
#!/bin/bash
# Verify TrackConsumer API usage
rg -nP 'nextFrame\s*\(' -C2
rg -nP 'readFrame\s*\(' -C2
rg -nP '(class|interface)\s+TrackConsumer\b' -n -C3Length of output: 7274
Rename all nextFrame() calls to readFrame() (Track API rename)
To align with the updated Track API and avoid compile/runtime errors, every invocation of nextFrame() must be replaced with readFrame(). Specifically, update the following locations:
- js/hang/src/frame.ts (around line 62)
- const next = await this.#track.nextFrame();
- const next = await this.#track.readFrame();
- **js/hang/src/watch/video/index.ts** (around line 114)
```diff
- const next = await Promise.race([sub.nextFrame(), cancel]);
+ const next = await Promise.race([sub.readFrame(), cancel]);
You can verify that no other nextFrame() calls remain in your Hang codebase:
#!/bin/bash
# Search only under js/hang/src for any leftover nextFrame() usage
rg -nP 'nextFrame\s*\(' -C2 js/hang/srcAll nextFrame() references should now be readFrame().
🤖 Prompt for AI Agents
In js/hang/src/frame.ts around lines 61 to 67, the code calls
this.#track.nextFrame() but the Track API was renamed to readFrame(); replace
the nextFrame() invocation with readFrame() and adjust any local variable names
if needed (e.g., const next = await this.#track.readFrame(); and use next as
before). Also update the other occurrence noted in
js/hang/src/watch/video/index.ts around line 114 the same way, and run the
provided ripgrep command to ensure no remaining nextFrame() calls exist in
js/hang/src.
| this.#signals.effect((effect) => { | ||
| if (!effect.get(this.enabled)) return; | ||
| this.#catalog.set(effect.get(catalog)?.chat?.typing); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t gate catalog extraction on enabled; also clear it on teardown.
When enabled flips false, #catalog stays set and #run can continue subscribing. Always mirror the catalog and reset on cleanup; gate subscription in #run instead.
- this.#signals.effect((effect) => {
- if (!effect.get(this.enabled)) return;
- this.#catalog.set(effect.get(catalog)?.chat?.typing);
- });
+ this.#signals.effect((effect) => {
+ this.#catalog.set(effect.get(catalog)?.chat?.typing);
+ effect.cleanup(() => this.#catalog.set(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.
| this.#signals.effect((effect) => { | |
| if (!effect.get(this.enabled)) return; | |
| this.#catalog.set(effect.get(catalog)?.chat?.typing); | |
| }); | |
| this.#signals.effect((effect) => { | |
| this.#catalog.set(effect.get(catalog)?.chat?.typing); | |
| effect.cleanup(() => this.#catalog.set(undefined)); | |
| }); |
🤖 Prompt for AI Agents
In js/hang/src/watch/chat/typing.ts around lines 31-34, the current effect
handler gates catalog extraction on this.enabled which leaves #catalog populated
when enabled flips false; change the handler to always mirror the catalog value
by setting this.#catalog = effect.get(catalog)?.chat?.typing (no enabled check)
and ensure that on teardown/cleanup you explicitly clear this.#catalog (set to
undefined/null) so subscribers in #run don’t keep seeing stale data; move the
enabled gating logic into the #run subscription control instead of here.
| if (DEV) { | ||
| WatchProducer.finalizer.unregister(this); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unregister: add the same guard.
- if (DEV) {
- WatchProducer.finalizer.unregister(this);
+ if (DEV && WatchProducer.finalizer) {
+ WatchProducer.finalizer.unregister(this);
}📝 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 (DEV) { | |
| WatchProducer.finalizer.unregister(this); | |
| } | |
| } | |
| if (DEV && WatchProducer.finalizer) { | |
| WatchProducer.finalizer.unregister(this); | |
| } | |
| } |
🤖 Prompt for AI Agents
In js/moq/src/util/watch.ts around lines 78 to 81, the unregister call needs the
same safety guard used elsewhere: ensure you only call
WatchProducer.finalizer.unregister(this) when DEV is true and the
finalizer/unregister function actually exists; wrap the unregister call with the
same DEV check and a runtime existence check (e.g., confirm
WatchProducer.finalizer and WatchProducer.finalizer.unregister are present)
before invoking it.
| if (DEV) { | ||
| WatchProducer.finalizer.unregister(this); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Abort path: guard unregister too.
- if (DEV) {
- WatchProducer.finalizer.unregister(this);
+ if (DEV && WatchProducer.finalizer) {
+ WatchProducer.finalizer.unregister(this);
}📝 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 (DEV) { | |
| WatchProducer.finalizer.unregister(this); | |
| } | |
| } | |
| if (DEV && WatchProducer.finalizer) { | |
| WatchProducer.finalizer.unregister(this); | |
| } |
🤖 Prompt for AI Agents
In js/moq/src/util/watch.ts around lines 87 to 90, the abort/early-exit path
currently does not unregister the instance from WatchProducer.finalizer; add the
same DEV-only unregister call (WatchProducer.finalizer.unregister(this)) into
the abort/cleanup branch so the object is unregistered when the operation is
aborted, ensuring you use the same `this` reference and keep the DEV guard to
avoid unregistering in production.
| node = document.createDocumentFragment(); | ||
| for (const child of e) { | ||
| render(effect, node, child); | ||
| } | ||
| } else if (typeof e === "number" || typeof e === "boolean" || typeof e === "string") { | ||
| node = document.createTextNode(e.toString()); | ||
| } else { | ||
| const exhaustive: never = e; | ||
| throw new Error(`Invalid element type: ${exhaustive}`); | ||
| } | ||
|
|
||
| parent.appendChild(node); | ||
| effect.cleanup(() => { | ||
| try { | ||
| parent.removeChild(node); | ||
| } catch (e) { | ||
| console.log("cleanup failed", parent, node); | ||
| throw e; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: removing a DocumentFragment will always throw; arrays leak DOM nodes
After appendChild, the fragment is emptied and is not a child of parent; cleanup parent.removeChild(node) throws and leaves children mounted.
Apply boundary markers so arrays mount/unmount reliably:
export function render(effect: Effect, parent: Node, element: Element | ((effect: Effect) => Element)) {
const e = typeof element === "function" ? element(effect) : element;
if (e === undefined || e === null) return;
- let node: Node;
- if (e instanceof Node) {
- node = e;
- } else if (Array.isArray(e)) {
- node = document.createDocumentFragment();
- for (const child of e) {
- render(effect, node, child);
- }
- } else if (typeof e === "number" || typeof e === "boolean" || typeof e === "string") {
- node = document.createTextNode(e.toString());
- } else {
- const exhaustive: never = e;
- throw new Error(`Invalid element type: ${exhaustive}`);
- }
-
- parent.appendChild(node);
- effect.cleanup(() => {
- try {
- parent.removeChild(node);
- } catch (e) {
- console.log("cleanup failed", parent, node);
- throw e;
- }
- });
+ // Arrays: render between boundary markers so cleanup can remove the exact range.
+ if (Array.isArray(e)) {
+ const start = document.createComment("signals:start");
+ const end = document.createComment("signals:end");
+ parent.appendChild(start);
+ for (const child of e) render(effect, parent, child);
+ parent.appendChild(end);
+ effect.cleanup(() => {
+ let cur = start.nextSibling;
+ while (cur && cur !== end) {
+ const next = cur.nextSibling!;
+ parent.removeChild(cur);
+ cur = next;
+ }
+ parent.removeChild(start);
+ parent.removeChild(end);
+ });
+ return;
+ }
+
+ let node: Node;
+ if (e instanceof Node) {
+ node = e;
+ } else if (typeof e === "number" || typeof e === "boolean" || typeof e === "string") {
+ node = document.createTextNode(e.toString());
+ } else {
+ const exhaustive: never = e as never;
+ throw new Error(`Invalid element type: ${exhaustive}`);
+ }
+
+ parent.appendChild(node);
+ effect.cleanup(() => {
+ if (parent.contains(node)) parent.removeChild(node);
+ });
}📝 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.
| node = document.createDocumentFragment(); | |
| for (const child of e) { | |
| render(effect, node, child); | |
| } | |
| } else if (typeof e === "number" || typeof e === "boolean" || typeof e === "string") { | |
| node = document.createTextNode(e.toString()); | |
| } else { | |
| const exhaustive: never = e; | |
| throw new Error(`Invalid element type: ${exhaustive}`); | |
| } | |
| parent.appendChild(node); | |
| effect.cleanup(() => { | |
| try { | |
| parent.removeChild(node); | |
| } catch (e) { | |
| console.log("cleanup failed", parent, node); | |
| throw e; | |
| } | |
| }); | |
| } | |
| export function render(effect: Effect, parent: Node, element: Element | ((effect: Effect) => Element)) { | |
| const e = typeof element === "function" ? element(effect) : element; | |
| if (e === undefined || e === null) return; | |
| // Arrays: render between boundary markers so cleanup can remove the exact range. | |
| if (Array.isArray(e)) { | |
| const start = document.createComment("signals:start"); | |
| const end = document.createComment("signals:end"); | |
| parent.appendChild(start); | |
| for (const child of e) render(effect, parent, child); | |
| parent.appendChild(end); | |
| effect.cleanup(() => { | |
| let cur = start.nextSibling; | |
| while (cur && cur !== end) { | |
| const next = cur.nextSibling!; | |
| parent.removeChild(cur); | |
| cur = next; | |
| } | |
| parent.removeChild(start); | |
| parent.removeChild(end); | |
| }); | |
| return; | |
| } | |
| let node: Node; | |
| if (e instanceof Node) { | |
| node = e; | |
| } else if (typeof e === "number" || typeof e === "boolean" || typeof e === "string") { | |
| node = document.createTextNode(e.toString()); | |
| } else { | |
| const exhaustive: never = e as never; | |
| throw new Error(`Invalid element type: ${exhaustive}`); | |
| } | |
| parent.appendChild(node); | |
| effect.cleanup(() => { | |
| if (parent.contains(node)) parent.removeChild(node); | |
| }); | |
| } |
|
Claude finished @kixelated's task —— View job Code Review Complete 👍Reviewing the "Revamp chat typing and audio speaking" PR for bugs and security issues. Todo List:
✅ No significant bugs or security issues found. The PR shows a solid architectural refactoring with proper security practices: Security Analysis:
Code Quality:
Architecture:
The refactoring maintains security best practices while improving code organization. |
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/package.json (1)
8-22: Replace “./container” export with “./frame” in js/hang/package.jsonVerification confirms that
js/hang/src/frame.tsexists and there are no remaining imports referencing@kixelated/hang/containeror./container. Update the export map accordingly to avoid exposing a stale path.• Confirmed
js/hang/src/frame.tsis present.
• No code injs/(outsidedist/) imports./containeror@kixelated/hang/container.Apply this diff to the
exportsblock injs/hang/package.json:"exports": { ".": "./src/index.ts", "./publish": "./src/publish/index.ts", "./publish/element": "./src/publish/element.ts", "./watch": "./src/watch/index.ts", "./watch/element": "./src/watch/element.ts", "./meet": "./src/meet/index.ts", "./meet/element": "./src/meet/element.ts", "./catalog": "./src/catalog/index.ts", - "./container": "./src/container/index.ts", + "./frame": "./src/frame.ts", "./support": "./src/support/index.ts", "./support/element": "./src/support/element.ts", "./connection": "./src/connection.ts", "./preview": "./src/preview/index.ts" },
♻️ Duplicate comments (2)
js/hang/src/watch/chat/typing.ts (1)
31-34: Always mirror catalog; don’t gate on enabled, and clear on teardown.Gating leaves a stale #catalog when enabled flips false; if the catalog changes while disabled, re-enable will resubscribe to the old track.
Apply:
- this.#signals.effect((effect) => { - if (!effect.get(this.enabled)) return; - this.#catalog.set(effect.get(catalog)?.chat?.typing); - }); + this.#signals.effect((effect) => { + this.#catalog.set(effect.get(catalog)?.chat?.typing); + effect.cleanup(() => this.#catalog.set(undefined)); + });js/hang/src/watch/chat/message.ts (1)
32-36: Always mirror catalog; don’t gate on enabled, and clear on teardown.Same stale-catalog risk as Typing when toggling enabled.
- this.#signals.effect((effect) => { - if (!effect.get(this.enabled)) return; - this.#catalog.set(effect.get(catalog)?.chat?.message); - }); + this.#signals.effect((effect) => { + this.#catalog.set(effect.get(catalog)?.chat?.message); + effect.cleanup(() => this.#catalog.set(undefined)); + });
🧹 Nitpick comments (5)
js/hang/src/watch/chat/typing.ts (2)
6-9: Nit: adjust comment to reflect typing, not “chat download.”Minor clarity tweak for docs.
- // Whether to start downloading the chat. + // Whether to subscribe to typing activity.
51-58: Nit: use a more descriptive loop variable.Rename value -> typing to improve readability.
- const value = await Promise.race([track.readBool(), cancel]); - if (value === undefined) break; - this.active.set(value); + const typing = await Promise.race([track.readBool(), cancel]); + if (typing === undefined) break; + this.active.set(typing);js/hang/src/watch/chat/message.ts (1)
53-56: Avoid double teardown writes; simplify latest initialization.effect.set plus an explicit cleanup causes two writes on teardown. Either set directly and keep the cleanup, or pass a cleanup value to effect.set.
- // Undefined is only when we're not subscribed to the track. - effect.set(this.#latest, ""); - effect.cleanup(() => this.#latest.set(undefined)); + // Undefined is only when we're not subscribed to the track. + this.#latest.set(""); + effect.cleanup(() => this.#latest.set(undefined));js/hang-demo/package.json (1)
22-22: Consider using the workspace protocol for TypeScript to prevent version driftOptional: in
js/hang-demo/package.json, switch thetypescriptdependency toworkspace:^so the demo always uses the repo’s root TS version:- "typescript": "^5.9.2", + "typescript": "workspace:^",Then verify the package still type-checks against the workspace TS (note that your PNPM workspace root lives under
js/):#!/usr/bin/env bash set -euo pipefail # Move into the JS workspace root so pnpm picks up js/pnpm-workspace.yaml pushd js >/dev/null if command -v pnpm >/dev/null 2>&1; then # Run the hang-demo check script from the workspace pnpm -w -F @kixelated/hang-demo run check else # Fallback to pure tsc invocation (cd hang-demo && npx tsc --noEmit) fi popd >/dev/null– If you still see the “
--workspace-root may only be used inside a workspace” error, make sure you’re running this script from the repo root (sojs/contains the workspace file).
– Once this passes without error, you can be confident the demo will always align with your root TS version.js/moq-token/package.json (1)
22-23: Align Zod Versions Across Packages
No Node.js engine bump is needed—[email protected]doesn’t declare anyenginesrequirement—so focus on unifying your Zod minor versions for consistency.• In
js/moq/package.json, bump Zod from^4.0.0to^4.1.3to match the other packages.
• No changes required forjs/moq-tokenorjs/hang, as they’re already on^4.1.3.--- a/js/moq/package.json +++ b/js/moq/package.json @@ peerDependencies - "zod": "^4.0.0" + "zod": "^4.1.3"
📜 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 ignored due to path filters (1)
js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
js/hang-demo/package.json(1 hunks)js/hang/package.json(1 hunks)js/hang/src/watch/audio/captions.ts(1 hunks)js/hang/src/watch/chat/message.ts(1 hunks)js/hang/src/watch/chat/typing.ts(1 hunks)js/moq-token/package.json(1 hunks)js/moq/src/util/watch.ts(5 hunks)js/moq/src/zod.ts(1 hunks)js/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- js/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- js/moq/src/util/watch.ts
- js/hang/src/watch/audio/captions.ts
- js/moq/src/zod.ts
🧰 Additional context used
📓 Path-based instructions (2)
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang-demo/package.jsonjs/moq-token/package.jsonjs/hang/package.jsonjs/hang/src/watch/chat/message.tsjs/hang/src/watch/chat/typing.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/package.jsonjs/hang/src/watch/chat/message.tsjs/hang/src/watch/chat/typing.ts
🧬 Code graph analysis (2)
js/hang/src/watch/chat/message.ts (5)
js/hang/src/publish/chat/message.ts (2)
MessageProps(6-8)Message(10-52)js/signals/src/index.ts (3)
Signal(19-76)Getter(10-13)Effect(81-433)js/moq/src/broadcast.ts (1)
BroadcastConsumer(122-180)js/hang/src/watch/audio/captions.ts (1)
effect(32-55)js/hang/src/watch/chat/typing.ts (1)
effect(39-61)
js/hang/src/watch/chat/typing.ts (6)
js/hang/src/publish/chat/typing.ts (2)
TypingProps(6-8)Typing(10-55)js/signals/src/index.ts (3)
Signal(19-76)Getter(10-13)Effect(81-433)js/moq/src/broadcast.ts (1)
BroadcastConsumer(122-180)js/hang/src/watch/audio/captions.ts (1)
effect(32-55)js/hang/src/watch/chat/message.ts (1)
effect(41-66)js/hang/src/watch/audio/speaking.ts (1)
effect(32-56)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: Check
🔇 Additional comments (6)
js/hang/package.json (2)
41-41: Transformers.js bump to 3.7.2 — check runtime/bundle impact.Minor bump is fine; verify no breaking API, model asset URL changes, or unexpected ESM/CJS shifts and watch for bundle size regressions in web builds.
46-46: Zod 4.1.3 bump LGTM.Incremental v4 bump; low risk. Ensure any zod schema usage aligns with 4.x semantics.
js/hang/src/watch/chat/typing.ts (1)
39-61: Subscription gating and cleanup look correct.Enabled check, null-guards, track.close(), cancel-aware loop, and resetting active to undefined are consistent with audio watchers.
js/hang/src/watch/chat/message.ts (1)
41-51: LGTM on gating and subscription lifecycle.Enabled guard, null-guards, and track.close() are correct.
js/hang-demo/package.json (1)
17-21: No duplicate Tailwind registration detected.Inspected the configuration: only
@tailwindcss/viteis registered invite.config.tsand there is nopostcss.config.*file present. No further changes needed.js/moq-token/package.json (1)
22-23: Version bumps look aligned with repo-wide Zod + IO changes.Patch updates for jose and zod are reasonable and consistent with the PR’s Zod-backed flows.
| "@huggingface/transformers": "^3.7.2", | ||
| "@kixelated/moq": "link:../moq", | ||
| "@kixelated/signals": "link:../signals", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Using link: for local workspaces can break publish/install; consider peer+dev split or publish-time rewrite.
link: is great locally but is not publishable to npm and will break external consumers if it escapes packaging. Prefer:
- Declare
@kixelated/moqand@kixelated/signalsas peerDependencies (runtime contract). - Keep local development via devDependencies (can stay
link:) orworkspace:*. - Ensure your
../scripts/package.tsrewrites these to proper semver ranges before publish.
To verify, after your packaging step, inspect the tarball’s package.json has no link::
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# from repo root
pnpm -w -r build || true
# produce tarball for this package (adjust if your packaging script already emits one)
tarball=$(cd js/hang && npm pack --json | jq -r '.[0].filename')
tmpdir=$(mktemp -d)
tar -xzf "js/hang/$tarball" -C "$tmpdir"
jq '.dependencies' "$tmpdir/package/package.json"
if jq -e 'to_entries[] | select(.value|tostring|test("^link:"))' "$tmpdir/package/package.json" >/dev/null; then
echo "Error: link: deps leaked into published manifest"
exit 1
fi
echo "OK: No link: deps in publish manifest"Length of output: 751
Fix leaking link: dependencies in published package
Manual inspection of the js/hang tarball (kixelated-hang-0.3.12.tgz) shows that its bundled package.json still contains local link: entries under .dependencies:
@kixelated/moq:"link:../moq"@kixelated/signals:"link:../signals"
This will break installs for external consumers, since npm cannot resolve link: specifiers in published packages. To resolve:
• Move these to peerDependencies with proper semver ranges (e.g. "^X.Y.Z").
• Keep the local link: specifiers (or workspace:*) in devDependencies for local development.
• Enhance your scripts/package.ts (or packaging pipeline) to rewrite all link:/workspace: specifiers to actual version ranges before publishing.
Update your post-pack verification script to correctly test the .dependencies field, for example:
#!/usr/bin/env bash
set -euo pipefail
cd js/hang
# Pack the module
tarball=$(npm pack --json | jq -r '.[0].filename')
tmpdir=$(mktemp -d)
tar -xzf "$tarball" -C "$tmpdir"
# Fail if any dependency still uses link:
if jq -e '.dependencies
| to_entries[]
| select(.value | test("^link:"))' \
"$tmpdir/package/package.json" >/dev/null; then
echo "Error: link: deps leaked into dependencies"
exit 1
fi
echo "OK: No link: deps in dependencies"🤖 Prompt for AI Agents
In js/hang/package.json around lines 41-43, the packaged package.json still
contains local "link:../moq" and "link:../signals" entries in dependencies; move
these two entries from dependencies into peerDependencies with appropriate
semver ranges (e.g. "^X.Y.Z") and keep local link: or workspace:* entries in
devDependencies for local development; update scripts/package.ts (or the
packaging pipeline) to rewrite any "link:"/"workspace:" specifiers to real
version ranges when creating the published tarball, and add/update the post-pack
verification script to extract the generated tarball and assert that
package/package.json .dependencies contains no "link:" or "workspace:"
specifiers (failing the pack if any are found).
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores