familysync/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md

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