mirror of
https://github.com/tiennm99/noitu.git
synced 2026-10-15 08:17:12 +00:00
docs(plans): whole-project server, web and UX reviews
This commit is contained in:
tiennm99 committed
1 parent
3ef9f48df0
commit
e4f9917312
3 files changed
+1137
No files matched your search
@@ -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.
|
||||
@@ -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 `<ul class="seats">` at 101-162 plus the kick arming, which
|
||||
becomes an `ArmedButton`. Lobby → ~300. **S / L.** Lowest priority of the four.
|
||||
|
||||
### 1.5 Total
|
||||
|
||||
~700 LOC moved, ~150 new, two new pure modules Vitest can reach. No abstraction without a domain anchor:
|
||||
every extracted unit is an existing repeated pattern (armed button, seat list) or an existing named concept
|
||||
(the room session, the wire decoders).
|
||||
|
||||
---
|
||||
|
||||
## 2. Typing without TypeScript
|
||||
|
||||
18 `any` sites in `src/` (+ 12 in `tests/`+`e2e/`, 9 of which dissolve for free). `game_pb.d.ts` already
|
||||
carries everything needed: `ServerMessage.payload` is a proper discriminated union
|
||||
(`game_pb.d.ts:1185+`), and `moduleResolution: "bundler"` resolves `import('…/game_pb.js').ServerMessage`
|
||||
through the sibling `.d.ts`.
|
||||
|
||||
### Site A — `ws/client.js:100, 269, 297` (the oneof)
|
||||
|
||||
Before:
|
||||
|
||||
```js
|
||||
* @param {(msg: any) => void} options.onMessage
|
||||
…
|
||||
/** @param {any} msg */
|
||||
function intercept(msg) {
|
||||
const payload = msg.payload;
|
||||
if (payload.case === 'welcome') {
|
||||
attempt = 0;
|
||||
if (payload.value.resumeToken) storeToken(payload.value.resumeToken);
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```js
|
||||
/** @typedef {import('$lib/proto/noitu/v1/game_pb.js').ServerMessage} ServerMessage */
|
||||
/** @typedef {import('$lib/proto/noitu/v1/game_pb.js').ClientMessage} ClientMessage */
|
||||
…
|
||||
* @param {(msg: ServerMessage) => void} options.onMessage
|
||||
…
|
||||
/** @param {ServerMessage} msg */
|
||||
function intercept(msg) {
|
||||
const payload = msg.payload; // NOT destructured — see below
|
||||
if (payload.case === 'welcome') {
|
||||
attempt = 0;
|
||||
if (payload.value.resumeToken) storeToken(payload.value.resumeToken); // value: Welcome
|
||||
```
|
||||
|
||||
`payload.case === 'pong'` then narrows `payload.value` to `Pong`, whose `clientTimeMs` is `bigint` — which
|
||||
makes the `Number(...)` at 287-288 a *checked* conversion rather than a hopeful one. `send` (297) takes
|
||||
`ClientMessage`; `messages.js` builders already return it, so no change there. The three timer `any`s
|
||||
(107, 126, 128) become `ReturnType<typeof setTimeout>`, which matches the injected `schedule: typeof
|
||||
setTimeout` exactly.
|
||||
|
||||
### Site B — `stores/game.svelte.js:54` (the one that matters)
|
||||
|
||||
`@returns {any}` on `initialState()` makes `const state = $state(initialState())` `any`, therefore
|
||||
`game.state` is `any`, therefore **every** `game.state.foo` in all 16 components is unchecked. A typo
|
||||
(`chainLenght`) compiles, ships, and renders `undefined`. This is why `svelte-check` reports 0 errors.
|
||||
|
||||
Before:
|
||||
|
||||
```js
|
||||
/** @returns {any} */
|
||||
function initialState() { return { phase: 'idle', chain: [], … }; }
|
||||
…
|
||||
// eslint-disable-next-line svelte/prefer-svelte-reactivity
|
||||
const kept = new Set(['nickname', 'roomCode', …]);
|
||||
function reset() {
|
||||
const fresh = initialState();
|
||||
for (const key of Object.keys(fresh)) {
|
||||
if (kept.has(key)) continue;
|
||||
state[key] = fresh[key]; // untyped index write
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```js
|
||||
/**
|
||||
* @typedef {object} GameState
|
||||
* @property {'idle'|'lobby'|'playing'|'over'} phase
|
||||
* @property {ChainEntry[]} chain
|
||||
* @property {string[]} expanded
|
||||
* …one @property per field, ~30 lines, prose comments unchanged…
|
||||
* @property {string|null} error
|
||||
*/
|
||||
|
||||
/** @returns {GameState} */
|
||||
function initialState() { … }
|
||||
|
||||
/** Fields a game ending does not change: they describe the room, not the game. */
|
||||
const KEPT = /** @type {const} */ ([
|
||||
'nickname', 'roomCode', 'roomPlayers', 'canStart',
|
||||
'maxPlayers', 'minPlayers', 'graceMs', 'chat', 'chatCount'
|
||||
]);
|
||||
|
||||
/**
|
||||
* @template {keyof GameState} K
|
||||
* @param {GameState} into
|
||||
* @param {GameState} from
|
||||
* @param {readonly K[]} keys
|
||||
*/
|
||||
function carry(into, from, keys) {
|
||||
for (const k of keys) into[k] = from[k]; // both sides are GameState[K]
|
||||
}
|
||||
|
||||
function reset() {
|
||||
const fresh = initialState();
|
||||
carry(fresh, state, KEPT);
|
||||
Object.assign(state, fresh);
|
||||
}
|
||||
|
||||
function leave() { Object.assign(state, initialState()); }
|
||||
```
|
||||
|
||||
The generic `carry` typechecks without a suppression, and the `Set` plus its `eslint-disable` disappear.
|
||||
|
||||
### The one non-obvious step
|
||||
|
||||
`apply()` currently destructures: `const { case: kind, value } = msg.payload;` (287). TypeScript **loses
|
||||
discriminated-union narrowing across a destructure**. The switch must become `switch (payload.case)` with
|
||||
`payload.value` read inside each arm. That is the only mechanical change to the 230-line switch; everything
|
||||
else is annotations.
|
||||
|
||||
### Remaining sites
|
||||
|
||||
- `game.svelte.js:200/204/209/213/218/308/471` → the wire types, renamed on import to avoid colliding with
|
||||
the store's own same-named typedefs: `@typedef {import('…').PlayerScore} WirePlayerScore`. The `?? []`
|
||||
guards at 204/213 are dead under protobuf-es v2 (repeated fields always materialise).
|
||||
- `ChainHistory.svelte:115` → `@param {import('$lib/stores/game.svelte.js').PointPart} p` (already exported).
|
||||
- `bot-session.svelte.js:68/74` → widen from `object|null` to `{ myScore: number }|null`, drop the cast.
|
||||
- `connection.svelte.js:36` → `ClientMessage`.
|
||||
- `tests/game-store.test.js:624/635/661` dissolve once `GameState` exists; `tests/ws-client.test.js` (6)
|
||||
needs the same `ServerMessage` typedef; `e2e/helpers.js:123` → `@param {...Page} guests`.
|
||||
|
||||
Net: 33 warnings → 0, and `svelte-check` starts earning its exit code. Budget for a handful of *real* errors
|
||||
surfacing in components on the first run — that is the point.
|
||||
|
||||
---
|
||||
|
||||
## 3. Runtime correctness sweep
|
||||
|
||||
### Confirmed
|
||||
|
||||
**C1 — Resume latch has no timeout; the server answers an unknown token with silence.**
|
||||
`online/+page.svelte:160-173` sets `resuming = true` and, when the URL carries a code, `pending = {kind:
|
||||
'join', code}`. `flush()` refuses to send anything while `resuming` (288). `resuming` is cleared only by
|
||||
arriving in a room (137-143) or by an error (253-268). Server side: `session.go:643`
|
||||
`if prior, ok := s.hub.resumable(token); ok && … { s.resumeFrom(prior) }` — and `hub.go:127-135` returns
|
||||
`ok == false` for an unknown token, with **no message sent**. So a stale token (server restarted, or the
|
||||
session was GC'd after grace) plus an invite link leaves the screen with an OPEN socket, `resuming` stuck
|
||||
true, `pending` stuck set → all three buttons disabled reading "Đang kết nối…" (509/513/538), and the
|
||||
`stalled` banner never fires because it requires `connection.status !== OPEN` (240). Permanent dead end;
|
||||
only a manual reload escapes, which reproduces it.
|
||||
*Fix:* a `RESUME_TIMEOUT_MS` timer armed alongside `resuming`, running the same path the error branch does.
|
||||
Belongs in `room-session` (#4). Secondary: ask the server to answer a non-resumable token explicitly — the
|
||||
`resumeFrom` comment at 651-656 already commits to "every failing branch has to say so", and this branch
|
||||
does not.
|
||||
|
||||
**C2 — `leave()` does not forget the session.** 368-372 sends `LeaveRoom`, wipes the store, clears `pending`
|
||||
— but omits `forgetSession()`, which the page-teardown path at 203 does call. The token stays in
|
||||
sessionStorage, so the next load of `/online` in that tab takes the `hasStoredSession()` branch (160) and
|
||||
tries to resume into the room the player deliberately left. Best case the server refuses and C1's stuck
|
||||
state is one step away; worst case the seat is still live and they are put back.
|
||||
|
||||
**C3 — Fire-and-forget sends.** `send()` returns a boolean everywhere else in this file and is checked at
|
||||
302 and in `Lobby.report()` (62-66). It is *ignored* at `cancelQueue` (328) and `leave` (369). Cancelling a
|
||||
quick match while the socket is reconnecting therefore clears `pending` locally while `game.state.queued`
|
||||
stays true — the waiting panel (490-503) stays up with its elapsed clock running and its Cancel button
|
||||
doing nothing on every subsequent press. Same class as the flake below.
|
||||
|
||||
**C4 — Duplicate-key throw on meanings.** `ChainHistory.svelte:124`
|
||||
`{#each entry.meanings as sense (sense.gloss)}`. Two senses with different `pos` and identical `gloss` are
|
||||
not excluded by the wire contract; Svelte throws `each_key_duplicate`. Key by index — the list is neither
|
||||
reordered nor filtered. Same fragility, lower probability, at `ChainHistory.svelte:68`
|
||||
`{#each entries as entry, index (entry.word)}` (safe only because the server refuses repeats).
|
||||
|
||||
**C5 — The lobby chat never reopens after the first game.** `chatFolded` starts `false` (103) and is set
|
||||
`true` by the effect at 108-110 when `phase === 'playing'`. Nothing ever sets it back. After a game ends the
|
||||
phase goes `over` → (next game) `playing`; it never returns to `lobby` while the player stays in the room
|
||||
(`game.svelte.js:322` only promotes `idle` → `lobby`). So on narrow screens the between-games lobby has a
|
||||
folded chat forever after the first game — the regression that 5178a97's fix does not cover. The pill in the
|
||||
board header keeps it reachable, so this is UX, not a trap. Fix: fold on the `lobby → playing` transition
|
||||
only, or unfold on `over`.
|
||||
|
||||
**C6 — Seeding writes into a field the connection has disabled.** `WordInput.svelte:103-128` depends on
|
||||
`myTurn`/`phase`/`turnSeq`/`currentSyllable`/`rejection` but **not** on `connection.status`, while `enabled`
|
||||
(22-24) does. During a reconnect on the player's own turn the effect still calls `field.focus()` and writes
|
||||
`field.value = "${syllable} "`, after `lockedValue` was captured pre-seed at 70-73. The first composition
|
||||
event then calls `undoInput` (92-95) and yanks the seed back out. Cosmetic, but it makes the seed
|
||||
non-deterministic exactly when the player is anxious. Add `enabled` to the guard at 104.
|
||||
|
||||
**C7 — `console.warn` ships to production.** `game.svelte.js:514`. Intentional per the comment, and I agree
|
||||
with the intent — but it prints internal oneof case names to the console of every player on a version skew.
|
||||
Route it through a one-shot dev-only guard or leave it; low.
|
||||
|
||||
### Plausible
|
||||
|
||||
**P1 — the `toBeEnabled` flake in `e2e/helpers.js:130.** `readyAndStart` clicks the guest's ready button and
|
||||
immediately asserts the owner's Start is enabled (10s `expect` timeout). `Lobby.svelte` line ~213 gates it on
|
||||
`!s.canStart || offline`, both server-owned, so the assertion is correct — but nothing between the click and
|
||||
the assertion proves the `SetReady` *left the client*. `onready` → `report(onready(...))` (224) → `send()`,
|
||||
which returns `false` if the socket is not `readyState === 1` (`client.js:299`) and **nothing retries**:
|
||||
`unsent` (60) draws a banner and the request is gone. Meanwhile the button itself is only disabled on
|
||||
`offline`, which is `connection.status`, and status reaches `OPEN` inside `ws.onopen` — a window exists where
|
||||
Playwright sees an enabled button over a socket in an ambiguous state.
|
||||
*Two fixes, both worth it:* (a) give lobby actions the same held-request latch `pending` already gives
|
||||
join/create (lands free with #4); (b) in the helper, assert the guest's own state before starting —
|
||||
`await expect(guest.getByTestId('my-ready')).toHaveText('Sẵn sàng')` — which converts a mystery timeout into
|
||||
a precise failure on the right page.
|
||||
|
||||
**P2 — route-swap socket race.** `/online` teardown calls `disconnect()` (209) which nulls the module
|
||||
singleton; `/play` mount calls `connect()` (69). If SvelteKit ever creates the new page component before
|
||||
destroying the old one, `connect()` runs first and `disconnect()` then kills the socket the new screen owns,
|
||||
leaving `/play` with `pending` set and no transport and no retry (`session.flush` is only driven by a status
|
||||
*change*, `play/+page.svelte:49-52`). I could not prove the ordering without a browser. A `connect()` that is
|
||||
idempotent-by-generation, or an owner token on the singleton, removes the question.
|
||||
|
||||
**P3 — resume-failure handler swallows unrelated errors.** `online/+page.svelte:253-268` treats *any*
|
||||
`game.state.error` arriving while `resuming` as a failed resume: it clears the error silently and spends the
|
||||
held code. A `server_full` or `too_fast` landing in that window is erased with no trace. Narrow it to the
|
||||
codes the resume path can actually produce (`game_already_over`).
|
||||
|
||||
### Verified correct (do not "fix")
|
||||
|
||||
- **Countdown vs. server clock** — `countdown.js`, the midpoint offset estimate (`client.js:293`), and the
|
||||
rAF loop gated on `running` (`CountdownRing.svelte:30-37`), `SETTLE_MS` erring early included.
|
||||
- **Chat unread accounting** — the two effects at `ChatPanel.svelte:64-66` and 83-86 are correct under fold,
|
||||
resync (`count < seenAt`), the `CHAT_WINDOW` cap (counted against `chatCount`, not `chat.length`), and the
|
||||
single-mount-across-phases arrangement the page comment at 417-422 depends on.
|
||||
- **Storage failures** — `settings.svelte.js:389-421` and `client.js:68-90` guard the property access itself,
|
||||
not just the call; `readBestScores` (428-445) validates shape and value; `app.html`'s pre-paint theme
|
||||
script has its own `try`.
|
||||
- **Uncontrolled-input invariant** — `beforeinput` guard + `input` undo + composition flags
|
||||
(`WordInput.svelte:83-95, 205-208`) and the three direct-write exceptions (seed 124, suggestion 164, clear
|
||||
146) are each correct at their call site. Only C6 breaks the pattern.
|
||||
- **`untrack` usage** — load-bearing at `+page.svelte` 139/150/218/255 and `play/+page.svelte` 37/51/58; each
|
||||
prevents a self-retriggering effect, none hides a dependency that should be tracked.
|
||||
- **`PlayerStatus.svelte:36-48`** grace bookkeeping handles the same-length-different-ids case correctly.
|
||||
|
||||
---
|
||||
|
||||
## 4. Performance and bundle
|
||||
|
||||
Client build (`vite build`): largest chunk 83.13 kB / 25.98 kB gz (Svelte runtime + `@bufbuild/protobuf`),
|
||||
then 33.37 / 26.18 / 21.91 kB; `/online` route node 20.61 kB; CSS 13.61 kB (GameOverPanel) + 9.70 kB
|
||||
(online route) + 3.98 kB (layout). Whole build comfortably under the 400 kB budget.
|
||||
|
||||
**`tests/bundle.test.js` guards, well:** dictionary words absent *after unicode-escape decoding*; no
|
||||
`.db/.sqlite/.csv/.tsv`; total < 400 kB; build newer than src, so the assertions cannot go vacuous — that
|
||||
last one is the good idea here, most bundle tests are silently stale. **Gaps:** no per-chunk ceiling (a
|
||||
single 300 kB chunk passes), no gzip assertion, no sourcemap check. A largest-chunk budget is ~6 lines and
|
||||
is the one that would catch an accidental dependency.
|
||||
|
||||
**Chunking / fonts / CSS: nothing to do.** No web fonts (`app.css:45` — correct where system fonts already
|
||||
carry the diacritics). No `manualChunks`, and none is warranted: every screen needs the socket and the proto
|
||||
on first interaction. `preload-data="hover"` over five routes is cheap. `ssr = false` + `prerender = false`
|
||||
means the SSR output under `.svelte-kit/output/server/` is built but never copied into `build/`.
|
||||
|
||||
**Svelte 5 idioms: current throughout** — runes, snippets instead of slots, `page` from `$app/state` not the
|
||||
deprecated `$app/stores`. No `export let`, no `$:`, no `createEventDispatcher`. Two nits: the bare
|
||||
`messages.length;` dependency read at `ChatPanel.svelte:92` is obscure (assign it to a `const`), and
|
||||
`$effect(() => () => clearTimeout(t))` (`GameBoard.svelte:124-125`, `Lobby.svelte:81`) is a dependency-free
|
||||
effect used purely for teardown — `onDestroy` states that plainly and cannot later acquire a dependency.
|
||||
|
||||
---
|
||||
|
||||
## 5. Accessibility and i18n
|
||||
|
||||
**Good already, unusually so:** skip link; `sr-only` `<h1>` on the room screen (413); zero-specificity
|
||||
`:focus-visible` ring via `:where()` (`app.css:132-138`); 44px `.icon-button`; `prefers-reduced-motion`
|
||||
honoured in CSS *and* script (`motion.js` — necessary, since an explicit `behavior` beats the CSS rule);
|
||||
turn indicator as `role="status" aria-live="polite" aria-atomic` (`GameBoard.svelte:203-210`); `role="timer"`
|
||||
plus a separate two-mark spoken region rather than 60 announcements/second (`CountdownRing.svelte:48-51,
|
||||
77-79`); deliberate *non*-duplication of the connection announcement (165-171); focus moved to the result
|
||||
panel rather than to its button (`GameOverPanel.svelte:25-34`).
|
||||
|
||||
**Gaps, ranked:**
|
||||
|
||||
1. **Armed buttons announce nothing.** Resign/claim/kick change their own label on the first press
|
||||
(`GameBoard.svelte:239`, `Lobby.svelte:146`). A screen reader announces a control's name on focus, not on
|
||||
in-place mutation — so a non-sighted player presses once, hears nothing, and either presses again blind or
|
||||
walks away. Fold `aria-pressed` (or a discreet live region) into `ArmedButton` (#9).
|
||||
2. **Chat log live region.** `ChatPanel.svelte` (the `<ol aria-live="polite" aria-relevant="additions">`)
|
||||
announces the reader's *own* messages back to them, and `aria-relevant` is inconsistently implemented.
|
||||
Prefer a dedicated `sr-only` `role="log"` carrying only the newest line where `fromMe` is false — the
|
||||
pattern `CountdownRing` already uses.
|
||||
3. **Rejection alert doubles as a description.** `WordInput.svelte:200-202` points `aria-describedby` at the
|
||||
same element that carries `role="alert"` (220), so the text is announced twice, and the alert contains two
|
||||
focusable buttons. Split: an `sr-only` alert for the announcement, a plain `<p id="word-rejection">` for
|
||||
the description and the buttons.
|
||||
4. **Focus after suggestion-fill is right, state is not.** `useSuggestion()` (161-167) focuses the field and
|
||||
places the caret correctly, but leaves `game.state.rejection` set — so `aria-invalid` stays true and the
|
||||
banner still offers the suggestion the player just took.
|
||||
5. **`ScoreBoard` turn row has no `aria-current`.** `class:active` (24) is the turn indicator for sighted
|
||||
users only; the row is otherwise indistinguishable.
|
||||
6. **Colour contrast is fine.** Spot-checked the risky pairs: `--warn #8a5a08` on `--surface-alt #e9f0ea`
|
||||
≈ 5.1:1; `--text-muted #5b6a61` on `--bg #f4f7f4` ≈ 5.2:1; `--player-1 #1d5c8f` on white ≈ 7.1:1;
|
||||
`--player-3 #8a4c12` on white ≈ 6.7:1; dark-theme `--warn #e9b949` on `#1f2a23` ≈ 8.1:1. All ≥ 4.5:1 at
|
||||
the sizes used. `--border-strong` exists precisely because `--border` does not clear 3:1, and says so.
|
||||
|
||||
**`vi.js` (359 lines) — leave it flat.** Namespacing buys nothing here: one locale, no runtime loader, no
|
||||
key collisions, and `t.foo` is already checked by `svelte-check` (it is a typed object literal, so `t.subimt`
|
||||
is a compile error today — this is one of the few places typing currently works). The existing
|
||||
comment-grouped sections are the right amount of structure. The real gap is that `tests/i18n.test.js` covers
|
||||
the *enum-derived* maps exhaustively (reject reasons, end reasons, point kinds, difficulties, error codes)
|
||||
but nothing asserts that `fill()` placeholders in the flat `t` keys match their call sites — a `{name}` typo
|
||||
in `playerTurn` renders literally. A ~15-line test extracting `{…}` tokens from each `t` value and checking
|
||||
them against `fill()` call sites would close it.
|
||||
|
||||
---
|
||||
|
||||
## 6. Tests
|
||||
|
||||
**Vitest, by module.** Covered: `countdown` (10), `room-code` (14), `history-export` (8), `i18n` (14),
|
||||
`settings` store (16), `ws/client` (32 — genuinely good: injected clock, socket factory, scheduler),
|
||||
`game` store (61, fed real decoded `ServerMessage`s — the right call), `bot-session` (12), wire round-trip
|
||||
(43), error codes (3), dictionary source (4), bundle (4).
|
||||
|
||||
**Uncovered: every `.svelte` file.** There are zero component tests and no component-test harness. Nothing
|
||||
in `tests/` imports a component. So all of this is browser-only today:
|
||||
|
||||
| Behaviour | Where it lives |
|
||||
|---|---|
|
||||
| Chat unread across fold / phase / resync | `ChatPanel.svelte:58-96` — *this is what broke CI today* |
|
||||
| Word field seed, guard, undo, suggestion-fill, submit-clear | `WordInput.svelte:70-167` |
|
||||
| Arm/disarm and the disabled matrix | `GameBoard.svelte:74-125`, `Lobby.svelte:55-81` |
|
||||
| Ring states (mine / stalled / urgent / idle) | `CountdownRing.svelte:15-51` |
|
||||
| Grace countdown bookkeeping | `PlayerStatus.svelte:36-54` |
|
||||
|
||||
**Recommendation: mount components under jsdom.** `jsdom` is *already* a devDependency and two suites
|
||||
already opt in per-file; Svelte 5 components compiled by the vite plugin mount directly with no extra
|
||||
library:
|
||||
|
||||
```js
|
||||
// @vitest-environment jsdom
|
||||
import { mount, unmount, flushSync } from 'svelte';
|
||||
import ChatPanel from '../src/lib/components/ChatPanel.svelte';
|
||||
```
|
||||
|
||||
No new dependency, no browser, runs on this ARM64 host. Start with `ChatPanel` (unread) and `WordInput`
|
||||
(seed/guard) — between them they cover the two most-regressed behaviours in the tree.
|
||||
|
||||
**Playwright: 47 specs across 3 files, `workers: 1`, `fullyParallel: false`, 60s timeout.**
|
||||
|
||||
*Delete — the Go suite already proves the rule, and the Vietnamese string is proven by `i18n.test.js`:*
|
||||
`bot-game.spec.js:114` (word not in dictionary), `:127` (too short), `:137` (does not link),
|
||||
`pvp-game.spec.js:588` (unknown room code), `:608` (latecomer refused), `:624` (fifth player turned away),
|
||||
`:394` (room outlives its owner).
|
||||
|
||||
*Replace with a unit or component test:* `bot-game.spec.js:242` (personal best — `settings-store.test.js`
|
||||
owns the logic), `:264` (attribution footer — static markup), `:40` (syllable pre-seeded — a `WordInput`
|
||||
component test), `pvp-game.spec.js:460` and `:528` (chat unread / send gating — a `ChatPanel` component
|
||||
test). `bot-game.spec.js:272` (deep link served by the binary, not a 404) is a *server* concern and belongs
|
||||
in the Go suite.
|
||||
|
||||
*Keep — genuinely browser-only:* all of `reconnect.spec.js` (socket cut, resume, forfeit, dead-connection
|
||||
typing — multi-context and transport-level); `pvp-game.spec.js:100` (turns alternate), `:140`/`:164` (invite
|
||||
link, nameless guest), `:193` (uncontrolled field refuses text out of turn — IME behaviour no jsdom test can
|
||||
prove), `:497` (a turn does not steal the chat field), `:515` (rendered as text, never markup — the XSS
|
||||
assertion), `:674` (spectator after knockout); `bot-game.spec.js:16` (one end-to-end game), `:195` (download
|
||||
plumbing), `:217` (rematch starts exactly one game).
|
||||
|
||||
47 → ~14, multi-player and reconnect coverage untouched. Sequence *after* the component tests land.
|
||||
|
||||
---
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. **P2 (route-swap socket race):** does SvelteKit destroy the outgoing page component before creating the
|
||||
incoming one? Unverifiable here without a browser. If it does not, `/online → /play` can leave `/play`
|
||||
transport-less.
|
||||
2. **C1's server half:** should `handleHello` answer a non-resumable token explicitly? That is a protocol
|
||||
change and a server decision — the client-side timeout (#1) is sufficient and should land regardless, but
|
||||
the explicit answer is the better contract and `resumeFrom`'s own comment already argues for it.
|
||||
3. Does the dictionary builder guarantee gloss-unique senses per word (C4)? If it does, the duplicate-key
|
||||
risk is theoretical — but the key should still not depend on a guarantee nothing in this repo states.
|
||||
4. Is the `chatFolded` behaviour in C5 intended (fold once, stay folded) or an unnoticed consequence of
|
||||
5178a97? This is a product call, not a defect call.
|
||||
5. Playwright wall-clock today is unmeasured here (no browser). The 47 → 14 recommendation assumes the suite
|
||||
is a meaningful part of CI time; if it runs in under three minutes, prioritise #8's component-test half
|
||||
and defer the deletions.
|
||||
@@ -0,0 +1,203 @@
|
||||
# noitu — whole-game UX review (code-based, read-only)
|
||||
|
||||
Tree: `web/` at `dev` 5178a97, 2026-09-21. No browser on this host: every claim below is read off Svelte markup, CSS tokens and `vi.js`, with ratios computed from the hex in `app.css`. Prior 260910 reports checked against the tree first — their P1/P2 items (turn live region, `--border-strong`, accent darkened to `#12692f`, spacing/type ramps, `min-height` on the chain, focus on game-over, two-press resign/kick, seat colours, owner marker, monospace code, reduced-motion scroll) are all shipped and are not repeated.
|
||||
|
||||
## Verdict
|
||||
|
||||
Structurally one of the better-reasoned small game UIs I have reviewed: the board keeps syllable/clock/input fixed and lets only the chain scroll, the IME handling is right, the colour tokens hold AA in both themes (measured, table in §3), and nearly every control explains itself in `vi.js`. Two things are wrong at a level above polish: (1) the in-game "Luật chơi" link forfeits the game — the route change unmounts the board and its cleanup resigns/leaves; (2) a newcomer reaches the first running clock without ever being told the chain rule. Below those, today's additions (chips, pill, claim, suggestion/report) each work alone but together crowd the 360px board and hand the rejection row three sub-24px targets. The token set is disciplined but looks like nothing in particular — a single display face for the four hero numbers and a flatter type ramp would give it a face without a redesign.
|
||||
|
||||
## Top-10 ranked changes
|
||||
|
||||
| # | Change | Size | Screen | Why |
|
||||
|---|---|---|---|---|
|
||||
| 1 | In-room "Luật chơi" opens in a new tab (or `<dialog>` summary); never a same-tab route | S | board, lobby | `GameBoard.svelte:133`, `Lobby.svelte:90` are plain `<a href="/rules">`; `play/+page.svelte:39-44` cleanup sends `resign()`, `online/+page.svelte:185-212` sends `leaveRoom()` + `forgetSession()`. Reading the rules mid-game = forfeit. |
|
||||
| 2 | State the chain rule before the first clock: one line + example on the landing, caption on the opening row | S | landing, board | `+page.svelte:387` tagline is the only rule text; `rulesLink` sits below the CTAs (`:397`). Opening row `ChainHistory.svelte:249-252` is dashed but unlabelled. |
|
||||
| 3 | Slim the board header: badge text `sr-only` while open, rules as 44px "?" icon, room code moves under the turn prompt | M | board 360px | `GameBoard.svelte:129-154` puts badge (~105px) + code + "Luật chơi" + chat pill (~150px w/ badge) in 328px → two wrapped rows above the scoreboard. |
|
||||
| 4 | Rejection row: suggestion and report become 36px-tall buttons on their own line; suggestion styled primary | S | board | `WordInput.svelte:304-313` pills are `2px` padding at 0.8rem ≈ 23px tall — under WCAG 2.2 24px minimum, and the one control that saves the turn. |
|
||||
| 5 | "Bí từ" stops appearing/vanishing each handover; sits in one secondary row with "Đầu hàng" | S | board | `GameBoard.svelte:227-241` mounts the button only on `myTurn`, shifting everything under the input every turn — the exact thing `.resign`'s comment (`:437-443`) avoids. |
|
||||
| 6 | Show elimination suggestions in the spectating box at knockout, not only at game over | M | board (out) | `elimination.suggestions` arrive with `playerEliminated` (`game.svelte.js:414-431`) but render only in `GameOverPanel.svelte:426-445`; a 4-seat spectator waits minutes. Also `.spectating` (`GameBoard:224`) and the `youAreOut` banner (`PlayerStatus:535`) say the same thing twice. |
|
||||
| 7 | Fix `not_your_turn` for the race it actually describes; route it beside the button | S | board | Both callers are gated on `myTurn` (`GameBoard:69,82`), so the server can only say this when the turn moved mid-press. "Chưa đến lượt bạn" says "not yet" about a turn that has *passed*, in the top banner (`game.svelte.js:498`) ~250px from the button. |
|
||||
| 8 | Quick-match wait: take the second counter out of the live region; nudge says it leaves the queue | S | online entry | `online/+page.svelte:494-503` `role="status"` wraps `{n} giây` → screen reader announces every second. `/play` link at default difficulty; cleanup cancels queue silently (`:207`). |
|
||||
| 9 | Breakdown line: chips, only on the latest row / when a row is open; rename `dài` | M | board | `ChainHistory.svelte:113-118` prints `+10 nền · +4 chuỗi · +5 dài` at 0.75rem muted under *every* row in a 9rem-high list — three rows of history become three rows of arithmetic. |
|
||||
| 10 | Visual identity: one display face for syllable/clock/score/code, ramp collapsed 9→7 steps, hover token, lifted `--bg` | M | all | §6. Nothing here is broken; it is `system-ui` at 600/700 in 8px boxes and reads as a default. |
|
||||
|
||||
## 1. First-run flow
|
||||
|
||||
Path: `/` → nickname (`NicknameInput`) → `DifficultyPicker` → "Chơi với máy" → `/play?difficulty=` → socket → `StartBotGame` → `TurnUpdate` with clock running.
|
||||
|
||||
- **No rule before the clock.** `+page.svelte:387` `tagline` = "Trò chơi nối từ tiếng Việt"; nothing says *what* nối từ requires. `rulesLink` (`:397`) is a muted underline under the CTAs — right that it is not a button, wrong that the *rule* itself needs a click. Newcomer's first read of the rule is `rejectMessages.WRONG_LINK` under a running clock.
|
||||
Sketch (landing, between tagline and nickname):
|
||||
```svelte
|
||||
<p class="how">
|
||||
Tiếng đầu của từ bạn phải là tiếng cuối của từ trước —
|
||||
<span class="ex">ngôn ngữ → ngữ pháp → pháp luật</span>. Mỗi lượt có đồng hồ.
|
||||
</p>
|
||||
```
|
||||
```css
|
||||
.how { margin:0; font-size: var(--text-5); color: var(--text-muted); }
|
||||
.ex { color: var(--text); font-weight: 600; white-space: nowrap; }
|
||||
```
|
||||
Copy: `howToPlay: 'Tiếng đầu của từ bạn phải là tiếng cuối của từ trước — {example}. Mỗi lượt có đồng hồ.'`, `howToPlayExample: 'ngôn ngữ → ngữ pháp → pháp luật'`. Turn length is a server constant not known on the landing, so "có đồng hồ", not "30 giây".
|
||||
- **Opening row unlabelled.** `ChainHistory.svelte:73,249-252` marks `.opening` dashed; a newcomer sees one grey word and a syllable. Add a caption on the opening row only: `openingCaption: 'Từ mở đầu — nối tiếp bằng tiếng cuối'` rendered like `.corrected` (`:310-314`). Stateless, helps every game, costs one 0.8rem line.
|
||||
- **Rules discoverability is right on landing, absent on `/online` entry.** The join form (`online/+page.svelte:466-546`) has no `.rules-link`; a stranger arriving by invite link never sees one before being seated. Add the same link under `.back` (`:546`).
|
||||
- **Difficulty picker: clear as a ladder, opaque as a choice.** `difficultyLabels` = Dễ/Trung bình/Khó + "Kỷ lục". Server: easy = random legal move, medium = 1-ply greedy, hard = multi-ply + 85% kill rate (`server/internal/bot/strategy_*.go`). One truthful caption each, replacing the `.best` line when best = 0:
|
||||
`difficultyHints: { EASY: 'Máy chọn từ ngẫu nhiên', MEDIUM: 'Máy chọn từ tốt nhất trước mắt', HARD: 'Máy tính trước vài nước, hay dồn bí từ' }`. Fits the 96px min column at 0.75rem over two lines.
|
||||
- **Nickname hint is three sentences on a 360px screen** (`vi.js:22-23`, rendered `NicknameInput:411`). Collapse: `'Tối đa 20 ký tự. Để trống sẽ là “Người chơi”.'` — server truncation is an edge case the server already reports by displaying the sanitized name.
|
||||
- **Rules page itself** (`rules/+page.svelte`): good — one anchored page, `scroll-margin-top`, live scoring constants. Two nits: `.toc a` (`:248-256`) is ~27px tall (`4px` padding, 0.8rem) — raise to `padding: var(--space-2) var(--space-3); min-height: 32px`; `.back` (`:220`) always goes `/` — when opened from a game in a new tab (fix #1) the natural action is "close", so hide `.back` when `window.opener || history.length <= 1`, or label it "Đóng" via `history.back()` fallback.
|
||||
|
||||
## 2. In-game board
|
||||
|
||||
**Hierarchy (360–420px).** Reading order in `GameBoard.svelte:128-272`: header row (badge, code, rules, pill) → `ScoreBoard` → banners → error → `.turn` (80px ring + "Đến lượt bạn" + syllable 1.6rem) → `WordInput` → rejection → "Bí từ" → claim error → "Đầu hàng" (right) → chain (`min-height: 9rem`). Priority is right: syllable and clock share a row, input beneath. Problems are density above the ring and churn beneath the input.
|
||||
|
||||
- **Header crowding (#3).** At 360 the inner width is 328px. "Đã kết nối" badge ≈ 105px, `K7M 2QP` ≈ 60px, "Luật chơi" ≈ 60px, chat pill with "3 tin mới" ≈ 150px. `.meta` wraps (`:291-297`) so nothing breaks, but the board opens on two rows of chrome. Sketch:
|
||||
```svelte
|
||||
<div class="top">
|
||||
<ConnectionBadge compact /> <!-- dot only while OPEN; label sr-only -->
|
||||
<div class="meta">
|
||||
<a class="icon-button rules" href="/rules" target="_blank" rel="noopener" aria-label={t.rulesLink}>?</a>
|
||||
{#if onchatopen}<button class="chat-pill" …>💬 <span class="sr-only">{t.chatTitle}</span>{#if chatUnread}<span class="pill-badge">{chatUnread}</span>{/if}</button>{/if}
|
||||
</div>
|
||||
</div>
|
||||
…
|
||||
<p class="syllable"><span class="label">{t.currentSyllable}{#if modeLabel} · {modeLabel}{/if}</span><strong>…</strong></p>
|
||||
```
|
||||
`ConnectionBadge` gains `compact` → when `status==='open'` render `.dot` + `.sr-only` label; any other status shows the text (that is when it matters). Pill count drops the word: `chatUnread` becomes `'{n}'` visually with `aria-label={fill(t.chatUnread,{n})}`. Header then fits one row at 320px.
|
||||
- **Input row is not crowded — what follows it is.** `WordInput` row = input + "Gửi" (`:175-213`), fine. Below it, in order and each conditional: `.rejection` (word — message + up to two pills, `:220-232`), `.report-confirmation` (`:235-239`), "Bí từ" (`GameBoard:232-240`), `.claim-error` (`:243-251`), then "Đầu hàng" right-aligned (`:260-268`). Worst case is five stacked blocks between input and chain, four of them pop in and out. Sketch — one persistent secondary row, everything else inline in a single message slot:
|
||||
```svelte
|
||||
<WordInput … />
|
||||
<div class="secondary">
|
||||
<button class="claim-dead-end" disabled={!canClaimDeadEnd} …>{claimArming ? t.claimDeadEndSure : t.claimDeadEnd}</button>
|
||||
<button class="resign" disabled={!canResign} …>{arming ? t.resignSure : t.resign}</button>
|
||||
</div>
|
||||
{#if message}<p class="notice" class:danger role="alert">…</p>{/if} <!-- rejection | claimError | reportConfirmation, one at a time -->
|
||||
```
|
||||
```css
|
||||
.secondary { display:flex; justify-content:space-between; gap:var(--space-2); }
|
||||
```
|
||||
Both buttons keep their existing arming styles and disabled looks; both stay mounted so the row height is constant (what `.resign` already argues for at `:437-443`).
|
||||
- **Rejection affordances (#4).** `.suggestion, .report` (`WordInput:304-313`): `padding: 2px 8px`, 0.8rem → ≈23×90px. Under the clock these are the two most valuable taps on the board. Sketch:
|
||||
```svelte
|
||||
<p class="rejection" id="word-rejection" role="alert">
|
||||
<strong>{word}</strong> — {message}
|
||||
</p>
|
||||
{#if suggestion || reason === NOT_IN_DICTIONARY}
|
||||
<div class="fixes">
|
||||
{#if suggestion}<button class="fix primary" onclick={useSuggestion}>{fill(t.suggestionPrompt,{word:suggestion})}</button>{/if}
|
||||
{#if reason === NOT_IN_DICTIONARY}<button class="fix" onclick={report}>{t.reportWord}</button>{/if}
|
||||
</div>
|
||||
{/if}
|
||||
```
|
||||
```css
|
||||
.fixes { display:flex; flex-wrap:wrap; gap:var(--space-2); margin-top:var(--space-2); }
|
||||
.fix { min-height:36px; padding:var(--space-1) var(--space-3); border:1px solid var(--border-strong); border-radius:var(--radius-pill); background:var(--surface); color:var(--text); font-size:var(--text-4); font-weight:600; }
|
||||
.fix.primary { border-color:var(--accent); background:var(--accent-soft); }
|
||||
```
|
||||
Consider `useSuggestion()` also submitting — one tap, not tap + Enter — since the server already vouched the word exists; only ambiguity is the player may want to change it. Keep as fill-only if unsure; then label `'Dùng “{word}”'` is more honest than a question.
|
||||
- **Breakdown chips (#9).** `.parts` (`ChainHistory:113-118, 316-322`) is a joined string, not chips, under every row. In a 9rem list that is ~3 rows, each now ~64px with parts. Show parts only on `.latest` and on an `open` row (row is already a toggle, `:79-86`), and render as chips:
|
||||
```svelte
|
||||
{#if entry.parts.length && (index === 0 || open)}
|
||||
<ul class="parts" aria-label={t.pointsBreakdown}>{#each entry.parts as p}<li>+{p.value} <span>{pointKindLabels[p.kind]}</span></li>{/each}</ul>
|
||||
{/if}
|
||||
```
|
||||
```css
|
||||
.parts { display:flex; flex-wrap:wrap; gap:var(--space-1); margin:0; padding:0 12px 8px; list-style:none; }
|
||||
.parts li { padding:0 var(--space-2); border-radius:var(--radius-pill); background:var(--surface-alt); color:var(--text-muted); font-size:var(--text-2); font-variant-numeric:tabular-nums; }
|
||||
```
|
||||
Labels: keep `nền/chuỗi/nhanh/hiếm` (they match `/rules`); change `SYLLABLES: 'dài'` → `'từ dài'` — "+5 dài" is not a phrase; "+5 từ dài" is. Add `pointsBreakdown: 'Cách tính điểm'`.
|
||||
- **Wide screen.** Bot game uses the 560px shell — fine, it is a reading column. Online room at ≥900px is `1fr / 320px` (`online/+page.svelte:610-616`): game pane can reach 700px, so `.turn` (ring left, prompt right) leaves the right half empty and the chain rows stretch to 700px with `.meta` pushed far right of `.word`. Cap the board: `.pane.game { max-width: 560px; }` or centre `.turn`. Cheap, and it keeps the syllable near the input on a 1440 monitor.
|
||||
- **Motion / reduced motion.** Inventory: `pulse` on the reconnecting dot (`ConnectionBadge:58`), two 150ms background transitions (`GameBoard:430,465`), rAF ring, smooth scroll via `motion.js`. Global kill switch `app.css:217-229` is correct (iteration-count fix noted). Motion is nearly absent, which is a defensible choice; two micro-interactions would earn their place and are auto-covered by the global rule: accepted word entering (`li.latest { animation: rise 160ms ease-out }` with `@keyframes rise { from { opacity:0; transform: translateY(4px) } }`) and a 2-frame horizontal nudge on `.rejection` insertion (`translateX(-3px)→3px→0`, 180ms). Nothing loops.
|
||||
- **5 seconds left.** `CountdownRing:365,418-420,450-456`: `urgent` = `mine && !stalled && left ≤ 5` → `--danger`, stroke 6→9, value 1.5→1.75rem, spoken at 10 and 5. Colour-independent, doesn't move, opponent's clock stays neutral — correct. Gap: the eye is on the caret, 80px to the right. Lift `urgent` to the board (`data-urgent` on `.board`, or a store derived) and add `.board[data-urgent] .input-row input { border-color: var(--danger); box-shadow: inset 0 0 0 1px var(--danger); }`. Small, no motion.
|
||||
- **Elimination / spectating (#6).** `game.iAmOut` → `PlayerStatus:535` banner "Bạn đã bị loại. Ván đấu vẫn đang tiếp tục." *and* `GameBoard:224` box "Bạn đang xem ván đấu." Same news twice, 60px apart. Merge into the spectating box and put the suggestions there — the player has minutes to look at them:
|
||||
```svelte
|
||||
<div class="spectating" role="status">
|
||||
<p>{t.youAreOut}</p>
|
||||
{#if game.state.elimination?.suggestions.length}
|
||||
<p class="could">{t.suggestionsTitle}: {game.state.elimination.suggestions.join(' · ')}</p>
|
||||
{:else if game.state.elimination}
|
||||
<p class="could">{fill(t.noSuggestions,{syllable})}</p> <!-- capture syllable at knockout; currentSyllable moves on -->
|
||||
{/if}
|
||||
</div>
|
||||
```
|
||||
Note the existing `GameOverPanel:442` fills `noSuggestions` with `game.state.currentSyllable` — by game over in a 4-seat room that is somebody else's syllable. Store the syllable in `elimination` at `game.svelte.js:425-430`.
|
||||
- **Game-over standings.** `GameOverPanel:396-424`: standings list *then* `.stats` "Điểm cuối cùng / Số từ trong chuỗi" — the score is already on the player's own standings row (`:408`); drop `finalScore` from `.stats`, keep `chainLength`. `🏆` on a bare `<span aria-label>` (`:409`) is not reliably announced — `role="img"`. `h2` gets `aria-live="polite"` on insertion (`:390`) plus panel focus (`:365-367`); either is enough, keep the focus, drop the live attribute to avoid a double announcement.
|
||||
|
||||
## 3. Online
|
||||
|
||||
- **Entry choice architecture.** `online/+page.svelte:509-543`: two identical `.primary` filled buttons ("Chơi ngay", "Tạo phòng") stacked, then the join form. Two primaries = no primary. `onlineIntro` (`vi.js:100`) still describes only create/join. Sketch: "Chơi ngay" stays filled; "Tạo phòng" becomes outlined (`border:1px solid var(--border-strong); background:var(--surface); color:inherit`); join form unchanged. Copy: `onlineIntro: 'Chơi ngay với người lạ, tạo phòng cho bạn bè, hoặc nhập mã bạn được mời.'` Add sub-labels under the two buttons at `--text-3` muted: `quickMatchHint: 'Ghép với người đang chờ, ván đầu tự bắt đầu'`, `createRoomHint: 'Nhận mã sáu ký tự để gửi cho bạn bè'`.
|
||||
- **Waiting panel (#8).** `:494-503`. Split: `<p aria-live="polite">{t.quickMatchSearching}</p>` announced once, and `<p aria-hidden="true">{n} giây</p>` for the eye; or keep one line with `aria-live="off"` and a separate `sr-only` status announcing at 0s and 20s. Nudge copy: `quickMatchNudge: 'Chưa có ai chờ.'`, `quickMatchNudgeLink: 'Chơi với máy trong lúc đợi (sẽ rời hàng chờ)'`; link to `/play?difficulty={settings' last or MEDIUM}`. Also `.waiting` has no `.cancel` styling differentiation — fine as is.
|
||||
- **Lobby (2–4 seats).** `Lobby.svelte:97-162`: count line, seat rows with role pill, wins, state text, owner-only 36px `×` (44px hit via `::after`, `:361-379`). Readiness = tint + text label (`:294-297,129-131`) — colour-independent. Owner = 3px accent left border (`:299-301`). Hints cover every role/state (`:172-184`). Two additions: (a) **seat colour appears nowhere except chat.** Move the owner marker to the role pill (it already says "Chủ phòng") and use the left border for the seat colour: `.seat { border-inline-start: 3px solid var(--seat) }` with `style:--seat={`var(--player-${game.seatIndexOf(player.playerId)})`}`; the chat log's colours then mean something the first time they are seen. (b) `kick:disabled` at `opacity .35` with no reason — `title` is not enough; when `player.ready` add `.sr-only` "{t.player_is_ready}" text or drop the button for ready seats and keep the row height (`visibility:hidden`).
|
||||
- **Chat log contrast (computed from `app.css:38-41, 93-96`).** Light, on `--surface`/`--surface-alt`: p1 7.05/6.08, p2 7.15/6.17, p3 6.72/5.80, p4 5.95/**5.13**. Dark: p1 7.92/7.05, p2 6.77/6.02, p3 7.89/7.02, p4 9.13/8.13. All pass AA; p4 light is the tight one — `--player-4: #0b5f6a` lifts it to ~5.8 without touching hue. Distinguishability between seats is low for CVD (p1 vs p4 1.18, p1 vs p2 1.02 luminance ratio) — acceptable only because every line also carries the name (`ChatPanel:170`); keep that invariant. Timestamp `.at` at `--text-1` (0.7rem = 11.2px) muted on every line is noise: show it only when the minute changes from the previous line, or on hover/focus of the line.
|
||||
- **Chat as log.** `ChatPanel:159-176` one line per message, `aria-live="polite" aria-relevant="additions"` — reads author, text *and* timestamp for every message; move `.at` outside the live text or `aria-hidden` it. `chatAuthorLeft: 'Đã rời phòng'` renders as `Đã rời phòng: hello` — reads as a sentence, not a name. Use `'(đã rời phòng)'`. Folded header button (`:142-147`) is 44px, good; the board pill (#3) and this header are two ways to the same thing on a phone — acceptable since the pill is above the chain.
|
||||
- **Reconnect surfaces.** Three layers, no conflict: `ConnectionBadge` live label; `.offline` line with 44px "Thử lại" (`GameBoard:172-177`, warn on surface-alt 5.11 light / 8.13 dark); per-player grace banners with countdown (`PlayerStatus:540-549`). Lobby `ownerAway` hint (`Lobby:173-174`) is the right override. One gap: the ring at `stalled` goes to `opacity .4` (`CountdownRing:428-430`) but the value keeps counting — the number should read "–" or the label should say offline, else the player reads a live clock they cannot beat.
|
||||
- **Invite link.** `RoomCodePanel`: 3+3 grouping, letter-by-letter `aria-label`, monospace, copy code / copy link / native share, clipboard-failure fallback selects the code, full URL visible and `user-select: all`. Nothing to add; the compact variant correctly keeps only "Sao chép mã".
|
||||
|
||||
## 4. Copy
|
||||
|
||||
Tone: consistent second-person "bạn", imperative "Hãy …" for recovery, no exclamation except `won`, `newRecord`, `TIMEOUT`. Explains rather than blames in almost every rejection (`NOT_IN_DICTIONARY`, `WRONG_LINK` with the syllable filled). Issues:
|
||||
|
||||
- `not_your_turn` / `NOT_YOUR_TURN` "Chưa đến lượt bạn." — for resign and claim this can only fire on a race (both buttons are turn-gated), where the truth is the turn *just left*. Propose for the server-error key: `not_your_turn: 'Lượt vừa chuyển đi, thao tác này không còn hiệu lực.'`; keep the reject-reason string for a raced word submit but extend it: `'Chưa đến lượt bạn — từ này chưa được gửi.'`. Route the error to the same slot as `claimError` (#7).
|
||||
- `not_a_dead_end: 'Vẫn còn từ nối được. Hãy thử lại.'` — "thử lại" invites re-pressing "Bí từ". → `'Vẫn còn từ nối được với tiếng này. Nghĩ thêm chút nữa!'`
|
||||
- `winsLabel: 'Tỉ số'` beside a per-game `score` (`ScoreBoard:361-365`) — two numbers, one called "điểm", one "tỉ số", both look like scores. → `'Ván thắng'` (lobby) / `'Thắng {n}'` compact on the board.
|
||||
- `need_more_players` hardcodes "hai" while `ownerNeedsMore` interpolates `{n}` — server constant leaks in prose. → `'Chưa đủ người để bắt đầu.'`
|
||||
- `opponentTurn: 'Đối thủ đang suy nghĩ…'` fine for bot; online it is the fallback when `nameOf` is empty — reword neutral: `'Đang chờ người chơi khác…'`.
|
||||
- `nicknameHint` — see §1. `app.html:246` meta description says "đấu 1v1"; the game is 2–4: `'Trò chơi nối từ tiếng Việt: chơi với máy hoặc đấu trực tuyến 2–4 người.'`
|
||||
- `too_fast: 'Bạn thao tác quá nhanh. Chậm lại một chút nhé.'` — the only "nhé" in the file; either adopt the warmer register elsewhere (`not_a_dead_end` above does) or drop it: `'Thao tác quá nhanh. Đợi một chút rồi thử lại.'`
|
||||
- Length at 360: `claimDeadEndSure 'Chắc chắn bí từ?'` and `resignSure` fit the 32px buttons; `reportWord 'Báo từ này là từ thật'` (~150px at 0.8rem) wraps the rejection row — with #4 it moves to its own line. `quickMatchNudgeLink` + `quickMatchNudge` on one line wrap at 360; the sketch in §3 accepts that.
|
||||
- Rules body paragraphs are single 90–120-word sentences (`rulesDeadEndBody`, `rulesRoomBody`). Split each at the first "—" into two sentences; content is fine.
|
||||
|
||||
## 5. Accessibility
|
||||
|
||||
- **Focus order**: header (brand, theme) → skip link target `#main` → badge (not focusable) → pill → scoreboard (static) → ring (static) → input → Gửi → suggestion/report → Bí từ → Đầu hàng → chain rows. Logical. Game over: panel takes focus (`GameOverPanel:365`), good. The input keeps focus off-turn with hidden caret (`WordInput:266-271`) and `aria-disabled` — correct trade for the keyboard; the placeholder swap tells a screen reader nothing because placeholder isn't re-announced. Add `aria-describedby` to a `sr-only` "Chưa đến lượt bạn" status when `!enabled`, or announce via the existing turn `role="status"` (already does — fine, no change).
|
||||
- **Labels**: word input `aria-label` stable (`:204`); chat input labelled; kick has state-aware `aria-label` (`Lobby:146`); theme toggle `aria-pressed` + sr text. `.chat-pill` reads "Trò chuyện 3 tin mới" — good. `🏆` span — `role="img"` (§2).
|
||||
- **Live regions**: turn (`GameBoard:207-209` polite, atomic), badge (polite), spoken clock at 10/5 (`CountdownRing:400-402`), errors `role="alert"`, chat `aria-live` additions, quick-match counter every second (**fix**, #8), game-over `h2` + focus (double, §2). Count of simultaneously-live nodes on the board is five; acceptable because each fires on a distinct event.
|
||||
- **Keyboard-only play**: fully possible. Two-press arming for resign/claim/kick works with Enter/Space; the 4s disarm (`GameBoard:46`) is short for a switch-access user — 8s costs nothing.
|
||||
- **Colour-independent state**: ring thickness+size at urgent; scoreboard active row = border + tint + 3px underline (`ScoreBoard:403-413`); ready seat = tint + text; chain `.mine` = border + tint; `.latest` = 3px bar; connection = text label. Pass. `li.mine` tint vs surface is 1.17 light — the accent border carries it.
|
||||
- **Targets**: pass — inputs ≈52px, Gửi, Ready/Start/Copy 44px, kick 44 via `::after`, icon dismiss 44. Fail/tight — `.suggestion/.report` ≈23px (#4), `.toc a` ≈27px (§1), `ThemeToggle` 40px (`:86-97`; make 44), `.chat-pill` 32px and `.claim-dead-end`/`.resign` 32px by stated intent (`GameBoard:418-423`) — WCAG 2.2 2.5.8 minimum is 24, so compliant; I would still take `.claim-dead-end` to 40 since it is an in-turn action, unlike resign.
|
||||
- **Contrast** (computed): all text pairs in use ≥4.88 light / ≥5.14 dark; `--border-strong` on surface 3.64/3.82 and on surface-alt 3.14/3.40 (≥3:1 for UI boundaries, pass); ring track vs arc 1.87/2.51 — non-text but the arc is the information; the thickness/number carry it, and the arc is `currentColor` on accent so it is legible against the track by lightness in dark mode only. Consider `.track { stroke: var(--border); }` (fainter track = arc reads higher).
|
||||
|
||||
## 6. Visual identity
|
||||
|
||||
What `app.css` adds up to: a coherent *palette* (paper with a green cast, one accent that is also the "text green", four seat hues) and disciplined *tokens*, drawn in `system-ui` with 600/700 weights inside 8px-radius outlined boxes. It is clean and reads as "well-built default". Verified drift: 34 spacing declarations still use 6/10/14px literals (17×10px, 8×14px, 9×6px) beside the 4px ramp; four font-size literals off the ramp (`GameBoard:365` 1.6rem, `CountdownRing:132,149` 1.75/1.5rem, `online:636` 1.3rem); the ramp's first five steps span 0.7–0.9rem (11.2→14.4px in 0.8px increments) — too fine to be a scale. Light theme surfaces are near-flat: `--surface-alt`/`--surface` 1.16, `--bg`/`--surface` 1.08.
|
||||
|
||||
Restrained direction — one face, one accent, fewer steps:
|
||||
|
||||
- **Display face for the four hero glyphs only** (syllable, clock value, score, room code): `Be Vietnam Pro` 700 — designed for Vietnamese, full diacritic stacking, Google Fonts, self-hostable as two `woff2` (vietnamese+latin subset ≈ 45KB total). Body stays `system-ui`. Fallback chain keeps the current look if the font fails.
|
||||
```css
|
||||
--font-display: 'Be Vietnam Pro', var(--font);
|
||||
/* GameBoard .syllable strong, CountdownRing .value, ScoreBoard .score, RoomCodePanel .code (keep monospace there? — no: the display face's 5/S 2/Z are distinct enough; if in doubt keep ui-monospace) */
|
||||
```
|
||||
Alternative at zero bytes: keep `system-ui`, but give the syllable the room a hero deserves — `font-size: var(--text-10); letter-spacing: -0.01em; line-height: 1.25`.
|
||||
- **Type ramp 9 → 7**, mapped:
|
||||
```css
|
||||
--text-1: 0.75rem; /* was text-1/text-2 (0.7, 0.75): badges, series, timestamps */
|
||||
--text-2: 0.875rem; /* was text-3/text-4/text-5 (0.8, 0.85, 0.9): meta, hints, labels */
|
||||
--text-3: 1rem; /* was text-6: body, inputs (iOS floor) */
|
||||
--text-4: 1.125rem; /* was text-7 (1.1): scoreboard score, h2 */
|
||||
--text-5: 1.5rem; /* was text-8 (1.4) + ring value 1.5 */
|
||||
--text-6: 2rem; /* was text-9: room code */
|
||||
--text-7: 2.25rem; /* new: the syllable (was 1.6 literal) */
|
||||
```
|
||||
Losing 0.7rem (11.2px) removes the smallest text in the app (pill badge, series, chat time) — all of which are currently at the edge of legible on a phone.
|
||||
- **Accent hover / pressed tokens** — hovers today borrow `--surface-alt`, which is grey, so the accent never darkens under the pointer:
|
||||
```css
|
||||
:root { --accent-hover: #0e5626; --accent-pressed: #0a4520; }
|
||||
[data-theme='dark'] { --accent-hover: #7ae6a1; --accent-pressed: #93edb3; }
|
||||
.primary:hover { background: var(--accent-hover); } .primary:active { background: var(--accent-pressed); }
|
||||
```
|
||||
Both keep white/dark ink ≥ 7:1.
|
||||
- **Lift the page from the cards** (light only): `--bg: #eef3ef` (surface/bg 1.25, cards read as cards; text-muted on bg stays 5.0+). Dark is already layered (1.35/1.49).
|
||||
- **Spacing**: replace the 34 literals with the ramp (`6→--space-2` or `--space-1`, `10→--space-3` or `--space-2`, `14→--space-4`), then the ramp is the rhythm rather than a suggestion. Radius: keep 12/8/pill.
|
||||
- **Where the identity will actually show**: the `.turn` block. With the syllable at 2.25rem in the display face, the ring beside it, and the chain in body face below, the board has one obvious focal point — which is also the one glyph read every turn (`GameBoard:362-367` already argues this).
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. Fix #1: new tab vs in-page `<dialog>` summary of the rules? A dialog keeps the clock visible but needs a second copy of the rules or an iframe of `/rules`; a new tab is one attribute.
|
||||
2. Bot game: does the human take the first turn after the opening word (engine `players[0]`)? Determines whether the landing one-liner should mention the clock at all.
|
||||
3. Are the two micro-interactions (word entrance, rejection nudge) wanted, given the current near-zero-motion stance reads as deliberate?
|
||||
4. Font: is a ~45KB self-hosted display face acceptable for a single-binary deploy, or is the zero-byte `system-ui` variant preferred?
|
||||
5. `useSuggestion` — fill only (current) or fill-and-submit?
|
||||
6. Storing the knockout syllable in `elimination` is a store change beyond CSS/markup — in scope for the owner's "accept redesigns", or should it be filed separately?
|
||||
|
||||
Status: DONE
|
||||
Summary: Full read-only review written to `plans/reports/ui-ux-designer-260921-1529-whole-game-ux-review.md`; two structural findings (rules link forfeits the game; no rule before first clock) plus a ranked top-10 with markup/CSS/copy sketches, contrast figures computed from tokens.
|
||||
Concerns/Blockers: Contrast and layout widths are computed/estimated from source, not rendered; §2 header-width estimates assume system-ui metrics.
|
||||
Reference in new issue
Block a user