docs(03): revise gap plans 03-10/11/12 per checker feedback
This commit is contained in:
Lucas Berger
@@ -16,7 +16,7 @@ must_haves:
|
||||
truths:
|
||||
- "The worker parses the stored form JSON and PUTs a real VCALENDAR string built by buildVeventString — never raw {\"title\":...} JSON"
|
||||
- "The PUT body begins with 'BEGIN:VCALENDAR' for both create and update operations"
|
||||
- "A single-day all-day event produces DTEND = DTSTART + 1 day (RFC 5545 exclusive end)"
|
||||
- "A single-day all-day event produces DTEND = DTSTART + 1 day (RFC 5545 exclusive end), proven by a DIRECT buildVeventString unit test against the D-13 contract"
|
||||
- "A credential-load failure leaves the row pending for retry — the worker never PUTs with empty Basic-auth"
|
||||
- "The first transient failure waits 15s (BACKOFF_SECONDS[0]), not 60s"
|
||||
artifacts:
|
||||
@@ -24,7 +24,7 @@ must_haves:
|
||||
provides: "ICS-building dispatch path + removed empty-cred fallback + corrected backoff index + explicit randomUUID import"
|
||||
contains: "buildVeventString"
|
||||
- path: apps/api/src/broker/vevent.ts
|
||||
provides: "All-day DTEND+1-day exclusivity fix"
|
||||
provides: "All-day DTEND+1-day exclusivity fix (the WR-04 owning boundary)"
|
||||
key_links:
|
||||
- from: "apps/api/src/broker/outboxWorker.ts"
|
||||
to: "apps/api/src/broker/vevent.ts"
|
||||
@@ -42,7 +42,9 @@ raw form JSON (`{"title":...}`) to Fastmail — `buildVeventString` (the whole D
|
||||
DATE/DATETIME contract) is dead code (CR-02). It also silently authenticates with
|
||||
empty credentials on any credential-load error (CR-03), skips its first backoff
|
||||
delay (WR-01), and mishandles the all-day exclusive DTEND (WR-04). This plan wires
|
||||
the VEVENT builder into the dispatch path and fixes those correctness defects.
|
||||
the VEVENT builder into the dispatch path, adds a direct unit test that pins the
|
||||
D-13 DATE-vs-DATETIME / RFC-5545 contract independent of the worker, and fixes
|
||||
those correctness defects.
|
||||
|
||||
Purpose: a queued write becomes a real, RFC-5545-valid VEVENT on the correct calendar.
|
||||
Output: a worker that builds ICS from the stored form fields and fails closed on bad credentials.
|
||||
@@ -72,20 +74,24 @@ an internal `JSON.parse(row.payload)` → `buildVeventString` step.
|
||||
<tasks>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 1: RED+GREEN — worker builds and PUTs a real VEVENT (CR-02, WR-04)</name>
|
||||
<name>Task 1: RED+GREEN — worker builds and PUTs a real VEVENT (CR-02) + direct D-13 contract unit test + all-day DTEND+1 (WR-04)</name>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/src/broker/vevent.ts, apps/api/tests/broker/outboxWorker.test.ts, apps/api/tests/broker/vevent.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (dispatchRow lines 131-188 — the create/update branches that pass row.payload straight through)
|
||||
- apps/api/src/broker/vevent.ts (buildVeventString signature lines 52-118; NewEventParams lines 21-33; RRULE_PRESETS lines 39-44; all-day DATE handling lines 69-88)
|
||||
- apps/api/src/broker/vevent.ts (buildVeventString signature lines 52-118; NewEventParams lines 21-33; RRULE_PRESETS lines 39-44; all-day DATE handling lines 69-88 — WR-04 lives in THIS branch)
|
||||
- apps/api/tests/broker/vevent.test.ts (existing direct unit tests: note the all-day test at lines 53-68 asserts DTSTART format but NOT DTEND+1 — the new contract block extends this)
|
||||
- apps/api/src/routes/events.ts (the route stores payload: JSON.stringify(payload) with the new title/start/end fields from plan 03-09)
|
||||
- apps/api/tests/broker/outboxWorker.test.ts (line ~85 hardcodes payload:'BEGIN:VCALENDAR' and mocks write.js — the wrong boundary; the new test must stop mocking the ICS string and assert the worker BUILDS it)
|
||||
- apps/api/tests/broker/outboxWorker.test.ts (line ~85 hardcodes payload:'BEGIN:VCALENDAR' and mocks write.js — the wrong boundary; the new worker test must stop mocking the ICS string and assert the worker BUILDS it; match the existing vi.hoisted DB-mock + makeRow + makeResponse patterns)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (CR-02, WR-04)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED (worker): with `write.js` NOT mocking away the payload — i.e. spy on `createCalendarEvent` and capture its 4th arg `icsString` — enqueue a create row whose `payload` is `JSON.stringify({title:'Lunch',allDay:false,start:'2026-06-10T12:00:00',end:'2026-06-10T13:00:00',recurrence:'none'})`. Assert the captured icsString `.startsWith('BEGIN:VCALENDAR')` and contains `SUMMARY:Lunch`. Fails today (raw JSON is passed).
|
||||
- RED (vevent UNIT — D-13 contract, the authoritative regression guard): in tests/broker/vevent.test.ts add a `describe('buildVeventString — D-13 form-parsed contract')` block that calls buildVeventString DIRECTLY (no worker in the loop) with the SAME field shape the worker parses from form JSON. Two cases:
|
||||
• timed: `{uid:'u1@familysync', summary:'Lunch', allDay:false, dtstart:new Date('2026-06-10T12:00:00Z'), dtend:new Date('2026-06-10T13:00:00Z')}` (recurrence omitted) → assert icsString contains `BEGIN:VCALENDAR`, `SUMMARY:Lunch`, `UID:u1@familysync`, a DTSTART line WITH a time component (matches `/DTSTART:\d{8}T\d{6}Z/`), and a DTEND line present (matches `/DTEND:\d{8}T\d{6}Z/`).
|
||||
• all-day single-day: `{summary:'Birthday', allDay:true, dtstart:'2026-06-10', dtend:'2026-06-10'}` → assert DTSTART is DATE format (matches `/DTSTART[^:]*:20260610/` and does NOT match `/DTSTART[^:]*:20260610T/` — no time), and DTEND = DTSTART + 1 day (matches `/DTEND[^:]*:20260611/`, RFC-5545 exclusive end), and the DTEND date string is NOT equal to the DTSTART date string.
|
||||
This unit test is the regression the worker integration test cannot catch: a vevent.ts regression would still pass the worker spy if both used the same broken builder. Fails today — the current all-day branch emits DTEND == DTSTART (no +1), so the `20260611` assertion fails.
|
||||
- RED (worker INTEGRATION — wiring, complementary to the unit test): with `write.js` NOT mocking away the payload — i.e. spy on `createCalendarEvent` and capture its 4th arg `icsString` — enqueue a create row whose `payload` is `JSON.stringify({title:'Lunch',allDay:false,start:'2026-06-10T12:00:00',end:'2026-06-10T13:00:00',recurrence:'none'})`. Assert the captured icsString `.startsWith('BEGIN:VCALENDAR')` and contains `SUMMARY:Lunch`. Fails today (raw JSON is passed).
|
||||
- RED (worker): an update row likewise yields an icsString starting with `BEGIN:VCALENDAR` passed to `updateCalendarEvent`.
|
||||
- RED (worker): a row whose `payload` is not valid JSON marks the row `failed` (hard fail, no retry).
|
||||
- RED (vevent): a single-day all-day event `{allDay:true, start:'2026-06-10', end:'2026-06-10'}` produces an icsString whose DTEND is `2026-06-11` (start + 1 day), not equal to DTSTART.
|
||||
</behavior>
|
||||
<action>
|
||||
In `dispatchRow`, for `operation === 'create'` and `operation === 'update'`:
|
||||
@@ -102,10 +108,15 @@ an internal `JSON.parse(row.payload)` → `buildVeventString` step.
|
||||
and to `updateCalendarEvent(client, row.calendarObjectUrl, icsString, row.etag ?? null)`.
|
||||
Import `{ buildVeventString, RRULE_PRESETS }` from `./vevent.js`. Delete operations are unchanged (no payload).
|
||||
|
||||
WR-04 in vevent.ts: in the all-day branch (lines 69-88), after computing the end DATE
|
||||
components, advance the end DATE by one calendar day before constructing `endTime`
|
||||
(use a Date built from ey/em/ed, `setUTCDate(getUTCDate()+1)`, re-read y/m/d) so a one-day
|
||||
all-day event serializes DTEND = DTSTART + 1 (RFC 5545 exclusive). Keep the timed branch untouched.
|
||||
WR-04 — ONE owning boundary: the all-day DTEND+1 exclusivity fix lives in vevent.ts ONLY,
|
||||
NOT in form/route validation. Rationale: vevent.ts is the single serialization point for every
|
||||
write path, so fixing it there covers all callers; the form/route should keep passing the
|
||||
user-entered inclusive end date unchanged. In the all-day branch (vevent.ts lines 69-88), after
|
||||
parsing the end DATE components (ey/em/ed), advance the end DATE by one calendar day before
|
||||
constructing `endTime`: build a Date from ey/em/ed, `setUTCDate(getUTCDate()+1)`, re-read the
|
||||
rolled-over y/m/d, and use those for `endTime`. A one-day all-day event then serializes
|
||||
DTEND = DTSTART + 1. Keep the timed branch untouched. The acceptance test for WR-04 is the
|
||||
DIRECT vevent unit-test case above (the owning boundary), not the worker integration path.
|
||||
|
||||
Update the existing outbox test that fed a pre-built ICS string so it instead feeds
|
||||
form JSON and asserts the built ICS (it was testing the wrong boundary). Commit RED then GREEN.
|
||||
@@ -114,21 +125,23 @@ an internal `JSON.parse(row.payload)` → `buildVeventString` step.
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts tests/broker/vevent.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- behavior: the icsString passed to createCalendarEvent starts with 'BEGIN:VCALENDAR' and contains the summary.
|
||||
- behavior (unit, owning boundary): a DIRECT buildVeventString call on a single-day all-day event yields DTEND = start + 1 day (`20260611`) and DTEND != DTSTART.
|
||||
- behavior (unit): a DIRECT buildVeventString call on a timed form-shaped event yields icsString containing BEGIN:VCALENDAR, SUMMARY:, UID:, a timed DTSTART (`/DTSTART:\d{8}T\d{6}Z/`), and a DTEND line.
|
||||
- behavior (integration): the icsString passed to createCalendarEvent starts with 'BEGIN:VCALENDAR' and contains the summary.
|
||||
- behavior: an unparseable payload marks the row failed with no retry.
|
||||
- behavior: single-day all-day event yields DTEND = start + 1 day.
|
||||
- source: `grep -c 'buildVeventString' apps/api/src/broker/outboxWorker.ts` returns >= 1.
|
||||
- source: `grep -c 'D-13 form-parsed contract' apps/api/tests/broker/vevent.test.ts` returns 1 (the new direct unit-test block exists).
|
||||
- test-command: `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts tests/broker/vevent.test.ts` passes.
|
||||
</acceptance_criteria>
|
||||
<done>Every create/update PUTs a real RFC-5545 VCALENDAR built from the stored form fields, honoring D-13 DATE-vs-DATETIME and the exclusive all-day DTEND.</done>
|
||||
<done>Every create/update PUTs a real RFC-5545 VCALENDAR built from the stored form fields; the D-13 DATE-vs-DATETIME contract and the exclusive all-day DTEND are pinned by a direct buildVeventString unit test that a worker-only test could not catch.</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 2: RED+GREEN — fail closed on bad credentials, fix backoff index, explicit randomUUID (CR-03, WR-01, WR-08)</name>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/tests/broker/outboxWorker.test.ts</files>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/src/routes/events.ts, apps/api/tests/broker/outboxWorker.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (dispatchRow try/catch fallback lines 135-143; backoff math lines 316-341; BACKOFF_SECONDS lines 40-44)
|
||||
- apps/api/src/routes/events.ts (uses bare `crypto.randomUUID()` at lines 241,317,318 — WR-08 is the route-side instance; vevent.ts already imports from 'crypto')
|
||||
- apps/api/src/routes/events.ts (uses bare `crypto.randomUUID()` at line 241 and the edit/move handlers — WR-08 is the route-side instance; vevent.ts already imports `randomUUID` from 'crypto')
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (CR-03, WR-01, WR-08)
|
||||
</read_first>
|
||||
<behavior>
|
||||
@@ -147,39 +160,47 @@ an internal `JSON.parse(row.payload)` → `buildVeventString` step.
|
||||
`const backoffMs = (BACKOFF_SECONDS[row.attemptCount] ?? 1800) * 1000`. Keep `nextAttemptCount = row.attemptCount + 1`
|
||||
for the persisted `attemptCount` and the `>= MAX_ATTEMPTS` dead-letter check. This makes the first retry wait 15s.
|
||||
|
||||
WR-08: in events.ts replace the three bare `crypto.randomUUID()` calls (lines 241,317,318) with `randomUUID()`
|
||||
imported via `import { randomUUID } from 'node:crypto'`, matching vevent.ts. (events.ts is also edited by plan 03-09;
|
||||
this plan runs after it in a later wave so there is no concurrent edit.)
|
||||
WR-08: in events.ts replace every bare `crypto.randomUUID()` call (the create handler at line ~241 plus the
|
||||
edit/move handlers) with `randomUUID()` imported via `import { randomUUID } from 'node:crypto'`, matching
|
||||
vevent.ts. Confirm with grep that no bare `crypto.randomUUID(` remains. (events.ts is also edited by plan 03-09;
|
||||
this plan runs in a later wave so there is no concurrent edit.) Because this task edits events.ts but its vitest
|
||||
command only runs broker tests, the route edit is proven to COMPILE via the `npm run build` (tsc) assertion in
|
||||
this plan's <verification> and the acceptance criterion below — this closes Warning 5.
|
||||
|
||||
Commit RED then GREEN.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts</automated>
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts && cd apps/api && npm run build</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- behavior: a credential-load failure leaves the row pending and never calls createFastmailClient('', '').
|
||||
- behavior: first transient retry delay equals BACKOFF_SECONDS[0] (15s).
|
||||
- source: `grep -c "createFastmailClient('', '')" apps/api/src/broker/outboxWorker.ts` returns 0.
|
||||
- source: `grep -c "import { randomUUID } from 'node:crypto'" apps/api/src/routes/events.ts` returns 1.
|
||||
- source: `grep -c 'crypto.randomUUID(' apps/api/src/routes/events.ts` returns 0 (no bare calls remain).
|
||||
- test-command: `cd apps/api && npm run build` (tsc) succeeds — proves the edited events.ts route compiles (Warning 5 closed).
|
||||
- test-command: `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` passes.
|
||||
</acceptance_criteria>
|
||||
<done>The worker fails closed on credential errors (retryable), uses the documented 15s-first backoff schedule, and uses an explicitly-imported randomUUID.</done>
|
||||
<done>The worker fails closed on credential errors (retryable), uses the documented 15s-first backoff schedule, and uses an explicitly-imported randomUUID; the edited route is proven to compile via tsc.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
- `cd apps/api && npx vitest run tests/broker/` green.
|
||||
- `cd apps/api && npm run build` succeeds.
|
||||
- `cd apps/api && npm run build` succeeds (also proves the WR-08 events.ts edit compiles — Warning 5).
|
||||
- `grep -rn buildVeventString apps/api/src` shows a live call site outside vevent.ts (IN-01 closed).
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
Queued writes serialize to valid VCALENDAR via buildVeventString, all-day DTEND is exclusive,
|
||||
credential failures retry instead of writing with empty auth, and the backoff schedule matches its docs.
|
||||
Queued writes serialize to valid VCALENDAR via buildVeventString, the D-13 DATE-vs-DATETIME contract and
|
||||
exclusive all-day DTEND are pinned by a direct unit test, credential failures retry instead of writing with
|
||||
empty auth, the backoff schedule matches its docs, and the edited route compiles.
|
||||
CR-02, CR-03, WR-01, WR-04, WR-08, IN-01 closed.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/03-event-write-back-pwa-install/03-10-SUMMARY.md` when done.
|
||||
</output>
|
||||
</content>
|
||||
</invoke>
|
||||
|
||||
@@ -17,7 +17,7 @@ must_haves:
|
||||
- "A same-calendar update re-reads the freshest etag from calendarEvents just before PUT, so rapid successive edits do not spuriously 412"
|
||||
artifacts:
|
||||
- path: apps/api/src/broker/outboxWorker.ts
|
||||
provides: "Durable create-before-delete gating, drain concurrency guard, fresh-etag-before-PUT"
|
||||
provides: "Durable create-before-delete gating, drain concurrency guard (single-process), fresh-etag-before-PUT"
|
||||
contains: "isDraining"
|
||||
key_links:
|
||||
- from: "runOutboxDrain"
|
||||
@@ -67,29 +67,42 @@ gated by querying its sibling create's status, not enqueued as a new enum value)
|
||||
<name>Task 1: RED+GREEN — durable create-before-delete + drain concurrency guard (CR-04, CR-05)</name>
|
||||
<files>apps/api/src/broker/outboxWorker.ts, apps/api/tests/broker/outboxWorker.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (failedCreateGroups Set lines 271-296; per-batch sort lines 263-269; startOutboxWorker schedule lines 359-365; runOutboxDrain entry line 247)
|
||||
- apps/api/src/broker/outboxWorker.ts (failedCreateGroups Set lines 271-296; per-batch sort lines 263-269; the pending-rows select at lines 249-257; startOutboxWorker schedule lines 359-365; runOutboxDrain entry line 247)
|
||||
- apps/api/src/db/schema.ts (calendarOutbox: status enum pending|done|failed|dead, groupId, lines 125-154)
|
||||
- apps/api/tests/broker/outboxWorker.test.ts (the vi.hoisted DB mock: `mockSelectFn → mockFromFn → mockWherePending`; today EVERY `db.select().from().where()` resolves to the single `mockPendingRows` array. To return DIFFERENT results for the pending-rows select vs the sibling-status select, give `mockWherePending` a per-call implementation via `.mockImplementationOnce(...)` queued in call order, OR branch on the `where(...)` condition arg. Match the existing `beforeEach` chain-restore style at lines 99-109.)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (CR-04, CR-05)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED (CR-04 cross-batch): simulate a move pair where the create row and the delete row are returned in SEPARATE drain calls. Drain 1 returns only the delete row (create not yet done). Assert the delete is NOT dispatched (deleteCalendarEvent not called) because the paired create (queried by groupId) is not `done`. Drain 2, after the create has reached `done`, dispatches the delete. Fails today (the in-memory Set is empty in drain 1, so the delete proceeds).
|
||||
- RED (CR-05): invoke `runOutboxDrain` twice concurrently (do not await the first before starting the second) against the same pending row; assert the dispatch wrapper (createCalendarEvent) is invoked exactly once across both calls.
|
||||
- RED (CR-04 cross-batch) — CONCRETE setup, two separate `await runOutboxDrain()` calls:
|
||||
Build a move pair sharing `groupId='edit-move-group-001'`: a create row (id 3, operation 'create', status 'pending') and a delete row (id 2, operation 'delete', calendarObjectUrl set, etag set, payload null).
|
||||
DRAIN 1: mock the pending-rows select to return ONLY the delete row (the create is not yet due/returned). Mock the sibling-status select (the query for `groupId='edit-move-group-001' AND operation='create'`) to return `[{ status: 'pending' }]`. Assert after drain 1: `deleteCalendarEvent` was NOT called, and the delete row's status update was NOT set to 'done'/'failed' (it is left pending for a later cycle). This FAILS today: the in-memory `failedCreateGroups` Set is empty in this batch, so the delete proceeds and `deleteCalendarEvent` IS called.
|
||||
DRAIN 2: now mock the pending-rows select to return the delete row again, and mock the sibling-status select to return `[{ status: 'done' }]` (the create succeeded in a prior cycle). Assert after drain 2: `deleteCalendarEvent` WAS called exactly once. State each assertion explicitly so the test cannot pass trivially: drain-1 asserts `expect(deleteCalendarEvent).not.toHaveBeenCalled()`; drain-2 asserts `expect(deleteCalendarEvent).toHaveBeenCalledTimes(1)`.
|
||||
- RED (CR-04 paired-create-failed): with the same pair, mock the sibling-status select to return `[{ status: 'failed' }]`; assert `deleteCalendarEvent` is NOT called and the delete row is marked `failed` with a lastError mentioning the paired create (original event preserved per D-04).
|
||||
- RED (CR-05): invoke `runOutboxDrain` twice concurrently (start the second WITHOUT awaiting the first) against the same single pending create row; assert `createCalendarEvent` is invoked exactly once across both calls (`expect(createCalendarEvent).toHaveBeenCalledTimes(1)`).
|
||||
</behavior>
|
||||
<action>
|
||||
CR-04 — make the ordering durable. For a `delete` row that has a `groupId`, before dispatching,
|
||||
query calendarOutbox for the sibling row with the same `groupId` and `operation='create'`:
|
||||
- if that sibling is not yet `done`, SKIP this delete this cycle (leave it `pending` for a later
|
||||
drain) — do not rely on `failedCreateGroups` co-occurring in the batch;
|
||||
- if the sibling create is `failed`/`dead`, skip the delete permanently per D-04 (mark it `failed`
|
||||
with a "paired create did not succeed" lastError so the original event is preserved).
|
||||
CR-04 — make the ordering durable. For a `delete` row that has a `groupId`, BEFORE dispatching,
|
||||
query calendarOutbox for the sibling row with the same `groupId` and `operation='create'`
|
||||
(a `db.select(...).from(calendarOutbox).where(and(eq(groupId, row.groupId), eq(operation,'create')))`):
|
||||
- if that sibling create is not yet `done` (e.g. still `pending`), SKIP this delete this cycle —
|
||||
leave the delete row `pending` (do not update its status) so a later drain re-evaluates it.
|
||||
Do NOT rely on `failedCreateGroups` co-occurring in the batch.
|
||||
- if the sibling create is `failed` or `dead`, skip the delete PERMANENTLY per D-04: mark the
|
||||
delete row `failed` with lastError `'paired create did not succeed — original preserved'` so the
|
||||
original event is not lost.
|
||||
- if the sibling create is `done`, dispatch the delete normally.
|
||||
Keep the within-batch create-before-delete sort as a fast path, but the DB sibling-status query is
|
||||
the authoritative gate. Remove reliance on `failedCreateGroups` as the sole cross-cycle mechanism.
|
||||
|
||||
CR-05 — add a module-level `let isDraining = false`. At the top of `runOutboxDrain`, if `isDraining`
|
||||
return immediately; else set `isDraining = true` and wrap the whole drain body in try/finally that
|
||||
resets `isDraining = false`. The 15s scheduler in `startOutboxWorker` already calls runOutboxDrain;
|
||||
the guard makes an overlapping invocation a no-op. (Document that a more robust DB row-claim is a
|
||||
future option, but the in-process guard is sufficient for the single-worker two-user deployment.)
|
||||
is true return immediately; else set `isDraining = true` and wrap the whole drain body in a
|
||||
`try { ... } finally { isDraining = false }`. The 15s scheduler in `startOutboxWorker` already calls
|
||||
runOutboxDrain; the guard makes an overlapping invocation a no-op.
|
||||
Add an EXPLICIT code comment next to the guard (and restate in <done>) that this in-process guard is
|
||||
valid ONLY for the single-process Unraid deployment of this two-user app; a multi-process or
|
||||
multi-replica deployment would require a DB row-claim (e.g. `UPDATE ... SET status='processing'
|
||||
WHERE id=? AND status='pending'` with affected-rows check) instead. Document the limitation; do not
|
||||
silently rely on it.
|
||||
|
||||
Commit RED then GREEN.
|
||||
</action>
|
||||
@@ -97,13 +110,14 @@ gated by querying its sibling create's status, not enqueued as a new enum value)
|
||||
<automated>cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- behavior: a delete whose paired create is not `done` is not dispatched, even when they arrive in different drain cycles.
|
||||
- behavior: a paired create that ends failed/dead causes the delete to be skipped (original event preserved).
|
||||
- behavior: two overlapping runOutboxDrain calls dispatch each row exactly once.
|
||||
- behavior: drain 1 (sibling create 'pending') leaves the delete pending and does NOT call deleteCalendarEvent; drain 2 (sibling create 'done') calls deleteCalendarEvent exactly once.
|
||||
- behavior: a paired create that is 'failed'/'dead' causes the delete to be marked failed and never dispatched (original event preserved).
|
||||
- behavior: two overlapping runOutboxDrain calls invoke createCalendarEvent exactly once.
|
||||
- source: `grep -c 'isDraining' apps/api/src/broker/outboxWorker.ts` returns >= 2.
|
||||
- source: `grep -c 'single-process' apps/api/src/broker/outboxWorker.ts` returns >= 1 (the documented-limitation comment exists).
|
||||
- test-command: `cd apps/api && npx vitest run tests/broker/outboxWorker.test.ts` passes.
|
||||
</acceptance_criteria>
|
||||
<done>The create-before-delete invariant holds across drain cycles and overlapping cycles never double-apply a row.</done>
|
||||
<done>The create-before-delete invariant holds across drain cycles (proven by a two-drain sibling-status test) and overlapping cycles never double-apply a row. The isDraining guard carries an explicit comment that it is single-process-only and that multi-process needs a DB row-claim.</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
@@ -112,10 +126,12 @@ gated by querying its sibling create's status, not enqueued as a new enum value)
|
||||
<read_first>
|
||||
- apps/api/src/broker/outboxWorker.ts (update dispatch lines 158-173; the etag comes from row.etag captured at enqueue time)
|
||||
- apps/api/src/db/schema.ts (calendarEvents.etag line 95; calendarEvents.uid line 94)
|
||||
- apps/api/tests/broker/outboxWorker.test.ts (to make the calendarEvents etag select return 'new-etag' while the pending-rows select returns the update row, use the same per-call `mockImplementationOnce` / where-condition-branch technique introduced in Task 1)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (WR-02)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED: an update row carries a stale `etag` ('old-etag'), but calendarEvents has been re-synced to 'new-etag'. Assert updateCalendarEvent is called with 'new-etag' (the freshest value read from calendarEvents at dispatch time), not the row's stale 'old-etag'. Fails today (row.etag is used verbatim).
|
||||
- RED: an update row carries a stale `etag` ('old-etag'), but calendarEvents has been re-synced to 'new-etag'. Mock the calendarEvents etag select to return `[{ etag: 'new-etag' }]`. Assert updateCalendarEvent is called with 'new-etag' (the freshest value read from calendarEvents at dispatch time), not the row's stale 'old-etag'. Fails today (row.etag is used verbatim).
|
||||
- RED: when the calendarEvents select returns `[]` for the uid, assert updateCalendarEvent falls back to `row.etag`.
|
||||
</behavior>
|
||||
<action>
|
||||
In the `operation === 'update'` branch of `dispatchRow`, before calling `updateCalendarEvent`,
|
||||
@@ -149,10 +165,12 @@ gated by querying its sibling create's status, not enqueued as a new enum value)
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
The outbox is durable (create-before-delete across cycles), non-duplicating (concurrency guard),
|
||||
and avoids spurious conflicts (fresh-etag). CR-04, CR-05, WR-02 closed.
|
||||
The outbox is durable (create-before-delete across cycles, proven by a two-drain sibling-status test),
|
||||
non-duplicating (concurrency guard, documented single-process-only), and avoids spurious conflicts
|
||||
(fresh-etag). CR-04, CR-05, WR-02 closed.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/03-event-write-back-pwa-install/03-11-SUMMARY.md` when done.
|
||||
</output>
|
||||
</content>
|
||||
|
||||
@@ -10,11 +10,12 @@ requirements: [CAL-05, CAL-07, PWA-01, PWA-02]
|
||||
files_modified:
|
||||
- apps/pwa/src/components/EventForm.tsx
|
||||
- apps/pwa/src/components/EventForm.test.tsx
|
||||
- apps/pwa/src/store/calendarStore.ts
|
||||
must_haves:
|
||||
truths:
|
||||
- "Opening the form in edit mode populates Title/Start/End from the cached occurrence even when the form opens before the occurrence is resolved (no blank edit form)"
|
||||
- "Editing a recurring event preselects its existing recurrence preset instead of resetting to 'none'"
|
||||
- "The edit form shows the event's original date/time consistently (no UTC-date / local-time mismatch that shifts the day)"
|
||||
- "The edit form shows the event's original date/time consistently (no UTC-date / local-time mismatch that shifts the day), proven by a test that pins TZ so it cannot pass by coincidence on an EDT runner"
|
||||
- "Tab and Shift+Tab cycle focus within the open dialog and never reach background controls"
|
||||
- "The PWA install assets (icon-192/512, apple-touch-icon) exist so Add-to-Home-Screen installs with a real icon (PWA-01/PWA-02)"
|
||||
artifacts:
|
||||
@@ -56,47 +57,50 @@ Output: an EventForm that round-trips an existing event's fields and traps focus
|
||||
</context>
|
||||
|
||||
<artifacts_this_phase_produces>
|
||||
No new exported symbols. Internal changes to EventForm: reset effect deps gain
|
||||
`occurrence`, a recurrence-deriving initializer, a zone-consistent `parseDateTime`,
|
||||
and a real Tab/Shift+Tab focus-cycle handler. IN-03 (collapse the duplicate
|
||||
getDefaultStartDate/getDefaultEndDate to one helper) is folded in as a cheap adjacent cleanup.
|
||||
No new exported symbols beyond exporting the existing `todayIso` from calendarStore.ts
|
||||
(see IN-03 below — it is currently a private module function, NOT yet exported). Internal
|
||||
changes to EventForm: reset effect deps gain `occurrence`, a recurrence-deriving initializer,
|
||||
a zone-consistent `parseDateTime`, and a real Tab/Shift+Tab focus-cycle handler.
|
||||
</artifacts_this_phase_produces>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 1: RED+GREEN — edit-mode population, recurrence derivation, zone-consistent dates (WR-03, WR-05, IN-03)</name>
|
||||
<files>apps/pwa/src/components/EventForm.tsx, apps/pwa/src/components/EventForm.test.tsx</files>
|
||||
<files>apps/pwa/src/components/EventForm.tsx, apps/pwa/src/components/EventForm.test.tsx, apps/pwa/src/store/calendarStore.ts</files>
|
||||
<read_first>
|
||||
- apps/pwa/src/components/EventForm.tsx (occurrence IIFE lines 113-124; reset effect lines 167-181 with deps [eventFormOpen,eventFormMode,eventFormUid]; parseDateTime lines 84-101; getDefaultStartDate/EndDate lines 47-53)
|
||||
- apps/pwa/src/components/EventForm.tsx (occurrence IIFE lines 113-124; reset effect deps `[eventFormOpen,eventFormMode,eventFormUid]`; parseDateTime lines 84-101 — note it mixes `d.toISOString().slice(0,10)` (UTC date) with `d.getHours()/getMinutes()` (local time): THIS is the WR-05 bug; getDefaultStartDate/getDefaultEndDate lines 47-53)
|
||||
- apps/pwa/src/api/client.ts (CalendarOccurrence.start/end format note lines 69-73: 'YYYY-MM-DD' for allDay, ISO 8601 with IANA tz for timed)
|
||||
- apps/pwa/src/store/calendarStore.ts (setEventForm/eventFormMode/eventFormUid lines 43-44,162-163; a todayIso helper exists per IN-03)
|
||||
- apps/pwa/src/store/calendarStore.ts (todayIso at lines 121-124 is a PRIVATE module function — it is NOT currently exported; IN-03 requires adding `export` to it before EventForm can import it)
|
||||
- .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md (WR-03, WR-05, IN-03)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- RED (WR-03 blank): render EventForm in edit mode where the occurrence becomes available in the ['events'] cache AFTER the form opens; assert the Title input value equals the occurrence title (not empty). Fails today because the reset effect deps exclude `occurrence`.
|
||||
- RED (WR-03 recurrence): edit an occurrence whose recurrence is 'weekly'; assert the Repeat select value is 'weekly', not 'none'.
|
||||
- RED (WR-05 zone): a timed occurrence start of '2026-06-10T23:30:00-04:00[America/Toronto]' renders Start date '2026-06-10' and time '23:30' (the event's own wall time), not a UTC-shifted '2026-06-11'/'03:30'.
|
||||
- RED (WR-05 zone — DETERMINISTIC, TZ-pinned so it cannot pass by coincidence): pin the test runner timezone to UTC for this test file. Use the top-of-file `// @vitest-environment jsdom` already in place, and add `process.env.TZ = 'UTC'` in a `beforeAll` (set BEFORE any Date is constructed in the test) — OR, preferred, add `env: { TZ: 'UTC' }` to the pwa vitest config's `test` block so the runner zone is fixed for the whole suite. State which approach you used in a comment. With TZ pinned to UTC, feed a timed occurrence start of `'2026-06-10T23:30:00-04:00'` (i.e. UTC instant `2026-06-11T03:30:00Z`) and assert the rendered Start date and time equal the event's OWN wall-clock as derived by the fixed extraction rule (see <action>): the test must assert the exact strings the corrected `parseDateTime` produces for that input under TZ=UTC, and document why those values are correct regardless of the developer's machine zone. The point: the assertion is stable on a UTC CI runner AND would fail loudly if `parseDateTime` reverted to the toISOString/getHours mismatch.
|
||||
</behavior>
|
||||
<action>
|
||||
WR-03: add `occurrence` (or `occurrence?.uid` plus `occurrence?.start`) to the reset effect dependency
|
||||
array so the form re-initializes when the occurrence resolves after open. In the reset effect, derive
|
||||
the initial recurrence from the occurrence instead of always `setRecurrence('none')` — if the
|
||||
CalendarOccurrence carries a recurrence preset use it; if the occurrence shape does not expose one,
|
||||
add the preset to the occurrence/expand contract is OUT OF SCOPE — instead read it from the cached
|
||||
raw recurrence if present and default to 'none' only when genuinely absent (document the limitation in
|
||||
a comment citing WR-03). Guard against opening edit mode before the cache is populated: keep fields
|
||||
blank-safe but re-run on arrival.
|
||||
extending the occurrence/expand contract is OUT OF SCOPE — read it from the cached raw recurrence if
|
||||
present and default to 'none' only when genuinely absent (add a comment citing WR-03 documenting that
|
||||
occurrence edits whose recurrence is not present in the cache default to 'none' in v1). Guard against
|
||||
opening edit mode before the cache is populated: keep fields blank-safe but re-run on arrival.
|
||||
|
||||
WR-05: rewrite `parseDateTime` so date and time are derived in ONE consistent frame. For a timed ISO
|
||||
with an offset/IANA suffix, compute the local wall-clock components with `getFullYear/getMonth/getDate/
|
||||
getHours/getMinutes` together (or reuse the Temporal-based conversion the calendar render path already
|
||||
uses) — never mix `toISOString().slice(0,10)` (UTC date) with `getHours()` (local time). The all-day
|
||||
`^\d{4}-\d{2}-\d{2}$` branch is unchanged.
|
||||
WR-05 (the owning fix): rewrite `parseDateTime` so date and time are derived in ONE consistent frame.
|
||||
For a timed ISO with an offset/IANA suffix, build the JS Date, then extract BOTH the date and time from
|
||||
the SAME accessor family — use local accessors together (`getFullYear/getMonth/getDate/getHours/getMinutes`,
|
||||
zero-padded) so the date string and the time string describe the same wall clock. NEVER mix
|
||||
`toISOString().slice(0,10)` (UTC date) with `getHours()` (local time). Because the WR-05 test pins TZ=UTC,
|
||||
"local" == UTC in the test and the extracted wall clock is deterministic; in production the user's own
|
||||
zone yields their own wall clock consistently. The all-day `^\d{4}-\d{2}-\d{2}$` branch is unchanged.
|
||||
|
||||
IN-03: collapse `getDefaultStartDate`/`getDefaultEndDate` into one `todayIso()` helper (reuse the
|
||||
existing one in calendarStore.ts if exported); keep the separate '09:00'/'10:00' default times at the
|
||||
call sites.
|
||||
IN-03: export the existing `todayIso` from calendarStore.ts (add the `export` keyword to the function at
|
||||
lines 121-124 — it is currently private), then import it into EventForm and collapse
|
||||
`getDefaultStartDate`/`getDefaultEndDate` into calls to `todayIso()`; keep the separate '09:00'/'10:00'
|
||||
default times at the call sites. Do not duplicate the helper — there must be exactly one `todayIso`.
|
||||
|
||||
Commit RED then GREEN.
|
||||
</action>
|
||||
@@ -106,11 +110,13 @@ getDefaultStartDate/getDefaultEndDate to one helper) is folded in as a cheap adj
|
||||
<acceptance_criteria>
|
||||
- behavior: edit form Title is populated even when occurrence resolves after open.
|
||||
- behavior: editing a recurring event preselects its recurrence preset.
|
||||
- behavior: a timed occurrence renders its own wall-clock date and time (no UTC/local day shift).
|
||||
- behavior (deterministic): with the runner TZ pinned to UTC, a timed occurrence `'2026-06-10T23:30:00-04:00'` renders the wall-clock date/time the corrected parseDateTime yields under UTC, and the assertion is hard-coded to those exact strings (cannot pass by a coincidentally-EDT runner).
|
||||
- source: the reset effect dependency array in EventForm.tsx includes occurrence (grep for occurrence in the deps line).
|
||||
- source: `grep -c 'export function todayIso' apps/pwa/src/store/calendarStore.ts` returns 1 (todayIso is now exported; IN-03).
|
||||
- source: parseDateTime no longer mixes UTC and local accessors — `grep -c 'toISOString' apps/pwa/src/components/EventForm.tsx` does not appear inside parseDateTime's timed branch (verify by reading the function).
|
||||
- test-command: `cd apps/pwa && npx vitest run src/components/EventForm.test.tsx` passes.
|
||||
</acceptance_criteria>
|
||||
<done>Edit mode pre-populates correctly (fields, recurrence, correct zone); duplicate date helpers collapsed.</done>
|
||||
<done>Edit mode pre-populates correctly (fields, recurrence, correct zone proven by a TZ-pinned deterministic test); duplicate date helpers collapsed to one exported todayIso.</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
@@ -160,11 +166,12 @@ getDefaultStartDate/getDefaultEndDate to one helper) is folded in as a cheap adj
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
Edit mode pre-populates fields/recurrence in the correct zone, the dialog traps focus,
|
||||
and the PWA install assets are confirmed present. WR-03, WR-05, WR-07, IN-03, IN-04 closed;
|
||||
Edit mode pre-populates fields/recurrence in the correct zone (proven by a TZ-pinned deterministic test),
|
||||
the dialog traps focus, and the PWA install assets are confirmed present. WR-03, WR-05, WR-07, IN-03, IN-04 closed;
|
||||
PWA-01/PWA-02 verified.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/03-event-write-back-pwa-install/03-12-SUMMARY.md` when done.
|
||||
</output>
|
||||
</content>
|
||||
|
||||
Reference in New Issue
Block a user