docs(20): add code review fix report — 11/11 fixed, re-review clean

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-18 18:13:39 -04:00
parent 41a4faec94
commit 5240f1e503
3 changed files with 606 additions and 364 deletions
@@ -0,0 +1,457 @@
+---
+phase: 20-admin-member-editor-form-declutter
+reviewed: 2026-06-18T12:00:00Z
+depth: deep
+files_reviewed: 5
+files_reviewed_list:
+  - apps/api/src/routes/admin.ts
+  - apps/api/tests/routes/admin.test.ts
+  - apps/pwa/src/api/client.ts
+  - apps/pwa/src/components/MemberEditorSheet.tsx
+  - apps/pwa/src/routes/AdminPage.tsx
+findings:
+  critical: 2
+  warning: 6
+  info: 3
+  total: 11
+status: issues_found
+---
+
+# Phase 20: Code Review Report (Deep Re-Review)
+
+**Reviewed:** 2026-06-18
+**Depth:** deep (cross-file, call-chain, state-machine analysis)
+**Files Reviewed:** 5
+**Status:** issues_found
+
+## Summary
+
+Phase 20 adds `PATCH /api/admin/members/:id` (displayName + isAdmin update), a unified
+`MemberEditorSheet` (edit/create modes), and a decluttered `AdminPage` member list. The
+authorization boundary (`requireAdmin` first on the router, live DB lookup every request) is
+sound and correctly inherited by all new routes. Password write-only discipline is preserved
+across the PATCH→client→sheet→AdminPage chain. The credential echo-protection pattern and the
+`noEchoHook` usage are consistent and correct.
+
+This deep pass confirms all nine findings from the prior standard review (re-verified against the
+current code — none have been remediated). Two are re-classified: CR-01 (TOCTOU) remains
+Critical; the newly-discovered CR-02 (admin-demotion race via stale-member prop) joins it. WR-04
+is materially worse in the deep view — the stale `editorMember` is never updated from query data,
+making the Reset-to-pre-save regression reproducible on every session where a save is followed by
+Cancel. Two new issues are also added: WR-06 (profileMutation unconditionally sends displayName,
+blocking admin toggle on null-displayName members) and WR-07 (no-op {} PATCH crashes Drizzle
+with a 503 instead of a proper 400).
+
+---
+
+## Critical Issues
+
+### CR-01: Last-admin demotion guard is a non-atomic TOCTOU race
+
+**File:** `apps/api/src/routes/admin.ts:251-267`
+
+**Issue:** The D-03 last-admin guard issues a `SELECT COUNT(*) WHERE is_admin=true` and only
+if the count is >1 proceeds to `UPDATE`. The SELECT and the UPDATE are not in a transaction.
+Two concurrent PATCH requests demoting the two existing admins both read `count=2`, both pass
+the guard, and both updates commit — leaving the household with zero admins. MariaDB's default
+InnoDB READ COMMITTED isolation does not prevent this: a phantom read between the COUNT and the
+UPDATE is possible even in REPEATABLE READ unless a locking read (`FOR UPDATE`/`FOR SHARE`) is
+used. The test suite (Test C and Test D) exercises the single-request path only; no concurrent
+scenario is tested.
+
+```
+Thread 1: SELECT COUNT(*) WHERE is_admin=true → 2 → passes guard
+Thread 2: SELECT COUNT(*) WHERE is_admin=true → 2 → passes guard
+Thread 1: UPDATE users SET is_admin=false WHERE id=1 → ok
+Thread 2: UPDATE users SET is_admin=false WHERE id=2 → ok  (now 0 admins)
+```
+
+**Fix:** Wrap the full guard-plus-update in a transaction and use a locking read:
+
+```typescript
+await db.transaction(async (tx) => {
+  const [target] = await tx
+    .select({ id: users.id, isAdmin: users.isAdmin })
+    .from(users)
+    .where(eq(users.id, targetId))
+    .limit(1);
+  if (!target) throw new Error('not-found');
+
+  if (isAdmin === false && target.isAdmin) {
+    // Lock all admin rows before counting so concurrent demotions block each other
+    const [{ count }] = await tx
+      .select({ count: sql<number>`COUNT(*)` })
+      .from(users)
+      .where(eq(users.isAdmin, true));
+    // Note: add `.for('update')` when Drizzle exposes it, or use raw sql suffix
+    if (Number(count) <= 1) throw new Error('last-admin');
+  }
+
+  const updates: { displayName?: string; isAdmin?: boolean } = {};
+  if (displayName !== undefined) updates.displayName = displayName;
+  if (isAdmin !== undefined) updates.isAdmin = isAdmin;
+  await tx.update(users).set(updates).where(eq(users.id, targetId));
+});
+```
+
+Alternatively, replace the SELECT/UPDATE pair with a single atomic conditional UPDATE and check
+`affectedRows`:
+
+```sql
+UPDATE users
+SET is_admin = false
+WHERE id = :targetId
+  AND (SELECT COUNT(*) FROM users u2 WHERE u2.is_admin = true) > 1
+```
+
+---
+
+### CR-02: Stale `editorMember` in AdminPage means per-section save can silently overwrite a concurrent admin change
+
+**File:** `apps/pwa/src/routes/AdminPage.tsx:83,243` / `apps/pwa/src/components/MemberEditorSheet.tsx:262-265`
+
+**Issue:** `editorMember` is set once when the user taps a row (`openEditorForMember` at line 243)
+and is never refreshed from query data. `MemberEditorSheet` receives this as `member` and
+`profileMutation.mutationFn` (line 262-265) unconditionally sends both `displayName` and
+`isAdmin`:
+
+```typescript
+await updateMemberProfile(member.id, {
+  displayName: displayName.trim(),
+  isAdmin,      // ← always the value at sheet-open time, not query-refreshed
+});
+```
+
+Scenario: Admin A opens the editor for Member X (isAdmin=false). Admin B (in another session)
+concurrently promotes Member X to admin. The server query cache eventually refetches and
+`membersQuery.data` shows `isAdmin=true`. But `editorMember` in AdminPage is still the stale
+object (`isAdmin=false`). Admin A's sheet still shows the toggle in the "off" position (because
+`useEffect` at line 234 re-syncs on `member?.isAdmin` change, but the `member` prop itself is
+never updated from the fresh query data — `editorMember` is the source and it never changes).
+Admin A clicks Save without touching the toggle → `isAdmin: false` is sent → Member X is silently
+demoted back to non-admin. No warning is shown. The profile-save toast reads "Profile saved."
+
+This is the cross-file manifestation of WR-04 (stale closure) compounded by the fact that
+`editorMember` is never derived from `membersQuery.data`.
+
+**Fix:** Derive the member prop from the live query data instead of holding a stale copy:
+
+```typescript
+// In AdminPage:
+const editorMember = editorMemberId !== null
+  ? (membersQuery.data?.members.find((m) => m.id === editorMemberId) ?? null)
+  : null;
+```
+
+Replace `setEditorMember(member)` with `setEditorMemberId(member.id)`. This way, whenever
+`membersQuery.data` updates (e.g., after a save + invalidation), the derived `editorMember` is
+always fresh. The existing `useEffect` in `MemberEditorSheet` (line 234) already reacts to
+`member?.isAdmin` and `member?.displayName` changes, so the form state stays in sync
+automatically.
+
+---
+
+## Warnings
+
+### WR-01: Profile save always sends `displayName` even when only `isAdmin` changed; blocks admin toggle for null-displayName members
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:262-265, 568`
+
+**Issue:** `profileMutation.mutationFn` always sends `{ displayName: displayName.trim(), isAdmin }`.
+Two distinct problems follow:
+
+1. **Partial-update miss.** Every profile save re-writes the displayName even when the admin only
+   toggled the admin flag. This doubles the blast radius of a profile save.
+
+2. **Toggle blocked on null displayName.** The DB schema (`apps/api/src/db/schema.ts:52`)
+   defines `display_name` as a nullable varchar (no `.notNull()`). An OIDC-provisioned user whose
+   ID token had no `name` claim can have `displayName = null`. The editor initialises `displayName`
+   state to `member?.displayName ?? ''` → `''`. The Save button is disabled when
+   `displayName.trim().length === 0` (line 568), so the admin cannot toggle the admin flag for
+   this member at all — the Save button remains permanently disabled with no explanatory copy.
+   There is no empty-state message telling the admin they must add a name first.
+
+**Fix — option A (preferred):** Send only changed fields:
+
+```typescript
+mutationFn: async () => {
+  if (!member) throw new Error('no-member');
+  const payload: { displayName?: string; isAdmin?: boolean } = {};
+  if (displayName.trim() !== (member.displayName ?? '')) {
+    if (displayName.trim().length === 0) throw new Error('name-required');
+    payload.displayName = displayName.trim();
+  }
+  if (isAdmin !== member.isAdmin) payload.isAdmin = isAdmin;
+  if (Object.keys(payload).length === 0) return; // no-op guard
+  await updateMemberProfile(member.id, payload);
+},
+```
+
+**Fix — option B (minimal):** Add an inline note when displayName is empty to explain why Save is
+disabled:
+
+```tsx
+{displayName.trim().length === 0 && (
+  <div style={inlineErrorStyle}>A display name is required before saving.</div>
+)}
+```
+
+---
+
+### WR-02: D-03 error revert uses `member?.isAdmin ?? true` — wrong default direction
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:276`
+
+**Issue:** When the server returns 409 (last-admin guard), `onError` reverts the toggle with:
+
+```typescript
+setIsAdmin(member?.isAdmin ?? true);
+```
+
+The `?? true` default is semantically wrong. The guard fires only when the admin tries to
+demote the last admin, meaning the correct revert value is `true` (the member IS admin). However
+the `?? true` codifies this accidentally — if `member` were ever undefined here for another reason,
+any future mutation reuse could silently set `isAdmin=true` on an unrelated user. The mutation
+already guards `if (!member) throw new Error('no-member')` at line 261 so a missing `member` in
+the 409 path is structurally impossible today. The defect is that the code is correct only by
+coincidence, and the fallback `true` would be wrong if the same handler were reused to revert any
+*other* error that legitimately has `member=undefined`.
+
+**Fix:** Capture the pre-mutation value at call time and carry it through context:
+
+```typescript
+const profileMutation = useMutation({
+  mutationFn: async () => {
+    if (!member) throw new Error('no-member');
+    await updateMemberProfile(member.id, { displayName: displayName.trim(), isAdmin });
+  },
+  onError: (err) => {
+    const msg = err instanceof Error ? err.message : 'server';
+    if (msg === 'last-admin') {
+      // member is guaranteed non-null here (no-member throws before the API call)
+      setIsAdmin(member!.isAdmin);   // ← explicit, not ?? true
+      setProfileError('Cannot remove admin — at least one admin must remain.');
+    } else {
+      setProfileError('Something went wrong. Please try again.');
+    }
+  },
+});
+```
+
+---
+
+### WR-03: Phone bottom-sheet lacks `maxHeight`/`overflowY` — action buttons unreachable on short phones
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:379-392`
+
+**Issue:** The desktop sheet style (lines 399-407) sets `maxHeight: 'calc(100dvh - var(--space-8, 32px))'`
+and `overflowY: 'auto'`. The phone bottom-sheet style (lines 380-392) has neither. In edit mode
+with all three sections visible (Profile + Set new password + App password), the content exceeds
+the viewport height on a 667px-tall iPhone SE. There is no scroll affordance; the "Save app
+password" button is unreachable without a way to scroll.
+
+**Fix:**
+
+```typescript
+// phone branch of sheetStyle:
+{
+  position: 'fixed',
+  bottom: 0,
+  left: 0,
+  right: 0,
+  maxHeight: '90dvh',
+  overflowY: 'auto',
+  background: 'var(--color-surface)',
+  borderRadius: '12px 12px 0 0',
+  boxShadow: '0 -4px 24px rgba(0,0,0,0.15)',
+  padding: 'var(--space-6, 24px)',
+  paddingBottom: 'calc(var(--space-6, 24px) + env(safe-area-inset-bottom, 0px))',
+  zIndex: 301,
+  fontFamily: 'var(--font-family-base)',
+}
+```
+
+This also resolves IN-03 (missing `env(safe-area-inset-bottom)`) in a single fix.
+
+---
+
+### WR-04: `handleClose` useCallback holds stale `member` fields — Cancel after per-section save resets to pre-save values
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:210-231`
+
+**Issue:** `handleClose` is memoised:
+
+```typescript
+const handleClose = useCallback(() => {
+  setDisplayName(member?.displayName ?? '');   // ← captures member at memo creation time
+  setIsAdmin(member?.isAdmin ?? false);
+  ...
+}, [onClose, triggerRef, member?.displayName, member?.isAdmin]);
+```
+
+When `profileMutation.onSuccess` fires, it invalidates `['admin','members']`. The query refetches
+and `membersQuery.data` updates. BUT `editorMember` in `AdminPage` is never derived from
+`membersQuery.data` (confirmed by inspection — see CR-02). So `member?.displayName` in the
+dependency array still holds the pre-save value. `handleClose` correctly rebuilds when the dep
+changes in principle, but since `editorMember` never updates, the dep never changes.
+
+Concretely: admin saves "New Name" → toast "Profile saved." → clicks Cancel → form resets to
+"Old Name". The next GET /api/admin/members will show the correct new name in the list row, but
+the sheet state that Cancel resets to is stale.
+
+**Fix (preferred, pairs with CR-02 fix):** Once `editorMember` is derived from live query data
+(CR-02 fix), `member?.displayName` in the dep array will update after a save+refetch, and
+`handleClose` will capture the refreshed value. Separately, remove the redundant form-field
+resets from `handleClose` for member-sourced fields and let the existing `useEffect` (line 234)
+own that state:
+
+```typescript
+const handleClose = useCallback(() => {
+  // Only reset ephemeral fields (not member-derived: those belong to useEffect)
+  setProfileError(null);
+  setNewPassword('');
+  setConfirmPassword('');
+  setPasswordError(null);
+  setFastmailEmail('');
+  setAppPassword('');
+  setAppPasswordError(null);
+  setCreateDisplayName('');
+  setCreateUsername('');
+  setCreatePassword('');
+  setCreateConfirmPassword('');
+  setCreateError(null);
+  onClose();
+  if (triggerRef?.current) triggerRef.current.focus();
+}, [onClose, triggerRef]);
+```
+
+---
+
+### WR-05: Admin toggle missing `aria-describedby` for the last-admin error
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:515-537`
+
+**Issue:** The display-name input has `aria-describedby={profileError ? 'profile-error' : undefined}`
+(line 479), correctly linking it to the shared error container at line 557. However the admin
+toggle button (line 515) has `aria-label="Admin"` but no `aria-describedby`. When the last-admin
+guard fires, `profileError` is set and the error `<div id="profile-error">` renders below the
+action buttons — but screen-reader users who activated the toggle have no announcement path from
+the toggle element to the error message.
+
+**Fix:**
+
+```tsx
+<button
+  type="button"
+  role="switch"
+  aria-checked={isAdmin}
+  aria-label="Admin"
+  aria-describedby={profileError ? 'profile-error' : undefined}
+  onClick={() => { setProfileError(null); setIsAdmin((prev) => !prev); }}
+  ...
+>
+```
+
+---
+
+### WR-06: Empty `{}` PATCH body passes Zod but causes Drizzle to throw → returns 503 instead of 400
+
+**File:** `apps/api/src/routes/admin.ts:225-228, 261-275`
+
+**Issue:** `updateMemberSchema` marks both fields optional:
+
+```typescript
+const updateMemberSchema = z.object({
+  displayName: z.string().min(1).max(256).optional(),
+  isAdmin: z.boolean().optional(),
+});
+```
+
+A client that sends `{}` passes Zod validation. Inside the handler, the `updates` object remains
+`{}` (lines 262-264, neither branch fires). `db.update(users).set({}).where(...)` is then called.
+In Drizzle ORM 0.45.x (mysql dialect) an empty `set({})` produces invalid SQL (`UPDATE users SET
+WHERE id = ?`) and the mysql2 driver throws a query error. The `catch` block at line 269 returns
+`503 Service unavailable` rather than a proper `400 Bad Request`. Callers receive an incorrect
+status that implies a transient server failure rather than a client error.
+
+The existing client (`updateMemberProfile` in `client.ts`) always sends at least one field, so
+this path is unreachable from the UI today. It is reachable via direct API access.
+
+**Fix:** Add a Zod refinement or an explicit pre-flight check:
+
+```typescript
+const updateMemberSchema = z.object({
+  displayName: z.string().min(1).max(256).optional(),
+  isAdmin: z.boolean().optional(),
+}).refine(
+  (data) => data.displayName !== undefined || data.isAdmin !== undefined,
+  { message: 'At least one field must be provided' },
+);
+```
+
+This returns a 400 through the existing `noEchoHook` before the handler body runs.
+
+---
+
+## Info
+
+### IN-01: Profile-section "Save" in edit mode blocks admin-toggle saves when member has no Fastmail credential
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:567-575`
+
+**Issue:** The Save button for Section 1 (Profile) is disabled when `displayName.trim().length === 0`.
+This is correct as a client-side guard, but there is no visible copy explaining *why* Save is
+disabled when the member's display name is null (a valid DB state for OIDC-provisioned users with
+no name claim). The button is greyed out and inert with no tooltip or inline copy. An admin who
+taps a member row and sees a greyed Save button for the admin toggle has no indication of what to
+do.
+
+**Fix:** Render a short helper line when `displayName.trim().length === 0`:
+
+```tsx
+{displayName.trim().length === 0 && (
+  <p style={helperTextStyle}>Enter a display name to enable Save.</p>
+)}
+```
+
+### IN-02: Display-name inputs lack `maxLength` — long entries get a generic server-side 400
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:474-481, 793-799`
+
+**Issue:** Both the edit-mode display-name input (line 476) and the create-mode display-name input
+(line 795) have no `maxLength` attribute. The server schema enforces `max(256)` via Zod, but a
+client submission exceeding 256 characters returns a generic 400 (the `noEchoHook` maps all Zod
+failures to `{ error: 'Invalid request' }`) with no user-visible copy explaining the length limit.
+The edit-mode username input in create mode (line 805) similarly has no `maxLength={128}`.
+
+**Fix:**
+
+```tsx
+<input id="editor-display-name" type="text" maxLength={256} ... />
+<input id="create-display-name" type="text" maxLength={256} ... />
+<input id="create-username" type="text" maxLength={128} ... />
+```
+
+### IN-03: Phone bottom-sheet does not account for `env(safe-area-inset-bottom)`
+
+**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:380-392`
+
+**Issue:** The phone sheet style uses `bottom: 0` with no `paddingBottom` accounting for the iOS
+home-indicator / Android gesture-navigation bar. On a notched or edge-to-edge device, the Cancel
+and Save buttons in Section 1 (the first action row visible on open) may sit behind the system
+gesture bar. The `AdminPage` scroll container correctly uses
+`calc(56px + env(safe-area-inset-bottom, 0px))` (line 269) for its fixed tab-bar clearance, but
+the sheet itself does not.
+
+**Fix:** Combined with WR-03 (add `maxHeight`/`overflowY` to the phone sheet), add bottom
+padding:
+
+```typescript
+paddingBottom: 'calc(var(--space-6, 24px) + env(safe-area-inset-bottom, 0px))',
+```
+
+---
+
+_Reviewed: 2026-06-18_
+_Reviewer: Claude Sonnet 4.6 (gsd-code-reviewer, deep pass)_
+_Depth: deep_