tiennm99/loto
1
0 Fork 0
mirror of https://github.com/tiennm99/loto.git synced 2026-10-15 20:16:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore(plans): add review reports and dual-platform fix cycle journal

Browse Source
This commit is contained in:
tiennm99 committed 2026-09-01 00:11:30 +07:00
1 parent 57ec6bfbcf
commit 0a16cef4ae
13 files changed
+2506
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

plans/260831-2314-review-followups/plan.md
+93
View File
@@ -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<Boolean>`); 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.
plans/journals/2026-08-31-dual-platform-review-fix-cycle-web-pwa-android-state-races.md
+31
View File
@@ -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.
plans/reports/code-reviewer-260831-2213-android-review.md
+395
View File
@@ -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<Unit>) : DataStore<Preferences> {
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.
plans/reports/code-reviewer-260831-2213-web-review.md
+387
View File
@@ -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}
<div role="status" aria-live="polite" …>…</div>
{/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 `<body>` 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` — `<button>` wrapping `<p>`
elements is an invalid content model (phrasing content only). Use a
`<div role="button" tabindex="0">` or move the text out and keep a labelled
button.
- **L6** `web/src/lib/AutoCountdown.svelte:41-48` — the rAF loop runs at display
refresh rate for the whole round even when `reduceMotion` is true and
`dashOffset` is pinned to 0 (`:63-65`); only the integer second changes. Gate
the loop on `!reduceMotion` and drive the digit from a 1 Hz timer.
- **L7** `web/vite.config.js:23-25` — `revision: "audio-v1-…"` is a manual
version string; regenerating clips without editing it serves stale audio from
precache forever. Derive it from a file hash or the manifest mtime.
- **L8** `web/firebase.json` — no `headers` block. Add explicit `Cache-Control`
(`no-cache` for `index.html`/`sw.js`, `immutable` for `_app/immutable/**`)
rather than relying on Hosting defaults, especially once the SW ships.
- **L9** `web/scripts/generate-audio.py:44-49` — `voice_id()` slugs from the last
hyphen segment only; two voices whose names slug identically would silently
overwrite each other's folder. Assert uniqueness before writing.
- **L10** `web/src/lib/PlayerBoard.svelte:176` — the modal says "Hàng {1..9}" but
the card is presented as three labelled 3-row sections
(`:374-379`); users must count across sections to find the row. Consider
"Hàng 1 — TN1 (2014-2017)".
---
## Edge Cases Checked and Found Sound
Recorded so they are not re-litigated:
- `pickFilledColsOnce` (`game-logic.js:80-121`) cannot produce `need < 0` (that
would recurse `combinations` with a negative `k`): more than 5 columns with
`quota === rowsLeft` would imply `sum(quota) > 5·rowsLeft`, contradicting the
invariant that each row removes exactly 5. Column quotas hold on the fallback
path too, so `picked.shift() ?? 0` (`:160`) never silently blanks a cell.
- The `crossed`/`lastHandledIndex` self-writing effect
(`PlayerBoard.svelte:245-258`) converges: `applyMasterCalls` short-circuits at
`lastHandledIndex >= called.length` (`player-auto-cross.js:33-35`).
- Store hydration order is safe either way: `MasterPanel` cannot mount before
`loadSettings()` (default `mode` is `"player"`), and `PlayerBoard`'s replay
from cursor 0 is idempotent via `findUncrossedCell`.
- Unticking a called number does not get re-crossed on the next effect run
(cursor already at `called.length`).
- Freezing an inactive tab unmounts the page, which clears the auto-call
interval, releases the wake lock, and cancels audio.
- Shuffle bias, column ranges, ascending-within-column, and 5/5 quotas match
`docs/project-overview-pdr.md:16-27` exactly.
## Recommended Actions (priority order)
1. Register the service worker and add a CI assertion that the built HTML
contains the registration (C1).
2. Prefix `additionalManifestEntries` with the resolved base path; verify
`build/sw.js` for the `gh` profile (H1).
3. Re-hydrate stores in `claimActiveTab()` and gate `saveMaster()` on hydration (H2).
4. Fix the toast remount/pointer-events defect (M1) and the bingo/waiting audio
collision (M2).
5. Add the inline pre-paint theme script (M3).
6. Add focus management to both dialogs (M4).
7. Wire `npm run lint` + `svelte-check` into CI and turn on `checkJs` (M7).
8. Resolve the double-claim freeze (M5) and iOS audio unlock (M6).
9. Add component/effect tests covering the paths above (M8); reconcile the
update-UX docs (M9).
## Metrics
- Unit tests: 121 passed / 8 files (`vitest run`), all module-level; 0 component tests.
- Lint: `eslint .` → 0 problems.
- Type coverage: not enforced — `checkJs: false`, no `svelte-check` in the repo.
- Build: `BUILD_PROFILE=gh vite build` succeeds; precache manifest 115 entries
(92 of them unreachable on that base path, see H1).
## Unresolved Questions
1. Was the PWA ever verified in a deployed browser (Application → Service Workers)?
If it worked previously, something in the `@vite-pwa/sveltekit@1` /
`@sveltejs/kit@2` / `vite@8` upgrade path dropped the injection, and the fix
should be pinned by a regression check rather than a one-off patch.
2. Multi-tab policy: is the intended contract "last claimant wins and adopts the
stored round" (implies re-hydrate, H2) or "each tab owns its own session"?
The fix differs.
3. `manifest.webmanifest` ships identically to two origins with different base
paths — is the GitHub Pages deployment still a supported target, or is
Firebase now canonical? That decides L1 and the H1 fix's urgency.
4. Update policy for mid-round deploys (M9): silent auto-reload, or prompt?
plans/reports/code-reviewer-260831-2314-precommit-review.md
+283
View File
@@ -0,0 +1,283 @@
# Pre-commit review — accumulated review-fix cycle (web + android + CI)
Reviewer: code-reviewer | Date: 2026-08-31 | Mode: static reading only (no builds/tests run)
Scope: full uncommitted working tree (42 modified + 14 new files), plan
`plans/260831-2314-review-followups/plan.md` and the six fix reports.
## Verdict
Plan acceptance criteria are substantially met, but three of them are met only
under an ordering assumption or only on the path that was tested. One **High**
regression unrelated to this plan (introduced by the earlier L1/L2 audio fix) is
a launch-blocker for the announcer.
| Criterion | Verdict |
|---|---|
| Multi-tab newest-claim-wins, exactly one winner | Partially met — symmetric cases correct; a losing claimer is never told it lost (M-1) |
| Game-state-only backup exclusion, settings kept | Met — path verified against DataStore's default location |
| Settings-loaded gate never advances the cursor early | Met for `onMasterState`; not for `generate()`/`clearMarks()` (M-3) |
| Gate cannot deadlock the ViewModel | Met for the `IOException` path; not for a non-IO failure (M-4) |
---
## High
### H-1 — `voicePlayer.release()` permanently kills audio after "Thoát" + relaunch in the same process
`android/app/src/main/java/com/miti99/loto/MainActivity.kt:58-59`
`android/app/src/main/java/com/miti99/loto/LotoApplication.kt:85-87`
`onDestroy()` releases the **app-scoped** ExoPlayer when `isFinishing`. The
"Thoát" dialog calls `finish()` (MainActivity.kt:104), but `finish()` does not
end the process — Android keeps it cached. On relaunch from the launcher/Recents
the same `LotoApplication` instance is reused, and `voicePlayer` is a `by lazy`
singleton that is never rebuilt, so `LotoViewModelFactory` hands both ViewModels
the already-released `ExoVoicePlayer` (`VoicePlayer.kt:98-100` → `player.release()`
is terminal, per the interface's own doc: "the instance is unusable afterwards").
Every subsequent `speak()` either throws or silently drops — the announcer is dead
until the OS reclaims the process. This is a new failure mode: before this change
the VMs only called `cancel()`.
Fix (pick one):
```kotlin
// LotoApplication.kt — make it recreatable instead of a one-shot lazy
private var _voicePlayer: VoicePlayerApi? = null
val voicePlayer: VoicePlayerApi
get() = _voicePlayer ?: ExoVoicePlayer(this)
.also { it.voiceId = VoiceCatalog.defaultVoiceId(voices); _voicePlayer = it }
fun releaseVoicePlayer() { _voicePlayer?.release(); _voicePlayer = null }
```
then call `releaseVoicePlayer()` from `MainActivity.onDestroy()`. Alternatively
drop `release()` entirely and call `voicePlayer.cancel()` there — the player is a
tiny resource and the process teardown reclaims it.
Note the release path is untested: no unit test covers `onDestroy`, and
`FakeVoicePlayer` has no "released" state to assert against.
---
## Medium
### M-1 — Newest-claim-wins can now leave **two** tabs active (a losing claimer is never corrected)
`web/src/lib/active-tab.svelte.js:57-66`
The protocol is one-way: a tab that receives a *losing* peer claim just ignores
it. The peer therefore never learns it lost and stays active locally
(`claimActiveTab()` sets `inactive = false` before broadcasting, line 83). The
old bug (both tabs frozen) has been traded for both tabs *active* — double
auto-call intervals, double `localStorage` writers, overlapping audio, i.e. the
exact thing the module exists to prevent.
Reachable paths:
1. The repo's own new test demonstrates it —
`web/src/lib/active-tab.test.js` "reclaim updates myClaimTs so a stale peer
claim can't re-freeze the reclaimed tab": after `tabB.claimActiveTab()` with a
stale ts, `tabA.inactive === false` **and** `tabB.inactive === false`. The test
only asserts A.
2. Wall-clock stepping backwards (NTP correction / user changing device time on a
fairground tablet) makes any new claim "older" than the incumbent's.
3. Equal-millisecond opens where delivery is asymmetric: a tab created *after*
the peer's `postMessage` never receives that claim (real `BroadcastChannel`
does not replay), so only one side evaluates the tie-break; if the surviving
evaluator holds the greater id it ignores the peer and both stay active.
4. Mixed-version tabs (now more likely, since `registerType: "prompt"` keeps old
tabs alive): a pre-fix tab broadcasts `{type, id}` with no `ts`, so
`peerTs > myClaimTs` and `peerTs === myClaimTs` are both `false` → the new tab
never freezes.
Fix — make the winner echo, and treat a missing `ts` as winning:
```js
bc.onmessage = (e) => {
if (e.data?.type !== "claim" || e.data.id === TAB_ID) return;
const peerTs = typeof e.data.ts === "number" ? e.data.ts : Infinity; // legacy peer wins
const peerWins =
peerTs > myClaimTs || (peerTs === myClaimTs && e.data.id > TAB_ID);
if (peerWins) {
activeTab.inactive = true;
} else if (!activeTab.inactive && bc) {
// Tell the loser it lost — same (ts,id), no refresh, so this terminates:
// the peer freezes and a frozen tab never echoes.
bc.postMessage({ type: "claim", id: TAB_ID, ts: myClaimTs });
}
};
```
Add a test for the asymmetric case (only one channel open at post time) asserting
exactly one `inactive === false`.
### M-2 — The settings-loaded gate relies on undocumented emission ordering between two independent collectors
`android/app/src/main/java/com/miti99/loto/LotoApplication.kt:58-74`
`android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt:126-127, 192`
`settingsState` and `settingsLoaded` are two separate `stateIn` collectors over
two separate `settingsRepository.settingsFlow` chains. `onMasterState` reads
`settings.value` directly (line 192), not the combine snapshot. Nothing guarantees
that the `settingsState` collector applies the loaded value *before* the
`settingsLoaded` collector flips to `true` — both are resumed as independent tasks
on `Dispatchers.Main.immediate`. If `loaded` wins the race, the gate opens while
`settings.value` still returns the PLAYER default and the M4 bug reappears exactly
as before, silently. It currently works only because `settingsState` happens to be
subscribed first (`onCreate` line 100, and factory arg order).
Fix — single source of truth, so "loaded" and "value" cannot disagree:
```kotlin
// LotoApplication
val settingsOrNull: StateFlow<Settings?> by lazy {
settingsRepository.settingsFlow.stateIn(appScope, SharingStarted.Eagerly, null)
}
```
and in `PlayerBoardViewModel` take `settings: StateFlow<Settings?>` (or keep both
but pass the settings snapshot *into* `onMasterState(master, settings)` from the
combine transform, and derive `loaded` as `snapshot != null`). Reading
`settings.value` inside a gated handler is the defect shape to remove.
### M-3 — The gate covers `onMasterState` only; `generate()` / `clearMarks()` still consume the master history under the placeholder mode
`android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt:224-237, 258-271`
`android/app/src/main/java/com/miti99/loto/ui/player/PlayerBoardScreen.kt:78`
Both methods read `settings.value.mode` unguarded and, in the non-BOTH branch, do
`lastHandledIndex = masterStore.state.value.called.size`. The generate button is
gated on `state.loading` (player restore only), not on settings having resolved.
So a tap in the pre-settings window — the most likely moment to tap, since the
empty board's only action is "Tạo bảng mới" — with a persisted `mode = BOTH` and a
restored master history advances the cursor to `called.size` with no crossing.
`onMasterState` later runs under the real BOTH mode but `cursor >= called.size`
returns `changed = false` (`PlayerAutoCross.kt:34-36`), so the replay is
permanently lost. This is the same class of bug M4 was filed for; the new test
`master restore landing with full history before settings resolves...` avoids it
only because it calls `generate()` *before* `masterStore.restore()`.
Fix: fold the settings gate into the UI loading flag, e.g. publish
`loading = !restoreDone || !settingsLoaded.value` (collect `settingsLoaded` in the
existing `init` combine and republish), so both buttons are disabled until the
mode is real. Add a test that restores the master history *before* `generate()`
while settings is still gated.
### M-4 — `settingsLoaded` can stick at `false` forever, permanently disabling auto-cross
`android/app/src/main/java/com/miti99/loto/settings/SettingsRepository.kt:76`
`loaded` is derived from `settingsFlow`, whose `catch` re-throws anything that is
not an `IOException` (line 52-60). A non-IO failure therefore cancels the
`settingsLoaded` collector (`SupervisorJob` keeps the app alive), leaving
`loaded == false` for the process lifetime — `onMasterState` is then never invoked
and the player board silently stops following the master. The plan explicitly
required the gate not to deadlock the ViewModel on a settings-load failure.
Fix (one line, fail-open):
```kotlin
val loaded: Flow<Boolean> =
settingsFlow.map { true }.onStart { emit(false) }.catch { emit(true) }
```
Fail-open is correct here: a broken settings store should degrade to defaults, not
to a dead board. Add a `ThrowingDataStore`-with-`RuntimeException` test.
---
## Low
- **L-a `generate()` is now asynchronous** (`PlayerBoardViewModel.kt:210-216`):
two fast taps enqueue two generations (the confirm dialog is skipped while
`grid` is still `null`), and the H1 guard `saved != null && grid == null`
(line 137) is weaker than its comment claims, because `grid` is only assigned at
the end of `applyGeneratedGrid`. Consider a `generating` flag that both
short-circuits a second `generate()` and participates in the restore guard.
- **L-b iOS unlock is master-only** (`web/src/lib/voice.js:56-72`,
`MasterPanel.svelte:156, 170`): `unlockAudio()` is called only from the master's
draw/auto buttons. In player-only mode with `voiceEnabledPlayer` on, the shared
element is never primed inside a gesture, so Chờ/Kinh may stay silent on iOS.
Call `unlockAudio()` from the player cell-tap handler too. Also: the unlock's
`play().then(() => a.pause())` would cut a live clip if `unlockAudio()` were ever
called while something is playing — safe today only because the `src` swap in
`playClip` happens synchronously in the same click task.
- **L-c web/android validation parity**: Android now rejects a restored round that
is not a complete 90-number partition (`GameStateRepository.kt:70-73`), while
`web/src/lib/master-store.svelte.js:56-62` still accepts any two in-range arrays
(duplicates, overlap, short deck). Either port the check or record the
divergence — a shared localStorage origin can produce a double-crossed cell.
- **L-d dead overrides**: `PlayerBoardViewModel.onCleared()` (line 316-323) and
`MasterPanelViewModel.onCleared()` now only call `super`. Delete the overrides and
move the rationale to the class doc; an empty override invites someone to
"fix" it later.
- **L-e loading flags never time out**: `MasterStore.loading` /
`PlayerUiState.loading` gate the only two primary buttons and only clear when the
DataStore read resolves. An `IOException` resolves (handled), but a hung read
leaves the app unusable with no escape hatch. Optional: `withTimeoutOrNull` around
the restore read.
- **L-f brittle CI gate**: `web/scripts/verify-pwa-build.mjs:52-56` requires one
built chunk to contain both `"serviceWorker"` and `"sw.js"`. A future Rollup
chunking change can split them and fail CI with no real regression. Prefer
asserting on the presence of the `virtual:pwa-register` output chunk plus a
`sw.js` reference anywhere under `_app`.
- **L-g backup exclusion misses the DataStore temp sibling**:
`android/app/src/main/res/xml/{data_extraction_rules,backup_rules}.xml` exclude
`datastore/loto_game_state.preferences_pb` only. DataStore writes through
`<name>.preferences_pb.tmp`; a backup taken mid-write can capture it. Add the
`.tmp` path (or exclude by directory if you ever move the round store to its own
subdirectory).
---
## Verified as intentional / no action
- `android:launchMode="singleTask"` removal is finding L8 from
`plans/reports/code-reviewer-260831-2213-android-review.md:276` — deliberate, and
the launcher intent already brings the existing task to front, so no behavioral
regression.
- Backup path is correct: `preferencesDataStore(name = "loto_game_state")`
(`LotoApplication.kt:33-36`) → `files/datastore/loto_game_state.preferences_pb`;
`loto_settings` is not excluded, as the decision requires. `allowBackup="true"`
unchanged.
- Storage keys/file names unchanged on both platforms (`loto_settings`,
`loto_master`, `loto_game_state`); `masterState.hydrated` is *not* serialized
(`master-store.svelte.js:72-84`), so no stored-shape change.
- Removed strings (`player_board_label`, `toast_dismiss`, `kinh_close`,
`master_board_label`) have no remaining references.
- H1 loading-gate behavior survives the M4 gate: `restore()` still publishes
`loading = false` on both branches (`PlayerBoardViewModel.kt:153`), the early-return
branch of `publish()` now carries `loading` through (line 404), and `prevCalledLen`
stays `0` while the gate is closed, so no false "Ván mới" shrink is detected when
it opens.
- Web reclaim/hydration flow is coherent: `reclaimTab()` re-reads storage *before*
flipping `inactive` (`+layout.svelte:64-68`), children are destroyed while frozen
so `PlayerBoard` re-initializes from storage, and `MasterPanel`'s save effect is
gated on `masterState.hydrated`.
- Update banner vs other-tab overlay cannot collide: they live in mutually
exclusive branches of `{#if activeTab.inactive}`; a prompt raised while frozen
simply appears after reclaim.
- `PlayerAutoCross.applyMasterCalls` already resets a stale cursor
(`PlayerAutoCross.kt:33`), so the L6 change (clearing `manualUnticks` on a shrink
in every mode without resetting `lastHandledIndex` outside BOTH) does not strand
the cursor.
- New tests are behavioral, not phantom: `SlowDataStore`/`ThrowingDataStore` drive
real suspension/failure paths, and the M4-residual and H1 tests would fail without
their respective fixes.
- CI additions (`npm run lint`, `npm run check`, `verify:pwa` with `BUILD_PROFILE`)
and the keystore cleanup step (`if: always()`) are contract-safe additions.
## Unresolved questions
1. H-1 — is exiting via "Thoát" and immediately reopening in-scope for QA? If the
answer is "the process always dies", H-1 drops to Low; nothing in the code
guarantees that.
2. M-1 — is "two active tabs" acceptable in any scenario, or should a losing
claimer always freeze itself? The echo fix assumes the latter.
3. Is `unlockAudio()`'s src-less priming `play()` actually sufficient to unlock iOS
Safari? It has not been verified on a device, and the comment asserts it as
fact. A silent one-sample data-URI source would be the safer primer.
4. Scope: the diff carries a large amount of work beyond this plan's three items
(PWA prompt + registration, voice single-element rewrite, `checkJs`, focus trap,
pre-paint theme script). Confirm this is intended to land as one commit, or split
the PWA/audio work out — it has a different blast radius than the three plan items
and (H-1, L-b) is where the residual risk sits.
plans/reports/fullstack-developer-260831-2226-android-fixes.md
+200
View File
@@ -0,0 +1,200 @@
# Android review fixes (2026-08-31)
Source review: `plans/reports/code-reviewer-260831-2213-android-review.md`.
Scope: `android/` only (per delegation; `web/` read-only for M2 parity check;
`.github/workflows/` and `android/local.properties` untouched).
## Fixed
### H1 — restore/user-action race
- `state/PlayerBoardViewModel.kt`: `restore()` now only applies the saved
round `if (saved != null && grid == null)` — a `generate()` that lands
while the DataStore read is still in flight can no longer be clobbered.
Added `loading: Boolean` to `PlayerUiState` (`true` until restore
resolves); `PlayerBoardScreen.kt`'s "Tạo bảng mới" button is now
`enabled = !state.loading`, closing the no-confirm branch at its root
(previously `grid == null` meant both "no card" and "not loaded yet").
- `state/MasterStore.kt`: added `loading: StateFlow<Boolean>` and a
`mutatedBeforeRestore` guard set by `startNewGame()`/`drawNext()`;
`restore()` skips applying the saved snapshot once either has run.
`state/MasterPanelViewModel.kt` exposes `loading`;
`ui/master/MasterPanelScreen.kt`'s "Ván mới" button is now
`enabled = !loading`, same shape as the player fix.
- Regression tests: added `app/src/test/java/com/miti99/loto/SlowDataStore.kt`
(delays a wrapped `DataStore`'s first `.data` emission until a
`CompletableDeferred` gate completes — `InMemoryDataStore` alone resolves
synchronously so the race was invisible to the existing suite, per the
report's test-quality note). Added
`restore does not clobber a card generated while the load is still in
flight` (PlayerBoardViewModelTest) and
`restore does not clobber a new game started while the load is still in
flight` (new `MasterStoreTest.kt`) — both fail without the fix, pass with it.
### H2 — tier-2 confetti invisible
- `ui/player/KinhDialog.kt`: `Dialog` now uses
`DialogProperties(usePlatformDefaultWidth = false)` so the dialog window
spans the full screen, and `ConfettiOverlay()` is composed *inside* that
window (drawn after/above the card `Column` in a `Box`), gated by a new
`showConfetti: Boolean` param. `PlayerBoardScreen.kt` no longer emits
`ConfettiOverlay()` as a scroll-column sibling. This is option (b) from
the report (embed in the dialog's own window) rather than a separate
`Popup`, since it guarantees same-window z-order over the scrim without
relying on cross-window layering behavior.
### M1 — RGB truncation
- `ui/settings/EmptyCellColorPicker.kt`: `r`/`g`/`b` now use `roundToInt()`
instead of `toInt()`.
### M2 — force-scroll to hero in `both` mode
- Checked `web/src/lib/MasterPanel.svelte:86-158` (read-only): `handleDrawNext()`
sets `scrollOnNextDraw = true` unconditionally, called by both the manual
draw button and the auto-call `setInterval`, with no `settings.mode` gate.
`MasterPanel.svelte` is rendered the same way in `master` and `both` mode
(`+page.svelte:37`). **Web already force-scrolls on every draw in `both`
mode** — Android's `LaunchedEffect(tickKey) { ... heroRequester.bringIntoView() }`
in `MasterPanelScreen.kt` already matches. Left as-is; added a comment
recording the parity check so this isn't re-litigated as a bug.
### M3 — hardcoded toast copy / unused strings
- `PlayerUiState.toast: String?` → `waitingNumber: Int?`. The ViewModel now
stores the raw number; `PlayerBoardScreen.kt` formats it with
`stringResource(R.string.toast_waiting, waitNum)`.
- Deleted 4 of the 5 unreferenced resources from `strings.xml`:
`toast_dismiss`, `player_board_label`, `master_board_label`, `kinh_close`.
`toast_waiting` is now wired (not deleted).
- Did **not** move `PlayerCell.kt`'s `stateDescription` fragments into
`strings.xml` — the task scope named only the toast + the five unused
resources; that's a separate, additive accessibility cleanup the report
suggests but didn't require. Noted below as deferred.
### M4 — cold-start settings/master ordering
- `LotoApplication.kt`: split `onCreate`'s single coroutine into two
`appScope.launch` blocks (`masterStore.restore()` and
`settingsState.collect { ... }`) so a slow round-state restore can't stall
voice-id sync.
- `PlayerBoardViewModel.kt`: `init` now does
`combine(masterStore.state, settings) { master, _ -> master }.collect { onMasterState(it) }`
instead of a plain `masterStore.state.collect`, so a settings arrival
re-runs the replay against the current master snapshot.
- **Scope note (see Unresolved below)**: `PlayerAutoCross.applyMasterCalls`
advances the cursor to `called.size` on every non-`BOTH` pass, even a
no-op one. `combine()` closes the ordering where settings resolves to its
real value no later than `masterStore.restore()`'s first real emission
(verified by test). It does **not** close the reverse ordering — master's
restore landing with full history before settings ever resolves — because
by the time settings arrives, the cursor has already consumed that
history under the wrong mode. Fully closing that needs a "settings
loaded" signal threaded through `SettingsRepository`/`LotoApplication`,
which is a larger change than this narrow, "usually invisible" (report's
words) edge case warrants; documented as a deferred follow-up rather than
silently left unfixed.
### M5 — `generateGrid()` off the main thread
- `PlayerBoardViewModel.generate()`: the pure `CardGenerator.generateGrid()`
call now runs on an injectable `computeDispatcher` (defaults to
`Dispatchers.Default`), then `withContext(Dispatchers.Main.immediate)`
applies the result via a new private `applyGeneratedGrid()`. Only the
computation is offloaded; all `ViewModel` field mutations still happen on
Main, so there's no new cross-thread mutable-state hazard.
- Added the `computeDispatcher` constructor param (defaulted) specifically
so unit tests can pass the same `StandardTestDispatcher` used for Main —
otherwise `generate()` would race a real background thread against
`runCurrent()`/`advanceUntilIdle()`, making the suite flaky.
`LotoViewModelFactory.kt` needed no change (named-arg construction, new
param is defaulted).
### M6 — master-panel recomposition
- **Deferred, not applied.** The report frames this as "not a blocker...
worth measuring," conditional on API 24 device profiling that
`plans/todo.md` shows as still unchecked. The suggested mitigations
(`key(num)` blocks or lambda-based cell props) touch the hot
`MasterBoard`/cell-render path with no compose UI test harness in this
project to catch a visual regression — the risk/verification ratio didn't
clear the bar for a code-review-driven change without a measured
baseline. Flagging back to the user rather than guessing at scope.
### L1 — ExoPlayer never released
- `MainActivity.kt`: `onDestroy()` now calls
`(application as LotoApplication).voicePlayer.release()` when
`isFinishing` is true. Guarded by `isFinishing` (not fired on a
rotation/config-change recreate) since `ExoPlayer.release()` is terminal
— the single Activity finishing via the exit-confirm dialog's `finish()`
is the one path where no further `speak()` call can follow.
### Manifest/CI hygiene (clear-cut only)
- `AndroidManifest.xml` (L8): dropped `android:launchMode="singleTask"`
(default `standard`) — no deep links or external `startActivity` targets.
- `app/build.gradle.kts` (L11): moved the Vietnamese-only trim from the
deprecated `defaultConfig.resourceConfigurations` to
`androidResources.localeFilters` (AGP 8.13).
- **L10 (CI keystore cleanup) not applied**: `.github/workflows/android-release.yml`
is outside `android/`, and the file-ownership instruction for this task is
explicit ("Files you may modify: anything under .../android"). Flagged,
not fixed — a one-line `if: always()` rm step for whoever owns that file.
- **L9 (`allowBackup`/`dataExtractionRules`) not applied**: the report
itself frames this as conditional ("if a restored `loto_game_state` from
another device is undesirable") — that's a product decision about
cross-device transfer behavior, not a clear-cut technical fix. Left as-is
pending a decision.
- **L7 (`Color.parseColor` → `toColorInt()`) not applied**: not in the
task's enumerated item list (task named Manifest/CI hygiene specifically);
`ColorParsing.kt` is core app code, out of the explicitly requested scope.
## Verification
- `JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:testDebugUnitTest`
→ **BUILD SUCCESSFUL**. `PlayerBoardViewModelTest`: 17/17 (14 original +
3 new). `MasterStoreTest` (new): 2/2. All other existing suites
unaffected (no other test files touched).
- `./gradlew :app:lintDebug` → **BUILD SUCCESSFUL**, no findings in any file
touched by this change (checked by grepping the HTML report for each
modified filename).
- No gradle daemon left running beyond the two builds above (Gradle's
default idle-timeout will reap it; not force-killed since this repo's
standard flow doesn't request that and the daemon is reused by whoever
runs the next Gradle command in this workspace).
## Deferred / out of scope
- M6 (master-panel recomposition) — needs a profiled baseline first.
- L9 (dataExtractionRules) — needs a product decision on cross-device
round-state transfer.
- L10 (CI keystore cleanup) — file outside `android/`, blocked by
file-ownership scope.
- L7 (`Color.parseColor` deprecation) — not in the delegated item list.
- `PlayerCell.kt`'s hardcoded `stateDescription` Vietnamese fragments — the
report bundles this with M3 but the task scoped M3 to the toast + the
five unused resources; left untouched.
- L2–L6 (shared voice player double-cancel, restore shape validation,
swallowed `IOException` with no log signal, stale `autoRunning` after
manual-draw exhaustion, `manualUnticks` surviving a non-BOTH reset) — not
named in the task's item list; not touched.
## Unresolved questions
1. M4: is the residual race (master's restore landing with full history
*before* settings ever resolves) worth a "settings loaded" signal, or is
the report's own "usually invisible" framing enough to leave it as
documented risk? I implemented the report's suggested `combine()` fix,
which closes the more common ordering, but flagging the gap rather than
silently claiming full closure.
2. L9: should `loto_game_state` be excluded from Android backup/device
transfer via `dataExtractionRules`, or is cross-device restore of an
in-progress round acceptable/desired? Needs a product call, not a code
fix.
3. M6: is there an actual profiled slowdown on the API 24 device from
`plans/todo.md`, or should this stay deferred until QA reports one?
Status: DONE_WITH_CONCERNS
Summary: Fixed H1, H2, M1, M3, M4 (partial, documented gap), M5, L1, and the two clear-cut manifest/gradle hygiene items (L8, L11); confirmed M2 already matches web and left as-is; deferred M6, L2-L7, L9, L10 with reasons. All unit tests (including 5 new race-regression tests) and lint pass.
Concerns/Blockers: M4's fix does not close every interleaving (see report); M6/L9/L10 deferred rather than guessed at; none are build-blocking.
plans/reports/fullstack-developer-260831-2226-web-review-fixes.md
+236
View File
@@ -0,0 +1,236 @@
# Fix Report — code review findings, `web/` (SvelteKit lô tô app)
Date: 2026-08-31 · Source review: `plans/reports/code-reviewer-260831-2213-web-review.md`
## Summary
Fixed C1, H1, H2, and mediums M1/M2/M3/M4/M6/M7 (partial — see checkJs below).
Skipped M5, M9, and the rest of Low as out of scope (product decisions or not
requested). 133/133 tests pass (121 original + 12 new), lint clean, svelte-check
clean, both `build`/`build:gh` profiles verified against real output.
## C1 — Service worker never registered
**Fix**: `src/routes/+layout.svelte` — added `registerServiceWorker()`, called
from the existing `onMount`. Guarded on `import.meta.env.PROD` and
`"serviceWorker" in navigator`; dynamically imports `virtual:pwa-register` (so
the module is never evaluated during SSR/prerender) and calls
`registerSW({ immediate: true })`. This reuses `@vite-pwa/sveltekit`'s own
base/scope resolution (`__SW__`/`__SCOPE__` placeholders), so it stays correct
for both the Firebase (`base ""`) and GitHub Pages (`base "/loto"`) builds
without duplicating that logic.
**Verified against real builds** (`BUILD_PROFILE=gh npx vite build` and
`npx vite build`): the compiled `_app/immutable/nodes/0.*.js` (the root
layout) contains the call site `import('../chunks/*.js')` →
`registerSW({immediate:!0})`; the resolved chunk constructs
`new Workbox('./sw.js', {scope: './', type: 'classic'})`. `build/index.html`
itself still shows no literal `sw.js`/`registerSW` string — expected, since
registration now lives in a JS chunk, not injected markup; grepping
`index.html` was never going to prove this either way (that's exactly C1's
root cause). Confirmed the actual reachability by inspecting the compiled
chunk content directly instead.
**Regression guard**: added `scripts/verify-pwa-build.mjs` (new
`npm run verify:pwa`, wired into the CI `build` job for both matrix legs)
that fails the build if no client JS chunk under `build/_app` contains both
`"serviceWorker"` and `"sw.js"`, so this can't silently regress again.
## H1 — Audio precache URLs ignored the base path
**Fix**: extracted `resolveBase()` out of `svelte.config.js` into a new
`web/base-path.js`, imported by both `svelte.config.js` and `vite.config.js`.
`vite.config.js`'s `defaultVoicePrecacheEntries` now prefix every URL with the
same resolved base SvelteKit itself uses, so the two configs can never drift
apart again.
**Verified**: `BUILD_PROFILE=gh npx vite build` →
`grep -o '/loto/' build/sw.js | wc -l` = 92 (all default-voice clips),
`grep -o '"/audio' build/sw.js | wc -l` = 0 (no more origin-rooted entries).
Root/Firebase build (`base ""`) still produces `"/audio/..."` entries
correctly (empty base + `/audio/...` = same as before).
**Regression guard**: `scripts/verify-pwa-build.mjs` also asserts every
precached `*.mp3` URL in `build/sw.js` starts with `${resolveBase()}/audio/`
for the build's actual `BUILD_PROFILE`.
## H2 — Reclaiming a frozen tab rolled back the live round
**Fix**, two parts:
1. `src/routes/+layout.svelte` — the "Nhấn để tiếp tục tại đây" button now
calls a new `reclaimTab()` (`loadSettings(); loadMaster(); claimActiveTab();`)
instead of `claimActiveTab` directly, so this tab re-reads
localStorage — including whatever the peer tab persisted while this tab
was frozen — BEFORE `activeTab.inactive` flips back to `false` and
children remount.
2. Defense in depth: `master-store.svelte.js` — `masterState` gained a
`hydrated` boolean, set `true` at the end of `loadMaster()` (via
`finally`, so it's set on every path: success, corrupt JSON, missing
data). `MasterPanel.svelte`'s save effect now only calls `saveMaster()`
when `masterState.hydrated` is true, so a mount can never persist stale
in-memory state before a load has actually happened in this tab's
session.
**Tests added** (`master-store.test.js`, new `describe("hydrated flag …")`):
flag starts false and flips true after load (including the "nothing to
load" and "corrupt JSON" paths), and a scenario test simulating the exact
reclaim sequence (draw 2, persist, peer persists a 3rd draw, reload →
in-memory state matches the peer's newer data, not the stale copy).
## M1 — Invisible-but-clickable toast
**Fix**: `PlayerBoard.svelte` — added `toastId` state, incremented in
`showToast()`; wrapped the toast markup in `{#key toastId}` so every
`showToast()` call forces a full DOM remount instead of reusing the node,
which restarts the 5s CSS fade animation and guarantees the
`pointer-events-auto` button is torn down when its own timer expires, even
if a second "Chờ" arrived mid-animation.
## M2 — "Kinh!" cut off by "Chờ" in the same effect pass
**Fix**: `PlayerBoard.svelte`'s bingo/waiting `$effect` — added a local
`announcedBingo` flag set when pass 1 calls `playBingo()`; pass 2 now skips
the `playWaiting()` call (not the toast/state bookkeeping) when
`announcedBingo` is true, so a draw that completes one row and puts another
one number away no longer has the win clip silently cancelled a
millisecond after it starts.
## M3 — Theme flash on cold load
**Fix**: `src/app.html` — added a blocking inline `<script>` in `<head>`,
before `%sveltekit.head%` (so it runs before app.css's injected `<link>`),
that reads `localStorage.getItem("loto_settings")`, applies `.dark` and
`--empty-cell-bg` with the same validation semantics as
`settings-store.svelte.js` (defensive try/catch, hex6 regex), falling back
to light/default on any parse failure. `loadSettings()` in `+layout.svelte`
remains the authoritative pass.
## M4 — No focus management in the two `aria-modal` dialogs
**Fix**: new `src/lib/focus-trap.js` — a Svelte action (`use:focusTrap`)
that focuses the dialog container on mount (`tabindex="-1"` added if
missing), traps Tab/Shift+Tab among focusable descendants, restores focus to
whatever was focused before the dialog opened, and locks `document.body`
scroll for as long as any trap is active (reference-counted so the bingo
modal and settings sheet can't fight over the lock if both are somehow
open). Wired into `PlayerBoard.svelte`'s bingo dialog and
`SettingsButton.svelte`'s settings dialog. Existing window-level Escape
handling in both components is untouched.
**Tests added**: `focus-trap.test.js` (6 cases) — focuses container on
mount, Tab wraps last→first, Shift+Tab wraps first→last, body scroll
lock/unlock, focus restoration on destroy, and reference-counted lock across
two stacked traps.
## M6 — iOS auto-call voice silent (+ 92 retained `Audio` elements)
**Fix**: rewrote `voice.js`'s element management — one reusable
`HTMLAudioElement` (`.src` swapped per clip) instead of a `Map` growing to
one element per clip URL. New `unlockAudio()` export, called synchronously
from `MasterPanel.svelte`'s `toggleAuto()` ("Bắt đầu") and
`handleDrawNext()` ("Xổ số") — both real click handlers — primes the shared
element inside a genuine user gesture so it stays unlocked for later
`play()` calls from `setInterval` (auto-call), which iOS Safari does not
treat as a gesture. `clearAudioCache()` kept as an export (now just
`cancelPlayback()`) so callers that change `settings.voice` don't need to
change.
**Tests**: rewrote `voice.test.js` to match the new architecture (the old
tests asserted "one Audio instance per URL", now structurally false by
design) using `vi.resetModules()` + dynamic re-import per test for clean
module-singleton isolation, preserving every original assertion's intent,
plus two new cases for `unlockAudio()` (idempotent priming, later playback
reuses the primed element).
## M7 — Lint/typecheck never ran in CI
**Fix**: `.github/workflows/ci.yml` `test` job now runs `npm run lint` and
`npm run check` before `npm test`. Added `svelte-check` as a devDependency
and a `check` script (`svelte-check --tsconfig ./jsconfig.json`).
**checkJs — left off, per task's explicit escape hatch.** Tried enabling it:
`checkJs: true` produced 58 errors across 10 files. The large majority
cascade from one pre-existing, unrelated root cause — `DEFAULT_SETTINGS` is
`Object.freeze`d, so TS infers each field's literal type (`"#7030A0"`,
`false`, `5`, …) instead of its declared JSDoc type, and every subsequent
`settings.foo = <widened value>` assignment across `settings-store.svelte.js`,
`SettingsButton.svelte`, and their test files fails. The rest are similarly
pre-existing and unrelated to this task's fixes: `PlayerBoard.svelte`'s
nullable-grid call sites, `active-tab.test.js`'s Vitest cache-busting
`?query` dynamic imports, `overlay-history.test.js` implicit-any locals. None
of these were introduced by my changes — verified by re-running
`svelte-check` with `checkJs` back to `false`: 0 errors, 0 warnings. Fixing
the settings-store typing alone would mean re-typing every settings field
and touching ~5 files well outside this task's file list; that's the "large
refactor" the task told me to leave for later rather than attempt here.
`svelte-check` still runs in CI (and locally passes clean with `checkJs:
false`, catching real Svelte-specific issues like unused CSS/a11y/reactivity
mistakes independent of TS types) — only the JS-type-checking half of M7 is
deferred. `jsconfig.json` is unchanged (`checkJs: false`).
I did fix the two type errors my own new `focus-trap.js` produced under
`checkJs: true` (an untyped `filter()` losing its `HTMLElement` narrowing) —
cheap, and keeps that file honest even though `checkJs` ends up off.
## Skipped (per task instructions — product decisions / not requested)
- **M5** — two tabs opened simultaneously freeze each other (mutual claim).
Explicitly excluded ("multi-tab contract" needs a product decision per the
review's Unresolved Question #2).
- **M9** — docs/comment say auto-reload "must not happen"
(`vite.config.js:40-41`'s comment), but `registerType: "autoUpdate"`
auto-reloads on SW update. This was silently inert before C1 (nothing ever
registered the SW, so it never fired); now that C1 is fixed, a deploy
during a live round CAN trigger `window.location.reload()` mid-game — the
exact scenario the comment warns against. Left `registerType: "autoUpdate"`
unchanged since the review flags the resolution (implement a reload-prompt
UI vs. accept auto-reload) as needing a product decision (Unresolved
Question #4). **Flagging this explicitly since C1 makes the risk real
where it was previously theoretical** — recommend deciding this soon.
- **L1–L10** — not in the requested fix list; untouched.
- **M8** (add broad component/effect test coverage) — not in the requested
fix list either; added the specific unit tests called out above
(hydration flag, focus-trap, voice unlock) that directly cover the fixed
logic, but did not add `@testing-library/svelte`/component-mount
infrastructure for M1/M2's Svelte-effect-level behavior — that's the
larger, separately-scoped ask M8 describes, and doing it well means adding
a new test-rendering dependency and harness, not a few lines.
## Files changed
- `web/src/routes/+layout.svelte` — SW registration, reclaim re-hydration.
- `web/src/lib/master-store.svelte.js` — `hydrated` flag.
- `web/src/lib/MasterPanel.svelte` — hydration-gated save effect, `unlockAudio()` wiring.
- `web/src/lib/PlayerBoard.svelte` — toast remount key, bingo/waiting priority, focus trap wiring.
- `web/src/lib/SettingsButton.svelte` — focus trap wiring.
- `web/src/lib/voice.js` — single shared `<audio>` element + `unlockAudio()`.
- `web/src/app.html` — pre-paint theme script.
- `web/vite.config.js`, `web/svelte.config.js` — shared base resolver.
- `web/base-path.js` (new) — shared `resolveBase()`.
- `web/src/lib/focus-trap.js` (new) — reusable dialog focus-trap action.
- `web/scripts/verify-pwa-build.mjs` (new) — C1/H1 build-time regression guard.
- `web/package.json`, `web/package-lock.json` — `check`/`verify:pwa` scripts, `svelte-check` devDependency.
- `.github/workflows/ci.yml` — lint/check/verify:pwa steps.
- Tests: `web/src/lib/master-store.test.js`, `web/src/lib/voice.test.js` (rewritten), `web/src/lib/focus-trap.test.js` (new).
## Verification
- `npx vitest run` → 133/133 pass (9 files; was 121/8).
- `npx eslint .` → 0 problems.
- `npx svelte-check --tsconfig ./jsconfig.json` → 0 errors, 0 warnings (418 files).
- `BUILD_PROFILE=gh npx vite build` then `BUILD_PROFILE=gh node scripts/verify-pwa-build.mjs` → pass (base `/loto`, 92 precached audio entries all prefixed, SW registration reachable in compiled JS).
- `npx vite build` (root/Firebase profile) then `node scripts/verify-pwa-build.mjs` → pass (base `""`).
- Manually inspected the compiled `_app/immutable/nodes/0.*.js` and its dynamically-imported chunk to confirm `registerSW({immediate:true})` → `Workbox('./sw.js', {scope: './'})` is really reachable at runtime, not just present in dead code.
## Unresolved questions
1. M9 is now a live risk, not a theoretical one (see above) — worth deciding
soon whether to accept mid-round auto-reload or add a "Có bản mới, tải
lại?" prompt banner.
2. `registerType: "autoUpdate"` was left as-is since changing it is the M9
product decision; flagging in case the intent was actually to fix C1 *and*
settle M9 together.
Status: DONE_WITH_CONCERNS
Summary: Fixed C1/H1/H2 and mediums M1/M2/M3/M4/M6/M7(CI half); checkJs left off after confirming the 58 errors are a pre-existing, unrelated settings-store typing refactor outside this task's scope; M5/M9/Lows skipped as instructed.
Concerns/Blockers: M9 (mid-round auto-reload) is now a real risk now that C1 registers the SW — was previously inert. Recommend a follow-up product decision soon.
plans/reports/fullstack-developer-260831-2250-android-followup-fixes.md
+199
View File
@@ -0,0 +1,199 @@
# Android follow-up fixes (2026-08-31)
Source: `plans/reports/code-reviewer-260831-2213-android-review.md` (L2–L7,
L10) + the review's M3 note on `PlayerCell.kt`'s hardcoded `stateDescription`.
Prior pass: `plans/reports/fullstack-developer-260831-2226-android-fixes.md`
(H1/H2/M1/M3-partial/M4-partial/M5/L1/L8/L11; explicitly deferred L2–L7, L9,
L10, and the `PlayerCell.kt` cleanup — this task closes that deferred list
except L9, which stays a product decision, and M6, out of scope per the task).
Scope: `android/` + `.github/workflows/android-release.yml` (L10 only). `web/`
untouched. `android/local.properties` not read.
## Fixed
### L2 — shared voice player double-cancel
`state/MasterPanelViewModel.kt`, `state/PlayerBoardViewModel.kt`:
`onCleared()` no longer calls `voicePlayer.cancel()`, only `super.onCleared()`.
Root cause check (not just the described symptom): both ViewModels are
activity-scoped (`LotoViewModelFactory` via `by viewModels { factory }` in
`MainActivity`), so `onCleared()` only fires together, on a real finish — and
`MainActivity.onDestroy()` already calls the shared `voicePlayer.release()`
unconditionally on that same `isFinishing` path (the prior pass's L1 fix).
`release()` supersedes `cancel()` (it stops and tears the player down), so the
per-VM cancel calls were dead weight duplicating L1's fix, plus the fragility
the review flagged (two independent owners unilaterally acting on a shared
singleton). Removing them and adding the missing `super.onCleared()` closes
both parts of the finding without behavior change on any currently-possible
scoping.
Not unit-tested: `onCleared()` is `protected` on `ViewModel` and neither test
class subclasses it, so it isn't reachable from a JVM unit test without a
lifecycle test harness this project doesn't have (same gap as L1, which also
shipped without a dedicated test).
### L3 — restore payload shape validation
`state/GameStateRepository.kt`:
- `parseNumberList` (backs `master_called`, `master_remaining`,
`player_manual_unticks`) now rejects duplicates (`toSet().size != size`),
not just out-of-range values.
- `parseGrid` now rejects a duplicate non-zero cell value (a real card never
repeats a number).
- `loadMasterState` adds `isCompleteDeckPartition`: `called` and `remaining`
must be disjoint and together cover exactly the 90-number deck. Combined
size ≠ 90 or an overlap (detected via combined-set size) both fail closed to
"no saved round" — consistent with the class's existing "malformed payload
shape-validates to empty" contract, so nothing new crashes or propagates.
Tests added (`state/GameStateRepositoryTest.kt`): duplicate grid cell,
duplicate manual unticks, three `isCompleteDeckPartition` violations
(duplicate-within-field, called/remaining overlap, doesn't sum to 90). Updated
`master state survives a repository recreation` to use a full valid 90-number
partition instead of 6 arbitrary numbers — it would otherwise now fail the new
invariant it wasn't designed to satisfy.
### L4 — swallowed IOException with no log signal
`state/GameStateRepository.kt`, `settings/SettingsRepository.kt`: every
`catch (_: IOException)` now does `Log.w(TAG, "...", e)` before falling back
(read → empty/defaults, write → silently keeps in-memory state, unchanged
behavior otherwise).
`app/build.gradle.kts`: added
`testOptions { unitTests { isReturnDefaultValues = true } }` — the stock
`android.jar` stub throws on any unmocked call (no Robolectric in this
project), so an untouched `Log.w` call would crash any unit test that
exercises the catch block. This is scoped to unit tests only, no production
behavior change.
Added `ThrowingDataStore` (`app/src/test/java/com/miti99/loto/`) — a
`DataStore<Preferences>` fake that throws `IOException` from both `data` and
`updateData`, per the task's note that file-backed DataStore can't be
exercised on Windows. Tests added in `GameStateRepositoryTest` and
`SettingsRepositoryTest` assert the read/write paths degrade instead of
throwing. (Log content itself isn't asserted — no mocking framework is set up
for `android.util.Log`; behavior is verified instead, matching this project's
existing test style.)
### L5 — stale autoRunning after a draw exhausts the deck
`state/MasterPanelViewModel.kt`: `drawNext()` now checks
`masterStore.state.value.remaining.isEmpty()` right after drawing and flips
`_autoRunning.value = false` immediately, instead of relying solely on the
ticker loop's next-tick top-of-loop check. Verified the `StateFlow` update
propagates through `combine(...).distinctUntilChanged().collectLatest{...}`
and cancels the in-flight `while(true)` loop within the same virtual-time
`advanceTimeBy` call (no extra tick needed) — confirmed by the updated test.
Also correct for a manual "Xổ số" draw that happens to be the one exhausting
the deck while auto-call was separately left on.
Left the ticker loop's own top-of-loop check in place as a harmless second
line of defense.
Tests: renamed/strengthened `auto-call stops when the deck is exhausted` →
`auto-call stops immediately when a draw exhausts the deck (L5)`, asserting
`autoRunning` is false right after the exhausting draw (previously only
asserted after a second tick, which is exactly the stale window L5 describes).
Added `manual drawNext exhausting the deck leaves autoRunning false (L5)` for
the literal "manual draw" wording.
### L6 — manualUnticks surviving a non-BOTH reset
`state/PlayerBoardViewModel.kt`: `onMasterState()`'s master-"Ván mới"
detection (`prev > 0 && len < prev`) now clears `manualUnticks` — and persists
that if it had entries — in every mode, not only inside the `BOTH`-gated
branch. `onCellClick` has no mode gate (any number in the master's `called[]`
can be manually unticked regardless of the currently displayed mode), so a
reset must invalidate that suppression state everywhere, not just when `BOTH`
happens to be selected at reset time. The `crossed`/`lastHandledIndex`/row-
tracker reset stays `BOTH`-only, unchanged — L6 only concerns
`manualUnticks`.
Test added: `manual unticks are cleared and persisted on a master Ván mới
outside both mode (L6)` — crosses/unticks a called number in `PLAYER` mode,
resets the master round, and reads `GameStateRepository.loadPlayerState()`
directly (sidesteps the unrelated, not-in-scope question of whether
`lastHandledIndex` also needs resetting on a non-`BOTH` shrink — L6 is
specifically about `manualUnticks`).
### L7 — deprecated Color.parseColor
`ui/ColorParsing.kt`: `android.graphics.Color.parseColor(this)` →
`this.toColorInt()` (androidx-core, already a dependency via
`androidx.core.ktx`). Same `IllegalArgumentException` catch/fallback
unchanged. Not unit-tested (no test existed before either — parsing depends
on the unmocked `android.graphics.Color` stub, which is untestable in a pure
JVM unit test regardless of which API calls it; the settings-layer regex is
what's actually covered, in `SettingsRepositoryTest`).
### PlayerCell.kt stateDescription → strings.xml
`res/values/strings.xml`: added `player_cell_number` ("Số %1$d"),
`player_cell_crossed_suffix` (", đã đánh dấu"),
`player_cell_waiting_suffix` (", đang chờ").
`ui/player/PlayerCell.kt`: resolves all three via `stringResource(...)`
unconditionally (composable-safe — no conditional `stringResource` calls) and
builds the same `stateDescription` string as before from those values. No
change in the rendered accessibility text.
### L10 — CI keystore cleanup
`.github/workflows/android-release.yml`: added a
`Remove decoded keystore` step (`if: always()`, `rm -f
"$GITHUB_WORKSPACE/keystore.p12"`) between the signed-build step and the
GitHub Release upload step, so the decoded `keystore.p12` is removed from the
runner workspace whether or not the build succeeds.
## Deferred / out of scope (unchanged from prior pass or explicitly excluded)
- L9 (`dataExtractionRules`) — product decision, not touched (task's NOT-IN-SCOPE list).
- M4's residual settings-loaded-signal race, M6 recomposition perf — explicitly out of scope per the task.
## Verification
- `JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:testDebugUnitTest`
→ **BUILD SUCCESSFUL**. 118 tests across 13 suites, 0 failures/errors
(includes all newly added L3/L4/L5/L6 regression tests).
- `JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:lintDebug`
→ **BUILD SUCCESSFUL**, 17 warnings — all pre-existing (AGP/Gradle
dependency version notices, one unrelated string typo, two plurals
candidates, one manifest `enableOnBackInvokedCallback` API-level notice).
Verified none reference any file touched by this change and no
`UnusedResources` finding for the three new strings.
- No gradle daemon force-killed; left to Gradle's own idle-timeout per the
process-management rule.
## Files modified
- `android/app/src/main/java/com/miti99/loto/state/MasterPanelViewModel.kt`
- `android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt`
- `android/app/src/main/java/com/miti99/loto/state/GameStateRepository.kt`
- `android/app/src/main/java/com/miti99/loto/settings/SettingsRepository.kt`
- `android/app/src/main/java/com/miti99/loto/ui/ColorParsing.kt`
- `android/app/src/main/java/com/miti99/loto/ui/player/PlayerCell.kt`
- `android/app/src/main/res/values/strings.xml`
- `android/app/build.gradle.kts`
- `android/app/src/test/java/com/miti99/loto/ThrowingDataStore.kt` (new)
- `android/app/src/test/java/com/miti99/loto/state/GameStateRepositoryTest.kt`
- `android/app/src/test/java/com/miti99/loto/settings/SettingsRepositoryTest.kt`
- `android/app/src/test/java/com/miti99/loto/state/MasterPanelViewModelTest.kt`
- `android/app/src/test/java/com/miti99/loto/state/PlayerBoardViewModelTest.kt`
- `.github/workflows/android-release.yml`
## Unresolved questions
1. L2: is it worth adding a lifecycle-test harness (e.g. pulling in
`androidx.lifecycle:lifecycle-viewmodel-testing` or Robolectric) so
`onCleared()` behavior is directly testable, or does the code-level
ownership argument above (both VMs share the exact same clearing
condition as `MainActivity`'s existing guarded `release()`) suffice
without one? Same gap already existed for L1.
2. L3: `isCompleteDeckPartition` treats *any* deviation from a full 1..90
partition as corrupt (falls back to "no saved round"). This matches the
review's literal ask, but is strict — e.g. it would also reject a
hypothetical future variable-length deck. Flagging in case that's ever a
real product direction, not just a theoretical one.
Status: DONE
Summary: Implemented and tested L2, L3, L4, L5, L6, L7, L10, and the
PlayerCell.kt strings.xml cleanup; all 118 unit tests and lint pass clean with
no new findings.
Concerns/Blockers: none blocking; two unresolved questions above are informational only.
plans/reports/fullstack-developer-260831-2250-web-followup-fixes.md
+186
View File
@@ -0,0 +1,186 @@
# Fix Report — M9 (update-prompt UX) + checkJs, `web/` (SvelteKit lô tô app)
Date: 2026-08-31 · Source: `plans/reports/code-reviewer-260831-2213-web-review.md` (M9, M7)
and `plans/reports/fullstack-developer-260831-2226-web-review-fixes.md` (checkJs left off)
## Summary
Both items done in full, no escape hatch needed. `registerType` is now
`"prompt"` with a real reload-prompt banner; `checkJs: true` with all 56
pre-existing errors fixed via type-only changes (no runtime behavior
changed). 138/138 tests pass (133 + 5 new), lint clean, svelte-check clean,
both build profiles + `verify-pwa-build.mjs` pass.
## Item 1 — M9: mid-round auto-reload
**`vite.config.js`**: `registerType: "autoUpdate"` → `"prompt"`. Comment
above it rewritten to describe the new flow instead of just warning against
the old one.
**`src/lib/update-prompt.svelte.js`** (new): small `.svelte.js` store module,
same shape as `active-tab.svelte.js`/`master-store.svelte.js`:
- `updatePrompt = $state({ visible: false })`
- `setUpdateSW(fn)` — stores the function `registerSW()` returns
- `showUpdatePrompt()` — wired to `onNeedRefresh`, flips `visible = true`
- `applyUpdate()` — hides the banner, calls `updateSW(true)` (reload)
- `dismissUpdate()` — hides the banner, does NOT call `updateSW` (old
version keeps running this visit)
- `_resetUpdatePromptForTest()` — test-only reset, matches
`_resetOverlayHistoryForTest()`'s convention
**`src/routes/+layout.svelte`**: `registerServiceWorker()` now calls
`registerSW({ onNeedRefresh: showUpdatePrompt })` (was
`registerSW({ immediate: true })`) and stores the returned `updateSW` via
`setUpdateSW()`. Template: banner renders as a sibling of `{@render
children()}` inside the active-tab branch only (hidden while the "other tab
active" full-screen overlay is showing, so the two never fight over
z-index). Markup: `fixed`, bottom-4/corner-anchored, `max-w-xs` on ≥sm,
`z-40` (below the app's `z-50` dialogs, so a modal correctly covers it) —
deliberately NOT full-screen and NOT near the board or MasterPanel's
`heroEl` "Số vừa xổ" element, both of which live in normal document flow
mid-page. Vietnamese copy: "Có bản mới. Tải lại?" + a rose-600 "Tải lại"
button (matches the bingo modal's primary-button styling) + a plain-text
"Để sau" dismiss button. Both light and dark variants via the existing
`bg-white dark:bg-slate-800` / `border-slate-200 dark:border-slate-600`
pattern used elsewhere (`SettingsButton.svelte`'s sticky header/footer).
**Verified against real builds**: compiled `_app/immutable/nodes/0.*.js` +
its `virtual:pwa-register` chunk contain `onNeedRefresh` and no longer
contain `immediate:!0`. `build/sw.js`'s only `self.skipWaiting()` call is
gated behind `self.addEventListener("message", i => i.data?.type ===
"SKIP_WAITING" && self.skipWaiting())` — i.e. the new SW never activates
itself; it only does so on an explicit `postMessage` that workbox-window's
`updateSW(true)` sends, which only fires from the banner's "Tải lại" tap.
**`scripts/verify-pwa-build.mjs`**: unchanged — its C1/H1 assertions only
check for `"serviceWorker"` + `"sw.js"` reachability and the base-prefixed
audio precache list, neither of which depends on `registerType`. Re-ran it
against both profiles; both pass.
**Test added**: `src/lib/update-prompt.test.js` (5 cases) — starts hidden;
`showUpdatePrompt` reveals it; `dismissUpdate` hides without calling
`updateSW`; `applyUpdate` hides and calls `updateSW(true)` exactly once;
`applyUpdate` before `registerSW` resolves is a safe no-op.
## Item 2 — `checkJs: true`
`jsconfig.json`: `checkJs: false` → `true`. Fixed all 56 errors
(`npx svelte-kit sync && npx svelte-check --tsconfig ./jsconfig.json` → the
list matched the prior report's count almost exactly — 56 not 58, small
line-number drift from the Item-1 edits). All fixes are type-only; no
runtime behavior changed anywhere.
**Root cause 1 — `Object.freeze(DEFAULT_SETTINGS)` literal narrowing**
(~25 of the 56 errors, across `settings-store.svelte.js`,
`settings-store.test.js`, `SettingsButton.svelte`, `voice.test.js`): fresh
object-literal fields like `autoCallEnabled: false` were inferred as the
literal type `false`, not `boolean`, and that leaked into
`settings = $state({ ...DEFAULT_SETTINGS })`. **Fix**: added an explicit
`@typedef {Object} Settings` (one property per field, general types) in
`settings-store.svelte.js` and cast the object literal passed to
`Object.freeze()` — `Object.freeze(/** @type {Settings} */ ({ ... }))` —
so every field keeps its widened type instead of narrowing to the default's
literal value. Removed the two now-redundant per-field inline casts
(`theme`, `mode`) since the typedef covers them. Zero other files touched
for this cause — it single-handedly cleared the settings-store,
SettingsButton, and voice/settings test errors.
**Root cause 2 — `voice.js`'s `new Promise` executor**: `playClip()` had no
return-type annotation, so TS couldn't infer `T` for `new Promise<T>` and
fell back to a `resolve` that requires an argument (can't be called as
`resolve()`). **Fix**: added `@returns {Promise<void>}` to `playClip`'s
JSDoc — `void` gets TS's special "callable with zero args" treatment for
`resolve`. No behavior change (the function always resolved with no value).
**Root cause 3 — `PlayerBoard.svelte` nullable-grid narrowing lost inside
closures**: `grid` is `$state(number[][] | null)`; three call sites read it
inside a `.map()`/`.some()` callback after an outer null check, and TS
doesn't carry narrowing of a mutable `let` binding into a nested function
expression. **Fix**: capture `grid` into a local `const g = grid` right
after each null check, then reference `g` (not `grid`) inside the closures
— `rowCompleteness` and `waitingRows` changed from `$derived(expr)` to
`$derived.by(() => { const g = grid; return … })` (same dependency-tracking
semantics, since the same reactive reads happen either way); the
bingo/waiting `$effect` got a `const g = grid;` right after its early
return and had its `grid` references swapped to `g`. Purely a type-narrowing
refactor — no logic changed.
**Remaining one-off fixes** (implicit-`any`s, one unresolvable-module class,
all pre-existing per the prior report):
- `SettingsButton.svelte`'s `{#snippet switchRow(label, isOn, onToggle)}` —
added inline JSDoc casts on the three params (`string`, `boolean`,
`() => void`), same style already used elsewhere in the file for event
handler params.
- `settings-store.test.js`'s `mockMatchMedia()` stub — added
`(e: MediaQueryListEvent) => void` JSDoc to the `fn` params of
`addEventListener`/`removeEventListener`.
- `overlay-history.test.js`'s `back`/`pushState` — typed as
`ReturnType<typeof vi.spyOn>` instead of left to bare `let` inference.
- `active-tab.test.js`'s `FakeBC.onmessage` — added
`{((e: {data: any}) => void) | null}` JSDoc to the constructor
assignment.
- `active-tab.test.js`'s six `import("./active-tab.svelte.js?<tag>")` —
these are real relative imports at runtime (Vitest's cache-busting query
suffix loads a fresh module instance per test for isolation) but not a
specifier TS can resolve. Tried an ambient wildcard module declaration
(`declare module "./active-tab.svelte.js?*" { export * from "..." }`)
first; it resolved but produced a wrapped `{ default: typeof import(...) }`
shape instead of the real named exports (wildcard ambient modules don't
behave like `export *` re-exports for query-string specifiers). Dropped
that file and used `@ts-expect-error` with an explanatory comment on each
import line instead — simpler, guaranteed correct, and each error was a
genuine one-to-one match for `@ts-expect-error` to consume.
- `+layout.svelte`'s `import("virtual:pwa-register")` — no `app.d.ts`
existed in this project (SvelteKit's usual scaffold file was never
created). Added `src/app.d.ts` with the standard SvelteKit template plus
`/// <reference types="vite-plugin-pwa/client" />`, which supplies
`vite-plugin-pwa`'s own ambient types for the virtual module (already a
transitive dependency; no new package added).
**Verified `checkJs` didn't just move the goalposts**: after all fixes,
`npx svelte-kit sync && npx svelte-check --tsconfig ./jsconfig.json` →
`0 ERRORS, 0 WARNINGS` (429 files). No runtime test assertions changed —
`vitest run` stayed at the same pass/fail shape (133 → 138, the +5 being the
new Item-1 test file only).
## Files changed
- `web/vite.config.js` — `registerType: "prompt"`, comment rewritten.
- `web/src/routes/+layout.svelte` — `onNeedRefresh` wiring, update banner.
- `web/src/lib/update-prompt.svelte.js` (new) — update-prompt store.
- `web/src/lib/update-prompt.test.js` (new) — 5 tests.
- `web/jsconfig.json` — `checkJs: true`.
- `web/src/lib/settings-store.svelte.js` — `Settings` typedef, cast on
`DEFAULT_SETTINGS`'s object literal.
- `web/src/lib/voice.js` — `@returns {Promise<void>}` on `playClip`.
- `web/src/lib/PlayerBoard.svelte` — local `const g = grid` narrowing at 3
call sites (2 `$derived` → `$derived.by`, 1 `$effect`).
- `web/src/lib/SettingsButton.svelte` — JSDoc param casts on the
`switchRow` snippet.
- `web/src/lib/settings-store.test.js` — JSDoc param casts on
`mockMatchMedia`'s stub.
- `web/src/lib/overlay-history.test.js` — typed `back`/`pushState`.
- `web/src/lib/active-tab.test.js` — typed `FakeBC.onmessage`,
`@ts-expect-error` on the 6 query-string dynamic imports.
- `web/src/app.d.ts` (new) — standard SvelteKit ambient types file +
`vite-plugin-pwa/client` reference.
## Verification
- `npx vitest run` → 138/138 pass (10 files; was 133/9).
- `npx eslint .` → 0 problems.
- `npx svelte-kit sync && npx svelte-check --tsconfig ./jsconfig.json` →
0 errors, 0 warnings, `checkJs: true` (429 files).
- `BUILD_PROFILE=gh npx vite build` + `BUILD_PROFILE=gh node
scripts/verify-pwa-build.mjs` → pass (base `/loto`, 92 audio entries,
registerSW/sw.js reachable).
- `npx vite build` + `node scripts/verify-pwa-build.mjs` → pass (base `""`).
- Manually inspected compiled `_app/immutable/nodes/0.*.js` + its
`virtual:pwa-register` chunk: `onNeedRefresh` present, `immediate:!0`
gone. Manually inspected `build/sw.js`: `self.skipWaiting()` only fires
on an incoming `SKIP_WAITING` postMessage, never unconditionally.
## Unresolved questions
None — both items were resolvable in full within the stated scope; no
escape hatch was needed for `checkJs`.
plans/reports/fullstack-developer-260831-2314-android-backup-settings.md
+127
View File
@@ -0,0 +1,127 @@
# Fix report: Android backup rules (L9) + settings-loaded signal (M4 residual)
Phase 2 of `plans/260831-2314-review-followups/plan.md`. Scope: two decisions
delegated by the user — exclude game-state DataStore from backup/transfer,
keep settings backed up; close the M4 residual race (master restore landing
before settings resolves) with an explicit settings-loaded signal.
## Item A — backup rules
Verified on-disk DataStore names from `LotoApplication.kt`'s
`preferencesDataStore(name = ...)` delegates: `loto_settings` and
`loto_game_state`. `androidx.datastore.preferences.preferencesDataStore`'s
default file location is `files/datastore/<name>.preferences_pb`, so the
excluded path is `datastore/loto_game_state.preferences_pb`.
- New `android/app/src/main/res/xml/data_extraction_rules.xml` — API 31+,
excludes that path from both `<cloud-backup>` and `<device-transfer>`.
- New `android/app/src/main/res/xml/backup_rules.xml` — `full-backup-content`
counterpart for API 24-30 (minSdk 24, so this path is live).
- `AndroidManifest.xml`: added `android:dataExtractionRules` and
`android:fullBackupContent` to `<application>`; `android:allowBackup="true"`
left unchanged. Settings DataStore is not mentioned anywhere in either XML,
so it stays backed up/transferred by default.
## Item B — M4 residual: settings-loaded signal
Root cause (confirmed by reading `PlayerAutoCross.applyMasterCalls`): the
cursor advances to `called.size` on every non-`BOTH` pass even when nothing
changes. If `masterStore.restore()` lands with the full history before the
settings DataStore read resolves, the prior `combine()` fix still evaluates
that first master emission with `settings.value.mode` at its `PLAYER`
default, consumes the cursor to `called.size`, and the later real-mode
arrival finds `cursor >= called.size` — replay lost, permanently, since
`masterStore.state` doesn't re-emit until the next draw.
Fix — a loaded signal threaded end to end:
- `settings/SettingsRepository.kt`: new `val loaded: Flow<Boolean> =
settingsFlow.map { true }.onStart { emit(false) }` — cold, derived purely
from `settingsFlow`, mirrors the existing `settingsFlow`/`settingsState`
pairing rather than adding a scope to the repository.
- `LotoApplication.kt`: new `val settingsLoaded: StateFlow<Boolean> =
settingsRepository.loaded.stateIn(appScope, SharingStarted.Eagerly, false)`,
next to `settingsState`.
- `state/PlayerBoardViewModel.kt`: new constructor param `settingsLoaded:
StateFlow<Boolean> = MutableStateFlow(true)` (defaults to "already loaded"
so every existing call site, including all prior tests, is unaffected).
`init`'s collector changed from
`combine(masterStore.state, settings) { master, _ -> master }.collect { onMasterState(it) }`
to `combine(masterStore.state, settings, settingsLoaded) { master, _, loaded -> master to loaded }.collect { (master, loaded) -> if (loaded) onMasterState(master) }`.
While `!loaded`, `onMasterState` is skipped entirely — `lastHandledIndex`
stays untouched — so once `settingsLoaded` flips true, `combine`'s cached
master snapshot is re-delivered and replayed under the real mode.
- `state/LotoViewModelFactory.kt`: passes `settingsLoaded = app.settingsLoaded`
to the production `PlayerBoardViewModel`. This file isn't in the plan's
Phase 2 file-ownership list but is load-bearing — without it the fix would
only exist in test wiring, not in the shipped app. The direct task
instructions ("Files you may modify: anything under android/") permit it.
- `state/MasterPanelViewModel.kt` / `state/MasterStore.kt`: unchanged. Neither
consumes master emissions through the mode-gated cursor the task scoped
this fix to; `MasterPanelViewModel` reads `settings.value` synchronously
per-draw with no persistent cursor to corrupt.
- H1/loading behavior (restore vs. in-flight user action, from the prior fix
pass) is untouched — `restore()`/`publish(loading = ...)` logic wasn't
touched, and the H1 regression tests in `PlayerBoardViewModelTest` still
pass unmodified.
## Tests
- `settings/SettingsRepositoryTest.kt`: new `loaded stays false until the
first DataStore read resolves` — `SlowDataStore` + `CompletableDeferred`
gate, collects `loaded` into a list via `backgroundScope`, asserts
`[false]` before the gate opens and `[false, true]` after.
- `state/PlayerBoardViewModelTest.kt`: new `master restore landing with full
history before settings resolves still replays under the real mode (M4
residual)` — builds a real `SettingsRepository` over a gated
`SlowDataStore` (mode persisted as `BOTH` ahead of time on the ungated
backing store), constructs the `ViewModel` with `settingsFlow`/`loaded`
`stateIn`'d the same way `LotoApplication` does, resolves `masterStore`
with the full 90-number history *before* completing the settings gate,
asserts nothing crossed yet, then completes the gate and asserts the full
board replays (`rowComplete` all true).
- **Verified fail-without-fix**: temporarily reverted the `init` collector to
the pre-fix `combine(...) { master, _, _ -> master }.collect { onMasterState(it) }`
(ignoring `loaded`) and reran `:app:testDebugUnitTest --tests
"...PlayerBoardViewModelTest"` — exactly the new regression test failed (18
others passed), confirming it actually exercises the closed gap. Reverted
back to the fix immediately after.
- Fixed a pre-existing opt-in warning while touching `SettingsRepositoryTest.kt`
(`@OptIn(ExperimentalCoroutinesApi::class)` on the class, matching the
pattern already used in `PlayerBoardViewModelTest.kt`) since the new test
needed `runCurrent`.
## Verification
```
JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:testDebugUnitTest :app:lintDebug
```
`BUILD SUCCESSFUL`, all unit tests pass (including the two new ones), lint
clean. Not committed, per instructions.
## Files changed
- `android/app/src/main/AndroidManifest.xml`
- `android/app/src/main/res/xml/data_extraction_rules.xml` (new)
- `android/app/src/main/res/xml/backup_rules.xml` (new)
- `android/app/src/main/java/com/miti99/loto/settings/SettingsRepository.kt`
- `android/app/src/main/java/com/miti99/loto/LotoApplication.kt`
- `android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt`
- `android/app/src/main/java/com/miti99/loto/state/LotoViewModelFactory.kt`
- `android/app/src/test/java/com/miti99/loto/settings/SettingsRepositoryTest.kt`
- `android/app/src/test/java/com/miti99/loto/state/PlayerBoardViewModelTest.kt`
Note: `git status` also shows other android files modified/new
(`build.gradle.kts`, `MainActivity.kt`, `MasterStore.kt`,
`GameStateRepository.kt`, `MasterPanelViewModel.kt`, `ColorParsing.kt`, UI
files, `MasterStoreTest.kt`, `ThrowingDataStore.kt`, etc.) — these are
pre-existing uncommitted changes from the earlier `fullstack-developer-260831-2226`
fix pass, not touched by this task.
## Unresolved questions
None.
Status: DONE
Summary: Added API-31+ and pre-31 backup-exclusion rules for the game-state DataStore only (settings stay backed up), and closed the M4 residual race with a `SettingsRepository.loaded`/`LotoApplication.settingsLoaded` signal that gates `PlayerBoardViewModel`'s master-emission consumption until settings have really resolved; new regression test verified to fail without the fix. `:app:testDebugUnitTest` and `:app:lintDebug` both green.
Concerns/Blockers: none.
plans/reports/fullstack-developer-260831-2314-web-multitab.md
+115
View File
@@ -0,0 +1,115 @@
# Web M5 fix: simultaneous-tab double-freeze
Plan: `plans/260831-2314-review-followups/plan.md` Phase 1.
Source: `plans/reports/code-reviewer-260831-2213-web-review.md` M5.
## Change
`web/src/lib/active-tab.svelte.js`:
- Added module-scoped `myClaimTs` (last `Date.now()` this tab claimed) and a
`postClaim()` helper that sets it and broadcasts `{type:"claim", id, ts}`.
Called from both `watchActiveTab()` (mount) and `claimActiveTab()`
(reclaim) — same call sites as the old unconditional `postMessage`.
- `onmessage` now applies newest-claim-wins: ignore `peerTs < myClaimTs`;
freeze if `peerTs > myClaimTs`; on exact tie, freeze only if peer `id` is
lexicographically greater than `TAB_ID` (deterministic, exactly one tab
wins). Reclaim's fresh `myClaimTs` means a stale/late peer claim can no
longer re-freeze the reclaiming tab.
- `+layout.svelte` untouched — `watchActiveTab()`/`claimActiveTab()` keep
their signatures, reclaim re-hydration flow unchanged.
`web/src/lib/active-tab.test.js`:
- `FakeBC.postMessage` now defers delivery via `queueMicrotask` and
re-reads the peer list at delivery time instead of snapshotting it at
call time. This matches real `BroadcastChannel`'s queued-task delivery
and — critically — is what makes a true "both tabs mount in the same
tick" test possible: with the old synchronous/snapshot delivery, a tab
created after another tab's `postMessage` call could never receive that
earlier message, making one side of any tie-break untestable (and, per
spec, unfaithful to the real API). Added `deliverBcMessages()` helper
(`await Promise.resolve()`) to flush it in tests.
- `beforeEach` now stubs `Date.now` with a strictly-increasing counter by
default (removes flakiness/ambiguity in the pre-existing tests, which
don't care about specific ts values) and restores mocks in `afterEach`.
- Added three tests: unequal-ts simultaneous mount (older freezes, newer
stays active), equal-ts tie-break (both tabs receive each other's claim
via the same-tick delivery fix above; exactly one — the lexicographically
greater id — stays active), and reclaim-updates-myClaimTs (a stale,
older-ts claim from a peer after reclaim doesn't re-freeze).
- Existing 4 tests updated only to `await deliverBcMessages()` after
actions that need cross-tab delivery; assertions/intent unchanged.
## Verification (from `web/`)
- `npx vitest run` → 141/141 passed (10 files), incl. 7/7 in
`active-tab.test.js`.
- `npx eslint .` → 0 problems.
- `npx svelte-check --tsconfig ./jsconfig.json` → 0 errors, 0 warnings
(429 files, `checkJs: true`).
Builds skipped per instruction (unaffected — no import/signature changes).
## Unresolved questions
None — decision and algorithm were fully specified; no ambiguity hit
during implementation.
Status: DONE
Summary: Implemented newest-claim-wins with id tie-break in `active-tab.svelte.js`; extended the test file (and made its `BroadcastChannel` fake deliver messages the way the real API does) to cover unequal-ts, equal-ts tie-break, and reclaim-clears-staleness; all gates green.
Concerns/Blockers: none
## M-1 follow-up — losing claimer never told it lost (2026-08-31)
Source: `plans/reports/code-reviewer-260831-2314-precommit-review.md` M-1.
### Change
`web/src/lib/active-tab.svelte.js` `onmessage` handler:
- `peerTs` now falls back to `Infinity` when `e.data.ts` is not a number
(legacy pre-fix peer broadcasting `{type, id}` only) — a no-ts claim
always wins, matching the old always-freeze behavior instead of being
silently ignored (`peerTs > myClaimTs` / `=== ` were both `false` against
`undefined`).
- Added an `else` branch: when this tab does **not** lose (`!peerWins`) and
is currently active, it echoes its own unchanged `{type:"claim", id:
TAB_ID, ts: myClaimTs}` back on the channel. The peer that sent the
losing claim receives this echo, re-evaluates, strictly loses (same
`myClaimTs` it already lost against, or a higher `ts`/`id` under the
`Infinity` legacy path), and freezes. A frozen tab only ever hits the
`peerWins` branch, so it never echoes — the exchange terminates after one
echo round by construction, no counter/dedup needed.
No public API change — `watchActiveTab()`/`claimActiveTab()` signatures
and `activeTab` shape untouched.
`web/src/lib/active-tab.test.js`:
- Updated "reclaim updates myClaimTs..." — a stale claim from B now also
asserts `tabB.activeTab.inactive === true` after an extra
`deliverBcMessages()` flush (previously only checked A, per M-1 finding
#1).
- Added "a losing claimer is echoed at and freezes instead of leaving both
tabs active" — reproduces the asymmetric-delivery race (tabA's initial
claim is flushed with zero peers before tabB even mounts, so tabB never
receives it, matching real `BroadcastChannel` non-replay), equal ts, id
tie-break; asserts exactly one tab ends up active.
- Added "the echo does not loop" — spies on `FakeBC.prototype.postMessage`,
asserts message count settles at exactly 3 (two initial claims + one
echo) and stays there across further flushes.
- Added "a peer claim with no ts (legacy tab) always wins" — posts a raw
`{type:"claim", id}` (no `ts`) from a second `BroadcastChannel` instance
directly and asserts the tab freezes.
### Verification (from `web/`)
- `npx vitest run` → 144/144 passed (10 files), incl. 10/10 in
`active-tab.test.js`.
- `npx eslint .` → 0 problems.
- `npx svelte-check --tsconfig ./jsconfig.json` → 0 errors, 0 warnings
(429 files).
Files touched: `web/src/lib/active-tab.svelte.js`,
`web/src/lib/active-tab.test.js` only, per assignment. Not committed.
Status: DONE
Summary: Fixed M-1 by echoing a winning claim back at a losing peer (so it freezes instead of silently ignoring the loss) and treating a missing peer `ts` as `Infinity` (legacy peer always wins); added echo, no-loop, and legacy-no-ts test coverage; all gates green.
Concerns/Blockers: none
plans/reports/fullstack-developer-260831-2334-android-regression-fixes.md
+158
View File
@@ -0,0 +1,158 @@
# Android regression fixes — H1, M2, M3, M4, backup .tmp, generate() double-tap guard
Source: `plans/reports/code-reviewer-260831-2314-precommit-review.md`
Scope: `android/` only. No commit made.
## H-1 — voicePlayer.release() kills audio on relaunch-in-process
Chose the **recreatable holder**, not the `cancel()` fallback: `cancel()` would
fully abandon L1's intent (never releasing the ExoPlayer on explicit exit,
reverting to pre-L1 behavior). The holder keeps L1's actual release on
"Thoát" while surviving a relaunch into the same cached process.
- New `android/app/src/main/java/com/miti99/loto/audio/VoicePlayerHolder.kt`:
nullable-instance holder, `value` rebuilds via a factory after `release()`.
- `LotoApplication.kt`: `voicePlayer` now backed by a private
`VoicePlayerHolder`; added `releaseVoicePlayer()`.
- `MainActivity.kt`: `onDestroy()` calls `releaseVoicePlayer()` instead of
`voicePlayer.release()`.
- Testability: `LotoApplication`/`ExoVoicePlayer` need a real `Context`
(no Robolectric in this project — `testOptions.unitTests` only sets
`isReturnDefaultValues`), so the recreate-after-release contract is
extracted into `VoicePlayerHolder` and unit-tested in isolation
(`VoicePlayerHolderTest.kt`, 4 tests) against `FakeVoicePlayer`. The
`LotoApplication`/`MainActivity` wiring itself stays unverified by a JVM
test, same residual gap the reviewer already flagged — out of scope to
close without adding Robolectric.
## M-2 — settings/loaded sibling-collector race
Replaced the two independent `stateIn` collectors PlayerBoardViewModel
depended on (`settingsState: StateFlow<Settings>` + a separate loaded
boolean) with one: `LotoApplication.settingsOrNull: StateFlow<Settings?>`
(null = not loaded), the sole collector over `settingsRepository.settingsFlow`.
`settingsState` (used by `MainActivity`, `MasterPanelViewModel`,
`SettingsViewModel`, `SettingsSheetTest`) is now *derived* from
`settingsOrNull` rather than an independent collection, removing the
top-level race for everyone, not just the reported call site.
`PlayerBoardViewModel` now takes `settingsOrNull` directly and reads
`settingsOrNull.value` for both "is it loaded" and "what is the mode" —
by construction the same snapshot, so they can never disagree.
`LotoViewModelFactory` updated to wire `app.settingsOrNull` (+
`app.settingsRepository.defaults` as the pre-load fallback for the rare
`onCellClick`-on-a-restored-card-before-settings-load path).
## M-3 — generate()/clearMarks() bypass the settings gate
- `PlayerUiState.loading` now folds in settings-loaded, not just restore:
`publishLoading() = !restoreDone || settingsOrNull.value == null`,
called from `restore()` and from the `init` combine's collector (which now
combines `masterStore.state` with `settingsOrNull` directly — settings
changes still re-trigger `onMasterState`, same as before). The existing UI
gate (`state.loading` disables both buttons) now also covers the
settings-unresolved window.
- Defense in depth, mirroring `restore()`'s own H1 guard: `generate()` and
`clearMarks()` early-return (no-op) while `settingsOrNull.value == null`,
so a caller that acts before the UI gate resolves can't corrupt the
auto-cross cursor under a placeholder mode.
- Updated the M4-residual test in `PlayerBoardViewModelTest.kt` to actually
exercise the ordering the review said it dodged: `masterStore.restore()`
now resolves *before* `generate()` is tapped (previously the reverse),
asserting the tap while gated is a no-op and the full replay lands once
settings resolve and the user taps again. The other ordering test
("a settings arrival that lands before masterStore's restore...") is
unchanged/kept.
- New: `clearMarks() is also a no-op while settings have not resolved yet`
and `loading stays true while settings have not resolved... (M3)`.
## M-4 — non-IO settings read failure deadlocks the gate
`SettingsRepository.settingsFlow`'s `catch` now falls back to
`emptyPreferences()` (→ defaults) for **any** read failure, not only
`IOException` — the fail-open fix applies at the source now that
`LotoApplication.settingsOrNull` is the sole downstream collector (a fix
scoped only to the separate `loaded: Flow<Boolean>`, as the review's
snippet suggested, would no longer reach the actual production consumer
after the M2 unification). `loaded` is kept as a plain derived signal for
tests/other consumers; it no longer needs its own `.catch` since upstream
never throws.
`ThrowingDataStore` gained an optional `exception: Throwable` constructor
param (defaults to `IOException`, unchanged for existing callers) so tests
can exercise a non-IO failure. Added
`non-IO failure on read still falls back to defaults...` and
`loaded still flips true after a non-IO read failure` to
`SettingsRepositoryTest.kt`.
## Low — backup `.tmp` sibling
Added `datastore/loto_game_state.preferences_pb.tmp` exclusion alongside the
existing `.preferences_pb` one in both `data_extraction_rules.xml` and
`backup_rules.xml`.
## Low — generate() double-tap guard
Added `private var generating` to `PlayerBoardViewModel`: `generate()` no-ops
while a previous call's off-main compute hasn't resolved yet; also folded
into `restore()`'s clobber guard (`!generating`) per the review's "participates
in the restore guard" suggestion, since `grid` alone can't detect an
in-flight generation. Test: `a second rapid generate() tap while the first
is still computing is a no-op (L-a)`, asserting `FakeVoicePlayer.cancel()`
(new `cancelCount` counter) fires only once.
## Files Modified
- `android/app/src/main/java/com/miti99/loto/audio/VoicePlayerHolder.kt` (new)
- `android/app/src/main/java/com/miti99/loto/LotoApplication.kt`
- `android/app/src/main/java/com/miti99/loto/MainActivity.kt`
- `android/app/src/main/java/com/miti99/loto/settings/SettingsRepository.kt`
- `android/app/src/main/java/com/miti99/loto/state/LotoViewModelFactory.kt`
- `android/app/src/main/java/com/miti99/loto/state/PlayerBoardViewModel.kt`
- `android/app/src/main/res/xml/data_extraction_rules.xml`
- `android/app/src/main/res/xml/backup_rules.xml`
- `android/app/src/test/java/com/miti99/loto/audio/VoicePlayerHolderTest.kt` (new)
- `android/app/src/test/java/com/miti99/loto/audio/FakeVoicePlayer.kt`
- `android/app/src/test/java/com/miti99/loto/ThrowingDataStore.kt`
- `android/app/src/test/java/com/miti99/loto/settings/SettingsRepositoryTest.kt`
- `android/app/src/test/java/com/miti99/loto/state/PlayerBoardViewModelTest.kt`
Untouched by me but pre-existing modified/untracked in the working tree
(part of the earlier fix cycle under review, not this task): `build.gradle.kts`,
`AndroidManifest.xml`, `GameStateRepository.kt`, `GameStateRepositoryTest.kt`,
`MasterPanelViewModel.kt`, `MasterPanelViewModelTest.kt`, `MasterStore.kt`,
`MasterStoreTest.kt`, `SlowDataStore.kt`, various `ui/*` files, `strings.xml`.
## Tests Status
- `JAVA_HOME=jdk-21 ./gradlew :app:testDebugUnitTest` — **BUILD SUCCESSFUL**,
129 tests, 0 failures, 0 errors (was 120; +9 new: 4 `VoicePlayerHolderTest`,
3 `PlayerBoardViewModelTest`, 2 `SettingsRepositoryTest`).
- `JAVA_HOME=jdk-21 ./gradlew :app:lintDebug` — **BUILD SUCCESSFUL**, 0
errors, 17 warnings (all pre-existing: dependency-version notices, one
Vietnamese string typo heuristic, two plurals-candidate heuristics,
`enableOnBackInvokedCallback` API-33 note — none touch the changed files).
## Issues Encountered
None. No file-ownership conflicts (single-agent task). `local.properties`
was not read.
## Unresolved Questions
1. H-1: the `LotoApplication`/`MainActivity` glue around `VoicePlayerHolder`
(real `finish()` + relaunch through a real `Application`/`Context`) is
still not exercised by any automated test — this project has no
Robolectric dependency, and adding one was out of scope. The holder's
own recreate-after-release contract is unit-tested; the wiring is
equivalent to what the reviewer already called out as untested.
2. `fallbackSettings` for `PlayerBoardViewModel` (used only if `onCellClick`
fires on a restored card before settings ever resolve) is wired to the
real manifest-derived defaults in production
(`app.settingsRepository.defaults`) and to a placeholder
`Settings(voice = "")` in the constructor's own default parameter (never
hit by existing tests, all of which pass a non-null-typed settings flow).
Flagging this only so the placeholder default doesn't surprise a future
reader who instantiates `PlayerBoardViewModel` with a null-capable flow
without supplying `fallbackSettings`.
Status: DONE
Summary: Fixed H1 (recreatable voice player holder), M2 (single settingsOrNull source of truth), M3 (settings-loaded folded into loading + generate()/clearMarks() guards, test reordered to hit the previously-dodged case), M4 (fail-open on any settingsFlow read failure), plus the backup .tmp exclusion and a generate() double-tap/in-flight guard; all 129 unit tests and lint pass clean.
plans/reports/tester-260831-2314-full-verify.md
+96
View File
@@ -0,0 +1,96 @@
# Full Verification Report — Uncommitted Changes
**Date:** 2026-08-31 23:24
**Scope:** All uncommitted changes in web/ and android/
---
## Web Verification Results
### 1. Unit Tests (vitest)
- **Command:** `npx vitest run`
- **Status:** ✓ PASS
- **Result:** 141 tests passed, 0 failed
- Test files: 10
- Duration: 2.53s
### 2. Linting (eslint)
- **Command:** `npx eslint .`
- **Status:** ✓ PASS
- **Errors:** 0
### 3. Type Checking (svelte-check)
- **Command:** `npx svelte-check --tsconfig ./jsconfig.json`
- **Status:** ✓ PASS
- **Checked:** 429 files
- **Errors:** 0
- **Warnings:** 0
### 4. Build (BUILD_PROFILE=gh)
- **Command:** `BUILD_PROFILE=gh npx vite build`
- **Status:** ✓ PASS
- **Duration:** 2.20s
- **PWA Build Check:** ✓ PASS (base "/loto", 13 client JS files, 92 precached audio entries)
### 5. Build (default)
- **Command:** `npx vite build`
- **Status:** ✓ PASS
- **Duration:** 2.18s
- **PWA Build Check:** ✓ PASS (base "", 13 client JS files, 92 precached audio entries)
---
## Android Verification Results
### 1. Unit Tests (testDebugUnitTest)
- **Command:** `JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:testDebugUnitTest`
- **Status:** ✓ PASS
- **Result:** 120 tests passed, 0 failed
- Packages: 6
- Test classes: 12
- Duration: 4.629s
- Success rate: 100%
**Test Breakdown by Package:**
- com.miti99.loto.audio: 13 tests (0.032s)
- com.miti99.loto.game: 39 tests (4.240s)
- com.miti99.loto.settings: 19 tests (0.125s)
- com.miti99.loto.state: 41 tests (0.231s)
- com.miti99.loto.ui: 3 tests (0s)
- com.miti99.loto.ui.master: 5 tests (0.001s)
### 2. Lint Check (lintDebug)
- **Command:** `JAVA_HOME="C:/Program Files/Eclipse Adoptium/jdk-21.0.10.7-hotspot" ./gradlew :app:lintDebug`
- **Status:** ✓ PASS
- **Build Result:** BUILD SUCCESSFUL in 7s
- **Errors:** 0
- **Warnings:** 14 (all non-blocking)
**Warning Summary:**
- 1 API level compatibility warning (enableOnBackInvokedCallback @ API 33+)
- 10 dependency version upgrade suggestions (AGP, androidx libs, Kotlin)
- 2 plurals candidate warnings (Vietnamese string formatting)
- 1 typo detection (cosmetic)
---
## Summary
| Component | Status | Details |
|-----------|--------|---------|
| **Web Tests** | ✅ PASS | 141/141 tests |
| **Web Lint** | ✅ PASS | 0 errors |
| **Web Types** | ✅ PASS | 0 errors (429 files checked) |
| **Web Build (gh)** | ✅ PASS | PWA verified |
| **Web Build (default)** | ✅ PASS | PWA verified |
| **Android Tests** | ✅ PASS | 120/120 tests |
| **Android Lint** | ✅ PASS | 0 errors (14 warnings only) |
**Total:** 261 unit tests passing (141 web + 120 android)
**Build Status:** Both web and android build successfully
**Code Quality:** No errors, no blocking issues
---
Status: DONE
Summary: All verification checks passed — 141 web tests, 120 android tests, builds succeed, no errors detected.
Concerns/Blockers: None