Lucas BergerandClaude Opus 4.8
9730d3dcdb
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
14 KiB
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 | standard | 13 |
|
|
issues_found |
Phase 17: Code Review Report
Reviewed: 2026-06-18 Depth: standard Files Reviewed: 13 Status: issues_found
Summary
Phase 17 is a UI optimization/polish pass: themeable tokens.css layer with a --bottom-chrome-h chrome token, brand logo via BrandSlot, desktop-centered vs phone bottom-sheet branches across the credential/settings/admin sheets, a Sign-out control, admin success toasts, and a two-tab ARIA strip on /admin. The intentional design decisions (amber --color-member-0, single shipped light theme, logo.svg) were treated as approved and not flagged.
No BLOCKER-class correctness or security defects were found — the password fields use new-password autocomplete and are never pre-filled, copy is plain-text JSX (no dangerouslySetInnerHTML), and the external links carry rel="noopener noreferrer". The substantive findings are a cluster of accessibility and robustness regressions introduced by the new desktop-modal branches and the responsive isPhone() snapshots, plus a stale test mock that no longer matches the API contract. None are data-loss or auth-bypass risks, so all land at WARNING/INFO.
Warnings
WR-01: Dialog phone branch is a render-time snapshot — resize/rotation desyncs modal vs bottom-sheet layout
File: apps/pwa/src/components/SettingsSheet.tsx:135, apps/pwa/src/components/CredentialSheet.tsx:156, apps/pwa/src/routes/AdminPage.tsx:59,1378
Issue: Every new desktop-centered-vs-bottom-sheet branch computes const phone = window.matchMedia('(max-width: 767px)').matches; once during render with no matchMedia change listener and no resize subscription. App.tsx (isPhone() at line 64/84) and CalendarShell.tsx (line 74/252) have the same pattern. On a viewport resize or device rotation that crosses the 767px breakpoint, the component does not re-render, so the modal keeps its stale geometry: a desktop window narrowed below 768px keeps a centered modal (correct target is a bottom sheet) and vice-versa. The FAB offset and paddingBottom: var(--bottom-chrome-h) in App.tsx also stay on the wrong branch until an unrelated state change forces a re-render. This is a real layout regression on tablets/foldables and desktop-window-resize, which the phase explicitly set out to fix.
Fix: Centralize a useIsPhone() hook that subscribes to the media query and triggers re-render:
function useIsPhone() {
const [phone, setPhone] = useState(
() => typeof window !== 'undefined' && window.matchMedia('(max-width: 767px)').matches,
);
useEffect(() => {
const mq = window.matchMedia('(max-width: 767px)');
const onChange = (e: MediaQueryListEvent) => setPhone(e.matches);
mq.addEventListener('change', onChange);
return () => mq.removeEventListener('change', onChange);
}, []);
return phone;
}
Use it in App.tsx, CalendarShell, and all four sheet variants instead of the one-shot snapshot.
WR-02: New desktop modals have no focus trap and do not restore focus — keyboard/AT regression
File: apps/pwa/src/components/SettingsSheet.tsx:199 (ChangePasswordSheet :667, LinkOidcSheet :979), apps/pwa/src/routes/AdminPage.tsx:1420 (ResetPasswordSheet)
Issue: All of these are role="dialog" aria-modal="true" but nothing constrains Tab focus inside the dialog — pressing Tab walks into the page content behind the backdrop. aria-modal="true" is a promise to AT that the rest of the page is inert; without a focus trap (and the backgrounded content lacks inert/aria-hidden), screen-reader and keyboard users can land on occluded controls. Additionally, ChangePasswordSheet and LinkOidcSheet (opened from SettingsSheet) never return focus to the row button that opened them on close — handleClose/onClose just unmount. CredentialSheet and ResetPasswordSheet do restore focus via triggerRef, so the omission in the SettingsSheet sub-sheets is an inconsistent regression. SettingsSheet itself also only focuses the close button on open but never restores focus to the avatar trigger on close.
Fix: Add a shared focus-trap (cycle Tab/Shift+Tab within the dialog, e.g. via a useFocusTrap(ref) hook) and capture/restore document.activeElement on open/close for every dialog. At minimum, give ChangePasswordSheet/LinkOidcSheet a triggerRef and refocus it on close to match the established CredentialSheet pattern.
WR-03: Modal dialogs do not lock background scroll — desktop centered modal scrolls the page behind it
File: apps/pwa/src/components/SettingsSheet.tsx:199, apps/pwa/src/components/CredentialSheet.tsx:173, apps/pwa/src/routes/AdminPage.tsx:1420
Issue: When a centered desktop modal (or bottom sheet) is open, scrolling with wheel/trackpad scrolls the document body behind the backdrop because no scroll lock (overflow: hidden on body/root, or overscroll-behavior) is applied while the dialog is mounted. On /admin the content column is long (member form + calendar picker + timezone combobox), so this is reachable in normal use: opening the credential sheet and scrolling moves the admin page underneath. This undermines the "modal" contract and the polish goal.
Fix: While any dialog is open, set document.body.style.overflow = 'hidden' in a useEffect and restore on unmount. Centralize this with the dialog/focus-trap helper so all sheets get consistent behavior.
WR-04: InstructionSheet.test.tsx mock is stale — fetchLinkOidc shape no longer matches the real contract
File: apps/pwa/src/components/InstructionSheet.test.tsx:43
Issue: The mock declares fetchLinkOidc: vi.fn().mockResolvedValue({ redirectUrl: '/oidc' }), but the real fetchLinkOidc (api/client.ts:247) resolves { signedState, authorizationUrl }, and LinkOidcSheet reads data.authorizationUrl (SettingsSheet.tsx:947). The mock returns the wrong key entirely. The test happens to pass today only because permission is 'denied' (hasLocalCredential:false keeps the OIDC row from rendering, so the mutation is never exercised), but the mock encodes an incorrect contract and will silently mask a real regression if the test surface expands to the link flow. The mock also omits fetchLocalLogout and fetchAdminResetPassword, which SettingsSheet/AdminPage import — fine for this isolated render, but the file's "mock client so the test doesn't make real network calls" claim is only partial.
Fix: Update the mock to the real shape: fetchLinkOidc: vi.fn().mockResolvedValue({ signedState: 's', authorizationUrl: '/oidc' }), and add fetchLocalLogout: vi.fn().mockResolvedValue(undefined) so future expansion of the suite does not hit undefined is not a function.
WR-05: Timezone combobox blur timer can fire after unmount / tab switch — setState on stale closure
File: apps/pwa/src/routes/AdminPage.tsx:104,859-865
Issue: onBlur schedules tzBlurTimer.current = setTimeout(() => { setTzOpen(false); setTzSearch(null); }, 120). This timer is cleared in selectTimezone and onFocus, but there is no cleanup useEffect that clears it on unmount. If the admin blurs the timezone field and then immediately switches tabs or the AdminPage unmounts within 120ms, the timer fires setTzOpen/setTzSearch on an unmounted component (React warns, and in strict mode this is a leak). The two-tab strip uses hidden (the Settings panel stays mounted), which reduces but does not eliminate the unmount path (navigating away from /admin).
Fix: Add a cleanup effect:
useEffect(() => () => { if (tzBlurTimer.current) clearTimeout(tzBlurTimer.current); }, []);
WR-06: LinkOidcSheet "Continue with OIDC" stays enabled after success during the redirect window
File: apps/pwa/src/components/SettingsSheet.tsx:942-958,1100-1125
Issue: On onSuccess the mutation sets window.location.href = data.authorizationUrl and calls onClose(). The button is disabled only while linkMutation.isPending; once the mutation settles (success), isPending is false again. Between onSuccess firing and the browser actually committing the top-level navigation, the sheet is closed so this is largely shielded — but if authorizationUrl is null the sheet stays open with the button re-enabled and a generic error, which is correct. The latent issue is the success path: there is no guard preventing a second mutate() if the navigation is slow and the user re-taps before unload. A double POST to /api/me/link-oidc would mint a second signedState, and (as the App.tsx OIDC-state-cookie comment at CalendarShell.tsx:110-120 documents) concurrent state issuance is exactly the class of bug that produces OAUTH_INVALID_RESPONSE.
Fix: Keep the button disabled through the redirect: track a local redirecting flag set in onSuccess before assigning window.location.href, and include it in the disabled expression (disabled={linkMutation.isPending || redirecting}).
Info
IN-01: Decorative logo gives screen readers no accessible product mark, relies solely on <h1> text
File: apps/pwa/src/components/BrandSlot.tsx:26-40
Issue: The logo <img> is alt="" aria-hidden="true" (correct for purely decorative), and the <h1>FamilySync</h1> carries the name — so this is acceptable. Noting only that the comment claims the SVG "draws its own rounded-square background (rx=104)" and --brand-logo-border-radius: 0; logo.svg confirms rx=104, so no clipping bug. No action required; documented for completeness.
Fix: None — works as designed.
IN-02: --brand-logo-bg / --brand-logo-text tokens are now dead
File: apps/pwa/src/styles/tokens.css:103-104
Issue: BrandSlot was rewritten to render logo.svg; the placeholder-circle background/initials tokens (--brand-logo-bg, --brand-logo-text) are no longer referenced by any component. They linger as dead design tokens.
Fix: Remove the two unused tokens, or add a comment that they are retained intentionally for a future fallback.
IN-03: CalendarShell error-Retry uses refetchQueries({ queryKey: ['events'] }) without start/end
File: apps/pwa/src/components/CalendarShell.tsx:362
Issue: The Retry handler refetches the broad ['events'] prefix rather than the active ['events', start, end]. This is harmless (it refetches all matching, including the active window) but slightly over-broad; consistent with the AdminPage shared-calendar invalidation pattern. Out of scope as a perf concern; flagged only as a minor consistency note.
Fix: Optionally narrow to ['events', start, end] to match the active query key.
IN-04: AdminPage phone toast offset is a render snapshot (same root cause as WR-01)
File: apps/pwa/src/routes/AdminPage.tsx:59,1035
Issue: The success-toast bottom offset chooses var(--bottom-chrome-h) vs var(--space-6) based on the one-shot phone snapshot. A resize across 767px while a toast is visible would misplace it. Low impact (toast auto-dismisses in 3s) — folded into WR-01's recommended useIsPhone() fix.
Fix: Covered by WR-01.
IN-05: Magic z-index ladder duplicated across every sheet
File: apps/pwa/src/components/SettingsSheet.tsx (300/301/302/303), CredentialSheet.tsx (300/301), AdminPage.tsx (300/301)
Issue: Backdrop/sheet z-index values (300/301/302/303) are hard-coded literals repeated across all dialogs. The nested ChangePassword/LinkOidc sheets at 302/303 correctly stack above the parent SettingsSheet at 300/301, so there is no current layering bug — but the magic numbers are duplicated and easy to desync.
Fix: Promote to tokens (e.g. --z-backdrop, --z-sheet, --z-sheet-nested) in tokens.css.
IN-06: eslint-disable react-hooks/exhaustive-deps on Escape-listener effects hides a real omission
File: apps/pwa/src/components/CredentialSheet.tsx:95, apps/pwa/src/components/SettingsSheet.tsx:605 (ChangePasswordSheet)
Issue: The Escape-key effect calls handleClose but disables exhaustive-deps and depends only on [isOpen]. handleClose is redefined every render, so the listener closes over the first render's handleClose. It works because handleClose only touches setState (stable) and onClose/triggerRef, but the disable comment masks the fact that a future handleClose change won't propagate. Sibling components (LinkOidcSheet:934, ResetPasswordSheet:1361) correctly depend on [isOpen, onClose] without the disable — inconsistent.
Fix: Wrap handleClose in useCallback and add it to the dependency array, removing the disable comment, to match the other sheets.
IN-07: OidcRedirect performs window.location.replace during render (side effect in render body)
File: apps/pwa/src/App.tsx:77-80
Issue: OidcRedirect calls window.location.replace('/api/login') directly in the function body rather than in a useEffect. The inline comment acknowledges this ("useEffect isn't available here; use a helper element"). In practice it fires once and the browser navigates away, but a render-phase navigation side effect is a React anti-pattern (double-invoked in StrictMode dev, and not safe if React bails the render). It works today because the navigation unmounts everything.
Fix: Move the replace into a useEffect(() => { window.location.replace('/api/login'); }, []) inside the helper component.
Reviewed: 2026-06-18 Reviewer: Claude (gsd-code-reviewer) Depth: standard