diff --git a/plans/260430-2050-both-mode-state-consistency/phase-01-lift-master-state.md b/plans/260430-2050-both-mode-state-consistency/phase-01-lift-master-state.md new file mode 100644 index 0000000..b3b4f54 --- /dev/null +++ b/plans/260430-2050-both-mode-state-consistency/phase-01-lift-master-state.md @@ -0,0 +1,121 @@ +--- +phase: 1 +title: "Lift master state to shared store" +status: completed +priority: P1 +effort: "1h" +dependencies: [] +--- + +# Phase 1: Lift master state to shared store + +## Overview + +Extract the `{called, remaining}` state from `MasterPanel.svelte` into a +new reactive module `master-store.svelte.js`. MasterPanel becomes a +view over the store; nothing else changes UX-wise. Foundation for +phase 2's player auto-cross. + +## Requirements + +**Functional** +- Same persistence semantics: localStorage `loto_master`, same shape +- Same load-on-mount, save-on-change behavior +- Existing master grid / "Số vừa xổ" / history list unchanged + +**Non-functional** +- File ≤ 200 lines +- Validators preserved (16 KB cap, `__proto__` stripping, shape check) +- No behavior change observable from user — pure refactor + +## Architecture + +**`src/lib/master-store.svelte.js`** (new) + +```js +const STORAGE_KEY = "loto_master"; +const MAX_STORAGE_BYTES = 16_384; + +export const masterState = $state({ + /** @type {number[]} */ + called: [], + /** @type {number[]} */ + remaining: [], +}); + +export function loadMaster() { /* parse + validate, write into masterState */ } +export function saveMaster() { /* serialize from masterState */ } +export function startNewGame() { /* fill remaining with shuffled 1..90, clear called */ } +export function drawNext() { + /** @returns {number | null} */ + if (masterState.remaining.length === 0) return null; + const next = masterState.remaining[0]; + masterState.called = [...masterState.called, next]; + masterState.remaining = masterState.remaining.slice(1); + return next; +} +export function resetMaster() { + /** Clears both arrays — used by "Ván mới" */ + masterState.called = []; + masterState.remaining = []; +} +``` + +The `lastCalled` derived value lives in `MasterPanel` since it's +display-only: +```js +const lastCalled = $derived( + masterState.called.length ? masterState.called.at(-1) : null, +); +``` + +**Persistence pattern**: a single $effect in `MasterPanel` (or in the +store module if cleaner) calls `saveMaster()` on `masterState.called` +or `masterState.remaining` change. Keep load gated on first mount so +SSR doesn't try to touch localStorage. + +## Related Code Files + +- Create: `src/lib/master-store.svelte.js` +- Modify: `src/lib/MasterPanel.svelte` + - Remove `state`, `loadState`, `saveState`, `createFreshState` from + component-level — move to `master-store.svelte.js` + - `handleNewGame` calls `startNewGame()` + - `handleDrawNext` calls `drawNext()`, then `broadcastDraw(next)` + (still using bus until phase 2) + - `lastCalled` becomes `$derived(masterState.called.at(-1))` + - The 11×9 board's `callOrder` map derived from `masterState.called` + +## Implementation Steps + +1. Create `master-store.svelte.js` with the 5 exports above +2. Move `STORAGE_KEY`, `MAX_STORAGE_BYTES`, `loadState`, `saveState`, + and the shuffle helper from `MasterPanel.svelte` into the store +3. Replace `MasterPanel`'s `let state = $state(...)` with reads against + `masterState` (all six mutations: load, draw, new, called list, + remaining list, lastCalled derive) +4. Wire load on mount: `$effect(() => { loadMaster(); });` +5. Wire save on change: `$effect(() => { saveMaster(); });` — + reading `masterState.called` + `masterState.remaining` inside +6. Smoke-test: refresh tab, master state persists; click "Ván mới", + reset works; click "Xổ số", draws + broadcasts as before + +## Success Criteria + +- [ ] `master-store.svelte.js` exists, ≤ 100 lines +- [ ] `MasterPanel.svelte` shrinks (no state/storage code inside) +- [ ] Master flow unchanged: load on mount, draw, new game, reload-restore +- [ ] All 123 existing vitest tests still pass +- [ ] `npx svelte-check` clean +- [ ] `npm run build` succeeds + +## Risk Assessment + +- **Reactivity break**: rune state inside a `.svelte.js` module re-exports + fine via destructuring? Yes — `settings-store.svelte.js` proves the + pattern. Make sure consumers import `masterState` (not destructure + fields) so reactivity threads through. +- **Migration of saved data**: localStorage shape unchanged, no migration + needed. Validators carried over verbatim. +- **Multiple MasterPanel mounts**: not exercised today (mode toggle + unmounts). Store is module-singleton, so two mounts would share — fine. diff --git a/plans/260430-2050-both-mode-state-consistency/phase-02-player-via-store.md b/plans/260430-2050-both-mode-state-consistency/phase-02-player-via-store.md new file mode 100644 index 0000000..2346cd8 --- /dev/null +++ b/plans/260430-2050-both-mode-state-consistency/phase-02-player-via-store.md @@ -0,0 +1,157 @@ +--- +phase: 2 +title: "Player auto-cross via shared store" +status: completed +priority: P1 +effort: "1.5h" +dependencies: [1] +--- + +# Phase 2: Player auto-cross via shared store + +## Overview + +Replace `PlayerBoard`'s bus-driven auto-tick with a $effect that +watches `masterState.called`. New numbers are auto-crossed; the +"already-handled" cursor moves to track length, not timestamp. Kills +the F7 (1ms collision) and F8 (single-slot history loss) classes +because we read the array directly. + +## Requirements + +**Functional** +- Auto-cross still fires only in `mode === "both"` +- Manual untick stays manual — auto-cross does NOT re-cross numbers the + user explicitly unticked since the last reset +- All previously called numbers still on the board get crossed if the + cursor catches up (covers F1 player-regen replay in phase 3) + +**Non-functional** +- No regression in existing 53 player-side test scenarios +- Auto-tick logic unit-testable as a pure helper + +## Architecture + +**Replace `auto-tick.js` with `player-auto-cross.js`** (new pure helper): + +```js +/** + * Decide which cells flip given the master's full called[] history + * and the player's already-applied cursor. Cursor advances strictly, + * even when no cell flips, so manual unticks don't re-fire. + * + * @param {object} args + * @param {number[][] | null} args.grid + * @param {boolean[][]} args.crossed + * @param {number[]} args.called - master's full history + * @param {number} args.lastHandledIndex - index already consumed + * @param {Set} args.manualUnticks - numbers user unticked + * @param {"player" | "master" | "both"} args.mode + * @returns {{ crossed: boolean[][], lastHandledIndex: number, changed: boolean }} + */ +export function applyMasterCalls({ grid, crossed, called, lastHandledIndex, manualUnticks, mode }) { + if (lastHandledIndex >= called.length) return { crossed, lastHandledIndex, changed: false }; + if (mode !== "both" || !grid || crossed.length === 0) { + // Advance cursor anyway to keep player→both transitions catching up + // only on FUTURE draws, not the whole back-history. + return { crossed, lastHandledIndex: called.length, changed: false }; + } + let next = crossed; + let changed = false; + for (let i = lastHandledIndex; i < called.length; i++) { + const num = called[i]; + if (manualUnticks.has(num)) continue; + const target = findUncrossedCell(grid, next, num); + if (!target) continue; + next = next.map((row, ri) => + ri === target.row ? row.map((v, ci) => (ci === target.col ? true : v)) : row, + ); + changed = true; + } + return { crossed: next, lastHandledIndex: called.length, changed }; +} +``` + +**Manual untick tracking** + +Add `let manualUnticks = $state(new Set())` at PlayerBoard's top. In the +cell click handler, if the user transitions a cell from `true → false` +on a number that's in `masterState.called`, add it to `manualUnticks`. +`true → false` on an uncalled number doesn't need tracking. `false → true` +removes from the set (re-cross overrides the untick). + +**Persistence**: `manualUnticks` persisted as a sorted array under key +`{prefix}_manualUnticks`, loaded on mount. + +**Player effect** (replaces old bus-watching effect): + +```js +let lastHandledIndex = $state(0); + +$effect(() => { + const result = applyMasterCalls({ + grid, + crossed, + called: masterState.called, + lastHandledIndex, + manualUnticks, + mode: settings.mode, + }); + if (result.lastHandledIndex !== lastHandledIndex) { + lastHandledIndex = result.lastHandledIndex; + } + if (result.changed) crossed = result.crossed; +}); +``` + +The `lastHandledIndex` is in-memory only — phase 3 covers the reload +behavior (reload re-applies all called numbers as a catch-up). + +## Related Code Files + +- Create: `src/lib/player-auto-cross.js` + tests +- Modify: `src/lib/PlayerBoard.svelte` + - Drop `bus`, `resetBus` import (kept only until phase 3 cleanup) + - Add `masterState` import from `$lib/master-store.svelte.js` + - Replace `lastHandledDrawAt` with `lastHandledIndex` + - Add `manualUnticks` $state + persistence + - Update cell click handler to track `true → false` transitions +- Keep (read-only ref): `findUncrossedCell` from `game-logic.js` + +## Implementation Steps + +1. Write `player-auto-cross.js` with `applyMasterCalls` (copy `findUncrossedCell` logic OR import it) +2. Write `player-auto-cross.test.js` covering: + - mode mismatch advances cursor without flipping + - mode=both crosses uncrossed cells, skips already-crossed + - manualUnticks numbers skipped + - empty called array → no-op + - cursor at length → no-op +3. Update `PlayerBoard.svelte`: + - Replace bus auto-tick effect with `applyMasterCalls` effect + - Add `manualUnticks` state + load/save helpers in `game-logic.js` + - Track unticks in `handleCellClick` (locate it; add 2-line transition check) +4. Verify mode toggle player→both: cursor advances to `called.length` in + non-both modes, so toggle-on doesn't replay back-history (intentional; + F6 explicitly accepts this trade-off — refresh path covers catch-up) +5. Run `npm test` — fix any auto-tick.test.js fallout (delete old tests if helper retired) + +## Success Criteria + +- [ ] `player-auto-cross.js` ≤ 100 lines, fully unit-tested +- [ ] PlayerBoard auto-crosses correctly on master draws (manual smoke) +- [ ] Manual untick → next master draw of SAME number does NOT re-cross +- [ ] Manual re-cross clears the untick (next draw of same number works) +- [ ] All vitest tests pass (with old `auto-tick.test.js` removed if helper retired) + +## Risk Assessment + +- **`manualUnticks` persistence shape**: stored as array of numbers, + reconstructed to `Set` on load. Validate `Number.isInteger` + + range 1..90 to defend against poisoned localStorage. +- **Cursor-vs-history mismatch on first run**: `lastHandledIndex` defaults + to 0; on mount, $effect runs, applies all `called` history. This is + the desired catch-up behavior for reload (covers F4). +- **Effect re-entry**: writing `crossed` inside an effect that reads + `crossed` — same pattern as today, dedup via `result.changed` makes + it stable. No new exposure. diff --git a/plans/260430-2050-both-mode-state-consistency/phase-03-replay-flows.md b/plans/260430-2050-both-mode-state-consistency/phase-03-replay-flows.md new file mode 100644 index 0000000..9dbfd25 --- /dev/null +++ b/plans/260430-2050-both-mode-state-consistency/phase-03-replay-flows.md @@ -0,0 +1,182 @@ +--- +phase: 3 +title: "Replay flows + retire bus" +status: completed +priority: P1 +effort: "1h" +dependencies: [1, 2] +--- + +# Phase 3: Replay flows + retire bus + +## Overview + +Wire up the four cross-panel flows (master "Ván mới", player "Tạo bảng +mới", player "Xoá đánh dấu", master draw → player auto-cross) to the +new shared store. Delete the now-dead `call-bus.svelte.js` and remove +the F3 violation (player handlers calling `resetBus()`). + +## Requirements + +**Functional** +- **Master "Ván mới"** in both mode → player crossed clears + manualUnticks clears (locked decision #1) +- **Player "Tạo bảng mới"** mid-game → new grid, then auto-cross all current `masterState.called` numbers found on it (covers F1) +- **Player "Xoá đánh dấu"** in both mode → clear crossed + manualUnticks, then immediately re-apply `masterState.called` so all called numbers cross again (locked decision #2) +- **Master draw** → player auto-cross via phase 2's effect, no bus needed + +**Non-functional** +- No reference to `call-bus.svelte.js` remains in source +- `resetBus` import in PlayerBoard removed (F3 fixed) + +## Architecture + +**Master "Ván mới" propagation** + +Detection: PlayerBoard runs an `$effect` watching `masterState.called.length`. +When it transitions from `> 0` to `0` AND `settings.mode === "both"`, +clear player crossed + manualUnticks + reset cursor. + +```js +let prevCalledLen = $state(0); +$effect(() => { + const len = masterState.called.length; + const wasReset = prevCalledLen > 0 && len === 0; + prevCalledLen = len; + if (wasReset && settings.mode === "both" && grid) { + crossed = grid.map(row => row.map(() => false)); + manualUnticks = new Set(); + lastHandledIndex = 0; + celebratedRows.clear(); + notifiedWaitingRows.clear(); + } +}); +``` + +**Player "Tạo bảng mới" replay** (`handleGenerate`) + +```js +function handleGenerate() { + if (grid && !confirm("Bạn có muốn tạo lại bảng không?")) return; + cancelPlayback(); + const newGrid = generateGrid(); + let newCrossed = newGrid.map(row => row.map(() => false)); + // Replay master's called[] onto the new grid (no-op outside both mode). + if (settings.mode === "both") { + const result = applyMasterCalls({ + grid: newGrid, crossed: newCrossed, + called: masterState.called, lastHandledIndex: 0, + manualUnticks: new Set(), mode: "both", + }); + newCrossed = result.crossed; + lastHandledIndex = result.lastHandledIndex; + } else { + lastHandledIndex = masterState.called.length; + } + grid = newGrid; + crossed = newCrossed; + manualUnticks = new Set(); + saveGrid(newGrid, STORAGE_PREFIX); + saveCrossedState(newCrossed, STORAGE_PREFIX); + saveManualUnticks(manualUnticks, STORAGE_PREFIX); + celebratedRows.clear(); + notifiedWaitingRows.clear(); + dismissToast(); + showCongrats = false; + // No resetBus — bus is gone. +} +``` + +**Player "Xoá đánh dấu" replay** (`handleClear`) + +```js +function handleClear() { + if (!grid) return; + const hasMarks = crossed.some(row => row.some(Boolean)); + if (hasMarks && !confirm("Bạn có muốn xoá tất cả đánh dấu không?")) return; + cancelPlayback(); + manualUnticks = new Set(); + let cleared = grid.map(row => row.map(() => false)); + if (settings.mode === "both") { + const result = applyMasterCalls({ + grid, crossed: cleared, + called: masterState.called, lastHandledIndex: 0, + manualUnticks: new Set(), mode: "both", + }); + cleared = result.crossed; + lastHandledIndex = result.lastHandledIndex; + } else { + lastHandledIndex = masterState.called.length; + } + crossed = cleared; + saveCrossedState(crossed, STORAGE_PREFIX); + saveManualUnticks(manualUnticks, STORAGE_PREFIX); + celebratedRows.clear(); + notifiedWaitingRows.clear(); + dismissToast(); + showCongrats = false; +} +``` + +**Bus retirement** + +After phase 2 + the above wiring, nothing imports from `call-bus.svelte.js` +or `auto-tick.js`. Delete the source files and their tests: + +- `src/lib/call-bus.svelte.js` +- `src/lib/call-bus.test.js` +- `src/lib/auto-tick.js` +- `src/lib/auto-tick.test.js` + +Remove `MasterPanel`'s `import { broadcastDraw, resetBus } ...` and the +`broadcastDraw(next)` / `resetBus()` calls — they're no-ops now since +`masterState` itself is the signal. + +## Related Code Files + +- Modify: `src/lib/PlayerBoard.svelte` (handleGenerate, handleClear, new $effect for master-reset detection) +- Modify: `src/lib/MasterPanel.svelte` (drop bus imports + calls) +- Modify: `src/lib/game-logic.js` (add `saveManualUnticks` / `loadManualUnticks`) +- Delete: `src/lib/call-bus.svelte.js`, `src/lib/call-bus.test.js`, + `src/lib/auto-tick.js`, `src/lib/auto-tick.test.js` + +## Implementation Steps + +1. Add `saveManualUnticks` / `loadManualUnticks` to `game-logic.js` + (mirror existing patterns; validate ints in [1,90]) +2. Update `handleGenerate` per architecture above +3. Update `handleClear` per architecture above +4. Add `prevCalledLen` $state + master-reset $effect +5. Strip bus imports + calls from MasterPanel and PlayerBoard +6. Delete the four files above +7. `npm test` — should still pass (after old auto-tick.test removal) +8. `npm run lint`, `npm run build`, `npx svelte-check` + +## Success Criteria + +- [ ] No source file imports from `call-bus` or `auto-tick` +- [ ] All four flows behave per locked decisions: + - Master "Ván mới" wipes player crossed in both mode + - Player regen replays master.called onto new grid + - Player "Xoá đánh dấu" in both mode replays immediately + - Master draw auto-crosses on player side +- [ ] Lint, build, svelte-check clean +- [ ] All remaining tests pass + +## Risk Assessment + +- **Mode-aware reset detection**: a player toggling mode AWAY from + "both" mid-game shouldn't accidentally trigger the reset clear. The + $effect gates on `settings.mode === "both"` at trigger time, so toggling + to "player" then master "Ván mới" in another window won't wipe player + crossed (multi-tab is out of scope, but local mode-toggle is covered). +- **`prevCalledLen` race on mount**: it initializes to 0; first effect run + sees `len = stored.called.length`, transition `0 → N` is NOT a reset. + Only `>0 → 0` triggers, so safe. +- **Replay performance**: replay loop is O(called × grid) ≈ O(90 × 81) = + ~7k ops worst case. Trivial. + +## Open question + +If the user wants the same "force-clear" behavior on mode toggle +player→both (auto-replay back-history), phase 2's cursor logic needs a +tweak. Current locked decisions don't cover this. Flag for sếp post-impl. diff --git a/plans/260430-2050-both-mode-state-consistency/phase-04-tests-and-verify.md b/plans/260430-2050-both-mode-state-consistency/phase-04-tests-and-verify.md new file mode 100644 index 0000000..178e4df --- /dev/null +++ b/plans/260430-2050-both-mode-state-consistency/phase-04-tests-and-verify.md @@ -0,0 +1,110 @@ +--- +phase: 4 +title: "Tests and verify" +status: completed +priority: P1 +effort: "45m" +dependencies: [1, 2, 3] +--- + +# Phase 4: Tests and verify + +## Overview + +Add coverage for the new helper + flows, run the full suite, and +manually verify the four locked behaviors in the browser. + +## Requirements + +- New unit tests for `applyMasterCalls` +- New unit tests for `master-store.svelte.js` (load/save/draw/new/reset) +- Manual browser verification of every locked behavior +- All existing tests still pass + +## Test Matrix + +### `player-auto-cross.test.js` (new) + +| Case | Expected | +|------|----------| +| empty called[] | no-op, cursor stays at 0 | +| cursor = called.length | no-op | +| mode="player", called grows | cursor advances to length, no flips | +| mode="both", called=[5], grid has 5 | crosses cell, cursor=1 | +| mode="both", called=[5,5,5] (impossible but test) | first match crosses, second / third no-op (already crossed) | +| mode="both", manualUnticks={5}, called=[5] | no flip, cursor advances | +| mode="both", grid=null | no flip, cursor stays | + +### `master-store.test.js` (new) + +| Case | Expected | +|------|----------| +| `loadMaster` with empty storage | masterState stays empty | +| `loadMaster` with corrupt JSON | falls back to empty | +| `loadMaster` with > 16 KB | rejected | +| `startNewGame` | called=[], remaining=shuffle(1..90) | +| `drawNext` | called appends, remaining shifts; returns drawn num | +| `drawNext` with empty remaining | returns null, no mutation | +| `resetMaster` | both arrays empty | + +### `game-logic.test.js` (extend) + +| Case | Expected | +|------|----------| +| `saveManualUnticks` round-trip | Set in → Set out, sorted on disk | +| `loadManualUnticks` with garbage | empty Set | +| `loadManualUnticks` with out-of-range nums | filtered | + +### Manual browser verification + +Enable both mode + auto-call. Click through: + +| # | Action | Expected | +|---|--------|----------| +| 1 | Master "Ván mới" with player marks present | Player crossed wipes | +| 2 | Master draws 3 numbers, then player "Tạo bảng mới" | New grid, those 3 numbers (if on grid) crossed | +| 3 | Master draws 5 numbers, player "Xoá đánh dấu" | All 5 immediately cross again | +| 4 | Player manually unticks #42 after auto-cross, master re-broadcast not possible (each num drawn once) | n/a — verify unticks persist across reload instead | +| 5 | Reload mid-game | Master state restored, player crossed restored, new master draws still auto-cross | +| 6 | Mode toggle both → player → both | New draws auto-cross; back-history NOT replayed (phase 3 open Q) | +| 7 | "Bắt đầu" auto-call → countdown shows + draws every N seconds | (regression check for last task) | + +## Implementation Steps + +1. Write `player-auto-cross.test.js` +2. Write `master-store.test.js` +3. Extend `game-logic.test.js` +4. `npm test` — expect green (delete old auto-tick.test.js if it broke + per phase 3) +5. `npm run lint`, `npx svelte-check`, `npm run build` +6. `npm run dev`, walk through the 7-case manual matrix +7. Commit per-phase or as one feat commit (sếp's call) + +## Success Criteria + +- [ ] All new test files green +- [ ] Full suite green (count > 123 minus retired tests + new tests) +- [ ] Lint clean (only pre-existing errors in `verify-build-inline-scripts.mjs`, + `MasterEmptyState.svelte`, `PlayerBoard.svelte` 396 — those are pre-existing, + not from this refactor) +- [ ] All 7 manual cases pass +- [ ] No console errors / warnings during the walkthrough + +## Risk Assessment + +- **Test fallout**: removing `auto-tick.js` retires 53 tests' worth of + coverage. The replacement helper covers equivalent ground; verify + count parity before declaring done. +- **Manual case 6 (mode toggle replay)**: this is currently OUT of scope + per the open question in phase 3. If sếp wants replay-on-toggle later, + it's a one-line tweak in `applyMasterCalls`'s mode-mismatch branch + (don't advance cursor on mismatch — let the next both-mode pass replay). + +## Docs Impact + +- Update `docs/codebase-summary.md`: + - Replace "call-bus.svelte.js" entry with "master-store.svelte.js" + - Replace "auto-tick.js" entry with "player-auto-cross.js" + - Update PlayerBoard / MasterPanel descriptions +- Update `docs/system-architecture.md` if it diagrams the bus +- Update `plans/todo.md` carryover items if any diff --git a/plans/260430-2050-both-mode-state-consistency/plan.md b/plans/260430-2050-both-mode-state-consistency/plan.md new file mode 100644 index 0000000..017b610 --- /dev/null +++ b/plans/260430-2050-both-mode-state-consistency/plan.md @@ -0,0 +1,62 @@ +--- +title: "Both-mode state consistency refactor" +status: completed +created: 2026-04-30 +completed: 2026-04-30 +slug: both-mode-state-consistency +--- + +# Both-mode state consistency refactor + +Fix the cross-panel inconsistencies surfaced in the 2026-04-30 audit +(`plans/reports/code-reviewer-260430-2024-both-mode-consistency.md` and +`plans/reports/brainstorm-260430-2024-both-mode-edge-cases.md`). + +Targets findings F1, F2, F4, F6, F7, F8, F10. Out of scope: F9 (voice +collision) and #20 (multi-tab) — separate plans to follow. + +## Why + +Single-slot `call-bus` carries only the latest draw. Any state event +that happens off-bus (player regen, master "Ván mới", reload, mode +toggle, throttled tab) silently loses history. Symptom the host hit: +fresh player board doesn't replay master's existing draws. + +## Product decisions (locked 2026-04-30) + +1. Master "Ván mới" → **force-clear** player's crossed in both mode. +2. Player "Xoá đánh dấu" in both mode → **replay all** called numbers + immediately after the clear. + +## Approach (surgical, KISS) + +Lift master's `called[]` to a shared reactive store. Player auto-cross +becomes a $effect on `masterStore.called` length growth, not a bus +slot. Existing tests keep passing; the bus dies because nothing reads +it. No full crossed-derivation rewrite — keep `crossed` as $state to +preserve manual cross/uncross UX. + +## Phases + +| # | Phase | Status | +|---|-------|--------| +| 1 | [Lift master state to shared store](phase-01-lift-master-state.md) | completed | +| 2 | [Player auto-cross via shared store](phase-02-player-via-store.md) | completed | +| 3 | [Replay flows + retire bus](phase-03-replay-flows.md) | completed | +| 4 | [Tests + verify](phase-04-tests-and-verify.md) | completed | + +## Key Files + +- Create: `src/lib/master-store.svelte.js` +- Modify: `src/lib/MasterPanel.svelte`, `src/lib/PlayerBoard.svelte`, + `src/lib/auto-tick.js` (or replace with new helper) +- Delete (after migration): `src/lib/call-bus.svelte.js`, + `src/lib/call-bus.test.js`, `src/lib/auto-tick.js`, + `src/lib/auto-tick.test.js` (if signature changes too much to keep) + +## Out of Scope + +- F9 voice ownership (master vs player playback collision) +- #20 multi-tab guard (banner / single-master lock) +- Full derived-crossed model (would erase manualUntick UX) +- AutoCountdown — already shipped, untouched here diff --git a/plans/reports/brainstorm-260430-2024-both-mode-edge-cases.md b/plans/reports/brainstorm-260430-2024-both-mode-edge-cases.md new file mode 100644 index 0000000..29635a1 --- /dev/null +++ b/plans/reports/brainstorm-260430-2024-both-mode-edge-cases.md @@ -0,0 +1,331 @@ +# Brainstorm — "Both Mode" Edge Cases & Inconsistencies + +Date: 2026-04-30 +Scope: Hypotheses only. No code verification. Cold-eyes triage list for the team. +Repo: /config/workspace/tiennm99/loto + +Mode model recap: +- `mode = "player" | "master" | "both"` — in `both`, MasterPanel + PlayerBoard mount on same page. +- Master draw → `broadcastDraw(num)` writes `{num, at: Date.now()}` to in-memory bus. +- Player `$effect` watches `bus.lastDrawn`; `processAutoTick` dedupes by `at`, advances `lastHandledAt` on every new `at`. +- Master state in `localStorage["loto_master"]`; player state in prefixed keys (`loto_grid`, `loto_crossed`); bus NOT persisted. +- Auto-call interval, voice (master-call + player "Chờ N"/"Kinh"), AutoCountdown component all overlay this. + +Legend: critical = data loss / can't recover game state | major = wrong gameplay outcome / user confusion | minor = cosmetic / rare | unknown = needs spike. + +--- + +## 1. Bus history loss on player board regeneration (CONFIRMED) +- Risk: **major** +- Why: Player presses "Tạo bảng mới" mid-game → fresh grid won't auto-cross numbers already called pre-regenerate, because bus only carries `lastDrawn`. Player must manually cross or wait for next draw. +- Verify: `PlayerBoard.svelte` regenerate handler + `auto-tick.js` (no replay of `master.called`). + +## 2. Bus is in-memory only → reload silently desyncs both panels +- Risk: **critical** +- Why: After page reload in `both` mode, `loto_master.called` rehydrates but bus.lastDrawn is null. If player grid had uncrossed numbers from before reload, no auto-replay happens — player relies on master's *next* draw to ever fire `$effect`. Player display "last called" pill / "Chờ N" toast goes blank vs master's visible called list. +- Verify: `call-bus.svelte.js` initial state + `+page.svelte` mount order + `PlayerBoard.svelte` `$effect`. + +## 3. `lastHandledAt` not persisted → after reload first new draw may double-process or be skipped +- Risk: **major** +- Why: If `lastHandledAt` lives in component state only, post-reload it resets to 0/null. On next master draw the `$effect` will process it, but if `lastDrawn` already exists from a *previous* in-memory state mid-session, behavior is undefined. Worse: auto-tick may treat a stale `at` as fresh. +- Verify: `auto-tick.js` dedup logic + where `lastHandledAt` is held (component vs store). + +## 4. Two broadcasts in same millisecond → one dropped +- Risk: **minor** (low likelihood) but **major** when it bites +- Why: `at: Date.now()` has 1ms resolution. Auto-call at high speed or clock with low-res timers (some Windows VMs) → identical `at` → dedup-by-`at` discards second broadcast. Number called by master never reaches player auto-cross. +- Verify: `call-bus.svelte.js` broadcastDraw, `auto-tick.js` dedup; consider monotonic counter instead of `at`. + +## 5. System clock backwards jump → `at` stops advancing +- Risk: **minor** +- Why: NTP correction or user changing clock can make new `at` ≤ old `at`. Dedup-by-`at` would treat new draw as old → skipped silently. +- Verify: `call-bus.svelte.js`; recommend `performance.now()` or sequence number. + +## 6. Rapid mode toggle player→both→master→both during a draw +- Risk: **major** +- Why: Player `$effect` may re-mount/unmount mid-tick. If `broadcastDraw` fires while PlayerBoard is unmounted (mode=master), player misses it; on toggling back to both, bus.lastDrawn is the missed number but `at` may already be ≤ player's `lastHandledAt` if it was persisted, or it gets re-processed if not. +- Verify: `+page.svelte` conditional rendering + lifecycle of PlayerBoard `$effect`. + +## 7. Mode toggle wipes wrong state +- Risk: **major** +- Why: Switching to player mode and back may clear master's auto-call interval but not its `loto_master`, OR may unmount MasterPanel mid-auto-call leaving an orphan `setInterval` that keeps broadcasting. Either way, "both"-mode timing breaks. +- Verify: `MasterPanel.svelte` onMount/onDestroy, AutoCountdown lifecycle, `settings-store.svelte.js` mode transitions. + +## 8. Auto-call interval not cleared on mode change / route change +- Risk: **major** +- Why: If MasterPanel registers `setInterval` but only clears in onDestroy, switching to a route that doesn't unmount it (SvelteKit nav can keep layout) leaves it firing. Numbers keep getting called against a hidden master. +- Verify: `MasterPanel.svelte` interval cleanup, `+layout.svelte`. + +## 9. Auto-call speed change mid-run +- Risk: **minor** +- Why: Changing `autoCallSpeed` while interval running typically requires clear+set; if implemented as reactive `$effect` watching speed, may double-register, leaving old interval ticking + new one. Player gets bursts. +- Verify: `MasterPanel.svelte` (or wherever interval is owned) + `settings-store.svelte.js`. + +## 10. AutoCountdown drift vs actual interval +- Risk: **minor** +- Why: Countdown likely uses `setInterval(1000)` or rAF; setInterval throttled in background tabs to ≥1s, rAF paused. Master's draw interval may also be throttled. Visible countdown can desync from actual draw firing — user sees "0s" but draw fires 5s later. +- Verify: `AutoCountdown.svelte`, visibility handling. + +## 11. Tab backgrounded → setInterval throttling +- Risk: **major** +- Why: Backgrounded host tab → master's auto-call throttled to 1Hz min (sometimes paused). Player on same tab is also throttled. When tab refocuses, multiple draws may fire in burst, voice queue overflows, and `lastHandledAt` skips multiple `at`s — but only ONE will be processed (only the latest is in bus). +- Verify: visibilitychange handlers (likely none); `MasterPanel.svelte` interval; `voice.js` queueing. + +## 12. Voice queue collisions in "both" mode +- Risk: **major** +- Why: Master speaks "Số N" and player speaks "Chờ N" / "Kinh" from same `speechSynthesis` queue on same page. Either they serialize (lag, "Chờ" announced 5s after the call) or one cancels the other (`speechSynthesis.cancel()` typical pattern). Either is wrong UX. +- Verify: `voice.js` — does it `cancel()` before `speak()`? Single shared queue? + +## 13. "Kinh" (bingo) speaks twice — once on auto-cross, once on detection +- Risk: **minor/major** +- Why: If bingo is detected both during `processAutoTick` (auto-cross caused win) and during render `$derived(bingo)`, two voice triggers may fire. Or worse, "Chờ N" speaks for the winning number first, then "Kinh" — confusing. +- Verify: `PlayerBoard.svelte` bingo detection effect, `voice.js`, ordering vs `processAutoTick`. + +## 14. "Chờ N" toast/voice fires for stale numbers after Tạo bảng mới +- Risk: **major** +- Why: After regenerate, board has new numbers. If `processAutoTick` re-runs against current bus.lastDrawn (because `lastHandledAt` reset), it announces "Chờ N" for a number that was called minutes ago — misleading the player it's a fresh call. +- Verify: regenerate handler + auto-tick re-run logic. + +## 15. Master "Ván mới" doesn't reset player +- Risk: **major** +- Why: Master clears `loto_master.called/remaining/last`. Player still has `loto_grid + loto_crossed` from previous game. Player sees old marks, no new "Chờ" because bus now empty. Need explicit player reset signal — does the bus carry "reset" event? +- Verify: MasterPanel "Ván mới" handler; bus contract; PlayerBoard listening for reset. + +## 16. Master "Ván mới" while player mid-bingo +- Risk: **minor** +- Why: Player has bingo state showing, master starts new game, player still announcing "Kinh" while master draws number 1 of new game. Voice collision + player's bingo ribbon stays. +- Verify: bingo state lifecycle, reset propagation. + +## 17. Player "Xoá đánh dấu" doesn't replay history either +- Risk: **major** (same root cause as #1) +- Why: Clears `crossed` only. Master's already-called list is still valid but player won't re-cross them automatically. Player must re-cross by hand. +- Verify: PlayerBoard clear handler. + +## 18. localStorage shape drift after settings refactor +- Risk: **major** +- Why: Old users with `loto_master` v1 schema (e.g. array vs object, missing `last`) load on new code → JSON.parse succeeds but destructure yields undefined → app crashes or silently breaks (called list shows blank). No version field visible from filename. +- Verify: `settings-store.svelte.js` parse + default-merge; check for `schemaVersion` field; wrap parse in try/catch with reset fallback. + +## 19. localStorage quota exceeded / disabled (private mode, Safari) +- Risk: **minor** +- Why: Setting `loto_master` throws `QuotaExceededError`. If unhandled, mode toggling and game state silently fails to persist; reload returns blank board. Master-only writes might succeed while player writes fail (or vice versa) → divergence. +- Verify: try/catch around localStorage.setItem in stores; user-facing error toast? + +## 20. Multiple tabs of the host +- Risk: **critical** +- Why: Two tabs both running master = two independent bus instances (in-memory, per-tab). Each writes to same `loto_master` localStorage key, last-write-wins, called numbers from one tab overwrite the other. Player in tab A sees draws from tab A only. No `storage` event listener to reconcile. +- Verify: any `window.addEventListener('storage', ...)`; document this is unsupported or add BroadcastChannel. + +## 21. Storage event in another tab triggers $effect cascade +- Risk: **unknown** +- Why: If reactive store does subscribe to storage events, cross-tab edits could rehydrate state mid-draw, replacing `called` array out from under MasterPanel render — visible flicker, possible double-add. +- Verify: settings-store + master state hydration. + +## 22. ARIA live regions stack on auto-call +- Risk: **minor** +- Why: Each draw probably writes to `aria-live="assertive"` (called pill, "Chờ N" toast, master's call display). At 2s/draw, screen reader queues 3+ announcements per number → unintelligible. +- Verify: any `aria-live="assertive"` in PlayerBoard / MasterPanel / AutoCountdown; prefer `polite` or single region. + +## 23. AutoCountdown announces every second to AT +- Risk: **minor** +- Why: If countdown digit is in an aria-live region, it will read "5… 4… 3…" every tick. Annoying and pre-empts important call announcement. +- Verify: `AutoCountdown.svelte` aria attributes. + +## 24. Bingo detection runs on every cross including auto +- Risk: **minor** +- Why: Auto-cross from `processAutoTick` flips a cell → bingo `$derived` recomputes → if it triggers a side-effect (voice "Kinh", confetti) inside an `$effect` that also fires for manual crosses, the path may differ subtly. E.g. on manual cross, master's draw display has updated; on auto-cross it has too — but order of effects between voice "Chờ" and bingo detection is the question. +- Verify: PlayerBoard ordering of `$effect`s. + +## 25. processAutoTick advances lastHandledAt even when number not on grid +- Risk: **minor** (intentional) but **major** if combined with regenerate +- Why: Stated behavior: dedup advances on every new `at`, even if no cell flipped. Fine — until player regenerates and now the number IS on grid but `lastHandledAt` has already moved past `at`. No retro-cross happens. Same root as #1. +- Verify: `auto-tick.js`. + +## 26. PWA service worker serves stale JS, but localStorage is fresh +- Risk: **major** +- Why: User had v1 app open, we deploy v2 with new bus contract. SW caches v1 assets; localStorage has v2 schema written by another device or refresh-on-other-tab. v1 code reads v2 data → crash or wrong rendering. Or vice versa. +- Verify: `service-worker.js` (if exists), Workbox/SvelteKit PWA config, schemaVersion. + +## 27. PWA offline: bus state lost, called list survives +- Risk: **major** +- Why: User goes offline, app keeps running from SW cache. Page reload offline — works. But bus history lost on every reload, divergence becomes more frequent because user reloads more often without connectivity feedback. +- Verify: SW + #2. + +## 28. Visibility change: rejoin race +- Risk: **major** +- Why: Tab returns from background. Master's interval was throttled → catches up by firing draws back-to-back. Player `$effect` sees rapid `at` increments but only the LAST `lastDrawn` is in bus → all intermediate numbers silently lost from auto-cross perspective, but they ARE in master's `called` list. Massive divergence. +- Verify: visibilitychange handler; need bus to be a queue or to replay from `master.called`. + +## 29. `$effect` re-runs on grid change post-cross +- Risk: **minor** +- Why: If `processAutoTick` is called from an `$effect` that depends on both `bus.lastDrawn` and `grid`, swapping the grid triggers re-run with same `lastDrawn` — but `lastHandledAt` already advanced, so it's a no-op. Confirm dedup is robust to this. +- Verify: PlayerBoard `$effect` deps list. + +## 30. Two PlayerBoards on page (future / accidental) +- Risk: **unknown** +- Why: If `both` mode somehow renders both routes' PlayerBoard or component is reused, both subscribe to bus; both write to the same `loto_grid` key → last-write-wins, crossed cells flicker. +- Verify: `+page.svelte` + `+layout.svelte`; current code likely single mount but worth checking. + +## 31. Master's "remaining" pool out of sync with "called" +- Risk: **major** +- Why: If "called" array is updated optimistically before "remaining" splice (or vice versa) and a render happens between, draw next could pick a number already called. Especially under React-style batching that Svelte 5 may or may not apply. +- Verify: MasterPanel draw handler; consider single transactional update. + +## 32. Manual call entry vs auto-call collide +- Risk: **major** +- Why: If master can manually enter a number while interval is running, two `broadcastDraw` paths exist. They could fire in the same ms (#4) or out of order. Also, manual entry might bypass "remaining" pool update. +- Verify: MasterPanel manual call (if exists) + auto-call interaction. + +## 33. Voice "Chờ N" speaks for number not on player's board +- Risk: **minor** +- Why: If "Chờ N" is announced whenever a draw happens regardless of whether it's on grid (vs the intent: announce only when player needs to wait/has it). Spec ambiguity — "Chờ" = "wait for N"? clarify. +- Verify: `voice.js` + PlayerBoard call site. + +## 34. Speech synthesis voice not loaded yet +- Risk: **minor** +- Why: `voiceschanged` event fires async. First few calls may speak with default voice (English) instead of Vietnamese. Especially on mobile Safari where voices load on first user gesture. +- Verify: `voice.js` voice selection + fallback. + +## 35. iOS audio policy: needs user gesture +- Risk: **major** for iOS users +- Why: Auto-call interval fires draw without user gesture → speechSynthesis silent on iOS Safari. User thinks voice is broken. Especially in `both` mode where master never clicked draw button after enabling auto-call. +- Verify: `voice.js`; consider primer gesture. + +## 36. settings-store mode=both with stale prefix +- Risk: **minor** +- Why: `storagePrefix` setting (player keys like `loto_grid`) could be edited while in both mode; old keys remain orphaned in localStorage; new prefix has empty grid; player auto-rehydrates blank. +- Verify: settings-store prefix change handler. + +## 37. broadcastDraw called with non-number / 0 / NaN +- Risk: **minor** +- Why: Auto-tick dedup runs but cell match `grid.includes(NaN)` returns false; lastHandledAt advances. Player silently ignores. But voice announces "Số NaN". +- Verify: type guards in `call-bus.svelte.js` + `voice.js`. + +## 38. Negative auto-call speed / 0 +- Risk: **minor** +- Why: If user sets autoCallSpeed=0 in settings (or via DevTools), `setInterval(fn, 0)` = ~4ms minimum, draws 90 numbers in ~1 sec. Bus only retains last; ALL but final auto-cross lost. +- Verify: settings-store validation min/max. + +## 39. Master draws 90 numbers, then 91st click +- Risk: **minor** +- Why: Empty `remaining` array → draw next fails silently or throws. If interval is still on, it fires every Ns hitting empty array — does it auto-stop? +- Verify: MasterPanel handleDrawNext when remaining empty + interval guard. + +## 40. processAutoTick runs in master mode (mode=master) +- Risk: **minor** +- Why: Player effect should be guarded by mode!==master. If guard is missing or off-by-one ("both" treated as master), auto-cross runs but no PlayerBoard renders, `lastHandledAt` advances pointlessly. Not data-corrupting but wastes work. +- Verify: `processAutoTick({mode})` mode check. + +## 41. lastHandledAt advancement inside test vs production +- Risk: **unknown** +- Why: `auto-tick.test.js` exists — if tests pass with mocked Date.now but production uses real Date.now plus throttling, test coverage may not catch #28 / #11. +- Verify: `auto-tick.test.js`; add throttling/burst test. + +## 42. Confetti / celebration replay on reload after bingo +- Risk: **minor** +- Why: Bingo state is `$derived(crossed)` → on reload, crossed rehydrates → bingo true → confetti fires again. Annoying. +- Verify: PlayerBoard bingo effect + flag like "celebrated". + +## 43. Both mode disables one panel by mistake +- Risk: **minor** +- Why: A `if (mode === 'master')` instead of `if (mode === 'master' || mode === 'both')` on a master button hides it in `both` mode. Vice versa for player. Small typos with three-way enum. +- Verify: every `mode ===` check across components. + +## 44. Settings change persists before save +- Risk: **minor** +- Why: SettingsButton may use 2-way binding directly to store instead of staging — toggling mode in dialog immediately remounts panels behind the dialog. UX confusion + state loss if user cancels. +- Verify: `SettingsButton.svelte` binding model. + +## 45. Reload during auto-call +- Risk: **major** +- Why: Auto-call running, user F5. `loto_master` saved up to last draw. On reload, auto-call interval is NOT auto-restarted (probably) — game appears paused without indication. Or auto-restarted from autoplay setting → first draw fires with no UI feedback yet. +- Verify: MasterPanel onMount + `autoCall` setting persistence. + +## 46. Network/CDN-cached audio mismatch +- Risk: **minor** +- Why: `audio-manifest.js` may reference numbered MP3s; if some 404 due to cache miss, voice fallback to TTS for some numbers and audio for others — inconsistent UX. +- Verify: `audio-manifest.js` + `voice.js` fallback chain. + +## 47. Master's "called" history > UI display window +- Risk: **minor** +- Why: After 50+ calls, called list display may overflow / paginate. If player only sees last N, reload doesn't help — but that's master display only. Just confirm. +- Verify: MasterPanel called list rendering. + +## 48. Reactive cycle: $effect → state change → $effect re-runs +- Risk: **major** +- Why: If `processAutoTick` mutates `lastHandledAt` AND the `$effect` reads it, infinite loop possible. Svelte 5 has guards but they're not free — perf hit, console warnings. +- Verify: `auto-tick.js` return contract + how PlayerBoard wires it. + +## 49. "Kinh" voice on regen-induced auto-cross sweep (if #1 is fixed) +- Risk: **major** (forward-looking) +- Why: If team fixes #1 by replaying `master.called` on regen, the replay might trigger 5+ auto-crosses, last one a bingo, which speaks "Kinh" instantly when user hits "Tạo bảng mới" — startling and wrong (didn't actually win this round). +- Verify: any future replay logic; suppress voice during replay. + +## 50. AutoCountdown shows when auto-call disabled +- Risk: **minor** +- Why: Recently added component — if its mount logic doesn't guard on `autoCall` setting, it shows stale "0s" countdown when manual mode active. +- Verify: `AutoCountdown.svelte` mount conditions. + +## 51. processAutoTick assumes `crossed` is mutable Set +- Risk: **unknown** +- Why: If crossed is a `$state` reactive Set, mutating in-place vs replacing affects reactivity. Auto-tick may flip cell but PlayerBoard not re-render. +- Verify: `auto-tick.js` mutation strategy + PlayerBoard. + +## 52. localStorage write thrash during auto-call +- Risk: **minor** +- Why: Each draw writes `loto_master`. At 2s cadence x 90 draws = 90 writes. Player on same tab also writes `loto_crossed`. Combined with reactive sync (every state change writes), localStorage is hot. Devices with slow storage hitch every draw. +- Verify: persistence layer in stores; consider debounce. + +## 53. Dialog/Modal stealing focus during draw +- Risk: **minor** +- Why: SettingsButton dialog open while auto-call fires — focus trap doesn't know about toast/announcement; SR users may miss draws. +- Verify: SettingsButton focus management. + +## 54. Reload during call-bus dispatch (race) +- Risk: **minor** +- Why: User hits F5 between `master.called.push(N)` and `broadcastDraw(N)`. localStorage has N in called, bus never broadcast. After reload bus is empty anyway (#2) so net effect is same divergence — but called list now contains N that was never voiced. +- Verify: MasterPanel handleDrawNext atomicity. + +## 55. Settings test coverage (false confidence) +- Risk: **unknown** +- Why: `settings-store.test.js` is 369 lines but tests probably mock localStorage. Real-world quota / disabled storage / private mode untested → #19 lurks. +- Verify: test file scenarios. + +## 56. broadcastDraw before player mounts (initial both-mode) +- Risk: **minor** +- Why: First render of `+page.svelte` mounts MasterPanel and PlayerBoard. Order matters: if MasterPanel onMount triggers a draw (e.g. resume autoplay) before PlayerBoard `$effect` registered, player misses #1. +- Verify: mount order; defer master autoplay to next tick. + +## 57. `Date.now()` in test environment vs SSR +- Risk: **minor** +- Why: SvelteKit may SSR `+page.svelte`. `Date.now()` differs between server and client → hydration mismatch warnings if `at` is used in rendered output. +- Verify: any direct render of `at` value; SSR config. + +--- + +## Triage Recommendation (top 10 to fix first) + +1. **#2 Bus reload desync** + **#1 regenerate replay**: root-cause fix = persist `lastHandledAt` AND replay `master.called` on regenerate/reload. Single shared design. +2. **#28 Visibility burst loss**: bus must become queue or pull from `master.called` since last `at`. +3. **#15 Master "Ván mới" doesn't reset player**: define explicit "session reset" signal on bus. +4. **#20 Multiple host tabs**: at minimum show warning, ideally BroadcastChannel. +5. **#12 Voice queue collisions**: define ownership — only player speaks "Chờ/Kinh" after master finishes "Số N". Or pick one speaker in both mode. +6. **#18 Schema drift**: add `schemaVersion`, parse defensively. +7. **#35 iOS gesture**: prime audio on first user click. +8. **#11 Background throttling**: visibilitychange listener to flush queue / pause auto-call. +9. **#7 Mode-toggle interval orphan**: audit interval/effect cleanup. +10. **#43 Three-way enum typos**: grep all `mode ===` usages, normalize to helper `isHost(mode)` / `isGuest(mode)`. + +--- + +## Unresolved questions + +- Q1: Is `lastHandledAt` persisted? (drives #3, #14, #25) +- Q2: Does master's "Ván mới" emit any signal player can react to, or is it localStorage-only? (drives #15) +- Q3: What is the exact spec of "Chờ N" — fired for every draw, or only when N is on grid and uncrossed? (drives #33) +- Q4: Is auto-call resumed on reload? (drives #45) +- Q5: Is there any `addEventListener('storage')` for cross-tab? (drives #20, #21) +- Q6: Does bus have any "reset" / "session" event type, or only number broadcasts? (drives #15, #49) +- Q7: How does `processAutoTick` handle `mode==="master"` — early return or run anyway? (drives #40) +- Q8: Is voice serialized via `cancel()+speak()` or queued? (drives #12, #13) +- Q9: Is there a schemaVersion in any localStorage payload? (drives #18, #26) +- Q10: SSR — does `+page.svelte` actually render dynamic state on server, or fully client-only? (drives #57) diff --git a/plans/reports/code-reviewer-260430-2024-both-mode-consistency.md b/plans/reports/code-reviewer-260430-2024-both-mode-consistency.md new file mode 100644 index 0000000..48c31fd --- /dev/null +++ b/plans/reports/code-reviewer-260430-2024-both-mode-consistency.md @@ -0,0 +1,182 @@ +# Both-mode consistency review + +## Summary +13 findings: 4 critical, 6 major, 3 minor. + +Theme: master's `called[]` is the source of truth, but the bus only carries `lastDrawn`. Any time the player's `crossed` is rebuilt or remounted while master mid-game, prior history is lost. `resetBus()` clears the slot but never resets the consumer's `lastHandledDrawAt`, so cross-side reset semantics are subtly broken. + +## Findings + +### F1: Player regen mid-game loses all prior master draws (CRITICAL) +**Where:** `src/lib/PlayerBoard.svelte:193-207` (`handleGenerate`) +**Symptom:** Master has called e.g. 30 numbers. Player taps "Tạo bảng mới" to reroll the card. New grid intersects with called numbers, but no cells are pre-crossed. Player must wait for the NEXT master draw (which will only mark that one number) — the other ~29 hits are silently lost forever. +**Cause:** Two reasons compounding: +1. `handleGenerate` calls `resetBus()` (line 206) which only nulls `bus.lastDrawn`. Master's `state.called[]` is still the truth, but no replay path exists. +2. The `processAutoTick` effect (line 181) is bus-driven, never history-driven. It can't "catch up" because there's nowhere to read history from. + +Worse: `resetBus()` here punishes the master too. If the master is currently auto-running and the player hits regen, the next master draw will broadcast normally, BUT every other PlayerBoard mount (if `+page.svelte` ever rendered two) loses its bus slot too. The reset reaches across the trust boundary. + +**Repro:** +1. Mode = both, master "Ván mới", draw 10 numbers. +2. Player "Tạo bảng mới" → confirm. +3. Inspect: zero crossed cells on new grid even when 3-4 numbers from `called[]` exist on it. +4. Master "Xổ số" once → only the just-drawn number gets crossed (if on grid). The historical 10 are gone. + +**Fix idea:** +Make master's called list the authority. Either (a) export `getCalledNumbers()` from a shared store and have player's regen replay-cross all of them via `findUncrossedCell` in a loop, or (b) when `mode === "both"` skip the `resetBus()` call AND, on regen, walk `state.called[]` from MasterPanel (lift to a shared store) to pre-cross the new grid. Also drop `resetBus()` from `handleGenerate` — regenerating a card has nothing to do with the master's broadcast slot. + +--- + +### F2: Master "Ván mới" leaves player's crossed marks stale (CRITICAL) +**Where:** `src/lib/MasterPanel.svelte:165-172` (`handleNewGame`) +**Symptom:** Master ends a game (everyone Kinh!), starts a new one. Player's grid is still the OLD card with OLD crossed marks. The new game's first `Xổ số` triggers an auto-tick that may flip a cell on the stale board. Player sees "Chờ X" toasts and even possibly a fake "Kinh!" celebration for a row that was already complete from the prior game. +**Cause:** `handleNewGame` resets the master's own `state` and calls `resetBus()`, but never signals the player to clear `crossed`. Player's `loto_crossed` localStorage entry is untouched. The `processAutoTick` effect's `lastHandledDrawAt` is never reset (it's a `let` $state in PlayerBoard), but `resetBus()` sets `bus.lastDrawn = null` which the effect ignores via the `!lastDraw` early-return — so the dedup cursor stays at the OLD game's last `at`. Then the new game's first broadcast (`{ num, at: Date.now() }`) has a fresh `at > old at`, so it ticks against the stale grid. +**Repro:** +1. Mode = both, master draws until player completes row 1 (Kinh modal shows). +2. Master "Ván mới" → confirm. +3. Master "Xổ số" → if the new number happens to be on the stale grid, player sees an auto-cross on what is visually still the old card. + +**Fix idea:** Either (a) on master "Ván mới", clear player storage (`loto_crossed` only, keep grid) and broadcast a `gameReset` signal — extend the bus to `{ lastDrawn, gameId }`; player effect resets `crossed` when gameId changes; or (b) keep cards/marks across master games and only require player to manually "Xoá đánh dấu" — but document this and at minimum reset `lastHandledDrawAt` to 0 on a `gameReset` signal so timing math doesn't drift. + +--- + +### F3: `resetBus()` is fired by the player but only resets a single shared slot (CRITICAL) +**Where:** `src/lib/PlayerBoard.svelte:206, 219` and `src/lib/call-bus.svelte.js:20-22` +**Symptom:** Player tapping "Tạo bảng mới" or "Xoá đánh dấu" wipes `bus.lastDrawn` for the master too. If two PlayerBoards were ever mounted (or the master's own logic ever depended on the last-drawn marker — currently it has its own `lastCalled`, but that coupling is fragile), this is a cross-component side effect. +More concretely: after player calls `resetBus()`, master's NEXT `Xổ số` broadcasts a fresh `{num, at}`, so player auto-tick fires again — but the player just chose to reset, expecting silence. With the dedup cursor still at the OLD `at`, master's new broadcast (`Date.now()` > old) does fire. So `resetBus` doesn't even achieve "ignore future master draws on this fresh card", it just creates a one-broadcast lull. +**Cause:** The bus has cross-component write semantics but no scoping. `resetBus` is a sledgehammer — the player can't tell "I want my local cursor to reset" from "I want to wipe the master's broadcast slot". Combining (a) the bus state (master broadcasts) with (b) the consumer cursor (player's `lastHandledDrawAt`) is the structural error. +**Repro:** Player taps "Xoá đánh dấu" (line 219) right after master's draw. Now `bus.lastDrawn` is null even though master drew. Immediately switching mode `both → player → both` (which doesn't republish) leaves player with no marker of what was last drawn. Combined with F11, the player's $effect can re-fire on `crossed` change and silently advance state. +**Fix idea:** Drop the `resetBus()` call from BOTH player handlers. Replace with a local-only cursor reset: `lastHandledDrawAt = bus.lastDrawn?.at ?? 0` so the player consumes the current slot without acting. The master owns the bus. + +--- + +### F4: Page reload mid-game leaves bus null but `lastHandledDrawAt = 0`; first new master draw rewrites a fully-restored crossed grid (CRITICAL) +**Where:** `src/lib/PlayerBoard.svelte:47, 86-103, 181-191` + `src/lib/call-bus.svelte.js:10` +**Symptom:** Master has drawn 20, player has 20 cells crossed (auto-ticked + persisted to `loto_crossed`). User reloads the page. Both panels rehydrate from localStorage. Bus is module-state — fresh, `lastDrawn = null`. Player's `lastHandledDrawAt = 0` (its $state init). On the master's NEXT draw (say number 21), the bus publishes `{ num: 21, at: T }`. Player's effect sees `lastDraw.at !== lastHandledAt`, advances, ticks number 21. So far OK. But because the bus IS null on mount, if the player double-clicks "Tạo bảng mới"+"Xoá đánh dấu" quickly, the regen effect runs → `crossed` becomes empty, persisted. Now master's history is gone AND there's no in-bus draw to replay. Same as F1 but post-reload, harder to recover from because the user thinks the reload preserved state. +**Cause:** Bus is in-memory only (`call-bus.svelte.js`), but `crossed` is persisted. State coupling broken across reload. +**Repro:** +1. Mode = both, draw 20, player's grid is half-crossed and persisted. +2. Reload page. +3. Player taps "Tạo bảng mới" → ALL prior auto-ticks gone with no recourse. +4. Or: if master never draws again (game ended), player has no record at all of what was called — only that some cells were once crossed. + +**Fix idea:** Persist the bus alongside master state, or expose master's `called[]` as the canonical source on mount and have player effect run an initial replay-cross pass when it detects `lastHandledDrawAt === 0` AND `state.called.length > 0`. Tied to F1 — same root fix. + +--- + +### F5: `lastHandledDrawAt` is never persisted, so crossed-state and dedup cursor diverge (MAJOR) +**Where:** `src/lib/PlayerBoard.svelte:47` +**Symptom:** Across reloads, `crossed` is restored from localStorage but `lastHandledDrawAt = 0`. If `bus.lastDrawn` happens to be non-null (it never is on cold reload, but could be after HMR or if you ever persist the bus), the player would re-tick the latest already-crossed number. With current code: harmless because bus is null on reload. With any future change to persist the bus, this becomes a re-tick bug. +**Cause:** `let lastHandledDrawAt = 0` is in-memory only, while the state it dedupes against (the bus) and the state it gates (`crossed`) both have persistence stories. +**Repro:** Force-set `bus.lastDrawn` to `{ num: 5, at: 1 }` in a dev tool right after mount (simulating a future persisted bus). Effect fires, double-flips cell containing 5 if it was the only call. +**Fix idea:** Persist `lastHandledDrawAt` as part of `loto_crossed` payload (bump shape to `{ crossed, lastHandledAt }`) or derive it from master state on mount: `lastHandledDrawAt = bus.lastDrawn?.at ?? 0` immediately after the initial-load $effect. + +--- + +### F6: Mode toggle player→both mid-game replays the LAST draw only (MAJOR, partial bug) +**Where:** `src/lib/PlayerBoard.svelte:181-191` + `src/lib/auto-tick.js:35-40` +**Symptom:** User starts in `mode = player` (solo). Master friend joins, host flips to `mode = both` after master has drawn 10 numbers. Player's effect re-fires (mode is reactive in `processAutoTick` args), sees `lastDraw.at !== lastHandledAt` (cursor was 0 in solo), `mode === "both"`, finds an uncrossed cell holding `bus.lastDrawn.num` → ticks ONE cell (the most recent draw). The other 9 historical draws are lost. +This is the same flavour as F1, just triggered by mode toggle. The auto-tick.test.js explicitly documents the "advance lastHandledAt even when mode mismatch" invariant, calling it intentional. The test-comment justification ("solo player switching to 'both' mid-game shouldn't replay a stale draw") explicitly bakes in the partial-replay bug. +**Cause:** `processAutoTick` advances `lastHandledAt` even when `mode !== "both"`. So during solo play, every master broadcast silently consumed the cursor. Toggling to both then has no history to replay. +**Repro:** +1. Mode = player. Master mode toggled off. +2. Set `mode = "both"` first to bind player effect, then back to "player". Master draws 5 numbers (broadcasts still fire). Player effect runs each time, advances `lastHandledDrawAt` to the latest `at`, but mode mismatches so no tick. +3. Switch to "both". Effect re-runs but `lastDraw.at === lastHandledAt` → no-op. Player has zero crosses for 5 already-called numbers. + +**Fix idea:** Don't advance `lastHandledAt` when mode isn't "both" — let it sit at 0 until the first "both"-mode tick. Then on the mode flip, do a one-shot replay over master's `called[]`. Requires exposing `called[]` outside MasterPanel (lift to a shared `master-state.svelte.js`). + +--- + +### F7: `Date.now()` collision swallows the second broadcast within the same ms (MAJOR) +**Where:** `src/lib/call-bus.svelte.js:17` + `src/lib/auto-tick.js:35-37` +**Symptom:** Two master draws within the same millisecond produce `{at: T}` twice with identical `at`. The second is treated as a re-fire and silently skipped by `processAutoTick`'s `lastDraw.at === lastHandledAt` check. +Realistic? Manual button mashing on mobile likely produces ≥2-3ms gaps, but: (a) auto-call interval ≥1s so safe there, (b) the master's `handleDrawNext` is synchronous and could be invoked twice in a microtask boundary if called from a synthetic test, (c) future code (e.g. "skip a number" UX) could draw twice in one tick. Bus assumes monotonic strictly-increasing `at`; `Date.now()` doesn't. +**Cause:** `Date.now()` has 1ms resolution; consumer compares with `===` not `>=`. +**Repro:** Synthetic test: `broadcastDraw(1); broadcastDraw(2);` in same tick; mock `Date.now()` to return `1000` for both. Player's effect runs once with `lastDrawn.num = 2`, ticks 2, never sees 1. +**Fix idea:** Use a monotonic counter instead of `Date.now()`: `let seq = 0; broadcastDraw = n => { bus.lastDrawn = { num: n, seq: ++seq } }`. Update `processAutoTick` to compare `seq`. Also guarantees ordering across clock skew (browser tab throttling can move clock). + +--- + +### F8: `called[]` is the source of truth but only the latest leaks via the bus; effect throw drops history forever (MAJOR) +**Where:** `src/lib/MasterPanel.svelte:88-90, 174-186` + `src/lib/PlayerBoard.svelte:181-191` +**Symptom:** If the player's auto-tick `$effect` ever throws (e.g. `findUncrossedCell` is fed corrupted state, immutable map throws on a frozen sub-array, future feature adds a new code path with a bug), Svelte may continue but the cell flip is lost. There's no retry. `lastHandledDrawAt` may or may not have advanced depending on where in the function the throw happened. Subsequent master draws keep pushing forward, and the "missed" number is permanently lost — the bus only carries the latest. +**Cause:** Single-slot bus + no master-side authority for replay. +**Repro:** Hard to repro deliberately, but consider: `crossed.map(...)` allocates O(81) per draw. On a memory-constrained device, an OOM could throw. Or, more realistically, a future refactor introducing async into the effect breaks ordering. +**Fix idea:** Promote `called[]` to a shared `$state` store (lift from MasterPanel into `master-state.svelte.js`). Player effect derives "what should be crossed" from `called` + grid via a pure projection, not a per-event flip. The grid+called → crossed function is idempotent and immune to throw losses. + +--- + +### F9: voiceEnabledMaster + voiceEnabledPlayer simultaneous → waiting/Kinh cancels number announcement (MAJOR) +**Where:** `src/lib/voice.js:53-95` (single `activeClip`/`activeToken` slot) + `PlayerBoard.svelte:140, 152` + `MasterPanel.svelte:185` +**Symptom:** Mode = both, both voice flags on. Master draws, calls `playNumber(n)` → audio "bốn mươi hai" begins. Same draw triggers `processAutoTick` → `crossed` updates → second $effect re-runs → if a row is now waiting OR complete, `playWaiting` or `playBingo` is called, which immediately `cancelPlayback()`s the master's "bốn mươi hai" mid-syllable, then plays "chờ" or "kinh!". +The host hears "bốn—chờ" or "bốn—kinh!" — the number itself is cut. Players around the host don't hear what was called, only the reaction. +**Cause:** `voice.js` uses a single global activeClip slot with `cancelPlayback()` at the top of every `playX`. There's no priority queue; last writer wins. Master and player publishers race during the same draw → render → effect chain. +**Repro:** +1. Mode = both, both voice toggles on, voiceWaitingNumber off. +2. Manually set up a player grid where one row needs exactly one number. +3. Master "Xổ số" → that exact number is the next call. Listen: number cut off mid-pronunciation by "Kinh!". + +**Fix idea:** Introduce a small queue: `enqueue(clip, priority)`. Number takes precedence over Chờ; Kinh is highest. Or sequence them: number → 250ms gap → chờ/kinh. Or — simplest — when both flags are on and mode is both, suppress the player-side announcement (master is the announcer; player flag becomes redundant). The current effect at PlayerBoard.svelte:117-118 already partially routes around this for solo player, but doesn't suppress the duplication when both flags are on. + +--- + +### F10: Player "Xoá đánh dấu" loses already-called numbers permanently (MAJOR) +**Where:** `src/lib/PlayerBoard.svelte:209-220` (`handleClear`) +**Symptom:** Player accidentally taps "Xoá đánh dấu" (or genuinely wants to clear). All marks gone. Master is mid-game with 30 called numbers. The player is now at zero crosses with no replay path. Next master draw will re-cross only that one number. +**Cause:** Same root as F1: master's history is unreachable from the player. +**Repro:** +1. Mode = both, master at 30 calls, player has ~17 crosses. +2. Player "Xoá đánh dấu" → grid blank. +3. Master keeps drawing — only new draws cross. The 17 historical hits never come back unless that exact number is re-broadcast (which it won't, it's been consumed from `remaining`). + +**Fix idea:** On clear, in `mode === "both"`, replay-cross master's `called[]` against the (existing) grid before marking complete. Keep the manual single-cell untick separate from a wholesale clear. Additionally, prompt the user "This will clear and re-apply called numbers" when `mode === "both"` so the action is informed. + +--- + +### F11: Reactive effect re-fires on `crossed`/`grid` change but is correctly gated — verify under Svelte 5 fine-grained reactivity (MINOR/uncertain) +**Where:** `src/lib/PlayerBoard.svelte:181-191` +**Symptom:** The auto-tick `$effect` reads `bus.lastDrawn`, `lastHandledDrawAt`, `grid`, `crossed`, `settings.mode`. Any of these changing re-fires it. The dedup-by-`at` invariant claims to prevent re-runs from causing a re-flip — and the `auto-tick.test.js` tests verify the pure function. But the effect WRITES `crossed` (line 190) which is one of its dependencies. Svelte 5 runes do detect cycles; usually batches and short-circuits. +However: if Svelte ever schedules the rerun BEFORE the assignment to `lastHandledDrawAt` is committed (line 189 fires a $state write), there's a brief window where `lastHandledAt` is the OLD value and `lastDraw.at` is the new one — re-firing would `findUncrossedCell` on the now-already-crossed cell, return null, no harm. So this is theoretically safe BUT the safety hangs entirely on the `findUncrossedCell` fallthrough. +**Cause:** Effect both reads and writes `crossed` and `lastHandledDrawAt` in the same execution. Standard Svelte 5 should serialize this, but there's no test covering "what if the reactive system schedules a re-run between line 189 and 190". +**Repro:** Hard. Theoretical. Code currently passes tests. +**Fix idea:** Use `untrack()` for the writes, or restructure: compute the result, then in a `flushSync`-style microtask write both. Or accept current semantics and add a comment + test for "re-entrant scheduling cannot double-tick". + +--- + +### F12: `autoCallEnabled` toggle off mid-run leaves player partially up-to-date (MINOR) +**Where:** `src/lib/MasterPanel.svelte:120-139` (master auto-call effect) +**Symptom:** Master is auto-running. Host flips `autoCallEnabled` off in settings. Master effect tears down the interval (line 122-125 sets `autoRunning = false`), so no more auto-broadcasts. Player auto-tick effect doesn't care — it just stops receiving new draws. So far OK. +But: if host then flips `autoCallEnabled` back on, `autoRunning` is now false (it was reset on disable), so the master would have to manually press "Bắt đầu" again. Meanwhile the player has been quietly accruing nothing. No bug per se, just a UX cliff. +**Cause:** `autoRunning` and `autoCallEnabled` are two pieces of state with overlapping semantics. The disable path resets `autoRunning` to false (correct), but there's no "remembered intent" to restart on re-enable. +**Repro:** +1. Mode = both, autoCallEnabled = true, autoRunning = true. Master is calling every 5s. +2. Open settings, toggle autoCallEnabled off, then on. +3. Auto-call doesn't resume; manual "Bắt đầu" required. + +**Fix idea:** Either document this in the settings UI ("Toggling off stops auto-run; tap Bắt đầu to resume") or persist `autoRunning` across the toggle. Probably the former — it's user-initiated. + +--- + +### F13: Multiple PlayerBoard mounts share `lastHandledDrawAt = 0` initial state but each instance has its own copy — tested behavior unclear (MINOR) +**Where:** `src/lib/PlayerBoard.svelte:47` + `src/routes/+page.svelte:33-35` +**Symptom:** `+page.svelte` only mounts ONE PlayerBoard. But the architecture suggests "two cards on one device" might be a future ask (verified by F-search of the repo). If two PlayerBoards mounted, both share the same `bus.lastDrawn`, `STORAGE_PREFIX = "loto"` (so SAME localStorage key for grid and crossed — they'd overwrite each other). Each has its own `lastHandledDrawAt` $state, so each correctly handles its own dedup. But the localStorage collision means one's persistence eats the other's. +**Cause:** `STORAGE_PREFIX` is hardcoded; bus is single-slot global. +**Repro:** Mount two `` instances in `+page.svelte`. Tap "Tạo bảng mới" on instance A → instance B's localStorage is overwritten on next persist effect run. Both grids end up showing the same card. +**Fix idea:** Accept `prefix` as a prop with default `"loto"`. Caller mounts `` and ``. Bus remains global (correct — both should auto-tick on master draw). Safe even if not used now. + +--- + +## Prioritized fix path + +1. **Lift `called[]` to a shared store** (`src/lib/master-state.svelte.js`): `{ called: $state([]), remaining: $state([]) }` with `drawNext()`, `newGame()`, `reset()`. MasterPanel imports this, so does PlayerBoard. +2. **Replace bus dedup with seq counter** (F7). +3. **Player auto-tick becomes `called`-derived**: `crossed` is a $derived projection of `(grid, called)` via a pure idempotent function, with manual untick handled by an "exclude" set. Eliminates F1, F2, F4, F6, F8, F10 in one structural change. +4. **Drop `resetBus()` calls from PlayerBoard handlers** (F3) — they don't belong there. +5. **Voice queue** (F9): trivially solved by suppressing player-side announcements when `mode === "both" && voiceEnabledMaster`. + +## Unresolved questions + +- Is "card persists across master Ván mới" the intended UX, or should new master game force-clear player marks? (Affects F2 fix shape.) +- Should "Xoá đánh dấu" in mode=both behave as "clear AND replay-cross called" or "true clear, ignore called"? Need product call. (Affects F10.) +- Future ask: multiple PlayerBoards on one device — is that on the roadmap? (Affects whether F13 is worth fixing now.) +- Manual untick behavior: today, a re-broadcast of the same number doesn't happen (master never repeats). But if it did (testing-only `replay` button), would re-cross be desired? Current pure function says yes. diff --git a/plans/reports/code-reviewer-260430-2103-both-mode-refactor.md b/plans/reports/code-reviewer-260430-2103-both-mode-refactor.md new file mode 100644 index 0000000..b1d9570 --- /dev/null +++ b/plans/reports/code-reviewer-260430-2103-both-mode-refactor.md @@ -0,0 +1,354 @@ +# Code review: both-mode state consistency refactor + +Plan: `plans/260430-2050-both-mode-state-consistency/` +Reviewer scope: static review of `master-store.svelte.js`, +`player-auto-cross.js`, `MasterPanel.svelte`, `PlayerBoard.svelte`, +`game-logic.js` (manualUnticks helpers), the three new/extended test +files, and `+page.svelte` to confirm mode mounting. + +## Severity counts + +- **Critical:** 0 +- **High:** 1 +- **Medium:** 4 +- **Low:** 4 +- **Nits / informational:** 3 + +Net: refactor is solid. The shared store is small, deletes more code +than it adds, and the bus is fully retired (`grep call-bus|auto-tick| +broadcastDraw|resetBus` only finds a stale doc comment in +`game-logic.js` line 309). The four product flows are all wired per +locked decisions. One real cross-component bug + a few subtle traps +documented below. + +--- + +## High + +### H1 — `MasterPanel` unmount in mode=player drops live master state silently + +`+page.svelte` lines 33–50 conditionally renders MasterPanel only when +`settings.mode !== "player"`. Persistence and `loadMaster()` live in +`MasterPanel`'s `$effect`, so: + +- In **player** mode the master panel never mounts → `loadMaster()` + never runs → `masterState.called` stays `[]` for the entire session. + That's fine in solo player mode. +- BUT the moment the host toggles **player → both**, the panel mounts + fresh, `loadMaster()` reads persisted state, and `masterState.called` + jumps `0 → N` reactively. This trips PlayerBoard's master-reset + detection effect *in reverse*: it sees `prevCalledLen=0, len=N`, no + reset fires (correct), and `applyMasterCalls` sees a backlog of N + calls with `lastHandledIndex=N` (initialized that way at PlayerBoard + mount). Result: **the player board does NOT replay master's persisted + history when switching to both mode** — phase 3's "open question" + behavior, but undocumented in code. + +Phase 3 doc says this is intentional ("cursor was advanced past it"), +but the load order is fragile: PlayerBoard's load $effect runs *first* +because PlayerBoard always mounts (it lives outside the +`mode !== "master"` gate? — actually it's inside `mode !== "master"`, +so it unmounts in master-only mode but mounts in both mode). When mode +flips player→both: +- PlayerBoard already mounted with `lastHandledIndex = 0` (first mount + in player mode read `masterState.called.length === 0`). +- Then MasterPanel mounts → `loadMaster()` → `masterState.called` becomes + the persisted N entries. +- Reactivity fires the auto-cross effect. `mode === "both"` now, grid + exists, `lastHandledIndex (0) < called.length (N)` → it WILL replay + the entire back-history at once. + +So the documented "no replay on toggle" property is **only true if +mode toggle happens after MasterPanel was already mounted at least +once**. If the user lands the page in `mode=player` and toggles to +`both` for the first time, every persisted master draw will auto-cross +the player board in one shot — surprising, possibly desirable, but not +what phase 3's "open question" claims. + +**Fix options:** +1. Accept the behavior, update phase 3's "open question" to "we do + replay on first mount in both/master modes — known and OK". +2. Move `loadMaster()` to module scope (or `+page.svelte`), so master + state is always primed. Then PlayerBoard's mount-time + `lastHandledIndex = masterState.called.length` line correctly + captures the persisted backlog as "already in sync with persisted + crossed", honoring the docstring. + +Option 2 is cleaner and matches the docstring "Treat reload as already +in sync with master's full history". Recommend this. + +--- + +## Medium + +### M1 — Effect self-trigger: `applyMasterCalls` $effect reads `crossed`, writes `crossed` + +PlayerBoard.svelte:205–218. + +The effect reads `grid`, `crossed`, `masterState.called`, +`lastHandledIndex`, `manualUnticks`, `settings.mode`, then conditionally +writes `lastHandledIndex` and `crossed`. Svelte 5 effects re-run on +ANY tracked-dep change, including the very ones they wrote. + +- When `applyMasterCalls` returns `changed: true`, the effect writes a + new `crossed`. That triggers a re-run. +- Re-run: `lastHandledIndex` was bumped to `called.length` on the same + pass, so guard `lastHandledIndex >= called.length` short-circuits to + `{changed: false, lastHandledIndex unchanged}`. No write, no further + re-run. **Safe.** +- When `manualUnticks` changes (user untick): the effect re-runs with + the same `lastHandledIndex === called.length` → short-circuit. **Safe.** +- When `crossed` changes via manual click: same — short-circuit. **Safe.** + +Verdict: not a bug, but the safety hinges on `applyMasterCalls`'s early +return at line 33–35. Add a comment in PlayerBoard pointing at the +guard, otherwise a future "always recompute crossed from called[]" pure +refactor would silently introduce an infinite loop. + +### M2 — Master-reset detection effect — first-run semantics + +PlayerBoard.svelte:223–234. + +```js +$effect(() => { + const len = masterState.called.length; + const wasReset = prevCalledLen > 0 && len === 0; + prevCalledLen = len; + if (wasReset && settings.mode === "both" && grid) { ... } +}); +``` + +The effect declares a dep on `masterState.called.length` (read) and +writes `prevCalledLen` (written). It does not read `prevCalledLen` — +wait, it DOES read it (`prevCalledLen > 0`). So it reads + writes +`prevCalledLen` and reads `masterState.called.length`. Self-trigger +risk is real **but** the write happens unconditionally to the current +length, and once they're equal the read+write pair is idempotent: write +the same value → Svelte's proxy short-circuits identical assignments to +the underlying signal? In Svelte 5 runes, `$state` writes that produce +the same value DO NOT trigger reactivity (proxy short-circuit on +primitive equality). So no infinite loop. + +Mount semantics: `prevCalledLen` is initialized to `0` in `$state`, then +the load effect at line 119 sets it to `masterState.called.length`. +Effect ordering between the load effect and the reset-detect effect is +NOT guaranteed by Svelte. If the reset-detect effect runs first on +mount with `prevCalledLen = 0` and persisted `called.length > 0`, then +`wasReset = (0 > 0 && N === 0)` = false. **Safe by accident** — only +the `>0 → 0` shape qualifies as reset, so first-mount transitions +`0 → N` and `N → N` are both no-ops. + +Suggest hardening with a one-line comment: `// prevCalledLen=0 + len>0 +is NOT a reset — only >0 → 0 qualifies, so init order vs load $effect +doesn't matter.` + +### M3 — `manualUnticks` correctness: pre-call manual cross then user untick + +`handleCellClick` lines 318–323: + +```js +if (num > 0 && masterState.called.includes(num)) { + ... +} +``` + +Walk-through: user manually crosses cell holding `42` BEFORE master +draws 42. `crossed[r][c] = true`, `manualUnticks` unchanged (because +`called.includes(42) === false`). Master then draws 42; auto-cross +runs `findUncrossedCell(grid, crossed, 42)` which returns null (already +crossed) → no-op, `lastHandledIndex` advances. So far so good. + +Now user clicks the cell again (untick). `wasCrossed = true`, +`willBeCrossed = false`. `called.includes(42)` is now true → +`next.add(42)`. `manualUnticks = {42}`. Cell becomes false. ✓ Replay +flows skip 42 thereafter. Correct. + +Edge case: user manually crosses 42 pre-draw, master draws, user does +NOT untick, then user clicks "Xoá đánh dấu". `manualUnticks` was empty +at handleClear time → it's reset to `new Set()`, then `applyMasterCalls` +re-crosses 42 (because `findUncrossedCell` finds it on the cleared +grid). Correct outcome — the manual cross was indistinguishable from +auto, replay redoes it. + +Edge case: `includes()` is O(n) and runs on every cell click. With max +90 calls it's trivial — fine. (Could be a Set on `masterState`, but +YAGNI.) + +**Verdict:** logic is correct. Documented `O(called)` cost is +acceptable. + +### M4 — `lastCalled` re-derive over the whole array + +MasterPanel.svelte:59–63 reads `masterState.called[masterState.called.length-1]`. +Fine. But line 77 `callOrder` rebuilds a `Map` on every change to +`masterState.called`. Since `called` is replaced (not mutated) on every +draw, this is unavoidable cost — O(n) per draw, n ≤ 90. Trivial. Just +flagging that `derived` does not memoize per-element diffs. + +--- + +## Low + +### L1 — `applyMasterCalls` builds a new outer array per call (allocation churn) + +Line 49–53 does `next = next.map(...)` once per matched call. Replaying +90 back-history hits: 90 outer-array allocations of length 9. Negligible +(<10 µs total). Could pre-clone once and mutate, but that breaks the +pure-function contract documented at the top. Leave as is — KISS. + +### L2 — Deep-equal short-circuit absent in `applyMasterCalls` no-flip path + +Line 33–35 short-circuits on cursor-at-end. Line 39–41 short-circuits +on mode mismatch. But line 44–55 walks all calls even if every single +one is in `manualUnticks` or off-board, returning the same `next` +reference. The test `returns same crossed reference when no flip +happens` (line 122) confirms this works for off-board nums. Not a bug, +just verifying the test asserts it correctly. ✓ + +### L3 — `saveMaster` / persistence effect runs on mount with empty arrays + +MasterPanel.svelte:69–74 — second $effect reads `called` + `remaining` +and calls `saveMaster()`. On mount BEFORE the load effect runs, both +are empty → `saveMaster()` writes `{"called":[],"remaining":[]}` to +storage, **clobbering any persisted state**. Then the load effect runs +and reads … the just-clobbered empty state. Game state lost on every +mount. + +Wait — let me re-read. The two `$effect`s are sibling effects. +Per Svelte 5, on initial mount they run in declaration order: load +($effect at 65) runs first, populates `masterState`, THEN save effect +at 69 runs and persists what was just loaded. **Safe by declaration +order.** + +But this depends on declaration order. Move them out of order and you +silently corrupt storage. Add a comment: `// IMPORTANT: load effect +must remain declared before save effect, or save will clobber storage +on mount.` + +Edge: if `loadMaster()` is a no-op (no key in localStorage), save +effect writes `{[],[]}`. That's fine — persists "no game" cleanly. + +### L4 — `prevCalledLen` exposed as `$state` but only used internally + +It doesn't need to be `$state` for the current logic — a plain `let` +read+written in the same effect works. Making it `$state` adds a +reactive dep that the effect both reads and writes (see M2). Switching +to plain `let` would remove the self-trigger concern entirely. Same for +`lastHandledIndex` — it's only read inside `applyMasterCalls`, and +written in a single effect; tagging it `$state` opts it into Svelte's +reactivity graph for no UI consumption. Consider non-reactive `let` for +both. Cosmetic only. + +--- + +## Nits / informational + +### N1 — Stale doc comment in `game-logic.js` + +Line 309 still says `Used by the master→player auto-tick path.` The +"auto-tick" name dies with this refactor (replaced by "auto-cross"). +Cosmetic — update to `auto-cross` for greppability. + +### N2 — `resetMaster` vs `startNewGame` from player POV + +PlayerBoard's reset-detect effect fires on `called.length: >0 → 0`. +- `startNewGame`: sets `called = []`, then `remaining = shuffled1to90()`. + Two writes, but Svelte batches sibling reactive writes within a tick. + Effect re-runs once with `called.length === 0` → triggers reset. + Correct. +- `resetMaster`: sets both to `[]`. Effect sees `called.length === 0` → + triggers reset. Same outcome. + +Player POV is identical. ✓ matches the question in the brief. + +### N3 — Test coverage gaps + +- No PlayerBoard.svelte component test. The four product flows + (master Ván mới wipes, player regen replays, player Xoá đánh dấu + replays, master draw auto-crosses) are tested at the helper level + only. A tiny `vitest-svelte` mount + `flushSync` harness for one + flow would catch effect-ordering regressions like H1 / L3. +- No test for `prevCalledLen` mount-edge (load order vs effect order). +- No test for "manual cross before draw → master draws → manual untick + populates manualUnticks correctly" (M3 walkthrough). + +Not blocking; flag for follow-up. + +--- + +## Persistence-ordering audit + +Multiple effects write to localStorage on the same reactive change: + +- `MasterPanel`: 1 save effect on `masterState`. ✓ +- `PlayerBoard`: 3 save effects — `manualUnticks`, `crossed`, plus + `saveGrid` inline in `handleGenerate`. They all touch different keys + (`loto_master`, `loto_manualUnticks`, `loto_crossed`, `loto_grid`). + No key collision → no stale-write race. Svelte batches state + mutations within a tick, so a single user action triggers each save + effect at most once per tick. + +**Verdict:** no race. Keys are disjoint, writes are last-write-wins per +key, and there's only one writer per key. + +--- + +## Mode-toggle behavior matrix (verified) + +| Initial mode | Toggle to | Master state behavior | Player crossed behavior | +|---|---|---|---| +| both | player | MasterPanel unmounts; cancelPlayback fires | PlayerBoard stays mounted; auto-cross effect re-runs with `mode=player` → advances cursor, no flips ✓ | +| both | master | PlayerBoard unmounts | masterState retained; on remount PlayerBoard reloads, sets `lastHandledIndex = called.length` (sync) ✓ | +| player | both | MasterPanel mounts, loads persisted master state → `called.length` jumps `0 → N` | PlayerBoard's auto-cross effect sees `lastHandledIndex=0 < N` → **replays full back-history** (see H1) | +| master | both | PlayerBoard mounts fresh, `lastHandledIndex = called.length` = N | No replay (in sync with persisted crossed) ✓ | + +Row 3 is the H1 surprise. Row 1 is correctly handled by the cursor +advance in `applyMasterCalls` non-both branch. + +--- + +## Recommended actions (in priority order) + +1. **H1**: decide policy for player→both toggle (replay all or skip). + If "replay all" is desired, hoist `loadMaster()` to module-init or + `+page.svelte`. If "skip", add a guard that bumps `lastHandledIndex + = masterState.called.length` whenever `settings.mode` transitions + into `both`. +2. **M1, M2 comments**: short pointer comments locking in the + self-trigger safety conditions so a future refactor doesn't break + them silently. +3. **L3 comment**: declaration-order dependency in MasterPanel. +4. **N1**: rename "auto-tick" stale comment. +5. **N3**: one component test for the master Ván mới reset flow would + add real value; leave the rest if quota is tight. + +--- + +## Positive observations + +- Pure helper extraction (`applyMasterCalls`) is clean — easy to test, + easy to reason about, no Svelte coupling. +- `findUncrossedCell` reused (DRY) for both auto-cross and the regen + replay path. +- All persistence helpers validate input (`isValidNumberArray`, + `isUnticksArray`, size cap, `__proto__` reviver). Defense-in-depth + pattern is consistent across the new `loto_master` key and the + pre-existing `loto_grid`/`loto_crossed` paths. +- Bus is fully retired. Only stale reference is a doc comment. +- The locked product decisions (master Ván mới wipes; player Xoá đánh + dấu replays) are implemented exactly as specified. +- `manualUnticks` solves the "regen wipes my manual untick" problem + cleanly — minimal state, mirrors a single user intent. + +--- + +## Unresolved questions + +1. H1: is the player→both first-toggle replay surprising or expected? + Phase 3 doc claims "no replay on toggle" but actual behavior depends + on whether MasterPanel mounted earlier. +2. Should `prevCalledLen` and `lastHandledIndex` drop the `$state` + wrapper since they're not consumed by templates? (perf nit, not + correctness) +3. Multi-tab scenario explicitly out of scope (#20) — confirmed in + plan.md, no action.