Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for running Next.js applications mounted under a subpath (urlPath) in Harper, updating the request handler in src/plugin.ts to use request.withNodeAdapter when a URL rewrite is detected. It also adds a new fixture and integration tests for this scenario. The review feedback correctly identifies a potential crash (TypeError) when response.body is null or undefined (such as in 204 or 304 responses) and suggests using optional chaining to safely attach the error listener.
| response.body.on('error', (error) => | ||
| scope.logger.debug?.(`Next.js response stream error for ${request.pathname}: `, error) | ||
| ); |
There was a problem hiding this comment.
If the Next.js response has no body (for example, with 204 No Content, 304 Not Modified, or HEAD requests), response.body will be null or undefined. Calling response.body.on('error', ...) directly will throw a TypeError: Cannot read properties of null (reading 'on') immediately when the promise resolves, causing these requests to fail with a 500 error.
Using optional chaining (response.body?.on) prevents this crash for empty responses.
| response.body.on('error', (error) => | |
| scope.logger.debug?.(`Next.js response stream error for ${request.pathname}: `, error) | |
| ); | |
| response.body?.on('error', (error) => | |
| scope.logger.debug?.(`Next.js response stream error for ${request.pathname}: `, error) | |
| ); |
|
The upstream blocker in entry 1 now has an issue: Request.withNodeAdapter() cannot serve real Node middleware: Next.js 500s, then >16 KB responses hang silently (harper#2527, P1, Bug, v5.3, under [Epic] @harperdb/nextjs component integration). It carries the full six-symptom chain, the root cause, and the partial patch that closes 1–4 but not 5–6. A dev-agent task is queued against it. Confirmed unfixed on The design question in entry 1 is answered there as the rewrite, not dropping the adapter for entry 3 — the — Claude (Opus 5) |
50ab54b to
fdec4f3
Compare
|
Moving this back to draft. It isn't safe to land yet, for two reasons:
I think this is ready to come back once HarperFast/harper#2527 closes and the dev dependency moves to a Harper that carries the fix. At that point the On the bot thread: 🤖 Generated by Claude Opus 5.5; posted via @Ethan-Arrowood. |
Harper's router strips an application's mount from the Harper `Request` before the plugin's handler runs, but the plugin was reaching past that proxy to `request._nodeRequest`, which still carries the un-stripped URL. An app mounted at a `urlPath` therefore handed Next.js a path it cannot route. Route the request through `Request.withNodeAdapter()`, which presents the Harper Request's own method/url/headers over the underlying Node request, and attach the `error` listener the adapter's response body contract requires. Adds a `next-16-mounted` fixture (mounted at `/mounted`) and integration coverage that the mount root, a nested page, an API route and its query string all reach the right Next.js route, and that paths outside the mount are not served by Next.js. Refs #61 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Note that routing through withNodeAdapter blocks on harper presenting a faithful Node request/response, so a future agent does not chase the red page-based tests, and that the Playwright browser binaries have to be installed for those tests to run at all. Refs #61 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Routing every request through withNodeAdapter moved apps with no urlPath — every existing user — onto a path that today's harper cannot serve, and paid a proxy, a promise, a PassThrough and a header copy per request for a rewrite they never needed. Take the adapter only when middleware actually changed the URL; otherwise hand Next.js the Node request directly, exactly as before. Also guard on `_nodeResponse == null` rather than `=== undefined`, since harper's Bun and uWS requests carry null and implement no Node adapter, and make the echo fixture return the search string so the query-preservation test can fail for the reason it names. Refs #61 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Without a guard, a mounted application on a Harper predating Request.withNodeAdapter throws per request from inside the HTTP chain. Fall back to the un-stripped hand-off — the behaviour that Harper already had — and warn once so the operator learns why the mount does not route. Skip the mounted assertions with test.describe.fixme rather than landing a permanently red file: CI is disabled here, so a green local run is the repo's only regression signal, and a file that is always red hides the next real failure inside it. Refs #61 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Refs #61 Co-Authored-By: Claude Opus <noreply@anthropic.com>
AGENTS.md already says CI runs integration-tests.yml on every PR (corrected on main while this branch was rebased); this comment still claimed otherwise. The "outside the fixme block" line narrated file structure a reader can see. Dispatch-Task: pr-maint-fe1f4d8cb37821cbaefd78a61e6079ef Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…base Dispatch-Task: pr-maint-fe1f4d8cb37821cbaefd78a61e6079ef Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
33fe550 to
27eff90
Compare
Harper's router strips an application's
urlPathmount before a handler runs, but the plugin handed Next.js the un-stripped URL fromrequest._nodeRequest, so an app mounted at/foosaw/foo/file.htmlfor a path Harper had resolved to/file.html.When middleware rewrote the URL (which mounting does), the request now goes through
Request.withNodeAdapter(). Unmounted apps keep today's direct hand-off, and a Harper with no adapter falls back to it with a one-time warning.Mounted apps still do not work on current Harper: the adapter 500s and stalls past 16 KB, so the mounted assertions are
test.describe.fixme. Nothing that works today regresses (28 passed, 5 skipped).🔧 Changes
src/plugin.tsgate: adapter only whenrequest.urldiffers from the Node request's URL; look hardest at theadaptHttp2ForNextcall that stays on the direct path (R484).config.yamlsetsurlPath: /mounted;next.config.ts,package.jsonand.npmrcset it up; layout, home and about pages and the echo route are the render targets.integrationTests/next-16-mounted.pw.ts: five mounted assertions underfixme, one active out-of-mount check.AGENTS.md: records the adapter dependency and the browser-binary requirement.Decisions and open questions (7)
For the human reviewer
{}, plusappendHeader,_implicitHeader,finished) getcurl-level requests working — with them applied locally the fixture passes 6/6 — and still leave browser page loads hanging: chunks of 9.7 KB, 14.4 KB and 16.8 KB complete while 183 KB and 229 KB never finish, becauseres.write()returnsfalsepast the responsePassThrough's 16 KB high-water mark, Node'sReadable.pipewaits for'drain', andcompression'sres.onoverride buffers that registration until a gzip stream that is never created. Making_implicitHeader()flush headers the way Node'sServerResponsedoes clears the stall and produces the next failure: raw gzip bytes with noContent-Encodingand truncated transfers. Root cause: the adapter's response isObject.assign(new EventEmitter(), {…})rather than a realWritable, and Next'scompression/on-headers/sendstack depends onServerResponsesemantics. Decision needed: take the harper rewrite (response built on thePassThroughitself with header capture layered over it, plus a test that drives real middleware), or drop the adapter for entry 3. Reproducers, a README and the partial patch are at~/dev/scripts/harper-node-adapter-repro.request.url !== request._nodeRequest.urlrather than applied to every request. Routing everything through it moved unmounted apps — every existing user — onto that broken path, and charged aProxy, aPromise, aPassThrough, aResponseHeaders, anEventEmitterand a full header copy per request for a rewrite they never needed. The predicate is exact for URL rewrites: the two are equal precisely when nothing touched the URL. It is deliberately narrower than the adapter's purpose — a component that mutates only the Harper-levelmethodorheadersstill takes the direct path, so Next.js reads the raw Node values. Making it "adapt whenever anything might have been mutated" means adapting always, which is the version this replaced.urlParse(request.url, true). All three supported Next majors funnel throughNextCustomServer.getRequestHandler, which doesreq.url = formatUrl(parsedUrl), so passing the mount-stripped parsed URL makes Next route mount-relatively with no adapter at all — measured against an unpatched harper: pages, nested pages, API route with query string and static assets all correct. It gives up the adapter's forwarding of middleware-mutated method and headers, which is why the issue prescribes the adapter. Switching costs one commit.withNodeAdapterat all; where the method exists but is unfaithful there is nothing to detect. Mounted apps are non-functional either way, so this is a change in failure mode rather than a loss of function — but it is louder and, for the >16 KB stall, silent-and-hanging rather than a clean status code. Accepting it is what lets the fix land before the harper work; the alternative is holding this PR entirely. Mounted requests over Harper's HTTP/2 port take the same adapter path, which hands Next.js its own synthesized objects, soadaptHttp2ForNext(applied on the direct path only) cannot help there; the adapter rewrite covers it.request.url === '/_next/webpack-hmr'compares the stripped URL so the branch is taken under a mount, butupgradeHandler(request._nodeRequest, …)then hands Next/mounted/_next/webpack-hmr. Not fixed here:withNodeAdapteris an HTTP adapter and does not apply to an upgrade, so the fix is a different mechanism needing its own design and test, and mounted dev mode cannot work anyway while Next generates the HMR client's URL at the root (entry 6). Dev-only; worth its own issue.basePathis out of scope and mounted apps are still incomplete even once harper is fixed. Inbound stripping makes Next route correctly under a mount, but Next still generates/_next/*asset URLs at the root, which 404 outside it. Only a build-timebasePathfixes that. Whether the plugin should require, inject, or validatebasePathagainsturlPath— or fail loudly when they disagree — is an open product decision left for a human, per the issue. The browser test asserts on server-rendered markup for exactly this reason.debug, which is off in production, so a truncated response and a completed one look alike — but Harper's ownpipeBodyToResponsealready warns on that error, and a secondwarnhere would be the redundant error logging the guidelines rule out. And the listener is attached one microtask after the response resolves, so a handler that flushes headers and destroys the response in the same synchronous turn would still throw; Next.js's handler is async and does not, and harper's API offers no earlier hook — that one belongs upstream onwithNodeAdapterrather than here.Verification
Fails-on-base: the new
next-16-mountedfixture (urlPath: /mounted) fails onorigin/main—/mounted,/mounted/aboutand/mounted/api/echoall 404 because Next receives the un-stripped path — and passes6 passed (9.2s)with this change against a harper carrying the partial adapter fix, includingGET /mounted/api/echo?q=1returning{"pathname":"/api/echo","search":"?q=1"}from inside the Next route handler and a chromium page load rendering the mounted home page. Those assertions arefixmeon the branch as it stands; un-skipping them locally is how the run above was produced.No regression:
npm run test:integrationon this branch against stock harper 5.1.23 is28 passed, 5 skipped. An earlier revision that routed every request through the adapter scored15 passed, 17 failedon the same suite, every failure apage.gototimeout on an unmounted fixture —integrationTests/next-16.pw.tsalone went from5 passed (8.8s)onorigin/mainto 2 timeouts. That revision is what entry 2's gate replaced.The adapter gaps were isolated in a standalone harness — plain Node
httpserver, harper'sRequestclass, Next's request handler, no Harper runtime — plus a chromium driver that reports which requests never finish; both are kept with a README at~/dev/scripts/harper-node-adapter-repro.compress: falsein the fixture'snext.config.tsmaking the identical page load succeed is the experiment that isolates the 16 KB stall to Next's compression middleware rather than to Next or the harness.Related PRs: #73 independent
Complexity: medium
Deferred findings
717feccffacd6e52minor correctness —src/plugin.ts— No-adapter fallback warns then hands Next.js the unstripped URL; suggestion to parse request.url — NOT FILED53983f2d938d6fc1nit test-coverage —integrationTests/next-16-mounted.pw.ts— No test exercises the adapter branch, warning branch, or URL-mismatch selection — NOT FILEDReview-Coverage: authored=claude; ran=muse,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=3; full=1 @ 27eff90
Review-Attention: study ~9m (decisions: adapter-vs-url-rewrite, strip-vs-basepath, enabled-by-default-while-fixme, no-adapter-fallback; raised: open major) @ 27eff90