Conversation
A "Twitch Plays" style demo where Game Boy Color games run server-side and stream live H.264 video + Opus audio to web viewers via MoQ. Anyone can send inputs in anarchy mode (all inputs applied immediately, union of all viewers' held buttons). Features: - Boytacean Game Boy Color emulator with CGB color support - H.264 video at native 160x144 resolution (~60fps, CRF 18) - Opus audio encoding via ffmpeg-next - Per-viewer button state tracking (hold support, union logic) - End-to-end latency measurement per viewer - Auto-reset after 5 minutes of inactivity with countdown - Reset button for viewers - Audio on hover at 50% volume - rom.moq.dev Cloudflare Worker + R2 for hosting homebrew ROMs - Bundled Big2Small (GPLv3 puzzle game) as default ROM - Session names default to ROM filename Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove extra blank line in index.ts (biome formatting) and drop unused hex and rand dependencies from dev/boy/Cargo.toml (cargo-shear). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughThe demo workspace was changed from 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
dev/boy/src/index.html (2)
188-190: Footer uses inline styles while the rest uses embedded CSS.Minor consistency issue: the footer has inline styles while all other styling is in the embedded
<style>block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/index.html` around lines 188 - 190, The footer element currently uses inline styles in its style attribute; move those CSS rules into the embedded <style> block and replace the inline style on the <footer> with a class or id (e.g., .app-footer or `#footer`) to match the project's existing styling pattern; update the embedded <style> block to include the same rules (position, bottom, left, right, padding, background, border-top, font-family, font-size, color, text-align) and change the <footer> tag to use the new class/id.
111-147: Consider adding ARIA labels for accessibility.The D-pad and action buttons use visual indicators (arrows and letters) but lack
aria-labelattributes. Screen reader users won't understand the button purposes.Example for one button
<button class="dpad-btn dpad-up" aria-label="D-pad Up">↑</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/index.html` around lines 111 - 147, Add accessible ARIA labels to the interactive buttons so screen readers can announce their purpose: update each D-pad button element with aria-labels (e.g., elements using classes dpad-btn and specific classes dpad-up, dpad-down, dpad-left, dpad-right), each AB action button using class ab-btn (e.g., "A button", "B button" or the specific action), and the start/select controls using meta-btn to include aria-label="Start" and aria-label="Select" (or equivalent descriptive labels). Ensure labels are concise and match the visible glyphs/text so locate and edit the actual <button> elements that use those class names in index.html.dev/boy/src/input.rs (1)
93-111: Silent channel send failures may hide issues.Multiple
let _ = cmd_tx.send(...).awaitcalls silently ignore send failures. While this is acceptable during graceful shutdown, it could hide issues if the receiver unexpectedly drops.Consider logging at
traceordebuglevel when sends fail, to aid debugging without cluttering production logs:if cmd_tx.send(Command::Press { ... }).await.is_err() { tracing::trace!("command channel closed"); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/input.rs` around lines 93 - 111, The send calls on cmd_tx for Command::Press, Command::Release and Command::Reset currently ignore failures (let _ = ...), so change each .await send to check the Result; on Err, emit a tracing::trace or tracing::debug log including the command type and viewer_id (and ts_ms for Press) and then break/return to stop processing if the channel is closed; specifically update the send sites that construct Command::Press, Command::Release and Command::Reset to call .await, inspect .is_err() or match the Result, log the failure with context, and exit the loop/handler when appropriate.dev/boy/src/video.rs (1)
77-79: Thread silently exits on initialization failure.If encoder or scaler initialization fails,
lazy_initreturnsNone, and the thread exits viareturn. This is acceptable behavior, but the thread handle is stored in_threadwithout any mechanism to detect this failure from the caller.Consider logging a more prominent error or exposing initialization status if the caller needs to know the encoder failed to start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/video.rs` around lines 77 - 79, The thread silently exits when lazy_init returns None because the pattern match in lazy_init (let (Some(enc), Some(color_scaler)) = (enc, color_scaler) else { return; }) simply returns from the thread, leaving the caller with an opaque _thread handle; change this so initialization failures are signaled: return a Result from lazy_init (e.g., Result<JoinHandle<...>, InitError>) or send an InitError/ok boolean on a one-shot channel (oneshot::channel or std::sync::mpsc) before returning so the caller can detect failure, and add a processLogger/error or eprintln inside the else branch referencing enc and color_scaler to log details about the failure; update call sites that store _thread to handle the Result or receive the init signal.dev/boy/src/worker.ts (2)
12-16: Missing handler for OPTIONS preflight requests.If CORS is needed and the browser sends preflight OPTIONS requests, the current implementation returns 405. Consider handling OPTIONS for CORS preflight.
Handle OPTIONS if CORS is needed
async fetch(request: Request, env: Env): Promise<Response> { + if (request.method === "OPTIONS") { + return new Response(null, { + headers: { + "Access-Control-Allow-Origin": "*", + "Access-Control-Allow-Methods": "GET, HEAD, OPTIONS", + }, + }); + } + if (request.method !== "GET" && request.method !== "HEAD") { return new Response("Method Not Allowed", { status: 405 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/worker.ts` around lines 12 - 16, The fetch handler currently rejects non-GET/HEAD methods, causing OPTIONS preflight to return 405; update the exported object's async fetch(request: Request, env: Env) to explicitly handle OPTIONS by returning a short-circuit Response (e.g., 204 No Content) with the required CORS headers (Access-Control-Allow-Origin, Access-Control-Allow-Methods including OPTIONS, Access-Control-Allow-Headers and optionally Access-Control-Max-Age) so browsers receive a proper preflight response; ensure the same CORS headers are also added to actual GET/HEAD responses so CORS is consistently supported.
34-44: The web client uses the Moq relay protocol and does not make direct HTTP requests torom.moq.dev. The ROM worker only serves GET/HEAD requests internally via the relay architecture, which does not require CORS headers. No code in the codebase makes cross-origin fetch calls to this endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/worker.ts` around lines 34 - 44, Remove any added CORS response headers from the ROM worker response generation: keep the internal GET/HEAD handling as-is (the block using request.method, headers, object.size, object.body and the Response(...) returns) but ensure you do not add Access-Control-Allow-* headers anywhere in the response construction since the Moq relay protocol is used and cross-origin requests do not occur; adjust the headers object to only include Content-Type, Cache-Control and Content-Length and ensure both the HEAD branch and the body Response branch reuse that headers object.dev/boy/src/emulator.rs (1)
62-63: Name the CGB boot-state register value.
0x11is written twice, and its meaning is only obvious if you already know the boot-state contract. Pull it into a small constant/helper so init and reset stay aligned.As per coding guidelines, "Avoid using magic numbers; use named constants instead."
Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/emulator.rs` around lines 62 - 63, Replace the magic literal 0x11 used when setting the CPU A register during boot/reset with a named constant to document the CGB boot-state value and keep init and reset aligned: introduce a constant like CGB_BOOT_A (or CGB_BOOT_REGISTER_A) and use it instead of the literal in the gb.load_boot_state() + gb.cpu().a assignment sites (and the other occurrence noted around the reset/init code). Update any helper or reset/init functions that rely on this value to reference the new constant so both boot and reset behavior remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/boy/justfile`:
- Around line 14-19: The "download rom" justfile recipe should fail on HTTP
errors and write atomically: replace the current curl -L -o "rom/{{rom}}" call
with a curl that uses --fail (and optionally --show-error -S) and writes to a
temporary file (e.g., "rom/{{rom}}.tmp"); create the rom directory first (mkdir
-p rom), run curl -fSL to that temp file, verify curl exits successfully, then
mv the temp file into "rom/{{rom}}" (atomic move) and remove the temp on failure
so a 404/500 won't leave a bad file behind.
In `@dev/boy/package.json`:
- Around line 7-11: Update the package.json "repository" object to use the
correct directory path: change the "directory" value from "dev/gb" to "dev/boy"
so the repository metadata accurately reflects the package location; locate the
"repository" object in dev/boy/package.json and update the "directory" property
accordingly.
In `@dev/boy/src/audio.rs`:
- Around line 54-66: The current logic creates resampler in resampler and then
chunks the input buffer by input_sample_rate before resampling, which yields
wrong frame duration for the Opus encoder (frame_size = 960 @ OPUS_SAMPLE_RATE).
Change the flow so you resample the continuous input stream into the Opus sample
rate first (use the existing resampler Context), buffer the resampled output
continuously, and only then slice out frame_size (960) samples per frame for the
encoder; update the code paths referenced around resampler, frame_size,
OPUS_SAMPLE_RATE and the chunking logic (the block where frames are created,
lines ~90–126) to consume the resampled buffer rather than resampling per
pre-chunked frames. Ensure streaming resampling handles partial frames across
calls so you always emit exact 960-sample frames to the encoder.
In `@dev/boy/src/index.ts`:
- Around line 112-123: The canvas click-only expansion must be made
keyboard-accessible: replace or augment the non-focusable canvas trigger
(symbol: canvas) with a real focusable control (e.g., a button or make canvas
tabindex="0") that toggles expanded (symbol: expanded) for the given sessionId,
and attach key handlers for Enter/Space so keyboard users toggle the same logic
used by the click handler (currently toggling expanded.peek() === sessionId).
Also expose state to assistive tech by setting aria-expanded on the trigger and
aria-controls referencing the controls element (symbol: controls), keep the
existing this.#signals.run behavior and el.classList.toggle("expanded",
isExpanded) unchanged, and ensure the trigger has an accessible label
(aria-label or screen-visible text) reflecting open/close state.
- Around line 316-346: Controls become live before the `commandTrack` is
available causing lost first presses; update the logic around the
`this.#signals.run` block and `this.#sendCommand` so input is gated or queued
until `commandTrack` is set: introduce a small in-scope pending queue (e.g.,
pendingCommands) that `this.#sendCommand` pushes into when `commandTrack` is
undefined, and when you set `commandTrack` inside the `effect.spawn` loop (where
you currently assign `if (req.track.name === "command") commandTrack =
req.track;`) flush the queue by writing each queued command to
`commandTrack.writeJson(...)`; also ensure the `effect.cleanup` clears the queue
and closes any resources so no stale commands remain.
- Around line 125-166: The key handlers (keyMap, onKeyDown, onKeyUp) currently
send press/release but don't handle cases where the browser cancels input (blur,
touchcancel, pointercancel) so buttons can stay latched; add a local Set (e.g.,
heldButtons) that onKeyDown adds the button and onKeyUp removes it, and register
additional listeners (window.blur, touchcancel, pointercancel — and optionally
mouseup/touchend) that iterate heldButtons and call this.#sendCommand({type:
"release", button}) for each then clear the set; ensure these new listeners are
registered alongside the existing document listeners and removed in the same
this.#signals.cleanup callback so cleanup/unmount works correctly (update
onKeyDown/onKeyUp to use the Set and reference expanded/sessionId as before).
In `@dev/boy/src/main.rs`:
- Around line 47-75: The code allows user-provided config.name to contain path
separators which creates invalid/undiscoverable MoQ paths and can trigger a
panic at consume_origin.with_root(...).expect(...); validate or sanitize the
name before composing broadcast_path and viewer_prefix (e.g., reject or replace
'/' and '\\' characters from config.name when building the local variable name
used for broadcast_path and viewer_prefix), and change the
consume_origin.with_root(...).expect(...) call to handle invalid roots
gracefully (return an error instead of panicking) so
publish_origin.publish_broadcast(...) and viewer_consumer creation use a safe,
validated name.
- Around line 98-225: The emulator loop spawned by emulator_handle via
tokio::task::spawn_blocking never receives a shutdown signal and thus keeps
running after session.closed() or input::handle_viewers() wins the
tokio::select!, leaking ffmpeg/encoder resources; fix by creating a shutdown
channel (e.g., tokio::sync::watch or oneshot) that you pass into the
spawn_blocking closure (alongside cmd_rx/video_encoder/audio_encoder), have the
loop poll/inspect the receiver each iteration and break out to clean up when a
shutdown is seen, and after the select! branch that wins send the shutdown
signal and await emulator_handle to completion before returning from run() so
encoders and ffmpeg are dropped deterministically.
---
Nitpick comments:
In `@dev/boy/src/emulator.rs`:
- Around line 62-63: Replace the magic literal 0x11 used when setting the CPU A
register during boot/reset with a named constant to document the CGB boot-state
value and keep init and reset aligned: introduce a constant like CGB_BOOT_A (or
CGB_BOOT_REGISTER_A) and use it instead of the literal in the
gb.load_boot_state() + gb.cpu().a assignment sites (and the other occurrence
noted around the reset/init code). Update any helper or reset/init functions
that rely on this value to reference the new constant so both boot and reset
behavior remain consistent.
In `@dev/boy/src/index.html`:
- Around line 188-190: The footer element currently uses inline styles in its
style attribute; move those CSS rules into the embedded <style> block and
replace the inline style on the <footer> with a class or id (e.g., .app-footer
or `#footer`) to match the project's existing styling pattern; update the embedded
<style> block to include the same rules (position, bottom, left, right, padding,
background, border-top, font-family, font-size, color, text-align) and change
the <footer> tag to use the new class/id.
- Around line 111-147: Add accessible ARIA labels to the interactive buttons so
screen readers can announce their purpose: update each D-pad button element with
aria-labels (e.g., elements using classes dpad-btn and specific classes dpad-up,
dpad-down, dpad-left, dpad-right), each AB action button using class ab-btn
(e.g., "A button", "B button" or the specific action), and the start/select
controls using meta-btn to include aria-label="Start" and aria-label="Select"
(or equivalent descriptive labels). Ensure labels are concise and match the
visible glyphs/text so locate and edit the actual <button> elements that use
those class names in index.html.
In `@dev/boy/src/input.rs`:
- Around line 93-111: The send calls on cmd_tx for Command::Press,
Command::Release and Command::Reset currently ignore failures (let _ = ...), so
change each .await send to check the Result; on Err, emit a tracing::trace or
tracing::debug log including the command type and viewer_id (and ts_ms for
Press) and then break/return to stop processing if the channel is closed;
specifically update the send sites that construct Command::Press,
Command::Release and Command::Reset to call .await, inspect .is_err() or match
the Result, log the failure with context, and exit the loop/handler when
appropriate.
In `@dev/boy/src/video.rs`:
- Around line 77-79: The thread silently exits when lazy_init returns None
because the pattern match in lazy_init (let (Some(enc), Some(color_scaler)) =
(enc, color_scaler) else { return; }) simply returns from the thread, leaving
the caller with an opaque _thread handle; change this so initialization failures
are signaled: return a Result from lazy_init (e.g., Result<JoinHandle<...>,
InitError>) or send an InitError/ok boolean on a one-shot channel
(oneshot::channel or std::sync::mpsc) before returning so the caller can detect
failure, and add a processLogger/error or eprintln inside the else branch
referencing enc and color_scaler to log details about the failure; update call
sites that store _thread to handle the Result or receive the init signal.
In `@dev/boy/src/worker.ts`:
- Around line 12-16: The fetch handler currently rejects non-GET/HEAD methods,
causing OPTIONS preflight to return 405; update the exported object's async
fetch(request: Request, env: Env) to explicitly handle OPTIONS by returning a
short-circuit Response (e.g., 204 No Content) with the required CORS headers
(Access-Control-Allow-Origin, Access-Control-Allow-Methods including OPTIONS,
Access-Control-Allow-Headers and optionally Access-Control-Max-Age) so browsers
receive a proper preflight response; ensure the same CORS headers are also added
to actual GET/HEAD responses so CORS is consistently supported.
- Around line 34-44: Remove any added CORS response headers from the ROM worker
response generation: keep the internal GET/HEAD handling as-is (the block using
request.method, headers, object.size, object.body and the Response(...) returns)
but ensure you do not add Access-Control-Allow-* headers anywhere in the
response construction since the Moq relay protocol is used and cross-origin
requests do not occur; adjust the headers object to only include Content-Type,
Cache-Control and Content-Length and ensure both the HEAD branch and the body
Response branch reuse that headers object.
🪄 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: f8b1534f-15b4-4a8b-9c3a-e986e4a76c1d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomldev/boy/.gitignoredev/boy/Cargo.tomldev/boy/justfiledev/boy/package.jsondev/boy/src/audio.rsdev/boy/src/emulator.rsdev/boy/src/index.htmldev/boy/src/index.tsdev/boy/src/input.rsdev/boy/src/main.rsdev/boy/src/video.rsdev/boy/src/worker.tsdev/boy/tsconfig.jsondev/boy/vite.config.tsdev/boy/wrangler.jsoncdev/drone/CHANGELOG.mddev/drone/justfiledev/drone/src/drone.rsdev/drone/src/game.rsdev/drone/src/index.htmldev/drone/src/index.tsdev/drone/src/main.rsdev/drone/src/sensor.rsdev/drone/src/video.rsdev/justfiledev/pub/wrangler.jsoncdoc/.vitepress/config.tsdoc/setup/demo/boy.mddoc/setup/demo/drone.mddoc/setup/index.mdjustfilepackage.json
💤 Files with no reviewable changes (11)
- dev/drone/CHANGELOG.md
- dev/drone/src/index.html
- dev/pub/wrangler.jsonc
- doc/setup/demo/drone.md
- dev/drone/src/drone.rs
- dev/drone/justfile
- dev/drone/src/index.ts
- dev/drone/src/game.rs
- dev/drone/src/sensor.rs
- dev/drone/src/video.rs
- dev/drone/src/main.rs
Comment on lines
+54
to
+66
| // Set up resampler if input rate differs from Opus rate. | ||
| let resampler = if input_sample_rate != OPUS_SAMPLE_RATE { | ||
| Some(ffmpeg_next::software::resampling::Context::get( | ||
| ffmpeg_next::format::Sample::I16(ffmpeg_next::format::sample::Type::Packed), | ||
| ffmpeg_next::ChannelLayout::STEREO, | ||
| input_sample_rate, | ||
| ffmpeg_next::format::Sample::I16(ffmpeg_next::format::sample::Type::Packed), | ||
| ffmpeg_next::ChannelLayout::STEREO, | ||
| OPUS_SAMPLE_RATE, | ||
| )?) | ||
| } else { | ||
| None | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For FFmpeg/libopus, what does encoder.frame_size() represent, and when resampling 44.1 kHz PCM into a 48 kHz Opus encoder should the input chunks still be 960 samples per channel or the 20 ms equivalent at the input rate?
💡 Result:
encoder.frame_size in libopus (used by FFmpeg) represents the number of samples per channel that must be provided to the opus_encode function for one Opus frame. It corresponds to one of the valid Opus frame durations (2.5/5/10/20/40/60 ms) at the encoder's sampling rate. For a standard 48 kHz Opus encoder and 20 ms frames, this is 960 samples per channel. When resampling 44.1 kHz PCM to a 48 kHz Opus encoder, resample the input first so that each chunk provided to the encoder contains exactly 960 samples per channel (20 ms at 48 kHz). Do not use the 20 ms equivalent at the input rate (882 samples at 44.1 kHz), as the encoder expects input frames sized according to its own 48 kHz rate, not the original source rate.
Citations:
- 1: https://opus-codec.org/docs/html_api/group__opusencoder.html
- 2: https://www.opus-codec.org/docs/opus_api-1.2/group__opus__encoder.html
- 3: https://manpages.ubuntu.com/manpages/noble/man3/opus_encoder.3.html
- 4: https://man.archlinux.org/man/opus_encoder.3.en
- 5: https://www.opus-codec.org/docs/html_api/group__opusencoder.html
- 6: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/remoting/codec/audio_encoder_opus.cc
- 7: https://unanswered.io/guide/opus-codec-sampling-rate
- 8: https://hydrogenaud.io/index.php/topic,97051.0.html
🏁 Script executed:
sed -n '24,126p' dev/boy/src/audio.rs | cat -nRepository: moq-dev/moq
Length of output: 4925
Resample audio before chunking into encoder frames, not after.
frame_size (960 samples) represents the 48 kHz Opus encoder's frame requirement. The current code chunks the input buffer at input_sample_rate first, then resamples. When input is 44.1 kHz, this produces 960 input samples ≈ 21.8 ms, not the 20 ms that the encoder expects. Per Opus specifications, resampling must occur before framing; apply resampling to a continuous buffer and extract 960-sample (20 ms at 48 kHz) frames from the resampled output.
This also applies to lines 90–126.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/audio.rs` around lines 54 - 66, The current logic creates
resampler in resampler and then chunks the input buffer by input_sample_rate
before resampling, which yields wrong frame duration for the Opus encoder
(frame_size = 960 @ OPUS_SAMPLE_RATE). Change the flow so you resample the
continuous input stream into the Opus sample rate first (use the existing
resampler Context), buffer the resampled output continuously, and only then
slice out frame_size (960) samples per frame for the encoder; update the code
paths referenced around resampler, frame_size, OPUS_SAMPLE_RATE and the chunking
logic (the block where frames are created, lines ~90–126) to consume the
resampled buffer rather than resampling per pre-chunked frames. Ensure streaming
resampling handles partial frames across calls so you always emit exact
960-sample frames to the encoder.
Comment on lines
+112
to
+123
| // Click to toggle expand. | ||
| canvas.addEventListener("click", () => { | ||
| expanded.set(expanded.peek() === sessionId ? undefined : sessionId); | ||
| }); | ||
|
|
||
| // React to expand state. | ||
| this.#signals.run((effect) => { | ||
| const exp = effect.get(expanded); | ||
| const isExpanded = exp === sessionId; | ||
| this.el.classList.toggle("expanded", isExpanded); | ||
| controls.style.display = isExpanded ? "flex" : "none"; | ||
| }); |
Contributor
There was a problem hiding this comment.
Make card expansion keyboard-accessible.
The only way to reveal the controls is a click on a non-focusable <canvas>, so keyboard-only users cannot open a session or reach the on-screen controls. Please expose a real focusable trigger here and handle Enter/Space with an accessible label/state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/index.ts` around lines 112 - 123, The canvas click-only expansion
must be made keyboard-accessible: replace or augment the non-focusable canvas
trigger (symbol: canvas) with a real focusable control (e.g., a button or make
canvas tabindex="0") that toggles expanded (symbol: expanded) for the given
sessionId, and attach key handlers for Enter/Space so keyboard users toggle the
same logic used by the click handler (currently toggling expanded.peek() ===
sessionId). Also expose state to assistive tech by setting aria-expanded on the
trigger and aria-controls referencing the controls element (symbol: controls),
keep the existing this.#signals.run behavior and el.classList.toggle("expanded",
isExpanded) unchanged, and ensure the trigger has an accessible label
(aria-label or screen-visible text) reflecting open/close state.
Comment on lines
+125
to
+166
| // Keyboard input when expanded — press on keydown, release on keyup. | ||
| const keyMap: Record<string, string> = { | ||
| ArrowUp: "up", | ||
| ArrowDown: "down", | ||
| ArrowLeft: "left", | ||
| ArrowRight: "right", | ||
| z: "b", | ||
| Z: "b", | ||
| x: "a", | ||
| X: "a", | ||
| Enter: "start", | ||
| Shift: "select", | ||
| }; | ||
|
|
||
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if (expanded.peek() !== sessionId) return; | ||
| if (e.repeat) return; // Ignore OS key repeat | ||
|
|
||
| const button = keyMap[e.key]; | ||
| if (button) { | ||
| this.#sendCommand({ type: "press", button }); | ||
| e.preventDefault(); | ||
| } else if (e.key === "Escape") { | ||
| expanded.set(undefined); | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
| const onKeyUp = (e: KeyboardEvent) => { | ||
| if (expanded.peek() !== sessionId) return; | ||
|
|
||
| const button = keyMap[e.key]; | ||
| if (button) { | ||
| this.#sendCommand({ type: "release", button }); | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
| document.addEventListener("keydown", onKeyDown); | ||
| document.addEventListener("keyup", onKeyUp); | ||
| this.#signals.cleanup(() => { | ||
| document.removeEventListener("keydown", onKeyDown); | ||
| document.removeEventListener("keyup", onKeyUp); | ||
| }); |
Contributor
There was a problem hiding this comment.
Release held inputs on blur and gesture cancellation.
keyup, mouseup, and touchend are not guaranteed when the tab loses focus or the browser cancels a gesture. In those cases the relay never sees a release, so a button can stay latched until disconnect/reset. Track held buttons locally and flush releases from window.blur plus touchcancel/pointercancel.
Also applies to: 375-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/index.ts` around lines 125 - 166, The key handlers (keyMap,
onKeyDown, onKeyUp) currently send press/release but don't handle cases where
the browser cancels input (blur, touchcancel, pointercancel) so buttons can stay
latched; add a local Set (e.g., heldButtons) that onKeyDown adds the button and
onKeyUp removes it, and register additional listeners (window.blur, touchcancel,
pointercancel — and optionally mouseup/touchend) that iterate heldButtons and
call this.#sendCommand({type: "release", button}) for each then clear the set;
ensure these new listeners are registered alongside the existing document
listeners and removed in the same this.#signals.cleanup callback so
cleanup/unmount works correctly (update onKeyDown/onKeyUp to use the Set and
reference expanded/sessionId as before).
Comment on lines
+316
to
+346
| this.#signals.run((effect) => { | ||
| const conn = effect.get(connection.established); | ||
| if (!conn) return; | ||
|
|
||
| const exp = effect.get(expanded); | ||
| if (exp !== sessionId) return; | ||
|
|
||
| const viewerId = Math.random().toString(36).slice(2, 8); | ||
| currentViewerId.set(viewerId); | ||
| const viewerBroadcast = new Moq.Broadcast(); | ||
| conn.publish(Moq.Path.from(`boy/${sessionId}/viewer/${viewerId}`), viewerBroadcast); | ||
| effect.cleanup(() => { | ||
| viewerBroadcast.close(); | ||
| commandTrack = undefined; | ||
| }); | ||
|
|
||
| effect.spawn(async () => { | ||
| for (;;) { | ||
| const req = await Promise.race([effect.cancel, viewerBroadcast.requested()]); | ||
| if (!req) break; | ||
| if (req.track.name === "command") commandTrack = req.track; | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| this.#sendCommand = (cmd: Record<string, unknown>) => { | ||
| if (!commandTrack) return; | ||
| // Attach the current video timestamp so the publisher can measure latency. | ||
| const ts = videoDecoder.timestamp.peek(); | ||
| commandTrack.writeJson({ ...cmd, ts: ts ?? 0 }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Don't accept input before the command track exists.
Controls become live as soon as the card expands, but #sendCommand is a no-op until viewerBroadcast.requested() yields the command track. The first presses after expanding can disappear entirely. Gate input on readiness or queue the pending commands until commandTrack is set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/index.ts` around lines 316 - 346, Controls become live before the
`commandTrack` is available causing lost first presses; update the logic around
the `this.#signals.run` block and `this.#sendCommand` so input is gated or
queued until `commandTrack` is set: introduce a small in-scope pending queue
(e.g., pendingCommands) that `this.#sendCommand` pushes into when `commandTrack`
is undefined, and when you set `commandTrack` inside the `effect.spawn` loop
(where you currently assign `if (req.track.name === "command") commandTrack =
req.track;`) flush the queue by writing each queued command to
`commandTrack.writeJson(...)`; also ensure the `effect.cleanup` clears the queue
and closes any resources so no stale commands remain.
Comment on lines
+47
to
+75
| // Default name to ROM filename without extension. | ||
| let name = config.name.clone().unwrap_or_else(|| { | ||
| rom_path | ||
| .file_stem() | ||
| .and_then(|s| s.to_str()) | ||
| .unwrap_or("unknown") | ||
| .to_string() | ||
| }); | ||
|
|
||
| tracing::info!(rom = %rom_path.display(), %name, "starting Game Boy emulator"); | ||
|
|
||
| let (cmd_tx, mut cmd_rx) = tokio::sync::mpsc::channel::<input::Command>(64); | ||
| let client = config.client.clone().init()?; | ||
|
|
||
| // Create the broadcast producer. | ||
| let mut broadcast = moq_lite::BroadcastProducer::default(); | ||
|
|
||
| // Publish origin: the GB session broadcast. | ||
| let publish_origin = moq_lite::Origin::produce(); | ||
| let broadcast_path = format!("boy/{}", name); | ||
| publish_origin.publish_broadcast(&broadcast_path, broadcast.consume()); | ||
|
|
||
| // Consume origin: viewer broadcasts under boy/{name}/viewer/. | ||
| let viewer_prefix = format!("boy/{}/viewer", name); | ||
| let consume_origin = moq_lite::Origin::produce(); | ||
| let mut viewer_consumer = consume_origin | ||
| .with_root(&viewer_prefix) | ||
| .expect("viewer prefix should be valid") | ||
| .consume(); |
Contributor
There was a problem hiding this comment.
Reject path separators in --name before composing MoQ paths.
The web client only surfaces top-level boy/{sessionId} announcements and explicitly skips nested suffixes, so a name like foo/bar can be published here but never discovered in dev/boy/src/index.ts. It also turns with_root(...).expect(...) into a panic path for user input.
Suggested guard
let name = config.name.clone().unwrap_or_else(|| {
rom_path
.file_stem()
.and_then(|s| s.to_str())
.unwrap_or("unknown")
.to_string()
});
+
+if name.contains('/') {
+ anyhow::bail!("session name must not contain '/'");
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/main.rs` around lines 47 - 75, The code allows user-provided
config.name to contain path separators which creates invalid/undiscoverable MoQ
paths and can trigger a panic at consume_origin.with_root(...).expect(...);
validate or sanitize the name before composing broadcast_path and viewer_prefix
(e.g., reject or replace '/' and '\\' characters from config.name when building
the local variable name used for broadcast_path and viewer_prefix), and change
the consume_origin.with_root(...).expect(...) call to handle invalid roots
gracefully (return an error instead of panicking) so
publish_origin.publish_broadcast(...) and viewer_consumer creation use a safe,
validated name.
Comment on lines
+98
to
+225
| let emulator_handle = tokio::task::spawn_blocking(move || -> Result<()> { | ||
| ffmpeg_next::init().context("failed to init ffmpeg")?; | ||
|
|
||
| let mut emu = emulator::Emulator::new(&rom_path)?; | ||
|
|
||
| // Set up audio encoder (runs on this thread since Opus encoding is fast). | ||
| // GB APU typically outputs at ~44100Hz but we'll check. | ||
| let mut audio_encoder = | ||
| audio::AudioEncoder::new(broadcast.clone(), catalog.clone(), 44100)?; | ||
|
|
||
| let frame_duration = std::time::Duration::from_micros(16_742); // ~59.73fps | ||
| let mut next_frame = std::time::Instant::now(); | ||
| let start = std::time::Instant::now(); | ||
| let mut last_input = std::time::Instant::now(); | ||
| let mut last_status = String::new(); | ||
| let timeout = std::time::Duration::from_secs(timeout_secs); | ||
| // Per-viewer latency: viewer_id -> (latency_ms, last_seen). | ||
| let mut viewer_latency: std::collections::HashMap<String, (f64, std::time::Instant)> = | ||
| std::collections::HashMap::new(); | ||
|
|
||
| loop { | ||
| // Wait for next frame. | ||
| let now = std::time::Instant::now(); | ||
| if now < next_frame { | ||
| std::thread::sleep(next_frame - now); | ||
| } | ||
| next_frame += frame_duration; | ||
|
|
||
| // Current media timestamp in milliseconds. | ||
| let current_ts_ms = start.elapsed().as_secs_f64() * 1000.0; | ||
|
|
||
| // Drain pending commands. | ||
| while let Ok(cmd) = cmd_rx.try_recv() { | ||
| match cmd { | ||
| input::Command::Press { | ||
| button, | ||
| viewer_id, | ||
| ts_ms, | ||
| } => { | ||
| emu.press(&viewer_id, button); | ||
| last_input = std::time::Instant::now(); | ||
|
|
||
| // Calculate end-to-end latency: current time - viewer's displayed time. | ||
| let latency = current_ts_ms - ts_ms; | ||
| if latency >= 0.0 { | ||
| viewer_latency.insert(viewer_id, (latency, std::time::Instant::now())); | ||
| } | ||
| } | ||
| input::Command::Release { button, viewer_id } => { | ||
| emu.release(&viewer_id, button); | ||
| last_input = std::time::Instant::now(); | ||
| } | ||
| input::Command::ViewerLeft { viewer_id } => { | ||
| emu.viewer_left(&viewer_id); | ||
| viewer_latency.remove(&viewer_id); | ||
| } | ||
| input::Command::Reset => { | ||
| tracing::info!("resetting emulator (viewer request)"); | ||
| emu.reset()?; | ||
| last_input = std::time::Instant::now(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check inactivity timeout. | ||
| let idle_time = std::time::Instant::now() - last_input; | ||
| if idle_time > timeout { | ||
| tracing::info!("resetting emulator (inactivity timeout)"); | ||
| emu.reset()?; | ||
| last_input = std::time::Instant::now(); | ||
| } | ||
|
|
||
| // Tick the emulator. | ||
| emu.tick(); | ||
|
|
||
| // Expire stale viewer latency entries (no input for 30s). | ||
| let stale = std::time::Duration::from_secs(30); | ||
| viewer_latency.retain(|_, (_, last_seen)| last_seen.elapsed() < stale); | ||
|
|
||
| // Publish status with held buttons, idle countdown, and per-viewer latency. | ||
| let held: Vec<_> = emu.pressed_buttons().iter().copied().collect(); | ||
| let idle_secs = idle_time.as_secs(); | ||
| let remaining = timeout_secs.saturating_sub(idle_secs); | ||
|
|
||
| let latency_map: serde_json::Map<String, serde_json::Value> = viewer_latency | ||
| .iter() | ||
| .map(|(k, (ms, _))| (k.clone(), serde_json::json!((*ms as u32)))) | ||
| .collect(); | ||
|
|
||
| let new_status = serde_json::json!({ | ||
| "buttons": held, | ||
| "reset_in": remaining, | ||
| "latency": latency_map, | ||
| }); | ||
| let new_status_str = new_status.to_string(); | ||
|
|
||
| if new_status_str != last_status { | ||
| last_status = new_status_str.clone(); | ||
| if let Ok(mut group) = status_producer.append_group() { | ||
| let _ = group.write_frame(new_status_str.into_bytes()); | ||
| let _ = group.finish(); | ||
| } | ||
| } | ||
|
|
||
| // Grab and publish video frame. | ||
| let rgba = Bytes::from(emu.framebuffer()); | ||
| let pts_micros = start.elapsed().as_micros() as u64; | ||
| let ts = hang::container::Timestamp::from_micros(pts_micros) | ||
| .context("timestamp overflow")?; | ||
|
|
||
| // Send to video encoder thread (non-blocking, drop frame if behind). | ||
| video_encoder.try_frame(rgba, ts); | ||
|
|
||
| // Grab and encode audio. | ||
| let samples = emu.audio_samples(); | ||
| if !samples.is_empty() { | ||
| if let Err(e) = audio_encoder.push_samples(&samples) { | ||
| tracing::warn!(error = %e, "audio encode error"); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| tokio::select! { | ||
| res = emulator_handle => res?.context("emulator error"), | ||
| res = session.closed() => res.map_err(Into::into), | ||
| res = input::handle_viewers(&mut viewer_consumer, &cmd_tx) => res, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Tokio, does dropping or ignoring the JoinHandlefromspawn_blocking stop a task that is already running, or does the blocking task continue until completion?
💡 Result:
Dropping or ignoring the JoinHandle from tokio::spawn_blocking does not stop the task. The blocking task continues running on its dedicated blocking thread until it completes naturally. The JoinHandle detaches the task when dropped, losing access to its result, but the task itself persists.
Citations:
- 1: https://docs.rs/tokio/latest/tokio/task/struct.JoinHandle.html
- 2: https://docs.rs/tokio/1.7.1/tokio/task/struct.JoinHandle.html
- 3: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html
- 4: https://users.rust-lang.org/t/tokios-spawn-tasks-and-join-handles/131438
Pass a shutdown signal into the emulator loop and await the handle before returning.
tokio::select! does not automatically cancel the spawn_blocking task when another branch wins. The blocking task continues running on the threadpool indefinitely, even if session.closed() or handle_viewers() completes first. Since the emulator loop has no explicit exit condition, this causes ffmpeg and encoder resources to remain active in the background, potentially hanging shutdown and leaking resources. Pass a shutdown channel into the loop so it can terminate cleanly, and ensure the emulator_handle is awaited before returning from run().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/main.rs` around lines 98 - 225, The emulator loop spawned by
emulator_handle via tokio::task::spawn_blocking never receives a shutdown signal
and thus keeps running after session.closed() or input::handle_viewers() wins
the tokio::select!, leaking ffmpeg/encoder resources; fix by creating a shutdown
channel (e.g., tokio::sync::watch or oneshot) that you pass into the
spawn_blocking closure (alongside cmd_rx/video_encoder/audio_encoder), have the
loop poll/inspect the receiver each iteration and break out to clean up when a
shutdown is seen, and after the select! branch that wins send the shutdown
signal and await emulator_handle to completion before returning from run() so
encoders and ffmpeg are dropped deterministically.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
dev/boy/src/main.rs (2)
93-94:⚠️ Potential issue | 🟠 MajorShut down the blocking emulator loop before returning from
run().If
session.closed()orhandle_viewers()wins thisselect!, theJoinHandleis dropped but thespawn_blockingloop has no exit path, so FFmpeg/encoder resources keep running in the background. Pass a shutdown signal into the loop and awaitemulator_handleafter the winning branch completes.In Tokio, does dropping the JoinHandle returned by tokio::task::spawn_blocking cancel a running task, or does the blocking task continue until completion?Also applies to: 113-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/main.rs` around lines 93 - 94, The blocking FFmpeg loop spawned with tokio::task::spawn_blocking (emulator_handle) has no shutdown path and will continue after its JoinHandle is dropped; modify run() so the spawn_blocking closure (where ffmpeg_next::init() is called) listens for a shutdown signal (e.g., an async or sync flag/channel) that you can trigger when session.closed() or handle_viewers() wins the select!, send that shutdown signal from the winning branch, and then await emulator_handle.join()/await the JoinHandle to ensure the blocking task has exited before returning from run(); update code paths that drop emulator_handle (references: emulator_handle, run(), spawn_blocking, session.closed(), handle_viewers()) to send the shutdown and await the handle.
42-49:⚠️ Potential issue | 🟡 MinorReject path separators in
--nameand return an error instead of panicking.The web client only discovers top-level
boy/{sessionId}announcements and skips nested suffixes, so--name foo/barpublishes a session the browser will never list. And if the derived root is invalid,with_root(...).expect(...)turns that CLI input into a panic instead of a normal error.Suggested fix
let name = config.name.clone().unwrap_or_else(|| { rom_path .file_stem() .and_then(|s| s.to_str()) .unwrap_or("unknown") .to_string() }); + if name.contains('/') || name.contains('\\') { + anyhow::bail!("session name must not contain path separators"); + } @@ let mut viewer_consumer = consume_origin .with_root(&viewer_prefix) - .expect("viewer prefix should be valid") + .context("invalid viewer prefix")? .consume();Also applies to: 61-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/main.rs` around lines 42 - 49, Validate and reject any provided config.name that contains path separator characters (e.g., '/' or '\\' or std::path::MAIN_SEPARATOR) and return a CLI error instead of accepting it; replace the unwrap_or_else block using config.name.clone().unwrap_or_else(...) so that if config.name.is_some() you check for separators and return Err with a clear message (referencing config.name and rom_path/rom filename logic), and for the derived root code replace the with_root(...).expect(...) call with proper error handling (propagate or return a Result error) so invalid roots produce a normal error return rather than a panic; apply the same separator validation to the other occurrence around lines 61-70.dev/boy/src/index.ts (3)
117-122:⚠️ Potential issue | 🟠 MajorFlush held buttons on blur, cancellation, and collapse.
keyup/mouseup/touchendare not guaranteed if the tab blurs, the browser cancels the gesture, or this card collapses before release. In those cases the publisher never sees arelease, so the button can stay latched until disconnect/reset. Track held buttons locally and flush them fromwindow.blur,touchcancel/pointercancel, and when expansion moves away from this session.Also applies to: 138-165, 374-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/index.ts` around lines 117 - 122, The UI currently relies on global keyup/mouseup/touchend to publish releases, which can miss events on blur/cancel or when the card collapses; update the component (around the this.#signals.run(effect => { const exp = effect.get(expanded); ... }) block and similar handlers) to maintain a local Set of held buttons/pointers when press events occur and ensure you flush/publish release for all entries on window.blur, touchcancel/pointercancel, and whenever isExpanded transitions from true to false (i.e., when effect.get(expanded) !== sessionId); wire cleanup into the same collapse path so held state is cleared and release events are emitted when the session loses expansion or the page loses focus.
111-122:⚠️ Potential issue | 🟠 MajorMake the expand/collapse trigger keyboard accessible.
The only way to open a session is clicking a non-focusable
<canvas>, so keyboard users cannot reach the controls at all. Use a real focusable trigger here and exposearia-expanded/aria-controlswith the same toggle logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/index.ts` around lines 111 - 122, The expand/collapse trigger uses a non-focusable canvas so keyboard users can't access controls; replace or augment the canvas click handler with a focusable trigger element (or make the canvas focusable) and wire the same toggle logic (use expanded.peek(), sessionId and expanded.set(...)) to it, update aria state (set aria-expanded true/false and aria-controls referencing the controls element) whenever this.#signals.run computes isExpanded, and ensure keyboard activation handles Enter/Space to toggle the same way as the click handler; also keep the existing this.el.classList.toggle and controls.style.display behavior in sync with the aria attributes.
315-345:⚠️ Potential issue | 🟠 MajorDon't drop the first commands before the viewer track exists.
Controls become live as soon as the card expands, but
#sendCommandis still a no-op untilviewerBroadcast.requested()yields the"command"track. The first presses after opening the card can disappear entirely; gate input on readiness or queue commands untilcommandTrackis set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/boy/src/index.ts` around lines 315 - 345, The first commands are dropped because `#sendCommand` is a no-op until commandTrack is set; fix by adding a small in-memory queue and flush logic: inside the this.#signals.run callback (where currentViewerId, viewerBroadcast, and commandTrack are managed) create a queuedCommands array and when you set commandTrack (in the spawn loop handling viewerBroadcast.requested()) immediately drain the queuedCommands by writing each entry via commandTrack.writeJson(...); update this.#sendCommand to push the command (with ts from videoDecoder.timestamp.peek()) into queuedCommands if commandTrack is not yet set, otherwise write directly; also clear/replace queuedCommands on effect.cleanup to avoid leaks and ensure commandTrack is set to undefined there as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/boy/src/index.ts`:
- Around line 57-63: When removing a session card in the block handling
"!entry.active" (the branch that gets the card from sessions via
sessions.get(id) and calls card.close(), card.el.remove(), sessions.delete(id),
updateEmptyState()), also clear the expanded state if it equals that id (i.e.,
set expanded = null or '' accordingly) so the UI doesn't keep referring to a
non-existent expanded session; apply the same fix to the analogous removal code
around the other branch (the code referenced at lines 193-196) so both removal
paths reset the expanded variable when deleting the expanded card.
---
Duplicate comments:
In `@dev/boy/src/index.ts`:
- Around line 117-122: The UI currently relies on global keyup/mouseup/touchend
to publish releases, which can miss events on blur/cancel or when the card
collapses; update the component (around the this.#signals.run(effect => { const
exp = effect.get(expanded); ... }) block and similar handlers) to maintain a
local Set of held buttons/pointers when press events occur and ensure you
flush/publish release for all entries on window.blur, touchcancel/pointercancel,
and whenever isExpanded transitions from true to false (i.e., when
effect.get(expanded) !== sessionId); wire cleanup into the same collapse path so
held state is cleared and release events are emitted when the session loses
expansion or the page loses focus.
- Around line 111-122: The expand/collapse trigger uses a non-focusable canvas
so keyboard users can't access controls; replace or augment the canvas click
handler with a focusable trigger element (or make the canvas focusable) and wire
the same toggle logic (use expanded.peek(), sessionId and expanded.set(...)) to
it, update aria state (set aria-expanded true/false and aria-controls
referencing the controls element) whenever this.#signals.run computes
isExpanded, and ensure keyboard activation handles Enter/Space to toggle the
same way as the click handler; also keep the existing this.el.classList.toggle
and controls.style.display behavior in sync with the aria attributes.
- Around line 315-345: The first commands are dropped because `#sendCommand` is a
no-op until commandTrack is set; fix by adding a small in-memory queue and flush
logic: inside the this.#signals.run callback (where currentViewerId,
viewerBroadcast, and commandTrack are managed) create a queuedCommands array and
when you set commandTrack (in the spawn loop handling
viewerBroadcast.requested()) immediately drain the queuedCommands by writing
each entry via commandTrack.writeJson(...); update this.#sendCommand to push the
command (with ts from videoDecoder.timestamp.peek()) into queuedCommands if
commandTrack is not yet set, otherwise write directly; also clear/replace
queuedCommands on effect.cleanup to avoid leaks and ensure commandTrack is set
to undefined there as currently done.
In `@dev/boy/src/main.rs`:
- Around line 93-94: The blocking FFmpeg loop spawned with
tokio::task::spawn_blocking (emulator_handle) has no shutdown path and will
continue after its JoinHandle is dropped; modify run() so the spawn_blocking
closure (where ffmpeg_next::init() is called) listens for a shutdown signal
(e.g., an async or sync flag/channel) that you can trigger when session.closed()
or handle_viewers() wins the select!, send that shutdown signal from the winning
branch, and then await emulator_handle.join()/await the JoinHandle to ensure the
blocking task has exited before returning from run(); update code paths that
drop emulator_handle (references: emulator_handle, run(), spawn_blocking,
session.closed(), handle_viewers()) to send the shutdown and await the handle.
- Around line 42-49: Validate and reject any provided config.name that contains
path separator characters (e.g., '/' or '\\' or std::path::MAIN_SEPARATOR) and
return a CLI error instead of accepting it; replace the unwrap_or_else block
using config.name.clone().unwrap_or_else(...) so that if config.name.is_some()
you check for separators and return Err with a clear message (referencing
config.name and rom_path/rom filename logic), and for the derived root code
replace the with_root(...).expect(...) call with proper error handling
(propagate or return a Result error) so invalid roots produce a normal error
return rather than a panic; apply the same separator validation to the other
occurrence around lines 61-70.
🪄 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: ddc9e896-3e29-432b-9e0b-6d759bf6ed82
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
dev/boy/Cargo.tomldev/boy/src/index.tsdev/boy/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dev/boy/Cargo.toml
Comment on lines
+57
to
+63
| } else if (!entry.active) { | ||
| const card = sessions.get(id); | ||
| if (card) { | ||
| card.close(); | ||
| card.el.remove(); | ||
| sessions.delete(id); | ||
| updateEmptyState(); |
Contributor
There was a problem hiding this comment.
Clear expanded when the active session disappears.
If id is the expanded card, expanded keeps pointing at a session that no longer exists. Every remaining card then evaluates active to false, so playback/input stay disabled until the user manually opens another card.
Suggested fix
} else if (!entry.active) {
const card = sessions.get(id);
if (card) {
+ if (expanded.peek() === id) {
+ expanded.set(undefined);
+ }
card.close();
card.el.remove();
sessions.delete(id);
updateEmptyState();
}Also applies to: 193-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/boy/src/index.ts` around lines 57 - 63, When removing a session card in
the block handling "!entry.active" (the branch that gets the card from sessions
via sessions.get(id) and calls card.close(), card.el.remove(),
sessions.delete(id), updateEmptyState()), also clear the expanded state if it
equals that id (i.e., set expanded = null or '' accordingly) so the UI doesn't
keep referring to a non-existent expanded session; apply the same fix to the
analogous removal code around the other branch (the code referenced at lines
193-196) so both removal paths reset the expanded variable when deleting the
expanded card.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dev/drone/withdev/boy/— a "Twitch Plays" style Game Boy Color streaming demorom.moq.devCloudflare Worker + R2 for hosting homebrew ROMsTest plan
just boy— starts relay + emulator + web viewerjust boy start --rom path/to/game.gb— custom ROM🤖 Generated with Claude Code