feat(watch): latency range with buffered playback#1620
Merged
Conversation
Reframe playback latency as a range [latency-min, latency-max] in @moq/watch, unifying live (minimize) and buffered playback into one mechanism. The reference (wall-clock anchor) is re-anchored only when keeping it would push latency past the ceiling, so future-dated frames (e.g. TTS written faster than real-time) build up a buffer instead of dragging the playhead forward. Minimize latency is the degenerate case where the ceiling equals the floor, so there is no separate buffered code path. latency-max forms: - "real-time" (default) or <= floor: minimize, today's behavior. - a number (ms): buffer up to the cap, then skip ahead. - undefined / "none": uncapped, never skip. latency is sugar that collapses the floor and ceiling to one value; latency-min sets only the floor. reset() flushes the buffer and re-anchors at an utterance boundary. The decoded audio ring stays small (~1.5s). The lookahead lives upstream as encoded frames: the decoder awaits AudioBuffer.wait(timestamp) before decode(), which holds the frame as Opus until the playhead nears it, instead of buffering seconds of PCM. Backpressure works on both transports (Atomics poll on the SharedArrayBuffer path, worklet state messages on the postMessage fallback). A finite latency-max keeps sizing the ring to the cap and dropping via overflow. Follow-up: plumb latency-max into the wire SubscribeUpdate.maxLatency so the relay also reorders/drops groups past the cap (currently sends 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kixelated
force-pushed
the
claude/thirsty-cannon-63c134
branch
from
6c3d19e to
e38c77f
Compare
…-63c134 # Conflicts: # doc/lib/js/@moq/watch.md # js/watch/src/element.ts
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughThis pull request introduces a buffered playback mode for the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/watch/src/element.ts (1)
147-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInternal
latencyreflection collapseslatencyMaxand disables range mode.Reflecting
backend.latencyMininto thelatencyattribute re-entersattributeChangedCallback("latency"), which callsthis.latency = ...and sets bothlatencyMinandlatencyMax. That unintentionally wipes any previously configured ceiling (including the UI flow usingwatch.backend.latencyMin), so buffered playback can silently collapse back to minimize mode.Targeted fix (guard reflected updates)
export default class MoqWatch extends HTMLElement { + `#reflectingLatencyAttribute` = false; @@ this.signals.run((effect) => { const min = effect.get(this.backend.latencyMin); - if (min === "real-time") { - this.setAttribute("latency", "real-time"); - } else { - const jitter = Math.floor(effect.get(this.backend.jitter)); - this.setAttribute("latency", jitter.toString()); - } + const reflected = min === "real-time" ? "real-time" : Math.floor(effect.get(this.backend.jitter)).toString(); + if (this.getAttribute("latency") !== reflected) { + this.#reflectingLatencyAttribute = true; + try { + this.setAttribute("latency", reflected); + } finally { + this.#reflectingLatencyAttribute = false; + } + } }); @@ } else if (name === "latency") { + if (this.#reflectingLatencyAttribute) return; // Sugar: collapse the floor and ceiling to a single value. this.latency = this.#parseLatency(newValue);Also applies to: 230-237, 310-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/element.ts` around lines 147 - 155, The effect that reflects backend.latencyMin into the latency attribute using setAttribute is causing attributeChangedCallback to re-enter and set both latencyMin and latencyMax, which overwrites the previously configured latencyMax and collapses range mode. Guard the reflected updates by detecting when the attribute change is coming from an internal reflection (not an external user action) within attributeChangedCallback, and skip the setter logic that wipes latencyMax in those cases. Apply the same guard pattern to the other instances mentioned at lines 230-237 and 310-313 where similar reflection occurs.
🧹 Nitpick comments (3)
js/watch/src/sync.test.ts (1)
8-62: ⚡ Quick winAdd a reset regression test for backward timestamps between utterances.
This suite covers range derivation well, but not the new utterance-boundary reset semantics. Add a test that calls
reset(), then sends a lower timestamp and assertssync.timestampreflects the new utterance.As per coding guidelines, "
**/*.{test,spec}.{js,ts,tsx,jsx,py,java,cs,go,rb}: Write unit tests for critical functionality`."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/sync.test.ts` around lines 8 - 62, The test suite covers latency range derivation well but lacks coverage for the utterance-boundary reset semantics with backward timestamps. Add a new test case to this describe block that instantiates a Sync object, calls its reset() method, then sets a lower timestamp value, and asserts that sync.timestamp reflects the new utterance correctly. This ensures that backward timestamps between utterances are handled properly after a reset, preventing regressions in the timestamp handling logic.Source: Coding guidelines
js/watch/src/element.ts (1)
196-200: ⚡ Quick winReplace fallback magic number with a named constant.
#parseLatencyuses inline100as a fallback. Extracting this to a named constant avoids drift and makes intent explicit.Suggested refactor
+const DEFAULT_LATENCY_FALLBACK = 100 as Time.Milli; @@ `#parseLatency`(value: string | null): Latency { if (!value || value === "real-time") return "real-time"; const parsed = Number.parseFloat(value); - return (Number.isFinite(parsed) ? parsed : 100) as Time.Milli; + return (Number.isFinite(parsed) ? parsed : DEFAULT_LATENCY_FALLBACK) as Time.Milli; }As per coding guidelines, "Avoid using magic numbers; use named constants instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/element.ts` around lines 196 - 200, The `#parseLatency` method contains a magic number 100 used as a fallback latency value when the parsed value is not finite. Extract this inline 100 to a named constant at the class or module level that clearly describes its purpose (such as DEFAULT_LATENCY or FALLBACK_LATENCY_MS), then replace the inline 100 with this constant reference. This makes the intent explicit and prevents accidental drift if this value needs to be used elsewhere.Source: Coding guidelines
js/watch/src/backend.ts (1)
39-55: ⚡ Quick winUse JSDoc blocks for the new public API surface.
The new
Backend/MultiBackendPropsmembers andreset()are public API additions, but they are documented with line comments instead of/** ... */doc comments.Suggested update shape
export interface Backend { - // The latency floor: "real-time" auto-computes jitter, a number sets a fixed jitter. + /** The latency floor: "real-time" auto-computes jitter, a number sets a fixed jitter. */ latencyMin: Signal<Latency>; @@ - // Re-anchor playback (and flush the audio buffer) at an utterance boundary in buffered mode. + /** Re-anchor playback (and flush the audio buffer) at an utterance boundary in buffered mode. */ reset(): void; }As per coding guidelines, "Document every exported JavaScript/TypeScript symbol with doc comments (
/** */), including functions, classes, interfaces, types, consts, enums, and their notable public members."Also applies to: 61-70, 261-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/backend.ts` around lines 39 - 55, Convert the single-line comments (`//`) documenting the public API members latencyMin, latencyMax, jitter, buffered, and the reset() method to JSDoc blocks using the `/** */` syntax. Each comment should be reformatted as a proper doc comment block that precedes its corresponding member or method. Apply the same JSDoc conversion pattern to all similar instances across the file where public API members are documented with line comments instead of doc blocks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/watch/src/sync.ts`:
- Around line 226-231: The reset() method clears several internal state fields
but does not clear the timestamp field, which can cause consumers to see stale
timestamp values from the previous utterance. Since received() updates timestamp
monotonically and a new utterance with lower PTS will not update it, you need to
add a line in the reset() method to clear the timestamp field along with the
other cleared fields like `#reference` and `#late` to ensure clean state across
utterances.
- Around line 98-116: The `#runRange` method can toggle the `buffered` flag at
runtime, but audio buffers created during construction (via `peek()`) retain
their initial buffered state and never reconfigure when the mode changes. To fix
this, ensure that when `this.#buffered.set()` is called in the `#runRange`
method, there is a corresponding mechanism to update existing audio buffers
(SharedAudioBuffer and PostAudioBuffer instances) to reconfigure their capacity,
backpressure, skip, and anchor behavior based on the new buffered mode.
Additionally, add a handler in the render worklet to respond to buffered-mode
changes so that the new configuration takes effect immediately rather than
persisting with stale skip/anchor behavior from the previous mode.
---
Outside diff comments:
In `@js/watch/src/element.ts`:
- Around line 147-155: The effect that reflects backend.latencyMin into the
latency attribute using setAttribute is causing attributeChangedCallback to
re-enter and set both latencyMin and latencyMax, which overwrites the previously
configured latencyMax and collapses range mode. Guard the reflected updates by
detecting when the attribute change is coming from an internal reflection (not
an external user action) within attributeChangedCallback, and skip the setter
logic that wipes latencyMax in those cases. Apply the same guard pattern to the
other instances mentioned at lines 230-237 and 310-313 where similar reflection
occurs.
---
Nitpick comments:
In `@js/watch/src/backend.ts`:
- Around line 39-55: Convert the single-line comments (`//`) documenting the
public API members latencyMin, latencyMax, jitter, buffered, and the reset()
method to JSDoc blocks using the `/** */` syntax. Each comment should be
reformatted as a proper doc comment block that precedes its corresponding member
or method. Apply the same JSDoc conversion pattern to all similar instances
across the file where public API members are documented with line comments
instead of doc blocks.
In `@js/watch/src/element.ts`:
- Around line 196-200: The `#parseLatency` method contains a magic number 100 used
as a fallback latency value when the parsed value is not finite. Extract this
inline 100 to a named constant at the class or module level that clearly
describes its purpose (such as DEFAULT_LATENCY or FALLBACK_LATENCY_MS), then
replace the inline 100 with this constant reference. This makes the intent
explicit and prevents accidental drift if this value needs to be used elsewhere.
In `@js/watch/src/sync.test.ts`:
- Around line 8-62: The test suite covers latency range derivation well but
lacks coverage for the utterance-boundary reset semantics with backward
timestamps. Add a new test case to this describe block that instantiates a Sync
object, calls its reset() method, then sets a lower timestamp value, and asserts
that sync.timestamp reflects the new utterance correctly. This ensures that
backward timestamps between utterances are handled properly after a reset,
preventing regressions in the timestamp handling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0473462a-9e03-4d0c-91a4-4c3a7ee999a2
📒 Files selected for processing (15)
doc/lib/js/@moq/watch.mdjs/watch/src/audio/buffer.tsjs/watch/src/audio/decoder.tsjs/watch/src/audio/render-worklet.tsjs/watch/src/audio/render.tsjs/watch/src/audio/ring-buffer.test.tsjs/watch/src/audio/ring-buffer.tsjs/watch/src/audio/shared-ring-buffer.test.tsjs/watch/src/audio/shared-ring-buffer.tsjs/watch/src/backend.tsjs/watch/src/element.tsjs/watch/src/sync.test.tsjs/watch/src/sync.tsjs/watch/src/ui/components/buffer-control.tsjs/watch/src/ui/components/latency.ts
Comment on lines
+98
to
+116
| // Derive `buffered` / `maxBuffer` from the floor (`buffer`) and the ceiling (`latencyMax`). | ||
| #runRange(effect: Effect): void { | ||
| const max = effect.get(this.latencyMax); | ||
| const floor = effect.get(this.buffer); | ||
|
|
||
| if (max === undefined) { | ||
| // Uncapped: buffer indefinitely (the ring bounds memory and applies backpressure). | ||
| this.#buffered.set(true); | ||
| this.#maxBuffer.set(undefined); | ||
| } else if (max === "real-time") { | ||
| // Ceiling tracks the floor: minimize latency, the live default. | ||
| this.#buffered.set(false); | ||
| this.#maxBuffer.set(floor); | ||
| } else { | ||
| // Buffered only when the ceiling is above the floor; otherwise it collapses to minimize. | ||
| this.#buffered.set(max > floor); | ||
| this.#maxBuffer.set(Time.Milli.max(max, floor)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Where ring buffers are created:"
rg -n --type=ts 'new AudioRingBuffer|new SharedRingBuffer|allocSharedRingBuffer\(' js/watch/src
echo
echo "Where sync buffered/cap signals are consumed:"
rg -n --type=ts 'sync\.buffered|sync\.maxBuffer|latencyMax|latencyMin' js/watch/src
echo
echo "Where runtime reconfiguration paths exist:"
rg -n --type=ts 'createAudioBuffer|resize\(|setLatency\(|reset\(' js/watch/src/audio js/watch/src/backend.ts js/watch/src/element.tsRepository: moq-dev/moq
Length of output: 15601
🏁 Script executed:
# Check if decoder watches sync.buffered for changes
rg -A 20 "this.source.sync.buffered" js/watch/src/audio/decoder.ts
echo "---"
# Check render-worklet handling of buffered mode
rg -B 5 -A 10 "buffered" js/watch/src/audio/render-worklet.ts
echo "---"
# Check if buffer creation is tied to any effect/watcher
rg -B 10 -A 5 "createAudioBuffer" js/watch/src/audio/decoder.ts | head -40
echo "---"
# Check buffer.ts constructor to see how buffered is stored
rg -B 5 -A 15 "constructor.*buffered" js/watch/src/audio/buffer.tsRepository: moq-dev/moq
Length of output: 2029
🏁 Script executed:
# Check for any effect.get() dependencies on buffered or maxBuffer (would trigger reactivity)
rg "effect.get.*buffered|effect.get.*maxBuffer" js/watch/src/audio/decoder.ts
echo "---"
# Check what messages render-worklet handles
rg -B 2 -A 10 "case.*:" js/watch/src/audio/render-worklet.ts | head -60
echo "---"
# Check buffer.ts constructor signature and how buffered is used
sed -n '100,180p' js/watch/src/audio/buffer.ts
echo "---"
# Check if there's any watcher on sync.buffered in decoder or anywhere else
rg -n "effect.get.*sync\.buffered|sync\.buffered.*effect.get" js/watch/srcRepository: moq-dev/moq
Length of output: 2867
🏁 Script executed:
# Get full render-worklet.ts to see all message handling
cat js/watch/src/audio/render-worklet.ts
echo "---"
# Check if decoder or buffer has any watchers on sync.buffered signal
rg -n "sync\\.buffered|sync\\.maxBuffer" js/watch/src/audio/decoder.ts js/watch/src/audio/buffer.tsRepository: moq-dev/moq
Length of output: 2653
🏁 Script executed:
# Check if there's any code that reconstructs buffers when sync.buffered changes
rg -n "effect.get.*buffered|sync\.buffered" js/watch/src/audio/decoder.ts js/watch/src/backend.ts js/watch/src/element.ts
echo "---"
# Check if buffer recreation is triggered anywhere when buffered signal changes
rg -B 5 -A 10 "this\.#ring = " js/watch/src/audio/decoder.ts
echo "---"
# Verify that buffered parameter is only used at construction, not stored/used later
sed -n '155,200p' js/watch/src/audio/buffer.tsRepository: moq-dev/moq
Length of output: 2600
Verify runtime reconfiguration when buffered mode toggles after construction.
Sync.#runRange can toggle buffered/maxBuffer at runtime (lines 106-114), but the decoder creates audio buffers once with peek() (lines 119-120), establishing construction-time state that never updates. Ring buffers in SharedAudioBuffer/PostAudioBuffer compute capacity and backpressure based on the initial buffered flag; setLatency() adjusts latency but not mode. Render worklet has no handler for buffered-mode changes. When sync.buffered flips post-creation, existing buffers retain stale skip/anchor behavior instead of reconfiguring to the new mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/sync.ts` around lines 98 - 116, The `#runRange` method can
toggle the `buffered` flag at runtime, but audio buffers created during
construction (via `peek()`) retain their initial buffered state and never
reconfigure when the mode changes. To fix this, ensure that when
`this.#buffered.set()` is called in the `#runRange` method, there is a
corresponding mechanism to update existing audio buffers (SharedAudioBuffer and
PostAudioBuffer instances) to reconfigure their capacity, backpressure, skip,
and anchor behavior based on the new buffered mode. Additionally, add a handler
in the render worklet to respond to buffered-mode changes so that the new
configuration takes effect immediately rather than persisting with stale
skip/anchor behavior from the previous mode.
Comment on lines
+226
to
+231
| reset(): void { | ||
| this.#reference.set(undefined); | ||
| this.#late.clear(); | ||
| this.#update.resolve(); | ||
| this.#update = Promise.withResolvers(); | ||
| } |
Contributor
There was a problem hiding this comment.
Reset should also clear timestamp to avoid stale state across utterances.
received() updates timestamp monotonically (Line 172). After reset(), a new utterance with lower PTS will not update timestamp, so consumers can keep seeing the previous utterance’s value. Clear it in reset().
Proposed fix
reset(): void {
this.#reference.set(undefined);
+ this.timestamp.set(undefined);
this.#late.clear();
this.#update.resolve();
this.#update = Promise.withResolvers();
}📝 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.
Suggested change
| reset(): void { | |
| this.#reference.set(undefined); | |
| this.#late.clear(); | |
| this.#update.resolve(); | |
| this.#update = Promise.withResolvers(); | |
| } | |
| reset(): void { | |
| this.#reference.set(undefined); | |
| this.timestamp.set(undefined); | |
| this.#late.clear(); | |
| this.#update.resolve(); | |
| this.#update = Promise.withResolvers(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/sync.ts` around lines 226 - 231, The reset() method clears
several internal state fields but does not clear the timestamp field, which can
cause consumers to see stale timestamp values from the previous utterance. Since
received() updates timestamp monotonically and a new utterance with lower PTS
will not update it, you need to add a line in the reset() method to clear the
timestamp field along with the other cleared fields like `#reference` and `#late` to
ensure clean state across utterances.
The audio Decoder's hang Container.Consumer was given the latency floor (sync.buffer) as its skip threshold. In buffered playback — where the producer writes faster than real-time with future PTS — the group span legitimately exceeds the floor, so every group was skipped. Use the ceiling (sync.maxBuffer) instead, falling back to the floor when uncapped. Hold the value in a plain Signal driven by a running effect rather than a computed: the Consumer only peek()s it (it never subscribes), and an unsubscribed computed peeks as undefined, which makes the threshold NaN and skips every group.
…d buffering
Replace the parallel latencyMin/latencyMax signals with a single `latency`
value typed as `Bound | { min?: Bound; max?: Bound }` (Bound = "real-time" |
ms). A scalar collapses the range and minimizes latency (today's default and
today's type, so existing code keeps compiling); an object opens a range for
buffered playback. Add `latencyBounds` / `latencyFromBounds` helpers; the
element keeps `latency-min` / `latency-max` attributes and `latencyMin` /
`latencyMax` accessors as read-modify-write sugar over the one signal.
Drop the uncapped ("none") ceiling. The ceiling is always finite, so the audio
ring is simply sized to the cap and drops its oldest samples on overflow. That
removes the entire backpressure subsystem (Backpressure class, AudioBuffer.wait,
the decoder's await-before-decode, UNCAPPED_RING_MS) and every maxBuffer ===
undefined branch; maxBuffer is now a plain finite Time.Milli.
Guard the element's `latency` attribute reflection so it only writes the
collapsed sugar attribute when the range is actually collapsed, preventing a
round-trip that would clobber an open range.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/watch/src/audio/buffer.ts (1)
123-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
latency-maxcap when latency updates in buffered mode.Line 126 can resize
SharedAudioBufferbeyond the buffered ceiling (maxBuffer) because it always usessamples * 2. That breaks the finite-cap contract introduced for buffered mode.Suggested fix
class SharedAudioBuffer implements AudioBuffer { readonly rate: number; readonly channels: number; `#worklet`: AudioWorkletNode; `#ring`: SharedRingBuffer; + `#buffered`: boolean; + `#maxBuffer`?: Time.Milli; constructor( worklet: AudioWorkletNode, channels: number, rate: number, latencySamples: number, buffered: boolean, maxBuffer?: Time.Milli, ) { + this.#buffered = buffered; + this.#maxBuffer = maxBuffer; // ... } setLatency(samples: number): void { + if (this.#buffered) { + const cappedCapacity = Math.ceil( + this.rate * Time.Second.fromMilli(this.#maxBuffer ?? DEFAULT_RING_MS), + ); + if (this.#ring.capacity !== cappedCapacity) { + this.#ring = this.#ring.resize(cappedCapacity); + const msg: InitShared = { type: "init-shared", ...this.#ring.init }; + this.#worklet.port.postMessage(msg); + } + this.#ring.setLatency(samples); + return; + } + // Grow the ring (preserving the unread window) if it's too small for the new latency. if (this.#ring.capacity < samples * 1.5) { const newCapacity = Math.max(this.rate, samples * 2); this.#ring = this.#ring.resize(newCapacity); this.#ring.setLatency(samples);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/audio/buffer.ts` around lines 123 - 128, The newCapacity calculation in the setLatency method does not respect the maxBuffer constraint and can grow the ring buffer beyond the buffered ceiling. Modify the newCapacity assignment to cap the calculated value at maxBuffer by applying Math.min alongside the existing Math.max call, ensuring that samples * 2 never exceeds the maximum buffer limit while still accommodating the required latency.
🧹 Nitpick comments (2)
js/watch/src/audio/render.ts (1)
4-5: ⚡ Quick winAdd doc comments for exported type aliases.
Line 4 and Line 5 export public API types without
/** */docs. Please add short API comments toMessageandToMainfor consistency with TS API docs standards.As per coding guidelines, "Document every exported JavaScript/TypeScript symbol with doc comments (
/** */), including functions, classes, interfaces, types, consts, enums, and their notable public members."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/audio/render.ts` around lines 4 - 5, Add JSDoc doc comments above the exported type aliases Message and ToMain in the file. Both type exports are missing the standard /** */ documentation comments required by the coding guidelines. Add a brief descriptive comment for each type alias above its export statement to document what the type represents and its purpose in the public API.Source: Coding guidelines
js/watch/src/backend.ts (1)
29-50: ⚡ Quick winUse JSDoc blocks for exported backend API surface.
These newly-touched exported interface members are documented with
//comments instead of/** */, which makes API docs/lint tooling inconsistent.Suggested change
-export interface Backend { - // The latency target: a scalar minimizes (collapsed range), an object opens a range. See {`@link` Latency}. - latency: Signal<Latency>; +export interface Backend { + /** The latency target: a scalar minimizes (collapsed range), an object opens a range. See {`@link` Latency}. */ + latency: Signal<Latency>; @@ - // The jitter buffer in milliseconds. - jitter: Signal<Moq.Time.Milli>; + /** The jitter buffer in milliseconds. */ + jitter: Signal<Moq.Time.Milli>; @@ - // Derived: whether buffered playback is active (ceiling above floor). - buffered: Signal<boolean>; + /** Derived: whether buffered playback is active (ceiling above floor). */ + buffered: Signal<boolean>; @@ - // Re-anchor playback (and flush the audio buffer) at an utterance boundary in buffered mode. - reset(): void; + /** Re-anchor playback (and flush the audio buffer) at an utterance boundary in buffered mode. */ + reset(): void; }As per coding guidelines, “Document every exported JavaScript/TypeScript symbol with doc comments (
/** */), including functions, classes, interfaces, types, consts, enums, and their notable public members.”Also applies to: 56-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/backend.ts` around lines 29 - 50, The Backend interface and its members are documented with single-line comments (`//`) instead of JSDoc comments (`/** */`). Convert all comment blocks for the Backend interface members (paused, video, audio, latency, jitter, buffered, and the reset method) from single-line format to proper JSDoc block format using `/** */` delimiters. Apply the same change to other exported interface members mentioned at lines 56-60.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/watch/src/element.ts`:
- Around line 201-205: The `#parseBound` method currently accepts negative numbers
for latency bounds, which is invalid. Add an additional check to the condition
on line 204 to verify that the parsed number is not only finite but also
non-negative (greater than or equal to zero). If the parsed value is negative,
fall back to the default value of 100 instead of returning the negative number.
- Around line 148-153: When the latency range transitions from collapsed to open
(when min !== max is detected in the latencyBounds check), the stale latency
attribute needs to be removed from the DOM to maintain consistency between DOM
state and runtime state. Before the early return statement that executes when
min !== max, remove the latency attribute from the element to ensure the DOM
properly reflects the open-range state.
---
Outside diff comments:
In `@js/watch/src/audio/buffer.ts`:
- Around line 123-128: The newCapacity calculation in the setLatency method does
not respect the maxBuffer constraint and can grow the ring buffer beyond the
buffered ceiling. Modify the newCapacity assignment to cap the calculated value
at maxBuffer by applying Math.min alongside the existing Math.max call, ensuring
that samples * 2 never exceeds the maximum buffer limit while still
accommodating the required latency.
---
Nitpick comments:
In `@js/watch/src/audio/render.ts`:
- Around line 4-5: Add JSDoc doc comments above the exported type aliases
Message and ToMain in the file. Both type exports are missing the standard /**
*/ documentation comments required by the coding guidelines. Add a brief
descriptive comment for each type alias above its export statement to document
what the type represents and its purpose in the public API.
In `@js/watch/src/backend.ts`:
- Around line 29-50: The Backend interface and its members are documented with
single-line comments (`//`) instead of JSDoc comments (`/** */`). Convert all
comment blocks for the Backend interface members (paused, video, audio, latency,
jitter, buffered, and the reset method) from single-line format to proper JSDoc
block format using `/** */` delimiters. Apply the same change to other exported
interface members mentioned at lines 56-60.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96f5d8d0-e9e7-4638-8848-4283c13892f7
📒 Files selected for processing (11)
doc/lib/js/@moq/watch.mdjs/watch/src/audio/buffer.tsjs/watch/src/audio/decoder.tsjs/watch/src/audio/render.tsjs/watch/src/audio/ring-buffer.tsjs/watch/src/backend.tsjs/watch/src/element.tsjs/watch/src/sync.test.tsjs/watch/src/sync.tsjs/watch/src/ui/components/buffer-control.tsjs/watch/src/ui/components/latency.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- js/watch/src/ui/components/buffer-control.ts
- js/watch/src/audio/ring-buffer.ts
- doc/lib/js/@moq/watch.md
- js/watch/src/ui/components/latency.ts
Comment on lines
+148
to
+153
| const { min, max } = latencyBounds(effect.get(this.backend.latency)); | ||
| // Only reflect the collapsed `latency` sugar attribute when the range is actually | ||
| // collapsed. An open range is expressed via latency-min/latency-max, and writing | ||
| // `latency` here would round-trip back through attributeChangedCallback and collapse it. | ||
| if (min !== max) return; | ||
| if (min === "real-time") { |
Contributor
There was a problem hiding this comment.
Remove stale latency attribute when range opens.
At Line 152, the early return leaves any existing latency="..." attribute in place, so the DOM can represent collapsed latency while runtime state is open-range.
Suggested change
// `latency` here would round-trip back through attributeChangedCallback and collapse it.
- if (min !== max) return;
+ if (min !== max) {
+ this.removeAttribute("latency");
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { min, max } = latencyBounds(effect.get(this.backend.latency)); | |
| // Only reflect the collapsed `latency` sugar attribute when the range is actually | |
| // collapsed. An open range is expressed via latency-min/latency-max, and writing | |
| // `latency` here would round-trip back through attributeChangedCallback and collapse it. | |
| if (min !== max) return; | |
| if (min === "real-time") { | |
| const { min, max } = latencyBounds(effect.get(this.backend.latency)); | |
| // Only reflect the collapsed `latency` sugar attribute when the range is actually | |
| // collapsed. An open range is expressed via latency-min/latency-max, and writing | |
| // `latency` here would round-trip back through attributeChangedCallback and collapse it. | |
| if (min !== max) { | |
| this.removeAttribute("latency"); | |
| return; | |
| } | |
| if (min === "real-time") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/element.ts` around lines 148 - 153, When the latency range
transitions from collapsed to open (when min !== max is detected in the
latencyBounds check), the stale latency attribute needs to be removed from the
DOM to maintain consistency between DOM state and runtime state. Before the
early return statement that executes when min !== max, remove the latency
attribute from the element to ensure the DOM properly reflects the open-range
state.
Comment on lines
+201
to
205
| #parseBound(value: string | null): Bound { | ||
| if (!value || value === "real-time") return "real-time"; | ||
| const parsed = Number.parseFloat(value); | ||
| return (Number.isFinite(parsed) ? parsed : 100) as Time.Milli; | ||
| } |
Contributor
There was a problem hiding this comment.
Reject negative latency bounds in attribute parsing.
Line 204 currently accepts negative numbers, which can feed invalid latency/jitter values into sync calculations.
Suggested change
`#parseBound`(value: string | null): Bound {
if (!value || value === "real-time") return "real-time";
const parsed = Number.parseFloat(value);
- return (Number.isFinite(parsed) ? parsed : 100) as Time.Milli;
+ return (Number.isFinite(parsed) && parsed >= 0 ? parsed : 100) as Time.Milli;
}📝 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.
Suggested change
| #parseBound(value: string | null): Bound { | |
| if (!value || value === "real-time") return "real-time"; | |
| const parsed = Number.parseFloat(value); | |
| return (Number.isFinite(parsed) ? parsed : 100) as Time.Milli; | |
| } | |
| `#parseBound`(value: string | null): Bound { | |
| if (!value || value === "real-time") return "real-time"; | |
| const parsed = Number.parseFloat(value); | |
| return (Number.isFinite(parsed) && parsed >= 0 ? parsed : 100) as Time.Milli; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/element.ts` around lines 201 - 205, The `#parseBound` method
currently accepts negative numbers for latency bounds, which is invalid. Add an
additional check to the condition on line 204 to verify that the parsed number
is not only finite but also non-negative (greater than or equal to zero). If the
parsed value is negative, fall back to the default value of 100 instead of
returning the negative number.
…ackpressure Keep the decoded PCM ring sized to the latency floor and hold everything above it (the buffered lookahead, up to the ceiling) upstream as encoded Opus, gated by backpressure on the decode loop. A large `latency-max` now costs encoded bytes, not seconds of decoded PCM. maxBuffer no longer sizes the ring; it only governs the container consumer's Opus retention and the re-anchor cap. Backpressure is enabled whenever buffered (ceiling above floor), with the gate set to the floor so decoded PCM stays ~floor ahead of the playhead. While the ring is stalled (bootstrap or underflow) `wait()` resolves freely so the decode loop can refill the floor; once playing it holds back. The post-message fallback ring is likewise floor-sized and reallocs on latency change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reframes playback latency in
@moq/watchas a range, unifying live (minimize-latency) and buffered playback into a single mechanism. This unblocks voice-AI / TTS use cases (e.g. pipecat-ai/pipecat#4629) where a response is written faster than real-time with future timestamps and should be buffered and played at the encoded pace, rather than the player aggressively minimizing latency and skipping ahead.The model
Latency was never really a single point. It's a range, and "minimize latency" is just the degenerate case where the range collapses (
max == min).The sync reference (the wall-clock anchor) is re-anchored (skipped forward) only when keeping it would push latency past the ceiling. So:
"real-time"(default) or<= floor-> re-anchor on every early frame -> today's behavior.<ms>-> buffer freely, skip ahead only past the cap.There's no separate "buffered" code path. Minimize falls out of the same logic.
API
A single
latencyvalue, typed as:latency: 100/"real-time"-> collapsed, minimize. This is the old type and old behavior, so existing code compiles unchanged (the change is non-breaking).latency: { min: 100, max: 30_000 }-> buffered: float between floor and ceiling, skip ahead only past 30s.{ min }/{ max }default the other bound to"real-time".Helpers
latencyBounds/latencyFromBoundsconvert between the value and explicit bounds. On the<moq-watch>element thelatency/latency-min/latency-maxattributes and thelatencyMin/latencyMaxproperties remain as read-modify-write sugar over the one signal.reset()flushes the audio buffer and re-anchors at an utterance boundary.No uncapped buffering
The ceiling is always finite. Unbounded buffering is a footgun (a publisher writing fast-enough future timestamps could exhaust the tab's memory), so there is no
"none"/uncapped mode.Buffered lookahead stays as encoded Opus
The decoded PCM ring is sized to the floor (
latency-min) only. Everything above the floor (the buffered lookahead, up tolatency-max) stays upstream as encoded Opus, gated by backpressure on the decode loop: the decoder stops decoding ahead once it would run more than the floor past the playhead, and the container consumer retains the encoded frames. So a largelatency-max(e.g. 30s for a TTS turn) costs encoded bytes, not 30s of decoded PCM. The floor itself is still decoded PCM in the ring (it's the jitter buffer), so the latency presets behave as before.Backpressure is enabled whenever the range is open. While the ring is stalled (startup or underflow) it releases freely so the floor can refill; once playing it holds the decode loop to ~the floor ahead. Both transports are covered (Atomics polling on the SharedArrayBuffer path, worklet state messages on the postMessage fallback).
Test plan
bun run check(tsc) clean (watch + moq-boy consumer)bun test-- 78 pass (sync.test.tscovers the scalar/range derivation; ring-buffer / shared-ring-buffer cover anchor / no-skip / overflow-drop)reset()cleanly switches utterancesNotes / follow-ups
latency-maxis conceptually the same knob as the wireSubscribeUpdate.maxLatency(the watch currently sends0). Plumbing it down so the relay reorders/drops groups past the cap is the real end-to-end throttle, left as a follow-up.🤖 Generated with Claude Code
(Written by Claude)