Skip to content

refactor(app-rsc-entry): centralize request-derived page inputs [1/6]#838

Merged
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:feat/pr-768-1-page-inputs-refactor
Apr 14, 2026
Merged

refactor(app-rsc-entry): centralize request-derived page inputs [1/6]#838
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:feat/pr-768-1-page-inputs-refactor

Conversation

@NathanDrake2406
Copy link
Copy Markdown
Contributor

Summary

PR 1 of 6 — restack of #768. Pure refactor, no behavior change. Reshapes buildPageElements to take a single pageRequest object and lifts the x-vinext-mounted-slots header read to handler scope so every call site shares one source of truth.

Previously, both the handler and buildPageElements read and normalized x-vinext-mounted-slots independently. A future refactor changing only one of those paths would silently drift. This PR collapses the two reads into one at the handler scope and passes the normalized value through the pageRequest bag.

Stack (in order)

  1. [1/6] this PR — centralize request-derived page inputs
  2. [2/6] emit per-layout flags in the RSC payload
  3. [3/6] filter skipped layouts from RSC responses and cached reads
  4. [4/6] send X-Vinext-Router-Skip on navigation requests
  5. [5/6] wire build-time layout classification into the generated RSC entry
  6. [6/6] classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION

Sibling: #837 (chore: shim error boundary import).

Test plan

  • vp test run tests/entry-templates.test.ts tests/app-router.test.ts (327 tests)
  • Snapshot regenerated and inspected for PR 1 concerns only

Reshape buildPageElements to accept a single pageRequest object bundling
opts, searchParams, isRscRequest, request, and mountedSlotsHeader. Lift
the mounted-slots header read to the handler scope so every call site
shares one source of truth and a future refactor cannot silently drift
one read path out of the other.
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 14, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@838

commit: 431df18

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the App RSC entry template to centralize request-derived page inputs by threading a single pageRequest object into buildPageElements, and ensuring the x-vinext-mounted-slots header is normalized exactly once at handler scope.

Changes:

  • Updated buildPageElements signature to accept a single pageRequest bag and destructure needed values from it.
  • Moved x-vinext-mounted-slots header normalization to _handleRequest and passed the normalized value through pageRequest to all call sites.
  • Regenerated template snapshot output to reflect the new function signature and header threading.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/vinext/src/entries/app-rsc-entry.ts Centralizes mounted-slots header normalization at handler scope and threads request-derived inputs via a pageRequest object.
tests/snapshots/entry-templates.test.ts.snap Updates snapshot to match the refactored generated entry template output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean refactor — the motivation (collapsing two independent header reads into one shared value at handler scope) is sound and the execution is mechanical. All CI checks pass.

A few observations:

  1. Optional chaining removal is safe. The old buildPageElements code used request?.headers?.get("x-vinext-mounted-slots") defensively, since request was the last positional arg and could theoretically be omitted. The new code reads the header at handler scope with request.headers.get(...) (no optional chaining), which is correct — request is the first parameter of _handleRequest and is always a Request object.

  2. route and params stay positional. Good call keeping them outside the bag — they are route-matched values, not request-derived inputs. The pageRequest bag cleanly scopes the request-derived fields (opts, searchParams, isRscRequest, request, mountedSlotsHeader). This makes the signature easier to extend in the follow-up PRs without growing the positional arg list.

  3. Snapshot diff is consistent. All 7 snapshot variants show the same mechanical transformation, matching the source change.

One minor nit below (non-blocking).

isRscRequest,
request,
mountedSlotsHeader,
} = pageRequest;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking): If the stack plans to keep extending pageRequest (PRs 2–6 mention adding more fields), it might be worth adding a brief JSDoc type comment near the destructuring or at the function signature — even just /** @typedef {{ opts, searchParams, isRscRequest, request, mountedSlotsHeader }} PageRequest */ — so future readers know the shape at a glance without scanning all call sites. Generated code doesn't benefit from TypeScript, so a JSDoc hint would be the only inline documentation.

Fine to defer to a later PR in the stack if the shape isn't stable yet.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 14, 2026

Review submitted with approval. Here's the summary:

Approved PR #838 — a clean, mechanical refactor that:

  • Consolidates the x-vinext-mounted-slots header read from two locations (handler scope + inside buildPageElements) into one read at handler scope, threading the normalized value through a pageRequest bag
  • Reshapes buildPageElements from 7 positional args to (route, params, routePath, pageRequest) — keeping route-matched values positional while grouping request-derived inputs into an object
  • The optional chaining removal (request?.headers?.getrequest.headers.get) is safe since the read is now at _handleRequest scope where request is always defined
  • All 17 CI checks pass (Check, Vitest unit + integration, all 9 E2E projects, create-next-app)

Left one non-blocking nit suggesting a JSDoc type comment for the pageRequest shape, since this is generated code without TypeScript inference.

github run

@james-elicx james-elicx merged commit e8b5911 into cloudflare:main Apr 14, 2026
28 of 29 checks passed
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.

3 participants