Repository navigation
Conversation
…responses Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
bugbot run |
Contributor
size-limit report 📦Compared: 38759ce (PR test merge using develop at f2ee787) Against: f2ee787 (develop) · base run #65001 ✅ Baseline matches: this diff contains only this PR's changes.
|
Contributor
Optional E2E testsSome optional checks were not successful. They do not block the merge. 2 not successful, 53 successful. See the run.
Successful checks
|
… is sent Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ccacd. Configure here.
| import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration'; | ||
| import { registerNextSentryPropagator } from './nextSentryPropagator'; | ||
| import { addNextjsServerSpanHooks, NEXTJS_SERVER_IGNORE_SPANS } from './serverSpanHooks'; | ||
| import { addServerTimingTraceHeader } from './serverTimingHeader'; |
There was a problem hiding this comment.
Server-Timing trace header can ship on CDN-cacheable responses
isSharedCacheable() in serverTimingHeader.ts only inspects Cache-Control, so responses with CDN cache headers (CDN-Cache-Control, Vercel-CDN-Cache-Control, etc.) or max-age=0, stale-while-revalidate still get a per-request sentry-trace/baggage Server-Timing value a shared cache can replay to other users. Align it with the Remix middleware checks.
Evidence
addServerTimingTraceHeader()is registered from this import during Next.js serverinitand setsServer-Timingin thewriteHeadhook viasetServerTimingTraceHeader().- Guard is only
isSharedCacheable()readingcache-control;private/no-storeforcefalseeven when a CDN-specific lifetime header allows shared storage. max-age=0withstale-while-revalidatealso fails the positivemax-age=(?!0\b)check, so the trace header is still attached.- Remix
packages/remix/src/v3/server/middleware.tstreats those same CDN headers and stale directives as shared-cacheable and has tests that omit the trace in those cases.
Identified by Warden · security-review · 3S7-667
This branch has not been deployed
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.
Why: A
sentry-tracemeta tag only reaches the browser with the HTML that carries it. UndercacheComponentsthat is the resumed part, which usually arrives after the SDK started, so the pageload has to wait for it (#25142). AServer-Timingheader arrives before any HTML, the browser SDK already reads it, and Remix and Nitro already send it. On a self-hosted server Next.js flushes the headers with the first chunk, inside the request span, so the header is set whenwriteHeadruns rather than when the request comes in.The header is left off responses a shared cache may store (
s-maxage,public,max-agewithoutprivate/no-store), which is what Next.js sends for static and ISR app router documents. A CDN in front of the server would otherwise replay one visitor's trace and sampling decision to everyone. Next's own caches never replay it, they only store headers the render set. The pages router sends noCache-Controlfor a static page, so there the header goes out in production too. It is also left off responses that are not documents, since only the navigation entry exposes it.With the header, the pageload no longer waits for the tag, so
page.server_rendering.typeis not known at span start for a runtime shell with a dynamic part. It is settled when the tag arrives or the document finishes parsing, sotracesSamplerdoes not see it in that case. The link to the prerender is still set at start.The formatting moved from Remix to
getTraceServerTimingHeaderin core; Nitro keeps its own twoappendcalls for now.How to review:
packages/nextjs/src/server/serverTimingHeader.tsis the feature.traceMetaTags.tson the client only changes what happens after the pageload started (observeLateTraceMetaTag). The e2e changes show which pages get the header:/pageload-tracing,/runtime-shelland/slow-holedo,/static-shelland the[id]shells do not.Unsure about: The
Cache-Controlgate is a heuristic, and a response without anyCache-Control(pages router) gets the header. Vercel is out of scope (JS-3866).Closes #24945
Refs JS-3864
🤖 Generated with Claude Code