Set up moq-boy for publishing and add CDN infrastructure#1205
Merged
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>
Contributor
WalkthroughAdds a new cdn/boy module and related deployment, packaging, and UI artifacts. Introduces Terraform to provision a Linode instance, firewall, generated systemd unit files, and a DNS A record; a Justfile for remote deploy operations; systemd unit templates ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 6
🧹 Nitpick comments (10)
cdn/boy/versions.tf (1)
1-7: Consider adding a version constraint for the Linode provider.While the parent module may pin the provider version, adding a version constraint here documents the minimum compatible version and prevents accidental use with incompatible provider versions.
♻️ Suggested improvement
terraform { required_providers { linode = { source = "linode/linode" + version = ">= 2.0" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/versions.tf` around lines 1 - 7, Add a provider version constraint to the terraform required_providers block for linode (the required_providers.linode entry) to document and enforce a minimum compatible provider version; update the linode provider declaration to include an appropriate version constraint (e.g., a minimum or a semantic pin) that matches the parent module's compatibility, and ensure any changes are reflected in module documentation or comments so future consumers know the supported provider range.cdn/boy/outputs.tf (1)
1-4: Minor: Description could clarify this is a list of IPs.The
linode_instance.boy.ipv4attribute returns a list of IPv4 addresses. The current description is slightly misleading as it implies a single IP. This is functionally correct for the DNSrrdatasfield which expects a list.📝 Suggested description update
output "instance_ip" { - description = "Boy instance IP" + description = "Boy instance IPv4 addresses" value = linode_instance.boy.ipv4 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/outputs.tf` around lines 1 - 4, The output "instance_ip" description is misleading because linode_instance.boy.ipv4 returns a list; update the output "instance_ip" description to indicate it's a list of IPv4 addresses (e.g., "List of Boy instance IPv4 addresses" or "Boy instance IPv4 addresses (list)") and ensure any consumers (e.g., DNS rrdatas) treat the value as a list; locate the output block that references linode_instance.boy.ipv4 and change only the description string accordingly.cdn/boy/main.tf (1)
62-69: Consider restricting SSH access to known IP ranges.SSH is open to the entire internet (
0.0.0.0/0and::/0). While key-based authentication provides protection, restricting source IPs to your operations network or bastion ranges would add defense-in-depth.If open SSH is intentional for operational flexibility, consider documenting this decision or adding fail2ban via the stackscript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/main.tf` around lines 62 - 69, The inbound block labeled "allow-ssh" (protocol TCP, ports "22") currently allows IPv4 "0.0.0.0/0" and IPv6 "::/0"; update this rule to restrict SSH to your known operations/bastion IP ranges instead of open access, or if open SSH is required, add a clear comment/documentation in the Terraform or the stackscript and/or provision host-level mitigation (e.g., fail2ban) to accompany the wide-open "allow-ssh" rule.cdn/boy/boy-prepare.service.tftpl (1)
11-11: Consider adding wget retry options for transient network failures.The
wgetcommand has no retry flags. If the ROM server is temporarily unavailable during boot, the prepare service will fail and block the main service. SinceRemainAfterExit=yeskeeps the unit active on success, a failed download requires manual intervention.Consider adding retry flags:
-ExecStart=/var/lib/moq/pkg/bin/wget -nv https://rom.moq.dev/${rom} -O /var/lib/moq/${rom} +ExecStart=/var/lib/moq/pkg/bin/wget -nv --tries=5 --retry-connrefused --waitretry=10 https://rom.moq.dev/${rom} -O /var/lib/moq/${rom}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/boy-prepare.service.tftpl` at line 11, The ExecStart line invoking wget currently has no retry behavior; update the wget invocation (the ExecStart entry that calls /var/lib/moq/pkg/bin/wget) to add retry flags for transient network failures—e.g., include --tries=<n> (e.g. 10), --waitretry=<s> (e.g. 5), --timeout=<s> (e.g. 30) and --retry-connrefused so the unit will retry the ROM download instead of failing the prepare service; keep the same output path (-O /var/lib/moq/${rom}) and ensure flags are ordered before the URL.cdn/boy/boy.service.tftpl (1)
18-20: Comment is misleading —RestartSec=10sis a fixed delay, not exponential backoff.
RestartSec=10salone does not implement exponential backoff. If exponential backoff is desired, you'd needRestartSteps=andRestartMaxDelaySec=(systemd 254+), or handle it in a wrapper script.If a fixed 10-second delay is intentional, update the comment:
-# Restart with exponential backoff +# Restart after fixed delay Restart=always RestartSec=10s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/boy.service.tftpl` around lines 18 - 20, The comment claiming "Restart with exponential backoff" is incorrect for the unit using Restart=always and RestartSec=10s; either change the comment to state a fixed 10s restart delay or enable systemd exponential backoff by replacing/augmenting RestartSec with RestartSteps= and RestartMaxDelaySec= (requires systemd 254+) and keep Restart=always; update the comment and unit to reflect which behavior you choose and reference the Restart=always, RestartSec, RestartSteps, and RestartMaxDelaySec settings when making the change.cdn/boy/justfile (2)
93-98: Same sync issue inrestarttarget.The game names are hardcoded here as well. If the ROM list changes, this command will reference nonexistent services or miss new ones.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/justfile` around lines 93 - 98, The restart target currently hardcodes service names (in the restart target's ssh systemctl restart command), which will break if the ROM list changes; update the restart target to compute the service list dynamically instead of embedding names: call or run the existing ROM-listing command/target (e.g., replace the hardcoded names in ssh root@$HOST "systemctl restart ..." with a command substitution that invokes the project's ROM enumeration target/command and maps ROM names to their corresponding boy-<rom> service names), then feed that generated list to systemctl so new or removed ROM services are handled automatically.
55-78: Hardcoded service names must stay in sync withlocal.romsinmain.tf.The service names (
boy-big2small,boy-dangan, etc.) are duplicated here and incdn/boy/main.tf:2-9. Adding or removing a ROM requires updating both files, which is error-prone.Consider generating a list file during
tofu applyor using a loop over a shared source:# Example: derive names from generated service files for svc in gen/boy-*-prepare.service; do name=$(basename "$svc" .service) ssh root@$HOST "systemctl enable $name && systemctl start $name" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/boy/justfile` around lines 55 - 78, The hardcoded systemd unit names in justfile (the repeated "systemctl enable/start/restart boy-...") must be replaced with a generated/shared source that is kept in sync with local.roms in main.tf; update your pipeline so tofu apply writes a canonical list (or generates service unit files under gen/ like gen/boy-*-prepare.service) and change the justfile step to iterate over that list or the gen/ files to derive unit base names and run systemctl enable/start/restart for each, removing all literal "boy-big2small", "boy-dangan", "boy-opossum", "boy-capybara", "boy-fofk", "boy-gb-run" entries so additions/removals in local.roms (main.tf) are automatically reflected.js/moq-boy/src/element.ts (3)
8-8: FinalizationRegistry is a fallback, not a guarantee.The
FinalizationRegistrycallback is not guaranteed to run promptly (or at all). This is appropriate as a last-resort cleanup, but the primary cleanup path should be throughdisconnectedCallbackor explicitclose()calls. The current implementation correctly uses#enabled.set(false)indisconnectedCallback, but if the element is GC'd without being disconnected (e.g., orphaned in JS), the connection may remain open until GC runs—if ever.Consider whether exposing a public
close()method for explicit teardown would be beneficial for consumers who manage lifecycle manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/moq-boy/src/element.ts` at line 8, The FinalizationRegistry usage (cleanup) is only a fallback; add an explicit public close() method on the element that performs the same teardown currently done in disconnectedCallback (e.g., call the existing logic that sets `#enabled.set`(false), closes any connections/signals and releases resources), update disconnectedCallback to call this.close(), and keep the FinalizationRegistry as a last-resort fallback; reference FinalizationRegistry/cleanup, disconnectedCallback, `#enabled.set`(false), and close() when implementing the change so consumers can explicitly call close() to deterministically tear down the element.
55-57: Consider extracting the emoji to a named constant.The Unicode escape
\u{1F3AE}(🎮) is a magic value. Extracting it to a named constant improves readability and makes it easier to change if needed.Suggested change
+const GAMEPAD_ICON = "\u{1F3AE}"; // 🎮 + // ... in constructor: const emptyIcon = document.createElement("div"); emptyIcon.className = "icon"; -emptyIcon.textContent = "\u{1F3AE}"; +emptyIcon.textContent = GAMEPAD_ICON;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/moq-boy/src/element.ts` around lines 55 - 57, The emoji literal "\u{1F3AE}" used when creating emptyIcon is a magic value; extract it into a named constant (e.g., GAME_EMOJI or EMPTY_ICON_EMOJI) near the top of element.ts and replace the inline escape in the emptyIcon.textContent assignment with that constant so the symbol is documented and easy to change; ensure the constant is exported if used elsewhere.
111-112: Extract status colors to named constants.The inline hex color values are magic strings that lack semantic meaning. Per coding guidelines, named constants should be used instead.
Suggested refactor
+const STATUS_COLORS = { + connected: "#8bac0f", + connecting: "#facc15", + disconnected: "#888", +} as const; + // ... in the effect: this.#statusEl.style.color = - status === "connected" ? "#8bac0f" : status === "connecting" ? "#facc15" : "#888"; + STATUS_COLORS[status as keyof typeof STATUS_COLORS] ?? STATUS_COLORS.disconnected;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/moq-boy/src/element.ts` around lines 111 - 112, Replace the inline hex color magic strings used when setting this.#statusEl.style.color (the ternary that checks status === "connected" / "connecting" / else) with named constants (e.g., STATUS_COLOR_CONNECTED, STATUS_COLOR_CONNECTING, STATUS_COLOR_DEFAULT or STATUS_COLOR_DISCONNECTED) declared at module or class scope; update the assignment in the method where this.#statusEl.style.color is set to use those constants so the intent is clear and colors are centralized for reuse and testing.
🤖 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/justfile`:
- Around line 37-38: The recursive chmod line (chmod -R 755 /var/lib/moq) will
override the JWT file permission change (chmod 600 /var/lib/moq/*.jwt); update
the deployment steps so JWTs are protected after any recursive permission
changes — either run the jwt chmod after the recursive step, or replace chmod -R
755 /var/lib/moq with a more selective approach (e.g., set directory permissions
only and set normal file perms excluding *.jwt, or use find to exclude *.jwt) so
that the chmod -R 755 /var/lib/moq and chmod 600 /var/lib/moq/*.jwt commands do
not conflict.
In `@js/moq-boy/README.md`:
- Around line 1-3: The logo src currently points to the GitHub blob HTML page;
update the img src in the README's <img> tag to use the raw asset URL (replace
the blob path with raw.githubusercontent.com and the same repo/branch/path) so
the SVG bytes are served directly and the image renders in consumers like npm;
locate the <img> element in README.md and change its src accordingly.
In `@js/moq-boy/src/element.ts`:
- Around line 169-181: attributeChangedCallback and the url setter call new
URL(...) without protection and will throw on malformed input; wrap the URL
construction in a try/catch in both attributeChangedCallback(name: Observed,
...) and set url(value: string | URL | undefined) so that if value is already a
URL you use it directly, and if string parsing fails you call
this.connection.url.set(undefined) (and optionally log the error via
console.error or a logger) instead of letting the exception escape.
- Around line 116-154: The effect registered on this.#signals.run can re-enter
on reconnection while this.#sessions still holds old GameCard instances, causing
orphaned DOM cards; fix by clearing existing sessions when (re)entering the
effect or when connection.established becomes falsy: iterate this.#sessions,
call card.close(), remove card.el from the DOM, and this.#sessions.clear()
before subscribing to the new announced stream (the block that creates announced
via this.connection.established and spawns the loop), and also ensure the same
cleanup happens in effect.cleanup() so stale cards are removed on disconnect;
reference: this.#signals.run, connection.established, announced, this.#sessions,
GameCard, effect.cleanup.
In `@js/moq-boy/src/index.ts`:
- Around line 40-41: The mute handler is being bound using
root.querySelector('.mute-btn') which selects from the shared root and causes
incorrect/null bindings because GameCard is constructed before card.el is
appended; update the constructor and any binding sites in GameCard (e.g., the
constructor and the method that binds the mute button around lines referencing
'.mute-btn') to query the mute button from this card's element (card.el or
this.el) instead of root, and attach the event listener directly to that element
so each GameCard instance wires its own mute button rather than rebinding the
shared root element.
In `@js/moq-boy/src/preview.ts`:
- Around line 65-70: The reactive runner in preview.ts (the this.#signals.run
callback) only sets broadcastName when name is truthy, leaving the previous
broadcast active if name becomes empty; change the callback in the
this.#signals.run used for preview name handling to explicitly clear or disable
the broadcast when name is falsy (e.g., call broadcastName.clear() or set to
undefined/null) instead of no-op, and make the same change for the other two
identical blocks referenced (the handlers around lines 143-147 and 165-167) so
that removing or setting preview.name = "" stops the old boy/<name> broadcast.
---
Nitpick comments:
In `@cdn/boy/boy-prepare.service.tftpl`:
- Line 11: The ExecStart line invoking wget currently has no retry behavior;
update the wget invocation (the ExecStart entry that calls
/var/lib/moq/pkg/bin/wget) to add retry flags for transient network
failures—e.g., include --tries=<n> (e.g. 10), --waitretry=<s> (e.g. 5),
--timeout=<s> (e.g. 30) and --retry-connrefused so the unit will retry the ROM
download instead of failing the prepare service; keep the same output path (-O
/var/lib/moq/${rom}) and ensure flags are ordered before the URL.
In `@cdn/boy/boy.service.tftpl`:
- Around line 18-20: The comment claiming "Restart with exponential backoff" is
incorrect for the unit using Restart=always and RestartSec=10s; either change
the comment to state a fixed 10s restart delay or enable systemd exponential
backoff by replacing/augmenting RestartSec with RestartSteps= and
RestartMaxDelaySec= (requires systemd 254+) and keep Restart=always; update the
comment and unit to reflect which behavior you choose and reference the
Restart=always, RestartSec, RestartSteps, and RestartMaxDelaySec settings when
making the change.
In `@cdn/boy/justfile`:
- Around line 93-98: The restart target currently hardcodes service names (in
the restart target's ssh systemctl restart command), which will break if the ROM
list changes; update the restart target to compute the service list dynamically
instead of embedding names: call or run the existing ROM-listing command/target
(e.g., replace the hardcoded names in ssh root@$HOST "systemctl restart ..."
with a command substitution that invokes the project's ROM enumeration
target/command and maps ROM names to their corresponding boy-<rom> service
names), then feed that generated list to systemctl so new or removed ROM
services are handled automatically.
- Around line 55-78: The hardcoded systemd unit names in justfile (the repeated
"systemctl enable/start/restart boy-...") must be replaced with a
generated/shared source that is kept in sync with local.roms in main.tf; update
your pipeline so tofu apply writes a canonical list (or generates service unit
files under gen/ like gen/boy-*-prepare.service) and change the justfile step to
iterate over that list or the gen/ files to derive unit base names and run
systemctl enable/start/restart for each, removing all literal "boy-big2small",
"boy-dangan", "boy-opossum", "boy-capybara", "boy-fofk", "boy-gb-run" entries so
additions/removals in local.roms (main.tf) are automatically reflected.
In `@cdn/boy/main.tf`:
- Around line 62-69: The inbound block labeled "allow-ssh" (protocol TCP, ports
"22") currently allows IPv4 "0.0.0.0/0" and IPv6 "::/0"; update this rule to
restrict SSH to your known operations/bastion IP ranges instead of open access,
or if open SSH is required, add a clear comment/documentation in the Terraform
or the stackscript and/or provision host-level mitigation (e.g., fail2ban) to
accompany the wide-open "allow-ssh" rule.
In `@cdn/boy/outputs.tf`:
- Around line 1-4: The output "instance_ip" description is misleading because
linode_instance.boy.ipv4 returns a list; update the output "instance_ip"
description to indicate it's a list of IPv4 addresses (e.g., "List of Boy
instance IPv4 addresses" or "Boy instance IPv4 addresses (list)") and ensure any
consumers (e.g., DNS rrdatas) treat the value as a list; locate the output block
that references linode_instance.boy.ipv4 and change only the description string
accordingly.
In `@cdn/boy/versions.tf`:
- Around line 1-7: Add a provider version constraint to the terraform
required_providers block for linode (the required_providers.linode entry) to
document and enforce a minimum compatible provider version; update the linode
provider declaration to include an appropriate version constraint (e.g., a
minimum or a semantic pin) that matches the parent module's compatibility, and
ensure any changes are reflected in module documentation or comments so future
consumers know the supported provider range.
In `@js/moq-boy/src/element.ts`:
- Line 8: The FinalizationRegistry usage (cleanup) is only a fallback; add an
explicit public close() method on the element that performs the same teardown
currently done in disconnectedCallback (e.g., call the existing logic that sets
`#enabled.set`(false), closes any connections/signals and releases resources),
update disconnectedCallback to call this.close(), and keep the
FinalizationRegistry as a last-resort fallback; reference
FinalizationRegistry/cleanup, disconnectedCallback, `#enabled.set`(false), and
close() when implementing the change so consumers can explicitly call close() to
deterministically tear down the element.
- Around line 55-57: The emoji literal "\u{1F3AE}" used when creating emptyIcon
is a magic value; extract it into a named constant (e.g., GAME_EMOJI or
EMPTY_ICON_EMOJI) near the top of element.ts and replace the inline escape in
the emptyIcon.textContent assignment with that constant so the symbol is
documented and easy to change; ensure the constant is exported if used
elsewhere.
- Around line 111-112: Replace the inline hex color magic strings used when
setting this.#statusEl.style.color (the ternary that checks status ===
"connected" / "connecting" / else) with named constants (e.g.,
STATUS_COLOR_CONNECTED, STATUS_COLOR_CONNECTING, STATUS_COLOR_DEFAULT or
STATUS_COLOR_DISCONNECTED) declared at module or class scope; update the
assignment in the method where this.#statusEl.style.color is set to use those
constants so the intent is clear and colors are centralized for reuse and
testing.
🪄 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: ea1f58af-d1bf-4836-a1cf-211a608796d6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
cdn/boy/.gitignorecdn/boy/boy-prepare.service.tftplcdn/boy/boy.service.tftplcdn/boy/flake.nixcdn/boy/justfilecdn/boy/main.tfcdn/boy/outputs.tfcdn/boy/variables.tfcdn/boy/versions.tfcdn/dns.tfcdn/main.tfflake.nixjs/moq-boy/README.mdjs/moq-boy/package.jsonjs/moq-boy/src/element.tsjs/moq-boy/src/index.htmljs/moq-boy/src/index.tsjs/moq-boy/src/preview.tsjs/moq-boy/src/styles.tsjs/moq-boy/tsconfig.jsonjs/moq-boy/vite.config.tsnix/overlay.nixrs/moq-boy/Cargo.toml
Comment on lines
+116
to
+154
| this.#signals.run((effect) => { | ||
| const conn = effect.get(this.connection.established); | ||
| if (!conn) return; | ||
|
|
||
| const announced = conn.announced(Moq.Path.from("boy")); | ||
| effect.cleanup(() => announced.close()); | ||
|
|
||
| effect.spawn(async () => { | ||
| for (;;) { | ||
| const entry = await Promise.race([effect.cancel, announced.next()]); | ||
| if (!entry) break; | ||
|
|
||
| // Strip "boy/" prefix, skip nested paths like "boy/x/viewer/..." | ||
| const suffix = Moq.Path.stripPrefix(Moq.Path.from("boy"), entry.path); | ||
| if (!suffix || suffix.includes("/")) continue; | ||
|
|
||
| const id = suffix; | ||
| if (entry.active && !this.#sessions.has(id)) { | ||
| const card = new GameCard({ | ||
| sessionId: id, | ||
| connection: this.connection, | ||
| expanded: this.#expanded, | ||
| root: shadow, | ||
| }); | ||
| this.#sessions.set(id, card); | ||
| this.#gridEl.appendChild(card.el); | ||
| this.#updateEmptyState(); | ||
| } else if (!entry.active) { | ||
| const card = this.#sessions.get(id); | ||
| if (card) { | ||
| card.close(); | ||
| card.el.remove(); | ||
| this.#sessions.delete(id); | ||
| this.#updateEmptyState(); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Potential stale cards on reconnection.
When the connection drops and re-establishes, this effect re-runs, but the #sessions map still contains cards from the previous connection. The announced stream from the new connection won't emit active: false for sessions that were known before, leaving orphaned cards in the DOM.
Consider clearing all sessions when the connection is lost or when re-entering this effect:
Suggested fix
this.#signals.run((effect) => {
const conn = effect.get(this.connection.established);
- if (!conn) return;
+ if (!conn) {
+ // Clear all sessions when disconnected
+ for (const [id, card] of this.#sessions) {
+ card.close();
+ card.el.remove();
+ }
+ this.#sessions.clear();
+ this.#updateEmptyState();
+ return;
+ }
const announced = conn.announced(Moq.Path.from("boy"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/moq-boy/src/element.ts` around lines 116 - 154, The effect registered on
this.#signals.run can re-enter on reconnection while this.#sessions still holds
old GameCard instances, causing orphaned DOM cards; fix by clearing existing
sessions when (re)entering the effect or when connection.established becomes
falsy: iterate this.#sessions, call card.close(), remove card.el from the DOM,
and this.#sessions.clear() before subscribing to the new announced stream (the
block that creates announced via this.connection.established and spawns the
loop), and also ensure the same cleanup happens in effect.cleanup() so stale
cards are removed on disconnect; reference: this.#signals.run,
connection.established, announced, this.#sessions, GameCard, effect.cleanup.
Comment on lines
+169
to
+181
| attributeChangedCallback(name: Observed, _oldValue: string | null, newValue: string | null) { | ||
| if (name === "url") { | ||
| this.connection.url.set(newValue ? new URL(newValue) : undefined); | ||
| } | ||
| } | ||
|
|
||
| get url(): URL | undefined { | ||
| return this.connection.url.peek(); | ||
| } | ||
|
|
||
| set url(value: string | URL | undefined) { | ||
| this.connection.url.set(value ? new URL(value) : undefined); | ||
| } |
Contributor
There was a problem hiding this comment.
URL parsing can throw on invalid input.
Both attributeChangedCallback and the url setter call new URL(value) without error handling. If a malformed URL is passed (either via attribute or property), this will throw an uncaught exception.
Suggested defensive handling
attributeChangedCallback(name: Observed, _oldValue: string | null, newValue: string | null) {
if (name === "url") {
- this.connection.url.set(newValue ? new URL(newValue) : undefined);
+ try {
+ this.connection.url.set(newValue ? new URL(newValue) : undefined);
+ } catch {
+ console.warn(`Invalid URL for moq-boy: ${newValue}`);
+ this.connection.url.set(undefined);
+ }
}
}
set url(value: string | URL | undefined) {
- this.connection.url.set(value ? new URL(value) : undefined);
+ if (!value) {
+ this.connection.url.set(undefined);
+ return;
+ }
+ try {
+ this.connection.url.set(value instanceof URL ? value : new URL(value));
+ } catch {
+ console.warn(`Invalid URL for moq-boy: ${value}`);
+ this.connection.url.set(undefined);
+ }
}📝 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.
Suggested change
| attributeChangedCallback(name: Observed, _oldValue: string | null, newValue: string | null) { | |
| if (name === "url") { | |
| this.connection.url.set(newValue ? new URL(newValue) : undefined); | |
| } | |
| } | |
| get url(): URL | undefined { | |
| return this.connection.url.peek(); | |
| } | |
| set url(value: string | URL | undefined) { | |
| this.connection.url.set(value ? new URL(value) : undefined); | |
| } | |
| attributeChangedCallback(name: Observed, _oldValue: string | null, newValue: string | null) { | |
| if (name === "url") { | |
| try { | |
| this.connection.url.set(newValue ? new URL(newValue) : undefined); | |
| } catch { | |
| console.warn(`Invalid URL for moq-boy: ${newValue}`); | |
| this.connection.url.set(undefined); | |
| } | |
| } | |
| } | |
| get url(): URL | undefined { | |
| return this.connection.url.peek(); | |
| } | |
| set url(value: string | URL | undefined) { | |
| if (!value) { | |
| this.connection.url.set(undefined); | |
| return; | |
| } | |
| try { | |
| this.connection.url.set(value instanceof URL ? value : new URL(value)); | |
| } catch { | |
| console.warn(`Invalid URL for moq-boy: ${value}`); | |
| this.connection.url.set(undefined); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/moq-boy/src/element.ts` around lines 169 - 181, attributeChangedCallback
and the url setter call new URL(...) without protection and will throw on
malformed input; wrap the URL construction in a try/catch in both
attributeChangedCallback(name: Observed, ...) and set url(value: string | URL |
undefined) so that if value is already a URL you use it directly, and if string
parsing fails you call this.connection.url.set(undefined) (and optionally log
the error via console.error or a logger) instead of letting the exception
escape.
Comment on lines
+65
to
+70
| this.#signals.run((effect) => { | ||
| const name = effect.get(this.#name); | ||
| if (name) { | ||
| broadcastName.set(Moq.Path.from(`boy/${name}`)); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Reset or disable the broadcast when name becomes empty.
Line 67 only updates broadcastName for truthy values. If a consumer removes the name attribute or sets preview.name = "", this element keeps streaming the previous boy/<old-name> broadcast instead of disconnecting or clearing.
Also applies to: 143-147, 165-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/moq-boy/src/preview.ts` around lines 65 - 70, The reactive runner in
preview.ts (the this.#signals.run callback) only sets broadcastName when name is
truthy, leaving the previous broadcast active if name becomes empty; change the
callback in the this.#signals.run used for preview name handling to explicitly
clear or disable the broadcast when name is falsy (e.g., call
broadcastName.clear() or set to undefined/null) instead of no-op, and make the
same change for the other two identical blocks referenced (the handlers around
lines 143-147 and 165-167) so that removing or setting preview.name = "" stops
the old boy/<name> broadcast.
- 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>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/moq-boy/src/index.ts (1)
12-17: Remove unusedrootproperty fromGameCardConfig.The
rootproperty is defined in the interface but never destructured or used in the constructor (line 41 only extractssessionId, connection, expanded). This appears to be leftover from the previous implementation that usedroot.querySelector(".mute-btn")before the mute button fix.Consider removing it from both the interface and the call sites (e.g.,
element.ts:134-139).♻️ Proposed fix
export interface GameCardConfig { sessionId: string; connection: Moq.Connection.Reload; expanded: Moq.Signals.Signal<string | undefined>; - root: ShadowRoot | HTMLElement; }And in
js/moq-boy/src/element.ts:const card = new GameCard({ sessionId: id, connection: this.connection, expanded: this.#expanded, - root: shadow, });Also applies to: 40-41
🤖 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 12 - 17, Remove the unused `root` property from the `GameCardConfig` interface and from any places that construct/pass that config (e.g., the call site currently creating the config object in element.ts); also remove any references to `root` in the `GameCard` constructor (the constructor currently only destructures `sessionId, connection, expanded`) so its parameter type no longer requires `root`. Update the `GameCardConfig` type definition to exclude `root`, update all callers that build a `GameCardConfig` object to stop passing `root`, and remove any now-unused variables/imports related to `root`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/moq-boy/src/index.ts`:
- Around line 12-17: Remove the unused `root` property from the `GameCardConfig`
interface and from any places that construct/pass that config (e.g., the call
site currently creating the config object in element.ts); also remove any
references to `root` in the `GameCard` constructor (the constructor currently
only destructures `sessionId, connection, expanded`) so its parameter type no
longer requires `root`. Update the `GameCardConfig` type definition to exclude
`root`, update all callers that build a `GameCardConfig` object to stop passing
`root`, and remove any now-unused variables/imports related to `root`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f26b0ca-ff01-4bba-8934-de0238af2c63
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
cdn/boy/justfilejs/moq-boy/README.mdjs/moq-boy/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- js/moq-boy/README.md
- cdn/boy/justfile
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
rs/moq-boy/Cargo.toml, addmoq-boyto nix overlay + flake packagesjs/moq-boyfrom a monolithic SPA into a publishable web component library with<moq-boy>(game grid with Fullscreen API) and<moq-boy-preview>(single-game thumbnail with audio on hover)cdn/boy/Terraform module —g6-standard-2Linode instance running 6 Game Boy emulators as separate systemd services, with ROM downloads fromrom.moq.devTest plan
just checkpasses (Rust, JS, Nix, Terraform)bun run buildinjs/moq-boyproduces dist/ with publint validationmoq-boyderivationbun run devinjs/moq-boyagainst a local relaytofu planincdn/shows expected resources🤖 Generated with Claude Code