96 lines
8.3 KiB
Markdown
96 lines
8.3 KiB
Markdown
---
|
|
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 `</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.:
|
|
```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 `<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_
|