feat(app-router): mint semantic route graph ids#1060
feat(app-router): mint semantic route graph ids#1060james-elicx merged 1 commit intocloudflare:mainfrom
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/726-graph-semantic-ids
branch
from
95babdf to
9656dd7
Compare
NathanDrake2406
force-pushed
the
nathan/726-graph-semantic-ids
branch
from
9656dd7 to
f4ee21b
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(app-router): mint semantic route graph ids
Solid, well-scoped first step for #726-GRAPH-01. Tests pass, types check, and the new IDs are carried as inert metadata without touching any runtime paths. The approach of minting IDs from existing graph facts (pattern, tree positions, slot ownership) is clean and produces stable, root-independent identifiers.
A few observations and one thing worth resolving before merge:
Key concern
ids.slots map keys are slot.key (relative filesystem path), not semantic IDs. The values in ids.slots are proper graph-owned semantic IDs (slot:modal:/(marketing)/blog/[slug]), but the keys are slot.key strings like modal@(marketing)/blog/[slug]/@modal — which embed the @-directory name and relative filesystem structure. This means the AppRouteSemanticIds.slots record isn't purely semantic; it's a mapping from an infrastructure key to a semantic ID. That's fine as a bridge, but worth a doc comment on the slots field in AppRouteSemanticIds explaining this is keyed by slot.key (an infrastructure identifier) intentionally, and that later #726 work may re-key it.
Minor items
Inline comments below.
| routeHandler: string | null; | ||
| layouts: readonly string[]; | ||
| templates: readonly string[]; | ||
| slots: Readonly<Record<string, string>>; |
There was a problem hiding this comment.
The slots field is keyed by slot.key (the infrastructure-level name@relative-path identifier), not by the graph-owned semantic slot ID. This is a reasonable bridge for #726-GRAPH-01, but it's the one place in AppRouteSemanticIds where a non-semantic key leaks in. Consider adding a doc comment so a later PR knows whether to re-key this by semantic ID or keep the slot.key → semantic-ID mapping.
| slots: Readonly<Record<string, string>>; | |
| slots: Readonly<Record<string, string>>; | |
| // ↑ Keyed by slot.key (infrastructure id: "name@relative/path"), value is graph-owned semantic slot id. | |
| // Later #726 PRs may re-key by semantic id once the planner consumes these. |
There was a problem hiding this comment.
Documented this bridge map on AppRouteSemanticIds.slots so later #726 work can see it is currently slot.key -> semantic slot id.
| }): AppRouteSemanticIds { | ||
| const slots: Record<string, string> = {}; | ||
| for (const slot of input.slots) { | ||
| if (slot.id) { |
There was a problem hiding this comment.
Since AppRouteGraphParallelSlot makes id required (non-optional string), this truthiness check is always true for graph slots. The if (slot.id) guard only matters for the base ParallelSlot type where id is optional. Since the input type here is readonly AppRouteGraphParallelSlot[], the guard is dead code.
Not a bug — it's defensive — but it could be confusing. Consider either removing the check (since the type guarantees presence) or adding a comment explaining why it's there.
There was a problem hiding this comment.
Removed the guard. createAppRouteSemanticIds() now accepts graph slots, so AppRouteGraphParallelSlot enforces the required id invariant.
| function createAppRouteGraphRouteId(pattern: string): string { | ||
| return `route:${pattern}`; | ||
| } | ||
|
|
||
| function createAppRouteGraphPageId(pattern: string): string { | ||
| return `page:${pattern}`; | ||
| } | ||
|
|
||
| function createAppRouteGraphRouteHandlerId(pattern: string): string { | ||
| return `route-handler:${pattern}`; | ||
| } | ||
|
|
||
| function createAppRouteGraphLayoutId(treePath: string): string { | ||
| return `layout:${treePath}`; | ||
| } | ||
|
|
||
| function createAppRouteGraphTemplateId(treePath: string): string { | ||
| return `template:${treePath}`; | ||
| } | ||
|
|
||
| function createAppRouteGraphSlotId(slotName: string, ownerTreePath: string): string { | ||
| return `slot:${slotName}:${ownerTreePath}`; | ||
| } |
There was a problem hiding this comment.
These six constructor functions are clean and local, which is fine for this PR. One thought for a follow-up: the ID format uses : as a separator (slot:modal:/(marketing)/blog/[slug]), which means a slot name containing : would be ambiguous. Real-world Next.js slot names can't contain : (they're directory names starting with @), so this is safe in practice, but it might be worth documenting the format contract (or adding a debug-mode assertion) before later PRs start parsing these IDs.
| ); | ||
|
|
||
| expect(routeEntry).toBeDefined(); | ||
| expect(routeEntry).not.toContain(appDir); |
There was a problem hiding this comment.
Good assertion — verifying semantic IDs don't leak the filesystem root. Note that routeEntry also contains the slot.key values (e.g. "modal@(marketing)/blog/[slug]/@modal") which are relative to appDir, so they won't match appDir either. The assertion holds, but it's testing two things at once (semantic IDs don't contain appDir + slot keys don't contain appDir). That's fine for now, just noting it.
| TModule extends AppPageModule = AppPageModule, | ||
| TErrorModule extends AppPageErrorModule = AppPageErrorModule, | ||
| > = { | ||
| ids?: AppRouteSemanticIds | null; |
There was a problem hiding this comment.
Good that this is ids?: AppRouteSemanticIds | null rather than required — keeps backward compatibility with routes that don't have IDs yet (e.g. hand-crafted test fixtures or older codepaths). The null union is appropriate since the manifest serializes route.ids ?? null.
| function createAppRouteGraphTreePath( | ||
| routeSegments: readonly string[], | ||
| treePosition: number, | ||
| ): string { | ||
| const treePathSegments = routeSegments.slice(0, treePosition); | ||
| if (treePathSegments.length === 0) { | ||
| return "/"; | ||
| } | ||
| return `/${treePathSegments.join("/")}`; | ||
| } |
There was a problem hiding this comment.
This function is straightforward but has a subtle interaction with route groups: routeSegments.slice(0, treePosition) includes route group segments like (marketing) in the tree path. That's intentional per the PR description ("route groups and dynamic segment markers that affect topology must remain visible in graph IDs"), and the tests verify it. Just confirming this is the intended behavior since it differs from the URL pattern (where route groups are invisible).
|
Review submitted successfully. Here's a summary of the review: Overall assessment: Clean, well-scoped PR that implements Key concern raised:
Minor observations:
|
App Router route graph entries previously left route, page, layout, template, and slot identity to later render and wire layers. That kept topology meaning coupled to reconstructed payload keys instead of a graph-owned fact. Mint stable semantic IDs during graph construction, preserve them in generated RSC route metadata, and cover route-group, dynamic-segment, slot-owner, and filesystem-root stability cases.
NathanDrake2406
force-pushed
the
nathan/726-graph-semantic-ids
branch
from
f4ee21b to
ebd6f72
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(app-router): mint semantic route graph ids
All tests pass, types check, and the new semantic IDs are carried as inert metadata without touching any runtime codepaths. The approach is clean — IDs are minted from existing graph facts at scan time, the type hierarchy (AppRouteGraphRoute extends AppRoute) preserves backward compatibility for all existing consumers, and the two test additions (stability across filesystem roots + no root leak in serialized manifest) are the right assertions for this layer.
The previous review's feedback has been addressed (dead if (slot.id) guard removed, doc comment added to slots bridge map). No new concerns.
Verified
vp test run tests/app-route-graph.test.ts— 13 tests passvp test run tests/entry-templates.test.ts— 10 tests passvp checkon all changed files — clean (format + lint + types)idsandslot.idare only referenced during manifest serialization, never in request-handling runtime codeAppRouteGraphRoute[]is assignable toAppRoute[]— no downstream breakage- Inherited slots preserve
idthrough the spread indiscoverInheritedParallelSlots applySlotSubPagesmutations don't invalidateids.slots(slot keys are stable, onlypagePath/routeSegmentschange)
One nit (non-blocking)
The routesByPattern map in discoverSlotSubRoutes (line 339) holds AppRouteGraphRoute values but is typed as Map<string, AppRoute>, and applySlotSubPages takes AppRoute. The downcast is harmless — the mutation only touches parallelSlots via spread (preserving id), and ids.slots remains valid since slot keys don't change. But narrowing the map and function parameter types to AppRouteGraphRoute would let the compiler catch any future mutations that might drop graph-specific fields.
|
Review submitted successfully. Here's a summary: Approved — this is a well-scoped, clean implementation of Key things verified:
One non-blocking nit: the |
What this changes
Implements
#726-GRAPH-01from #726 by having the existing App Router route graph mint stable semantic IDs for route, page, route handler, layout, template, and parallel slot facts.The generated RSC manifest now carries those graph-owned IDs through route and slot metadata, but this PR intentionally does not promote planner decisions, root-boundary IDs, cache compatibility, or wire-key enforcement. Those are later
#726-GRAPH-*,#726-CORE-*, and#726-WIRE-*tasks.Why
The current flat payload bridge works, but route meaning is still reconstructed from route patterns, tree paths, and slot keys in later layers. For the #726 migration, topology identity needs to be a build-time graph fact before the planner and compatibility layers can stop treating wire keys as semantic authority.
Correctness oracle: Vinext internal invariant. Equivalent app filesystem topology should produce the same semantic IDs regardless of absolute filesystem root, while route groups and dynamic segment markers that affect topology must remain visible in graph IDs.
Approach
AppRouteSemanticIdsto the existing route graph output.idsplus slotidin generated App Router RSC route metadata.#726-GRAPH-01only needs minted graph facts. Exporting canonical constructors can happen when a later PR has an actual import boundary for them.Validation
vp test run tests/app-route-graph.test.ts tests/entry-templates.test.tsvp check packages/vinext/src/routing/app-route-graph.ts packages/vinext/src/entries/app-rsc-manifest.ts packages/vinext/src/server/app-page-route-wiring.tsx tests/app-route-graph.test.ts tests/entry-templates.test.tsvp run knip --no-progressvp stagedvp test run tests/entry-templates.test.ts -uNote: the pre-commit hook currently assumes
tests/__snapshots__/entry-templates.test.ts.snapexists and fails when entry-template files are staged, even after the snapshot update test passes. I ran the hook checks above manually and committed with--no-verifyfor that broken hook path.Risks / follow-ups
This PR preserves current runtime behavior. The new IDs are carried as metadata only; later #726 PRs still need to define root boundary IDs, RouteManifest read models, wire-key fences, planner contracts, and compatibility envelopes before semantic decisions move out of current runtime paths.
Refs #726