Skip to content

Migrate UI from SolidJS to vanilla Web Components#1412

Open
kixelated wants to merge 2 commits into
mainfrom
claude/compare-ui-implementations-d3DJa
Open

Migrate UI from SolidJS to vanilla Web Components#1412
kixelated wants to merge 2 commits into
mainfrom
claude/compare-ui-implementations-d3DJa

Conversation

@kixelated
Copy link
Copy Markdown
Collaborator

Removes the @moq/ui-core package and migrates @moq/watch/ui and @moq/publish/ui from SolidJS to vanilla Web Components using the @moq/signals reactive 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/signals for reactivity and state management, reducing dependencies and simplifying the UI layer.

Key Changes

  • Removed @moq/ui-core package entirely, including:

    • Shared Button and Icon components
    • Stats panel infrastructure and providers (network, video, audio, buffer)
    • Solid.js context and hook utilities
    • Shared CSS variables and flex utilities
  • Migrated @moq/watch/ui from SolidJS to vanilla Web Components:

    • Converted all components (BufferControl, PlayPauseButton, VolumeSlider, QualitySelector, FullscreenButton, StatsButton, WatchStatusIndicator, BufferingIndicator) to TypeScript modules
    • Replaced context-based state management with direct @moq/signals Effect usage
    • Implemented stats panel directly using @moq/signals interval polling instead of provider pattern
    • Added base CSS variables and component styles directly to watch UI
  • Migrated @moq/publish/ui from SolidJS to vanilla Web Components:

    • Converted all components (CameraSourceButton, MicrophoneSourceButton, FileSourceButton, ScreenSourceButton, NothingSourceButton, PublishStatusIndicator) to TypeScript modules
    • Replaced context-based state management with direct element property access
    • Implemented media source selector as reusable utility function
    • Added base CSS variables and component styles directly to publish UI
  • Updated exports and configuration:

    • Changed UI entry points from ./src/ui/index.tsx to ./src/ui/element.ts in both watch and publish packages
    • Removed SolidJS plugin and JSX configuration from vite.config.ts and tsconfig.json
    • Updated CSS imports to use local variables instead of @moq/ui-core
    • Removed @moq/ui-core from moq-boy dependencies and documentation
  • Simplified icon handling:

    • Moved icon SVG imports and utility functions directly into watch and publish UI modules
    • Removed centralized icon component library

Implementation Details

  • Web Components use FinalizationRegistry to ensure Effect cleanup when elements are garbage collected
  • Reactive updates leverage @moq/signals interval polling and signal subscriptions instead of SolidJS reactivity
  • CSS custom properties are defined at the :host level for proper Web Component scoping
  • Component composition uses direct DOM manipulation and event listeners instead of JSX

https://claude.ai/code/session_01MyRFg6YmnMnadiaem63ZYn

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.
@kixelated kixelated marked this pull request as ready for review May 17, 2026 22:51
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

Review Change Stack

Walkthrough

This PR completes a migration from a SolidJS-based UI framework with a shared @moq/ui-core component library to plain Web Components built directly on the @moq/signals reactive foundation. The @moq/watch and @moq/publish packages each gain a new custom element (<moq-watch-ui> and <moq-publish-ui>) that replaces their previous SolidJS wrappers. Documentation is updated throughout to reflect Web Component architecture. The shared @moq/ui-core package is removed entirely, and all UI-related styling and component logic is now co-located within each package. Build configurations are simplified by removing Solid plugin usage.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: migrating UI implementation from SolidJS to vanilla Web Components, which is the core objective of this changeset.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, covering the removal of @moq/ui-core, migration of watch/publish UI components, updated exports/configuration, and implementation details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/compare-ui-implementations-d3DJa
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/compare-ui-implementations-d3DJa

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6511be3 and 3641402.

⛔ Files ignored due to path filters (21)
  • bun.lock is excluded by !**/*.lock
  • js/publish/src/ui/icons/arrow-down.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/arrow-up.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/ban.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/camera.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/file.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/microphone.svg is excluded by !**/*.svg
  • js/publish/src/ui/icons/screen.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/audio.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/buffer.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/fullscreen-enter.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/fullscreen-exit.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/mute.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/network.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/pause.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/play.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/stats.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/video.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/volume-high.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/volume-low.svg is excluded by !**/*.svg
  • js/watch/src/ui/icons/volume-medium.svg is excluded by !**/*.svg
