Refactor the JS core #593
Refactor the JS core #593
Conversation
WalkthroughThe PR refactors the Moq runtime from producer/consumer pairs to unified classes (Track, Group, Broadcast, Announced) and introduces Connection.Established and Connection.Reload; connect() now returns Established. Many js/hang modules were adapted: connection removal, publishers moved to catalog-driven static TRACK identifiers with new serve(track, effect) methods, location split into window/peers, and preview moved into catalog. Watch modules adopt Broadcast/PRIORITY usage. Signals API gained changed(), watch(), race(), updated Effect.spawn(), and closed/cancel getters. CI and scripts were split into check vs test; numerous tests and files were added, removed, or version-bumped. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
js/signals/package.json (1)
8-16: Fix package exports/files to publish compiled JS + type declarations (don’t ship .ts source).npm pack --dry-run for js/signals shows the tarball contains src/.ts and package.json exports point to ./src/.ts — consumers will receive TS sources. Update js/signals/package.json or ensure the release script rewrites these to built outputs.
Location: js/signals/package.json
- "exports": { - ".": "./src/index.ts", - "./solid": "./src/solid.ts", - "./react": "./src/react.ts", - "./dom": "./src/dom.ts" - }, - "files": [ - "./src" - ], + "exports": { + ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" }, + "./solid": { "types": "./dist/solid.d.ts", "import": "./dist/solid.js" }, + "./react": { "types": "./dist/react.d.ts", "import": "./dist/react.js" }, + "./dom": { "types": "./dist/dom.d.ts", "import": "./dist/dom.js" } + }, + "files": [ + "./dist", + "README.md", + "LICENSE" + ], + "sideEffects": false,js/moq/README.md (1)
94-105: Fix variable name in discovery loop (example currently throws).announced.next() references a non-existent identifier; should call next() on announcement.
Apply this diff:
-// Discover broadcasts announced by the server -let announcement = await connection.announced(); -while (announcement) { +// Discover broadcasts announced by the server +let announcement = await connection.announced(); +while (announcement) { console.log("New stream available:", announcement.name); // Subscribe to new streams const broadcast = connection.consume(announcement.name); // ... handle the broadcast - announcement = await announced.next(); + announcement = await announcement.next(); }js/signals/src/index.ts (1)
21-23: Setter interface mismatches Signal.set — TypeScript compile errorSetter still declares the updater form but Signal.set is declared as set(value: T, notify?: boolean): void; (js/signals/src/index.ts — Setter: lines 21–23; Signal.set: line 49). Add overloads and keep notify, or remove the updater from Setter and require .update for function-style updates.
-export interface Setter<T> { - set(value: T | ((prev: T) => T)): void; -} +export interface Setter<T> { + set(value: T, notify?: boolean): void; + set(updater: (prev: T) => T): void; +}-// Set the current value, by default notifying subscribers if the value is different. -// If notify is undefined, we'll check if the value has changed after the microtask. -set(value: T, notify?: boolean): void { +// Overloads to keep BC with old setter style. +set(value: T, notify?: boolean): void; +set(updater: (prev: T) => T): void; +set(valueOrFn: T | ((prev: T) => T), notify?: boolean): void { + if (typeof valueOrFn === "function") { + // Route function-style setter through update() + // @ts-expect-error narrowing + this.update(valueOrFn as (prev: T) => T, notify !== false); + return; + } + const value = valueOrFn as T; const old = this.#value; this.#value = value; // If notify is false, don't notify. if (notify === false) return; // Don't even queue a microtask if the value is the EXACT same. if (notify === undefined && old === this.#value) return; // If there are no subscribers, don't queue a microtask. if (this.#subscribers.size === 0 && this.#changed.size === 0) return; const subscribers = this.#subscribers; const changed = this.#changed; this.#changed = new Set(); queueMicrotask(() => { if (notify === undefined && dequal(old, this.#value)) { for (const fn of changed) { this.#changed.add(fn); } return; } for (const fn of subscribers) { try { fn(value); } catch (error) { console.error("signal subscriber error", error); } } for (const fn of changed) { try { fn(value); } catch (error) { console.error("signal changed error", error); } } }); }
- Audit repo for call sites that pass updater lambdas to .set and convert them to .update where appropriate.
js/moq/src/connection/connect.ts (1)
124-130: Abort in-flight fingerprint fetch on cancel to avoid leaks.Promise.race stops awaiting but does not cancel the network request.
Apply this diff:
- // TODO cancel the request if the effect is cancelled. - const fingerprint = await Promise.race([fetch(fingerprintUrl), cancel]); - if (!fingerprint) return undefined; - - const fingerprintText = await Promise.race([fingerprint.text(), cancel]); - if (fingerprintText === undefined) return undefined; + const ac = new AbortController(); + cancel.then(() => ac.abort()).catch(() => {}); + let fingerprintText: string | undefined; + try { + const fingerprint = await fetch(fingerprintUrl, { signal: ac.signal }); + fingerprintText = await fingerprint.text(); + } catch (err) { + // AbortError => lost race + if ((err as DOMException)?.name !== "AbortError") throw err; + return undefined; + }js/hang/src/catalog/location.ts (1)
21-39: Make LocationSchema strict and reconcile JS↔Rust catalog renames
- Apply:
-export const LocationSchema = z.object({ +export const LocationSchema = z.strictObject({
- Critical mismatch found: Rust still defines updates/peers as a map (rs/hang/src/catalog/location.rs — around lines 24–30). JS expects track (string) and a single peers track. Update the Rust catalog (or add a compatibility bridge) so the serialized catalog matches the JS shape (rename updates→track and convert peers map→single track or keep dual-format compatibility).
- Verify Catalog encoders/decoders and any publishers/subscribers that read/write the catalog (js/hang/src/publish/, js/hang/src/watch/, rs/hang/src/catalog/*) after the change.
js/hang/src/publish/video/detection.ts (1)
60-79: Detection can stall when no frame is available; always reschedule.If frame is undefined, the loop returns without scheduling the next run. Also consider cancellation during awaits.
Apply this diff:
- effect.spawn(async () => { - const ready = await api.ready(); - if (!ready) return; - process(); - }); + effect.spawn(async () => { + const ready = await Promise.race([effect.cancel, api.ready()]); + if (!ready) return; + process(); + }); @@ - const process = async () => { + const process = async () => { const frame = this.frame(); - if (!frame) return; + if (!frame) { + timeout = setTimeout(process, this.#interval); + return; + } @@ - timeout = setTimeout(process, this.#interval); + timeout = setTimeout(process, this.#interval); };js/moq/src/ietf/subscriber.ts (1)
95-107: Add a timeout for SUBSCRIBE_OK to avoid hanging subscriptionsIf the server never replies,
responsePromisehangs and the track stays mapped. Add a bounded timeout and clear it on resolve/reject.const responsePromise = new Promise<SubscribeOk>((resolve, reject) => { this.#subscribeCallbacks.set(subscribeId, { resolve, reject }); }); await this.#control.write(msg); try { - await responsePromise; + const timer = setTimeout(() => { + const e = new Error(`SUBSCRIBE timeout: id=${subscribeId} broadcast=${broadcast}`); + this.#subscribeCallbacks.get(subscribeId)?.reject(e); + }, 10_000); + await responsePromise; + clearTimeout(timer); await request.track.closed; const msg = new Unsubscribe(subscribeId); await this.#control.write(msg); } catch (err) { + // Ensure timer doesn't linger if we errored before clearing. + try { clearTimeout(timer); } catch {} const e = error(err); request.track.close(e); } finally {js/hang/src/watch/preview.ts (1)
44-53: Preview stops after first update; loop until track closesRead once means subsequent preview updates are missed. Loop until Zod.read returns undefined or errors.
- effect.spawn(async () => { - try { - const info = await Zod.read(track, Catalog.PreviewSchema); - if (!info) return; - - this.preview.set(info); - } catch (error) { - console.warn("Failed to parse preview JSON:", error); - } - }); + effect.spawn(async () => { + for (;;) { + try { + const info = await Zod.read(track, Catalog.PreviewSchema); + if (!info) break; + this.preview.set(info); + } catch (error) { + console.warn("Failed to parse preview JSON:", error); + break; + } + } + });
🧹 Nitpick comments (81)
js/hang/tsconfig.json (1)
7-7: Redundant include of src/time.ts; keep just "src"."time.ts" is already under "src", so explicitly listing it is unnecessary and risks drift if files move.
Apply this diff:
- "include": ["src", "src/time.ts"] + "include": ["src"]If this was added to work around an inherited "exclude" in ../tsconfig.json, prefer fixing that exclude instead of enumerating specific files.
js/hang/package.json (2)
35-37: Add a publish guard.Recommend adding
prepublishOnlyto ensure builds precede publishes; consider also addingpackageManagerandenginesto prevent CI drift."scripts": { "build": "rimraf dist && tsc -b && bun ../scripts/package.ts", "check": "tsc --noEmit", "test": "vitest", "release": "bun ../scripts/release.ts", + "prepublishOnly": "bun run build" },
38-47: Polyfill addition: load conditionally and avoid unintended side effects.Ensure
@kixelated/libavjs-webcodecs-polyfillis only imported when needed (feature-detected), not in hot watch-only paths; if its import patches globals at module top level, list the importing module undersideEffectsto prevent tree-shake mis-optimizations. Optional: if AI features are truly on‑demand, consider moving heavy deps like@huggingface/transformerstopeerDependencieswithpeerDependenciesMeta.optional=trueto keep installs lighter.rs/moq/src/session/publisher.rs (2)
144-144: Unify subscribe log phrasing and fields; prefer consistent keys over positional args.Use “subscribe …” (present tense) everywhere and name all fields to keep structured logs queryable. Also avoid formatting
%idas a string to prevent type drift vs. other modules.Apply:
- tracing::info!(%id, broadcast = %absolute, %track, "subscribed started"); + tracing::info!(id, broadcast = %absolute, track = %track, "subscribe started"); … - tracing::warn!(%id, broadcast = %absolute, %track, %err, "subscribed error") + tracing::warn!(id, broadcast = %absolute, track = %track, %err, "subscribe error") … - tracing::info!(%id, broadcast = %absolute, %track, "subscribed complete") + tracing::info!(id, broadcast = %absolute, track = %track, "subscribe complete")Also applies to: 157-158, 162-162
153-155: Preserve cancellation reason in logs.Lumping Cancel and Transport together loses useful signal. Emit a reason field to distinguish user cancel vs. network transport.
- Error::Cancel | Error::Transport(_) => { - tracing::info!(%id, broadcast = %absolute, %track, "subscribed cancelled") - } + Error::Cancel => { + tracing::info!(id, broadcast = %absolute, track = %track, reason = "cancel", "subscribe cancelled") + } + Error::Transport(_) => { + tracing::info!(id, broadcast = %absolute, track = %track, reason = "transport", "subscribe cancelled") + }rs/moq/src/session/subscriber.rs (1)
125-125: LGTM; align announce phrasing across file for consistency.This “announce” message reads well. Consider updating the earlier “announced start” (Line 85) to “announce started” for symmetry with “unannounced” and to match the subscribe logs.
js/signals/package.json (1)
34-35: Don’t require @types/react for non-React consumers.Having @types/react in peerDependencies forces TS users (even non-React ones) to install it. Either remove it from peerDependencies or mark it optional.
Apply one of:
- "peerDependencies": { - "@types/react": "^19.1.8", - "react": "^19.0.0", - "solid-js": "^1.9.7" - }, + "peerDependencies": { + "react": "^19.0.0", + "solid-js": "^1.9.7" + },or:
"peerDependenciesMeta": { + "@types/react": { "optional": true }, "react": { "optional": true }, "solid-js": { "optional": true } }Also applies to: 38-45
js/deno.json (1)
1-3: Confirm project-wide impact of enabling unstable flags; add schema for editor support.
- Moving
"unstable": ["sloppy-imports","net"]to js/deno.json affects any Deno command that picks up this config; verify that’s intended for all JS subpackages. Also note sloppy‑imports has perf implications and is discouraged by Deno. Consider scoping to only packages that need it. (docs.deno.com)- Add the Deno config schema for better validation in editors.
{ + "$schema": "https://deno.land/x/deno/cli/schemas/config-file.v1.json", "unstable": ["sloppy-imports", "net"] }js/justfile (2)
18-27: Add strict bash options in shebang recipes for robustness.These are shebang recipes (executed by bash), so consider enabling strict mode to fail fast:
check: #!/usr/bin/env bash + set -euo pipefail bun install --frozen-lockfile if tty -s; then bun run --filter='*' --elide-lines=0 check else bun run --filter='*' check fiShebang recipes are executed with the specified interpreter; strict flags are recommended. (just.systems)
28-36: Mirror strict options in test recipe.test: #!/usr/bin/env bash + set -euo pipefail bun install --frozen-lockfile if tty -s; then bun run --filter='*' --elide-lines=0 test else bun run --filter='*' test firs/justfile (1)
150-152: LGTM: explicit test target.Keeps CI and local workflows clearer. Consider adding nextest later if test runtime becomes an issue.
.github/workflows/check.yml (1)
31-34: Run both static checks and tests in CI—nice. Consider caching JS deps for speed.With Nix you’re covered for toolchains; node_modules/bun cache isn’t. Optionally add an actions/cache keyed on bun.lockb to shave minutes on cold runs. Keep as-is if Nix cache suffices.
js/hang/src/support/element.ts (1)
202-204: Atomic toggle via Signal.update looks good.Switching to update() prevents stale reads during rapid clicks. Consider minor a11y: reflect expanded state on the button.
Apply this diff to set aria-expanded:
effect.event(detailsButton, "click", () => { this.#details.update((prev) => !prev); }); effect.effect((effect) => { detailsButton.textContent = effect.get(this.#details) ? "Details ➖" : "Details ➕"; + detailsButton.setAttribute("aria-expanded", effect.get(this.#details) ? "true" : "false"); });js/hang/src/publish/source/microphone.ts (1)
42-55: Cancellation shift to effect.cancel is correct, but clear the stream on teardown to avoid stale refs.On disable/cancel, the track is stopped but this.stream still holds the old track.
Apply this diff to clear the signal on cleanup:
effect.cleanup(() => media.then((media) => media?.getTracks().forEach((track) => { track.stop(); }), ), ); const stream = await Promise.race([media, effect.cancel]); if (!stream) return; + // Ensure consumers don't keep a stale track if this effect is later cancelled/reloaded. + effect.cleanup(() => effect.set(this.stream, undefined));js/hang/src/publish/source/camera.ts (1)
41-65: Good move to effect.cancel; also clear stream on cleanup like microphone.Prevents dangling references when toggling camera off.
Apply this diff:
const stream = await Promise.race([media, effect.cancel]); if (!stream) return; this.device.permission.set(true); const track = stream.getVideoTracks()[0] as VideoStreamTrack | undefined; if (!track) return; const settings = track.getSettings(); effect.set(this.device.active, settings.deviceId); effect.set(this.stream, track, undefined); + // Clear the signal when effect is cancelled/reloaded. + effect.cleanup(() => effect.set(this.stream, undefined));js/hang/src/publish/source/screen.ts (1)
45-61: Correct cancellation wiring; add stream clearing on cleanup.Tracks are stopped, but expose undefined to consumers on teardown to prevent stale usage.
Apply this diff:
const v = media.getVideoTracks().at(0) as VideoStreamTrack | undefined; const a = media.getAudioTracks().at(0) as AudioStreamTrack | undefined; effect.cleanup(() => v?.stop()); effect.cleanup(() => a?.stop()); effect.set(this.stream, { video: v, audio: a }, undefined); + // Clear exposed stream on effect cancel/reload. + effect.cleanup(() => effect.set(this.stream, undefined));js/hang/src/watch/priority.ts (1)
1-12: Freeze the catalog to prevent accidental runtime mutation.Type-level readonly doesn’t prevent property writes at runtime.
Apply this diff:
-export const PRIORITY = { +export const PRIORITY = Object.freeze({ catalog: 100, chat: 90, audio: 80, captions: 70, video: 60, speaking: 50, typing: 40, detection: 30, location: 20, preview: 10, -} as const; +} as const);js/moq/src/connection/established.ts (2)
12-13: Consider exposing close reason for parity with other types
Established.closed: Promise<void>hides failure cause, whileAnnounced.closedresolves toError | undefined. ConsiderPromise<Error | undefined>or a separatecloseReasonfor diagnostics.- closed: Promise<void>; + // Resolves undefined on clean close or an Error on failure. + closed: Promise<Error | undefined>;
11-11: Minor naming nit: parameter reads like a type, not a value
consume(broadcast: Path.Valid)suggests a Broadcast instance; prefernamefor clarity.- consume(broadcast: Path.Valid): Broadcast; + consume(name: Path.Valid): Broadcast;js/hang/src/publish/broadcast.ts (3)
40-71: Harden publish path against synchronous exceptionsWrap
connection.publish(name, broadcast)to avoid tearing down the effect on unexpected throws.- connection.publish(name, broadcast); + try { + connection.publish(name, broadcast); + } catch (err) { + console.warn("publish failed:", err); + return; // bail until next rerun + }
73-121: Make request loop cancel-aware to exit promptly on reloadBreak the loop on effect cancellation (mirrors
Reloadpattern).- for (;;) { - const request = await broadcast.requested(); + for (;;) { + const request = await Promise.race([ + broadcast.requested(), + effect.cancel.then(() => undefined), + ]); if (!request) break;
123-146: Avoid redundant catalog frames (dedupe on change)Cache and compare the encoded catalog to reduce churn on subscribers.
export class Broadcast { + #lastCatalog?: Uint8Array; ... #serveCatalog(track: Moq.Track, effect: Effect): void { if (!effect.get(this.enabled)) { - track.writeFrame(Catalog.encode({})); + const empty = Catalog.encode({}); + if (!this.#lastCatalog || !Uint8Array.prototype.every || this.#lastCatalog.length !== empty.length || !empty.every((b, i) => b === this.#lastCatalog![i])) { + track.writeFrame(empty); + this.#lastCatalog = empty; + } return; } ... - track.writeFrame(encoded); + if (!this.#lastCatalog || this.#lastCatalog.length !== encoded.length || !encoded.every((b, i) => b === this.#lastCatalog![i])) { + track.writeFrame(encoded); + this.#lastCatalog = encoded; + } }js/hang/src/catalog/track.ts (1)
3-12: Validate non-empty track names (and keep BC transform)Prevent empty names early; keep the object form transform.
-export const TrackSchema = z.union([ - z.string(), +export const TrackSchema = z.union([ + z.string().min(1, "track name cannot be empty"), // TODO remove backwards compatibility z .object({ name: z.string(), priority: z.number().int().min(0).max(255), }) .transform((val) => val.name), ]);js/hang/src/publish/element.ts (3)
166-173: Wiring looks correct; enable only when URL is set?Currently
enabled: truewith undefined URL yields “disconnected” (fine). If you prefer stricter semantics, gate enabled on url presence.- this.connection = new Moq.Connection.Reload({ - enabled: true, - url: this.parent.signals.url, - }); + this.connection = new Moq.Connection.Reload({ + enabled: this.parent.signals.url.map((u) => !!u), + url: this.parent.signals.url, + });Also confirm
Moq.Connection.Reloadand.establishedare stable exports across targets (bundler/TS path).
431-431: Caret toggle icon is invertedShow “▲” when expanded and “▼” when collapsed for clarity.
- effect.event(caret, "click", () => visible.update((v) => !v)); + effect.event(caret, "click", () => visible.update((v) => !v)); ... - caret.innerText = v ? "▼" : "▲"; + caret.innerText = v ? "▲" : "▼";
518-518: Apply the same caret fix for camera device selectorKeep icons consistent.
- effect.event(caret, "click", () => visible.update((v) => !v)); ... - caret.innerText = v ? "▼" : "▲"; + caret.innerText = v ? "▲" : "▼";js/moq/src/lite/connection.ts (4)
60-73: Close the session stream and wrap close calls to avoid noisy errors.Currently only WebTransport is closed; the session stream’s writer isn’t. Also, writer/transport close calls can throw and should be guarded.
Apply:
close() { - if (this.#closed) return; + if (this.#closed) return; this.#closed = true; this.#publisher.close(); this.#subscriber.close(); try { - // TODO: For whatever reason, this try/catch doesn't seem to work..? - this.#quic.close(); + // Close the session stream first. + try { + this.#session.writer.close(); + } catch { + // ignore + } + this.#quic.close(); } catch { // ignore } }
140-146: Guard bidi writer close in finally.Reset followed by close can error in some impls; don’t let it bubble.
- .finally(() => { - stream.writer.close(); - }); + .finally(() => { + try { + stream.writer.close(); + } catch { + // ignore + } + });
101-105: Fix doc: method returns an Announced reader for a prefix.- /** - * Gets the next announced broadcast. - */ + /** + * Gets an announced reader for the specified prefix. + */
113-118: Tighten doc wording for consume() return.- * @returns A Broadcast instance + * @returns Broadcastjs/hang/src/publish/video/encoder.ts (2)
107-110: Don’t throw on first non-key output; drop until first keyframe.Throwing here can tear down the encoder. Safer to skip frames until we see a key.
- } else if (!group) { - throw new Error("no keyframe"); - } + } else if (!group) { + // Drop non-key frames until the first keyframe arrives. + return; + }
39-50: Codec negotiation init is good; consider surfacing readiness.Optional: expose a readonly getter like ready: Getter that flips once codec is selected, to help callers gate serve().
js/hang/src/watch/chat/index.ts (1)
20-20: Type change to Moq.Broadcast — OK; consider accepting Getter for read-only dependency.To reduce coupling, accept Getter<Moq.Broadcast | undefined> instead of Signal. Safe if Message/Typing already accept Getter.
Apply minimally:
-import { Effect, Signal } from "@kixelated/signals"; +import { Effect, Signal, type Getter } from "@kixelated/signals"; - constructor( - broadcast: Signal<Moq.Broadcast | undefined>, + constructor( + broadcast: Getter<Moq.Broadcast | undefined>, catalog: Signal<Catalog.Root | undefined>, props?: ChatProps, ) {Confirm Message and Typing constructors accept the same (Getter) shape; otherwise keep as-is.
js/hang/src/catalog/root.ts (1)
9-9: Approve: Catalog.fetch now takes Moq.Track — callers updated.
Verified caller: js/hang/src/watch/broadcast.ts — async #fetchCatalog(catalog: Moq.Track) (line 144) calls Catalog.fetch(catalog) (line 147).
Optional: add JSDoc on fetch noting it may return undefined if the track closes before the first frame.js/hang/src/catalog/preview.ts (2)
3-14: Tighten schema (strict object + URL validation for avatar).
- Use z.strictObject to reject unknown keys explicitly.
- Validate avatar as a URL (or keep string if you need data: URIs).
Apply this diff:
-export const PreviewSchema = z.object({ +export const PreviewSchema = z.strictObject({ name: z.string().optional(), // name - avatar: z.string().optional(), // avatar + avatar: z.string().url().optional(), // avatarIf data: URIs must be allowed, stick with string or add a custom refine.
16-16: Consider validating on publish as well.Publish path js/hang/src/publish/preview.ts writes raw JSON. Prefer Zod.write there for symmetry with watch-side parsing.
Example:
Zod.write(track, info, Catalog.PreviewSchema);js/moq/src/connection/connect.ts (2)
32-38: Update JSDoc to match new return type.Comment still says “Connection”; function returns Established.
Apply:
- * @returns A promise that resolves to a Connection instance + * @returns A promise that resolves to an Established connection
131-136: Trim SHA256 text before hex decode.Many servers include a trailing newline.
Apply:
- value: Hex.toBytes(fingerprintText), + value: Hex.toBytes(fingerprintText.trim()),js/hang/src/watch/chat/message.ts (2)
58-66: Harden the read loop with try/catch to avoid unhandled rejections.track.readString() can throw on abort/error.
Apply:
- effect.spawn(async () => { - for (;;) { - const frame = await track.readString(); - if (frame === undefined) break; - - // Use a function to avoid the dequal check. - this.#latest.set(frame); - } - }); + effect.spawn(async () => { + try { + for (;;) { + const frame = await track.readString(); + if (frame === undefined) break; + this.#latest.set(frame); + } + } catch (error) { + console.warn("chat message read failed:", error); + } + });
63-64: Remove stale comment.No function is passed to set(); the dequal note is misleading.
Apply:
- // Use a function to avoid the dequal check.js/hang/src/catalog/location.ts (1)
4-19: Validate ranges and make schema strict.
- x/y are documented as -1..+1; enforce via min/max.
- z/s should be finite; s should be > 0 (or >= 0 if zero is valid).
- Use strictObject to reject unknown keys.
Apply:
-export const PositionSchema = z.object({ +export const PositionSchema = z.strictObject({ - x: z.number().optional(), + x: z.number().min(-1).max(1).finite().optional(), - y: z.number().optional(), + y: z.number().min(-1).max(1).finite().optional(), - z: z.number().optional(), + z: z.number().finite().optional(), - s: z.number().optional(), + s: z.number().positive().finite().optional(), });If s=0 is valid, use .nonnegative() instead of .positive().
js/hang/src/watch/user.ts (1)
24-28: Potential race condition with stale catalog data.The effect reads from the catalog signal and immediately sets the internal signals without checking if they're already closed. If the catalog updates rapidly or the component is closing, this could lead to setting state on disposed signals.
Consider adding a closed state check:
this.signals.effect((effect) => { if (!effect.get(this.enabled)) return; + if (effect.closed) return; this.#id.set(effect.get(catalog)?.user?.id); this.#name.set(effect.get(catalog)?.user?.name); this.#avatar.set(effect.get(catalog)?.user?.avatar); this.#color.set(effect.get(catalog)?.user?.color); });js/moq-clock/src/main.ts (1)
94-134: Consider adding error handling for track operations.The
publishTrackfunction performs multiple write operations but doesn't handle potential errors fromtrack.appendGroup()orgroup.writeString(). If the track is closed mid-publication, these operations might throw.Add error handling:
async function publishTrack(track: Moq.Track) { // Send timestamps over the wire, matching the Rust implementation format console.log("✅ Publishing clock data on track:", track.name); + try { for (;;) { + // Check if track is closed before starting a new group + if (await track.closed) break; + const now = new Date(); // Create a new group for each minute (matching Rust implementation) const group = track.appendGroup(); // ... rest of the code ... } + } catch (error) { + console.error("Error publishing to track:", error); + track.close(error instanceof Error ? error : new Error(String(error))); + } }js/hang/src/watch/audio/source.ts (1)
155-157: Simplified async initialization without cancellation race.The removal of the
cancelparameter and race condition simplifies the initialization flow. However, this means the polyfill loading cannot be cancelled if the effect is closed during loading.Consider checking effect state after async operations:
effect.spawn(async () => { const loaded = await libav.polyfill(); - if (!loaded) return; // cancelled + if (!loaded || effect.closed) return; // cancelled or effect closedjs/hang/src/publish/location/peers.ts (1)
31-39: Inconsistent null check for positions.Line 36 checks if
positionsis falsy, butpositionsis initialized as an empty object{}on line 21, which is truthy. This check will never fail unless someone explicitly sets it to a falsy value.Either check for empty object or remove the unnecessary check:
serve(track: Moq.Track, effect: Effect): void { const enabled = effect.get(this.enabled); if (!enabled) return; const positions = effect.get(this.positions); - if (!positions) return; + if (!positions || Object.keys(positions).length === 0) return; Zod.write(track, positions, Catalog.PeersSchema); }Or if empty positions are valid:
serve(track: Moq.Track, effect: Effect): void { const enabled = effect.get(this.enabled); if (!enabled) return; const positions = effect.get(this.positions); - if (!positions) return; Zod.write(track, positions, Catalog.PeersSchema); }js/hang/src/meet/element.ts (4)
41-44: Harden URL parsing against invalid/relative values.new URL(newValue) will throw on malformed or relative inputs. Use a base and catch errors to avoid breaking attribute updates.
Apply this diff:
- if (name === "url") { - this.url = newValue ? new URL(newValue) : undefined; + if (name === "url") { + try { + this.url = newValue ? new URL(newValue, window.location.href) : undefined; + } catch { + console.warn("hang-meet: invalid url attribute:", newValue); + this.url = undefined; + }
80-83: Key the #remotes map by Moq.Path.Valid for type‑safety.Align with #onRemote signature and #locals map to prevent accidental key mismatches.
Apply this diff:
- #remotes = new Map< - string, - { canvas: HTMLCanvasElement; renderer: Watch.Video.Renderer; emitter: Watch.Audio.Emitter } - >(); + #remotes = new Map< + Moq.Path.Valid, + { canvas: HTMLCanvasElement; renderer: Watch.Video.Renderer; emitter: Watch.Audio.Emitter } + >();
142-146: Reuse the connection instead of redialing for publish.You already left a TODO; consider sharing the established transport to avoid extra sockets.
I can sketch a small helper to pass this.connection.established to instances if you want.
223-224: Guard custom element registration (avoids HMR/double‑define crashes).Safe in dev/hot‑reload contexts.
Apply this diff:
-customElements.define("hang-meet", HangMeet); +if (!customElements.get("hang-meet")) { + customElements.define("hang-meet", HangMeet); +}js/hang/src/publish/chat/typing.ts (1)
32-38: Optional: suppress duplicate writes.If the parent effect reruns for unrelated deps, we may rewrite identical values. Gate on change if this track is chat‑spam sensitive.
Apply this diff:
- const active = effect.get(this.active); - track.writeBool(active); + const active = effect.get(this.active); + const last = this.active.peek(); + if (active !== last) { + track.writeBool(active); + } else { + // Still touch the dep so reruns occur on change. + void active; + }js/hang/src/publish/location/index.ts (2)
2-2: Import Catalog directly from the catalog module for consistency.Other publishers import as
import * as Catalog from "../../catalog";. Align for clarity unless../..intentionally re‑exports.Apply this diff:
-import { Catalog } from "../.."; +import * as Catalog from "../../catalog";If
Catalogis purposefully re‑exported atsrc/index.ts, feel free to ignore.
33-36: Avoid emitting undefined fields in the catalog.Only include peers when defined to keep the schema minimal.
Apply this diff:
- effect.set(this.catalog, { - peers: peers, - ...myself, - }); + const next = { ...(myself ?? {}) , ...(peers ? { peers } : {}) }; + effect.set(this.catalog, next);js/hang/src/watch/element.ts (1)
65-68: Consider usingsetinstead ofupdatefor clarityThe change from
settoupdatehere seems unnecessary. Since you're not using the previous value in the transformation and just returningundefined, usingset(undefined)after callingclose()would be clearer and more direct.- this.active.update((prev) => { - prev?.close(); - return undefined; - }); + const prev = this.active.peek(); + prev?.close(); + this.active.set(undefined);js/hang/src/publish/chat/message.ts (1)
37-37: Possible null reference in string writeThe code writes
latest ?? ""butlatestis defined asSignal<string>with an initial value of"", so it should never be null/undefined. The nullish coalescing operator seems unnecessary here.- track.writeString(latest ?? ""); + track.writeString(latest);js/hang/src/watch/location/index.ts (1)
16-16: UnusedsignalsEffect memberThe
signalsmember is created but never used throughout the class. It's only closed in theclose()method. Consider removing it if it's not needed.- signals = new Effect(); constructor( broadcast: Signal<Moq.Broadcast | undefined>, catalog: Signal<Catalog.Root | undefined>, props?: Props, ) { this.window = new Window(broadcast, catalog, props?.window); this.peers = new Peers(broadcast, catalog, props?.peers); } close() { - this.signals.close(); this.window.close(); this.peers.close(); }js/hang/src/watch/video/detection.ts (1)
53-53: Consider ifupdateis necessary hereUsing
update(() => frame)with a function that returns a new reference might bypass the equality check unnecessarily. Sinceframeis a new object fromZod.read, you could useset(frame)directly.- this.objects.update(() => frame); + this.objects.set(frame);js/hang/src/watch/location/window.ts (1)
61-73: Consider error handling in track reading loopThe
#runTrackmethod silently breaks on falsy position values but doesn't distinguish between legitimate end-of-stream and decoding errors. Consider logging errors for debugging.async #runTrack(track: Moq.Track) { try { for (;;) { const position = await Zod.read(track, Catalog.PositionSchema); if (!position) break; this.#position.set(position); } + } catch (err) { + console.warn("Error reading position track:", err); } finally { this.#position.set(undefined); track.close(); } }js/hang/src/publish/audio/captions.ts (2)
9-15: Remove unusedtranscribeprop or wire it into behavior
transcribe?: booleanis declared but unused. Drop it or gate Worker/WebAudio startup on it.Possible fix (remove the prop):
export type CaptionsProps = { enabled?: boolean | Signal<boolean>; - transcribe?: boolean; // Captions are cleared after this many milliseconds. (10s default) ttl?: Time.Milli; };
69-75: Minor: log message namingThe error logs say “VAD worker error” in the captions worker path; rename for clarity.
- console.error("VAD worker error:", data.message); + console.error("Captions worker error:", data.message);js/hang/src/publish/chat/index.ts (1)
27-33: Emitundefinedcatalog when both message/typing are absentAvoid publishing
{ message: undefined, typing: undefined }to reduce churn and match optional semantics.- this.#signals.effect((effect) => { - this.#catalog.set({ - message: effect.get(this.message.catalog), - typing: effect.get(this.typing.catalog), - }); - }); + this.#signals.effect((effect) => { + const message = effect.get(this.message.catalog); + const typing = effect.get(this.typing.catalog); + this.#catalog.set(message || typing ? { message, typing } : undefined); + });js/moq/src/ietf/subscriber.ts (1)
161-177: Close/drain the object stream on normal completionOn success path the
Readerstream isn’t explicitly closed/drained; rely on protocol semantics or explicitly drain/close to avoid leaks.- producer.close(); + producer.close(); + // Optionally ensure the stream is fully drained/closed. + try { + await stream.done?.(); + } catch {}js/moq/src/group.ts (1)
96-99: Align bool decoding with Track semanticsValidate size/value like
Track.readBool()to catch malformed frames.- 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) return undefined; + if (frame.byteLength !== 1 || !(frame[0] === 0 || frame[0] === 1)) { + throw new Error("invalid bool frame"); + } + return frame[0] === 1; + }js/hang/src/publish/audio/speaking.ts (2)
40-43: Consider reversing the guard condition order for clarity.Since the source is critical for the audio processing, checking it first would make the control flow clearer and align with the pattern used in other similar methods.
Apply this diff:
serve(track: Moq.Track, effect: Effect): void { + const source = effect.get(this.source); + if (!source) return; + const enabled = effect.get(this.enabled); if (!enabled) return; - - const source = effect.get(this.source); - if (!source) return;
72-76: Consider making the sample rate configurable.The sample rate is hardcoded to 16000 Hz for the model. If different models with different sample rate requirements are used in the future, this should be configurable.
Consider extracting this as a constant or making it part of the configuration:
+const VAD_SAMPLE_RATE = 16000; // Required by the voice activity detection model + const ctx = new AudioContext({ latencyHint: "interactive", - sampleRate: 16000, // required by the model. + sampleRate: VAD_SAMPLE_RATE, });js/hang/src/watch/location/peers.ts (2)
45-46: Track closure happens twice in different contexts.The track is closed in the effect cleanup (line 46) and also in the finally block of
#runTrack(line 61). While this is safe due to idempotent close operations, it could lead to confusion.Consider removing the redundant close in the finally block since the effect cleanup already handles it:
} finally { this.#positions.set(undefined); - track.close(); }
51-63: Consider adding error handling for Zod parsing failures.The
Zod.readcall could throw if the data doesn't match the schema. Currently, any parsing error would reset positions to undefined, which might not be the intended behavior for transient parsing failures.async #runTrack(track: Moq.Track) { try { for (;;) { - const frame = await Zod.read(track, Catalog.PeersSchema); - if (!frame) break; - - this.#positions.set(frame); + try { + const frame = await Zod.read(track, Catalog.PeersSchema); + if (!frame) break; + + this.#positions.set(frame); + } catch (err) { + console.error("Failed to parse peers frame:", err); + // Continue to next frame instead of breaking the loop + continue; + } } } finally { this.#positions.set(undefined); track.close(); } }js/hang/src/frame.ts (1)
157-159: Silent error swallowing could hide issues.Catching and ignoring all errors without logging makes debugging difficult. At minimum, errors should be logged at a debug level.
-} catch (_err) { - // Ignore errors, we close groups on purpose to skip them. +} catch (err) { + // Ignore errors from intentionally closed groups, but log unexpected ones + if (err instanceof Error && !err.message.includes("closed")) { + console.debug("Error processing group:", err); + } }js/moq/src/lite/publisher.ts (1)
177-198: Minor: Double-closing is benign but noisy.
#runTrackcloses bothtrackandstream; caller also closes inrunSubscribe. Not harmful; just noting duplication.Option: drop one close at the caller or here to avoid redundant logs.
js/hang/src/publish/audio/encoder.ts (1)
203-219: Guard against zero channels before accessingchannels[0].If
worklet.channelCountis 0/mismatched,channels[0]is undefined.Apply:
const channels = data.channels.slice(0, worklet.channelCount); -const joinedLength = channels.reduce((a, b) => a + b.length, 0); +if (channels.length === 0) return; +const joinedLength = channels.reduce((a, b) => a + b.length, 0); ... -numberOfFrames: channels[0].length, +numberOfFrames: channels[0].length,js/hang/src/publish/location/window.ts (1)
44-52: Confirm write semantics: isZod.writesynchronous?If
Zod.writereturns a Promise, this should be awaited andservemarked async to avoid silent failures.Please confirm
@kixelated/moq/zod.writesignature. If async:-serve(track: Moq.Track, effect: Effect): void { +async serve(track: Moq.Track, effect: Effect): Promise<void> { ... - Zod.write(track, position, Catalog.PositionSchema); + await Zod.write(track, position, Catalog.PositionSchema); }js/moq/src/announced.ts (1)
64-75: Minor:next()mutates queue in place; no notification.Fine internally, but if external observers read
queue, they won’t see shifts. Document thatqueueis internal‑use.Suggestion: after
shift(), optionallystate.queue.set(state.queue.peek(), /*notify*/ false)to keep semantics explicit.js/moq/src/connection/reload.ts (1)
81-99: Improve UX on failures: setstatusto"disconnected"immediately; add jitter.Currently status may linger as
"connecting"until the next tick; also consider jitter to avoid herd effects.Apply:
try { const pending = connect(url, { websocket: this.websocket, webtransport: this.webtransport }); const connection = await Promise.race([effect.cancel, pending]); if (!connection) { pending.then((conn) => conn.close()).catch(() => {}); return; } ... await Promise.race([effect.cancel, connection.closed]); } catch (err) { console.warn("connection error:", err); + effect.set(this.status, "disconnected"); - const tick = this.#tick.peek() + 1; - effect.timer(() => this.#tick.update((prev) => Math.max(prev, tick)), this.#delay); + const tick = this.#tick.peek() + 1; + const jitter = Math.random() * Math.min(250, this.#delay * 0.1); + effect.timer(() => this.#tick.update((prev) => Math.max(prev, tick)), this.#delay + jitter); this.#delay = Math.min(this.#delay * this.delay.multiplier, this.delay.max); }Also applies to: 100-107
js/hang/src/watch/preview.ts (2)
27-29: Use effect.set for dependency-tracked writesAlign with other modules and ensure dependency tracking when writing inside an effect.
- this.#signals.effect((effect) => { - this.#catalog.set(effect.get(catalog)?.preview); - }); + this.#signals.effect((effect) => { + effect.set(this.#catalog, effect.get(catalog)?.preview); + });
40-41: Outdated comment references preview.jsonThe track name is catalog-derived now. Update the comment.
- // Subscribe to the preview.json track directly + // Subscribe to the catalog-provided preview trackjs/moq/src/broadcast.ts (1)
37-41: Consume atomically with mutate to avoid subtle racesKeeps queue ops consistent with the signal’s mutation model.
- for (;;) { - const track = this.state.requested.peek().shift(); + for (;;) { + const track = this.state.requested.mutate((requested) => requested.shift()); if (track) return track;Also applies to: 46-47
js/hang/src/publish/preview.ts (2)
30-37: Option: stream updates when info changesIf preview can change, write on info updates rather than once per subscription.
- serve(track: Moq.Track, effect: Effect): void { - if (!effect.get(this.enabled)) return; - - const info = effect.get(this.info); - if (!info) return; - - track.writeJson(info); - } + serve(track: Moq.Track, effect: Effect): void { + effect.effect((inner) => { + if (!inner.get(this.enabled)) return; + const info = inner.get(this.info); + if (!info) return; + track.writeJson(info); + }); + }
14-15: Option: validate before publishingGuard against bad payloads in dev builds with Zod.parse to fail fast.
+// import { z } from "zod"; // if not already available via Catalog ... - track.writeJson(info); + // In dev, validate shape before writing + // Catalog.PreviewSchema.parse(info); + track.writeJson(info);Also applies to: 30-37
js/hang/src/meet/room.ts (1)
87-89: Redundant url checkEstablished.url is always present; drop the guard.
- const url = connection.url; - if (!url) return; -js/moq/src/track.ts (1)
161-165: Minor: reuse TextDecoderAvoid per-call allocations on hot paths.
+const TEXT_DECODER = new TextDecoder(); ... async readString(): Promise<string | undefined> { const next = await this.readFrame(); if (!next) return undefined; - return new TextDecoder().decode(next); + return TEXT_DECODER.decode(next); }js/moq/src/lite/subscriber.ts (1)
101-101: Parameter naming inconsistency.The parameter name
broadcastis misleading since it represents a path, not a Broadcast instance. Consider renaming tobroadcastPathor justpathfor clarity.-async #runSubscribe(broadcast: Path.Valid, request: TrackRequest) { +async #runSubscribe(broadcastPath: Path.Valid, request: TrackRequest) {js/moq/src/ietf/publisher.ts (1)
191-192: TODO comment for UNSUBSCRIBE handler.This TODO should be tracked to ensure proper cleanup when clients unsubscribe.
Would you like me to create an issue to track implementing the UNSUBSCRIBE handler to properly clean up resources when clients disconnect?
js/hang/package.json
Outdated
| "./connection": "./src/connection.ts" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the ./connection subpath and publish outputs.
Confirm src/connection.ts is the intended stable entry and that the publish step rewrites exports to built dist/* files (not .ts) to avoid consumer toolchain issues.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Ensure source exists and exports public API
test -f js/hang/src/connection.ts || { echo "missing: js/hang/src/connection.ts"; exit 1; }
rg -nC1 -g 'js/hang/src/connection.ts' -nP '^\s*export\s+'
# Sanity: no lingering imports of removed subpaths
rg -nP '@kixelated/hang/(container|preview)' -g '!**/dist/**' || trueLength of output: 34
Fix export mapping: ./connection points to a missing source — publish must reference built files
js/hang/package.json exports "./connection" → "./src/connection.ts" but js/hang/src/connection.ts is missing (verification: "missing: js/hang/src/connection.ts"). Either add the missing src/connection.ts or update the export to point at the built outputs (e.g., ./dist/connection.js and ./dist/connection.d.ts) and ensure the publish/build step rewrites subpath exports to dist/* (not .ts). Location: js/hang/package.json (around lines 19–20) and js/hang/src/connection.ts (missing).
🤖 Prompt for AI Agents
In js/hang/package.json around lines 19–20 the export "./connection" points at
"./src/connection.ts" which is missing; update the subpath export to point to
the built artifacts (e.g., "./dist/connection.js" and "./dist/connection.d.ts")
or add the missing js/hang/src/connection.ts file and ensure your build outputs
those dist files. Modify package.json exports to reference dist/* (both .js and
.d.ts) and update your build/publish step to rewrite/emit subpath exports to the
dist folder so published packages don't reference .ts sources.
| effect.effect((nested) => { | ||
| const text = nested.get(this.text) ?? ""; | ||
| this.#track.writeString(text); | ||
| track.writeString(text); | ||
|
|
There was a problem hiding this comment.
Avoid throwing when the remote closes the track
track.writeString throws if the track is closed; throwing inside an Effect will tear down this effect tree. Guard and catch to make writes best‑effort.
Apply this diff:
- const text = nested.get(this.text) ?? "";
- track.writeString(text);
+ const text = nested.get(this.text) ?? "";
+ try {
+ if (!track.state.closed.peek()) {
+ track.writeString(text);
+ }
+ } catch (err) {
+ if (!track.state.closed.peek()) {
+ console.warn("captions write failed:", err);
+ }
+ }📝 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.effect((nested) => { | |
| const text = nested.get(this.text) ?? ""; | |
| this.#track.writeString(text); | |
| track.writeString(text); | |
| effect.effect((nested) => { | |
| const text = nested.get(this.text) ?? ""; | |
| try { | |
| if (!track.state.closed.peek()) { | |
| track.writeString(text); | |
| } | |
| } catch (err) { | |
| if (!track.state.closed.peek()) { | |
| console.warn("captions write failed:", err); | |
| } | |
| } |
🤖 Prompt for AI Agents
In js/hang/src/publish/audio/captions.ts around lines 57 to 60, the effect calls
track.writeString directly which will throw if the remote has closed the track
and will tear down the effect tree; wrap the write in a defensive guard and/or
try/catch so writes are best‑effort: first check any available
track.closed/readyState flag if provided, then call writeString inside a
try/catch and swallow or log the error (do not rethrow) so the Effect does not
throw and the effect tree remains intact.
| 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(Typing.TRACK); | ||
| }); |
There was a problem hiding this comment.
Catalog not cleared when disabled; use effect.set.
Mirror other publishers so disabling removes the catalog entry.
Apply this diff:
- this.catalog.set(Typing.TRACK);
+ effect.set(this.catalog, Typing.TRACK);📝 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) => { | |
| 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(Typing.TRACK); | |
| }); | |
| this.#signals.effect((effect) => { | |
| const enabled = effect.get(this.enabled); | |
| if (!enabled) return; | |
| effect.set(this.catalog, Typing.TRACK); | |
| }); |
🤖 Prompt for AI Agents
In js/hang/src/publish/chat/typing.ts around lines 24 to 29, the effect
currently only sets the catalog entry when enabled but does not remove it when
disabled; update the effect to call effect.set(...) instead of directly mutating
the catalog so it mirrors other publishers — when enabled set the catalog to
Typing.TRACK, otherwise call effect.set(undefined) (or the equivalent to remove
the entry) so disabling clears the catalog entry.
| this.signals.effect((effect) => { | ||
| if (!effect.get(this.enabled)) return; | ||
|
|
||
| const info = effect.get(this.info); | ||
| if (!info) return; | ||
|
|
||
| this.#publish(info); | ||
| effect.set(this.catalog, Preview.TRACK); | ||
| }); |
There was a problem hiding this comment.
Catalog entry not cleared when disabled → stale advertisement
When enabled flips to false, catalog still exposes preview.json. Set to undefined.
- this.signals.effect((effect) => {
- if (!effect.get(this.enabled)) return;
- effect.set(this.catalog, Preview.TRACK);
- });
+ this.signals.effect((effect) => {
+ effect.set(this.catalog, effect.get(this.enabled) ? Preview.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.
| this.signals.effect((effect) => { | |
| if (!effect.get(this.enabled)) return; | |
| const info = effect.get(this.info); | |
| if (!info) return; | |
| this.#publish(info); | |
| effect.set(this.catalog, Preview.TRACK); | |
| }); | |
| this.signals.effect((effect) => { | |
| effect.set(this.catalog, effect.get(this.enabled) ? Preview.TRACK : undefined); | |
| }); |
🤖 Prompt for AI Agents
In js/hang/src/publish/preview.ts around lines 24–27, the effect callback
currently returns when enabled is false which leaves the catalog entry stale;
change the logic so the effect sets the catalog to Preview.TRACK when enabled is
true and sets the catalog to undefined (clearing the entry) when enabled is
false, i.e. replace the early return with a conditional that calls
effect.set(this.catalog, Preview.TRACK) if enabled and effect.set(this.catalog,
undefined) otherwise.
| for (const added of newActive.difference(active)) { | ||
| console.debug(`announce: broadcast=${added} active=true`); | ||
| const wire = new Announce(added, true); | ||
| await wire.encode(stream.writer); | ||
| } | ||
|
|
||
| const wire = new Announce(announcement.name, announcement.active); | ||
| await wire.encode(stream.writer); | ||
| // Announce any removed broadcasts. | ||
| for (const removed of active.difference(newActive)) { | ||
| console.debug(`announce: broadcast=${removed} active=false`); | ||
| const wire = new Announce(removed, false); | ||
| await wire.encode(stream.writer); | ||
| } |
There was a problem hiding this comment.
Bug: Using nonstandard Set.prototype.difference.
Standard JS Set has no difference. This code will crash.
Apply:
-// Announce any new broadcasts.
-for (const added of newActive.difference(active)) {
+// Announce any new broadcasts.
+for (const added of newActive) {
+ if (active.has(added)) continue;
console.debug(`announce: broadcast=${added} active=true`);
const wire = new Announce(added, true);
await wire.encode(stream.writer);
}
-// Announce any removed broadcasts.
-for (const removed of active.difference(newActive)) {
+// Announce any removed broadcasts.
+for (const removed of active) {
+ if (newActive.has(removed)) continue;
console.debug(`announce: broadcast=${removed} active=false`);
const wire = new Announce(removed, false);
await wire.encode(stream.writer);
}📝 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.
| for (const added of newActive.difference(active)) { | |
| console.debug(`announce: broadcast=${added} active=true`); | |
| const wire = new Announce(added, true); | |
| await wire.encode(stream.writer); | |
| } | |
| const wire = new Announce(announcement.name, announcement.active); | |
| await wire.encode(stream.writer); | |
| // Announce any removed broadcasts. | |
| for (const removed of active.difference(newActive)) { | |
| console.debug(`announce: broadcast=${removed} active=false`); | |
| const wire = new Announce(removed, false); | |
| await wire.encode(stream.writer); | |
| } | |
| // Announce any new broadcasts. | |
| for (const added of newActive) { | |
| if (active.has(added)) continue; | |
| console.debug(`announce: broadcast=${added} active=true`); | |
| const wire = new Announce(added, true); | |
| await wire.encode(stream.writer); | |
| } | |
| // Announce any removed broadcasts. | |
| for (const removed of active) { | |
| if (newActive.has(removed)) continue; | |
| console.debug(`announce: broadcast=${removed} active=false`); | |
| const wire = new Announce(removed, false); | |
| await wire.encode(stream.writer); | |
| } |
🤖 Prompt for AI Agents
In js/moq/src/lite/publisher.ts around lines 100 to 111 the code calls
nonstandard Set.prototype.difference which will throw at runtime; replace those
calls by iterating the source set and checking membership in the other set
(e.g., for (const item of newActive) if (!active.has(item)) { ... } and for
(const item of active) if (!newActive.has(item)) { ... }) so you still create
Announce for added and removed items and await wire.encode(stream.writer) for
each; ensure you preserve the same logging and boolean active flag semantics.
| const announce = await Promise.race([Announce.decodeMaybe(stream.reader), announced.closed]); | ||
| if (!announce) break; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify proper cleanup on Promise.race with closed property.
The race condition between Announce.decodeMaybe and announced.closed looks correct, but ensure that announced.closed is a Promise that resolves when the Announced instance is closed.
🏁 Script executed:
#!/bin/bash
# Verify that announced.closed is indeed a Promise property
ast-grep --pattern 'class Announced {
$$$
readonly closed: Promise<$_>
$$$
}'Length of output: 2707
Handle announced.closed aborts as errors (don't treat resolved Error as an announcement).
announced.closed is Promise<Error | undefined> (js/moq/src/announced.ts:27–36) and resolves with an Error on abort; the race in js/moq/src/lite/subscriber.ts (lines ~65–66) only does if (!announce) break; — a resolved Error is truthy and will be treated as an announcement.
- Fix A (recommended): reject the closed promise on abort so Promise.race throws.
- Fix B: keep current resolved value but handle it after the race:
if (announce instanceof Error) throw announce; if (!announce) break;.
🤖 Prompt for AI Agents
In js/moq/src/lite/subscriber.ts around lines 65–66, the Promise.race between
Announce.decodeMaybe(stream.reader) and announced.closed treats a resolved Error
from announced.closed as a valid announcement; change this so aborts are handled
as errors: preferred fix — modify announced.closed (in js/moq/src/announced.ts)
to reject the promise when aborted so Promise.race will throw and be caught as
an error; alternative fix — keep announced.closed resolving but immediately
after the race check if announce is an Error and throw it (if announce
instanceof Error throw announce), otherwise continue to handle null/undefined as
before.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/signals/src/index.ts (1)
21-23: Signal.set no longer matches Setter; TypeScript contract is broken.Setter declares set(value | updater), but Signal.set now only accepts value(+notify). This will fail “implements Setter” checks and can break callers that pass updater lambdas.
Apply:
-export interface Setter<T> { - set(value: T | ((prev: T) => T)): void; -} +export interface Setter<T> { + set(value: T | ((prev: T) => T), notify?: boolean): void; +}-// If notify is undefined, we'll check if the value has changed after the microtask. -set(value: T, notify?: boolean): void { - const old = this.#value; - this.#value = value; +// If notify is undefined, we'll check if the value has changed after the microtask. +set(valueOrFn: T | ((prev: T) => T), notify?: boolean): void { + const value = typeof valueOrFn === "function" ? (valueOrFn as (prev: T) => T)(this.#value) : valueOrFn; + const old = this.#value; + this.#value = value;This preserves back-compat while keeping the new notify flag.
Also applies to: 47-59
js/hang/src/publish/audio/encoder.ts (1)
215-223: Invalid AudioDataInit property “transfer”.AudioData constructor doesn’t accept a transfer list; this will fail type-checking. Remove it (passing Float32Array as BufferSource is fine).
Apply:
-const frame = new AudioData({ +const frame = new AudioData({ format: "f32-planar", sampleRate: worklet.context.sampleRate, numberOfFrames: channels[0].length, numberOfChannels: channels.length, timestamp: data.timestamp, - data: joined, - transfer: [joined.buffer], + data: joined, });
♻️ Duplicate comments (8)
js/signals/src/solid.ts (1)
8-9: Subscription cleanup tied to Solid root — leak resolved.Capturing
disposeand calling it inonCleanupaddresses the leak noted earlier. Nice fix.js/hang/package.json (1)
8-19: Removal of legacy subpath exports looks good; prior ./connection mismatch is resolved.Previous review flagged "./connection" pointing to a missing file—now removed, which resolves it. One ask: exports still reference .ts sources. Please confirm the publish step rewrites these to dist .js/.d.ts to avoid consumer(Node) toolchain issues.
#!/bin/bash set -euo pipefail # 1) Locate packaging script and verify it rewrites .ts -> .js in exports fd -a package.ts | sed -n '1,120p' rg -nP 'exports|\.ts|\.d\.ts|dist' -n $(fd -a package.ts) # 2) Sanity: ensure all exported entrypoints have corresponding dist/* outputs after build # (Run build first in your environment) printf "\nExpect these dist files to exist after build:\n" cat <<'EOF' dist/index.js dist/publish/index.js dist/publish/element.js dist/watch/index.js dist/watch/element.js dist/meet/index.js dist/meet/element.js dist/catalog/index.js dist/support/index.js dist/support/element.js EOF # 3) If you can build locally, pack and list tarball contents # pnpm -F @kixelated/hang build && pnpm -F @kixelated/hang pack --pack-destination /tmp # tar -tf /tmp/kixelated-hang-*.tgz | rg -nP '(^package/src/|\.ts$)' || truejs/moq/src/broadcast.ts (1)
60-64: Priority handling is correct now.Ascending sort with pop() yields highest priority first; aligns with earlier review intent.
Also applies to: 38-48
js/moq/src/lite/publisher.ts (3)
65-67: Protocol robustness: always send ANNOUNCE_INIT (even if empty).Early return skips ANNOUNCE_INIT; peers can hang waiting for init. Use empty map when closed.
- const broadcasts = this.#broadcasts.peek(); - if (!broadcasts) return; // closed + const broadcasts = this.#broadcasts.peek() ?? new Map<Path.Valid, Broadcast>();
75-76: Bug: iterator has no .toArray(); use Array.from.This throws at runtime.
- const init = new AnnounceInit(active.values().toArray()); + const init = new AnnounceInit(Array.from(active));
100-112: Bug: Set.prototype.difference is nonstandard.Will crash; compute added/removed via membership checks.
- // Announce any new broadcasts. - for (const added of newActive.difference(active)) { + // Announce any new broadcasts. + for (const added of newActive) { + if (active.has(added)) continue; console.debug(`announce: broadcast=${added} active=true`); const wire = new Announce(added, true); await wire.encode(stream.writer); } - // Announce any removed broadcasts. - for (const removed of active.difference(newActive)) { + // Announce any removed broadcasts. + for (const removed of active) { + if (newActive.has(removed)) continue; console.debug(`announce: broadcast=${removed} active=false`); const wire = new Announce(removed, false); await wire.encode(stream.writer); }js/hang/src/publish/video/encoder.ts (2)
132-151: Guard framerate and compute bitrate with a number; set framerate in config.settings.frameRate can be undefined; bestBitrate expects a number and config should get a concrete framerate.
- const bitrate = bestBitrate({ - codec, - width, - height, - framerate: settings.frameRate, - }); + const framerate = settings.frameRate ?? 30; + const bitrate = bestBitrate({ codec, width, height, framerate }); ... - const config: VideoEncoderConfig = { + const config: VideoEncoderConfig = { codec, width, height, - framerate: settings.frameRate, + framerate, bitrate, avc: codec.startsWith("avc1") ? { format: "annexb" } : undefined, // @ts-expect-error Typescript needs to be updated. hevc: codec.startsWith("hev1") ? { format: "annexb" } : undefined, latencyMode: "realtime", hardwareAcceleration: "prefer-hardware", };
173-196: Catalog safety: avoid NaN/undefined in bitrate/aspect; bail or default.Width/height may be absent; passing undefined to bestBitrate/u53 is unsafe.
- const settings = source.getSettings(); - const bitrate = bestBitrate({ - codec, - width: settings.width, - height: settings.height, - framerate: settings.frameRate, - }); + const settings = source.getSettings(); + const width = settings.width; + const height = settings.height; + if (width === undefined || height === undefined) return; // wait until known + const framerate = settings.frameRate ?? 30; + const bitrate = bestBitrate({ codec, width, height, framerate }); ... - bitrate: u53(bitrate), - displayAspectWidth: u53(settings.width), - displayAspectHeight: u53(settings.height), - - framerate: settings.frameRate, + bitrate: u53(bitrate), + displayAspectWidth: u53(width), + displayAspectHeight: u53(height), + framerate,
🧹 Nitpick comments (6)
js/signals/src/solid.ts (2)
8-9: Trim tiny closures in subscribe/onCleanup.Slightly simpler and more idiomatic in Solid; also avoids an extra per-notification allocation.
- const dispose = signal.subscribe((value) => set(() => value)); - onCleanup(() => dispose()); + const dispose = signal.subscribe((value) => set(value)); + onCleanup(dispose);
1-1: Optional import style nit.You can split value vs type imports for clarity (no behavior change):
- import { createSignal, onCleanup, type Accessor as SolidAccessor } from "solid-js"; + import { createSignal, onCleanup } from "solid-js"; + import type { Accessor as SolidAccessor } from "solid-js";js/hang/package.json (3)
4-4: Semver signaling: 0.5.0 includes breaking export removals—call this out as “breaking.”Minor bumps in 0.x are treated as breaking by convention; ensure release notes/changeset clearly mark removal of ./container, ./connection, ./preview.
Can you confirm a “BREAKING CHANGES” section will be included in the 0.5.0 release notes?
32-36: Publishing depends on bun; declare toolchain or guard the script.Release script invokes bun. Add an engines/packageManager field or guard to fail with a clear message if bun is missing.
+ "packageManager": "pnpm@9", + "engines": { "node": ">=18.18" }, "scripts": { "build": "rimraf dist && tsc -b && bun ../scripts/package.ts", "check": "tsc --noEmit", "test": "vitest", "release": "bun ../scripts/release.ts" },
37-46: Heavy AI/runtime deps: consider optional/peer to keep baseline install + bundle size small.@huggingface/transformers and the WebCodecs polyfill are large. Given “AI models run on-demand,” ship them as optional peer deps and dynamically import where needed. Keeps cold path light and avoids bundling for apps not using captions/AI.
"dependencies": { - "@huggingface/transformers": "^3.7.2", - "@kixelated/libavjs-webcodecs-polyfill": "^0.5.5", + // keep core light; load these via dynamic import when requested + // move heavy/conditional deps to peers "@kixelated/moq": "workspace:^", "@kixelated/signals": "workspace:^", "@libav.js/variant-opus-af": "^6.8.8", "async-mutex": "^0.5.0", "comlink": "^4.4.2", "zod": "^4.1.5" }, + "peerDependencies": { + "@huggingface/transformers": ">=3.7.2", + "@kixelated/libavjs-webcodecs-polyfill": ">=0.5.5" + }, + "peerDependenciesMeta": { + "@huggingface/transformers": { "optional": true }, + "@kixelated/libavjs-webcodecs-polyfill": { "optional": true } + },js/moq/src/broadcast.ts (1)
54-59: Avoid constructing Track when broadcast is already closed.Minor: check closed before new Track to avoid creating/leaking an unused instance on error.
Apply:
-subscribe(name: string, priority: number): Track { - const track = new Track(name); - - if (this.state.closed.peek()) { - throw new Error(`broadcast is closed: ${this.state.closed.peek()}`); - } +subscribe(name: string, priority: number): Track { + if (this.state.closed.peek()) { + throw new Error(`broadcast is closed: ${this.state.closed.peek()}`); + } + const track = new Track(name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
js/hang/package.json(3 hunks)js/hang/src/publish/audio/encoder.ts(9 hunks)js/hang/src/publish/chat/message.ts(1 hunks)js/hang/src/publish/chat/typing.ts(1 hunks)js/hang/src/publish/location/peers.ts(1 hunks)js/hang/src/publish/video/encoder.ts(3 hunks)js/hang/src/publish/video/types.ts(1 hunks)js/moq/src/broadcast.ts(1 hunks)js/moq/src/lite/publisher.ts(9 hunks)js/signals/src/index.ts(7 hunks)js/signals/src/solid.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- js/hang/src/publish/location/peers.ts
- js/hang/src/publish/chat/typing.ts
🧰 Additional context used
📓 Path-based instructions (1)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
{rs,js}/hang/**: All media-specific logic (encoding/streaming, codec handling) must live in the hang layer.
Keep the hang layer generic: expose primitives (e.g., access to individual frames) and avoid application/UI-specific code.
Files:
js/hang/src/publish/video/types.tsjs/hang/package.jsonjs/hang/src/publish/video/encoder.tsjs/hang/src/publish/audio/encoder.tsjs/hang/src/publish/chat/message.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-11T20:16:03.134Z
Learning: Applies to rs/moq-relay/** : In the CDN/relay (moq-relay), do not include application logic, media codecs, or track-specific details; keep it transport-agnostic.
🧬 Code graph analysis (6)
js/signals/src/solid.ts (1)
js/signals/src/index.ts (1)
Getter(10-19)
js/hang/src/publish/video/encoder.ts (4)
js/hang/src/publish/video/detection.ts (1)
Detection(15-87)js/hang/src/publish/video/types.ts (2)
Source(1-1)VideoStreamTrack(4-8)js/hang/src/publish/video/polyfill.ts (1)
VideoTrackProcessor(7-64)js/hang/src/util/hacks.ts (1)
isFirefox(5-5)
js/moq/src/broadcast.ts (3)
js/moq/src/track.ts (1)
Track(9-190)js/signals/src/index.ts (2)
Signal(25-145)closed(522-524)js/hang/src/publish/broadcast.ts (2)
Broadcast(23-157)track(123-146)
js/hang/src/publish/audio/encoder.ts (6)
js/hang/src/publish/video/encoder.ts (4)
effect(52-78)effect(166-200)track(93-163)Encoder(22-211)js/signals/src/index.ts (2)
effect(376-386)Effect(150-529)js/hang/src/publish/audio/speaking.ts (2)
Speaking(13-111)effect(30-38)js/hang/src/publish/audio/captions.ts (2)
Captions(17-131)effect(39-47)js/moq/src/track.ts (1)
Track(9-190)js/moq/src/group.ts (1)
Group(9-104)
js/moq/src/lite/publisher.ts (4)
js/signals/src/index.ts (3)
Signal(25-145)Dispose(3-3)changed(119-122)js/moq/src/broadcast.ts (1)
Broadcast(19-83)js/moq/src/track.ts (1)
Track(9-190)js/moq/src/stream.ts (2)
Writer(223-346)next(420-424)
js/hang/src/publish/chat/message.ts (6)
js/hang/src/watch/chat/message.ts (2)
MessageProps(6-10)Message(12-72)js/moq/src/track.ts (1)
Track(9-190)js/hang/src/publish/broadcast.ts (2)
effect(55-71)track(123-146)js/hang/src/publish/audio/speaking.ts (1)
effect(30-38)js/hang/src/publish/audio/captions.ts (1)
effect(39-47)js/hang/src/publish/video/detection.ts (1)
effect(38-45)
⏰ 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 (6)
js/signals/src/solid.ts (1)
6-7: Confirm upstream de‑duplication before forcingequals: false.If the external
signal.subscribecan emit identical values, disabling Solid’s equality check may trigger unnecessary updates. If upstream already de‑dupes, you’re good; otherwise consider keeping the default or adding a shallow/Object.is gate.js/hang/package.json (1)
39-39: WebCodecs polyfill: ensure conditional loading to avoid conflicts/perf hits on browsers that already support WebCodecs.If not already, load via feature detect (e.g., if (!('VideoEncoder' in globalThis))) and dynamic import to keep it off modern paths.
Can you confirm the polyfill is only imported when WebCodecs is unavailable?
js/signals/src/index.ts (1)
112-114: Leak check fix looks good.Using Number.isInteger(Math.log10(size)) correctly trips at powers of 10 (e.g., 100, 1000). Thanks for addressing the previous review.
js/hang/src/publish/audio/encoder.ts (1)
171-189: Rollover initialization fix is correct.Initializing groupTimestamp lazily avoids immediate rollover on the first frame. Matches prior guidance.
js/hang/src/publish/video/types.ts (1)
14-20: Make all VideoTrackSettings fields optional (align with MediaTrackSettings)Mark all fields optional (or derive from lib types) and remove the "I'm not sure" comment; verify call sites that assume required fields.
File: js/hang/src/publish/video/types.ts (lines 14-20)
- // I'm not sure what fields are always present. -export interface VideoTrackSettings { - deviceId: string; - groupId: string; - aspectRatio: number; - facingMode: "user" | "environment" | "left" | "right"; - frameRate?: number; - height: number; - resizeMode: "none" | "crop-and-scale"; - width: number; -} +export interface VideoTrackSettings { + deviceId?: string; + groupId?: string; + aspectRatio?: number; + facingMode?: "user" | "environment" | "left" | "right"; + frameRate?: number; + height?: number; + resizeMode?: "none" | "crop-and-scale"; + width?: number; +}Optional alternative (stay synced with lib types):
type VideoTrackSettings = Partial<Pick<MediaTrackSettings, "deviceId"|"groupId"|"aspectRatio"|"facingMode"|"frameRate"|"height"|"resizeMode"|"width">>;Verify call sites that access these properties without guards (run locally):
rg -nP --type=ts -C3 '\.getSettings\(\)\.(?:width|height|aspectRatio|frameRate|deviceId|groupId)\b' rg -nP --type=ts '\bgetSettings\('js/moq/src/lite/publisher.ts (1)
81-89: Listener cleanup LGTM.Disposing the changed() subscription after the race resolves fixes the leak raised earlier.
| "comlink": "^4.4.2", | ||
| "@kixelated/libavjs-webcodecs-polyfill": "^0.5.5", | ||
| "zod": "^4.1.0" | ||
| "zod": "^4.1.5" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Zod bump: align versions workspace‑wide.
Avoid multiple zod copies across packages. Please check all workspaces use ^4.1.5 or compatible.
🏁 Script executed:
#!/bin/bash
# Check zod versions across the repo
rg -nP '"zod"\s*:\s*"\^?[^"]+"' --json | jq -r '.data.path.text + ":" + .data.lines.text'Length of output: 405
Zod bump: align versions workspace‑wide.
js/moq/package.json uses "zod": "^4.1.0" while js/moq-token/package.json and js/hang/package.json use "^4.1.5"; js/bun.lock contains entries for both. Update js/moq/package.json to "^4.1.5" and regenerate the lockfile (e.g., run your package manager) so all workspaces use ^4.1.5 and avoid duplicate zod copies.
🤖 Prompt for AI Agents
In js/hang/package.json around line 45 (and the workspace), zod versions are
inconsistent across workspaces: update js/moq/package.json to set "zod":
"^4.1.5" (matching js/moq-token and js/hang) and then regenerate the workspace
lockfile (run your package manager install/lock command — e.g., npm/yarn/bun
install) so the lockfile is updated and duplicate zod entries are removed.
| serve(track: Moq.Track, effect: Effect): void { | ||
| const enabled = effect.get(this.enabled); | ||
| if (!enabled) return; | ||
|
|
||
| const latest = effect.get(this.latest); | ||
| this.#track.writeString(latest ?? ""); | ||
| }); | ||
| const latest = effect.get(this.latest); | ||
| track.writeString(latest ?? ""); | ||
| } |
There was a problem hiding this comment.
serve() writes only once; it won’t publish subsequent message changes.
Wrap in a nested effect so new messages emit frames, and drop the unnecessary nullish coalescing.
Apply:
-serve(track: Moq.Track, effect: Effect): void {
- const enabled = effect.get(this.enabled);
- if (!enabled) return;
-
- const latest = effect.get(this.latest);
- track.writeString(latest ?? "");
-}
+serve(track: Moq.Track, effect: Effect): void {
+ if (!effect.get(this.enabled)) return;
+ effect.effect((nested) => {
+ track.writeString(nested.get(this.latest));
+ });
+}📝 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.
| serve(track: Moq.Track, effect: Effect): void { | |
| const enabled = effect.get(this.enabled); | |
| if (!enabled) return; | |
| const latest = effect.get(this.latest); | |
| this.#track.writeString(latest ?? ""); | |
| }); | |
| const latest = effect.get(this.latest); | |
| track.writeString(latest ?? ""); | |
| } | |
| serve(track: Moq.Track, effect: Effect): void { | |
| if (!effect.get(this.enabled)) return; | |
| effect.effect((nested) => { | |
| track.writeString(nested.get(this.latest)); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In js/hang/src/publish/chat/message.ts around lines 32 to 38, serve currently
reads the latest value once and writes it once (using nullish coalescing), so
subsequent message updates won't publish; change serve to, after checking
enabled, create a nested effect that subscribes to this.latest and calls
track.writeString(latest) whenever it changes (dropping the "?? ''" coalescing),
so new messages emit frames continuously while enabled.
| // 30fps is the baseline, applying a multiplier for higher framerates. | ||
| // Framerate does not cause a multiplicative increase in bitrate because of delta encoding. | ||
| // TODO Make this better. | ||
| const framerateFactor = 30.0 + (props.framerate ?? 30 - 30) / 2; | ||
| let bitrate = Math.round(pixels * 0.07 * framerateFactor); | ||
|
|
There was a problem hiding this comment.
Bitrate math bug: operator precedence breaks intended formula.
Currently evaluates 30 + (fps/2). Should be 30 + (fps - 30)/2 to match examples below.
- const framerateFactor = 30.0 + (props.framerate ?? 30 - 30) / 2;
+ const framerate = props.framerate ?? 30;
+ const framerateFactor = 30 + (framerate - 30) / 2;📝 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.
| // 30fps is the baseline, applying a multiplier for higher framerates. | |
| // Framerate does not cause a multiplicative increase in bitrate because of delta encoding. | |
| // TODO Make this better. | |
| const framerateFactor = 30.0 + (props.framerate ?? 30 - 30) / 2; | |
| let bitrate = Math.round(pixels * 0.07 * framerateFactor); | |
| // 30fps is the baseline, applying a multiplier for higher framerates. | |
| // Framerate does not cause a multiplicative increase in bitrate because of delta encoding. | |
| // TODO Make this better. | |
| const framerate = props.framerate ?? 30; | |
| const framerateFactor = 30 + (framerate - 30) / 2; | |
| let bitrate = Math.round(pixels * 0.07 * framerateFactor); |
🤖 Prompt for AI Agents
In js/hang/src/publish/video/encoder.ts around lines 223-228, the
framerateFactor calculation suffers from operator precedence: currently 30.0 +
(props.framerate ?? 30 - 30) evaluates as 30 + (fps/2) instead of the intended
30 + (fps - 30)/2. Fix by applying the null-coalescing default first and then
subtracting 30 before dividing; e.g., compute a local framerate =
props.framerate ?? 30 and set framerateFactor = 30.0 + (framerate - 30) / 2 (or
use ( (props.framerate ?? 30) - 30 ) / 2 inline) so the formula matches the
intended behavior.
quick feature list:
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores