fix(error): stop the boundary stranding admins and misreporting the write - #81
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Comment |
…rite The route error boundary was a member-shaped page regardless of who hit it or where. Four changes, each backed by a measurement rather than an assumption about what the framework does: - Chrome follows the section. `usePathname()` is the only session-free signal a client boundary has; `/admin/*` and `/payouts/*` are both behind guards the member cleared at their last render, so the path is evidence. An admin now keeps the admin nav and lands back on Members instead of four hops away at /account. The reasoning, including what a de-role costs, is written down in the file. - The lede stops claiming "The request didn't go through." A throw while enqueueing follows a committed write, so the sentence is false in the case this file's own comment describes. The fault assignment survives verbatim; the freed clause tells the member to check before resending. - The digest moves inside the instruction that asks for it, as a `code` rather than a `mono` run over English prose, with a branch for the client-side throws measured to arrive without one. - Retry gets a pending state via `aria-busy`, using the styling that already exists. A failed `reset()` remounts the boundary (measured), which re-runs FocusHeading and re-announces — the confirmation a screen-reader user previously never got. The same remount rules out an attempt counter; recorded as the reason rather than left to be rediscovered. Also records the "no global-error.tsx" decision durably, and pins the hoisted <title> behaviour, which is the opposite of what the segment-scoped not-found was measured doing. e2e/error-boundary.spec.ts covers all seven behaviours against real throws.
d5d0958 to
717e3b9
Compare
|
Rebased onto The conflict was one paragraph in #80 removed this spec's throw trigger. The replacement is the technique the admin test already used: rename a table out from under a page, restore in
Re-measured rather than assumed. The title test only proves "beats" if the static metadata actually resolves through the throw, so I stripped the hoisted The metadata does resolve; the hoisted title genuinely wins. Restored, and the assertion is not vacuous. The "Back to Payouts" test now follows the link only after the table is restored, so it asserts the escape reaches a working list rather than bouncing off the same throw. Re-verified on the new base
All 7 boundary tests pass on the new trigger, as do #80's own additions to |
1 participant
Design-sweep HANDOVER items 1 and 2 — both target
src/app/error.tsx, done as one pass. (Item 1's other half, the not-found boundaries, shipped in #78.)What changed
Chrome follows the section (finding 4). The boundary was a member-shaped page for everyone.
usePathname()is the only session-free signal a client component has, and it's whatAdminNavis already built on —ADMIN_ITEMSmatches itsITEMSlist exactly, "Members" naming included (WCAG 3.2.4: the same route must not acquire a second name because the page under it threw).The honesty question is answered in the file rather than left to the next reader: the path is evidence, not proof.
/admin/*is behind the admin guard,/payouts/*behindrequirePayoutReader— a member standing on either cleared that guard at their last render. The gap is a de-role landing between that render and the throw, and what that member gets is links that redirect to/account, which is exactly where the un-branched boundary sent them anyway. Bad case costs one hop; good case stops stranding an admin four destinations from the section they were working in.Deliberately the opposite answer from
not-found.tsx, which shows minimal nav because an unrouted URL is no evidence at all.The lede stops claiming the write failed (critique #2). "The request didn't go through" is a claim this component has no evidence for — it holds a thrown Error and an optional digest. This file's own comment names the counterexample: a DB hiccup while enqueueing is a throw that follows a committed tier change. The critique's replacement copy was judged, not transcribed: the fault assignment is kept verbatim (it's the best sentence on the page) and the freed clause spends itself on the one thing that changes the member's next move — check whether it took effect before resending.
The digest moves into the instruction that asks for it (findings 6, 10). It was a separate
dim monoline below the alert with no instruction attached. Now it's inside the sentence, as<code className="mono">— only the value is monospaced, the prose around it stays proportional, which is DESIGN.md's split run the right way round. Plus a branch for the no-digest case: a client-side throw was measured arriving without one, and the old markup dropped the whole line and left "tell an admin" pointing at nothing.Retry gets a pending state (finding 3), via
aria-busyon the existing.btn[aria-busy="true"]styling. Nodisabled— that would move focus to<body>at the moment the member is waiting to hear something.Measurements
Everything below was taken with throwaway instrumented probes, since removed. The reports predate #71/#74/#75/#78 and several of their premises no longer hold.
<title>inerror.tsxdoes win over the failed segment'smetadata— measured"Something broke · Zoo Landers"on/payouts/not-a-uuid, whosepage.tsxexportstitle: "Payout operation". This is the opposite of what fix(404): ship the app's own not-found boundaries, with focus that survives a soft nav #78 measured for the segment-scopednot-found.tsx, so both files now carry cross-referencing comments warning against generalising.usePathname()resolves inside the root error boundary.reset()remounts the boundary rather than reusing it — an in-component attempt counter went 1 → 0 across one press. An "it still hasn't cleared" escalation would needsessionStoragekeyed on the digest; not built, and the reason is recorded so it isn't rediscovered.useTransition's pending state is observable before that remount, soaria-busyfires and can't get stuck.Measurement 3 killed the critique's retry-counter proposal. It also pays for itself: the remount re-runs
FocusHeading, so a failed retry re-focuses the h1 and re-announces — the only confirmation a screen-reader user gets that the press landed and produced the same answer. That's the compensating win, and it's tested.global-error.tsx(finding 9): not built, per the report's own verdict, and the decision is now recorded durably inerror.tsx's docblock. It catches only aRootLayoutthrow (font registration and a<body>, self-hosted at build time — no request-time failure path) and a throw insideerror.tsxitself. Covering them means emitting its own<html>/<body>, rendering withoutglobals.cssand both faces: a permanently off-brand second copy of the shell for a failure this layout can't produce.Dropped, with reasons
payouts/loading.tsx) — an addition to a different segment, not a fix to this file, and a separate synthesis item. Not in this pass.CopyAmountButtonto a shared primitive. That's shared-component work the sweep explicitly doesn't carry. The digest is ~10 characters and now sits in the sentence that asks for it, which is most of the value.overflow-wrap: anywhereon.mono— a global CSS change touching every mono surface in the app for a value that doesn't overflow.Try againfrom primary — kept as the gold ration. It's the only control that keeps the member where they are, and the Notice names it in its own words.Verification
typechecklintno-img-elementinui.tsx:84,login/page.tsx:42,81format:checktesttest:e2ebuilde2e/error-boundary.spec.tsis 7 new tests, all driving a real throw via/payouts/not-a-uuid(22P02, deterministic — a retry that might succeed would test nothing). The admin-section test can't use a URL, because every admin page validates its own params and none can be made to throw; it renamessync_runout from under/admin/sync, which reads that one table afterrequireAdminPage(), and renames it back infinally.The 5 pre-existing
e2e/not-found.spec.tstests are untouched and pass — including the soft-navigation one, whose deliberate absence of a title assertion was left alone.🤖 Generated with Claude Code