---
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_