From d3eb13e36e889c11a5a95069751e692d6c9b093c Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 28 Sep 2026 15:19:59 +0700 Subject: [PATCH] docs(reports): record the dev branch review and refactor --- ...efactor-260928-1348-review-and-refactor.md | 34 ++++++ ...efactor-260928-1348-review-and-refactor.md | 75 ++++++++++++ ...efactor-260928-1348-review-and-refactor.md | 112 ++++++++++++++++++ 3 files changed, 221 insertions(+) create mode 100644 plans/reports/server-core-refactor-260928-1348-review-and-refactor.md create mode 100644 plans/reports/web-refactor-260928-1348-review-and-refactor.md create mode 100644 plans/reports/wsapi-refactor-260928-1348-review-and-refactor.md diff --git a/plans/reports/server-core-refactor-260928-1348-review-and-refactor.md b/plans/reports/server-core-refactor-260928-1348-review-and-refactor.md new file mode 100644 index 0000000..9c9b488 --- /dev/null +++ b/plans/reports/server-core-refactor-260928-1348-review-and-refactor.md @@ -0,0 +1,34 @@ +# Server core review and refactor (dev vs main) + +Scope: `git diff main...dev -- server/cmd server/internal/{game,dictionary,vietnamese,bot} Dockerfile Makefile .github`. +wsapi, proto, gen, and web were not touched. No exported identifier or signature changed. + +## Findings + +| Sev | Location | Finding | Action | +|-----|----------|---------|--------| +| Low | server/cmd/noitu-server/main.go (envInt/envDuration/envNonNegDuration) | Three copies of the same read/trim/parse/validate/warn logic; the two duration helpers differed by a single comparison | Added one generic `envParsed[T]` helper. The three named wrappers stay, with the same names, log messages, and fallback rules | +| Low | server/cmd/noitu-server/main.go (run shutdown tail) | The `context.WithTimeout` + `Shutdown` block appeared twice, once for the debug server and once for the public server | Added `shutdownServer(*http.Server) error`, which does nothing when passed nil | +| Low | server/internal/game/engine.go (pointsFor/capParts) | The score total was summed three times across two functions (twice in capParts, once in pointsFor), plus a hand-written in-place filter | Moved the capping into pointsFor. A single `sumParts` helper (moved from the test file) does all summing, and `slices.DeleteFunc` does the filtering. Trim order and results are unchanged | +| Low | server/internal/game/state.go:201 | The `State.Standings` doc said "meaningless" during play, but Snapshot now leaves it nil during play | Doc now says "Nil while the game is in play". wsapi only reads it when the game ends (room_game.go:569) | +| Nit | server/internal/dictionary/store.go | `dsn()` just wrapped `DSN(path, true)` and had one caller. `math/rand/v2` sat in its own misplaced import group | Replaced the call with `DSN(path, true)` directly and let gofmt sort the imports | +| Low | server/internal/dictionary/store_test.go (nearMissFixtureAt) | Copied the open/exec/close logic that the existing `writeDB` helper already provides | `nearMissFixture` now calls `writeDB`. Same data, same assertions | + +No problems found in: build-dictionary (the Windows-only fallback when rename fails is correct and not duplicated; the `defer func(){ _ = x.Close() }()` changes are there to satisfy lint), the vietnamese double NFC (it is needed: `lower(NFC(x))` can stop being NFC, and dropping the first NFC is not safe either because Go's simple case mapping of U+0130 differs between composed and decomposed input), Dockerfile, Makefile, dependabot, and the CI workflows (moving major tags, as house rules require; no pins changed). + +## Bugs fixed + +None. I checked for resource leaks: the dictionary DB handle is closed inside `Open`, and the goroutines in main exit when the process ends. I also looked at the path where the listener fails at startup, which returns without shutting down the rooms or the debug listener. The process exits right away, so this is not a real leak, and I left the behaviour as it was. + +## Verification + +- `go vet ./...` clean. `go build ./...` passes. +- `go test -race -count=1 ./cmd/... ./internal/game/... ./internal/dictionary/... ./internal/vietnamese/... ./internal/bot/...` all pass. +- `gofmt -l` clean. `golangci-lint run` on the owned packages reports 0 issues. + +## Deferred items (not changed) + +- `wsapi/convert.go` `PointKind` switch and `game.PointKind.String` are parallel switches that must be kept in sync by hand. This belongs to wsapi, so I left it alone. +- `Engine.UsedWords` returns `maps.Keys` over the engine's live map. That is safe only because the room goroutine is the only one that touches the engine. The doc could say "do not Submit while ranging". I left it as is because the existing single-owner rule already covers it. +- engine.go `LegalMoves`/`HasLegalMove` repeat the `e.used[word]` lookup inline where `e.Used(word)` would do. This is older code, outside the diff. +- `.github/workflows/*`: `actions/setup-go@v5` could move to the current major. Left alone to avoid pin churn; dependabot will propose it. diff --git a/plans/reports/web-refactor-260928-1348-review-and-refactor.md b/plans/reports/web-refactor-260928-1348-review-and-refactor.md new file mode 100644 index 0000000..66a1350 --- /dev/null +++ b/plans/reports/web-refactor-260928-1348-review-and-refactor.md @@ -0,0 +1,75 @@ +# Web review and refactor: dev vs main + +Scope: `git diff main...dev -- web` (proto and lockfile excluded). Nothing committed. +Gates: `npm run lint` (0 errors, 0 warnings), `npm run check` (0/0), `npm test` (269 passed; the baseline was 266, plus 3 new tests). + +## Findings + +| Sev | Where (before the change) | Finding | +|---|---|---| +| High | `web/src/app.css` `.icon-button` (`--text-7`), `.rules-link` (`--text-4`), `.skip` (`--text-5`) | When the type ramp dropped from nine steps to seven, these three global rules kept their old token numbers. The meanings of those numbers changed: the dismiss "×" and the board's "?" went from 1.1rem to 2.25rem, rules links from 0.85rem to 1.125rem, and the skip link from 0.9rem to 1.5rem. The component files were remapped; app.css was missed. | +| Medium | `WordInput.svelte` style `button:hover/active:not(:disabled)` | The bare `button` rule also matched the `.fix` buttons. Its hover and press states outranked `.fix:hover`, so on hover or press the report button turned solid accent with dark text on top (low contrast). | +| Medium | `ChatPanel.svelte` draft vs field | The input unmounts while the panel is folded. After unfolding, the field was empty but `draft` still held the old text. "Gửi" stayed enabled and did nothing when pressed, and the typed text was lost. | +| Medium | `GameBoard.svelte` `:global(.resign)`, `:global(.claim-dead-end)`; `Lobby.svelte` `:global(.kick)` | These selectors were global without any scope, so they applied to every element in the app with that class. | +| Low | `online/+page.svelte` `ready/start/kick/leave/cancelQueue` | Five copies of `send(x); if (!sent) session.holdAction(...)`, next to a `dispatchAction` switch that already mapped each action to its message. | +| Low | `online/+page.svelte` resume timeout and resume error effects | The same three steps (`noteResumeFailed`, `forgetSession`, conditional `flush`) were written out in both places. | +| Low | `online/+page.svelte` queued-seconds effect | `resetQueued()` was called on both branches. | +| Low | `online/+page.svelte` doc above `ready()` | The comment described leaving the room, not readying. | +| Low | `play/+page.svelte` and `online/+page.svelte` | Identical `play`, `giveUp`, `claim` and `report` wrappers in both routes. | +| Low | `game-apply.js` `chatMessage` / `chatHistory` | The chat-line mapping was duplicated. The `error` case chained `||` comparisons across three code groups. `turnUpdate` had two separate `if (played)` blocks. | +| Low | `GameBoard.svelte` `canResign` / `canClaimDeadEnd` | Two identical deriveds. The `.resign` and `.claim-dead-end` CSS was about 80% the same, and `.claim-error` repeated `.error`. | +| Low | Banner CSS (`.error`/`.notice`) | Copied into GameBoard, Lobby and the online page, each with its own markup for the dismiss button. | +| Low | `ChatPanel.svelte` header doc | Said "Three facts" but listed two. Some spacing still used px literals where tokens exist. | +| Low | `room-session.svelte.js` `startResume` doc | Said "optionally behind a held join", but the function takes no argument. | +| Low | `ArmedButton.svelte` | `clearTimeout(timer); armed = false` appeared three times. | +| Low | `Lobby.svelte` `ownerAway` | Had an inline JSDoc param type that inference already covers. | +| Low | Tests | Finding codes in comments (`C6` in word-input, `C1`/"the review" in room-session and chat-panel). Each jsdom suite had its own copy of the gameStarted builder and the mount helper. `room-code.test.js:77` produced an `any` lint warning. | + +No unused i18n keys were found: every `t.*` key has a `t.` reference. + +## Changes + +- **app.css**: remapped `.icon-button` to `--text-4`, `.rules-link` to `--text-2` and `.skip` to `--text-2`, which are the same sizes they had on the old ramp. Updated the `.rules-link` comment, since the board header now uses a "?" icon-button. +- **New `AlertBanner.svelte`**: one `role="alert"` banner with `tone` (`error`/`notice`), `testid`, and an optional `ondismiss`. It replaces 8 hand-written banners in GameBoard (error, claim error), Lobby (`lobby-error`, `lobby-unsent`) and the online page (`name-needed`, `join-error`, `resume-failed`, `connect-stalled`). All test ids and roles are unchanged. GameBoard's `.offline` strip keeps its own markup because it is deliberately not announced and it carries the retry button. +- **`turnActions` in `ws/connection.svelte.js`**: `submit`, `resign`, `claimDeadEnd`, `reportWord`. Both game routes pass these to GameBoard, and the four duplicated wrapper functions in each route are gone. GameBoard's props contract is unchanged, so the component tests still inject their own handlers. +- **online/+page.svelte**: + - `act(action)` = `dispatchAction` + hold-on-failure. `ready`, `start`, `kick`, `leave` and `cancelQueue` are now one-liners on top of it. + - `abandonResume()` is shared by the timeout path and the error path. + - The queued-seconds effect is simplified. + - The misplaced doc comment is fixed. +- **game-apply.js**: + - `toChatLine()` is used by both chat cases. + - The error codes are grouped into named Sets: `LEAVES_ROOM`, `ANSWERED_BY_THE_BUTTON`, `ENDS_THE_QUEUE`. + - The rejection clearing moved into the single `if (played)` block. + - Behaviour is identical and the game-store tests pass unchanged. +- **GameBoard.svelte**: one `canPlayInsteadOfAWord` derived. The shared secondary-button CSS is written once as `.secondary :global(button)`, with the resign/claim colour differences layered on top and everything scoped under `.secondary`. +- **Lobby.svelte**: kick CSS scoped under `.seats :global(...)`, `ownerAway` simplified, the local `.error` CSS removed. +- **WordInput.svelte**: submit-button styles scoped to `.input-row button`. A `caretToEnd()` helper replaces two copies of the same `setSelectionRange` call. +- **ChatPanel.svelte**: an untracked, once-per-mount effect writes `draft` back into the field when it remounts. It never writes during a composition. The doc is fixed and px spacing moved to tokens. +- **ArmedButton.svelte**: added a `disarm()` helper. +- **room-session.svelte.js**: fixed the `startResume` doc. +- **Tests**: + - New `tests/component-support.js` (`receive`, `startGame`, `render`), used by the game-board, word-input and chat-panel suites. + - Removed the finding codes from comments and fixed the `room-code.test.js:77` warning with an `unknown`-to-`string` cast. + - New tests: a fold/unfold round trip keeps a sendable chat draft (confirmed to fail without the fix); dismissing a board error; a refused claim appears beside the claim/resign row and can be dismissed. +- **tests/error-codes.test.js**: the scanner now also picks up the two code literals at `toRoom(limiter, notIn, dropped, …)` call sites. Server work in progress (not mine) moved `not_in_a_game` behind that helper in `server/internal/wsapi/dispatch.go`, and without this the "no stale message" guard failed. + +## Bugs fixed (user-visible) + +1. The dismiss "×", the board's "?" button, rules links and the skip link are back to their intended sizes (they had grown to as much as 2.25rem). +2. The report-word button no longer turns solid accent with dark text on hover or press. +3. A chat draft now survives folding and unfolding the panel, and "Gửi" no longer stays enabled over an empty field. +4. The resign, claim and kick button styles no longer apply to unrelated elements elsewhere in the app that share those class names. + +## Checked + +- The e2e selectors (`getByTestId`, `getByRole('alert')`, `.badge`, `.role`) and the `.resign`/`.claim-dead-end`/`.suggestion` classes used by the unit tests are all still present. Playwright was not run (no browser on this host). +- The wire contract is untouched: `messages.js` did not change, and `turnActions` calls the same builders. + +## Deferred + +- A global `.primary` accent-button class. The same fill, hover, press and disabled styles are repeated in six places (online page, Lobby, WordInput, ChatPanel, GameOverPanel, landing page). Consolidating them runs into Lobby's `.actions button` specificity and needs a visual check that this host cannot do. +- `rules/+page.svelte` repeats the section id in the table of contents and in each `
`. It could be generated from one array, but it is static and easy to read as it is. +- `online/+page.svelte` `h1 { font-size: 1.3rem }` is off the type ramp. +- `startResume()` + `holdPendingJoin()` could be merged into a single `startResume(heldJoin?)`. Left alone to keep the store API and its tests stable. +- The guard in `error-codes.test.js` depends on the server's call-site shape. If the in-flight server refactor changes `toRoom` again, the regex will need updating too. diff --git a/plans/reports/wsapi-refactor-260928-1348-review-and-refactor.md b/plans/reports/wsapi-refactor-260928-1348-review-and-refactor.md new file mode 100644 index 0000000..09b98da --- /dev/null +++ b/plans/reports/wsapi-refactor-260928-1348-review-and-refactor.md @@ -0,0 +1,112 @@ +# wsapi review and refactor (dev vs main) + +Scope: `server/internal/wsapi/` only. Nothing committed. + +## Flake root cause (fixed) + +Three separate races, each proven by repetition before and after its fix. + +1. **Test-harness race — `TestAGameOutlivesItsFirstElimination`, + `TestLeavingMidGameFreesTheSeatAndLeavesTheRestPlaying` (and + `TestResigningOutOfTurnIsRefused`, `TestStandingsReachEverySeat` share the + same block).** Two guests sent `SetReady` from separate connections at + once, and the test awaited only one `room_state` per client before the owner + sent `StartGame`. When the owner's start reached the room before the third + player's ready, the server correctly refused it with `not_everyone_ready`, + and the test then timed out waiting for `game_started`. The improved `await` + diagnostic showed exactly this: `awaiting "game_started": ... (skipped + [error:not_everyone_ready room_state])`. The server was right, so this is not + a server bug. Fix: a new `startWith` helper (multiplayer_test.go) readies the + guests one at a time, drains each resulting `room_state` from every client, + then starts. Before the fix it failed about 1 run in 20 when run in + isolation. After it: 0 failures in 80. +2. **Server ordering bug — `TestChatFromASeatlessConnectionIsRefused` + (turned up by `-race -count=8`).** `lobbyKick` sent `kicked` to the target + *before* `vacate` released the connection's room binding. A client that + reacted to `kicked` right away could get its next frame routed back to the + room, where it was refused as `not_your_seat` instead of `not_in_a_room`. + Fix: vacate first, then notify (room_lobby.go, `lobbyKick`). With + `-race -cpu 1,2,4 -count=100`: 2/300 failures before, 0/300 after. + +3. **Server ordering bug — `TestMetricsCountWordSubmissions`.** + `handleSubmit` sent `move_rejected` *before* `recordRejection` incremented + `noitu_words_rejected`, so a reader that saw the refusal could read the + counter before it moved (`= 2, want 3`). Fix: count first, then send. This + is the order every other metric in the package already uses. With + `-race -cpu 1,2,4 -count=100`: 1/300 failures before, 0/300 after. + +## Bugs fixed (behaviour changes) + +| Sev | Where | Bug | Fix | +|---|---|---|---| +| Low | room_lobby.go `lobbyKick` | A kicked client was told before it was released, so its next action got the wrong refusal (see above) | Release, then send `kicked` | +| Low | room_game.go `handleSubmit` | A rejection was sent to the player before it was counted in metrics (see above) | Count, then send | +| Low | hub.go `newRegisteredRoom` / old `reserveCode` | The code was checked under one lock hold and registered under another, so two creators that drew the same code could overwrite each other's room in `h.rooms`. The room was also built before the capacity check | The capacity check, code draw and registration now run in one critical section (`unusedCodeLocked`) | +| Low | room_presence.go `handleResume` | Two connections presenting the same resume token at once could both pass `hub.resumable`. The second resume displaced the first, which stayed attached to a seat that was no longer its own (never closed, and refused as `not_your_seat` from then on) | Refuse with `session_not_resumable` unless the seat is empty or still held by the session being replaced | + +No wire, close-code, log-line, metric or env-var names changed. + +## Refactors (no behaviour change) + +- **dispatch.go**: one `toRoom(limiter, notIn, dropped, build)` helper replaces + five hand-rolled copies of "charge limiter → find room → send → answer on + drop" (submit, resign, claim, chat, lobby). Every error code is kept per + message. `toLobby` wraps it for the four lobby actions. `allowRoom` replaces + three copies of the room-budget check. `session.handleSubmit` was folded in + and removed. +- **Handshake gate**: `dispatch` now keys on `s.greeted` instead of + `nickname() == ""`. The mutex around `greeted` is gone, because only the + reader goroutine touches it (the same as `reportedWords`). +- **session.go**: `send` and `trySend` shared their encode-and-select body. + Both now go through `enqueue`, which returns `errOutboxFull` or + `errSessionClosed`. `closeOnce` was dropped because `context.CancelFunc` is + already idempotent. `attach` takes a `game.PlayerID`. The no-op + `playerIDFor` cast was removed. +- **room.go**: `connected()` (an `iter.Seq[*seat]`) replaces nine copies of + `if s == nil || s.sess == nil { continue }` across the broadcast paths. + `takeSeat` replaces three copies of seat construction plus attach plus + quick-match dequeue (create, join, bot). `stopCountingLive` replaces the + duplicated `liveCounted` CompareAndSwap. `occupied` is now + `seatedCount() > 0`. `seatIDs` moved next to the seat type. The unused + `sendTo` was removed (callers already hold the acting session). +- **room_presence.go**: `holdSeat` merges `disconnectGhostSeat` with the body + of `handleDisconnect`. +- **room_lobby.go**: `takenNicknames` lost its argument, which never excluded + anything (the joiner's seat is free when it is called). +- **room_game.go**: the `wordsSubmitted` increment sat between a comment and + the code that comment describes. It now comes before the comment. +- **Comments**: removed plan and report references (a `plans/reports/...` path + in metrics.go, "the review's file split", "the improvement report"). + +## Tests + +- The harness `await` now reports every frame it skipped on failure + (`describe`), and `read` is `recv` without the Fatal. +- New helpers: `createRoom`, `joinRoom` (62 inline proto literals replaced), + `agreeAndStart` (the ready-then-start sequence used in about 10 places), + `startWith` (multiplayer). +- `pvpGame` moved into wsapi_test.go, and `startPvP` and `pvpRoom` are now built + on `pvpLobby` plus `agreeAndStart` instead of repeating the handshake. + `awaitNoRooms` uses `hub.roomCount()`. +- No test was deleted or weakened. + +## Verification + +- `go vet ./...` clean, `gofmt -l` clean. +- `go test -race -count=3 ./internal/wsapi/` passes. + `go test -race -count=10 ./internal/wsapi/` passes (290s). + `go test -count=10 ./internal/wsapi/` passes. +- go.mod and go.sum are untouched. + +## Deferred / not changed + +- `handleResign` and `handleClaimDeadEnd` answer a missing game differently + (resign is silent, claim sends `game_not_started`). Merging them would change + the wire behaviour, so both were left as they are. +- `session.attach` sends a `disconnectInput` to the previous room, so a player + who moves from room A to room B holds a reconnect window in A instead of + leaving it at once. This may be intentional, and it is a product call. +- Test bodies in regression_test.go overlap with the topic files (lobby, game, + chat). Merging them needs a case-by-case coverage review and was not done. +- `hub.expireToken` uses `time.AfterFunc`, which is not cancelled on + shutdown. Each timer lives at most GraceFor, so this is harmless.