From e4f99173124cea1e3997b6c374c27edd1a07386a Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 21 Sep 2026 16:01:54 +0700 Subject: [PATCH] docs(plans): whole-project server, web and UX reviews --- ...-260921-1529-server-architecture-review.md | 429 +++++++++++++++ ...wer-260921-1529-web-architecture-review.md | 505 ++++++++++++++++++ ...signer-260921-1529-whole-game-ux-review.md | 203 +++++++ 3 files changed, 1137 insertions(+) create mode 100644 plans/reports/code-reviewer-260921-1529-server-architecture-review.md create mode 100644 plans/reports/code-reviewer-260921-1529-web-architecture-review.md create mode 100644 plans/reports/ui-ux-designer-260921-1529-whole-game-ux-review.md diff --git a/plans/reports/code-reviewer-260921-1529-server-architecture-review.md b/plans/reports/code-reviewer-260921-1529-server-architecture-review.md new file mode 100644 index 0000000..9baa543 --- /dev/null +++ b/plans/reports/code-reviewer-260921-1529-server-architecture-review.md @@ -0,0 +1,429 @@ +# Server architecture review — noitu Go server + +Branch `dev` @ 5178a97 · 2026-09-21 · read-only · scope `server/`, `proto/`, `Makefile`, `Dockerfile`, `.github/` + +## Verdict + +The server is in good shape and the design is sound. `go vet`, `golangci-lint` +(defaults) and `go test ./... -race -count=1` are all clean; coverage is +88–100% everywhere except `cmd/noitu-server` at **0.0%**. No data race, no +goroutine leak (`TestGoroutinesReturnToBaseline`), no auth bypass, no injection +surface, no PII in logs. + +The one-goroutine-per-room actor with a single `inputs chan any` is **the right +shape and should stay**. `room.go`'s 1858 lines are a *file* problem, not an +architecture problem: every method is on `*room`, called only from `run()`, so +the engine-ownership invariant holds by construction and a pure file split +preserves it at zero risk. Do not convert it to typed handlers or a state +machine, and do not split lobby from game — see §1 for why each would cost more +than it buys. + +Real defects are small and few. The most valuable work is not defect-fixing: it +is (a) the per-IP join limiter, which is simultaneously the CI flake and a +CGNAT availability bug, (b) removing `State.History` from the hot broadcast +path, (c) splitting the four oversized files, (d) closing the 0% coverage hole. + +## Top 10 ranked actions + +| # | Action | Kind | Size | Risk | Why now | +|---|---|---|---|---|---| +| 1 | Per-IP join limiter is shared by every client behind one NAT egress (1/s, burst 5) — raise it, key it better, or make it configurable | fix | S | low | Explains today's CI flake **and** breaks a whole café/CGNAT of real players (§4 C2, §5) | +| 2 | Add a per-IP concurrent-connection cap (F6) | fix | S | low | One host can hold all 2000 sockets; the only remaining unbounded-per-peer resource | +| 3 | Split `room.go` into 7 files along existing seams | refactor | M | ~nil | 1858 lines is past the point where a reviewer can hold it; pure moves, invariants preserved by construction (§1) | +| 4 | Drop `History` from `game.State`; add `LastMove()` + `UsedWords()` | refactor | S | low | O(chain) copy on **every** TurnUpdate and every bot move, for two call sites (§2) | +| 5 | Refuse `StartGame` / quick-match auto-start while draining (F7) | fix | S | low | `beginGame` can raise `liveGames` after the drain decision; drain then waits its full timeout (§4 C3) | +| 6 | Guard `attach` against a session already torn down | fix | S | low | Quick match can auto-start a game against a dead socket, burning a 30 s turn clock in front of a real player (§4 C1) | +| 7 | `cmd/noitu-server` at 0% — add `main_test.go` for the env parsers + drain loop | fix | S | nil | Only real coverage hole; all pure functions; config parsing is where a prod misconfig hides | +| 8 | Split `wsapi_test.go` (2534 lines, 75 tests) by topic; delete the 3-line `hub_test.go` | refactor | M | nil | Mirrors #3; a test file nobody can navigate stops being extended (§5) | +| 9 | Split `session.go` into socket half / protocol half | refactor | S | nil | Two unrelated concerns in one 746-line file (§1) | +| 10 | Dockerfile: `golang:1.25-alpine` → `golang:1-alpine`, `alpine:3.22` → `alpine:3` | fix | S | low | House rule is moving-major; dependabot is currently generating churn the rule exists to avoid (§6) | + +Deliberately **not** in the list, with reasons in-section: typed handler +dispatch (§1), lobby/game actor split (§1), `Engine` API consolidation (§2), +bot `Board` changes (§2), committing word data (§3). + +--- + +## 1. Structural assessment + +### `wsapi/room.go` — keep the actor, split the file + +The actor is doing real work that a redesign would have to re-earn: + +- `room.go:387` `run()` is the only place the engine is touched; every handler + is a `*room` method reachable only from the type switch at `room.go:474-511`. +- `room.go:540-543` — one `RoomState` per input, driven by the `lobbyChanged` + flag, from the single place that knows the input finished. No handler has to + remember to broadcast. +- `room.go:498-511` — timers are recomputed after *every* input rather than per + arm, so a new input type cannot silently forget one. +- `room.go:362` `send()` never blocks a session goroutine, and checks + `ctx.Done()` on its own before the `select` so a dead room cannot report a + false success. + +**Proposal (recommended): 7 files, pure moves, no signature changes.** + +| File | Contents | ~LOC | +|---|---|---| +| `room.go` | struct, `newRoom`, `send`, `run` + timer closures, `inLobby`/`occupied`/`freeSeat`/`seatedCount`/`allConnected`/`seatOf`/`sendTo`/`broadcastError` | 350 | +| `room_inputs.go` | every `*Input` struct + `lobbyAction` consts (`room.go:94-204`) | 120 | +| `room_lobby.go` | `handleCreate`, `handleJoin`, `handleLobby`, `canStart`, `guestsReady`, `takenNicknames`, `vacate`, `promote`, `playerSlots`, `broadcastRoomState`, `seatIDs` | 380 | +| `room_game.go` | `beginGame`, `handleSubmit`, `handleResign`, `handleClaimDeadEnd`, `handleBotMove`, `maybeScheduleBot`, `mark`/`inputMark`, `applyEliminations`, `broadcastTurn`/`sendTurnUpdate`/`sendGameStarted`/`broadcastElimination`/`broadcastGameOver`, `scoreRows`, `wireEndReason`, `nearMissFor`, `recordRejection`, `handleReportWord` | 420 | +| `room_presence.go` | `handleDisconnect`, `nextGraceExpiry`, `handleGraceExpiry`, `eliminateAbsent`, `handleResume`, `detachAll` | 200 | +| `room_chat.go` | `chatEntry`, `handleChat`, `sendChatHistory`, `chatMessageFor` | 150 | +| `bot_board.go` | `frozenBoard`, `freezeBoard` (`room.go:1824-1858`) | 50 | + +Invariants: **all preserved, by construction.** Same package, same receiver, +same call graph — the engine stays on the room goroutine because nothing else +can reach a `*room` method; the one-broadcast-per-input site does not move; the +per-recipient rendering loops are untouched. Risk is limited to a bad +copy-paste, which `go build` catches. + +**Rejected — typed handlers (`interface{ apply(*room) }` or a handler map).** +The type switch is 30 lines and is the *only* dispatch in the package. Moving it +onto the input types scatters the two exceptions that currently read at a glance +(`chatInput` and `reportWordInput` set `idleActivity = false`, `room.go:489`, +`room.go:497`) into the types themselves, and buys no exhaustiveness — Go gives +none for either shape. Revisit only past ~20 arms. + +**Rejected — a phase state machine.** The lobby/playing/over phase is already +derived in exactly one place (`room.go:1595` `inLobby()`), and every handler's +guard asks a *different* question — seat ownership (`occupies`), turn ownership, +owner role, readiness. A phase enum answers none of them and adds a field that +can disagree with `r.engine`. + +**Rejected — separate lobby and game actors.** The state that spans both is +exactly the state that makes it hard: `seats` (with `wins`, `chatFrom`, +`graceUntil`), `chat`, `chatSeq`, `owner`, `turnSeq`. Two actors would need a +handoff protocol for all of it and would reintroduce the race the single +goroutine removes. `seat.wins` (`room.go:222-226`) and the chat log explicitly +outlive games; `handleGraceExpiry` (`room.go:1402`) must work identically in and +out of a game. + +### `wsapi/hub.go` (402) — one split worth doing + +Size is fine and the mutex discipline is right (registries only, never engine +state — `hub.go:46-51`). One concern has its own policy, its own metrics and the +most obvious growth path: extract `quickmatch.go` (~90 lines: `waiting`, +`quickMatch`, `cancelQuickMatch`, `errAlreadyQueued`). While there, decide +whether the queue needs `hub.mu` at all — it is never mutated atomically with +`rooms` or `sessions`, so its own mutex would document that. Optional second +split: `roomcode.go` for `roomCodeAlphabet`/`randomCode`/`reserveCode` (~60). + +### `wsapi/session.go` (746) — split by concern, keep the switch + +Two unrelated halves: the socket (`run`, `readLoop`, `writeLoop`, `write`, +`drain`, `keepalive`, `close`, `send`, `trySend`, `attach`/`release` — ~330) and +the protocol (`dispatch` and its handlers, `handleHello`, `resumeFrom`, +`handleSubmit`, `handleReportWord`, `toRoom`, `roomCreateError` — ~350). Split +into `session.go` and `dispatch.go`. + +The 16-arm `switch` at `session.go:441` is fine as a switch — it is the wire +contract read top to bottom, and each arm's error code is deliberately +different. The duplication worth removing is smaller: `Resign` +(`session.go:513`), `ClaimDeadEnd` (`session.go:524`) and `SendChat` +(`session.go:560`) each re-implement `toRoom`'s currentRoom-then-send shape with +their own codes. One helper taking `(limiter, input, missingCode, droppedCode)` +collapses ~30 lines and turns "which limiter pays for which action" into a +table. Do it with #9, not before. + +**Missing arm:** `dispatch` has no `default`. A `ClientMessage` with an unset or +future payload is silently accepted and ignored. Add +`default: s.send(errorMsg("unknown_message"))` plus a counter — see §4 C5. + +--- + +## 2. Engine and bot + +**`State.History` is copied on every broadcast for two readers.** +`engine.go:576` does `append([]Move{}, e.history...)` inside `Snapshot()`, and +`Snapshot()` is called at `room.go:908` (beginGame), `room.go:1142` +(broadcastTurn — **every turn**), `room.go:1207` (applyEliminations) and +`room.go:1831` (freezeBoard — **every bot move**). `History` is read at exactly +two of those: `room.go:1487-1488` wants only the last move, and `room.go:1836` +wants only the played-word set. + +Fix: drop `History` from `State`; add `Engine.LastMove() (Move, bool)` and +`Engine.UsedWords() iter.Seq[string]` (or a copied map). This removes an +O(chain) allocation from the hot path entirely. `State` is internal and `wsapi` +is its only non-test consumer, so the blast radius is `engine_test.go` + +`wsapi_test.go` assertions. Size S. + +**`Standings()` runs on every `Snapshot()`** (`engine.go:587`) even mid-game, +where its own doc says it is meaningless (`engine.go:539-540`). Guard it: +`if e.over`. Trivial, same commit as above. + +**`Engine` API surface: leave it.** `Submit`/`Timeout`/`NoMove`/`Resign` are +four genuinely different rules, not four spellings of one, and the caller-side +pairing with `mark()`/`applyEliminations` is already centralized in exactly one +function (`room.go:1203`). `pointsFor` returning `(int, []PointPart)` with +`capParts` trimming from the end (`engine.go:298-322`) is correct and +well-reasoned — a client with no wordlist cannot re-derive the breakdown, so it +has to travel. No change. + +**Rule duplication engine↔room: essentially none.** One near-miss: +`room.go:1053-1056` (`nearMissFor`) re-applies the link and used-word checks +that `Submit` would apply. That is deliberate and commented — offering a +suggestion that would immediately be refused is worse than offering none. Leave. +`handleClaimDeadEnd` (`room.go:641`) calls `HasLegalMove()` then `NoMove()`, +which re-walks; a wasted scan on a rare input. Leave. + +**Bot `Board` (4 methods, `bot/bot.go:45-51`) is minimal and correctly narrow** — +a strategy can read the position and cannot play a move. `freezeBoard` +(`room.go:1830`) is the right boundary: frozen on the room goroutine before the +worker exists, so `Used` cannot race a concurrent resign. Cost is O(chain) per +bot move → O(chain²) per bot game; chains are tens of words, so this is not +worth restructuring. It gets cheaper for free once `UsedWords()` lands. The +worker's leak guard (`room.go:1123-1129`, `select` on `r.ctx.Done()` around the +thinking delay) is correct. No change. + +--- + +## 3. Dictionary + +`Store` is load-once, read-only, never hands out a mutable reference +(`store.go:60-64`), so unsynchronized concurrent use is genuinely safe. The +`validate()` pass (`store.go:289`) checks declared-vs-loaded counts, orphan +meanings, out-degree consistency and dangling aliases — that is the right set, +and it is what makes a truncated file fail at boot rather than mid-game. + +`NearMiss` (`store.go:421`) and its `stripped` index (`store.go:376`) are a +clean addition: built once at `Open`, ambiguity refused rather than guessed, +and `stripDiacritics` correctly folds `đ/Đ` by hand since they have no canonical +decomposition. Cost: one extra map plus one string per word. On a 30 k-word +corpus the `count != 1` rule will suppress most hits (common stems have many +candidates) — worth a metric on suggestion hit rate before tuning it. + +**Builder version drift risk:** `dictionary/store.go:41` +`requiredBuilderVersion = "5"` and `cmd/build-dictionary/main.go:40` +`builderVer = "5"` are two constants in two packages that must agree, with no +compile-time link and (unlike the dump URL, per the `Makefile` comment) I found +no test asserting they do. Either have the builder import the store's constant +or add a one-line test. Size S. + +**Community allowlist overlay — design only, no data.** Plug point is +`store.go`, between `loadAliases` and `buildStrippedIndex`: a `mergeOverlay()` +step, after which `buildStrippedIndex()` and `validate()` run over the *merged* +result unchanged. Shape: + +- `Open(path string, opts ...Option)` with `WithOverlay(path)`. The overlay is a + second database built by the **same** `build-dictionary --words` path, so + there is one schema and one validator. +- Merge rules to settle before writing code: (a) an overlay word colliding with + a canonical word is a no-op, not an error; (b) `outDegree` and `openers` must + be recomputed after merge, not merged — merging them is what would break the + `validate()` out-degree invariant; (c) a *suppression* list needs its own + table, because removing a word can orphan an `opener` and lower an out-degree + below `minOpeningOutDegree`. +- Licensing: the overlay is a **separate artifact with its own LICENSE and + ATTRIBUTION**, copied as its own layer exactly the way `Dockerfile:74-77` + already does for `data/`. The licence question is open and out of scope here; + no word data should be committed to this repo either way. + +--- + +## 4. Correctness and robustness sweep, ranked + +**Confirmed (path traced)** + +- **C1 · medium · ghost seat from an attach-after-teardown race.** + `session.run` defers `leaveRoom()` (`session.go:280`), which sends + `disconnectInput` only if `currentRoom()` is non-nil. A session that dies + between `hub.createRoom`/`quickMatch` handing the room an input and the room + goroutine draining it is never in a room at that moment, so no disconnect is + ever sent — and then `handleCreate`/`handleJoin` calls `attach` on it + (`room.go:578`, `room.go:715`), leaving a seat with a non-nil dead `sess`. No + grace window opens (that needs `disconnectInput`), so `allConnected()` reports + true. Worst case is quick match: `handleJoin`'s auto-start (`room.go:726`) + gates on `allConnected()`, so a real player is auto-started into a game + against a dead socket and waits out the 30 s turn clock. Fix: check + `m.sess.ctx.Err() != nil` at the top of `handleCreate`/`handleJoin` and refuse + (for join) or `r.cancel()` (for create). Bounded elsewhere: a PvP lobby closes + on the 10-min idle timer, a bot room on the turn clock. +- **C2 · medium · per-IP join limiter is far too tight for shared egress.** + `joinsPerSecond = 1, joinBurst = 5` (`session.go:50-51`), keyed on + `clientIP` (`session.go:475`). Every player behind one NAT/CGNAT egress — a + café, a school, a Vietnamese mobile carrier — shares five room joins and then + one per second, refused with `too_many_attempts`. This is also the CI flake + (§5). The limiter is right in kind (it protects the 6-char room-code secret + from brute force at `31^6 ≈ 8.9e8`); the *rate* is wrong. At 5/s it still + takes centuries to walk the space. Raise the burst and rate substantially, and + make them configurable so the e2e suite and a CGNAT deployment can tune them. +- **C3 · low-medium · F7, games can start after the drain decision.** + `beginGame` (`room.go:855`) has no `isDraining()` check, so `hub.gameStarted()` + (`room.go:907`) can raise `liveGames` after `startDraining()`. `newRegisteredRoom` + refuses *new rooms* (`hub.go:261`) but an existing lobby is untouched. + Consequence is bounded by `NOITU_DRAIN_TIMEOUT` — the drain waits its full + timeout instead of exiting early — and the game is then killed mid-play + anyway, which is the outcome the drain existed to avoid. Fix: in + `handleLobby`'s `lobbyStart` arm and in `handleJoin`'s auto-start, refuse with + `server_restarting` when `r.hub.isDraining()`. +- **C4 · low · player-driven log volume.** `recordRejection` (`room.go:1021`) + emits `slog.Info("word_rejected", …)` for *every* rejected submission, up to + 5/s per connection. The corpus feedback loop it serves is real, but + `noitu_words_rejected` already carries the reason breakdown. Sample it, or + drop the line to Debug and keep the counter. The word is normalized and capped + before logging and no player identity is attached — that part is right. +- **C5 · low · silent unknown-payload handling.** No `default` in `dispatch` + (`session.go:441-575`). A future or malformed `ClientMessage` costs a decode + and gets no answer. Add the arm plus a counter; the frame limiter + (`session.go:328`) already bounds the flood case. +- **C6 · low · `Config.IdleFor` is a dead knob in production.** + `server.go:27` is documented as operator-facing but `cmd/noitu-server` + never sets it (`main.go:88-98`) and no `NOITU_*` variable exists — `docs/` + and `README` correctly do not claim one. Rooms are fixed at + `defaultIdleWindow` (10 min, `room.go:83`). Either wire `NOITU_IDLE_TIMEOUT` + and document it, or delete the field and let the constant be the only source. +- **C7 · low · `/debug/vars` publishes `cmdline` and `memstats`.** + `expvar.Handler()` (`main.go:166`) always exposes the full argv and heap + stats. It is opt-in and on a separate listener (`main.go:158-162`), which is + the right design — but `docs/deployment.md` should say explicitly: never bind + `NOITU_DEBUG_ADDR` to a public interface. +- **C8 · informational · resume token has no liveness check on the prior + connection.** `handleResume` (`room.go:1448`) accepts the takeover even when + `s.sess != nil`, then closes the old connection (`room.go:1471`). With the + token in `localStorage`, a second tab evicts the first. That reads as a + deliberate "one seat, latest tab wins" — record it as a decision rather than + fix it. The token itself is 128 bits from `crypto/rand` (`session.go:742`) and + is registered/expired correctly (`hub.go:122`), so it is not guessable. + +**Plausible (not traced to a failure)** + +- **C9 · `reserveCode` before the ceiling check.** `newRegisteredRoom` + (`hub.go:268`) draws a code — up to 10 `crypto/rand` rounds and 10 lock + acquisitions (`hub.go:332-341`) — before checking `maxRooms` at + `hub.go:277`. At the ceiling every refused create pays that. Cheap reorder: + an advisory count check first, keeping the authoritative one under the + registration lock where it correctly is today. + +**Checked and clean** + +Trust boundaries: `sanitizeText` is applied to nicknames, chat **and** the +echoed `typed` word (`room.go:966`, `nickname.go:52`), with NFC first so the +rune cap sees what the browser renders — correct order. `clientIP` +(`server.go:242`) walks XFF from the right and only when the peer is a +configured proxy; malformed hops fall back to the socket address rather than +keying a limiter on garbage. Static file serving is root-confined +(`server.go:225` `underRoot`). Seat authority is always the seat, never the +claimed id (`room.go:849` `occupies`, used by every handler that acts on a +game). `liveGames` accounting is exactly-once via `CompareAndSwap` on both the +normal path (`room.go:1289`) and the cancelled-mid-game path (`room.go:392`). +Limiter maps are swept (`ratelimit.go:81`, `server.go:316`). `errorMsg` carries +UI keys, never prose or internal errors — asserted by +`TestErrorMessagesAreUIKeysNotProse`. + +--- + +## 5. Tests + +**Structure.** 13 files, 143 tests. `wsapi_test.go` alone is 2534 lines / 75 +tests spanning ~9 topics; `hub_test.go` is 3 lines of comment and should be +deleted (fold the note into `hub.go`). Split `wsapi_test.go` along the §1 +seams — `lobby_test.go`, `chat_test.go`, `resume_test.go`, `deadend_test.go`, +`report_test.go`, `ratelimit_test.go`, `nickname_test.go` (already exists) — +keeping `wsapi_test.go` as the harness only (`newTestServer`, client helpers, +fixture dictionaries, ~350 lines). + +**The browser-suite join flake — answer: no, there is no server-side ordering +that makes `can_start` late.** Traced: `SetReady` → `toRoom` +(`session.go:603`) → `handleLobby` sets `mine.ready` (`room.go:764-766`) → the +run loop broadcasts exactly one `RoomState` in the *same* iteration +(`room.go:540-543`), with `can_start` computed in that same frame +(`room.go:1755`, `room.go:1671`). One RTT, no timer, no second input. The client +derives both `isReady` and `canStart` straight from that frame +(`game.svelte.js:537`, `Lobby.svelte:211`), so there is no local optimism to +drift either. + +What can make the frame **missing** rather than late, in likelihood order: + +1. **The join was refused and the test never noticed.** `playingPair` + (`pvp-game.spec.js:74-78`) uses `joinRoom`, **not** `joinRoomSeated` — it + clicks "Vào phòng" and goes straight to `readyAndStart` with no assertion + that the seat exists. The suite runs `workers: 1` against one server process + with every page on `127.0.0.1`, so **all pages share one join bucket** + (C2: 1/s, burst 5). A test that joins twice in quick succession after a + previous test drained the bucket gets `too_many_attempts`, the guest never + reaches the lobby, the `ready` click is a no-op, and the failure surfaces + 10 s later on the owner's `start-game` — exactly the observed symptom at + `helpers.js:130`. +2. `Lobby.svelte:211` also gates on `offline`; a socket blip keeps the button + disabled independently of `can_start`. + +Fixes, in order: raise/expose the join limiter (C2, which is a production fix +first and a test fix second); use `joinRoomSeated` in `playingPair`; and have +`readyAndStart` assert each guest's own ready state landed before waiting on the +owner's button, so a dropped `SetReady` fails at the guest with a clear message +instead of timing out on a third party. + +**What to move from Playwright into Go.** The server-rule assertions in +`pvp-game.spec.js` already have named in-process equivalents — +`TestStartIsRefusedUntilTheGuestIsReady` (wsapi_test.go:1332), +`TestOnlyTheOwnerStartsAndKicks` (:1373), `TestKickFreesAnUnreadySeatOnly` +(:1515), `TestOwnerLeavingPromotesTheOtherPlayer` (:1548), +`TestResumeWithinGraceRestoresGame` (:607). The browser suite should keep only +what a browser proves: rendering, focus, the unread badge, deep links, the +bundle carrying no wordlist. Concrete task: walk `pvp-game.spec.js` and delete +every assertion whose rule is already named by one of those tests. + +**Coverage floors.** Current: `vietnamese` 100, `game` 95.5, `bot` 91.2, +`wsapi` 91.1, `dictionary` 89.0, `build-dictionary` 88.6, **`cmd/noitu-server` +0.0**. Propose an 85% per-package floor in CI (`go test -coverprofile` + a +small check script), excluding `gen/`. That floor is already met everywhere +except the one package that has never been tested — `loadConfig`, `envInt`, +`envDuration`, `envNonNegDuration`, `envList` and `waitForGamesToFinish` are all +pure and all trivially testable, and config parsing is precisely where a +production misconfiguration hides. + +**Fuzz.** Three targets today: `FuzzNormalize`, `FuzzSanitizeText`, +`FuzzDecode` — all at the right boundaries. The gap is the *composition*: add +`FuzzSubmit` over the fixture dictionary asserting the property that any input +`Engine.Submit` accepts resolves to a canonical word whose `FirstSyllable` +equals the link and whose `LastSyllable` becomes the next link. A second cheap +one: `FuzzStripDiacritics` for idempotence +(`strip(strip(x)) == strip(x)`), which is what `NearMiss` silently relies on. + +--- + +## 6. Build and ops + +- **Dockerfile version pins.** `Dockerfile:19` `golang:1.25-alpine` and + `Dockerfile:37` `alpine:3.22` are exact-minor pins; the house rule prefers a + moving major, and `node:24-alpine` (`:4`) and + `distroless/static-debian12:nonroot` already follow it. As written, the + dependabot `docker` ecosystem generates exactly the churn the rule exists to + avoid. Change to `golang:1-alpine` and `alpine:3`, or record in the README why + these two are pinned harder. `go.mod` says `go 1.25.0`, so the Go pin is + currently *consistent* — just needlessly tight. +- **`make test-go` does not mirror CI.** It runs `go vet && go test -race` + (`Makefile`), while CI additionally enforces `gofmt -l` and + `golangci-lint` (`ci.yml`). A contributor can pass `make test` and fail CI. + Add a `lint-go` target running both, and call it from `test-go`. +- **Duplicated vet.** `golangci-lint` runs `govet` by default, so the separate + `go vet` step in `ci.yml` is redundant. It is ~free and it fails faster with a + clearer message — leave it. +- **Everything else is clean.** The image job's licence assertion (`ci.yml`, + "The licence travels with the data") is a genuinely good check, as is the + negative assertion that the upstream dump never reaches the final image. The + `proto.yml` breaking-change gate with `--against '.git#ref=origin/main'` and + the generated-tree sync check (with `--intent-to-add` for newly emitted files) + are both correct. `dependabot.yml` covers all four manifests with sensible + grouping. The `build-dictionary` binary is built once in the `build` stage and + copied into `dict` — no duplicated toolchain. Nothing stale found. + +--- + +## Unresolved questions + +1. **C2 rate.** What join rate and burst do you want? The security argument + caps nothing below ~5/s; the availability argument (Vietnamese mobile CGNAT) + wants much more. This is a product call, not a review call — I have not + changed it. +2. **C8 two-tab takeover.** Is "second tab evicts the first" the intended + behaviour? If yes it should be a line in `docs/`; if no it needs a liveness + check in `handleResume`. +3. **C6 `IdleFor`.** Wire it as `NOITU_IDLE_TIMEOUT`, or delete the field? +4. **Coverage floor.** Is 85% the number you want, and should `cmd/` be held to + it or exempted with a documented reason? +5. **Overlay (§3).** Does the suppression case (removing a shipped word) need to + exist at all in v1? It is the only part of the overlay design that touches + `openers` and `outDegree` and therefore the only part that is not additive. 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 `