Conversation
Apparently React 19 will only use properties, with no support for attributes. So we should to go back to exposing get/set methods instead of signals. Power users can access stuff like .backend or .state directly for signal access. IDK if we should put everything in there or not.
WalkthroughThe changes refactor state management in both the publish and watch components. In the publish component, individual reactive signals are consolidated into a state object containing source, muted, and invisible properties, while url and path handling are moved to connection and broadcast respectively. Public getter/setter accessors are added to proxy access to these internal signals. In the watch component, the formerly private backend field is exposed as a public backend member of type MultiBackend, and all internal references are updated to access audio, video, and other properties through this public backend. Additionally, getter/setter accessors are introduced for properties like url, path, paused, and jitter. UI context and component files are updated throughout to use the new property access patterns, switching from signal-style methods (.peek(), .set()) to direct property assignment. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🧹 Nitpick comments (2)
js/watch/src/element.ts (1)
28-29: Remove orphanedsyncfield —MultiBackendmanages its own internalSyncinstance.
this.syncis never passed toMultiBackendand is not referenced anywhere in the class or codebase.MultiBackend(perbackend.tsline 120) creates its own internalthis.#sync = new Sync({ jitter: this.jitter })and passes it toVideo.SourceandAudio.Source. The field is unreferenced dead code with no cleanup path, potentially leaking aSyncresource.🗑️ Remove orphaned field and import
- // Used to sync audio and video playback at a target latency. - sync = new Sync(); - // The backend that powers this element. backend: MultiBackend;-import { Sync } from "./sync";🤖 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 28 - 29, Remove the orphaned Sync field and its import: delete the `sync = new Sync();` property declaration in the class (the `sync` field in element.ts) and remove the `Sync` import since `MultiBackend` creates and manages its own internal `Sync` instance and this field is never referenced or passed anywhere; ensure no references to `this.sync` remain after removal.js/watch/src/ui/context.tsx (1)
72-72: Missing public setters forvolume,muted, and renditiontarget— inconsistent with PR direction
togglePlayback(line 60) andsetJitter(line 80) correctly use the new public property setters introduced by this PR, butsetVolume,toggleMuted, andsetActiveRenditionValuestill reach directly into backend signals:// setVolume props.moqWatch.backend.audio.volume.set(volume / 100); // line 72 — direct signal write // toggleMuted props.moqWatch.backend.audio.muted.update((muted) => !muted); // line 76 — direct signal write // setActiveRenditionValue props.moqWatch.backend.video.source.target.update(...); // line 84 — direct signal writeThis means the public element API is incomplete for
volume,muted, and rendition selection. Adding the corresponding setters onMoqWatch(similar toset paused/set jitterinelement.ts) would make the mutation surface consistent and insulate the UI from future internal restructuring of the backend.💡 Suggested additions to
element.tsto complete the API+ set volume(value: number) { + this.backend.audio.volume.set(value); + } + + get muted(): boolean { + return this.backend.audio.muted.peek(); + } + + set muted(value: boolean) { + this.backend.audio.muted.set(value); + }Then in
context.tsx:- props.moqWatch.backend.audio.volume.set(volume / 100); + props.moqWatch.volume = volume / 100; - props.moqWatch.backend.audio.muted.update((muted) => !muted); + props.moqWatch.muted = !props.moqWatch.muted;Also applies to: 76-77, 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/ui/context.tsx` at line 72, The UI is directly mutating backend signals (props.moqWatch.backend.audio.volume.set, .muted.update, and .video.source.target.update); add public setters on the MoqWatch element (e.g., set volume(value:number), set muted(flag:boolean) and setActiveRendition(target:string|number)) in element.ts alongside the existing setters (like set paused and set jitter), implement them to forward to the internal signals, and update context.tsx (setVolume, toggleMuted, setActiveRenditionValue) to call these new public setters instead of touching backend.* signals directly so the UI uses the element's public API.
🤖 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/watch/src/element.ts`:
- Around line 167-197: Add property accessors for muted and volume mirroring the
existing pattern (see url, path, paused, jitter). Implement get muted(): boolean
and set muted(value: boolean) that read/write the same observable state the
attributeChangedCallback uses for the "muted" attribute, and implement get
volume(): number and set volume(value: number) that read/write the same
observable for the "volume" attribute; follow the exact style used by
url/path/paused/jitter (use .peek() in the getter and .set(...) in the setter,
converting types as needed, and reference the same backing observable property
that attributeChangedCallback updates).
In `@js/watch/src/ui/element.tsx`:
- Around line 24-26: The getElement accessor returns ctx?.moqWatch.backend (a
MultiBackend) which is structurally incompatible with the Stats component's
expected ProviderProps; create an adapter that maps MultiBackend to the
ProviderProps shape (or adjust MultiBackend) by implementing a wrapper
function/class that converts Audio backend source to { source: { track:
Peekable; config: Peekable } } and Video backend source to { source: { catalog:
Peekable } } and return that adapted object from getElement; locate the code
paths around getElement, MultiBackend (in js/watch/src/backend.ts), and the
Stats consumer expecting ProviderProps (from `@moq/ui-core`) and ensure the
adapter implements the AudioBackend and VideoBackend interfaces required by
ProviderProps before returning it.
---
Nitpick comments:
In `@js/watch/src/element.ts`:
- Around line 28-29: Remove the orphaned Sync field and its import: delete the
`sync = new Sync();` property declaration in the class (the `sync` field in
element.ts) and remove the `Sync` import since `MultiBackend` creates and
manages its own internal `Sync` instance and this field is never referenced or
passed anywhere; ensure no references to `this.sync` remain after removal.
In `@js/watch/src/ui/context.tsx`:
- Line 72: The UI is directly mutating backend signals
(props.moqWatch.backend.audio.volume.set, .muted.update, and
.video.source.target.update); add public setters on the MoqWatch element (e.g.,
set volume(value:number), set muted(flag:boolean) and
setActiveRendition(target:string|number)) in element.ts alongside the existing
setters (like set paused and set jitter), implement them to forward to the
internal signals, and update context.tsx (setVolume, toggleMuted,
setActiveRenditionValue) to call these new public setters instead of touching
backend.* signals directly so the UI uses the element's public API.
| get url(): URL | undefined { | ||
| return this.connection.url.peek(); | ||
| } | ||
|
|
||
| get path(): Signal<Moq.Path.Valid | undefined> { | ||
| return this.broadcast.path; | ||
| set url(value: string | URL | undefined) { | ||
| this.connection.url.set(value ? new URL(value) : undefined); | ||
| } | ||
|
|
||
| get jitter(): Signal<Time.Milli> { | ||
| return this.#backend.jitter; | ||
| get path(): Moq.Path.Valid | undefined { | ||
| return this.broadcast.path.peek(); | ||
| } | ||
|
|
||
| get paused(): Signal<boolean> { | ||
| return this.#backend.paused; | ||
| set path(value: string | Moq.Path.Valid | undefined) { | ||
| this.broadcast.path.set(value ? Moq.Path.from(value) : undefined); | ||
| } | ||
|
|
||
| get audio(): Audio.Backend { | ||
| return this.#backend.audio; | ||
| get paused(): boolean { | ||
| return this.backend.paused.peek(); | ||
| } | ||
|
|
||
| get video(): Video.Backend { | ||
| return this.#backend.video; | ||
| set paused(value: boolean) { | ||
| this.backend.paused.set(value); | ||
| } | ||
|
|
||
| get jitter(): Time.Milli { | ||
| return this.backend.jitter.peek(); | ||
| } | ||
|
|
||
| set jitter(value: number) { | ||
| this.backend.jitter.set(value as Time.Milli); | ||
| } |
There was a problem hiding this comment.
muted and volume are missing getter/setter pairs — incomplete React 19 compatibility.
Both muted and volume are observable attributes handled in attributeChangedCallback and synced back to the DOM, but neither has a corresponding property accessor. Per the PR's stated goal ("React 19 will only use properties and will not support attributes"), omitting these leaves React 19 consumers unable to control audio mute/volume state via JSX props.
🔧 Proposed additions
set jitter(value: number) {
this.backend.jitter.set(value as Time.Milli);
}
+
+ get volume(): number {
+ return this.backend.audio.volume.peek();
+ }
+
+ set volume(value: number) {
+ this.backend.audio.volume.set(value);
+ }
+
+ get muted(): boolean {
+ return this.backend.audio.muted.peek();
+ }
+
+ set muted(value: boolean) {
+ this.backend.audio.muted.set(value);
+ }
}📝 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.
| get url(): URL | undefined { | |
| return this.connection.url.peek(); | |
| } | |
| get path(): Signal<Moq.Path.Valid | undefined> { | |
| return this.broadcast.path; | |
| set url(value: string | URL | undefined) { | |
| this.connection.url.set(value ? new URL(value) : undefined); | |
| } | |
| get jitter(): Signal<Time.Milli> { | |
| return this.#backend.jitter; | |
| get path(): Moq.Path.Valid | undefined { | |
| return this.broadcast.path.peek(); | |
| } | |
| get paused(): Signal<boolean> { | |
| return this.#backend.paused; | |
| set path(value: string | Moq.Path.Valid | undefined) { | |
| this.broadcast.path.set(value ? Moq.Path.from(value) : undefined); | |
| } | |
| get audio(): Audio.Backend { | |
| return this.#backend.audio; | |
| get paused(): boolean { | |
| return this.backend.paused.peek(); | |
| } | |
| get video(): Video.Backend { | |
| return this.#backend.video; | |
| set paused(value: boolean) { | |
| this.backend.paused.set(value); | |
| } | |
| get jitter(): Time.Milli { | |
| return this.backend.jitter.peek(); | |
| } | |
| set jitter(value: number) { | |
| this.backend.jitter.set(value as Time.Milli); | |
| } | |
| get url(): URL | undefined { | |
| return this.connection.url.peek(); | |
| } | |
| set url(value: string | URL | undefined) { | |
| this.connection.url.set(value ? new URL(value) : undefined); | |
| } | |
| get path(): Moq.Path.Valid | undefined { | |
| return this.broadcast.path.peek(); | |
| } | |
| set path(value: string | Moq.Path.Valid | undefined) { | |
| this.broadcast.path.set(value ? Moq.Path.from(value) : undefined); | |
| } | |
| get paused(): boolean { | |
| return this.backend.paused.peek(); | |
| } | |
| set paused(value: boolean) { | |
| this.backend.paused.set(value); | |
| } | |
| get jitter(): Time.Milli { | |
| return this.backend.jitter.peek(); | |
| } | |
| set jitter(value: number) { | |
| this.backend.jitter.set(value as Time.Milli); | |
| } | |
| get volume(): number { | |
| return this.backend.audio.volume.peek(); | |
| } | |
| set volume(value: number) { | |
| this.backend.audio.volume.set(value); | |
| } | |
| get muted(): boolean { | |
| return this.backend.audio.muted.peek(); | |
| } | |
| set muted(value: boolean) { | |
| this.backend.audio.muted.set(value); | |
| } | |
| } |
🤖 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 167 - 197, Add property accessors for
muted and volume mirroring the existing pattern (see url, path, paused, jitter).
Implement get muted(): boolean and set muted(value: boolean) that read/write the
same observable state the attributeChangedCallback uses for the "muted"
attribute, and implement get volume(): number and set volume(value: number) that
read/write the same observable for the "volume" attribute; follow the exact
style used by url/path/paused/jitter (use .peek() in the getter and .set(...) in
the setter, converting types as needed, and reference the same backing
observable property that attributeChangedCallback updates).
| getElement={(ctx) => { | ||
| return ctx?.moqWatch.backend; | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the Stats component and understand its getElement prop type
fd -e ts -e tsx | xargs rg -l "Stats" | head -20Repository: moq-dev/moq
Length of output: 684
🏁 Script executed:
# Look for the Stats component definition
rg -n "export.*Stats|function Stats|const Stats" --type=ts --type=tsxRepository: moq-dev/moq
Length of output: 82
🏁 Script executed:
# Search for getElement property/prop definition with context
rg -n "getElement" --type=ts --type=tsx -B2 -A2 | head -50Repository: moq-dev/moq
Length of output: 82
🏁 Script executed:
# Find the `@moq/ui-core` package location and Stats definition
fd -e ts -e tsx -path "*ui-core*" | head -20Repository: moq-dev/moq
Length of output: 223
🏁 Script executed:
# Read the Stats component definition
cat js/ui-core/src/stats/index.tsxRepository: moq-dev/moq
Length of output: 793
🏁 Script executed:
# Read the stats types
cat js/ui-core/src/stats/types.tsRepository: moq-dev/moq
Length of output: 1131
🏁 Script executed:
# Look at the element.tsx file to understand the full context
cat js/watch/src/ui/element.tsxRepository: moq-dev/moq
Length of output: 1066
🏁 Script executed:
# Search for MultiBackend type definition
rg -n "type MultiBackend|interface MultiBackend" -A5Repository: moq-dev/moq
Length of output: 533
🏁 Script executed:
# Search for MoqWatch type definition
rg -n "type MoqWatch|interface MoqWatch" -A5Repository: moq-dev/moq
Length of output: 1197
🏁 Script executed:
# Find the MoqWatch type definition
cat js/watch/src/element.tsRepository: moq-dev/moq
Length of output: 6291
🏁 Script executed:
# Get more details about MultiBackend
rg -n "class MultiBackend|export.*MultiBackend" -A10 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 917
🏁 Script executed:
# Check the Backend interface definition
rg -n "interface Backend|type Backend" -A15 js/watch/src/backend.ts | head -40Repository: moq-dev/moq
Length of output: 548
🏁 Script executed:
# Get the complete MultiBackend class implementation focusing on audio and video
rg -n "export class MultiBackend" -A30 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 1108
🏁 Script executed:
# Check what Video.Backend and Audio.Backend actually are
rg -n "export.*Video|export.*Audio" js/watch/src/video/backend.ts -A5 | head -30Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Also check audio backend
rg -n "export.*Audio" js/watch/src/audio/backend.ts -A5 | head -30Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Compare the types - look at the import statements in backend.ts
head -30 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 953
🏁 Script executed:
# Search for VideoBackend class/type definition
rg -n "class VideoBackend|interface VideoBackend|export VideoBackend" js/watch/src/video/backend.ts -A10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Search for AudioBackend class/type definition
rg -n "class AudioBackend|interface AudioBackend|export AudioBackend" js/watch/src/audio/backend.ts -A10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check what's exported from video module
cat js/watch/src/video/backend.ts | head -50Repository: moq-dev/moq
Length of output: 771
🏁 Script executed:
# Check what's exported from audio module
cat js/watch/src/audio/backend.ts | head -50Repository: moq-dev/moq
Length of output: 709
🏁 Script executed:
# Check if Peekable is structurally compatible with Signal or Getter
rg -n "interface Peekable|type Peekable" js/ui-core/src/stats/types.ts -B2 -A5Repository: moq-dev/moq
Length of output: 232
🏁 Script executed:
# Check Signal and Getter types from `@moq/signals`
fd -e ts "signals" -path "*node_modules/@moq/signals*" 2>/dev/null || echo "Not in node_modules, checking source"Repository: moq-dev/moq
Length of output: 91
🏁 Script executed:
# Look for type definitions in the actual watch backend to understand what AudioBackend and VideoBackend actually are at runtime
rg -n "AudioBackend|VideoBackend" js/watch/src/backend.ts -B2 -A2Repository: moq-dev/moq
Length of output: 793
🏁 Script executed:
# Check the imports to see if there's aliasing happening
head -15 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 485
🏁 Script executed:
# Get the full VideoBackend class implementation from watch
rg -n "^class VideoBackend" -A25 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 856
🏁 Script executed:
# Get the full AudioBackend class implementation from watch
rg -n "^class AudioBackend" -A25 js/watch/src/backend.tsRepository: moq-dev/moq
Length of output: 965
🏁 Script executed:
# Check if there's a type compatibility layer or adapter
rg -n "ProviderProps|AudioBackend|VideoBackend" js/watch/src/ui/element.tsx -B3 -A3Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check if there are any type errors or compatibility issues by looking at the entire Stats usage context
cat -n js/watch/src/ui/element.tsx | sed -n '1,40p'Repository: moq-dev/moq
Length of output: 1318
🏁 Script executed:
# Verify the exact source types to be thorough
rg -n "export.*class.*Source|export.*interface.*Source" js/watch/src/audio/ js/watch/src/video/ -A3Repository: moq-dev/moq
Length of output: 519
Fix the type mismatch between MultiBackend and ProviderProps.
The code returns ctx?.moqWatch.backend (a MultiBackend from js/watch/src/backend.ts) to the Stats component, which expects ProviderProps from @moq/ui-core. These types are structurally incompatible:
@moq/ui-core'sAudioBackendrequiressource: { track: Peekable; config: Peekable };watch'sAudioBackendprovidessource: Audio.Source(withbroadcastandtargetproperties instead).@moq/ui-core'sVideoBackendrequiressource: { catalog: Peekable };watch'sVideoBackendprovidessource: Video.Source.
Either adapt MultiBackend to match the expected interface, or create a wrapper/adapter that transforms it into the correct shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/watch/src/ui/element.tsx` around lines 24 - 26, The getElement accessor
returns ctx?.moqWatch.backend (a MultiBackend) which is structurally
incompatible with the Stats component's expected ProviderProps; create an
adapter that maps MultiBackend to the ProviderProps shape (or adjust
MultiBackend) by implementing a wrapper function/class that converts Audio
backend source to { source: { track: Peekable; config: Peekable } } and Video
backend source to { source: { catalog: Peekable } } and return that adapted
object from getElement; locate the code paths around getElement, MultiBackend
(in js/watch/src/backend.ts), and the Stats consumer expecting ProviderProps
(from `@moq/ui-core`) and ensure the adapter implements the AudioBackend and
VideoBackend interfaces required by ProviderProps before returning it.
2 participants
Apparently React 19 will only use properties, with no support for attributes. So we should to go back to exposing get/set methods instead of signals.
Power users can access stuff like .backend or .state directly for signal access. IDK if we should put everything in there or not. We'll bump to version 0.2.0 after this.
@pzanella now I understand some of the changes in #932. I can confirm this fixes your vite-7 branch but is much simpler than full blown React components.