diff --git a/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md index af360d3..8a85f97 100644 --- a/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md +++ b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md @@ -1,76 +1,89 @@ --- phase: 20-admin-member-editor-form-declutter -reviewed: 2026-06-18T00:00:00Z -depth: standard -files_reviewed: 4 +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: 1 - warning: 5 + critical: 2 + warning: 6 info: 3 - total: 9 + total: 11 status: issues_found --- -# Phase 20: Code Review Report +# Phase 20: Code Review Report (Deep Re-Review) **Reviewed:** 2026-06-18 -**Depth:** standard -**Files Reviewed:** 4 +**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), wires a unified `MemberEditorSheet` (edit/create modes), and declutters the `AdminPage` member list by replacing the always-open add form and per-row action buttons with a single tappable row + bottom trigger. +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. -The authorization boundary is sound: `requireAdmin` does a live DB lookup on every request, mounted first on the router, and the new route inherits that correctly. Zod validation is applied consistently. The last-admin guard logic has one correctness defect (TOCTOU race — classified Critical) and several warning-level issues in the frontend. +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 TOCTOU race +### CR-01: Last-admin demotion guard is a non-atomic TOCTOU race -**File:** `apps/api/src/routes/admin.ts:251-258` +**File:** `apps/api/src/routes/admin.ts:251-267` -**Issue:** The D-03 last-admin guard does a `SELECT COUNT(*)` then, if count > 1, proceeds to `UPDATE`. Between the COUNT and the UPDATE another concurrent request could demote the other admin simultaneously, leaving the system with zero admins. Both concurrent PATCH requests read count=2, both pass the guard, and both updates succeed — violating the "at least one admin must remain" invariant. +**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. -The guard logic is: ``` -SELECT COUNT(*) WHERE is_admin=true → 2 - (concurrent PATCH2 also reads 2) -UPDATE SET is_admin=false WHERE id=X → ok - (concurrent PATCH2 also runs) -UPDATE SET is_admin=false WHERE id=Y → ok, now 0 admins +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) ``` -For a two-person household the window is narrow but the invariant is still breakable under network retry or any concurrent admin sessions. - -**Fix:** Wrap the existence check, COUNT, and UPDATE in a single transaction and use `SELECT ... FOR UPDATE` to serialize the guard: +**Fix:** Wrap the full guard-plus-update in a transaction and use a locking read: ```typescript await db.transaction(async (tx) => { - // Lock the target row const [target] = await tx .select({ id: users.id, isAdmin: users.isAdmin }) .from(users) .where(eq(users.id, targetId)) - .limit(1) - // Drizzle/mysql2: append raw FOR UPDATE via sql suffix or use db.execute - // Alternative: use an atomic conditional UPDATE described below - ; - + .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'); } @@ -81,121 +94,220 @@ await db.transaction(async (tx) => { }); ``` -Or use an atomic conditional UPDATE to avoid the SELECT entirely: +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 WHERE is_admin = true) > 1 + AND (SELECT COUNT(*) FROM users u2 WHERE u2.is_admin = true) > 1 ``` -Check `affectedRows === 0` to detect the guard firing without a round-trip. +--- + +### 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 +### 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` +**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:262-265, 568` -**Issue:** `profileMutation.mutationFn` unconditionally sends `{ displayName: displayName.trim(), isAdmin }` to the server. If the admin opens the sheet only to toggle the admin flag — touching nothing else — the current `displayName` (already persisted) is re-written. This is benign under normal conditions, but there is a subtle edge: the server's `updateMemberSchema` requires `displayName` to be `min(1)` when present. If a member somehow has an empty displayName in the DB and the editor initialises to `''`, the profile save fires a 400 because the trimmed displayName fails `min(1)` validation — even though the admin only wanted to change the admin flag. The UI provides no feedback for this case: the mutation fails with the generic "Something went wrong" copy, and the displayName state in the editor does not indicate an error because `profileError` is set but `inputStyle(!!profileError)` correctly highlights the field — however the user has no idea why touching only the toggle caused a validation failure. +**Issue:** `profileMutation.mutationFn` always sends `{ displayName: displayName.trim(), isAdmin }`. +Two distinct problems follow: -A more specific problem: the "Save" button is disabled when `displayName.trim().length === 0` (line 568), which prevents the user from saving. But there is no guard that prevents opening the editor for a member with a null/empty displayName and immediately being stuck — the button is disabled with no explanation. +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. -**Fix:** Either (a) send only the changed fields (track `dirtyDisplayName` and `dirtyIsAdmin` flags), or (b) show an explicit validation error on the displayName field before the user even attempts to 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 -// Option A: only include fields that changed -const payload: { displayName?: string; isAdmin?: boolean } = {}; -if (displayName.trim() !== (member.displayName ?? '')) { - payload.displayName = displayName.trim(); -} -if (isAdmin !== member.isAdmin) { - payload.isAdmin = isAdmin; -} -if (Object.keys(payload).length === 0) return; // nothing to update -await updateMemberProfile(member.id, payload); +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); +}, ``` -### WR-02: D-03 error revert uses stale fallback when member prop is undefined +**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 a 409 (last-admin guard), the `onError` handler reverts `isAdmin` to `member?.isAdmin ?? true`. The `?? true` default means that if `member` is somehow `undefined` at the time the error fires (e.g., the parent closes the sheet mid-flight), the revert silently sets `isAdmin` to `true` rather than the correct prior value. More practically: the mutation already guards `if (!member) throw new Error('no-member')` at line 261, so a missing `member` in `onError` is the result of a `no-member` throw, not a 409. The 409 path always has `member` available via the closure. However the `?? true` default is semantically wrong — if it ever fires it could reverse the toggle into an incorrect state and mislead the user. +**Issue:** When the server returns 409 (last-admin guard), `onError` reverts the toggle with: -**Fix:** Use the value that was stable at mutation start: +```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'); - const previousIsAdmin = member.isAdmin; // captured at call time await updateMemberProfile(member.id, { displayName: displayName.trim(), isAdmin }); - return { previousIsAdmin }; }, - onError: (err, _vars, context) => { - // context.previousIsAdmin is the value at mutation start - ... + 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.'); + } }, }); ``` -Alternatively, capture `member.isAdmin` at the time the button is clicked and pass it through mutation context. +--- -### WR-03: Phone bottom-sheet has no max-height / scroll, content can overflow on tall forms +### WR-03: Phone bottom-sheet lacks `maxHeight`/`overflowY` — action buttons unreachable on short phones -**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:379-393` +**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:379-392` -**Issue:** The desktop sheet style (lines 399-407) correctly applies `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 + password + app password), the sheet content can easily exceed the viewport height — on a short phone (SE-sized) it will push content off screen with no scroll affordance, making the "Save app password" button unreachable. The UI-SPEC §Surface B does not specify a phone max-height, but the same overflow scenario exists. +**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:** Add a `maxHeight` and `overflowY: 'auto'` to the phone style: +**Fix:** ```typescript -// phone sheet style: +// phone branch of sheetStyle: { position: 'fixed', bottom: 0, left: 0, right: 0, - maxHeight: 'calc(85dvh)', // or 'calc(100dvh - env(safe-area-inset-top, 0px))' + 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)', } ``` -### WR-04: MemberEditorSheet `handleClose` dependency on `member` causes stale-close on concurrent edits +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 with `useCallback` and depends on `[onClose, triggerRef, member?.displayName, member?.isAdmin]`. The closure captures `member` at memoisation time. When a section save succeeds, `queryClient.invalidateQueries` triggers a refetch, `membersQuery.data` updates, and `AdminPage` may pass a new `member` object to the sheet (with updated `displayName`/`isAdmin`). At that point `handleClose` is still holding the old captured values for the form reset lines: - -```typescript -setDisplayName(member?.displayName ?? ''); // uses stale member -setIsAdmin(member?.isAdmin ?? false); -``` - -If the user saves the Profile section (updating displayName), then clicks Cancel, the form resets to the pre-save value rather than the now-persisted value. This is a visual inconsistency: the field resets to outdated data. The useEffect at line 234 (syncing state on `member?.id` change) does handle re-sync when a different row is opened — but not when the same row's data refreshes while the sheet is open. - -**Fix:** Either remove the reset logic from `handleClose` (instead rely solely on the useEffect sync), or widen the effect's dependency to also fire when `member?.displayName` or `member?.isAdmin` changes while the sheet is open: - -```typescript -useEffect(() => { - // Re-sync whenever the member data changes (including after a save + refetch) - setDisplayName(member?.displayName ?? ''); - setIsAdmin(member?.isAdmin ?? false); - setProfileError(null); -}, [member?.id, member?.displayName, member?.isAdmin]); -``` - -The `useEffect` already has these dependencies (line 238) — so the sync is correct on open and on member-change. The remaining problem is only that `handleClose` also does a redundant reset using the stale closure. The simplest fix is to remove the redundant reset lines from `handleClose` and let the effect own that state: +**Issue:** `handleClose` is memoised: ```typescript const handleClose = useCallback(() => { - // Only reset fields that don't correspond to member data (passwords, app pw) + 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(''); @@ -213,13 +325,20 @@ const handleClose = useCallback(() => { }, [onClose, triggerRef]); ``` -### WR-05: `aria-describedby` on profile display-name input points to an error id that may not be in DOM +--- -**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:479` +### WR-05: Admin toggle missing `aria-describedby` for the last-admin error -**Issue:** The display-name input has `aria-describedby={profileError ? 'profile-error' : undefined}`. The error `
` is rendered at line 557, which is _after_ the Section 1 save button. Screen readers resolving `aria-describedby` expect the referenced element to exist in the DOM at time of announcement — this is fine because the error node is in the same DOM tree. However there is a second problem: the `profileError` div is rendered below the action buttons (lines 556-559), not directly adjacent to the field it describes. WCAG 1.3.1 expects that error associations are explicit. More critically: the same `profile-error` id is also described by both the displayName input (line 479) and the admin toggle button (implicitly — it is the only error output for the section, but the toggle has no `aria-describedby`). The admin toggle button (line 519) has `aria-label="Admin"` but no `aria-describedby="profile-error"`, so a screen-reader user who activates the toggle and receives the last-admin error won't have it announced. +**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:515-537` -**Fix:** Add `aria-describedby="profile-error"` to the toggle `