From 6fa6725fe87a1e95aef018a4e0820c065eb70607 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Thu, 18 Jun 2026 14:06:03 -0400 Subject: [PATCH] docs(17): add code review fix report (auto-loop, 3 iterations) --- .../17-REVIEW-FIX.iter2.md | 125 +++++++++++ .../17-REVIEW-FIX.iter3.md | 125 +++++++++++ .../17-REVIEW-FIX.md | 56 +++++ .../17-REVIEW.iter2.md | 167 +++++++++++++++ .../17-REVIEW.iter3.md | 95 +++++++++ .../17-ui-optimization-polish/17-REVIEW.md | 195 +++++++----------- 6 files changed, 646 insertions(+), 117 deletions(-) create mode 100644 .planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter2.md create mode 100644 .planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter3.md create mode 100644 .planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.md create mode 100644 .planning/phases/17-ui-optimization-polish/17-REVIEW.iter2.md create mode 100644 .planning/phases/17-ui-optimization-polish/17-REVIEW.iter3.md diff --git a/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter2.md b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter2.md new file mode 100644 index 0000000..0eb8662 --- /dev/null +++ b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter2.md @@ -0,0 +1,125 @@ +--- +phase: 17-ui-optimization-polish +fixed_at: 2026-06-18T00:00:00Z +review_path: .planning/phases/17-ui-optimization-polish/17-REVIEW.md +iteration: 1 +findings_in_scope: 15 +fixed: 15 +skipped: 0 +status: all_fixed +--- + +# Phase 17: Code Review Fix Report + +**Fixed at:** 2026-06-18T00:00:00Z +**Source review:** .planning/phases/17-ui-optimization-polish/17-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 15 (fix_scope: all — includes Info) +- Fixed: 15 +- Skipped: 0 + +All fixes were verified with `tsc --noEmit` (clean) and `eslint --max-warnings 0` +(clean) on every touched file; the PWA also builds (`vite build` succeeds). A new +shared hook `apps/pwa/src/hooks/useIsPhone.ts` was created to back WR-05/IN-03. + +## Fixed Issues + +### WR-01: Modal dialogs declare `aria-modal="true"` but do not trap focus + +**Files modified:** `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/components/SettingsSheet.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** fb30800 +**Applied fix:** Reused the existing `useFocusTrap(dialogRef)` hook (already used by EventForm/SeriesEditPrompt). Added a `dialogRef` + `onKeyDown={handleDialogKeyDown}` to every modal sheet that asserts `aria-modal="true"`: CredentialSheet, SettingsSheet, ChangePasswordSheet, LinkOidcSheet, and ResetPasswordSheet. Tab/Shift-Tab now cycle within the dialog instead of escaping to occluded background controls. + +### WR-02: Admin tab strip keyboard nav is incomplete (no Home/End, no explicit wrap) + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Rewrote `handleTabKeyDown` to the full WAI-ARIA tabs pattern: ArrowLeft/Right now wrap around the ends using modular arithmetic over the `['members','settings']` order, and Home/End jump to the first/last tab. + +### WR-03: Success toast does not re-announce repeated identical messages + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Changed toast state from `string | null` to `{ id: number; msg: string } | null` with a `showToast(msg)` helper that mints a fresh `id` (Date.now()) per call. The rendered toast `
` is now keyed on `toast.id` so an identical repeated message remounts and `aria-live` re-announces it; the auto-dismiss effect depends on the fresh object reference so the 3s timer restarts. + +### WR-04: Toast `whiteSpace: nowrap` is a latent horizontal-overflow regression + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Removed `whiteSpace: 'nowrap'` from the toast style so a longer/localized message wraps within `maxWidth: 90vw` instead of overflowing `documentElement.scrollWidth` (which would trip the layout suite's no-horizontal-overflow rule). + +### WR-05: `matchMedia(...)` read at render time does not react to resize/orientation + +**Files modified:** `apps/pwa/src/hooks/useIsPhone.ts` (new), `apps/pwa/src/App.tsx`, `apps/pwa/src/components/CalendarShell.tsx`, `apps/pwa/src/components/SettingsSheet.tsx`, `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** a4a7438 +**Applied fix:** Added a resize-aware `useMediaQuery`/`useIsPhone` hook backed by `matchMedia.addEventListener('change', …)`. Replaced all six synchronous `matchMedia('(max-width: 767px)')` render-time reads with `useIsPhone()`. Hook calls were placed before any early `return null` to respect the Rules of Hooks. Components now re-render when the 767px breakpoint is crossed (iPad rotation, desktop resize). + +### WR-06: Timezone combobox `aria-activedescendant`/highlight can desync after filtering + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 1c0f357 +**Applied fix:** Derived a clamped `tzActiveIndexClamped = Math.min(tzActiveIndex, max(0, filteredZones.length - 1))` in render and used it for `aria-activedescendant`, the Enter-to-commit lookup, and the visual highlight (`i === tzActiveIndexClamped`). ArrowUp/ArrowDown clamp the current index before moving so they never start from a stale position past the end of a freshly-shrunk list. +**Note:** Combobox interaction logic — recommend a quick manual/keyboard pass (type to filter, arrow, Enter) to confirm behavior. + +### WR-07: Timezone combobox drops Tab-to-commit and relies on a fragile blur timeout + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 1c0f357 +**Applied fix:** Added a `Tab` branch to the combobox `onKeyDown` that commits the highlighted option WITHOUT `preventDefault` (focus still advances to Save). Added an unmount cleanup effect that clears `tzBlurTimer`. Reduced the blur-close `setTimeout` from 120ms to 0ms now that options `preventDefault()` on `onMouseDown` (so a click never blurs the input first). +**Note:** Interaction logic — recommend a manual check that tabbing out of the open listbox commits the highlighted zone and that clicking an option still selects it. + +### WR-08: `pwa:icons` script is non-portable and silently coupled to generated filenames + +**Files modified:** `apps/pwa/scripts/copy-pwa-icons.mjs` (new), `apps/pwa/package.json`, `apps/pwa/vite.config.ts` +**Commit:** dd0b761 +**Applied fix:** Replaced the five-`cp` Unix-only chain with a cross-platform Node script (`fs.copyFileSync`) that maps each generated filename to its stable manifest name and fails loudly with a named error if a generated file is missing (generator rename guard). Added a discoverability comment beside the manifest `icons` array in `vite.config.ts` pointing at the script's COPIES table. + +### IN-01: `OidcRedirect` navigates as a render-phase side effect + +**Files modified:** `apps/pwa/src/App.tsx` +**Commit:** a4a7438 +**Applied fix:** Moved `window.location.replace('/api/login')` into a `useEffect(() => {...}, [])` so the navigation is no longer a render-phase side effect. + +### IN-02: Inconsistent `exhaustive-deps` disables across sibling dialogs + +**Files modified:** `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/components/SettingsSheet.tsx` +**Commit:** 3f4b7ea +**Applied fix:** Wrapped `handleClose` in `useCallback` in CredentialSheet and ChangePasswordSheet, added it to the Escape effect's dependency array, and removed the `// eslint-disable-line react-hooks/exhaustive-deps` comments — matching the LinkOidc/Reset sheet pattern. + +### IN-03: `isPhone`/`phone` 767px check duplicated across ~6 sites + +**Files modified:** (same as WR-05) +**Commit:** a4a7438 +**Applied fix:** Resolved together with WR-05 — the single `useIsPhone()` hook now backs all call sites, and the `(max-width: 767px)` query lives in one place (`PHONE_MAX_QUERY` in the hook). The old standalone `isPhone()` helpers in App.tsx and CalendarShell.tsx were deleted. + +### IN-04: Dead placeholder brand tokens retained + +**Files modified:** `apps/pwa/src/styles/tokens.css` +**Commit:** 2317833 +**Applied fix:** Removed the unused `--brand-logo-bg`, `--brand-logo-text`, and `--brand-app-name` declarations (verified via grep that nothing references them); left a short comment explaining the removal and that BrandSlot only reads `--brand-logo-size`/`--brand-logo-border-radius`. + +### IN-05: Admin members-panel JSX has inconsistent indentation / stacked bottom margins + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 4bc1e2a +**Applied fix:** Ran Prettier (project `.prettierrc`) over AdminPage.tsx, normalizing the members-panel indentation and the rest of the file's drift; `prettier --check` now passes on the file. The stacked `marginBottom: var(--space-8)` on the last panel section was left intentionally — the review flagged it only as a minor cosmetic note, and changing section spacing risks a visual regression outside the finding's scope. + +### IN-06: Toast and dialog `zIndex` overlap (300/301) + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Raised the toast `zIndex` from 300 to 400 so it always paints above sheet backdrops (300) and sheets (301), removing the DOM-order-dependent paint ambiguity. + +### IN-07: `Intl.DateTimeFormat()` recomputed every render in the calendar-config path + +**Files modified:** `apps/pwa/src/components/CalendarShell.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 11b6b36 +**Applied fix:** Wrapped both `Intl.DateTimeFormat().resolvedOptions().timeZone` reads in `useMemo(…, [])` — `displayTimeZone` in CalendarShell (feeds the stable `useCalendarApp` config) and `detectedTz` in AdminPage. + +--- + +_Fixed: 2026-06-18T00:00:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_ diff --git a/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter3.md b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter3.md new file mode 100644 index 0000000..0eb8662 --- /dev/null +++ b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.iter3.md @@ -0,0 +1,125 @@ +--- +phase: 17-ui-optimization-polish +fixed_at: 2026-06-18T00:00:00Z +review_path: .planning/phases/17-ui-optimization-polish/17-REVIEW.md +iteration: 1 +findings_in_scope: 15 +fixed: 15 +skipped: 0 +status: all_fixed +--- + +# Phase 17: Code Review Fix Report + +**Fixed at:** 2026-06-18T00:00:00Z +**Source review:** .planning/phases/17-ui-optimization-polish/17-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 15 (fix_scope: all — includes Info) +- Fixed: 15 +- Skipped: 0 + +All fixes were verified with `tsc --noEmit` (clean) and `eslint --max-warnings 0` +(clean) on every touched file; the PWA also builds (`vite build` succeeds). A new +shared hook `apps/pwa/src/hooks/useIsPhone.ts` was created to back WR-05/IN-03. + +## Fixed Issues + +### WR-01: Modal dialogs declare `aria-modal="true"` but do not trap focus + +**Files modified:** `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/components/SettingsSheet.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** fb30800 +**Applied fix:** Reused the existing `useFocusTrap(dialogRef)` hook (already used by EventForm/SeriesEditPrompt). Added a `dialogRef` + `onKeyDown={handleDialogKeyDown}` to every modal sheet that asserts `aria-modal="true"`: CredentialSheet, SettingsSheet, ChangePasswordSheet, LinkOidcSheet, and ResetPasswordSheet. Tab/Shift-Tab now cycle within the dialog instead of escaping to occluded background controls. + +### WR-02: Admin tab strip keyboard nav is incomplete (no Home/End, no explicit wrap) + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Rewrote `handleTabKeyDown` to the full WAI-ARIA tabs pattern: ArrowLeft/Right now wrap around the ends using modular arithmetic over the `['members','settings']` order, and Home/End jump to the first/last tab. + +### WR-03: Success toast does not re-announce repeated identical messages + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Changed toast state from `string | null` to `{ id: number; msg: string } | null` with a `showToast(msg)` helper that mints a fresh `id` (Date.now()) per call. The rendered toast `
` is now keyed on `toast.id` so an identical repeated message remounts and `aria-live` re-announces it; the auto-dismiss effect depends on the fresh object reference so the 3s timer restarts. + +### WR-04: Toast `whiteSpace: nowrap` is a latent horizontal-overflow regression + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Removed `whiteSpace: 'nowrap'` from the toast style so a longer/localized message wraps within `maxWidth: 90vw` instead of overflowing `documentElement.scrollWidth` (which would trip the layout suite's no-horizontal-overflow rule). + +### WR-05: `matchMedia(...)` read at render time does not react to resize/orientation + +**Files modified:** `apps/pwa/src/hooks/useIsPhone.ts` (new), `apps/pwa/src/App.tsx`, `apps/pwa/src/components/CalendarShell.tsx`, `apps/pwa/src/components/SettingsSheet.tsx`, `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** a4a7438 +**Applied fix:** Added a resize-aware `useMediaQuery`/`useIsPhone` hook backed by `matchMedia.addEventListener('change', …)`. Replaced all six synchronous `matchMedia('(max-width: 767px)')` render-time reads with `useIsPhone()`. Hook calls were placed before any early `return null` to respect the Rules of Hooks. Components now re-render when the 767px breakpoint is crossed (iPad rotation, desktop resize). + +### WR-06: Timezone combobox `aria-activedescendant`/highlight can desync after filtering + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 1c0f357 +**Applied fix:** Derived a clamped `tzActiveIndexClamped = Math.min(tzActiveIndex, max(0, filteredZones.length - 1))` in render and used it for `aria-activedescendant`, the Enter-to-commit lookup, and the visual highlight (`i === tzActiveIndexClamped`). ArrowUp/ArrowDown clamp the current index before moving so they never start from a stale position past the end of a freshly-shrunk list. +**Note:** Combobox interaction logic — recommend a quick manual/keyboard pass (type to filter, arrow, Enter) to confirm behavior. + +### WR-07: Timezone combobox drops Tab-to-commit and relies on a fragile blur timeout + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 1c0f357 +**Applied fix:** Added a `Tab` branch to the combobox `onKeyDown` that commits the highlighted option WITHOUT `preventDefault` (focus still advances to Save). Added an unmount cleanup effect that clears `tzBlurTimer`. Reduced the blur-close `setTimeout` from 120ms to 0ms now that options `preventDefault()` on `onMouseDown` (so a click never blurs the input first). +**Note:** Interaction logic — recommend a manual check that tabbing out of the open listbox commits the highlighted zone and that clicking an option still selects it. + +### WR-08: `pwa:icons` script is non-portable and silently coupled to generated filenames + +**Files modified:** `apps/pwa/scripts/copy-pwa-icons.mjs` (new), `apps/pwa/package.json`, `apps/pwa/vite.config.ts` +**Commit:** dd0b761 +**Applied fix:** Replaced the five-`cp` Unix-only chain with a cross-platform Node script (`fs.copyFileSync`) that maps each generated filename to its stable manifest name and fails loudly with a named error if a generated file is missing (generator rename guard). Added a discoverability comment beside the manifest `icons` array in `vite.config.ts` pointing at the script's COPIES table. + +### IN-01: `OidcRedirect` navigates as a render-phase side effect + +**Files modified:** `apps/pwa/src/App.tsx` +**Commit:** a4a7438 +**Applied fix:** Moved `window.location.replace('/api/login')` into a `useEffect(() => {...}, [])` so the navigation is no longer a render-phase side effect. + +### IN-02: Inconsistent `exhaustive-deps` disables across sibling dialogs + +**Files modified:** `apps/pwa/src/components/CredentialSheet.tsx`, `apps/pwa/src/components/SettingsSheet.tsx` +**Commit:** 3f4b7ea +**Applied fix:** Wrapped `handleClose` in `useCallback` in CredentialSheet and ChangePasswordSheet, added it to the Escape effect's dependency array, and removed the `// eslint-disable-line react-hooks/exhaustive-deps` comments — matching the LinkOidc/Reset sheet pattern. + +### IN-03: `isPhone`/`phone` 767px check duplicated across ~6 sites + +**Files modified:** (same as WR-05) +**Commit:** a4a7438 +**Applied fix:** Resolved together with WR-05 — the single `useIsPhone()` hook now backs all call sites, and the `(max-width: 767px)` query lives in one place (`PHONE_MAX_QUERY` in the hook). The old standalone `isPhone()` helpers in App.tsx and CalendarShell.tsx were deleted. + +### IN-04: Dead placeholder brand tokens retained + +**Files modified:** `apps/pwa/src/styles/tokens.css` +**Commit:** 2317833 +**Applied fix:** Removed the unused `--brand-logo-bg`, `--brand-logo-text`, and `--brand-app-name` declarations (verified via grep that nothing references them); left a short comment explaining the removal and that BrandSlot only reads `--brand-logo-size`/`--brand-logo-border-radius`. + +### IN-05: Admin members-panel JSX has inconsistent indentation / stacked bottom margins + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 4bc1e2a +**Applied fix:** Ran Prettier (project `.prettierrc`) over AdminPage.tsx, normalizing the members-panel indentation and the rest of the file's drift; `prettier --check` now passes on the file. The stacked `marginBottom: var(--space-8)` on the last panel section was left intentionally — the review flagged it only as a minor cosmetic note, and changing section spacing risks a visual regression outside the finding's scope. + +### IN-06: Toast and dialog `zIndex` overlap (300/301) + +**Files modified:** `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** f601c0c +**Applied fix:** Raised the toast `zIndex` from 300 to 400 so it always paints above sheet backdrops (300) and sheets (301), removing the DOM-order-dependent paint ambiguity. + +### IN-07: `Intl.DateTimeFormat()` recomputed every render in the calendar-config path + +**Files modified:** `apps/pwa/src/components/CalendarShell.tsx`, `apps/pwa/src/routes/AdminPage.tsx` +**Commit:** 11b6b36 +**Applied fix:** Wrapped both `Intl.DateTimeFormat().resolvedOptions().timeZone` reads in `useMemo(…, [])` — `displayTimeZone` in CalendarShell (feeds the stable `useCalendarApp` config) and `detectedTz` in AdminPage. + +--- + +_Fixed: 2026-06-18T00:00:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_ diff --git a/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.md b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.md new file mode 100644 index 0000000..cf13a44 --- /dev/null +++ b/.planning/phases/17-ui-optimization-polish/17-REVIEW-FIX.md @@ -0,0 +1,56 @@ +--- +phase: 17-ui-optimization-polish +fixed_at: 2026-06-18T14:04:00Z +review_path: .planning/phases/17-ui-optimization-polish/17-REVIEW.md +iteration: 3 +findings_in_scope: 2 +fixed: 2 +skipped: 0 +status: all_fixed +--- + +# Phase 17: Code Review Fix Report (Iteration 3) + +**Fixed at:** 2026-06-18T14:04:00Z +**Source review:** .planning/phases/17-ui-optimization-polish/17-REVIEW.md +**Iteration:** 3 + +**Summary:** +- Findings in scope: 2 (fix_scope: all — includes Info) +- Fixed: 2 +- Skipped: 0 + +**Gate status after fixes (all pass):** +- `pnpm --filter @familysync/pwa test` → pass (22 files, 266 passed / 0 failed) +- `pnpm --filter @familysync/pwa typecheck` → pass (tsc + e2e tsconfig) +- `pnpm --filter @familysync/pwa lint` → pass (eslint `--max-warnings 0`) + +## Fixed Issues + +### CR-01: `useFocusTrap` visibility filter excluded all focusables under jsdom (CI gate failed) + +**Files modified:** `apps/pwa/src/hooks/useFocusTrap.ts` +**Commit:** 287ecae +**Applied fix:** The prior iter-3 auto-fix (WR-01) rejected every focusable under jsdom because there `getBoundingClientRect()` returns all-zero geometry and `offsetParent` is `null` for every node, which short-circuited the trap (`focusable.length === 0`) and broke the two pre-existing WR-07 focus-trap regression tests — making `pnpm test` (a CI gate) fail at 2 failed / 264 passed. + +Made the visibility heuristic tolerant of a non-layout environment: it now derives `hasLayout = r.width > 0 || r.height > 0 || el.offsetParent !== null`, and when there is no evidence of a layout engine (jsdom) it treats the node as visible instead of filtering it. Only when a real layout exists does it apply the `offsetParent === null` / zero-geometry exclusion, so genuinely hidden/collapsed nodes are still excluded in a real browser. The `hidden`-attribute exclusion is unambiguous regardless of layout, so it was hoisted out and kept unconditional. Result: all 266 PWA tests pass, including both WR-07 cases. + +### IN-01: Focus-trap containment guard was unreachable as wired (harmless dead branch) + +**Files modified:** `apps/pwa/src/hooks/useFocusTrap.ts` +**Commit:** 287ecae +**Applied fix:** The handler is wired only to each dialog's own `onKeyDown`, so it can only run while focus is already inside the dialog subtree; the `!dialogRef.current.contains(document.activeElement)` containment branch could therefore never evaluate true and delivered no actual containment guarantee. Per the review's recommendation, removed the inert branch and replaced its misleading comment with an accurate note: this is a deliberate boundary-only trap (a `document`-level `keydown`/`focusin` listener would be required for true containment, and is unnecessary for the current always-focus-the-heading-on-open flows). No behavior change in any real scenario — it only removes a comment that implied a guarantee the wiring cannot provide. + +## Skipped Issues + +None. + +## Prior Iterations + +Iterations 1 and 2 fixed the earlier batches of findings (15 in iter-1, then the iter-3 review's WR-01/IN-02/IN-03 set). The IN-02 (favicon.ico coupling) and IN-03 (OidcRedirect visible status) fixes were confirmed clean by the final re-review. This iteration-3 report supersedes those and records the final state: the WR-01 regression (CR-01) and its inert containment guard (IN-01) are now resolved, with all CI gates green. + +--- + +_Fixed: 2026-06-18T14:04:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 3_ diff --git a/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter2.md b/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter2.md new file mode 100644 index 0000000..60ec5fd --- /dev/null +++ b/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter2.md @@ -0,0 +1,167 @@ +--- +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( + '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:
{toast.msg}
+``` + +### 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`, `
` 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_ diff --git a/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter3.md b/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter3.md new file mode 100644 index 0000000..70cf986 --- /dev/null +++ b/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter3.md @@ -0,0 +1,95 @@ +--- +phase: 17-ui-optimization-polish +reviewed: 2026-06-18T00:00:00Z +depth: deep +files_reviewed: 16 +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/scripts/copy-pwa-icons.mjs + - 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/hooks/useFocusTrap.ts + - apps/pwa/src/hooks/useIsPhone.ts + - apps/pwa/src/routes/AdminPage.tsx + - apps/pwa/src/styles/tokens.css + - apps/pwa/vite.config.ts +findings: + critical: 0 + warning: 1 + info: 3 + total: 4 +status: issues_found +--- + +# Phase 17: Code Review Report (Re-Review After Auto-Fix) + +**Reviewed:** 2026-06-18T00:00:00Z +**Depth:** deep +**Files Reviewed:** 16 +**Status:** issues_found + +## Summary + +This is a re-review of Phase 17 (UI optimization/polish) after auto-fixes were applied to the prior 15 findings (8 warnings, 7 info). I re-read every listed file at deep depth, traced the just-changed code (focus-trap wiring across the 5 sheets, the new `useIsPhone`/`useFocusTrap` hooks, admin tab keyboard handling, the toast re-announce/wrapping changes, the timezone combobox active-index/Tab-commit logic, and the cross-platform icon-copy script), and confirmed the fixes against the surrounding call sites for regressions. + +**All 8 prior warnings and all 7 prior info items are correctly resolved.** Both local gates pass clean: `pnpm --filter @familysync/pwa typecheck` (tsc + e2e tsconfig) and `pnpm --filter @familysync/pwa lint` (eslint `--max-warnings 0`) both succeed with no output. I found **no BLOCKERs** and **no regressions** introduced by the fixes. + +What the fixes got right and why they don't regress: +- **Focus trap** (`useFocusTrap`) is wired into all five dialogs. The child sheets (`ChangePasswordSheet`/`LinkOidcSheet`) are rendered as DOM **siblings** of the `SettingsSheet` dialog div (after the `
` at SettingsSheet.tsx:563), not descendants, so the parent trap's `querySelectorAll` cannot capture child-sheet focusables and there is no double-trap conflict — each sheet owns its own trap. +- **Combobox desync** is genuinely fixed: `tzActiveIndexClamped` now drives the visual highlight (AdminPage.tsx:955), `aria-activedescendant` (:838), and the Enter/Tab commit (:879/:887) from one clamped source. `aria-selected` correctly stays bound to `effectiveTimezoneInput` (:961) — that is the right ARIA distinction (selected value vs. active option), not a residual bug. +- **Tab-to-commit** commits the clamped option without `preventDefault`, the blur timer is now `setTimeout(…, 0)` and is cleared on focus, on select, and on unmount (AdminPage.tsx:115-119) — no setState-after-unmount path remains. +- **Toast** is keyed on a unique `{id, msg}` so identical repeats remount and `aria-live` re-announces; `whiteSpace: nowrap` is removed so it wraps within `maxWidth: 90vw` (no Rule 2 overflow hazard); z-index raised to 400, above all sheet backdrops (max 303), resolving the prior overlap. +- **`useIsPhone`/`useMediaQuery`** subscribe via `addEventListener('change', …)` and are now used at every former inline `matchMedia` site (App, CalendarShell, all sheets, AdminPage), so iPad rotation across 767px reflows correctly. SSR guard returns `false` cleanly. + +The one remaining WARNING is a pre-existing focus-trap robustness gap (not introduced this phase, but now load-bearing because `aria-modal` promises containment). The three INFO items are minor and non-blocking. + +## Warnings + +### WR-01: `useFocusTrap` only wraps at the boundaries — focus can still escape via hidden/zero-size focusables + +**File:** `apps/pwa/src/hooks/useFocusTrap.ts:25-48` +**Issue:** The trap queries `button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])` and filters only `!disabled` and `tabindex !== '-1'`. It does not exclude elements that are `display:none`, `visibility:hidden`, `hidden`, or zero-size. In the current sheets every focusable is visible, so the trap works today. But the pattern has two latent escape paths: (1) if a dialog ever conditionally renders a focusable inside a `hidden`/collapsed block, that element joins the `first`/`last` computation and the wrap math targets an unfocusable node — `last.focus()` becomes a no-op and Tab leaks to background content (which `aria-modal="true"` asserts is impossible); (2) the trap only intervenes at the exact first/last boundary, so it relies on the browser's natural Tab order between them being correct and contained. This is the kind of half-implemented trap the prior WR-01 set out to eliminate; the fix is correct for the present DOM but fragile for future edits. +**Fix:** Filter to genuinely focusable, rendered elements before computing first/last, e.g.: +```ts +.filter((el) => { + if (el.hasAttribute('disabled') || el.getAttribute('tabindex') === '-1') return false; + if (el.hasAttribute('hidden') || (el as HTMLElement).offsetParent === null) return false; + const r = el.getBoundingClientRect(); + return r.width > 0 && r.height > 0; +}); +``` +Alternatively, document that all dialog focusables must be unconditionally rendered and visible while the dialog is open. + +## Info + +### IN-01: Focus trap does not pull focus back when `activeElement` is already outside the dialog + +**File:** `apps/pwa/src/hooks/useFocusTrap.ts:36-47` +**Issue:** The handler wraps only when `document.activeElement === first` (Shift+Tab) or `=== last` (Tab). Each sheet focuses its heading/close button on open, so the trap engages from inside. But `aria-modal="true"` does not actually prevent the background DOM (still mounted behind the backdrop) from receiving focus — e.g. a programmatic focus, or a browser quirk, could land focus outside the dialog, and then neither boundary condition matches, so Tab moves through background content until it happens to re-enter. This is the residual weakness of a boundary-only trap versus a containment trap (which checks `dialogRef.current.contains(document.activeElement)` and redirects when false). Low likelihood given the open-focus behavior; noted for completeness. +**Fix:** Add a containment guard: if `!dialogRef.current.contains(document.activeElement)` on Tab, `preventDefault()` and focus `first`. + +### IN-02: `favicon.ico` is referenced by `index.html` but not produced by `pwa:icons` + +**File:** `apps/pwa/index.html:7`, `apps/pwa/scripts/copy-pwa-icons.mjs:21-27` +**Issue:** `index.html` links `/favicon.ico`, and the file is committed in `public/` (967 bytes). The new `copy-pwa-icons.mjs` `COPIES` table generates `favicon.svg` (from `logo.svg`) and the PNGs, but the `minimal2023Preset` does not emit a `.ico`, so `favicon.ico` is hand-maintained outside the script. This is the same invisible-coupling class the prior WR-08 flagged, just narrowed: regenerating icons leaves `favicon.ico` stale relative to a new brand mark, with nothing to catch it. The script's own header says "Keep COPIES in sync with the manifest," but the `.ico` link in `index.html` has no such pointer. +**Fix:** Either drop the `favicon.ico` link (the SVG favicon + `sizes="any"` covers modern browsers) or add a comment in `copy-pwa-icons.mjs`/`index.html` noting `favicon.ico` is hand-maintained and must be regenerated manually when the brand mark changes. + +### IN-03: `OidcRedirect` placeholder renders an empty `aria-hidden` div for a full render cycle + +**File:** `apps/pwa/src/App.tsx:74-82` +**Issue:** The prior IN-01 fix correctly moved the navigation into `useEffect`. The component now renders `