Hang UI as a separate package - Issue 695 #717
Conversation
WalkthroughAdds a new TypeScript SolidJS package Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
…-publish exports from compiled sources
js/hang-demo/src/publish.html
Outdated
| <!-- It's optional to provide a video element to preview the outgoing media. --> | ||
| <video style="width: 100%; height: auto; border-radius: 4px; margin: 0 auto;" muted autoplay></video> | ||
| </hang-publish> | ||
| <hang-publish-ui url="%VITE_RELAY_URL%" path="me"></hang-publish-ui> |
There was a problem hiding this comment.
I think we should have:
<hang-publish-ui>
<hang-publish><hang-publish>
<hang-publish-ui>That way we don't need to duplicate all of the attributes, and can make attributes that only apply to the UI (ex. the location of various controls).
Of course it's more annoying to implement but that's web components baybee.
There was a problem hiding this comment.
I was rendering <hang-publish> within the web component for hang-publish-ui because it's easier to get a ref to the hang-publish el, but I see what you're saying about having to duplicate all the attributes. I don't think it'll be an issue to get the ref using query selectors though.
js/hang-ui/.prettierrc
Outdated
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
Give biome a try; it's sooo much better than prettier IMO.
js/hang-ui/package.json
Outdated
| "rollup": "^4.53.3", | ||
| "rollup-plugin-string": "^3.0.0", | ||
| "solid-element": "^1.9.1", | ||
| "solid-js": "^1.9.10", |
There was a problem hiding this comment.
your choice if you use solids. I'd be down with trying out other frameworks
js/moq/package.json
Outdated
| "exports": { | ||
| ".": "./src/index.ts", | ||
| "./zod": "./src/zod.ts" | ||
| ".": "./dist/index.js", |
There was a problem hiding this comment.
Ideally we can avoid this change because it makes local testing (via link) more difficult.
There was a problem hiding this comment.
I was having difficulties getting the types for HangPublish in the UI without this change. Is there another way to do that? Maybe I can avoid using the types, but it'd make things easier to have access to them.
There was a problem hiding this comment.
I got around this requirement by using esbuild to do the typescript handling in Rollup.
|
@kixelated do we want to do the "Meet" UI as well? |
jdreetz
force-pushed
the
package-hang-ui
branch
from
fa5a559 to
8cad818
Compare
…by using esbuild instead of typescript rollup plugin.
jdreetz
force-pushed
the
package-hang-ui
branch
from
8cad818 to
8b23c5b
Compare
jdreetz
force-pushed
the
package-hang-ui
branch
from
1782e8a to
c187923
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
js/hang-ui/src/Components/publish/styles.css (2)
14-21: Add focus-visible styles for keyboard accessibility.The
.publishButtonresets default browser styles including focus outlines. This breaks keyboard navigation accessibility. Add explicit focus styles..publishButton { background: transparent; border: none; cursor: pointer; display: inline-block; font: inherit; color: inherit; } + +.publishButton:focus-visible { + outline: 2px solid currentColor; + outline-offset: 2px; +}
43-48: Missingleft: 50%for horizontal centering.The
transform: translateX(-50%)requiresleft: 50%to center the element. Without it, the element starts at the parent's left edge and shifts left by half its width, causing misalignment..mediaSourceSelector { position: absolute; top: 100%; + left: 50%; transform: translateX(-50%); display: block; }
🧹 Nitpick comments (2)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (2)
70-88: Potential issue ifhangPublishsignal changes before event fires.While
{ once: true }handles cleanup after the event fires, ifprops.hangPublish()reactively changes to a different element before thepublish-instance-availableevent fires on the original element, the old listener remains attached (though it will auto-remove after firing once on the old element).Consider using
onCleanupto explicitly remove the listener when the effect re-runs:+import { createContext, createEffect, createSignal, onCleanup } from "solid-js"; // in createEffect: createEffect(() => { const hangPublishEl = props.hangPublish(); if (!hangPublishEl) return; + const handleInstanceAvailable = (event: CustomEvent) => { + const publishInstance = event.detail.instance.peek?.() as HangPublishInstance; + onPublishInstanceAvailable(hangPublishEl, publishInstance); + }; + if (!hangPublishEl.active) { // @ts-expect-error ignore custom event - todo add event map - hangPublishEl.addEventListener( - "publish-instance-available", - (event: CustomEvent) => { - const publishInstance = event.detail.instance.peek?.() as HangPublishInstance; - onPublishInstanceAvailable(hangPublishEl, publishInstance); - }, - { once: true }, - ); + hangPublishEl.addEventListener("publish-instance-available", handleInstanceAvailable, { once: true }); + onCleanup(() => { + // @ts-expect-error ignore custom event + hangPublishEl.removeEventListener("publish-instance-available", handleInstanceAvailable); + }); } else {
44-53: DualsetFileActivecalls may cause UI flicker.
setFileActive(true)is called insetFile(line 52), andtrackFileActiveeffect (lines 186-189) also updatesfileActivebased onel.signals.source. WhensetFileis called, line 52 sets it totrue, then immediately after, if the source signal updates,trackFileActivewill also fire and potentially set the same value.This is likely harmless (SolidJS dedupes identical signal values), but the explicit
setFileActive(true)insetFilemay be redundant sincetrackFileActivewill set it correctly whensourcebecomes"file".Consider removing the explicit call in
setFileto rely solely on the reactive effect:const setFile = (file: File) => { const hangPublishEl = props.hangPublish(); if (!hangPublishEl) return; hangPublishEl.file = file; hangPublishEl.source = "file"; hangPublishEl.video = true; hangPublishEl.audio = true; - setFileActive(true); };Also applies to: 186-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
README.md(1 hunks)js/hang-ui/src/Components/publish/FileSourceButton.tsx(1 hunks)js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/publish/element.tsx(1 hunks)js/hang-ui/src/Components/publish/styles.css(1 hunks)js/hang-ui/src/Components/watch/VolumeSlider.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/element.tsx(1 hunks)js/hang-ui/src/Components/watch/styles.css(1 hunks)js/hang-ui/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- js/hang-ui/tsconfig.json
- js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
- js/hang-ui/src/Components/watch/styles.css
- README.md
- js/hang-ui/src/Components/watch/element.tsx
- js/hang-ui/src/Components/watch/VolumeSlider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/publish/FileSourceButton.tsxjs/hang-ui/src/Components/publish/element.tsxjs/hang-ui/src/Components/publish/PublishUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/publish/FileSourceButton.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/publish/FileSourceButton.tsx (1)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
PublishUIContext(28-28)
🔇 Additional comments (3)
js/hang-ui/src/Components/publish/element.tsx (1)
11-26: LGTM!The race condition handling is well implemented. The code first attempts to find an existing
hang-publishelement, and if not found, sets up a one-time event listener with{ once: true }which automatically removes itself after firing. This correctly handles both scenarios where the child element is already present or connects later.js/hang-ui/src/Components/publish/FileSourceButton.tsx (1)
4-16: LGTM!The file input pattern is correctly implemented. The component properly:
- Uses a ref signal to access the hidden input
- Triggers the file dialog via the visible button
- Handles file selection with appropriate validation
- Resets the input value to allow re-selecting the same file
- Uses optional chaining on context for safety
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
92-189: The suggested solution uses the wrong library and will not work in this codebase.The review correctly identifies that
publishInstance.signals.effect()calls return disposal functions that must be captured and called to prevent memory leaks. However, the suggested diff incorrectly imports from Solid.js (createEffect,createContext,onCleanup,createSignal), when the code uses the@kixelated/signalslibrary.The correct approach is to:
- Capture the dispose functions returned by
signals.effect():const dispose1 = publishInstance?.signals.effect(...)- Store them in an array
- Call each dispose function in a cleanup callback (the mechanism depends on whether the component itself uses Solid.js or another framework)
The concern about memory leaks from undisposed effects is valid, but the specific implementation provided will not resolve it for this codebase.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx (2)
12-18: Simplify toggle handler with functional updaterYou can simplify
toggleSourcesVisibleto avoid the explicitif/elseand rely on the previous value:- const toggleSourcesVisible = () => { - if (sourcesVisible()) { - setSourcesVisible(false); - } else { - setSourcesVisible(true); - } - }; + const toggleSourcesVisible = () => { + setSourcesVisible((visible) => !visible); + };
22-39: Minor UX/a11y nits on toggle button and selectTwo small polish points:
- The constant title
"Show Sources"and the ▼/▲ indicators are slightly inverted from common patterns (▼ usually means “expand”; ▲ “collapse”). Consider flipping the symbols or updating the title dynamically ("Show Sources"/"Hide Sources").- The
<select>lacks an accessible name here. If it isn’t wrapped by a<label>elsewhere, addaria-labelor an explicit label element so screen readers can identify what this selector controls.These are non-blocking but would improve clarity and accessibility.
js/hang-ui/src/Components/watch/element.tsx (1)
27-29: Consider tracking listener registration for targeted cleanup.The cleanup unconditionally removes the event listener even when it was only added in the
elsebranch (line 23). WhileremoveEventListeneron a never-added listener is a safe no-op, you could optimize this by tracking whether the listener was registered.Apply this diff to optimize cleanup:
customElement("hang-watch-ui", {}, function WatchUIWebComponent(_, { element }) { const [hangWatchEl, setHangWatchEl] = createSignal<HangWatch>(); + let listenerAdded = false; const onInstanceAvailable = (event: CustomEvent) => { const hangWatchEl = event.target as HangWatch; setHangWatchEl(hangWatchEl); }; onMount(() => { const watchEl = element.querySelector("hang-watch"); if (watchEl) { setHangWatchEl(watchEl); } else { element.addEventListener("watch-instance-available", onInstanceAvailable); + listenerAdded = true; } }); onCleanup(() => { - element.removeEventListener("watch-instance-available", onInstanceAvailable); + if (listenerAdded) { + element.removeEventListener("watch-instance-available", onInstanceAvailable); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx(1 hunks)js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/publish/element.tsx(1 hunks)js/hang-ui/src/Components/publish/styles.css(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/element.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- js/hang-ui/src/Components/publish/styles.css
- js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
- js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx
- js/hang-ui/src/Components/publish/element.tsx
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/element.tsxjs/hang-ui/src/Components/publish/MediaSourceSelector.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/element.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/element.tsx (5)
js/hang/src/watch/element.ts (1)
HangWatch(23-181)js/signals/src/index.ts (1)
event(489-504)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
WatchUIContextProvider(27-137)js/hang-ui/src/Components/watch/BufferingIndicator.tsx (1)
BufferingIndicator(4-14)js/hang-ui/src/Components/watch/WatchControls.tsx (1)
WatchControls(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (4)
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx (1)
1-43: Component structure and typings look solidTyping of props, SolidJS usage (
createSignal,Show,For), and theonSelectedwiring are all correct and straightforward. Behavior forundefinedsourcesis safe, and the optional callback is handled properly.js/hang-ui/src/Components/watch/element.tsx (3)
12-15: LGTM!The event handler correctly captures the HangWatch element from
event.targetand updates the signal. The WatchUIContextProvider will later access the instance via the element'sactiveproperty.
31-40: LGTM!The render structure is well-organized: the WatchUIContextProvider wraps all UI components to expose watch state and controls, the slot allows the child
<hang-watch>element to be inserted, and the BufferingIndicator and WatchControls are properly positioned.
17-25: The referenced filejs/hang/src/watch/element.tsdoes not exist in the repository, and thewatch-instance-availableevent dispatch is not found anywhere in the codebase. The verification cannot be completed as described because the complementary code being referenced is not present. Consider verifying that the HangWatch component implementation exists and implements the expected event dispatch behavior before addressing this concern.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
js/hang-ui/src/Components/watch/LatencySlider.tsx (1)
27-27: Apply the latencyValueDisplay class and avoid redundant calls.The display span doesn't use the
.latencyValueDisplayclass defined in styles.css, andcontext?.latency()is called twice (inefficient).Apply this diff:
-<span>{typeof context?.latency() !== "undefined" ? `${Math.round(context?.latency())}ms` : ""}</span> +<span class="latencyValueDisplay">{typeof context?.latency() !== "undefined" ? `${Math.round(context.latency())}ms` : ""}</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
js/hang-demo/src/index.html(1 hunks)js/hang-ui/src/Components/publish/MediaSourceSelector.tsx(1 hunks)js/hang-ui/src/Components/watch/LatencySlider.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchControls.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/styles.css(1 hunks)js/hang/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- js/hang-demo/src/index.html
- js/hang-ui/src/Components/publish/MediaSourceSelector.tsx
- js/hang-ui/src/Components/watch/WatchControls.tsx
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang/src/index.tsjs/hang-ui/src/Components/watch/LatencySlider.tsxjs/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/styles.css
🧬 Code graph analysis (2)
js/hang-ui/src/Components/watch/LatencySlider.tsx (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
WatchUIContext(28-28)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
js/hang/src/watch/element.ts (10)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (2)
js/hang/src/index.ts (1)
7-7: Time namespace re-export is consistent with existing barrel patternAdding
export * as Time from "./time";matches the existing namespace-style exports (Moq, Catalog, Frame, Publish, Support, Watch) and cleanly exposes the time utilities as part of the public hang surface. As long as./timeis intended to be stable/public API, this looks good to ship.js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
86-106: LGTM! Event listener cleanup implemented.The event listener is now properly removed using
onCleanup, which addresses the memory leak concern from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
32-36: Avoid desync betweenisMutedsignal and the underlying Hang audio stateRight now
isMutedis managed as local Solid state and only updated intoggleMuted, whilecurrentVolumeis driven fromwatchInstance.audio.volume. This can cause drift:
toggleMuted(Lines 54-62) flipsisMuted()even whenprops.hangWatch()isundefined, so the UI may display “muted” with no underlying element.- External changes to
hangWatchEl.mutedor the underlying audio emitter won’t be reflected back intoisMuted.To align with how volume is handled, consider:
- Drive
isMutedfrom the instance, similar totrackVolume:@@ function onWatchInstanceAvailable(watchEl: HangWatch, watchInstance: HangWatchInstance) { watchInstance?.signals.effect(function trackVolume(effect) { const volume = effect.get(watchInstance?.audio.volume); setCurrentVolume(volume * 100); }); + + watchInstance?.signals.effect(function trackMuted(effect) { + const muted = effect.get(watchInstance?.audio.muted); + setIsMuted(muted); + });
- Make
toggleMutedonly toggle the element and rely on the effect above to update the context:- const toggleMuted = () => { - setIsMuted(!isMuted()); - - const hangWatchEl = props.hangWatch(); - - if (hangWatchEl) { - hangWatchEl.muted = isMuted(); - } - }; + const toggleMuted = () => { + const hangWatchEl = props.hangWatch(); + if (!hangWatchEl) return; + + hangWatchEl.muted = !hangWatchEl.muted; + };This keeps the UI state in sync with the actual audio pipeline and avoids misleading toggles when the watch element isn’t available yet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang-ui/src/Components/watch/styles.css
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
js/hang/src/watch/element.ts (10)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
110-155: Signal wiring fromHangWatchInstanceinto context looks solidThe effects for status, playing, volume, buffering, and latency correctly subscribe to the corresponding
HangWatchInstance/HangWatchsignals and project them into Solid signals, matching the shapes exposed byWatchUIContextValues. No issues stand out here.
|
@christriants updated this branch to include the Latency Slider as a component. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/hang-ui/src/Components/watch/LatencySlider.tsx (1)
10-14: Consider usingonInputfor real-time updates.The
onChangeevent fires only when the user releases the slider. For a latency control, usingonInputinstead would provide continuous updates as the user drags, offering better real-time feedback.Apply this diff to enable real-time updates:
const onInputChange = (event: Event) => { const target = event.currentTarget as HTMLInputElement; const latency = parseFloat(target.value); context?.setLatencyValue(latency); }; return ( <div class="latencySliderContainer"> <span class="latencyLabel">Latency: </span> <input - onChange={onInputChange} + onInput={onInputChange} class="latencySlider" type="range" value={context?.latency()} min={MIN_RANGE} max={MAX_RANGE} step={RANGE_STEP} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/hang-ui/src/Components/watch/LatencySlider.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/LatencySlider.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/LatencySlider.tsx (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
WatchUIContext(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
86-106: Instance binding logic and event listener cleanup look correct nowThe
createEffectcorrectly:
- Reads the current
HangWatchelement fromprops.hangWatch().- Uses
active.peek()to decide between immediate binding vs. waiting for"watch-instance-available".- Registers the event listener only when needed and removes it in
onCleanup.This addresses the earlier concerns about dead code (
if (!hangWatchEl.active)) and potential listener leaks.
🧹 Nitpick comments (4)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (2)
44-52: Consider validating file type and handling errors.The
setFilefunction assumes the provided file is valid and supported. Adding file type validation and error handling would improve robustness, especially if the underlying API rejects certain file types.
164-185: Consider explicitly handling all connection status values.The status derivation logic implicitly assumes that if
statusis neither "disconnected" nor "connecting", the connection is established. Making this explicit would improve code clarity and prevent subtle bugs if new status values are added.Consider adding an explicit check:
} else if (status === "connecting") { setPublishStatus("connecting"); + } else if (status !== "connected") { + // Handle unexpected status values + setPublishStatus("disconnected"); } else if (!audio && !video) {js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (2)
32-62: DeriveisMutedfrom the HangWatch instance instead of maintaining local-only mute stateRight now
isMutedis only updated whentoggleMutedruns; if something else toggleshangWatch.muted(e.g., another UI or attribute change), the context gets out of sync. Since other fields (isPlaying,currentVolume) are driven fromwatchInstancesignals, it would be more consistent to do the same for mute and lettoggleMutedjust flip the element property.Suggested refactor:
- const [isMuted, setIsMuted] = createSignal<boolean>(false); + const [isMuted, setIsMuted] = createSignal<boolean>(false); @@ - const toggleMuted = () => { - setIsMuted(!isMuted()); - - const hangWatchEl = props.hangWatch(); - - if (hangWatchEl) { - hangWatchEl.muted = isMuted(); - } - }; + const toggleMuted = () => { + const hangWatchEl = props.hangWatch(); + + if (hangWatchEl) { + hangWatchEl.muted = !hangWatchEl.muted; + } + }; @@ - watchInstance?.signals.effect(function trackVolume(effect) { + watchInstance?.signals.effect(function trackVolume(effect) { const volume = effect.get(watchInstance?.audio.volume); setCurrentVolume(volume * 100); }); + + watchInstance?.signals.effect(function trackMuted(effect) { + const muted = effect.get(watchInstance?.audio.muted); + setIsMuted(muted); + });This keeps the context as a pure view of the underlying instance state and avoids divergence.
Also applies to: 138-141, 151-154
46-52: Optionally clamp volume to [0, 1] when mapping from 0–100 UI range
setVolumedivides by 100 andtrackVolumemultiplies by 100, which is symmetric, but if a caller passes values outside 0–100 you can end up feeding >1 or <0 intohangWatchEl.volume.A small defensive tweak would make this more robust:
- const setVolume = (volume: number) => { + const setVolume = (volume: number) => { const hangWatchEl = props.hangWatch(); if (hangWatchEl) { - hangWatchEl.volume = volume / 100; + const normalized = Math.min(1, Math.max(0, volume / 100)); + hangWatchEl.volume = normalized; } };Not critical, but it hardens the API against bad inputs.
Also applies to: 138-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/LatencySlider.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang-ui/src/Components/watch/LatencySlider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsxjs/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
js/hang/src/watch/element.ts (10)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (3)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
93-191: Effect subscriptions require verification of cleanup implementation.The nine
publishInstance.signals.effect()calls (trackCameraDevices, trackMicrophoneDevices, trackNothingSourceActive, trackMicrophoneSourceActive, trackVideoSourcesActive, trackSelectedCameraSource, trackSelectedMicrophoneSource, trackPublishStatus, trackFileActive) may not be properly disposed. Verify whether:
effect()returns a dispose/cleanup function- These cleanup functions are collected and invoked in an
onCleanupcallback- This cleanup is already implemented elsewhere in the component
If effects are not being disposed, this will cause memory leaks and stale state updates when the component unmounts or
hangPublishElchanges.js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (2)
7-28: Context types and value shape look consistent and sufficient
WatchUIContextProviderProps,WatchStatus, andWatchUIContextValuesline up well with the HangWatch/HangWatchInstance API and the controls you expose; nothing blocking here.
110-155: Signal effects for status/playback/volume/buffering/latency are wired coherentlyThe
onWatchInstanceAvailableeffects correctly derive:
watchStatusfrom connection URL/status and broadcast status.isPlayingfromvideo.paused.currentVolumefromaudio.volumewith the expected 0–100 mapping.bufferingfromsyncStatus/bufferStatus(matching the existing buffering overlay behavior).latencyfromwatchEl.signals.latency.This is a solid integration of the HangWatch signals into the UI context; aside from the separate mute handling already noted, there are no functional issues here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
86-86: Inconsistent optional chaining onactiveproperty.Line 78 uses
hangPublishEl?.active?.peek()with optional chaining onactive, but line 86 useshangPublishEl.active.peek?.()without optional chaining onactive. While technically safe due to the control flow (the else branch only executes whenactive.peek()is truthy), this inconsistency could cause confusion or become error-prone during refactoring.Apply this diff for consistency:
} else { - const publishInstance = hangPublishEl.active.peek?.() as HangPublishInstance; + const publishInstance = hangPublishEl.active?.peek?.() as HangPublishInstance; onPublishInstanceAvailable(hangPublishEl, publishInstance); }
🧹 Nitpick comments (1)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
44-52: Consider potential intermediate states when setting multiple properties.The function sets four properties sequentially on
hangPublishEl. While this likely works correctly given synchronous setters, intermediate states could theoretically cause issues if any internal reactions observe partial updates.If the hang library supports batching or atomic updates, consider using that pattern:
const setFile = (file: File) => { const hangPublishEl = props.hangPublish(); if (!hangPublishEl) return; // If hang supports batching, wrap in a batch operation: // batch(() => { hangPublishEl.file = file; hangPublishEl.source = "file"; hangPublishEl.video = true; hangPublishEl.audio = true; // }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsxjs/hang-ui/src/Components/publish/PublishUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧬 Code graph analysis (2)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
js/hang/src/watch/element.ts (10)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (2)
js/hang/src/publish/source/file.ts (2)
File(9-126)setFile(35-37)js/hang/src/publish/element.ts (1)
HangPublishInstance(178-339)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (4)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx (1)
93-191: The nested effects registered withinonPublishInstanceAvailableare automatically cleaned up by Solid.js when the outercreateEffectreruns. In Solid.js, effects created within a reactive scope are part of that scope's lifecycle—when the scope reruns, previous inner effects are disposed before new ones are created. No manual cleanup is required for this nested effect pattern.Likely an incorrect or invalid review comment.
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (3)
86-106: LGTM: Instance binding logic correctly handles both immediate and event-driven binding.The conditional check on Line 95 properly examines
active.peek()(the signal's value) rather than the signal itself, ensuring the event listener is registered only when no instance exists yet. The cleanup logic correctly removes the listener when the effect re-runs or the component unmounts.
151-154: LGTM: Latency tracking is correctly implemented.The
trackLatencyeffect properly subscribes to the latency signal and updates the UI state. The previously reported typo has been fixed.
143-149: Optional chaining is unnecessary here.The
syncStatusandbufferStatusvalues returned byeffect.get()are guaranteed to be defined. These values come from Signal instances that are always initialized in the Source class and maintain their.stateproperty. Adding optional chaining would be defensive programming without practical benefit.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
38-60: Optionally clamp volume and latency inputs before writing to HangWatch.Right now
setVolumeandsetLatencyValuetrust callers to pass sane ranges; a stray value (e.g., >100 or negative) would propagate directly into the Hang signals. Consider clamping to defensive ranges (0–100 for volume slider percentage, and>= 0for latency) to make the context safer to use from any UI component.- const setVolume = (volume: number) => { - const hangWatchEl = props.hangWatch(); - - if (hangWatchEl) { - hangWatchEl.volume = volume / 100; - } - }; + const setVolume = (volume: number) => { + const hangWatchEl = props.hangWatch(); + if (!hangWatchEl) return; + + const clamped = Math.min(100, Math.max(0, volume)); + hangWatchEl.volume = clamped / 100; + }; @@ - const setLatencyValue = (latency: number) => { - const hangWatchEl = props.hangWatch(); - - if (hangWatchEl) { - hangWatchEl.signals.latency.set(latency as Time.Milli); - } - }; + const setLatencyValue = (latency: number) => { + const hangWatchEl = props.hangWatch(); + if (!hangWatchEl) return; + + const clamped = Math.max(0, latency); + hangWatchEl.signals.latency.set(clamped as Time.Milli); + };Please double‑check expected ranges for
volumeandlatencyagainst@kixelated/hang’s current API before adopting this.Also applies to: 62-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
js/hang/src/watch/element.ts (12)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)muted(150-152)muted(154-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (3)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (3)
7-37: Context shape and signal wiring look coherent.Props,
WatchStatus,WatchUIContextValues, and the provider’s internal signals line up cleanly with the exposed context API; the provider value matches the declared types and keeps UI state localized while delegating actual media control to the element/instance. No changes needed here.Also applies to: 70-82, 106-106
84-104: Instance binding and listener cleanup correctly handle eager and deferred availability.Using
hangWatchEl.active.peek()to decide between immediate binding and subscribing to"watch-instance-available", plusonCleanupto remove the listener, addresses the earlier leak/race concerns and should work both when an instance already exists and when it appears later. This looks good; just ensure the demos still behave as expected when swapping/re‑creating<hang-watch>elements.
108-158: Effects for status, playback, volume, mute, buffering, and latency are well-aligned with HangWatchInstance signals.The
onWatchInstanceAvailablehelpers consistently derive UI state fromconnection,broadcast,video,audio, andlatencysignals, and push into the Solid signals backing the context. The watch-status mapping covers the documented connection/broadcast states, and buffering/latency tracking aligns with the underlying element signals. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (2)
62-68: Use element property for consistency with other setters.The other action functions (
togglePlayback,setVolume,toggleMuted) consistently use element properties (paused,volume,muted), butsetLatencyValuedirectly accessessignals.latency.set(). SinceHangWatchhas alatencyproperty setter, using it would maintain consistency.Apply this diff:
const setLatencyValue = (latency: number) => { const hangWatchEl = props.hangWatch(); if (hangWatchEl) { - hangWatchEl.signals.latency.set(latency as Time.Milli); + hangWatchEl.latency = latency; } };
93-93: Type assertion could be more precise.The type assertion
as HangWatchInstanceis misleading becausepeek()returnsHangWatchInstance | undefined. While the code is safe due to the subsequentif (hangWatchInstance)check, removing the type assertion or usingas HangWatchInstance | undefinedwould be more accurate.Apply this diff:
- const hangWatchInstance = hangWatchEl?.active?.peek?.() as HangWatchInstance; + const hangWatchInstance = hangWatchEl?.active?.peek?.();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang-ui/src/Components/publish/PublishUIContextProvider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
js/**/*.{js,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use bun workspaces configured in
js/package.jsonfor JavaScript/TypeScript development
Files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-22T15:03:15.091Z
Learnt from: pzanella
Repo: kixelated/moq PR: 714
File: js/hang/src/watch/element.ts:469-469
Timestamp: 2025-11-22T15:03:15.091Z
Learning: In js/hang/src/watch/element.ts, the buffering overlay intentionally uses `pointerEvents: "auto"` to block clicks and prevent accidental interactions with underlying controls during buffering.
Applied to files:
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
🧬 Code graph analysis (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (2)
js/hang/src/watch/element.ts (12)
HangWatch(23-181)latency(174-176)latency(178-180)volume(142-144)volume(146-148)HangWatchInstance(185-300)url(109-111)url(113-115)paused(134-136)paused(138-140)muted(150-152)muted(154-156)js/signals/src/index.ts (1)
event(489-504)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
109-159: Well-structured effect tracking with automatic cleanup.The
onWatchInstanceAvailablefunction correctly sets up reactive effects to track all relevant watch state (status, playback, volume, muting, buffering, latency). The effects are automatically cleaned up when theHangWatchInstanceis closed (viasignals.close()), which happens when the element disconnects from the DOM.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Oh and we could roll this back into @kixelated/hang if we migrate that package over to rollup as well.
| }, | ||
| resolve: { | ||
| alias: { | ||
| "@kixelated/hang-ui/publish/element": path.resolve( |
There was a problem hiding this comment.
kinda gross, but harmless I guess. You should leave a comment that this isn't needed when using the package from NPM.
This wasn't needed when package.json pointed to src instead of dist.
| }, | ||
| "sideEffects": true, | ||
| "scripts": { | ||
| "build": "npm run clean && rollup -c", |
There was a problem hiding this comment.
And yeah, the other packages use a script to rewrite package.json to change src to dist for publishing. Your alias approach works too.
|
|
||
| function inlineCss() { | ||
| return { | ||
| name: 'inline-css', |
There was a problem hiding this comment.
We might want to do this anyway for @kixelated/hang because of worklets. I left some thoughts in #677
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "^2.2.2", | ||
| "@kixelated/hang": "workspace:^0.7.0", |
There was a problem hiding this comment.
Should this be a peerDependency?
| return <PublishUIContext.Provider value={value}>{props.children}</PublishUIContext.Provider>; | ||
|
|
||
| function onPublishInstanceAvailable(el: HangPublish, publishInstance: HangPublishInstance) { | ||
| publishInstance?.signals.effect(function trackCameraDevices(effect) { |
There was a problem hiding this comment.
you should remove the ? from all of these, it can't be null.
| const [hangPublishEl, setHangPublishEl] = createSignal<HangPublish>(); | ||
|
|
||
| const onInstanceAvailable = (event: CustomEvent) => { | ||
| const hangPublishEl = event.target as HangPublish; |
There was a problem hiding this comment.
Not a fan of casting, the event should be typed.
| if (publishEl) { | ||
| setHangPublishEl(publishEl); | ||
| } else { | ||
| element.addEventListener("publish-instance-available", onInstanceAvailable); |
There was a problem hiding this comment.
You can use the signal instead if you want.
const dispose = element.active.watch((instance?: PublishInstance) => {
});
onCleanup(dispose);There was a problem hiding this comment.
Also we might be able to avoid the instance being undefined. The issue is the web component lifetime; we need to start work when inserted into the DOM and stop work when removed. Toggling enabled instead might do the trick.
2 participants
What does it change?
This PR addresses #695. It migrates the Watch and Publish UIs to a separate
hang-uipackage, that provides two Web Components (hang-publish-uiandhang-watch-ui). Internally, the Web Components use SolidJS to render and interact with the elements.Web Components
The Web Components expect to wrap their respective
hangWeb Components (hang-publishforhang-publish-uiandhang-watchforhang-watch-ui). In order to use the existing Signal reactivity, the UI components will wait until they have a reference to the respectiveHang...Instance(published as aCustomEvent:watch-instance-available,publish-instance-available), that provides information about the state of the watch/publish. To interact with the Signals on the Instances, we had to make some of their properties public:video,audio,signals,file.Context Provider
Once the Instance properties are ready, we use a SolidJS Context to expose the Instance properties to the UI components:
WatchUIContextProvider.tsxPublishUIContextProvider.tsxI'm open to using the Instance Signals directly in the components, but we still need something that waits for the Instance to become in the wrapped component (for some reason it's not available immediately) before the UI components can use them. Personally, I think it's helpful to keep the binding layers (Context) separate from the UI component implementation, as it reduces the noise in the components and allows the developer to focus on the markup and interactions, but I'm open to other design ideas.
How to test
nix develop -c just dev) to start your local devmainmain