Skip to content

fix: address PR 750 review findings (Bonk + additional)#2

Closed
NathanDrake2406 wants to merge 1 commit into
feat/layout-persistence-pr-2cfrom
claude/pr-750-review-fixes-GO9ZD
Closed

fix: address PR 750 review findings (Bonk + additional)#2
NathanDrake2406 wants to merge 1 commit into
feat/layout-persistence-pr-2cfrom
claude/pr-750-review-fixes-GO9ZD

Conversation

@NathanDrake2406

Copy link
Copy Markdown
Owner

Addresses all Bonk review comments on PR 750 plus the 7 additional issues.

Changes

Bug fixes

shouldHardNavigate null semantics (behavioral fix)
The old condition currentPath !== null && nextPath !== null && currentPath !== nextPath silently soft-navigated when one side was null — e.g. going from a page with no root layout to one with a root layout. The component tree structure changes fundamentally in that case, so a hard navigate is required. Simplified to currentPath !== nextPath which covers all four cases correctly. 4 new test cases added.

Server action leaking AppWireElements
The non-ServerActionResult branch in registerServerActionCallback returned result (raw wire elements) to the caller. Fixed to return undefined.

Dead code removal

applyAppRouterStateUpdate
Exported but never called in production — only in tests. Removed. Tests refactored to call createPendingNavigationCommit + shouldHardNavigate directly (same coverage).

Correctness / Strict Mode

browserRouterStateRef render side-effect
browserRouterStateRef = stateRef ran every render, mutating module state during the render phase. Moved into the existing useLayoutEffect with a cleanup function (browserRouterStateRef = null on unmount). stateRef.current = treeState still runs every render so external callers always read current state. Cleanup prevents dangling refs during React Strict Mode's mount → unmount → remount cycle.

Clarity

>= 8 length check
segment.length > "[[...x]]".length - 1segment.length >= 8. Mathematically equivalent, but the original form evaluates at runtime and is harder to read.

ReleaseAppRenderDependency comment
Added a comment explaining why calling dependency.release() during render is safe: RSC server components render exactly once (no Strict Mode double-invoke), making render-time side effects equivalent to a mount effect.

Type safety

AppElements type strengthened
Record<string, AppElementValue>Record<string, AppElementValue> & Record<"__route", string> & Record<"__rootLayout", string | null>. The required keys are now enforced at compile time. as AppElements casts added at the two trust boundaries (normalizeAppElements, buildAppPageElements) where keys are set dynamically and validated at runtime by readAppElementsMetadata.

