Phase 18: Auto timezone detection and ability to change timezone #21
+121
@@ -0,0 +1,121 @@
|
||||
---
|
||||
phase: 18-auto-timezone-detection-and-ability-to-change-timezone
|
||||
reviewed: 2026-06-15T02:49:55Z
|
||||
depth: standard
|
||||
files_reviewed: 6
|
||||
files_reviewed_list:
|
||||
- apps/api/src/lib/householdTimezone.ts
|
||||
- apps/api/src/routes/admin.ts
|
||||
- apps/api/src/broker/reminderScheduler.ts
|
||||
- apps/api/src/broker/outboxWorker.ts
|
||||
- apps/pwa/src/api/client.ts
|
||||
- apps/pwa/src/routes/AdminPage.tsx
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 3
|
||||
info: 4
|
||||
total: 7
|
||||
status: wr-01-resolved wr-02-resolved wr-03-accepted
|
||||
---
|
||||
|
||||
# Phase 18: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-15T02:49:55Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 6 (production) + 5 test files (coverage review)
|
||||
**Status:** issues-found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 18 adds a stored household timezone with auto-detection, an admin picker, and rewires the all-day "9 AM local" reminder math through a single shared accessor. The core security and correctness contract holds up well under adversarial review:
|
||||
|
||||
- **Access control (D-04) is sound.** All three new endpoints (`GET`/`PUT /config/timezone`, `POST /config/timezone/seed`) are registered on `adminRouter` *after* `adminRouter.use('*', requireAdmin)` (admin.ts:42), so they inherit the guard. Tests assert 403 for non-admins on GET and PUT (admin.test.ts:603-625).
|
||||
- **Injection safety is solid.** User-supplied timezone strings are validated by `isValidIanaTimezone` (Zod `.refine`) *and* Drizzle parameterizes the INSERT/upsert — no string interpolation reaches SQL. No path traversal or command surface is touched.
|
||||
- **D-03 no-overwrite seed semantics are correct** (SELECT-then-conditional-INSERT, admin.test.ts:730-754).
|
||||
- **D-05/D-06 accessor + fallback chain is correct** and well-tested (householdTimezone.test.ts), including the null-value fall-through.
|
||||
- **D-07 boundary respected** — the browser-local display/serialization path is untouched.
|
||||
|
||||
No Critical findings. Three Warnings: a genuine logic defect that prevents an admin from ever saving the displayed system-default value (contradicting the on-screen instruction), an unhandled race/duplicate-key path on the seed endpoint, and a redundant DB round-trip on the GET handler. Four Info items.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Admin cannot save the displayed system-default timezone to make it explicit — RESOLVED
|
||||
|
||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:111-118` (with 362-372)
|
||||
**Status:** Fixed in commit `173e06e` — `fix(18): enable first-run timezone save when not explicitly set (WR-01)`
|
||||
**Issue:** When no row is stored, `GET /config/timezone` returns `isExplicitlySet: false` and `timezone` = the D-06 fallback (e.g. `UTC` inside Docker, or the detected zone). The UI pre-fills the input with that fallback (`effectiveTimezoneInput = timezoneInput ?? storedTimezone`) and simultaneously shows the notice **"Using system default — save a timezone to make it explicit."** But `timezoneSaveDisabled` includes `effectiveTimezoneInput === storedTimezone` (line 118). Since the input already equals `storedTimezone` (the fallback), **Save is disabled** — the admin literally cannot perform the action the notice instructs. They must first change the value to something else, then back, or pick a different zone, to enable Save. This defeats the explicit-set affordance for the common first-run case where the detected/fallback zone is already correct.
|
||||
**Fix applied:**
|
||||
```tsx
|
||||
const isExplicit = timezoneQuery.data?.isExplicitlySet ?? false;
|
||||
const timezoneSaveDisabled =
|
||||
timezoneMutation.isPending ||
|
||||
effectiveTimezoneInput === '' ||
|
||||
(isExplicit && effectiveTimezoneInput === storedTimezone);
|
||||
```
|
||||
Unit tests added in `apps/pwa/src/routes/AdminPage.timezone.test.ts` (7 tests covering first-run enabled and already-explicit disabled cases).
|
||||
|
||||
### WR-02: Seed endpoint has an unhandled duplicate-key path (race + no try/catch) — RESOLVED
|
||||
|
||||
**File:** `apps/api/src/routes/admin.ts:248-265`
|
||||
**Status:** Fixed in commit `bda31a3` — `fix(18): make timezone seed idempotent under concurrent race (WR-02)`
|
||||
**Issue:** `POST /config/timezone/seed` does a non-transactional SELECT, then a bare `INSERT` (no `onDuplicateKeyUpdate`) when `!alreadySet`. Two concurrent seeds, or a seed racing a `PUT`, can both observe "unset" and both attempt the INSERT; the second hits the `app_config.key` primary-key constraint and throws. Because there is no `try/catch`, the rejection propagates as an unhandled 500 rather than the documented `200 { ok, seeded }`. The first-run wizard calling this on initial load makes the seed-vs-PUT overlap plausible. (For a two-person household the probability is low, hence Warning not Blocker — but the failure mode is a 500 with a stack, not graceful no-op.)
|
||||
**Fix applied:** Added `onDuplicateKeyUpdate({ set: { value: sql\`value\` } })` to the INSERT so the PK constraint can never be violated. The `seeded` flag still reflects the pre-flight SELECT. Three new tests added in `apps/api/tests/routes/admin.test.ts`: 403 access control on POST seed, `seeded:true` on first seed, and sequential idempotent second seed returns `seeded:false` without throwing.
|
||||
|
||||
### WR-03: GET /config/timezone issues a redundant second DB query on the unset path — ACCEPTED (not fixing)
|
||||
|
||||
**File:** `apps/api/src/routes/admin.ts:200-213`
|
||||
**Issue:** The handler selects `household_timezone` (lines 201-205), then when `row?.value == null` calls `await getHouseholdTimezone(db)` (line 210) — which runs the *same* `SELECT` again before applying the fallback chain. Two round-trips for the common first-run case. Not a correctness bug, but it duplicates the query the accessor already performs and couples the handler to the accessor's internals.
|
||||
**Fix:** Compute the fallback inline from the already-fetched `row`, or have `getHouseholdTimezone` accept an optional pre-fetched value. Minimal inline version:
|
||||
```ts
|
||||
const isExplicitlySet = row?.value != null;
|
||||
const timezone = isExplicitlySet
|
||||
? (row.value as string)
|
||||
: (process.env.TZ ?? Intl.DateTimeFormat().resolvedOptions().timeZone);
|
||||
```
|
||||
(Keeps a single SELECT; mirrors the accessor's D-06 chain — extract a shared `resolveFallbackTz()` if you want to avoid drift.)
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `isValidIanaTimezone` accepts non-canonical zones Intl tolerates
|
||||
|
||||
**File:** `apps/api/src/lib/householdTimezone.ts:48-55`
|
||||
**Issue:** `Intl.DateTimeFormat(undefined, { timeZone: tz })` accepts inputs beyond the canonical IANA set the picker offers (e.g. legacy aliases like `Etc/GMT+5`, or case-insensitive `utc`). This is *correct and intentional* for the security gate (it rejects garbage, which is all that matters before the parameterized write), and the comment correctly explains why `Intl.supportedValuesOf` is avoided. Noting only that the stored value may not match a `datalist` option exactly, which is harmless. No fix required; documenting the accepted-set breadth would help future readers.
|
||||
**Fix:** Optional — add a one-line note that any zone Intl accepts is storable, not only `supportedValuesOf` entries.
|
||||
|
||||
### IN-02: Detected-zone affordance hidden once input matches detected zone
|
||||
|
||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:405`
|
||||
**Issue:** `detectedTz && detectedTz !== effectiveTimezoneInput` hides the "Use detected" button as soon as the input equals the detected zone. Combined with WR-01, a first-run user whose fallback already equals their browser zone sees neither an enabled Save nor the detected affordance — there is no single tap to make the correct value explicit. Resolving WR-01 removes the dead-end; this is just the contributing display condition.
|
||||
**Fix:** No change needed once WR-01 is fixed.
|
||||
|
||||
### IN-03: Broker reads the timezone once per tick / per outbox item (acceptable, worth a note)
|
||||
|
||||
**File:** `apps/api/src/broker/reminderScheduler.ts:250`, `apps/api/src/broker/outboxWorker.ts:504,612`
|
||||
**Issue:** The scheduler reads `getHouseholdTimezone(db)` once per tick (good — hoisted above the `allDayRows` loop). The outbox worker reads it once per processed all-day create/update item. Both are correct (no caching bug, picks up admin changes promptly) and async ordering is sound — `await` completes before `computeAlertInstantUtc` consumes `tz`. Per-item reads in the worker are a minor extra query but well within scope and not a performance concern at household scale (performance is out of v1 review scope regardless).
|
||||
**Fix:** None required. If desired later, hoist the worker read to once per drain batch.
|
||||
|
||||
### IN-04: `<section aria-label="Timezone">` relies on implicit region role for e2e selectors
|
||||
|
||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:332`; `apps/pwa/e2e/timezone-verify.spec.ts:17`
|
||||
**Issue:** The e2e spec selects `getByRole('region', { name: 'Timezone' })`. A `<section>` only exposes the `region` role when it has an accessible name — which `aria-label` provides here, so the selectors are valid. This is correct; flagged only because the coupling is implicit (removing the `aria-label` would silently break both the a11y affordance and the e2e suite).
|
||||
**Fix:** None required; keep the `aria-label`.
|
||||
|
||||
---
|
||||
|
||||
## Test Coverage Assessment
|
||||
|
||||
Coverage is strong and behavior-pinned:
|
||||
- **householdTimezone.test.ts** covers all four `getHouseholdTimezone` branches (stored / TZ env / Intl / null-value fall-through) and `isValidIanaTimezone` valid+invalid incl. the `UTC` Pitfall-2 case.
|
||||
- **admin.test.ts** covers access control (403) for GET/PUT, the PUT→GET round-trip, UTC acceptance, invalid-zone 400 + no-write assertion, seed-when-unset, and seed-no-overwrite (D-03).
|
||||
|
||||
Gaps (not blocking, recommend adding):
|
||||
- No test for `POST /config/timezone/seed` returning the `seeded: boolean` flag value explicitly (only the stored value is asserted).
|
||||
- No test asserting `POST seed` 403 for non-admins (GET and PUT are covered; seed is the same guard but untested).
|
||||
- No test for the WR-01 UI dead-end (admin saving the displayed default to make it explicit) — add once WR-01 is fixed.
|
||||
- Broker rewire tests (reminderScheduler/outboxWorker) were not re-read in full here; confirm they assert the stored value is actually threaded into `computeAlertInstantUtc` (not just that the accessor is called).
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-15T02:49:55Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user