From 71e77eae0084a1ef36509a5b066524da5107b7c8 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 31 Aug 2026 22:08:41 +0700 Subject: [PATCH] chore(plans): add plan, journals, and review reports for UI/UX quick wins Document 260831-1906 plan phases, execution journal, and code review reports covering scoring ladder, game state refactor, home region continuity, and leaderboard UI fixes. Multiple review passes verified test coverage and component contracts. --- plans/260831-1906-uiux-quick-wins/plan.md | 49 +++ ...s-loading-split-region-relative-scoring.md | 28 ++ ...de-reviewer-260831-1906-uiux-quick-wins.md | 282 +++++++++++++++ ...wer-260831-2117-breaking-change-recheck.md | 317 ++++++++++++++++ ...viewer-260831-2145-client-state-recheck.md | 315 ++++++++++++++++ ...wer-260831-2145-server-contract-recheck.md | 36 ++ ...reviewer-260831-2145-ui-surface-recheck.md | 339 ++++++++++++++++++ .../researcher-260831-1853-geo-game-ux.md | 307 ++++++++++++++++ .../synthesis-260831-1853-uiux-improvement.md | 121 +++++++ .../ui-ux-review-260831-1853-uiux-audit.md | 136 +++++++ 10 files changed, 1930 insertions(+) create mode 100644 plans/260831-1906-uiux-quick-wins/plan.md create mode 100644 plans/journals/2026-08-31-ui-ux-quick-win-fixes-loading-split-region-relative-scoring.md create mode 100644 plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md create mode 100644 plans/reports/code-reviewer-260831-2117-breaking-change-recheck.md create mode 100644 plans/reports/code-reviewer-260831-2145-client-state-recheck.md create mode 100644 plans/reports/code-reviewer-260831-2145-server-contract-recheck.md create mode 100644 plans/reports/code-reviewer-260831-2145-ui-surface-recheck.md create mode 100644 plans/reports/researcher-260831-1853-geo-game-ux.md create mode 100644 plans/reports/synthesis-260831-1853-uiux-improvement.md create mode 100644 plans/reports/ui-ux-review-260831-1853-uiux-audit.md diff --git a/plans/260831-1906-uiux-quick-wins/plan.md b/plans/260831-1906-uiux-quick-wins/plan.md new file mode 100644 index 0000000..2479408 --- /dev/null +++ b/plans/260831-1906-uiux-quick-wins/plan.md @@ -0,0 +1,49 @@ +# UI/UX Quick-Win Fixes + +Status: done (2026-08-31) | Source: plans/reports/synthesis-260831-1853-uiux-improvement.md +Review: plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md — H1 resolved +per user decision as per-level ladders (`submitRoundScore`); H2/M1/M3 fixed. +Follow-up pass fixed the accepted leftovers too: M2 (roundLoading now clears on +panorama 'ready'; viewer keyed by a per-round counter so a repeated pano URL +still remounts), distance-board tints graded by each board's own ladder +(`LeaderboardList` regionCode prop), home table "1km" label trim. +Third review round (3 parallel scoped reviewers, reports +code-reviewer-260831-2145-*.md): no breaking change found. Fixed from it: +stale-guess clobber after watchdog release (applyRound clears guess + stamps +applied epoch; ready handlers epoch-guarded), Submit gated on sessionId, Skip +clears loadError, submitRoundScore rejects non-numeric/negative distance, +SCORE_BANDS deep-frozen, bandsForDiagonal NaN guard, honest e2e fixtures +(real TPHCM ladder, sessionId echo, image-swap assertion), literal per-level +unit expectations, docs/caption wording ("base scale", not "district round"). +Verified: 248 unit, 9 e2e, lint 0 errors, build clean. + +Outcome: apply quick-win fixes without breaking existing behavior or contracts. +Constraints: JS only, individual params, keep 0-5 score scale + existing board +keys, no schema changes, district-round scoring unchanged for typical districts. +Non-goals: 5-round set, daily challenge, keyboard guess placement, mobile rework. +Acceptance: `npm test`, `npm run lint`, `npm run build`, Playwright e2e all green. + +## Phases + +1. **Region-relative scoring** — `src/lib/game.js`: `bandsForDiagonal`, + `bandsForBbox`, `calculateScore(distance, bands=SCORE_BANDS)`; + `src/app/api/guess/route.js`: scale bands by picked region bbox + (`session.pickedRegion ?? session.cityCode`), return `bands` in gameResult. + Tests: extend `tests/game.test.js`, `tests/guess-route.test.js`. +2. **GameClient state split + error + prefetch + last-region** — + `initialLoading`/`roundLoading`/`submitting`/`loadError`; inline error panel + with Retry/Back replacing `alert()`; prefetch next round (+image preload) on + result open; write last-played region to localStorage + (`src/lib/last-region.js`, new). +3. **Result dialog** — no dismiss (no close button, noop onOpenChange), sr-only + DialogDescription, aria-live fix (static sr-only status, animation not + announced), band-scale chips from `result.bands` (skip when absent). +4. **Home** — scoring table derived from SCORE_BANDS + region-scaling caption; + "Continue in X" row in RegionPicker from last-region storage. +5. **Map robustness** — self-hosted Leaflet marker icons via static imports in + `LeafletMap.js`; drop dead cdnjs icon config in `ResultMap.js`. +6. **Verify + docs** — full test suite, lint, build, e2e; update + `docs/features.md` + `docs/game-flow.md` scoring sections. + +Risk: score semantics change for province/country rounds (accepted; boards mix +eras). Rollback: revert commit; no data migration involved. diff --git a/plans/journals/2026-08-31-ui-ux-quick-win-fixes-loading-split-region-relative-scoring.md b/plans/journals/2026-08-31-ui-ux-quick-win-fixes-loading-split-region-relative-scoring.md new file mode 100644 index 0000000..f8b660a --- /dev/null +++ b/plans/journals/2026-08-31-ui-ux-quick-win-fixes-loading-split-region-relative-scoring.md @@ -0,0 +1,28 @@ +--- +title: "UI/UX quick-win fixes: loading split, region-relative scoring, per-level board ladders" +date: 2026-08-31 +summary: Applied audited quick-win UI/UX fixes; review-driven pivot to per-level leaderboard ladders via submitRoundScore +--- + +# UI/UX quick-win fixes: loading split, region-relative scoring, per-level board ladders + +## What happened +Three advisory agents (audit, research, brainstorm) converged on the same defects; applied the quick wins per plans/260831-1906-uiux-quick-wins/plan.md: + +- Split GameClient's single `loading` flag into `initialLoading` / `roundLoading` / `submitting` / `loadError` — submitting a guess no longer unmounts the panorama viewer, and next-round loads show an overlay spinner. +- Region-relative scoring: `bandsForDiagonal`/`bandsForBbox` in src/lib/game.js scale the 0-5 ladder by the picked region's bbox diagonal (10km reference); `gameResult.bands` returned and rendered as a chip strip in the result dialog. +- Result dialog is non-dismissable (session already consumed); sr-only DialogDescription carries the outcome instead of a 60fps count-up inside aria-live. +- Panorama load failure: inline error panel with Retry/Back replacing `alert()` dead end. +- Next-round prefetch during the result dialog (metadata + image warm), "Continue in " home row (src/lib/last-region.js), self-hosted Leaflet marker icons. + +Gotchas hit: Turbopack dev returns a bare URL string for static PNG imports where webpack returns `{src}` — Leaflet threw "iconUrl not set" only in `next dev`; normalized with an `imageUrl()` helper. E2e strict-mode collision between the sr-only outcome text and the visible distance badge. + +## Decision +Code review flagged a real asymmetry (H1): the picked-region ladder fanned out to district boards, making country rounds ~100x cheaper per district-board point. User chose per-level ladders: `submitRoundScore(username, distance, regionCode)` credits each board from the raw distance against that region's own ladder; the flat `submitScore` primitive remains for callers that already hold points. Headline score stays on the picked ladder; each level entry now carries `points`. + +## Next steps +- Accepted, not fixed: roundLoading clears on fetch return, not panorama-visible (M2, pre-existing); LeaderboardList still colors distances with the unscaled base ladder; home table reads "1.00km+". +- Backlog from synthesis: 5-round set (M1), then daily challenge + emoji share grid; keyboard guess placement on the Leaflet map. +- Uncommitted; user to review and commit. + +> Historical work record — not durable authority. Prefer docs/specs/ADRs for current decisions. diff --git a/plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md b/plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md new file mode 100644 index 0000000..7282e69 --- /dev/null +++ b/plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md @@ -0,0 +1,282 @@ +# Code Review — UI/UX Quick Wins (working tree, 2026-08-31) + +Scope: `git diff` (13 files) + untracked `src/lib/last-region.js`. ~396 insertions. +Plan: `plans/260831-1906-uiux-quick-wins/plan.md`. Advisory only; no code modified. + +Gates re-run: `npx vitest run` 240/240 (15 files); `npx eslint .` 0 errors / 16 +warnings. E2E and build not re-run per instruction; build artifacts in `.next` +were inspected and are current (contain this diff's strings). + +## Overall + +The state split, error panel, prefetch and last-region work is coherent and the +round state machine survives the walk: no dead loading states, no stale-state +leak between rounds, no double-submit, no scoring hazard from session-id reuse. +Two substantive issues: a leaderboard fairness regression created by scoring +against the picked region while still crediting the resolved district, and a +new response field plus a new UI block that no test exercises. + +--- + +## High + +### H1 — Region-relative scoring credits district boards with country-scale scores + +`src/app/api/guess/route.js:68-74` derives the ladder from the **picked** +region. `src/lib/leaderboard.js:187` (`submitScore`) still fans that score out +to the **resolved** district, its province and the country +(`ancestorsOf(regionCode)` where `regionCode = session.regionCode`). + +Measured against the real generated bboxes: + +| Board credited | 5 pts if you picked that district | 5 pts if you picked Vietnam | +|---|---|---| +| `leaderboard:city:tphcm-q7` | ≤ 62 m | ≤ 6,380 m (~100x) | +| `leaderboard:city:tphcm` | ≤ 442 m | ≤ 6,380 m (~14x) | + +The cheapest route to the top of any district board is now to play the country +round repeatedly and let the fan-out credit whatever district you land in. This +also holds district-to-district on a province board: `DN-HOAVANG` gets 273 m for +5 points, `HN-BADINH` gets 50 m (factor clamps at 1). + +The plan's accepted risk reads "score semantics change for province/country +rounds (accepted; boards mix eras)" — that covers old-vs-new scores over time, +not this permanent per-round asymmetry within the new scheme. Treating it as +covered would be reading more into the user's acceptance than it says. + +Options (user decision — do not pick silently): + +- **(a) Per-level ladders.** Score each credited level against that level's own + bbox inside the existing fan-out. `submitScore` already iterates + `ancestorsOf(regionCode)`; pass a scorer instead of a number: + ```js + // leaderboard.js + export async function submitScore(username, scoreFor, regionCode) { // scoreFor(code) -> 0..5 + const levels = await Promise.all( + ancestorsOf(regionCode).map((code) => creditScore(h, code, scoreFor(code), trimmedUsername)) + ); + ``` + with `scoreFor = (code) => calculateScore(distance, bandsForBbox(getRegion(code).bbox))` + in the route. Each board then means "how good was this guess for this region". + Note this changes the country board's meaning too. +- **(b) Display only.** Return `bands` and show the scaled ladder in the reveal, + but credit the boards with `SCORE_BANDS`. Keeps boards untouched; the reveal + then shows a score that does not match what the board got, so `gameResult.score` + would need splitting into `displayScore` / `boardScore`. +- **(c) Accept and document.** Add the asymmetry to `docs/features.md` explicitly + so it is a stated rule rather than an emergent one. + +## Medium-High + +### H2 — New `bands` contract not reflected in the e2e stub; band chips untested + +`tests/e2e/helpers.js:36-68` `guessResponse()` returns no `bands`. +`src/app/components/RoundResultDialog.js:121` gates the entire band strip on +`Array.isArray(result.bands) && result.bands.length > 0`, so the new UI block +renders in zero tests. This repo has no component tests, so e2e is the only +place it could be proven. + +The helper's own header (lines 6-7) states: "The stub payloads mirror the real +route response shapes; a contract change should be made in the route tests +first, then reflected here." The route test was added +(`tests/guess-route.test.js:140`); the stub was not updated. + +Fix: +```js +// tests/e2e/helpers.js, inside gameResult +bands: [ + { maxMeters: 442, points: 5 }, { maxMeters: 885, points: 4 }, + { maxMeters: 1770, points: 3 }, { maxMeters: 4425, points: 2 }, + { maxMeters: 8849, points: 1 }, +], +``` +```js +// tests/e2e/game.spec.js +await expect(dialog.getByText('≤442m = 5')).toBeVisible(); +await expect(dialog.getByText('beyond = 0')).toBeVisible(); +``` + +Related phantom coverage in the same file: `newGameResponse()` hands out the +same `PANO_IMAGE_URL` for every round, so after Next Round `imageData.url` is +unchanged, `PanoramaViewer`'s `key` does not change, and the viewer never +remounts. The "next round loads a fresh round" assertion therefore passes +without a new panorama ever mounting. Give round 2 a distinct URL and stub it. + +## Medium + +### M1 — `handleNextRound` has no re-entrancy guard; the dialog stays clickable for 200 ms + +`src/app/components/GameClient.js:251`. `src/components/ui/dialog.jsx` +`DialogContent` carries `data-[state=closed]:animate-out ... duration-200`, so +Radix Presence keeps the content mounted and interactive through the close +animation. A double-click fires `handleNextRound` twice; the second call finds +`prefetchRef.current === null` (nulled at line 257) and issues a second +`/api/new-game`, creating a second server session. + +Not a scoring hazard — `applyRound` (line 89) sets `sessionId` and `imageData` +together, so whichever promise resolves last leaves a consistent pair. Cost is +a wasted Neon/Mapillary round trip plus a wasted Redis session per double-click, +and a non-deterministic choice of panorama. + +Fix: guard with a ref, or pass `roundLoading` into `RoundResultDialog` and set +`disabled={roundLoading}` on Next Round. + +### M2 — Between-rounds spinner clears before the panorama is visible; `handlePanoramaReady` is now dead + +`GameClient.js:262, 271, 295, 302` all call `setRoundLoading(false)` as soon as +`/api/new-game` returns. `applyRound` swaps `imageData.url` in the same batch, +remounting `PanoramaViewer` (`key={imageData.url}`), which renders a bare black +container with no indicator until PSV fires `ready`. `handlePanoramaReady` +(line 174) consequently has nothing left to clear. + +This matches pre-change behaviour (the old `getRandomImage` also cleared +`loading` on data arrival), so it is not a regression — but it undercuts the +stated goal of the split, which was for the spinner to cover the transition. + +If you want ready-driven clearing, clear only on the failure path: +```js +const ok = await loadRound(location, currentSession); +if (!ok) setRoundLoading(false); // success: handlePanoramaReady/Error clears it +``` +**Trap if you do:** when the next round returns the same `imageData.url`, the +`key` does not change, the viewer does not remount, `onReady` never fires again, +and `roundLoading` sticks true forever with Submit and Skip both disabled — a +real dead state. Key the viewer on a per-round token (`sessionId`, or a counter) +rather than the URL before making this change. + +### M3 — `role="status"` inside a subtree that mounts all at once is not reliably announced + +`src/app/components/RoundResultDialog.js:97-99`. A live region inserted together +with its content is generally not announced — the AT observes the region +appearing, not a change inside an existing region — and +`DialogContent key={open ? 'open' : 'closed'}` (line 60) guarantees a full +remount every time. The comment at lines 94-96 claims screen readers get the +numbers once; that is not dependable. + +The dialog's `aria-describedby` → `DialogDescription` **is** announced on open. +Fold the outcome into it and drop the separate paragraph — DRY, and it removes +the text collision that forced `exact: true` at `tests/e2e/game.spec.js:37`: + +```jsx + + {result?.failed + ? 'The guess could not be saved and nothing was scored.' + : `Scored ${score} of 5 points, ${formatDistance(result.distance)} away.`} + +``` + +## Low + +### L1 — Home scoring table now reads "500m-1.00km = 1 pt" / "1.00km+ = 0 pts" + +`src/app/page.js:27-33`. `formatDistance(1000)` returns `'1.00km'`; the replaced +hardcoded copy said `1km`. Deriving from `SCORE_BANDS` is right; `formatDistance` +is a measured-distance formatter, not a threshold formatter. Add a local +`threshold(m)` that trims trailing zeros if the cosmetic change matters. + +### L2 — Distance-board colours still use the district ladder + +`src/app/components/LeaderboardList.js:18` calls `calculateScore(distance)` with +the default bands to pick a colour. Now that the awarded score depends on the +picked region, that colour corresponds to no score anyone earned: a country-round +3 km entry renders in the 0-point colour while it actually scored 4. The board +stores no record of which ladder applied, so this cannot be fixed locally. +Either accept it as a purely absolute "accuracy" scale, or drop the colouring. + +Either way, the comment at `src/lib/game.js:20-22` ("anything that bands a result +(colors, labels, the scoring table) should derive from calculateScore") now +overstates the invariant — there is no longer one ladder. Worth correcting since +that comment is the reason a future change will trust it. + +### L3 — `bandsForBbox` has no finite-value guard (currently unreachable) + +`src/lib/game.js:59-63`. A malformed bbox yields `NaN` thresholds, and +`calculateScore` then scores every round 0 silently rather than failing. +Verified unreachable today: of 67 regions exactly one (`TPHCM-CUCHI`) has no +bbox and it is not playable, so no playable region reaches the `SCORE_BANDS` +fallback. A `Number.isFinite(diagonal)` check would make this fail-safe rather +than fail-silent if a future boundary rebuild drops a bbox. + +### L4 — One new lint warning + +`src/app/components/RegionPicker.js:112` adds a 16th +`react-hooks/set-state-in-effect` warning. It matches the established local +pattern (`ThemeToggle.js:20`, `page.js:43`, `MapSearchBox.js:42`), and reading +localStorage in an effect is the correct call to avoid a hydration mismatch. +Noted only because a new warning is normally a signal in this repo. + +--- + +## Verified clean + +- **Anti-cheat.** `bands` derives from `session.pickedRegion ?? session.cityCode` + (`guess/route.js:68`) — the region the player chose and already knows, whose + bbox is already in the client bundle via `regions.js`. `bandsForBbox` is a pure + function of that bbox, so the client can invert nothing about the resolved + district from it. `session.regionCode` still reaches the client only as + `publicRegion(scoringRegion)` after the session is atomically consumed + (`route.js:93`). No new leak, no cheaper extraction path. +- **Contracts.** `/api/guess` gained `gameResult.bands` only; `/api/new-game` + unchanged. `calculateScore(distance)` default preserves the old ladder, and the + one other caller (`LeaderboardList.js:18`) is unaffected. Leaderboard keys and + the 0-5 integer scale untouched: `bandsForDiagonal` passes `points` through and + `factor >= 1`, so the ladder stays strictly ascending and rounding never + collapses two thresholds. +- **Client-safety boundary.** `last-region.js` imports nothing; `game.js` imports + only `@turf/turf`. Neither `pano-index` nor `pano-db` is reachable from client + code. Bundle checked in `.next/static/chunks`: turf is fully tree-shaken + (`@turf/turf` declares `sideEffects: false`; no client chunk contains + `booleanPointInPolygon`, `earthRadius`, or `Failed to calculate distance`), so + the new `page.js → lib/game` import costs no client bytes. +- **Leaflet icons.** Build output confirms the static import resolves to a bare + URL string under this bundler (`e.q("/_next/static/media/marker-icon.1le94j_pe_ih1.png")`), + and `marker-icon`, `marker-icon-2x`, `marker-shadow` are all emitted to + `.next/static/media`. `imageUrl()`'s object branch is the unused webpack + fallback — cheap, keep it. The `ResultMap.js` removal is safe: that map's only + markers are divIcons. +- **`lg:static` → `lg:relative`** (`GameClient.js:364`) is not scope drift — the + new `absolute inset-0` roundLoading overlay needs a positioned ancestor at lg. +- **No dead loading states.** Every `setRoundLoading(true)` (lines 258, 293, 300) + is matched by a clear on both branches; `initialLoading` clears in `finally`. + `loadError` is recoverable via Retry (`handleRetryLoad`, deliberately passes no + session id) and via Skip (`if (!imageData && !loadError)` now lets Skip through + during an error). +- **Session / prefetch semantics.** No double-submit: `handleSubmitGuess` is + guarded by `submitting` and the server claims the session with an atomic DEL. + Prefetch reuses the just-consumed id purely as the storage key for a fresh + round (`new-game/route.js:52-64` overwrites), and the client's `sessionId` + always comes from the response. On the failed-submit path the still-live old + session is overwritten with the new round rather than orphaned. Leaving via + Menu strands one session that expires on its own, as the comment states. +- **Round reset.** `resetRoundState` clears `guessCoordinates`, `result` and + `mapExpanded` on both Next Round and Skip; `prefetchRef` is cleared on Skip + (line 290) and consumed exactly once on Next (256-257). No leak found. +- **Input validation.** `getLastRegion()` output passes `isRegion` + `isPlayable` + before reaching `getRegion()` or the href (`RegionPicker.js:111`), so a + hand-edited localStorage value cannot inject into the URL or throw. +- **Docs.** `bandsForBbox` grep-verified to exist (`game.js:59`) and to be called + from `guess/route.js:70`, as both docs claim. `README.md` has no threshold + table (only "0-5 point system"), so nothing went stale there. + +## Acceptance criteria vs plan + +| Phase | State | +|---|---| +| 1 Region-relative scoring | Implemented and tested; see H1 for the fairness consequence | +| 2 GameClient split / error / prefetch / last-region | Implemented; M1, M2 | +| 3 Result dialog | Implemented; M3, and H2 (chips untested) | +| 4 Home + Continue row | Implemented; L1 | +| 5 Map robustness | Implemented, verified against build output | +| 6 Verify + docs | Gates green (240/240, 0 lint errors); docs accurate | + +Runtime-only criteria — no gate in this repo can prove them, manual check needed: +the panorama does not unmount on submit; the Retry panel actually recovers; the +prefetched round visibly swaps in without a wait; the "Continue in X" row +appears; Leaflet markers render without network access to a CDN. + +## Unresolved questions + +1. H1: is the district/province board asymmetry accepted, or should scoring move + to per-level ladders? This is the only blocking decision. +2. L2: should distance-board colouring keep an absolute district scale now that + awarded scores are relative? diff --git a/plans/reports/code-reviewer-260831-2117-breaking-change-recheck.md b/plans/reports/code-reviewer-260831-2117-breaking-change-recheck.md new file mode 100644 index 0000000..50ece7d --- /dev/null +++ b/plans/reports/code-reviewer-260831-2117-breaking-change-recheck.md @@ -0,0 +1,317 @@ +# Breaking-Change Recheck — working tree, 2026-08-31 21:17 + +Adversarial regression pass over the NEW deltas since +`plans/reports/code-reviewer-260831-1906-uiux-quick-wins.md`. Advisory only; no +code modified. Read the diff directly rather than the delegation summary. + +Gates re-run here: `npx vitest run` → 242 passed / 15 files. `npx eslint .` → +0 errors, 16 warnings (same 16; `use-count-up.js:26` and `page.js:45` shown as +the tail). E2E and build not re-run per instruction. + +Verdict: **no contract break, no data-loss path, no new trust-boundary leak.** +One real state-machine hole (H1), one product-level consequence worth an +explicit sign-off (M1), one client-bundle fact that changed (M3). + +--- + +## Critical + +None. + +## High + +### H1 — The 15s watchdog reopens the controls while a round fetch is still in flight, and `applyRound` has no generation guard + +`GameClient.js:193-197` clears `roundLoading` unconditionally after 15s. +`GameClient.js:279-288` is still parked on `await prefetched` at that moment, and +`applyRound` (`GameClient.js:94-102`) applies whatever eventually resolves with +no check that it is still the round the user is waiting for. + +Sequence: + +1. Submit → `startPrefetch` (`:255`) issues `/api/new-game`; the fetch stalls + (Neon cold start, Mapillary slow, mobile handover). No timeout anywhere. +2. Next Round → `roundLoading = true`, `sessionId = null`, awaiting the prefetch. +3. 15s → watchdog clears `roundLoading` → Skip (`:454`) and Retry (`:393`) + re-enable. +4. User clicks Skip → `loadRound` → `applyRound(roundB)` → viewer shows B. +5. Prefetch resolves at t=20s → `handleNextRound` resumes → `applyRound(roundA)` + → `sessionId`/`imageData`/`roundKey` all replaced by A. + +Consequences: the panorama swaps under the player unprompted, round B's session +is orphaned for 30 minutes, and `applyRound`'s `setLoadError(null)` (`:101`) +silently dismisses the "Couldn't load a street view image" panel if step 4 had +failed. No scoring hazard — `applyRound` writes `sessionId` and `imageData` in +one batch, so the pair stays consistent and Submit is disabled anyway until a +fresh map click. + +The same shape without the prefetch: watchdog fires while `handleSkipGuess`'s +`loadRound` is in flight (`:314`), user clicks Skip again, two `loadRound`s race, +last write wins, first session orphaned. The watchdog is the only thing that +opens this window — before it, every entry point was mutually excluded by +`roundLoading`. + +Fix (one epoch counter, no new abstraction): + +```js +const roundEpochRef = useRef(0); +// handleNextRound / handleSkipGuess / handleRetryLoad, before starting: +const epoch = ++roundEpochRef.current; +// applyRound and the loadError branch, before writing: +if (epoch !== roundEpochRef.current) return; +``` + +Then the watchdog only re-enables controls; it cannot let a stale round land. + +## Medium + +### M1 — Per-level ladders make the Vietnam board a rounds-played counter, and inflate new points against banked ones + +Measured against the real generated bboxes: + +| Board | 5 pts | 4 | 3 | 2 | 1 | +|---|---|---|---|---|---| +| `VN` (country) | ≤6,380 m | 12,759 | 25,519 | 63,797 | 127,593 | +| `TPHCM` | ≤442 m | 885 | 1,770 | 4,425 | 8,849 | +| `HN` | ≤594 m | 1,187 | 2,374 | 5,936 | 11,872 | +| `TPHCM-Q7` | ≤62 m | 123 | 247 | 617 | 1,234 | +| `HN-BADINH` | ≤50 m | 100 | 200 | 500 | 1,000 | + +The old asymmetry is genuinely gone: `submitRoundScore` (`leaderboard.js:245-268`) +credits each board from the raw distance against that board's own bbox, so a +country round can no longer buy district points — `guess-route.test.js:140-172` +pins exactly that property. Accepted fix, correctly implemented. + +The consequence to sign off on: **any** guess within 6.38 km now adds +5 to the +national board, whatever region was picked. A district player who lands within +6 km — which is almost every honest district guess — banks +5 nationally every +round. The Vietnam leaderboard stops measuring accuracy and starts measuring +volume, and it mixes with points banked under the old ladder where +5 needed +50 m. `docs/features.md:47-69` states the era mixing qualitatively ("Scores +earned before this change stay on the boards under the old absolute ladder") but +not that the country board is now effectively uncapped by skill. + +This is a product decision, not a defect — flagging it for an explicit yes, +because it is a second-order effect of the accepted fix rather than something the +user approved directly. If it is unwanted, the lever is +`REFERENCE_DIAGONAL_METERS` (`game.js:39`) or a sub-linear factor +(`Math.sqrt`) in `bandsForDiagonal` (`game.js:46-52`); either is a one-line +change confined to `game.js`, and both are user decisions. + +### M2 — `gameResult.score` keeps its name and shape but is now credited to no board + +`guess/route.js:74` grades against the picked region; `:105` credits the boards +per level. The field is additive-compatible (shape unchanged), so this is not a +contract break, but its meaning changed from "the points added to every level" +to "a display-only grade". Three follow-ons: + +- **Deploy skew, old cached client + new server:** an old bundle renders the + headline score and `levels[].score` totals, and ignores the new `points`. It + will show "3" for a round where the district board received 0, with no + breakdown to explain it. Cosmetic, no crash — the new client is guarded + (`GameClient.js:233` `submitted.bands ?? null`, + `RoundResultDialog.js:172` `typeof entry.points === 'number'`). +- **New client, in-flight session from before the deploy:** covered. + `session.pickedRegion ?? session.cityCode ?? null` for the ladder + (`route.js:68`) and `session.regionCode ?? session.cityCode` for the fan-out + (`route.js:83`); a legacy `cityCode` that is not a region code falls through + `isRegion` to `SCORE_BANDS` rather than throwing. +- **District round whose panorama resolves elsewhere:** the pano is drawn from + the picked district but the district is resolved by polygon, so a border pano + can resolve to a neighbour or fall out to province level. Then the picked + region is not in `ancestorsOf(scoringRegion)` and the headline score matches + no credited board at all. Rare, cosmetic, worth one sentence in the docs if + anyone asks. + +### M3 — Turf now ships to the browser (the prior review's "fully tree-shaken" claim is stale) + +`LeaderboardList.js:45-49` calls `bandsForBbox` → `calculateDistance` +(`game.js:3-18`) → `turf.point` / `turf.distance` from a `"use client"` module. +Verified in the current build (`.next/BUILD_ID` 20:46:06 postdates +`LeaderboardList.js` 20:45:01, so the artifact includes this delta): +`.next/static/chunks/02wfjqj_i3an0.js` contains both `Failed to calculate +distance` and `6371008.8`. + +Cost is small — tree-shaking held (no `booleanPointInPolygon`, `convex`, +`voronoi`, `clustersKmeans` in any client chunk; the chunk is 28 KB raw). Report +it as a change of state, not a defect: the previous audited invariant "no turf in +the client bundle" is gone, and `@turf/turf`'s `sideEffects: false` is now +load-bearing for client payload size. If that matters, the two-line alternative +is a bbox-diagonal helper that does the equirectangular math inline for the +tint, leaving turf server-side. + +## Low + +- **L1 — `handleNextRound`'s re-entrancy guard reads state, not a ref.** + `GameClient.js:270` `if (roundLoading) return`. Correct only because React + flushes discrete click events between renders; two clicks dispatched inside one + task both read `false` from the same closure. A ref (or the H1 epoch) is + unconditional; `disabled={roundLoading}` on the dialog button is the cheaper + version. +- **L2 — `creditScore` JSDoc drifted from its signature.** `leaderboard.js:134` + still documents `@param {number} score`; the parameter is `points` + (`:138`). +- **L3 — New comment is already wrong.** `tests/e2e/helpers.js:32-33` says "the + viewer remounts on the URL"; it remounts on `roundKey` + (`GameClient.js:405`, `key={roundKey}`). The per-round URL is still worth + keeping — for realism and to exercise the image route — but not for the reason + stated. +- **L4 — `submitRoundScore` is absent from the unknown-region table.** + `tests/leaderboard.test.js:290-293` covers `submitScore`, + `submitDistanceRecord`, `getLeaderboard`. `submitRoundScore` is the only + fan-out entry point `/api/guess` actually calls and its `requireRegion` path + is untested; one array row closes it. +- **L5 — The default distance board is now almost uniformly green.** + `LeaderboardList.js:47` grades against the displayed board's ladder and + `LeaderboardModal.js:19` opens on `COUNTRY_CODE`, where 5 points is ≤6.38 km. + Consistent with how that board is credited (this is the fix for the prior + L2), so intended — but the tint no longer discriminates on the tab users land + on first. +- **L6 — `page.js` label cosmetics.** `label()` (`page.js:29`) trims only the + exact `.00km`, so the first row now reads `0m-50m` (was `0-50m`) and a future + 2,500 m threshold would render `2.50km`. Inert for the current bands. +- **L7 — Pre-existing, not a regression: Skip's DEL/SET race.** + `GameClient.js:298-303` fires `/api/skip` without awaiting, then reuses the + same session id for the new round (`:314`). If the DEL lands after new-game's + SET, the fresh round's session is gone and the next submit reads as + "Round Not Recorded". Byte-identical ordering in the pre-change code; noted + once so it is not rediscovered as new. +- **L8 — Pre-existing: the first round still has no spinner over the viewer.** + `initialLoading` clears when `/api/new-game` returns + (`GameClient.js:136`), not when the texture is up, and `roundLoading` is never + raised for the initial load. Matches old behaviour; the `roundKey` and + watchdog work does not touch this path. +- **L9 — The migration script grew a transitive turf dependency.** + `scripts/lib/leaderboard-migration.mjs:14` → `leaderboard.js:11` → + `game.js:1`. Verified it still imports cleanly under this Node + (`BACKFILLS,PATTERNS,backfillPairs,copySortedSet,exportAll,findRegressions,restore,verifyTargets`) + and `tests/migrate-leaderboards.test.js` passes. `@turf/turf` is a prod + dependency, so no packaging risk. + +--- + +## Round state machine — exhaustive walk (check b) + +Every `setRoundLoading(true)` and its clearing paths: + +| Entry | Line | Success clears via | Failure clears via | Stall clears via | +|---|---|---|---|---| +| `handleNextRound`, prefetch hit | `:277` → `:281` | `applyRound` bumps `roundKey` (`:100`) → viewer remounts (`:405`) → `ready` → `handlePanoramaReady` (`:180`) | prefetch rejects → `catch {}` → falls to `loadRound`; `if (!loaded) setRoundLoading(false)` (`:291`) | watchdog `:195` | +| `handleNextRound`, no prefetch | `:277` → `:290` | same remount → `ready` | `:291` | watchdog | +| `handleSkipGuess` | `:313` | same remount → `ready` | `:315` | watchdog | +| `handleRetryLoad` | `:320` | same remount → `ready` | `:322` | watchdog | + +- **`roundKey` closes the M2 trap from the last review.** The viewer keys on + `roundKey` (`:405`), not on the URL, and `PanoramaViewer`'s effect deps are + `[imageUrl]` (`PanoramaViewer.js:97`) — so a repeated panorama URL still + remounts and still fires `ready`. Without this, the ready-driven clear would + have been a permanent dead state. +- **`panorama-error` also clears.** `PanoramaViewer.js:67-71` calls `onReady` + before/alongside the fallback ``, and the constructor's `catch` + (`:72-77`) calls both `onReady` and `onError`. The only path that fires + neither is the never-settling texture promise the file's own comment describes + (`:22-25`) — which is exactly what the watchdog is for. +- **No permanently disabled Submit/Skip.** Submit is + `!guessCoordinates || !imageData || submitting || roundLoading` (`:445`); + `submitting` always clears at `:253` (outside the try/catch), `roundLoading` + per the table, `imageData` is null only in the `loadError` state, which shows + Retry + Back and lets Skip through (`:295` `if (!imageData && !loadError)`). +- **Prefetch consumed exactly once.** `prefetchRef.current` is read and nulled + synchronously (`:275-276`), cleared on Skip (`:310`), and the abandoned + promise has a detached `.catch` (`:216`) so it cannot surface as an unhandled + rejection. Double-fetch on rapid Next Round: guarded, with the caveat in L1. +- **No stale state across rounds.** `resetRoundState` (`:261-265`) clears + `mapExpanded`, `guessCoordinates`, `result` on both Next and Skip; + `applyRound` clears `loadError`; `roundKey` guarantees a fresh viewer. +- **`initialLoading` untouched by `roundKey`/watchdog.** The watchdog is inert + while `roundLoading` is false (`:194`), the initial path never raises it, and + `initialLoading` clears in `finally` (`:136`). The `ready` from the first round + hits a no-op `setRoundLoading(false)`. + +## Contract audit (check a) + +- **`/api/new-game`:** no diff. Response shape identical. +- **`/api/guess`:** additive only — `gameResult.bands` (`route.js:127`) and + `levels[].points` (`leaderboard.js:158`). `distance`, `score`, `levels`, + `distanceLevels`, `region`, `globalRank`, `cityRank`, `globalDistanceRank`, + `cityDistanceRank`, `exactLocation`, `leaderboard`, `distance`, `message` all + unchanged in name and type. `leaderboard.message` text changed + (`+3` → `+3, +5, +5`); it is rendered, never parsed + (`RoundResultDialog` via `leaderboardMessage`). +- **`submitScore`:** signature, semantics and message format preserved + (`leaderboard.js:211-230`); it now delegates to `fanOutScore` with a constant + `pointsFor`. Remaining callers: tests only (grep across `src`, `scripts`, + `tests` — no production caller left). Keeping it is defensible as the flat + primitive, but it is now dead production code; if nothing plans to use it, + deleting it and its tests is the DRY call. +- **Redis keys and encodings:** `getRegionLeaderboardKey` / + `getDistanceLeaderboardKey` untouched (`:51-64`), country still maps to + `leaderboard:vietnam`, score boards still store an integer total keyed by the + bare username (`creditScore:143`), distance entries still + `username:distance:timestamp` (`:318`). `leaderboardKeys` export intact + (`:69-72`). +- **Migration script:** consumes only `leaderboardKeys`; unaffected by the + `fanOutScore` refactor. `tests/migrate-leaderboards.test.js` green, live + import verified (L9). +- **`fanOutScore` semantics vs the old inline body:** identical — + `Promise.all(ancestorsOf(regionCode).map(...))`, same `byLevel` aliases + (`district`/`province`/`global`/`city`), same `success: true`. `pointsFor` is + evaluated synchronously per level before its credit call, so no interleaving + changed. +- **`submitRoundScore` failure mode:** `requireRegion` (`:252`) throws before any + Redis write, and the route consumes the session at `:93` before calling it at + `:105` — so an unknown region still loses the round with a 500. Identical to + `submitScore`'s pre-existing behaviour; not worsened. `distance === undefined` + (not falsy) preserves a legitimate 0 — pinned by + `tests/leaderboard.test.js:268-271`. A non-numeric distance now yields 0 + points instead of pushing `NaN` into `zAdd`: a small improvement. + +## Client-safety boundary (check d) + +- No client module imports `lib/leaderboard.js` (only `api/guess`, + `api/leaderboard`, `scripts`, tests) — so `leaderboard.js:11`'s new + `game.js` import does not cross the boundary. +- `LeaderboardList.js` (client) → `lib/game.js` (→ `@turf/turf` only) and + `lib/regions.js` (→ `data/regions/index.js`, `data/regions/counts.js`). No + path to `pano-index`, `pano-db`, `data/panos`, or `data/boundaries`. +- `tests/regions.test.js:199-229` still meaningful: the walk from `regions.js` + asserts the exact module set, and `:231-249` walks every `"use client"` file + for the four forbidden path fragments. Both green in this run. Neither test + says anything about *bundle size*, which is why M3 slipped past them. + +## Anti-cheat re-check + +Unchanged from the prior pass and re-verified against the new deltas: +`bands` derives from `session.pickedRegion` (`route.js:68-71`), a value the +player chose and whose bbox is already in the client bundle via `regions.js`; +`bandsForBbox` is a pure function of it. `levels[].points` is only emitted after +the atomic `deleteGameSession` claim (`:93`) and alongside `code`/`name`/`path` +that already reveal the resolved district. No new pre-guess signal. + +## Metrics + +- Tests: 242 passed / 15 files (`npx vitest run`). +- Lint: 0 errors, 16 warnings — all `react-hooks/set-state-in-effect`, matching + the established localStorage/animation pattern; no new warning in this delta. +- Type coverage: n/a (JavaScript-only repo, no checker configured). + +## Recommended actions + +1. Add the epoch guard from H1 — the only defect that can put a stale round on + screen. +2. Confirm M1 explicitly (country board = volume board) or turn the + `REFERENCE_DIAGONAL_METERS` / sub-linear-factor lever. User decision. +3. Decide on M3: accept turf in the client bundle, or inline the diagonal math + for the board tint. +4. Sweep L2-L4 (JSDoc param, stale e2e comment, `submitRoundScore` in the + unknown-region table) — five minutes, all in touched files. +5. Decide whether `submitScore` earns its keep now that no production code calls + it. + +## Unresolved questions + +1. M1: is the country board becoming a rounds-played counter acceptable, or + should the scale factor be sub-linear? +2. M3: does turf in the client bundle matter enough to inline the tint's + diagonal math? +3. Is `submitScore` retained for a planned caller, or is it dead code to remove? diff --git a/plans/reports/code-reviewer-260831-2145-client-state-recheck.md b/plans/reports/code-reviewer-260831-2145-client-state-recheck.md new file mode 100644 index 0000000..a75e5ff --- /dev/null +++ b/plans/reports/code-reviewer-260831-2145-client-state-recheck.md @@ -0,0 +1,315 @@ +# Client Round State Machine — Recheck (working tree, 2026-08-31 21:45) + +Adversarial read-only pass over the `roundKey` / epoch / watchdog rework in +`src/app/components/GameClient.js` + `RoundResultDialog.js`, read against +`PanoramaViewer.js` and `GuessMapPanel.js` (both unchanged). Advisory only; no +code modified. + +Gate re-run: `npx vitest run` → **243 passed / 15 files**. E2E and build not run +per instruction. React 19.2.8, Radix Dialog 1.1.19, Next 16.3.3. + +Prior H1 (epoch guard) from `code-reviewer-260831-2117-breaking-change-recheck.md` +is implemented and closes the branch it was written for (watchdog fires → player +starts another load → stale result discarded). It does **not** close the branch +where the player starts no further load, which is H1 below. + +Verdict: no contract break, no crash path, no permanently-stuck `roundLoading`. +One real scoring hazard (H1) and three state-consistency holes, all in the same +post-watchdog window and all fixable with four small edits. + +--- + +## Critical + +None. + +## High + +### H1 — The watchdog releases the controls but never invalidates the in-flight fetch: a stale round lands with the player's guess still attached + +`GameClient.js:203-207` (watchdog), `GameClient.js:99-107` (`applyRound`), +`GameClient.js:109-125` (`loadRound`). + +`roundEpochRef` is bumped only by load-*starting* actions (`:136`, `:281`, +`:327`, `:337`). The watchdog bumps nothing. So when the player takes no further +action after the watchdog fires, the in-flight load still carries the current +epoch and `applyRound` runs. + +Sequence (fetch stalls; `fetch()` has no `AbortSignal`, browser default is +minutes, so >15 s on mobile is ordinary): + +1. Skip (or Next Round) → `roundLoading = true`, `setSessionId(null)` (`:329`), + `imageData` still round A's → viewer still shows A. +2. t=15 s → watchdog → `roundLoading = false`. Submit/Skip re-enable; the + between-rounds overlay (`:435`) disappears. **The guess map was never + disabled** (`GuessMapPanel` takes no loading prop), so it was clickable the + whole time. +3. Player, looking at panorama A, clicks the map → `guessCoordinates` = a guess + for A. +4. t=20 s → fetch resolves → epoch matches → `applyRound(roundB)` → `imageData`, + `sessionId`, `roundKey` all become B. `guessCoordinates` is **not** cleared — + `resetRoundState` (`:271`) ran back at step 1, before the guess existed. +5. `roundLoading` is already false and is never re-raised, so Submit is enabled + immediately. Player clicks Submit → `/api/guess` scores **A's guess against + B's session**, and credits B's resolved district on the leaderboards. + +Impact: a leaderboard entry the player did not make, plus the panorama swapping +under them unprompted. This is exactly the invariant the epoch work was meant to +hold ("no stale result/guess leaking into the next round"), and it is open on the +no-further-action path. + +Same shape, second entry point: `handleRetryLoad` (`:335-341`) never calls +`resetRoundState`, so a guess placed while the error panel is up (the map is live +there too; Submit is only blocked by `!imageData`) survives into the retried +round. + +Fix — one line closes both, and is a no-op on every happy path because +`resetRoundState` already ran: + +```js +const applyRound = useCallback((data) => { + setSessionId(data.sessionId); + setImageData({ url: data.imageData.url, isPano: data.imageData.isPano }); + setRoundKey((key) => key + 1); + // A round that arrives after the watchdog gave the controls back (or after a + // retry) must never inherit a guess aimed at the panorama it replaces. + setGuessCoordinates(null); + setLoadError(null); +}, []); +``` + +Stronger option, if you want the watchdog to actually abandon the load rather +than race it: track a `fetchInFlightRef`, and in the watchdog do +`roundEpochRef.current += 1; setLoadError('That round took too long to load.');` +only when a fetch is in flight (when it is not, the stall is the viewer's texture +and the current behaviour — hand the controls back, keep the panorama — is +right). Note the current invariant "every epoch bump owns `roundLoading = true`" +becomes "…or has already cleared it"; both are safe, since `loadRound` returning +`true` for a stale epoch means "nothing to clear". + +## Medium + +### M1 — Submit is gated on `imageData` but not on `sessionId`, and the two are unpaired for the whole load window + +`GameClient.js:463` (`disabled={!guessCoordinates || !imageData || submitting || roundLoading}`) +and `GameClient.js:231` (same condition in the handler). + +Between `setSessionId(null)` (`:285`, `:329`) and `applyRound`, `sessionId` is +null while `imageData` still holds the previous round. `roundLoading` normally +covers that window — except after the watchdog (H1 step 2), and except after M3 +below. Submit then runs with `sessionId === null`, `submitGameResult` bails at +`:162` and returns null, and the player gets the **"Round Not Recorded"** dialog +for a round that was never live — plus `startPrefetch(location, null)` (`:265`) +burns a fresh server session behind the dialog. + +Fix: add `!sessionId` to both the button's `disabled` and the handler's early +return. Cheap, and it makes the pairing invariant explicit rather than implied by +`roundLoading`. + +### M2 — Skip from the error state never clears `loadError`: the recovery looks dead until the fetch lands + +`GameClient.js:310-333`. `handleSkipGuess` now accepts the error state +(`if (!imageData && !loadError) return`, `:311`) but never resets `loadError`. +During the ensuing load: + +- the error panel keeps rendering (`:403`, `loadError` is checked first); +- the between-rounds spinner is suppressed by `roundLoading && !loadError` + (`:435`); +- "Try again" goes `disabled={roundLoading}` (`:411`). + +So the screen shows the same failure message with both buttons inert and no +progress indicator, for as long as the fetch takes. It recovers correctly +(`applyRound` clears `loadError`), but the feedback gap is indistinguishable from +a hang. `handleRetryLoad` does clear it (`:336`) — the two entry points +disagree. + +Fix: `setLoadError(null)` as the first line of `handleSkipGuess`'s load section +(after the `/api/skip` call), matching `handleRetryLoad`. + +### M3 — A late `ready` from the *previous* round's viewer clears the *next* round's `roundLoading` + +`GameClient.js:190-192` (`handlePanoramaReady`), `GameClient.js:421-426` +(`key={roundKey}`). + +`roundKey` only changes in `applyRound`, so from the start of a load until the +new data lands, the mounted viewer is still the previous round's instance. +`PanoramaViewer`'s `disposed` flag (`PanoramaViewer.js:64, 88`) only suppresses +`ready` from an *unmounted* run — this instance is very much mounted. If its +texture finishes during the load window (the exact scenario the watchdog exists +for: slow texture, watchdog fired, player pressed Skip, texture then completes), +`onReady` fires and clears the loading state of a round that has not arrived. + +Consequence: overlay vanishes, Submit/Skip re-enable mid-fetch → feeds directly +into M1, and a second Skip issues a parallel `/api/new-game` (the first is then +epoch-discarded, so no corruption — just an orphaned session). + +Fix — reuse the epoch, no new machinery: + +```js +const appliedEpochRef = useRef(0); +// inside applyRound (which only runs with a current epoch): +appliedEpochRef.current = roundEpochRef.current; + +const handlePanoramaReady = useCallback(() => { + // A viewer from the round being replaced must not clear its successor's wait. + if (appliedEpochRef.current !== roundEpochRef.current) return; + setRoundLoading(false); +}, []); +``` + +Apply the same guard to `handlePanoramaError` (`:194-197`). + +## Low + +- **L1 — Double announcement on the failed round.** `RoundResultDialog.js:90` + keeps `role="alert"` on the failure body while `:75-81` now carries the same + outcome in the `aria-describedby` description. Screen readers get it twice on + open. Drop the `role="alert"` — the description is the reliable channel and + the reason the previous `role="status"` block was removed. +- **L2 — `formatDistance(undefined)` renders "NaNkm".** `RoundResultDialog.js:79` + formats `result.distance` unconditionally. `/api/guess` always sends it, so + this is unreachable in prod today, but the sr-only string is the one place a + malformed payload would be spoken rather than seen. `typeof result.distance === 'number'` + guard, or omit the clause. +- **L3 — Epoch read inline vs captured.** `handleNextRound` captures + `const epoch = roundEpochRef.current` (`:282`); `handleSkipGuess` (`:331`), + `handleRetryLoad` (`:339`) and `loadLibrariesAndInitialize` (`:137`) pass + `roundEpochRef.current` directly as an argument. Correct today (evaluated + before the `await`), but it is one refactor away from reading the ref after a + suspension point. Make all four capture first. +- **L4 — Dead `try` around a fire-and-forget fetch.** `GameClient.js:313-323`: + `fetch()` does not throw synchronously here and already has `.catch`. The outer + `try/catch` can never fire. Pre-existing; noted because the nesting reads as if + it protects something. +- **L5 — Pre-existing, re-verified unchanged (prior L7/L8):** Skip's DEL/SET race + (`:315` unawaited `/api/skip` then `:331` reuses the same id) and the first + round having no spinner over the viewer (`initialLoading` clears when the fetch + returns, `roundLoading` is never raised for the initial load). Neither is + touched by this delta. + +--- + +## Interleaving walk + +`roundLoading` lifecycle, all entry points. "Watchdog" is armed by +`useEffect [roundLoading]` and is pending in every row. + +| Entry | Raises at | Clears on success | Clears on failure | Clears on stall | +|---|---|---|---|---| +| `handleNextRound`, prefetch hit | `:289` | `applyRound` → `roundKey`++ → viewer remount → `ready` (`:191`) | prefetch rejects → falls through → `if (!loaded) setRoundLoading(false)` (`:307`) | watchdog `:205` | +| `handleNextRound`, no/failed prefetch | `:289` | remount → `ready` | `:307` | watchdog | +| `handleSkipGuess` | `:330` | remount → `ready` | `:332` | watchdog | +| `handleRetryLoad` | `:338` | remount → `ready` | `:340` | watchdog | +| initial load | never raised | n/a (`initialLoading` clears in `finally`, `:146`) | `loadError` panel | n/a | + +Checked and clean: + +- **No dangling `roundLoading = true`.** Every epoch bump that can strand a load + (`:281`, `:327`, `:337`) also raises `roundLoading` in the same handler, so a + discarded load's "nothing to clear" is always true — the newer action owns the + flag and armed its own watchdog. The one bump that does not (`:136`, init) runs + under `initialLoading`, which clears in `finally`. +- **Watchdog cannot be short-changed.** The effect re-runs only on a + `false → true` transition, so the 15 s always starts when the wait starts. A + second `setRoundLoading(true)` while already true would inherit a partly-spent + timer — unreachable, because every entry point is either `disabled` on + `roundLoading` (`:411`, `:472`) or guarded (`:280`). +- **`roundKey` does its job.** Viewer keyed on the counter, `PanoramaViewer`'s + effect deps `[imageUrl]`, so a repeated panorama URL still remounts and still + fires `ready`. The dead state the previous review warned about is closed. +- **`panorama-error` clears too.** `PanoramaViewer.js:67-71` calls `onReady` + alongside the flat-image fallback; the constructor `catch` (`:72-77`) calls + both `onReady` and `onError`. The only path firing neither is the + never-settling texture promise — the watchdog's stated purpose. +- **Prefetch consumed exactly once.** Read-and-null is synchronous (`:287-288`), + Skip clears it (`:326`), the abandoned promise carries a detached `.catch` + (`:226`) so it cannot surface as an unhandled rejection. Menu leaves one + self-expiring session, as documented. +- **Submit is not concurrent with any load.** During `submitting`, Skip is + disabled (`:472`) and Retry is not rendered; `submitting` clears at `:263` + outside the try/catch, so no path leaves the button spinning. +- **Session/imageData pairing at rest.** `applyRound` writes both in one batch, + so whichever load wins leaves a consistent pair. The only unpaired window is + during a load — see M1. + +## Epoch guard completeness (check 2) + +Every writer of round-scoped state, and whether a stale epoch can reach it: + +| Writer | Site | Guard | +|---|---|---| +| `applyRound` (session + image + key + error) | `:115` via `loadRound` | `:114` ✓ | +| `applyRound` | `:295` prefetch resolve | `:294` ✓ | +| `setImageData(null)`/`setSessionId(null)`/`setLoadError` | `:120-122` | `:118` ✓ | +| prefetch **reject** → fall-through to `loadRound` | `:299-303` | `:302` ✓ — and the subsequent `loadRound(…, epoch)` re-checks with the same epoch, so a bump during the fresh fetch is caught too | +| `setLoadError(null)` | `:336` retry | synchronous in the handler, no await before it ✓ | +| `setRoundLoading(false)` | `:307`, `:332`, `:340` | reached only when `loadRound` returned `false`, which it never does for a stale epoch ✓ | +| `setRoundLoading(false)` | `:191`, `:196` viewer callbacks | **unguarded — M3** | +| `applyRound` after the watchdog with no newer action | `:115` | epoch still current by construction — **H1** | + +## React correctness (check 3) + +- **`router.push('/')` with a fetch in flight.** `handleGoBack` bumps no epoch, + so a resolving `loadRound`/prefetch may call `setState`. On React 19 that is a + silent no-op after unmount (the "update on unmounted component" warning was + removed in 18) and a harmless extra render if the navigation has not committed + yet. Nothing worse; no listener, timer or viewer is leaked — the watchdog and + `PanoramaViewer` both clean up in their effect returns. +- **Dep arrays.** `[roundLoading]` on the watchdog captures nothing else. + `[searchParams, loadLibrariesAndInitialize, initialized]` on the init effect: + `loadLibrariesAndInitialize` → `loadRound` → `applyRound` all have empty/stable + dep chains, so the identity is stable and the effect is driven only by the + `initialized` gate. +- **StrictMode.** The init effect's double-invoke is absorbed by + `initializingRef` (a ref survives the remount, state does too, so the second + run returns at `:128` and issues no second fetch). The watchdog effect's + double-invoke clears its own timer in the cleanup — one live timer. + `useCountUp`'s interval likewise. No duplicated `/api/new-game` in dev. +- **`handleNextRound`'s `if (roundLoading) return`** reads render state, not a + ref (prior L1, still open). Two clicks dispatched in one task would both read + `false`. Practically closed here because `DialogContent`'s + `key={open ? 'open' : 'closed'}` (`RoundResultDialog.js:60`) forces the content + to unmount the instant `open` flips rather than lingering through the exit + animation — but that is an accident of the key, not a guard. The M3 fix does + not help; use a ref or `disabled` on the button if you want it unconditional. + +## Dialog (check 4) + +- **Esc / overlay / close.** `showCloseButton={false}` removes the X; + `onOpenChange={() => {}}` swallows Radix's dismiss request. Radix still runs + its dismiss path (`onEscapeKeyDown` → `onOpenChange(false)`), the controlled + `open` stays true, nothing re-renders differently — no strand, no crash, no + focus-trap leak. Repeated Esc is idempotent. Two labelled exits remain + (Next Round, Menu), so the modal is not a dead end. +- **Count-up.** `useCountUp(score, open)` lives above `DialogContent`, so the + content's key flip does not reset it; `open → false` sets it to 0, and a repeat + score on the next round re-animates because `active` toggled. `failed` results + have `score = 0` and take the `!value` short-circuit — no timer spun. +- **aria.** The description carries the *final* score, not `shownScore`, so the + announcement never races the animation. It is always rendered (empty string + when `result` is null), so Radix never warns about a missing description, and + `showResult` is only ever set in the same batch as `setResult`, so the empty + case is unreachable. See L1 for the one duplication. + +## Metrics + +- Tests: 243 passed / 15 files (`npx vitest run`), matching the expected count. +- Lint/type/build: not re-run per instruction. +- Type coverage: n/a (JavaScript-only repo). + +## Recommended actions + +1. H1 — clear `guessCoordinates` in `applyRound` (one line). Decide separately + whether the watchdog should abandon the fetch outright. +2. M1 — add `!sessionId` to Submit's `disabled` and to `handleSubmitGuess`. +3. M3 — epoch-guard `handlePanoramaReady` / `handlePanoramaError`. +4. M2 — clear `loadError` in `handleSkipGuess`. +5. L1-L3 sweep — all inside the two touched files. + +## Unresolved questions + +1. Should the 15 s watchdog *abandon* a stalled round fetch (error + retry) or + keep racing it? Abandoning is safer but throws away a round that may be two + seconds from arriving. +2. Should the guess map be inert while `roundLoading` is true? Every finding + above is reachable only because it stays live across the round boundary. diff --git a/plans/reports/code-reviewer-260831-2145-server-contract-recheck.md b/plans/reports/code-reviewer-260831-2145-server-contract-recheck.md new file mode 100644 index 0000000..abce861 --- /dev/null +++ b/plans/reports/code-reviewer-260831-2145-server-contract-recheck.md @@ -0,0 +1,36 @@ +# Server scoring + leaderboard contract recheck (260831-2145) + +Gates: vitest 243/15 pass; eslint 0 errors/16 preexisting warnings; migration lib import smoke OK; node --check migrate-leaderboards.mjs clean. + +Overall: no blocking breaking change. Response shapes strictly additive (gameResult.bands, levels[].points). Redis key names, member encodings (username; username:distance:timestamp), trim windows, accumulation arithmetic untouched; no write path can reset/corrupt totals. /api/leaderboard byte-identical. Real contract change is semantic: gameResult.score no longer equals any board delta; leaderboard.message format changed — all in-repo consumers updated in lockstep incl. e2e stub. + +## High +- H1 submitRoundScore (leaderboard.js:249-257): only guard is `distance === undefined`; null/''/false → Number() → 0 → 5 points at every level (max reward on absent value). Old submitScore path paid 0 for same garbage. Unreachable from /api/guess today; reachable by any future caller. Fix: `Number.isFinite` + non-negative guard. + +## Medium +- M2 semantic change: gameResult.score graded on picked ladder, credited to no board; leaderboard.message now "(+3, +5, +5)" format. In-repo consumers verified updated (GameClient, RoundResultDialog, e2e helpers/spec). No external consumers. Doc one line. +- M3 bandsForBbox/calculateScore return module-level SCORE_BANDS by identity (game.js:61,67); test pins with toBe. One in-place sort/mutation anywhere (server or the new client render path in LeaderboardList) rewrites the base ladder process-wide. Fix: Object.freeze deep, or return copy + relax test to toEqual. +- M4 submitScore now production-dead export (only tests use it as seeding helper); docstring claims callers that don't exist. Delete or relabel as test/backfill primitive. +- M5 bandsForDiagonal(NaN/undefined) → maxMeters NaN → JSON null → "≤NaNm = 5" in dialog. Currently unreachable (only TPHCM-CUCHI lacks bbox; unplayable; bandsForBbox short-circuits). Fix: Number.isFinite factor guard. + +## Low +- L6 pre-existing: creditScore zScore→zAdd read-modify-write not atomic; concurrent same-user rounds can lose an increment; adapter lacks zIncrBy. Backlog. +- L7 message "(+3, +5, +5)" unlabelled, duplicates per-card (+N). Consider dropping line. +- L8 Turf on client list-render path via LeaderboardList→bandsForBbox; no new bundle weight (page.js already imported game.js client-side). +- L9 game-flow.md describes only picked-region ladder; add sentence pointing at submitRoundScore per-level crediting. + +## Verified clean (evidence) +1. /api/guess field inventory unchanged + additive only; levels[] types unchanged (score/rank null when trimmed). +2. Redis: key builders unchanged (country → leaderboard:vietnam), members, trims (score 0..-(201); distance 200..-1), accumulation `(existing||0)+points`. +3. No board stricter than HEAD: computed all 67 regions' ladders — ≤10km-diagonal districts keep exact base ladder (HN-BADINH 50m→5); larger only widen (DN-HOAVANG 273m, TPHCM 442m, HN 594m, VN 6,380m). +4. calculateScore boundary inclusivity (<=) and default arg preserved; scaling factor ≥1 preserves strict ordering. +5. bands leaks nothing (derives from player-chosen pickedRegion only). +6. Session skew: pickedRegion??cityCode??null guarded by isRegion before consume; pre-tree cityCode always valid uppercase region (verified vs 00a8f8b~1); neither-field shape 500s after consume — identical to HEAD, unreachable (no deploy era wrote it). +7. Callers: only /api/guess changed; migration consumes only leaderboardKeys; plain-node import path unaffected by turf edge. +8. Tests behavioural: guess-route pins district-vs-country property from store readback; leaderboard tests pin per-level points against independently computed ladders. + +## Unresolved +1. VN board pays 5 within 6.38km / 1 out to 127.6km mixed with old-ladder points — accepted by owner; makes country board volume-driven. +2. Keep or delete submitScore export? + +Status: DONE_WITH_CONCERNS — fixes recommended pre-landing: H1, M3, M4 label, optional M5/L9. diff --git a/plans/reports/code-reviewer-260831-2145-ui-surface-recheck.md b/plans/reports/code-reviewer-260831-2145-ui-surface-recheck.md new file mode 100644 index 0000000..18ae472 --- /dev/null +++ b/plans/reports/code-reviewer-260831-2145-ui-surface-recheck.md @@ -0,0 +1,339 @@ +# Code Review — Remaining UI Surface + Test Integrity + +Adversarial read-only recheck of the uncommitted working tree on `main`. + +## Scope + +- `src/app/page.js`, `src/app/components/RegionPicker.js`, `src/lib/last-region.js` (untracked) +- `src/app/components/LeafletMap.js`, `src/app/components/ResultMap.js` +- `src/app/components/LeaderboardList.js`, `src/app/components/LeaderboardModal.js` +- `tests/e2e/helpers.js`, `tests/e2e/game.spec.js` +- `docs/features.md`, `docs/game-flow.md` +- Cross-read for verification only: `src/lib/game.js`, `src/lib/leaderboard.js`, + `src/lib/regions.js`, `src/app/api/guess/route.js`, `src/app/api/new-game/route.js`, + `src/app/api/leaderboard/route.js`, `src/app/components/GameClient.js`, + `src/app/components/RoundResultDialog.js`, `src/app/debug/**`, + `tests/leaderboard.test.js`, `tests/guess-route.test.js`, `tests/game.test.js` + +Checks run: `npx vitest run` → **15 files, 243 tests passing**. `npm run lint` → **0 errors, +16 warnings**. Build and e2e not run (declared green). + +## Overall Assessment + +No critical or breaking defect. Every claim in the task brief about mechanism was verified +against source; the one that did not hold is the *label* on the new scoring table, not its +derivation. The map-icon change is sound and the `Icon.Default` removal is safe. The real +weakness is test integrity: the e2e stub's new `bands`/`points` values are invented and +contradict their own comment, and two new unit assertions re-implement the production +formula rather than pinning it. + +--- + +## Critical Issues + +None. + +--- + +## High Priority + +### H1 — The new scoring table claims to describe "a district round"; it does not for 38 of 58 districts + +`src/app/page.js:118-121`, `docs/features.md:47-48`, `docs/game-flow.md:55` + +The caption reads *"For a district round — playing a whole province or the country widens +these distances to match its size."* and both docs call `SCORE_BANDS` the "district-round +base ladder". `bandsForDiagonal` (`src/lib/game.js:46-52`) floors the factor at a **10 km** +bbox diagonal, and most real districts are larger than that. Measured over the generated +tree: + +``` +playable districts with bbox: 58 +at factor 1.00 (<=10km diagonal): 20 +min HN-HOANKIEM 3906m (1.00) | median TPHCM-BINHTAN 15101m (1.51) | max TPHCM-CANGIO 47277m (4.73) +``` + +So a District 7 round actually grades on `62 / 123 / 247 / 617 / 1234` metres, not +`50 / 100 / 200 / 500 / 1000`. The table tells a player that 50 m earns 5 points in a +district round when the real threshold is 62 m in Q7 and 236 m in Can Gio. The whole point +of deriving the table from `SCORE_BANDS` was that a hardcoded copy drifts from what the +server awards — the derivation is right, the label reintroduces exactly that drift. + +Fix — reword the caption and both docs to describe the reference, not a district: + +```js +// src/app/page.js +

