Lucas BergerandClaude Opus 4.8
c7955a46b9
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
93 lines
4.4 KiB
Markdown
93 lines
4.4 KiB
Markdown
---
|
|
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_
|