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