Improve the rest of the web components lifecycle. #582
Conversation
Spin up and spin down based on connected/disconnected callback.
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/hang/src/meet/room.ts (1)
98-111: Don’t self-close the Room on effect cancellation; it prevents re-init on URL/name changes
finally { this.close(); }will close#signals, killing reactivity when the effect cancels (e.g., URL/name change). Result: the room never restarts. Clear broadcasts without closing the effect.async #runRemotes(announced: Moq.AnnouncedConsumer, cancel: Promise<void>) { try { for (;;) { const update = await Promise.race([announced.next(), cancel]); // We're donezo. if (!update) break; this.#handleUpdate(update); } } finally { - this.close(); + // Clear current state but keep the Room alive for the next init run. + this.#clearBroadcasts(); } } -close() { - this.#signals.close(); - - // Swap out the maps so they're empty when the callbacks run. - const remotes = this.remotes; - const active = this.active; - const locals = this.locals; - - this.remotes = new Map(); - this.active = new Map(); - this.locals = new Map(); - - // Clear all remote/active broadcasts when there are no more announcements. - for (const [name, broadcast] of remotes) { - broadcast.close(); - this.#onRemote?.(name, undefined); - } - - for (const name of locals.keys()) { - this.#onLocal?.(name, undefined); - } - - for (const name of active.keys()) { - this.#onActive?.(name, undefined); - } -} +close() { + // Fully shut down: stop reactivity, then clear state. + this.#signals.close(); + this.#clearBroadcasts(); +} + +// Clear broadcasts and notify callbacks, but keep the Room reactive. + #clearBroadcasts() { + const remotes = this.remotes; + const active = this.active; + const locals = this.locals; + + this.remotes = new Map(); + this.active = new Map(); + this.locals = new Map(); + + for (const [name, broadcast] of remotes) { + broadcast.close(); + this.#onRemote?.(name, undefined); + } + for (const name of locals.keys()) { + this.#onLocal?.(name, undefined); + } + for (const name of active.keys()) { + this.#onActive?.(name, undefined); + } +}Also applies to: 156-181
js/hang/src/publish/element.ts (1)
54-79: Attribute parsing edge cases: invalid URL and error message wording.
- Parsing
urlcan throw; guard it to avoid taking down the component.- For
name === "source", the error reads “Invalid device”; use the actual attribute name in the message.if (name === "url") { - this.url = newValue ? new URL(newValue) : undefined; + if (newValue === null) { + this.url = undefined; + } else { + try { + this.url = new URL(newValue); + } catch { + console.warn("hang-publish: invalid url attribute:", newValue); + this.url = undefined; + } + } } else if (name === "name") { this.name = newValue ?? undefined; } else if (name === "device" || name === "source") { if (newValue === "camera" || newValue === "screen" || newValue === null) { this.source = newValue ?? undefined; } else { - throw new Error(`Invalid device: ${newValue}`); + throw new Error(`Invalid ${name}: ${newValue}`); }
🧹 Nitpick comments (18)
js/hang/src/support/element.ts (2)
26-34: Reset the “close” state on reconnect to avoid a permanently hidden UIAfter clicking Close,
#closestays true across disconnections; reconnecting won’t render anything. Reset it inconnectedCallback()so visibility is per-attachment, not permanent.connectedCallback() { + // Reset visibility each time the element is attached. + this.#close.set(false); this.#signals = new Effect(); this.#signals.effect(this.#render.bind(this)); }
26-34: Make lifecycle idempotent (guard double connect/disconnect)
Browsers should call these in pairs, but defensive guards prevent accidental double wiring in app code.connectedCallback() { - this.#signals = new Effect(); + if (this.#signals) return; // already connected + this.#signals = new Effect(); this.#signals.effect(this.#render.bind(this)); } disconnectedCallback() { - this.#signals?.close(); - this.#signals = undefined; + if (!this.#signals) return; + this.#signals.close(); + this.#signals = undefined; }js/hang/src/meet/room.ts (1)
145-146: Be tolerant to out-of-order updatesThrowing when a broadcast isn’t found on a “remove” can crash on races (announce/remove crossing). Prefer a warn-and-ignore.
- const existing = this.remotes.get(update.name); - if (!existing) throw new Error(`broadcast not found: ${update.name}`); + const existing = this.remotes.get(update.name); + if (!existing) { + console.warn(`broadcast not found on remove: ${update.name}`); + return; + }js/hang/src/meet/element.ts (2)
30-41: Make lifecycle idempotent instead of throwingThrowing on double connect/disconnect is brittle in host apps. A no-op guard is friendlier and avoids hard failures.
connectedCallback() { - if (this.active.peek()) throw new Error("connectedCallback called twice"); - this.active.set(new HangMeetInstance(this)); + if (!this.active.peek()) { + this.active.set(new HangMeetInstance(this)); + } } disconnectedCallback() { - if (!this.active.peek()) throw new Error("disconnectedCallback called without connectedCallback"); - this.active.set((prev) => { - prev?.close(); - return undefined; - }); + this.active.set((prev) => { + prev?.close(); + return undefined; + }); }
113-121: MutationObserver reloads: limit scope to reduce spurious rerunsReloading on any child change can be chatty. Narrow to
subtree: false, childList: trueand filter tohang-publishchanges if needed.- const observer = new MutationObserver(() => effect.reload()); - observer.observe(this.parent, { childList: true }); + const observer = new MutationObserver((mutations) => { + if (mutations.some(m => Array.from(m.addedNodes).concat(Array.from(m.removedNodes)) + .some(n => n instanceof Element && n.tagName.toLowerCase() === "hang-publish"))) { + effect.reload(); + } + }); + observer.observe(this.parent, { childList: true });js/hang/src/publish/element.ts (6)
41-44: Throwing on doubleconnectedCallbackis harsh; prefer idempotence.Repeated connects can happen with DOM moves. Consider no-op if already active instead of throwing.
connectedCallback() { - if (this.active.peek()) throw new Error("connectedCallback called twice"); - this.active.set(new HangPublishInstance(this)); + if (!this.active.peek()) { + this.active.set(new HangPublishInstance(this)); + } }
46-52: Same fordisconnectedCallback: prefer safe no-op.disconnectedCallback() { - if (!this.active.peek()) throw new Error("disconnectedCallback called without connectedCallback"); - this.active.set((prev) => { - prev?.close(); - return undefined; - }); + this.active.set((prev) => { + prev?.close(); + return undefined; + }); }
97-105: Alias property is fine, but document deprecation.Add a deprecation JSDoc on
devicepointing devs tosource. Also reconcile with the earlier suggestion to dropsignals.device.
190-207: Preview effect: minor robustness.Consider setting
preview.playsInline = truethe first time to avoid autoplay quirks on iOS.- preview.srcObject = new MediaStream([media]); + preview.playsInline = true; + preview.srcObject = new MediaStream([media]);
268-351: UI controls: consistent use ofeffect.eventis good; device selectors defer until enabled.Overall LGTM. One nit: caret text flips “▼/▲” opposite typical conventions; if you care, swap them.
- caret.innerText = v ? "▼" : "▲"; + caret.innerText = v ? "▲" : "▼";Also applies to: 353-438, 440-525, 527-548, 550-571, 573-601
603-607: Teardown order differs from HangWatch.You close signals first here but last in HangWatch. Pick one order across components to avoid surprises; closing signals first is fine (ensures temporary sets clean), but keep it consistent.
js/hang/src/watch/element.ts (7)
56-64: Idempotent lifecycle: avoid throwing inconnectedCallback/disconnectedCallback.Mirror the pattern suggested for HangPublish so DOM moves don’t throw.
connectedCallback() { - if (this.active.peek()) throw new Error("connectedCallback called twice"); - this.active.set(new HangWatchInstance(this)); + if (!this.active.peek()) this.active.set(new HangWatchInstance(this)); } disconnectedCallback() { - if (!this.active.peek()) throw new Error("disconnectedCallback called without a connectedCallback"); - this.active.set((prev) => { - prev?.close(); - return undefined; - }); + this.active.set((prev) => { + prev?.close(); + return undefined; + }); }Also applies to: 66-72
222-271: Attribute reflection is partial — add ‘captions’ and ‘reload’ for symmetry.This keeps attributes and signals in sync like the others.
this.#signals.effect((effect) => { const controls = effect.get(this.parent.signals.controls); if (controls) { this.parent.setAttribute("controls", ""); } else { this.parent.removeAttribute("controls"); } }); + +this.#signals.effect((effect) => { + const captions = effect.get(this.parent.signals.captions); + if (captions) { + this.parent.setAttribute("captions", ""); + } else { + this.parent.removeAttribute("captions"); + } +}); + +this.#signals.effect((effect) => { + const reload = effect.get(this.parent.signals.reload); + if (reload) { + this.parent.setAttribute("reload", ""); + } else { + this.parent.removeAttribute("reload"); + } +});Also consider using boolean presence for
pausedinstead of"true"for consistency.
346-363: Useeffect.eventfor auto-cleanup of listeners.- button.addEventListener("click", (e) => { - e.preventDefault(); - this.video.paused.set((prev) => !prev); - }); + effect.event(button, "click", (e) => { + e.preventDefault(); + this.video.paused.set((prev) => !prev); + });
374-394: Same: prefereffect.eventfor mute/volume handlers.- muteButton.addEventListener("click", () => { - this.audio.muted.set((p) => !p); - }); + effect.event(muteButton, "click", () => { + this.audio.muted.set((p) => !p); + }); - volumeSlider.addEventListener("input", (e) => { - const target = e.currentTarget as HTMLInputElement; - const volume = parseFloat(target.value) / 100; - this.audio.volume.set(volume); - }); + effect.event(volumeSlider, "input", (e) => { + const target = e.currentTarget as HTMLInputElement; + const volume = parseFloat(target.value) / 100; + this.audio.volume.set(volume); + });
446-465: And for fullscreen toggle, useeffect.event.- button.addEventListener("click", () => { + effect.event(button, "click", () => { if (document.fullscreenElement) { document.exitFullscreen(); } else { this.parent.requestFullscreen(); } });
276-283: Teardown sequencing LGTM; consider aligning order with HangPublish for consistency.
1-8: Definition/registration looks fine.If you expect hot module reload, guard
customElements.defineto avoidNotSupportedError.- customElements.define("hang-watch", HangWatch); + if (!customElements.get("hang-watch")) { + customElements.define("hang-watch", HangWatch); + }(Same consideration for hang-publish.)
Also applies to: 468-475
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
js/hang/src/meet/element.ts(6 hunks)js/hang/src/meet/room.ts(3 hunks)js/hang/src/publish/element.ts(14 hunks)js/hang/src/support/element.ts(1 hunks)js/hang/src/watch/element.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/element.tsjs/hang/src/watch/element.tsjs/hang/src/support/element.tsjs/hang/src/meet/element.tsjs/hang/src/meet/room.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/element.tsjs/hang/src/watch/element.tsjs/hang/src/support/element.tsjs/hang/src/meet/element.tsjs/hang/src/meet/room.ts
🧬 Code graph analysis (5)
js/hang/src/publish/element.ts (7)
js/signals/src/index.ts (2)
Signal(19-83)Effect(88-459)js/hang/src/meet/element.ts (7)
url(54-56)url(58-60)name(62-64)name(66-68)name(153-182)name(184-217)effect(123-151)js/hang/src/connection.ts (1)
Connection(36-117)js/hang/src/publish/broadcast.ts (1)
Broadcast(25-163)js/hang/src/publish/source/camera.ts (2)
Camera(11-72)effect(28-66)js/hang/src/publish/source/screen.ts (1)
Screen(11-76)js/hang/src/publish/source/microphone.ts (2)
Microphone(11-79)effect(30-73)
js/hang/src/watch/element.ts (1)
js/signals/src/index.ts (2)
Signal(19-83)Effect(88-459)
js/hang/src/support/element.ts (2)
js/signals/src/index.ts (1)
Effect(88-459)js/hang/src/support/index.ts (1)
isSupported(120-167)
js/hang/src/meet/element.ts (4)
js/signals/src/index.ts (2)
Signal(19-83)Effect(88-459)js/hang/src/publish/element.ts (7)
name(89-91)name(93-95)url(81-83)url(85-87)effect(213-266)effect(268-288)effect(290-331)js/hang/src/connection.ts (2)
Connection(36-117)effect(69-112)js/hang/src/meet/room.ts (2)
Room(11-182)effect(83-96)
js/hang/src/meet/room.ts (5)
js/hang/src/publish/broadcast.ts (1)
Broadcast(25-163)js/hang/src/watch/broadcast.ts (1)
Broadcast(33-175)js/hang/src/preview/room.ts (1)
RoomProps(7-10)js/moq/src/path.ts (1)
Valid(31-31)js/signals/src/index.ts (1)
Signal(19-83)
⏰ 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 (7)
js/hang/src/meet/room.ts (1)
90-96: Defaultundefinedprefix yields an unfiltered subscription; no changes needed.js/hang/src/publish/element.ts (3)
81-96: LGTM: property accessors wrap signals cleanly.
169-188: LGTM: connection and broadcast wiring look correct, captions/speaking gating is coherent.
208-266: Source lifecycle looks sound; cleanup closes devices and clears broadcast sources.js/hang/src/watch/element.ts (3)
418-444: Status rendering looks correct and user-friendly.
102-166: Accessors and attribute mapping are clean and consistent.
12-21: Public API shape looks good; signal defaults are sensible.Also applies to: 27-55
| export interface HangPublishSignals { | ||
| url: Signal<URL | undefined>; | ||
| name: Signal<Moq.Path.Valid | undefined>; | ||
| device: Signal<SourceType | undefined>; | ||
| audio: Signal<boolean>; | ||
| video: Signal<boolean>; | ||
| controls: Signal<boolean>; | ||
| captions: Signal<boolean>; | ||
| source: Signal<SourceType | undefined>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Signals.device is exposed but never updated — remove it or keep it in sync with source.
Leaving signals.device publicly exposed yet stale will confuse consumers and break any code subscribing to it. Either remove device from HangPublishSignals (recommended) or mirror it to signals.source consistently.
Option A (remove now):
export interface HangPublishSignals {
url: Signal<URL | undefined>;
name: Signal<Moq.Path.Valid | undefined>;
- device: Signal<SourceType | undefined>;
audio: Signal<boolean>;
video: Signal<boolean>;
controls: Signal<boolean>;
captions: Signal<boolean>;
source: Signal<SourceType | undefined>;
}
// ...
signals: HangPublishSignals = {
url: new Signal<URL | undefined>(undefined),
name: new Signal<Moq.Path.Valid | undefined>(undefined),
- device: new Signal<SourceType | undefined>(undefined),
audio: new Signal<boolean>(false),
video: new Signal<boolean>(false),
controls: new Signal(false),
captions: new Signal(false),
source: new Signal<SourceType | undefined>(undefined),
};If you must keep it for compatibility, explicitly document it as deprecated and wire a one-way mirror from source to device (read-only), avoiding two-way loops. Want a patch for that path?
Also applies to: 28-37
🤖 Prompt for AI Agents
In js/hang/src/publish/element.ts around lines 14 to 23 (and similarly lines 28
to 37), the HangPublishSignals interface exposes a device Signal that is never
updated; remove the device property from the interface to avoid stale public
state (recommended). If you must preserve it for compatibility, mark it
deprecated in the type comment and implement a one-way mirror so signals.device
is a read-only Signal that subscribes to and reflects signals.source changes (no
two-way updates) ensuring it stays in sync; update any consumers/tests
accordingly.
| // Watch to see if the preview element is added or removed. | ||
| this.#preview = new Signal(this.parent.querySelector("video") as HTMLVideoElement | undefined); | ||
| const observer = new MutationObserver(() => { | ||
| this.#preview.set(this.parent.querySelector("video") as HTMLVideoElement | undefined); | ||
| }); | ||
| observer.observe(this.parent, { childList: true }); | ||
| this.#signals.cleanup(() => observer.disconnect()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
MutationObserver should watch subtree to catch nested
- observer.observe(this.parent, { childList: true });
+ observer.observe(this.parent, { childList: true, subtree: true });📝 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.
| // Watch to see if the preview element is added or removed. | |
| this.#preview = new Signal(this.parent.querySelector("video") as HTMLVideoElement | undefined); | |
| const observer = new MutationObserver(() => { | |
| this.#preview.set(this.parent.querySelector("video") as HTMLVideoElement | undefined); | |
| }); | |
| observer.observe(this.parent, { childList: true }); | |
| this.#signals.cleanup(() => observer.disconnect()); | |
| // Watch to see if the preview element is added or removed. | |
| this.#preview = new Signal(this.parent.querySelector("video") as HTMLVideoElement | undefined); | |
| const observer = new MutationObserver(() => { | |
| this.#preview.set(this.parent.querySelector("video") as HTMLVideoElement | undefined); | |
| }); | |
| observer.observe(this.parent, { childList: true, subtree: true }); | |
| this.#signals.cleanup(() => observer.disconnect()); |
🤖 Prompt for AI Agents
In js/hang/src/publish/element.ts around lines 161 to 168, the MutationObserver
is only observing direct children so nested <video> insertions are missed;
update the observer.observe call to include subtree: true in the options (e.g.,
{ childList: true, subtree: true }) so mutations in descendant nodes are
detected, and keep the existing querySelector update and cleanup logic
unchanged.
| const canvas = new Signal(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | ||
| const observer = new MutationObserver(() => { | ||
| canvas.set(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | ||
| }); | ||
| observer.observe(this.parent, { subtree: true, childList: true }); | ||
| this.effects.cleanup(() => observer.disconnect()); | ||
| observer.observe(this.parent, { childList: true }); | ||
| this.#signals.cleanup(() => observer.disconnect()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Observer should watch subtree to detect nested changes.
- observer.observe(this.parent, { childList: true });
+ observer.observe(this.parent, { childList: true, subtree: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const canvas = new Signal(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | |
| const observer = new MutationObserver(() => { | |
| canvas.set(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | |
| }); | |
| observer.observe(this.parent, { subtree: true, childList: true }); | |
| this.effects.cleanup(() => observer.disconnect()); | |
| observer.observe(this.parent, { childList: true }); | |
| this.#signals.cleanup(() => observer.disconnect()); | |
| const canvas = new Signal(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | |
| const observer = new MutationObserver(() => { | |
| canvas.set(this.parent.querySelector("canvas") as HTMLCanvasElement | undefined); | |
| }); | |
| observer.observe(this.parent, { childList: true, subtree: true }); | |
| this.#signals.cleanup(() => observer.disconnect()); |
🤖 Prompt for AI Agents
In js/hang/src/watch/element.ts around lines 204 to 210, the MutationObserver is
currently only observing direct childList changes on this.parent so it misses
canvas elements added or removed inside nested descendants; update the
observer.observe call to include subtree: true (i.e., { childList: true,
subtree: true }) so mutations within nested DOM trees are detected and the
canvas Signal is updated accordingly; keep the existing disconnect cleanup.
Spin up and spin down based on connected/disconnected callback.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor