fix(stats): prevent SSR crash loop caused by Sentry consoleIntegration#2792
Merged
fix(stats): prevent SSR crash loop caused by Sentry consoleIntegration#2792
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Vitest-based unit testing and config, enforces a 10s timeout in serverFetch with tests, introduces additional logging and 404 handling in address/deployment pages, and disables Sentry's Console integration on the server. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/App
participant SF as serverFetch
participant API as External API
participant Logger as Logger
Client->>SF: serverFetch(url, init)
SF->>SF: merge headers (cf-connecting-ip, x-forwarded-for)
SF->>SF: start 10s timeout (AbortController)
SF->>API: fetch(url, { signal, cache: "no-store", headers })
alt API responds before timeout
API-->>SF: response
SF->>SF: clear timeout
SF-->>Client: return response
else timeout or external abort
SF->>API: abort triggered
API-->>SF: abort/error
SF->>Logger: log timeout/abort
SF-->>Client: throw AbortError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
baktun14
force-pushed
the
fix/improve-ssr-error-handling
branch
from
dfa24b7 to
f71583d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2792 +/- ##
=======================================
Coverage 53.83% 53.83%
=======================================
Files 1020 1020
Lines 23604 23604
Branches 5769 5769
=======================================
Hits 12708 12708
- Misses 9502 9511 +9
+ Partials 1394 1385 -9
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 37-44: The test overwrites globalThis.clearTimeout and doesn't
restore it, which can leak into other tests; modify the spec to save the
original clearTimeout (e.g., const originalClearTimeout =
globalThis.clearTimeout) before assigning clearTimeoutSpy and then restore it
after the test (either in a finally block inside the test or in an afterEach
hook) so that globalThis.clearTimeout is set back to originalClearTimeout;
reference the clearTimeoutSpy and setup(...) usage in the "clears timeout after
successful fetch" test when making the change.
- Line 21: Replace the hardcoded root describe label "serverFetch" with the
function's name property by changing the root test suite to use serverFetch.name
(i.e., use describe(serverFetch.name, ...)); ensure the symbol serverFetch is
imported or in scope in serverFetch.spec.ts so the name property resolves at
runtime.
- Around line 3-5: The tests currently use module-level mutable state via
headerValues (created with vi.hoisted) which is mutated inside setup; remove
this shared mutable variable and instead create per-test header state inside the
setup function (do not use vi.hoisted). Implement a setup function at the bottom
of the root describe that accepts a single inline-typed parameter, creates a
fresh headers object (e.g., local Record<string,string|null>) and any test
subject (serverFetch or similar), returns the test utilities (no explicit return
type), and performs cleanup locally so each test owns and resets its header
state; update any references to headerValues.values to use the local headers
from setup and remove the module-level headerValues/hoisted usage.
stalniy
approved these changes
Feb 20, 2026
Contributor
stalniy
left a comment
stalniy
left a comment
There was a problem hiding this comment.
@baktun14 I'd like to migrate stats to pure react app, only frontend without SSR and do pre-rendering using headless browser or cloudflare pre-render. This SSR issues take a lot of time, like proxying IPs, observability, memory leaks, slow response, etc. This should not be such a pain for so simple app
Disable Sentry's Console integration on the server to prevent console.error patching that crashes the process when Node's util.inspect fails on Next.js internal error objects during SSR timeouts. Add 10s AbortController timeout to serverFetch as defense in depth, and add 404 handling to the deployment detail page.
baktun14
force-pushed
the
fix/improve-ssr-error-handling
branch
from
f71583d to
5ab69fb
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/stats-web/src/lib/serverFetch.spec.ts (3)
3-5:⚠️ Potential issue | 🟡 MinorAvoid shared mutable header state in
setup.
headerValuesis module‑level state mutated per test; please move header state intosetupand removevi.hoistedusage so each test owns its own data. As per coding guidelines: “Usesetupfunction … avoid shared state.”Also applies to: 64-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` around lines 3 - 5, The test uses module-level mutable headerValues created with vi.hoisted (symbol: headerValues) which leads to shared state across tests; refactor by removing vi.hoisted and moving the header state into the per-test setup function (e.g., create a local headers object inside setup or return it from setup), update references in the spec to use the setup-scoped headers instead of the module-level headerValues, and ensure any mocks that previously accessed headerValues are reinitialized inside each setup so each test owns its own header data.
21-21:⚠️ Potential issue | 🟡 MinorUse
serverFetch.namefor the root describe label.This keeps refactors safe and aligns with project conventions. As per coding guidelines: “Use
<Subject>.namein the root describe suite description…”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` at line 21, Replace the hard-coded describe label string with the function's name by using serverFetch.name in the root describe call; locate the root test suite declaration describe("serverFetch", ...) and change it to describe(serverFetch.name, ...) so future refactors keep the test label in sync with the subject under test.
37-44:⚠️ Potential issue | 🟡 MinorRestore
globalThis.clearTimeoutafter the test.Leaving the mocked global in place can leak into subsequent tests.
🧹 Suggested fix
it("clears timeout after successful fetch", async () => { - const clearTimeoutSpy = vi.fn(clearTimeout); - globalThis.clearTimeout = clearTimeoutSpy; - - await setup({ url: "https://api.example.com/v1/test" }); - - expect(clearTimeoutSpy).toHaveBeenCalled(); + const originalClearTimeout = globalThis.clearTimeout; + const clearTimeoutSpy = vi.fn(originalClearTimeout); + globalThis.clearTimeout = clearTimeoutSpy; + try { + await setup({ url: "https://api.example.com/v1/test" }); + expect(clearTimeoutSpy).toHaveBeenCalled(); + } finally { + globalThis.clearTimeout = originalClearTimeout; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` around lines 37 - 44, The test replaces globalThis.clearTimeout with a spy but doesn't restore it; save the original clearTimeout before mocking (e.g., const originalClearTimeout = globalThis.clearTimeout), assign the spy to globalThis.clearTimeout as done now, and then restore globalThis.clearTimeout = originalClearTimeout after the test (use a finally block or afterEach) so the mock from the test (clearTimeoutSpy) does not leak into other tests; reference the existing symbols clearTimeoutSpy and setup in your change.
🧹 Nitpick comments (1)
apps/stats-web/src/lib/serverFetch.ts (1)
66-78: Consider preserving the 10s timeout even when a caller supplies a signal.Right now, providing
init.signalbypasses the internal timeout. If you still want a hard 10s guard, combine signals usingAbortSignal.any(), which is supported by the project's Node.js version (24.13.0+).♻️ Suggested adjustment
- const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 10_000); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + const signal = init?.signal + ? AbortSignal.any([init.signal, controller.signal]) + : controller.signal; try { return await fetch(url, { ...init, cache: "no-store", headers: mergedHeaders, - signal: init?.signal ?? controller.signal + signal }); } finally { clearTimeout(timeout); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.ts` around lines 66 - 78, The internal 10s timeout is skipped when callers pass init.signal; update serverFetch (the fetch wrapper that creates controller and timeout) to combine the internal timeout signal with any provided init.signal using AbortSignal.any([controller.signal, init.signal]) so the fetch call always uses a signal that aborts after 10s while still honoring caller cancellation; replace the current signal assignment (signal: init?.signal ?? controller.signal) with the combined AbortSignal.any expression and keep clearing the timeout in the finally block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 3-5: The test uses module-level mutable headerValues created with
vi.hoisted (symbol: headerValues) which leads to shared state across tests;
refactor by removing vi.hoisted and moving the header state into the per-test
setup function (e.g., create a local headers object inside setup or return it
from setup), update references in the spec to use the setup-scoped headers
instead of the module-level headerValues, and ensure any mocks that previously
accessed headerValues are reinitialized inside each setup so each test owns its
own header data.
- Line 21: Replace the hard-coded describe label string with the function's name
by using serverFetch.name in the root describe call; locate the root test suite
declaration describe("serverFetch", ...) and change it to
describe(serverFetch.name, ...) so future refactors keep the test label in sync
with the subject under test.
- Around line 37-44: The test replaces globalThis.clearTimeout with a spy but
doesn't restore it; save the original clearTimeout before mocking (e.g., const
originalClearTimeout = globalThis.clearTimeout), assign the spy to
globalThis.clearTimeout as done now, and then restore globalThis.clearTimeout =
originalClearTimeout after the test (use a finally block or afterEach) so the
mock from the test (clearTimeoutSpy) does not leak into other tests; reference
the existing symbols clearTimeoutSpy and setup in your change.
---
Nitpick comments:
In `@apps/stats-web/src/lib/serverFetch.ts`:
- Around line 66-78: The internal 10s timeout is skipped when callers pass
init.signal; update serverFetch (the fetch wrapper that creates controller and
timeout) to combine the internal timeout signal with any provided init.signal
using AbortSignal.any([controller.signal, init.signal]) so the fetch call always
uses a signal that aborts after 10s while still honoring caller cancellation;
replace the current signal assignment (signal: init?.signal ??
controller.signal) with the combined AbortSignal.any expression and keep
clearing the timeout in the finally block.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
apps/stats-web/package.jsonapps/stats-web/src/app/addresses/[address]/deployments/[dseq]/page.tsxapps/stats-web/src/app/addresses/[address]/page.tsxapps/stats-web/src/instrumentation.tsapps/stats-web/src/lib/serverFetch.spec.tsapps/stats-web/src/lib/serverFetch.tsapps/stats-web/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- apps/stats-web/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/stats-web/src/instrumentation.ts
- apps/stats-web/package.json
- apps/stats-web/src/app/addresses/[address]/deployments/[dseq]/page.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/stats-web/src/lib/serverFetch.spec.ts (1)
3-5:⚠️ Potential issue | 🟡 MinorShared mutable
currentHeaders.refstate remains unaddressed.
currentHeaders.refis module-level state thatsetupmutates on entry (line 91) and clears on exit (line 98). Test isolation relies entirely on successful completion of the.then()callback; a rejectedserverFetchor a thrown assertion leavescurrentHeaders.refdirty for subsequent tests.As per coding guidelines, the
setupfunction must avoid shared state.Also applies to: 91-92, 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` around lines 3 - 5, The test uses shared mutable module-level state currentHeaders.ref which setup mutates and clears, risking leakage if serverFetch rejects; to fix, remove mutation of a hoisted module variable by creating headers locally inside setup (e.g., replace currentHeaders with a factory or move the Record<string,string|null> creation into the setup function) and ensure cleanup always runs (use a try/finally inside setup or register an afterEach that resets the headers) so currentHeaders.ref is never relied on across tests; update references to currentHeaders.ref and ensure serverFetch tests use the new local headers instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 96-100: Move the cleanup that restores globalThis.fetch and resets
currentHeaders.ref out of the .then() handler and into a .finally() callback on
the Promise returned by serverFetch so cleanup runs whether the call resolves or
rejects; specifically, keep the serverFetch(...) call and the .then(response =>
({ response, mockFetch })) to produce the return value, then chain .finally(()
=> { globalThis.fetch = originalFetch; currentHeaders.ref = {}; }) so
originalFetch and currentHeaders are always restored even on errors.
---
Duplicate comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 3-5: The test uses shared mutable module-level state
currentHeaders.ref which setup mutates and clears, risking leakage if
serverFetch rejects; to fix, remove mutation of a hoisted module variable by
creating headers locally inside setup (e.g., replace currentHeaders with a
factory or move the Record<string,string|null> creation into the setup function)
and ensure cleanup always runs (use a try/finally inside setup or register an
afterEach that resets the headers) so currentHeaders.ref is never relied on
across tests; update references to currentHeaders.ref and ensure serverFetch
tests use the new local headers instance.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/stats-web/src/lib/serverFetch.spec.ts (1)
3-5:⚠️ Potential issue | 🟡 MinorShared mutable state in
currentHeadersstill unresolved.
currentHeadersis module-level state mutated insidesetup(lines 91, 100). Each test should own its header state to avoid implicit coupling between tests.As per coding guidelines:
setupmust avoid shared state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` around lines 3 - 5, The module-level mutable currentHeaders (defined via vi.hoisted) is being mutated inside setup and causing shared state across tests; fix by removing the hoisted module-level currentHeaders and instead create a fresh headers object per test (e.g., instantiate currentHeaders inside setup or inside beforeEach) so each test gets its own Record<string,string|null>; update references to currentHeaders in the file to use the locally returned/created value from setup (or a helper makeCurrentHeaders function) and ensure no other code relies on the removed module-level symbol.
🧹 Nitpick comments (1)
apps/stats-web/src/lib/serverFetch.spec.ts (1)
7-17:vi.mock()violates the no-jest.mock()guideline; prefer DI +vitest-mock-extended.
vi.mock()is the Vitest equivalent ofjest.mock()— same module-hoisting mechanism, same testing anti-pattern. The coding guideline mandates passing mocks as constructor/function dependencies instead. CurrentlyserverFetchimportsnext/headersand@/services/diat module scope, making them non-injectable. To comply, extract those calls into injectable parameters (or adepsbag):// serverFetch.ts +type ServerFetchDeps = { + getHeaders?: () => Pick<ReadonlyHeaders, "get">; + getTraceData?: () => Record<string, string>; +}; -export async function serverFetch(url: string, init?: RequestInit): Promise<Response> { +export async function serverFetch(url: string, init?: RequestInit, deps?: ServerFetchDeps): Promise<Response> { + const headersGetter = deps?.getHeaders ?? headers; + const traceDataGetter = deps?.getTraceData ?? (() => errorHandler.getTraceData()); ... }Then in tests, drop
vi.mock()and pass mock implementations directly throughsetup:-vi.mock("next/headers", () => ({ headers: () => ({ get: ... }) })); -vi.mock("@/services/di", () => ({ errorHandler: { getTraceData: () => ({}) } })); ... function setup(input: { url: string; ... }) { ... - return serverFetch(input.url, ...) + return serverFetch(input.url, undefined, { + getHeaders: () => ({ get: (name: string) => input.headers?.[name] ?? null }), + getTraceData: () => ({}) + })Based on learnings: "Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/lib/serverFetch.spec.ts` around lines 7 - 17, serverFetch imports next/headers and "@/services/di" at module scope and tests use vi.mock(), which violates the no-mock guideline; refactor serverFetch to accept an injectable deps bag (e.g., an argument or setup function) that provides a headers getter and an errorHandler with getTraceData, replace module-scope calls to headers() and errorHandler.getTraceData() with calls to those injected functions, and then update tests to remove vi.mock() and pass vitest-mock-extended mocks (or plain mock objects) for the headers.get and errorHandler.getTraceData into the serverFetch setup/constructor so behavior is controlled via DI instead of vi.mock() calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 3-5: The module-level mutable currentHeaders (defined via
vi.hoisted) is being mutated inside setup and causing shared state across tests;
fix by removing the hoisted module-level currentHeaders and instead create a
fresh headers object per test (e.g., instantiate currentHeaders inside setup or
inside beforeEach) so each test gets its own Record<string,string|null>; update
references to currentHeaders in the file to use the locally returned/created
value from setup (or a helper makeCurrentHeaders function) and ensure no other
code relies on the removed module-level symbol.
---
Nitpick comments:
In `@apps/stats-web/src/lib/serverFetch.spec.ts`:
- Around line 7-17: serverFetch imports next/headers and "@/services/di" at
module scope and tests use vi.mock(), which violates the no-mock guideline;
refactor serverFetch to accept an injectable deps bag (e.g., an argument or
setup function) that provides a headers getter and an errorHandler with
getTraceData, replace module-scope calls to headers() and
errorHandler.getTraceData() with calls to those injected functions, and then
update tests to remove vi.mock() and pass vitest-mock-extended mocks (or plain
mock objects) for the headers.get and errorHandler.getTraceData into the
serverFetch setup/constructor so behavior is controlled via DI instead of
vi.mock() calls.
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
2 participants
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.
Why
The
stats-webpod has 70 container restarts due to a two-stage crash:serverFetch()has no timeout, so when the API is slow, Next.js's internal SSR timeout firesconsoleIntegrationpatchesconsole.error. When the SSR timeout callsconsole.error, Node'sutil.inspectcrashes atformatPropertytrying to access.valueon an undefined property descriptor of Next.js's internal error object. This uncaught exception kills the process.Without Sentry's patch, the SSR timeout would be handled gracefully by Next.js's error boundary.
What
Consoleintegration on the server — prevents Sentry from patchingconsole.error, eliminating the fatal secondary crash. Sentry still captures errors viacaptureException.AbortControllertimeout toserverFetch— defense in depth to abort slow API calls before Next.js's own SSR timeout fires.logger.error()before throwing on non-404 errors, matching other pages.serverFetchunit tests — 5 tests covering timeout behavior, signal forwarding, and header merging.Summary by CodeRabbit
Bug Fixes
Tests
Chores