diff --git a/plans/reports/code-reviewer-260921-1529-web-architecture-review.md b/plans/reports/code-reviewer-260921-1529-web-architecture-review.md new file mode 100644 index 0000000..1e20120 --- /dev/null +++ b/plans/reports/code-reviewer-260921-1529-web-architecture-review.md @@ -0,0 +1,505 @@ +# Web frontend — whole-project architecture review + +Branch `dev` @ 5178a97 · 2026-09-21 · scope `/workspace/tiennm99/noitu/web` (src 4,392 LOC, tests 2,537, e2e 1,472) + +## Checks run (read-only) + +| Command | Result | +|---|---| +| `npm run lint` | 0 errors, **33 warnings** (32 × `jsdoc/reject-any-type`, 1 × `check-param-names` in `e2e/helpers.js:123`) | +| `npm run check` | 380 files, **0 errors, 0 warnings** — see A4: this number is mostly meaningless today | +| `npm test` | build OK, **221 passed / 12 files**, 3.8s | +| Playwright | not run (no browser on this host, per workspace rules) | + +## Verdict + +Structurally sound and unusually well-reasoned — the "store is a projection" invariant holds everywhere I +checked, the uncontrolled-input invariant is respected, and the comments explain *why* rather than *what*. +Three things are genuinely wrong and one of them is a stuck-UI dead end reachable after any deploy. The +bigger problem is not a defect: **`svelte-check`'s clean run is an illusion** — `initialState()` returns +`any` (`stores/game.svelte.js:54`), so every `game.state.*` read in every component is unchecked. Fix that +before any refactor, or the refactor lands blind. + +Do not slice the store into per-domain stores. Do extract the page's request state machine. + +## Top 10 ranked actions + +| # | Action | Kind | Size | Risk | Why now | +|---|---|---|---|---|---| +| 1 | Time-box the resume latch; a stale token gets **silence** from the server, not an error → `?code=` + stale token = permanently disabled join form | fix | S | L | Confirmed against `server/internal/wsapi/session.go:643` + `hub.go:127`. Reachable after every deploy | +| 2 | `leave()` omits `forgetSession()` → next reload resumes into the room just left | fix | S | L | Confirmed; one-line asymmetry vs. the page-teardown path | +| 3 | Type `GameState`; delete `@returns {any}` on `initialState()` | fix | M | M | Unblocks every other item. Expect real errors to surface | +| 4 | Extract `stores/room-session.svelte.js` (join/resume/quick-match machine) from `online/+page.svelte` | refactor | M | M | Removes 5 of 7 `$effect`s; makes #1 unit-testable; `bot-session` is the precedent | +| 5 | Type the wire from `game_pb.d.ts`; `switch (payload.case)` for oneof narrowing | refactor | M | L | Clears 18 src lint warnings; makes the oneof exhaustive at build time | +| 6 | Fire-and-forget sends (`cancelQueue`, `leave`, lobby `report`) silently drop requests | fix | S | L | Best explanation for the `toBeEnabled` flake; user-visible dead buttons | +| 7 | Split `game.svelte.js` → `game-shape.js` / `game-apply.js` / store; wrap `apply` in try/catch | refactor | M | L | A throw mid-`apply` leaves a half-applied snapshot on screen | +| 8 | Component tests under jsdom via `mount()`; then cut Playwright 47 → ~12 | refactor | M | L | jsdom is already a devDependency; today 0 component tests exist | +| 9 | `ArmedButton.svelte` (3 duplicated arm/disarm blocks) + announce the armed state | refactor | S | L | DRY + the only a11y gap that loses information | +| 10 | `{#each entry.meanings as sense (sense.gloss)}` — duplicate gloss = Svelte duplicate-key throw | fix | S | L | Dictionary data is not guaranteed gloss-unique | + +**Leave alone:** per-domain store slices (§1), `vi.js` namespacing (§5), manual chunking / font strategy (§4), +the uncontrolled-field design (§3), the `Set` + eslint-disable in `reset()` (dissolves under #3). + +--- + +## 1. Structure + +### 1.1 `routes/online/+page.svelte` (757) — extract the request machine + +Seven `$effect`s, five of which are one state machine wearing a costume: `pending` (48), `resuming` (121), +`needName` (125), `stalled` (126), `queuedForS` (56), and the latch-clearing effect at 137-143, the flush at +216-219, the queue timer at 225-233, the stall timer at 239-247, the resume-failure handler at 253-268. + +**Proposal** (mirrors `stores/bot-session.svelte.js` exactly): + +- `lib/stores/room-session.svelte.js` (~130 LOC, no DOM, no runes beyond `$state`) — owns `pending`, + `resuming`, `needName`, `stalled`, `waitedS`; exposes `request(req)`, `flush(isOpen)`, `noteRoom()`, + `noteError(code)`, `noteResumeTimeout()`, `leave()`. Unit-testable in Vitest with a fake clock, exactly as + `ws/client.js` already is (630 lines of tests prove the pattern works). +- `lib/components/JoinPanel.svelte` (~160) — the `{:else}` branch at 465-547: nickname, quick match, create, + join form, `needName` / `error` / `stalled` notices. +- `lib/components/RoomLayout.svelte` (~90) — the two-column shell (408-464) plus `wide` (85), `chatFolded` + (103), `chatUnread` (104), `talkPane` (106) and the media-query effect (87-94). +- Page drops to ~130: store wiring + the 11 one-line message senders (349-401). + +**Invariant impact: none.** `room-session` holds *client intent* (what the player asked for), never server +state. `game` stays the sole projection. This is the boundary `bot-session.svelte.js:1-12` already argues for +in prose. + +**Size M, risk M** — the risk is entirely in the teardown effect (185-212), which is load-bearing for seat +release. Port it verbatim; cover it with the existing `pvp-game.spec.js:235` spec before and after. + +Secondary: the teardown at 185 is coupled to `inviteCode` (`$derived` on `page.url`, 128). Any future +in-app URL mutation on `/online` — a `replaceState` to drop the used `?code=`, say — would fire a full +leave-room-and-disconnect. Move teardown to `onDestroy` so it is not a reactive dependency of a query +parameter. + +### 1.2 `stores/game.svelte.js` (646) — split the file, keep one state object + +**Do not make this four stores with a dispatcher.** Three reasons: + +1. The store's own comment at 300-302 states the failure mode a slice design invites: *"Merging fields + selectively is how a client ends up believing a mixture of two states the server was never in."* Four + reducers each handling part of a `RoomState` is precisely that, with the atomicity now spread across + module boundaries. +2. Components read across the proposed domains. `ScoreBoard.svelte:15-17` reads `gamePlayers` + + `standings` + `roomPlayers` + `nickname`; `nameOf()` (593-601) falls back across game → room; `myScore` + (566-569) picks its table by `phase`. +3. There is no performance motive. 61 store tests run in 36ms. + +**Proposal — mechanical file split, one `$state`:** + +- `stores/game-shape.js` (~210) — the `ChainEntry`/`Sense`/`PointPart`/`PlayerSlot`/`PlayerScore` typedefs, + the new `GameState` typedef (#3), `initialState()`, and the three wire decoders `toSenses`/`toParts`/ + `toScore` (200-230). Pure, zero reactivity, the natural home for the generated-type imports. +- `stores/game-apply.js` (~230) — `applyTo(state, msg)`: the switch at 286-518 as a pure function over a + plain object. Testable without the runes compiler. +- `stores/game.svelte.js` (~200) — `$state`, `reset`, `leave`, the 12 derived accessors, the singleton. + +**Size M, risk L** (mechanical). Sequence it *after* #3 and #5 so the decoders land typed. + +While splitting, wrap the call site: `apply()` has no error boundary and is invoked from +`ws/client.js:245-246` inside `ws.onmessage`. A throw anywhere in the switch aborts mid-mutation — e.g. +`roomState` sets `queued`/`roomCode`/`canStart` (296-306) *before* mapping `players` (308), so a throw there +leaves a room on screen with no seats. Protobuf-es v2 always materialises repeated fields, so this is +plausible rather than confirmed, but the cost of `try { applyTo(...) } catch { /* report */ }` is one line. + +### 1.3 `components/GameBoard.svelte` (495) + +Two near-identical arm/disarm blocks: `arming`/`armTimer`/`armOrResign` (48-51, 99-109, 124) and +`claimArming`/`claimArmTimer`/`armOrClaim` (52-54, 112-122, 125), plus their disarm-on-turn-loss effects +(74-78, 84-88). `Lobby.svelte:55-81` has a third copy for kick. + +- `components/ArmedButton.svelte` (~45) — props `{ label, confirmLabel, disabled, onconfirm }`; owns the + timer, the disarm-on-disable effect, and (see §5) the announcement the armed state currently lacks. Three + call sites, ~70 LOC deleted. **S / L.** +- `components/BoardHeader.svelte` (~55) — the `top` row at 129-154 (badge, mode label, rules link, chat + pill). Board falls to ~330. + +### 1.4 `components/Lobby.svelte` (467) + +Extract `components/SeatList.svelte` (~140): the `