diff --git a/plans/reports/code-reviewer-260427-1151-full-project.md b/plans/reports/code-reviewer-260427-1151-full-project.md new file mode 100644 index 0000000..af70ea8 --- /dev/null +++ b/plans/reports/code-reviewer-260427-1151-full-project.md @@ -0,0 +1,77 @@ +# Code review — full project (260427-1151) + +Scope: `src/` SvelteKit + Svelte 5. Skip `node_modules`/`.svelte-kit`/`build`. +Focus: runes correctness, bus/state coordination, voice cancellation, edge cases, JSDoc, tests, anti-patterns. +Overall: code is clean, well-commented, JSDoc-typed throughout. One real auto-tick re-mark bug; rest are minor. + +## P0 — blocking + +**1. Auto-tick re-marks cells the user cleared / unticked / regenerated** — `src/lib/PlayerBoard.svelte:136-151`. +The auto-tick `$effect` reads `bus.lastDrawn` AND `crossed` AND `grid` reactively. So it re-fires when `crossed` changes (manual untick, "Xoá đánh dấu", "Tạo bảng mới") even though no new draw happened. With `bus.lastDrawn` still set, it scans for that number and re-marks it. Reproductions in "both" mode: +- Master draws 42. User taps 42 to untoggle → effect re-runs, sees `!crossed[r][c]`, re-marks. User cannot ever untick the latest draw. +- User taps "Xoá đánh dấu" → all marks clear → effect re-fires → 42 re-marked instantly. +- User taps "Tạo bảng mới" → new `crossed` matrix → effect re-fires → 42 marked on the new card. +Fixes: track `lastDrawn.at` (or the bus reference) in a non-reactive ref and early-exit when unchanged; OR call `resetBus()` from `handleClear`/`handleGenerate` AND skip the effect on the very first run after generate. Same hazard exists conceptually with the comment "auto-tick can't fight manual taps" — comment is wrong. + +## P1 — high + +**2. `handleClear` / `handleGenerate` don't reset `bus.lastDrawn`** — same file, lines 153-177. Even after fixing #1, the bus stays sticky across player-card resets. Master `handleNewGame` does reset it (`MasterPanel:139`); player resets should too in "both" mode (or at minimum, snapshot `lastDrawn.at` so post-clear effect knows to ignore stale draws). + +**3. `handleClear` doesn't reset `congratsRow`/`celebrationTier` and leaves `showCongrats`** — `PlayerBoard.svelte:167-177` sets `showCongrats = false` but if a celebration was mid-animation the state is fine. However `congratsRow`/`celebrationTier` aren't reset; harmless until next bingo overwrites. P2. + +**4. Multiple-row simultaneous bingo silently swallowed** — `PlayerBoard.svelte:102-112`. Pass 1 `break`s after first new completion, but pass 2 won't surface another. Not reachable today (one cell-toggle = one row at most). But auto-tick + cross-row coincidence + future logic changes could expose it. Add a short queue or track "pending celebrations" if you ever batch-mark. + +**5. Bingo modal Escape only works when backdrop has focus** — `PlayerBoard.svelte:393-402`. `onkeydown={onModalKeydown}` is on the backdrop `