fix(cloudflare): support Sentry request errors on Workers#2425
Conversation
|
/bigbonk review for issues |
commit: |
|
Review: fix(cloudflare): support Sentry request errors on WorkersReviewed the symbol bridge, the export-conditions wiring, and the new fixtures/tests against the actual Sentry consumer code. The core mechanism is correct, but there is one real wiring bug where the Cloudflare conditions land in a branch that can never run for Cloudflare. 1.
|
|
Posted the review on PR #2425. Summary of findings: Key issue (must fix)
The Verified correct
Should verify
The review is posted at the PR thread. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
Review: fix(cloudflare): support Sentry request errors on WorkersReviewed the symbol bridge, the (now reverted) export-conditions wiring, the fixtures, and CI integration. The prior bot review's "must fix" — 1. New e2e Playwright projects are not wired into CI (must fix)
But the CI E2E job in Because Fix: add both projects to the - project: cloudflare-sentry-app
label: cloudflare-sentry-app
shardIndex: 1
shardTotal: 1
- project: cloudflare-sentry-pages
label: cloudflare-sentry-pages
shardIndex: 1
shardTotal: 12. PR description is now stale relative to the codeThe summary still says "prefer Worker/edge export conditions for Cloudflare server builds so Sentry resolves its Worker-compatible entry." Commit 3. Pages fixture imports
|
|
Posted the review on PR #2425. SummaryThe prior bot's "must fix" (conditions in the wrong branch) was already resolved in the follow-up commit I found one new significant issue plus two minor ones: Must fix — new e2e tests never run in CI. Should fix — stale PR description. The body still claims a Worker/edge export-conditions change that commit Nice to have — Pages fixture imports I verified the bridge wiring across all server entries, ran the unit tests (pass after |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Review: fix(cloudflare): support Sentry request errors on WorkersRe-reviewed at
I verified the core mechanism end-to-end and re-ran the unit suites. Verified correct
Minor (non-blocking)
SummaryThe previously-flagged blocking issues are fixed. The symbol bridge is correct, matches the real |
|
Posted my review on PR #2425. SummaryI re-reviewed the PR at
Verified correct (with evidence):
Minor non-blocking notes: stale PR description (mentions removed export-conditions change), pages fixture imports No blocking issues remain. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(cloudflare): support Sentry request errors on Workers
Re-reviewed at a3ab66f (two commits newer than the last bot pass at b9c1be0). Both previously-flagged blocking issues remain resolved, and I independently re-verified the symbol bridge against the installed @sentry/nextjs@10.62.0. No blocking issues. A few minor, non-blocking notes below.
Verified correct (with evidence)
- Symbol bridge matches the real consumer exactly.
@sentry/nextjs/build/cjs/common/utils/responseEnd.jsdoescore.GLOBAL_OBJ[Symbol.for("__cloudflare-context__")]?.ctxand calls.ctx.waitUntil(task)(andisCloudflareWaitUntilAvailable()checkstypeof ...?.waitUntil === "function"). The bridge inshims/request-context.ts:68-77returns{ ctx }wherectxis theExecutionContextLike(haswaitUntil), andGLOBAL_OBJ === globalThis, so read target and shape line up. - Double-install across separate Vite environments is benign. RSC and SSR have separate module graphs, so
installOpenNextCloudflareContextBridge()can run twice against the sameglobalThis. I simulated this: the second install reads the first accessor's getter to capture the fallback (undefinedat init), then redefines aconfigurable: trueaccessor. In/out-of-scope reads and thesettrap all still behave correctly. - Test cleanup works.
Sentry.closeisundefinedon the namespace butSentry.default.closeis a function — confirmed at runtime — so(Sentry as ...).default?.close?.(0)actually tears down the client.afterEachdeletes__VINEXT_onRequestErrorHandler__. - Unit suites pass after
vp install:tests/request-context.test.ts(12) andtests/instrumentation.test.ts(35) green. ThecaptureRequestErrortest exercises the real SDK, assertswaitUntilfires 4×, and validatescontexts.nextjsfor both router kinds. - CI wiring + export-conditions from the prior rounds are still correct: both
cloudflare-sentry-{app,pages}are inci.yml'smatrix.include, andCLOUDFLARE_SERVER_EXPORT_CONDITIONSstays removed (relying on@cloudflare/vite-plugin'sworkerd/workerdefaults). - Fixture wrangler/build commands are internally consistent: the app fixture uses
main: "vinext/server/fetch-handler"with a top-levelwrangler.jsonc, so plainnpx wrangler dev(no--config dist/server/wrangler.json) is correct — this matches thenextra-docs-template/pages-router-cloudflarepattern rather than theapp-router-cloudflaregenerated-config pattern.
Minor (non-blocking)
- Stale PR description. The summary still says "prefer Worker/edge export conditions for Cloudflare server builds." That change was removed in
b226ba7; the body now points at code that no longer exists. Worth updating before the squash/changelog message is derived from it. - Test cleanup is not failure-safe. In
instrumentation.test.ts,Sentry.default.close(0)runs as the last statement of the test body, so an earlier assertion failure leaves the global Sentry client open. Files are isolated in vitest so cross-file leakage isn't a concern, but moving the close intoafterEach/finallywould make the teardown deterministic regardless of assertion outcome. - Fixtures aren't self-contained for typecheck.
cf-sentry-{app,pages}/package.jsondon't declarenext(used viaimport type { NextConfig } from "next"/NextApiRequest), and the app fixture usesReact.ReactNodeinlayout.tsxwithout@types/reactdeclared. All are type-only (erased at build) and resolve via monorepo hoisting, so the e2e build works — flagging only if fixture-level typechecking is ever expected to be isolated. - Single-owner-of-the-symbol assumption. The bridge owns
Symbol.for("__cloudflare-context__")and returns only{ ctx }(noenv/cf). Confirmed nothing else in vinext or@cloudflare/vite-pluginreads this symbol, so it's safe today. A one-line comment oninstallOpenNextCloudflareContextBridgedocumenting the "vinext is the sole owner of this symbol; do not run alongside @opennextjs/cloudflare" contract would help future readers understand theconfigurable: falseguard and fallback capture.
Summary
No blocking issues. The symbol bridge is correct, matches the real @sentry/nextjs consumer exactly, survives multi-environment double-install, and is covered by unit + e2e tests that are now actually wired into CI. Remaining items are documentation/robustness nits.
|
Review posted to PR #2425. SummaryI re-reviewed at Both prior "must fix" items remain resolved:
Verified correct with evidence:
Minor non-blocking notes I added: stale PR description (export-conditions claim no longer in code), test cleanup not failure-safe (close should be in |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cloudflare): support Sentry request errors on Workers
Re-reviewed at the current head (6b91e22). I independently verified the symbol bridge against the installed @sentry/nextjs@10.62.0, ran the unit suites, and re-checked the items flagged in earlier rounds. No blocking issues. Both prior "must fix" items remain resolved.
Verified correct (with evidence)
- Symbol bridge matches the real consumer exactly.
@sentry/nextjs/build/esm/common/utils/responseEnd.jsresolves the ctx asGLOBAL_OBJ[Symbol.for("__cloudflare-context__")]?.ctxandisCloudflareWaitUntilAvailable()checkstypeof ...?.waitUntil === "function". The bridge (shims/request-context.ts:68-77) returns{ ctx }wherectxis theExecutionContextLike(haswaitUntil).captureRequestError.jsends withwaitUntil(flushSafelyWithTimeout()), so the flush is scheduled through the bridge'sctx.waitUntil. Shape and read target line up. - Bridge side effect reaches the worker.
installOpenNextCloudflareContextBridge()runs at import time andshims/request-context.tsis imported across the server entries that run on Cloudflare (app-router-entry.ts:32,pages-api-route.ts:21,instrumentation.ts:41,app-rsc-handler.ts,middleware-runtime.ts, etc.), so the accessor is installed in every runtime. - Route-error reporting path exists. App Router route handlers report through
reportRequestError(... { routeType: "route" })(app-route-handler-dispatch.ts:254,293), which the e2e asserts end-to-end. - CI matrix wiring (prior must-fix) is in place:
cloudflare-sentry-appandcloudflare-sentry-pagesare in.github/workflows/ci.yml'smatrix.include(lines 411-419), so the e2e suites actually run. - Export conditions (prior must-fix) stays removed; the build relies on
@cloudflare/vite-plugin's defaultworkerd/workerconditions, which is how Sentry resolves its Worker entry. No Node-pages pollution. - Tests pass after
vp install:tests/request-context.test.ts(12) andtests/instrumentation.test.ts(35) green. ThecaptureRequestErrortest exercises the real SDK and assertswaitUntilfires 4x with the correctcontexts.nextjsfor both router kinds.vp lintandvp fmt --checkare clean on the changed files. - Fixtures match established patterns: app fixture mirrors
cf-app-basic(main: vinext/server/fetch-handler, noassets.directory); pages fixture mirrors the pages pattern (assets.directory: dist/client). The pages envelope route disablesbodyParserand reads the raw stream, correct for Sentry envelopes.
Minor (non-blocking)
- Stale PR description. The summary still says "prefer Worker/edge export conditions for Cloudflare server builds." That change was removed in
b226ba7; the body points at code that no longer exists. Worth fixing before the squash/changelog message is derived from it. - Test cleanup is not failure-safe. In
instrumentation.test.ts:544,Sentry...default?.close?.(0)is the last statement of the test body, so an earlier assertion failure leaves the global Sentry client open. Files are isolated in vitest, so no cross-file leakage — but moving the close intoafterEach/finallymakes teardown deterministic. - Fixtures aren't self-contained for typecheck.
cf-sentry-{app,pages}/package.jsondon't declarenext(used viaimport type { NextConfig }/NextApiRequest), and the app fixture usesReact.ReactNodeinlayout.tsxwithout@types/reactdeclared. All type-only (erased at build) and resolve via monorepo hoisting, so the e2e build works — flag only if isolated fixture typechecking is ever expected. - Single-owner-of-the-symbol assumption. The bridge owns
Symbol.for("__cloudflare-context__")and, on install, snapshots the prior accessor's value (not a live read) as the fallback. That's fine since vinext and a real@opennextjs/cloudflareruntime would not coexist, but a one-line comment oninstallOpenNextCloudflareContextBridgedocumenting the contract would help future readers understand theconfigurable: falseguard and fallback capture.
Summary
No blocking issues. The symbol bridge is correct, matches the real @sentry/nextjs@10.62.0 consumer exactly, is installed across all worker entries, and is covered by unit + e2e tests now actually wired into CI. Remaining items are documentation/robustness nits.
|
Review posted to PR #2425. SummaryI re-reviewed the PR at the current head and independently verified the core mechanism against the installed Both prior "must fix" items remain resolved:
Verified correct with evidence:
Minor non-blocking notes: stale PR description (export-conditions claim no longer in code), test cleanup not failure-safe ( The PR is solid and ready to merge once the minor items are considered. |
1 participant
Summary
Tests