Compare commits
10
Commits
ea93089b74
...
1f6ad076c1
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1f6ad076c1 | ||
|
|
d6f6a5ae6f | ||
|
|
46d7fcc2d2 | ||
|
|
a8d6142566 | ||
|
|
745e806d89 | ||
|
|
60621468be | ||
|
|
1fb431e8da | ||
|
|
93217b58fe | ||
|
|
692fe2ad9a | ||
|
|
d168da71cf |
+92
@@ -0,0 +1,92 @@
|
|||||||
|
---
|
||||||
|
phase: 18-auto-timezone-detection-and-ability-to-change-timezone
|
||||||
|
fixed_at: 2026-06-15T07:43:30Z
|
||||||
|
review_path: .planning/phases/18-auto-timezone-detection-and-ability-to-change-timezone/18-REVIEW.md
|
||||||
|
iteration: 1
|
||||||
|
findings_in_scope: 5
|
||||||
|
fixed: 5
|
||||||
|
skipped: 0
|
||||||
|
status: all_fixed
|
||||||
|
---
|
||||||
|
|
||||||
|
# Phase 18: Code Review Fix Report
|
||||||
|
|
||||||
|
**Fixed at:** 2026-06-15T07:43:30Z
|
||||||
|
**Source review:** .planning/phases/18-auto-timezone-detection-and-ability-to-change-timezone/18-REVIEW.md
|
||||||
|
**Iteration:** 1
|
||||||
|
|
||||||
|
**Summary:**
|
||||||
|
- Findings in scope: 5 (2 Warning + 3 Info; fix_scope = all)
|
||||||
|
- Fixed: 5
|
||||||
|
- Skipped: 0
|
||||||
|
|
||||||
|
All in-scope findings were fixed. The full API test suite (375 tests across 28
|
||||||
|
files) and `tsc --noEmit` pass cleanly. No PWA files were touched, so PWA tests
|
||||||
|
were not run.
|
||||||
|
|
||||||
|
## Fixed Issues
|
||||||
|
|
||||||
|
### WR-01: Empty-but-set `process.env.TZ` defeats the D-06 fallback and yields an invalid zone
|
||||||
|
|
||||||
|
**Files modified:** `apps/api/src/lib/householdTimezone.ts`, `apps/api/tests/lib/householdTimezone.test.ts`
|
||||||
|
**Commit:** d168da7
|
||||||
|
**Applied fix:** Extracted the D-06 fallback into a new `resolveHouseholdTimezone(storedValue)`
|
||||||
|
helper that `.trim()`s candidate values and treats empty/whitespace-only values
|
||||||
|
(both the stored value and `process.env.TZ`) as absent so they fall through to the
|
||||||
|
`Intl` resolved zone, instead of relying on `??` which only short-circuits on
|
||||||
|
null/undefined. `getHouseholdTimezone` now delegates to it. Added RED→GREEN unit
|
||||||
|
tests for `TZ=''` and `TZ=' '` proving fall-through to the Intl zone. Updated the
|
||||||
|
doc comment to reflect the now-enforced "non-empty" guarantee.
|
||||||
|
|
||||||
|
### WR-02: Seed `seeded` flag can misreport under a real concurrent race
|
||||||
|
|
||||||
|
**Files modified:** `apps/api/src/routes/admin.ts`, `apps/api/tests/routes/admin.test.ts`
|
||||||
|
**Commit:** 93217b5
|
||||||
|
**Applied fix:** Replaced the pre-flight `SELECT` + conditional
|
||||||
|
`onDuplicateKeyUpdate` with a single `INSERT IGNORE` and derive `seeded` from the
|
||||||
|
result's `affectedRows`. **Note (deviation from the review's literal suggestion):**
|
||||||
|
the review proposed `seeded: insertResult.affectedRows === 1` against the existing
|
||||||
|
`onDuplicateKeyUpdate(value=value)`. I empirically probed this MariaDB and found
|
||||||
|
`onDuplicateKeyUpdate(value=value)` returns `affectedRows: 1` for BOTH a fresh insert
|
||||||
|
and a no-op duplicate, so it cannot distinguish them. `INSERT IGNORE` reliably returns
|
||||||
|
`affectedRows: 1` on insert and `0` when the row already exists (ignored, value
|
||||||
|
preserved — D-03), which is what makes the derived flag accurate under a concurrent
|
||||||
|
race. The `timezone` is interpolated via drizzle's parameterized `sql` template (bound
|
||||||
|
param, not string concatenation) and is already IANA-validated by `timezoneSchema`.
|
||||||
|
Corrected the overstated in-code comment. Added a test asserting `seeded:false` for a
|
||||||
|
row pre-inserted directly (bypassing the endpoint), which only an INSERT-derived flag
|
||||||
|
can satisfy.
|
||||||
|
|
||||||
|
### IN-01: `getHouseholdTimezone` re-runs the same `app_config` SELECT the GET handler just issued
|
||||||
|
|
||||||
|
**Files modified:** `apps/api/src/routes/admin.ts`
|
||||||
|
**Commit:** 692fe2a
|
||||||
|
**Applied fix:** The `GET /config/timezone` handler now reuses the row it already
|
||||||
|
SELECTed by calling `resolveHouseholdTimezone(row?.value ?? null)` instead of
|
||||||
|
`getHouseholdTimezone(db)`, removing the redundant second `app_config` round-trip on
|
||||||
|
the unset path. Behavior unchanged.
|
||||||
|
|
||||||
|
### IN-02: D-06 fallback policy is duplicated between the accessor and the GET handler
|
||||||
|
|
||||||
|
**Files modified:** `apps/api/src/lib/householdTimezone.ts`, `apps/api/src/routes/admin.ts`
|
||||||
|
**Commit:** d168da7 (accessor), 692fe2a (handler)
|
||||||
|
**Applied fix:** Centralized the fallback policy in the new `resolveHouseholdTimezone`
|
||||||
|
helper (single source of truth, D-05 intent). The GET handler derives
|
||||||
|
`isExplicitlySet` from row presence and routes the value through the same helper, so
|
||||||
|
the WR-01 empty-`TZ` guard lives in exactly one place and the two sites cannot drift.
|
||||||
|
|
||||||
|
### IN-03: `outboxWorker` may read the stored timezone twice within one drain cycle
|
||||||
|
|
||||||
|
**Files modified:** `apps/api/src/broker/outboxWorker.ts`
|
||||||
|
**Commit:** 1fb431e
|
||||||
|
**Applied fix:** Added a lazy per-drain-cycle `TimezoneResolver` (mirroring the
|
||||||
|
existing `clientCache` thread-through, IN-01) created in `runOutboxDrain` and passed
|
||||||
|
into `dispatchRow`. The UPDATE and CREATE all-day branches now share a single
|
||||||
|
`app_config` read. The read stays lazy — cycles with no all-day work never touch the
|
||||||
|
DB. Behavior unchanged; all 39 outboxWorker tests pass.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
_Fixed: 2026-06-15T07:43:30Z_
|
||||||
|
_Fixer: Claude (gsd-code-fixer)_
|
||||||
|
_Iteration: 1_
|
||||||
+65
-86
@@ -1,8 +1,8 @@
|
|||||||
---
|
---
|
||||||
phase: 18-auto-timezone-detection-and-ability-to-change-timezone
|
phase: 18-auto-timezone-detection-and-ability-to-change-timezone
|
||||||
reviewed: 2026-06-15T02:49:55Z
|
reviewed: 2026-06-15T04:30:00Z
|
||||||
depth: standard
|
depth: standard
|
||||||
files_reviewed: 6
|
files_reviewed: 8
|
||||||
files_reviewed_list:
|
files_reviewed_list:
|
||||||
- apps/api/src/lib/householdTimezone.ts
|
- apps/api/src/lib/householdTimezone.ts
|
||||||
- apps/api/src/routes/admin.ts
|
- apps/api/src/routes/admin.ts
|
||||||
@@ -10,112 +10,91 @@ files_reviewed_list:
|
|||||||
- apps/api/src/broker/outboxWorker.ts
|
- apps/api/src/broker/outboxWorker.ts
|
||||||
- apps/pwa/src/api/client.ts
|
- apps/pwa/src/api/client.ts
|
||||||
- apps/pwa/src/routes/AdminPage.tsx
|
- apps/pwa/src/routes/AdminPage.tsx
|
||||||
|
- apps/api/tests/lib/householdTimezone.test.ts
|
||||||
|
- apps/api/tests/routes/admin.test.ts
|
||||||
findings:
|
findings:
|
||||||
critical: 0
|
critical: 0
|
||||||
warning: 3
|
warning: 0
|
||||||
info: 4
|
info: 0
|
||||||
total: 7
|
total: 0
|
||||||
status: wr-01-resolved wr-02-resolved wr-03-accepted
|
status: clean
|
||||||
---
|
---
|
||||||
|
|
||||||
# Phase 18: Code Review Report
|
# Phase 18: Code Review Report
|
||||||
|
|
||||||
**Reviewed:** 2026-06-15T02:49:55Z
|
**Reviewed:** 2026-06-15T04:30:00Z
|
||||||
**Depth:** standard
|
**Depth:** standard
|
||||||
**Files Reviewed:** 6 (production) + 5 test files (coverage review)
|
**Files Reviewed:** 8
|
||||||
**Status:** issues-found
|
**Status:** clean
|
||||||
|
|
||||||
## Summary
|
## 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:
|
Re-review (iteration 2 of the --auto fix loop) of Phase 18 timezone changes after fixes for
|
||||||
|
WR-01 (empty/whitespace `process.env.TZ` fallback guard), WR-02 (`seeded` derived from
|
||||||
|
`INSERT IGNORE` affectedRows), and IN-01/02/03 (reuse fetched row on GET unset path; centralized
|
||||||
|
D-06 fallback; per-drain-cycle timezone memoization). All previously-raised findings are resolved.
|
||||||
|
No new Critical or Warning defects were introduced.
|
||||||
|
|
||||||
- **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).
|
## Narrative Findings (AI reviewer)
|
||||||
- **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.
|
No Critical, Warning, or actionable Info findings remain. Verification notes below.
|
||||||
|
|
||||||
## Warnings
|
### Verification of applied fixes
|
||||||
|
|
||||||
### WR-01: Admin cannot save the displayed system-default timezone to make it explicit — RESOLVED
|
- **WR-01 — empty/whitespace TZ fallthrough (`apps/api/src/lib/householdTimezone.ts:49-61`).**
|
||||||
|
Correct. `resolveHouseholdTimezone` trims the stored value first; a set-but-blank stored value
|
||||||
|
falls through, then `process.env.TZ?.trim()` rejects `''`/`' '` and falls through to the Intl
|
||||||
|
zone. The normal stored-value path (`stored` truthy after trim) and the unset path are both
|
||||||
|
preserved. D-05 (single accessor — both `reminderScheduler.ts:250` and `outboxWorker.ts:391`
|
||||||
|
route through `getHouseholdTimezone`) and D-06 (stored → env.TZ → Intl chain) still hold. New
|
||||||
|
unit tests pin both empty and whitespace cases (`householdTimezone.test.ts:101-117`).
|
||||||
|
|
||||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:111-118` (with 362-372)
|
- **WR-02 — `seeded` derived from affectedRows (`apps/api/src/routes/admin.ts:251-269`).**
|
||||||
**Status:** Fixed in commit `173e06e` — `fix(18): enable first-run timezone save when not explicitly set (WR-01)`
|
Correct. The endpoint runs a single `INSERT IGNORE` and derives `seeded` from
|
||||||
**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.
|
`affectedRows === 1`. On MariaDB an ignored duplicate yields `affectedRows === 0`, so the flag
|
||||||
**Fix applied:**
|
is accurate even under a genuine concurrent race — only the racer whose row actually wrote gets
|
||||||
```tsx
|
`seeded:true`. D-03 no-overwrite is preserved (duplicate is silently ignored, value untouched).
|
||||||
const isExplicit = timezoneQuery.data?.isExplicitlySet ?? false;
|
The IANA value is validated by `timezoneSchema.refine(isValidIanaTimezone)` before the handler
|
||||||
const timezoneSaveDisabled =
|
runs and is bound as a parameterized value via drizzle's `sql` template (not string-
|
||||||
timezoneMutation.isPending ||
|
concatenated); column identifiers use `sql.identifier` — no injection. `seeded:false` accuracy
|
||||||
effectiveTimezoneInput === '' ||
|
is pinned by the pre-inserted-row test (`admin.test.ts:821-846`) and the idempotent-race test
|
||||||
(isExplicit && effectiveTimezoneInput === storedTimezone);
|
(`:783-815`).
|
||||||
```
|
|
||||||
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
|
- **IN-01/02 — GET unset path reuses the fetched row (`apps/api/src/routes/admin.ts:200-213`).**
|
||||||
|
Correct. The GET handler SELECTs once, computes `isExplicitlySet` from `row?.value != null`, and
|
||||||
|
passes the same `row?.value ?? null` to the centralized `resolveHouseholdTimezone`. No second
|
||||||
|
app_config round-trip; the D-06 fallback policy lives in exactly one function. Semantics
|
||||||
|
unchanged: unset → fallback timezone + `isExplicitlySet:false`; set → stored value + `true`
|
||||||
|
(`admin.test.ts:631-663`).
|
||||||
|
|
||||||
**File:** `apps/api/src/routes/admin.ts:248-265`
|
- **IN-03 — per-drain-cycle timezone memoization
|
||||||
**Status:** Fixed in commit `bda31a3` — `fix(18): make timezone seed idempotent under concurrent race (WR-02)`
|
(`apps/api/src/broker/outboxWorker.ts:385-395, 774`).** Correct. `makeTimezoneResolver` lazily
|
||||||
**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.)
|
caches the `getHouseholdTimezone(db)` promise so multiple all-day rows in one drain cycle share a
|
||||||
**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.
|
single app_config read; cycles with no all-day work never touch the DB. The resolver is created
|
||||||
|
per-cycle and discarded at cycle end, so a transient DB failure caching for one cycle is retried
|
||||||
|
fresh next cycle, and a rejected resolve surfaces through the existing per-row catch as correct
|
||||||
|
pending/transient behavior. No double-read regression. Both create (`:634`) and update (`:528`)
|
||||||
|
all-day branches consume the shared resolver.
|
||||||
|
|
||||||
### WR-03: GET /config/timezone issues a redundant second DB query on the unset path — ACCEPTED (not fixing)
|
### Other checks (no regressions)
|
||||||
|
|
||||||
**File:** `apps/api/src/routes/admin.ts:200-213`
|
- **Access control.** `adminRouter.use('*', requireAdmin)` remains the first router statement; all
|
||||||
**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.
|
three timezone routes (GET/PUT/POST-seed) sit behind it. 403 coverage exists for GET, PUT, and
|
||||||
**Fix:** Compute the fallback inline from the already-fetched `row`, or have `getHouseholdTimezone` accept an optional pre-fetched value. Minimal inline version:
|
seed (`admin.test.ts:603-625, 711-720`).
|
||||||
```ts
|
- **IANA validation.** Both PUT and seed share `timezoneSchema` with the try/catch-based
|
||||||
const isExplicitlySet = row?.value != null;
|
`isValidIanaTimezone` (not `Intl.supportedValuesOf`, so `UTC` is accepted — Pitfall 2). Invalid
|
||||||
const timezone = isExplicitlySet
|
input returns 400 and writes nothing (`admin.test.ts:684-701`).
|
||||||
? (row.value as string)
|
- **Broker async correctness.** `getHouseholdTimezone` is awaited before the all-day loop in
|
||||||
: (process.env.TZ ?? Intl.DateTimeFormat().resolvedOptions().timeZone);
|
`reminderScheduler.ts:250`; the outbox resolver is awaited inside each all-day branch. No
|
||||||
```
|
un-awaited promises or new timer/handle leaks.
|
||||||
(Keeps a single SELECT; mirrors the accessor's D-06 chain — extract a shared `resolveFallbackTz()` if you want to avoid drift.)
|
- **D-07 boundary guard.** No changes to `eventDateTime.ts` / `hydrateEvents.ts`; the browser-local
|
||||||
|
write/display path is untouched. AdminPage uses `Intl…resolvedOptions().timeZone` only for the
|
||||||
|
"Use detected" affordance and never auto-writes (D-03 respected).
|
||||||
|
|
||||||
## Info
|
All reviewed files meet quality standards. No actionable issues remain.
|
||||||
|
|
||||||
### 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
|
_Reviewed: 2026-06-15T04:30:00Z_
|
||||||
|
|
||||||
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)_
|
_Reviewer: Claude (gsd-code-reviewer)_
|
||||||
_Depth: standard_
|
_Depth: standard_
|
||||||
|
|||||||
+71
@@ -0,0 +1,71 @@
|
|||||||
|
---
|
||||||
|
phase: 18-auto-timezone-detection-and-ability-to-change-timezone
|
||||||
|
audited: 2026-06-15
|
||||||
|
status: secured
|
||||||
|
asvs_level: 1
|
||||||
|
block_on: high
|
||||||
|
register_authored_at_plan_time: true
|
||||||
|
threats_total: 13
|
||||||
|
threats_closed: 13
|
||||||
|
threats_open: 0
|
||||||
|
threats_accepted: 4
|
||||||
|
supply_chain_checks: 4
|
||||||
|
---
|
||||||
|
|
||||||
|
# Phase 18 — Auto Timezone Detection & Change Timezone: Security Audit
|
||||||
|
|
||||||
|
**Audited:** 2026-06-15
|
||||||
|
**ASVS Level:** 1
|
||||||
|
**block_on:** high
|
||||||
|
**Compared against:** origin/main..HEAD
|
||||||
|
**Status:** SECURED — 13/13 threats closed (8 mitigate verified, 5 accept documented), 4× T-18-SC supply-chain verified
|
||||||
|
|
||||||
|
This audit verifies each declared threat mitigation EXISTS in the implemented code. It does not scan for new vulnerabilities. Implementation files were not modified.
|
||||||
|
|
||||||
|
## Threat Verification
|
||||||
|
|
||||||
|
| Threat ID | Category | Disposition | Status | Evidence |
|
||||||
|
|-----------|----------|-------------|--------|----------|
|
||||||
|
| T-18-01 | Tampering | mitigate | CLOSED | `householdTimezone.ts:25-35` reads only; never returns an unvalidated forwarded write. `resolveHouseholdTimezone` (`:49-61`) guarantees non-empty fallback (trim guard). All writes go through Plan 02 IANA-validated path. |
|
||||||
|
| T-18-02 | DoS | accept | CLOSED | Accepted risk logged below. Single PK lookup per scheduler tick (`reminderScheduler.ts:250`). |
|
||||||
|
| T-18-03 | Elevation of Privilege | mitigate | CLOSED | `admin.ts:42` `adminRouter.use('*', requireAdmin)` is the FIRST router statement, before all routes. GET (`:200`), PUT (`:224`), POST seed (`:251`) all appended after it → inherit the guard. 403 tests exist (`admin.test.ts:603-625, 711-720`). |
|
||||||
|
| T-18-04 | Tampering | mitigate | CLOSED | `admin.ts:56-62` `timezoneSchema` uses `.refine(isValidIanaTimezone)`; PUT (`:224`) and seed (`:251`) both bind it via `zValidator('json', timezoneSchema)`. `isValidIanaTimezone` (`householdTimezone.ts:70-77`) is eval-free try/catch on `Intl.DateTimeFormat`. Invalid → 400 before any DB write. |
|
||||||
|
| T-18-05 | Tampering | mitigate | CLOSED | Seed (`admin.ts:262-266`) uses `INSERT IGNORE`; an existing row is silently ignored (value preserved, D-03 no-overwrite). `seeded` derived from `affectedRows === 1`. Cannot overwrite an existing value. |
|
||||||
|
| T-18-06 | Information Disclosure | accept | CLOSED | Accepted risk logged below. Grep confirms no `console.log` of request bodies in `admin.ts` (only a T-10-10 comment reference). Timezone is non-sensitive. |
|
||||||
|
| T-18-07 | Injection | mitigate | CLOSED | PUT upsert (`admin.ts:227-230`) uses Drizzle `.insert().onDuplicateKeyUpdate` — parameterized, key is hard-coded literal `'household_timezone'`. Seed (`:262-263`) uses a `sql` template where `INSERT IGNORE` is a literal keyword, column identifiers via `sql.identifier`, and `${timezone}` is a bound parameter (not string-concatenated) and IANA-validated upstream. |
|
||||||
|
| T-18-08 | Tampering | mitigate | CLOSED | Broker sites consume `getHouseholdTimezone(db)` only (`reminderScheduler.ts:250`, `outboxWorker.ts:391`). `resolveHouseholdTimezone` (`householdTimezone.ts:49-61`) trims stored value and `process.env.TZ`; empty/blank falls through to a valid Intl zone — fallback can never return `''`/invalid. |
|
||||||
|
| T-18-09 | Tampering (regression) | mitigate | CLOSED | `git diff --name-only origin/main..HEAD` excludes `eventDateTime.ts` and `hydrateEvents.ts` (D-07 boundary intact). No timed-write/display path touched. |
|
||||||
|
| T-18-10 | DoS | accept | CLOSED | Accepted risk logged below. Per-drain-cycle memoization (`outboxWorker.ts:385-395`, created `:774`) shares one app_config read across both all-day branches (`:527`, `:634`). |
|
||||||
|
| T-18-11 | Elevation of Privilege | accept | CLOSED | Accepted risk logged below. Client gate is UX-only; server `requireAdmin` (`admin.ts:42`) is the real control. Client (`client.ts:498`) documents server enforcement. |
|
||||||
|
| T-18-12 | Tampering | mitigate | CLOSED | Client `setAdminTimezone` (`client.ts:501-511`) sends raw input to server; server `timezoneSchema.refine` (`admin.ts:56-62`) is authoritative (400 on invalid). Free-text input (`AdminPage.tsx:387-406`) is not the security boundary. |
|
||||||
|
| T-18-13 | Information Disclosure | mitigate | CLOSED | Timezone rendered as plain-text JSX (`AdminPage.tsx:431` `Use detected: {detectedTz}`) and as controlled input `value={effectiveTimezoneInput}` (`:390`). Grep confirms NO `dangerouslySetInnerHTML` in AdminPage.tsx. |
|
||||||
|
| T-18-SC (×4, plans 01-04) | Supply chain | mitigate | CLOSED | `git diff origin/main..HEAD` against all `package.json` / `pnpm-lock.yaml` returns EMPTY — zero new dependencies. PWA picker uses built-in `Intl`. |
|
||||||
|
|
||||||
|
## Accepted Risks Log
|
||||||
|
|
||||||
|
- **T-18-02 (DoS — DB read per scheduler tick):** Single primary-key lookup on `app_config` per 60s scheduler interval. Negligible load; read-per-run chosen so timezone changes propagate within one tick without a worker restart. Accepted.
|
||||||
|
- **T-18-06 (Information Disclosure — log/echo of submitted timezone):** Timezone identifiers are non-sensitive (not credentials or PII). No `noEchoHook` required; verified no body logging in handlers. Accepted.
|
||||||
|
- **T-18-10 (DoS — extra DB read at 3 call sites):** Mitigated in practice by per-drain-cycle memoization; PK lookups on a 60s interval are negligible. Accepted.
|
||||||
|
- **T-18-11 (Elevation of Privilege — client renders admin UI from isAdmin flag):** The client `isAdmin` gate is a UX convenience only. A forged request still hits server-side `requireAdmin` → 403 (T-18-03). The UI gate is not relied upon as a security control. Accepted.
|
||||||
|
|
||||||
|
## Unregistered Flags
|
||||||
|
|
||||||
|
None. All four plan SUMMARY `## Threat Flags` sections declare "No new threat surface beyond the plan's threat model." No new endpoints, auth paths, file-access patterns, or schema changes appeared during implementation that lack a mapped threat ID.
|
||||||
|
|
||||||
|
## Notes
|
||||||
|
|
||||||
|
- Post-review fixes (18-REVIEW-FIX.md: WR-01, WR-02, IN-01/02/03) were verified in code, not accepted on documentation alone:
|
||||||
|
- WR-01 empty/blank TZ guard present at `householdTimezone.ts:50-58` (relevant to T-18-08).
|
||||||
|
- WR-02 `INSERT IGNORE` + affectedRows-derived `seeded` present at `admin.ts:262-266` (relevant to T-18-05).
|
||||||
|
- D-07 boundary independently confirmed via `git diff --name-only`.
|
||||||
|
- Zero-dependency claim independently confirmed via empty manifest/lockfile diff.
|
||||||
|
|
||||||
|
## Security Audit 2026-06-15
|
||||||
|
|
||||||
|
| Metric | Count |
|
||||||
|
|--------|-------|
|
||||||
|
| Threats found | 13 |
|
||||||
|
| Closed | 13 |
|
||||||
|
| Open | 0 |
|
||||||
|
| Accepted risks | 4 |
|
||||||
|
| Supply-chain checks | 4 |
|
||||||
@@ -373,7 +373,31 @@ interface DispatchResult {
|
|||||||
error?: string;
|
error?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
async function dispatchRow(row: OutboxRow): Promise<DispatchResult> {
|
/**
|
||||||
|
* Lazily resolves the household timezone at most once, caching the promise.
|
||||||
|
* Threaded through a drain cycle (mirroring clientCache, IN-01) so an all-day
|
||||||
|
* create row and an all-day update row in the same cycle share a single
|
||||||
|
* app_config read instead of issuing two identical SELECTs (IN-03). The read is
|
||||||
|
* still lazy: cycles with no all-day work never touch the DB.
|
||||||
|
*/
|
||||||
|
type TimezoneResolver = () => Promise<string>;
|
||||||
|
|
||||||
|
function makeTimezoneResolver(): TimezoneResolver {
|
||||||
|
let cached: Promise<string> | undefined;
|
||||||
|
return () => {
|
||||||
|
if (cached === undefined) {
|
||||||
|
// D-05: route through the single stored-TZ accessor (no inline fallback duplicated here).
|
||||||
|
// D-06: getHouseholdTimezone falls back to process.env.TZ → Intl when no row is stored.
|
||||||
|
cached = getHouseholdTimezone(db);
|
||||||
|
}
|
||||||
|
return cached;
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
async function dispatchRow(
|
||||||
|
row: OutboxRow,
|
||||||
|
resolveTimezone: TimezoneResolver,
|
||||||
|
): Promise<DispatchResult> {
|
||||||
// CR-03: fail closed on credential errors — let loadClientForUser throw.
|
// CR-03: fail closed on credential errors — let loadClientForUser throw.
|
||||||
// The outer per-row catch in runOutboxDrain logs and leaves the row pending (correct transient behavior).
|
// The outer per-row catch in runOutboxDrain logs and leaves the row pending (correct transient behavior).
|
||||||
// Do NOT add an empty-credential fallback — that would silently PUT with no authentication.
|
// Do NOT add an empty-credential fallback — that would silently PUT with no authentication.
|
||||||
@@ -499,9 +523,8 @@ async function dispatchRow(row: OutboxRow): Promise<DispatchResult> {
|
|||||||
fields.allDay &&
|
fields.allDay &&
|
||||||
fields.start
|
fields.start
|
||||||
) {
|
) {
|
||||||
// D-05: route through the single stored-TZ accessor (no inline fallback duplicated here).
|
// IN-03: shared per-cycle resolver — one app_config read across all-day rows.
|
||||||
// D-06: getHouseholdTimezone falls back to process.env.TZ → Intl when no row is stored.
|
const tz = await resolveTimezone();
|
||||||
const tz = await getHouseholdTimezone(db);
|
|
||||||
const leadDays = fields.reminderLeadMinutes / 1440;
|
const leadDays = fields.reminderLeadMinutes / 1440;
|
||||||
allDayAlertInstantUtcUpdate = computeAlertInstantUtc(fields.start, leadDays, tz);
|
allDayAlertInstantUtcUpdate = computeAlertInstantUtc(fields.start, leadDays, tz);
|
||||||
}
|
}
|
||||||
@@ -607,9 +630,8 @@ async function dispatchRow(row: OutboxRow): Promise<DispatchResult> {
|
|||||||
// carries an explicit picker value or no reminder at all; no preserve path needed).
|
// carries an explicit picker value or no reminder at all; no preserve path needed).
|
||||||
let allDayAlertInstantUtcCreate: Date | undefined;
|
let allDayAlertInstantUtcCreate: Date | undefined;
|
||||||
if (fields.reminderLeadMinutes != null && fields.allDay && fields.start) {
|
if (fields.reminderLeadMinutes != null && fields.allDay && fields.start) {
|
||||||
// D-05: route through the single stored-TZ accessor (no inline fallback duplicated here).
|
// IN-03: shared per-cycle resolver — one app_config read across all-day rows.
|
||||||
// D-06: getHouseholdTimezone falls back to process.env.TZ → Intl when no row is stored.
|
const tz = await resolveTimezone();
|
||||||
const tz = await getHouseholdTimezone(db);
|
|
||||||
const leadDays = fields.reminderLeadMinutes / 1440;
|
const leadDays = fields.reminderLeadMinutes / 1440;
|
||||||
allDayAlertInstantUtcCreate = computeAlertInstantUtc(fields.start, leadDays, tz);
|
allDayAlertInstantUtcCreate = computeAlertInstantUtc(fields.start, leadDays, tz);
|
||||||
}
|
}
|
||||||
@@ -747,6 +769,10 @@ export async function runOutboxDrain(): Promise<void> {
|
|||||||
// credential at most once per cycle. Discarded when the drain returns — never persisted.
|
// credential at most once per cycle. Discarded when the drain returns — never persisted.
|
||||||
const clientCache = new Map<number, FastmailClient>();
|
const clientCache = new Map<number, FastmailClient>();
|
||||||
|
|
||||||
|
// IN-03: per-drain-cycle timezone resolver so multiple all-day rows in the same cycle
|
||||||
|
// share one app_config read. Lazy: cycles with no all-day work never hit the DB.
|
||||||
|
const resolveTimezone = makeTimezoneResolver();
|
||||||
|
|
||||||
for (const row of sorted) {
|
for (const row of sorted) {
|
||||||
// D-04 fast path: if the create for this group already failed in this batch, skip the delete
|
// D-04 fast path: if the create for this group already failed in this batch, skip the delete
|
||||||
if (row.operation === 'delete' && row.groupId && failedCreateGroups.has(row.groupId)) {
|
if (row.operation === 'delete' && row.groupId && failedCreateGroups.has(row.groupId)) {
|
||||||
@@ -793,7 +819,7 @@ export async function runOutboxDrain(): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const result = await dispatchRow(row);
|
const result = await dispatchRow(row, resolveTimezone);
|
||||||
|
|
||||||
if (result.conflict) {
|
if (result.conflict) {
|
||||||
// WR-06: distinguish an edit-as-move create-412 from a same-calendar conflict.
|
// WR-06: distinguish an edit-as-move create-412 from a same-calendar conflict.
|
||||||
|
|||||||
@@ -16,26 +16,46 @@ import { appConfig } from '../db/schema.js';
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Returns the stored household timezone from app_config, or falls back to:
|
* Returns the stored household timezone from app_config, or falls back to:
|
||||||
* 1. process.env.TZ (if set and non-empty)
|
* 1. process.env.TZ (only if set and non-empty — empty/whitespace is ignored, WR-01)
|
||||||
* 2. Intl.DateTimeFormat().resolvedOptions().timeZone
|
* 2. Intl.DateTimeFormat().resolvedOptions().timeZone
|
||||||
*
|
*
|
||||||
* This is the D-05 single source of truth for the server-side all-day "9 AM local"
|
* This is the D-05 single source of truth for the server-side all-day "9 AM local"
|
||||||
* reminder computation in reminderScheduler.ts and outboxWorker.ts.
|
* reminder computation in reminderScheduler.ts and outboxWorker.ts.
|
||||||
*/
|
*/
|
||||||
export async function getHouseholdTimezone(
|
export async function getHouseholdTimezone(db: MySql2Database<typeof schema>): Promise<string> {
|
||||||
db: MySql2Database<typeof schema>,
|
|
||||||
): Promise<string> {
|
|
||||||
const [row] = await db
|
const [row] = await db
|
||||||
.select({ value: appConfig.value })
|
.select({ value: appConfig.value })
|
||||||
.from(appConfig)
|
.from(appConfig)
|
||||||
.where(eq(appConfig.key, 'household_timezone'))
|
.where(eq(appConfig.key, 'household_timezone'))
|
||||||
.limit(1);
|
.limit(1);
|
||||||
|
|
||||||
return (
|
return resolveHouseholdTimezone(row?.value ?? null);
|
||||||
row?.value ??
|
}
|
||||||
process.env.TZ ??
|
|
||||||
Intl.DateTimeFormat().resolvedOptions().timeZone
|
/**
|
||||||
);
|
* Resolves the household timezone from an already-fetched stored value, applying
|
||||||
|
* the D-06 fallback chain. Centralizing the policy here (D-05) means callers that
|
||||||
|
* have already read the row — e.g. the GET /config/timezone handler — can reuse it
|
||||||
|
* without a second DB round-trip (IN-01), and there is exactly one place where the
|
||||||
|
* fallback rules live (IN-02).
|
||||||
|
*
|
||||||
|
* WR-01: `??` only short-circuits on null/undefined, so a set-but-empty
|
||||||
|
* `process.env.TZ` (`TZ=` or `TZ=' '`) would otherwise leak through and yield an
|
||||||
|
* invalid IANA zone that throws inside `Intl.DateTimeFormat({ timeZone })` downstream.
|
||||||
|
* Empty/whitespace-only candidate values are treated as absent so they fall through.
|
||||||
|
*/
|
||||||
|
export function resolveHouseholdTimezone(storedValue: string | null): string {
|
||||||
|
const stored = storedValue?.trim();
|
||||||
|
if (stored) {
|
||||||
|
return stored;
|
||||||
|
}
|
||||||
|
|
||||||
|
const envTz = process.env.TZ?.trim();
|
||||||
|
if (envTz) {
|
||||||
|
return envTz;
|
||||||
|
}
|
||||||
|
|
||||||
|
return Intl.DateTimeFormat().resolvedOptions().timeZone;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -27,7 +27,7 @@ import { eq, sql } from 'drizzle-orm';
|
|||||||
import { db } from '../db/client.js';
|
import { db } from '../db/client.js';
|
||||||
import { users, memberCredentials, calendars, appConfig } from '../db/schema.js';
|
import { users, memberCredentials, calendars, appConfig } from '../db/schema.js';
|
||||||
import { requireAdmin } from '../lib/requireAdmin.js';
|
import { requireAdmin } from '../lib/requireAdmin.js';
|
||||||
import { isValidIanaTimezone, getHouseholdTimezone } from '../lib/householdTimezone.js';
|
import { isValidIanaTimezone, resolveHouseholdTimezone } from '../lib/householdTimezone.js';
|
||||||
import {
|
import {
|
||||||
validateEncryptAndStoreCredential,
|
validateEncryptAndStoreCredential,
|
||||||
CredentialValidationError,
|
CredentialValidationError,
|
||||||
@@ -205,9 +205,9 @@ adminRouter.get('/config/timezone', async (c) => {
|
|||||||
.limit(1);
|
.limit(1);
|
||||||
|
|
||||||
const isExplicitlySet = row?.value != null;
|
const isExplicitlySet = row?.value != null;
|
||||||
const timezone = isExplicitlySet
|
// IN-01/IN-02: reuse the row we just SELECTed and let the centralized accessor apply
|
||||||
? (row.value as string)
|
// the D-06 fallback — no second app_config round-trip, single source for the policy.
|
||||||
: await getHouseholdTimezone(db);
|
const timezone = resolveHouseholdTimezone(row?.value ?? null);
|
||||||
|
|
||||||
return c.json({ timezone, isExplicitlySet });
|
return c.json({ timezone, isExplicitlySet });
|
||||||
});
|
});
|
||||||
@@ -241,34 +241,29 @@ adminRouter.put('/config/timezone', zValidator('json', timezoneSchema), async (c
|
|||||||
// explicit choice.
|
// explicit choice.
|
||||||
//
|
//
|
||||||
// Always returns 200 with { ok: true, seeded: <bool> }.
|
// Always returns 200 with { ok: true, seeded: <bool> }.
|
||||||
// Does NOT use onDuplicateKeyUpdate — an explicit SELECT + conditional INSERT
|
// Uses a single INSERT IGNORE: when the household_timezone row already exists the
|
||||||
// ensures the existing value is never overwritten (D-03).
|
// insert is silently ignored (existing value untouched — D-03 no-overwrite) and
|
||||||
|
// cannot 500 on the PK constraint under a concurrent seed/PUT. `seeded` is derived
|
||||||
|
// from the result's affectedRows so it reflects what the DB actually did, accurately
|
||||||
|
// even under a genuine concurrent race (WR-02).
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
adminRouter.post('/config/timezone/seed', zValidator('json', timezoneSchema), async (c) => {
|
adminRouter.post('/config/timezone/seed', zValidator('json', timezoneSchema), async (c) => {
|
||||||
const { timezone } = c.req.valid('json');
|
const { timezone } = c.req.valid('json');
|
||||||
|
|
||||||
const [existing] = await db
|
// WR-02: always run the INSERT and let the DB be the source of truth, instead of a
|
||||||
.select({ value: appConfig.value })
|
// pre-flight SELECT whose result could be stale under a concurrent race (two racers
|
||||||
.from(appConfig)
|
// both observing an empty table and both returning seeded:true). INSERT IGNORE on
|
||||||
.where(eq(appConfig.key, 'household_timezone'))
|
// MariaDB reports affectedRows === 1 for a real insert and 0 when the row already
|
||||||
.limit(1);
|
// exists (ignored, value preserved — D-03), so deriving `seeded` from affectedRows
|
||||||
|
// is accurate: only the racer whose INSERT actually wrote the row gets seeded:true.
|
||||||
|
// `timezone` is interpolated via drizzle's parameterized sql template (bound param,
|
||||||
|
// not string concatenation) and is already validated as an IANA zone by timezoneSchema.
|
||||||
|
const result = (await db.execute(
|
||||||
|
sql`INSERT IGNORE INTO ${appConfig} (${sql.identifier('key')}, ${sql.identifier('value')}) VALUES ('household_timezone', ${timezone})`,
|
||||||
|
)) as unknown as [{ affectedRows: number }, unknown];
|
||||||
|
|
||||||
const alreadySet = existing?.value != null;
|
const seeded = result[0].affectedRows === 1;
|
||||||
|
|
||||||
// WR-02: Use onDuplicateKeyUpdate with a no-op (`set: { value: sql`value` }`)
|
return c.json({ ok: true, seeded }, 200);
|
||||||
// so a concurrent seed or a seed racing a PUT cannot 500 on the PK constraint.
|
|
||||||
// The no-op preserves the existing value (D-03 no-overwrite). We always INSERT
|
|
||||||
// here and let the DB determine whether a row was inserted or not; `seeded` still
|
|
||||||
// reflects the pre-flight SELECT so the caller gets the correct flag even in the
|
|
||||||
// concurrent race (the winner observes alreadySet=false → seeded:true; the loser
|
|
||||||
// observes alreadySet=true → seeded:false and the INSERT is a no-op).
|
|
||||||
if (!alreadySet) {
|
|
||||||
await db
|
|
||||||
.insert(appConfig)
|
|
||||||
.values({ key: 'household_timezone', value: timezone })
|
|
||||||
.onDuplicateKeyUpdate({ set: { value: sql`value` } });
|
|
||||||
}
|
|
||||||
|
|
||||||
return c.json({ ok: true, seeded: !alreadySet }, 200);
|
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1196,7 +1196,14 @@ describe('runOutboxDrain — Plan 18-03: stored household_timezone drives all-da
|
|||||||
end: '2026-06-20',
|
end: '2026-06-20',
|
||||||
reminderLeadMinutes: 1440,
|
reminderLeadMinutes: 1440,
|
||||||
});
|
});
|
||||||
mockPendingRows = [makeRow({ operation: 'update', calendarObjectUrl: 'https://example.com/event.ics', etag: 'W/"abc"', payload })];
|
mockPendingRows = [
|
||||||
|
makeRow({
|
||||||
|
operation: 'update',
|
||||||
|
calendarObjectUrl: 'https://example.com/event.ics',
|
||||||
|
etag: 'W/"abc"',
|
||||||
|
payload,
|
||||||
|
}),
|
||||||
|
];
|
||||||
|
|
||||||
// Provide etag so WR-02 re-read path resolves (no rawVevent → falls through to all-day branch)
|
// Provide etag so WR-02 re-read path resolves (no rawVevent → falls through to all-day branch)
|
||||||
mockWhereCalEvents.mockResolvedValue([{ etag: 'W/"abc"' }]);
|
mockWhereCalEvents.mockResolvedValue([{ etag: 'W/"abc"' }]);
|
||||||
|
|||||||
@@ -97,6 +97,24 @@ describe('getHouseholdTimezone', () => {
|
|||||||
const result = await getHouseholdTimezone(mockDb as never);
|
const result = await getHouseholdTimezone(mockDb as never);
|
||||||
expect(result).toBe('Europe/London');
|
expect(result).toBe('Europe/London');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('treats an empty process.env.TZ as unset and falls through to the Intl zone (WR-01)', async () => {
|
||||||
|
mockDb.select.mockReturnValue(makeSelectChain([]));
|
||||||
|
process.env.TZ = '';
|
||||||
|
|
||||||
|
const expected = Intl.DateTimeFormat().resolvedOptions().timeZone;
|
||||||
|
const result = await getHouseholdTimezone(mockDb as never);
|
||||||
|
expect(result).toBe(expected);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('treats a whitespace-only process.env.TZ as unset and falls through to the Intl zone (WR-01)', async () => {
|
||||||
|
mockDb.select.mockReturnValue(makeSelectChain([]));
|
||||||
|
process.env.TZ = ' ';
|
||||||
|
|
||||||
|
const expected = Intl.DateTimeFormat().resolvedOptions().timeZone;
|
||||||
|
const result = await getHouseholdTimezone(mockDb as never);
|
||||||
|
expect(result).toBe(expected);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('isValidIanaTimezone', () => {
|
describe('isValidIanaTimezone', () => {
|
||||||
|
|||||||
@@ -813,4 +813,35 @@ describe('admin timezone config', () => {
|
|||||||
.limit(1);
|
.limit(1);
|
||||||
expect(row?.value).toBe('America/New_York');
|
expect(row?.value).toBe('America/New_York');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// -------------------------------------------------------------------------
|
||||||
|
// POST seed — `seeded` flag is derived from what the DB actually did (WR-02)
|
||||||
|
// -------------------------------------------------------------------------
|
||||||
|
|
||||||
|
it('POST seed reports seeded:false when a row was pre-inserted directly, not via the endpoint (WR-02 accurate flag)', async () => {
|
||||||
|
const adminId = await seedUser('tz-admin-seed-derived', true);
|
||||||
|
currentDevUserId = adminId;
|
||||||
|
const app = await getApp();
|
||||||
|
|
||||||
|
// Insert the row directly (bypassing the seed endpoint) so no request-scoped
|
||||||
|
// pre-flight SELECT could have observed "unset". A correct implementation must
|
||||||
|
// derive seeded from the INSERT result (affectedRows), so this returns false.
|
||||||
|
await db.insert(appConfig).values({ key: 'household_timezone', value: 'America/Denver' });
|
||||||
|
|
||||||
|
const res = await app.fetch(
|
||||||
|
jsonRequest('POST', '/api/admin/config/timezone/seed', { timezone: 'Asia/Tokyo' }),
|
||||||
|
);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const body = (await res.json()) as { ok: boolean; seeded: boolean };
|
||||||
|
expect(body.ok).toBe(true);
|
||||||
|
expect(body.seeded).toBe(false);
|
||||||
|
|
||||||
|
// D-03 preserved: the directly-inserted value is untouched.
|
||||||
|
const [row] = await db
|
||||||
|
.select({ value: appConfig.value })
|
||||||
|
.from(appConfig)
|
||||||
|
.where(eq(appConfig.key, 'household_timezone'))
|
||||||
|
.limit(1);
|
||||||
|
expect(row?.value).toBe('America/Denver');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -4,8 +4,10 @@
|
|||||||
* Verifies the admin Timezone section with the real 18-02 API endpoints.
|
* Verifies the admin Timezone section with the real 18-02 API endpoints.
|
||||||
* Runs on desktop profile only (admin UI is desktop-focused).
|
* Runs on desktop profile only (admin UI is desktop-focused).
|
||||||
*
|
*
|
||||||
* NOTE: the IANA picker input is type="text" with list="iana-zones" which gives
|
* NOTE: the IANA picker is a searchable combobox — a text input (role=combobox)
|
||||||
* it the ARIA combobox role (not textbox) in Chromium.
|
* that opens a role=listbox of role=option items on focus. Selecting a zone means
|
||||||
|
* focusing the input, typing to filter, then clicking the option (not selectOption).
|
||||||
|
* Option accessible names are the full IANA id (e.g. "America/Chicago").
|
||||||
*/
|
*/
|
||||||
import { test, expect } from '@playwright/test';
|
import { test, expect } from '@playwright/test';
|
||||||
|
|
||||||
@@ -22,15 +24,17 @@ test.describe('Admin Timezone section — 18-04 round-trip', () => {
|
|||||||
await expect(page.getByRole('region', { name: 'Timezone' })).toBeVisible();
|
await expect(page.getByRole('region', { name: 'Timezone' })).toBeVisible();
|
||||||
});
|
});
|
||||||
|
|
||||||
test('Timezone input (combobox) is visible and pre-filled', async ({ page }) => {
|
test('Timezone picker (combobox) is visible and pre-filled', async ({ page }) => {
|
||||||
// ARIA role for <input type="text" list="iana-zones"> is combobox
|
// The searchable text input exposes role=combobox
|
||||||
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
||||||
await expect(input).toBeVisible();
|
await expect(input).toBeVisible();
|
||||||
const val = await input.inputValue();
|
const val = await input.inputValue();
|
||||||
expect(val.length, 'Input should have a non-empty timezone').toBeGreaterThan(0);
|
expect(val.length, 'Picker should have a non-empty timezone').toBeGreaterThan(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('Save is enabled on first run when timezone is not yet explicit (WR-01)', async ({ page }) => {
|
test('Save is enabled on first run when timezone is not yet explicit (WR-01)', async ({
|
||||||
|
page,
|
||||||
|
}) => {
|
||||||
// On first run the GET returns isExplicitlySet:false with the detected zone
|
// On first run the GET returns isExplicitlySet:false with the detected zone
|
||||||
// pre-filled. Saving that value to make the choice explicit is a meaningful
|
// pre-filled. Saving that value to make the choice explicit is a meaningful
|
||||||
// action, so Save must be ENABLED even though the input matches the displayed
|
// action, so Save must be ENABLED even though the input matches the displayed
|
||||||
@@ -45,20 +49,41 @@ test.describe('Admin Timezone section — 18-04 round-trip', () => {
|
|||||||
await expect(saveBtn).toBeEnabled();
|
await expect(saveBtn).toBeEnabled();
|
||||||
});
|
});
|
||||||
|
|
||||||
test('Changing the input enables Save', async ({ page }) => {
|
test('Changing the selection enables Save', async ({ page }) => {
|
||||||
const tzSection = page.getByRole('region', { name: 'Timezone' });
|
const tzSection = page.getByRole('region', { name: 'Timezone' });
|
||||||
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
||||||
|
await input.click();
|
||||||
await input.fill('America/Chicago');
|
await input.fill('America/Chicago');
|
||||||
|
await page.getByRole('option', { name: 'America/Chicago' }).click();
|
||||||
|
await expect(input).toHaveValue('America/Chicago');
|
||||||
const saveBtn = tzSection.getByRole('button', { name: /Save/ });
|
const saveBtn = tzSection.getByRole('button', { name: /Save/ });
|
||||||
await expect(saveBtn).toBeEnabled();
|
await expect(saveBtn).toBeEnabled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('Typing filters the list (type-to-search)', async ({ page }) => {
|
||||||
|
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
||||||
|
const listbox = page.getByRole('listbox', { name: 'Timezones' });
|
||||||
|
|
||||||
|
// Focus opens the full list with no typing required.
|
||||||
|
await input.click();
|
||||||
|
await expect(listbox).toBeVisible();
|
||||||
|
await expect(listbox.getByRole('option').first()).toBeVisible();
|
||||||
|
|
||||||
|
// Human-friendly partial query (case-insensitive, underscores ignored) filters.
|
||||||
|
await input.fill('york');
|
||||||
|
await expect(page.getByRole('option', { name: 'America/New_York' })).toBeVisible();
|
||||||
|
await expect(page.getByRole('option', { name: 'Europe/Paris' })).toHaveCount(0);
|
||||||
|
});
|
||||||
|
|
||||||
test('Save persists timezone across reload', async ({ page }) => {
|
test('Save persists timezone across reload', async ({ page }) => {
|
||||||
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
const input = page.getByRole('combobox', { name: 'Household timezone' });
|
||||||
const tzSection = page.getByRole('region', { name: 'Timezone' });
|
const tzSection = page.getByRole('region', { name: 'Timezone' });
|
||||||
|
|
||||||
// Set to a known value
|
// Set to a known value via the searchable combobox
|
||||||
|
await input.click();
|
||||||
await input.fill('America/Chicago');
|
await input.fill('America/Chicago');
|
||||||
|
await page.getByRole('option', { name: 'America/Chicago' }).click();
|
||||||
|
await expect(input).toHaveValue('America/Chicago');
|
||||||
const saveBtn = tzSection.getByRole('button', { name: /^Save$/ });
|
const saveBtn = tzSection.getByRole('button', { name: /^Save$/ });
|
||||||
await expect(saveBtn).toBeEnabled();
|
await expect(saveBtn).toBeEnabled();
|
||||||
await saveBtn.click();
|
await saveBtn.click();
|
||||||
|
|||||||
@@ -64,6 +64,13 @@ export function AdminPage() {
|
|||||||
|
|
||||||
// Timezone picker state
|
// Timezone picker state
|
||||||
const [timezoneInput, setTimezoneInput] = useState<string | null>(null);
|
const [timezoneInput, setTimezoneInput] = useState<string | null>(null);
|
||||||
|
// Searchable combobox state: tzSearch is the live filter text while the list is
|
||||||
|
// open (null = closed, input shows the selected zone). tzActiveIndex tracks the
|
||||||
|
// keyboard-highlighted option.
|
||||||
|
const [tzOpen, setTzOpen] = useState(false);
|
||||||
|
const [tzSearch, setTzSearch] = useState<string | null>(null);
|
||||||
|
const [tzActiveIndex, setTzActiveIndex] = useState(0);
|
||||||
|
const tzBlurTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||||
|
|
||||||
// Members query
|
// Members query
|
||||||
const membersQuery = useQuery({
|
const membersQuery = useQuery({
|
||||||
@@ -127,12 +134,31 @@ export function AdminPage() {
|
|||||||
effectiveTimezoneInput === '' ||
|
effectiveTimezoneInput === '' ||
|
||||||
(isExplicit && effectiveTimezoneInput === storedTimezone);
|
(isExplicit && effectiveTimezoneInput === storedTimezone);
|
||||||
|
|
||||||
// IANA zones list for the datalist (Intl.supportedValuesOf may not be present in all runtimes)
|
// IANA zones list (Intl.supportedValuesOf may not be present in all runtimes)
|
||||||
const ianaZones: string[] =
|
const ianaZones: string[] =
|
||||||
typeof (Intl as { supportedValuesOf?: (key: string) => string[] }).supportedValuesOf === 'function'
|
typeof (Intl as { supportedValuesOf?: (key: string) => string[] }).supportedValuesOf ===
|
||||||
|
'function'
|
||||||
? (Intl as { supportedValuesOf: (key: string) => string[] }).supportedValuesOf('timeZone')
|
? (Intl as { supportedValuesOf: (key: string) => string[] }).supportedValuesOf('timeZone')
|
||||||
: [];
|
: [];
|
||||||
|
|
||||||
|
// Searchable combobox: filter zones by the live search text (case-insensitive,
|
||||||
|
// ignoring underscores so "new york" matches "America/New_York"). When the search
|
||||||
|
// is empty the full list shows — so tapping the field reveals every zone with no
|
||||||
|
// typing required.
|
||||||
|
const tzNorm = (s: string) => s.toLowerCase().replace(/_/g, ' ');
|
||||||
|
const tzQuery = tzOpen ? tzNorm(tzSearch ?? '') : '';
|
||||||
|
const filteredZones = tzQuery
|
||||||
|
? ianaZones.filter((tz) => tzNorm(tz).includes(tzQuery))
|
||||||
|
: ianaZones;
|
||||||
|
|
||||||
|
// Commit a zone selection from the list, then close.
|
||||||
|
function selectTimezone(tz: string) {
|
||||||
|
if (tzBlurTimer.current) clearTimeout(tzBlurTimer.current);
|
||||||
|
setTimezoneInput(tz);
|
||||||
|
setTzSearch(null);
|
||||||
|
setTzOpen(false);
|
||||||
|
}
|
||||||
|
|
||||||
// Save shared calendar mutation
|
// Save shared calendar mutation
|
||||||
const sharedCalMutation = useMutation({
|
const sharedCalMutation = useMutation({
|
||||||
mutationFn: (calId: number) => setSharedCalendar(calId),
|
mutationFn: (calId: number) => setSharedCalendar(calId),
|
||||||
@@ -382,15 +408,64 @@ export function AdminPage() {
|
|||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
{/* Searchable IANA picker */}
|
{/* IANA picker — searchable combobox. Focusing shows the full list
|
||||||
<div style={{ marginBottom: 'var(--space-3, 12px)' }}>
|
(no typing/erasing needed); typing filters it case-insensitively
|
||||||
|
(underscores ignored, so "new york" matches America/New_York). */}
|
||||||
|
<div style={{ position: 'relative', marginBottom: 'var(--space-3, 12px)' }}>
|
||||||
<input
|
<input
|
||||||
type="text"
|
type="text"
|
||||||
list="iana-zones"
|
role="combobox"
|
||||||
value={effectiveTimezoneInput}
|
|
||||||
onChange={(e) => setTimezoneInput(e.target.value)}
|
|
||||||
placeholder="e.g. America/Chicago"
|
|
||||||
aria-label="Household timezone"
|
aria-label="Household timezone"
|
||||||
|
aria-expanded={tzOpen}
|
||||||
|
aria-controls="tz-listbox"
|
||||||
|
aria-autocomplete="list"
|
||||||
|
aria-activedescendant={
|
||||||
|
tzOpen && filteredZones.length ? `tz-opt-${tzActiveIndex}` : undefined
|
||||||
|
}
|
||||||
|
autoComplete="off"
|
||||||
|
value={tzOpen ? (tzSearch ?? '') : effectiveTimezoneInput}
|
||||||
|
placeholder={
|
||||||
|
tzOpen ? effectiveTimezoneInput || 'Search timezones…' : 'Search timezones…'
|
||||||
|
}
|
||||||
|
onFocus={() => {
|
||||||
|
if (tzBlurTimer.current) clearTimeout(tzBlurTimer.current);
|
||||||
|
setTzOpen(true);
|
||||||
|
setTzSearch('');
|
||||||
|
setTzActiveIndex(0);
|
||||||
|
}}
|
||||||
|
onChange={(e) => {
|
||||||
|
setTzSearch(e.target.value);
|
||||||
|
setTzOpen(true);
|
||||||
|
setTzActiveIndex(0);
|
||||||
|
}}
|
||||||
|
onKeyDown={(e) => {
|
||||||
|
if (e.key === 'ArrowDown') {
|
||||||
|
e.preventDefault();
|
||||||
|
if (!tzOpen) {
|
||||||
|
setTzOpen(true);
|
||||||
|
setTzSearch('');
|
||||||
|
}
|
||||||
|
setTzActiveIndex((i) => Math.min(i + 1, filteredZones.length - 1));
|
||||||
|
} else if (e.key === 'ArrowUp') {
|
||||||
|
e.preventDefault();
|
||||||
|
setTzActiveIndex((i) => Math.max(i - 1, 0));
|
||||||
|
} else if (e.key === 'Enter') {
|
||||||
|
if (tzOpen && filteredZones[tzActiveIndex]) {
|
||||||
|
e.preventDefault();
|
||||||
|
selectTimezone(filteredZones[tzActiveIndex]);
|
||||||
|
}
|
||||||
|
} else if (e.key === 'Escape') {
|
||||||
|
setTzOpen(false);
|
||||||
|
setTzSearch(null);
|
||||||
|
}
|
||||||
|
}}
|
||||||
|
onBlur={() => {
|
||||||
|
// Delay so an option's onClick fires before the list unmounts.
|
||||||
|
tzBlurTimer.current = setTimeout(() => {
|
||||||
|
setTzOpen(false);
|
||||||
|
setTzSearch(null);
|
||||||
|
}, 120);
|
||||||
|
}}
|
||||||
style={{
|
style={{
|
||||||
width: '100%',
|
width: '100%',
|
||||||
boxSizing: 'border-box',
|
boxSizing: 'border-box',
|
||||||
@@ -404,11 +479,77 @@ export function AdminPage() {
|
|||||||
minHeight: '44px',
|
minHeight: '44px',
|
||||||
}}
|
}}
|
||||||
/>
|
/>
|
||||||
<datalist id="iana-zones">
|
{tzOpen && (
|
||||||
{ianaZones.map((tz) => (
|
<ul
|
||||||
<option key={tz} value={tz} />
|
id="tz-listbox"
|
||||||
))}
|
role="listbox"
|
||||||
</datalist>
|
aria-label="Timezones"
|
||||||
|
style={{
|
||||||
|
position: 'absolute',
|
||||||
|
zIndex: 10,
|
||||||
|
top: 'calc(100% + 4px)',
|
||||||
|
left: 0,
|
||||||
|
right: 0,
|
||||||
|
margin: 0,
|
||||||
|
padding: 'var(--space-1, 4px)',
|
||||||
|
listStyle: 'none',
|
||||||
|
maxHeight: '260px',
|
||||||
|
overflowY: 'auto',
|
||||||
|
background: 'var(--color-surface, #ffffff)',
|
||||||
|
border: '1px solid var(--color-border)',
|
||||||
|
borderRadius: 'var(--space-1, 4px)',
|
||||||
|
boxShadow: '0 6px 20px rgba(0,0,0,0.12)',
|
||||||
|
}}
|
||||||
|
>
|
||||||
|
{filteredZones.length === 0 && (
|
||||||
|
<li
|
||||||
|
style={{
|
||||||
|
padding: 'var(--space-2, 8px) var(--space-3, 12px)',
|
||||||
|
color: 'var(--color-text-muted)',
|
||||||
|
fontSize: 'var(--text-body-size, 15px)',
|
||||||
|
}}
|
||||||
|
>
|
||||||
|
No matching timezones
|
||||||
|
</li>
|
||||||
|
)}
|
||||||
|
{filteredZones.map((tz, i) => {
|
||||||
|
const active = i === tzActiveIndex;
|
||||||
|
return (
|
||||||
|
<li
|
||||||
|
key={tz}
|
||||||
|
id={`tz-opt-${i}`}
|
||||||
|
role="option"
|
||||||
|
aria-selected={tz === effectiveTimezoneInput}
|
||||||
|
ref={
|
||||||
|
active
|
||||||
|
? (el) => {
|
||||||
|
el?.scrollIntoView({ block: 'nearest' });
|
||||||
|
}
|
||||||
|
: undefined
|
||||||
|
}
|
||||||
|
onMouseDown={(e) => e.preventDefault()}
|
||||||
|
onMouseEnter={() => setTzActiveIndex(i)}
|
||||||
|
onClick={() => selectTimezone(tz)}
|
||||||
|
style={{
|
||||||
|
padding: 'var(--space-2, 8px) var(--space-3, 12px)',
|
||||||
|
fontSize: 'var(--text-body-size, 15px)',
|
||||||
|
fontFamily: 'var(--font-family-base)',
|
||||||
|
color: 'var(--color-text-primary)',
|
||||||
|
borderRadius: 'var(--space-1, 4px)',
|
||||||
|
cursor: 'pointer',
|
||||||
|
background: active ? 'var(--color-member-0, #4A90D9)' : 'transparent',
|
||||||
|
...(active ? { color: '#ffffff' } : null),
|
||||||
|
minHeight: '44px',
|
||||||
|
display: 'flex',
|
||||||
|
alignItems: 'center',
|
||||||
|
}}
|
||||||
|
>
|
||||||
|
{tz}
|
||||||
|
</li>
|
||||||
|
);
|
||||||
|
})}
|
||||||
|
</ul>
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* Use detected zone affordance (D-02) */}
|
{/* Use detected zone affordance (D-02) */}
|
||||||
|
|||||||
@@ -44,6 +44,10 @@ export default defineConfig({
|
|||||||
}),
|
}),
|
||||||
],
|
],
|
||||||
server: {
|
server: {
|
||||||
|
// Allow the internal split-DNS domain (and any subdomain) to reach the dev
|
||||||
|
// server through the reverse proxy / tunnel. A leading dot matches the apex
|
||||||
|
// and all subdomains. Dev-server only — production builds ignore this.
|
||||||
|
allowedHosts: ['.bergerhouse.net'],
|
||||||
proxy: {
|
proxy: {
|
||||||
'/health': 'http://localhost:3000',
|
'/health': 'http://localhost:3000',
|
||||||
'/api': 'http://localhost:3000',
|
'/api': 'http://localhost:3000',
|
||||||
|
|||||||
Reference in New Issue
Block a user