Test plan

  • tests/app-browser-entry.test.ts — 9 tests (refactored + 4 new shouldHardNavigate cases)
  • tests/app-elements.test.ts — 5 tests (unchanged, verifies type cast doesn't break runtime)
  • tests/app-render-dependency.test.ts — 2 tests (unchanged)
  • vp check passes on all 6 changed files (format + lint + types)

https://claude.ai/code/session_01CS3cFQj2UMrwzrFxnxyt7r

- shouldHardNavigate: fix null semantics — navigating between a route
  with no root layout (null) and one with a root layout (non-null) now
  triggers a hard navigate. The previous implementation required both
  paths to be non-null, silently soft-navigating when one side was null
  even though the component tree structure changes fundamentally.
  Simplify to `currentPath !== nextPath` which covers all cases.

- Remove dead code: applyAppRouterStateUpdate was exported but never
  called in production — only in tests. Replace test usages with direct
  calls to createPendingNavigationCommit + shouldHardNavigate.

- browserRouterStateRef: move module-level pointer assignment from render
  into useLayoutEffect (with cleanup) to avoid mutating module state
  during render. stateRef.current = treeState still runs every render so
  external callers always read the current state; the pointer itself is
  set once on mount and cleared on unmount, which fixes the interaction
  with React Strict Mode's mount → unmount → remount cycle.

- Length check clarity: restore segment.length >= 8 in place of the
  equivalent but confusing segment.length > "[[...x]]".length - 1.

- ReleaseAppRenderDependency: add a comment explaining why calling
  dependency.release() during render is safe in RSC (server components
  render exactly once — no Strict Mode double-invoke).

- Server action return value: fix non-ServerActionResult branch returning
  raw AppWireElements to the caller; return undefined instead.

- AppElements type: strengthen from Record<string, AppElementValue> to
  enforce the required __route (string) and __rootLayout (string | null)
  keys at compile time. Add as AppElements casts at the two trust
  boundaries (normalizeAppElements, buildAppPageElements) where the keys
  are set dynamically and validated at runtime by readAppElementsMetadata.

https://claude.ai/code/session_01CS3cFQj2UMrwzrFxnxyt7r
@NathanDrake2406
NathanDrake2406 deleted the claude/pr-750-review-fixes-GO9ZD branch April 22, 2026 13:14
NathanDrake2406 pushed a commit that referenced this pull request May 21, 2026
…re#1397)

* fix(basepath): enforce scoping on rewrites/redirects/routes

Sub-issues addressed under cloudflare#1333:

1. Rewrites/redirects/headers were firing on requests outside the
   configured `basePath`. Threaded a `BasePathMatchState` through the
   matchers and prod-server / deploy worker / dev plugin so default
   rules (no `basePath: false` opt-out) only evaluate when the request
   was under basePath, and opt-out rules only evaluate outside it.

5. Pages Router requests that landed outside basePath fell through to
   internal route matching. After redirects and beforeFiles rewrites
   run, the request now 404s when it was outside basePath and no
   `basePath: false` rule rewrote it — matching Next.js's
   `resolve-routes.ts:304-309`.

4. With rewrites no longer firing on out-of-basepath requests, the
   downstream code that prepends basePath to destinations no longer
   sees a stripped-but-already-internal pathname, eliminating the
   `/docs/docs/...` doubling for the rewrite-driven cases.

App Router uses the same gating; `basePath: false` rules don't yet
fire there because `normalizeRscRequest` 404s out-of-basepath requests
before the matchers see them — flagged as follow-up in the rule
gating site. External rewrite proxying (sub-issue #2) and static
asset doubling (sub-issue #3, partly addressed by cloudflare#1337/cloudflare#1383) are
deferred.

Refs cloudflare#1333

* test: update deploy.test signature assertions and format basepath tests

* review: address bonk nits — add inverse basePath:false tests and TODO comment
NathanDrake2406 pushed a commit that referenced this pull request Jun 9, 2026
… context (cloudflare#1868)

* fix(app-router): extend OTel tracer provider after instrumentation register() for Cache Component spans

During Cache Component fallback resume, `workUnitAsyncStorage` carries a
prerender/cache store. Without the tracer extension, calls to
`tracer.startActiveSpan()` / `tracer.startSpan()` inside user RSC code
inherited that frozen context, causing span IDs to be reused across requests
or not created at all.

Mirrors Next.js's `extendInstrumentationAfterRegistration()` in
`instrumentation-node-extensions.ts`: wraps the OTel tracer provider's
`startSpan` and `startActiveSpan` to exit `workUnitAsyncStorage` when
creating spans, then re-enters the store for the `startActiveSpan` callback.

Fixes cloudflare#1495

* fix(otel-tracer-extension): address bonk review findings

BLOCKING: replace bare require("@opentelemetry/api") with globalThis.require
guard so the call is safe in ESM Worker bundles where bare require is
undefined; matches the pattern in client-trace-metadata.ts.

SHOULD-FIX #1: restore the isUseCacheFunction warning when a "use cache"
function is passed to startActiveSpan — mirrors upstream
instrumentation-node-extensions.ts. Adds USE_CACHE_FUNCTION_SYMBOL tag in
registerCachedFunction and the inline isUseCacheFn check in the extension
(avoids importing the full cache-runtime dep graph from this lightweight file).

SHOULD-FIX #2: add focused unit tests in tests/otel-tracer-extension.test.ts
covering no-op paths, startSpan ALS exit, startActiveSpan ALS re-entry,
use-cache warning, provider/tracer double-wrap guards, and provider-swap
re-wrapping. Isolated from the full vinext plugin import graph so they run
without optional build-time packages.

SHOULD-FIX #3: replace module-global boolean tracerProviderExtended with a
WeakSet<object> (extendedProviders) so that a swapped provider gets wrapped
rather than being silently skipped forever.

NIT: correct the upstream filename in the instrumentation-runtime.ts comment
from instrumentation-globals.external.ts to instrumentation-node-extensions.ts
(where afterRegistration() is actually defined).

* refactor(cache-runtime): drop dead isUseCacheFunction export

Nothing imported `isUseCacheFunction` — `otel-tracer-extension.ts`
already keeps its own inline copy (`isUseCacheFn`) with a comment
explaining why (avoids pulling heavy cache-runtime deps into the
lightweight server helper).  Remove the unused export to eliminate the
dead code flagged in review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants