diff --git a/plans/260831-2314-review-followups/plan.md b/plans/260831-2314-review-followups/plan.md new file mode 100644 index 0000000..dece9c2 --- /dev/null +++ b/plans/260831-2314-review-followups/plan.md @@ -0,0 +1,93 @@ +# Review follow-ups: multi-tab contract, backup rules, settings-loaded race + +Status: done (2026-08-31) | Branch: main | Mode: auto (user delegated decisions) + +Completion: Phases 1-3 done. Pre-commit review surfaced H-1/M-1..M-4 +regressions in the fixes themselves; all fixed same cycle (reports +`fullstack-developer-260831-2314-web-multitab.md`, +`fullstack-developer-260831-2334-android-regression-fixes.md`, +`code-reviewer-260831-2314-precommit-review.md`). Final: web 144/144 +tests, eslint/svelte-check clean, both builds + verify-pwa pass; +android 129/129 tests, lint clean. Deferred (unchanged): M6 +recomposition perf (needs profiled baseline); reviewer lows — +web loadMaster deck validation parity, unlockAudio player-gesture, +loading-flag timeouts, verify-pwa chunk heuristic, empty onCleared, +Robolectric-less VoicePlayerHolder glue test. +Source evidence: `plans/reports/code-reviewer-260831-2213-web-review.md` (M5), +`plans/reports/code-reviewer-260831-2213-android-review.md` (M4 residual, L9), +`plans/reports/fullstack-developer-260831-2226-android-fixes.md` (M4 gap detail). + +## Outcome + +Close the last three review items left pending as "product decisions", using +the decisions delegated to the assistant: + +1. **Web M5** — two tabs opened simultaneously freeze each other. Decision: + newest claim wins, deterministic tie-break. +2. **Android L9** — decision: exclude in-progress round state + (`loto_game_state` DataStore) from Android backup/device-transfer; keep + settings backed up. Rationale: a fairground round is device-local and + transient; restoring it on another device mid-round is confusing, settings + are worth keeping. +3. **Android M4 residual** — master restore landing before settings resolves + replays history under default PLAYER mode. Decision: thread a + settings-loaded signal and gate master replay on it. + +## Non-goals + +- Master-panel recomposition perf (M6): deferred until a profiled baseline on + the API 24 QA device exists. +- Any redesign of the multi-tab UX beyond fixing the double-freeze. + +## Phases + +### Phase 1 — web: claim contract (owner: web agent) + +Files: `web/src/lib/active-tab.svelte.js`, `web/src/lib/active-tab.test.js`, +`web/src/routes/+layout.svelte` (only if wiring changes). + +- Add a claim timestamp: record `myClaimTs` when this tab claims (mount and + `claimActiveTab()`). Include `ts` + `id` in the broadcast. +- On peer claim: ignore if `ts < myClaimTs` (this tab is newer); freeze if + newer; tie-break equal `ts` deterministically on `id` so exactly one tab + wins. +- Keep the H2 reclaim re-hydration flow intact. + +Acceptance: simulated simultaneous mount (equal + unequal ts) leaves exactly +one tab active; existing reclaim/handover tests still pass; 138+ vitest suite +green, eslint + svelte-check (checkJs on) clean. + +### Phase 2 — android: backup rules + settings-loaded signal (owner: android agent) + +Files: `android/app/src/main/AndroidManifest.xml`, new +`android/app/src/main/res/xml/data_extraction_rules.xml` (+ +`backup_rules.xml` for API < 31), `settings/SettingsRepository.kt`, +`LotoApplication.kt`, `state/PlayerBoardViewModel.kt` (+ MasterStore only if +needed), tests under `app/src/test`. + +- Backup: `android:dataExtractionRules` + `android:fullBackupContent` + excluding the game-state DataStore file(s) (verify actual on-disk name); + settings DataStore stays included. +- Settings-loaded signal: expose loaded-state from `SettingsRepository` + (e.g. nullable initial value or `loaded: StateFlow`); ViewModel + must not consume master emissions (must not advance the auto-cross cursor) + until settings have resolved — closes the reverse interleaving documented + in the prior fix report §M4. + +Acceptance: new SlowDataStore-based test proving master-restore-before- +settings no longer mis-consumes history under default mode (fails without +fix); backup XML referenced from manifest and lint-clean; full +`:app:testDebugUnitTest` + `:app:lintDebug` green. + +### Phase 3 — verify + review (owner: controller) + +- `tester` agent: full web + android suites, builds. +- `code-reviewer` agent: diff review vs acceptance criteria, contract/blast- + radius check. +- Finalize: plan sync-back, docs impact check, offer commit. + +## Risk / rollback + +All changes additive and localized; rollback = git checkout of touched files. +No schema/data migrations. Backup-rule change affects only future device +backups. diff --git a/plans/journals/2026-08-31-dual-platform-review-fix-cycle-web-pwa-android-state-races.md b/plans/journals/2026-08-31-dual-platform-review-fix-cycle-web-pwa-android-state-races.md new file mode 100644 index 0000000..202f0da --- /dev/null +++ b/plans/journals/2026-08-31-dual-platform-review-fix-cycle-web-pwa-android-state-races.md @@ -0,0 +1,31 @@ +--- +title: "Dual-platform review-fix cycle: web PWA + Android state races" +date: 2026-08-31 +summary: "Review agents found dead PWA + state races on both platforms; three fix waves, pre-commit review caught regressions in our own fixes, all verified green" +--- + +# Dual-platform review-fix cycle: web PWA + Android state races + +## What happened + +Full review-fix cycle over `web/` (SvelteKit) and `android/` (Kotlin/Compose), driven by parallel review and fix subagents. + +Review pass found: web PWA entirely dead (SW never registered — adapter-static prerenders after the PWA plugin's closeBundle; audio precache also missing the `/loto` base, 92 would-be 404s), frozen-tab reclaim rolling back the live round via a stale save effect, and on Android a restore/DataStore race that silently discarded a fresh card plus confetti rendered invisibly behind the kinh Dialog window. + +Fix wave 1 closed all criticals/highs + clear-cut mediums (SW registration + `verify-pwa-build.mjs` CI guard, shared `base-path.js` resolver, hydration-gated saves, loading-gated actions with a delay-injecting `SlowDataStore` fake, confetti embedded in the dialog window). Wave 2: `registerType: "prompt"` reload banner (docs already promised a toast; config contradicted them), `checkJs: true` with 56 type-only fixes, Android L2–L6/L7/L10. Wave 3 (delegated decisions): newest-claim-wins multi-tab contract, `dataExtractionRules` excluding only `datastore/loto_game_state.preferences_pb`, settings-loaded signal closing the M4 reverse-ordering replay race. + +## Key lesson + +Independent pre-commit review of the accumulated diff caught real regressions **introduced by our own fixes**: `voicePlayer.release()` on `isFinishing` left the `by lazy` app-scoped ExoPlayer permanently dead after relaunch into a cached process (fixed with a recreatable `VoicePlayerHolder`), and the one-way claim protocol could leave two tabs active (fixed with winner-echo + missing-`ts` = `Infinity`). Also folded settings-loaded into the exposed `loading` state after the reviewer showed `generate()`/`clearMarks()` dodged the gate. + +## Final state + +Web 144/144 vitest, eslint + svelte-check (checkJs on) clean, both build profiles + PWA verification pass. Android 129/129 unit tests, lint clean. Uncommitted on `main`. Deferred: master-panel recomposition perf (needs profiled baseline), reviewer lows (loadMaster deck-validation parity, player-gesture audio unlock, loading timeouts, verify-pwa chunk heuristic). + +## Next steps + +- Commit (user decision pending). +- Profile master panel on the API 24 QA device before touching recomposition. +- Consider Robolectric for the VoicePlayerHolder/Activity glue, currently untestable on JVM. + +> Historical work record — not durable authority. Prefer docs/specs/ADRs for current decisions. diff --git a/plans/reports/code-reviewer-260831-2213-android-review.md b/plans/reports/code-reviewer-260831-2213-android-review.md new file mode 100644 index 0000000..e883c08 --- /dev/null +++ b/plans/reports/code-reviewer-260831-2213-android-review.md @@ -0,0 +1,395 @@ +# Android native app — code review (2026-08-31) + +Scope: `android/app/src/**`, `android/*.gradle.kts`, `gradle.properties`, +`gradle/libs.versions.toml`, `AndroidManifest.xml`, `res/**`, +`.github/workflows/android-release.yml`. Read-only review; no source changed. + +Baseline: ~2.9k LOC main + ~1.5k LOC unit tests + 4 instrumented tests. +Overall the port is disciplined — pure game logic is isolated and well +covered, DataStore validation is per-field, and the web-parity notes are +specific rather than decorative. Findings below are the gaps. + +--- + +## Critical + +None. No trust-boundary, data-loss-at-rest, or crash-on-launch defect found. +The app takes no network input, declares no `INTERNET` permission, exports +only the LAUNCHER activity, and the signing config reads env vars only. + +--- + +## High + +### H1 — Restore races a user action and silently clobbers a fresh card/round + +`android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt:76-101` +`android/app/src/main/java/com/miti99/loto/state/MasterStore.kt:25-29` + +`restore()` suspends on `dataStore.data.first()` while the UI is already +composed and interactive. During that window `uiState.grid == null`, so +`PlayerBoardScreen.kt:70-73` takes the **no-confirm** branch: +`if (state.grid != null) confirmGenerate = true else viewModel.generate()`. +A single tap on "Tạo bảng mới" during the load therefore generates + persists +a new card; `restore()` then resumes and unconditionally overwrites +`grid`/`crossed`/`manualUnticks` with the stale saved round, without +re-persisting. Result: the user's new card vanishes, and in-memory state now +disagrees with what is on disk until the next `persist()`. + +Identical shape on the master side: `MasterPanelScreen.kt:86` calls +`viewModel.newGame()` with no confirmation while `hasGame == false` (which is +exactly the pre-restore state), and `MasterStore.restore()` then replaces the +deck with the persisted one. + +Fix: make restore a no-op once the user has acted, and/or gate the buttons on +a load-complete flag. + +```kotlin +// PlayerBoardViewModel +private var restored = false +private suspend fun restore() { + val saved = repository.loadPlayerState() + if (grid != null) { restored = true; return } // user already generated + ... +} +// MasterStore +suspend fun restore() { + if (deck.remaining.isNotEmpty() || deck.called.isNotEmpty()) return + ... +} +``` + +Cleaner alternative: add `val loading: Boolean` to `PlayerUiState` / +`MasterRoundState` exposure and render the empty state as non-interactive +until both restores settle. + +### H2 — Tier-2 confetti is emitted inside the scrolling column, behind the dialog + +`android/app/src/main/java/com/miti99/loto/ui/player/PlayerBoardScreen.kt:154-159` +`android/app/src/main/java/com/miti99/loto/ui/player/Confetti.kt:615-624` + +`ConfettiOverlay` uses `Modifier.fillMaxSize()`, but it is emitted as a +sibling in `LotoAppRoot`'s `Column(Modifier.verticalScroll(...))` +(`LotoAppRoot.kt:64-100`). Under an unbounded max height, Compose's fill +modifier falls back to `constraints.minHeight`, so the Box wraps its content +instead of filling the screen — `size.height` used for `translationY` +(`Confetti.kt:640`) is the height of a single emoji, not the viewport. + +Worse, `KinhDialog` is a real `Dialog` in its own window with the default dim +scrim, so the confetti is drawn *behind* the dim layer in the activity +window. The tier-2 celebration is effectively invisible and also injects +layout into the scroll content. + +Fix: host the confetti in the same window as the dialog, or in a `Popup`: + +```kotlin +if (state.showCongrats) { + KinhDialog(congratsRow = state.congratsRow, onDismiss = { viewModel.dismissCongrats() }) + if (state.celebrationTier >= 2 && !reducedMotion) { + Popup(properties = PopupProperties(clippingEnabled = false)) { + Box(Modifier.fillMaxSize()) { ConfettiOverlay() } // bounded by the popup window + } + } +} +``` + +(Or move `ConfettiOverlay()` inside `KinhDialog`'s `Dialog { }` content with +`usePlatformDefaultWidth = false` + `decorFitsSystemWindows = false`.) + +--- + +## Medium + +### M1 — RGB channel truncation drifts the empty-cell colour on every drag + +`android/app/src/main/java/com/miti99/loto/ui/settings/EmptyCellColorPicker.kt:369-403` + +```kotlin +val r = (color.red * 255).toInt() // truncation, not rounding +``` + +`Color.red/green/blue` are Float. `toInt()` truncates, so a float round-trip +that lands at `126.9999` yields 126 for a stored `0x7F`. Because dragging the +R slider rebuilds the hex from the *derived* `g` and `b` +(`draft = hex(it, g, b)`), any truncation error on the untouched channels is +committed too — repeated drags walk the colour downward. + +Fix: `val r = (color.red * 255).roundToInt()` (same for g/b). + +### M2 — Every draw force-scrolls the page to the master hero, even in "both" mode + +`android/app/src/main/java/com/miti99/loto/ui/master/MasterPanelScreen.kt:73-75` + +```kotlin +LaunchedEffect(tickKey) { if (tickKey > 0 && lastCalled != null) heroRequester.bringIntoView() } +``` + +In `AppMode.BOTH` with auto-call at 1–10 s, the shared scroll is yanked away +from the player card on every draw — the player physically cannot keep their +board on screen. Suggest gating on mode: + +```kotlin +LaunchedEffect(tickKey) { + if (tickKey > 0 && lastCalled != null && settings.mode == AppMode.MASTER) { + heroRequester.bringIntoView() + } +} +``` + +Confirm against the web behaviour before changing (see Unresolved Q1). + +### M3 — Toast copy and cell accessibility text are hardcoded Vietnamese in non-UI layers + +`android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt:282` — `showToast("Chờ $waitNum")` +`android/app/src/main/java/com/miti99/loto/ui/player/PlayerCell.kt:563-569` — `"Số $num", ", đã đánh dấu", ", đang chờ"` + +`res/values/strings.xml` already ships `toast_waiting` (`Chờ %1$d`), +`toast_dismiss`, `player_board_label`, `master_board_label` and `kinh_close` +— **all five are unreferenced** (verified by grep). The ViewModel emits +presentation copy instead, which also makes the string un-translatable and +couples the VM to display text. + +Fix: put a `waitingNumber: Int?` in `PlayerUiState` and format with +`stringResource(R.string.toast_waiting, n)` at the call site; move the cell +`stateDescription` fragments into `strings.xml`. Delete whichever resources +stay genuinely unused (lint `UnusedResources` will otherwise keep flagging +them; `isShrinkResources` hides it only in release). + +### M4 — Cold-start ordering can leave `settings.mode` at its default during the first master emission + +`android/app/src/main/java/com/miti99/loto/LotoApplication.kt:58-61, 76-83` +`android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt:103-135` + +`settingsState` is `stateIn(..., SharingStarted.Eagerly, defaults)` — its +initial value is `mode = PLAYER` until the settings DataStore read lands. +`onMasterState` reads `settings.value.mode` synchronously. Two independent +DataStore reads (settings vs. game state) race, so on a cold start with a +persisted `mode = BOTH`, the first master emission can be evaluated as +`PLAYER`: `PlayerAutoCross` then advances `lastHandledIndex` to +`called.size` with `changed = false` and no replay ever happens for that +history (`masterStore.state` will not re-emit until the next draw). + +In practice the persisted `crossed` matrix already reflects the earlier +auto-crosses, so this is usually invisible — but it is load-bearing on the +"restore replays idempotently" claim in the code comment +(`PlayerBoardViewModel.kt:65-67`), which does not hold under this ordering. + +Fix: make the ordering explicit rather than accidental — e.g. `settings.first +{ it !== defaults }`-style gating, a `Settings?` (null = not loaded) flow, or +`combine(masterStore.state, settings)` so a mode arrival re-triggers the +replay pass. + +Also: `LotoApplication.onCreate` serialises two unrelated jobs in one +coroutine (`masterStore.restore()` then `settingsState.collect`). Split them +into two `launch` blocks so a slow/failed restore cannot stall voice-id sync. + +### M5 — Card generation runs on the main thread with a 200-attempt rejection loop + +`android/app/src/main/java/com/miti99/loto/game/CardGenerator.kt:147-155` +called synchronously from `PlayerBoardViewModel.generate()` (`:140`). + +`pickFilledCols` can run `pickFilledColsOnce` up to 201 times; each pass +enumerates up to `C(9,5) = 126` combinations per row across 9 rows, with a +recursive `combinations()` that allocates a fresh `List` per node. Typical +runtime is fine, but the worst case is an unbounded-ish main-thread stall on +a low-end device. The column-quota invariant is correct (verified: `forced.size +<= 5` and `candidates.size > need` hold for every `rowsLeft`), so this is a +latency risk, not a correctness one. + +Fix: `viewModelScope.launch(Dispatchers.Default) { val g = CardGenerator.generateGrid(); withContext(Dispatchers.Main) { ... } }`, +or memoise `combinations()` per `(candidates.size, need)` shape. + +### M6 — Master panel recomposes ~200 non-lazy nodes on every draw + +`android/app/src/main/java/com/miti99/loto/ui/master/MasterPanelScreen.kt:187-209, 270-325` + +The called-history `FlowRow` (up to 90 tokens) plus the 11×9 `MasterBoard` +(99 cells, each a Box + Box + up to 2 Texts) are plain `Column`/`Row` +inside a `verticalScroll` — everything is composed and measured. Each draw +mutates `master` and recomposes the whole subtree. Combined with the +per-frame `withFrameNanos` loop in `AutoCountdown.kt:143-153`, auto-call at +1 s/number keeps the main thread busy. + +Not a blocker for a fairground app on modern hardware, but worth measuring +on the API 24 target listed in `plans/todo.md`. Cheap mitigations: hoist +`MasterBoardLayout.BOARD` rows into `key(num)` blocks, or pass `isCalled` / +`isLast` as lambdas so the token colour changes without recomposing the cell. + +--- + +## Low + +### L1 — `ExoPlayer` is never released + +`LotoApplication.kt:72-74`, `VoicePlayer.kt:98-100`. `VoicePlayerApi.release()` +has no call site anywhere in `main/`. The player is app-scoped over an +Application context, so no Activity leak — but audio renderers/codecs stay +resident for the process lifetime. Either wire it to a +`ProcessLifecycleOwner` `ON_STOP`+timeout, or drop the unused `release()` from +the interface so it does not read as a contract that is being honoured. + +### L2 — Shared voice player + per-ViewModel `onCleared` cancel + +`MasterPanelViewModel.kt:105-107`, `PlayerBoardViewModel.kt:238-240`. Both +VMs `cancel()` the *same* singleton on clear, and neither calls +`super.onCleared()`. Harmless today (both clear together), fragile if +scoping ever changes. + +### L3 — Restore payload is range-validated but not shape-validated + +`GameStateRepository.kt:107-113`. `parseNumberList` accepts duplicates and +does not check `called ∪ remaining == 1..90` or disjointness. A +partially-written/tampered `loto_game_state` file can restore a deck with a +repeated number, which `PlayerAutoCross` will then cross twice on different +cells. Local-only file, low impact; a `toSet().size == size` check plus a +disjointness assert would close it cheaply. + +### L4 — `write()` swallows every `IOException` with no signal + +`SettingsRepository.kt:107-113`, `GameStateRepository.kt:85-90`. Documented as +intentional ("web parity"), and correct for this product, but a persistently +failing store now degrades to "settings silently never save" with zero +telemetry. Consider at least a `Log.w` so device QA can spot it. + +### L5 — `MasterPanelViewModel` leaves `autoRunning = true` after a manual draw exhausts the deck + +`MasterPanelViewModel.kt:65-72, 85-91`. `_autoRunning` only flips false at the +*next* tick. Between exhaustion and that tick the UI shows "Dừng" while +`MasterPanelScreen.kt:88` has already hidden the button (guarded on +`remaining.isNotEmpty()`), so it is invisible — but `toggleAuto()` can no +longer reset it either. Add the `remaining.isEmpty()` check inside +`drawNext()`. + +### L6 — `manualUnticks` survives a master "Ván mới" outside `BOTH` mode + +`PlayerBoardViewModel.kt:107-119`. The reset branch is gated on +`mode == AppMode.BOTH`, so unticks recorded in a previous round persist. A +later switch to BOTH + "Xoá đánh dấu" replays with stale suppressions. Narrow +edge; clear `manualUnticks` on any shrink regardless of mode. + +### L7 — `Color.parseColor` is deprecated + +`ColorParsing.kt:9-13`. Works on all supported API levels; `androidx.core` +ships `String.toColorInt()` which is the non-deprecated equivalent and keeps +the same `IllegalArgumentException` contract. + +### L8 — `launchMode="singleTask"` on the LAUNCHER activity + +`AndroidManifest.xml:44`. No deep links or external `startActivity` targets +exist, so nothing needs it, and `singleTask` changes task-reparenting and +`onNewIntent` semantics. `standard` is the safer default unless the wrapper +era needed it for a reason worth recording. + +### L9 — `allowBackup="true"` with no backup rules + +`AndroidManifest.xml:36`. Both DataStores (settings + round state) are +included in cloud backup / D2D transfer by default. Nothing sensitive is +stored, so this is a data-hygiene note rather than a security finding — but +if a restored `loto_game_state` from another device is undesirable, exclude +it via `android:dataExtractionRules`. + +### L10 — CI leaves the decoded keystore in the workspace + +`.github/workflows/android-release.yml:66-70`. `keystore.p12` is written to +`$GITHUB_WORKSPACE` and never removed. The runner is ephemeral and the +release upload globs only `*.apk`/`*.aab`, so there is no actual exposure — +but an `if: always()` cleanup step is one line and removes the class of +mistake where a future step archives the workspace. + +### L11 — `resourceConfigurations` is deprecated in AGP 8.13 + +`app/build.gradle.kts:22`. Replaced by `androidResources.localeFilters`. +Functional today; will warn/break on an AGP major bump. + +--- + +## Edge cases checked and found sound + +Recorded so a future audit does not re-litigate them: + +- `CardGenerator` column-quota invariant holds for every `rowsLeft` + (`forced.size <= 5`, `candidates.size > need`), so + `picked.removeFirstOrNull() ?: 0` never silently drops a number. +- `PlayerAutoCross` cursor advances strictly in non-`BOTH` modes, so a + `player → both` toggle does not dump back-history (`PlayerAutoCross.kt:40-43`). +- `MasterStore.publishAndSave` launches saves on `Dispatchers.Main.immediate` + and each coroutine reaches `DataStore.edit` in dispatch order, so snapshots + cannot be persisted out of order. +- `parseCrossed` returns null for a 0-length string, so an empty `crossed` + matrix falls back to all-false rather than corrupting the grid. +- `AutoCountdown` deliberately uses `withFrameNanos` instead of a Compose + animation so the digit still ticks under "remove animations" + (`AutoCountdown.kt:139-153`) — correct, and the comment explains why. +- `KeepScreenOn` releases the flag in `onDispose` and is keyed on `remaining`, + matching the `plans/todo.md` QA items. +- Manifest surface is minimal: `VIBRATE` only, `ACCESS_NETWORK_STATE` + stripped, one exported activity with a LAUNCHER filter. + +## Test-quality note + +`InMemoryDataStore` (`app/src/test/java/com/miti99/loto/InMemoryDataStore.kt`) +is a `MutableStateFlow`, so `dataStore.data.first()` resolves *synchronously* +in unit tests. That is why H1 and M4 — both of which are purely about the +async gap between construction and the first DataStore emission — are +invisible to the existing suite despite `PlayerBoardViewModelTest` and +`GameStateRepositoryTest` looking thorough. The Windows rename limitation +that motivated the fake is real; the fix is a delay-injecting fake, not a +file-backed one: + +```kotlin +class SlowDataStore(private val gate: CompletableDeferred) : DataStore { + override val data = flow { gate.await(); emitAll(state) } + ... +} +``` + +Then assert that `generate()` before `gate.complete(Unit)` wins. + +--- + +## Recommended actions (priority order) + +1. Guard `PlayerBoardViewModel.restore()` and `MasterStore.restore()` against + a concurrent user action, or disable the action buttons until restore + settles (H1). +2. Re-host `ConfettiOverlay` in a `Popup`/dialog window so tier-2 + celebrations are actually visible and stop injecting scroll layout (H2). +3. `roundToInt()` in `EmptyCellColorPicker` (M1) — one-line, prevents + user-visible colour drift. +4. Decide and document the `bringIntoView` policy for `BOTH` mode (M2). +5. Move the toast/`stateDescription` copy into `strings.xml` and delete or + wire the five orphaned resources (M3). +6. Make the settings-vs-master-restore ordering explicit and split + `LotoApplication.onCreate` into two launches (M4). +7. Move `generateGrid()` off the main thread (M5). +8. Add a delay-injecting DataStore fake and regression tests for 1 and 6. +9. Sweep the Low list opportunistically; L1/L5/L6 are each a few lines. + +## Metrics + +- Type coverage: 100% Kotlin, no `Any`/unchecked casts outside the one + documented `@Suppress("UNCHECKED_CAST")` in `LotoViewModelFactory.kt:13` + (standard `ViewModelProvider.Factory` idiom). +- Unit tests: 13 files / ~1.5k LOC over `game`, `audio`, `settings`, `state`, + `ui`. Instrumented: 4 files. Pure game logic is genuinely covered (real + assertions on invariants, not smoke calls). Gap is the async-boundary + behaviour described above. +- Lint/typecheck/build: **not run** — no Android SDK configured in this + review environment (`local.properties` was out of scope by instruction). + Findings are static-analysis + code-reading only. + +## Unresolved questions + +1. **M2** — does `web/src/lib/MasterPanel.svelte` scroll the hero into view + in `both` mode, or only in master-only mode? The web app is the stated + behavioural spec, so the Kotlin side should match whatever it does. I did + not read `web/` (out of the delegated file scope). +2. **H1** — is the pre-restore no-confirm branch intentional (an empty board + genuinely has nothing to confirm), or was the `grid == null` check meant + to be a "not loaded yet" check? The fix differs: guard the restore vs. + gate the button. +3. **L8** — was `launchMode="singleTask"` carried over deliberately from the + Capacitor wrapper manifest, or copied without a reason? +4. **M6** — has the master panel been profiled on the API 24 device named in + `plans/todo.md`, or is that QA item still open? The `todo.md` list shows + all device-QA boxes unchecked. diff --git a/plans/reports/code-reviewer-260831-2213-web-review.md b/plans/reports/code-reviewer-260831-2213-web-review.md new file mode 100644 index 0000000..182fa47 --- /dev/null +++ b/plans/reports/code-reviewer-260831-2213-web-review.md @@ -0,0 +1,387 @@ +# Code Review — `web/` (SvelteKit lô tô app) + +Date: 2026-08-31 · Reviewer: code-reviewer · Mode: report-only (no source edits) + +## Scope + +- Files: all of `web/src` (23 files, ~4.7k LOC incl. tests), `package.json`, + `svelte.config.js`, `vite.config.js`, `firebase.json`, `eslint.config.mjs`, + `jsconfig.json`, `static/manifest.webmanifest`, `scripts/generate-audio.py`, + `.github/workflows/ci.yml`, `README.md`, `docs/`. +- Verification run locally: `npm ci`, `npx vitest run` (121 pass / 8 files), + `npx eslint .` (0 problems), `BUILD_PROFILE=gh npx vite build` + inspection of + `build/sw.js`, `build/index.html`, `.svelte-kit/output/prerendered/pages/index.html`. + +## Overall Assessment + +Game logic is solid and matches `docs/project-overview-pdr.md` (5-per-row and +5-per-column invariants, ascending column order, unbiased Fisher–Yates, soft +"no 3 consecutive" via rejection sampling). Storage parsing has size caps and +per-key validators; there are no XSS sinks (`@html`/`innerHTML` absent), no +client-side Firebase SDK, no secrets, and CI uses `pull_request` (not +`pull_request_target`) with a SHA-pinned deploy action. + +The real risk sits at the edges the unit tests don't reach: the PWA layer is +built but never wired up, and the multi-tab coordinator can roll back a live +round. Component/effect code (PlayerBoard, MasterPanel) has zero test coverage +and carries several timing/UX defects. + +--- + +## Critical + +### C1. Service worker is generated but never registered — offline mode does not exist +`web/vite.config.js:39-81` (SvelteKitPWA config), `web/src/routes/+layout.svelte:19-23` + +Empirically verified on a real build (`BUILD_PROFILE=gh npx vite build`): + +- `build/sw.js`, `build/workbox-*.js`, `build/registerSW.js` are emitted. +- `build/index.html` contains **zero** occurrences of `registerSW`, `sw.js`, or + `serviceWorker` (`grep -oi 'sw\.js\|registerSW\|serviceWorker' build/index.html` + → empty). Same for `.svelte-kit/output/prerendered/pages/index.html`. +- The only `serviceWorker` reference in `_app/immutable/**` is SvelteKit's own + `updated.check()` helper, not vite-pwa's registration. + +Cause: with `adapter-static`, page HTML is produced during `adapt()`, which runs +after the PWA plugin's `closeBundle`; `@vite-pwa/sveltekit` therefore never +injects the tag, and nothing in `src/` imports `virtual:pwa-register`. + +Impact: no precache, no runtime audio cache, no offline start. `README.md:27` +("works offline after first load"), `docs/development-roadmap.md:21` ("Works +offline at fairground (no signal venues)") and `docs/system-architecture.md:173-178` +are all false for both the GitHub Pages and Firebase deployments. This is the +app's stated core venue requirement and it fails silently — no test or CI step +catches it. (Android is unaffected: the APK mounts the assets locally.) + +Fix (in `+layout.svelte`, alongside the existing `onMount`): + +```js +import { base } from "$app/paths"; +onMount(() => { + if (import.meta.env.PROD && "serviceWorker" in navigator) { + navigator.serviceWorker.register(`${base}/sw.js`, { scope: `${base}/` }); + } +}); +``` + +or use the plugin's own entry point (`const { registerSW } = await import("virtual:pwa-register"); registerSW({ immediate: true })`). +Then re-verify with the greps above, and add a build-time assertion (a tiny node +script in CI: fail if `build/index.html` lacks a SW registration) so this cannot +silently regress again. + +--- + +## High + +### H1. Audio precache URLs ignore the SvelteKit base path — SW install will 404 on GitHub Pages +`web/vite.config.js:21-26`, base resolution at `web/svelte.config.js:17-25` + +`additionalManifestEntries` are passed verbatim to workbox-build; the plugin only +rewrites entries it generates from globs. Verified in the `gh` build: + +``` +$ grep -o '"[^"]*audio[^"]*mp3"' build/sw.js | head -1 +"/audio/hoai-my/1.mp3" # absolute, origin-rooted +$ grep -oE '"[^"]*\.(js|css)"' build/sw.js | head -2 +"registerSW.js" "_app/immutable/nodes/2.QP53jZoe.js" # relative → correct +$ grep -c '/loto/' build/sw.js +0 +``` + +On `tiennm99.github.io/loto/` those 92 entries resolve to +`https://tiennm99.github.io/audio/hoai-my/N.mp3` → 404. Workbox rejects the +`install` event when any precache request fails, so once C1 is fixed the SW will +install-fail on every load of the Pages deployment and never activate — offline +stays broken there while appearing fixed on Firebase (base `""`). + +Fix: share one base resolver between the two configs and prefix the entries. + +```js +// web/base-path.js (imported by svelte.config.js and vite.config.js) +export function resolveBase() { /* moved verbatim from svelte.config.js */ } +``` +```js +const base = resolveBase(); +const defaultVoicePrecacheEntries = clipNames.map((n) => ({ + url: `${base}/audio/${defaultVoiceId}/${n}.mp3`, + revision: `audio-v1-${defaultVoiceId}-${n}`, +})); +``` + +Verify: `BUILD_PROFILE=gh npm run build:gh && grep -o '"[^"]*audio[^"]*mp3"' build/sw.js | head -1` +must print `"/loto/audio/hoai-my/1.mp3"`. + +### H2. Reclaiming a frozen tab overwrites the live round with stale state (data loss) +`web/src/lib/active-tab.svelte.js:58-61`, `web/src/lib/MasterPanel.svelte:69-74`, +`web/src/routes/+layout.svelte:26-44` + +Sequence: tab A hosts a round → user opens tab B → B broadcasts `claim` → A sets +`activeTab.inactive = true` → the layout `{#if}` unmounts all children of A, but +the `masterState` module singleton in A keeps its snapshot. B draws 20 more +numbers and persists them. User returns to A and taps "Nhấn để tiếp tục tại đây" +→ `claimActiveTab()` only flips the flag → children remount → `MasterPanel`'s +save effect (`MasterPanel.svelte:69-74`) runs unconditionally on mount and calls +`saveMaster()` with A's stale `called`/`remaining` → `loto_master` in +localStorage is rolled back and B's 20 draws are gone. + +There is no re-hydration on unfreeze and no `storage`-event listener anywhere. + +Fix: re-hydrate before handing control back. + +```js +// active-tab.svelte.js — keep the store dumb, or do it in +layout.svelte +export function claimActiveTab() { + loadSettings(); + loadMaster(); // re-read authoritative state written by the peer + activeTab.inactive = false; + if (bc) bc.postMessage({ type: "claim", id: TAB_ID }); +} +``` + +Defence in depth: gate the `saveMaster()` effect on a "hydrated" flag so a mount +never writes before a load has happened in this tab's current active session. + +--- + +## Medium + +### M1. Second "Chờ" toast within 5 s is invisible but still swallows taps +`web/src/lib/PlayerBoard.svelte:117-123` and `:493-512`, `web/src/app.css:194-207` + +`showToast()` sets `toast = null` then `toast = msg` in the same synchronous +tick, so the `{#if toast}` block never tears down — the DOM node is reused and +the `animate-toast` 5 s animation is **not** restarted. Consequences when a +second row enters "Chờ" 3 s after the first: + +1. The chip shows the new text but keeps the old animation's progress; with + `forwards` fill it reaches `opacity: 0` at t=5 s while the JS timer + (`:120`) keeps the node mounted until t=8 s. +2. That node is `pointer-events-auto` and sits centred over the card, so for + ~3 s an invisible ~150×60 px button eats taps on the middle cells — the user + taps a number and nothing happens. + +Fix: force a remount and let one owner control the lifetime. + +```svelte +{#key toastId} +
…
+{/key} +``` +with `let toastId = $state(0)` incremented in `showToast`, or drop the CSS +`forwards` fade and remove the node from JS only. + +### M2. "Kinh!" is cut off by "Chờ" in the same effect pass +`web/src/lib/PlayerBoard.svelte:191` (pass 1 `playBingo()`), `:203` (pass 2 +`playWaiting()`), `web/src/lib/voice.js:107,123` (every entry point starts with +`cancelPlayback()`) + +A single draw that completes one row and puts another row one number away runs +both passes in one effect run: `playBingo()` starts, then `playWaiting()` +cancels it a fraction of a millisecond later. The win announcement — the whole +point of the feature — is never heard in exactly the most exciting case. + +Fix: give the two events a priority or a queue. Cheapest correct version: in +pass 2, skip `playWaiting()` when pass 1 fired this run +(`let announcedBingo = false` local flag), or add a small FIFO in `voice.js` +(`enqueue(clip)`) instead of unconditional `cancelPlayback()`. + +### M3. Theme flash on every cold load +`web/src/routes/+layout.svelte:19-23`, `web/src/app.html` (no inline script) + +`loadSettings()` runs in `onMount`, so the prerendered shell always paints with +`:root` light tokens (`app.css:14-25`) before `documentElement.classList.toggle("dark")` +is applied. Dark-theme users get a full-page white flash on every launch, +including the Android WebView. + +Fix: add a blocking inline script in `app.html` head that reads +`localStorage.getItem("loto_settings")`, applies the `dark` class and +`--empty-cell-bg`, before the first paint. Keep `loadSettings()` as the +authoritative pass. + +### M4. Both modal dialogs have no focus management +`web/src/lib/PlayerBoard.svelte:566-618` (bingo), `web/src/lib/SettingsButton.svelte:175-190` (settings) + +Both use `role="dialog" aria-modal="true"` but never move focus into the dialog, +never trap Tab, never restore focus on close, and leave background content +tabbable and scrollable. Screen-reader users get no announcement (focus stays on +the trigger, outside the dialog), and keyboard users tab straight into the +board behind the overlay. `aria-modal` claims a containment that does not exist. + +Fix: on open, focus the dialog container (`tabindex="-1"` + `element.focus()`); +cycle Tab between the first/last focusable descendants; restore +`document.activeElement` on close; add `overflow: hidden` on `` while open +(or `inert` on the page wrapper). Escape handling already exists. + +### M5. Two tabs opened at the same time freeze each other +`web/src/lib/active-tab.svelte.js:35-51` + +Both tabs `postMessage({type:"claim"})` on mount; each receives the other's claim +and sets `inactive = true`. Result: both windows show the "đang chạy ở tab khác" +banner and the app is unusable until the user manually clicks one. Not +hypothetical — restoring a browser session with two pinned tabs does exactly this. + +Fix: include a monotonic claim timestamp and ignore a peer claim that is older +than this tab's own claim (`if (e.data.ts < myClaimTs) return;`), tie-break on id. + +### M6. Auto-call voice is silent on iOS Safari (and 92 `Audio` objects are retained) +`web/src/lib/voice.js:27-35`, driven from `web/src/lib/MasterPanel.svelte:97-103` + +`getAudio()` constructs a **new** `Audio` per clip URL, lazily, at play time. +Under auto-call, every `play()` happens inside a `setInterval` callback, i.e. +outside a user gesture, on an element that was never unlocked. iOS Safari +rejects it; `playClip` swallows the rejection via `a.play().catch(done)` +(`voice.js:93`) so the host gets silence with no diagnostic. Secondary cost: up +to 92 `preload="auto"` elements per voice held in the `cache` Map. + +Fix: use a single reusable `HTMLAudioElement` whose `src` is swapped per clip and +which is `play()`/`pause()`-unlocked during the first user gesture (the +"Bắt đầu"/"Xổ số" click). Same for `clearAudioCache`. Keep the token/cancel +bookkeeping as-is. + +### M7. Lint and type checking never run in CI; JSDoc types are decorative +`.github/workflows/ci.yml:42-50`, `web/jsconfig.json:1-6`, `web/package.json:11-16` + +The `test` job runs `npm test` only. `npm run lint` exists but is not wired into +any job (the Android job does run `:app:lint`). `jsconfig.json` sets +`"checkJs": false` and there is no `svelte-check` dependency or script, so none +of the extensive `@type`/`@param` annotations are ever verified — they can drift +arbitrarily and CI stays green. + +Fix: add `- run: npm run lint` to the `test` job, add +`svelte-check --tsconfig ./jsconfig.json` as a `check` script (with +`"checkJs": true`), and run it in the same job. Expect an initial batch of +findings; fix or explicitly ignore them once. + +### M8. Riskiest code has no tests +`web/src/lib/*.test.js` (8 files, all module-level) + +Coverage is good for pure/logic modules but there is not a single component or +effect test. Every defect above (C1, H2, M1, M2, M5) lives in component effects +or build config — exactly the untested surface. The passing 121 tests give a +false sense of safety. + +Fix: add `@testing-library/svelte`/`vitest-browser-svelte` cases for at least: +master draw → player auto-cross in `both` mode, "Ván mới" clearing player marks, +consecutive-toast behaviour, and the bingo+waiting audio ordering. + +### M9. Docs describe an update UX that does not exist +`web/docs/codebase-summary.md:64`, `web/docs/development-roadmap.md:21`, +`web/vite.config.js:40-42` + +Docs say "Auto-update w/ reload toast"; there is no reload-prompt component +anywhere in `src/`. `registerType: "autoUpdate"` makes vite-plugin-pwa emit +`skipWaiting`/`clientsClaim` and reload on controller change — the exact +mid-game swap the comment at `vite.config.js:40-41` says must not happen. +Once C1 is fixed, a deploy during a fairground round can reload the page and +drop in-memory state (`autoRunning`, `showCongrats`, countdown). + +Fix: pick one and make code, comment, and docs agree — either implement the +prompt (`registerType: "prompt"` + a small "Có bản mới, tải lại?" banner) or +document that auto-reload is accepted and delete the contradicting comment. + +--- + +## Low + +- **L1** `web/static/manifest.webmanifest:2` — `"id": "/loto/"` is hardcoded, but + the same file is deployed to the Firebase root (base `""`), where that path + does not exist. Use `"id": "."` to match `start_url`/`scope`, or template the + file per build profile. +- **L2** `web/src/lib/PlayerBoard.svelte:197-207` — when several rows enter "Chờ" + on the same draw, each iteration overwrites the previous toast and cancels the + previous clip; only the last row is ever announced. Aggregate + (`Chờ 12, 47`) or queue. +- **L3** `web/src/lib/settings-store.svelte.js:204-216` — `resetSettings()` can + change `settings.voice` without calling `clearAudioCache()` (unlike + `SettingsButton.svelte:116-120`), leaving the old voice's elements cached. +- **L4** `web/src/lib/game-logic.js:192-217` and + `web/src/lib/master-store.svelte.js:34-56` — validators check shape and range + but not game semantics: a grid may contain duplicates/values outside a + column's range, and `called`/`remaining` may overlap, contain duplicates, or + not sum to 90. Self-inflicted only (localStorage), but a duplicate in `called` + silently collapses in the `callOrder` Map (`MasterPanel.svelte:77-79`). Add + `called.length + remaining.length === 90` and a disjointness check; fall back + to defaults otherwise. +- **L5** `web/src/routes/+layout.svelte:27-41` — `