fix: address PR 750 review findings (Bonk + additional)#2
Closed
NathanDrake2406 wants to merge 1 commit into
Closed
fix: address PR 750 review findings (Bonk + additional)#2NathanDrake2406 wants to merge 1 commit into
NathanDrake2406 wants to merge 1 commit into
Conversation
- 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
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses all Bonk review comments on PR 750 plus the 7 additional issues.
Changes
Bug fixes
shouldHardNavigatenull semantics (behavioral fix)The old condition
currentPath !== null && nextPath !== null && currentPath !== nextPathsilently 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 tocurrentPath !== nextPathwhich covers all four cases correctly. 4 new test cases added.Server action leaking
AppWireElementsThe non-
ServerActionResultbranch inregisterServerActionCallbackreturnedresult(raw wire elements) to the caller. Fixed toreturn undefined.Dead code removal
applyAppRouterStateUpdateExported but never called in production — only in tests. Removed. Tests refactored to call
createPendingNavigationCommit+shouldHardNavigatedirectly (same coverage).Correctness / Strict Mode
browserRouterStateRefrender side-effectbrowserRouterStateRef = stateRefran every render, mutating module state during the render phase. Moved into the existinguseLayoutEffectwith a cleanup function (browserRouterStateRef = nullon unmount).stateRef.current = treeStatestill runs every render so external callers always read current state. Cleanup prevents dangling refs during React Strict Mode's mount → unmount → remount cycle.Clarity
>= 8length checksegment.length > "[[...x]]".length - 1→segment.length >= 8. Mathematically equivalent, but the original form evaluates at runtime and is harder to read.ReleaseAppRenderDependencycommentAdded 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
AppElementstype strengthenedRecord<string, AppElementValue>→Record<string, AppElementValue> & Record<"__route", string> & Record<"__rootLayout", string | null>. The required keys are now enforced at compile time.as AppElementscasts added at the two trust boundaries (normalizeAppElements,buildAppPageElements) where keys are set dynamically and validated at runtime byreadAppElementsMetadata.Test plan
tests/app-browser-entry.test.ts— 9 tests (refactored + 4 newshouldHardNavigatecases)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 checkpasses on all 6 changed files (format + lint + types)https://claude.ai/code/session_01CS3cFQj2UMrwzrFxnxyt7r