Conversation
Also split hang-support into moq-publish-support and moq-watch-support.
WalkthroughThe PR renames web components and types across the repo: hang-watch → moq-watch, hang-publish → moq-publish, hang-config → moq-watch-config; HangWatch → MoqWatch; HangPublish → MoqPublish. The single hang-support component is split into moq-watch-support and moq-publish-support (new support components and support detection modules added). The demo package moved from js/hang-demo to js/demo and package exports/docs/usage examples were updated accordingly. Several package.json exports were added/removed to surface the new support modules. No control-flow or runtime behavior changes beyond renaming and support-component additions. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
js/watch/src/support/element.ts (1)
28-32:⚠️ Potential issue | 🟡 MinorUnhandled promise rejection from
isSupported().Same issue as the publish counterpart — if
isSupported()rejects, this becomes an unhandled promise rejection.🛡️ Proposed fix
constructor() { super(); - isSupported().then((s) => this.#support.set(s)); + isSupported() + .then((s) => this.#support.set(s)) + .catch((err) => console.error("Failed to detect watch support:", err)); }js/demo/src/index.html (1)
105-131:⚠️ Potential issue | 🟡 MinorStale
hangreferences in inline code examples.The code examples in this HTML still reference the old naming:
- Line 106:
import { Watch } from "@moq/hang"- Line 109:
function Volume(hang: Watch)- Lines 111, 125:
hang.volumeThese should be updated to use the new
moq-*naming for consistency with the rest of the rename.doc/js/index.md (2)
180-188:⚠️ Potential issue | 🟡 MinorMissing
MoqPublishtype import in React example snippet.The snippet casts to
MoqPublish(Line 182) but doesn't show the corresponding import. Readers may not know where the type comes from.📝 Suggested addition
```typescript import react from "@moq/signals/react"; +import type MoqPublish from "@moq/publish/element"; const publish = document.querySelector("moq-publish") as MoqPublish;
192-201:⚠️ Potential issue | 🟡 MinorSame missing import in the SolidJS example snippet.
Line 195 uses
MoqPublishwithout showing the import, same as the React example above.
🤖 Fix all issues with AI agents
In `@doc/js/env/web.md`:
- Around line 82-93: The example uses an undocumented value for the show
attribute on the <moq-watch-support> element: the documentation lists show as
accepting "full", "partial", or "none" but the example uses show="warning";
update the doc so the allowed values and example match — either add "warning" to
the show attribute list (and describe its behavior) or change the example to use
one of the documented values ("full", "partial", or "none") so the attribute
table and the <moq-watch-support show="..."> example are consistent.
- Around line 101-112: The example uses an invalid show value ("warning") for
the <moq-publish-support> element while the documented valid values for the show
attribute are "full", "partial", or "none"; update the example to use one of the
documented values (e.g., change show="warning" to show="partial") or
alternatively add "warning" to the documented allowed values if that behavior is
supported by the <moq-publish-support> implementation—adjust the text around the
show attribute and the example so they are consistent with the actual supported
values.
- Around line 140-144: The import line is using a named import for MoqWatch but
the element is exported as a default; replace the named import `import {
MoqWatch } from "@moq/watch/element";` with a default import `import MoqWatch
from "@moq/watch/element";` so the import of MoqWatch matches the export and the
other examples (MoqWatch/MoqPublish) that use default imports; keep the `const
watch = document.querySelector("moq-watch") as MoqWatch;` line unchanged.
In `@js/publish/src/support/element.ts`:
- Around line 28-32: The constructor currently calls isSupported() without
handling rejections which can cause unhandled promise errors; update the
constructor so the promise chain for isSupported() includes a .catch() that logs
the error (using your logging utility or console.error) and sets a safe fallback
via this.#support.set(false) (or another appropriate default) to ensure the
internal state is deterministic; reference the constructor, isSupported(), and
this.#support.set to locate where to append the .catch().
In `@js/watch/src/support/element.ts`:
- Around line 76-89: The truthiness checks in getSummary(support: Full) are
fragile because Object.values on audio.decoding/video.decoding can include
Partial-typed values (e.g., "none") that are truthy; update the .some/.every
predicates to explicit comparisons: for audio.decoding use
Object.values(support.audio.decoding).some(v => v === true) and .every(v => v
=== true), and for video.decoding use
Object.values(support.video.decoding).some(v => (v as any).software === true ||
(v as any).hardware === true) and .every(v => (v as any).software === true || (v
as any).hardware === true); leave the early checks for support.webtransport,
audio.render, and video.render unchanged. Ensure you reference getSummary and
the fields audio.decoding and video.decoding when making the changes.
🧹 Nitpick comments (5)
js/publish/src/support/element.ts (2)
1-290: Significant code duplication withMoqWatchSupport.This file shares ~90% of its code with
js/watch/src/support/element.ts— the entire rendering infrastructure (#render,#renderHeader,#renderDetailsgrid layout), the signal-based lifecycle (connectedCallback/disconnectedCallback), attribute handling,Showtype, andisFirefoxconstant are near-identical. The only meaningful differences are the#getSummarylogic and the detail rows.Consider extracting a shared base class or a composition helper (e.g., in a common support-ui package) that accepts the summary function and detail-row definitions as parameters. This would keep the two components in sync and reduce maintenance surface.
5-6:isFirefoxis duplicated in three places.This constant now appears in
js/publish/src/support/element.ts,js/watch/src/support/element.ts, andjs/watch/src/support/index.ts(plus the existingjs/hang/src/util/hacks.ts). Consider importing from a single shared location to keep the detection logic DRY.#!/bin/bash # Verify the duplication of isFirefox across the codebase rg -n 'isFirefox' --type=ts -C1js/watch/src/support/index.ts (2)
4-46: Shared types and constants are duplicated withjs/publish/src/support/index.ts.
Partial,Codec,Audio,Video, andCODECSare identical between the watch and publish support modules. Consider extracting them into a shared package (e.g.,@moq/support-commonor a shared types file) to keep them in sync.
60-78:videoDecoderSupportedlacks aVideoDecoderavailability guard, unlikeaudioDecoderSupported.
audioDecoderSupported(Line 49) guards withif (!globalThis.AudioDecoder) return false, butvideoDecoderSupportedhas no equivalent check forVideoDecoder. The caller (isSupported) does guard this externally, but for consistency and defensive coding, consider adding an internal guard.🛡️ Proposed fix
async function videoDecoderSupported(codec: keyof typeof CODECS): Promise<Codec> { + if (!globalThis.VideoDecoder) return { software: false }; + const software = await VideoDecoder.isConfigSupported({js/publish/src/support/index.ts (1)
4-14: Consider extracting shared types (Partial,Codec,Audio, andVideo) to avoid duplication between@moq/watch/supportand@moq/publish/support.These four types are identical across both packages. Since both packages are independent, a shared types module (e.g., in
@moq/hangor a new@moq/support-typespackage) could reduce future drift when these types evolve. Note: theFulltype intentionally differs between packages due to their different purposes (encoding vs decoding).
| - `show` - "full", "partial", or "none" (default: "full") | ||
| - `details` - show detailed codec information | ||
|
|
||
| **Example:** | ||
|
|
||
| ```html | ||
| <script type="module"> | ||
| import "@moq/hang/support/element"; | ||
| import "@moq/watch/support/element"; | ||
| </script> | ||
|
|
||
| <!-- Show only when publishing is not supported --> | ||
| <hang-support mode="publish" show="partial"></hang-support> | ||
| <!-- Show only when a polyfill/fallback is needed --> | ||
| <moq-watch-support show="warning"></moq-watch-support> |
There was a problem hiding this comment.
show="warning" is not a documented value for the show attribute.
Line 82 documents show as accepting "full", "partial", or "none", but the example on Line 93 uses show="warning". Either the attribute table is incomplete (missing "warning") or the example uses an invalid value.
🤖 Prompt for AI Agents
In `@doc/js/env/web.md` around lines 82 - 93, The example uses an undocumented
value for the show attribute on the <moq-watch-support> element: the
documentation lists show as accepting "full", "partial", or "none" but the
example uses show="warning"; update the doc so the allowed values and example
match — either add "warning" to the show attribute list (and describe its
behavior) or change the example to use one of the documented values ("full",
"partial", or "none") so the attribute table and the <moq-watch-support
show="..."> example are consistent.
| - `show` - "full", "partial", or "none" (default: "full") | ||
| - `details` - show detailed codec information | ||
|
|
||
| **Example:** | ||
|
|
||
| ```html | ||
| <script type="module"> | ||
| import "@moq/publish/support/element"; | ||
| </script> | ||
|
|
||
| <!-- Show only when a polyfill/fallback is needed --> | ||
| <moq-publish-support show="warning"></moq-publish-support> |
There was a problem hiding this comment.
Same show="warning" inconsistency in <moq-publish-support>.
Line 101 documents show as "full" | "partial" | "none" but Line 112 uses show="warning" in the example. Same issue as the watch-support section above.
🤖 Prompt for AI Agents
In `@doc/js/env/web.md` around lines 101 - 112, The example uses an invalid show
value ("warning") for the <moq-publish-support> element while the documented
valid values for the show attribute are "full", "partial", or "none"; update the
example to use one of the documented values (e.g., change show="warning" to
show="partial") or alternatively add "warning" to the documented allowed values
if that behavior is supported by the <moq-publish-support> implementation—adjust
the text around the show attribute and the example so they are consistent with
the actual supported values.
|
|
||
| ```typescript | ||
| import { HangWatch } from "@moq/watch/element"; | ||
| import { MoqWatch } from "@moq/watch/element"; | ||
|
|
||
| const watch = document.querySelector("hang-watch") as HangWatch; | ||
| const watch = document.querySelector("moq-watch") as MoqWatch; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the export patterns in the element files
echo "=== Watch element export ==="
rg -n 'export\s+(default\s+)?class\s+MoqWatch' --type=ts
echo ""
echo "=== Publish element export (for comparison) ==="
rg -n 'export\s+(default\s+)?class\s+MoqPublish' --type=ts
echo ""
echo "=== Check the actual file lines in web.md ==="
# Look at lines around 140-144
sed -n '140,144p' doc/js/env/web.md
echo ""
echo "=== Check lines around 261-262 in web.md ==="
sed -n '260,263p' doc/js/env/web.mdRepository: moq-dev/moq
Length of output: 923
Fix inconsistent import syntax: use default import for MoqWatch element.
Line 142 uses a named import import { MoqWatch } but MoqWatch is exported as a default export from js/watch/src/element.ts. This conflicts with the correct default import pattern used at lines 261–262 for both MoqWatch and MoqPublish.
📝 Suggested fix
-import { MoqWatch } from "@moq/watch/element";
+import type MoqWatch from "@moq/watch/element";🤖 Prompt for AI Agents
In `@doc/js/env/web.md` around lines 140 - 144, The import line is using a named
import for MoqWatch but the element is exported as a default; replace the named
import `import { MoqWatch } from "@moq/watch/element";` with a default import
`import MoqWatch from "@moq/watch/element";` so the import of MoqWatch matches
the export and the other examples (MoqWatch/MoqPublish) that use default
imports; keep the `const watch = document.querySelector("moq-watch") as
MoqWatch;` line unchanged.
| constructor() { | ||
| super(); | ||
|
|
||
| isSupported().then((s) => this.#support.set(s)); | ||
| } |
There was a problem hiding this comment.
Unhandled promise rejection from isSupported().
If isSupported() rejects (e.g., a browser API throws unexpectedly), the promise in the constructor becomes an unhandled rejection. Add a .catch() to set a safe fallback or log the error.
🛡️ Proposed fix
constructor() {
super();
- isSupported().then((s) => this.#support.set(s));
+ isSupported()
+ .then((s) => this.#support.set(s))
+ .catch((err) => console.error("Failed to detect publish support:", 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.
| constructor() { | |
| super(); | |
| isSupported().then((s) => this.#support.set(s)); | |
| } | |
| constructor() { | |
| super(); | |
| isSupported() | |
| .then((s) => this.#support.set(s)) | |
| .catch((err) => console.error("Failed to detect publish support:", err)); | |
| } |
🤖 Prompt for AI Agents
In `@js/publish/src/support/element.ts` around lines 28 - 32, The constructor
currently calls isSupported() without handling rejections which can cause
unhandled promise errors; update the constructor so the promise chain for
isSupported() includes a .catch() that logs the error (using your logging
utility or console.error) and sets a safe fallback via this.#support.set(false)
(or another appropriate default) to ensure the internal state is deterministic;
reference the constructor, isSupported(), and this.#support.set to locate where
to append the .catch().
| #getSummary(support: Full): Partial { | ||
| if (support.webtransport === "none") return "none"; | ||
|
|
||
| if (!support.audio.decoding || !support.video.decoding) return "none"; | ||
| if (!support.audio.render || !support.video.render) return "none"; | ||
|
|
||
| if (!Object.values(support.audio.decoding).some((v) => v)) return "none"; | ||
| if (!Object.values(support.video.decoding).some((v) => v.software || v.hardware)) return "none"; | ||
|
|
||
| if (!Object.values(support.audio.decoding).every((v) => v)) return "partial"; | ||
| if (!Object.values(support.video.decoding).every((v) => v.software || v.hardware)) return "partial"; | ||
|
|
||
| return "full"; | ||
| } |
There was a problem hiding this comment.
Truthiness checks on Object.values() are fragile with mixed boolean | Partial types.
Audio is { aac: boolean; opus: Partial }, so Object.values() yields [boolean, string]. The .some((v) => v) and .every((v) => v) checks rely on truthiness, but Partial includes "none" which is a truthy string. If opus were ever "none", it would incorrectly pass the truthiness check.
Currently this doesn't manifest because isSupported() only returns "full" or "partial" for opus, but it's a latent bug if the support detection ever changes. Consider explicit value comparisons for the Partial-typed fields.
🤖 Prompt for AI Agents
In `@js/watch/src/support/element.ts` around lines 76 - 89, The truthiness checks
in getSummary(support: Full) are fragile because Object.values on
audio.decoding/video.decoding can include Partial-typed values (e.g., "none")
that are truthy; update the .some/.every predicates to explicit comparisons: for
audio.decoding use Object.values(support.audio.decoding).some(v => v === true)
and .every(v => v === true), and for video.decoding use
Object.values(support.video.decoding).some(v => (v as any).software === true ||
(v as any).hardware === true) and .every(v => (v as any).software === true || (v
as any).hardware === true); leave the early checks for support.webtransport,
audio.render, and video.render unchanged. Ensure you reference getSummary and
the fields audio.decoding and video.decoding when making the changes.
1 participant
Also split
hang-supportinto pieces.hang-watch->moq-watchhang-watch-ui->moq-watch-uihang-publish->moq-publishhang-publish-ui->moq-publish-uihang-support->moq-watch-supportmoq-publish-supporthang-demo->moq-demo