@moq/watch: add static catalog format#1349
Conversation
Lets callers supply a Catalog.Root directly instead of fetching it via hang or MSF, while still reusing the connection/broadcast for media tracks. Broadcast.catalog is now a writable Signal so users can update it at runtime; <moq-watch> exposes a matching JS property. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A textarea + Apply button to test catalog-format="static" against the default bbb broadcast. Cross-linked from the existing watch/mse/publish demos. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Single source of truth so adding a new format is one edit. parseCatalogFormat becomes a lookup with a default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new "manual" catalog mode to 🚥 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🧪 Generate unit tests (beta)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
demo/web/src/static.ts (2)
27-42: PreferEffect.event()over rawaddEventListener.Per the project guideline, click handling should use
Effect.event()from@moq/signalsso cleanup is handled automatically. For this demo a top-levelEffect(mirroringMoqWatch.signals) would be the simplest fit; it also gives you a place to scope future timers/intervals without leaks if the page wires more controls later.As per coding guidelines: "Use
Effect.interval(),Effect.timer(), andEffect.event()helpers from@moq/signalsinstead of rawsetInterval,setTimeout,addEventListener. These handle cleanup automatically when the Effect is closed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/web/src/static.ts` around lines 27 - 42, Replace the raw apply.addEventListener usage with an Effect.event-based handler so the click listener is created under a top-level Effect and cleaned up automatically; create or reuse the top-level Effect (e.g., MoqWatch.signals or a new Effect instance) and call Effect.event(effect, apply, "click", () => { const text = input.value.trim(); if (!text) { watch.catalog = undefined; setStatus("cleared"); return; } try { const parsed = JSON.parse(text) as Catalog.Root; watch.catalogFormat = "static"; watch.catalog = parsed; setStatus("applied"); } catch (err) { setStatus(`parse error: ${(err as Error).message}`, false); } }); so the listener is registered via Effect.event instead of apply.addEventListener and will be automatically disposed when the Effect closes.
12-14: Make the missing-element diagnostics consistent.
watchgets a friendlythrow new Error("missing <moq-watch> element")on line 10, but#catalog-input,#apply, and#statusare blind-cast and will fail later with a confusingnullderef if any element is renamed/removed. A small null-check (or a tinybyId()helper that throws with the offending id) keeps the demo's failure modes consistent and easier to debug.♻️ Suggested helper
-const input = document.getElementById("catalog-input") as HTMLTextAreaElement; -const apply = document.getElementById("apply") as HTMLButtonElement; -const status = document.getElementById("status") as HTMLSpanElement; +function byId<T extends HTMLElement>(id: string): T { + const el = document.getElementById(id); + if (!el) throw new Error(`missing #${id} element`); + return el as T; +} + +const input = byId<HTMLTextAreaElement>("catalog-input"); +const apply = byId<HTMLButtonElement>("apply"); +const status = byId<HTMLSpanElement>("status");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/web/src/static.ts` around lines 12 - 14, The code currently blind-casts DOM lookups (input, apply, status) and will later throw confusing null dereferences; create a small helper (e.g., byId(id: string): HTMLElement that queries document.getElementById and throws a clear Error(`missing ${id} element`) when null) and replace the raw casts with calls like const input = byId("catalog-input") as HTMLTextAreaElement, const apply = byId("apply") as HTMLButtonElement, and const status = byId("status") as HTMLSpanElement so missing elements produce the same friendly diagnostic as the existing watch check.js/watch/src/broadcast.ts (2)
99-113: Static branch: status never reaches"offline".In
"static"mode,statusonly flips between"live"and"loading"based on whethercatalogis set. If the broadcast transitioned from"hang"/"msf"to"static", the previous run'sfinallywill already have written"offline", but as soon as this branch runs it'll be reset to"loading"(or"live") regardless of whether the underlying connection/active broadcast exists. If consumers usestatus === "offline"as a proxy for "no media available", static mode will be inconsistent with the other formats.Probably acceptable since
statusis conceptually catalog availability here, but worth a short// whycomment so readers don't assume"offline"is reachable in static mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 99 - 113, The static-branch of `#runCatalog` currently sets this.status to "live" or "loading" based only on this.catalog, which prevents "offline" from ever being reached when format === "static"; add a short explanatory comment inside the static branch (near the use of this.catalogFormat / this.catalog and the this.status.set(...) call) that documents that in "static" mode status represents catalog availability (live/loading) and that "offline" is intentionally not used here to reflect that there is no live broadcast connection, so readers won’t mistakenly expect "offline" to be reachable in static mode.
50-52: Public writable catalog races with the fetch loop in non-static modes.Promoting
catalogto a public writableSignalis what enables both the static path and the<moq-watch>.catalogsetter inelement.ts(lines 270-276). However, in"hang"/"msf"modes the fetch loop also writes to this same signal at lines 137 and 143, so any externalbroadcast.catalog.set(...)will be silently overwritten on the next fetch (or wiped toundefinedwhen the spawn unwinds). The doc onBroadcastProps.catalogcovers initial values, but the runtime setter doesn't surface the same caveat.Consider one of:
- Documenting on the field (line 50-52) that external writes are only stable when
catalogFormat === "static", and mirroring that onMoqWatch.cataloginelement.ts.- Or guarding the setter so it's a no-op (or warns) when
catalogFormat !== "static".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 50 - 52, The public writable Signal catalog (BroadcastProps.catalog / catalog in broadcast.ts) races with the internal fetch loop (which also writes the same signal), so either document that external writes are only stable when catalogFormat === "static" or prevent accidental overwrites by guarding the external setter; implement the latter by making MoqWatch.catalog's setter a no-op or emit a console/process warning when catalogFormat !== "static" (and similarly enforce/validate in broadcast.ts if there is a direct public setter), ensuring the fetch loop continues to own writes in non-static modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/watch/src/broadcast.ts`:
- Around line 140-146: The finally block in broadcast.ts unconditionally calls
this.catalog.set(undefined), which can clobber a user-provided Signal (see
Signal.from behavior); change the logic so the writable catalog is only cleared
when the stream actually errors or ends: remove this.catalog.set(undefined) from
the finally block and instead call this.catalog.set(undefined) inside the catch
(for errors) and at the explicit end-of-stream handling path, leaving planned
Effect teardowns (re-runs) from wiping a consumer's Signal; reference the
this.catalog.set(undefined) call and the surrounding try/catch/finally in
broadcast.ts when making the change.
---
Nitpick comments:
In `@demo/web/src/static.ts`:
- Around line 27-42: Replace the raw apply.addEventListener usage with an
Effect.event-based handler so the click listener is created under a top-level
Effect and cleaned up automatically; create or reuse the top-level Effect (e.g.,
MoqWatch.signals or a new Effect instance) and call Effect.event(effect, apply,
"click", () => { const text = input.value.trim(); if (!text) { watch.catalog =
undefined; setStatus("cleared"); return; } try { const parsed = JSON.parse(text)
as Catalog.Root; watch.catalogFormat = "static"; watch.catalog = parsed;
setStatus("applied"); } catch (err) { setStatus(`parse error: ${(err as
Error).message}`, false); } }); so the listener is registered via Effect.event
instead of apply.addEventListener and will be automatically disposed when the
Effect closes.
- Around line 12-14: The code currently blind-casts DOM lookups (input, apply,
status) and will later throw confusing null dereferences; create a small helper
(e.g., byId(id: string): HTMLElement that queries document.getElementById and
throws a clear Error(`missing ${id} element`) when null) and replace the raw
casts with calls like const input = byId("catalog-input") as
HTMLTextAreaElement, const apply = byId("apply") as HTMLButtonElement, and const
status = byId("status") as HTMLSpanElement so missing elements produce the same
friendly diagnostic as the existing watch check.
In `@js/watch/src/broadcast.ts`:
- Around line 99-113: The static-branch of `#runCatalog` currently sets
this.status to "live" or "loading" based only on this.catalog, which prevents
"offline" from ever being reached when format === "static"; add a short
explanatory comment inside the static branch (near the use of this.catalogFormat
/ this.catalog and the this.status.set(...) call) that documents that in
"static" mode status represents catalog availability (live/loading) and that
"offline" is intentionally not used here to reflect that there is no live
broadcast connection, so readers won’t mistakenly expect "offline" to be
reachable in static mode.
- Around line 50-52: The public writable Signal catalog (BroadcastProps.catalog
/ catalog in broadcast.ts) races with the internal fetch loop (which also writes
the same signal), so either document that external writes are only stable when
catalogFormat === "static" or prevent accidental overwrites by guarding the
external setter; implement the latter by making MoqWatch.catalog's setter a
no-op or emit a console/process warning when catalogFormat !== "static" (and
similarly enforce/validate in broadcast.ts if there is a direct public setter),
ensuring the fetch loop continues to own writes in non-static modes.
🪄 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: fe30709d-ec7b-4778-a684-02ef228a6527
📒 Files selected for processing (9)
demo/web/src/index.htmldemo/web/src/mse.htmldemo/web/src/publish.htmldemo/web/src/static.htmldemo/web/src/static.tsdemo/web/vite.config.tsdoc/js/@moq/watch.mdjs/watch/src/broadcast.tsjs/watch/src/element.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/watch/src/broadcast.ts (1)
143-146:⚠️ Potential issue | 🟡 Minor
finallystill clobbers a user-supplied catalog Signal on every re-run.The previously raised concern is unresolved. Because
Signal.from(props?.catalog)returns the caller'sSignalwhen one was passed, and this effect re-runs wheneverenabled/catalogFormat/activechanges, thefinallyblock writesundefinedinto a signal the consumer may still own and observe. Concretely, switchingcatalogFormatfrom"hang"/"msf"to"static"cancels the spawn, triggersfinally, and wipes the user's initial catalog before the static branch at lines 106–111 reads it — so a freshly-applied static catalog ends up asundefined/"loading".Move the clear into the
catchand the explicitbreak(end-of-stream) paths so planned teardowns don't destroy consumer-owned state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 143 - 146, The finally block in the effect is clobbering consumer-owned Signal instances by always calling this.catalog.set(undefined) and this.status.set("offline") on every re-run; instead, remove those clears from the finally and only clear catalog/status in the error path and in the deliberate termination (the catch block and the explicit end-of-stream/break handling) so Signal.from(props?.catalog) returns and retains the caller's Signal when a planned branch switch (e.g., from hang/msf to static) re-runs; update the code around the effect that uses this.catalog.set and this.status.set to perform clears only on errors or explicit end-of-stream, not inside the finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/watch/src/broadcast.ts`:
- Around line 143-146: The finally block in the effect is clobbering
consumer-owned Signal instances by always calling this.catalog.set(undefined)
and this.status.set("offline") on every re-run; instead, remove those clears
from the finally and only clear catalog/status in the error path and in the
deliberate termination (the catch block and the explicit end-of-stream/break
handling) so Signal.from(props?.catalog) returns and retains the caller's Signal
when a planned branch switch (e.g., from hang/msf to static) re-runs; update the
code around the effect that uses this.catalog.set and this.status.set to perform
clears only on errors or explicit end-of-stream, not inside the finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7bacfa1-8b39-4743-8d0a-3927106db332
📒 Files selected for processing (2)
js/watch/src/broadcast.tsjs/watch/src/element.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- js/watch/src/element.ts
There was a problem hiding this comment.
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/broadcast.ts (1)
100-155:⚠️ Potential issue | 🟡 MinorStatus can get stuck stale after a planned teardown.
Previously the old
finallyalways resetstatusto"offline"on every re-run. Now, when the run is aborted (e.g.,enabledflips tofalse,namechanges, orcatalogFormatswitches), the spawn returns early at Line 139/148 without touchingstatus, and the new run bails at Line 102 (whenenabledisfalse) or hits Line 113-114 (whenactiveisundefined) without resetting it either. Net effect: a previously"live"(or"loading") broadcast keeps showing"live"after the user disables it, until something else writes status. Thecatalog-clobber fix is the right shape; consider decouplingstatusfromcatalogso abort-driven teardown still resetsstatuswhile leaving the user-owned catalogSignalalone.♻️ Sketch: reset status on abort, keep catalog intact
effect.spawn(async () => { try { for (;;) { const update = await Promise.race([effect.cancel, fetchNext()]); - if (aborted.aborted) return; + if (aborted.aborted) { + this.status.set("offline"); + return; + } if (!update) break; console.debug("received catalog", format, this.name.peek(), update); this.catalog.set(update); this.status.set("live"); } } catch (err) { - if (aborted.aborted) return; + if (aborted.aborted) { + this.status.set("offline"); + return; + } console.warn("error fetching catalog", this.name.peek(), err); } this.catalog.set(undefined); this.status.set("offline"); });You may also want to set
statusto"offline"in the early-return branches at Line 102 and Line 114 for the non-static path, so disabling/disconnecting reflects in the UI immediately rather than waiting for the spawn to wind down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 100 - 155, The runCatalog method can leave this.status stale when a run is aborted or when early-returning (e.g., enabled flips false or no active broadcast); update `#runCatalog` to explicitly reset status to "offline" on those early-return paths (before the returns at the top when enabled is false and after the check for !broadcast) and also ensure the spawned task sets this.status to "offline" on abort-return (the branches that return when aborted.aborted) while deliberately not touching this.catalog to preserve user-owned catalog Signal; locate changes around the symbols runCatalog, this.status.set, this.catalog.set, effect.abort/aborted, and effect.spawn/fetchNext.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/watch/src/broadcast.ts`:
- Around line 100-155: The runCatalog method can leave this.status stale when a
run is aborted or when early-returning (e.g., enabled flips false or no active
broadcast); update `#runCatalog` to explicitly reset status to "offline" on those
early-return paths (before the returns at the top when enabled is false and
after the check for !broadcast) and also ensure the spawned task sets
this.status to "offline" on abort-return (the branches that return when
aborted.aborted) while deliberately not touching this.catalog to preserve
user-owned catalog Signal; locate changes around the symbols runCatalog,
this.status.set, this.catalog.set, effect.abort/aborted, and
effect.spawn/fetchNext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75aca57b-4516-4ce7-9d73-4b18a78b8246
📒 Files selected for processing (1)
js/watch/src/broadcast.ts
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
looks good, small nits
|
|
||
| // Capture the AbortSignal of *this* run; it flips to aborted on teardown | ||
| // (re-run or close) and lets us tell that apart from a natural stream end, | ||
| // so we never clobber a user-provided catalog Signal on a planned re-run. |
There was a problem hiding this comment.
Do we need this? If the format is static, we return early and never reach this code.
There was a problem hiding this comment.
claude added this because of coderabbits review: Basically when changing format from hang to static and the other way it was not happy. But yea, I also don't like this logic. I will try to simplify it.
|
|
||
| ### Static catalogs | ||
|
|
||
| Use `catalog-format="static"` (or `catalogFormat: "static"`) to skip the catalog |
There was a problem hiding this comment.
nit: Is static the best name if it can change? Maybe use manual instead?
Per PR review: "static" implies immutable, but the catalog can be reassigned at any time — "manual" better reflects that the user owns updates. Renames the format value, the demo files (static.html/ts → manual.html/ts), the Vite entry, cross-links, and docs. Also documents that switching catalogFormat between "manual" and a fetched format tears down the previous fetch loop and clears the catalog signal, so users should set the catalog after switching to "manual", not before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
skirsten
force-pushed
the
watch-static-catalog
branch
from
5b63b51 to
16b2b34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/watch/src/broadcast.ts (1)
133-149:⚠️ Potential issue | 🟠 MajorFetch-loop teardown still clears
catalogunconditionally.
finallyalways callsthis.catalog.set(undefined), so effect re-runs/teardowns still wipe catalog state (including externally supplied signal-backed state), not just genuine end/error paths.#!/bin/bash # Verify the unconditional clear remains in fetched-mode spawn teardown. rg -nP --type=ts -C4 'effect\.spawn\(|Promise\.race\(\[effect\.cancel,\s*fetchNext\(\)\]\)|finally\s*{\s*this\.catalog\.set\(undefined\)' js/watch/src/broadcast.ts # Inspect Effect cancellation contract to distinguish teardown vs stream-end handling. rg -nP --type=ts -C4 '\bclass\s+Effect\b|\bcancel\b' js/signals/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 133 - 149, The finally block unconditionally clears this.catalog (this.catalog.set(undefined)) so effect re-runs or cancellations wipe externally supplied/catalog state; fix by detecting cancellation vs genuine end/error: in the effect.spawn async block add a local flag (e.g., let cancelled = false), set cancelled = true when Promise.race returns effect.cancel (or when you detect the Effect cancellation error in the catch), and then in finally only call this.catalog.set(undefined) and this.status.set("offline") when cancelled === false (or when the stream actually ended/errored), preserving catalog when the effect was cancelled/re-run; reference effect.spawn, fetchNext(), the Promise.race([...effect.cancel, fetchNext()]) branch, the catch block, and the finally block (this.catalog.set(undefined), this.status.set("offline")) to apply the conditional clear.
🧹 Nitpick comments (2)
demo/web/src/manual.ts (1)
27-42: UseEffect.event()instead of rawaddEventListenerfor auto-cleanup consistency.This listener is currently unmanaged and bypasses the project’s effect-lifecycle pattern.
As per coding guidelines, "Use
Effect.interval(),Effect.timer(), andEffect.event()helpers from@moq/signalsinstead of rawsetInterval,setTimeout,addEventListener."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/web/src/manual.ts` around lines 27 - 42, Replace the unmanaged apply.addEventListener call with the project lifecycle helper Effect.event so the listener is tracked and auto-cleaned; locate the click handler where apply.addEventListener(...) is used (and references input.value, watch.catalog/watch.catalogFormat, and setStatus) and register it via Effect.event(...) from `@moq/signals`, moving the same logic (trim, parse, try/catch, setting watch.* and setStatus) into the Effect.event callback, then remove the raw addEventListener to ensure consistent effect lifecycle management.demo/web/src/manual.html (1)
40-52: Add a proper label and live status semantics for the catalog form.
textarea#catalog-inputhas no explicit label, and#statusupdates are not announced to assistive tech. This makes parse/apply feedback harder to use non-visually.♿ Suggested markup tweak
- <h3>Catalog JSON</h3> + <h3><label for="catalog-input">Catalog JSON</label></h3> @@ - <span id="status" class="ml-2 opacity-80 text-sm"></span> + <span id="status" role="status" aria-live="polite" class="ml-2 opacity-80 text-sm"></span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/web/src/manual.html` around lines 40 - 52, The catalog textarea lacks an accessible label and the status span is not announced to assistive tech; add a visible or visually-hidden <label> associated with textarea#catalog-input (match the id and reference it from the label) so screen readers announce the field, and make span#status (or replace it) use live region semantics (e.g., add aria-live="polite" and aria-atomic="true" and/or role="status") so updates after clicking button#apply are announced; ensure the label text clearly describes the expected JSON/catalog input and keep button#apply behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/web/src/manual.ts`:
- Line 3: The code currently uses a type-only import for Catalog and assigns
parsed JSON directly to watch.catalog without schema validation; change the
import to a runtime import (remove "type") so Catalog.RootSchema is available at
runtime, then parse the JSON and run Catalog.RootSchema.safeParse(parsed) and
only assign watch.catalog when safeParse succeeds (handle or surface validation
errors otherwise); update the code paths that set watch.catalog to perform this
validation using Catalog.RootSchema.safeParse().
---
Duplicate comments:
In `@js/watch/src/broadcast.ts`:
- Around line 133-149: The finally block unconditionally clears this.catalog
(this.catalog.set(undefined)) so effect re-runs or cancellations wipe externally
supplied/catalog state; fix by detecting cancellation vs genuine end/error: in
the effect.spawn async block add a local flag (e.g., let cancelled = false), set
cancelled = true when Promise.race returns effect.cancel (or when you detect the
Effect cancellation error in the catch), and then in finally only call
this.catalog.set(undefined) and this.status.set("offline") when cancelled ===
false (or when the stream actually ended/errored), preserving catalog when the
effect was cancelled/re-run; reference effect.spawn, fetchNext(), the
Promise.race([...effect.cancel, fetchNext()]) branch, the catch block, and the
finally block (this.catalog.set(undefined), this.status.set("offline")) to apply
the conditional clear.
---
Nitpick comments:
In `@demo/web/src/manual.html`:
- Around line 40-52: The catalog textarea lacks an accessible label and the
status span is not announced to assistive tech; add a visible or visually-hidden
<label> associated with textarea#catalog-input (match the id and reference it
from the label) so screen readers announce the field, and make span#status (or
replace it) use live region semantics (e.g., add aria-live="polite" and
aria-atomic="true" and/or role="status") so updates after clicking button#apply
are announced; ensure the label text clearly describes the expected JSON/catalog
input and keep button#apply behavior unchanged.
In `@demo/web/src/manual.ts`:
- Around line 27-42: Replace the unmanaged apply.addEventListener call with the
project lifecycle helper Effect.event so the listener is tracked and
auto-cleaned; locate the click handler where apply.addEventListener(...) is used
(and references input.value, watch.catalog/watch.catalogFormat, and setStatus)
and register it via Effect.event(...) from `@moq/signals`, moving the same logic
(trim, parse, try/catch, setting watch.* and setStatus) into the Effect.event
callback, then remove the raw addEventListener to ensure consistent effect
lifecycle management.
🪄 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: 34440543-682b-4394-ab4a-4c98554978d1
📒 Files selected for processing (9)
demo/web/src/index.htmldemo/web/src/manual.htmldemo/web/src/manual.tsdemo/web/src/mse.htmldemo/web/src/publish.htmldemo/web/vite.config.tsdoc/js/@moq/watch.mdjs/watch/src/broadcast.tsjs/watch/src/element.ts
✅ Files skipped from review due to trivial changes (4)
- demo/web/src/publish.html
- demo/web/src/mse.html
- demo/web/vite.config.ts
- demo/web/src/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- js/watch/src/element.ts
The Apply handler was casting JSON.parse output straight to Catalog.Root, so malformed input silently produced garbage that broke later subscribe calls in confusing ways. Switch to a runtime import and use Catalog.RootSchema.safeParse, surfacing parse vs. schema errors separately in the status line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
thanks @skirsten ! |
2 participants
Summary
"static"catalog format on@moq/watchthat lets callers pass aCatalog.Rootdirectly instead of fetching it via the hang or MSF catalog track. Connection + broadcast name are still required so media tracks can be subscribed normally.Broadcast.catalogfromGetterto writableSignal<Catalog.Root | undefined>and accepts an initialcatalogprop.<moq-watch>exposes a matchingcatalogJS property.demo/web/src/static.htmldemo with a textarea + Apply button to manually drive<moq-watch catalog-format="static">against the defaultbbbbroadcast, cross-linked from the existing watch/mse/publish demos.doc/js/@moq/watch.md.Test plan
bun run checkinjs/watchpasses.bun run buildindemo/webproducesdist/static.html.just demorunning, openstatic.html, paste a realCatalog.Root(capture fromindex.htmlviaJSON.stringify(watch.catalog)), click Apply, video plays without acatalog.jsontrack subscription.hangandmsfflows onindex.htmlstill behave as before.🤖 Generated with Claude Code