fix(mount): restore pathname on error with try/finally#1319
fix(mount): restore pathname on error with try/finally#1319productdevbook wants to merge 5 commits intomainfrom
Conversation
The mount middleware modified event.url.pathname for child middleware but only restored it inside the wrappedNext closure. If a child middleware threw an error, the pathname remained in its stripped form when the error handler or subsequent middleware accessed it. Now uses try/finally (consistent with withBase) to ensure pathname is always restored, even when middleware throws. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughH3.mount now wraps mounted FetchHandler middleware to capture and restore the original request pathname, supports synchronous and asynchronous middleware results (awaiting thenables), and rethrows errors while guaranteeing pathname restoration in all outcomes. Tests added to verify restoration for sync and async throws. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/mount.test.ts (1)
159-176: Cover the async throw path in this regression test.This only exercises a synchronous throw. A non-awaited
callMiddleware(...)wrapped intry/finallywould still pass here, so the test does not pin the awaited restoration behavior from the runtime fix. Making the mounted middlewareasyncand throwing after anawaitwill catch regressions if thatawaitis removed later.Suggested test tweak
- subApp.use((_event) => { - throw new HTTPError({ status: 500, statusText: "Test Error" }); - }); + subApp.use(async (_event) => { + await Promise.resolve(); + throw new HTTPError({ status: 500, statusText: "Test Error" }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mount.test.ts` around lines 159 - 176, The test "restores pathname when mounted middleware throws" currently only exercises a synchronous throw; update the mounted middleware on subApp to be async and perform an await before throwing so the test covers the asynchronous throw path (e.g., change subApp.use((_event) => { throw ... }) to subApp.use(async (_event) => { await Promise.resolve(); throw new HTTPError({...}) });), keeping the rest of the test (subApp.get("/test"), mounting via t.app.mount, and t.app.config.onError) identical so the restoration behavior from the awaited middleware throw is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/mount.test.ts`:
- Around line 159-176: The test "restores pathname when mounted middleware
throws" currently only exercises a synchronous throw; update the mounted
middleware on subApp to be async and perform an await before throwing so the
test covers the asynchronous throw path (e.g., change subApp.use((_event) => {
throw ... }) to subApp.use(async (_event) => { await Promise.resolve(); throw
new HTTPError({...}) });), keeping the rest of the test (subApp.get("/test"),
mounting via t.app.mount, and t.app.config.onError) identical so the restoration
behavior from the awaited middleware throw is asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d95830fe-0031-41bb-9b8e-7fe362e75a26
📒 Files selected for processing (2)
src/h3.tstest/mount.test.ts
src/h3.ts
Outdated
| event.url.pathname = event.url.pathname.slice(base.length) || "/"; | ||
| return callMiddleware(event, input["~middleware"], () => { | ||
| try { | ||
| return await callMiddleware(event, input["~middleware"], () => { |
There was a problem hiding this comment.
For performance reasons, we should avoid async/await (in microbenchmarks it adds up)
Use async middleware that throws after an await to properly test the awaited try/finally pathname restoration behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace async/await + try/finally with synchronous try/catch for sync throws and conditional .finally() for async rejections. This avoids the microtask overhead of async functions in the hot middleware path. Pattern follows the codebase convention of checking typeof result?.then === "function" before chaining promises. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/h3.ts`:
- Around line 130-145: The synchronous short-circuit path in callMiddleware
handling can return while event.url.pathname is still modified; ensure you
restore the pathname before any early return by assigning event.url.pathname =
originalPathname just prior to returning a non-thenable result from the
callMiddleware invocation in the block that handles result (i.e., after
computing result from callMiddleware(event, input["~middleware"], () => { ...
next(); }) and before the final return), keeping the existing promise .finally
behavior for thenable results intact; references: callMiddleware,
event.url.pathname, originalPathname, next().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ac105d2-94fc-4ae7-9c67-4d9ca830be25
📒 Files selected for processing (2)
src/h3.tstest/mount.test.ts
The sync success path (line 145) was missing pathname restoration when callMiddleware returned synchronously without calling next(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test that onResponse sees the original pathname when mounted middleware returns a value without calling next(). Uses onResponse hook to verify the pathname within the same request lifecycle. Fails without the sync restore fix at L145, passes with it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/h3.ts`:
- Around line 140-143: The code checks for a thenable by testing (result as
PromiseLike<unknown>)?.then but calls .finally directly, which can throw for
thenables without finally; normalize the thenable by wrapping with
Promise.resolve(result) and call .finally on that normalized Promise so
event.url.pathname is always restored (update the branch handling result to
return Promise.resolve(result).finally(() => { event.url.pathname =
originalPathname; }) and keep existing return behavior).
| if (typeof (result as PromiseLike<unknown>)?.then === "function") { | ||
| return (result as Promise<unknown>).finally(() => { | ||
| event.url.pathname = originalPathname; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where thenable checks are followed by direct .finally assumptions.
rg -n --type=ts -C3 'typeof\s*\(.+\)\?\.\s*then\s*===\s*"function"|\.finally\s*\(' src
# Inspect middleware return behavior and promise-like contracts.
sed -n '60,130p' src/middleware.ts
rg -n --type=ts -C3 'type MaybePromise|interface Middleware|EventHandlerResponse' src/typesRepository: h3js/h3
Length of output: 5810
Normalize thenables before calling .finally().
At Line 140 you check for .then, but Line 141 calls .finally which is not part of the PromiseLike interface. A custom thenable without .finally will throw TypeError and skip the pathname restoration. Use Promise.resolve() to normalize the thenable to a proper Promise first.
💡 Proposed fix
- if (typeof (result as PromiseLike<unknown>)?.then === "function") {
- return (result as Promise<unknown>).finally(() => {
+ if (typeof (result as PromiseLike<unknown>)?.then === "function") {
+ return Promise.resolve(result as PromiseLike<unknown>).finally(() => {
event.url.pathname = originalPathname;
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof (result as PromiseLike<unknown>)?.then === "function") { | |
| return (result as Promise<unknown>).finally(() => { | |
| event.url.pathname = originalPathname; | |
| }); | |
| if (typeof (result as PromiseLike<unknown>)?.then === "function") { | |
| return Promise.resolve(result as PromiseLike<unknown>).finally(() => { | |
| event.url.pathname = originalPathname; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/h3.ts` around lines 140 - 143, The code checks for a thenable by testing
(result as PromiseLike<unknown>)?.then but calls .finally directly, which can
throw for thenables without finally; normalize the thenable by wrapping with
Promise.resolve(result) and call .finally on that normalized Promise so
event.url.pathname is always restored (update the branch handling result to
return Promise.resolve(result).finally(() => { event.url.pathname =
originalPathname; }) and keep existing return behavior).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/mount.test.ts (2)
192-194: Consider asserting HTTP status in both error-path tests.Adding
expect(res.status).toBe(500)makes these regressions more explicit and easier to diagnose when they fail.Also applies to: 212-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mount.test.ts` around lines 192 - 194, Add an explicit HTTP status assertion for the error-path tests by asserting res.status is 500 before (or immediately after) reading the JSON body; update the test cases that call t.fetch("/api/test") (and the similar call referenced at lines 212-214) to include expect(res.status).toBe(500) alongside the existing body.path assertion so failures surface the HTTP error code as well as the response body.
195-195: Avoid silently clearing tracked errors without an expectation.
t.errors = []can hide unexpected error behavior; consider asserting expected error tracking state before clearing.Based on learnings, "Applies to test/**/*.test.ts : Track unhandled errors with
ctx.errorsand auto-assert inafterEach".Also applies to: 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mount.test.ts` at line 195, The test currently resets t.errors with "t.errors = []", which silently discards tracked unhandled errors; instead, in the test(s) referencing t.errors (and following the project's t/ctx error-tracking convention), assert the expected state of t.errors (e.g., that it's empty or contains specific errors) before clearing, or remove the manual reset so the global afterEach auto-asserts; update occurrences around the referenced tests (the assignment at t.errors and the similar one at line ~215) to either add explicit assertions about t.errors or eliminate the manual clearing to rely on the existing afterEach auto-assert behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/mount.test.ts`:
- Around line 192-194: Add an explicit HTTP status assertion for the error-path
tests by asserting res.status is 500 before (or immediately after) reading the
JSON body; update the test cases that call t.fetch("/api/test") (and the similar
call referenced at lines 212-214) to include expect(res.status).toBe(500)
alongside the existing body.path assertion so failures surface the HTTP error
code as well as the response body.
- Line 195: The test currently resets t.errors with "t.errors = []", which
silently discards tracked unhandled errors; instead, in the test(s) referencing
t.errors (and following the project's t/ctx error-tracking convention), assert
the expected state of t.errors (e.g., that it's empty or contains specific
errors) before clearing, or remove the manual reset so the global afterEach
auto-asserts; update occurrences around the referenced tests (the assignment at
t.errors and the similar one at line ~215) to either add explicit assertions
about t.errors or eliminate the manual clearing to rely on the existing
afterEach auto-assert behavior.
2 participants
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests