Repository navigation
Matrix-critical refactors from review (fresh/revalidation unification, trie suffix order, loader-cache rebind) #568
Description
Summary
Internal refactors and one correctness item flagged by the multi-agent review that sit on semantic-matrix-critical paths (middleware scope, handler-first ordering, segment resolution, the matching trie). Each requires a deliberate change plus a semantic-matrix re-run (pnpm --filter @rangojs/router exec playwright test semantic-matrix), so they were intentionally deferred from the stabilization PR rather than applied as "safe cleanups". Mostly internal — the one consumer-observable item (trie suffix ordering) has a userland example.
Correctness (consumer-observable)
Trie suffix-param matching has no longest-suffix-wins rule
walkTrie's suffix-param tier (node.xp) iterates suffix keys in object order; overlapping suffixes resolve by declaration order, not by specificity.
// Consumer expects the more specific suffix to win:
path("/:file.min.js", MinJs) // more specific
path("/:file.js", Js)
// Request /app.min.js may match :file.js (file="app.min") depending on declaration order.Fix: pre-sort node.xp suffix keys by descending length at build time (src/build/route-trie.ts, where .xp is constructed — allocation-free on the match hot path). Add trie tests + a semantic-matrix row.
Internal refactors (no consumer-facing change; do for maintainability + measured before/after)
- fresh.ts vs revalidation.ts are near-duplicate resolution trees.
resolveSegment/resolveOrphanLayout/resolveParallelEntry/resolveLoaders+collectEntryLoaders(byte-identical) mirror each other, both encoding the handler-first guarantee. Unify behind a single traversal with a per-segment strategy. Matrix-critical — the review explicitly says treat this as a deliberate change with a full matrix re-run. - M11: orphan-revalidation inlines ~166 lines of parallel-slot resolution instead of calling the shared helper (
revalidation.ts:1154-1319). Two deliberate divergences (unconditionaldefaultOverride:{value:true}, loader-resolve-before-push order) must become explicit policy args, pinned byparallel-revalidate-fns-invocation.test.ts. PR feat(router): opt out of router-placed ViewTransition via transition({ viewTransition }) #526 shows the copies have drifted before. - Parallel-slot component-resolution block duplicated within the two revalidation functions (~40 lines, on the partial-revalidation hot path, entangles
_currentSegmentId/handlerRanmutation + streamed-handler call). - Duplicated merge-and-clone Response builder in
executeMiddleware(+ a related block inexecuteInterceptMiddleware) — touches the response-header-precedence contract pinned bymiddleware-throw-response.test.ts/intercept-middleware-headers.test.ts. -
yieldFromStorevs runtime cache-hit fresh-loader resolution duplicated inmatch-middleware/cache-lookup.ts— parity-sensitive, on the cache hot path. -
loader-cachepermanently rebindsctx.use, chaining one wrapper per cached loader (O(N) dispatch); correctness relies on an undocumented synchronous capture-before-overwrite invariant. Replace mutate-and-chain with a stable interceptor + Map prime (touchessetupLoaderAccessinloader-resolution.ts); needs multi-cached-loader accumulation tests.
Acceptance
- Each item: behavior-preserving (or matrix rows updated if intentionally changed),
execution-model.mdupdated where the contract is restated, semantic matrix green,debugPerformancebefore/after for the hot-path ones.
Activity
fix(router): matrix-critical review follow-ups — #568 trie-suffix, #569…
refactor(router): matrix-critical de-duplications — middleware merge,…
ivogt commentedon Jun 14, 2026
Reopening note: #576 shipped 4 of the 6 internal refactors + the trie-suffix fix, but two sub-items remain (orphan→shared-helper unification with policy args, and the fresh/reval traversal merge). They were deferred because divergence (A) is intentional and the existing pinning test doesn't actually lock it — see #577 for the forensic verdict and the coverage-first plan.
ivogt commentedon Jun 14, 2026
Status: 5 of 7 items done; 2 sub-items remain (this issue tracks them)
Done:
executeMiddleware/executeInterceptMiddleware— refactor(router): matrix-critical de-duplications — middleware merge, loader-cache interceptor, cache-hit loaders, parallel-slot (#568) #576yieldFromStorevs runtime cache-hit fresh-loader resolution duplication — refactor(router): matrix-critical de-duplications — middleware merge, loader-cache interceptor, cache-hit loaders, parallel-slot (#568) #576loader-cacheO(N)ctx.userebind → single stable interceptor + Map — refactor(router): matrix-critical de-duplications — middleware merge, loader-cache interceptor, cache-hit loaders, parallel-slot (#568) #576All shipped behavior-preserving (semantic matrix 61/61 dev+prod, no row changes; 4-way adversarial refutation, all
refuted: false).Remaining (deferred — real semantic risk the current tests don't cover):
Orphan-revalidation calls the shared parallel helper (M11).
resolveOrphanLayoutWithRevalidationstill inlines ~166 lines. Forensic verdict on the two divergences:defaultOverride: { value: true }vs mainbelongsToRoute || isNewParent— INTENTIONAL (since refactor(router): split 4 god files into focused submodules #228, deliberately preserved in fix(router): invoke parallel slot revalidate() fns when slot missing from clientSegmentIds #482 with a justifying comment; reachable atbelongsToRoute=false; it avoids the fix(router): invoke parallel slot revalidate() fns when slot missing from clientSegmentIds #482 blank-parent-chain-slot bug). Must be carried as an explicit policy arg, never normalized.$$id). Safe to normalize to the shared order.Unify
fresh.tsvsrevalidation.tsresolution trees. The "byte-identical" premise is false —collectEntryLoaderstwins differ in the loader call (+8 args) andmatchedIdsaccumulation;resolveSegmenttwins diverge in revalidate gating + route-handler ordering. Strategy-parameterized traversal, not a literal dedup; do after sub-item 1.Blocker — coverage first.
parallel-revalidate-fns-invocation.test.tsdoes not lock either divergence (every orphan case usesbelongsToRoute=true, where (A) coincides; fns override the seed). Before refactoring, add a pinning test for the divergent (A) case: orphan parallel withbelongsToRoute=false, slot ∉clientSegmentIds, parent ∈clientSegmentIds, zero revalidate fns → assert the slot resolves non-null and is inmatchedIds.(Consolidating the duplicate follow-up #577 into this issue, since this one is the natural home.)
ivogt commentedon Jun 14, 2026
Sub-item 1 (fresh.ts vs revalidation.ts traversal unify): experiment run, mostly not worth it
Attempted the strategy-parameterized traversal unification on a throwaway branch (fully reverted). Findings, so it doesn't get re-run:
collectEntryLoadersDOES unify cleanly — it was the one genuinely byte-identical walk (skip-already-seen, dedup, parallel/layout recursion, orphan-inherited loaders); only "how one entry's loaders resolve" differed, a clean one-line strategy. Implemented + green (matrix 61/61 dev+prod, cache e2e 135/135), then dropped because it's a modest win not worth the file churn on its own.resolveLoadersbreaks the premise — fresh has aloadingDisabledawait-all branch (+ its own metrics) that the revalidation twin doesn't. Unifying = either change partial-render behavior or fork inside the "shared" fn anyway.resolveSegment/resolveOrphanLayoutis the wall — five genuinely-divergent behavioral hooks (per-segment client-cache gate,component:nullreuse signal,_handlerRanbookkeeping, parallel-slot policy, matchedIds threading) and two different return shapes (ResolvedSegment[]vs{segments, matchedIds}). A strategy object there is more abstraction than the duplication it removes, and would itself become the thing that drifts.One durable artifact landed: #579 pins the revalidation skipped-loader-stays-matched behavior the suite never covered.
Recommendation: keep this sub-item deferred / consider WON'T-FIX. The remaining 'duplication' is two genuinely different resolution engines (full render vs partial-delta), not a dedup opportunity. The other 5 #568 items (trie suffix + 4 internal de-dups in #575/#576, orphan→shared helper in #578) are done.
refactor(router): orphan revalidation calls the shared parallel helpe…
ivogt commentedon Jun 14, 2026
Closing — all 7 items resolved
Both remaining sub-items have landed on
main, so this issue is done.Final ledger
executeMiddleware/executeInterceptMiddleware)yieldFromStorevs cache-hit fresh-loader resolution deduploader-cacheO(N)ctx.userebind → stable interceptor + Mapfresh.tsvsrevalidation.tsresolution treesSub-item 6 (#578) — how the divergences were carried, not normalized
resolveOrphanLayoutWithRevalidationno longer inlines ~166 lines of parallel-slot resolution; it calls the shared helper with the two divergences expressed as explicit policy args:parentChainDefault: "force-render"— the orphan's unconditionaldefaultOverride:{value:true}(since refactor(router): split 4 god files into focused submodules #228, the fix(router): invoke parallel slot revalidate() fns when slot missing from clientSegmentIds #482 blank-slot guard). Intentional, carried as policy, never normalized.loaderOrder: "before"— the cosmetic loader-resolve-before-push order. Provably unobservable, but kept explicit rather than silently flipped.Output is byte-identical on both paths (4-way adversarial refutation, all
refuted:false). The coverage-first blocker was cleared first: a pinning test for the divergent (A) case (belongsToRoute=false, slot ∉clientSegmentIds, parent ∈ it, zero revalidate fns) that goes red if (A) is normalized totype-derived.Sub-item 7 (#578's sibling, #579) — WON'T-FIX, with rationale
The strategy-parameterized traversal unification was attempted on a throwaway branch (fully reverted). The "byte-identical twins" premise is false:
collectEntryLoadersdoes unify cleanly — but it's a modest win not worth the file churn alone.resolveLoadersbreaks the premise (fresh has aloadingDisabledawait-all branch the reval twin lacks).resolveSegment/resolveOrphanLayoutis the wall: five genuinely-divergent behavioral hooks (per-segment client-cache gate,component:nullreuse signal,_handlerRanbookkeeping, parallel-slot policy, matchedIds threading) and two different return shapes (ResolvedSegment[]vs{segments, matchedIds}). A strategy object there is more abstraction than the duplication it removes — it would itself become the thing that drifts.These are two genuinely different resolution engines (full render vs partial-delta), not a dedup opportunity. The one durable artifact from the experiment landed as #579, which pins the revalidation skipped-loader-stays-matched behavior (skipped loader dropped from emitted segments but its id kept in
matchedIds) that the suite never covered.All matrix-critical work shipped behavior-preserving: semantic matrix 61/61 dev+prod, no row changes.