📒 Files selected for processing (96)
  • CLAUDE.md
  • doc/.vitepress/config.ts
  • doc/js/@moq/hang/index.md
  • doc/js/@moq/hang/publish.md
  • doc/js/@moq/hang/watch.md
  • doc/js/@moq/publish.md
  • doc/js/@moq/ui-core.md
  • doc/js/@moq/watch.md
  • doc/js/env/web.md
  • doc/js/index.md
  • js/hang/README.md
  • js/moq-boy/package.json
  • js/moq-boy/src/ui/styles/index.css
  • js/moq-boy/src/ui/styles/variables.css
  • js/moq-boy/vite.config.ts
  • js/publish/README.md
  • js/publish/package.json
  • js/publish/src/ui/components/CameraSourceButton.tsx
  • js/publish/src/ui/components/FileSourceButton.tsx
  • js/publish/src/ui/components/MediaSourceSelector.tsx
  • js/publish/src/ui/components/MicrophoneSourceButton.tsx
  • js/publish/src/ui/components/NothingSourceButton.tsx
  • js/publish/src/ui/components/PublishControls.tsx
  • js/publish/src/ui/components/PublishStatusIndicator.tsx
  • js/publish/src/ui/components/ScreenSourceButton.tsx
  • js/publish/src/ui/components/camera-source-button.ts
  • js/publish/src/ui/components/file-source-button.ts
  • js/publish/src/ui/components/media-source-selector.ts
  • js/publish/src/ui/components/microphone-source-button.ts
  • js/publish/src/ui/components/nothing-source-button.ts
  • js/publish/src/ui/components/publish-status-indicator.ts
  • js/publish/src/ui/components/screen-source-button.ts
  • js/publish/src/ui/context.tsx
  • js/publish/src/ui/element.ts
  • js/publish/src/ui/element.tsx
  • js/publish/src/ui/hooks/use-publish-ui.ts
  • js/publish/src/ui/icons.ts
  • js/publish/src/ui/index.tsx
  • js/publish/src/ui/styles/base.css
  • js/publish/src/ui/styles/index.css
  • js/publish/tsconfig.json
  • js/publish/vite.config.ts
  • js/ui-core/README.md
  • js/ui-core/package.json
  • js/ui-core/src/button/button.css
  • js/ui-core/src/button/button.tsx
  • js/ui-core/src/flex.css
  • js/ui-core/src/icon/icon.tsx
  • js/ui-core/src/index.ts
  • js/ui-core/src/stats/README.md
  • js/ui-core/src/stats/components/StatsItem.tsx
  • js/ui-core/src/stats/components/StatsPanel.tsx
  • js/ui-core/src/stats/index.tsx
  • js/ui-core/src/stats/providers/audio.ts
  • js/ui-core/src/stats/providers/base.ts
  • js/ui-core/src/stats/providers/buffer.ts
  • js/ui-core/src/stats/providers/index.ts
  • js/ui-core/src/stats/providers/network.ts
  • js/ui-core/src/stats/providers/registry.ts
  • js/ui-core/src/stats/providers/video.ts
  • js/ui-core/src/stats/types.ts
  • js/ui-core/src/vite-env.d.ts
  • js/ui-core/tsconfig.json
  • js/ui-core/vite.config.ts
  • js/watch/README.md
  • js/watch/package.json
  • js/watch/src/ui/components/BufferControl.tsx
  • js/watch/src/ui/components/BufferingIndicator.tsx
  • js/watch/src/ui/components/FullscreenButton.tsx
  • js/watch/src/ui/components/PlayPauseButton.tsx
  • js/watch/src/ui/components/QualitySelector.tsx
  • js/watch/src/ui/components/StatsButton.tsx
  • js/watch/src/ui/components/VolumeSlider.tsx
  • js/watch/src/ui/components/WatchControls.tsx
  • js/watch/src/ui/components/WatchStatusIndicator.tsx
  • js/watch/src/ui/components/buffer-control.ts
  • js/watch/src/ui/components/buffering-indicator.ts
  • js/watch/src/ui/components/fullscreen-button.ts
  • js/watch/src/ui/components/play-pause.ts
  • js/watch/src/ui/components/quality-selector.ts
  • js/watch/src/ui/components/stats-button.ts
  • js/watch/src/ui/components/volume-slider.ts
  • js/watch/src/ui/components/watch-status-indicator.ts
  • js/watch/src/ui/context.tsx
  • js/watch/src/ui/element.ts
  • js/watch/src/ui/element.tsx
  • js/watch/src/ui/hooks/use-watch-ui.ts
  • js/watch/src/ui/icons.ts
  • js/watch/src/ui/index.tsx
  • js/watch/src/ui/stats.ts
  • js/watch/src/ui/styles/base.css
  • js/watch/src/ui/styles/index.css
  • js/watch/src/ui/styles/stats.css
  • js/watch/tsconfig.json
  • js/watch/vite.config.ts
  • package.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

Comment thread js/publish/README.md
<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";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 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.

Comment on lines +14 to +31
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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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().

Comment thread js/publish/src/ui/element.ts Outdated
Comment on lines +11 to +17
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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Comment on lines +71 to +86
.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);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
.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.

Comment on lines +21 to +27
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

Comment thread js/watch/src/ui/element.ts Outdated
Comment thread js/watch/src/ui/element.ts Outdated
}
}

customElements.define("moq-watch-ui", MoqWatchUi);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.ts

Repository: 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 -20

Repository: 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 jsx

Repository: 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 -40

Repository: 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 -20

Repository: 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 -30

Repository: 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.ts

Repository: 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 -10

Repository: 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 -40

Repository: 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 -20

Repository: 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.

Suggested change
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.

Comment on lines +57 to +84
.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);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
.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 {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put all of these source buttons in the same file? They're so similar


wrapper.append(toggle, select);

let visible = false;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should be a signal perhaps? We can make a new effect, instead of manually calling render().

Comment thread js/publish/src/ui/element.ts Outdated
});
}

#mount(publish: MoqPublish) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we unmount?

Can you check how the old code used to work, before we ported to solid?

Comment thread js/watch/src/ui/element.ts Outdated

const slot = document.createElement("slot");
videoContainer.append(slot, statsPanel(this.signals, watch, visible), bufferingIndicator(this.signals, watch));
root.appendChild(videoContainer);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
js/publish/src/ui/element.ts (1)

56-62: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3641402 and 022ccbb.

📒 Files selected for processing (5)
  • js/publish/src/ui/components/camera-source-button.ts
  • js/publish/src/ui/components/microphone-source-button.ts
  • js/publish/src/ui/element.ts
  • js/watch/src/ui/element.ts
  • js/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants