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

---
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: 1
  warning: 0
  info: 1
  total: 2
status: issues_found
---

# Phase 17: Code Review Report (Final Re-Review After Iter-3 Auto-Fixes)

**Reviewed:** 2026-06-18T00:00:00Z
**Depth:** deep
**Files Reviewed:** 16
**Status:** issues_found

## Summary

This is the final re-review of Phase 17 after the second round of auto-fixes, which targeted the three iter-3 findings:

- **WR-01** — `useFocusTrap` hidden/zero-size focusable exclusion + containment guard.
- **IN-03** — `App.tsx` `OidcRedirect` now renders a visible "Redirecting to sign in…" status.
- **IN-02** — favicon.ico hand-maintained coupling documented in `copy-pwa-icons.mjs` and `index.html`.

I re-read every listed file at deep depth and traced the changed code against its call sites and the existing test suite.

**Two of the three fixes are correct and regression-free:**
- **IN-03 (OidcRedirect):** Correct. The navigation stays in `useEffect` (no render-phase side effect), and the placeholder is now a perceivable `role="status"` "Redirecting to sign in…" (App.tsx:83-97). No regression.
- **IN-02 (favicon.ico coupling):** Correct and complete. Both `copy-pwa-icons.mjs` (lines 13-17) and `index.html` (line 7) now carry the hand-maintained-`.ico` pointer. Verified on disk: `favicon.svg` is byte-identical to `logo.svg` (produced by the `COPIES` table), and `favicon.ico` (967 B) is committed separately. The invisible coupling is now documented at both ends.

**The WR-01 fix introduces a CR-tier regression.** The new visibility filter in `useFocusTrap.ts` (lines 36-38) relies on `offsetParent` and `getBoundingClientRect()` width/height. Both are `null`/`0` under jsdom — the environment the existing focus-trap unit tests run in — so the filter now excludes **every** focusable, `focusable.length === 0` short-circuits, and the trap silently stops wrapping focus. This breaks the two pre-existing `EventForm.test.tsx` WR-07 tests and makes `pnpm test` (a CI gate per CLAUDE.md) fail.

Gate status after the fixes:
- `pnpm --filter @familysync/pwa typecheck` → **pass** (tsc + e2e tsconfig).
- `pnpm --filter @familysync/pwa lint` → **pass** (eslint `--max-warnings 0`).
- `pnpm --filter @familysync/pwa test` (vitest) → **FAIL**: 2 failed / 264 passed / 266 total. Both failures are the WR-07 focus-trap tests, caused directly by the WR-01 change under review.

