feat(ui): retry transient RPC failures in the error boundary - #1207
Open
bilhokista wants to merge 4 commits into
Open
feat(ui): retry transient RPC failures in the error boundary#1207bilhokista wants to merge 4 commits into
bilhokista wants to merge 4 commits into
Conversation
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.
Closes #1080.
The core problem
The boundary's only recovery action was
window.location.reload(). For a testnet node returning 504 for a moment, that throws away every piece of client state to recover from a blip — and until the reader clicks it, the subtree stays dead, becausehasErrorwas never cleared.The boundary can now recover in place: clearing
hasErrorremounts the subtree, which re-runs whatever request failed.Automatic backoff for transient failures
shouldAutoRetrygates retries on both a budget (3) and the error being transient. Delays are 500ms → 1s → 2s, capped at 8s, with ±25% jitter — without jitter every open tab hits a recovering node at the same instant.While a retry is pending the boundary shows a
role="status"panel naming the attempt, rather than an error the reader is being asked to act on when the app is already handling it.The classification is the part worth reviewing. Terminal contract failures are checked before transient patterns, because a revert message can mention a timeout in its own text —
"simulation failed: timeout in host function"must not be retried, since the node answered correctly and the answer was "no". Retrying that burns time and fees on an outcome that will not change. There is a test for exactly that string.The default is also deliberately conservative: an error matching nothing is treated as not retryable. Auto-retrying anything unrecognised risks looping on a real bug.
The RPC node switcher, and one thing I did not do
network.tshas a singleSOROBAN_RPC_URL, so there was no node list to switch between. I addedNEXT_PUBLIC_SOROBAN_RPC_URLS(comma-separated) which defaults to empty.I deliberately did not seed it with public third-party nodes. Which providers this app is willing to send user traffic to is an operator's decision about trust and rate limits, not a default that should arrive inside a UI component. So with no configuration the button is hidden — as it is when no
onEndpointChangehandler is supplied, because a switcher that changes an endpoint nothing listens to is a button that appears to work and does not.That means the switcher is inert until someone wires it up, which is a real limitation of this PR rather than something I want to bury. If you would rather I pick default fallback nodes, tell me which and I will add them.
Technical details drawer
Collapsed by default,
aria-expandedon the toggle, showing the stack, React's component stack and the active RPC endpoint.componentDidCatchnow stores the component stack instead of only logging it.Tests
frontend/tests/lib/stellar/rpcHealth.test.ts— 25 cases: the transient/terminal split across HTTP statuses and message shapes, the three client shapes a status can arrive in (status,statusCode,response.status), the revert-mentioning-timeout case, backoff values and the cap, jitter staying in band and never going negative, the budget, and endpoint parsing including duplicates, blank entries and an endpoint that has vanished from the list.Honest note on verification: I could not run Vitest locally (no full workspace install), so the suite above is unexecuted here — but this frontend does have Vitest configured, unlike some others, so it will run in CI as a normal test file rather than needing infrastructure first.
What I did run: all four files parse with the TypeScript compiler API, and
rpcHealth.tswas transpiled and executed standalone against 39 assertions covering the same ground — all passed.The boundary component itself has no test here. Testing a React error boundary needs a throwing child and fake timers for the backoff, which is worth doing but is a larger piece; I would rather land the policy with the pure module fully covered than ship a shallow component test. Happy to add it if you want it in this PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CrfEY1tvXrbeMDAUzxfuk7