fix(03): correct event-write timezone + per-user calendar identity (Gate 2 Part D)
BUG A — timed events written 4h off: EventForm sent a naive local wall-clock string with no offset; the UTC API container parsed it via new Date() as UTC, so 09:00 America/Toronto serialized to DTSTART:...090000Z. Fix: new apps/pwa/src/lib/eventDateTime.ts serializes timed events to an unambiguous UTC instant in the browser (where the operator's zone is known); all-day stays a DATE string. No backend change. BUG B — created events attached to the wrong user's calendar + duplicate calendar rows per poll: calendars had no unique key on url, and poller/sync matched calendars by url alone — so under the shared single Fastmail account (D-16) one member's collection resolved to the other member's row. Fix: composite unique(user_id, url); scope poller lookup + sync select to (userId, url); hand migration 0001 (dedup + add key), applied to the live DB. Regression tests fail against the buggy url-only predicate. API 98/98, PWA 140/140, tsc clean both packages.
This commit is contained in:
@@ -16,7 +16,7 @@
|
||||
*/
|
||||
|
||||
import { schedule } from 'node-cron'
|
||||
import { eq } from 'drizzle-orm'
|
||||
import { and, eq } from 'drizzle-orm'
|
||||
import { db } from '../db/client.js'
|
||||
import { memberCredentials, calendars } from '../db/schema.js'
|
||||
import { decryptPassword } from './crypto.js'
|
||||
@@ -44,11 +44,15 @@ export async function runPoll(): Promise<void> {
|
||||
const davCalendars = await client.fetchCalendars()
|
||||
|
||||
for (const davCal of davCalendars) {
|
||||
// Look up the stored calendar row to get the known ctag (D-13)
|
||||
// Look up the stored calendar row to get the known ctag (D-13).
|
||||
// BUG B: scope by (userId, url). The two members share one Fastmail account
|
||||
// (D-16), so the same collection URL exists for both. A url-only predicate
|
||||
// matched the OTHER member's row → wrong ctag compared, and syncCalendar
|
||||
// wrote events under the wrong calendar. Match on this member's row only.
|
||||
const [stored] = await db
|
||||
.select()
|
||||
.from(calendars)
|
||||
.where(eq(calendars.url, davCal.url))
|
||||
.where(and(eq(calendars.userId, cred.userId), eq(calendars.url, davCal.url)))
|
||||
.limit(1)
|
||||
|
||||
// ctag/syncToken: defensive null handling (Pitfall #6)
|
||||
|
||||
@@ -18,7 +18,7 @@
|
||||
import type { DAVCalendar } from 'tsdav'
|
||||
import type { FastmailClient } from './client.js'
|
||||
import ICAL from 'ical.js'
|
||||
import { eq } from 'drizzle-orm'
|
||||
import { and, eq } from 'drizzle-orm'
|
||||
import { db } from '../db/client.js'
|
||||
import { calendars, calendarEvents } from '../db/schema.js'
|
||||
|
||||
@@ -56,10 +56,17 @@ export async function syncCalendar(
|
||||
})
|
||||
|
||||
// 2. Select the calendar row to get its DB id (insertId is unreliable on ON DUPLICATE KEY UPDATE).
|
||||
const [cal] = await db.select().from(calendars).where(eq(calendars.url, davCal.url)).limit(1)
|
||||
// BUG B: scope by (userId, url) — the same collection URL exists for both members
|
||||
// (shared Fastmail account, D-16). A url-only lookup returned the OTHER member's
|
||||
// row (lowest id), so events were cached under the wrong calendarId.
|
||||
const [cal] = await db
|
||||
.select()
|
||||
.from(calendars)
|
||||
.where(and(eq(calendars.userId, userId), eq(calendars.url, davCal.url)))
|
||||
.limit(1)
|
||||
if (!cal) {
|
||||
// Should never happen — we just upserted it
|
||||
throw new Error(`syncCalendar: could not find calendar row for url=${davCal.url}`)
|
||||
throw new Error(`syncCalendar: could not find calendar row for userId=${userId} url=${davCal.url}`)
|
||||
}
|
||||
|
||||
// 3. Fetch all calendar objects (REPORT calendar-query).
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
-- BUG B — calendars: add composite unique key (user_id, url).
|
||||
--
|
||||
-- Context (D-16): the two household members share ONE Fastmail account, so the
|
||||
-- SAME collection URL is polled by both credentials. The calendar upsert keyed on
|
||||
-- url alone never triggered onDuplicateKeyUpdate (no unique key on url), so every
|
||||
-- poll inserted a fresh calendar row; the url-only lookup then resolved to the
|
||||
-- other member's row, caching events under the wrong calendarId.
|
||||
--
|
||||
-- This migration is hand-written (not drizzle-kit generated) because drizzle-kit
|
||||
-- push is unsafe on populated MariaDB (false destructive diffs) and there is no
|
||||
-- migrations baseline. Apply it directly to the live DB before/with the image
|
||||
-- rebuild that ships the schema + broker fixes.
|
||||
--
|
||||
-- Order matters: duplicate (user_id, url) rows must be collapsed BEFORE the unique
|
||||
-- key is added, or ADD UNIQUE fails. We keep the LOWEST id per (user_id, url),
|
||||
-- repoint any cached events from the loser rows onto the keeper, then delete losers.
|
||||
|
||||
-- 1. Repoint calendar_events from duplicate calendar rows onto the keeper
|
||||
-- (lowest id) for each (user_id, url) group.
|
||||
UPDATE calendar_events ce
|
||||
JOIN calendars dup ON dup.id = ce.calendar_id
|
||||
JOIN (
|
||||
SELECT user_id, url, MIN(id) AS keep_id
|
||||
FROM calendars
|
||||
GROUP BY user_id, url
|
||||
) keeper ON keeper.user_id = dup.user_id AND keeper.url = dup.url
|
||||
SET ce.calendar_id = keeper.keep_id
|
||||
WHERE ce.calendar_id <> keeper.keep_id;
|
||||
|
||||
-- 2. Delete the duplicate (loser) calendar rows, keeping the lowest id per group.
|
||||
DELETE c FROM calendars c
|
||||
JOIN (
|
||||
SELECT user_id, url, MIN(id) AS keep_id
|
||||
FROM calendars
|
||||
GROUP BY user_id, url
|
||||
) keeper ON keeper.user_id = c.user_id AND keeper.url = c.url
|
||||
WHERE c.id <> keeper.keep_id;
|
||||
|
||||
-- 3. Add the composite unique key that makes the upsert idempotent per (user_id, url).
|
||||
ALTER TABLE calendars
|
||||
ADD CONSTRAINT uniq_calendar_user_url UNIQUE (user_id, url);
|
||||
@@ -73,7 +73,16 @@ export const calendars = mysqlTable(
|
||||
lastSyncedAt: timestamp('last_synced_at'),
|
||||
isShared: boolean('is_shared').default(false).notNull(),
|
||||
},
|
||||
(t) => [index('idx_calendars_user_id').on(t.userId)],
|
||||
(t) => [
|
||||
index('idx_calendars_user_id').on(t.userId),
|
||||
// BUG B: calendar identity is (userId, url), not url alone. The two household
|
||||
// members share one Fastmail account (D-16), so the SAME collection URL is
|
||||
// polled by both credentials. Without this unique key the calendar upsert's
|
||||
// onDuplicateKeyUpdate never fired → a new row per poll, and the url-only
|
||||
// lookup resolved to the other member's row → events cached under the wrong
|
||||
// calendarId. Keying on (userId, url) makes the upsert idempotent per member.
|
||||
unique('uniq_calendar_user_url').on(t.userId, t.url),
|
||||
],
|
||||
)
|
||||
|
||||
/**
|
||||
|
||||
@@ -194,6 +194,44 @@ describe('broker poller — runPoll', () => {
|
||||
expect(mockSyncCalendar).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('BUG B: scopes the stored-calendar lookup to (userId, url), not url alone', async () => {
|
||||
// Capture the predicate passed to db.select().from(calendars).where(...).
|
||||
// The buggy code passed eq(url) only; the fix passes and(eq(userId), eq(url)).
|
||||
// We serialize the predicate and assert it references the member's user_id column.
|
||||
const capturedWhere: unknown[] = []
|
||||
mockSelectWhere.mockImplementation((pred: unknown) => {
|
||||
capturedWhere.push(pred)
|
||||
return { limit: mockSelectLimit }
|
||||
})
|
||||
|
||||
const { runPoll } = await import('../../src/broker/poller.js')
|
||||
|
||||
mockCredentialsSelectResult.push({
|
||||
id: 7,
|
||||
userId: 42,
|
||||
fastmailEmail: 'lucas@fastmail.com',
|
||||
encryptedPassword: 'enc',
|
||||
})
|
||||
|
||||
mockFetchCalendars.mockResolvedValue([
|
||||
{ url: 'https://caldav.fastmail.com/cal/', displayName: 'Calendar', ctag: 'c', syncToken: null },
|
||||
])
|
||||
|
||||
await runPoll()
|
||||
|
||||
expect(capturedWhere.length).toBeGreaterThan(0)
|
||||
// A composite and(...) predicate exposes multiple queryChunks; a single eq does not
|
||||
// contain a nested SQL referencing the user_id column. Serialize and inspect.
|
||||
const pred = capturedWhere[0] as { queryChunks?: unknown[] }
|
||||
const serialized = JSON.stringify(pred, (_k, v) =>
|
||||
typeof v === 'object' && v !== null && 'name' in (v as Record<string, unknown>)
|
||||
? (v as { name?: unknown }).name
|
||||
: v,
|
||||
)
|
||||
expect(serialized).toContain('user_id')
|
||||
expect(serialized).toContain('url')
|
||||
})
|
||||
|
||||
it('processes all member credentials in a poll cycle', async () => {
|
||||
const { runPoll } = await import('../../src/broker/poller.js')
|
||||
|
||||
|
||||
@@ -247,6 +247,52 @@ describe('syncCalendar', () => {
|
||||
expect(eventUpdateArg.set).toHaveProperty('hasRrule', true)
|
||||
})
|
||||
|
||||
it('BUG B: scopes the calendar-row select to (userId, url), not url alone', async () => {
|
||||
// Capture the predicate passed to db.select().from(calendars).where(...).limit(1).
|
||||
const capturedWhere: unknown[] = []
|
||||
mockWhere.mockImplementation((pred: unknown) => {
|
||||
capturedWhere.push(pred)
|
||||
return { limit: mockLimit }
|
||||
})
|
||||
|
||||
const { syncCalendar } = await import('../../src/broker/sync.js')
|
||||
const mockClient = { fetchCalendarObjects: vi.fn().mockResolvedValue([]) }
|
||||
const mockDavCal = {
|
||||
url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
|
||||
displayName: 'Test',
|
||||
ctag: 'v1',
|
||||
syncToken: null,
|
||||
}
|
||||
|
||||
await syncCalendar(mockClient as never, mockDavCal as never, 99)
|
||||
|
||||
expect(capturedWhere.length).toBeGreaterThan(0)
|
||||
const serialized = JSON.stringify(capturedWhere[0], (_k, v) =>
|
||||
typeof v === 'object' && v !== null && 'name' in (v as Record<string, unknown>)
|
||||
? (v as { name?: unknown }).name
|
||||
: v,
|
||||
)
|
||||
expect(serialized).toContain('user_id')
|
||||
expect(serialized).toContain('url')
|
||||
})
|
||||
|
||||
it('BUG B: calendar upsert is idempotent — onDuplicateKeyUpdate fires for the calendar row', async () => {
|
||||
const { syncCalendar } = await import('../../src/broker/sync.js')
|
||||
const mockClient = { fetchCalendarObjects: vi.fn().mockResolvedValue([]) }
|
||||
const mockDavCal = {
|
||||
url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
|
||||
displayName: 'Test',
|
||||
ctag: 'v1',
|
||||
syncToken: null,
|
||||
}
|
||||
await syncCalendar(mockClient as never, mockDavCal as never, 1)
|
||||
// The calendar insert (call 0) must use onDuplicateKeyUpdate so the (userId,url)
|
||||
// unique key makes re-polls update-in-place instead of inserting duplicate rows.
|
||||
expect(mockOnDuplicateKeyUpdate).toHaveBeenCalled()
|
||||
const calUpdateArg = mockOnDuplicateKeyUpdate.mock.calls[0][0]
|
||||
expect(calUpdateArg.set).toHaveProperty('ctag')
|
||||
})
|
||||
|
||||
it('updates the calendar ctag/syncToken after a successful sync', async () => {
|
||||
const { syncCalendar } = await import('../../src/broker/sync.js')
|
||||
|
||||
|
||||
@@ -40,6 +40,7 @@ import {
|
||||
type RecurrencePreset,
|
||||
} from '../api/client.js'
|
||||
import type { CalendarOccurrence } from '../api/client.js'
|
||||
import { serializeEventDateTime } from '../lib/eventDateTime.js'
|
||||
|
||||
// ── Constants ─────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -262,11 +263,23 @@ export function EventForm() {
|
||||
const handleSubmit = () => {
|
||||
if (!validate()) return
|
||||
|
||||
// BUG A fix: serialize timed events to an unambiguous UTC instant here in
|
||||
// the browser (operator's zone is known) instead of sending a naive local
|
||||
// wall-clock string. The API container is UTC; a naive string was being read
|
||||
// as UTC, shifting 09:00 local to 09:00Z (4h off). See lib/eventDateTime.ts.
|
||||
const { start: serializedStart, end: serializedEnd } = serializeEventDateTime(
|
||||
allDay,
|
||||
startDate,
|
||||
startTime,
|
||||
endDate,
|
||||
endTime,
|
||||
)
|
||||
|
||||
const payload: CreateEventPayload = {
|
||||
title: title.trim(),
|
||||
allDay,
|
||||
start: allDay ? startDate : `${startDate}T${startTime}:00`,
|
||||
end: allDay ? endDate : `${endDate}T${endTime}:00`,
|
||||
start: serializedStart,
|
||||
end: serializedEnd,
|
||||
recurrence,
|
||||
...(location.trim() ? { location: location.trim() } : {}),
|
||||
...(description.trim() ? { description: description.trim() } : {}),
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
/**
|
||||
* BUG A regression — write-path timezone serialization.
|
||||
*
|
||||
* Verifies that timed events are serialized to an unambiguous UTC instant
|
||||
* (so the operator's wall-clock time round-trips correctly regardless of the
|
||||
* API container's timezone), while all-day events keep their DATE strings.
|
||||
*
|
||||
* The PWA vitest harness runs with TZ=UTC, so the assertions are computed
|
||||
* relative to the local zone (whatever it is) rather than hard-coding an offset.
|
||||
* The core guarantee under test: the serialized timed value is a UTC instant
|
||||
* (ends in 'Z') derived from the LOCAL wall clock — never the naive wall-clock
|
||||
* string passed through verbatim, and never an instant that loses the local hour.
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { serializeEventDateTime, localWallClockToUtcIso } from './eventDateTime.js'
|
||||
|
||||
describe('serializeEventDateTime (BUG A — write-path TZ)', () => {
|
||||
it('serializes a timed start to a UTC instant (ends in Z)', () => {
|
||||
const { start } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:00')
|
||||
expect(start.endsWith('Z')).toBe(true)
|
||||
// It must NOT be the naive wall-clock string (the original bug shape).
|
||||
expect(start).not.toBe('2026-06-07T09:00:00')
|
||||
})
|
||||
|
||||
it('the serialized instant round-trips back to the SAME local wall clock', () => {
|
||||
// This is the heart of BUG A: 09:00 in → 09:00 back out in the operator's zone.
|
||||
const { start, end } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:30')
|
||||
const startBack = new Date(start)
|
||||
expect(startBack.getHours()).toBe(9)
|
||||
expect(startBack.getMinutes()).toBe(0)
|
||||
const endBack = new Date(end)
|
||||
expect(endBack.getHours()).toBe(10)
|
||||
expect(endBack.getMinutes()).toBe(30)
|
||||
})
|
||||
|
||||
it('equals the instant new Date(local parts) produces — not a passthrough', () => {
|
||||
const { start } = serializeEventDateTime(false, '2026-06-07', '09:00', '2026-06-07', '10:00')
|
||||
expect(start).toBe(new Date('2026-06-07T09:00:00').toISOString())
|
||||
})
|
||||
|
||||
it('leaves all-day events as DATE strings (no time, no Z) — D-13 contract', () => {
|
||||
const { start, end } = serializeEventDateTime(true, '2026-06-07', '09:00', '2026-06-09', '10:00')
|
||||
expect(start).toBe('2026-06-07')
|
||||
expect(end).toBe('2026-06-09')
|
||||
})
|
||||
|
||||
it('localWallClockToUtcIso round-trips a local wall clock to a UTC instant', () => {
|
||||
const iso = localWallClockToUtcIso('2026-06-07', '09:00')
|
||||
expect(iso.endsWith('Z')).toBe(true)
|
||||
expect(new Date(iso).getHours()).toBe(9)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,61 @@
|
||||
/**
|
||||
* Event date/time serialization for the write path (BUG A fix).
|
||||
*
|
||||
* The bug: EventForm previously sent a NAIVE local wall-clock string
|
||||
* (`${date}T${time}:00`, e.g. "2026-06-07T09:00:00") with NO timezone offset.
|
||||
* The API outbox worker then did `new Date(thatString)`, which Node parses in
|
||||
* the SERVER container's local timezone (UTC in Docker). So "09:00" Toronto
|
||||
* became 09:00 UTC, and buildVeventString serialized it as `090000Z`, which
|
||||
* displays back as 05:00 EDT — a 4-hour error.
|
||||
*
|
||||
* The fix: serialize timed events to an UNAMBIGUOUS instant here in the browser,
|
||||
* where the operator's timezone IS known. `new Date(date + 'T' + time)` is parsed
|
||||
* in the browser's local zone (the operator's wall clock), and `.toISOString()`
|
||||
* converts that instant to a correct UTC `...Z` string. The worker's `new Date()`
|
||||
* then parses an unambiguous UTC instant regardless of container timezone, and
|
||||
* the round-trip preserves the operator's wall-clock time.
|
||||
*
|
||||
* All-day events carry no time component and remain `YYYY-MM-DD` DATE strings
|
||||
* (D-13) — they are timezone-independent by contract and must NOT be converted.
|
||||
*/
|
||||
|
||||
/**
|
||||
* Serialize the start/end of an event for the create/update payload.
|
||||
*
|
||||
* @param allDay when true, returns the date strings unchanged (DATE contract, D-13)
|
||||
* @param startDate 'YYYY-MM-DD'
|
||||
* @param startTime 'HH:MM' (ignored when allDay)
|
||||
* @param endDate 'YYYY-MM-DD'
|
||||
* @param endTime 'HH:MM' (ignored when allDay)
|
||||
* @returns { start, end } — for timed events, ISO-8601 UTC instants ('...Z');
|
||||
* for all-day events, the raw 'YYYY-MM-DD' date strings.
|
||||
*/
|
||||
export function serializeEventDateTime(
|
||||
allDay: boolean,
|
||||
startDate: string,
|
||||
startTime: string,
|
||||
endDate: string,
|
||||
endTime: string,
|
||||
): { start: string; end: string } {
|
||||
if (allDay) {
|
||||
// DATE contract (D-13): no time component, timezone-independent.
|
||||
return { start: startDate, end: endDate }
|
||||
}
|
||||
|
||||
// Timed: build the instant from local wall-clock parts (browser is in the
|
||||
// operator's zone) and serialize to a UTC instant so the wire value is
|
||||
// unambiguous and container-timezone-independent.
|
||||
return {
|
||||
start: localWallClockToUtcIso(startDate, startTime),
|
||||
end: localWallClockToUtcIso(endDate, endTime),
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Convert a local wall-clock date+time to a UTC ISO-8601 instant.
|
||||
* `new Date('YYYY-MM-DDTHH:MM:00')` (no offset) is parsed in the browser's
|
||||
* local timezone per ECMAScript, giving the correct instant for the operator.
|
||||
*/
|
||||
export function localWallClockToUtcIso(date: string, time: string): string {
|
||||
return new Date(`${date}T${time}:00`).toISOString()
|
||||
}
|
||||
Reference in New Issue
Block a user