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 `