168 lines
15 KiB
Markdown
168 lines
15 KiB
Markdown
---
|
|
phase: 17-ui-optimization-polish
|
|
reviewed: 2026-06-18T00:00:00Z
|
|
depth: deep
|
|
files_reviewed: 13
|
|
files_reviewed_list:
|
|
- apps/pwa/e2e/admin.spec.ts
|
|
- apps/pwa/e2e/layout.spec.ts
|
|
- apps/pwa/index.html
|
|
- apps/pwa/package.json
|
|
- apps/pwa/pwa-assets.config.ts
|
|
- 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/routes/AdminPage.tsx
|
|
- apps/pwa/src/styles/tokens.css
|
|
- apps/pwa/vite.config.ts
|
|
findings:
|
|
critical: 0
|
|
warning: 8
|
|
info: 7
|
|
total: 15
|
|
status: issues_found
|
|
---
|
|
|
|
# Phase 17: Code Review Report
|
|
|
|
**Reviewed:** 2026-06-18T00:00:00Z
|
|
**Depth:** deep
|
|
**Files Reviewed:** 13
|
|
**Status:** issues_found
|
|
|
|
## Summary
|
|
|
|
Phase 17 is UI optimization/polish: brand logo swap, PWA manifest/icon hand-maintenance, an admin two-tab ARIA strip, a success toast, and a searchable timezone combobox, plus structural layout/admin Playwright suites. No structural-findings pre-pass was provided.
|
|
|
|
The code is generally careful — XSS surfaces are plain-text JSX, password fields use `new-password` autocomplete and are never pre-filled, the OIDC `authorizationUrl` null is guarded before navigation, and touch targets are consistently ≥44px. I found **no BLOCKERs** (no injection, no secret leakage, no data-loss path, no crash on the happy path).
|
|
|
|
There are real correctness/robustness defects worth fixing before ship: the **admin tab keyboard handler half-implements the WAI-ARIA tabs pattern** (no Home/End, no wrap); the **timezone combobox `aria-activedescendant`/highlight can desync after filtering** and **drops Tab-to-commit**; the **success toast does not re-announce** repeated identical messages and its `whiteSpace: nowrap` is a latent Rule-2 horizontal-overflow hazard against the project's own layout suite; **none of the modal dialogs trap focus** despite `aria-modal="true"`; and several `window.matchMedia` reads at render time **do not react to resize/orientation**, a stale-UI class this project explicitly cares about (iPad rotation).
|
|
|
|
## Warnings
|
|
|
|
### WR-01: Modal dialogs declare `aria-modal="true"` but do not trap focus
|
|
|
|
**File:** `apps/pwa/src/components/CredentialSheet.tsx:173-176`, `apps/pwa/src/components/SettingsSheet.tsx:199-202` (plus ChangePasswordSheet ~667-701 and LinkOidcSheet ~979-1013), `apps/pwa/src/routes/AdminPage.tsx:1420-1423` (ResetPasswordSheet)
|
|
**Issue:** Every sheet sets `role="dialog"` + `aria-modal="true"` and focuses the heading/close button on open, but none implements a focus trap. Tab/Shift-Tab can move focus out of the dialog to content behind the backdrop (still in the DOM). `aria-modal="true"` asserts to assistive tech that focus is contained — it is not. The app's stated UX hard-constraint is "slick and low-friction for a non-technical Apple member"; VoiceOver/keyboard users will escape the dialog silently and interact with occluded background controls.
|
|
**Fix:** Add a focus trap — capture Tab/Shift-Tab in the dialog keydown handler and cycle between first/last focusable descendants, ideally as a shared `useFocusTrap(ref)` hook reused by all sheets:
|
|
```tsx
|
|
onKeyDown={(e) => {
|
|
if (e.key !== 'Tab') return;
|
|
const f = dialogRef.current?.querySelectorAll<HTMLElement>(
|
|
'a[href],button:not([disabled]),input:not([disabled]),[tabindex]:not([tabindex="-1"])');
|
|
if (!f?.length) return;
|
|
const first = f[0], last = f[f.length - 1];
|
|
if (e.shiftKey && document.activeElement === first) { e.preventDefault(); last.focus(); }
|
|
else if (!e.shiftKey && document.activeElement === last) { e.preventDefault(); first.focus(); }
|
|
}}
|
|
```
|
|
|
|
### WR-02: Admin tab strip keyboard nav is incomplete (no Home/End, no explicit wrap)
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:205-225`
|
|
**Issue:** `handleTabKeyDown` handles only `ArrowRight`/`ArrowLeft`, and the next/prev computation is a two-state toggle that does not wrap (ArrowRight on the Settings tab is a no-op rather than wrapping to Members). The WAI-ARIA tabs pattern requires `Home`/`End` to jump to first/last tab. `admin.spec.ts` (lines 134-152) only exercises the Arrow keys, so this gap is untested and ships a half-pattern.
|
|
**Fix:** Handle `Home`/`End` and decide wrap behavior explicitly:
|
|
```tsx
|
|
const order = ['members', 'settings'] as const;
|
|
const idx = order.indexOf(current);
|
|
let next: typeof order[number] | null = null;
|
|
if (e.key === 'ArrowRight') next = order[(idx + 1) % order.length];
|
|
else if (e.key === 'ArrowLeft') next = order[(idx - 1 + order.length) % order.length];
|
|
else if (e.key === 'Home') next = order[0];
|
|
else if (e.key === 'End') next = order[order.length - 1];
|
|
if (next) { e.preventDefault(); setActiveTab(next); /* focus #admin-tab-${next} */ }
|
|
```
|
|
|
|
### WR-03: Success toast does not re-announce repeated identical messages
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:62-69`, `1028-1065`
|
|
**Issue:** The toast is a single `role="status" aria-live="polite"` region rendering the `toast` string. If the same message fires twice (two password resets, two "Member added.") and the second `setToast('…')` lands before the first cleared, React's state-equality short-circuit means the DOM text does not change, so `aria-live` does not re-announce — the second success is silent to screen-reader users, and the 3s auto-dismiss timer (keyed on `toast` identity) does not reset for an identical string.
|
|
**Fix:** Make each toast a distinct value and remount it so AT re-announces and the timer resets:
|
|
```tsx
|
|
const [toast, setToast] = useState<{ id: number; msg: string } | null>(null);
|
|
const show = (msg: string) => setToast({ id: Date.now(), msg });
|
|
// effect dep: [toast?.id]; render: <div key={toast.id} role="status" ...>{toast.msg}</div>
|
|
```
|
|
|
|
### WR-04: Toast `whiteSpace: nowrap` is a latent horizontal-overflow regression against Rule 2
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:1054-1055`
|
|
**Issue:** The toast sets `whiteSpace: 'nowrap'` with `maxWidth: '90vw'`. `nowrap` + `maxWidth` does not shrink text; it overflows. A longer/localized toast on a 390px viewport will exceed 90vw and, because the toast is `position: fixed`, contribute to `documentElement.scrollWidth` — violating `layout.spec.ts` Rule 2 (lines 176-200), which asserts no horizontal overflow on `/calendar` and `/lists`. Current strings are short, so the bug is latent, not active, but it is a direct hazard to the project's own quality bar.
|
|
**Fix:** Remove `whiteSpace: 'nowrap'` (let it wrap), or bound the width and use `overflow:hidden; text-overflow:ellipsis`. Wrapping is safer for a toast that may localize.
|
|
|
|
### WR-05: `matchMedia(...)` read at render time does not react to resize/orientation
|
|
|
|
**File:** `apps/pwa/src/App.tsx:64-66` (`isPhone()`), `apps/pwa/src/components/CalendarShell.tsx:74-76`, `apps/pwa/src/components/SettingsSheet.tsx:135`, `apps/pwa/src/components/CredentialSheet.tsx:156`, `apps/pwa/src/routes/AdminPage.tsx:59`, `1378-1379`
|
|
**Issue:** These components compute `phone` once per render via synchronous `matchMedia('(max-width: 767px)').matches`, with no `change` listener. Rotating an iPad across 767px (or resizing a desktop window across the breakpoint) does not trigger a re-render, so the layout (FAB vs toolbar button in `CalendarShell`, bottom-sheet vs centered modal in the sheets, content `paddingBottom` in `App`) stays stale until an unrelated state change forces a re-render. The developer profile explicitly flags resize/orientation correctness; iPad rotation is a realistic trigger for this cross-ecosystem app.
|
|
**Fix:** Use a `useMediaQuery` hook backed by `matchMedia.addEventListener('change', …)` so components re-render on breakpoint crossing; share a single `phone` value through context/hook so all call sites stay consistent.
|
|
|
|
### WR-06: Timezone combobox `aria-activedescendant`/highlight can desync after filtering
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:819-821`, `833-837`, `839-845`, `912-919`
|
|
**Issue:** `onChange` resets `tzActiveIndex` to 0 while `ArrowDown` clamps against `filteredZones.length - 1` from the *current render closure*. With batched updates, interleavings exist where `tzActiveIndex` (and thus `aria-activedescendant={tz-opt-${tzActiveIndex}}`, line 820) references an option index that no longer exists after the filtered list shrinks (e.g., active 12, then a keystroke filters to 3 rows before re-clamp). Separately, the visual highlight uses `i === tzActiveIndex` (line 913) while `aria-selected` uses `tz === effectiveTimezoneInput` (line 919) — two different bases, so the highlighted row and the AT-announced row can disagree.
|
|
**Fix:** Derive a clamped active index in render and use it everywhere (visual + `aria-activedescendant`): `const activeIndex = Math.min(tzActiveIndex, Math.max(0, filteredZones.length - 1))`, or reset `tzActiveIndex` to 0 in a `useEffect` keyed on `tzSearch`.
|
|
|
|
### WR-07: Timezone combobox drops Tab-to-commit and relies on a fragile blur timeout
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:838-865`, `927`
|
|
**Issue:** (1) `onKeyDown` handles ArrowUp/Down/Enter/Escape but not `Tab`. Tabbing out with the listbox open and an option highlighted moves focus to Save without committing — the input/`effectiveTimezoneInput` still holds the raw search text, so the admin can attempt to save a partial string (server 400s, but the UX is a confusing failure). (2) The `onBlur` 120ms `setTimeout` to let an option's `onClick` fire is a race; since options already `onMouseDown` `preventDefault()` (line 927), blur won't fire on option click, so the 120ms hack may be unnecessary. The `tzBlurTimer` is cleared on focus/select but not on unmount.
|
|
**Fix:** Commit the active option on `Tab` (without `preventDefault`, so focus still advances); clear `tzBlurTimer` in an unmount cleanup effect; reassess/remove the 120ms blur delay now that `onMouseDown` preventDefault is in place.
|
|
|
|
### WR-08: `pwa:icons` script is non-portable and silently coupled to generated filenames
|
|
|
|
**File:** `apps/pwa/package.json:16`
|
|
**Issue:** `pwa:icons` chains the assets generator with five `cp` commands. (1) `cp` is Unix-only — breaks on Windows contributors and minimal CI containers. (2) It hard-codes the generator's output names (`pwa-192x192.png`, `maskable-icon-512x512.png`, `apple-touch-icon-180x180.png`); a generator version bump that renames outputs breaks it with an opaque `cp: cannot stat`. (3) The manifest icon entries in `vite.config.ts:38-42` (`/icon-192.png`, etc.) only stay in sync because of these manual renames — an invisible coupling with no test. Regenerating icons without running the full script leaves the manifest referencing stale files.
|
|
**Fix:** Configure the generator to emit the final filenames directly, or replace the `cp` chain with a small cross-platform Node script (`fs.copyFileSync`). At minimum, add a comment in `vite.config.ts` by the icon entries pointing at the `pwa:icons` rename step so the coupling is discoverable.
|
|
|
|
## Info
|
|
|
|
### IN-01: `OidcRedirect` navigates as a render-phase side effect
|
|
|
|
**File:** `apps/pwa/src/App.tsx:77-80`
|
|
**Issue:** `OidcRedirect` calls `window.location.replace('/api/login')` directly in the function body (render phase). React may render a component more than once (StrictMode double-invoke in dev, concurrent re-renders); side effects in render are an anti-pattern. It works because `replace` is idempotent and the page unloads, but it is fragile.
|
|
**Fix:** Move the navigation into `useEffect(() => { window.location.replace('/api/login'); }, [])` and render the placeholder.
|
|
|
|
### IN-02: Inconsistent `exhaustive-deps` disables across sibling dialogs
|
|
|
|
**File:** `apps/pwa/src/components/CredentialSheet.tsx:95`, `apps/pwa/src/components/SettingsSheet.tsx:605`
|
|
**Issue:** The Escape `useEffect` disables `react-hooks/exhaustive-deps` (because `handleClose` is referenced but not listed), while the `LinkOidcSheet`/`ResetPasswordSheet` versions list `[isOpen, onClose]` with no disable. The blanket disable also hides any future missing dep added to that effect.
|
|
**Fix:** Wrap `handleClose` in `useCallback` and add it to the dep array, removing the disable; make the pattern consistent across all sheets.
|
|
|
|
### IN-03: `isPhone`/`phone` 767px check duplicated across ~6 sites
|
|
|
|
**File:** `apps/pwa/src/App.tsx:64-66`, `CalendarShell.tsx:74-76`, `SettingsSheet.tsx:135`, `CredentialSheet.tsx:156`, `AdminPage.tsx:59`, `1378-1379`
|
|
**Issue:** The same breakpoint check is reimplemented in two spellings (`isPhone()` helper vs inline `matchMedia`), and the JS hard-codes `767` while `tokens.css` declares `--bp-tablet: 768px`. Drift risk if the breakpoint changes.
|
|
**Fix:** Extract one `useIsPhone()` hook (ideally the resize-aware one from WR-05) and import it everywhere; reference the breakpoint in a single place.
|
|
|
|
### IN-04: Dead placeholder brand tokens retained
|
|
|
|
**File:** `apps/pwa/src/styles/tokens.css:103-104,107`
|
|
**Issue:** `--brand-logo-bg`, `--brand-logo-text`, and `--brand-app-name` are leftovers from the Phase 19 "FS initials circle." BrandSlot now renders `logo.svg` and reads only `--brand-logo-size`/`--brand-logo-border-radius`; `--brand-app-name` is commented "drives doc only — not used as CSS content." These are dead declarations.
|
|
**Fix:** Remove them, or add a comment that they're retained intentionally for a planned fallback.
|
|
|
|
### IN-05: Admin members-panel JSX has inconsistent indentation / stacked bottom margins
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:364-418`
|
|
**Issue:** Inside `admin-panel-members`, `<section aria-label="Members">` and its children are indented inconsistently (section at one level, children shallower), and both the Members and Local Accounts sections carry `marginBottom: var(--space-8)`, adding trailing space at the panel boundary. Cosmetic, but will trip future edits.
|
|
**Fix:** Reformat the panel JSX — Prettier should normalize it. Confirm `pnpm --filter @familysync/pwa lint`/format was run (a recurring pre-push gate on this project).
|
|
|
|
### IN-06: Toast and dialog `zIndex` overlap (300/301)
|
|
|
|
**File:** `apps/pwa/src/routes/AdminPage.tsx:1033` (toast 300) vs `CredentialSheet.tsx:169,189` (backdrop 300 / sheet 301), ResetPasswordSheet (300/301)
|
|
**Issue:** The toast shares `zIndex: 300` with the sheet backdrops. If a toast lingers while a sheet opens within the 3s window, paint order becomes DOM-order-dependent and the toast can render under the backdrop dim. Low likelihood, but the z-index scale is not cleanly layered.
|
|
**Fix:** Put the toast above dialogs (e.g. `zIndex: 400`) and document a named z-index scale (backdrop/sheet/toast) in `tokens.css`.
|
|
|
|
### IN-07: `Intl.DateTimeFormat()` recomputed every render in the calendar-config path
|
|
|
|
**File:** `apps/pwa/src/components/CalendarShell.tsx:159`, `apps/pwa/src/routes/AdminPage.tsx:146`
|
|
**Issue:** `Intl.DateTimeFormat().resolvedOptions().timeZone` is called inline in render. Cheap, but in `CalendarShell` it feeds `useCalendarApp` config, whose stability the file's own comments warn about. (Flagged as a note, not a perf-scope item, because it touches the calendar-app config the code explicitly tries to keep stable.)
|
|
**Fix:** `const displayTimeZone = useMemo(() => Intl.DateTimeFormat().resolvedOptions().timeZone, [])`.
|
|
|
|
---
|
|
|
|
_Reviewed: 2026-06-18T00:00:00Z_
|
|
_Reviewer: Claude (gsd-code-reviewer)_
|
|
_Depth: deep_
|