[PR #843] [MERGED] chore(debug): classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION [6/6] #892

New issue

Closed

opened 2026-05-06 13:10:41 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 13:10:41 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/843
Author: @NathanDrake2406
Created: 4/14/2026
Status: ✅ Merged
Merged: 4/21/2026
Merged by: @james-elicx

Base: main ← Head: chore/pr-768-6-classification-debug-reasons

📝 Commits (2)

f33d77b chore(debug): add classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION
dcab0c5 test(debug): cover runtime-probe error path and Layer 2 reasons dispatch

📊 Changes

6 files changed (+539 additions, -3 deletions)

View changed files

📝 packages/vinext/src/entries/app-rsc-entry.ts (+22 -0)
📝 packages/vinext/src/index.ts (+20 -0)
📝 packages/vinext/src/server/app-page-execution.ts (+43 -0)
📝 tests/__snapshots__/entry-templates.test.ts.snap (+150 -0)
📝 tests/app-page-execution.test.ts (+174 -0)
📝 tests/build-time-classification-integration.test.ts (+130 -3)

📄 Description

Summary

PR 6 of 6, restack of #768. Previously stacked on #842; now rebased directly onto main after #842 merged.

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 #842 as tested-but-uncalled infrastructure. This PR wires it up end-to-end.

What changes

index.ts generateBundle hook reads VINEXT_DEBUG_CLASSIFICATION at build time; when set, it calls buildReasonsReplacement and patches the __VINEXT_CLASS_REASONS stub alongside __VINEXT_CLASS. Runs before the existing target.map = null / manifest-clear so both inputs are still available.
Stub-drift detection extended: when build-time debug is on, a missing __VINEXT_CLASS_REASONS stub throws loudly, matching the existing policy for __VINEXT_CLASS.
Generated RSC entry emits the __VINEXT_CLASS_REASONS null stub and an env-gated __classDebug logger. Each route's __buildTimeReasons field 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.ts accepts optional buildTimeReasons and debugClassification in LayoutClassificationOptions. The probe loop emits one debug line per layout across all three outcome paths: build-time classified, runtime probe success, runtime probe error. The no-classifier variant has a single documented producer here, the ?? fallback when a build-time classified layout has no attached reason payload.
Tests: new gated-on and gated-off cases in app-page-execution.test.ts; integration test asserts the __classDebug gating expression and the null stub when build-time debug is off.

Combinations

Both env checks are independent and all four combinations are safe:

build-time	runtime	behaviour
off	off	stub stays `return null`, `__classDebug` undefined, zero cost
on	off	stub patched but never called (`__classDebug` undefined)
off	on	stub stays `return null`, reasons map is null, fallback emits `{ layer: "no-classifier" }`
on	on	full reason payload per layout

Stack

[1/6] #838, merged, centralize request-derived page inputs
[2/6] #839, merged, emit per-layout flags
[3/6] #840, closed, filter skipped layouts. The server-side skip mechanism relied on rewriting the React Flight wire-format payload on egress, which proved too coupled to React-internal tag conventions for a framework that does not vendor React. The skip-mechanism design is being reworked around render-time omission rather than byte-level filtering.
[4/6] #841, closed, send X-Vinext-Router-Skip. Closed for the same reason as #840.
[5/6] #842, merged, wire build-time layout classification into the RSC entry
[6/6] this PR, classification reasons sidecar

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.ts
vp check clean

Risks / follow-ups

Regex-based stub patching is the wrong abstraction boundary. This PR extends the same generateBundle + string-replace pattern that @james-elicx flagged on #842 (https://github.com/cloudflare/vinext/pull/842#discussion_r3109733377). 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 #863, which scopes out a virtual-module-with-renderChunk replacement that would fold both __VINEXT_CLASS and __VINEXT_CLASS_REASONS into a single typed module contract. Landing that refactor before the next dispatch surface (render-time layout skip) avoids shipping a third parallel regex.
The env-var name VINEXT_DEBUG_CLASSIFICATION is a literal in three sites (plugin, emitted codegen template, runtime type docblock). A rename requires grepping all three. Acceptable for a debug flag.
The classifyAllRouteLayouts test-only divergence risk called out in #842 is unchanged by this PR.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/cloudflare/vinext/pull/843 **Author:** [@NathanDrake2406](https://github.com/NathanDrake2406) **Created:** 4/14/2026 **Status:** ✅ Merged **Merged:** 4/21/2026 **Merged by:** [@james-elicx](https://github.com/james-elicx) **Base:** `main` ← **Head:** `chore/pr-768-6-classification-debug-reasons` --- ### 📝 Commits (2) - [`f33d77b`](https://github.com/cloudflare/vinext/commit/f33d77b953c49f80e54365237b4d8bf5ab91ec95) chore(debug): add classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION - [`dcab0c5`](https://github.com/cloudflare/vinext/commit/dcab0c53e2467b018e731f1f0204b795f4ef0626) test(debug): cover runtime-probe error path and Layer 2 reasons dispatch ### 📊 Changes **6 files changed** (+539 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `packages/vinext/src/entries/app-rsc-entry.ts` (+22 -0) 📝 `packages/vinext/src/index.ts` (+20 -0) 📝 `packages/vinext/src/server/app-page-execution.ts` (+43 -0) 📝 `tests/__snapshots__/entry-templates.test.ts.snap` (+150 -0) 📝 `tests/app-page-execution.test.ts` (+174 -0) 📝 `tests/build-time-classification-integration.test.ts` (+130 -3) </details> ### 📄 Description ## Summary **PR 6 of 6, restack of #768.** Previously stacked on #842; now rebased directly onto `main` after #842 merged. 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 #842 as tested-but-uncalled infrastructure. This PR wires it up end-to-end. ## What changes - `index.ts` `generateBundle` hook reads `VINEXT_DEBUG_CLASSIFICATION` at build time; when set, it calls `buildReasonsReplacement` and patches the `__VINEXT_CLASS_REASONS` stub alongside `__VINEXT_CLASS`. Runs before the existing `target.map = null` / manifest-clear so both inputs are still available. - Stub-drift detection extended: when build-time debug is on, a missing `__VINEXT_CLASS_REASONS` stub throws loudly, matching the existing policy for `__VINEXT_CLASS`. - Generated RSC entry emits the `__VINEXT_CLASS_REASONS` null stub and an env-gated `__classDebug` logger. Each route's `__buildTimeReasons` field 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.ts` accepts optional `buildTimeReasons` and `debugClassification` in `LayoutClassificationOptions`. The probe loop emits one debug line per layout across all three outcome paths: build-time classified, runtime probe success, runtime probe error. The `no-classifier` variant has a single documented producer here, the `??` fallback when a build-time classified layout has no attached reason payload. - Tests: new gated-on and gated-off cases in `app-page-execution.test.ts`; integration test asserts the `__classDebug` gating expression and the null stub when build-time debug is off. ## Combinations Both env checks are independent and all four combinations are safe: | build-time | runtime | behaviour | |---|---|---| | off | off | stub stays `return null`, `__classDebug` undefined, zero cost | | on | off | stub patched but never called (`__classDebug` undefined) | | off | on | stub stays `return null`, reasons map is null, fallback emits `{ layer: "no-classifier" }` | | on | on | full reason payload per layout | ## Stack 1. [1/6] #838, merged, centralize request-derived page inputs 2. [2/6] #839, merged, emit per-layout flags 3. [3/6] #840, closed, filter skipped layouts. The server-side skip mechanism relied on rewriting the React Flight wire-format payload on egress, which proved too coupled to React-internal tag conventions for a framework that does not vendor React. The skip-mechanism design is being reworked around render-time omission rather than byte-level filtering. 4. [4/6] #841, closed, send X-Vinext-Router-Skip. Closed for the same reason as #840. 5. [5/6] #842, merged, wire build-time layout classification into the RSC entry 6. **[6/6] this PR**, classification reasons sidecar ## Validation Run locally via `vp test run <file>`: - [x] `tests/app-page-execution.test.ts` (17 cases, including 3 new debug-gated cases) - [x] `tests/build-time-classification-integration.test.ts` (7 cases, including 2 new reasons-sidecar cases) - [x] `tests/entry-templates.test.ts` (snapshot regenerated) - [x] `tests/app-router.test.ts` - [x] `vp check` clean ## Risks / follow-ups - **Regex-based stub patching is the wrong abstraction boundary.** This PR extends the same `generateBundle` + string-replace pattern that @james-elicx flagged on #842 (https://github.com/cloudflare/vinext/pull/842#discussion_r3109733377). 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 #863, which scopes out a virtual-module-with-`renderChunk` replacement that would fold both `__VINEXT_CLASS` and `__VINEXT_CLASS_REASONS` into a single typed module contract. Landing that refactor before the next dispatch surface (render-time layout skip) avoids shipping a third parallel regex. - The env-var name `VINEXT_DEBUG_CLASSIFICATION` is a literal in three sites (plugin, emitted codegen template, runtime type docblock). A rename requires grepping all three. Acceptable for a debug flag. - The `classifyAllRouteLayouts` test-only divergence risk called out in #842 is unchanged by this PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>