.planning/phases/04-shared-lists-live-sync/04-REVIEW.md

@@ -0,0 +1,439 @@
+---
+phase: 04-shared-lists-live-sync
+reviewed: 2026-06-09T15:30:00Z
+depth: standard
+files_reviewed: 33
+files_reviewed_list:
+  - apps/api/src/db/schema.ts
+  - apps/api/src/index.ts
+  - apps/api/src/lib/listAccess.ts
+  - apps/api/src/lib/listEmitter.ts
+  - apps/api/src/lib/rank.ts
+  - apps/api/src/routes/lists.ts
+  - apps/api/src/routes/sse.ts
+  - apps/api/test/setup.ts
+  - apps/api/tests/lib/listAccess.test.ts
+  - apps/api/tests/lib/listEmitter.test.ts
+  - apps/api/tests/lib/rank.test.ts
+  - apps/api/tests/routes/lists.test.ts
+  - apps/api/vitest.config.ts
+  - apps/pwa/src/App.tsx
+  - apps/pwa/src/api/listsClient.ts
+  - apps/pwa/src/components/AddItemInput.tsx
+  - apps/pwa/src/components/AppNav.tsx
+  - apps/pwa/src/components/BottomTabBar.tsx
+  - apps/pwa/src/components/CalendarShell.test.tsx
+  - apps/pwa/src/components/CreateListSheet.tsx
+  - apps/pwa/src/components/ItemRow.tsx
+  - apps/pwa/src/components/ListCard.tsx
+  - apps/pwa/src/components/ListDeleteDialog.tsx
+  - apps/pwa/src/components/ListsEmptyState.tsx
+  - apps/pwa/src/components/LiveSyncIndicator.tsx
+  - apps/pwa/src/hooks/useListSSE.test.ts
+  - apps/pwa/src/hooks/useListSSE.ts
+  - apps/pwa/src/routes/ListDetail.test.tsx
+  - apps/pwa/src/routes/ListDetail.tsx
+  - apps/pwa/src/routes/ListsIndex.tsx
+  - apps/pwa/src/store/listsStore.ts
+  - apps/pwa/package.json
+  - apps/api/package.json
+findings:
+  critical: 3
+  warning: 5
+  info: 3
+  total: 11
+status: issues_found
+---
+
+# Phase 4: Code Review Report
+
+**Reviewed:** 2026-06-09T15:30:00Z
+**Depth:** standard
+**Files Reviewed:** 33
+**Status:** issues_found
+
+## Summary
+
+Reviewed the full Phase 4 shared-lists + live-sync implementation: API routes, schema,
+access-control helpers, SSE fan-out, fractional rank, and the React PWA layer (mutations,
+SSE hook, drag-to-reorder, components). The previously-recorded rank collation bug
+(uppercase fractional-indexing keys sort incorrectly under utf8mb4_uca1400_ai_ci) is
+acknowledged but not re-litigated here per brief instructions.
+
+Three critical issues were found: a privilege-escalation hole that lets any list sharee
+unilaterally de-share or re-share a list (purging or creating list_shares rows for ALL
+household members), an SSE subscription scope that is computed once at connect time and
+never refreshed (so a newly-shared list never reaches a live subscriber without a
+disconnect/reconnect), and a missing NaN-guard on URL path parameters that causes DB
+queries to execute with a filter of `id = NaN` instead of returning 400.
+
+---
+
+## Critical Issues
+
+### CR-01: Sharee can de-share or re-share a list — privilege escalation on `isShared` toggle
+
+**File:** `apps/api/src/routes/lists.ts:319-393`
+
+**Issue:** `PATCH /api/lists/:id` gates on `checkListAccess` (owner OR sharee) but does
+not restrict the `isShared` field to the owner. A sharee — any household member who was
+granted access — can send `{ isShared: false }` and the handler will:
+
+1. Write `is_shared = false` to the `lists` row (changing the list's visibility state on
+   behalf of the owner without consent).
+2. Delete ALL rows from `list_shares` for that list (line 366-368), immediately revoking
+   every other member's access including the owner's own sharee visibility.
+
+The inverse (a sharee escalating a private list to shared by sending `{ isShared: true }`)
+is also possible, inserting `list_shares` rows for every user in the DB without the owner's
+consent.
+
+The comment on line 344 reads "Reconcile list_shares on visibility change (owner only
+affects shares)" but there is no `isOwner` guard anywhere in the PATCH handler — the
+reconciliation runs unconditionally for any `allowed` user.
+
+The test at `lists.test.ts:438-450` explicitly tests and asserts that a sharee CAN rename
+a list, which is correct, but there is no test asserting that a sharee CANNOT toggle
+`isShared`. The gap is uncovered.
+
+**Fix:** Add an owner-only guard before the `isShared` reconciliation block (and before
+writing `isShared` itself, since the DB field controls visibility semantics):
+
+```typescript
+// In PATCH /:id, after the access check at line 327-332:
+if (patch.isShared !== undefined && !access.isOwner) {
+  return c.json({ error: 'Only the list owner can change sharing settings' }, 403)
+}
+```
+
+---
+
+### CR-02: SSE subscription scope is stale — newly shared lists never delivered to live subscribers
+
+**File:** `apps/api/src/routes/sse.ts:85-121`
+
+**Issue:** `GET /api/sse/lists` calls `getAccessibleListIds(userId)` exactly once at
+connection time (line 89), then subscribes only to those list channels. If the user's
+access set changes while the SSE connection is open — for example, another member creates
+a new shared list (which inserts a `list_shares` row for this user), or a PATCH toggles
+`isShared` — the live subscriber never receives `list:updated` events for the new list
+because no subscription was registered for its channel.
+
+From the client's perspective: member A creates "Groceries" (shared). The `publishListEvent`
+fires on channel `list:${newId}`. Member B's open SSE stream has no subscriber on that
+channel — it was computed before the list existed. B only learns about the list when the
+30-second polling fallback fires (D-12).
+
+This means the "other member sees the change appear without refreshing" requirement (Truth
+3) is not met for newly-created shared lists while both members are simultaneously connected.
+The 30-second polling fallback (D-12) masks the failure but does not eliminate it.
+
+**Fix (two options):**
+
+Option A (minimal): When `POST /api/lists` creates a shared list, publish a special
+`list:created` event to a well-known global channel (e.g. `global:lists`) that all
+authenticated SSE connections also subscribe to. On receiving `list:created`, the client
+invalidates `['lists']` and re-establishes (or the server issues a reconnect hint).
+
+Option B (structural, recommended): Store the SSE handler's `userId` and wire the
+`list:created` event through a per-user "inbox" channel (`user:${userId}`) that the SSE
+endpoint subscribes to in addition to the per-list channels. `POST /api/lists` fans out
+to each sharee's inbox. The SSE handler then dynamically adds a new per-list subscription
+when it receives the inbox event.
+
+At minimum, `POST /api/lists`, `PATCH /api/lists/:id` (when toggling `isShared`), and
+the `list:deleted` flow all need to trigger re-subscription updates for affected users.
+
+---
+
+### CR-03: `Number(c.req.param(...))` — NaN propagates silently into DB queries
+
+**File:** `apps/api/src/routes/lists.ts:323, 407, 459, 521, 569, 663`
+
+**Issue:** Every route that reads a URL path parameter converts it with bare `Number(...)`.
+`Number('abc')` is `NaN`. All subsequent Drizzle `eq(lists.id, NaN)` calls emit SQL like
+`WHERE id = NaN` which MariaDB coerces to `WHERE id = 0`. This returns "not found" for
+most paths, but the behavior is implementation-defined and fragile:
+
+- A crafted request to `PATCH /api/lists/abc` skips the `checkListAccess` notFound→404
+  branch and returns a 404, which is benign but by accident.
+- A crafted request to `GET /api/lists/abc/items` proceeds past the access check with
+  `listId = 0`, queries `WHERE list_id = 0` (no rows), and returns `{ items: [] }` — a
+  200 with empty data rather than a 400.
+- The `listItemsRouter` `PATCH /:itemId` at line 569 fetches `WHERE id = 0` from
+  `list_items`, gets no row, and returns 404, which again masks rather than rejects.
+
+Silently treating invalid input as a DB query is incorrect behavior. Every route should
+validate the path parameter before touching the DB.
+
+**Fix:** Add NaN validation immediately after each `Number(...)` conversion:
+
+```typescript
+const listId = Number(c.req.param('id'))
+if (!Number.isInteger(listId) || listId < 1) {
+  return c.json({ error: 'Invalid id' }, 400)
+}
+```
+
+Apply the same pattern to `itemId` at lines 569 and 663. `ListDetail.tsx` already does
+this check for its own `parsedListId` (line 315), confirming the pattern is known; it
+just was not applied server-side.
+
+---
+
+## Warnings
+
+### WR-01: SSE listener registered as `async` but errors inside it are silently dropped
+
+**File:** `apps/api/src/routes/sse.ts:96-103`
+
+**Issue:** The handler passed to `subscribeListEvents` is declared `async`:
+
+```typescript
+const unsub = subscribeListEvents(listId, async (event) => {
+  if (stream.aborted) return
+  await stream.writeSSE(...)
+})
+```
+
+`EventEmitter.emit()` does not await Promises returned by listeners. If `stream.writeSSE`
+rejects (e.g. the underlying socket was half-closed but `stream.aborted` has not been set
+yet), the rejection is an unhandled Promise rejection. Under Node.js 18+ this can crash
+the process depending on the `unhandledRejection` policy. In production behind Pangolin the
+risk is a silent dropped write followed by an eventual crash.
+
+**Fix:** Wrap the async body in a try/catch:
+
+```typescript
+subscribeListEvents(listId, (event) => {
+  if (stream.aborted) return
+  stream.writeSSE({
+    data: JSON.stringify(event),
+    event: event.type,
+    id: `${listId}-${Date.now()}`,
+  }).catch((err) => {
+    console.error('[sse/lists] writeSSE failed:', err)
+  })
+})
+```
+
+---
+
+### WR-02: Unsubscribers run AFTER the heartbeat loop exits — they may never run if `writeSSE` throws
+
+**File:** `apps/api/src/routes/sse.ts:107-119`
+
+**Issue:** The cleanup block (`unsubscribers.forEach(...)` at line 119) is placed after the
+`while (!stream.aborted)` loop. If `stream.writeSSE` inside the heartbeat loop throws
+synchronously, the loop exits via exception propagation and the `unsubscribers.forEach`
+line is never reached. This leaves orphaned listeners attached to the module-level emitter
+for the lifetime of the process — a listener leak that accumulates with every aborted
+connection.
+
+In the current implementation `streamSSE` from Hono likely catches the inner Promise, but
+the placement creates a fragile dependency on that behavior.
+
+**Fix:** Use a try/finally block to guarantee cleanup:
+
+```typescript
+return streamSSE(c, async (stream) => {
+  const unsubscribers: Array<() => void> = []
+
+  try {
+    for (const listId of accessibleListIds) {
+      const unsub = subscribeListEvents(listId, (event) => { ... })
+      unsubscribers.push(unsub)
+    }
+
+    let tick = 0
+    while (!stream.aborted) {
+      await stream.writeSSE({ ... })
+      await stream.sleep(30_000)
+    }
+  } finally {
+    unsubscribers.forEach((unsub) => unsub())
+  }
+})
+```
+
+---
+
+### WR-03: `position` field in `patchItemSchema` accepts any string — no fractional-indexing format validation
+
+**File:** `apps/api/src/routes/lists.ts:107-117`
+
+**Issue:** `patchItemSchema` validates `position` as `z.string().min(1).max(255)`. A client
+can send any arbitrary string as a rank (e.g. `"aaaaa..."` 255 chars, or `"\x00"`).
+`fractional-indexing` has specific format constraints: keys must match a particular
+character set and structure. An invalid rank value written to the DB will permanently
+corrupt the ordering for all items in the list, since subsequent `generateKeyBetween`
+calls against a malformed neighbor will throw or produce unpredictable output.
+
+This is particularly relevant because malformed ranks survive server-side silently — the
+DB stores whatever string is written and returns it in ORDER BY, but `generateKeyBetween`
+on the PWA side will throw when encountering an out-of-spec rank as a neighbor.
+
+**Fix:** Add a regex validator matching the fractional-indexing key format. The library
+produces keys in `[A-Za-z0-9]` with specific leading-character rules. At minimum, restrict
+to the documented safe character set:
+
+```typescript
+position: z.string()
+  .min(1)
+  .max(255)
+  .regex(/^[A-Za-z0-9]+$/, 'Invalid fractional rank format'),
+```
+
+Or call `validateOrderKey` from the `fractional-indexing` package inside a `.refine()`.
+
+---
+
+### WR-04: `PATCH /api/lists/:id` does not guard `isShared` changes against non-owner callers in test coverage
+
+**File:** `apps/api/tests/routes/lists.test.ts:438-450`
+
+**Issue:** The test `"sharee can rename a shared list they have access to"` asserts the
+correct behaviour (sharees can rename), but there is no corresponding negative test
+asserting that a sharee CANNOT change `isShared`. Given CR-01 above is a confirmed bug,
+the absence of this test means the regression will go undetected after the fix unless a
+test is added simultaneously.
+
+**Fix:** Add a test case in the `PATCH /api/lists/:id` describe block:
+
+```typescript
+it('returns 403 when a sharee attempts to change isShared (owner-only)', async () => {
+  const ownerId = await seedUser('patch-isshared-owner')
+  const shareeId = await seedUser('patch-isshared-sharee')
+  const listId = await seedList(ownerId, 'Shared List', true)
+  await shareList(listId, shareeId)
+
+  currentDevUserId = shareeId
+  const app = await getApp()
+  const res = await app.request(
+    jsonRequest('PATCH', `/api/lists/${listId}`, { isShared: false }),
+  )
+  expect(res.status).toBe(403)
+})
+```
+
+---
+
+### WR-05: Optimistic rank computation in `addMutation` can produce a duplicate rank when a concurrent add is in-flight
+
+**File:** `apps/pwa/src/routes/ListDetail.tsx:159-162`
+
+**Issue:** `addMutation.onMutate` computes the optimistic rank using:
+
+```typescript
+const lastRank = activeItems.at(-1)?.rank ?? null
+const optimisticRank = generateKeyBetween(lastRank, null)
+```
+
+`activeItems` is the locally-computed split of the React Query cache at the time the
+mutation fires. If two concurrent adds are initiated in quick succession (e.g. rapid Enter
+key taps), the second `onMutate` reads the cache that already contains the first optimistic
+item (with `id: -Date.now()`). However, the first optimistic item's rank was computed from
+the same `lastRank`, so `generateKeyBetween(lastRank, null)` is called twice with the
+same `lastRank`, producing the same rank string for both optimistic items.
+
+Both items render visually without issue, but on settlement the first item gets rank R1
+from the server and the second gets rank R2 > R1. The transient duplicate rank in the cache
+can cause a visible re-ordering flash during the `onSettled` invalidation.
+
+This is a cosmetic issue only (server round-trips produce correct ordering), but it violates
+the "no accidental reorder flash" UX expectation.
+
+**Fix:** After the first optimistic insert, re-read the cache to get the updated last rank
+for the second add. Since `onMutate` is async, read the updated cache state after
+`cancelQueries` completes:
+
+```typescript
+onMutate: async (text: string) => {
+  await queryClient.cancelQueries({ queryKey: ['list', parsedListId] })
+  // Read AFTER cancel so concurrent in-flight optimistic updates are visible
+  const previous = queryClient.getQueryData<ListItemsResponse>(['list', parsedListId])
+  const currentActiveItems = (previous?.items ?? [])
+    .filter((i) => !i.checked)
+    .sort((a, b) => (a.rank < b.rank ? -1 : 1))
+  const lastRank = currentActiveItems.at(-1)?.rank ?? null
+  ...
+}
+```
+
+---
+
+## Info
+
+### IN-01: `useListSSE` connects to `/api/sse/lists` — not scoped to the current `listId`
+
+**File:** `apps/pwa/src/hooks/useListSSE.ts:65`
+
+**Issue:** The hook is parameterized on `listId` and invalidates `['list', listId]` on
+events, but the SSE connection it opens is `/api/sse/lists` — the server-side global
+fan-out stream for ALL lists the user can access. Events for other lists the user owns or
+shares (e.g. a grocery list while viewing a gift list) also trigger `handleListChange`,
+which only invalidates the currently-viewed list's query key. Events for other lists are
+received and ignored, which is harmless but slightly wasteful.
+
+The `listId` parameter to the hook is used only for cache invalidation, not for scoping
+the server subscription. This is by design per D-10, but the hook's name (`useListSSE`)
+and the `listId` parameter imply it is scoped to one list, which may confuse future
+maintainers.
+
+**Fix (documentation):** Add a comment clarifying that the connection is intentionally
+global and `listId` is only the invalidation target. Alternatively, rename the parameter
+to `activeListId` to signal its limited scope.
+
+---
+
+### IN-02: `getAccessibleListIds` issues two sequential DB round-trips that could be one query
+
+**File:** `apps/api/src/lib/listAccess.ts:29-43`
+
+**Issue:** The function issues two separate `SELECT` queries — one for owned lists, one
+for shared lists — then unions the results in JavaScript. This is two DB round-trips
+where one `UNION` or a single query with an `OR` would suffice. In a two-member household
+the cost is negligible; it is called at SSE connection time and can be called on every
+request to `GET /api/lists` in the future. As the call count grows this becomes a latency
+doubling point.
+
+**Fix:** Not urgent, but a single query avoids the double round-trip:
+
+```typescript
+// Single query with OR
+const rows = await db
+  .selectDistinct({ id: lists.id })
+  .from(lists)
+  .leftJoin(listShares, eq(listShares.listId, lists.id))
+  .where(
+    or(eq(lists.ownerId, userId), eq(listShares.userId, userId)),
+  )
+return rows.map((r) => r.id)
+```
+
+---
+
+### IN-03: The 401 test in `lists.test.ts` is a no-op assertion
+
+**File:** `apps/api/tests/routes/lists.test.ts:263-287`
+
+**Issue:** The test `"returns 401 when no session is set"` contains the assertion
+`expect(true).toBe(true)` with a comment explaining why the 401 path is not actually
+exercised. The test body documents a known gap in test coverage (the dev-bypass path makes
+it impossible to test 401 via the same app instance without module-level re-mocking). This
+is a real gap — the 401 enforcement path is never exercised in the automated suite.
+
+The test gives false confidence by appearing in the describe block as a passing test while
+asserting nothing about the code under review.
+
+**Fix:** Either remove the test (if it cannot be implemented), or implement it properly by
+using `vi.doMock` before a fresh `import()` of `app` to override `devAuthBypass` to a
+no-op in that test only, then assert `res.status === 401`. The pattern is already used in
+the `@hono/oidc-auth` mock above it. Keeping a passing test that asserts `true === true`
+is misleading.
+
+---
+
+_Reviewed: 2026-06-09T15:30:00Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_