Skip to content

fix(islands): wrap every client island in ErrorBoundary, not just AppIsland - #227

Open
nastaso wants to merge 2 commits into
mainfrom
fix/island-error-boundaries
Open

fix(islands): wrap every client island in ErrorBoundary, not just AppIsland#227
nastaso wants to merge 2 commits into
mainfrom
fix/island-error-boundaries

Conversation

@nastaso

@nastaso nastaso commented Aug 4, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Closes #70. Reuses the existing ErrorBoundary (no new component, no new dependency) and wraps every top-level client island root, so a render throw in any island degrades to the existing recovery card and logs via logError, instead of blanking the region silently.

Wrapped (first pass, matching #70's own list): CertDashboardIsland, HeaderInteractive, FooterIsland, DonateButtonIsland, CookieConsentIsland, AccountLinkNoticeIsland.

Wrapped (second pass, after a codex adversarial audit found #70's own list was incomplete): AuthLinkNotice (index.astro), DomainProgressStrip ([domain].astro), BlogShareButton (BlogLayout.astro), AnalyticsBootstrap (BaseLayout.astro). None of these four were named in #70 - I verified them with a full repo grep for every client:only/client:idle/client:load mount in src/, which turned up exactly these 4 gaps and no more.

Already had it, untouched:

No island hydration directives (client:idle/client:only) or island logic changed anywhere - only an import + a JSX wrap around each existing return (or, for the 4 multi-early-return components, a rename-and-wrap: the original function becomes an unexported *Inner, and the exported name becomes a thin <ErrorBoundary> wrapper around it, so no internal branching had to be touched).

Notes

  • CookieConsentIsland is currently unmounted in BaseLayout.astro (dormant - see the comment above the GA script block). The wrap is harmless and future-proofs it; no live effect today.
  • AnalyticsBootstrap always renders null - the wrap still protects against a synchronous render-phase throw before that return, for consistency with every other island's policy.

Testing

This repo's vitest environment is node (no DOM) and has no react-test-renderer / @testing-library/react (AGENTS.md: no new dependencies). I checked both renderToStaticMarkup and renderToPipeableStream by hand before writing the test - neither recovers inline from a synchronous root-level throw the way the client reconciler does, so an end-to-end "mount and throw" test isn't possible here without a new dependency.

Instead src/components/ErrorBoundary.test.tsx drives ErrorBoundary's own lifecycle methods directly (getDerivedStateFromError, componentDidCatch, render()) - the exact methods React itself calls - and asserts on the returned element tree. Mutation-tested by hand: changed render() to always return children regardless of hasError, confirmed the "renders the recovery card instead of blanking" test goes red, reverted, confirmed green again.

  • npm run check (typecheck + lint + unit tests): 0 errors, 0 warnings, 304/304 tests pass (added 4).
  • npm run build: full guard chain green (citation, internal-graph, person-graph, check:seo-head, csp:hash, cf:headers).
  • PW_PORT=4399 PW_REUSE_SERVER=0 npm run e2e: 109/109 non-skipped tests pass (20 skipped need real Supabase creds, same as baseline) - re-run after the second-pass fixes too.

Checklist

  • npm run check passes locally
  • npm run build passes locally (full guard chain)
  • npm run e2e passes locally (isolated port)
  • No new third-party dependencies
  • No changes to island hydration directives or island logic - only the ErrorBoundary wrap

…Island

CertDashboardIsland, HeaderInteractive, FooterIsland, DonateButtonIsland,
CookieConsentIsland, and AccountLinkNoticeIsland previously mounted with no
boundary, so a render throw blanked the region silently instead of degrading
to the recovery card. StatsIsland already had this (PR #145) and AppIsland's
own boundary is unchanged.

Closes #70.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploying cloudcertprep with  Cloudflare Pages  Cloudflare Pages

Latest commit: deba5ea
Status: ✅  Deploy successful!
Preview URL: https://3c4c93c0.cloudcertprep.pages.dev
Branch Preview URL: https://fix-island-error-boundaries.cloudcertprep.pages.dev

View logs

Codex adversarial audit (lens: find an island the change missed) found
AuthLinkNotice (index.astro), DomainProgressStrip ([domain].astro),
BlogShareButton (BlogLayout.astro), and AnalyticsBootstrap (BaseLayout.astro)
- none named in #70's own list, all genuinely top-level client:-mounted
components with no boundary. Verified independently with a full grep of
every client:only/client:idle/client:load mount in src/ before fixing;
that grep turned up exactly these 4 and no more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(islands): wrap every client island in ErrorBoundary, not just AppIsland

1 participant