Repository navigation
Conversation
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Overall looks good already, will trigger ci
|
The hydrogen e2e failure was a regression from this PR: the trace tags were resolved inside the stream transform, which workerd runs outside the request context. They're now resolved up front. Fixed in 424465d. |
da1cd76 to
aa51022
Compare
aa51022 to
c6da107
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
83f7782 to
c28aa9c
Compare
…dleRequest Adds a Web Streams variant of createSentryHandleRequest, accepted through an overload and exported from the cloudflare entry, and handles a </head> tag split across chunks in both the Node and Web transformers. Closes getsentry#25168
…types Resolve the trace meta tags when injectTraceMetaTags is called instead of inside the stream transform. Runtimes such as workerd run the transform outside the request's async context, where no trace data is available, so the tags were silently dropped. Also let allReady rejections propagate for bots and SPA mode, and move the handler types shared by the Node and Web Streams handlers into their own module.
…ceMetaTags workerd only supports a custom TransformStream transformer from compatibility date 2024-11-11, and Hydrogen's MiniOxygen runs with an older one, so the constructor threw and no trace meta tags were injected. Use a ReadableStream with a pull handler instead, as before, while keeping the shared </head> handling.
The abort timer can run on Node through renderToReadableStream, where a timer that is not unref'd can keep the process alive until streamTimeout while a render is still pending.
c28aa9c to
b7f8d1f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b7f8d1f. Configure here.
| try { | ||
| body = await renderToReadableStream(app, { | ||
| nonce, | ||
| signal: controller.signal, |
There was a problem hiding this comment.
Render ignores request abort signal
Medium Severity
The Web handler aborts renderToReadableStream only through an internal streamTimeout controller and never links that render signal to request.signal. Client disconnects and platform cancellations therefore keep rendering until the timeout, which burns Cloudflare Worker CPU and drops the abort behavior that the official renderToReadableStream template and the previous hand-written handler used.
Reviewed by Cursor Bugbot for commit b7f8d1f. Configure here.
This branch has not been deployed
2 participants
Adds a Web Streams variant of
createSentryHandleRequest, so apps onrenderToReadableStream(non-Node runtimes, or Node withunstable_enableNodeReadableStream) no longer have to hand-composewrapSentryHandleRequestandinjectTraceMetaTags.Following the constraints from the discussion on the issue to keep it non-breaking:
SentryHandleRequestOptionsand the Node path are untouched. The Web options are a separate type, accepted through an overload and only used whenrenderToReadableStreamis passed in.</head>split across chunks was previously missed by both transformers. A shared helper now holds back only a trailing partial</head>(rather than a fixed number of characters) and flushes it at the end of the stream, so it adds as little streaming delay as possible. It is used bygetMetaTagTransformer(Node) andinjectTraceMetaTags(Web).injectTraceMetaTagsmoved to a Node-free module and is still re-exported from/cloudflare, so the public API is unchanged.react-router-8-frameworkcloudflare e2e entry now uses the new API instead of a hand-written copy of it.Fixes #25168