From 44ba72b36a46950fe7c15693e91cc89e36d1b261 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 27 Apr 2026 20:51:40 +0700 Subject: [PATCH] docs: housekeeping after whole-project review - README: drop incorrect 'syncs across all devices' claim about localStorage progress. - package.json: description still said Phaser 3; it's Svelte 5 now. - Mark the two old plans as completed (work shipped long ago). - Add the 4 review reports to plans/reports/. --- README.md | 2 +- package.json | 2 +- plans/260411-2027-sokoban-overhaul/plan.md | 2 +- plans/260412-0002-svelte-migration/plan.md | 2 +- ...viewer-260427-2036-whole-project-review.md | 265 +++++++++++++ ...plifier-260427-2036-whole-project-audit.md | 368 ++++++++++++++++++ .../debugger-260427-2040-project-health.md | 170 ++++++++ ...security-scan-260427-2050-whole-project.md | 78 ++++ 8 files changed, 885 insertions(+), 4 deletions(-) create mode 100644 plans/reports/code-reviewer-260427-2036-whole-project-review.md create mode 100644 plans/reports/code-simplifier-260427-2036-whole-project-audit.md create mode 100644 plans/reports/debugger-260427-2040-project-health.md create mode 100644 plans/reports/security-scan-260427-2050-whole-project.md diff --git a/README.md b/README.md index d549a20..ed02084 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ Play: [https://tiennm99.github.io/sokoban/](https://tiennm99.github.io/sokoban/) - **Mobile-optimized**: Touch-safe controls, haptic feedback (vibrate on push & win), safe-area insets for notches/nav bars, browser gesture blocking. - **Installable PWA**: Add to home screen on iOS/Android, play offline, standalone fullscreen mode. - **Undo history**, live move counter, animated moves. -- **Progress saved** locally in `localStorage`, syncs across all devices you use. +- **Progress saved** locally in `localStorage` (per browser, per device). - **Responsive tile sizing** so small and large levels both look right on any screen. ## Development diff --git a/package.json b/package.json index b73e7c8..eb605cd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sokoban", - "description": "A simple Sokoban game built with Phaser 3 and Vite", + "description": "A simple Sokoban game built with Svelte 5 and Vite", "version": "1.0.0", "type": "module", "repository": { diff --git a/plans/260411-2027-sokoban-overhaul/plan.md b/plans/260411-2027-sokoban-overhaul/plan.md index f3005df..8907eec 100644 --- a/plans/260411-2027-sokoban-overhaul/plan.md +++ b/plans/260411-2027-sokoban-overhaul/plan.md @@ -1,7 +1,7 @@ # Sokoban Overhaul **Date:** 2026-04-11 -**Status:** In Progress +**Status:** Completed (work shipped; see git log for details) ## Goal - Replace 3 hand-crafted levels with 100 solvable Microban levels (David W. Skinner, public, freely distributable). diff --git a/plans/260412-0002-svelte-migration/plan.md b/plans/260412-0002-svelte-migration/plan.md index 2db84af..65c32f0 100644 --- a/plans/260412-0002-svelte-migration/plan.md +++ b/plans/260412-0002-svelte-migration/plan.md @@ -1,7 +1,7 @@ # Svelte Migration **Date:** 2026-04-12 -**Status:** In progress +**Status:** Completed (work shipped — see commit 8a3d4b4 `feat!: rewrite on Svelte 5, drop Phaser`) ## Goal Replace Phaser 3 with Svelte 5 as the rendering/UI layer. Keep framework-agnostic modules untouched. Ship a smaller, more structured, natively-clickable version of the same game. diff --git a/plans/reports/code-reviewer-260427-2036-whole-project-review.md b/plans/reports/code-reviewer-260427-2036-whole-project-review.md new file mode 100644 index 0000000..ea9ea78 --- /dev/null +++ b/plans/reports/code-reviewer-260427-2036-whole-project-review.md @@ -0,0 +1,265 @@ +--- +agent: code-reviewer +date: 2026-04-27 +slug: whole-project-review +plan: 260427-1151-mobile-comfort +--- + +# Sokoban — Whole-Project Adversarial Review + +Full-tree review (Svelte 5 + Vite, GH Pages, PWA). Mobile-comfort items already covered in `code-reviewer-260427-2023-mobile-comfort-review.md` are skipped here. + +## Scope +- 1395 LOC across `src/` (excl. microban-levels.js) +- Configs: `vite/*.mjs`, `package.json`, `index.html` +- Plans: `plans/260411-2027-*`, `plans/260412-0002-*`, `plans/260427-1151-*` + +## Overall Assessment +Code is small, clean, and idiomatic for Svelte 5 runes. KISS/DRY mostly respected. Real issues center on **global keyboard listener collisions** and a few small parser/storage robustness gaps. Nothing blocks shipping. Most findings are hygiene + a11y. + +--- + +## Critical + +### C1. Escape on win+donate triggers BOTH modal close AND navigate-to-levels +**Files:** `src/views/DonateModal.svelte:10-12, 22`, `src/views/GameView.svelte:99-100, 140` + +Both components register independent `` listeners. When `won === true` and the user opens "BUY ME A COFFEE", the win-overlay modal sits on top, but **GameView's listener is still active**. Pressing Escape: +1. DonateModal `onKey` → `onClose()` → modal closes. +2. GameView `onKey` matches Escape → `onLevels()` → navigates away. + +Net effect: user is yanked to the level-select screen even though they only wanted to dismiss the donate modal. Reproduces also from MenuView donate (pressing Escape triggers nothing else — no bug there) and from any future modal stacked over GameView. + +Same hazard exists for `R` (restart) and `U`/`Z` (undo) keys — those bypass even the donate modal because DonateModal only handles Escape. So keyboard users can accidentally restart the level while reading the QR. + +**Fixes (pick one):** +- Add an `inert` flag prop / module-level "modal open" guard that GameView's `onKey` checks. +- Track a tiny global `modalsOpen` count in a store; GameView's `onKey` early-returns when > 0. +- Or, in DonateModal `onKey`, also intercept R/U/Z/Arrows when open and call `e.stopPropagation()` — but `` listeners are siblings, stopPropagation between them does NOT prevent the other from firing. So this approach won't work; needs a shared flag. + +Smallest fix: a module-scoped `let isModalOpen = $state(false)` in a shared store, GameView's `onKey` returns early if true. + +--- + +### C2. Win-overlay dialog has no focus management or focus trap +**File:** `src/views/GameView.svelte:182-198` + +When win overlay opens, focus stays wherever it was (probably body on touch). On desktop: +- Tab order continues through HUD, Board (no focusables), and finally reaches the overlay buttons. +- An external keyboard user has no idea where they are. +- Screen readers don't announce a dialog because the overlay isn't `role="dialog"` and lacks `aria-modal`. + +The `
` is presentational; the inner `.dialog` has no role. Add: +- `role="dialog" aria-modal="true" aria-labelledby="winTitle"` on the dialog. +- Auto-focus the primary action (`NEXT LEVEL` if `hasNext`, else `LEVELS`). +- Wrap focus inside the dialog (or accept escape risk and at least set initial focus). + +(The earlier mobile-comfort review noted the auto-focus part as Low priority #20; flagging higher here because there's also no role/aria.) + +--- + +### C3. DonateModal lacks focus management — same a11y gap +**File:** `src/views/DonateModal.svelte:24-37` + +`role="dialog" aria-modal="true"` is set ✓. But: +- `tabindex="-1"` on dialog without programmatic `.focus()` on open → keyboard users start tabbing from body, hit nothing useful. +- No focus trap: Tab leaves the modal back to the underlying view's buttons. +- No restoration of focus to the trigger button on close. + +Use a `$effect` to focus the dialog (or its CLOSE button) on `open` becoming true; cache the prior `document.activeElement` and restore on close. + +--- + +## High + +### H1. `progressStore` does N+1 localStorage reads on level-select page render +**File:** `src/views/LevelSelectView.svelte:20-32` + `src/lib/core/progress-store.js:9-24` + +Each `progressStore.isCompleted(i)` and `getBestMoves(i)` call does a fresh `localStorage.getItem` + `JSON.parse`. For PER_PAGE=20 that's 40 parses per page change. Each `readRaw()` rebuilds the same object. Trivial CPU hit on desktop, real jank on cold-start low-end Android with the new SW also booting. + +**Fix:** add `progressStore.snapshot()` returning the full record object once; map over it in `visibleLevels`. Keeps API stable, eliminates the 39 redundant parses. + +### H2. `progressStore` has no schema-version handling for future bumps +**File:** `src/lib/core/progress-store.js:7` + +Key is hard-coded `sokoban-progress-v1`. If you ever change the shape (e.g. add `time` per level), you must either: +- Bump to `-v2` (orphans v1 data — users lose progress silently). +- Migrate v1 → v2 inline. + +Neither path exists today. Fine for the current design, but add a `_v: 1` field inside the JSON and a `migrate(data)` step in `readRaw` so the next bump is painless. Document this in code-standards.md. + +Also: `readRaw` accepts ANY shape from JSON.parse — if a hostile extension or another tab corrupts the value with `{completed: "totally"}`, `data.completed[levelIndex]` becomes `"o"` (truthy), `getCompletedCount` returns `Object.keys("totally").length === 7`. Add a shape check: if `completed` isn't an object, fall back to default. Same for `bestMoves`. + +### H3. `level-parser.js` silently merges malformed levels +**File:** `src/lib/core/level-parser.js:20-23` + +`parseGrid` filters out blank lines and `;` comments, so a future maintainer who adds a level with a blank middle row, or two levels accidentally pasted into one backtick string with a blank separator, will get them merged with no warning. Multiple `@` characters likewise overwrite each other silently (last wins). + +Defensive validations (cheap, ~8 lines): +- After `extractEntities`, `console.warn` if multiple `@` are found. +- `console.warn` if `boxes.length !== targets.size`. +- `console.warn` if `floors.size === width * height` (suggests an unsealed level — flood-fill escaped). +- Throw if `player == null` (already handled in GameView; move to parser to fail loudly at module-load if levels ever ship broken). + +Microban data is solid today; this is forward-defense for forks / additions. + +### H4. PWA-only-in-prod is undocumented, easy to break +**Files:** `vite/config.{dev,codeserver,prod}.mjs` + +`VitePWA` only registered in `config.prod.mjs`. **This is the right choice** (avoids stale SW during dev/HMR), but there's no comment in any of the three configs explaining the asymmetry. Next maintainer "harmonising" the configs will accidentally enable SW in dev and create caching nightmares. + +**Fix:** one-line header comment in each of the three configs: +- `config.dev.mjs`: `// SW intentionally NOT registered — see config.prod.mjs` +- `config.codeserver.mjs`: same +- `config.prod.mjs`: `// SW only in prod; auto-injected via vite-plugin-pwa default registerType` + +--- + +## Medium + +### M1. GameView.svelte is 295 LOC — exceeds documented 200-LOC limit +**File:** `src/views/GameView.svelte`, ref: `docs/code-standards.md:15-17` + +CSS bulk is ~90 lines so the JS+template is ~205. Borderline. Consider extracting: +- `WinOverlay.svelte` — the `{#if won}` block (and own role/aria, fixes C2 cleanly). +- A `hooks/useTileSize.js` or just inline `computeTileSize` plus a small `useResizeEffect` helper. + +Not blocking; flagging because the standard is project-internal and reviewer-cited. + +### M2. Stale plan files marked "In Progress" +**Files:** +- `plans/260411-2027-sokoban-overhaul/plan.md:4` — Status: In Progress (work shipped in commit `1d2fff6`, then expanded in `2ee7ac8` + `8a3d4b4`). +- `plans/260412-0002-svelte-migration/plan.md:4` — Status: In progress (Svelte rewrite shipped in `8a3d4b4`). + +Both should be flipped to `Status: Complete` (or moved to a `plans/archive/` dir) so the planner agent's "active plans" listing is accurate. Pure housekeeping. + +### M3. iOS `100vh` issue in `.board-wrap` height calc +**File:** `src/views/GameView.svelte:235, 243` + +`max-height: calc(100vh - 140px)` (and -260px on coarse) uses `100vh`, which on iOS Safari does NOT shrink when the URL bar is visible. On phones, the bottom of the board can slide UNDER the bar. Modern fix: `100dvh` (supported iOS 15.4+, Chrome 108+, FF 101+ — same target audience as the PWA). + +### M4. `MenuView.svelte` reads `getCompletedCount()` once at component init +**File:** `src/views/MenuView.svelte:13` + +`const completed = ...` not `$state(...)`. Currently fine because `App.svelte`'s `{#if view ===}` remounts MenuView when navigating back. But the moment someone refactors App to keep MenuView mounted (e.g. for a transition animation), the counter goes stale. + +Same caveat in `LevelSelectView.svelte:18` — `let completedCount = $state(...)` but never updated. Add a comment, or read it inside `$derived.by` with a dummy reactive trigger when needed. + +### M5. `boxAt` is O(n) called twice per move +**File:** `src/lib/core/board-model.js:21-23, 39-43` + +For Microban max ~50 boxes this is negligible. But `tryMove` calls `boxAt(nx, ny)` then `boxAt(bx, by)` — that's 2n comparisons. Trivial today; if levels ever go bigger, consider a `Map` rebuilt on each move. YAGNI for now — flagging only. + +### M6. `level-parser.js` flood-fill uses a stack of objects (allocs) +**File:** `src/lib/core/level-parser.js:64-79` + +`stack.push({ x, y })` allocates on every neighbor visit — for the giant 50×50 finale level that's ~thousands of allocs per parse. Parse runs once per level load (rare). Acceptable. Could pack as `x * width + y` integers, but YAGNI. + +--- + +## Low + +### L1. `Object.keys(readRaw().completed).length` counts even falsy values +**File:** `src/lib/core/progress-store.js:46` + +If anything ever writes `data.completed[i] = false` (no current code path, but defensive), the count would be wrong. Cheap fix: `Object.values(readRaw().completed).filter(Boolean).length`. + +### L2. `MICROBAN_LEVELS[levelIndex]` not bounds-checked +**File:** `src/views/GameView.svelte:21` + +If `levelIndex` > 154, `MICROBAN_LEVELS[levelIndex]` is `undefined`, parseLevel splits `undefined`, throws TypeError, caught by try/catch, error displayed. Fine, but UX is a generic "Failed to load level X" — could be a clearer "Level X does not exist". `App.svelte`'s `playLevel(levelIndex + 1)` on the win screen could also bump past the end (`hasNext` guards UI but not API). Belt-and-braces. + +### L3. `won` flag is never reset when GameView remounts on level change — but that's by design +**File:** `src/App.svelte:24` (`{#key levelIndex}`) + `src/views/GameView.svelte:60` + +The keyed remount makes `won = $state(false)` re-init. Confirmed correct. Just noting that this depends on the `{#key}` — if removed, `won` would persist across levels. Comment in `App.svelte:24` explaining the keying intent already exists ✓. + +### L4. README claims "syncs across all devices you use" for localStorage +**File:** `README.md:14` + +Misleading — localStorage is per-browser-per-device. Should read "Progress saved locally" or "saved in your browser". Minor honesty fix. + +### L5. `index.html` title is "Sokoban — 155 Puzzles" — magic number +**File:** `index.html:9` + +If level count ever changes, this drifts. Not worth scripting; just flag. + +### L6. `apple-touch-icon` referenced in `index.html` with relative path +**File:** `index.html:6` + +`href="./apple-touch-icon.png"` works because `base: '/sokoban/'` is set in prod and the file is in `public/`. On dev (`base: './'`) and codeserver (`base: '/absproxy/...'`), the resolved path differs but the icon is still in `public/` so it works. Just noting the coupling. + +### L7. `package.json` description still says "Phaser 3" +**File:** `package.json:3` + +`"description": "A simple Sokoban game built with Phaser 3 and Vite"` — outdated since the Svelte rewrite (`8a3d4b4`). Flip to "Svelte 5 + Vite". 1-second fix. + +### L8. CSS `.error` class in GameView has no test path +Flagged as "manual smoke test only" in `docs/code-standards.md:38` — acceptable, no action. + +### L9. `app.css` `body { user-select: none }` blocks copy of "Best: N" stat +**File:** `src/app.css:55-58` + +Aggressive but intentional for game feel. Some users may want to copy their best score for sharing. Cosmetic only. + +--- + +## Edge Cases Found by Scout + +- **DonateModal `onClose` undefined?** `let { open = false, onClose } = $props();` — if a parent forgets to pass `onClose`, calling it throws `TypeError: onClose is not a function`. Both call sites (MenuView, GameView) pass it. Add a default `onClose = () => {}` for safety. +- **`computeTileSize` returns 48 when level is null** but `tileSize = $state(computeTileSize())` runs before parseError is set. The Board then never renders (`parseError` branch returns early). No issue. +- **`tryMove` won-gate vs. async**: `won` is a `$state` — synchronously updated in `syncFromModel`. Subsequent `tryMove` calls in the same JS task see the updated value. No race. +- **`#key levelIndex` remount discards the resize listener** — Svelte tears down `` correctly. ✓ +- **Level data: 155 backtick strings, all non-empty, no embedded `;` comments, no exotic chars** — verified via grep count. ✓ +- **`floors.has` lookup for wallCells** uses on-the-fly template literal `\`${x + dx},${y + dy}\`` — correct, matches `cellKey` format. ✓ +- **`pulse(60)` after `pulse(10)` in same tick** — `navigator.vibrate(60)` overrides the prior. Already noted in prior review #14. No new finding. +- **`progressStore.recordCompletion` ignores quota errors silently** — losing a high-score record on quota-full is acceptable for a tiny progress blob (~1 KB max). + +--- + +## Positive Observations + +- `core/` is genuinely framework-free, untouched by the Svelte rewrite. Architecture goal honored. +- `progress-store.js` `try/catch` around localStorage is correct (private mode / disabled storage). +- `BoardModel.tryMove` returns booleans cleanly, history records exactly what's needed for undo. Tight design. +- `Board.svelte` is purely presentational; the `wallCells` optimization (only render walls that touch a floor) is the right call for the "lots of border `#`" XSB format. +- `App.svelte` `{#key levelIndex}` is the cleanest possible "reset on level change" pattern. +- `haptics.js` defensive coding (`typeof navigator`, try/catch around vibrate) is exactly right for the mixed-environment target. +- `AppButton.svelte` wraps native `