familysync/.planning/milestones/v1.0-phases/04-shared-lists-live-sync/04-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
04-shared-lists-live-sync	2026-06-09T15:30:00Z	standard	33	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	critical warning info total 3 5 3 11	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):

// 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:

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:

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:

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:

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:

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:

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:

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:

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:

// 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