familysync/.planning/milestones/v1.1-phases/18-auto-timezone-detection-and-ability-to-change-timezone/18-REVIEW-FIX.md

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
18-auto-timezone-detection-and-ability-to-change-timezone	2026-06-15T07:43:30Z	.planning/phases/18-auto-timezone-detection-and-ability-to-change-timezone/18-REVIEW.md	1	5	5	0	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