Move demo page to demo/boy and bootstrap video catalog#1212
Conversation
- Add keywords/categories to rs/moq-boy Cargo.toml for crates.io - Add moq-boy to nix overlay and flake.nix packages - Restructure js/moq-boy from SPA into publishable web component library: - <moq-boy> element: game grid with fullscreen API expansion - <moq-boy-preview> element: single-game thumbnail with audio on hover - Library exports: GameCard class, types, no side effects - Add cdn/boy/ Terraform module with per-ROM systemd services on g6-standard-2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix GameCard mute button: query from own #buildControls() instead of root.querySelector() which would bind the wrong/null button - Use raw.githubusercontent.com for README logo so it renders on npm - Use find-based chmod in deploy to avoid overriding JWT file permissions - Merge with main (clean, no conflicts) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…omponent - Move index.html from js/moq-boy/src/ to demo/boy/src/ (library vs demo separation) - Move worker.ts to demo/boy/src/ and add vite.config.ts + package.json - Add demo/boy to workspace, use JS API directly instead of web component - Export styles from @moq/boy/styles for demo consumption - Remove moq-boy-preview web component (replaced by direct JS API usage) - Bootstrap one video frame at startup so the catalog includes video renditions before any viewer connects (fixes chicken-and-egg with auto-pause) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review WalkthroughConsolidates per-ROM prepare units into a single templated 🚥 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: 5
🧹 Nitpick comments (2)
cdn/boy/justfile (1)
57-58: Derive the game unit list instead of hardcoding it here.
cdn/boy/main.tfLines 20-28 already defines the source of truth vialocal.roms. Duplicating the names here means the next add/remove will leave deploy out of sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/justfile` around lines 57 - 58, Replace the hardcoded service list in the justfile systemctl enable/restart line with a derived list built from the canonical source local.roms defined in cdn/boy/main.tf: read or generate the service names from local.roms (transform rom names into their corresponding systemd unit names like boy-<rom>.service) and use that computed list when constructing the systemctl enable and systemctl restart commands so adds/removals to local.roms automatically propagate to the deploy step.demo/boy/src/index.ts (1)
10-76: Extract the static page sections into small builders.This block assembles the header, empty state, and about section inline, and locals like
aboutP1/aboutP2are already starting to reflect that. Pulling those sections into helpers would keep the entrypoint easier to scan and evolve.As per coding guidelines, "Use clear and descriptive variable names that convey intent" and "Keep functions focused and single-purpose".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/boy/src/index.ts` around lines 10 - 76, Extract the inline DOM assembly into three small helper builders: createHeader (returns the header element and sets statusEl with class "status"), createEmptyState (returns the empty-state div with icon/msg/hint children), and createAboutSection (returns the about div including the two paragraphs and aboutUl list); in index.ts replace the inline blocks that build header, emptyState, and about with calls to these helpers and append their returned elements to document.body, keep existing class names/text content and local identifiers (e.g., statusEl) inside the builders so consumers can still reference statusEl if needed, and ensure each helper is single-purpose, named clearly (createHeader/createEmptyState/createAboutSection) and only constructs/returns its top-level HTMLElement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cdn/boy/boy-prepare.service.tftpl`:
- Around line 1-2: The unit file for boy-prepare.service must declare its own
ordering to wait for network-online.target; update the [Unit] section in
cdn/boy/boy-prepare.service.tftpl to include "Wants=network-online.target" and
"After=network-online.target" (so the unit is pulled in and started after the
network is online) rather than relying on the separate boy.service ordering.
In `@cdn/boy/justfile`:
- Around line 55-56: The deploy currently runs "systemctl start
boy-prepare.service" which is a no-op after a oneshot unit has succeeded; change
the deploy commands to use "systemctl restart boy-prepare.service" (keeping the
enable step) so the boy-prepare.service (Type=oneshot, RemainAfterExit=yes) is
re-executed on each deploy and newly added/updated ROMs are fetched before game
services restart.
In `@cdn/boy/main.tf`:
- Around line 13-17: The generated unit file created by local_file
"boy_prepare_service" (boy-prepare.service) bundles multiple ROM fetch
ExecStart= commands under Type=oneshot, creating a shared failure domain so any
single wget failure prevents all units that Require=boy-prepare.service from
starting; fix by splitting responsibilities: either revert to per-ROM prepare
units (e.g., keep boy-<rom>.service units) or refactor the template so
boy-prepare.service only does non-failable bootstrap (directory creation) and
move each ROM fetch into its own unit or into ExecStartPre lines prefixed with
'-' to ignore failures, and update the templatefile invocation and any
boy-*.service Requires dependencies accordingly.
In `@demo/boy/package.json`:
- Around line 11-13: The package.json for the demo/boy package is missing a
direct dependency on `@moq/lite` even though demo/boy/src/index.ts imports it; add
"@moq/lite": "<appropriate-version-or-workspace:^>" to the "dependencies" object
in the demo/boy package.json so the package declares the runtime dependency
explicitly (this avoids relying on hoisting/transitive presence when resolving
imports from demo/boy/src/index.ts).
In `@demo/boy/src/index.ts`:
- Around line 79-81: The current fallback to "http://localhost:4443/anon" when
VITE_RELAY_URL is unset causes production builds to point at a developer-only
relay; change the logic around the url/connection creation so the localhost
fallback is only used when running in development (check import.meta.env.DEV)
and otherwise either use the provided import.meta.env.VITE_RELAY_URL or
throw/exit fast with a clear error; update the variables referenced
(VITE_RELAY_URL, url, and the Moq.Connection.Reload instantiation) to enforce
this behavior.
---
Nitpick comments:
In `@cdn/boy/justfile`:
- Around line 57-58: Replace the hardcoded service list in the justfile
systemctl enable/restart line with a derived list built from the canonical
source local.roms defined in cdn/boy/main.tf: read or generate the service names
from local.roms (transform rom names into their corresponding systemd unit names
like boy-<rom>.service) and use that computed list when constructing the
systemctl enable and systemctl restart commands so adds/removals to local.roms
automatically propagate to the deploy step.
In `@demo/boy/src/index.ts`:
- Around line 10-76: Extract the inline DOM assembly into three small helper
builders: createHeader (returns the header element and sets statusEl with class
"status"), createEmptyState (returns the empty-state div with icon/msg/hint
children), and createAboutSection (returns the about div including the two
paragraphs and aboutUl list); in index.ts replace the inline blocks that build
header, emptyState, and about with calls to these helpers and append their
returned elements to document.body, keep existing class names/text content and
local identifiers (e.g., statusEl) inside the builders so consumers can still
reference statusEl if needed, and ensure each helper is single-purpose, named
clearly (createHeader/createEmptyState/createAboutSection) and only
constructs/returns its top-level HTMLElement.
🪄 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: 618ba2d4-bce1-457b-ba6a-1e09a9838760
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignorecdn/boy/.gitignorecdn/boy/boy-prepare.service.tftplcdn/boy/boy.service.tftplcdn/boy/justfilecdn/boy/main.tfdemo/boy/.gitignoredemo/boy/package.jsondemo/boy/src/index.htmldemo/boy/src/index.tsdemo/boy/src/worker.tsdemo/boy/vite.config.tsdemo/boy/wrangler.jsoncjs/moq-boy/package.jsonjs/moq-boy/src/preview.tsjs/moq-boy/src/styles.tsjs/moq-boy/vite.config.tspackage.jsonrs/moq-boy/src/main.rs
💤 Files with no reviewable changes (3)
- cdn/boy/.gitignore
- js/moq-boy/vite.config.ts
- js/moq-boy/src/preview.ts
| [Unit] | ||
| Description=Download ROM for boy-${name} | ||
| Before=boy-${name}.service | ||
| Description=Download all Game Boy ROMs |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In systemd, if service A has After=network-online.targetandRequires=service B, does service B also wait fornetwork-online.target, or must service B declare its own Wants=/After=onnetwork-online.target?
💡 Result:
No, service B does not wait for network-online.target. Service A having After=network-online.target means A waits for the target (ordering dependency). Requires=serviceB in A means when A is started, B is also started (requirement dependency), and A is ordered after B if both are starting. However, B itself has no dependency on the target, so B starts without waiting for network-online.target. To make B wait, B must declare its own After=network-online.target (and typically Wants=network-online.target to pull it in if needed).
Citations:
- 1: https://www.man7.org/linux/man-pages/man5/systemd.unit.5.html
- 2: https://man7.org/linux/man-pages/man5/systemd.unit.5.html
- 3: https://www.freedesktop.org/software/systemd/man/latest/systemd.target.html
- 4: https://stackoverflow.com/questions/40593242/systemd-using-both-after-and-requires/40610058
- 5: https://serverfault.com/questions/812584/in-systemd-whats-the-difference-between-after-and-requires
- 6: https://serverfault.com/questions/812584/in-systemd-whats-the-difference-between-after-and-requires/812589
Add network-online.target ordering to this unit too.
cdn/boy/boy.service.tftpl waiting on network-online.target does not make boy-prepare.service wait for it. In systemd, ordering dependencies do not propagate from dependent services to their required/wanted dependencies. On boot, this unit can run wget before the network is up and fail the whole startup chain. This unit must declare its own ordering.
Suggested change
[Unit]
Description=Download all Game Boy ROMs
+Wants=network-online.target
+After=network-online.target📝 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.
| [Unit] | |
| Description=Download ROM for boy-${name} | |
| Before=boy-${name}.service | |
| Description=Download all Game Boy ROMs | |
| [Unit] | |
| Description=Download all Game Boy ROMs | |
| Wants=network-online.target | |
| After=network-online.target |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cdn/boy/boy-prepare.service.tftpl` around lines 1 - 2, The unit file for
boy-prepare.service must declare its own ordering to wait for
network-online.target; update the [Unit] section in
cdn/boy/boy-prepare.service.tftpl to include "Wants=network-online.target" and
"After=network-online.target" (so the unit is pulled in and started after the
network is online) rather than relying on the separate boy.service ordering.
| systemctl enable boy-prepare.service && \ | ||
| systemctl start boy-prepare.service && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In systemd, for a service with Type=oneshotandRemainAfterExit=yes, does systemctl start rerunExecStart=when the unit is already active, or issystemctl restart required?
💡 Result:
No, systemctl start does not rerun ExecStart= when the unit is already active (with Type=oneshot and RemainAfterExit=yes). systemctl start on an already active unit is a no-op; it does not execute the start commands again. To rerun ExecStart=, systemctl restart is required, which stops the unit (running ExecStop= if defined) and then starts it again.
Citations:
- 1: https://stackoverflow.com/questions/39032100/what-is-the-difference-between-systemds-oneshot-and-simple-service-types/39050387
- 2: https://www.commandlinux.com/man-page/man5/systemd.service.5.html
- 3: https://man.archlinux.org/man/systemd.service.5.en.txt
- 4: https://gist.github.com/drmalex07/d006f12914b21198ee43
- 5: https://www.freedesktop.org/software/systemd/man/249/systemctl.html
🏁 Script executed:
# Check if the files exist and read the relevant sections
fd -t f "justfile" cdn/boy/ 2>/dev/null | head -5Repository: moq-dev/moq
Length of output: 71
🏁 Script executed:
# Read the justfile around lines 55-56
if [ -f "cdn/boy/justfile" ]; then
sed -n '50,60p' "cdn/boy/justfile" | cat -n
else
echo "File not found"
fiRepository: moq-dev/moq
Length of output: 720
🏁 Script executed:
# Read the boy-prepare.service.tftpl file around lines 5-6
if [ -f "cdn/boy/boy-prepare.service.tftpl" ]; then
sed -n '1,15p' "cdn/boy/boy-prepare.service.tftpl" | cat -n
else
echo "File not found"
fiRepository: moq-dev/moq
Length of output: 453
Use restart for the prepare unit on deploy.
The boy-prepare.service is configured with Type=oneshot and RemainAfterExit=yes, which means systemctl start becomes a no-op once the unit has succeeded. Subsequent deploys will skip fetching newly added or updated ROMs before the game services are restarted. Use systemctl restart instead to rerun the service on each deploy.
Suggested change
- systemctl enable boy-prepare.service && \
- systemctl start boy-prepare.service && \
+ systemctl enable boy-prepare.service && \
+ systemctl restart boy-prepare.service && \📝 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.
| systemctl enable boy-prepare.service && \ | |
| systemctl start boy-prepare.service && \ | |
| systemctl enable boy-prepare.service && \ | |
| systemctl restart boy-prepare.service && \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cdn/boy/justfile` around lines 55 - 56, The deploy currently runs "systemctl
start boy-prepare.service" which is a no-op after a oneshot unit has succeeded;
change the deploy commands to use "systemctl restart boy-prepare.service"
(keeping the enable step) so the boy-prepare.service (Type=oneshot,
RemainAfterExit=yes) is re-executed on each deploy and newly added/updated ROMs
are fetched before game services restart.
| resource "local_file" "boy_prepare_service" { | ||
| for_each = local.roms | ||
| content = templatefile("${path.module}/boy-prepare.service.tftpl", { | ||
| name = each.key | ||
| rom = each.value | ||
| roms = values(local.roms) | ||
| }) | ||
| filename = "${path.module}/gen/boy-${each.key}-prepare.service" | ||
| filename = "${path.module}/gen/boy-prepare.service" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In systemd, if a Type=oneshotunit has multipleExecStart=lines and one command exits non-zero, is the unit marked failed? If another unit hasRequires=andAfter= on that oneshot unit, can it still start?
💡 Result:
Yes, if a Type=oneshot unit has multiple ExecStart= lines and one command exits non-zero (without the '-' prefix to ignore failure), the unit is marked as failed. The subsequent ExecStart= commands are not executed, and the unit enters the "failed" state. A unit with Requires= and After= on that oneshot unit cannot start if the oneshot unit fails, because Requires= enforces that the dependency must be active (or at least successfully started), and a failed unit does not satisfy this.
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/latest/systemd.service.html
- 2: https://www.commandlinux.com/man-page/man5/systemd.service.5.html
- 3: https://www.man7.org/linux/man-pages/man5/systemd.service.5.html
🏁 Script executed:
cd cdn/boy && find . -type f \( -name "*.tftpl" -o -name "*.tf" \) | head -20Repository: moq-dev/moq
Length of output: 154
🏁 Script executed:
cat -n cdn/boy/boy-prepare.service.tftplRepository: moq-dev/moq
Length of output: 453
🏁 Script executed:
cat -n cdn/boy/boy.service.tftplRepository: moq-dev/moq
Length of output: 747
🏁 Script executed:
# Check if any ExecStart lines use the '-' prefix for failure ignore
rg "ExecStart" cdn/boy/Repository: moq-dev/moq
Length of output: 245
Consolidating ROM downloads into one service creates a shared failure domain.
The boy-prepare.service uses Type=oneshot with multiple ExecStart= lines (one per ROM), none with the '-' prefix to ignore failures. Per systemd specification, if any wget command fails, the entire unit fails. Since every boy-*.service has Requires=boy-prepare.service, a single bad ROM download blocks all games from starting.
Keep per-ROM prepare units or split shared bootstrap (e.g., directory prep) from ROM-specific fetches so failures are isolated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cdn/boy/main.tf` around lines 13 - 17, The generated unit file created by
local_file "boy_prepare_service" (boy-prepare.service) bundles multiple ROM
fetch ExecStart= commands under Type=oneshot, creating a shared failure domain
so any single wget failure prevents all units that Require=boy-prepare.service
from starting; fix by splitting responsibilities: either revert to per-ROM
prepare units (e.g., keep boy-<rom>.service units) or refactor the template so
boy-prepare.service only does non-failable bootstrap (directory creation) and
move each ROM fetch into its own unit or into ExecStartPre lines prefixed with
'-' to ignore failures, and update the templatefile invocation and any
boy-*.service Requires dependencies accordingly.
| "dependencies": { | ||
| "@moq/boy": "workspace:^" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Direct imports of `@moq/lite` under demo/boy:"
rg -n --type ts --type js --type tsx --type jsx 'from "@moq/lite"|import \* as Moq from "@moq/lite"' demo/boy
echo
echo "package.json declarations of `@moq/lite`:"
rg -n --glob 'package.json' '"@moq/lite"' .Repository: moq-dev/moq
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Direct imports of `@moq/lite` under demo/boy ==="
rg '@moq/lite' demo/boy/ || true
echo
echo "=== Checking demo/boy/package.json dependencies ==="
cat demo/boy/package.json
echo
echo "=== Checking demo/boy/src/index.ts ==="
if [ -f "demo/boy/src/index.ts" ]; then
cat -n demo/boy/src/index.ts
else
echo "File not found"
fiRepository: moq-dev/moq
Length of output: 5706
Add @moq/lite as a direct dependency.
demo/boy/src/index.ts imports @moq/lite directly, but this manifest only declares @moq/boy. That makes the workspace rely on hoisting/transitives and can break isolated installs or stricter linkers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/boy/package.json` around lines 11 - 13, The package.json for the
demo/boy package is missing a direct dependency on `@moq/lite` even though
demo/boy/src/index.ts imports it; add "@moq/lite":
"<appropriate-version-or-workspace:^>" to the "dependencies" object in the
demo/boy package.json so the package declares the runtime dependency explicitly
(this avoids relying on hoisting/transitive presence when resolving imports from
demo/boy/src/index.ts).
| const url = import.meta.env.VITE_RELAY_URL || "http://localhost:4443/anon"; | ||
| const enabled = new Moq.Signals.Signal(true); | ||
| const connection = new Moq.Connection.Reload({ url: new URL(url), enabled }); |
There was a problem hiding this comment.
Don't ship a localhost relay fallback.
If VITE_RELAY_URL is missing in a deployed build, every browser silently points at http://localhost:4443/anon, which only works on the developer's machine and will also fail on HTTPS pages. Restrict that default to local development or fail fast outside dev.
💡 Suggested change
-const url = import.meta.env.VITE_RELAY_URL || "http://localhost:4443/anon";
+const relayUrl =
+ import.meta.env.VITE_RELAY_URL ??
+ (import.meta.env.DEV ? "http://localhost:4443/anon" : undefined);
+if (!relayUrl) {
+ throw new Error("VITE_RELAY_URL must be set outside local development");
+}
const enabled = new Moq.Signals.Signal(true);
-const connection = new Moq.Connection.Reload({ url: new URL(url), enabled });
+const connection = new Moq.Connection.Reload({
+ url: new URL(relayUrl, window.location.href),
+ enabled,
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const url = import.meta.env.VITE_RELAY_URL || "http://localhost:4443/anon"; | |
| const enabled = new Moq.Signals.Signal(true); | |
| const connection = new Moq.Connection.Reload({ url: new URL(url), enabled }); | |
| const relayUrl = | |
| import.meta.env.VITE_RELAY_URL ?? | |
| (import.meta.env.DEV ? "http://localhost:4443/anon" : undefined); | |
| if (!relayUrl) { | |
| throw new Error("VITE_RELAY_URL must be set outside local development"); | |
| } | |
| const enabled = new Moq.Signals.Signal(true); | |
| const connection = new Moq.Connection.Reload({ | |
| url: new URL(relayUrl, window.location.href), | |
| enabled, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/boy/src/index.ts` around lines 79 - 81, The current fallback to
"http://localhost:4443/anon" when VITE_RELAY_URL is unset causes production
builds to point at a developer-only relay; change the logic around the
url/connection creation so the localhost fallback is only used when running in
development (check import.meta.env.DEV) and otherwise either use the provided
import.meta.env.VITE_RELAY_URL or throw/exit fast with a clear error; update the
variables referenced (VITE_RELAY_URL, url, and the Moq.Connection.Reload
instantiation) to enforce this behavior.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
🤖 Generated with Claude Code