familysync/.planning/phases/17-ui-optimization-polish/17-REVIEW.iter3.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
17-ui-optimization-polish	2026-06-18T00:00:00Z	deep	16	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	critical warning info total 0 1 3 4	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 </div> 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.:

.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 <div aria-hidden="true" /> and the redirect fires post-commit. For the OIDC-only-mode unauthenticated path this means a brief blank frame before window.location.replace('/api/login') unloads the page. Functionally fine and a strict improvement over the render-phase side effect, but the blank aria-hidden div gives screen-reader/keyboard users no "redirecting…" affordance during the gap. Fix: Render a minimal visible "Redirecting to sign in…" status (e.g. role="status") instead of an empty aria-hidden div, so the transition is perceivable if the redirect is slow.

---

Reviewed: 2026-06-18T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: deep Re-review: prior 15 findings all confirmed resolved; gates (typecheck + lint) pass clean