From 72919ccd1bd411a02966ea6c72934eaae9d3ae5d Mon Sep 17 00:00:00 2001 From: Anso Date: Sat, 2 May 2026 04:15:49 -0400 Subject: [PATCH] chore(frontend): polish error boundaries and dismissal sync (#877) Bundles three small follow-ups deferred from the recent lazy-loading and gate-refactor PRs: ErrorBoundary visual harmonization. The top-level boundary used a red banner with custom button styling that no longer matched the glass- card aesthetic the lock cards and LazyBoundary settled on. Replace with the same glass-card + AlertTriangle layout. The user already knows something broke when this fires; a calm card with a clear Try again CTA is more actionable than the louder treatment, and a consistent recovery surface across both boundaries means a user never sees two different "something went wrong" treatments depending on which boundary catches the error. Keyboard focus on boundary trip. When either boundary trips, focus typically falls back to because the throwing subtree unmounted. Keyboard users would have to tab from the top to reach the recovery action. Add a ref on the CTA button and a componentDidUpdate gate that focuses it on the false-to-true hasError transition. The gate fires once per trip, not on every error-state re-render, so a user who tabbed elsewhere within the card does not get focus stolen back. Verified the gate also fires on a re-error after Try again (setState({hasError:false}) re-renders with prevState.hasError=false, the next throw flips to true and the transition condition triggers). Cross-tab dismissal sync in useDismissalState. The hook previously read localStorage only in the lazy initializer, so dismissing in tab A did not propagate to tab B until tab B re-mounted. Add a useEffect that listens for storage events on the configured key. The browser fires storage events only in OTHER tabs than the one that wrote the change, so this handles the tab B receives tab A's dismiss case; same-tab updates flow through setDismissed directly, unchanged. Malformed event.newValue (NaN, empty string) defaults to dismissed= false, the conservative outcome. Adds 4 new vitest cases for the storage-event paths: recent timestamp, null newValue (restore), unrelated key, and stale timestamp. --- frontend/src/components/ErrorBoundary.tsx | 102 ++++++++++++------ frontend/src/components/LazyBoundary.tsx | 21 +++- .../hooks/__tests__/useDismissalState.test.ts | 68 ++++++++++++ frontend/src/hooks/useDismissalState.ts | 50 ++++++--- 4 files changed, 195 insertions(+), 46 deletions(-) diff --git a/frontend/src/components/ErrorBoundary.tsx b/frontend/src/components/ErrorBoundary.tsx index 6b4211ee..2a672be8 100644 --- a/frontend/src/components/ErrorBoundary.tsx +++ b/frontend/src/components/ErrorBoundary.tsx @@ -1,47 +1,87 @@ -import { Component } from 'react'; +import { Component, createRef } from 'react'; import type { ErrorInfo, ReactNode } from 'react'; +import { AlertTriangle } from 'lucide-react'; +import { Button } from '@/components/ui/button'; interface Props { - children: ReactNode; + children: ReactNode; } interface State { - hasError: boolean; - error: Error | null; + hasError: boolean; + error: Error | null; } +/** + * App-level catch-all error boundary. Sits above feature-specific + * boundaries (e.g. `LazyBoundary` around `` blocks) so any + * uncaught render error in a non-lazy subtree still produces a + * consistent recovery card instead of a blank page. + * + * Visually matches `LazyBoundary`: glass card, AlertTriangle icon, + * single Try-again CTA. The shared aesthetic is intentional so a user + * never sees two different "something went wrong" treatments depending + * on which boundary catches the error. + * + * "Try again" resets the boundary's state, which causes React to + * re-render the children. For deterministic errors this just shows the + * card again, which is the expected behavior of any error boundary; + * for transient errors (stale API response, race) it can recover + * cleanly. + */ class ErrorBoundary extends Component { - public state: State = { - hasError: false, - error: null, - }; + public state: State = { + hasError: false, + error: null, + }; - public static getDerivedStateFromError(error: Error): State { - return { hasError: true, error }; - } + /** + * Ref on the CTA button so we can focus it when the boundary trips. + * When a render error fires, focus is typically inside the now- + * unmounted subtree and falls back to ; keyboard users would + * otherwise have to tab from the top to reach the recovery action. + */ + private ctaRef = createRef(); - public componentDidCatch(error: Error, errorInfo: ErrorInfo) { - console.error('ErrorBoundary caught an error:', error, errorInfo); - } - - public render() { - if (this.state.hasError) { - return ( -
-

Something went wrong

-

{this.state.error?.message || 'Unknown error'}

- -
- ); + public static getDerivedStateFromError(error: Error): State { + return { hasError: true, error }; } - return this.props.children; - } + public componentDidCatch(error: Error, errorInfo: ErrorInfo) { + console.error('ErrorBoundary caught an error:', error, errorInfo); + } + + public componentDidUpdate(_prevProps: Props, prevState: State) { + if (!prevState.hasError && this.state.hasError) { + this.ctaRef.current?.focus(); + } + } + + public render() { + if (!this.state.hasError) return this.props.children; + + return ( +
+
+
+ +
+
+

Something went wrong

+

{this.state.error?.message || 'Unknown error'}

+
+ +
+
+ ); + } } export default ErrorBoundary; diff --git a/frontend/src/components/LazyBoundary.tsx b/frontend/src/components/LazyBoundary.tsx index 93e150ba..0b328788 100644 --- a/frontend/src/components/LazyBoundary.tsx +++ b/frontend/src/components/LazyBoundary.tsx @@ -1,4 +1,4 @@ -import { Component } from 'react'; +import { Component, createRef } from 'react'; import type { ErrorInfo, ReactNode } from 'react'; import { AlertTriangle } from 'lucide-react'; import { Button } from '@/components/ui/button'; @@ -37,6 +37,14 @@ class LazyBoundary extends Component { error: null, }; + /** + * Ref on the CTA button so we can focus it when the boundary trips. + * When a render error fires, focus is typically inside the now- + * unmounted subtree and falls back to ; keyboard users would + * otherwise have to tab from the top to reach the recovery action. + */ + private ctaRef = createRef(); + public static getDerivedStateFromError(error: Error): State { return { hasError: true, error }; } @@ -45,6 +53,15 @@ class LazyBoundary extends Component { console.error('LazyBoundary caught an error:', error, errorInfo); } + public componentDidUpdate(_prevProps: Props, prevState: State) { + // Focus the CTA the moment the error UI mounts, not on every + // re-render of the error state, so we don't steal focus from a + // user who has already tabbed elsewhere within the card. + if (!prevState.hasError && this.state.hasError) { + this.ctaRef.current?.focus(); + } + } + public render() { if (!this.state.hasError) return this.props.children; @@ -68,7 +85,7 @@ class LazyBoundary extends Component {

{title}

{body}

- diff --git a/frontend/src/hooks/__tests__/useDismissalState.test.ts b/frontend/src/hooks/__tests__/useDismissalState.test.ts index 43a8f409..33dd93cd 100644 --- a/frontend/src/hooks/__tests__/useDismissalState.test.ts +++ b/frontend/src/hooks/__tests__/useDismissalState.test.ts @@ -89,4 +89,72 @@ describe('useDismissalState', () => { expect(a.current.dismissed).toBe(true); expect(b.current.dismissed).toBe(false); }); + + it('syncs to dismissed=true when another tab fires a storage event with a recent timestamp', () => { + // Browsers fire `storage` events only in OTHER tabs than the one + // that wrote the change, so this test simulates "tab B receives + // a dismiss from tab A" by dispatching a synthetic StorageEvent. + const { result } = renderHook(() => useDismissalState(KEY)); + expect(result.current.dismissed).toBe(false); + + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: KEY, + newValue: String(Date.now()), + }), + ); + }); + + expect(result.current.dismissed).toBe(true); + }); + + it('syncs to dismissed=false when another tab fires a storage event with a null newValue', () => { + // null newValue is what the spec emits when localStorage.removeItem + // is called in another tab. + localStorage.setItem(KEY, String(Date.now())); + const { result } = renderHook(() => useDismissalState(KEY)); + expect(result.current.dismissed).toBe(true); + + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: KEY, + newValue: null, + }), + ); + }); + + expect(result.current.dismissed).toBe(false); + }); + + it('ignores storage events for unrelated keys', () => { + const { result } = renderHook(() => useDismissalState(KEY)); + + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: 'unrelated-key', + newValue: String(Date.now()), + }), + ); + }); + + expect(result.current.dismissed).toBe(false); + }); + + it('treats a storage event carrying a stale timestamp as not-dismissed', () => { + const { result } = renderHook(() => useDismissalState(KEY)); + + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: KEY, + newValue: String(Date.now() - DISMISS_DURATION_MS - 1), + }), + ); + }); + + expect(result.current.dismissed).toBe(false); + }); }); diff --git a/frontend/src/hooks/useDismissalState.ts b/frontend/src/hooks/useDismissalState.ts index 5e36f28a..608efd71 100644 --- a/frontend/src/hooks/useDismissalState.ts +++ b/frontend/src/hooks/useDismissalState.ts @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; const DISMISS_DURATION_MS = 24 * 60 * 60 * 1000; @@ -11,20 +11,36 @@ const DISMISS_DURATION_MS = 24 * 60 * 60 * 1000; * lazy initializer treats it as expired and returns `dismissed: false`, * so the gate shows the full upsell again on next mount. * - * Each hook instance owns its own React state. If two `` - * mounts share the same key in the same tab, dismissing one does not - * sync to the other until that other re-mounts. Cross-instance pub-sub - * is not provided; the dismissal is a soft UX nicety, not a security - * boundary. + * Cross-tab sync: a `storage` event listener on `window` propagates + * dismiss / restore actions from other tabs. The browser fires + * `storage` events only on tabs OTHER than the one that wrote the + * change, so this listener handles tab B updates after tab A + * dismisses or restores; same-tab updates flow through `setDismissed` + * directly. Stale timestamps that arrive past the 24h window are + * treated as not-dismissed. */ export function useDismissalState(key: string) { - const [dismissed, setDismissed] = useState(() => { - const dismissedAt = localStorage.getItem(key); - if (!dismissedAt) return false; - const ts = Number.parseInt(dismissedAt, 10); - if (!Number.isFinite(ts)) return false; - return Date.now() - ts < DISMISS_DURATION_MS; - }); + const [dismissed, setDismissed] = useState(() => readDismissedFromStorage(key)); + + useEffect(() => { + const onStorage = (event: StorageEvent) => { + if (event.key !== key) return; + // event.newValue is null when the entry was removed (restore) + // and a string when it was set (dismiss). + if (event.newValue === null) { + setDismissed(false); + return; + } + const ts = Number.parseInt(event.newValue, 10); + setDismissed(Number.isFinite(ts) && Date.now() - ts < DISMISS_DURATION_MS); + }; + window.addEventListener('storage', onStorage); + return () => window.removeEventListener('storage', onStorage); + // setDismissed is stable per React's setter guarantee; the handler + // closes over `key` only. Do not add setDismissed (harmless) or + // dismissed (would re-attach the listener on every state change) + // to satisfy a future drive-by exhaustive-deps "fix." + }, [key]); const dismiss = () => { localStorage.setItem(key, Date.now().toString()); @@ -38,3 +54,11 @@ export function useDismissalState(key: string) { return { dismissed, dismiss, restore }; } + +function readDismissedFromStorage(key: string): boolean { + const dismissedAt = localStorage.getItem(key); + if (!dismissedAt) return false; + const ts = Number.parseInt(dismissedAt, 10); + if (!Number.isFinite(ts)) return false; + return Date.now() - ts < DISMISS_DURATION_MS; +}