moq-boy: Review+revamp JS player#1224
Conversation
- Split expanded view into left (controls) and right (stats) panels - Cap video at 4x native resolution (640x576) - Enter key now only triggers Game Boy start button, not fullscreen toggle - Rename "Latency" header to "Players" and "Stats" to "Uptime" - Show latency note only when players are present - Add note about emulation pausing when no viewers - Round stats seconds instead of truncating to fix desync - Simplify feedback timeout to use plain setTimeout - Fix Signal.set() bug where stale #hasCapturedOldValue state caused missed notifications when set() was called with no subscribers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis pull request contains updates across multiple components of the moq-boy project. The About section documentation text is simplified in multiple files. The UI layout is refactored to split a single controls container into separate left and right panels with updated keyboard handling (Space key only). Styling is adjusted to accommodate the new panel layout. The signals package version is bumped to 0.1.5 with a bug fix that clears old-value state in Signal.set() when there are no subscribers. The Rust backend changes how duration statistics are reported by rounding floating-point values instead of truncating them. 🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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
🤖 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/moq-boy/src/index.ts`:
- Around line 349-350: The idle timeout stored in feedbackTimeout (declared at
top) may survive card disposal because clearTimeout is only invoked on
deactivation; update the disposal cleanup to always clear it by adding
clearTimeout(feedbackTimeout) inside the this.#signals.cleanup() handler (so
when GameCard.close()/disposal runs the timeout is cancelled); ensure the same
feedbackTimeout used in _sendCommand is referenced and cleared to prevent
lingering timers.
In `@js/moq-boy/src/styles.ts`:
- Around line 15-16: The expanded layout uses a horizontal flex row with both
side panels fixed at 240px and flex-shrink: 0 which causes overflow on narrow
phones; update the CSS in js/moq-boy/src/styles.ts to make the layout responsive
by either (A) adding a media query (e.g., max-width ~420px) that switches the
container with display: flex; flex-direction: column so the side panels stack
under the main card, or (B) allow the side panels to shrink by
removing/overriding flex-shrink: 0 and using min-width/max-width constraints so
they can go below 240px on small screens; ensure the same change is applied to
the other expanded layout block mentioned (lines ~75-81) and verify touch
controls in js/moq-boy/src/index.ts are reachable when the layout collapses.
In `@js/signals/src/index.ts`:
- Around line 80-85: The early-return branch that resets old-value state (when
this.#subscribers.size === 0 && this.#changed.size === 0) must also clear the
stale this.#forceNotify flag; update that branch (the block that sets
this.#hasCapturedOldValue = false and this.#oldValue = undefined) to explicitly
set this.#forceNotify = false before returning so the instance won't wrongly
force a future notification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11289979-0b1b-429f-a914-793a77b35058
📒 Files selected for processing (7)
demo/boy/src/index.tsjs/moq-boy/src/element.tsjs/moq-boy/src/index.tsjs/moq-boy/src/styles.tsjs/signals/package.jsonjs/signals/src/index.tsrs/moq-boy/src/main.rs
| let feedbackTimeout: ReturnType<typeof setTimeout> | undefined; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.ts" -path "*/js/moq-boy/src/*" | head -5Repository: moq-dev/moq
Length of output: 80
🏁 Script executed:
cd js/moq-boy && git ls-files src/ | head -20Repository: moq-dev/moq
Length of output: 96
🏁 Script executed:
wc -l ./js/moq-boy/src/index.tsRepository: moq-dev/moq
Length of output: 84
🏁 Script executed:
cat -n ./js/moq-boy/src/index.ts | sed -n '340,410p'Repository: moq-dev/moq
Length of output: 2875
🏁 Script executed:
cat -n ./js/moq-boy/src/index.ts | sed -n '1,100p' | grep -E "(`#signals`|Signals)"Repository: moq-dev/moq
Length of output: 468
🏁 Script executed:
cat -n ./js/moq-boy/src/index.ts | grep -n "this.#signals" | head -15Repository: moq-dev/moq
Length of output: 952
🏁 Script executed:
cat -n ./js/moq-boy/src/index.ts | sed -n '550,593p'Repository: moq-dev/moq
Length of output: 1650
🏁 Script executed:
cat -n ./js/moq-boy/src/index.ts | grep -n "close()" | head -20Repository: moq-dev/moq
Length of output: 709
Clear the idle timeout when the card is disposed.
The feedbackTimeout set in _sendCommand (line 397) can persist after GameCard.close() if the card is disposed while the timeout is pending. Currently, clearTimeout is only called during the deactivation path (line 358), which doesn't guarantee cleanup on disposal. Add clearTimeout(feedbackTimeout) via this.#signals.cleanup() to ensure the timeout is always cleared when the card is disposed.
Suggested fix
const currentViewerId = new Moq.Signals.Signal<string | undefined>(undefined);
const feedbackActive = new Moq.Signals.Signal(false);
let feedbackTimeout: ReturnType<typeof setTimeout> | undefined;
+ this.#signals.cleanup(() => {
+ if (feedbackTimeout !== undefined) {
+ clearTimeout(feedbackTimeout);
+ }
+ });📝 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.
| let feedbackTimeout: ReturnType<typeof setTimeout> | undefined; | |
| const currentViewerId = new Moq.Signals.Signal<string | undefined>(undefined); | |
| const feedbackActive = new Moq.Signals.Signal(false); | |
| let feedbackTimeout: ReturnType<typeof setTimeout> | undefined; | |
| this.#signals.cleanup(() => { | |
| if (feedbackTimeout !== undefined) { | |
| clearTimeout(feedbackTimeout); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/moq-boy/src/index.ts` around lines 349 - 350, The idle timeout stored in
feedbackTimeout (declared at top) may survive card disposal because clearTimeout
is only invoked on deactivation; update the disposal cleanup to always clear it
by adding clearTimeout(feedbackTimeout) inside the this.#signals.cleanup()
handler (so when GameCard.close()/disposal runs the timeout is cancelled);
ensure the same feedbackTimeout used in _sendCommand is referenced and cleared
to prevent lingering timers.
| display: flex; flex-direction: row; justify-content: space-around; | ||
| } |
There was a problem hiding this comment.
Make the expanded layout responsive below phone widths.
Both side panels are fixed at 240px with flex-shrink: 0, so the expanded card overflows on common mobile widths before the video gets any space. Since the controls are touch-enabled in js/moq-boy/src/index.ts, this blocks the mobile/touch flow. Stack the panels under a breakpoint, or let them shrink below 240px.
Suggested fix
.card.expanded {
width: 100%; height: 100%;
border-radius: 0; border: none; aspect-ratio: auto;
display: flex; flex-direction: row; justify-content: space-around;
}
@@
.card.expanded .panel {
display: flex; flex-direction: column; align-items: center; justify-content: center;
width: 240px; background: `#111`; flex-shrink: 0;
padding: 1rem; gap: 1rem;
}
.card.expanded .panel-left { border-right: 1px solid `#333`; order: -1; }
.card.expanded .panel-right { border-left: 1px solid `#333`; }
+
+ `@media` (max-width: 900px) {
+ .card.expanded {
+ flex-direction: column;
+ justify-content: flex-start;
+ overflow-y: auto;
+ }
+
+ .card.expanded .panel {
+ width: 100%;
+ max-width: none;
+ }
+
+ .card.expanded .panel-left,
+ .card.expanded .panel-right {
+ order: 0;
+ border-left: none;
+ border-right: none;
+ }
+ }Also applies to: 75-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/moq-boy/src/styles.ts` around lines 15 - 16, The expanded layout uses a
horizontal flex row with both side panels fixed at 240px and flex-shrink: 0
which causes overflow on narrow phones; update the CSS in
js/moq-boy/src/styles.ts to make the layout responsive by either (A) adding a
media query (e.g., max-width ~420px) that switches the container with display:
flex; flex-direction: column so the side panels stack under the main card, or
(B) allow the side panels to shrink by removing/overriding flex-shrink: 0 and
using min-width/max-width constraints so they can go below 240px on small
screens; ensure the same change is applied to the other expanded layout block
mentioned (lines ~75-81) and verify touch controls in js/moq-boy/src/index.ts
are reachable when the layout collapses.
| // Reset old value state since no flush will occur to clear it. | ||
| if (this.#subscribers.size === 0 && this.#changed.size === 0) { | ||
| this.#hasCapturedOldValue = false; | ||
| this.#oldValue = undefined; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear stale #forceNotify on the no-listener early return.
Line 77 may set #forceNotify = true, but Lines 81-85 return without clearing it. That stale flag can force a later notification even when there is no net value change.
Proposed fix
if (this.#subscribers.size === 0 && this.#changed.size === 0) {
this.#hasCapturedOldValue = false;
this.#oldValue = undefined;
+ this.#forceNotify = false;
return;
}📝 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.
| // Reset old value state since no flush will occur to clear it. | |
| if (this.#subscribers.size === 0 && this.#changed.size === 0) { | |
| this.#hasCapturedOldValue = false; | |
| this.#oldValue = undefined; | |
| return; | |
| } | |
| // Reset old value state since no flush will occur to clear it. | |
| if (this.#subscribers.size === 0 && this.#changed.size === 0) { | |
| this.#hasCapturedOldValue = false; | |
| this.#oldValue = undefined; | |
| this.#forceNotify = false; | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/signals/src/index.ts` around lines 80 - 85, The early-return branch that
resets old-value state (when this.#subscribers.size === 0 && this.#changed.size
=== 0) must also clear the stale this.#forceNotify flag; update that branch (the
block that sets this.#hasCapturedOldValue = false and this.#oldValue =
undefined) to explicitly set this.#forceNotify = false before returning so the
instance won't wrongly force a future notification.
- Rename GameCard -> Game as a non-UI backend class with member signals - Add SolidJS UI layer (components, context, hooks) matching @moq/watch patterns - Replace styles.ts with proper CSS files supporting light/dark mode - Add zod/mini validation for status JSON - Add configurable game/viewer path prefixes for production vs localhost - Pass signals directly to Watch APIs (enabled, muted) instead of effect wrappers - Fix responsive layout for narrow screens (stack panels below video) - Fix stale forceNotify flag bug in @moq/signals - Update demo app to use <moq-boy-ui><moq-boy> web component - Update docs with new prefix architecture Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kixelated
changed the title
1 participant
Summary
#hasCapturedOldValuestate whenset()was called with no subscribers caused missed notifications on subsequent sets — this broke controls after collapsing and re-expanding a gamesetTimeoutTest plan
🤖 Generated with Claude Code