Conversation
IDK I think it's easier to understand. And moq-transport calls it namespace
WalkthroughThe change renames the broadcast identifier from 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/demo/src/publish.html (1)
45-45:⚠️ Potential issue | 🟡 MinorStale prose — "broadcast path" should be "broadcast name"
Line 45 still reads "Reusing the same broadcast path…" after the rename to
name. This is inconsistent with the rest of the file.📝 Proposed fix
- Reusing the same broadcast path means viewers will automatically reconnect to the new session. + Reusing the same broadcast name means viewers will automatically reconnect to the new session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/demo/src/publish.html` at line 45, Update the stale text "Reusing the same broadcast path means viewers will automatically reconnect to the new session." to use the new identifier by replacing "broadcast path" with "broadcast name" so the prose matches the renamed variable `name` (search for that exact sentence in publish.html and change "path" → "name").
🧹 Nitpick comments (2)
js/watch/src/element.ts (1)
89-96: Minor note:nameattribute may collide with HTML semantics in certain contexts.Using
"name"as an HTML attribute on a custom element works fine, but be aware thatnameis a well-known attribute in HTML (form elements). If<moq-watch>is ever placed inside a<form>, the browser may treat thenameattribute as the form control name. This is unlikely to be an issue for a media playback component, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/element.ts` around lines 89 - 96, The code sets the HTML "name" attribute in the reactive effect (this.signals.run -> effect.get(this.name) -> setAttribute("name") / removeAttribute("name")), which can collide with form semantics; change to a nonstandard or namespaced attribute (e.g. "data-name" or "watch-name") by introducing a single attribute constant (e.g. const ATTRIBUTE_NAME = "data-name") and use that constant in place of the literal "name" in setAttribute/removeAttribute and any related lookups or consumers; update any code that reads this attribute to use the new attribute name.js/demo/src/config.ts (1)
9-9: Internal naming still referencespathwhile public API usesname.The private field
#pathInput(line 9), the method#onPathChange(line 107), and the comment "Create path input" (line 32) still use the oldpathterminology while the public API (get name(), observed attribute"name", event detailname) has been renamed. This creates a mild disconnect for future maintainers.Consider renaming these internals in a follow-up for consistency:
♻️ Suggested internal renames
- `#pathInput`: HTMLInputElement; + `#nameInput`: HTMLInputElement;- // Create path input + // Create name input const pathLabel = document.createElement("label");- `#onPathChange`() { - this.setAttribute("name", this.#pathInput.value); + `#onNameChange`() { + this.setAttribute("name", this.#nameInput.value); this.#dispatchChange(); }(Plus updating all
#pathInputreferences to#nameInputand#onPathChangeto#onNameChangethroughout.)Also applies to: 32-33, 107-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/demo/src/config.ts` at line 9, Private internals still use "path" naming causing inconsistency with the public API; rename the private field `#pathInput` to `#nameInput`, rename the private method `#onPathChange` to `#onNameChange`, and update the comment "Create path input" to "Create name input" (also update every reference to `#pathInput` and `#onPathChange` throughout the file), ensuring the getters/setters get name(), observedAttributes include "name", and event.detail uses name remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/demo/src/index.html`:
- Line 74: The example uses a stale room attribute on the custom element
<moq-watch>; remove the unsupported room="demo" attribute from the tag so the
snippet only uses the element's observed attributes (url, name, controls,
paused, muted, volume, jitter, reload), i.e., update the <moq-watch ...> example
to omit room and keep only valid attributes like url and name.
In `@js/demo/src/index.ts`:
- Line 16: The assignment to const name currently uses
urlParams.get("broadcast") ?? urlParams.get("name") which drops support for the
legacy ?path=... parameter; update the expression in the initialization of name
(the const named "name") to include the old key as an additional fallback (e.g.,
urlParams.get("broadcast") ?? urlParams.get("name") ?? urlParams.get("path")) so
existing ?path=... links continue to work, or alternatively accept both by
checking for path first if you want backward priority.
---
Outside diff comments:
In `@js/demo/src/publish.html`:
- Line 45: Update the stale text "Reusing the same broadcast path means viewers
will automatically reconnect to the new session." to use the new identifier by
replacing "broadcast path" with "broadcast name" so the prose matches the
renamed variable `name` (search for that exact sentence in publish.html and
change "path" → "name").
---
Nitpick comments:
In `@js/demo/src/config.ts`:
- Line 9: Private internals still use "path" naming causing inconsistency with
the public API; rename the private field `#pathInput` to `#nameInput`, rename the
private method `#onPathChange` to `#onNameChange`, and update the comment "Create
path input" to "Create name input" (also update every reference to `#pathInput`
and `#onPathChange` throughout the file), ensuring the getters/setters get name(),
observedAttributes include "name", and event.detail uses name remain unchanged.
In `@js/watch/src/element.ts`:
- Around line 89-96: The code sets the HTML "name" attribute in the reactive
effect (this.signals.run -> effect.get(this.name) -> setAttribute("name") /
removeAttribute("name")), which can collide with form semantics; change to a
nonstandard or namespaced attribute (e.g. "data-name" or "watch-name") by
introducing a single attribute constant (e.g. const ATTRIBUTE_NAME =
"data-name") and use that constant in place of the literal "name" in
setAttribute/removeAttribute and any related lookups or consumers; update any
code that reads this attribute to use the new attribute name.
| // If query params are provided, use them. | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const path = urlParams.get("broadcast") ?? urlParams.get("path"); | ||
| const name = urlParams.get("broadcast") ?? urlParams.get("name"); |
There was a problem hiding this comment.
Breaking change for existing ?path= query parameter users.
The query parameter fallback changed from urlParams.get("path") to urlParams.get("name"). Users who bookmarked or shared URLs with ?path=... will need to update to ?name=.... The ?broadcast= param is still supported as the primary key, which is good.
This is expected given the PR's intent, but worth noting in release notes if applicable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/demo/src/index.ts` at line 16, The assignment to const name currently uses
urlParams.get("broadcast") ?? urlParams.get("name") which drops support for the
legacy ?path=... parameter; update the expression in the initialization of name
(the const named "name") to include the old key as an additional fallback (e.g.,
urlParams.get("broadcast") ?? urlParams.get("name") ?? urlParams.get("path")) so
existing ?path=... links continue to work, or alternatively accept both by
checking for path first if you want backward priority.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
js/publish/src/element.ts (2)
230-235: Add a short doc comment for the publicnameAPI.A brief JSDoc clarifying what “name” represents will help consumers use it correctly. As per coding guidelines, "Document public APIs with clear docstrings or comments".
📝 Example
+ /** Broadcast name (full namespace/path, if applicable). */ get name(): Moq.Path.Valid | undefined { return this.broadcast.name.peek(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/publish/src/element.ts` around lines 230 - 235, Add a short JSDoc for the public property "name" on the class: describe what the getter (name -> this.broadcast.name.peek()) returns (type Moq.Path.Valid | undefined) and what the setter (name(value) -> this.broadcast.name.set(value ? Moq.Path.from(value) : undefined)) accepts and does (accepts string or Moq.Path.Valid, converts via Moq.Path.from and stores it). Place the doc comment immediately above the get name()/set name(...) pair and mention any semantics (nullable/optional, normalized via Moq.Path.from) so callers know expected input and behavior.
123-129: Consider a deprecatedpathalias for smoother upgrades.Removing
pathis a breaking change for existing consumers of the web component. If you want a softer transition, keeppathas a deprecated alias that maps tonamefor at least one release (and optionally log a deprecation warning).♻️ Suggested compatibility shim
- } else if (name === "name") { + } else if (name === "name" || name === "path") { this.broadcast.name.set(newValue ? Moq.Path.from(newValue) : undefined);// Also keep the deprecated attribute in OBSERVED for one release: const OBSERVED = ["url", "name", "path", "muted", "invisible", "source"] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/publish/src/element.ts` around lines 123 - 129, The attributeChangedCallback currently handles "url" and "name" but drops "path" which breaks existing consumers; keep "path" as a deprecated alias by adding "path" to the OBSERVED list and treat it the same as "name" inside attributeChangedCallback (map "path" to set this.broadcast.name using Moq.Path.from(newValue)), and optionally emit a single deprecation warning (e.g., console.warn or a logger) when "path" is used so consumers see the migration hint.js/watch/src/element.ts (1)
172-177: Add a short doc comment for the publicnameAPI.A brief JSDoc clarifying what “name” represents will help consumers use it correctly. As per coding guidelines, "Document public APIs with clear docstrings or comments".
📝 Example
+ /** Broadcast name (full namespace/path, if applicable). */ get name(): Moq.Path.Valid | undefined { return this.broadcast.name.peek(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/element.ts` around lines 172 - 177, Add a short JSDoc comment above the public name getter/setter describing what the property represents (e.g., the broadcast's logical path/identifier), the accepted types (string | Moq.Path.Valid | undefined) and behavior (setter converts string via Moq.Path.from and stores undefined to clear), and mention any constraints or expected format so callers know how to use the name API (place the comment directly above the get name()/set name(...) declarations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/publish/src/element.ts`:
- Around line 230-235: Add a short JSDoc for the public property "name" on the
class: describe what the getter (name -> this.broadcast.name.peek()) returns
(type Moq.Path.Valid | undefined) and what the setter (name(value) ->
this.broadcast.name.set(value ? Moq.Path.from(value) : undefined)) accepts and
does (accepts string or Moq.Path.Valid, converts via Moq.Path.from and stores
it). Place the doc comment immediately above the get name()/set name(...) pair
and mention any semantics (nullable/optional, normalized via Moq.Path.from) so
callers know expected input and behavior.
- Around line 123-129: The attributeChangedCallback currently handles "url" and
"name" but drops "path" which breaks existing consumers; keep "path" as a
deprecated alias by adding "path" to the OBSERVED list and treat it the same as
"name" inside attributeChangedCallback (map "path" to set this.broadcast.name
using Moq.Path.from(newValue)), and optionally emit a single deprecation warning
(e.g., console.warn or a logger) when "path" is used so consumers see the
migration hint.
In `@js/watch/src/element.ts`:
- Around line 172-177: Add a short JSDoc comment above the public name
getter/setter describing what the property represents (e.g., the broadcast's
logical path/identifier), the accepted types (string | Moq.Path.Valid |
undefined) and behavior (setter converts string via Moq.Path.from and stores
undefined to clear), and mention any constraints or expected format so callers
know how to use the name API (place the comment directly above the get
name()/set name(...) declarations).
1 participant
IDK I think it's easier to understand.
And moq-transport calls it namespace