mirror of
https://github.com/tiennm99/loto.git
synced 2026-10-03 09:13:34 +00:00
fix: address pass-2 review findings (PWA + CSP + copy)
P0:
- CSP `script-src` was 'self' only, but SvelteKit's static export
emits a small inline bootstrap script. Without 'unsafe-inline' the
entire app silently fails under Cloudflare Pages CSP enforcement.
Verified by inspecting the built index.html.
- manifest `background_color` was the dark base (#0a0f1f); for the
~50% of users on light mode that gave a dark splash flash on every
install/launch. Switch to #f8fafc to match the default light theme.
- <title> bare "Lô tô" mismatched manifest name "Lô tô — Hội chợ TN1";
align both to the same string so OS install prompt + browser tab
match.
Medium:
- Audio runtime cache `cacheableResponse.statuses` was [0, 200].
Audio is same-origin, so opaque (0) responses can never legitimately
appear; tightening to [200] removes a CDN-poisoning replay window.
- Voice hint copy: "Đọc số đã xổ + báo Chờ/Kinh khi ở Cả hai" was
shown in master-only mode too, where the hint is wrong (no player
board → no Chờ/Kinh). Split copy per mode.
Cosmetic:
- Drop `includeAssets: ["icons/*.png", "audio/**/*.mp3"]` — both are
already in static/, so the option was a no-op.
- Replace `defaultVoiceId` fallback `"hoai-my"` with a hard read; the
manifest is committed and authoritative — duplicate fallbacks just
invite drift if the manifest ever rotates.
Verified: npm test 115/115; npm run build clean (305 precache entries,
no glob warnings); npm audit 0 vulnerabilities.
Reports: plans/reports/{code-reviewer,ui-ux-designer,security}-260427-2047-pass2-full.md
This commit is contained in:
1 parent
d94294d83b
commit
ee71bf041d
8 files changed
+301
-12
No files matched your search
@@ -0,0 +1,83 @@
|
||||
# Code Review — Pass 2 (full project, post-PWA)
|
||||
|
||||
**Scope:** `src/**`, `vite.config.js`, `static/_headers`, `static/manifest.webmanifest`, `package.json`.
|
||||
**Baseline:** prior pass at `f28279b`. Reviewed commits: `ad6291e`, `f7db20c`, `d94294d`.
|
||||
**Verdict:** clean. Prior P0/P1 findings landed correctly. No new P0. Two P1 around PWA/CSP. A few P2 nits.
|
||||
|
||||
---
|
||||
|
||||
## Prior fixes — landing check
|
||||
|
||||
- Auto-tick re-mark guard: `PlayerBoard.svelte:45,157-171` — `lastHandledDrawAt` compared against `bus.lastDrawn.at`. Manual untick / clear / regen no longer re-fires (re-runs read same `at`, early return). Correct.
|
||||
- Toast positioning: `PlayerBoard.svelte:329-340` — anchored to grid container with `-top-3 sm:-top-4`, `pointer-events-none` wrapper, button-only `pointer-events-auto`. Correct.
|
||||
- Modal Escape: window-level `keydown` in `PlayerBoard.svelte:142-150` and `SettingsButton.svelte:113-121`. Correct.
|
||||
- Master empty state: extracted `MasterEmptyState.svelte`, used at `MasterPanel.svelte:348`. Correct.
|
||||
- Biased shuffle replaced by Fisher-Yates: `game-logic.js:32-35`, `MasterPanel.svelte:35-38`. Correct.
|
||||
- Reduced-motion gates: `MasterPanel.svelte:145-153` (scroll), `PlayerBoard.svelte:18-23` (vibrate), `app.css:219-232` (animations). Correct.
|
||||
- Storage payload caps + `__proto__`/`constructor` reviver: `game-logic.js:166-189`, `MasterPanel.svelte:51-75`, `settings-store.svelte.js:127-132`. Correct.
|
||||
- `encodeURIComponent` on voice URL: `voice.js:41`. Correct.
|
||||
- Security headers: `static/_headers` present with strict CSP, COOP-equivalent (`frame-ancestors 'none'`), nosniff, etc.
|
||||
|
||||
---
|
||||
|
||||
## P1 (action recommended pre-merge)
|
||||
|
||||
**1. CSP ↔ inline SW registration race / failure mode** — `static/_headers:2`
|
||||
CSP has `script-src 'self'` (no `'unsafe-inline'`, no nonce). `@vite-pwa/sveltekit` with `registerType: "autoUpdate"` injects an inline `<script>` registering `/sw.js` into the prerendered `index.html`. On a strict CSP host (Cloudflare Pages honors `_headers`), that inline registration block will be blocked → no PWA install. Two fixes: (a) switch to virtual `import { registerSW } from 'virtual:pwa-register'` from a real module, OR (b) configure plugin `injectRegister: 'script-defer'` with a hashed/external file. Verify post-build that `build/index.html` does NOT contain inline registration; if it does, this is silently broken in production.
|
||||
|
||||
**2. `static/_headers` does not cover `/sw.js` MIME** — `static/_headers:9-10`
|
||||
Only `Cache-Control: no-cache` is set on `/sw.js`. Cloudflare Pages will infer `application/javascript` from extension, but spec-strict registrars reject if `Content-Type` isn't `text/javascript`/`application/javascript`. Low-risk on CF, but add explicit `Content-Type: application/javascript` to be safe alongside the manifest entry already doing so.
|
||||
|
||||
---
|
||||
|
||||
## P2 (nits / tech debt)
|
||||
|
||||
- `vite.config.js:13` — `defaultVoiceId = audioManifest.voices[0]?.id ?? "hoai-my"`. Hardcoded fallback drifts from `audio-manifest.js:23`. Either move to a shared `scripts/audio-default.js` import or assert at config-eval time. Low likelihood of mismatch but easy to lose on a manifest rewrite.
|
||||
- `vite.config.js:23` — `revision: 'audio-v1-{voice}-{n}'` is a manual cache-buster. Comment ("Bump the prefix when audio is regenerated") relies on humans. Consider hashing file content (`createHash('sha1', readFileSync(path))`) so audio regen invalidates automatically.
|
||||
- `vite.config.js:46` — `includeAssets: ["icons/*.png", "audio/**/*.mp3"]` lists ALL voice mp3s, but `globPatterns` (line 48) does NOT include `mp3`. Result: `includeAssets` only copies them into the build (already happens via `static/`); the actual precache list is `globPatterns ∪ additionalManifestEntries`. Default voice is precached via `additionalManifestEntries`; alt voices fall through runtime CacheFirst as documented. Behavior is correct — but `includeAssets` is dead config noise, drop it or document its no-op role.
|
||||
- `manifest.webmanifest:5-6` — `start_url: "."`, `scope: "."`. Under base path `/loto/`, browsers resolve relative to manifest URL, so this works on GH Pages. But Cloudflare and GH share the same file. If you ever add a non-root deploy without rewriting the manifest, scope drift will silently break PWA scope detection. Consider `%sveltekit.assets%`-templated manifest emitted at build time, or explicit `start_url: "/loto/"` + a CF-only override.
|
||||
- `MasterPanel.svelte:107-109` — `callOrder` rebuilds the whole `Map` on every state change; fine at 90 entries. No action.
|
||||
- `MasterPanel.svelte:90` — `heroEl` typed `HTMLDivElement | null` but `bind:this` runs at every render. Using `$state` here is correct in Svelte 5; OK.
|
||||
- `PlayerBoard.svelte:48-52` — `rowCompleteness` derived; uses `grid.map((_, r) => isRowComplete(grid, crossed, r))`. Reads both reactively — fires on every cell toggle (9 calls). Acceptable.
|
||||
- `MasterPanel.svelte:166-177` `handleDrawNext` does not check `settings.mode` before `broadcastDraw`; player auto-tick effect already gates on `settings.mode === "both"` (`PlayerBoard.svelte:162`), so harmless. But broadcasting in `master`-only mode is wasted work and pollutes the bus across mode flips. Suggest gating, OR documenting why it's intentional (so resuming from "both" → "master" → "both" mid-game works).
|
||||
- `call-bus.svelte.js:1-22` — JSDoc accurate. `broadcastDraw` lacks `@returns`, `resetBus` lacks docstring; trivial.
|
||||
- `game-logic.js:277-288` — `findUncrossedCell` JSDoc accurate. Top-down/left-right scan order matches test expectation.
|
||||
- Stale comment risk: `MasterPanel.svelte:51-53` says "16 KB has 30× headroom" — true after the cap landed; keep.
|
||||
- Dead/duplicate `isBrowser` check in `voice.js:54` (`cancelPlayback`) — fine, but `if (!isBrowser()) return;` is unreachable in test path; cosmetic.
|
||||
|
||||
---
|
||||
|
||||
## Test-coverage gaps
|
||||
|
||||
- No test for the `PlayerBoard` auto-tick effect (mode flip, dedup-by-`at`, manual-untick re-mark prevention). The behavior is the highest-risk new code path. Add one component test: drive `bus.lastDrawn`, assert `crossed[r][c]` flips once, untick by hand, broadcast same `at` → does NOT re-mark. Recommended.
|
||||
- No test for `MasterPanel.handleDrawNext` → `broadcastDraw` linkage; bus contract tested in isolation only.
|
||||
- No SW/Workbox integration test — out of scope for vitest, but a `npm run build:gh && grep -r 'sw.js' build/index.html` smoke check in CI would catch P1 #1.
|
||||
- `voice.test.js` does not cover the `cho → number` cancel-mid-chain case (cancel between the two awaited `playClip` calls). The token mismatch path resolves cleanly; one test would lock it in.
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- CSP unchanged, still strict. PWA SW served same-origin; runtime caching only matches `/audio/*.mp3` regex, no third-party cacheing. `manifest.webmanifest` is plain JSON, no scripts. Icons are local PNGs. New attack surface: SW lifecycle. `registerType: "autoUpdate"` + comment on line 38-39 ("Do NOT add `skipWaiting`") is correct — stale clients keep working until tab close.
|
||||
- `npm overrides` for `serialize-javascript@^7.0.5` and `cookie@^0.7.2` are dev-only build-chain transitive vulns. Lockfile is the source of truth — confirm `npm ls serialize-javascript cookie` shows resolved 7.x/0.7.x post-`npm install`.
|
||||
- No PII, no telemetry, no remote endpoints.
|
||||
|
||||
---
|
||||
|
||||
## Positive
|
||||
|
||||
- `lastHandledDrawAt` design (closed-over plain ref, not `$state`) is exactly right — avoids the auto-tick effect re-triggering itself.
|
||||
- Two-pass `$effect` on bingo + waiting in `PlayerBoard.svelte:97-131` reads cleanly; comment on "at most one bingo popup per render" matches code.
|
||||
- JSDoc + JSDoc-via-`/** @type */` casts give meaningful type narrowing without TS toolchain.
|
||||
- Test file naming and `// @vitest-environment happy-dom` annotation per file is consistent.
|
||||
|
||||
---
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. Is the PWA install actually working in production (Cloudflare Pages) under the strict CSP, or has nobody tested install + reload offline? See P1 #1.
|
||||
2. Is `BUILD_PROFILE=gh` (`/loto/` base) deployed anywhere live? If not, `manifest.webmanifest`'s relative `start_url` is untested at non-root scope.
|
||||
3. Should `MasterPanel.handleDrawNext` skip `broadcastDraw` when `settings.mode !== "both"`, or is the cross-mode bus intentional for future "Cả hai" toggling mid-game?
|
||||
|
||||
**Status:** DONE
|
||||
**Summary:** Prior P0/P1 fixes landed cleanly. Two P1 items around PWA SW registration vs strict CSP need a build-output check before next deploy. P2s are minor.
|
||||
@@ -0,0 +1,73 @@
|
||||
# Security & Reliability Audit — Lô tô (Pass 2)
|
||||
|
||||
Date: 2026-04-27 | Scope: SvelteKit static export + new PWA layer (since `f28279b`).
|
||||
Method: STRIDE + OWASP Top 10, manual.
|
||||
Files scanned: `src/**`, `static/_headers`, `static/manifest.webmanifest`, `vite.config.js`, `package.json`, `npm audit`.
|
||||
|
||||
## Summary
|
||||
- 0 Critical, 0 High, **2 Medium, 4 Low, 5 Informational**.
|
||||
- All 4 pass-1 mediums (M1 CSP, M2 proto-pollution, M3 payload cap, L1 encodeURIComponent) **resolved**.
|
||||
- `npm audit` clean (overrides verified: `cookie@0.7.2`, `serialize-javascript@7.0.5` — no API breakage; both transitive-only at build time).
|
||||
- No outbound network calls; no `{@html}`, `eval`, `fetch`, `XHR`, `WebSocket` anywhere in `src/`.
|
||||
|
||||
## Findings
|
||||
|
||||
### Medium
|
||||
|
||||
**M1 — CSP gap: SW-update / workbox / manifest fetches not explicitly covered, and `style-src 'unsafe-inline'` likely still required.**
|
||||
`static/_headers:2`. `connect-src 'self'` covers SW `update()` GETs, OK. But: (a) workbox's generated `sw.js` registers via `navigator.serviceWorker.register('/sw.js')` — covered by `worker-src 'self'`. (b) The plugin emits an inline `<script>` registration block in `app.html` injected at build; check post-build `index.html` for inline `<script>` — if present, current `script-src 'self'` will block it. `script-src` lacks `'unsafe-inline'` AND no nonce/hash declared (header docstring says "nonce for scripts" but no `'nonce-…'` token in the policy string). Verify built output. (c) `'unsafe-inline'` for style: confirmed needed — `style:` directives at `MasterPanel.svelte:309`, `PlayerBoard.svelte:282,356,384-386`, `SettingsButton.svelte:418`, `MasterEmptyState.svelte:18` all compile to inline `style=` attributes. Nonces don't help inline attributes; CSP3 `'unsafe-hashes'` does but isn't widely supported. Keep `'unsafe-inline'` for style. (file: `static/_headers:2`)
|
||||
|
||||
**M2 — Service-worker precache integrity not protected by SRI; CacheFirst stores opaque (status 0) responses.**
|
||||
`vite.config.js:54-67`. `cacheableResponse: { statuses: [0, 200] }` is **required** for CDN-served audio if range requests strip CORS, but it also means a poisoned CDN response is cached as opaque and served forever (CacheFirst). Risk vector: hijacked Cloudflare edge OR rogue CF Pages preview deploy serves a malicious mp3; SW caches and replays for the 30-day TTL. Mitigations available: (a) drop status `0` (audio is same-origin via `${base}/audio/...`, so 200 is sufficient — no opaque needed); (b) bump cache name on each redeploy via revision (already done for precache via `audio-v1-...`, but **not for runtime cache**). Recommend `cacheableResponse: { statuses: [200] }`. Same-origin `<audio>` does not need opaque mode. (file: `vite.config.js:65`)
|
||||
|
||||
### Low
|
||||
|
||||
**L1 — Manifest `start_url: "."` resolves relative; subpath deploys could mislead PWA install scope.**
|
||||
`static/manifest.webmanifest:5-6`. Both GH-Pages (`/loto/`) and CF root use `"."` which resolves at the manifest URL. Acceptable, but if `_headers` ever serves `manifest.webmanifest` from a non-root path with redirect, scope can drift. Defense-in-depth: pin `start_url` to absolute path per build profile, or `"./"` (trailing slash) for clarity. Cache-Control `no-cache` (line 13 of `_headers`) is correct. (file: `static/manifest.webmanifest:5`)
|
||||
|
||||
**L2 — Audio runtime cache `maxAgeSeconds: 30 days` with no integrity revision.**
|
||||
`vite.config.js:60-64`. Precache entries get `revision: audio-v1-...`. Runtime cache (alternate voices) has no revision → if a voice clip is regenerated, clients hold the stale 30-day copy until natural eviction. Functional issue (stale audio), low security impact. Bump prefix → also bump runtime `cacheName` in same release to force purge. (file: `vite.config.js:59`)
|
||||
|
||||
**L3 — Reactive bus is module-level singleton — fine for same-tab single user, but BroadcastChannel-style leakage if SW ever cross-posts.**
|
||||
`src/lib/call-bus.svelte.js:10-13`. `bus` lives in JS module memory, scoped per tab/window — no cross-tab leak. SW does not import it (SW runs in separate context). PASS for current arch. Risk would only appear if a future feature uses `BroadcastChannel`/`postMessage` to mirror draws; document that bus is intentionally tab-local. (file: `src/lib/call-bus.svelte.js`)
|
||||
|
||||
**L4 — `MasterPanel.loadState` minimal validator accepts any array shape.**
|
||||
`MasterPanel.svelte:55-75`. Reviver strips `__proto__`/`constructor`, length cap 16 KB, `Array.isArray` on both halves — but elements are not type-checked. Poisoned origin could store `{called: ["💀"], remaining: [{}]}` → `state.called[i]` rendered into DOM at line 288 as `{num}` (Svelte text-interpolation auto-escapes, so no XSS), but `callOrder.get(num)` and number comparisons silently produce NaN/`undefined`. Ugly UI, no security breach. Add `n => typeof n === "number" && n >= 1 && n <= 90` per element if hardening further. (file: `src/lib/MasterPanel.svelte:64-69`)
|
||||
|
||||
### Informational
|
||||
|
||||
**I1 — `'unsafe-inline'` for style is unavoidable today.** Svelte's `style:` compiles to inline attributes. Hash-based or nonce-based style CSP would require Svelte-side opt-out + extracting all dynamic styles to CSS variables (already done for `--empty-cell-bg`; not done for confetti, master cell bg toggle).
|
||||
|
||||
**I2 — `frame-ancestors 'none' + X-Frame-Options: DENY`**: belt-and-braces, OK.
|
||||
|
||||
**I3 — `manifest-src 'self'` correctly added.** Required by Chrome since 2020. PASS.
|
||||
|
||||
**I4 — Self-hosted font (`@fontsource/roboto-condensed`)** removes Google Fonts CDN dependency → `font-src 'self' data:` is sufficient and tight. PASS.
|
||||
|
||||
**I5 — npm overrides verified non-breaking.** `npm ls cookie serialize-javascript` resolves cleanly to `0.7.2` and `7.0.5`; no peer-dep warnings; build artifacts unchanged. Both packages are build-time only (kit dev internals + workbox-build via @rollup/plugin-terser) — runtime never executes them. PASS.
|
||||
|
||||
## Trust-boundary verification (re-confirmed)
|
||||
- localStorage payload caps applied to **all 4 keys**: `loto_settings` (8KB, `settings-store.svelte.js:17,127`), `loto_grid` + `loto_crossed` (32KB, `game-logic.js:169,180`), `loto_master` (16KB, `MasterPanel.svelte:53,58`). PASS.
|
||||
- `__proto__`/`constructor` reviver applied in **all 3 parse sites**: `settings-store.svelte.js:130`, `game-logic.js:182`, `MasterPanel.svelte:59`. PASS.
|
||||
- `clipUrl` `encodeURIComponent` belt-and-braces on both `voice` + `name` (`voice.js:41`). PASS.
|
||||
- Voice allowlist `VOICE_IDS.has(v)` (`settings-store.svelte.js:71`). PASS.
|
||||
- CSP `frame-ancestors 'none'` blocks clickjacking. PASS.
|
||||
- SW: `registerType: "autoUpdate"` without `skipWaiting` (`vite.config.js:40`) — explicit comment confirms intent. PASS.
|
||||
- `noopener noreferrer` on all external `<a target="_blank">`. PASS.
|
||||
|
||||
## Pass-1 follow-ups status
|
||||
| ID | Item | Status |
|
||||
|----|------|--------|
|
||||
| M1 (pass1) | CSP added | RESOLVED |
|
||||
| M2 (pass1) | proto/constructor stripping | RESOLVED (3 sites) |
|
||||
| M3 (pass1) | localStorage payload caps | RESOLVED (4 keys) |
|
||||
| L1 (pass1) | encodeURIComponent on clipUrl | RESOLVED |
|
||||
| L2 (pass1) | Audio cache LRU | DEFERRED — acceptable while voice count ≤ ~5; SW cache now also caps at 400 entries |
|
||||
| L3 (pass1) | crypto.getRandomValues | DEFERRED — not security-relevant |
|
||||
| L4 (pass1) | cookie <0.7.0 | RESOLVED via override |
|
||||
|
||||
## Unresolved questions
|
||||
1. Inspect post-build `build/index.html` — does workbox/SvelteKit-PWA inject any inline `<script>` for SW registration? If yes, `script-src 'self'` blocks it (M1c). Run `npm run build && grep -c "<script>" build/index.html`.
|
||||
2. Is `cacheableResponse.statuses: [0]` actually needed for same-origin `/audio/*.mp3`, or can it be tightened to `[200]` only? (M2)
|
||||
3. GitHub Pages mirror — does it serve `_headers`? (GH Pages ignores `_headers`; CSP only enforced on CF.) Acceptable since GH is mirror-only?
|
||||
4. Should runtime `cacheName: "loto-audio"` be versioned (e.g. `loto-audio-v1`) so a future audio regen forces purge? (L2)
|
||||
@@ -0,0 +1,125 @@
|
||||
# Lô tô — UI/UX Audit Pass 2 (260427-2047)
|
||||
|
||||
Scope: re-audit since `f28279b` against current `main`. Read `app.css`, `app.html`, `manifest.webmanifest`, `+page.svelte`, `PlayerBoard`, `MasterPanel`, `MasterEmptyState`, `SettingsButton`, `PageFooter`. Severity P0/P1/P2.
|
||||
|
||||
Mostly clean follow-up. Most pass-1 items addressed well. Findings below are new or partial-fix regressions.
|
||||
|
||||
---
|
||||
|
||||
## P0
|
||||
|
||||
### 1. PWA splash + tab theme color hardcoded saturated blue, mismatches light page background
|
||||
**Where:** `app.html:9` (`#1565c0` light) and `manifest.webmanifest:9-10` (`theme_color #1565c0`, `background_color #0a0f1f`).
|
||||
Light app bg is `#f8fafc` (near-white) but Safari tab strip / Chrome top bar paints `#1565c0`. Hard color jump where the bg should bleed into the chrome. PWA splash on iOS uses `background_color` only — `#0a0f1f` (deep navy) → light-mode users get a dark-navy splash that flashes into a near-white app. Jarring on cold launch.
|
||||
**Fix sketch:** light theme-color `#f3e9d7` or `#fff7ec` (warm off-white that echoes the amber top-glow). Manifest `background_color` to a neutral midpoint or fork by media: `background_color: #f8fafc`. iOS doesn't honor light/dark manifest yet, so pick the value matching the *more common* launch theme — `auto` defaults to user OS, so neutral cream is safer than near-black.
|
||||
|
||||
### 2. Tab title still bare "Lô tô" — PWA install card name mismatch
|
||||
**Where:** `app.html:6` `<title>Lô tô</title>` vs `manifest.name "Lô tô — Hội chợ TN1"`.
|
||||
Installed app shows full name; browser tab + history show only "Lô tô". Cold-share link previews lose the "Hội chợ TN1" context entirely. Also no Open Graph tags.
|
||||
**Fix sketch:** title `Lô tô — Hội chợ TN1`; add `<meta property="og:title">`, `og:description`, `og:image` (use `icon-512.png`). One-time copy, immediate brand lift on share.
|
||||
|
||||
---
|
||||
|
||||
## P1
|
||||
|
||||
### 3. PlayerBoard empty-state and MasterEmptyState are near-clones with conflicting prompts
|
||||
**Where:** `PlayerBoard.svelte:343-371` ghost grid + `MasterEmptyState.svelte:6-43` ghost grid.
|
||||
Both render same opacity-30 monochrome ghost-grid pattern. Master mode in `both` shows player ghost AND master ghost stacked when no game started. Visual repetition; the page reads "two empty boxes" not "two distinct roles". Also: master grid shows 99 cells but real master board is 11×9=99 with last row = single cell at col 8 — ghost should mirror the actual silhouette.
|
||||
**Fix sketch:** vary the ghosts visually — player ghost shows a subtle row-of-numbers stripe; master ghost shows scattered "called dots" pattern. Or hide the player ghost entirely in `both` mode pre-game (the master-mode hero CTA carries the call-to-action).
|
||||
|
||||
### 4. Mode picker glyphs read at first glance only for "player"; "master" megaphone is ambiguous, "both" looks like a stacked-window icon
|
||||
**Where:** `SettingsButton.svelte:259-275`.
|
||||
Player rect-with-grid-lines reads instantly = "card with rows". Master path `M3 11l14-6v14L3 13z` + arc is a megaphone but at 24×24 stroke 1.8 looks like an abstract triangle pointing right; not enough silhouette weight at 28px tall. "Both" stacked rectangles read as "two cards" not "player + master roles". Hint line below mitigates but the glyph itself doesn't sell.
|
||||
**Fix sketch:** master = filled megaphone with sound waves (use stroke-width 2.2 + fill-on-active). "Both" = player-card-glyph layered with mini-megaphone badge in corner — composes the two prior glyphs. Keeps semantic continuity ("both = the two things above stacked").
|
||||
|
||||
### 5. "Đặt lại" reset chip — bordered now, but still no confirm dialog
|
||||
**Where:** `SettingsButton.svelte:431-440`.
|
||||
Pass-1 fix turned reset into a chip-with-border (good). But it still resets all settings (theme, mode, color, voice, auto-call) on a single tap. Easy mis-tap on mobile next to "Xong". No undo.
|
||||
**Fix sketch:** `if (confirm("Đặt lại tất cả tuỳ chỉnh?"))` guard; or convert to two-step ("Tap to reset" → "Confirm reset" inline state for 3s). Native `confirm()` is fine here, low frequency.
|
||||
|
||||
### 6. Settings modal scroll on small viewports — sticky header/footer absent
|
||||
**Where:** `SettingsButton.svelte:166-167`. `max-h-[90vh] overflow-y-auto` whole-modal scroll.
|
||||
On 375×667 (iPhone SE) with both auto-call AND voice-waiting expanded, modal hits ~700px. User scrolls past "Cài đặt" title; "Đặt lại / Xong" footer scrolls off too — must scroll back to dismiss. Title and primary CTAs should be persistent.
|
||||
**Fix sketch:** sticky title row (`sticky top-0 bg-white dark:bg-slate-800 -mx-6 px-6 pt-6 pb-3 z-10`), sticky footer row similarly. Inner content gets the scroll. Saves a scroll-trip per session.
|
||||
|
||||
### 7. Auto-call slider — still no tick labels at 1s/5s/10s
|
||||
**Where:** `SettingsButton.svelte:306-316`.
|
||||
Pass-1 noted this. Fixed: nesting + left-border indent (good). Not fixed: tick labels. Slider value floats free, user has no anchor for "what is fast vs slow".
|
||||
**Fix sketch:** below slider, add `<div class="flex justify-between text-[10px] text-slate-400 mt-1"><span>1s</span><span>5s</span><span>10s</span></div>` aligned to track. Trivial.
|
||||
|
||||
### 8. Voice "Quản trò đọc số" hint copy still confusing in `both` mode
|
||||
**Where:** `SettingsButton.svelte:336-338`. Pass-1 note: hide in player mode — done. But: hint says `Đọc số đã xổ + báo Chờ/Kinh khi ở "Cả hai".` In `master` mode the second clause ("báo Chờ/Kinh") is wrong — there's no player board to call Chờ/Kinh from in solo master.
|
||||
**Fix sketch:** branch the hint by mode: master → `Đọc số đã xổ.`; both → `Đọc số đã xổ và báo Chờ/Kinh thay người chơi.`
|
||||
|
||||
### 9. Header subline `🏮 Hội chợ TN1` — lantern emoji renders monochrome on Windows/Linux, color on Apple/Android
|
||||
**Where:** `+page.svelte:31`.
|
||||
Cross-platform lantern inconsistency. On a Windows browser the lantern is line-art outlined, breaking the festive intent. Dashes-flank treatment is good though.
|
||||
**Fix sketch:** ship a tiny SVG lantern inline (12×16, color: rose-500) in place of the emoji. Same byte cost, consistent across OS, theme-tintable.
|
||||
|
||||
### 10. Master "Số vừa xổ" hero — w-32 mobile is good (pass-1 fixed), but border-[6px] eats interior
|
||||
**Where:** `MasterPanel.svelte:244-252`.
|
||||
128px circle - 12px border (×2) = 104px interior for an 8xl number. Number renders fine but the ring feels chunky at this size; reads as "thick outlined badge", not "called number". Aspect feels token-y not announcer-y.
|
||||
**Fix sketch:** `border-[4px] sm:border-[10px]`. Keep desktop chunk; trim mobile.
|
||||
|
||||
---
|
||||
|
||||
## P2
|
||||
|
||||
### 11. Section-divider hatch repeats under the bottom decorative band but with no label slot
|
||||
**Where:** `PlayerBoard.svelte:316`. `<div class="section-label" aria-hidden="true"></div>` — empty label = just the cross-hatch flanks ::before/::after with `flex:1` and 0 gap. Visually thinner than divider above section-label rows.
|
||||
**Fix sketch:** swap to `<div class="section-divider"></div>` — semantic match, consistent thickness. Tested: same color path via `--section-accent`.
|
||||
|
||||
### 12. `aria-live="assertive"` on hero number — still assertive in pass-1 had it as P2; updated to "polite" in code (good). No regression.
|
||||
|
||||
### 13. Confetti emoji set still `["🎊", "✨", "🎉", "🥳"]` — pass-1 P2 note re. lantern + Vietnamese-flavored set not addressed
|
||||
**Where:** `PlayerBoard.svelte:35`.
|
||||
Add `🏮 🎋 🥢` for fairground feel. Match the lantern in the header for cohesion.
|
||||
|
||||
### 14. `apple-touch-icon` only 192px (no 180px specifically; no `apple-touch-startup-image`)
|
||||
**Where:** `app.html:11`. iOS scales 192→180 fine but loses sharpness. Missing splash image = bare-color flash on cold launch.
|
||||
**Fix sketch:** generate `icon-180.png` and `apple-splash-{2048x2732,1668x2388,1170x2532}.png` from `source.svg`. Wire `<link rel="apple-touch-startup-image" media="...">`. Optional polish.
|
||||
|
||||
### 15. `MasterEmptyState` ghost grid uses `i % 11 < 9 && i % 7 === 0` — generates non-deterministic-looking sparse fill that doesn't mirror the master grid silhouette (col 8 row 10 only has 90)
|
||||
**Where:** `MasterEmptyState.svelte:14`. Cosmetic — looks like a noise-ghost rather than a master-board ghost. Fine as decoration but pass on opportunity to communicate "tracking grid" affordance.
|
||||
**Fix sketch:** mirror real `BOARD` shape: row 0 cols 1-8, rows 1-9 all cols, row 10 col 8. Then ghost-fill ~15-20% with a deterministic mod pattern.
|
||||
|
||||
### 16. Toast above grid (pass-1 fixed) — but on `both` mode the toast renders inside PlayerBoard's `relative` wrapper while master panel below pushes content; toast `-top-3` goes negative into the page-padding zone, can clip on very narrow screens (≤320px) where parent has only `px-2` (8px).
|
||||
**Where:** `+page.svelte:11`, `PlayerBoard.svelte:330`.
|
||||
Edge case (≤320 = older Android, rare). Toast still readable but center text near the screen edge.
|
||||
**Fix sketch:** add `mx-2` on the toast button so it can compress safely; or move toast to `top-1` (positive offset, sits inside the rounded board chrome).
|
||||
|
||||
---
|
||||
|
||||
## Pass-1 fix verification
|
||||
|
||||
| Pass-1 item | Verdict |
|
||||
|---|---|
|
||||
| 1. Master empty state | ✅ MasterEmptyState added, role pill shown |
|
||||
| 2. Dark winning row contrast | ✅ `bg-emerald-900/60 text-emerald-200` |
|
||||
| 3. Vietnamese font fallback | ✅ self-hosted Roboto Condensed 700 |
|
||||
| 4. Toast over cells | ✅ moved to `-top-3` (see P2 §16 edge case) |
|
||||
| 5. Mode picker glyphs | ⚠️ added but readability mixed (P1 §4) |
|
||||
| 7. Master hero w-40 mobile clip | ✅ w-32 sm:w-56 |
|
||||
| 8. Auto-call slider density | ⚠️ partial — nesting fixed, no tick labels (P1 §7) |
|
||||
| 9. Voice nesting wording | ⚠️ partial — hidden in player but copy still off in master (P1 §8) |
|
||||
| 10. Color picker grouped | ✅ bordered card + sub-headers |
|
||||
| 11. Reset button visibility | ⚠️ chip-bordered (good) but no confirm (P1 §5) |
|
||||
| 12. Header subline brand mood | ✅ dash-flanked, lantern emoji (see P1 §9 cross-OS) |
|
||||
| 13. Bingo modal row-number size | ✅ text-5xl/6xl |
|
||||
| 18. aria-live polite on hero | ✅ |
|
||||
| 19. Reduced-motion gating | ✅ media query added |
|
||||
| 14. Footer dual attribution | ✅ in-card credit removed |
|
||||
|
||||
---
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. Manifest `background_color: #0a0f1f` was chosen for dark; should we accept the light-mode PWA splash dark-flash, or pick a neutral cream? (P0 §1)
|
||||
2. iOS Safari standalone — has it actually been tested on a device, or only DevTools simulated? Apple-status-bar `black-translucent` interacts with `safe-area-inset-top`; nothing in the layout reserves that inset.
|
||||
3. Is there appetite to drop the `🏮` emoji entirely if the SVG-inline lantern is rejected? Plain dashes alone read fine and ship-stable. (P1 §9)
|
||||
4. Auto-call max 10s — is that the right ceiling? Real-life Lô tô callers often pause 15-20s for call-and-response. Out of scope but data point.
|
||||
|
||||
---
|
||||
|
||||
**Status:** DONE
|
||||
**Summary:** 16 findings — 2 P0 (PWA splash/theme color mismatch, tab title brand), 8 P1 (empty-state duplication, glyph readability, reset-confirm, sticky modal chrome, slider ticks, voice hint copy, lantern cross-OS, mobile hero ring), 6 P2. Pass-1 mostly addressed; partial-fixes on items 5/8/9/11.
|
||||
+2
-2
@@ -3,8 +3,8 @@
|
||||
<head>
|
||||
<meta charset="utf-8" />
|
||||
<meta name="viewport" content="width=device-width, initial-scale=1" />
|
||||
<title>Lô tô</title>
|
||||
<meta name="description" content="Bàn số của trò chơi Lô tô" />
|
||||
<title>Lô tô — Hội chợ TN1</title>
|
||||
<meta name="description" content="Bàn số của trò chơi Lô tô — Hội chợ TN1" />
|
||||
<link rel="manifest" href="%sveltekit.assets%/manifest.webmanifest" />
|
||||
<meta name="theme-color" content="#1565c0" media="(prefers-color-scheme: light)" />
|
||||
<meta name="theme-color" content="#0a0f1f" media="(prefers-color-scheme: dark)" />
|
||||
|
||||
@@ -333,9 +333,15 @@
|
||||
{#if settings.mode !== "player"}
|
||||
<div class="mb-2">
|
||||
{@render switchRow("Quản trò đọc số", settings.voiceEnabledMaster, toggleVoiceMaster)}
|
||||
<p class="text-xs text-slate-500 dark:text-slate-400 mt-1.5 px-1">
|
||||
Đọc số đã xổ + báo Chờ/Kinh khi ở "Cả hai".
|
||||
</p>
|
||||
{#if settings.mode === "both"}
|
||||
<p class="text-xs text-slate-500 dark:text-slate-400 mt-1.5 px-1">
|
||||
Đọc số đã xổ + báo Chờ/Kinh.
|
||||
</p>
|
||||
{:else}
|
||||
<p class="text-xs text-slate-500 dark:text-slate-400 mt-1.5 px-1">
|
||||
Đọc số đã xổ.
|
||||
</p>
|
||||
{/if}
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
|
||||
+1
-1
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
Content-Security-Policy: default-src 'self'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self'; connect-src 'self'; font-src 'self' data:; frame-ancestors 'none'; base-uri 'self'; form-action 'self'; manifest-src 'self'; worker-src 'self'
|
||||
Content-Security-Policy: default-src 'self'; img-src 'self' data:; media-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'; connect-src 'self'; font-src 'self' data:; frame-ancestors 'none'; base-uri 'self'; form-action 'self'; manifest-src 'self'; worker-src 'self'
|
||||
X-Content-Type-Options: nosniff
|
||||
Referrer-Policy: strict-origin-when-cross-origin
|
||||
Permissions-Policy: accelerometer=(), camera=(), geolocation=(), gyroscope=(), microphone=(), payment=(), usb=()
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
"display": "standalone",
|
||||
"orientation": "portrait",
|
||||
"theme_color": "#1565c0",
|
||||
"background_color": "#0a0f1f",
|
||||
"background_color": "#f8fafc",
|
||||
"lang": "vi",
|
||||
"icons": [
|
||||
{
|
||||
|
||||
+7
-5
@@ -7,10 +7,12 @@ import { defineConfig, loadEnv } from "vite";
|
||||
// Precache the default voice's clips so the app is fully offline-capable
|
||||
// on first install (without bloating the install with every voice).
|
||||
// Alternate voices fall through to runtime CacheFirst on first play.
|
||||
// `audio-manifest.js` is the single source of truth for voice ids; we
|
||||
// duplicate the read here because vite.config can't import .svelte.js.
|
||||
const audioManifest = JSON.parse(
|
||||
readFileSync("./static/audio/manifest.json", "utf8"),
|
||||
);
|
||||
const defaultVoiceId = audioManifest.voices[0]?.id ?? "hoai-my";
|
||||
const defaultVoiceId = audioManifest.voices[0].id;
|
||||
const clipNames = [
|
||||
...Array.from({ length: 90 }, (_, i) => String(i + 1)),
|
||||
"cho",
|
||||
@@ -18,8 +20,8 @@ const clipNames = [
|
||||
];
|
||||
const defaultVoicePrecacheEntries = clipNames.map((n) => ({
|
||||
url: `/audio/${defaultVoiceId}/${n}.mp3`,
|
||||
// Static asset with a stable name; Workbox needs a revision string
|
||||
// to track changes. Bump the prefix when audio is regenerated.
|
||||
// Workbox needs a revision string to invalidate stale clips.
|
||||
// Bump the prefix when audio is regenerated.
|
||||
revision: `audio-v1-${defaultVoiceId}-${n}`,
|
||||
}));
|
||||
|
||||
@@ -43,7 +45,6 @@ export default defineConfig(({ mode }) => {
|
||||
// confusing.
|
||||
strategies: "generateSW",
|
||||
manifest: false,
|
||||
includeAssets: ["icons/*.png", "audio/**/*.mp3"],
|
||||
workbox: {
|
||||
globPatterns: ["**/*.{js,css,html,svg,png,woff2,webmanifest}"],
|
||||
maximumFileSizeToCacheInBytes: 5 * 1024 * 1024,
|
||||
@@ -62,7 +63,8 @@ export default defineConfig(({ mode }) => {
|
||||
maxEntries: 400,
|
||||
maxAgeSeconds: 60 * 60 * 24 * 30,
|
||||
},
|
||||
cacheableResponse: { statuses: [0, 200] },
|
||||
// Audio is same-origin → no need to allow opaque (0).
|
||||
cacheableResponse: { statuses: [200] },
|
||||
},
|
||||
},
|
||||
],
|
||||
|
||||
Reference in new issue
Block a user