chore(debug): classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION [6/6]#843
Conversation
NathanDrake2406
force-pushed
the
chore/pr-768-6-classification-debug-reasons
branch
from
5355568 to
54296aa
Compare
commit: |
NathanDrake2406
force-pushed
the
chore/pr-768-6-classification-debug-reasons
branch
2 times, most recently
from
f2fa472 to
8fbefec
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in (via VINEXT_DEBUG_CLASSIFICATION) “classification reasons” sidecar to the generated RSC entry so operators can see why each layout was classified static/dynamic, while keeping the default hot path allocation-free.
Changes:
- Patch a new
__VINEXT_CLASS_REASONSstub ingenerateBundlewhenVINEXT_DEBUG_CLASSIFICATIONis set, emitting a per-route reasons dispatch table. - Extend the generated RSC entry to gate
__buildTimeReasonsbehind__classDebug, and passbuildTimeReasons+debugClassificationinto the runtime probe. - Emit one debug reason per layout in
probeAppPageLayouts, with new tests and updated entry template snapshots.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vinext/src/index.ts | Conditionally patches __VINEXT_CLASS_REASONS dispatch in generateBundle behind the debug env var. |
| packages/vinext/src/entries/app-rsc-entry.ts | Emits __classDebug, __VINEXT_CLASS_REASONS stub, and threads reasons/debug hook through the route table into the probe. |
| packages/vinext/src/server/app-page-execution.ts | Wires buildTimeReasons + debugClassification and emits structured per-layout debug reasons (build-time + runtime-probe). |
| tests/app-page-execution.test.ts | Adds coverage for debug-gated behavior and runtime-probe reason emission. |
| tests/build-time-classification-integration.test.ts | Updates route-index extraction regex and asserts reasons sidecar is gated + stub stays null when debug is off. |
| tests/snapshots/entry-templates.test.ts.snap | Snapshot updates for the new debug logger, reasons stub, and route table wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Per-layout classification reasons keyed by layout index. Populated by the | ||
| * generator only when `VINEXT_DEBUG_CLASSIFICATION` is set at runtime; the | ||
| * hot path never reads this and the wire payload is unchanged. |
There was a problem hiding this comment.
The docblock for buildTimeReasons says it is populated only when VINEXT_DEBUG_CLASSIFICATION is set at runtime, but the reasons dispatch table is actually only patched into the bundle when the env var is present at build time (generateBundle). Consider updating this comment to reflect the build-time + runtime gating so operators don’t set the flag only at runtime and then get no-classifier reasons unexpectedly.
| * Per-layout classification reasons keyed by layout index. Populated by the | |
| * generator only when `VINEXT_DEBUG_CLASSIFICATION` is set at runtime; the | |
| * hot path never reads this and the wire payload is unchanged. | |
| * Per-layout classification reasons keyed by layout index. Available only | |
| * when `VINEXT_DEBUG_CLASSIFICATION` was enabled at build time so the | |
| * reasons dispatch table was patched into the bundle, and populated by the | |
| * generator only when the same flag is also set at runtime. The hot path | |
| * never reads this and the wire payload is unchanged. |
There was a problem hiding this comment.
Addressed in f33d77b. The docblock now covers both lifecycle points — build-time for the stub patch, runtime for the route to read it — and calls out that runtime-only activation falls through to no-classifier.
| it("leaves __VINEXT_CLASS_REASONS as a null stub when build-time debug is off", () => { | ||
| expect(built.chunkSource).toMatch( | ||
| /function\s+__VINEXT_CLASS_REASONS\s*\(routeIdx\)\s*\{\s*return null;?\s*\}/, | ||
| ); | ||
| expect(built.chunkSource).not.toMatch( | ||
| /function\s+__VINEXT_CLASS_REASONS\s*\(routeIdx\)\s*\{[^}]*switch/, | ||
| ); |
There was a problem hiding this comment.
These new assertions assume VINEXT_DEBUG_CLASSIFICATION is unset during the fixture build. If the env var is set in the ambient test runner/CI, the plugin will patch __VINEXT_CLASS_REASONS and this test will fail. Consider making the test hermetic by explicitly saving/clearing process.env.VINEXT_DEBUG_CLASSIFICATION around the buildMinimalFixture() call (and restoring it) so the expected “debug off” state is guaranteed.
There was a problem hiding this comment.
Addressed in f33d77b. buildMinimalFixture now saves, overrides, and restores process.env.VINEXT_DEBUG_CLASSIFICATION around builder.buildApp() so the "debug off" assertions hold even on a developer machine that has the flag set in the ambient shell, and the new { debug: true } suite below exercises the patched path without polluting the sibling suite.
| if (enableClassificationDebug) { | ||
| const reasonsReplacement = buildReasonsReplacement( | ||
| rscClassificationManifest, | ||
| layer2PerRoute, | ||
| ); | ||
| const patchedReasonsBody = `function __VINEXT_CLASS_REASONS(routeIdx) { return (${reasonsReplacement})(routeIdx); }`; | ||
| target.code = target.code.replace(reasonsStubRe, patchedReasonsBody); | ||
| } |
There was a problem hiding this comment.
The new enableClassificationDebug branch patches __VINEXT_CLASS_REASONS, but there doesn’t appear to be a test asserting the “debug on” build output (i.e., that __VINEXT_CLASS_REASONS is replaced with a populated switch/dispatch and returns a per-route Map of reasons). Adding an integration test that sets VINEXT_DEBUG_CLASSIFICATION for the fixture build and validates the emitted dispatcher would help prevent regressions in this path.
There was a problem hiding this comment.
Addressed in f33d77b, and further strengthened in dcab0c5. The build-time classification integration (debug on) describe block now runs buildMinimalFixture({ debug: true }), evaluates the patched __VINEXT_CLASS_REASONS via extractReasonsDispatch, and asserts per-route reason entries. dcab0c5 extends that to cover both Layer 1 (segment-config) and Layer 2 (module-graph with result: "static") in the same /force-dyn dispatch entry.
…CLASSIFICATION Operators tracing why a layout was flagged static or dynamic can opt in with VINEXT_DEBUG_CLASSIFICATION=1 at build time. When active, the plugin patches a second stub (__VINEXT_CLASS_REASONS) with a dispatch table that returns per-layout ClassificationReason structures. The runtime probe loop emits a debug line per layout when debugClassification is wired, and the hot path remains allocation-free when debug is off because the stub returns null. The reasons machinery (types, report.ts tagged results, and buildReasonsReplacement) already shipped in the previous PR; this PR just enables the emission path and wires the debug consumer into the runtime probe.
NathanDrake2406
force-pushed
the
chore/pr-768-6-classification-debug-reasons
branch
from
8fbefec to
f33d77b
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
Clean PR that wires up the reasons sidecar end-to-end. The four-combination gating table is well thought out, and the buildTimeReasons field being lazily evaluated behind __classDebug ? keeps the hot path genuinely zero-cost. The test coverage is solid for the unit-level probe behavior and the integration-level codegen shape.
A few comments below, none blocking.
| layer: "runtime-probe", | ||
| outcome: "dynamic", | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); |
There was a problem hiding this comment.
The error-path debug emission isn't covered by a test. The success case (runtime-probe with outcome: "dynamic" / "static") and the build-time case are both tested, but there's no test where runWithIsolatedDynamicScope throws and debugClassification is set — so this branch with the error field is untested.
Worth adding a quick case that mirrors the runtime-probe test but has runWithIsolatedDynamicScope reject, then asserts the debug callback receives { layer: "runtime-probe", outcome: "dynamic", error: "..." }.
There was a problem hiding this comment.
Fixed in dcab0c5. Added emits runtime-probe reason with the error message when the probe throws which throws from inside runWithIsolatedDynamicScope, sets debugClassification, and pins the { layer: "runtime-probe", outcome: "dynamic", error: "headers() outside render" } payload alongside the static neighbour to cover both branches in one pass.
| // producer of the variant; see `layout-classification-types.ts`. | ||
| cls.debugClassification( | ||
| cls.getLayoutId(layoutIndex), | ||
| cls.buildTimeReasons?.get(layoutIndex) ?? { layer: "no-classifier" }, |
There was a problem hiding this comment.
Nit: cls.getLayoutId(layoutIndex) is called twice on the debug path (once for the flag assignment on line 151, once here). It's cheap today since it's just string concatenation, but if this ever becomes non-trivial you'd want to hoist. Not worth changing now, just noting the pattern.
There was a problem hiding this comment.
Acknowledged. The second cls.getLayoutId(layoutIndex) is on the debug-gated branch only, so it is free when debug is off. If the helper ever becomes non-trivial, the #863 refactor is the right place to hoist it along with the rest of the dispatch plumbing.
| // tracing why a layout was flagged static or dynamic. The reason payload is | ||
| // carried by __VINEXT_CLASS_REASONS and consumed inside probeAppPageLayouts. | ||
| const __classDebug = process.env.VINEXT_DEBUG_CLASSIFICATION | ||
| ? function(layoutId, reason) { |
There was a problem hiding this comment.
The __classDebug function parameter types are untyped (layoutId, reason). This is consistent with the existing __isrDebug pattern above, so it's fine for generated code, but noting it since the typed counterpart in LayoutClassificationOptions.debugClassification has full types. If you ever extract this into a helper module per the #863 refactor, the types come for free.
There was a problem hiding this comment.
Acknowledged. Keeping it shaped after __isrDebug so the codegen stays uniform. #863 folds both dispatch tables and both debug hooks into a typed module; the types arrive there.
| `vinext: build-time classification — expected __VINEXT_CLASS stub in exactly one RSC chunk, found ${chunksWithStubBody.length}`, | ||
| ); | ||
| } | ||
| if (enableClassificationDebug && !reasonsStubRe.test(chunksWithStubBody[0]!.chunk.code)) { |
There was a problem hiding this comment.
The drift detection for __VINEXT_CLASS_REASONS only fires when enableClassificationDebug is true. This means in a normal (non-debug) build, if the generator stops emitting the reasons stub, nobody notices until someone turns on debug and hits a confusing failure.
Not a bug — the stub is dead code when debug is off, so there's no behavioral impact. But if the #863 virtual-module refactor collapses both stubs, this asymmetry disappears naturally. Just flagging it as a design note.
There was a problem hiding this comment.
Agreed, recording the asymmetry rather than fixing it in this PR. The reasons stub is dead code when debug is off, so the silent-miss window is confined to the next operator that enables debug on a build that stopped emitting the stub. #863 collapses both stubs into one typed module and the drift check becomes symmetric by construction.
| ); | ||
| }); | ||
|
|
||
| it("emits a segment-config reason for the force-dyn layout", () => { |
There was a problem hiding this comment.
Good that the debug-on suite does a full buildMinimalFixture({ debug: true }). Copilot flagged the absence of a debug-on integration test in index.ts, but this covers it.
One thing that would strengthen coverage: assert that the reasons Map for the root layout (index 0) on the /force-dyn route has a module-graph reason with result: "static", since the root layout is pure JSX and Layer 2 should classify it. That would exercise both Layer 1 and Layer 2 reasons in a single route's dispatch entry.
There was a problem hiding this comment.
Fixed in dcab0c5. The debug-on integration test now reads the reasons Map for /force-dyn and asserts layer: "module-graph" + result: "static" on root layout index 0 alongside the existing segment-config assertion on nested layout index 1, so regressing either classifier layer trips the same test. Kept ReasonShape structural so this still survives the #863 codegen reshape.
| function extractRouteIndexByPattern(chunkSource: string): Map<string, number> { | ||
| const result = new Map<string, number>(); | ||
| const re = /__buildTimeClassifications:\s*__VINEXT_CLASS\((\d+)\)[^,]*,\s*pattern:\s*"([^"]+)"/g; | ||
| const re = /__buildTimeClassifications:\s*__VINEXT_CLASS\((\d+)\)[\s\S]*?pattern:\s*"([^"]+)"/g; |
There was a problem hiding this comment.
Good fix. The old regex [^,]* broke once __buildTimeReasons was inserted between __buildTimeClassifications and pattern, since commas now appear in that span. The [\s\S]*? non-greedy match is correct here.
There was a problem hiding this comment.
Acknowledged. Left as-is.
|
Review posted. Here's a summary of the feedback: Overall: Clean PR. The four-combination gating matrix (build-time on/off x runtime on/off) is well-designed, and the lazy Key comments:
|
The runtime-probe catch branch in `probeAppPageLayouts` emits
`{ layer: "runtime-probe", outcome: "dynamic", error: "..." }` when the
isolated dynamic scope throws. That emission had no coverage, so a
regression that stopped including the `error` field, or that skipped the
debug callback entirely on the error path, would not have tripped any
test.
The reasons sidecar integration test also only asserted the Layer 1
(`segment-config`) reason on the nested `force-dynamic` layout. Layer 2
(`module-graph`) on the pure-JSX root layout was exercised in the build
but never read, so a regression that stopped serializing the `result`
field or the `module-graph` discriminator for Layer 2 would slip through.
Extends the existing debug-on describe block to read reasons for layout
index 0 on `/force-dyn` and assert the `module-graph` + `result: "static"`
pair, keeping the structural `ReasonShape` so the test still survives
the cloudflare#863 codegen reshape.
Also removes a pre-existing `as` on the narrowed Map return in favour of
a proper `in`-narrowing type predicate.
Summary
PR 6 of 6, restack of #768. Previously stacked on #842; now rebased directly onto
mainafter #842 merged.Operators tracing why a layout was flagged static or dynamic can opt in with
VINEXT_DEBUG_CLASSIFICATION=1at build time. When active, the plugin patches a second stub (__VINEXT_CLASS_REASONS) with a dispatch table that returns per-layoutClassificationReasonstructures. The runtime probe loop emits a debug line per layout whendebugClassificationis wired, and the hot path remains allocation-free when debug is off because the stub returnsnull.The reasons machinery (types,
report.tstagged results, andbuildReasonsReplacement) already shipped in #842 as tested-but-uncalled infrastructure. This PR wires it up end-to-end.What changes
index.tsgenerateBundlehook readsVINEXT_DEBUG_CLASSIFICATIONat build time; when set, it callsbuildReasonsReplacementand patches the__VINEXT_CLASS_REASONSstub alongside__VINEXT_CLASS. Runs before the existingtarget.map = null/ manifest-clear so both inputs are still available.__VINEXT_CLASS_REASONSstub throws loudly, matching the existing policy for__VINEXT_CLASS.__VINEXT_CLASS_REASONSnull stub and an env-gated__classDebuglogger. Each route's__buildTimeReasonsfield is gated__classDebug ? __VINEXT_CLASS_REASONS(routeIdx) : null, so at module load the reasons Map is only constructed when runtime debug is also on.app-page-execution.tsaccepts optionalbuildTimeReasonsanddebugClassificationinLayoutClassificationOptions. The probe loop emits one debug line per layout across all three outcome paths: build-time classified, runtime probe success, runtime probe error. Theno-classifiervariant has a single documented producer here, the??fallback when a build-time classified layout has no attached reason payload.app-page-execution.test.ts; integration test asserts the__classDebuggating expression and the null stub when build-time debug is off.Combinations
Both env checks are independent and all four combinations are safe:
return null,__classDebugundefined, zero cost__classDebugundefined)return null, reasons map is null, fallback emits{ layer: "no-classifier" }Stack
Validation
Run locally via
vp test run <file>:tests/app-page-execution.test.ts(17 cases, including 3 new debug-gated cases)tests/build-time-classification-integration.test.ts(7 cases, including 2 new reasons-sidecar cases)tests/entry-templates.test.ts(snapshot regenerated)tests/app-router.test.tsvp checkcleanRisks / follow-ups
generateBundle+ string-replace pattern that @james-elicx flagged on feat(build): wire build-time layout classification into RSC entry #842 (feat(build): wire build-time layout classification into RSC entry #842 (comment)). Each new dispatch table (here,__VINEXT_CLASS_REASONS) multiplies the regex surface and the drift detection surface. The feature itself is fine; the way it crosses the build boundary needs an upgrade. Tracked in Replace regex-based__VINEXT_CLASSstub patching ingenerateBundle#863, which scopes out a virtual-module-with-renderChunkreplacement that would fold both__VINEXT_CLASSand__VINEXT_CLASS_REASONSinto a single typed module contract. Landing that refactor before the next dispatch surface (render-time layout skip) avoids shipping a third parallel regex.VINEXT_DEBUG_CLASSIFICATIONis a literal in three sites (plugin, emitted codegen template, runtime type docblock). A rename requires grepping all three. Acceptable for a debug flag.classifyAllRouteLayoutstest-only divergence risk called out in feat(build): wire build-time layout classification into RSC entry #842 is unchanged by this PR.