docs(phase-20): complete phase execution — verification passed (9/9), advisory review
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Lucas Berger
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
80fd683877
commit
dec8220da3
@@ -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<number>`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 `<div id="profile-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 `<button>` element:
|
||||
|
||||
```tsx
|
||||
<button
|
||||
type="button"
|
||||
role="switch"
|
||||
aria-checked={isAdmin}
|
||||
aria-label="Admin"
|
||||
aria-describedby={profileError ? 'profile-error' : undefined}
|
||||
onClick={...}
|
||||
...
|
||||
>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `PATCH /api/admin/members/:id` returns 200 when no fields are supplied (empty update)
|
||||
|
||||
**File:** `apps/api/src/routes/admin.ts:262-275`
|
||||
|
||||
**Issue:** `updateMemberSchema` marks both `displayName` and `isAdmin` as optional. If the client sends `{}`, then `updates` is an empty object and `db.update(users).set({}).where(...)` is called with no column changes. Drizzle and mysql2 permit this and issue a no-op UPDATE (0 rows changed), returning `{ ok: true }` with 200. This is harmless but semantically wrong — the server should return 400 if neither field is present. The client (`updateMemberProfile` in client.ts) always sends at least one field today, so this does not cause a user-visible bug. It is a latent issue for any future caller.
|
||||
|
||||
**Fix:** Add a Zod refinement or pre-flight check:
|
||||
|
||||
```typescript
|
||||
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 (displayName or isAdmin) must be provided',
|
||||
});
|
||||
```
|
||||
|
||||
### IN-02: Create mode does not validate displayName max length client-side
|
||||
|
||||
**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:345-349`
|
||||
|
||||
**Issue:** The create-mode submit fires `fetchCreateMember` with `displayName: createDisplayName.trim()`. The server schema enforces `max(256)`, but the client has no `maxLength` attribute on the input (line 793-799) and no client-side length check. A user who types a very long name will get a generic 400 from the server with no useful copy. The edit-mode "Save" button is already disabled when displayName is empty, but neither mode caps the length visually or in validation.
|
||||
|
||||
**Fix:** Add `maxLength={256}` to the create-mode display-name input and the edit-mode display-name input, and optionally add a client-side length check before calling the API.
|
||||
|
||||
### IN-03: Phone bottom-sheet does not account for `safe-area-inset-bottom` (notch/home-indicator)
|
||||
|
||||
**File:** `apps/pwa/src/components/MemberEditorSheet.tsx:380-392`
|
||||
|
||||
**Issue:** The phone sheet style places the sheet at `bottom: 0` with no `paddingBottom` for the iOS home-indicator or Android gesture bar. On notched devices the bottom padding/safe-area is missing, so the Cancel and Save buttons may be hidden behind the system gesture bar. The existing `AdminPage` scroll container accounts for this via `calc(56px + env(safe-area-inset-bottom, 0px))` (line 269), but the sheet itself does not.
|
||||
|
||||
**Fix:** Add bottom padding that respects the safe-area-inset:
|
||||
|
||||
```typescript
|
||||
// phone sheet style:
|
||||
{
|
||||
position: 'fixed',
|
||||
bottom: 0,
|
||||
left: 0,
|
||||
right: 0,
|
||||
paddingBottom: 'calc(var(--space-6, 24px) + env(safe-area-inset-bottom, 0px))',
|
||||
// ... rest unchanged
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-18_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user