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

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

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
20-admin-member-editor-form-declutter	2026-06-18T12:00:00Z	deep	5	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	critical warning info total 2 6 3 11	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:

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:

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:

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:

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

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:

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

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:

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:

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

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:

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:

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

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:

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:

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

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

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