ref(remix): Avoid unnecessary error wrapping HandleDocumentRequestFunction
#17680
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
packages/remix/src/server/errors.ts
Outdated
| captureRemixServerException(err, 'HandleDocumentRequestFunction', request); | ||
| }, |
There was a problem hiding this comment.
Potential bug: The error callback for HandleDocumentRequestFunction no longer re-throws errors, swallowing exceptions and breaking the error handling flow for document requests.
-
Description: The
onErrorcallback forerrorHandleDocumentRequestFunctionno longer re-throws the caught error. This violates the contract of thehandleCallbackErrorsutility, which is designed to always propagate exceptions after executing the callback. By removingthrow err;, any error occurring during the server-side document rendering process is captured but then swallowed. This prevents Remix's higher-level error handling from triggering, which can lead to clients receiving incomplete or invalid HTML responses instead of a proper error page, while the server continues in an inconsistent state. -
Suggested fix: Restore the original behavior by re-adding
throw err;at the end of theonErrorcallback. This ensures that after the exception is captured, it is correctly propagated, allowing Remix's standard error handling mechanisms to generate a proper error response.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
this already re-throws out of the box.
|
@onurtemizkan based on the tests, we could possibly also just remove this wrapping and let this bubble up anyhow? https://github.com/getsentry/sentry-javascript/actions/runs/17790747804/job/50567093382?pr=17680 not sure if we care about the specific function name here...? |
I agree 👍 The function name is not important or actionable by the end user |
mydea
changed the title
HandleDocumentRequestFunctionHandleDocumentRequestFunction
nice, then I adjust this PR to just drop this handler! |
mydea
changed the title
HandleDocumentRequestFunctionHandleDocumentRequestFunction
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
I've seen a few places where we are wrapping things that could be sync
or async, and we want to do something with the return value. By adding
an onSuccess handler to `handelCallbackErrors` we can handle this more
generically in the future:
```js
const wrapped = handleCallbackErrors(
() => fn(),
// error handler
(_err) => {},
// finally handler
() => {},
// success handler, gets value or resolved value if function returns a promise
(_value) => {}
);
```
While scanning a bit for places where we could already use this, I also
found two bugs around this, opened separate PRs for them:
* #17680
* #17681
Noticed while working on #17679, this was actually incorrect here.
handleCallbackErrorsdoes not actually do anything, you need to still call the function you want to call in the error case!Actually we can just drop this as this error is handled by some other handler anyhow, I think, according to the tests!