fix: pages router instrumentation error reporting for api routes

[james-elicx]

fix: instrumentation.ts Pages Router support + onRequestError for API routes

Extends instrumentation.ts support to Pages Router and fixes a gap where onRequestError was never called for API route errors.

What changed

onRequestError in API route handler

api-handler.ts caught errors from Pages Router API routes but never called reportRequestError, so onRequestError would silently not fire for any pages/api/* handler. Added the call in the catch block with routerKind: "Pages Router" and routeType: "render".

Pages Router fixture

tests/fixtures/pages-basic gets instrumentation.ts, instrumentation-state.ts, /api/instrumentation-test, and /api/error-route to support the new e2e tests. The state module uses globalThis keys rather than plain module-level variables — necessary because register() runs in an isolated ModuleRunner module graph (separate from the Vite SSR graph that handles API routes), so plain exports wouldn't be shared between them.

Pages Router e2e spec

tests/e2e/pages-router/instrumentation.spec.ts covers:

• Dev server starts without crashing when instrumentation.ts is present (regression guard for the outsideEmitter crash)
• register() was called before the first request
• onRequestError() fires when an API handler throws, with correct routerKind/routeType/routePath
• Multiple errors are captured independently
• Successful requests don't trigger onRequestError

cloudflare-dev added to CI matrix

The **cloudflare-dev` added to CI matrix**

The cloudflare-dev Playwright project was missing from the CI matrix despite existing in playwright.config.ts. Added it so the Vite dev + @cloudflare/vite-plugin startup tests run in CI.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@334

commit: 030f3c4

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[james-elicx]

/bigbonk review

[ask-bonk]

Review: test: pages router instrumentation

Good PR — these are well-structured regression tests for a real bug (startup crash from ssrLoadModule during configureServer in Vite 7). The code is clean and the test design is sound. A few observations:

What works well

1. Separate describe blocks for startup vs per-request tests. The startup tests correctly avoid resetting state (since register() fires once), while the onRequestError tests use beforeEach to reset. This is a thoughtful design that prevents test ordering issues.

2. globalThis bridge pattern. The instrumentation-state.ts module correctly uses globalThis to share state across Vite's separate module graphs (instrumentation runner vs SSR). The comment block explaining why this is necessary (lines 1-18 of instrumentation-state.ts) is excellent — this is a subtle Vite architecture detail that would otherwise confuse future contributors.

3. Regression is self-documenting. If the startup crash regresses, the Playwright webServer will fail to connect and the test never runs — which means the test suite itself is the regression detector, not just the assertions.

Suggestions

1. Missing onRequestError trigger test. The onRequestError describe block only tests the negative case (successful requests don't trigger it). There's no test that actually triggers an error and verifies onRequestError captures it. This would be the most valuable assertion for the onRequestError path — without it, you're only testing that the handler doesn't fire, not that it does. Consider adding a fixture route that throws and asserting errors.length > 0 with correct routerKind/routePath/routeType values.

2. Hardcoded setTimeout for async settling (instrumentation.spec.ts:74). The 200ms sleep works but is fragile — it could flake on slow CI runners or if the async path changes. Since the test immediately follows with a synchronous API call that reads the state, and the state is set synchronously within the same process via globalThis, the sleep may not even be necessary. If it is needed, consider polling the state endpoint with a short retry loop instead.

3. _errors() lazy init could mask bugs (instrumentation-state.ts:32-37). The _errors() helper silently creates the array if it doesn't exist. This means getCapturedErrors() never returns undefined — even if resetInstrumentationState() was never called. This is fine for test ergonomics, but worth noting that it hides the distinction between "state was initialized and empty" vs "state was never initialized."

4. Type assertion density in instrumentation-state.ts. The (globalThis as any) casts appear 6 times. A small helper or typed global declaration would clean this up:

   const g = globalThis as Record<string, unknown>;

   Minor style nit, not blocking.

Verdict

The tests cover the critical regression (startup crash) well. The main gap is the missing positive test for onRequestError — right now you're testing that instrumentation loads and that successful requests don't trigger errors, but not that errors are actually captured. Adding that would make this a complete instrumentation test suite.

Looks good otherwise. Clean code, good comments, correct use of the fixture patterns.

[ask-bonk]

Review posted on PR #334. Here's a summary:

Overall: Good PR. The regression tests are well-structured and cover the critical startup crash from ssrLoadModule during configureServer in Vite 7.

Key feedback:

1. Missing positive onRequestError test — The onRequestError describe block only tests the negative case (successful requests don't trigger it). There's no test that triggers an actual error and asserts it gets captured. This is the most significant gap.

2. Hardcoded setTimeout at line 74 — the 200ms sleep for async settling is fragile. Since state is set synchronously via globalThis in the same process, it may not be needed, or could be replaced with a polling loop.

3. Minor style nits: repeated (globalThis as any) casts could be cleaned up, and the lazy _errors() init silently hides uninitialized vs empty state.

The startup crash regression is well-covered — if the bug regresses, Playwright's webServer fails to connect and the tests never run, which is a clean detection mechanism.

github run

[james-elicx]

/bigbonk re-review please

[ask-bonk]

Re-review: fix: pages router instrumentation error reporting for api routes

All CI checks pass. The main gap from the first review (missing positive onRequestError test) was addressed in the second commit. The test suite now covers both the positive and negative cases.

Previous feedback — status

1. Missing positive onRequestError test — Fixed. The second commit (7b9d7d7) added three tests: error capture with field assertions, route path pattern check, and multiple independent error accumulation. This was the most important gap and it's well-addressed.

2. Hardcoded setTimeout — Still present (200ms at lines 74, 92, 114; 400ms at line 130). Acceptable for now since reportRequestError is fire-and-forget (.catch(() => {})) so there's no way to synchronously wait on it. The timeouts are generous enough for CI. If these flake in practice, a polling loop would be the fix, but not blocking.

3. (globalThis as any) repetition — Still present. Minor style nit, not blocking.

4. _errors() lazy init — Still present. Fine for test code.

New observations

1. routeType: "render" for API routes (api-handler.ts:220) — Next.js uses "route" for API/route handlers (and the App Router codepath in app-dev-server.ts:1918 correctly uses "route"). Using "render" here is semantically wrong for an API endpoint. The test at line 105 asserts routeType is "render", so both the implementation and the test are consistently wrong. This should be "route" to match Next.js behavior.

   The OnRequestErrorContext type already defines "route" as a valid value, so this is a one-line fix in api-handler.ts and a one-line fix in the test.

2. Stray tab at instrumentation.spec.ts:106 — Line 106 (\t});) uses a tab while the rest of the file uses spaces. The linter doesn't catch it, but it's visually inconsistent. Cosmetic.

3. PR description mentions cloudflare-dev added to CI matrix — This was actually done in PR #333, not this PR. The description should be corrected to avoid confusion in the commit history.

4. examples/app-router-cloudflare/pages/index.tsx — This file is unrelated to instrumentation. The commit message ("add pages route to app dir") suggests it's verifying that App Router works when a pages/ directory coexists. Fine to include, but worth mentioning in the PR description since it's a separate concern.

Verdict

The core change (adding reportRequestError to api-handler.ts) is correct and well-tested. The routeType: "render" vs "route" issue is the only functional concern — it's a behavioral mismatch with Next.js. Everything else is minor. CI is fully green.

[ask-bonk]

Review posted on PR #334. Here's the summary:

Approved — the main gap from the first review (missing positive onRequestError test) was fixed in the second commit. CI is fully green.

One functional concern: routeType: "render" is used for API routes in api-handler.ts:220, but Next.js uses "route" for API/route handlers. The App Router codepath (app-dev-server.ts:1918) already uses "route" correctly. This is a one-line fix in the handler and one-line fix in the test assertion.

Minor nits: stray tab at instrumentation.spec.ts:106, PR description incorrectly attributes the cloudflare-dev CI addition to this PR (it was PR #333), and the pages/index.tsx example file is an unrelated change that should be noted in the description.

github run