diff --git a/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW-FIX.md b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW-FIX.md new file mode 100644 index 0000000..7568689 --- /dev/null +++ b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW-FIX.md @@ -0,0 +1,59 @@ +--- +phase: 20-admin-member-editor-form-declutter +fixed_at: 2026-06-18T14:45:00Z +source_review: 20-REVIEW.md +fix_scope: all +findings_in_scope: 11 +fixed: 11 +skipped: 0 +iteration: 2 +status: all_fixed +gates: + typecheck: pass + eslint: pass + prettier: pass + api_tests: 462/462 +--- + +# Phase 20 — Code Review Fix Report + +Auto-fix pass over the deep review (`20-REVIEW.md`, 2 critical / 6 warning / 3 info). +All 11 findings fixed and committed atomically; a deep re-review (iteration 2) +independently confirmed 0 critical / 0 warning remain. + +> Note: this report was reconstructed by the orchestrator — the fixer agent applied +> and committed every fix but its `REVIEW-FIX.md` write did not persist. The commit +> hashes below are the source of truth. + +## Fixes applied + +| ID | Severity | Fix | Commit | +|----|----------|-----|--------| +| CR-01 | Critical | Last-admin guard made atomic — single conditional UPDATE / `affectedRows` check closes the TOCTOU window; 409 response shape unchanged | `7297733` | +| WR-06 | Warning | Empty `{}` PATCH body now rejected with a clean 400 via Zod refinement (was a Drizzle 503 on empty SET); new test added | `7297733` | +| CR-02 | Critical | `AdminPage` derives the editor's member from live query data and refetches/invalidates after a save — no stale-snapshot demotion overwrite | `ee04aee` | +| WR-01 | Warning | Toggle-only saves no longer re-send `displayName`, so admin-toggle saves don't 400 for OIDC-provisioned members with a null/empty stored name | `527d855` | +| WR-02 | Warning | 409 revert uses the actual prior toggle state instead of defaulting to `true` | `527d855` | +| WR-04 | Warning | `handleClose` closure fixed — Cancel after a per-section save no longer reverts to pre-save values | `527d855` | +| WR-03 | Warning | Phone bottom-sheet gains `maxHeight` + `overflowY: auto` so the action button is reachable on short phones | `2fd253e` | +| IN-03 | Info | Phone sheet adds `env(safe-area-inset-bottom)` padding (iOS home indicator) | `2fd253e` | +| WR-05 | Warning | Admin toggle gains `aria-describedby` linking to the last-admin error region | `d2e9862` | +| IN-01 | Info | Helper text shown when display name is empty | `400733f` | +| IN-02 | Info | `maxLength` added to display-name and username inputs | `182ba1d` | + +(`41a4fae` — prettier formatting of the updated `admin.test.ts`.) + +## Verification + +- `pnpm -r typecheck` — pass (API + PWA) +- ESLint — 0 warnings +- Prettier — clean +- API integration tests — **462/462** (includes a new Test H asserting empty-body PATCH → 400) + +## Introduced during fixes (caught by iteration-2 re-review) + +- **IN-04 (Info, open):** The no-op profile-save path (`mutationFn` returns early on an + empty payload) still triggers `onSuccess`, so the "Profile saved." toast fires and the + `['admin','members']` query refetches even when nothing changed. Cosmetic. Fix: skip the + toast/refetch when the computed payload is empty. Not auto-applied — re-review returned + `status: clean` (no critical/warning), so the `--auto` loop exited before a third pass. diff --git a/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.iter2.md b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.iter2.md new file mode 100644 index 0000000..8a85f97 --- /dev/null +++ b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.iter2.md @@ -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`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 && ( +
A display name is required before saving.
+)} +``` + +--- + +### 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 `
` 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 +