Skip to content

fix(adapters): sync raw node req.url with event.url in fromNodeHandler#1433

Merged
pi0 merged 1 commit into
mainfrom
fix/from-node-handler-url-sync
Jul 3, 2026
Merged

fix(adapters): sync raw node req.url with event.url in fromNodeHandler#1433
pi0 merged 1 commit into
mainfrom
fix/from-node-handler-url-sync

Conversation

@pi0x

@pi0x pi0x commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Context

Follow-up to #1432 (percent-decode normalization no longer mutates the shared _url, fixed in a1cf066).

Legacy Node handlers invoked via fromNodeHandler route on the raw req.url. Until now, h3-level URL rewrites (withBase, mount) only reached that raw request through srvx's implicit NodeRequestURL pathname write-back (srvx#118) — mutation-at-a-distance through the shared parsed URL object.

That channel is now half-dead: since a1cf066, requests whose pathname needs percent-decode normalization get a detached event.url clone, so withBase mutations never reach the shared NodeRequestURL, and legacy handlers saw the original un-stripped URL for such requests (e.g. withBase("/api", fromNodeHandler(m)) handed m a req.url of /api/h%65llo instead of /hello).

Change

Make the propagation explicit at the adapter boundary instead of relying on the shared-object side effect:

  • fromNodeHandler computes h3's view (event.url.pathname + event.url.search); if it differs from the raw req.url, it swaps it in before calling the legacy handler and restores the original once the handler settles.
  • When the views already match (no rewrite, no decode — the common case), the raw request is never touched.

Behavior for legacy middleware is unchanged vs. the pre-#1432 node behavior (they already received the decoded pathname, since the constructor write-back fired before any handler ran) — except it now also works for percent-encoded paths, which the implicit channel never covered after the clone fix.

This also unblocks srvx dropping the implicit write-back from srvx#118: h3 no longer depends on it, so the raw IncomingMessage.url can stay pristine wire data for every other consumer.

Tests

New node-target regression test: withBase("/api", fromNodeHandler(...)) echoing req.url:

  • /api/hello?q=1 → legacy handler sees /hello?q=1
  • /api/h%65llo → legacy handler sees /hello (failed before this change)
  • raw req.url is restored to /api/h%65llo after the handler settles (asserted via onResponse)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility for legacy Node.js handlers so they now see the same normalized URL as the app route.
    • Preserved the original request URL after handling, reducing the risk of affecting later routing or response behavior.
    • Added coverage for base-path stripping, URL rewrites, and percent-decoding behavior to help prevent regressions.

Legacy Node handlers route on the raw req.url, which previously only
reflected h3-level rewrites (withBase/mount) through srvx's implicit
NodeRequestURL write-back (srvx#118). That channel no longer fires when
the pathname needed percent-decode normalization (event.url is a
detached clone since #1432), so legacy handlers saw the original
un-stripped URL for such requests. Propagate explicitly at the adapter
boundary instead: set req.url from the h3 view around the call and
restore it once the handler settles. This also unblocks removing the
implicit write-back on the srvx side.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pi0x pi0x requested a review from pi0 as a code owner July 3, 2026 22:31
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e5302f5-bf4b-4a69-8357-e14c35d106e0

📥 Commits

Reviewing files that changed from the base of the PR and between a1cf066 and 2ed63a5.

📒 Files selected for processing (2)
  • src/adapters.ts
  • test/app.test.ts

📝 Walkthrough

Walkthrough

The fromNodeHandler runtime path in src/adapters.ts now normalizes node.req.url to match the h3 event's URL before invoking the legacy Node handler, temporarily overwriting it and restoring the original value afterward. A new test verifies this base-stripping and normalization behavior, including URL restoration.

Changes

fromNodeHandler URL Sync

Layer / File(s) Summary
Normalize and restore req.url
src/adapters.ts
Computes URL from event.url.pathname + event.url.search, temporarily overwrites node.req.url if it differs, invokes callNodeHandler(), and restores original value via .finally().
Test coverage for URL sync
test/app.test.ts
Adds withBase import and a Node-only test that registers /api/** with withBase(fromNodeHandler(...)), verifying base-stripped/decoded URL visibility and restoration of raw req.url.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Suggested reviewers: pi0

Poem

A rabbit hops through URLs anew,
Stripping bases, restoring true,
node.req.url borrowed for a spell,
Then given back—all is well!
🐇✨ /api/hop → /hop, and back again!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: syncing Node req.url with event.url in fromNodeHandler.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/from-node-handler-url-sync

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pi0 pi0 merged commit 8786f23 into main Jul 3, 2026
8 of 9 checks passed
@pi0 pi0 deleted the fix/from-node-handler-url-sync branch July 3, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants