fix: one dead subgraph no longer wipes out all payment data - #117
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Greptile SummaryThe PR isolates remote-schema failures so healthy chains continue returning payment and channel data.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Caller[Payment or SRF fetcher] --> FanOut[requestPerChain]
FanOut --> C1[Chain request 1]
FanOut --> C2[Chain request 2]
FanOut --> CN[Chain request N]
C1 --> Merge[Merge fulfilled results]
C2 --> Merge
CN --> Merge
Merge -->|At least one succeeds| Format[Existing formatter]
Merge -->|Every chain fails| Error[Propagate outage error]
Format --> UI[Partial or complete data]
Reviews (4): Last reviewed commit: "test: cover the per-chain merge behaviou..." | Re-trigger Greptile |
|
Fair, and accepted as a real limitation rather than fixed — here's the honest state of it. The smoke check was built to catch the class of bug in #116: a module-scope import throwing under Node, which turned every I tried to close the gap with a unit test of
A working version existed, but it needed a ~900-character Doing this properly means adding a test runner (vitest is the obvious pick — handles TS, ESM and module mocking natively, one devDependency) and that is a tooling decision for this repo, not something to slip into a bug-fix PR. This repo currently has zero tests, so there's no convention to follow either. Happy to add it in a follow-up PR if you want it — say the word. Worth noting either way that a CI unit test still wouldn't cover live merge behaviour against real subgraphs; that needs a mock backend in CI, which is a larger piece of work again. |
|
Fixed properly — I'd previously argued this was out of scope, and that call was overridden, so here it is with real coverage. Added vitest and five tests for
Rows 2 and 4 are the load-bearing ones — the entire point of the per-chain split is that one dead subgraph stops wiping out the other thirteen, and that a total outage stays distinguishable from "there is no data". I verified the tests can actually fail, twice and in different ways, because a test that can't fail would just recreate the blind spot you're reporting:
Both restored to byte-identical files afterwards, suite green again. Wired into CI before the smoke test, so a logic regression fails fast and with a clearer signal than a server probe. One note on why vitest rather than Node's built-in Being explicit about the remaining limit: this tests the merge logic in isolation, not live behaviour against real subgraphs. Catching that needs a mock backend in CI — a larger piece of work, and worth its own issue if you want it. |
bassgeta
left a comment
There was a problem hiding this comment.
Looks great, nice job cleaning up all the quirks of Hasura + the subgraphs, nice one 👌
Merge activity
|
Every `/request/<id>` URL on scan.request.network returns **HTTP 500**. This fixes it. Bottom of a 3-PR stack; independently mergeable.
Came out of an SEO audit (Linear REQ-285) that listed 132 request URLs returning 5XX. It is not 132 broken pages — it is one bug, and the set is unbounded: a request created minutes ago 500s just the same.
## Root cause
`html2pdf.js` **0.10.2** — the version `package-lock.json` pins — invokes its UMD wrapper with a bare `self` identifier, evaluated before the inner `require()`s and with no `typeof self !== "undefined"` guard. Node has no global `self`, so *importing* it server-side throws. It was imported at module scope in `use-export-pdf.tsx`, whose only importer is `request/[id]/page.tsx` — which is exactly why that one route 500ed while every other returned 200.
Reproduced locally before the fix:
```
ReferenceError: self is not defined
at 11378 (.next/server/app/request/[id]/page.js)
at Object.t [as require] (.next/server/webpack-runtime.js)
```
The import now happens inside the export handler, so it only evaluates in a browser.
**Impact worth stating precisely:** Next.js bails out to client rendering after the SSR throw, so a real browser renders the page fine — users clicking through from the homepage were unaffected. Crawlers, shared links, hard refreshes and anything status-code-sensitive got a 500 with no content.
**Side benefit:** the route's page bundle drops **196 kB → 22.8 kB** and first-load JS **1.02 MB → 851 kB**, since html2pdf no longer ships eagerly.
## Why CI never caught this
`next build` compiles the route but never requests it — `page.tsx` has no `generateStaticParams`, so the build marks it `ƒ (Dynamic) server-rendered on demand`. `npm run check` is `biome check --write`, formatting only, and `--write` means it cannot fail on anything auto-fixable. There are no tests in the repo. PR #117 adds the request-time smoke check that would have caught it.
## Also in this PR
All found while tracing the above:
- **Error boundaries.** There were none — no `app/error.tsx`, no `global-error.tsx`, no root `not-found.tsx` (`app/not-found/page.tsx` is an ordinary route, not the convention file). Any render-time throw escaped to a raw 500. Added all three; neither leaks an error message or stack.
- **A missing request reported 200.** The page called `redirect("/not-found")` — a 307 to a page that itself returns 200, i.e. a soft 404 telling crawlers a nonexistent request exists. Now `notFound()`.
- **"Failed" was indistinguishable from "not found."** The page read only `isLoading`, never `error`, so a backend failure rendered as "not found". Now gates on the query's `status`: pending → skeleton, error → error state, success-and-empty → `notFound()`. `isLoading` was doubly wrong here — with react-query v5 it is `isPending && isFetching`, and no fetch happens during SSR, so it is **false** on the server and the skeleton guard never fired there.
- **Error states in the three "Recent …" widgets**, which ignored the `status` their hooks already return and rendered "No data" on failure. This is live today: the payments subgraph returns `no allocations`, so the home page currently reports empty rather than broken.
- **Two crashers guarded.** The fully-unguarded `extensionsData[0].parameters` path in `request-table.tsx` throws while evaluating arguments, and therefore *outside* the try/catch inside `calculateShortPaymentReference` — the likely cause of **#89**. And `BigInt(Number(gasUsed) * Number(gasPrice))` in `payment-table.tsx`, where a missing field makes `BigInt(NaN)` throw a `RangeError`; it now multiplies in BigInt, which also avoids losing precision above `Number.MAX_SAFE_INTEGER`.
- **Dropped `@react-pdf/renderer`**, imported nowhere. The lockfile is regenerated to match — worth noting because `npm ci` *fails* on a package.json/lockfile mismatch, so CI would have broken on its first job otherwise.
Likely also closes **#72** (an unindexed request showing as "404") now that error and not-found are distinct states.
## Verification
`npm ci` → `npm run check` → `npm run build`, then a real server:
| Route | Before | After |
|---|---|---|
| `/request/<id>` | **500** | **200** |
| `/`, `/requests`, `/payments` | 200 | 200 |
| unmatched route | — | 404 |
| `self is not defined` in log | present | **zero** |
## Known gap, fixed in #118
The page still fetches on the client, so the served HTML is a skeleton with no request data, and a nonexistent id returns 200 rather than 404. Both need a server-side fetch — that is #118.
The payment and SRF queries were single GraphQL documents aliasing 11-14
chain remotes at once. Hasura returns `data: null` when any one remote
errors, so a single unavailable chain destroyed the entire multi-chain
result.
That is not hypothetical — it is live on scan.request.network today:
fetchRequestPayments Error: subgraph not found: no allocations
field 'singleRequestProxyDeployments' not found in type: 'payment_zksynceraQuery'
Consequences, confirmed by loading the site: /payments renders
completely empty, "Recent payments" shows "No data", and every request
page reports `Balance: 0` no matter what was actually paid. Thirteen
healthy chains were being discarded because of one.
Each fan-out is now one request per chain via Promise.allSettled, merged
through the existing unchanged formatters. A rejected chain is skipped
and logged with its name; healthy chains still return their data. Only a
total failure of every chain throws — a full outage must not be
indistinguishable from "no results", and the error states added in the
previous commit surface it.
Output is byte-identical when all chains are healthy. Worth stating
plainly: today's behaviour is already per-chain top-N, not global top-N —
`first: 10` across 14 chains can return 140 rows, because the formatters
merge and sort but never apply a global slice. That is preserved rather
than "fixed", since changing it would alter visible row counts and
pagination. It deserves its own decision.
Also here:
- Drop payment_zksyncera from the SRF queries only. That subgraph
genuinely lacks the singleRequestProxyDeployments entity, so querying
it could only ever error. Expressed as data in consts.ts via the
pre-existing but unused PAYMENT_CHAINS enum. Note SRF_CHAIN_REMOTES is
not "all chains minus zksyncera" — the SRF queries never covered
fantom, fuse or moonbeam, so an explicit list was the only way to
preserve today's coverage exactly.
- src/lib/hooks/payments.ts was a byte-for-byte duplicate of
queries/payments.ts whose fetchPayments swallowed errors and returned
[], making failure indistinguishable from empty at the query layer.
Nothing imports it; it is now a thin re-export of the fixed
implementation rather than a second copy of the same bug.
- Add a CI smoke check that actually serves the app and requests
/request/<id>, failing on 5xx. This is the signal that was missing:
`next build` only compiles that route (no generateStaticParams, so it
is server-rendered on demand and never requested during the build),
`npm run check` is formatting only, and the repo has no tests. The
previous commit's bug would have been caught by this and by nothing
else in CI.
Trade-off accepted deliberately: up to 14 parallel HTTP round-trips per
logical query instead of 1. `revalidate: 60` and Hasura's `@cached`
blunt it, and no batching layer or dependency was added. Worth watching
Hasura rate limits.
Not addressed here: queries/transactions.ts and address-transactions.ts
have the same all-or-nothing fragility against the two storage remotes
(if storage_sepolia is down, mainnet requests vanish too). Left alone to
keep this diff scoped; they need the same treatment.
Restoring the missing subgraph itself is upstream indexer work, not a
change in this repo. This commit stops one dead chain from zeroing the
UI; it does not bring the data back.
Greptile P1 on the server-rendering PR: fetchRequest sent a single document covering both the storage and storage_sepolia remotes. Hasura nulls the whole document when any one remote errors, graphql-request then throws, and once that call moved into the server component a hiccup in the sepolia remote took down server rendering for a perfectly good mainnet request. The two remotes are now queried independently and settled with Promise.allSettled, so either one can fail without taking the other with it. The subtle part is what "not found" is allowed to mean. null from this function now drives notFound() plus robots: noindex, so it must mean "this request does not exist" and nothing else: - either remote returns the channel -> return it, even if the other failed - both answered, neither has it -> null, a genuine not-found - not found but a remote failed -> throw - both failed -> throw That third case is the one worth spelling out. "Absent" and "unavailable" are indistinguishable there, and returning null would tell a crawler that a request which may well be live does not exist, and noindex a real page. Throwing routes it to the error boundary instead, which keeps the invariant this stack already established: a backend failure is never reported as "not found". The thrown error names the remotes that failed so it stays diagnosable. The per-transaction JSON.parse and its try/catch fallback are unchanged, just factored into one helper instead of being duplicated per remote. Signature and returned shape are untouched, so no caller changes. Note this fragility was pre-existing and is not limited to this file — queries/transactions.ts and address-transactions.ts still send combined storage documents. Fixing them is follow-up work; this file is urgent because it is the one now on the server-rendering path.
Greptile P2: the smoke check only inspects HTTP status, so it stays green if the per-chain query or merge logic regresses and starts producing empty or partial payment data. Correct — that check was built to catch an import throwing under Node, and it cannot see this. Adds vitest and five tests for requestPerChain, covering the behaviours whose regression would otherwise be invisible: - every chain succeeds -> one entry per chain in the merged result - one chain rejects -> skipped, the others survive, no throw - one resolves with no bucket -> treated as a failure, still no throw - every chain fails -> throws, naming the failed chains - empty chain list -> does not throw The second and fourth are the load-bearing ones. The whole point of the per-chain split is that one dead subgraph stops wiping out the other thirteen, and that a total outage stays distinguishable from "there is no data". Verified the tests can actually fail, twice and in different ways: making a rejected chain merge in as an empty bucket reds the skip test, and removing the total-failure throw reds the all-fail test. Both restored to byte-identical files afterwards, suite green again. A test that cannot fail would just recreate the blind spot being reported. vitest was needed rather than node:test because src/lib/graphQlClient.ts calls React's cache() at module scope and the installed react@18.3.1 does not export it — that only works under Next, which aliases react to its own bundled fork — so any runner that lets the real module execute dies on load. vi.mock keeps it from ever being evaluated. graphql-request v7 is also ESM-only, which vitest handles natively. Wired into CI ahead of the smoke test, so a logic regression fails fast and with a clearer signal than a server probe. Worth being explicit about what this does not do: it tests the merge logic in isolation, not live behaviour against real subgraphs. Catching that needs a mock backend in CI, which is a larger piece of work.
954c535 to
49c6ea4
Compare
Makes the request page serve the actual request in its HTML, instead of an empty skeleton. Top of the stack, on #117. #116 stopped the 500s, but the page still served nothing useful: it fetched on the client, so the HTML was a skeleton with **zero** request data. For the SEO audit that started this, a 200 with an empty shell is barely better than the 500 it replaced. `page.tsx` is now an async server component that awaits `fetchRequest` and passes the `Channel` to a new client component, `request-details.tsx`, which keeps everything genuinely browser-bound: PDF export, the JSON viewer, clipboard buttons, `TimeAgo`, `useState`. The relocation is verbatim — same markup, classNames, copy, table structure. **Verified** against a local stand-in backend: the served HTML now contains the gateway, payee, payer, expected amount, payment reference, encryption status, and the transactions table. Before, none of it was there. Plus `generateMetadata`, so these pages finally have a real title and description for search results and link previews. ## Two findings worth reading ### json-edit-react cannot be server-rendered Its theme provider calls `document.documentElement.style.setProperty("--jer-highlight-color", …)` **during render**. The moment the details actually server-rendered, that threw `ReferenceError: document is not defined`, React bailed out to client rendering, and the HTML went straight back to being an empty shell — the twin of #116's bug, hidden until now because SSR had never got as far as rendering this component. It is now loaded via `next/dynamic` with `ssr: false`. The raw JSON blob is not the indexable content, so nothing is lost. Note this is a *render-time* touch of `document`, not an import-time one — which is why an import-level audit of the very same file came back clean. Worth remembering when auditing the next one. ### A missing request still returns 200, not 404 It renders the not-found UI, but the status is wrong, and this is a framework limit rather than something left undone. The route streams (`Transfer-Encoding: chunked`), so the HTTP status commits when the shell flushes — before the lookup resolves. `notFound()` can then only swap the rendered UI. Both placements were tried and measured: | Approach | Result | | --- | --- | | `notFound()` from the page body | 200 | | `notFound()` from `generateMetadata` | 200 | A real 404 needs the existence check to run before the response commits — i.e. middleware, with a Hasura call on every request. That is a bigger trade than this PR should make unilaterally, so I stopped and flagged it instead. **What I did instead addresses the actual harm:** a missing request is served `<meta name="robots" content="noindex, nofollow">`, which is what stops soft 404s accumulating in the search index. Valid requests are not noindexed — checked both ways. ## Implementation notes - The request lookup is wrapped in React's `cache()` keyed on the id, so `generateMetadata` and the page share **one** network call. The fetch-level cache cannot do this: `graphql-request` issues a POST, which Next's Data Cache does not dedupe, and the `cache()` inside `graphQlClient` is keyed on a fresh `RequestInit` per call. - A thrown fetch is deliberately left to propagate to the error boundary. A backend outage must not be reported as "this request does not exist" — that distinction was established in #116 and is preserved here. - The two dependent queries (payments, SRF deployments) stay client-side: they hang off a payment reference derived from the request, they are the polling surface via `refetchInterval`, and keeping them there guarantees neither can affect the HTTP status or block the request's own content. - **One deliberate behaviour change:** the page no longer blocks on a full-page skeleton while payments load. The four payments-derived cells — Status, Balance, Modified, and the payments table — render their own inline loading state. Without that they would briefly display *wrong* values (`Balance: 0`, Status taken from the last transaction instead of "Paid") before the query lands. Everything else renders immediately from the server. ## Verification `npm run check` ✅, `npm run build` ✅, `tsc --noEmit` clean, zero SSR errors in the server log, and against a local stand-in backend: | Check | Result | | --- | --- | | Request fields in served HTML | ✅ present (were absent) | | Missing request → `noindex` | ✅ | | Valid request → not noindexed | ✅ | | Missing request → 404 status | ❌ 200 — framework limit, documented above |

