feat(router): expose route graph manifest read model#1089
feat(router): expose route graph manifest read model#1089james-elicx merged 2 commits intocloudflare:mainfrom
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
9311816 to
fa014f3
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(router): expose route graph manifest read model
Good PR overall -- this is a clean, minimal read model that lines up with what issue #726 Layer 3 asks for. The manifest is derived after validation/sorting, uses only stable semantic facts, and the test coverage is solid (determinism, locale insensitivity, root-boundary multiplicity, layoutless routes). A few observations below.
What looks good
compareStableStringsfor deterministic map ordering instead oflocaleCompare-- correct and tested.assertRouteManifestTreePositionas a fail-fast invariant rather than silent fallback.- Defensive copies (
[...route.patternParts], etc.) so the manifest is decoupled from mutable route objects. - The
routeManifestreturn is backward-compatible -- the only production caller (app-router.ts:47) destructuresgraph.routes. - No behavior changes to existing routing logic.
Issues and suggestions
| layouts.set(layoutId, { | ||
| id: layoutId, | ||
| treePath, | ||
| rootBoundaryId: route.ids.rootBoundary, |
There was a problem hiding this comment.
Shared intermediate layouts get last-writer-wins rootBoundaryId.
When two routes share a layout (e.g. layout:/dashboard used by both /dashboard/a under root boundary A and /dashboard/b under root boundary B, if such a topology were possible through route groups), the second route to iterate will silently overwrite the first's rootBoundaryId on the shared layout entry.
In practice today this is likely fine because layouts within the same tree path share the same root boundary. But it's an implicit invariant the code doesn't assert. Consider either:
- Adding an assertion that if
layouts.has(layoutId), the existingrootBoundaryIdmatches the new one, or - Adding a comment explaining why last-writer-wins is safe here.
This would make the invariant explicit and catch future regressions if the route graph shape changes.
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${fnv1a64(JSON.stringify(stableShape))}`; |
There was a problem hiding this comment.
JSON.stringify ordering depends on insertion order of object literal keys.
The stableShape object literal has string keys (routes, pages, routeHandlers, etc.), which JSON.stringify serializes in insertion order per spec. This is deterministic because the literal order is fixed in source. However, the values within each array come from sortedMapValues which sorts by map key -- good.
The subtle risk is that the RouteManifestRoute (and other) objects contain arrays like layoutIds, templateIds, slotIds whose ordering comes from the route iteration order (layouts/templates) or explicit sort (slotIds). If a future change affects route iteration order inside createStaticSegmentGraph, it could change the JSON output and therefore the graphVersion even when the semantic graph hasn't changed.
This is acceptable for now since the routes are already sorted before manifest creation, but worth a brief comment noting the stability contract: "Route iteration order is stable because routes is sorted by compareRoutes before manifest creation."
| templates.set(templateId, { | ||
| id: templateId, | ||
| treePath: createAppRouteGraphTreePath(route.routeSegments, treePosition), | ||
| rootBoundaryId: route.ids.rootBoundary, |
There was a problem hiding this comment.
Same last-writer-wins observation applies to templates -- if two routes share a template id but somehow have different rootBoundaryId, the second route silently wins. Same mitigation applies as for layouts.
| if (typeof originalLocaleCompare !== "function") { | ||
| throw new Error("Expected String.prototype.localeCompare to be a function"); | ||
| } | ||
| Object.defineProperty(String.prototype, "localeCompare", { |
There was a problem hiding this comment.
Monkey-patching String.prototype.localeCompare is creative but fragile.
This works because compareStableStrings uses </> operators (not localeCompare), so the test proves the graph version is locale-insensitive. But if any other code path called during buildAppRouteGraph relies on localeCompare (e.g. Array.prototype.sort with a default comparator in some dependency), this monkey-patch could cause unexpected side effects.
The test is still valuable as-is -- just noting the fragility. A comment explaining what the monkey-patch is proving ("verifies createRouteManifestGraphVersion does not use locale-sensitive comparison") would help future readers.
| }); | ||
| }); | ||
|
|
||
| it("exposes a minimal RouteManifest read model keyed by semantic ids", async () => { |
There was a problem hiding this comment.
The manifest test covers routes, pages, layouts, templates, slots, and root boundaries -- but does not exercise the routeHandlers map. Consider adding a fixture with a route.ts and asserting segmentGraph.routeHandlers is populated. The createSemanticIdsFixture only has pages, so routeHandlers is always empty in this test.
| parallelSlots: AppRouteGraphParallelSlot[]; | ||
| }; | ||
|
|
||
| export type GraphVersion = string; |
There was a problem hiding this comment.
Nit: GraphVersion and RootBoundaryId are both string type aliases. That's fine for now as the PR description says this is the thinnest read model. But as the #726 work continues, consider branded/opaque types (e.g. string & { __brand: 'GraphVersion' }) to prevent accidental interchange. Not blocking -- just a note for future layers.
| route: "route:/dashboard/:id", | ||
| page: "page:/dashboard/:id", | ||
| routeHandler: "route-handler:/dashboard/:id", | ||
| rootBoundary: "root-boundary:/", |
There was a problem hiding this comment.
Good -- adding rootBoundary to the existing fixture ensures the entry-template tests stay in sync with the new AppRouteSemanticIds shape.
|
Review posted. Here's a summary of the feedback: Overall: Clean, minimal read model that matches #726 Layer 3 scope. No behavior changes to existing routing, backward-compatible return type, good test coverage. Key observations:
|
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
fa014f3 to
b2f8ab2
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
Good work addressing the first bonk review — the assertRouteManifestRootBoundary invariant check is a solid improvement over the original last-writer-wins behavior. The core design (thin read model derived after validation/sort, fail-fast assertions, defensive copies, backward-compatible return type) is clean and well-scoped for Layer 3.
A few remaining items, one of which is a real bug in the test helper.
| pages: Array.from(manifest.segmentGraph.pages.entries()), | ||
| templates: Array.from(manifest.segmentGraph.templates.entries()), | ||
| slots: Array.from(manifest.segmentGraph.slots.entries()), | ||
| rootBoundaries: Array.from(manifest.segmentGraph.rootBoundaries.entries()), |
There was a problem hiding this comment.
snapshotRouteManifest omits routeHandlers from the snapshot, which means the "keeps RouteManifest graph output stable across different filesystem roots" test at line 418 does not cover route handler stability at all. This is a quiet gap — if route handler entries were non-deterministic across filesystem roots, this test wouldn't catch it.
| rootBoundaries: Array.from(manifest.segmentGraph.rootBoundaries.entries()), | |
| rootBoundaries: Array.from(manifest.segmentGraph.rootBoundaries.entries()), | |
| routeHandlers: Array.from(manifest.segmentGraph.routeHandlers.entries()), | |
| }; |
| const graph = await buildAppRouteGraph(appDir, createValidFileMatcher()); | ||
| const rootBoundaryIds = graph.routes | ||
| .map((route) => route.ids.rootBoundary) | ||
| .sort((left, right) => String(left).localeCompare(String(right))); |
There was a problem hiding this comment.
Nit: this sorts with localeCompare, but the manifest code itself deliberately avoids locale-sensitive sorting via compareStableStrings. Using localeCompare in the test assertion could mask a locale-dependent ordering difference in theory (though in practice these strings are ASCII). Consider using .sort() (default comparator, which is codepoint-based) for consistency with the production code's philosophy:
| .sort((left, right) => String(left).localeCompare(String(right))); | |
| .sort((left, right) => (String(left) < String(right) ? -1 : String(left) > String(right) ? 1 : 0)); |
Not blocking, just a consistency note.
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${fnv1a64(JSON.stringify(stableShape))}`; |
There was a problem hiding this comment.
The graphVersion comment at line 428-429 is helpful. One additional subtlety worth documenting: JSON.stringify on the stableShape object literal serializes top-level keys in source-code insertion order (per spec), and nested arrays within each RouteManifestRoute (like layoutIds, templateIds, slotIds) derive their order from route iteration. The slotIds are explicitly sorted by compareStableStrings (line 314), which is good. But layoutIds and templateIds inherit their order from route.ids.layouts / route.ids.templates, which come from layoutTreePositions.map(...) — meaning their order is governed by tree position depth, not string comparison.
This is actually fine and semantically correct (tree order is stable), but the comment could be extended slightly to note that inner arrays use tree-position order, not alphabetical order, so readers don't expect sortedMapValues to cover that.
| layoutTreePositions: readonly number[], | ||
| ): RootBoundaryId | null { | ||
| const rootLayoutPosition = layoutTreePositions[0]; | ||
| if (rootLayoutPosition === undefined) return null; |
There was a problem hiding this comment.
The resolveRootBoundaryId function returns null when layoutTreePositions[0] is undefined, which handles layoutless routes correctly. However, computeRootParamNames at line 944 also guards on layoutTreePositions[0] but uses rootLayoutPosition == null || rootLayoutPosition <= 0 (loose equality, catching both null and undefined, plus a <= 0 guard). The difference in guard styles between these two sibling functions is slightly surprising — resolveRootBoundaryId doesn't guard against rootLayoutPosition <= 0.
Is position 0 a valid root layout position (root layout in the app directory itself)? Looking at computeLayoutTreePositions, position 0 means the layout is at appDir itself — the true root. So computeRootParamNames returns [] for position 0 (no segments before the root), which is correct. resolveRootBoundaryId would create root-boundary:/ for position 0, which is also correct. The asymmetry is fine on closer reading, but a brief comment on why <= 0 isn't needed here would help future readers.
|
Review posted. Here's a summary of the feedback: Overall: Clean, well-scoped read model for #726 Layer 3. The Key findings:
|
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
b2f8ab2 to
3922aa2
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
This is a well-scoped Layer 3 deliverable. The read model is thin, derived after validation/sort, uses defensive copies, and the return type change is backward-compatible (the only production caller at app-router.ts:47 destructures graph.routes). The assertRouteManifestRootBoundary invariant check that was added after the first bonk review is a real improvement — it turns a silent last-writer-wins scenario into a fail-fast assertion.
Test coverage is good: determinism, locale insensitivity, root-boundary multiplicity, layoutless routes, cross-filesystem-root stability, and route handler entries in the manifest test. The two prior bonk reviews covered most of the surface area well.
One issue that should be fixed before merge, plus a few smaller items.
Must fix
routeManifestis not cached in the production caller —app-router.ts:47-51callsbuildAppRouteGraphand cachesgraph.routes, but discardsgraph.routeManifest. When a later #726 layer needs the manifest at runtime (Layer 4 NavigationPlanner needs it as input), the caller will need to be updated. This isn't a bug today, but the manifest is computed on every uncached call and immediately thrown away. Either cache it alongsidecachedRoutesnow, or add a comment noting it's intentionally discarded until a consumer exists. I'd prefer caching it since it's cheap and avoids a future "oops we recompute it" moment.
Should fix
snapshotRouteManifestis missing therouteHandlerskey — Wait, looking again: it does includerouteHandlersat line 48. The first bonk review's suggestion to add it appears to have been addressed. Good.
Nits / non-blocking
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${fnv1a64(JSON.stringify(stableShape))}`; |
There was a problem hiding this comment.
The comment at lines 428-431 is good but could be slightly more precise about what "semantic order" means for each inner array.
layoutIds and templateIds come from route.ids.layouts / route.ids.templates, which are mapped from layoutTreePositions / templateTreePositions — so their order is tree-position depth, not alphabetical. slotIds are explicitly sorted by compareStableStrings (line 314). The sortedMapValues call covers top-level map iteration order but not these inner arrays.
This is all correct and stable, but a reader skimming the comment might assume sortedMapValues handles everything. Consider extending the comment to note that inner arrays use tree-position order (layouts/templates) or explicit stable-string sort (slots).
| layoutTreePositions: readonly number[], | ||
| ): RootBoundaryId | null { | ||
| const rootLayoutPosition = layoutTreePositions[0]; | ||
| if (rootLayoutPosition === undefined) return null; |
There was a problem hiding this comment.
Minor asymmetry with computeRootParamNames at line 946, which guards rootLayoutPosition == null || rootLayoutPosition <= 0. Here you only guard === undefined.
Both are correct: computeRootParamNames returns [] for position 0 because there are no segments before the root layout, while resolveRootBoundaryId correctly returns a root-boundary:/ for position 0 (the root layout at the app directory itself). The <= 0 guard in computeRootParamNames is specifically about "no segments to slice", not about validity.
A one-line comment here explaining why position 0 is valid (unlike in computeRootParamNames) would help readers who notice the asymmetry:
| if (rootLayoutPosition === undefined) return null; | |
| const rootLayoutPosition = layoutTreePositions[0]; | |
| if (rootLayoutPosition === undefined) return null; | |
| // Position 0 means the root layout is at appDir itself — it still owns a | |
| // real root boundary. (computeRootParamNames treats 0 differently because | |
| // there are no segments before the root layout to extract param names from.) |
| parallelSlots: AppRouteGraphParallelSlot[]; | ||
| }; | ||
|
|
||
| export type GraphVersion = string; |
There was a problem hiding this comment.
Nit for future work: GraphVersion and RootBoundaryId as plain string aliases provide documentation value but no type safety. As #726 layers accumulate more ID types (OperationToken, RouteSnapshotId, DeploymentVersion, etc.), branded types would prevent accidental interchange:
export type GraphVersion = string & { readonly __brand: 'GraphVersion' };
export type RootBoundaryId = string & { readonly __brand: 'RootBoundaryId' };Not blocking — the issue description says "thinnest read model" and plain aliases are fine for now. Just flagging for when the type surface grows.
| const graph = await buildAppRouteGraph(appDir, createValidFileMatcher()); | ||
| const rootBoundaryIds = graph.routes | ||
| .map((route) => route.ids.rootBoundary) | ||
| .sort((left, right) => { |
There was a problem hiding this comment.
This sort comparator manually implements codepoint-based comparison, which is consistent with compareStableStrings in production code. Good.
Tiny nit: you could extract this into a small helper since the same pattern appears at line 375 and also conceptually in the withReverseLocaleCompare test. Not worth a change, just noting the duplication.
| } | ||
| // This proves RouteManifest graphVersion canonicalization does not depend on | ||
| // locale-sensitive sorting. Keep the patched window scoped to graph building. | ||
| Object.defineProperty(String.prototype, "localeCompare", { |
There was a problem hiding this comment.
The localeCompare monkey-patch is a creative way to prove locale insensitivity. The scoping comment at line 60-61 is helpful.
One fragility concern: if any code path during buildAppRouteGraph (including scanWithExtensions, fs.readdir sort order, etc.) uses localeCompare internally, this patch would affect it in unexpected ways. In practice this is fine because Node.js filesystem APIs don't use localeCompare, and the production sorting code uses compareRoutes / compareStableStrings which use </> operators.
The test is valuable as-is. Just noting the implicit assumption for future maintainers.
| }); | ||
|
|
||
| expect(rootBoundaryIds).toEqual(["root-boundary:/(marketing)", "root-boundary:/(shop)"]); | ||
| expect(Array.from(graph.routeManifest.segmentGraph.rootBoundaries.keys()).sort()).toEqual([ |
There was a problem hiding this comment.
.sort() with no comparator uses codepoint-based comparison by default, which matches the production compareStableStrings behavior. This is fine here since the values are simple ASCII strings. Just noting that this is a different sort invocation style than lines 375-381 (explicit comparator) — both are correct but inconsistent in style.
|
Review posted. Here's a summary: Overall: Clean, well-scoped Layer 3 deliverable for #726. The read model is thin, derived after validation/sort, backward-compatible (only production caller at Key findings:
|
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
2 times, most recently
from
9ec70f8 to
467ec30
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
Clean, well-scoped read model. The design is sound: thin derived manifest, fail-fast assertions, defensive copies, backward-compatible return type change. Three prior bonk reviews have already covered the surface well, so I'll focus on what remains unaddressed.
Must fix
- Unrelated
app-elements.test.tschange included in this PR — see inline comment.
Should fix
appRouteGraphis exported but has zero production callers — The newappRouteGraph()function is exported fromapp-router.tsbut nothing inpackages/vinext/src/imports it. Only the test does. The third bonk review flagged that the manifest isn't cached in the production path — it is cached (the whole graph object is cached), but it's unreachable from production code sinceindex.tsstill callsappRouter()which only returnsgraph.routes. I'd either (a) add aTODO(#726)comment on the export noting it exists for Layer 4, or (b) don't exportappRouteGraphat all yet and only export it when a production consumer exists. Exporting unused public API is a maintenance burden.
Nits (non-blocking)
| if (!isAppElementsRecord(payload)) return; | ||
|
|
||
| expect(AppElementsWire.readMetadata(payload)).toEqual({ | ||
| artifactCompatibility: createArtifactCompatibilityEnvelope(), |
There was a problem hiding this comment.
This change adds artifactCompatibility to the test expectation, but the test already passes on main without it (the .toEqual() assertion passes because readMetadata returns artifactCompatibility and .toEqual does a deep structural comparison — wait, actually .toEqual is a deep equality check, which means it would fail if the actual result has a key the expected object doesn't have... let me reconsider).
Actually, .toEqual ignores extra properties in the actual object? No — Vitest's .toEqual uses expect's deep.equal which checks both ways. So if readMetadata returns artifactCompatibility and the test expectation didn't include it, the test should have been failing. But I confirmed it passes on main.
Looking more carefully: toEqual in Vitest uses jestExpect's toEqual, which does asymmetric matching — it only checks that the expected structure is present in the actual result. Edit: No, toEqual checks deep strict equality. If this test passes on main without the artifactCompatibility key, then readMetadata must not have been returning it at the time of the branch point.
Regardless, this looks like a legitimate fix to make the test assertion complete. But it's unrelated to the route manifest feature. If it's fixing a pre-existing gap in the test, consider putting it in a separate commit or noting it in the PR description so reviewers understand why it's here.
There was a problem hiding this comment.
This is not a route-manifest behavior change. After rebasing onto current main, upstream had added artifactCompatibility to AppElementsWire.readMetadata(), and CI failed in tests/app-elements.test.ts at this legacy metadata assertion. The one-line test update keeps the branch aligned with the current codec read model; no runtime code changed. I added a Rebase note to the PR body so the diff context is explicit.
| routeHandlerId: route.ids.routeHandler, | ||
| layoutIds: [...route.ids.layouts], | ||
| templateIds: [...route.ids.templates], | ||
| slotIds: route.parallelSlots.map((slot) => slot.id).sort(compareStableStrings), |
There was a problem hiding this comment.
slot.id here is typed as string | undefined on the base ParallelSlot type (the id field is id?: string), but on AppRouteGraphParallelSlot it's required (id: string). The route.parallelSlots array is typed as AppRouteGraphParallelSlot[] so this is safe — but worth noting that if a code path ever passes a bare ParallelSlot with a missing id, the .sort() would silently sort "undefined" strings.
The type system prevents this today. Just flagging the narrowing dependency.
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${fnv1a64(JSON.stringify(stableShape))}`; |
There was a problem hiding this comment.
Minor: JSON.stringify on the stableShape serializes top-level object keys in source-literal order (per ES spec), and the values come from sortedMapValues which sorts by map key — good.
But the RouteManifestRoute objects inside the routes array contain inner arrays (layoutIds, templateIds) whose order is tree-position order (not alphabetical), and slotIds is explicitly sorted by compareStableStrings. Both are stable, but for different reasons. The comment at lines 428-431 is helpful but doesn't distinguish these two stability guarantees. A future maintainer changing template tree-position ordering might not realize it affects graphVersion. Consider extending the comment to note the inner-array ordering sources.
There was a problem hiding this comment.
Tightened in the latest push. The comment now names the two inner-array ordering sources directly: tree-position order for layouts/templates and stable string order for slots.
| * Scan the app/ directory and return the route graph. | ||
| */ | ||
| export async function appRouter( | ||
| export async function appRouteGraph( |
There was a problem hiding this comment.
appRouteGraph is exported but has no production callers — only the test in routing.test.ts imports it. All production code still goes through appRouter(), which discards the manifest.
This is fine if it's intentional scaffolding for Layer 4. But since this is a public API surface (any downstream code can import it), I'd add a brief doc comment noting its purpose:
| export async function appRouteGraph( | |
| /** | |
| * Scan the app/ directory and return the route graph. | |
| * TODO(#726): Currently unused in production — exposed for Layer 4 NavigationPlanner. | |
| */ | |
| export async function appRouteGraph( |
There was a problem hiding this comment.
Fixed in the latest push. appRouteGraph() now has a TODO(#726) doc comment making the Layer 4 planner handoff explicit while preserving appRouter() for current callers.
| } | ||
|
|
||
| for (const slot of route.parallelSlots) { | ||
| slots.set(slot.id, { |
There was a problem hiding this comment.
Slots don't get the same assertRouteManifestRootBoundary dedup check that layouts and templates get. A slot can appear on multiple routes (inherited parallel slots), but the set here silently last-writer-wins on the slot entry.
This is safe because RouteManifestSlot doesn't carry rootBoundaryId — it only has id, key, name, all of which are the same regardless of which route contributes the slot. But it's worth noting the asymmetry: layouts/templates get an assertion guard because they carry rootBoundaryId, while slots don't need one because they're boundary-agnostic. A brief comment would help readers understand why slots are treated differently.
There was a problem hiding this comment.
Fixed in the latest push. The slot loop now documents the asymmetry: slots are boundary-agnostic in this minimal manifest, while layouts/templates carry rootBoundaryId facts and need the invariant guard.
| parallelSlots: AppRouteGraphParallelSlot[]; | ||
| }; | ||
|
|
||
| export type GraphVersion = string; |
There was a problem hiding this comment.
All three bonk reviews have flagged this, so I'll just +1: plain string aliases are fine for the thinnest read model, but as #726 layers accumulate more ID types, branded types would prevent accidental interchange. Not blocking.
|
Review posted. Here's a summary of the feedback: Overall: Clean, well-scoped Layer 3 read model. Design is sound -- thin derived manifest, fail-fast assertions with Key findings:
|
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
467ec30 to
eb92039
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
Clean, well-scoped work. The read model is thin and derived, the fail-fast assertions (assertRouteManifestRootBoundary, assertRouteManifestTreePosition) are the right pattern, defensive copies prevent mutation leaks, and the caching refactor in app-router.ts is correct. Test coverage is solid: determinism, locale insensitivity, distinct root boundaries, layoutless routes, cross-filesystem-root stability, and route handler entries.
I have one issue to flag and a few nits. The prior bonk reviews already covered most of the surface -- I'll avoid repeating points that were already addressed.
Issue
fnv1a64 is not a real 64-bit hash — it's two independent 32-bit FNV-1a rounds concatenated. The second round uses a different seed but still processes bytes in exactly the same sequence. This means the two halves are correlated: they don't give you independent collision resistance, so the effective entropy is closer to 32 bits than 64. For a graphVersion used as a cache/identity key in future #726 work, this may matter. Using node:crypto (createHash('sha256') truncated, or even createHash('md5')) would give actual collision resistance with negligible perf difference at these input sizes. Not blocking for this PR since the hash is internal and the manifest is small, but worth noting before #726 layers start depending on graphVersion for cache invalidation correctness.
Nits (non-blocking)
Inline below.
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${fnv1a64(JSON.stringify(stableShape))}`; |
There was a problem hiding this comment.
Nit: JSON.stringify on an object literal serializes keys in insertion order (per spec), so top-level ordering is fixed by source code. But the readonly arrays inside each RouteManifestRoute (layoutIds, templateIds, slotIds) derive their ordering from different sources:
layoutIds/templateIds: tree-position order (fromlayoutTreePositions.map(...))slotIds: explicitcompareStableStringssort (line 314)
sortedMapValues only handles the top-level map iteration order. If a future change reorders tree positions, graphVersion changes even if the semantic graph is identical. The comment at 430-432 captures the contract well but could name these two distinct stability sources explicitly so a future editor doesn't assume sortedMapValues covers inner arrays.
| for (let index = 0; index < route.ids.layouts.length; index++) { | ||
| const layoutId = route.ids.layouts[index]; | ||
| const treePosition = route.layoutTreePositions[index]; | ||
| if (layoutId === undefined) continue; |
There was a problem hiding this comment.
Tiny robustness nit: the continue here skips a layout whose ID is undefined, but route.ids.layouts is built from layoutTreePositions.map(...) (line 990-992), which always returns a string for each position. So layoutId can never be undefined in practice — the array has no holes. This guard is harmless but might mislead a reader into thinking sparse layout ID arrays are expected. Consider either removing it or adding a comment noting it's defensive.
| parallelSlots: AppRouteGraphParallelSlot[]; | ||
| }; | ||
|
|
||
| export type GraphVersion = string; |
There was a problem hiding this comment.
+1 to prior reviews on branded types. Not blocking, but as a concrete suggestion for when this graduates: Flavor types (lightweight brands that don't require casts at construction) tend to work well for graph IDs:
type Flavor<T, Brand> = T & { readonly __flavor?: Brand };
export type GraphVersion = Flavor<string, 'GraphVersion'>;
export type RootBoundaryId = Flavor<string, 'RootBoundaryId'>;This prevents accidental interchange (GraphVersion isn't assignable to RootBoundaryId) without requiring explicit casts when creating the values from string literals.
| }); | ||
|
|
||
| expect(rootBoundaryIds).toEqual(["root-boundary:/(marketing)", "root-boundary:/(shop)"]); | ||
| expect(Array.from(graph.routeManifest.segmentGraph.rootBoundaries.keys()).sort()).toEqual([ |
There was a problem hiding this comment.
The default .sort() (no comparator) uses string coercion + codepoint comparison, which matches compareStableStrings for plain strings. But the null values in rootBoundaryIds at line 374 would be coerced to "null", which sorts after "root-boundary:...". Since this test fixture has no null root boundaries, it's fine — but if a future test adds a layoutless route to this fixture, the String(left) explicit coercion at line 376 would be needed. The explicit comparator at lines 375-381 is the safer choice and consistent with the production code's philosophy. Good.
| * navigation planner owns route graph facts. | ||
| */ | ||
| export async function appRouter( | ||
| export async function appRouteGraph( |
There was a problem hiding this comment.
The TODO(#726) comment addresses the prior review feedback well. One remaining observation: appRouteGraph is exported from app-router.ts, but app-router.ts's re-exports in index.ts (line 11) only import appRouter and invalidateAppRouteCache. So appRouteGraph is reachable by deep-importing vinext/routing/app-router, but not from the package's public surface. This is probably intentional (internal API for Layer 4), but worth confirming — if it's meant to stay internal, a @internal JSDoc tag would signal that.
|
Review posted. Here's a summary of the feedback: Overall: Clean, well-scoped Layer 3 read model for #726. The design is sound: thin derived manifest, fail-fast assertions ( Key findings:
|
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
eb92039 to
16ca243
Compare
|
Addressed the latest Bonk review in
Validated with targeted route graph/app-elements tests, touched-file |
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
16ca243 to
e04749e
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
This is a well-scoped, clean addition. The read model is thin, derived post-validation/sort, uses defensive copies, and the caching refactor in app-router.ts is correct. The five prior bonk reviews have covered this thoroughly and the author has addressed the substantive feedback (SHA-256 for graphVersion, Flavor types, @internal annotation, slot asymmetry comments, assertRouteManifestRootBoundary guards). I have one remaining observation and a few minor items.
What looks good
Flavorbranded types forGraphVersionandRootBoundaryId— lightweight and appropriate for this stage.- SHA-256 via
node:cryptoforgraphVersion— correct collision resistance, follows the project's "prefer Node built-ins" convention. assertRouteManifestRootBoundaryas a fail-fast invariant for shared layouts/templates.assertRouteManifestTreePositionto catch generated ID / tree-position metadata drift.appRouter()delegates toappRouteGraph()with shared cache — no duplication, backward-compatible.- Test coverage: determinism, locale insensitivity, distinct root boundaries, layoutless routes, cross-filesystem-root stability, route handler entries, caching identity.
compareStableStringsusing</>operators instead oflocaleCompare— correct and tested.- Slot boundary-agnostic comment is clear.
Observations below.
| slots: sortedMapValues(segmentGraph.slots), | ||
| rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries), | ||
| }; | ||
| return `graph:${createHash("sha256").update(JSON.stringify(stableShape)).digest("hex")}`; |
There was a problem hiding this comment.
The graph: prefix means graphVersion values are 71 characters ("graph:" + 64 hex chars). This is fine, but the prefix creates a slight oddity: the Flavor-branded GraphVersion type suggests it's an opaque token, yet the format is publicly observable and tested against (/^graph:[a-f0-9]{64}$/). If Layer 4 ever needs to compare graph versions for ordering (not just equality), the graph: prefix would need to be stripped first.
Not blocking — just noting that the prefix is a minor commitment to a format that tests now assert on. If the version is meant to be opaque, consider whether the test should assert typeof manifest.graphVersion === 'string' && manifest.graphVersion.length > 0 instead of matching the internal format.
| } | ||
|
|
||
| for (const [index, templateId] of route.ids.templates.entries()) { | ||
| const treePosition = route.templateTreePositions?.[index]; |
There was a problem hiding this comment.
Nit: route.templateTreePositions is accessed with optional chaining (?.) here, but route.layoutTreePositions at line 336 is not. Looking at the AppRoute type, templateTreePositions is typed as number[] | undefined (optional), while layoutTreePositions is number[] (required). The optional chaining is correct, but this asymmetry between the two loops is easy to miss — a reader might wonder why one needs ?. and the other doesn't.
The type system enforces this correctly so it's not a bug, just noting the visual inconsistency.
| patternParts: [...route.patternParts], | ||
| isDynamic: route.isDynamic, | ||
| paramNames: [...route.params], | ||
| rootParamNames: [...(route.rootParamNames ?? [])], |
There was a problem hiding this comment.
Minor: route.rootParamNames is spread with a fallback (?? []), implying it can be undefined. If the AppRouteGraphRoute type guarantees rootParamNames is always populated (even if empty), the fallback is misleading. If it can genuinely be undefined, the type should reflect that.
Looking at usage: computeRootParamNames always returns string[] (never undefined), but the field on AppRoute may be optional depending on how the route was constructed. The fallback is safe — just noting the implicit type gap.
| * @internal | ||
| */ | ||
| export async function appRouter( | ||
| export async function appRouteGraph( |
There was a problem hiding this comment.
The @internal JSDoc tag and TODO(#726) comment are good. One thing worth confirming: since app-router.ts re-exports are pulled into index.ts (line 11), and index.ts only imports appRouter and invalidateAppRouteCache, the appRouteGraph function is reachable only via deep import (vinext/routing/app-router). The @internal tag signals this correctly.
If the package's exports map in package.json restricts deep imports, appRouteGraph would be truly unreachable from external consumers — which is the right posture for scaffolding. Worth verifying the exports map doesn't inadvertently expose ./routing/*.
| // locale-sensitive sorting. Keep the patched window scoped to graph building. | ||
| Object.defineProperty(String.prototype, "localeCompare", { | ||
| configurable: true, | ||
| value(this: string, compareString: string) { |
There was a problem hiding this comment.
The monkey-patched localeCompare reverses the argument order via Reflect.apply(originalLocaleCompare, compareString, [this]) — swapping this and compareString. This is a clever way to invert sort order without changing the function signature.
One edge case: if localeCompare is called with additional arguments (locale, options), this patch would drop them since [this] only passes one arg. For the purpose of this test that's fine since no production code in the graph builder calls localeCompare at all (which is the whole point). Just flagging for completeness.
| const manifest = graph.routeManifest; | ||
| const segmentGraph = manifest.segmentGraph; | ||
|
|
||
| expect(manifest.graphVersion).toMatch(/^graph:[a-f0-9]{64}$/); |
There was a problem hiding this comment.
Good: the regex asserts the SHA-256 format (graph: prefix + 64 hex chars). This locks the format, which is appropriate for a test that validates determinism. If the format ever changes (e.g., switching hash algorithm), this test will catch it immediately — that's a feature, not a bug.
|
Review posted. Here's a summary: Overall: Clean, well-scoped Layer 3 read model for #726. The author has addressed all substantive feedback from the five prior bonk reviews (SHA-256 for graph version, Flavor types, Key observations posted:
No blocking issues. This is ready to merge pending CI. |
The App Router graph now exposes root-boundary identity and a stable RouteManifest read model instead of leaving these facts implicit inside per-route arrays. Future cloudflare#726 planner and compatibility work needs a semantic lookup surface for routes, layouts, pages, templates, slots, and root boundaries before it can stop reading meaning from transport keys. This adds RootBoundaryId, graphVersion, StaticSegmentGraph maps, and fail-fast invariant checks for mismatched layout/template positions while preserving the existing routes array for current callers.
NathanDrake2406
force-pushed
the
nathan/726-graph-02-route-manifest
branch
from
e04749e to
e23da31
Compare
|
Addressed the only latest-review item with a real code implication in Also verified the Validation:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(router): expose route graph manifest read model
This is a clean, well-scoped Layer 3 deliverable for #726. The prior five bonk reviews covered the surface thoroughly, and the author has addressed every substantive piece of feedback across three follow-up commits (SHA-256 for graphVersion, Flavor branded types, @internal annotation, required rootParamNames on the graph route type, slot boundary-agnostic comment, root boundary invariant guards).
What looks good
- Thin read model: The manifest is derived post-validation/sort and contains only stable semantic facts. No planner, cache, or transport concerns leak in.
- Fail-fast invariants:
assertRouteManifestRootBoundarycatches shared-layout root boundary divergence at build time rather than silently last-writer-wins.assertRouteManifestTreePositioncatches ID/tree-position metadata drift. - Defensive copies:
[...route.patternParts],[...route.params], etc. decouple the manifest from mutable route objects. - Backward-compatible:
appRouter()delegates toappRouteGraph()with a shared cache. All existing callers (index.ts,run-prerender.ts,nitro-route-rules.ts,report.ts) still callappRouter()and getAppRouteGraphRoute[]-- no breaking change. rootParamNamesmade required onAppRouteGraphRoute: Clean type narrowing viaOmit+ re-declaration.computeRootParamNamesalways returnsstring[], so the optional-to-required promotion is safe.- SHA-256 via
node:crypto: Correct collision resistance, follows the project's "prefer Node built-ins" convention. Thegraph:prefix + 64-char hex format is tested and deterministic. - Test coverage: Determinism, locale insensitivity, distinct root boundaries, layoutless routes, cross-filesystem-root stability, route handler entries, caching identity. The
localeComparemonkey-patch is a creative proof thatcompareStableStringsis locale-agnostic.
Minor observations (non-blocking)
-
sortedMapValueskey widening (app-route-graph.ts:439):sortedMapValuesis typed asReadonlyMap<string, T>, butsegmentGraph.rootBoundariesisReadonlyMap<RootBoundaryId, T>. This works becauseFlavor<string, ...>extendsstring, but the branded key is silently widened. If a future refactor changesFlavorto a true opaque type, this call would break. Not blocking -- just noting the generic could accept a looser key constraint. -
Sort style inconsistency in test (
tests/app-route-graph.test.ts:384):.sort()with no comparator vs explicit codepoint comparator (lines 375-381) use different styles for the same intent. Both are correct for ASCII strings, just visually inconsistent.
|
Review posted. Here's a summary: Approved. This is a clean, well-scoped Layer 3 deliverable for #726. The author has addressed all substantive feedback from the five prior bonk reviews across three follow-up commits. Key strengths:
Two non-blocking observations:
|
What this changes
Implements #726-GRAPH-02/03 for issue #726 by exposing root-boundary identity from the App Router route graph and adding a minimal
RouteManifestread model.The route graph now includes:
RootBoundaryIdon graph-minted semantic route IDsgraphVersionRouteManifestwith aStaticSegmentGraphBonk: please read issue #726 before reviewing so the big-picture route manifest and navigation planner architecture is fresh.
Why
Issue #726 is moving route meaning out of transport keys and scattered runtime observations. The graph already minted semantic IDs for route entries, but callers still only received the raw
routesarray, with root-boundary identity implicit inlayoutTreePositions[0].That makes future planner, compatibility, and cache-proof work depend on incidental route array shape rather than an owned graph read model.
Approach
This keeps the current route matching behavior intact and adds the thinnest read model on top of the existing route graph output.
The manifest is derived after route validation and sorting, uses only stable semantic facts for its version input, and fails fast if generated layout/template IDs ever lose their matching tree-position metadata. It deliberately does not add a transition automaton, planner ownership, cache dependency graph, skip transport, or runtime compatibility semantics.
appRouteGraph()is exposed as the Layer 4 handoff point for the future #726 navigation planner whileappRouter()remains backward-compatible for current callers.Correctness oracle: Vinext internal invariant for issue #726. The read model must expose stable graph facts without changing current App Router routing behavior.
Rebase note
This branch also includes a one-line
tests/app-elements.test.tsexpectation update after rebasing onto currentmain: upstream addedartifactCompatibilitytoAppElementsWire.readMetadata(), and the PR's Vitest unit CI failed until the legacy metadata assertion matched the current codec read model. This is test-only and does not change runtime behavior.Validation
vp test run tests/app-elements.test.ts tests/routing.test.ts tests/app-route-graph.test.ts tests/entry-templates.test.tsvp check packages/vinext/src/routing/app-router.ts packages/vinext/src/routing/app-route-graph.ts tests/app-elements.test.tsvp check --fix,knip --no-progress,vp packvp run vinext#buildvp run vinext#buildand the commit hook pack still emit the existing virtual-module external warnings for generated entry placeholders.Risks / follow-ups
This PR intentionally does not promote any planner decisions or cache authority. It only exposes the minimal graph read model needed by later #726 work.