diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 69e59c8..a973106 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -453,7 +453,7 @@ Plans: | 17. UI Optimization & Polish | v1.1 | 6/6 | Complete | 2026-06-18 | | 18. Auto Timezone Detection | v1.1 | 4/4 | Complete | 2026-06-14 | | 19. Local Auth (No-OIDC Mode) | v1.1 | 5/5 | Complete | 2026-06-17 | -| 20. Admin Member Editor & Declutter | v1.1 | 3/3 | Complete | 2026-06-18 | +| 20. Admin Member Editor & Declutter | v1.1 | 3/3 | Complete | 2026-06-18 | ## Backlog diff --git a/.planning/STATE.md b/.planning/STATE.md index 4678c4c..f8287fa 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,13 +2,13 @@ gsd_state_version: 1.0 milestone: v1.1 milestone_name: Operability & Polish -current_phase: 20 -current_phase_name: admin-member-editor-form-declutter +current_phase: 999.1 +current_phase_name: BACKLOG status: executing stopped_at: Phase 20 UI-SPEC approved -last_updated: "2026-06-18T21:40:44.719Z" +last_updated: "2026-06-18T21:47:42.443Z" last_activity: 2026-06-18 -last_activity_desc: Phase 20 execution started +last_activity_desc: Phase 20 complete, transitioned to Phase 999.1 progress: total_phases: 27 completed_phases: 13 @@ -28,10 +28,10 @@ See: .planning/PROJECT.md (updated 2026-06-16) ## Current Position -Phase: 20 (admin-member-editor-form-declutter) — EXECUTING -Plan: 2 of 3 +Phase: 999.1 — Treat Fastmail as one calendar provider; framework supports adding more providers (BACKLOG) +Plan: Not started Status: Ready to execute -Last activity: 2026-06-18 — Phase 20 execution started +Last activity: 2026-06-18 — Phase 20 complete, transitioned to Phase 999.1 ### ✅ Resolved Checkpoint — Phase 15 Plan 15-03 Task 2 (human-action) @@ -41,7 +41,7 @@ Done 2026-06-12. Gitea branch protection on `main` now requires EXACTLY `CI / fa **Velocity:** -- Total plans completed: 66 +- Total plans completed: 69 - Average duration: - - Total execution time: 0 hours @@ -62,6 +62,7 @@ Done 2026-06-12. Gitea branch protection on `main` now requires EXACTLY `CI / fa | 12 | 7 | - | - | | 19 | 5 | - | - | | 17 | 6 | - | - | +| 20 | 3 | - | - | **Recent Trend:** 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 new file mode 100644 index 0000000..af360d3 --- /dev/null +++ b/.planning/phases/20-admin-member-editor-form-declutter/20-REVIEW.md @@ -0,0 +1,289 @@ +--- +phase: 20-admin-member-editor-form-declutter +reviewed: 2026-06-18T00:00:00Z +depth: standard +files_reviewed: 4 +files_reviewed_list: + - apps/api/src/routes/admin.ts + - apps/pwa/src/api/client.ts + - apps/pwa/src/components/MemberEditorSheet.tsx + - apps/pwa/src/routes/AdminPage.tsx +findings: + critical: 1 + warning: 5 + info: 3 + total: 9 +status: issues_found +--- + +# Phase 20: Code Review Report + +**Reviewed:** 2026-06-18 +**Depth:** standard +**Files Reviewed:** 4 +**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. + +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. + +--- + +## Critical Issues + +### CR-01: Last-admin demotion guard is a TOCTOU race + +**File:** `apps/api/src/routes/admin.ts:251-258` + +**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. + +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 +``` + +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: + +```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 + ; + + if (!target) throw new Error('not-found'); + + if (isAdmin === false && target.isAdmin) { + const [{ count }] = await tx + .select({ count: sql`COUNT(*)` }) + .from(users) + .where(eq(users.isAdmin, true)); + 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)); +}); +``` + +Or use an atomic conditional UPDATE to avoid the SELECT entirely: + +```sql +UPDATE users +SET is_admin = false +WHERE id = :targetId + AND (SELECT COUNT(*) FROM users WHERE is_admin = true) > 1 +``` + +Check `affectedRows === 0` to detect the guard firing without a round-trip. + +--- + +## Warnings + +### WR-01: Profile save always sends displayName even when only isAdmin changed + +**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:262-265` + +**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. + +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. + +**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: + +```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); +``` + +### WR-02: D-03 error revert uses stale fallback when member prop is undefined + +**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. + +**Fix:** Use the value that was stable at mutation start: + +```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 + ... + }, +}); +``` + +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 + +**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:379-393` + +**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. + +**Fix:** Add a `maxHeight` and `overflowY: 'auto'` to the phone style: + +```typescript +// phone sheet style: +{ + position: 'fixed', + bottom: 0, + left: 0, + right: 0, + maxHeight: 'calc(85dvh)', // or 'calc(100dvh - env(safe-area-inset-top, 0px))' + overflowY: 'auto', + background: 'var(--color-surface)', + borderRadius: '12px 12px 0 0', + ... +} +``` + +### WR-04: MemberEditorSheet `handleClose` dependency on `member` causes stale-close on concurrent edits + +**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: + +```typescript +const handleClose = useCallback(() => { + // Only reset fields that don't correspond to member data (passwords, app pw) + 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: `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` + +**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. + +**Fix:** Add `aria-describedby="profile-error"` to the toggle `