One unavailable subgraph currently destroys all payment data. Stacked on #116.
On #114 — do not merge it first
This touches the same six files as #114 (
xdai → gnosis,mainnet → ethereum). I originally suggested merging #114 first to avoid conflicts. Having read it, I withdraw that.#114 renames the Hasura remote-schema names inside the GraphQL documents (
payment_xdai→payment_gnosis,payment_mainnet→payment_ethereum). Production evidence says those remotes are stillpayment_xdai/payment_mainnet: the SRF validation error in the console today lists both in the query text and Hasura rejects onlypayment_zksyncera's inner field — and Hasura validates the whole document, so an unknown top-level field would have been reported too.So merging #114 as-is would take payment data from "broken on one chain" to "broken on every chain". Details and a suggested identifier-only fix are in a comment on #114. This PR does not need to wait for it.
The problem, live in production right now
The payment and SRF queries were single GraphQL documents aliasing 11-14 chain remotes at once. Hasura returns
data: nullwhen any one remote errors, so a single unavailable chain destroyed the entire result:Confirmed by loading the site:
/paymentsrenders completely empty, "Recent payments" shows "No data", and every request page reportsBalance: 0no matter what was actually paid. Thirteen healthy chains were being discarded because of one.For actual users this is worse than the 500 in #116 — an explorer that reports every request as unpaid.
The fix
Each fan-out is now one request per chain via
Promise.allSettled, merged through the existing unchanged formatters. A rejected chain is skipped and logged with its name; healthy chains still return data. Only a total failure of every chain throws — a full outage must not be indistinguishable from "no results", and #116's error states surface it.Output is byte-identical when all chains are healthy.
One thing worth knowing, preserved rather than "fixed": today's behaviour is already per-chain top-N, not global top-N —
first: 10across 14 chains can return 140 rows, because the formatters merge and sort but never apply a global slice. Changing that would alter visible row counts and pagination, so it deserves its own decision rather than riding along here.Also here
payment_zksyncerafrom the SRF queries only — that subgraph genuinely lacks thesingleRequestProxyDeploymentsentity, so querying it could only ever error. Expressed as data inconsts.tsvia the pre-existing but unusedPAYMENT_CHAINSenum. NoteSRF_CHAIN_REMOTESis not "all chains minus zksyncera": the SRF queries never covered fantom, fuse or moonbeam, so an explicit list was the only way to preserve today's coverage exactly.hooks/payments.tswas a byte-for-byte duplicate ofqueries/payments.tswhosefetchPaymentsswallowed errors and returned[], making failure indistinguishable from empty at the query layer. Nothing imports it; it is now a thin re-export rather than a second copy of the same bug./request/<id>, failing on 5xx. This is the signal that was missing for fix: stop every /request/<id> URL returning HTTP 500 #116's bug:next buildonly compiles that route,npm run checkis formatting only, and there are no tests. Dummy env, no secrets, polls for readiness rather than sleeping.Trade-off, accepted deliberately
Up to 14 parallel HTTP round-trips per logical query instead of 1.
revalidate: 60and Hasura's@cachedblunt it, and no batching layer or dependency was added. Worth watching Hasura rate limits.Not addressed
queries/transactions.tsandaddress-transactions.tshave the identical fragility against the two storage remotes — ifstorage_sepoliais down, mainnet requests vanish too. Left alone to keep this diff scoped; they need the same treatment.payments-subgraph/ indexer allocations). This PR stops one dead chain from zeroing the UI; it does not bring the data back.Balance: 0will persist until the subgraph is served again.Verification
npm run build✅,tsc --noEmitclean, biome clean, built query documents validated throughgraphql.parse. Not verified: happy-path merge against live data — the local Hasura URL is a stand-in, so this is verified by construction and by its degradation behaviour, not against production data.