familysync/.planning/milestones/v1.1-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-18T14:30: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 0 0 0 0	clean

Phase 20: Code Review Report (Deep Re-Review — Iteration 2)

Reviewed: 2026-06-18 Depth: deep (cross-file, call-chain, state-machine analysis) Files Reviewed: 5 Status: clean

Summary

All 11 findings from the prior pass (2 Critical, 6 Warning, 3 Info) are genuinely resolved — not superficially patched. Verification traces are below.

One new Info-level issue was introduced by the no-op guard fix: the "Profile saved." toast fires even when the admin clicks Save without changing anything, because mutationFn returns early (no network call) but onSuccess still runs unconditionally.

---

Prior Finding Verification

CR-01 — Last-admin guard now atomic: RESOLVED

apps/api/src/routes/admin.ts:258–286

The guard and UPDATE are wrapped in a single db.transaction() call. Inside the transaction, the target row is re-read with a plain (non-locking) SELECT. The locking read is then a raw tx.execute(sql\SELECT COUNT(*) AS count FROM ... WHERE is_admin = true FOR UPDATE`). Under InnoDB REPEATABLE READ (MariaDB default), FOR UPDATE` acquires exclusive row locks on all qualifying rows, serialising concurrent demotion transactions: the second PATCH blocks until the first commits, then re-reads a count of 1 and trips the guard.

The tx.execute() call uses the transaction's dedicated connection (confirmed via drizzle-orm 0.45.2 mysql2/session.js: the transaction callback receives a MySql2Transaction whose session holds the connection obtained by pool.getConnection() — the same connection that issued BEGIN). The FOR UPDATE lock is therefore in-scope for the transaction.

The COUNT result is destructured as [[{ count }]] from the raw execute result [RowDataPacket[], FieldPacket[]]. The cast is correct. Number(count) safely handles both number and string returns from MariaDB.

The 409 response shape { error: 'Cannot remove the last admin' } is unchanged. The client (client.ts:262) maps 409 → throw new Error('last-admin'), and the sheet's onError checks msg === 'last-admin'. The chain is intact.

Test C and Test D exercise the single-request guard paths and still pass. No concurrent-scenario test exists, but the fix is structurally correct and cannot be unit-tested against a single in-process MariaDB without intentional sleep-based race staging.

---

CR-02 — Stale editorMember: RESOLVED

apps/pwa/src/routes/AdminPage.tsx:87, 122–127

AdminPage now stores only editorMemberId: number | null (line 87) and derives editorMember as a computed value on every render:

const editorMember =
  editorMemberId !== null
    ? (membersQuery.data?.members.find((m) => m.id === editorMemberId) ?? null)
    : null;

openEditorForMember calls setEditorMemberId(member.id) (line 254). After profileMutation.onSuccess invalidates ['admin', 'members'] and the query refetches, editorMember is rederived from fresh data on the next render. The useEffect in MemberEditorSheet (line 235–239) depends on [member?.id, member?.displayName, member?.isAdmin] and re-syncs form state immediately. The stale-snapshot overwrite path is closed.

---

WR-01 — Profile mutation sends diff-only payload: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:261–275

mutationFn now builds a partial payload: displayName is added only when trimmed !== (member.displayName ?? ''), and isAdmin only when isAdmin !== member.isAdmin. An admin toggling only the admin flag on a null-displayName member sends { isAdmin: true/false } with no displayName field — the Zod schema accepts this (both optional, refine requires at least one). The Save button remains enabled as long as displayName.trim().length > 0 (or the existing displayName is non-null and unchanged). Toggle-only saves on null-displayName members are now unblocked.

---

WR-02 — Error revert uses explicit value: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:290

setIsAdmin(member!.isAdmin) replaces the accidental ?? true default. The member! non-null assertion is safe here: mutationFn at line 262 throws Error('no-member') before any API call when member is undefined, so the 409 error path can only be reached with a non-null member. The revert is now semantically explicit.

---

WR-03 — Phone bottom-sheet overflow: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:395–413

The phone branch of sheetStyle now has maxHeight: '90dvh' and overflowY: 'auto' (lines 404–405). All three sections scroll within the 90dvh cap on short phones.

---

WR-04 — handleClose stale closure: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:214–232

handleClose dependency array is now [onClose, triggerRef] — it no longer captures member?.displayName or member?.isAdmin. Only ephemeral fields (password inputs, error states, create-mode fields) are reset in handleClose. Member-derived fields (displayName, isAdmin) are owned exclusively by the useEffect at lines 235–239, which fires whenever the live member prop changes. Cancel after a successful save now resets to the saved (fresh) values, not the pre-save snapshot.

---

WR-05 — Admin toggle aria-describedby: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:544

The admin toggle <button> now has aria-describedby={profileError ? 'profile-error' : undefined}, linking it to the shared <div id="profile-error"> error container (line 588). Screen-reader users who activated the toggle receive an announcement path to the last-admin guard error.

---

WR-06 — Empty {} PATCH returns 400: RESOLVED

apps/api/src/routes/admin.ts:225–232

updateMemberSchema now has a .refine() that rejects any body where both displayName and isAdmin are absent. The noEchoHook returns { error: 'Invalid request' } 400 before the handler body executes. Drizzle is never called with an empty set({}).

Test H (line 1231–1241 in admin.test.ts) asserts this path returns 400 with { error: 'Invalid request' }.

---

IN-01 — Helper text for empty displayName: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:580–584

The helper text "Enter a display name to enable Save." renders when displayName.trim().length === 0 && !profileError. Admins opening a null-displayName member's editor now see an explanation for why the Save button is disabled.

---

IN-02 — maxLength on display-name and username inputs: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:500, 826, 842

All three inputs now have maxLength: edit-mode display-name maxLength={256} (line 500), create-mode display-name maxLength={256} (line 826), create-mode username maxLength={128} (line 842). Over-length submissions are prevented at the browser input level.

---

IN-03 — Phone sheet safe-area padding: RESOLVED

apps/pwa/src/components/MemberEditorSheet.tsx:411

paddingBottom: 'calc(var(--space-6, 24px) + env(safe-area-inset-bottom, 0px))' is present in the phone branch, co-located with the maxHeight/overflowY fix from WR-03.

---

Info

IN-01: No-op profile save fires misleading "Profile saved." toast

File: apps/pwa/src/components/MemberEditorSheet.tsx:274, 277–280

Issue: When the admin opens the editor and clicks Save without making any changes, mutationFn detects an empty payload (Object.keys(payload).length === 0) and returns early without calling the API. TanStack Query v5 treats a non-throwing return as a successful mutation and calls onSuccess, which fires invalidateQueries(['admin', 'members']) and onToast('Profile saved.'). The admin sees a confirmation toast for an action that sent nothing. The query also refetches unnecessarily.

This cannot be reached through the empty-displayName path (Save is disabled then), but it is reachable any time an admin opens a sheet and saves without touching anything.

Fix: Guard the toast and invalidation on whether a payload was actually sent:

mutationFn: async () => {
  if (!member) throw new Error('no-member');
  const payload: { displayName?: string; isAdmin?: boolean } = {};
  const trimmed = displayName.trim();
  if (trimmed !== (member.displayName ?? '')) {
    if (trimmed.length === 0) throw new Error('name-required');
    payload.displayName = trimmed;
  }
  if (isAdmin !== member.isAdmin) payload.isAdmin = isAdmin;
  if (Object.keys(payload).length === 0) return { noop: true };
  await updateMemberProfile(member.id, payload);
  return { noop: false };
},
onSuccess: (result) => {
  if (result?.noop) return; // nothing changed — no toast, no refetch
  void queryClient.invalidateQueries({ queryKey: ['admin', 'members'] });
  onToast('Profile saved.');
},

Alternatively, disable the Save button when displayName.trim() === (member?.displayName ?? '') and isAdmin === member?.isAdmin (change-detection guard on the button itself).

---

Reviewed: 2026-06-18 Reviewer: Claude Sonnet 4.6 (gsd-code-reviewer, deep pass — iteration 2) Depth: deep