fix(frontend): the browser build stopped opening the Netplay pane at you - #517
Conversation
On the wasm demo, loading a ROM force-opened the "Netplay (browser)" window
every session. The comment at the site gave a real reason -- the WebRTC
handshake keys on the ROM hash, so the lobby has nothing to offer before a ROM
exists -- and that is an argument for ENABLING a menu entry at ROM load, not for
opening a window nobody asked for.
Why it was written that way is the actual defect: the Netplay menu item was
`cfg(not(target_arch = "wasm32"))`, so the browser had no entry at all and
force-opening was the only route to the lobby. Three changes:
* `app.rs` no longer sets `wasm_lobby.open` on `RomLoaded`. The struct's
default was already `false`; that one assignment was the whole behaviour,
and it is now the only site in the crate that touches the flag (the menu).
* `ui_shell.rs` gains a wasm Netplay entry in the same menu group as the
native one, with the same `glyph::WIFI`, labelled "Netplay (browser)..."
after the window it opens. A browser cannot open a UDP socket, so the two
are different transports rather than one feature built twice, and the label
says so. Gated on `rom_interactive`, the predicate that group already uses.
* `MenuAction::OpenPanel(ToolPanel::Netplay)` routes to the lobby on wasm
instead of to `netplay_panel`, which renders only a "native-only" note
there -- routing to it would offer the user a window that says it does not
work. The lobby is an `App`-owned window rather than a `ToolPanel` and draws
inside the overlay's egui frame, so the overlay is forced visible exactly as
`open_chip_panel` does.
Native behaviour is unchanged. Verified with the two wasm clippy gates
(`wasm-winit` default and `wasm-canvas`), the native workspace gate, and fmt.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
Antigravity review (Gemini via Ultra)Resolves an issue in the WebAssembly build by adding a dedicated, ROM-gated Netplay menu item and removing the automatic force-opening of the lobby upon ROM load. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by Earlier review rounds (newest first)Round reviewed at 2026-09-12 17:48 UTCAntigravity review (Gemini via Ultra)This PR introduces a dedicated menu entry for the browser Netplay lobby and removes the behavior that forced it open automatically upon loading a ROM. Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
…ch other
The v2.6.18 follow-up fix had no gate. Antigravity's review said so, and it
was the one of its four points that was right: this adds the gate.
WHY A SOURCE-SHAPE GATE. Both properties live behind
`#[cfg(target_arch = "wasm32")]`, so a native `cargo test` cannot execute
either one, and there is no wasm test harness in this crate. The shape is
what is available to assert, so it is asserted precisely rather than
approximately -- the same idiom, in the same module, as
`every_wasm_rom_entry_point_corrects_the_header`, including that test's
recorded lesson about a file that reads itself.
WHAT IS PINNED, AND WHY BOTH HALVES TOGETHER. The defect was that loading a
ROM force-opened the lobby on every browser session. Removing that alone
would have made the feature UNREACHABLE rather than unobtrusive, because the
Netplay menu item was `cfg(not(wasm32))` and the force-open was the only
route to the lobby that existed. So the fix is two changes that are only
correct as a pair, and the test fails if either is undone:
1. `app.rs` contains EXACTLY ONE site assigning `wasm_lobby.open = true`.
A count, not a `contains`, and that is what makes it fail in BOTH
directions: a bare `contains` would stay green if the ROM-load force-open
came back ALONGSIDE the menu route -- the pre-fix state plus a menu.
2. That one site is the `OpenPanel(ToolPanel::Netplay)` route.
3. `ui_shell.rs` offers the wasm entry at all, gated on `rom_interactive`.
The third assertion matches a WINDOW around the label rather than one long
literal: the enable predicate, the label and the emitted action sit on
separate lines that rustfmt is free to re-wrap, and pinning the exact joined
spelling would fail on a reflow that changed nothing. Characters, not bytes,
per the rule about slicing in a diagnostic path.
DEMONSTRATED BY MUTATION -- three, one per assertion, all CAUGHT:
M1 reintroduce the wasm-gated auto-open in the ROM-loaded arm
-> CAUGHT: "expected exactly one site opening the browser lobby", left: 2
M2 retarget the menu route to a different ToolPanel
-> CAUGHT: "the wasm OpenPanel(Netplay) route no longer opens the lobby"
M3 replace `rom_interactive` with `true` on the menu entry
-> CAUGHT: "the wasm Netplay entry is no longer gated on a loaded ROM"
Two of the three had to be REFORMULATED first, and both reformulations are
recorded because they are the project's own two traps:
- M1 first inserted the assignment ungated and came back BUILD-FAILED --
`wasm_lobby` is a `cfg(wasm32)` field, so the mutation has to reproduce
the defect AS IT EXISTED, wasm-gated, rather than approximate it. A
mutant that does not compile is not a catch.
- M2 first anchored on the single-line form of the condition and hit TWO
occurrences: the production site, and the test's own string literal.
That is the self-reading hazard the neighbouring test documents, showing
up in the mutation harness instead of in the assertion.
The self-read guard is therefore load-bearing and is asserted explicitly:
every literal this test searches for also appears in its own body, so
without cutting the test module off first all three assertions would be
permanently true.
Gates: `cargo fmt --all --check` clean; `cargo clippy -p rustynes-frontend
--all-targets -- -D warnings` clean; the test passes and fails under all
three mutations, with the tree restored byte-identical to its pre-mutation
snapshot afterwards.
No production code changes, so the emulation core is untouched and the
browser behaviour is exactly what the previous commit shipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
|
Worked all four points. One is adopted, one blocking finding is refuted with evidence, two are declined with reasons. Blocking: "silent failure path when
|
| mutation | result |
|---|---|
| reintroduce the wasm-gated auto-open in the ROM-loaded arm | CAUGHT — expected exactly one site opening the browser lobby, left: 2 |
retarget the menu route to a different ToolPanel |
CAUGHT — the wasm OpenPanel(Netplay) route no longer opens the browser lobby |
replace rom_interactive with true on the menu entry |
CAUGHT — the wasm Netplay entry is no longer gated on a loaded ROM |
Two needed reformulating first, and both are this project's recorded traps rather than incidental: the first came back BUILD-FAILED because wasm_lobby is a cfg(wasm32) field, so the mutation had to reproduce the defect as it existed (a mutant that does not compile is not a catch); the second anchored on the single-line form of the condition and matched two occurrences — the production site and the test's own string literal, which is exactly the self-reading hazard the neighbouring test documents, surfacing in the mutation harness instead of in the assertion. The self-read guard is asserted explicitly for that reason.
Comment length (app.rs ~9059, ui_shell.rs ~1537) — declined
House style here is deliberate and written down in AGENTS.md: comments explain the why at the depth of the surrounding code. More specifically, this repository has a standing rule that prose asserting an intent is how a defect survives releases — Pixel Provenance shipped non-functional for four releases under a comment claiming it worked.
The app.rs comment sits at a site where the code is an absence. Without it, the next reader finds a ROM-load path that conspicuously does not open the lobby and no record of whether that is deliberate; "restore the auto-open, the handshake needs the ROM hash" is a plausible and wrong next change, and the comment is what refutes it in place. The ui_shell.rs one records why the browser entry is a separate item with a different label rather than the native one un-gated — a browser cannot open a UDP socket, so these are two transports, not one feature built twice.
Git history is not an equivalent home for either: git blame on a line that does not exist finds nothing.
CHANGELOG tone — declined
The entry follows the format of every entry around it, which is this project's convention rather than this PR's invention.
The browser build stopped opening the Netplay pane at you
On the wasm demo, loading a ROM force-opened the "Netplay (browser)" window every single session. Reported by the maintainer.
Why it did that, and why the reason was half right
The comment at the site gave a genuine reason — the WebRTC handshake keys on the ROM hash, so the lobby has nothing to offer before a ROM exists. That is an argument for enabling a menu entry at ROM load. It is not an argument for opening a window nobody asked for.
The reason it was written that way is the actual defect: the Netplay menu item was
#[cfg(not(target_arch = "wasm32"))], so the browser had no Netplay entry at all. Force-opening was the only way a wasm user could ever reach the lobby.Three changes
app.rsno longer setswasm_lobby.openonRomLoaded.WasmLobbyState::default()was alreadyfalse; that one assignment was the entire behaviour. It is now the only site in the crate that touches the flag — the menu.ui_shell.rsgains a wasm entry in the same menu group as the native one, sameglyph::WIFI, labelled "Netplay (browser)..." after the window it opens. A browser cannot open a UDP socket, so these are two transports rather than one feature built twice, and the label says which you are getting. Gated onrom_interactive— the predicate that group already uses — so it lights up once a ROM is loaded.MenuAction::OpenPanel(ToolPanel::Netplay)routes to the lobby on wasm instead of tonetplay_panel, which renders only a "this UDP netplay panel is native-only" note there. Routing to it would have offered the user a window that says it does not work. The lobby is anApp-owned window rather than aToolPaneland draws inside the overlay's egui frame, so the overlay is forced visible exactly asopen_chip_panelalready does for chip panels.Native behaviour is unchanged — the native entry, its
replay_lockedgating and the UDP panel are untouched.Verified
Both wasm clippy gates (
wasm-winitdefault andwasm-canvas), the native workspace gate,cargo fmt, and markdownlint.grepconfirms exactly one site now opens the lobby and the default isfalse.The Pages deploy on merge is what puts it in front of a browser; I have not clicked it in one.
🤖 Generated with Claude Code
https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj