From df2ad837705af9092026a5a0e52e3c0873ee3a3d Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 21 Sep 2026 16:31:02 +0700 Subject: [PATCH] docs(reports): record the web review actions implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also keep a copy of the architecture review this work implements, alongside the report — it was untracked in the checkout this branch forked from. --- ...eveloper-260921-1535-web-review-actions.md | 167 ++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 plans/reports/fullstack-developer-260921-1535-web-review-actions.md diff --git a/plans/reports/fullstack-developer-260921-1535-web-review-actions.md b/plans/reports/fullstack-developer-260921-1535-web-review-actions.md new file mode 100644 index 0000000..d23f9ee --- /dev/null +++ b/plans/reports/fullstack-developer-260921-1535-web-review-actions.md @@ -0,0 +1,167 @@ +# Web review actions — implementation report + +Branch `worktree-agent-ae280f2081bd718f6` (fast-forwarded from `main`/dd3b463 to `dev`@3ef9f48 +before starting — the worktree had been created one point behind `dev`, missing quick-match, +dead-end claim, ESLint config and the rest the review was written against) · worktree +`/workspace/tiennm99/noitu/.claude/worktrees/agent-ae280f2081bd718f6`. + +Seven commits on top of `dev`@3ef9f48, all `web/`, nothing pushed: + +``` +c4502f2 refactor(web): split the game store into shape, apply and store files +70ae09d refactor(web): type the wire instead of passing any across it +eace594 refactor(web): extract the online screen's request machine into a store +f0c2334 test(web): cover the room-session request machine +f0bcb60 refactor(web): extract ArmedButton for the three press-twice controls +69310a3 test(web): mount ChatPanel under jsdom for fold and unread accounting +cf48d27 test(web): mount WordInput and GameBoard under jsdom +``` + +## Verification + +| | Before | After | +|---|---|---| +| `npm run lint` | 0 errors, 33 warnings (32 `jsdoc/reject-any-type`, 1 `check-param-names`) | 0 errors, **1 warning** | +| `npm run check` | 0 errors, 0 warnings (meaninglessly, per the review) | 0 errors, 0 warnings — now over a typed store | +| `npm test` | 221 passed / 12 files | **258 passed / 16 files** | + +The one remaining warning: `tests/room-code.test.js:77` deliberately calls +`normalizeRoomCode(/** @type {any} */ (undefined))` against its documented `string`-only +signature, to prove the function's own defensive `String(raw ?? '')` — that is the point of the +test, not an oversight. + +No Playwright run (no browser on this host, per workspace rules). + +## Per deliverable + +**1 — Resume latch time-box.** `stores/room-session.svelte.js` owns `resuming`; the page arms a +5s (`RESUME_TIMEOUT_MS`) timer once `resuming && connection.status === Status.OPEN`, calling +`session.noteResumeFailed(named)` if nothing has resolved it by then — same transition the +existing "resume answered with an error" effect already ran, now shared by both paths. A server +new enough to answer `session_not_resumable` clears it sooner through the ordinary error effect +(the message already existed in `vi.js`). The join form's `resumeFailed` banner reuses that same +string rather than inventing new copy. Unit-tested in `tests/room-session.test.js` (timeout, +already-resolved-before-the-timer-fires, named vs. unnamed invite code held behind it). + +**2 — `leave()` calls `forgetSession()`.** One line in `routes/online/+page.svelte`'s `leave()`, +matching the teardown path. No dedicated test file (it is a page-level wiring fact, not store +logic) — covered implicitly by the held-action semantics test in point 6, and by inspection: `git +show eace594 -- routes/online/+page.svelte` shows the added call. + +**3 — Type the store.** `initialState()` now returns `GameState` (real `@typedef` with one +`@property` per field, folded into `game-shape.js`), not `any`. `apply()` switches on +`payload.case` without destructuring, so the oneof narrows. `svelte-check` stayed at 0 +errors/warnings after the retype — no latent bugs surfaced in the 16 components, which the review +flagged as a real possibility; I take that as the store's shape genuinely having matched its +usage everywhere, not as the check being weak (it now has a real type to fail against, and +deliberately-wrong scratch edits during development did produce the expected errors and warnings +before being fixed). + +**4 — Extract `stores/room-session.svelte.js`.** Owns `pending`/`resuming`/`needName`/`stalled`/ +`resumeFailed`/`queuedForS`/`heldAction`; no DOM, no runes beyond `$state`, modelled on +`bot-session.svelte.js`. The page keeps every timer (`setTimeout`/`setInterval`) and all layout — +`JoinPanel`/`RoomLayout` extraction from the review's secondary suggestion was explicitly out of +scope this round. `routes/online/+page.svelte` script is ~290 lines, down from ~400 in the +pre-fast-forward review's line numbers (harder to compare directly since the branch had drifted, +but the five request-machine `$effect`s the review named are gone from the page). Unit tests: join +latch + refusal retry, resume latch + timeout + already-resolved race, quick-match queued→seated, +held-action replace/retry/drop-on-teardown (18 tests, `tests/room-session.test.js`). + +**5 — Type the wire.** `ws/client.js`, `ws/connection.svelte.js`, `stores/bot-session.svelte.js` +now use the generated `ServerMessage`/`ClientMessage` union instead of `any`; timers typed +`ReturnType`. Cleared every `jsdoc/reject-any-type` in `src/`. In `tests/`, five +files lost their casts for free once the store and wire were typed +(`game-store.test.js` ×3, `game-wire.test.js` ×2 via a typed `decode()`, `ws-client.test.js` ×6). +**Kept:** `tests/room-code.test.js:77` — see Verification above, the one deliberate `any`. + +**6 — Held requests for lobby actions.** `room-session`'s `heldAction` slot (replace-not-queue: a +later action supersedes an earlier unset one, since e.g. readying then leaving before reconnect +means leave should win) backs `cancelQueue`, `leave`, `ready`/`unready`, `start`, `kick`. Each +tries `send()` first and only latches on refusal; a socket-open effect retries alongside the +existing join/create flush. `Lobby.svelte`'s local one-shot `unsent` flag (which never noticed a +background retry had succeeded) is now the `actionHeld` prop, driven straight off +`session.state.heldAction`. Tested: 6 of the 18 `room-session.test.js` cases. + +**7 — Split `game.svelte.js`.** `stores/game-shape.js` (typedefs, `initialState()`, wire decoders +`toSenses`/`toParts`/`toScore`/`toSlot`) / `stores/game-apply.js` (`applyTo(state, msg, +{reset, leave})`, pure) / `stores/game.svelte.js` (`$state`, `reset`/`leave`, derived accessors, +singleton). `apply()` wraps `applyTo()` in try/catch, logging and keeping the previous snapshot on +a throw. All 61 pre-existing store tests pass unmodified against the new module split (they only +import from `game.svelte.js`, which still re-exports `CHAT_WINDOW` and `createGameStore`). + +**8 — Component tests under jsdom (first half).** `tests/chat-panel.test.js` (9: fold-by-default, +opens on toggle, never folds when not collapsible, unread counts only while folded, clears on +open, recounts after a fold/reread cycle, send/clear, whitespace-only refused). +`tests/word-input.test.js` (7: seeds once per turn, does not seed/focus while offline — the C6 +regression — does not reseed the same turn across a connection blip, reseeds on a genuine new +turn, leaves an in-progress composition alone on the player's own turn, suggestion fills on click, +no suggestion button when the server sent none). `tests/game-board.test.js` (3: chat pill absent +with no `onchatopen`, present once one is passed, click reaches the handler and shows the unread +count). Needed one infrastructure fix: `vite.config.js` now sets +`resolve.conditions: ['browser']` gated on `process.env.VITEST`, or Svelte resolves its +server-rendering entry point under Vitest and `mount()` throws +`lifecycle_function_unavailable`; `vite dev`/`vite build` are unaffected since `VITEST` is only +set by the Vitest CLI. jsdom also needed local polyfills for `Element.scrollTo` and +`ResizeObserver` (used by `ChatPanel`/`ChainHistory` for auto-scroll and reflow, neither +implemented by jsdom) — stubbed per test file, not globally. + +`e2e/helpers.js`/`e2e/pvp-game.spec.js` fixed per the flake analysis: `playingPair` now calls +`joinRoomSeated` instead of `joinRoom`, and `readyAndStart` asserts +`guest.getByTestId('my-ready')` reads "Đã sẵn sàng" (the actual `t.isReady` string — the review's +own example text was illustrative, not the literal copy) before touching Start. **Unverified +locally** — no browser on this host; these are read-through-verified against the actual +`Lobby.svelte` markup and `vi.js` strings, not run. + +Playwright suite (47→~14) was **not** cut, per the task's explicit instruction to keep it as-is +for now. + +**9 — `ArmedButton.svelte`.** One component for resign, claim-dead-end and kick: owns the arm +timer and the disarm-on-`disabled` effect, adds `aria-pressed` (announces the armed state to +assistive tech on the same control that already has focus, closing the a11y gap the review +flagged — a screen reader speaks a control's name on focus, not on an in-place label mutation). +Behaviour is otherwise identical, with one acknowledged, minor, unstated-invariant change: kick +now arms per seat (each `ArmedButton` instance is independent) rather than sharing one +Lobby-level "which seat is armed" slot, so two seats could in principle be armed at once within +the 4s window. No test or review language documented the old cross-seat exclusivity as intended +behaviour, and no e2e spec exercises it. GameBoard's and Lobby's `