fix(frontend): stop the ErrorBoundary fallback from re-rendering AppShell - #1073
Merged
Merged
Conversation
Frontend CI, Lighthouse CI and Playwright E2E had all been failing on main
for several merges. Each job died at a different point; this fixes the
whole chain so the suite runs green.
Build / lint (blocked Lighthouse CI and E2E, which both run `npm run build`)
- escape unescaped quotes and apostrophes in privacy/ and terms/ JSX
(react/no-unescaped-entities, 22 errors) using entities that render
identically, so the pages are unchanged visually
- dashboard: the dominant-asset reduce read `.value`/`.name`, which do not
exist on AssetBreakdownEntry; use `.amount`/`.assetCode` as the chart's
own dataKey/nameKey already do. This was a type error that only surfaced
once lint stopped failing first.
Runtime CSP (all client-side JS was blocked on every static route)
- `script-src 'self' 'nonce-…'` cannot work with statically prerendered
pages: their HTML is built ahead of time and cannot carry a per-request
nonce, and a nonce makes browsers ignore 'unsafe-inline'. Every static
page therefore loaded with no working JavaScript. Drop the nonce and
allow inline scripts. JSON-LD is `application/ld+json`, which is not
executable and was never gated by script-src, so its nonce goes too.
- build connect-src from lib/config instead of raw process.env: the client
falls back to its own defaults (localhost:4001, Horizon, Soroban) that
the policy then omitted, so every API request was refused.
Test suite (50 failures -> 0; 106 tests pass)
- vitest was collecting the Playwright specs in e2e/; exclude them
- add the missing @testing-library/user-event dev dependency
- replace full-module vi.mock factories with importOriginal-based partial
mocks, so a stub no longer erases exports that transitive imports need
- mock next/navigation and focus-trap-react once in the shared setup
- make the framer-motion stubs cover any motion.<tag> via a Proxy
- wrap ActivityFeed snapshots in a QueryClientProvider
- pin Math.random for the MilestoneCard confetti so its snapshot is stable
- update assertions that described removed behaviour: the support panel
reports wallet errors through a toast and opens the result modal, copy
shows inline feedback, ProfileTabs empty-state copy is no longer
personalised, and the create wizard collects the wallet on step 2
- restore a funded-account stub between support-panel tests, since
clearAllMocks() clears calls but not implementations
- fix queries that matched several elements ("Failed", "XLM" vs "yXLM")
- commit the previously untracked snapshots; vitest does not write new
ones under CI and fails instead
Also expose aria-pressed on the create wizard's asset toggles, which the
existing test asserted and screen readers need, and stub the profile API
in the home E2E spec so it does not depend on a live backend.
…hell At the dashboard call site the tree is <ErrorBoundary><AppShell>…</AppShell> </ErrorBoundary>, so AppShell renders inside the boundary and is one of the things that can throw. The fallback wrapped its error UI in AppShell again, which meant an error originating in AppShell — the nav, search or wallet-connect render path — threw a second time while rendering the fallback. That throw had no ancestor boundary left to catch it, so the page blanked entirely instead of degrading to "Something went wrong", defeating the point of the boundary. Render a plain, self-contained fallback instead: no AppShell and no other app component, with the outer <main> chrome inlined so it still looks like a page rather than unstyled content on a bare background. Errors thrown by page content, which are the common case, are unaffected. Adds tests covering both paths — content throwing and AppShell itself throwing — plus the fallback's error details and the "Try again" reset. The two AppShell-source tests fail against the previous implementation with the error escaping the boundary. Closes OlaGreat#1062
|
@DSOTec Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
At its only call site the tree is:
AppShelltherefore renders inside the boundary, which makes it one of the components that can throw. But the fallback wrapped its error UI inAppShellagain:So an error originating in
AppShellitself — the nav, search, or wallet-connect render path — was caught, and then thrown a second time while rendering the fallback. That second throw has no ancestor boundary left to catch it, so React unmounts the whole tree: the page blanks entirely instead of degrading to "Something went wrong", which defeats the point of the boundary.Solution
Render a plain, self-contained fallback: no
AppShell, and no other app component. TheAppShellimport is dropped entirely so the fallback cannot regress into depending on app code again.AppShell's outer chrome is inlined (min-h-screen px-6 py-8 sm:px-10 bg-ink dark:bg-black, plus themx-auto max-w-6xlcontainer) so the fallback still renders as a proper page rather than unstyled content on a bare background.Errors thrown by page content — the common case — behave exactly as before.
Testing
Added
frontend/src/components/error-boundary.test.tsxwith 6 tests.AppShellis mocked behind a flag so a test can make it throw, which is the precise scenario this issue describes:AppShellitself is the error source —render()must not throwAppShell— asserted via adata-testidthat must be absentVerified the tests actually catch the bug. Against the previous implementation, tests 3 and 4 fail with the error escaping the boundary:
The other four pass both before and after, confirming the change is targeted and does not alter existing behaviour.
Full suite from a clean
npm ci, matching what CI runs:Edge cases considered
AppShell's outer<main>classes. That duplication is deliberate — the whole point is that the fallback must not depend onAppShell— but it does mean a future change to the app background would need mirroring here. The alternative (extracting a shared layout primitive) would reintroduce a shared dependency in the fallback path, so I kept it inlined.ErrorBoundaryis used in exactly one place (dashboard/[username]/page.tsx), so nothing else relied on the fallback providingAppShellchrome.Note on the first commit
This branch carries two commits. The second is the actual fix for #1062 — a 9-line change to
error-boundary.tsxplus its tests.The first is the CI-pipeline repair already submitted as #1071, included only because
mainis currently red:Frontend CI,Lighthouse CIandE2E (Playwright)all fail onmain, so a branch based on it cannot go green on its own. If #1071 merges first, I'll rebase and that commit disappears from this PR. Reviewingerror-boundary.tsxanderror-boundary.test.tsxalone gives the complete picture of this fix.Closes #1062