diff --git a/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md b/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md index 3aba7ad..9607356 100644 --- a/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md +++ b/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md @@ -40,3 +40,15 @@ Both were fixed in the same change: the comment now states the exposure, and `data/boundaries`. Treat the invariant as enforced, and treat any *new* server-only module holding pano ids the same way -- the walk only guards what is named in it. + +Third exposure, found 2026-09-21 on branch `dev`: the daily challenge. `/api/daily` +mints a fresh session for the SAME panorama on every call with no per-player check, +and `/api/guess` scores it onto the permanent (untrimmed) score boards. Since the +first submit returns `exactLocation`, the loop GET /api/daily -> POST /api/guess +with the known coordinates is +5 on three boards for two HTTP requests, unbounded. +Ordinary rounds are immune only because `/api/new-game` draws randomly and reveals +nothing. The team's "browser-only attempt limit is fine, there is no daily board" +rationale assumes the daily credits no board; it credits the main ones. + +**How to apply:** whenever a mode makes the answer repeatable (daily, replay, +shared link), check what it credits before accepting a client-side attempt limit. diff --git a/.claude/agent-memory/code-reviewer/project-scoring-ladder-and-boards.md b/.claude/agent-memory/code-reviewer/project-scoring-ladder-and-boards.md index 6e1be2c..b140143 100644 --- a/.claude/agent-memory/code-reviewer/project-scoring-ladder-and-boards.md +++ b/.claude/agent-memory/code-reviewer/project-scoring-ladder-and-boards.md @@ -36,3 +36,10 @@ members). non-top-200 player's total lives. Also note the read-modify-write in the same function is not atomic — concurrent rounds under one username lose an update; `upstash.js` has no `zIncrBy` yet. Related: [[project-anti-cheat-invariant]]. + +Update 2026-09-21 (`dev`): the top-200 trim is GONE from the score boards +(`leaderboard.js`) and `creditScore` now uses ZINCRBY (2 commands/level instead of +4). Distance boards are still trimmed at 200; `MAX_LEADERBOARD_SIZE` is only a +serving window for scores. The `trimmed` field and the `score === null` -> +"Below top 200" dialog branch are gone — but `tests/e2e/helpers.js` still emits +`trimmed: false`. diff --git a/plans/reports/code-review-260921-0014-dev-implementation.md b/plans/reports/code-review-260921-0014-dev-implementation.md new file mode 100644 index 0000000..6b29bd6 --- /dev/null +++ b/plans/reports/code-review-260921-0014-dev-implementation.md @@ -0,0 +1,178 @@ +# Code review — `dev` implementation rollout (main...dev) + +Scope: `git diff main...dev -- src scripts tests .github package.json`, 66 files, ++2424/-803. Commits 3314da1, e649259, 8450b75, 6dbe2e0, 064c173. + +## 1. Gates + +| Gate | Result | +|------|--------| +| `npm run lint` | pass — 0 errors, 21 warnings, all pre-existing `react-hooks/*` in files outside and inside the diff alike (`use-count-up.js` untouched) | +| `npm test` | pass — 28 files, 348 tests | +| `npm run build:check` | pass — 101 pages; `/` and `/daily` static, `/api/daily` dynamic | +| e2e | not run (headless ARM64, no browser) — two specs reviewed by reading, see F6 | + +## 2. Findings + +### Blocker + +**B1 — the daily challenge is an unlimited leaderboard-farming loop.** +`src/app/api/daily/route.js:18-46` mints a fresh `crypto.randomUUID()` session on +every call with no per-player check, and the panorama is the same all day by +design. `src/app/api/guess/route.js:122-126` scores that session like any other, +so `submitRoundScore` credits the district, province and country score boards +(`src/lib/leaderboard.js:137-154`), which are now permanent (never trimmed). The +first submit returns `gameResult.exactLocation` (`guess/route.js:170-173`), so the +loop is: `GET /api/daily` → `POST /api/guess` with the coordinates just learned → ++5 points on three boards, repeatable for two HTTP requests, all day. +Ordinary rounds are immune: `/api/new-game` draws a random panorama and never +returns its id or coordinates, so a replay cannot be aimed. +The rationale at `src/lib/daily.js:15-19` ("no daily leaderboard… the only person +a second attempt cheats is the player") holds only if the daily credits no board. +It credits the main ones. This is not the accepted browser-only-attempt decision; +it is a premise of that decision that the code does not satisfy. +Fix: in `guess/route.js`, skip both fan-outs when `session.mode === 'daily'` and +record stats only — that matches "no daily leaderboard" exactly. Alternative: a +server-side `SET daily:credited:{day}:{playerId} NX` gate before crediting. + +### Should-fix + +**S1 — `stats:players:{day}` can be created with no TTL and live forever.** +`src/lib/stats.js:56-68`. The HLL's EXPIRE is gated on `count === 1` from the +*hash* increment. If the day's first recorded round arrives without a player +cookie (cookie-blocked browser, ITP), that round consumes `count === 1`; the next +round with the same `level:score` field and a playerId creates the HyperLogLog at +`count === 2`, so `expire` never runs. One permanent key per affected day, against +a 256 MB free tier. `tests/stats.test.js:51` only covers the playerId-first order. +Fix: EXPIRE the players key when `pfAdd` itself reports a new member +(`if (await pfAdd(...)) await expire(...)`) — distinct players per day are few, so +the extra command is negligible and always covers key creation. + +**S2 — home page hydration mismatch on the daily number, every day after deploy.** +`src/app/components/DailyCard.js:39`: `state?.number ?? dailyNumber(dailyDay())`. +`/` is statically prerendered (confirmed by `build:check`), so the HTML freezes the +build-day number, while the client's first render computes today's. From the day +after a deploy, every visitor hydrates `#N+1` over `#N`. `played` and `streak` +are correctly null-guarded; `number` is the one that is not, despite the comment +at lines 18-20 claiming the first paint matches the server. +Fix: derive `number` from `state` only and render a placeholder before mount. + +**S3 — the daily replay path forces `isPano: true`.** +`src/app/components/GameClient.js:216`. `saveDailyResult` never stores `isPano` +(`src/lib/daily-progress.js:58-67`), so a flat daily image is re-rendered through +the three.js panorama viewer on revisit. Fix: store and restore `isPano`. + +**S4 — a cached daily URL that stops resolving breaks the day with no lever.** +`src/lib/daily.js:32-36` returns the cached record unconditionally for 48 h; the +4-attempt retry loop runs only on a cache miss. If Mapillary's signed thumbnail +403s mid-day, every `/api/daily` keeps handing out the dead URL and the client has +no retry. The "valid for weeks" claim at lines 12-13 is asserted, not measured. +Fix: cache `{id, lat, lng, regionCode}` (the expensive, deterministic part) and +re-resolve `url` per request — one Mapillary call per player, which is what every +non-daily round already costs. + +**S5 — `/api/guess` can credit boards and still answer 500.** +`guess/route.js:127-131`. `Promise.all` rejects on the first failing level, but the +sibling `zIncrBy` calls already in flight are not undone and the session is already +deleted, so a retry is impossible. The client then renders +`FAILURE_COPY.default` — "Nothing was scored" — for a round that partially scored. +`distanceOrNone` already applies the right pattern to the lesser record; the score +fan-out, which is the one that can half-succeed, does not. +Fix: catch around `submitRoundScore` and return 200 with what landed, or at minimum +a distinct `reason` so the dialog does not claim nothing was recorded. + +**S6 — board rows in the result dialog are still English; the e2e stub says otherwise.** +`src/lib/leaderboard.js:145` and `:284` set `name: getRegion(code).name`, rendered +at `RoundResultDialog.js:303` and `:325`. Everything else on that screen is +Vietnamese via `regionName()`. `tests/e2e/helpers.js:55-56,62-63` was updated to +`'Quận 7'` / `'Hồ Chí Minh'`, so the stub now asserts text production does not +produce. Fix: `regionName(regionCode)` in both credit helpers. +(`src/app/api/leaderboard/route.js:33` has the same English `name`, but no client +reads it — cosmetic only.) + +**S7 — `tests/e2e/home.spec.js:16` will fail against the new UI.** +It still expects `'Ha Noi'`, `'Da Nang'`, `'Lam Dong'`; `RegionPicker` now renders +`'Hà Nội'`, `'Đà Nẵng'`, `'Lâm Đồng'`. Only the TPHCM cases in that file were +updated. e2e is not in CI, so nothing caught it. + +### Nit + +- `tests/e2e/helpers.js:47` still emits `trimmed: false`; no producer sets it and + no consumer reads it since the trim was removed. +- `src/lib/debug-access.js:31` keys off `VERCEL_ENV !== 'production'`, so any + non-Vercel deployment is fully open. Correct for the stated Vercel-only target; + a `NODE_ENV` fallback would make that not depend on the host. +- `GameClient.js:541-548` mounts two `ThemeToggle` instances and hides one with + CSS; both run their localStorage effect. Works, costs a component. +- `pickPanoBySeed` (`src/lib/pano-index.js`) hashes the row offset as + `${seed}:offset` without the province, so walking to a second province reuses the + same offset. Deterministic and harmless, marginally less uniform. +- `ci.yml` triggers on `push: [main, dev]` *and* `pull_request`, so a dev→main PR + runs all four gates twice per push. +- `leaderboard-backup.yml` uploads every board — all usernames — as a GitHub + artifact. On a public repo those are publicly downloadable. Self-chosen + pseudonyms only, so low, but it is player data leaving the boundary. +- `/api/daily` is unauthenticated and unrated like the rest of the API, and is now + linked from the home page. Two Redis commands per call; the standing free-tier + exposure, on a more visible URL. + +## 3. Checked and found correct + +- `/api/guess` validates body shape, username and both coordinates *before* + touching the session; `toCoordinate` rejects `null`/`true`/`"abc"`. +- The session is claimed by `DEL` and scoring runs only for the request whose + `deleteGameSession` returned true — concurrent double submit credits once + (`tests/guess-route.test.js`). +- A malformed body returns 400 `invalid-request`, not 500; only an unusable stored + target throws. +- The answer (`exactLocation`, resolved region) appears only in the post-guess + response; `/api/daily` and `/api/new-game` return URL and `isPano` only. +- `zIncrBy` is atomic and halves the score fan-out to 2 commands per level. +- Score boards are no longer trimmed; `creditScore` always returns a number, and + the dialog's `score === null` → "Below top 200" branch was removed with it. +- `getLeaderboard` clamps `limit` into `[1, 200]` and keeps `?city=` → country. +- `/api/skip` and `/api/new-game` both gate on `isUuid` before the value becomes a + Redis key; `generateSessionId` mints a matching lowercase UUID. +- The skip race is fixed: `handleSkipGuess` passes `null`, never the id whose DEL + is still in flight. +- `debugAccessAllowed` guards both surviving debug routes; `/api/debug/mapillary` + is deleted; `region-coverage` now has an error boundary around the Neon calls. +- `locateRegion`/`regionHit` run after the session is consumed, change no score, + and grade district / province / none correctly including province-level answers. +- Daily determinism: same seed → same panorama; a concurrent cold cache yields the + same record from both workers, so the race is cost-only. +- Day rollover at UTC+7 is offset arithmetic on an epoch, not a local timezone; + `nextStreak` / `currentStreak` extend, hold and break as documented, and a round + straddling midnight correctly counts as the earlier day. +- Stats: field layout, `stats:????-??-??` SCAN pattern (cannot match + `stats:players:*`), and `scripts/stats.mjs` arithmetic all line up. +- Redis fake: `hincrby`, `hgetall` (null when empty), `pfadd` (1 only when the + estimate changes), `pfcount` (union), `expire` (0 on a missing key) and the + non-string TTL bookkeeping all match Redis semantics. +- `regionName()` is applied everywhere a region is rendered except the two board + rows in S6; 84 of 85 nodes carry `nameVi`, `VN` intentionally does not. +- `geo-search` keeps both spellings as aliases; `foldDiacritics` handles `Đ`. +- Fonts add the `vietnamese` subset; `metadataBase` and per-region OG are sound. +- `share.js` carries no coordinates, panorama id or resolved district. +- The OSM "explore" link renders only after a guess and `exactLocation` is in fact + carried into the result object (`GameClient.js:350`). +- `validateUsername` is now the single rule for the modal and the route; `:` is + excluded, which is what keeps the packed distance member parseable. + +## 4. Merge recommendation + +**Not ready.** B1 must be fixed before this reaches production — it turns the +permanent score boards into a two-request-per-5-points faucet. S1–S5 are worth +taking in the same pass; S6/S7 are small and make the e2e suite honest again. +Everything else is sound and the security direction of the change (debug gate, +shared username rule, UUID gating, atomic session claim) is a clear improvement. + +## 5. Unresolved questions + +1. Is crediting the main boards from the daily intended at all? If yes, B1 needs a + server-side attempt gate; if no, skipping the fan-out for `mode === 'daily'` is + both the fix and the simplification. +2. What is the actual lifetime of a Mapillary `thumb_2048_url`? S4's severity is + "the day breaks silently" if under 24 h and "cosmetic" if genuinely weeks. +3. Is `tiennm99/vngeoguessr` public? That decides whether the backup artifact's + usernames are world-readable. diff --git a/plans/reports/synthesis-260921-0014-dev-review.md b/plans/reports/synthesis-260921-0014-dev-review.md new file mode 100644 index 0000000..f617d95 --- /dev/null +++ b/plans/reports/synthesis-260921-0014-dev-review.md @@ -0,0 +1,76 @@ +# Review of the dev implementation (2026-09-21) + +Three independent read-only reviews of `main...dev` (064c173). No code changed. + +- [Correctness and security](code-review-260921-0014-dev-implementation.md) +- [UX of the new surfaces](ui-ux-review-260921-0014-dev-surfaces.md) +- [Test quality](tester-260921-0015-dev-test-quality.md) + +Gates re-run by the reviewers: 348 tests pass, lint 0 errors, production +compile green. e2e not runnable here. + +## Verdict: not ready to merge + +One blocker, verified against source. + +**B1. The daily is a leaderboard-farming loop.** `/api/daily` mints a fresh +session on every call, the panorama is the same all day, and the first +`/api/guess` returns the coordinates. Two requests per 5 points on three +permanent boards, repeatable all day. The "no daily board, so a second attempt +only cheats the player" premise is false while the daily credits the main +boards. Fix: skip both fan-outs in the guess route when `session.mode === +'daily'` and record stats only. That is what "no daily leaderboard" meant. + +## Should fix before merge + +- **S1** `stats:players:{day}` can miss its EXPIRE and live forever: the TTL + is gated on the hash counter, not on the HyperLogLog's own first write. +- **S2** Home page hydration mismatch: DailyCard falls back to + `dailyNumber(dailyDay())` during SSR of a static page, so the frozen number + differs from the client's every day after deploy. Derive it from state only. +- **S3** Daily replay forces `isPano: true`; store the flag with the result. +- **S4** A cached daily URL that stops resolving breaks the day for 48h with + no lever. Cache id and coordinates, re-resolve the URL per request. +- **S5** Score fan-out `Promise.all` can half-credit and still answer 500, + which the client reports as "nothing was scored". +- **S6** Result-dialog board rows still show ASCII names + (`leaderboard.js` uses `getRegion(code).name`), so the reveal is + Vietnamese and the rows beneath it are not. The e2e stub was updated to + Vietnamese and now asserts text production does not produce. +- **S7** `tests/e2e/home.spec.js:16` still expects `Ha Noi`, `Da Nang`, + `Lam Dong`; only the Ho Chi Minh entry was renamed. Would fail on a local + e2e run. +- **UX H1** Nothing says "one guess" before the daily submit. Card copy and a + "Submit final guess" label in daily mode. +- **UX H2** Share button state is not announced: aria-label overrides the + sr-only text, no live region, and a cancelled share sheet reads "Retry". + +## Worth doing, not blocking + +- UX: failure copy says "start a new one" inside the daily; DailyCard buttons + are 36px; three-button action row is ~288px against 280px at 360 wide; + "Right province, wrong district" shown when the answer has no district; + streak badges are emoji plus title only. +- Tests: no test for the daily when Mapillary fails four times, for the guess + route when scoring throws after consumption, for stats tolerating a Redis + outage, or for EXPIRE across two days. `daily-route.test.js` compares + against the live `dailyDay()` and can flake across Vietnam midnight. +- Backup workflow uploads every username as an artifact on a public repo + (artifacts are downloadable by any logged-in GitHub user). Usernames only, + no personal data, but decide whether that is acceptable. +- `debug-access.js` keys off `VERCEL_ENV`; a non-Vercel deploy is fully open. + +## Confirmed correct by the reviewers + +Validation precedes session consumption; atomic DEL claim; malformed body is +400; answer only post-guess; ZINCRBY halves the fan-out; untrimmed boards with +a 200 window; UUID gating on skip and new-game; skip race fixed; debug gate on +both surviving routes; region-hit grading; daily determinism and UTC+7 +rollover; streak rule; stats key layout; Redis fake semantics for the new +commands; 84 of 85 nodes carry `nameVi`; share text carries no coordinates. + +## Unresolved + +- Should the daily credit the main boards at all? Recommendation: no. +- Real lifetime of a Mapillary `thumb_2048_url`. Decides how urgent S4 is. +- Is a world-readable username list in a backup artifact acceptable? diff --git a/plans/reports/tester-260921-0015-dev-test-quality.md b/plans/reports/tester-260921-0015-dev-test-quality.md new file mode 100644 index 0000000..aeeb173 --- /dev/null +++ b/plans/reports/tester-260921-0015-dev-test-quality.md @@ -0,0 +1,129 @@ +# Test Quality Assessment: dev branch + +**Date**: 2026-09-21 | **Branch**: dev | **Commit**: 064c173 + +## Test Execution Results + +**All tests pass**: 348 tests across 28 test files executed in 16.90s. + +- Unit/integration: daily-route, daily-calendar, daily-progress, debug-access, region-locate, share, skip-route, stats, guess-route, leaderboard, username, new-game-route, geo-search, regions, region-request, skip, leaderboard all pass +- No failing tests; no skipped tests +- Test coverage tool not available (missing @vitest/coverage-v8), so unmapped dead code may exist + +## Weak Tests (Would Pass Against Broken Implementations) + +1. **daily-calendar.test.js - `previousDay` across month boundaries** + - Tests `previousDay('2026-10-01')` → `'2026-09-30'` but only one boundary case + - **Mutation risk**: If `previousDay` ignored month calculation and subtracted days naively, test would still pass if year wrapping was not tested (e.g., Jan 1 → Dec 31) + - **Recommendation**: Add `previousDay('2026-01-01')` → `'2025-12-31'` and leap-year Feb 29 + +2. **region-locate.test.js - `locateRegion` only tests two districts** + - Tests Q7 (TPHCM) and HOANKIEM (HN), but only verifies exact match + - **Mutation risk**: If `locateRegion` always returned the first district in the boundary list, tests would not catch it (no randomness or edge-case bounds testing) + - **Recommendation**: Add tests for points near district boundaries and provinces with no mapped districts + +3. **stats.test.js - `recordRound` with no player ID** + - Tests tolerates null playerId but does not verify HyperLogLog is NOT incremented + - **Mutation risk**: If `pfAdd` was called unconditionally on null playerId, test would still pass + - **Recommendation**: Explicitly assert that `distinctPlayersAcross` returns 0 when all rounds have null playerId + +## Redis Fake Fidelity Issues + +**No critical issues found**. The fake Redis correctly implements: +- `hgetall` returns null on missing key, `null` on empty hash (matches real Redis behavior after EXPIRE) +- `hincrby` returns the new value (number, not string) ✓ +- `zincrby` returns the new score (number) ✓ +- `pfadd` returns 1 if estimate changed, 0 otherwise ✓ +- `pfcount` union across multiple keys ✓ +- `expire` on missing key returns 0, on existing returns 1 ✓ + +**Upstash SDK adaptation**: `hGetAllNumbers` properly coerces all hash values to numbers and returns `{}` instead of null (intentional normalization for stats hash). + +## Coverage Gaps (Ranked by Risk) + +1. **`/api/daily` when Mapillary fails 4 times (MAX_ATTEMPTS)** + - Code path: `daily.js` lines 39-59 throw after 4 retries, but no test exercises this + - **Risk**: High — silent failure or unexpected error format to client + - **Test spec**: `getDailyRound('2026-09-21')` where all 4 candidates fail fetchPanoramaById, verify error message includes "No daily panorama" + +2. **`/api/daily` when cached record exists but URL is stale** + - Code path: Redis caches with 48h TTL (lines 34-36, 51), but no test verifies URL validity past cache + - **Risk**: Medium — could serve expired Mapillary URLs (though Mapillary signs them for weeks) + - **Test spec**: Mock Mapillary to return different URL on re-fetch, verify first request uses cache, second (after expiry) uses new URL + +3. **POST /api/guess when submitRoundScore throws after session is consumed** + - Code path: `guess/route.js` lines 130-133 parallelize leaderboard/distance writes after deleteGameSession + - **Risk**: Medium — if submitRoundScore fails, player loses round credit but session is deleted (not retryable) + - **Test spec**: Mock leaderboard.submitRoundScore to throw, verify 500 response, session no longer exists, stats recorded (should be, per line 149) + +4. **POST /api/guess when stats recording (recordRound) fails** + - Code path: `guess/route.js` lines 149, wrapped in try-catch that logs but does not fail + - **Risk**: Low — design is intentional (stats failure must not block guess), but no test verifies this tolerance + - **Test spec**: Mock recordRound to throw, verify guess succeeds, session consumed, leaderboard updated, no error to client + +5. **stats.test.js - EXPIRE behavior across day boundaries** + - Code path: `stats.js` lines 61-67, EXPIRE called only when count==1 (first occurrence of level:score pair) + - **Risk**: Medium — if a new level appears on day 1 and again on day 2, second day's EXPIRE may not fire + - **Test spec**: recordRound on same level:score pair across two UTC days, verify both day keys have TTL set + +6. **region-request.js regionName fallback when nameVi is absent** + - Code path: Not visible in test reads, but `publicRegion()` must handle missing Vietnamese names + - **Risk**: Low-Medium — only affects UI, but can silently render undefined + - **Test spec**: Add mock region with no nameVi, verify publicRegion() returns English fallback or throws appropriately + +7. **validateUsername on auto-generated 'Player-abc123' names** + - Code path: Tests cover 'mai', too-long, colon-containing names, but no test for generated format + - **Risk**: Low — test coverage is broad, but the exact shape username.js generates may differ + - **Test spec**: Call validateUsername with output of username generation function, verify it accepts itself + +8. **DailyCard/GameClient daily replay path (component-level)** + - Code path: Browser-only, cannot test without DOM + - **Risk**: Low-Medium — hard to test without browser, but daily progress replay uses localStorage with no server validation + - **Test spec**: Note as untestable without e2e; defer to e2e suite + +## E2E Locator and API Shape Verification + +**All locators correctly updated** from English to Vietnamese: + +✓ `game.spec.js` line 18: `'Hồ Chí Minh'` (was `'Ho Chi Minh'`) +✓ `game.spec.js` line 45: `'Vietnam › Hồ Chí Minh › Quận 7'` (was `'Vietnam › Ho Chi Minh › District 7'`) +✓ `home.spec.js` line 16: `Hồ Chí Minh` button, `Quận 7` link +✓ `home.spec.js` line 32: `'Củ Chi'` uncovered district (was `'Cu Chi'`) + +**E2E response shapes** in `tests/e2e/helpers.js`: + +✓ `newGameResponse()` region name and path updated +✓ `guessResponse()` scoreLevel/distanceLevel names updated, path updated +✓ `{trimmed: false}` field present in scoreLevel (removed in earlier PR? verify) + +**Minor concern**: scoreLevel and distanceLevel mock helper on line 47-48 includes `trimmed: false` but tests do not assert this field's absence/presence. Verify real route response includes it. + +## Flakiness Risks + +1. **daily-route.test.js uses `dailyDay()` without mocking time** + - Line 96: `expect(body.day).toBe(dailyDay())` + - **Risk**: Test fails if run during Vietnam midnight (UTC 17:00) when day rolls over during execution + - **Fix**: Mock `Date.now()` in beforeEach, or use fixed timestamp in assertion + +2. **stats.test.js uses `statsDay()` without mocking** + - Lines 33-34: `statsDay(DAY1)` with hardcoded dates is safe (uses passed timestamps) + - Lines 41, 48: `statsDay()` with no args uses Date.now() — safe since test only counts relative days within one test run + - **Risk**: Low (test is internal to one run), but could fail if run at UTC/calendar boundary + +3. **daily-progress.test.js `nextStreak` uses hardcoded dates** + - All dates are passed as arguments; no `Date.now()` used + - **Risk**: None — test is deterministic + +## Unresolved Questions + +1. Does `trimmed: false` belong in e2e scoreLevel responses, and does the real route include it? +2. Is month-boundary handling in `previousDay()` tested elsewhere (e.g., in daily-calendar e2e)? +3. Does the game support 0-point rounds (score === 0) on the leaderboard, and are they tested? + +--- + +**Status**: DONE_WITH_CONCERNS + +**Summary**: 348 tests pass; core logic well-covered. Missing tests for Mapillary exhaustion (4 failures), stats failure tolerance, and edge cases in region/streak logic. Three low-level flakiness risks around date/time mocking. + +**Concerns**: Weak tests for nextStreak month boundaries and region location; missing coverage for daily route failure modes and stats recording robustness. E2E locators correctly updated; no shape mismatches found. diff --git a/plans/reports/ui-ux-review-260921-0014-dev-surfaces.md b/plans/reports/ui-ux-review-260921-0014-dev-surfaces.md new file mode 100644 index 0000000..69b26ae --- /dev/null +++ b/plans/reports/ui-ux-review-260921-0014-dev-surfaces.md @@ -0,0 +1,74 @@ +# UI/UX review — `dev` player-facing surfaces + +Date: 2026-09-21 · Scope: daily mode (card, route, dialog), Share, failure/hit copy, header collapse, Vietnamese names, compact ThemeToggle, Unicode usernames. Method: source only (no browser on this host). Read-only; nothing merged or changed. + +Prior audit checked: `ui-ux-review-260920-2201-improvement-brainstorm.md`. Width figures are from class values, not measurement. + +## 1. Findings + +### HIGH + +**H1. "One guess" is never stated before the guess.** Daily is one attempt (`GameClient.js:213-226` replays the stored result; `docs/features.md:186` "No skipping"), but the player is told only after: card copy `DailyCard.js:79` says "One street view, the same for everyone" — nothing about one guess; in-game the only signals are a missing Skip and a "Daily #N" badge; `FirstRoundHint.js:58-61` is the generic hint; the dialog line `RoundResultDialog.js:196` arrives after the fact. A first-timer who drops a quick pin to "see what happens" has spent the day. +Fix: card sub-copy "One street view, one guess, the same for everyone. Resets at midnight, Vietnam time."; in daily the Submit label reads `Submit final guess` (`GameClient.js:677`); optional one-line daily hint in `topBarSlot`. + +**H2. Share outcome is inaudible, and on phones sometimes invisible.** `RoundResultDialog.js:357-368`: `aria-label` overrides the sr-only `Copied/Retry/Share` span, so a screen reader never gets the state; a name change on the focused button is not reliably announced. Below `sm` the `failed` state shows the same Share icon — no feedback at all. `share.js:175` returns `failed` on `AbortError`, so closing the share sheet flips the desktop label to "Retry" for a non-error. `DailyCard.js:86-89` repeats it: static label "Share today's result", no failed state. +Fix: drop `aria-label`, let the visible/sr-only text name the button; add a sibling `` with "Result copied to clipboard" / "Could not copy — try again"; swap the icon on `failed` (e.g. `AlertCircle`); return `'cancelled'` for `AbortError` and leave the button untouched. Same treatment in `DailyCard`. + +### MEDIUM + +**M1. Failure copy tells a daily player to "start a new one."** `RoundResultDialog.js:19,27` — in daily there is no Next/Skip, "Done" goes home, and a failed round is not stored (`GameClient.js:363-370` saves only on success), so the honest instruction is to reopen `/daily`. +Fix: branch by `daily`: "Nothing was scored. Reopen today's challenge to try again." + +**M2. DailyCard played-state buttons are 36px.** `DailyCard.js:86,90` use `size="sm"`; `button.js:41-42` says sm is "never the sole tap target on a screen" — here they are the card's only targets on phones. +Fix: default size; below `sm` let the pair take the full second row (`w-full sm:w-auto`, `flex-1`). + +**M3. Result-dialog action row is over budget at 360px.** `RoundResultDialog.js:348-375`. Dialog `calc(100%-2rem)` = 328, `p-6` → 280px. Share: `size="lg"` keeps `has-[>svg]:px-5` (the `px-3` override only replaces `px-6`) ≈ 56px. Next Round min-content ≈ 136px (text-base, `px-6`, `whitespace-nowrap`). Menu ≈ 72px. Two `gap-3` = 24. Total ≈ 288 > 280, and every button is `shrink-0` (`button.js:18`), so the row overflows instead of squeezing. Menu is also `h-11` between two `h-12` buttons. +Fix: Share `className="size-12 px-0"`, Next Round `px-4 sm:px-6`, Menu `size="lg"`. Confirm on device (§4). + +**M4. "Right province, wrong district" when there is no district.** `region-locate.js:56-58` returns `province` whenever provinces match and the answer is not a district — including answers that only resolve to a province. Copy then asserts a wrong district that does not exist. +Fix: in the dialog, show "Right province" when `result.resolvedPath.length < 3`; or have `regionHit` return a fourth value for province-level answers. + +**M5. Streak badges rely on emoji + `title`.** `GameClient.js:474-477`, `DailyCard.js:71-73`, `RoundResultDialog.js:195`. `title` is hover-only; a reader hears "fire 3". +Fix: `aria-label="3-day streak"` on the badge (or sr-only "-day streak"), drop `title`. In the dialog line, move 🔥 after the text or mark it `aria-hidden`. + +### LOW + +- **L1** "Play today's" (`DailyCard.js:103`) is a dangling possessive → "Play today's challenge". +- **L2** `truncate` on the inline-flex Badge (`GameClient.js:466`) clips without an ellipsis: `text-overflow` needs a block container. Latent today (longest `nameVi` "Thủ Dầu Một" fits `8rem`); wrap the text in `` before longer names land. +- **L3** Two `prefers-reduced-motion` blocks both zero `.animate-fade-in-up` (`globals.css:334-345, 351-357`); merge. New dialog `animate-fade-in-up` uses (`RoundResultDialog.js:167-198`) are covered; the keyframe has no resting `opacity:0`, so nothing stays hidden. Count-up is guarded (`use-count-up.js:36-40`). +- **L4** Compact ThemeToggle wraps one button in `role="group"` (`ThemeToggle.js:73`); drop the group in compact mode. Label "Theme: Light. Switch to Dark" is good. +- **L5** Failure body has `role="alert"` (`RoundResultDialog.js:161`) while `DialogDescription` already carries the same sentence → double announcement. Drop the role. +- **L6** Vietnamese names sit in a `lang="en"` document (`layout.js:78`); a `` around `regionName()` output (one `RegionName` component) fixes pronunciation. +- **L7** DailyCard first paint is always the Play row, then swaps to the played state after mount (`DailyCard.js:28-41`) — a layout jump for returning players. Render the action slot `invisible` until `state` is set. +- **L8** Header badge reads "Daily" then "Daily #N" once the round lands (`GameClient.js:471`); reserve the width or show the number from `dailyNumber(dailyDay())` immediately, as the card does. + +Copy otherwise consistent: English UI, Vietnamese place names, "Vietnam" for the country; username help text (`UsernameModal.js:89`) still true under the Unicode rule; failure titles honest and reason-specific. + +## 2. Prior findings + +Closed by this diff: **H1** header overflow (compact Theme + Sound below `sm`, badge cap, tally hidden `