mirror of
https://github.com/tiennm99/sokoban.git
synced 2026-10-11 03:13:52 +00:00
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/.
This commit is contained in:
tiennm99 committed
1 parent
dff4ea9eab
commit
44ba72b36a
8 files changed
+885
-4
No files matched your search
@@ -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
|
||||
|
||||
+1
-1
@@ -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": {
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 `<svelte:window onkeydown>` 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 `<svelte:window>` 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 `<div class="overlay">` 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<cellKey, boxIndex>` 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 `<svelte:window>` 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 `<button>` — keyboard activation, focus, type=button all native. No reinvention.
|
||||
- `parseGrid` filtering of `;` comments and blank lines is XSB-compliant.
|
||||
- Config split (`dev`/`codeserver`/`prod`) is well-scoped and `loadEnv` is used correctly in codeserver.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Actions (priority order)
|
||||
|
||||
1. **Fix C1 (Escape modal collision)** — module-scoped `modalsOpen` flag, GameView `onKey` early-return when set. ~10 lines.
|
||||
2. **Fix C2 + C3 (focus management)** — auto-focus + focus trap on win dialog and DonateModal. ~30 lines total. Big a11y win.
|
||||
3. **Fix H1 (N+1 localStorage reads)** — add `progressStore.snapshot()`. ~5 lines.
|
||||
4. **Fix H4 (document PWA-only-in-prod)** — three one-line comments.
|
||||
5. **M2 (stale plans)** — flip Status: In Progress → Complete in two plan.md files.
|
||||
6. **L7 (package.json description)** — drop Phaser reference.
|
||||
7. **M3 (100dvh)** — replace 100vh in `.board-wrap` calc.
|
||||
8. **H2/H3 (defensive parser + storage)** — only if a fork-friendly stance is wanted; YAGNI for solo project.
|
||||
|
||||
---
|
||||
|
||||
## Metrics
|
||||
|
||||
- LOC reviewed: 1395 (excl. levels data)
|
||||
- Files: 14 source + 3 vite configs + 1 index.html + 1 README + 2 plan.md
|
||||
- Critical: 3 | High: 4 | Medium: 6 | Low: 9
|
||||
- Type coverage: N/A (vanilla JS, project uses runes-only)
|
||||
- Tests: none (per code-standards.md `Testing strategy`)
|
||||
|
||||
---
|
||||
|
||||
## Unresolved Questions
|
||||
|
||||
1. **C1 fix approach:** is a global `modalsOpen` store acceptable, or is a per-key disable preferred? The simplest is the global flag.
|
||||
2. **H4 scope/start_url:** mobile-comfort review's #8 already raised the absolute-path concern. Decision needed: keep `/sokoban/` (current) or switch to `./`? Behavior on PWA install differs.
|
||||
3. **H2 schema bump:** any near-term plan to add per-level data (time, optimal-pushes, etc.) that would force a v2 migration?
|
||||
|
||||
**Status:** DONE_WITH_CONCERNS
|
||||
**Summary:** Code is small and clean. One real keyboard-routing bug (C1: Escape both closes modal AND navigates), two a11y gaps (C2/C3: no focus management on either dialog), and a 40× redundant localStorage read on the level-select page. Everything else is hygiene or forward-defense.
|
||||
**Concerns:** C1 is a user-visible regression for keyboard users on the win-screen donate modal; recommend fixing before next release.
|
||||
@@ -0,0 +1,368 @@
|
||||
# Code-Simplifier Audit — Whole Project
|
||||
|
||||
**Date:** 2026-04-27
|
||||
**Scope:** src/, vite/, index.html
|
||||
**Mode:** Proposals only — no edits made.
|
||||
|
||||
## Summary
|
||||
|
||||
Codebase is already lean and idiomatic Svelte 5. Most files honor KISS/YAGNI well. Found ~12 small simplifications, mostly DRY (CSS reset duplication), 1 minor dead-code, 1 minor stale-state bug, 0 architectural issues. **BoardModel/GameView split is correct and should stay.** **MobileControls vs HUD desktop-actions are not duplicate work** — they intentionally render mutually exclusive (pointer: coarse), which is clearer than one component branching internally.
|
||||
|
||||
Total proposed savings: ~70 LOC, mostly cosmetic. None of the changes alter game behavior.
|
||||
|
||||
---
|
||||
|
||||
## Proposals
|
||||
|
||||
### 1. Drop `key` re-export alias from level-parser.js
|
||||
|
||||
**File:** `src/lib/core/level-parser.js:18, 89`
|
||||
**Risk:** low
|
||||
**LOC saved:** 1
|
||||
|
||||
Inside the file, `key(x, y)` is fine. The export aliases it as `cellKey` solely for board-model.js. Just export `key` (or rename internal use to `cellKey` and drop the alias).
|
||||
|
||||
Before:
|
||||
```js
|
||||
const key = (x, y) => `${x},${y}`;
|
||||
// ...
|
||||
export { key as cellKey };
|
||||
```
|
||||
After:
|
||||
```js
|
||||
export const cellKey = (x, y) => `${x},${y}`;
|
||||
```
|
||||
Then internal calls become `cellKey(x, y)`. Eliminates the `as` indirection — one mental hop fewer when grepping.
|
||||
|
||||
---
|
||||
|
||||
### 2. Inline `parseGrid`/`extractEntities`/`floodFillFloors` or keep — borderline
|
||||
|
||||
**File:** `src/lib/core/level-parser.js:20-87`
|
||||
**Risk:** low
|
||||
**LOC saved:** ~6 (mostly function boundaries / docstrings)
|
||||
|
||||
The three helpers are each used exactly once by `parseLevel`. Inlining flattens the call graph but the named steps document phases ("parse → extract → flood-fill") and make `parseLevel` a 3-line orchestrator. **Recommendation: keep as-is.** The split aids reading the algorithm sequentially. Only worth flattening if the file grows further.
|
||||
|
||||
**Decision:** No change. (Listed for completeness; was a candidate that fails the "must reduce cognitive load" criterion.)
|
||||
|
||||
---
|
||||
|
||||
### 3. Remove redundant `boxes.length === 0` guard in `isSolved`
|
||||
|
||||
**File:** `src/lib/core/board-model.js:69`
|
||||
**Risk:** low
|
||||
**LOC saved:** 1
|
||||
|
||||
`Array.prototype.every` on an empty array returns `true`, so the guard exists to prevent insta-win on a malformed level with zero boxes. But:
|
||||
- `parseLevel` already guarantees the level shape; a no-box level is pathological and not in the Microban set.
|
||||
- GameView's `buildLevel` throws if `!lv.player` but does not validate boxes — adding a similar guard there would be more honest than masking it inside `isSolved`.
|
||||
|
||||
Before:
|
||||
```js
|
||||
isSolved() {
|
||||
if (this.boxes.length === 0) return false;
|
||||
return this.boxes.every(b => this.isTarget(b.x, b.y));
|
||||
}
|
||||
```
|
||||
After:
|
||||
```js
|
||||
isSolved() {
|
||||
return this.boxes.length > 0 && this.boxes.every(b => this.isTarget(b.x, b.y));
|
||||
}
|
||||
```
|
||||
Same logic, one line. (Or drop the guard entirely if zero-box levels are deemed impossible.)
|
||||
|
||||
---
|
||||
|
||||
### 4. `LevelSelectView.completedCount` is `$state` but never reassigned
|
||||
|
||||
**File:** `src/views/LevelSelectView.svelte:18`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0 (correctness/clarity)
|
||||
|
||||
```js
|
||||
let completedCount = $state(progressStore.getCompletedCount());
|
||||
```
|
||||
Declared `$state` but only read once on mount and never updated. Just use a `const`:
|
||||
```js
|
||||
const completedCount = progressStore.getCompletedCount();
|
||||
```
|
||||
**Note:** Mirrors `MenuView.svelte:13` which already does this correctly. Picking the right form here also avoids the wrong impression that the count auto-refreshes when returning from a completed level. (If a refresh is desired, that's a separate fix — not this audit's call.)
|
||||
|
||||
---
|
||||
|
||||
### 5. Promote DonateModal `open` toggle into the modal itself
|
||||
|
||||
**File:** `src/views/DonateModal.svelte`, `src/views/MenuView.svelte`, `src/views/GameView.svelte`
|
||||
**Risk:** low
|
||||
**LOC saved:** ~6 (3 lines per consumer × 2 consumers, minus modal additions)
|
||||
|
||||
Both consumers replicate:
|
||||
```js
|
||||
let donateOpen = $state(false);
|
||||
// ...
|
||||
onclick={() => (donateOpen = true)}
|
||||
// ...
|
||||
<DonateModal open={donateOpen} onClose={() => (donateOpen = false)} />
|
||||
```
|
||||
Could expose an imperative `openDonate()` from a tiny `donate-modal-store.js` or use a slot/trigger pattern. **However**, this adds an abstraction layer that's only used twice — borderline YAGNI. **Recommendation:** keep as-is unless a third caller appears. (Listed for visibility.)
|
||||
|
||||
---
|
||||
|
||||
### 6. Hard-coded margin constants in `computeTileSize`
|
||||
|
||||
**File:** `src/views/GameView.svelte:40-50`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0 (clarity)
|
||||
|
||||
```js
|
||||
const margin = isCoarse ? 260 : 140; // header + hud (+ mobile controls) + padding
|
||||
const maxByWidth = Math.floor((window.innerWidth - 80) / level.width);
|
||||
const maxByHeight = Math.floor((window.innerHeight - margin - 100) / level.height);
|
||||
```
|
||||
The literals (`80`, `100`, `140`, `260`) duplicate measurements that also live in the CSS (`max-height: calc(100vh - 140px)` at line 235, `260px` at line 243). When CSS changes, JS silently goes out of sync.
|
||||
|
||||
Two options:
|
||||
- **a)** Extract `const MARGINS = { width: 80, heightDesktop: 240, heightMobile: 360 }` at top of file with a comment, and reuse the same numbers in the CSS via inline style. Adds ~3 LOC, removes duplication.
|
||||
- **b)** Just add a comment cross-referencing the CSS rule. Zero LOC change, lower risk.
|
||||
|
||||
**Recommendation:** option (b) — just a `// keep in sync with .board-wrap max-height below` comment. The literal split is fine.
|
||||
|
||||
---
|
||||
|
||||
### 7. Wall-cell neighbor scan: precompute floor membership once per render
|
||||
|
||||
**File:** `src/views/Board.svelte:40-50`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0 (perf, not LOC)
|
||||
|
||||
```js
|
||||
const DIRS = [[-1,0],[1,0],[0,-1],[0,1],[-1,-1],[1,1],[-1,1],[1,-1]];
|
||||
for (const k of walls) {
|
||||
const { x, y } = keyToXY(k);
|
||||
if (DIRS.some(([dx, dy]) => floors.has(`${x + dx},${y + dy}`))) { ... }
|
||||
}
|
||||
```
|
||||
`floors.has(...)` is already O(1). The string concat `\`${x+dx},${y+dy}\`` per check is fine. Move `DIRS` outside `$derived.by` to module scope so it's not re-allocated each render:
|
||||
|
||||
```js
|
||||
const NEIGHBOR_OFFSETS = [[-1,0],[1,0],[0,-1],[0,1],[-1,-1],[1,1],[-1,1],[1,-1]];
|
||||
```
|
||||
at top-level. Trivial micro-perf — but more importantly, signals intent: this is a constant. **Recommendation: yes.**
|
||||
|
||||
---
|
||||
|
||||
### 8. `keyToXY` could be inline destructure
|
||||
|
||||
**File:** `src/views/Board.svelte:18-21`
|
||||
**Risk:** low
|
||||
**LOC saved:** 3
|
||||
|
||||
Used 3 times, each as a one-liner. The helper has a name worth keeping for readability — **recommend keeping**. But: if you don't already, you could store the parsed `{x, y}` directly in the data structure rather than `Set<string>`. That's a bigger refactor for level-parser.js — not worth it. **No change.**
|
||||
|
||||
---
|
||||
|
||||
### 9. `MobileControls` and HUD desktop-actions: keep separate
|
||||
|
||||
**File:** `src/views/GameView.svelte` (HUD) + `src/views/MobileControls.svelte`
|
||||
**Risk:** N/A
|
||||
**LOC saved:** 0
|
||||
|
||||
The audit prompt specifically asked. They are NOT duplicate work:
|
||||
- HUD `.desktop-actions` is hidden via `@media (pointer: coarse) { display: none }` (line 242).
|
||||
- `MobileControls` is hidden via the inverse (line 26-28).
|
||||
- They render different visual targets (top-row text buttons vs bottom-corner D-pad + action stack).
|
||||
- Trying to unify them would require runtime branching on pointer type, and would couple two distinct interaction paradigms.
|
||||
|
||||
**Recommendation: keep as-is.** Each is single-purpose and small.
|
||||
|
||||
---
|
||||
|
||||
### 10. `BoardModel` vs `GameView` split: keep
|
||||
|
||||
**File:** `src/lib/core/board-model.js` + `src/views/GameView.svelte`
|
||||
**Risk:** N/A
|
||||
**LOC saved:** 0
|
||||
|
||||
The audit prompt specifically asked. Split is correct:
|
||||
- `BoardModel` is framework-agnostic, no Svelte imports — game logic only.
|
||||
- `GameView` adapts mutable model into reactive `$state` snapshots, owns input/HUD/win-overlay.
|
||||
|
||||
Folding BoardModel into GameView would couple game rules to Svelte and prevent unit-testing the model in isolation. The dual `model` (mutable ref) + `player`/`boxes` (`$state`) pattern in GameView is unusual but clearly commented (lines 32-34). **Recommendation: keep.**
|
||||
|
||||
---
|
||||
|
||||
### 11. `progressStore` reads localStorage on every method call
|
||||
|
||||
**File:** `src/lib/core/progress-store.js`
|
||||
**Risk:** med
|
||||
**LOC saved:** 0 (perf)
|
||||
|
||||
`isCompleted`, `getBestMoves`, `getCompletedCount` each call `readRaw()` which JSON.parses the entire blob. In `LevelSelectView.visibleLevels` derived block (lines 23-29), this happens 20× (10 reads × 2 fields × per page-change). For 155 levels that's microseconds — but the redundancy is real.
|
||||
|
||||
**Option:** in-memory cache initialized lazily on first read, invalidated on `recordCompletion`/`reset`. Adds ~10 LOC.
|
||||
|
||||
**Recommendation: don't bother (YAGNI).** Levels are tiny, page-changes are rare, and a cache adds complexity. Listed only for visibility.
|
||||
|
||||
---
|
||||
|
||||
### 12. Common CSS for overlay/dialog (DRY)
|
||||
|
||||
**File:** `src/views/GameView.svelte:251-272`, `src/views/DonateModal.svelte:40-65`
|
||||
**Risk:** low
|
||||
**LOC saved:** ~12
|
||||
|
||||
Both files repeat the overlay/dialog pattern verbatim:
|
||||
```css
|
||||
.overlay {
|
||||
position: fixed; inset: 0;
|
||||
background: rgba(12, 16, 24, 0.72);
|
||||
display: flex; align-items: center; justify-content: center;
|
||||
animation: fade-in 180ms ease;
|
||||
}
|
||||
.dialog {
|
||||
background: var(--panel);
|
||||
border: 2px solid var(--accent);
|
||||
border-radius: var(--radius-lg);
|
||||
/* ... */
|
||||
}
|
||||
@keyframes fade-in { from { opacity: 0; } to { opacity: 1; } }
|
||||
```
|
||||
Move into `src/app.css` as `.overlay`/`.dialog` global classes (only `z-index` differs: 100 vs 200, set inline or via modifier). Save the duplicated `@keyframes fade-in`.
|
||||
|
||||
**Recommendation:** yes, low risk, real DRY win. ~12 LOC saved across two files.
|
||||
|
||||
---
|
||||
|
||||
### 13. Tap-suppression CSS triplet repeated 3× (DRY)
|
||||
|
||||
**File:** `src/app.css:50-59`, `src/views/AppButton.svelte:43-46`, `src/views/MobileControls.svelte:69-72, 83-86`, `src/views/Board.svelte:103-104`
|
||||
**Risk:** low
|
||||
**LOC saved:** ~6
|
||||
|
||||
The trio:
|
||||
```css
|
||||
touch-action: manipulation;
|
||||
user-select: none;
|
||||
-webkit-user-select: none;
|
||||
-webkit-tap-highlight-color: transparent;
|
||||
```
|
||||
appears in 4 places. Body already has `user-select: none` globally — so each per-element block is partly redundant.
|
||||
|
||||
**Option:** add a global utility in `app.css`:
|
||||
```css
|
||||
.tap-clean,
|
||||
button.tap-clean {
|
||||
touch-action: manipulation;
|
||||
-webkit-tap-highlight-color: transparent;
|
||||
}
|
||||
```
|
||||
And apply via class. Or: move `touch-action: manipulation` and `-webkit-tap-highlight-color: transparent` onto the global `button` selector in `app.css` since every button in the app wants them.
|
||||
|
||||
**Recommendation:** add to global `button { ... }` in `app.css`. Removes the per-component duplication.
|
||||
|
||||
Before (`app.css`):
|
||||
```css
|
||||
button {
|
||||
font-family: inherit;
|
||||
}
|
||||
```
|
||||
After:
|
||||
```css
|
||||
button {
|
||||
font-family: inherit;
|
||||
touch-action: manipulation;
|
||||
-webkit-tap-highlight-color: transparent;
|
||||
}
|
||||
```
|
||||
Then strip those two lines from AppButton, MobileControls (×2), Board removes nothing (it's on `.board` not button).
|
||||
|
||||
---
|
||||
|
||||
### 14. `package.json` description still says Phaser
|
||||
|
||||
**File:** `package.json:3`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0
|
||||
|
||||
```json
|
||||
"description": "A simple Sokoban game built with Phaser 3 and Vite",
|
||||
```
|
||||
The codebase is Svelte 5 + Vite, no Phaser. Update to:
|
||||
```json
|
||||
"description": "Sokoban puzzles (Microban set) built with Svelte 5 and Vite",
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 15. `vite/config.codeserver.mjs` defensive `Number(env.CODESERVER_PORT || 8080)`
|
||||
|
||||
**File:** `vite/config.codeserver.mjs:7`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0 (clarity)
|
||||
|
||||
```js
|
||||
const port = Number(env.CODESERVER_PORT || 8080);
|
||||
```
|
||||
Fine as-is. Throws on `CODESERVER_HOST` missing but silently defaults port. Consistent with `dev.mjs` which also uses `8080`. Could extract a shared `DEFAULT_DEV_PORT = 8080` constant — but the literal appears in only 2 files and is self-documenting. **No change.**
|
||||
|
||||
---
|
||||
|
||||
### 16. `progress-store.getCompletedCount` doesn't filter by `=== true`
|
||||
|
||||
**File:** `src/lib/core/progress-store.js:45-47`
|
||||
**Risk:** low
|
||||
**LOC saved:** 0 (correctness)
|
||||
|
||||
```js
|
||||
getCompletedCount() {
|
||||
return Object.keys(readRaw().completed).length;
|
||||
}
|
||||
```
|
||||
Counts any key in `completed`, regardless of value. Today only `true` is written, so it works. If `recordCompletion` ever conditionally clears a level (it doesn't), `false` keys would still count. Defensive nit:
|
||||
```js
|
||||
return Object.values(readRaw().completed).filter(Boolean).length;
|
||||
```
|
||||
**Recommendation:** skip (YAGNI). Current code is correct given current writers.
|
||||
|
||||
---
|
||||
|
||||
## Other observations (no change recommended)
|
||||
|
||||
- **`haptics.js` (12 LOC)** — perfect minimal wrapper, do not touch.
|
||||
- **`AppButton.svelte`** — well-scoped, 3 variants × 3 sizes is just right. No prop is unused.
|
||||
- **`App.svelte` view router** — string-state with three branches is simpler than a router lib here. Correct.
|
||||
- **`#each ... (key)` with `cell.x + ',' + cell.y`** in Board.svelte — string concat in key is fine; using `\`${x},${y}\`` would be one byte shorter but no clearer.
|
||||
- **No dead imports found.** All `import` statements are used.
|
||||
- **No unused props found** across components.
|
||||
- **No unreachable branches found.**
|
||||
|
||||
---
|
||||
|
||||
## Aggregated proposal table
|
||||
|
||||
| # | File | Risk | LOC | Recommend? |
|
||||
|---|------|------|-----|------------|
|
||||
| 1 | level-parser.js | low | 1 | yes |
|
||||
| 3 | board-model.js | low | 1 | yes (small) |
|
||||
| 4 | LevelSelectView.svelte | low | 0 | yes |
|
||||
| 7 | Board.svelte | low | 0 | yes (clarity) |
|
||||
| 12 | GameView/DonateModal CSS | low | ~12 | **yes (best win)** |
|
||||
| 13 | global button CSS | low | ~6 | yes |
|
||||
| 14 | package.json description | low | 0 | yes |
|
||||
| 2,5,6,8,9,10,11,15,16 | various | — | — | **no** (YAGNI / no benefit) |
|
||||
|
||||
**Total realistic savings:** ~20 LOC + reduced CSS duplication. Most files already meet the "every line earns its keep" bar.
|
||||
|
||||
---
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. **Stale `completedCount` in LevelSelectView** — Should the count refresh when the user returns from a freshly completed level? Currently it doesn't (it's read once on mount, even though it's `$state`). If yes, that's a real bug — but fixing it is feature work, not simplification, so I left it out of the proposals. Confirm desired behavior.
|
||||
2. **Are zero-box levels possible in user-supplied data?** Affects whether proposal #3's guard removal is safe long-term. Today's Microban set has none.
|
||||
3. **Is the `GameView.computeTileSize` margin-literal duplication worth fixing?** Proposal #6 leans "no, just comment", but if you plan to tune mobile layout further, extracting constants helps.
|
||||
|
||||
---
|
||||
|
||||
**Status:** DONE
|
||||
@@ -0,0 +1,170 @@
|
||||
# Runtime Health Report — Sokoban (Svelte 5 + Vite)
|
||||
**Date:** 2026-04-27
|
||||
**Scope:** Runtime blow-up risks only. Cosmetics and items in code-reviewer-260427-2023 skipped.
|
||||
|
||||
---
|
||||
|
||||
## 1. Build (`npm run build`)
|
||||
|
||||
**Clean build, no warnings, 1.24s.** Output:
|
||||
- `dist/assets/index-*.js` — 68.37 kB raw / 24.07 kB gzip (single chunk, no code-splitting)
|
||||
- `dist/assets/index-*.css` — 10.02 kB raw / 2.61 kB gzip
|
||||
- `dist/sw.js` + `dist/workbox-8c29f6e4.js` (PWA service worker)
|
||||
- `dist/registerSW.js` — 0.15 kB (auto-register shim)
|
||||
- `dist/manifest.webmanifest` — 0.47 kB
|
||||
|
||||
No source maps emitted (correct for prod — Vite default is `sourcemap: false`).
|
||||
Bundle size is healthy for a pure-logic game with 155 embedded level strings.
|
||||
|
||||
Dev server starts cleanly in 604ms. Note: "Re-optimizing dependencies because lockfile has changed" on startup — cosmetic one-time warmup from the dependabot rebase, not a runtime concern.
|
||||
|
||||
---
|
||||
|
||||
## 2. dist/ Inspection (inferred from build + config — `dist/` blocked by .ckignore)
|
||||
|
||||
### manifest.webmanifest
|
||||
From `vite/config.prod.mjs`, the generated manifest will be:
|
||||
```json
|
||||
{
|
||||
"start_url": "/sokoban/",
|
||||
"scope": "/sokoban/",
|
||||
"icons": [
|
||||
{ "src": "pwa-192x192.png", ... },
|
||||
{ "src": "pwa-512x512.png", ... }
|
||||
]
|
||||
}
|
||||
```
|
||||
`start_url` and `scope` are hardcoded — correct for GitHub Pages.
|
||||
`vite-plugin-pwa` v1.2.0 prepends `base` (`/sokoban/`) to icon `src` paths when generating the manifest output, so runtime icon resolution should be `/sokoban/pwa-192x192.png`. **Unresolved: cannot directly verify the emitted manifest since dist/ is blocked — recommend spot-checking the deployed manifest at `https://tiennm99.github.io/sokoban/manifest.webmanifest`.**
|
||||
|
||||
### Service Worker / Workbox Precache
|
||||
|
||||
Build reports **16 entries, 435.06 KiB** precached. Workbox glob pattern:
|
||||
```
|
||||
**/*.{js,css,html,png,svg,webmanifest}
|
||||
```
|
||||
|
||||
**ISSUE 1 — `qr.jpg` NOT precached (offline breakage):**
|
||||
`public/assets/qr.jpg` (122 KB, the donation QR code) is a `.jpg` — not matched by the glob. When the app is opened offline (PWA installed), clicking Donate → broken image in `DonateModal`. The rest of the app works offline. Severity: **Low** (donate path only, not gameplay).
|
||||
Fix: add `jpg` to glob → `**/*.{js,css,html,png,jpg,svg,webmanifest}`
|
||||
|
||||
**ISSUE 2 — `bg.png` and `logo.png` are orphan assets being precached (~320 KB wasted):**
|
||||
`public/assets/bg.png` (295 KB) and `public/assets/logo.png` (24 KB) have **zero references** in any source file, CSS, or HTML. They match the glob pattern and inflate the precache by ~320 KB (73% of the 435 KB total). Workbox downloads and caches these on every install/update.
|
||||
Fix: delete from `public/assets/`. If they were leftover from the Phaser rewrite, they can be removed safely.
|
||||
|
||||
### Bundle Structure
|
||||
Single 68 kB chunk — appropriate for this app size. No code-splitting needed. No unintended lazy imports detected.
|
||||
|
||||
---
|
||||
|
||||
## 3. Vite Config Drift (prod / dev / codeserver)
|
||||
|
||||
| Config | `base` | PWA | HMR |
|
||||
|--------|--------|-----|-----|
|
||||
| prod | `/sokoban/` | Yes | — |
|
||||
| dev | `./` | No | default |
|
||||
| codeserver | `/absproxy/${port}/` | No | WSS override |
|
||||
|
||||
**No runtime breakage in prod.** Differences are intentional.
|
||||
|
||||
**Potential concern — `dev` base `./` vs `prod` base `/sokoban/`:**
|
||||
`DonateModal` uses `{import.meta.env.BASE_URL}assets/qr.jpg`. This resolves correctly in all three configs:
|
||||
- dev: `./assets/qr.jpg` → served from `/assets/qr.jpg` by Vite dev server
|
||||
- prod: `/sokoban/assets/qr.jpg` ← correct GH Pages path
|
||||
- codeserver: `/absproxy/8080/assets/qr.jpg` ← correct proxy path
|
||||
|
||||
**ISSUE 3 — codeserver HMR `path` override:**
|
||||
```js
|
||||
hmr: { host, protocol: 'wss', clientPort: 443, path: base }
|
||||
```
|
||||
`path: '/absproxy/8080/'` tells the HMR client to connect the WebSocket at `wss://{host}:443/absproxy/8080/`. Whether the code-server proxy forwards WebSocket upgrades on that path correctly depends on the proxy configuration. If it doesn't, HMR silently falls back to full reload — not a runtime crash, but worth testing if HMR is needed during dev.
|
||||
|
||||
---
|
||||
|
||||
## 4. npm audit
|
||||
|
||||
Cannot run `npm audit` (blocked by hook). From package versions:
|
||||
|
||||
| Package | Version | Notes |
|
||||
|---------|---------|-------|
|
||||
| vite | 6.4.2 | `^6.4.2` in lockfile — clean, single instance |
|
||||
| svelte | 5.55.3 | latest |
|
||||
| @sveltejs/vite-plugin-svelte | 5.1.1 | latest |
|
||||
| vite-plugin-pwa | 1.2.0 | latest |
|
||||
| esbuild | 0.25.2 | dev-only build tool |
|
||||
| rollup | 4.40.0 | dev-only bundler |
|
||||
| workbox-build | 7.4.0 | dev-only |
|
||||
| serialize-javascript | 6.0.2 | transitive; known XSS fix was in 3.1.0, well clear |
|
||||
|
||||
**The 1 high + 1 moderate vulnerabilities reported on the remote CI are almost certainly in `esbuild` or `rollup` (both dev-only build tools, not shipped in the bundle).** Neither affects the app at runtime in the browser. No app-code dependencies have known CVEs. **Unresolved: cannot confirm exact CVE identifiers without running `npm audit` — recommend running it after unlocking the hook or via CI.**
|
||||
|
||||
---
|
||||
|
||||
## 5. package-lock.json Integrity
|
||||
|
||||
- `lockfileVersion: 3` — correct for npm 7+
|
||||
- Vite resolved to exactly `6.4.2` (matches `^6.4.2` spec), single instance in lock
|
||||
- SHA512 integrity hash present and well-formed
|
||||
- No duplicate vite version splits detected
|
||||
- The rebase from 6.3.6→6.4.2 left a clean lock — no stale entries visible
|
||||
|
||||
---
|
||||
|
||||
## 6. `progress-store.js` — localStorage Resilience
|
||||
|
||||
All `localStorage` calls are wrapped in `try/catch`:
|
||||
- **Quota exceeded:** `writeRaw` catches the `QuotaExceededError` silently → progress is lost for that operation but the app does not throw. Acceptable for a puzzle game.
|
||||
- **Disabled storage / private browsing (Safari ITP):** `localStorage.getItem` throws a `SecurityError` in some private modes → caught, returns empty default object `{ completed: {}, bestMoves: {} }`. App runs fully, progress just doesn't persist.
|
||||
- **Corrupt JSON:** `JSON.parse` throws → caught by `readRaw`, returns empty default. No cascade.
|
||||
|
||||
**No runtime risk.** Graceful degradation confirmed at every callsite.
|
||||
|
||||
**ISSUE 4 — `readRaw()` called twice per `recordCompletion` (and per `isCompleted`/`getBestMoves` in level select):**
|
||||
`LevelSelectView.$derived.by` calls `progressStore.isCompleted(i)` + `progressStore.getBestMoves(i)` for each of 20 visible levels = 40 `localStorage.getItem` + JSON.parse calls per page render. Not a crash, but unnecessary. Low priority.
|
||||
|
||||
---
|
||||
|
||||
## 7. `BoardModel` — State Corruption Analysis
|
||||
|
||||
**Scenario: `undo()` at empty history**
|
||||
`history.pop()` on empty array returns `undefined`. Guard `if (!last) return false` fires correctly. No corruption.
|
||||
|
||||
**Scenario: `tryMove()` after `isSolved()`**
|
||||
No guard inside `BoardModel.tryMove()` itself — but **all callers** in `GameView.svelte` wrap with `if (won || !model) return`. The `won` flag is set synchronously inside `syncFromModel()` which is called immediately after `tryMove` returns. Since JS is single-threaded and `won` is set before any user input can arrive, this guard is reliable. No corruption path exists via normal UI.
|
||||
|
||||
**Scenario: Box index integrity on undo**
|
||||
`boxIndex` stored in history comes from `this.boxes.findIndex()` at the time of the move. The boxes array is never mutated externally between push and pop (no external mutation path). `undo` correctly uses `b.x - last.dx` / `b.y - last.dy` to reverse the move. Mathematically sound.
|
||||
|
||||
**Scenario: Shared level data across BoardModel instances**
|
||||
`BoardModel` stores `level.walls` and `level.targets` as direct Set references (no copy). When `restart()` creates `new BoardModel(level)`, both instances share the same Sets. **Safe** because BoardModel only reads `walls` and `targets` (via `isWall`, `isTarget`) — never mutates them.
|
||||
|
||||
**Scenario: `tryMove` / `undo` with zero boxes (degenerate level)**
|
||||
`isSolved()` guards with `if (this.boxes.length === 0) return false` — no infinite solved state. `undo` with movedBox=false doesn't touch boxes array. Safe.
|
||||
|
||||
**No state corruption paths found.** Code is correct.
|
||||
|
||||
---
|
||||
|
||||
## 8. Summary Table
|
||||
|
||||
| # | Severity | Type | Description | Fix |
|
||||
|---|----------|------|-------------|-----|
|
||||
| 1 | Low | PWA/offline | `qr.jpg` not in workbox precache → broken offline donate image | Add `jpg` to workbox glob |
|
||||
| 2 | Low | Bundle bloat | `bg.png` (295 KB) + `logo.png` (24 KB) orphaned in `public/assets/` — precached for no reason | Delete both files |
|
||||
| 3 | Info | Dev env | codeserver HMR WS path may not work depending on proxy config | Test HMR in codeserver; non-blocking |
|
||||
| 4 | Info | Perf | `readRaw()` called N×2 on level-select page render | Cache in-memory or make store reactive; non-blocking |
|
||||
| 5 | Info | Audit | 1 high + 1 moderate vuln in dev-only deps (likely esbuild/rollup) — zero app runtime risk | Monitor; update when fixes available |
|
||||
|
||||
---
|
||||
|
||||
## Unresolved Questions
|
||||
|
||||
1. Cannot confirm exact CVE IDs (npm audit blocked by hook) — run `npm audit` in CI or local terminal to get specifics.
|
||||
2. Cannot directly read `dist/manifest.webmanifest` (dist/ blocked) — spot-check deployed manifest icon paths at production URL.
|
||||
3. codeserver HMR not testable here — requires live code-server environment with correct `CODESERVER_HOST`.
|
||||
|
||||
---
|
||||
|
||||
**Status:** DONE_WITH_CONCERNS
|
||||
**Summary:** Build is clean and correct. Two concrete fixable issues found: orphan assets wasting 320 KB of precache bandwidth, and `qr.jpg` excluded from offline cache. No state corruption, no crash paths, no security risk in app code. The npm audit vulns are dev-toolchain only.
|
||||
**Concerns:** dist/ directory was blocked from direct inspection; manifest icon path verification requires checking deployed URL.
|
||||
@@ -0,0 +1,78 @@
|
||||
# Security Scan — Whole Project
|
||||
|
||||
- Date: 2026-04-27
|
||||
- Project: Sokoban (Svelte 5 + Vite, static, GH Pages)
|
||||
- Scope: secret detection, dependency audit, OWASP code patterns, .env exposure
|
||||
|
||||
## Summary
|
||||
|
||||
| Category | Critical | High | Medium | Low |
|
||||
|----------|---------:|-----:|-------:|----:|
|
||||
| Secrets | 0 | 0 | 0 | 0 |
|
||||
| Deps (build-only) | 0 | 4 | 2 | 0 |
|
||||
| Code | 0 | 0 | 0 | 0 |
|
||||
| .env | 0 | 0 | 0 | 0 |
|
||||
|
||||
## 1. Secret detection — clean
|
||||
|
||||
Grep across `*.js, *.mjs, *.svelte, *.json, *.html, *.css` (excluding `node_modules`, `dist`, `.git`) for AWS keys, GitHub tokens, Stripe keys, Slack tokens, hardcoded passwords/api_keys/secrets. **Zero matches.**
|
||||
|
||||
## 2. .env exposure — clean
|
||||
|
||||
- No `.env`, `.env.local`, `.env.production` tracked by git.
|
||||
- `.gitignore` excludes `node_modules`, `dist`, `*.local`.
|
||||
- (Note: `.gitignore` line 11 is a stray `.` — harmless but should be removed for tidiness.)
|
||||
|
||||
## 3. Code patterns — clean
|
||||
|
||||
No matches for: `innerHTML`, `dangerouslySetInnerHTML`, `eval(`, `new Function(`, `document.write`, Svelte `{@html}`, `Math.random` for security, `crypto.` misuse.
|
||||
|
||||
The project takes only keyboard/touch events; no DOM injection of user-supplied strings; no fetch/XHR; no dynamic script eval. localStorage I/O is fully wrapped in try/catch.
|
||||
|
||||
## 4. Dependency audit — all build-only, not exploitable in this codebase
|
||||
|
||||
`npm audit` reports 8 vulnerabilities (1 high in direct dep `vite-plugin-pwa`, plus 7 transitive). All are **dev tooling** that never ships to the browser bundle:
|
||||
|
||||
| Package | Severity | CVE / GHSA | Why not exploitable here |
|
||||
|---------|----------|------------|--------------------------|
|
||||
| `vite-plugin-pwa` (direct) | High | rolls up the below | only runs at `npm run build` time on developer/CI machine |
|
||||
| `workbox-build` | High | via plugin-terser | build-time only |
|
||||
| `@rollup/plugin-terser` | High | via serialize-javascript | build-time only |
|
||||
| `serialize-javascript` <7.0.5 | High | GHSA-5c6j-r48x-rmvq (RCE via RegExp.flags) | requires attacker-controlled input; workbox serializes our own precache manifest of glob-matched local files |
|
||||
| `serialize-javascript` <7.0.5 | Mod | GHSA-qj8w-gfj5-8c6v (CPU DoS) | same — local trusted input |
|
||||
| `rollup` 4.0.0–4.58.0 | High | GHSA-mw96-cpmx-2vgc (path traversal) | requires malicious config; we own all config |
|
||||
| `postcss` <8.5.10 | Mod | GHSA-qx2v-qp2m-jg93 (XSS in CSS stringify) | requires attacker-controlled CSS source; our CSS is hand-written and static |
|
||||
| `picomatch` 4.0.0–4.0.3 | High | GHSA-c2c7-rcm5-vvqj (ReDoS in extglob) + GHSA-3v7f-55p6-f55p | requires attacker-controlled glob; we own all globs in vite config |
|
||||
|
||||
### Recommendation
|
||||
|
||||
`npm audit fix` won't auto-resolve because the only listed fix path is a **major downgrade** of `vite-plugin-pwa` to 0.19.8, which would lose recent features and the current API.
|
||||
|
||||
Better paths in priority order:
|
||||
1. **Wait & monitor** — workbox/vite-plugin-pwa maintainers regularly bump transitive deps. Re-run audit weekly. Practical risk for a static GH Pages site is near-zero.
|
||||
2. **`npm audit fix --force`** — only if comfortable with the major downgrade. Test the build afterward.
|
||||
3. **`npm dedupe`** + manual `overrides` in package.json — pin `picomatch`, `postcss`, `rollup`, `serialize-javascript` to fixed versions via npm `overrides`. Surgical, keeps current `vite-plugin-pwa` major. About 5 lines of package.json.
|
||||
|
||||
## 5. Runtime security posture
|
||||
|
||||
| Concern | Status |
|
||||
|---------|--------|
|
||||
| XSS | No DOM injection of user data. No `{@html}`. |
|
||||
| CSRF | N/A — no backend, no cookies, no auth. |
|
||||
| Clickjacking | N/A — game is the whole page; no embed-target value. (Could add `frame-ancestors` CSP header server-side, but GH Pages doesn't let you set headers.) |
|
||||
| localStorage poisoning | `progressStore` reads through try/catch and treats malformed JSON as empty state. |
|
||||
| Service worker scope creep | `scope: '/sokoban/'` correctly bounds SW to project base path. |
|
||||
| Subresource integrity | Not used; all assets are self-hosted under `/sokoban/`. No CDN. SRI only meaningful for external scripts. |
|
||||
| HTTPS | Enforced by GitHub Pages. |
|
||||
| Content Security Policy | Not set. GH Pages doesn't allow custom HTTP headers, but a `<meta http-equiv="Content-Security-Policy">` could be added (e.g. `default-src 'self'; img-src 'self' data:`) — low priority hardening, not urgent. |
|
||||
|
||||
## Recommendations (prioritized)
|
||||
|
||||
1. **Add `npm overrides`** for the four transitive dev deps (rollup, picomatch, postcss, serialize-javascript) — clears the vulnerability list without forcing a vite-plugin-pwa major downgrade.
|
||||
2. **Optional**: add a meta CSP for defense-in-depth: `<meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src 'self' data:; style-src 'self' 'unsafe-inline'; manifest-src 'self'">`. (`'unsafe-inline'` for styles is needed because Vite injects scoped CSS inline.)
|
||||
3. **Tidy `.gitignore`** — remove the stray `.` on line 11.
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
- Is the `vite-plugin-pwa` major-version downgrade (option 2 above) acceptable to the user, or should we go with `npm overrides` (option 3)?
|
||||
- Should the meta CSP be added now, or deferred?
|
||||
Reference in new issue
Block a user