familysync/.planning/milestones/v1.1-phases/17-ui-optimization-polish/17-REVIEW-FIX.iter3.md

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
17-ui-optimization-polish	2026-06-18T00:00:00Z	.planning/phases/17-ui-optimization-polish/17-REVIEW.md	1	15	15	0	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 <div> 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