+ For a compact area about 10km across — a larger district, a province, or the + whole country widens these distances in proportion to its size. +

+``` + +and in `docs/features.md:47-48` / `docs/game-flow.md:55`, replace "the base ladder is for a +district round" with "the base ladder applies to any region up to a 10 km bbox diagonal; +everything larger scales up from it". + +--- + +## Medium Priority + +### M1 — The e2e stub's `bands` are fabricated and its comment asserts they are production values + +`tests/e2e/helpers.js:50-62`, `:60-62`, `:81` + +The comment says *"province-scaled, as the real route returns for a TPHCM pick"*. It is not. +Computed from the real code path (`bandsForBbox(getRegion('TPHCM').bbox)`): + +| | stub | real TPHCM | +|---|---|---| +| bands (m) | 300, 600, 1200, 3000, 6000 | **442, 885, 1770, 4425, 8849** | +| `gameResult.score` at 123 m | 3 | **5** | +| `leaderboard.message` | `(+3, +5, +5)` | **`(+4, +5, +5)`** (Q7 = 62/123/247/617/1234 → 123 m = 4) | + +A stub is allowed to be synthetic, but a stub that *claims* to mirror production and does +not is worse than an obviously fake one: the next maintainer will trust `≤1.20km = 3` as a +regression baseline for the TPHCM ladder, and it is not one. + +Fix — either derive the fixture so it cannot drift: + +```js +import { bandsForBbox, calculateScore } from '../../src/lib/game.js'; +import { getRegion } from '../../src/lib/regions.js'; +const BANDS = bandsForBbox(getRegion('TPHCM').bbox); +const SCORE = calculateScore(123, BANDS); +``` +(and update the spec to assert `≤442m = 5`), or keep the invented numbers and change the +comment to say the payload is a synthetic ladder chosen to exercise the strip's +highlight branch, not a production capture. + +### M2 — Phantom assertion: the new `submitRoundScore` test re-implements the code it tests + +`tests/leaderboard.test.js:255-262` + +```js +const expected = (code) => calculateScore(distance, bandsForBbox(getRegion(code).bbox)); +for (const level of result.levels) expect(level.points).toBe(expected(level.code)); +``` + +`expected()` is character-for-character the production expression at +`src/lib/leaderboard.js:255-257`. Change the reference diagonal, the band values, or the +bbox source and both sides move together — the test passes. Only the ordering check +(`expected('VN') > expected('TPHCM-Q7')`, which tests `game.js`) and the message format +carry information. + +Fix — pin the literals. Verified against the current tree, 2200 m in `TPHCM-Q7` yields: + +```js +expect(result.levels.map((l) => [l.code, l.points])).toEqual([ + ['TPHCM-Q7', 0], ['TPHCM', 2], ['VN', 5], +]); +expect((await getLeaderboard('VN'))[0].score).toBe(5); +``` + +The same shape appears at `tests/guess-route.test.js:148-150` +(`expect(body.gameResult.bands).toEqual(bandsForBbox(getRegion('TPHCM').bbox))`), but there +it is redeemed by the independent property assertions on the following lines +(`score > 0`, `calculateScore(distance) === 0`, `countryPoints > districtPoints`). Lower +priority, same recommendation. + +### M3 — The per-round pano URL strengthens nothing; no assertion reads it + +`tests/e2e/helpers.js:32-34`, `tests/e2e/game.spec.js:46-48` + +The comment claims *"a fixed URL would let a broken next-round image swap pass unnoticed"*. +After the change, nothing in the suite reads the served URL, counts pano requests, or +inspects the viewer's texture source. `game.spec.js:48` only re-asserts the submit button is +disabled — which is `guessCoordinates === null`, unrelated to the image. A broken swap still +passes unnoticed; only the browser cache behaviour changed. + +Fix — make the claim true, e.g. capture the URLs the pano route serves and assert two +distinct ones after Next Round: + +```js +const served = []; +await page.route(`${PANO_IMAGE_URL}*`, async (route) => { + served.push(route.request().url()); + await route.fulfill({ contentType: 'image/png', body: readFileSync(PANO_FIXTURE) }); +}); +// ... after Next Round: +await expect.poll(() => new Set(served).size).toBeGreaterThan(1); +``` + +### M4 — The new-game stub breaks the `sessionId` contract the real route implements + +`tests/e2e/helpers.js:110-113` vs `src/app/api/new-game/route.js:14,52` + +The real route reads `?sessionId=` and returns `sessionId || generateSessionId()` — the +client forwards the previous id (`GameClient.js:30-31`), so in production the session id is +**stable across rounds** and the pano URL is what changes. The stub ignores the parameter and +mints `e2e-session-${round}` on every call, then derives the image URL from it. Two +consequences: the stub's image-swap mechanism is keyed on something production holds +constant, and a regression where the client stops forwarding `sessionId` (burning a fresh +Redis session per round) cannot be caught. + +Fix — echo the parameter, and key the image on the round counter instead: + +```js +await page.route('**/api/new-game**', async (route) => { + round += 1; + const incoming = new URL(route.request().url()).searchParams.get('sessionId'); + await route.fulfill({ json: newGameResponse(incoming || `e2e-session-${round}`, round) }); +}); +``` + +### M5 — New user-visible behaviour is undocumented + +`src/lib/last-region.js`, `src/app/components/RegionPicker.js:102-122` + +The "Continue in <region>" row and the new `vngeoguessr_last_region` localStorage key +appear in neither `docs/features.md` nor `docs/game-flow.md` (whose section 1 already +documents the username localStorage read at `docs/game-flow.md:6`), and +`docs/project-structure.md` does not list `last-region.js`. That file's lib inventory at +`docs/project-structure.md:97` also still misattributes username storage to `game.js` +("Scoring, distance, formatting, username storage") — it lives in `src/lib/username.js`. + +Fix — one line in the game-flow entry step, and add `last-region.js` (and `username.js`) to +the `src/lib` list while correcting the `game.js` description. + +--- + +## Low Priority + +- **L1** `src/app/page.js:27` — module-level `const label` is shadowed at `:102` by the + `STEP_LABELS.map((label, i) => ...)` parameter. Harmless today (`SCORING_ROWS` is computed + at module load), but two different `label`s in one 150-line file is a trap. Rename to + `bandLabel`. +- **L2** `src/app/page.js:30` — the first row now renders `0m-50m = 5 pts` where it + previously read `0-50m = 5 pts`. Cosmetic drift from the derivation; use `'0'` instead of + `'0m'` if the old form was deliberate. +- **L3** `src/app/components/LeaderboardList.js:47-49` — `distanceBands` runs a Turf + great-circle computation on **every** render, including score boards and the + loading/empty early-returns at `:51` and `:67` where it is never read. Move it below the + early returns and gate on `isDistance`, or wrap in `useMemo([regionCode])`. +- **L4** `src/lib/game.js:61` — `bandsForBbox(null)` returns the shared module-level + `SCORE_BANDS` array *by identity* (pinned with `toBe` at `tests/game.test.js`). It is + handed to client components and, on the server, held at module scope across every request. + One accidental in-place mutation (`.sort()`, `.push()`) anywhere would corrupt scoring + process-wide until restart. Recommend `export const SCORE_BANDS = Object.freeze( + [...].map(Object.freeze));`. +- **L5** `src/app/components/GameClient.js:137-140` — `loadRound` returns `true` on the + superseded-epoch path (`:114`), so `setLastRegion(code)` fires for a round that was + discarded and never rendered. The comment says "Only a region that actually served a + round". Return a distinct sentinel for the superseded case, or move the write into + `applyRound`. +- **L6** `src/lib/last-region.js:8-14,19-23` — the try/catch is correct in isolation but buys + no net resilience on the home page: `src/app/page.js:43` calls `getUsername()` + (`src/lib/username.js:8`, unguarded) in the same effect pass, so a throwing `localStorage` + still tears down the page. Either guard `username.js` the same way or drop the "blocked + site data" justification from the comment. +- **L7** `src/app/components/RegionPicker.js:112` — adds a twelfth + `react-hooks/set-state-in-effect` warning. Consistent with the eleven that already exist + and genuinely required for the hydration-safe read; noted only so the count is on record + (0 errors, 16 warnings total). +- **L8** `tests/e2e/helpers.js:88-101` — the leaderboard stub returns score-shaped rows + (`{username, score, rank}`) regardless of `?type=`. The new `regionCode`-driven distance + colouring therefore has zero coverage in any suite; a distance row would render + `formatDistance(undefined)` → `"NaNkm"` (no crash, verified by reading + `src/lib/game.js:73-79`). +- **L9** `docs/game-flow.md:63-64` — "multiplies every threshold by the picked region's bbox + diagonal over a 10km reference" omits the `Math.max(1, ...)` floor + (`src/lib/game.js:47`). As written, Hoan Kiem (3.9 km diagonal) would shrink the ladder to + ~40 % — it does not. + +--- + +## Verified Sound (task questions answered) + +**1. Hydration & SSR.** `SCORING_ROWS` (`page.js:28-35`) is module-scope, derived from a +static const, and contains no `Date`/`Math.random`/`window` — server and client prerender +produce identical strings. `RegionPicker` uses `useState(null)` + `useEffect` +(`:107-114`), so the server-rendered tree and the first client render both omit the row; no +mismatch. `last-region.js` guards `typeof window` **and** wraps both `getItem` and `setItem` +in try/catch, which covers the Chrome "blocked site data" case where touching +`localStorage` throws on property access. + +**2. LeafletMap / ResultMap.** `imageUrl()` (`LeafletMap.js:14`) is sound for both shapes, +and the production build already emits the assets — `.next/static/media/` contains +`marker-icon.*.png`, `marker-icon-2x.*.png`, `marker-shadow.*.png`. Removing the +`Icon.Default` block from `ResultMap` is safe: every marker on that map is a `divIcon` +(`ResultMap.js:43-48` red, `:56-61` green) and the third layer is an `L.polyline` (`:68`), +which needs no icon. No `cdnjs`/`unpkg` URL remains anywhere in `src/` (only Mapillary Graph +API, Photon, and OSM tiles). The one remaining default-icon marker, +`src/app/debug/page.js:145`, is added onto a map created by `LeafletMap`, so it now gets the +bundled icons — a net improvement. `src/app/debug/coverage/CoverageMap.js` uses +`L.circleMarker` only. + +**3. LeaderboardList.** The only render site is `LeaderboardModal.js:135`; no test or other +component constructs it, so there is no stale prop shape. `regionCode` undefined → +`isRegion(undefined)` is false → `bandsForBbox(null)` → base ladder, identical to the +pre-change behaviour. `getScoreColor` (`:24-31`) is byte-identical to before — score-board +thresholds unchanged. `getRegion(regionCode).bbox` cannot throw: `isRegion` guards it, and a +region without a bbox falls through to the base ladder, matching what +`submitRoundScore` credits. All 58 playable district names are unique across the tree, so +"Continue in District 7" is unambiguous. + +**4. Playwright glob.** `${PANO_IMAGE_URL}*` still matches the per-round URL. Traced through +playwright-core 1.62.1: `resolveGlobBase` tokenises the last path segment (no `?` in the +*pattern*, so no query split), and `globToRegexPattern` compiles a single `*` to `([^/]*)`, +giving `^https://pano\.invalid/e2e-round\.png([^/]*)$`. The tail `?session=e2e-session-1` +contains no `/`, so it matches. No fix needed. + +**5. e2e assertions.** `getByText('123m away', { exact: true })` **narrows** the previous +matcher (the sr-only description at `RoundResultDialog.js:79` reads "Scored 3 of 5 points, +123m away." and no longer collides) — a strengthening. `'Score added at 3 levels +(+3, +5, +5)'` matches the real format string at `src/lib/leaderboard.js:260-262` +field-for-field. `getByText('District 7', { exact: true })` still resolves to exactly one +node (the path row is not exact, the distance row reads "District 7 distance"). Response +*shapes* — `gameResult.{distance,score,bands,levels,distanceLevels,region,globalRank, +cityRank,globalDistanceRank,cityDistanceRank,exactLocation}`, level entries +`{code,name,username,points,score,rank,trimmed}` — match `guess/route.js:120-147` and +`leaderboard.js:153-162` exactly. Only the *values* diverge (M1) and the stubbed +`leaderboard`/`distance` objects are trimmed to `{message}` (harmless: `GameClient.js:248` +reads only `.message`). + +**6. Docs claims fact-checked.** `bandsForBbox` exists in `src/lib/game.js:60`; +`REFERENCE_DIAGONAL_METERS = 10_000` at `:39`; `gameResult.bands` returned at +`guess/route.js:127` and rendered at `RoundResultDialog.js:121-145`; +`submitRoundScore` in `src/lib/leaderboard.js:245`; `points` on each level at +`leaderboard.js:157`. The worked example in `docs/features.md:65-67` ("a 2km miss on a +country round earns country points on the Vietnam board and nothing on the district board") +checks out: 2000 m → 0 on Q7 (`≤1234`), 5 on VN (`≤6380`). Only the "district round" framing +(H1) and the missing floor (L9) are wrong. + +--- + +## Metrics + +- Type coverage: n/a (JavaScript-only project, per `CLAUDE.md`) +- Unit tests: 243 passing / 15 files (`npx vitest run`, 3.2 s) +- Lint: 0 errors, 16 warnings (`npm run lint`); 1 warning newly introduced (L7) +- New/changed LOC in scope: ~130 added, ~20 removed across 11 files + +--- + +## Recommended Actions + +1. **H1** Reword the home-page caption and both docs' "district round" framing to describe + the 10 km reference. (user-visible correctness) +2. **M1** Make the e2e band fixture derive from `bandsForBbox('TPHCM')`, or retract the + "as the real route returns" comment. +3. **M2** Replace the self-referential `expected()` assertions in + `tests/leaderboard.test.js:255-262` with the literals `[0, 2, 5]`. +4. **M3/M4** Either assert the per-round pano URL actually changes and echo `sessionId` in + the stub, or revert both stub edits — as landed they add divergence without coverage. +5. **M5** Document the "Continue in <region>" row and `last-region.js`; fix the + `game.js` description in `docs/project-structure.md:97`. +6. **L3/L4** Cheap hardening: memoize `distanceBands`, freeze `SCORE_BANDS`. + +## Unresolved Questions + +1. Was the e2e `bands` fixture deliberately kept round-numbered for readability, or was it + believed to be a production capture? The answer decides between "fix the numbers" and + "fix the comment" in M1. +2. `submitScore` (`src/lib/leaderboard.js:211`) now has **no production caller** — only + ~25 assertions in `tests/leaderboard.test.js`. The bulk of leaderboard coverage therefore + exercises a path the routes no longer take. Is the flat primitive still wanted, or should + those tests migrate to `submitRoundScore`? +3. `getScoreColor`'s absolute thresholds (5/10/15/25/50) predate per-level scoring. With the + country board now paying 5 points for nearly any in-country guess, national totals + saturate to purple far faster than before. Intentional, or worth recalibrating? diff --git a/plans/reports/researcher-260831-1853-geo-game-ux.md b/plans/reports/researcher-260831-1853-geo-game-ux.md new file mode 100644 index 0000000..a158a6b --- /dev/null +++ b/plans/reports/researcher-260831-1853-geo-game-ux.md @@ -0,0 +1,307 @@ +# UI/UX Research: Geography Guessing Game Patterns (2024-2026) + +Scope: GeoGuessr + clones (GeoHub, WorldGuessr, Enigjewo, OpenGuessr), Worldle-style +games, map/panorama libraries (Leaflet, Photo Sphere Viewer), and accessibility/mobile +guidance from NN/g and library docs. Grounded against this repo's current +implementation (`GuessMapPanel.js`, `PanoramaViewer.js`, `RoundResultDialog.js`, +`ResultMap.js`, `LeaderboardList.js`) to flag what's already aligned vs. what's a gap. + +Source quality note: GeoGuessr has no official UX documentation; findings on it come +from community userscripts/extensions, Hacker News threads, and clone READMEs +(secondary, but consistent across many independent authors = corroborated pattern, +not single-source). Library docs (Leaflet, Photo Sphere Viewer) and NN/g are primary/ +authoritative. Retention-loop claims (Duolingo, Snapchat stats) are widely cited but +originate from company blog posts, not independent audits — treat magnitudes as +directional, not exact. + +--- + +## 1. Guess-map interaction (corner widget vs. split view) + +**Pattern across GeoGuessr and every clone surveyed** (GeoHub, WorldGuessr, +Enigjewo, community userscripts): desktop uses a **split view** — large panorama +dominant, a persistent map panel (either a fixed side panel or a bottom-left corner +overlay) always visible and always clickable, with a distance "Guess" confirm button +that only enables once a pin is placed. Mobile cannot fit both full-size, so the map +becomes a **collapsed corner thumbnail that expands to fullscreen on tap** — this is +GeoGuessr's own mobile-app pattern (an "enlarge map" button was added after years of +user complaints, and third-party userscripts existed for years specifically to fix +GeoGuessr's own tiny fixed-size guess map before the vendor did it natively). NN/g's +mobile-maps research explains the mechanism: **maps and touch-scroll gestures compete +for the same swipe input** ("swipe ambiguity" — users starting a scroll gesture on +the lower half of the screen accidentally pan the map instead), which is exactly why +collapsing the map to a small non-interactive-looking preview until explicitly +expanded is the correct mobile default, not just a screen-space compromise. + +**This repo already implements the corner-widget-to-fullscreen pattern correctly**: +`GuessMapPanel.js` renders a 144px corner thumbnail on mobile with a full-tile +tap-to-expand overlay button ("Tap to guess" / "Edit guess"), expanding to +`inset-x-3 top-3` fullscreen, and calls `map.invalidateSize()` after the CSS +transition to fix Leaflet's stale-container-size bug. Desktop already gets a +persistent flex-1 side panel via the `lg:` breakpoint. This matches the dominant +industry pattern, not a divergent one. + +**Recommendations:** +1. Keep the current corner-thumbnail → fullscreen-tap pattern; it matches both + GeoGuessr's shipped mobile behavior and NN/g's swipe-ambiguity guidance. No + change needed here. +2. Consider adding a lightweight "pin placed" visual confirmation *on the collapsed + thumbnail itself* (small dot) even when collapsed, so a mobile player who + guessed then collapsed the map isn't left wondering if the guess registered — + several clone READMEs and the userscript history suggest this was a recurring + complaint pattern ("did my guess register"). +3. Keep the confirm/submit button reachable without requiring the map to be + expanded (already true here per `hasGuess` prop threading) — this is the + detail most clones get wrong (forcing map-open to submit). + +--- + +## 2. Round result / reveal UX + +Common pattern across GeoGuessr, GeoHub, and community extensions ("Smart Zoom", +"Better Scoreboard" scripts): reveal screen shows, in order, (a) the score number +counting/animating up rather than appearing static, (b) a dashed line from guess pin +to actual location on a small reveal map, (c) distance text, (d) a persistent +"Play again / Next round" CTA sized larger than secondary actions (menu/quit). +Retention-loop research (Duolingo, casual-game design writeups) converges on: fast +loop restart with minimal friction, and a visible progress signal (streak or rank +delta) is what drives repeat sessions — the reward doesn't need to be large, it +needs to appear reliably and immediately after the action that earned it. + +**This repo already implements the core of this pattern well**: `RoundResultDialog.js` +uses `useCountUp` for the score, staggered `animate-fade-in-up` reveals (120ms/240ms/ +320ms delays) for score → distance → message, a `role="status" aria-live="polite"` +wrapper so screen readers announce the result, and `ResultMap.js` draws a dashed +red polyline (`dashArray: '8 4'`) between guess and actual pins with `fitBounds` to +frame both. "Next Round" is the primary (flex-[2]) button, "Menu" is secondary +(flex-1, ghost variant) — correct visual hierarchy. + +**Gaps relative to the pattern:** +- No streak/session counter visible anywhere in the reveal (rank vs. leaderboard is + shown, but no "N rounds played this session" or similar lightweight momentum + signal) — this repo has no accounts, so cross-session streaks aren't feasible, but + an in-session round counter costs nothing and is the retention primitive that + survives a no-login constraint. +- The distance line animates in as already-drawn (Leaflet polyline appears instantly + with the map), rather than drawing progressively — GeoGuessr's native reveal + animates the line growing from pin to pin, which reads as more deliberate/dramatic. + This is a nice-to-have, not a gap that blocks a good experience. + +**Recommendations:** +1. Add a simple in-session round counter (e.g., "Round 4" or "4 played today," reset + on tab close) near the score reveal — the no-accounts constraint rules out + persistent streaks, but session momentum is the cheapest retention lever left. +2. Keep the count-up + staggered fade-in pattern as-is; it already matches the + observed convention and needs no rework. +3. Optional polish only if time allows: animate the polyline drawing (e.g., via a + short requestAnimationFrame interpolation of the line's endpoint) instead of + rendering it instantly — low priority, cosmetic only. +4. Keep the `result.failed` distinct-state handling (never present a save failure as + a zero-point miss) — this is a correctness/trust issue that generic reveal-UX + research doesn't cover but matters more than any animation polish. + +--- + +## 3. Panorama viewer UX + +Photo Sphere Viewer's own docs and configuration guide establish the baseline +controls: drag/pinch to look around, a `panControl`/compass affordance, a +`zoomControl` near bottom-right, configurable `minFov`/`maxFov` for zoom limits, and +a navbar that can be reduced to just the controls a given game needs (PSV supports +`navbar: ['zoom', 'fullscreen']` etc.). For street-view-style games specifically, the +recurring UX requirements across GeoGuessr and clones are: (a) one-finger drag must +rotate the view on mobile — two-finger-only rotation is wrong for a viewer that is +the primary game surface, not an inline scrolling-page embed, (b) a graceful fallback +when a panorama fails to decode/load (flat image or retry, never an infinite +spinner), and (c) preserving viewport orientation across round transitions so the +player isn't reoriented every round. + +**This repo already implements items (a)-(c) directly**: `PanoramaViewer.js` sets +`touchmoveTwoFingers: false` specifically because "looking around is the core verb of +the game" (matches point a); it has an explicit `panorama-error` handler that calls +`showFallbackImage()` and still fires `onReadyRef.current?.()` so the loading state +resolves instead of hanging (matches point b); `navbar: ['zoom', 'fullscreen']` is a +deliberately reduced control set rather than PSV's full default navbar. + +**Gaps relative to the pattern:** +- No compass/heading indicator is configured (navbar omits any pan/compass control) + — for a geography guessing game, knowing compass orientation is sometimes part of + the puzzle-solving toolkit players expect (shadow direction, sun position reasoning + benefits from knowing which way is north). This is a judgment call: GeoGuessr's own + competitive community actually disables the compass in "no-move, no-pan, no-zoom" + ranked modes because it's considered a mild assist — so omitting it isn't a UX + miss, it may be an intentional difficulty choice already made correctly for a + casual/no-accounts context. Flagging as a decision to confirm, not a bug. +- No explicit loading-state UI is visible in the reviewed component beyond PSV's + default (`loadingImg: null` disables PSV's built-in loader entirely) — the fallback + path is handled, but the *slow-network-not-yet-errored* state (Mapillary imagery + can be slow) has no visible spinner/skeleton before `ready` fires, unlike the + `GuessMapPanel`'s explicit `Loading map...` status region. + +**Recommendations:** +1. Confirm whether omitting a compass control is an intentional difficulty choice + (matches competitive GeoGuessr's "no-compass" mode norms) or an oversight — if + the game wants to stay approachable/casual rather than competitive, adding a + small always-visible compass affordance is the more player-friendly default. +2. Add a visible loading indicator (skeleton or spinner) for the pre-`ready` window + since `loadingImg: null` removes PSV's own — this closes the one loading-state gap + relative to the rest of the app, which already treats loading states carefully + (map panel, dialogs). +3. Keep `touchmoveTwoFingers: false` and the `panorama-error` fallback exactly as + implemented; both match the dominant, validated pattern. + +--- + +## 4. Leaderboard / competitive UX without accounts + +NN/g's leaderboard literature (via IxDF/leaderboard pattern references) and hyper- +casual game UX guides converge on: leaderboards work best when **contextual** +(comparing to nearby-skill or nearby-in-time players, not a global all-time list +dominated by early adopters), and the current player's own row should always be +visible/highlighted even when off-screen from the top. For anonymous, no-account +casual games, the standard substitute for persistent identity is a **self-chosen +display name stored client-side** (localStorage/cookie) that's echoed back into the +scoreboard rather than a login — the game doesn't need auth, it needs the *same +device* to recognize "you" on return visits. + +**This repo already implements the core of this pattern**: `LeaderboardList.js` +does exact `entry.username === currentUsername` matching, applies a distinct +amber-highlighted row + a "YOU" badge, and top-3 get medal icons + rank number +together (rank number always shown alongside the medal, "colour alone must not +carry the result" per the code comment) — this independently satisfies the +color-contrast/colorblind-accessibility concern that leaderboard UX guides raise. +Two leaderboard types exist (score-based and distance-based, i.e., "best single +round" style), which maps to the contextual-leaderboard idea of giving players more +than one axis to compete on rather than a single global list. + +**Gaps relative to the pattern:** +- No visible "your best rank" indicator when the player isn't in the visible page of + results (e.g., rank 4000 of 10000) — the reviewed component doesn't show pagination/ + scroll-to-user behavior in what was read, though it may exist in the parent + (`LeaderboardModal.js`, not reviewed in depth here). +- Username collision handling wasn't reviewed — with no accounts, two players could + pick the same name and the "YOU" highlight would falsely mark both/neither. Worth + a quick check in `src/lib/username.js`/session handling if not already addressed. + +**Recommendations:** +1. Verify (or add) a "jump to my rank" affordance in `LeaderboardModal.js` for + players ranked far down the list — contextual self-visibility is the single + highest-value leaderboard feature per the sources above, and current-username + highlighting alone doesn't help if the row is off-screen. +2. Confirm session/username collision handling is scoped per-session (cookie/session + id, not raw username string) so the "YOU" highlight can't mismatch two different + players who picked the same display name — check `src/lib/session.js` and + `src/lib/username.js`. +3. Keep the medal-plus-rank-number (never color-only) pattern and the dual score/ + distance leaderboard axes; both are already correct per the sources. + +--- + +## 5. Mobile-first patterns for dual-canvas (panorama + map) games + +This is the hardest layout problem in the whole genre and the one most heavily +documented via complaint threads (GeoGuessr's multi-year "map too small" issue). +The converged pattern: **panorama is always the dominant/full-bleed layer; the map is +always an overlay, never a permanent split, on small viewports.** NN/g's mobile-map +guidance generalizes this: on small screens, prefer showing one primary interactive +surface with the second surface as an on-demand overlay rather than splitting the +viewport, because a split-screen map on mobile is usually too small to be usable for +precise interaction (pins too close together for fat-finger accuracy) and steals +scroll/pan gesture space from whichever surface is on the bottom half. + +**This repo already implements this correctly** — full-bleed panorama with corner +overlay expanding to fullscreen tap-target on mobile (`GuessMapPanel.js`, see §1), +and `PanoramaViewer` sets `touch-none` on its container so it doesn't fight page +scroll (the game screen itself doesn't scroll, sidestepping NN/g's swipe-ambiguity +issue entirely by design, per the code comment on `touchmoveTwoFingers`). + +**Recommendations:** +1. No architectural change needed — the current approach (full-bleed panorama, + overlay map, explicit expand state controlled by parent for reset-on-new-round) + already matches the validated pattern across the genre. +2. Double-check the `safe-area-inset-bottom` handling (already present in the + `bottom-[calc(5.25rem+env(safe-area-inset-bottom))]` class) against notched/ + gesture-nav Android devices in addition to iOS, since `env(safe-area-inset-*)` is + iOS-authored but Android WebView support varies by OS version. +3. When expanded, verify the fullscreen map still leaves the collapse/minimize + button reachable one-handed (bottom third of screen) on large phones — a common + mobile-map complaint is controls placed at the top of a fullscreen map being out + of thumb reach; current code places it at `top-2 right-2`, which is a candidate + to revisit if user feedback flags reachability. + +--- + +## 6. Accessibility patterns for map-based interaction + +Leaflet's own accessibility guide (authoritative, first-party) and the Leaflet/ +react-leaflet GitHub accessibility discussions are the primary sources here: +Leaflet ships keyboard-operable map containers and markers by default (Tab moves +focus, Enter/Space activates) — the responsibility for a consuming app is to *not +break* these defaults, label every marker with a descriptive `alt`/`title` (not just +an icon), test with a real screen reader (NVDA/Narrator/VoiceOver/Orca), and use +`inert` on purely decorative maps. `react-leaflet` has an open, still-unresolved +GitHub issue (#1009) asking for first-class ARIA-role support on the `` +— meaning teams generally still hand-add ARIA attributes themselves rather than +getting them for free. + +**This repo already does some of this correctly**: the `GuessMapPanel` loading +state uses `role="status" aria-live="polite"`, expand/collapse buttons have +`aria-label`, and icon-only buttons pair `aria-hidden="true"` icons with visible +text or `aria-label`. This is better than the median implementation the sources +describe (most consuming apps skip this entirely, per the GitHub discussion). + +**Gaps relative to the pattern:** +- The guess-placement pin itself (dropped by clicking/tapping the map, per + `onMapClick` in `GuessMapPanel`) has no reviewed keyboard-only path — Leaflet's + own guide notes map containers are keyboard-operable by default but *placing a + pin via keyboard* (as opposed to panning/zooming) isn't something Leaflet gives + for free; it requires an app-level keyboard handler (e.g., Enter/Space on a + focused map center, or a dedicated "confirm guess here" affordance) that wasn't + found in the reviewed files. +- `ResultMap.js`'s guess/actual markers use `bindPopup` text ("Your Guess"/"Actual + Location") which is good, but the divIcon markers are raw colored `
`s with no + `alt`/`title`/`aria-label` on the marker itself — a screen-reader user tabbing to + the marker (rather than opening its popup) may get no announcement. + +**Recommendations:** +1. Add a non-mouse way to place/confirm a guess (at minimum, a "confirm guess at map + center" keyboard-reachable button when the guess map is focused) — this is the + single actual accessibility gap found, since the whole core game action (placing + a pin) currently assumes pointer/touch input. +2. Add `alt`/`title`/`aria-label` to the `ResultMap.js` divIcon markers, not just + their popups, matching Leaflet's own guidance ("markers require unique, + descriptive labels"). +3. Test the guess-map and reveal-map flows with a screen reader (NVDA on Windows is + free and matches the dev environment) — Leaflet's guide explicitly recommends + this over inferring compliance from markup alone, since plugin behavior + (react-leaflet, custom overlays) can silently break the library's keyboard + defaults. + +--- + +## Limitations + +- GeoGuessr's actual native UX is not documented by the vendor; all claims about it + come from third-party scripts/extensions and community threads, which are + consistent but not an authoritative spec — treat as "converged community + consensus," not ground truth from GeoGuessr Inc. +- Did not evaluate performance/bandwidth aspects of panorama loading (image + compression, progressive loading strategies) — flagged as tangential to *UX* + patterns proper, but likely worth a separate pass given "Mapillary imagery can be + slow" is noted in this same report. +- Did not review `LeaderboardModal.js`, `GameClient.js` in full, `session.js`, or + `username.js` — leaderboard-scroll-to-self and username-collision recommendations + in §4 are therefore flagged as "verify," not confirmed gaps. +- Retention-loop statistics (Duolingo 12%→55%, Snapchat 30-40 opens/day) are sourced + from secondary aggregator articles citing company PR, not independently audited — + used only directionally to support "session momentum indicators matter," not as + precise benchmarks for this app. + +## Unresolved questions +1. Is the panorama viewer's lack of a compass control an intentional difficulty + choice, or should one be added for a casual (non-competitive) audience? +2. Does `LeaderboardModal.js` already scroll-to/highlight the current player when + off-screen in a long leaderboard? (Not reviewed.) +3. Is username uniqueness enforced per-session/per-cookie, or by raw string match, + in `src/lib/username.js` / `src/lib/session.js`? (Not reviewed — affects whether + the "YOU" leaderboard highlight can misfire.) diff --git a/plans/reports/synthesis-260831-1853-uiux-improvement.md b/plans/reports/synthesis-260831-1853-uiux-improvement.md new file mode 100644 index 0000000..4384618 --- /dev/null +++ b/plans/reports/synthesis-260831-1853-uiux-improvement.md @@ -0,0 +1,121 @@ +# UI/UX Improvement Synthesis — VNGeoGuessr + +Consolidates three advisory passes (2026-08-31): + +- Code audit: [ui-ux-review-260831-1853-uiux-audit.md](ui-ux-review-260831-1853-uiux-audit.md) +- External research: [researcher-260831-1853-geo-game-ux.md](researcher-260831-1853-geo-game-ux.md) +- Brainstorm: full text below (returned inline by agent, preserved here) + +## Convergent findings (multiple agents, highest confidence) + +1. **Loading-flag defect** — audit C1 + brainstorm Q2 found independently. Single + `loading` flag in `GameClient.js` full-screen-loaders on guess submit + (unmounts panorama + map, shows "Loading panoramic image..."), while the + Submit button's "Processing..." state is unreachable dead code; next-round + load shows nothing. Fix: split `initialLoading` / `submitting`. Defect, not + enhancement. +2. **Panorama failure dead end** — audit C2 + brainstorm Q4. Native `alert()` + then permanent fake loading state; only recovery is Skip. Fix: inline error + + Retry/Back. +3. **Session arc missing** — research (round counter is standard retention + pattern) + brainstorm M1 (5-round set, running total, summary screen). +4. **Keyboard users cannot place a guess** — audit H1 (WCAG 2.1.1) + research + accessibility topic. Also: score count-up inside `aria-live="polite"`, + missing marker aria-labels on reveal map. + +## Unique headline findings + +- **Region-relative scoring (brainstorm #1, highest leverage)**: `SCORE_BANDS` + is absolute (1km+ = 0 pts) across regions from ~35 km² to 331,000 km². The + emphasized "Play anywhere in Vietnam" CTA yields near-guaranteed zeros for + honest guesses. Scale bands by region bbox diagonal server-side; keep 0–5 + scale so boards stay valid. Fix before building any new mode. +- **Dead round on dialog dismiss (audit C3)**: result dialog closable via + X/Esc/overlay into a consumed session; re-submit shows misleading "Round Not + Recorded". Force Next Round / Menu. +- **Research**: app already implements most industry patterns correctly + (corner-widget map, tap-cover, animated reveal, score bands, safe-area) — + targeted gaps, not a rebuild. Mobile layout rework explicitly not needed. + +## Recommended sequence + +1. Split loading state (XS, defect) +2. Region-relative scoring bands + dynamic scoring copy (S) +3. Force result-dialog flow, no dismiss-to-dead-round (XS) +4. Inline panorama error + retry (XS) +5. "Continue in " localStorage shortcut on home (XS) +6. Prefetch next round during result dialog (S; preload image URL too, not just metadata) +7. Band-scale strip on result dialog (S, bundle with #2) +8. 5-round set w/ running total + summary screen (M) — then result-reveal + full-screen sheet, coach mark, targeted mobile fixes (larger minimap, + pin-placed indicator, confirm inside expanded map) +9. Accessibility batch: keyboard guess placement, aria-live fix, marker + labels, self-hosted marker PNGs (currently cdnjs-dependent) +10. Later: daily challenge (deterministic date seed, 5-round set) + emoji share + grid → async "beat my score" links. Skip: real-time duels, mobile rework, + sound, OG-image cards. + +## Full brainstorm (verbatim from agent) + +Grounded in: `src/app/page.js`, `src/app/components/GameClient.js`, `GuessMapPanel.js`, `RoundResultDialog.js`, `RegionPicker.js`, `PanoramaViewer.js`, `src/lib/game.js`, `docs/game-flow.md`, `docs/features.md`. + +### 0. Assumptions challenged first + +**Assumption 1: "the game needs more features."** It doesn't, yet. It has a broken *feedback gradient*. The scoring ladder in `src/lib/game.js` is absolute (1km+ = 0 pts) but the game lets you play a region tree spanning ~35 km² (Quận 7) to 331,000 km² (Vietnam). At country level almost every honest guess scores 0. A player's first session on "Play anywhere in Vietnam" — the emphasised primary CTA on the home page — is a near-guaranteed string of zeros. No multi-round mode, no streak, no daily challenge fixes that; they all multiply a broken reward signal. **Fix scoring before adding modes.** + +**Assumption 2: "the loading spinner is fine."** `GameClient` early-returns a full-screen spinner whenever `loading` is true, and `handleSubmitGuess` sets `loading = true`. So submitting a guess unmounts the entire game tree — including the Photo Sphere viewer, which gets `destroy()`ed and rebuilt behind the result dialog. The `loading` prop on the Submit button is dead code; it never renders. Symmetrically, `handleNextRound` never sets loading, so the next round shows a black `bg-neutral-900` box with no indicator. Two opposite bugs from one overloaded state flag. + +**Assumption 3: "no accounts means no social loop."** Wordle has no accounts. A daily challenge + emoji share grid works entirely on a deterministic server seed + `localStorage`. The absence of accounts costs anti-cheat (already an accepted trade-off) and cross-device continuity — not the social loop itself. + +**Assumption 4 (design around it): imagery is one static Mapillary thumbnail.** No walking, no zoom beyond `thumb_2048`. Precision is structurally capped — another argument for region-relative scoring, and against "duels on precision." + +### Horizon 1 — Quick wins + +- **Q1 Region-relative scoring bands** — keep 0–5 integer scale (board keys stay valid). Scale `SCORE_BANDS` thresholds by played region's bbox diagonal (already server-side). District keeps 50m…1km; Vietnam might be 5/15/50/150/400 km. One function, one call site, home-page scoring table becomes dynamic. Trade-off: boards silently mix two scoring eras; any "5 pts = 50m" copy must derive from `SCORE_BANDS`. Effort S. **DO — highest leverage.** +- **Q2 Split `loading` → `initialLoading`/`submitting`** — XS. **DO (defect).** +- **Q3 Prefetch next round during result dialog** — fire `/api/new-game` on `showResult`, swap in on Next Round; must preload `imageData.url` too. Costs: wasted lookup + orphan Redis session per quitter (self-expires); no tile-cap impact. Effort S. **DO.** +- **Q4 Inline error + Retry replacing `alert()`** — XS. **DO.** +- **Q5 "Continue in "** — localStorage last-played code, top row on home; don't give two rows accent emphasis. XS. **DO.** +- **Q6 Band scale strip on result dialog** — derives from `SCORE_BANDS`; answers "how close did I need to be?" (more important post-Q1). S. **DO bundled with Q1.** +- **Q7 Keyboard shortcuts** (Space/Enter submit, Enter next, Esc collapse map) — must not fire in search box. XS. **MAYBE (desktop share dependent).** +- **Q8 Defer username prompt to first result dialog** — S. **MAYBE** (current placement defensible, current blocking is not). +- **Q9 Result-map polish** — auto-fit bounds + "View on Mapillary" link (XS): **DO**; animated guess→target line: **MAYBE**. +- **Q10 Sound/haptics** — **SKIP** (mute-toggle plumbing outweighs value). + +### Horizon 2 — Medium + +- **M1 5-round set with running total** — client-side round counter + set summary (5 thumbnails, distances, total /25, Play again / Change region). Server scoring untouched. Client state is editable — irrelevant given accepted cheat tolerance, unless set totals ever post to a board (then server-side or nothing). Skip must consume a round or the set is farmable. Effort M. **DO, right after Q1.** +- **M2 Result reveal → full-screen sheet** — priority: score → distance → map → boards behind "Rankings" disclosure. Don't ship before M1 (rank rows are currently the only progression signal). M. **DO after M1.** +- **M3 Streaks** — localStorage-only streaks die on cache clear/second device; broken streak is worse than none. **MAYBE — only inside daily challenge, never standalone.** +- **M4 Mobile layout rework** — current pattern is right (corner minimap, tap-cover, safe-area). Instead: confirm inside expanded map, pin-placed indicator on collapsed minimap, larger minimap (144px small for Vietnam bounds). **SKIP rework, DO the three fixes.** +- **M5 Onboarding** — one-time coach mark on minimap ("tap to place your guess"). **DO coach mark, SKIP tour.** +- **M6 Region picker search + random district** — reuse diacritic matcher from `MapSearchBox` (extract, don't fork). S. **MAYBE.** + +### Horizon 3 — Ambitious (sketch) + +- **H1 Daily Challenge** — same 5 panoramas for all, date-seeded (Redis key per date), one attempt/day, daily board, resets 00:00 ICT (timezone locked forever). Strongest account-less retention mechanic; composes with M1/M3. Risk: a bad daily set hurts everyone; no easy quality filter. M–L on top of M1. **DO — flagship next-quarter, after M1.** +- **H2 Async "beat my score" links** — URL-encoded seed, identical 5 panoramas, target score. Same machinery as H1. S–M. **DO after H1 — the 10%-cost duels.** +- **H3 Emoji share grid** — `🟩🟩🟨⬜⬜ 18/25 — VNGeoGuessr #142`. S. **DO with H1; SKIP OG-image card until text grid proves sharing.** +- **H4 Real-time duels** — websockets, lobbies, ops burden, competitive cheating; static imagery makes precision duels arbitrary. **SKIP.** +- **H5 More provinces** — content, not UI; tile-cap-bound; competes for same maintainer hours. **MAYBE.** + +### Ranked top 5 + +| # | Idea | Effort | Improves | Key trade-off | +|---|---|---|---|---| +| 1 | Q1 region-relative scoring | S | reward signal, fairness, board meaning | mixes two scoring eras | +| 2 | Q2 split loading state | XS | clarity, stops viewer teardown | none | +| 3 | M1 5-round set | M | retention (arc + completion) | client state unauthoritative; skip rule | +| 4 | Q3 prefetch next round | S | perceived speed every round | orphan sessions from quitters | +| 5 | H1 daily challenge + H3 grid | L | retention + acquisition | timezone locked; bad set hurts all | + +Simplest viable: Q1 + Q2 + Q4 + Q5 (~1–2 days, no new state machines/schema/deps). + +## Unresolved questions + +1. Desktop vs mobile traffic split? (decides Q7 keyboard shortcuts, weight of mobile fixes) +2. Mix pre/post-rescale scores on existing boards, or new board keys for the rescaled era? +3. Does Skip consume a round inside a 5-round set? +4. Is Mapillary attribution rendered anywhere in the UI? Brainstormer saw none — verify against Mapillary terms before adding "View on Mapillary" links. +5. From audit: multi-round structure intent, geocoder choice, dark map-tile stance. +6. From research: compass-control intent, leaderboard scroll-to-self, username-collision handling (verify `LeaderboardModal.js`, `session.js`, `username.js` before acting). diff --git a/plans/reports/ui-ux-review-260831-1853-uiux-audit.md b/plans/reports/ui-ux-review-260831-1853-uiux-audit.md new file mode 100644 index 0000000..1a58289 --- /dev/null +++ b/plans/reports/ui-ux-review-260831-1853-uiux-audit.md @@ -0,0 +1,136 @@ +# VNGeoGuessr UI/UX Audit + +Date: 2026-08-31 | Scope: home → region pick → panorama → guess → result → leaderboard. Advisory only, no code changed. Debug pages out of scope. + +## Overall Assessment + +Codebase shows unusually strong UX discipline for a hobby project: tokenized theme system with documented rationale (globals.css:96-119), 44px touch-target floor baked into button variants (button.jsx:38-45), honest error states ("Round Not Recorded" instead of fake 99999m miss, RoundResultDialog.js:70-81; leaderboard outage not shown as empty board, LeaderboardModal.js:44-56), reduced-motion respected incl. count-up (use-count-up.js:37-43), combobox ARIA on map search (MapSearchBox.js:124-128). Findings below are mostly flow-level gaps, not craft gaps. + +--- + +## CRITICAL + +### C1. Submitting a guess replaces the entire game screen with a mislabeled full-screen spinner +- Evidence: `handleSubmitGuess` sets `loading=true` (GameClient.js:158) and the component early-returns the full-screen loader whenever `loading` (GameClient.js:233-243) whose copy is **"Loading panoramic image..."** — wrong verb for scoring a guess. Side effects: PanoramaViewer and LeafletMap unmount, then remount when the result dialog opens (texture re-fetch, map re-init, guess pin visually rebuilt). The in-button loading state `'Processing...'` (GameClient.js:322) and the `loading` spinner prop (GameClient.js:320) are unreachable — the early return swaps the whole tree first. +- Impact: every single round ends with a jarring white-out + wrong message; wasted re-render of the heaviest components in the app; the carefully built button spinner never renders. +- Recommendation: split state into `initialLoading` vs `submitting`. Only initial load uses the full-screen loader; submit keeps the game view mounted and lets the existing button `loading` prop + disabled state do the work. Copy for any submit indicator: "Scoring your guess…". + +### C2. Panorama-load failure is a native `alert()` followed by a dead end disguised as loading +- Evidence: fetch failure calls `alert('Failed to load street view image…')` (GameClient.js:81) with `imageData=null`; the game then renders a permanent "Loading panorama..." placeholder (GameClient.js:294-298) that never resolves. Submit is disabled; the only recovery is discovering that Skip re-fetches. Same path fires on a failed "Next Round". +- Impact: blocking OS-styled alert breaks immersion and theming; after dismissal the screen lies (says loading, nothing is loading). On mobile the player is stranded. +- Recommendation: replace `alert` with an in-panorama error state: "Couldn't load this street view" + `Retry` and `Back to menu` buttons. Never render "Loading…" when no request is in flight. + +### C3. Result dialog is dismissible into a dead round +- Evidence: `onOpenChange={() => setShowResult(false)}` (GameClient.js:338); DialogContent renders a visible X close by default (dialog.jsx:65-71); Radix also closes on Esc/overlay. Session was already consumed server-side (atomic DEL on guess — docs/game-flow.md:50-51). After dismissal the player sees the old panorama, `guessCoordinates` still set, Submit enabled reading "Submit Guess" (GameClient.js:315-323); pressing it hits a dead session → misleading "Round Not Recorded". +- Impact: an obvious affordance (X / Esc) leads to a trap state whose failure message blames the wrong thing. +- Recommendation: pass `showCloseButton={false}` and either make `onOpenChange` a no-op (force choice between Next Round / Menu) or treat dismiss as "Next Round". Alternatively keep the round screen but disable Submit and surface "Round finished — start the next round". + +--- + +## HIGH + +### H1. Core mechanic is pointer-only — keyboard users cannot play +- Evidence: guess placement exists only as a Leaflet `map.on('click')` handler (LeafletMap.js:65-83); no focusable pin-placement alternative. Panorama look-around is drag/wheel only (PanoramaViewer.js:47-61; PSV navbar keyboard zoom hidden below lg, globals.css:196-200). Mobile "Tap to guess" cover is a `