Migrate UI from SolidJS to vanilla Web Components#1412
Conversation
Rewrite js/watch/ui and js/publish/ui as plain Web Components that subscribe to @moq/signals signals directly, removing the SolidJS context/hook/accessor layer and the @moq/ui-core shared package. Each control is now a single function (effect, host) => HTMLElement that builds DOM imperatively and reacts via effect.run(). The Stats provider class registry collapses into four functions in stats.ts; Button collapses into plain <button>; Icon collapses into a single icon(svg) helper plus per-package SVG imports. CSS, public custom-element names (<moq-watch-ui>, <moq-publish-ui>), and the nested-element API are preserved. moq-boy's variables.css import is inlined since ui-core no longer exists. Docs are updated to drop SolidJS wording and the ui-core package page. Bundle: watch/ui drops from ~50 KB gz to 7.4 KB gz; publish/ui to 4.1 KB gz. No solid-js, solid-element, or vite-plugin-solid in the watch/publish builds.
WalkthroughThis PR completes a migration from a SolidJS-based UI framework with a shared 🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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/publish/README.md`:
- Line 30: Update the CDN import string in the README: replace the incorrect
module specifier "https://cdn.jsdelivr.net/npm/@moq/publish/ui/element.js/+esm"
with the package-exported path
"https://cdn.jsdelivr.net/npm/@moq/publish/ui/+esm" so the import matches
package.json exports (./ui -> ./src/ui/element.ts) and will resolve on jsDelivr;
locate and edit the import statement shown in the diff to use the shorter
/ui/+esm path.
In `@js/publish/src/ui/components/media-source-selector.ts`:
- Around line 14-31: The toggle button currently only changes visuals; update
the DOM attributes so assistive tech knows the state and control relationship:
give the <select> element a stable id (e.g., add id on the element created in
media-source-selector.ts) and an accessible label (set aria-label or link a
visible label via aria-labelledby), add aria-controls on the toggle pointing to
that select id, and add aria-expanded on the toggle that you update inside the
render() function to reflect the visible boolean (true when visible, false
otherwise); ensure these attributes are set when creating toggle and select and
kept in sync inside render().
In `@js/publish/src/ui/element.ts`:
- Around line 39-44: The `#mount`(publish: MoqPublish) method currently
unconditionally resets publish to the "nothing" state and can clobber
host-provided state; change it to only set default values when properties are
undefined (e.g., only set publish.muted = true if publish.muted is undefined,
same for publish.invisible and publish.source) so existing host state is
preserved when present while still providing sensible defaults when missing.
In `@js/publish/src/ui/icons.ts`:
- Around line 11-17: The exported icon(svg: string) helper currently uses
span.innerHTML which opens an XSS risk; replace raw innerHTML usage in icon with
a safe approach: either change the API to accept an SVGElement (e.g., require
callers to pass an actual SVG DOM node and append it to the span) or, if keeping
a string input, parse the string with DOMParser, validate that the top-level
node is an <svg> element (and reject/strip anything else), then import/append
that SVG node into the span via appendChild (no innerHTML). Update icon to
perform the validation/parsing logic and only append the sanitized SVG element.
In `@js/publish/src/ui/styles/base.css`:
- Around line 71-86: Buttons lack a visible keyboard focus state; add a
.button:not(:disabled):focus-visible rule (placed after the existing
.button:hover/.button:active/.button:disabled blocks) that provides a clear,
high-contrast indicator such as outline: 2px solid var(--color-brand) or
outline: 3px solid var(--color-white) combined with a visible box-shadow (e.g. 0
0 0 4px var(--color-white-alpha-20)) and avoid removing transform so keyboard
focus isn't confused with :hover/:active; target the existing .button selector
variants (.button:hover, .button:active, .button:disabled) when locating where
to add the new :focus-visible rule.
In `@js/watch/src/ui/components/buffer-control.ts`:
- Around line 129-144: The click/drag handler updateFromX can compute trackWidth
<= 0 causing invalid math and writes; in updateFromX (near viz, targetLine,
targetLabel, and watch.backend.latency) guard the width calculation by computing
const trackWidth = Math.max(0, rect.width - LABEL_WIDTH) and if trackWidth === 0
set a safe default display value (e.g. clamped = MIN_RANGE) without performing
the division, then compute ms/snapped/clamped only when trackWidth > 0; always
clamp the final value to [MIN_RANGE, max], update
watch.backend.latency.set(clamped) and then update targetLine.style.left,
targetLabel.textContent, and viz.setAttribute("aria-valuenow", ...) using the
clamped value so no NaN/Infinity or negative percentages are written.
In `@js/watch/src/ui/components/fullscreen-button.ts`:
- Around line 13-18: The fullscreen icon may be wrong on mount because
updateIcon only runs on "fullscreenchange"; call updateIcon immediately during
setup to initialize state. In the fullscreen-button component, after creating
updateIcon (the const updateIcon = () => { ... }) and registering the listener
with parent.event(document, "fullscreenchange", updateIcon), invoke updateIcon()
once so the button.replaceChildren(icon(...)) uses the current
document.fullscreenElement on mount.
In `@js/watch/src/ui/components/volume-slider.ts`:
- Around line 20-27: The range input "slider" lacks an accessible name;
associate it with the visible label element "label" (class
watch-ui__volume-label) by giving the span a unique id and either setting
slider.id and using a <label for="..."> association or setting
slider.setAttribute('aria-labelledby', '<span-id>') (or
slider.setAttribute('aria-label', 'Volume') if a visible label isn't used);
update creation of "slider" and "label" so the slider has an accessible name via
id/aria-labelledby or aria-label.
In `@js/watch/src/ui/components/watch-status-indicator.ts`:
- Around line 21-27: The status indicator currently creates elements wrapper,
dot, and text but doesn't expose changes to AT; update the wrapper element
(watch-ui__status-indicator) to be a live region by adding aria-live="polite"
(or "assertive" if immediate), aria-atomic="true" and/or role="status" so screen
readers announce transitions, and ensure status updates are written to the text
element's textContent (or updated node) so changes trigger the live-region
announcement.
In `@js/watch/src/ui/element.ts`:
- Around line 14-24: Add a deterministic cleanup by implementing
disconnectedCallback() on MoqWatchUi to immediately close the signal effect and
mark the element as unmounted: call this.signals.close(), set this.#mounted =
false, and (optionally) guard so you only close if signals are not already
closed; keep the existing FinalizationRegistry (`cleanup`) as a fallback but
ensure immediate teardown happens in disconnectedCallback to stop effects as
soon as the element is detached.
- Around line 31-39: The connectedCallback currently sets the private flag
`#mounted` before confirming the presence of the moq-watch element, which prevents
future mount attempts if querySelector("moq-watch") returns null; modify
connectedCallback so that you only set this.#mounted = true after
customElements.whenDefined("moq-watch") resolves and you have confirmed const
watch = this.querySelector("moq-watch") is non-null and after calling
this.#mount(watch); keep the early-return guard (if (this.#mounted) return) but
ensure the assignment happens after successful discovery and mounting via
`#mount`(watch).
- Line 75: The call to customElements.define("moq-watch-ui", MoqWatchUi) can
throw if the element is already registered; update the module to first check
customElements.get("moq-watch-ui") and only call
customElements.define("moq-watch-ui", MoqWatchUi) when the getter returns
undefined (do the same in the publish module for its registration), ensuring you
reference the MoqWatchUi class and the exact tag string so duplicate
registration is avoided.
In `@js/watch/src/ui/styles/base.css`:
- Around line 57-84: The .button styles lack a keyboard focus indicator—add a
.button:focus-visible rule that outlines or highlights the button when navigated
by keyboard: use a clear, accessible outline (e.g., a visible contrast color or
box-shadow) and ensure it doesn't conflict with .button:active or :hover; keep
the focus style distinct from hover (use .button:focus-visible { outline: /*
accessible color */; outline-offset: /* small value */; } or box-shadow) and
preserve existing border-radius and transition so .button,
.button:focus-visible, and .button:disabled interact correctly.
🪄 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: 90ed99b6-aa69-41e3-9d24-0e95e2b81aa4
⛔ Files ignored due to path filters (21)
bun.lockis excluded by!**/*.lockjs/publish/src/ui/icons/arrow-down.svgis excluded by!**/*.svgjs/publish/src/ui/icons/arrow-up.svgis excluded by!**/*.svgjs/publish/src/ui/icons/ban.svgis excluded by!**/*.svgjs/publish/src/ui/icons/camera.svgis excluded by!**/*.svgjs/publish/src/ui/icons/file.svgis excluded by!**/*.svgjs/publish/src/ui/icons/microphone.svgis excluded by!**/*.svgjs/publish/src/ui/icons/screen.svgis excluded by!**/*.svgjs/watch/src/ui/icons/audio.svgis excluded by!**/*.svgjs/watch/src/ui/icons/buffer.svgis excluded by!**/*.svgjs/watch/src/ui/icons/fullscreen-enter.svgis excluded by!**/*.svgjs/watch/src/ui/icons/fullscreen-exit.svgis excluded by!**/*.svgjs/watch/src/ui/icons/mute.svgis excluded by!**/*.svgjs/watch/src/ui/icons/network.svgis excluded by!**/*.svgjs/watch/src/ui/icons/pause.svgis excluded by!**/*.svgjs/watch/src/ui/icons/play.svgis excluded by!**/*.svgjs/watch/src/ui/icons/stats.svgis excluded by!**/*.svgjs/watch/src/ui/icons/video.svgis excluded by!**/*.svgjs/watch/src/ui/icons/volume-high.svgis excluded by!**/*.svgjs/watch/src/ui/icons/volume-low.svgis excluded by!**/*.svgjs/watch/src/ui/icons/volume-medium.svgis excluded by!**/*.svg
📒 Files selected for processing (96)
CLAUDE.mddoc/.vitepress/config.tsdoc/js/@moq/hang/index.mddoc/js/@moq/hang/publish.mddoc/js/@moq/hang/watch.mddoc/js/@moq/publish.mddoc/js/@moq/ui-core.mddoc/js/@moq/watch.mddoc/js/env/web.mddoc/js/index.mdjs/hang/README.mdjs/moq-boy/package.jsonjs/moq-boy/src/ui/styles/index.cssjs/moq-boy/src/ui/styles/variables.cssjs/moq-boy/vite.config.tsjs/publish/README.mdjs/publish/package.jsonjs/publish/src/ui/components/CameraSourceButton.tsxjs/publish/src/ui/components/FileSourceButton.tsxjs/publish/src/ui/components/MediaSourceSelector.tsxjs/publish/src/ui/components/MicrophoneSourceButton.tsxjs/publish/src/ui/components/NothingSourceButton.tsxjs/publish/src/ui/components/PublishControls.tsxjs/publish/src/ui/components/PublishStatusIndicator.tsxjs/publish/src/ui/components/ScreenSourceButton.tsxjs/publish/src/ui/components/camera-source-button.tsjs/publish/src/ui/components/file-source-button.tsjs/publish/src/ui/components/media-source-selector.tsjs/publish/src/ui/components/microphone-source-button.tsjs/publish/src/ui/components/nothing-source-button.tsjs/publish/src/ui/components/publish-status-indicator.tsjs/publish/src/ui/components/screen-source-button.tsjs/publish/src/ui/context.tsxjs/publish/src/ui/element.tsjs/publish/src/ui/element.tsxjs/publish/src/ui/hooks/use-publish-ui.tsjs/publish/src/ui/icons.tsjs/publish/src/ui/index.tsxjs/publish/src/ui/styles/base.cssjs/publish/src/ui/styles/index.cssjs/publish/tsconfig.jsonjs/publish/vite.config.tsjs/ui-core/README.mdjs/ui-core/package.jsonjs/ui-core/src/button/button.cssjs/ui-core/src/button/button.tsxjs/ui-core/src/flex.cssjs/ui-core/src/icon/icon.tsxjs/ui-core/src/index.tsjs/ui-core/src/stats/README.mdjs/ui-core/src/stats/components/StatsItem.tsxjs/ui-core/src/stats/components/StatsPanel.tsxjs/ui-core/src/stats/index.tsxjs/ui-core/src/stats/providers/audio.tsjs/ui-core/src/stats/providers/base.tsjs/ui-core/src/stats/providers/buffer.tsjs/ui-core/src/stats/providers/index.tsjs/ui-core/src/stats/providers/network.tsjs/ui-core/src/stats/providers/registry.tsjs/ui-core/src/stats/providers/video.tsjs/ui-core/src/stats/types.tsjs/ui-core/src/vite-env.d.tsjs/ui-core/tsconfig.jsonjs/ui-core/vite.config.tsjs/watch/README.mdjs/watch/package.jsonjs/watch/src/ui/components/BufferControl.tsxjs/watch/src/ui/components/BufferingIndicator.tsxjs/watch/src/ui/components/FullscreenButton.tsxjs/watch/src/ui/components/PlayPauseButton.tsxjs/watch/src/ui/components/QualitySelector.tsxjs/watch/src/ui/components/StatsButton.tsxjs/watch/src/ui/components/VolumeSlider.tsxjs/watch/src/ui/components/WatchControls.tsxjs/watch/src/ui/components/WatchStatusIndicator.tsxjs/watch/src/ui/components/buffer-control.tsjs/watch/src/ui/components/buffering-indicator.tsjs/watch/src/ui/components/fullscreen-button.tsjs/watch/src/ui/components/play-pause.tsjs/watch/src/ui/components/quality-selector.tsjs/watch/src/ui/components/stats-button.tsjs/watch/src/ui/components/volume-slider.tsjs/watch/src/ui/components/watch-status-indicator.tsjs/watch/src/ui/context.tsxjs/watch/src/ui/element.tsjs/watch/src/ui/element.tsxjs/watch/src/ui/hooks/use-watch-ui.tsjs/watch/src/ui/icons.tsjs/watch/src/ui/index.tsxjs/watch/src/ui/stats.tsjs/watch/src/ui/styles/base.cssjs/watch/src/ui/styles/index.cssjs/watch/src/ui/styles/stats.cssjs/watch/tsconfig.jsonjs/watch/vite.config.tspackage.json
💤 Files with no reviewable changes (55)
- js/watch/src/ui/components/PlayPauseButton.tsx
- js/ui-core/src/stats/providers/registry.ts
- js/watch/src/ui/components/BufferingIndicator.tsx
- js/ui-core/src/stats/README.md
- js/ui-core/README.md
- js/ui-core/src/button/button.tsx
- doc/js/@moq/ui-core.md
- js/publish/src/ui/element.tsx
- js/publish/src/ui/components/ScreenSourceButton.tsx
- js/ui-core/vite.config.ts
- js/watch/tsconfig.json
- package.json
- js/ui-core/src/vite-env.d.ts
- js/ui-core/tsconfig.json
- js/publish/src/ui/components/MediaSourceSelector.tsx
- js/publish/src/ui/components/PublishStatusIndicator.tsx
- js/watch/src/ui/context.tsx
- js/ui-core/src/stats/providers/video.ts
- js/ui-core/src/stats/providers/audio.ts
- js/publish/src/ui/components/FileSourceButton.tsx
- js/ui-core/src/stats/providers/buffer.ts
- js/watch/src/ui/components/FullscreenButton.tsx
- js/ui-core/src/flex.css
- js/publish/src/ui/components/PublishControls.tsx
- js/watch/src/ui/element.tsx
- js/watch/src/ui/components/WatchControls.tsx
- js/watch/src/ui/components/QualitySelector.tsx
- js/ui-core/src/stats/index.tsx
- js/ui-core/src/stats/components/StatsPanel.tsx
- js/publish/src/ui/components/MicrophoneSourceButton.tsx
- js/watch/src/ui/components/WatchStatusIndicator.tsx
- js/ui-core/src/button/button.css
- js/publish/tsconfig.json
- doc/.vitepress/config.ts
- js/watch/src/ui/components/StatsButton.tsx
- js/watch/src/ui/components/BufferControl.tsx
- js/publish/src/ui/context.tsx
- js/watch/src/ui/hooks/use-watch-ui.ts
- js/watch/src/ui/index.tsx
- js/ui-core/src/icon/icon.tsx
- js/ui-core/src/stats/providers/base.ts
- js/publish/src/ui/components/CameraSourceButton.tsx
- js/watch/src/ui/components/VolumeSlider.tsx
- js/ui-core/src/index.ts
- js/publish/src/ui/components/NothingSourceButton.tsx
- js/ui-core/src/stats/providers/network.ts
- js/moq-boy/package.json
- js/hang/README.md
- js/ui-core/src/stats/providers/index.ts
- js/publish/src/ui/index.tsx
- doc/js/@moq/hang/index.md
- js/ui-core/package.json
- js/ui-core/src/stats/components/StatsItem.tsx
- js/publish/src/ui/hooks/use-publish-ui.ts
- js/ui-core/src/stats/types.ts
| <script type="module"> | ||
| import "https://cdn.jsdelivr.net/npm/@moq/publish/element.js/+esm"; | ||
| import "https://cdn.jsdelivr.net/npm/@moq/publish/ui/index.js/+esm"; | ||
| import "https://cdn.jsdelivr.net/npm/@moq/publish/ui/element.js/+esm"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify package.json exports the ui/element entry point correctly
# Check package.json exports field for ui entry point
cat js/publish/package.json | jq -r '.exports["./ui"]' 2>/dev/null || echo "No ./ui export found"
cat js/publish/package.json | jq -r '.exports["./ui/element"]' 2>/dev/null || echo "No ./ui/element export found"
# Display full exports object for manual inspection
echo "--- Full exports configuration ---"
cat js/publish/package.json | jq '.exports'Repository: moq-dev/moq
Length of output: 303
Fix the CDN import path to match the package.json exports configuration.
The CDN import path references @moq/publish/ui/element.js, but the package.json exports only ./ui (which maps to ./src/ui/element.ts). The correct import should be:
import "https://cdn.jsdelivr.net/npm/@moq/publish/ui/+esm";The path ui/element.js does not have a corresponding export and will fail to resolve via jsDelivr's CDN.
🤖 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/publish/README.md` at line 30, Update the CDN import string in the README:
replace the incorrect module specifier
"https://cdn.jsdelivr.net/npm/@moq/publish/ui/element.js/+esm" with the
package-exported path "https://cdn.jsdelivr.net/npm/@moq/publish/ui/+esm" so the
import matches package.json exports (./ui -> ./src/ui/element.ts) and will
resolve on jsDelivr; locate and edit the import statement shown in the diff to
use the shorter /ui/+esm path.
| const toggle = document.createElement("button"); | ||
| toggle.type = "button"; | ||
| toggle.className = "button publish-ui__media-selector-toggle"; | ||
| toggle.title = "Show Sources"; | ||
| toggle.appendChild(icon(arrowDown)); | ||
|
|
||
| const select = document.createElement("select"); | ||
| select.className = "publish-ui__media-selector-dropdown"; | ||
| select.style.display = "none"; | ||
|
|
||
| wrapper.append(toggle, select); | ||
|
|
||
| let visible = false; | ||
| const render = () => { | ||
| toggle.replaceChildren(icon(visible ? arrowUp : arrowDown)); | ||
| toggle.title = visible ? "Hide Sources" : "Show Sources"; | ||
| select.style.display = visible ? "" : "none"; | ||
|
|
There was a problem hiding this comment.
Add ARIA state wiring for the source dropdown toggle.
The toggle currently changes visibility visually, but it does not expose expanded state/control linkage for assistive tech. Add aria-expanded and aria-controls, and label the <select>.
Suggested fix
const toggle = document.createElement("button");
toggle.type = "button";
toggle.className = "button publish-ui__media-selector-toggle";
toggle.title = "Show Sources";
+ const selectId = "publish-media-source-selector";
+ toggle.setAttribute("aria-controls", selectId);
+ toggle.setAttribute("aria-expanded", "false");
toggle.appendChild(icon(arrowDown));
const select = document.createElement("select");
+ select.id = selectId;
+ select.setAttribute("aria-label", "Media source");
select.className = "publish-ui__media-selector-dropdown";
select.style.display = "none";
@@
toggle.replaceChildren(icon(visible ? arrowUp : arrowDown));
toggle.title = visible ? "Hide Sources" : "Show Sources";
+ toggle.setAttribute("aria-expanded", String(visible));
select.style.display = visible ? "" : "none";📝 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.
| const toggle = document.createElement("button"); | |
| toggle.type = "button"; | |
| toggle.className = "button publish-ui__media-selector-toggle"; | |
| toggle.title = "Show Sources"; | |
| toggle.appendChild(icon(arrowDown)); | |
| const select = document.createElement("select"); | |
| select.className = "publish-ui__media-selector-dropdown"; | |
| select.style.display = "none"; | |
| wrapper.append(toggle, select); | |
| let visible = false; | |
| const render = () => { | |
| toggle.replaceChildren(icon(visible ? arrowUp : arrowDown)); | |
| toggle.title = visible ? "Hide Sources" : "Show Sources"; | |
| select.style.display = visible ? "" : "none"; | |
| const toggle = document.createElement("button"); | |
| toggle.type = "button"; | |
| toggle.className = "button publish-ui__media-selector-toggle"; | |
| toggle.title = "Show Sources"; | |
| const selectId = "publish-media-source-selector"; | |
| toggle.setAttribute("aria-controls", selectId); | |
| toggle.setAttribute("aria-expanded", "false"); | |
| toggle.appendChild(icon(arrowDown)); | |
| const select = document.createElement("select"); | |
| select.id = selectId; | |
| select.setAttribute("aria-label", "Media source"); | |
| select.className = "publish-ui__media-selector-dropdown"; | |
| select.style.display = "none"; | |
| wrapper.append(toggle, select); | |
| let visible = false; | |
| const render = () => { | |
| toggle.replaceChildren(icon(visible ? arrowUp : arrowDown)); | |
| toggle.title = visible ? "Hide Sources" : "Show Sources"; | |
| toggle.setAttribute("aria-expanded", String(visible)); | |
| select.style.display = visible ? "" : "none"; |
🤖 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/publish/src/ui/components/media-source-selector.ts` around lines 14 - 31,
The toggle button currently only changes visuals; update the DOM attributes so
assistive tech knows the state and control relationship: give the <select>
element a stable id (e.g., add id on the element created in
media-source-selector.ts) and an accessible label (set aria-label or link a
visible label via aria-labelledby), add aria-controls on the toggle pointing to
that select id, and add aria-expanded on the toggle that you update inside the
render() function to reflect the visible boolean (true when visible, false
otherwise); ensure these attributes are set when creating toggle and select and
kept in sync inside render().
| export function icon(svg: string): HTMLElement { | ||
| const span = document.createElement("span"); | ||
| span.className = "flex--center"; | ||
| span.setAttribute("role", "img"); | ||
| span.setAttribute("aria-hidden", "true"); | ||
| span.innerHTML = svg; | ||
| return span; |
There was a problem hiding this comment.
Avoid raw innerHTML in exported icon helper.
Line 16 injects arbitrary SVG markup into the DOM. Since icon is exported with a string input, this creates an avoidable XSS surface if any caller passes untrusted content.
Suggested fix
-export function icon(svg: string): HTMLElement {
+type IconSvg = typeof arrowDown | typeof arrowUp | typeof ban | typeof camera | typeof file | typeof microphone | typeof screen;
+
+export function icon(svg: IconSvg): HTMLElement {
const span = document.createElement("span");
span.className = "flex--center";
span.setAttribute("role", "img");
span.setAttribute("aria-hidden", "true");
- span.innerHTML = svg;
+ const tpl = document.createElement("template");
+ tpl.innerHTML = svg;
+ const node = tpl.content.firstElementChild;
+ if (node) span.appendChild(node.cloneNode(true));
return span;
}🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 15-15: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: span.innerHTML = svg
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 15-15: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: span.innerHTML = svg
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 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/publish/src/ui/icons.ts` around lines 11 - 17, The exported icon(svg:
string) helper currently uses span.innerHTML which opens an XSS risk; replace
raw innerHTML usage in icon with a safe approach: either change the API to
accept an SVGElement (e.g., require callers to pass an actual SVG DOM node and
append it to the span) or, if keeping a string input, parse the string with
DOMParser, validate that the top-level node is an <svg> element (and
reject/strip anything else), then import/append that SVG node into the span via
appendChild (no innerHTML). Update icon to perform the validation/parsing logic
and only append the sanitized SVG element.
| .button:hover:not(:disabled) { | ||
| transform: scale(1.02); | ||
| background-color: var(--color-white-alpha-20); | ||
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | ||
| } | ||
|
|
||
| .button:active:not(:disabled) { | ||
| transform: scale(0.98); | ||
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | ||
| } | ||
|
|
||
| .button:disabled { | ||
| opacity: 0.5; | ||
| cursor: default; | ||
| color: var(--color-gray-100); | ||
| } |
There was a problem hiding this comment.
Add a visible keyboard focus state for buttons.
On Line 71 to Line 86, pointer states are styled, but there is no :focus-visible treatment. Keyboard users will not get a clear focus indicator.
Suggested fix
.button:active:not(:disabled) {
transform: scale(0.98);
box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08);
}
+.button:focus-visible {
+ outline: var(--spacing-2) solid var(--color-blue-500);
+ outline-offset: var(--spacing-2);
+}
+
.button:disabled {
opacity: 0.5;
cursor: default;
color: var(--color-gray-100);
}📝 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.
| .button:hover:not(:disabled) { | |
| transform: scale(1.02); | |
| background-color: var(--color-white-alpha-20); | |
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | |
| } | |
| .button:active:not(:disabled) { | |
| transform: scale(0.98); | |
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | |
| } | |
| .button:disabled { | |
| opacity: 0.5; | |
| cursor: default; | |
| color: var(--color-gray-100); | |
| } | |
| .button:hover:not(:disabled) { | |
| transform: scale(1.02); | |
| background-color: var(--color-white-alpha-20); | |
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | |
| } | |
| .button:active:not(:disabled) { | |
| transform: scale(0.98); | |
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | |
| } | |
| .button:focus-visible { | |
| outline: var(--spacing-2) solid var(--color-blue-500); | |
| outline-offset: var(--spacing-2); | |
| } | |
| .button:disabled { | |
| opacity: 0.5; | |
| cursor: default; | |
| color: var(--color-gray-100); | |
| } |
🤖 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/publish/src/ui/styles/base.css` around lines 71 - 86, Buttons lack a
visible keyboard focus state; add a .button:not(:disabled):focus-visible rule
(placed after the existing .button:hover/.button:active/.button:disabled blocks)
that provides a clear, high-contrast indicator such as outline: 2px solid
var(--color-brand) or outline: 3px solid var(--color-white) combined with a
visible box-shadow (e.g. 0 0 0 4px var(--color-white-alpha-20)) and avoid
removing transform so keyboard focus isn't confused with :hover/:active; target
the existing .button selector variants (.button:hover, .button:active,
.button:disabled) when locating where to add the new :focus-visible rule.
| const wrapper = document.createElement("div"); | ||
| wrapper.className = "watch-ui__status-indicator flex--center"; | ||
|
|
||
| const dot = document.createElement("span"); | ||
| const text = document.createElement("span"); | ||
| wrapper.append(dot, text); | ||
|
|
There was a problem hiding this comment.
Expose status updates as a live region.
Without live-region semantics, status transitions may not be announced to assistive technologies.
Proposed fix
const wrapper = document.createElement("div");
wrapper.className = "watch-ui__status-indicator flex--center";
+wrapper.setAttribute("role", "status");
+wrapper.setAttribute("aria-live", "polite");📝 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.
| const wrapper = document.createElement("div"); | |
| wrapper.className = "watch-ui__status-indicator flex--center"; | |
| const dot = document.createElement("span"); | |
| const text = document.createElement("span"); | |
| wrapper.append(dot, text); | |
| const wrapper = document.createElement("div"); | |
| wrapper.className = "watch-ui__status-indicator flex--center"; | |
| wrapper.setAttribute("role", "status"); | |
| wrapper.setAttribute("aria-live", "polite"); | |
| const dot = document.createElement("span"); | |
| const text = document.createElement("span"); | |
| wrapper.append(dot, text); |
🤖 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/ui/components/watch-status-indicator.ts` around lines 21 - 27,
The status indicator currently creates elements wrapper, dot, and text but
doesn't expose changes to AT; update the wrapper element
(watch-ui__status-indicator) to be a live region by adding aria-live="polite"
(or "assertive" if immediate), aria-atomic="true" and/or role="status" so screen
readers announce transitions, and ensure status updates are written to the text
element's textContent (or updated node) so changes trigger the live-region
announcement.
| } | ||
| } | ||
|
|
||
| customElements.define("moq-watch-ui", MoqWatchUi); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current registration pattern in watch/publish UI elements.
rg -n -C2 'customElements\.define\("moq-watch-ui"|customElements\.get\("moq-watch-ui"' js/watch/src/ui/element.ts
rg -n -C2 'customElements\.define\("moq-publish-ui"|customElements\.get\("moq-publish-ui"' js/publish/src/ui/element.tsRepository: moq-dev/moq
Length of output: 233
🏁 Script executed:
# Check for imports of these element modules to understand re-evaluation risk
rg -n "from.*element" js/watch/src/ js/publish/src/ | head -20Repository: moq-dev/moq
Length of output: 1863
🏁 Script executed:
# Search for existing customElements.get patterns in the codebase
rg -n "customElements\.get" --type ts --type tsx --type js --type jsxRepository: moq-dev/moq
Length of output: 83
🏁 Script executed:
# Check the full context around both definitions to see if there are any guards
cat -n js/watch/src/ui/element.ts | sed -n '70,80p'
cat -n js/publish/src/ui/element.ts | sed -n '65,75p'Repository: moq-dev/moq
Length of output: 646
🏁 Script executed:
# Look for module federation or dynamic import patterns that could cause re-evaluation
rg -n "Module|federation|dynamic|import.*element" js/ --type ts --type tsx -A 2 | head -40Repository: moq-dev/moq
Length of output: 83
🏁 Script executed:
# Search for customElements.get patterns without the problematic file type flags
rg -n "customElements\.get"Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Look for actual runtime imports (not type imports) of the element modules
rg -n "import.*from.*element" --no-type-list | grep -v "import type" | head -20Repository: moq-dev/moq
Length of output: 499
🏁 Script executed:
# Check package.json and index files for entry points that might cause re-evaluation
cat js/watch/src/index.ts | head -30
cat js/publish/src/index.ts | head -30Repository: moq-dev/moq
Length of output: 968
🏁 Script executed:
# Check if there's any re-export or re-evaluation pattern in watch/publish main exports
rg -n "element.ts|customElements" js/watch/src/index.ts js/publish/src/index.tsRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Search for runtime imports without type keyword
rg "import.*from.*['\"].*element['\"]" --no-heading | grep -v "import type"Repository: moq-dev/moq
Length of output: 691
🏁 Script executed:
# Check the build configuration to understand bundling strategy
find . -name "tsconfig.json" -o -name "vite.config.*" -o -name "webpack.config.*" -o -name "package.json" | grep -E "(tsconfig|vite|webpack|package)" | head -10Repository: moq-dev/moq
Length of output: 297
🏁 Script executed:
# Look at package.json exports field to understand entry points
rg -A 10 '"exports"' package.json js/watch/package.json js/publish/package.json 2>/dev/null | head -40Repository: moq-dev/moq
Length of output: 1156
🏁 Script executed:
# Check if element modules are used anywhere at runtime (not type imports)
rg "element\.ts|element\.js" --no-heading | grep -v "import type" | head -20Repository: moq-dev/moq
Length of output: 1796
Guard custom element registration against duplicate definition.
Both js/watch/src/ui/element.ts (line 75) and js/publish/src/ui/element.ts (line 72) call customElements.define() without checking if the custom element is already registered. The modules are marked as having side effects in package.json and are designed for runtime import. If either module is imported more than once in the same realm—through bundling, module federation, or dynamic imports—the unguarded call will throw NotSupportedError.
Suggested fix
For js/watch/src/ui/element.ts (line 75):
-customElements.define("moq-watch-ui", MoqWatchUi);
+if (!customElements.get("moq-watch-ui")) {
+ customElements.define("moq-watch-ui", MoqWatchUi);
+}For js/publish/src/ui/element.ts (line 72):
-customElements.define("moq-publish-ui", MoqPublishUi);
+if (!customElements.get("moq-publish-ui")) {
+ customElements.define("moq-publish-ui", MoqPublishUi);
+}📝 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.
| customElements.define("moq-watch-ui", MoqWatchUi); | |
| if (!customElements.get("moq-watch-ui")) { | |
| customElements.define("moq-watch-ui", MoqWatchUi); | |
| } |
🤖 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/ui/element.ts` at line 75, The call to
customElements.define("moq-watch-ui", MoqWatchUi) can throw if the element is
already registered; update the module to first check
customElements.get("moq-watch-ui") and only call
customElements.define("moq-watch-ui", MoqWatchUi) when the getter returns
undefined (do the same in the publish module for its registration), ensuring you
reference the MoqWatchUi class and the exact tag string so duplicate
registration is avoided.
| .button { | ||
| color: var(--color-white); | ||
| cursor: pointer; | ||
| background-color: var(--color-white-alpha-10); | ||
| border: var(--spacing-1) solid var(--color-white-alpha-20); | ||
| border-radius: 50%; | ||
| width: 2.25rem; | ||
| height: 2.25rem; | ||
| transform-origin: center; | ||
| transition: transform 100ms cubic-bezier(0.4, 0, 0.2, 1); | ||
| } | ||
|
|
||
| .button:hover:not(:disabled) { | ||
| transform: scale(1.02); | ||
| background-color: var(--color-white-alpha-20); | ||
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | ||
| } | ||
|
|
||
| .button:active:not(:disabled) { | ||
| transform: scale(0.98); | ||
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | ||
| } | ||
|
|
||
| .button:disabled { | ||
| opacity: 0.5; | ||
| cursor: default; | ||
| color: var(--color-gray-100); | ||
| } |
There was a problem hiding this comment.
Add :focus-visible styling for keyboard accessibility.
Interactive .button states are defined, but there is no keyboard focus indicator.
💡 Suggested fix
.button:active:not(:disabled) {
transform: scale(0.98);
box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08);
}
+
+.button:focus-visible {
+ outline: 2px solid var(--color-blue-500);
+ outline-offset: 2px;
+}
.button:disabled {
opacity: 0.5;
cursor: default;
color: var(--color-gray-100);
}📝 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.
| .button { | |
| color: var(--color-white); | |
| cursor: pointer; | |
| background-color: var(--color-white-alpha-10); | |
| border: var(--spacing-1) solid var(--color-white-alpha-20); | |
| border-radius: 50%; | |
| width: 2.25rem; | |
| height: 2.25rem; | |
| transform-origin: center; | |
| transition: transform 100ms cubic-bezier(0.4, 0, 0.2, 1); | |
| } | |
| .button:hover:not(:disabled) { | |
| transform: scale(1.02); | |
| background-color: var(--color-white-alpha-20); | |
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | |
| } | |
| .button:active:not(:disabled) { | |
| transform: scale(0.98); | |
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | |
| } | |
| .button:disabled { | |
| opacity: 0.5; | |
| cursor: default; | |
| color: var(--color-gray-100); | |
| } | |
| .button { | |
| color: var(--color-white); | |
| cursor: pointer; | |
| background-color: var(--color-white-alpha-10); | |
| border: var(--spacing-1) solid var(--color-white-alpha-20); | |
| border-radius: 50%; | |
| width: 2.25rem; | |
| height: 2.25rem; | |
| transform-origin: center; | |
| transition: transform 100ms cubic-bezier(0.4, 0, 0.2, 1); | |
| } | |
| .button:hover:not(:disabled) { | |
| transform: scale(1.02); | |
| background-color: var(--color-white-alpha-20); | |
| box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); | |
| } | |
| .button:active:not(:disabled) { | |
| transform: scale(0.98); | |
| box-shadow: 0 var(--spacing-1) var(--spacing-4) var(--color-black-alpha-08); | |
| } | |
| .button:focus-visible { | |
| outline: 2px solid var(--color-blue-500); | |
| outline-offset: 2px; | |
| } | |
| .button:disabled { | |
| opacity: 0.5; | |
| cursor: default; | |
| color: var(--color-gray-100); | |
| } |
🤖 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/ui/styles/base.css` around lines 57 - 84, The .button styles
lack a keyboard focus indicator—add a .button:focus-visible rule that outlines
or highlights the button when navigated by keyboard: use a clear, accessible
outline (e.g., a visible contrast color or box-shadow) and ensure it doesn't
conflict with .button:active or :hover; keep the focus style distinct from hover
(use .button:focus-visible { outline: /* accessible color */; outline-offset: /*
small value */; } or box-shadow) and preserve existing border-radius and
transition so .button, .button:focus-visible, and .button:disabled interact
correctly.
| import { camera, icon } from "../icons"; | ||
| import { mediaSourceSelector } from "./media-source-selector"; | ||
|
|
||
| export function cameraSourceButton(parent: Effect, publish: MoqPublish): HTMLElement { |
There was a problem hiding this comment.
Can we put all of these source buttons in the same file? They're so similar
|
|
||
| wrapper.append(toggle, select); | ||
|
|
||
| let visible = false; |
There was a problem hiding this comment.
Seems like it should be a signal perhaps? We can make a new effect, instead of manually calling render().
| }); | ||
| } | ||
|
|
||
| #mount(publish: MoqPublish) { |
There was a problem hiding this comment.
How do we unmount?
Can you check how the old code used to work, before we ported to solid?
|
|
||
| const slot = document.createElement("slot"); | ||
| videoContainer.append(slot, statsPanel(this.signals, watch, visible), bufferingIndicator(this.signals, watch)); | ||
| root.appendChild(videoContainer); |
There was a problem hiding this comment.
We never unmount. That's why the moq/signals/dom helpers exist.
The watch and publish UI elements previously relied on a FinalizationRegistry to close their signals Effect, which only runs when the host element is garbage collected (unreliable, and never fires while the element is detached from the DOM but still reachable). This left intervals, event listeners on document/window, and signal subscriptions running after the element was removed. Mirror the pattern already used by `MoqWatchSupport` / `MoqPublishSupport`: construct the `#signals` Effect in `connectedCallback`, close it in `disconnectedCallback`, and render via a top-level `signals.run(#render)` that uses `DOM.create` and `DOM.render` from `@moq/signals/dom` so child nodes are torn down with the effect. While here, simplify two components that were juggling a manual `new Effect()` plus `appendChild` / `cleanup(() => remove())` pairs into the equivalent nested `effect.run()` + `DOM.render` pattern, and do the same for the stats panel's show/hide inner-effect bookkeeping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/publish/src/ui/element.ts (1)
56-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not force-reset host publish state during first render.
Line 59 to Line 61 still overwrite host state on attach (
muted,invisible,source). This can clobber valid preconfigured values before user interaction.Suggested minimal fix
- // Start with "nothing" selected so the UI matches what the user sees, but only once. - if (!this.#initialized) { - this.#initialized = true; - publish.muted = true; - publish.invisible = true; - publish.source = undefined; - } + if (!this.#initialized) { + this.#initialized = true; + if (publish.muted === undefined) publish.muted = true; + if (publish.invisible === undefined) publish.invisible = true; + if (publish.source === undefined) publish.source = undefined; + }🤖 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/publish/src/ui/element.ts` around lines 56 - 62, The current attach logic in the this.#initialized block is force-resetting host publish state (publish.muted, publish.invisible, publish.source) on first render; instead, preserve preconfigured host values by only initializing when those fields are strictly undefined (e.g., if publish.muted === undefined then set it, same for publish.invisible and publish.source) or remove the assignments entirely so you only toggle this.#initialized and avoid touching publish; keep the this.#initialized = true behavior but do not overwrite existing publish state.
🤖 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.
Duplicate comments:
In `@js/publish/src/ui/element.ts`:
- Around line 56-62: The current attach logic in the this.#initialized block is
force-resetting host publish state (publish.muted, publish.invisible,
publish.source) on first render; instead, preserve preconfigured host values by
only initializing when those fields are strictly undefined (e.g., if
publish.muted === undefined then set it, same for publish.invisible and
publish.source) or remove the assignments entirely so you only toggle
this.#initialized and avoid touching publish; keep the this.#initialized = true
behavior but do not overwrite existing publish state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0da44619-5060-44ba-a4aa-c0caa42aec86
📒 Files selected for processing (5)
js/publish/src/ui/components/camera-source-button.tsjs/publish/src/ui/components/microphone-source-button.tsjs/publish/src/ui/element.tsjs/watch/src/ui/element.tsjs/watch/src/ui/stats.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- js/publish/src/ui/components/camera-source-button.ts
Removes the
@moq/ui-corepackage and migrates@moq/watch/uiand@moq/publish/uifrom SolidJS to vanilla Web Components using the@moq/signalsreactive library.Summary
This change eliminates the shared UI component library and replaces SolidJS-based UI implementations with lightweight, framework-free Web Components. The new approach uses
@moq/signalsfor reactivity and state management, reducing dependencies and simplifying the UI layer.Key Changes
Removed
@moq/ui-corepackage entirely, including:Migrated
@moq/watch/uifrom SolidJS to vanilla Web Components:@moq/signalsEffect usage@moq/signalsinterval polling instead of provider patternMigrated
@moq/publish/uifrom SolidJS to vanilla Web Components:Updated exports and configuration:
./src/ui/index.tsxto./src/ui/element.tsin both watch and publish packages@moq/ui-core@moq/ui-corefrom moq-boy dependencies and documentationSimplified icon handling:
Implementation Details
FinalizationRegistryto ensureEffectcleanup when elements are garbage collected@moq/signalsinterval polling and signal subscriptions instead of SolidJS reactivity:hostlevel for proper Web Component scopinghttps://claude.ai/code/session_01MyRFg6YmnMnadiaem63ZYn