The IN-01 containment guard added alongside WR-01 is functionally inert (the handler is only wired to the dialog's `onKeyDown`, which cannot fire when focus is outside the dialog), but it is harmless — recorded as INFO.

## Critical Issues

### CR-01: `useFocusTrap` visibility filter excludes all focusables under jsdom — breaks the focus-trap test suite (CI gate fails)

**File:** `apps/pwa/src/hooks/useFocusTrap.ts:36-38`
**Issue:** The WR-01 fix added a "rendered/visible" filter to the focusable query:
```ts
if (el.hasAttribute('hidden') || el.offsetParent === null) return false;
const r = el.getBoundingClientRect();
return r.width > 0 && r.height > 0;
```
In a real browser this is correct. But the existing focus-trap regression tests (`apps/pwa/src/components/EventForm.test.tsx`, the two `WR-07` cases at lines 718-764) run under **jsdom**, where:
- `getBoundingClientRect()` returns all-zero geometry for every element (`width === 0`, `height === 0`), and
- `offsetParent` is `null` for every element.

Either condition alone causes the filter to reject **every** focusable. `focusable.length` becomes `0`, the handler hits the `if (focusable.length === 0) return;` early-out (line 41), and Tab/Shift+Tab no longer wrap. Both WR-07 tests now fail:

```
FAIL src/components/EventForm.test.tsx > WR-07: Tab from last focusable element wraps focus to first inside dialog
FAIL src/components/EventForm.test.tsx > WR-07: Shift+Tab from first focusable element wraps focus to last inside dialog
Test Files  1 failed | 21 passed (22)
      Tests  2 failed | 264 passed (266)
```

`pnpm test` (→ `vitest run`) is one of the CI gates the project requires to pass before push (CLAUDE.md "Frustrations" directive + the CI-checks-conformance memory). This regression ships as part of the file under review, so it is in scope even though `EventForm.test.tsx` is not in the listed-files set — those tests exist specifically to guard the changed behavior.

**Fix:** Make the visibility filter tolerant of a non-layout (jsdom) environment, so it excludes genuinely hidden nodes in a browser without nuking all nodes in tests. Treat zero-geometry as "visible" when no layout engine is present, and gate on `offsetParent` only when geometry is meaningful:
```ts
.filter((el) => {
  if (el.hasAttribute('disabled') || el.getAttribute('tabindex') === '-1') return false;
  if (el.hasAttribute('hidden')) return false;
  // jsdom has no layout: getBoundingClientRect() is all-zero and offsetParent is
  // null for every node. Only apply the visibility heuristic when a real layout
  // exists, so unit tests still see focusables.
  const r = el.getBoundingClientRect();
  const hasLayout = r.width > 0 || r.height > 0 || el.offsetParent !== null;
  if (!hasLayout) return true; // no layout engine → don't filter on visibility
  if (el.offsetParent === null) return false;
  return r.width > 0 && r.height > 0;
});
```
Alternatively, stub `getBoundingClientRect`/`offsetParent` in the test setup so jsdom reports non-zero geometry — but the production-side guard above is the safer minimal change, since other future tests will hit the same wall. Either way, re-run `pnpm --filter @familysync/pwa test` and confirm both WR-07 cases pass before considering this resolved.

## Info

### IN-01: Focus-trap containment guard is unreachable as wired (harmless dead branch)

**File:** `apps/pwa/src/hooks/useFocusTrap.ts:50-54`
**Issue:** The IN-01 fix added a containment guard:
```ts
if (!dialogRef.current.contains(document.activeElement)) {
  e.preventDefault();
  first.focus();
  return;
}
```
The comment claims this catches the case where "focus has somehow landed outside the dialog ... Tab would walk background content." But the handler is only attached to each dialog container's `onKeyDown` (verified across all 5 sheets + EventForm + SeriesEditPrompt — no `document`-level listener exists). React's synthetic `onKeyDown` on the dialog div only fires when the keydown event's target is **inside** the dialog subtree (the event must bubble up through that div). When `document.activeElement` is genuinely outside the dialog, the keydown fires on that outside element and bubbles through `document`, **not** through the dialog div — so `handleDialogKeyDown` never runs, and `dialogRef.current.contains(document.activeElement)` is effectively always `true` whenever this code executes. The guard is therefore a no-op in practice: it does not deliver the containment guarantee its comment promises.

This is not a correctness bug (it never produces wrong behavior), so it is INFO, not a blocker. But it is worth noting that the IN-01 concern (focus escaping a boundary-only trap) is **not actually addressed** by this change.

**Fix:** If true containment is desired, move the trap to a `document`-level `keydown` (or `focusin`) listener mounted while the dialog is open, so it can intercept Tab/focus originating outside the dialog. If the boundary-only trap is considered sufficient (it is, for the present always-focus-the-heading-on-open flows), drop the unreachable containment branch and its comment to avoid implying a guarantee the code does not provide.

---

_Reviewed: 2026-06-18T00:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: deep_
_Re-review: IN-02 + IN-03 fixes confirmed clean; WR-01 fix regresses the focus-trap test suite (CR-01) and its IN-01 containment guard is inert. typecheck + lint pass; `pnpm test` FAILS (2 WR-07 tests)._