docs(17): add code review report
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
ea49a4dc83
commit
9730d3dcdb
@@ -0,0 +1,145 @@
|
||||
---
|
||||
phase: 17-ui-optimization-polish
|
||||
reviewed: 2026-06-18T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 13
|
||||
files_reviewed_list:
|
||||
- apps/pwa/src/App.tsx
|
||||
- apps/pwa/src/components/BrandSlot.tsx
|
||||
- apps/pwa/src/components/CalendarShell.tsx
|
||||
- apps/pwa/src/components/CredentialSheet.tsx
|
||||
- apps/pwa/src/components/SettingsSheet.tsx
|
||||
- apps/pwa/src/components/InstructionSheet.test.tsx
|
||||
- apps/pwa/src/routes/AdminPage.tsx
|
||||
- apps/pwa/src/styles/tokens.css
|
||||
- apps/pwa/index.html
|
||||
- apps/pwa/vite.config.ts
|
||||
- apps/pwa/pwa-assets.config.ts
|
||||
- apps/pwa/e2e/admin.spec.ts
|
||||
- apps/pwa/e2e/layout.spec.ts
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 6
|
||||
info: 7
|
||||
total: 13
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 17: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-18
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 13
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 17 is a UI optimization/polish pass: themeable `tokens.css` layer with a `--bottom-chrome-h` chrome token, brand logo via `BrandSlot`, desktop-centered vs phone bottom-sheet branches across the credential/settings/admin sheets, a Sign-out control, admin success toasts, and a two-tab ARIA strip on `/admin`. The intentional design decisions (amber `--color-member-0`, single shipped light theme, `logo.svg`) were treated as approved and not flagged.
|
||||
|
||||
No BLOCKER-class correctness or security defects were found — the password fields use `new-password` autocomplete and are never pre-filled, copy is plain-text JSX (no `dangerouslySetInnerHTML`), and the external links carry `rel="noopener noreferrer"`. The substantive findings are a cluster of accessibility and robustness regressions introduced by the new desktop-modal branches and the responsive `isPhone()` snapshots, plus a stale test mock that no longer matches the API contract. None are data-loss or auth-bypass risks, so all land at WARNING/INFO.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Dialog `phone` branch is a render-time snapshot — resize/rotation desyncs modal vs bottom-sheet layout
|
||||
|
||||
**File:** `apps/pwa/src/components/SettingsSheet.tsx:135`, `apps/pwa/src/components/CredentialSheet.tsx:156`, `apps/pwa/src/routes/AdminPage.tsx:59,1378`
|
||||
**Issue:** Every new desktop-centered-vs-bottom-sheet branch computes `const phone = window.matchMedia('(max-width: 767px)').matches;` once during render with no `matchMedia` change listener and no resize subscription. `App.tsx` (`isPhone()` at line 64/84) and `CalendarShell.tsx` (line 74/252) have the same pattern. On a viewport resize or device rotation that crosses the 767px breakpoint, the component does not re-render, so the modal keeps its stale geometry: a desktop window narrowed below 768px keeps a centered modal (correct target is a bottom sheet) and vice-versa. The FAB offset and `paddingBottom: var(--bottom-chrome-h)` in `App.tsx` also stay on the wrong branch until an unrelated state change forces a re-render. This is a real layout regression on tablets/foldables and desktop-window-resize, which the phase explicitly set out to fix.
|
||||
**Fix:** Centralize a `useIsPhone()` hook that subscribes to the media query and triggers re-render:
|
||||
```ts
|
||||
function useIsPhone() {
|
||||
const [phone, setPhone] = useState(
|
||||
() => typeof window !== 'undefined' && window.matchMedia('(max-width: 767px)').matches,
|
||||
);
|
||||
useEffect(() => {
|
||||
const mq = window.matchMedia('(max-width: 767px)');
|
||||
const onChange = (e: MediaQueryListEvent) => setPhone(e.matches);
|
||||
mq.addEventListener('change', onChange);
|
||||
return () => mq.removeEventListener('change', onChange);
|
||||
}, []);
|
||||
return phone;
|
||||
}
|
||||
```
|
||||
Use it in App.tsx, CalendarShell, and all four sheet variants instead of the one-shot snapshot.
|
||||
|
||||
### WR-02: New desktop modals have no focus trap and do not restore focus — keyboard/AT regression
|
||||
|
||||
**File:** `apps/pwa/src/components/SettingsSheet.tsx:199` (ChangePasswordSheet `:667`, LinkOidcSheet `:979`), `apps/pwa/src/routes/AdminPage.tsx:1420` (ResetPasswordSheet)
|
||||
**Issue:** All of these are `role="dialog" aria-modal="true"` but nothing constrains Tab focus inside the dialog — pressing Tab walks into the page content behind the backdrop. `aria-modal="true"` is a promise to AT that the rest of the page is inert; without a focus trap (and the backgrounded content lacks `inert`/`aria-hidden`), screen-reader and keyboard users can land on occluded controls. Additionally, `ChangePasswordSheet` and `LinkOidcSheet` (opened from `SettingsSheet`) never return focus to the row button that opened them on close — `handleClose`/`onClose` just unmount. `CredentialSheet` and `ResetPasswordSheet` do restore focus via `triggerRef`, so the omission in the SettingsSheet sub-sheets is an inconsistent regression. `SettingsSheet` itself also only focuses the close button on open but never restores focus to the avatar trigger on close.
|
||||
**Fix:** Add a shared focus-trap (cycle Tab/Shift+Tab within the dialog, e.g. via a `useFocusTrap(ref)` hook) and capture/restore `document.activeElement` on open/close for every dialog. At minimum, give `ChangePasswordSheet`/`LinkOidcSheet` a `triggerRef` and refocus it on close to match the established CredentialSheet pattern.
|
||||
|
||||
### WR-03: Modal dialogs do not lock background scroll — desktop centered modal scrolls the page behind it
|
||||
|
||||
**File:** `apps/pwa/src/components/SettingsSheet.tsx:199`, `apps/pwa/src/components/CredentialSheet.tsx:173`, `apps/pwa/src/routes/AdminPage.tsx:1420`
|
||||
**Issue:** When a centered desktop modal (or bottom sheet) is open, scrolling with wheel/trackpad scrolls the document body behind the backdrop because no scroll lock (`overflow: hidden` on `body`/root, or `overscroll-behavior`) is applied while the dialog is mounted. On `/admin` the content column is long (member form + calendar picker + timezone combobox), so this is reachable in normal use: opening the credential sheet and scrolling moves the admin page underneath. This undermines the "modal" contract and the polish goal.
|
||||
**Fix:** While any dialog is open, set `document.body.style.overflow = 'hidden'` in a `useEffect` and restore on unmount. Centralize this with the dialog/focus-trap helper so all sheets get consistent behavior.
|
||||
|
||||
### WR-04: `InstructionSheet.test.tsx` mock is stale — `fetchLinkOidc` shape no longer matches the real contract
|
||||
|
||||
**File:** `apps/pwa/src/components/InstructionSheet.test.tsx:43`
|
||||
**Issue:** The mock declares `fetchLinkOidc: vi.fn().mockResolvedValue({ redirectUrl: '/oidc' })`, but the real `fetchLinkOidc` (api/client.ts:247) resolves `{ signedState, authorizationUrl }`, and `LinkOidcSheet` reads `data.authorizationUrl` (SettingsSheet.tsx:947). The mock returns the wrong key entirely. The test happens to pass today only because permission is `'denied'` (`hasLocalCredential:false` keeps the OIDC row from rendering, so the mutation is never exercised), but the mock encodes an incorrect contract and will silently mask a real regression if the test surface expands to the link flow. The mock also omits `fetchLocalLogout` and `fetchAdminResetPassword`, which `SettingsSheet`/`AdminPage` import — fine for this isolated render, but the file's "mock client so the test doesn't make real network calls" claim is only partial.
|
||||
**Fix:** Update the mock to the real shape: `fetchLinkOidc: vi.fn().mockResolvedValue({ signedState: 's', authorizationUrl: '/oidc' })`, and add `fetchLocalLogout: vi.fn().mockResolvedValue(undefined)` so future expansion of the suite does not hit `undefined is not a function`.
|
||||
|
||||
### WR-05: Timezone combobox blur timer can fire after unmount / tab switch — setState on stale closure
|
||||
|
||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:104,859-865`
|
||||
**Issue:** `onBlur` schedules `tzBlurTimer.current = setTimeout(() => { setTzOpen(false); setTzSearch(null); }, 120)`. This timer is cleared in `selectTimezone` and `onFocus`, but there is no cleanup `useEffect` that clears it on unmount. If the admin blurs the timezone field and then immediately switches tabs or the AdminPage unmounts within 120ms, the timer fires `setTzOpen`/`setTzSearch` on an unmounted component (React warns, and in strict mode this is a leak). The two-tab strip uses `hidden` (the Settings panel stays mounted), which reduces but does not eliminate the unmount path (navigating away from `/admin`).
|
||||
**Fix:** Add a cleanup effect:
|
||||
```ts
|
||||
useEffect(() => () => { if (tzBlurTimer.current) clearTimeout(tzBlurTimer.current); }, []);
|
||||
```
|
||||
|
||||
### WR-06: `LinkOidcSheet` "Continue with OIDC" stays enabled after success during the redirect window
|
||||
|
||||
**File:** `apps/pwa/src/components/SettingsSheet.tsx:942-958,1100-1125`
|
||||
**Issue:** On `onSuccess` the mutation sets `window.location.href = data.authorizationUrl` and calls `onClose()`. The button is disabled only while `linkMutation.isPending`; once the mutation settles (success), `isPending` is false again. Between `onSuccess` firing and the browser actually committing the top-level navigation, the sheet is closed so this is largely shielded — but if `authorizationUrl` is null the sheet stays open with the button re-enabled and a generic error, which is correct. The latent issue is the success path: there is no guard preventing a second `mutate()` if the navigation is slow and the user re-taps before unload. A double POST to `/api/me/link-oidc` would mint a second `signedState`, and (as the App.tsx OIDC-state-cookie comment at CalendarShell.tsx:110-120 documents) concurrent state issuance is exactly the class of bug that produces `OAUTH_INVALID_RESPONSE`.
|
||||
**Fix:** Keep the button disabled through the redirect: track a local `redirecting` flag set in `onSuccess` before assigning `window.location.href`, and include it in the `disabled` expression (`disabled={linkMutation.isPending || redirecting}`).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Decorative logo gives screen readers no accessible product mark, relies solely on `<h1>` text
|
||||
|
||||
**File:** `apps/pwa/src/components/BrandSlot.tsx:26-40`
|
||||
**Issue:** The logo `<img>` is `alt="" aria-hidden="true"` (correct for purely decorative), and the `<h1>FamilySync</h1>` carries the name — so this is acceptable. Noting only that the comment claims the SVG "draws its own rounded-square background (rx=104)" and `--brand-logo-border-radius: 0`; `logo.svg` confirms `rx=104`, so no clipping bug. No action required; documented for completeness.
|
||||
**Fix:** None — works as designed.
|
||||
|
||||
### IN-02: `--brand-logo-bg` / `--brand-logo-text` tokens are now dead
|
||||
|
||||
**File:** `apps/pwa/src/styles/tokens.css:103-104`
|
||||
**Issue:** `BrandSlot` was rewritten to render `logo.svg`; the placeholder-circle background/initials tokens (`--brand-logo-bg`, `--brand-logo-text`) are no longer referenced by any component. They linger as dead design tokens.
|
||||
**Fix:** Remove the two unused tokens, or add a comment that they are retained intentionally for a future fallback.
|
||||
|
||||
### IN-03: `CalendarShell` error-Retry uses `refetchQueries({ queryKey: ['events'] })` without `start`/`end`
|
||||
|
||||
**File:** `apps/pwa/src/components/CalendarShell.tsx:362`
|
||||
**Issue:** The Retry handler refetches the broad `['events']` prefix rather than the active `['events', start, end]`. This is harmless (it refetches all matching, including the active window) but slightly over-broad; consistent with the AdminPage shared-calendar invalidation pattern. Out of scope as a perf concern; flagged only as a minor consistency note.
|
||||
**Fix:** Optionally narrow to `['events', start, end]` to match the active query key.
|
||||
|
||||
### IN-04: `AdminPage` `phone` toast offset is a render snapshot (same root cause as WR-01)
|
||||
|
||||
**File:** `apps/pwa/src/routes/AdminPage.tsx:59,1035`
|
||||
**Issue:** The success-toast bottom offset chooses `var(--bottom-chrome-h)` vs `var(--space-6)` based on the one-shot `phone` snapshot. A resize across 767px while a toast is visible would misplace it. Low impact (toast auto-dismisses in 3s) — folded into WR-01's recommended `useIsPhone()` fix.
|
||||
**Fix:** Covered by WR-01.
|
||||
|
||||
### IN-05: Magic z-index ladder duplicated across every sheet
|
||||
|
||||
**File:** `apps/pwa/src/components/SettingsSheet.tsx` (300/301/302/303), `CredentialSheet.tsx` (300/301), `AdminPage.tsx` (300/301)
|
||||
**Issue:** Backdrop/sheet z-index values (300/301/302/303) are hard-coded literals repeated across all dialogs. The nested ChangePassword/LinkOidc sheets at 302/303 correctly stack above the parent SettingsSheet at 300/301, so there is no current layering bug — but the magic numbers are duplicated and easy to desync.
|
||||
**Fix:** Promote to tokens (e.g. `--z-backdrop`, `--z-sheet`, `--z-sheet-nested`) in tokens.css.
|
||||
|
||||
### IN-06: `eslint-disable react-hooks/exhaustive-deps` on Escape-listener effects hides a real omission
|
||||
|
||||
**File:** `apps/pwa/src/components/CredentialSheet.tsx:95`, `apps/pwa/src/components/SettingsSheet.tsx:605` (ChangePasswordSheet)
|
||||
**Issue:** The Escape-key effect calls `handleClose` but disables exhaustive-deps and depends only on `[isOpen]`. `handleClose` is redefined every render, so the listener closes over the first render's `handleClose`. It works because `handleClose` only touches setState (stable) and `onClose`/`triggerRef`, but the disable comment masks the fact that a future `handleClose` change won't propagate. Sibling components (`LinkOidcSheet:934`, `ResetPasswordSheet:1361`) correctly depend on `[isOpen, onClose]` without the disable — inconsistent.
|
||||
**Fix:** Wrap `handleClose` in `useCallback` and add it to the dependency array, removing the disable comment, to match the other sheets.
|
||||
|
||||
### IN-07: `OidcRedirect` performs `window.location.replace` during render (side effect in render body)
|
||||
|
||||
**File:** `apps/pwa/src/App.tsx:77-80`
|
||||
**Issue:** `OidcRedirect` calls `window.location.replace('/api/login')` directly in the function body rather than in a `useEffect`. The inline comment acknowledges this ("useEffect isn't available here; use a helper element"). In practice it fires once and the browser navigates away, but a render-phase navigation side effect is a React anti-pattern (double-invoked in StrictMode dev, and not safe if React bails the render). It works today because the navigation unmounts everything.
|
||||
**Fix:** Move the `replace` into a `useEffect(() => { window.location.replace('/api/login'); }, [])` inside the helper component.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-18_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user