fix(router): support experimental gesture push#1909
Conversation
Vinext did not expose useRouter().experimental_gesturePush when Next.js apps enabled experimental.gestureTransition. Upstream gesture-transition fixtures call the method inside a held async transition, so the target route never became visible before the canonical router.push completed. The fix resolves the config flag into the same __NEXT_GESTURE_TRANSITION define used by Next.js, exposes the optional router method only under that flag, and lets gesture navigation use the existing RSC navigation pipeline with an immediate approved visible commit. The ported e2e fixture preserves the upstream held-transition flow and target/static/dynamic assertions.
commit: |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: experimental gesture push
Clean, well-scoped change. The config plumbing, define injection, gated public method, and synchronous-commit lane are all consistent with the existing patterns, and the test/fixture changes track the new visibleCommitMode parameter correctly. I traced the key paths:
experimental_gesturePushis attached toappRouterInstance(_appRouter), which is whatAppRouterContext.Provideris fed (app-browser-entry.ts:1098, app-ssr-entry.ts:391) and whatinstallWindowNextregisters, so it flows to bothuseRouter()andwindow.next.router.- The gesture path uses
programmaticTransition = false, sopendingRouterStateis null anddispatchApprovedVisibleCommitreaches the newsynchronousbranch (controller line 529) rather than the earlypendingRouterStatereturn. Internally consistent. flushSyncfromreact-domis already imported inapp-browser-entry.ts, so the new controller import is safe in the browser bundle.JSON.stringify(nextConfig.gestureTransition)emits a raw boolean literal, soif (process.env.__NEXT_GESTURE_TRANSITION)tree-shakes away when disabled. Good.- The new
NextConfig.experimentaltype keeps a[key: string]: unknownindex signature, so existing configs that set otherexperimental.*keys still type-check (they were previously accepted via the top-level index signature).
A few non-blocking observations below.
| appHref = localPath; | ||
| } | ||
|
|
||
| void navigateClientSide(appHref, "push", options?.scroll !== false, false, "synchronous"); |
There was a problem hiding this comment.
Unlike push/replace, the gesture path does not call trackScheduledAppRouterNavigation() / releaseScheduledAppRouterNavigationAfterCurrentTask(). That bookkeeping is what lets a router.refresh() issued in the same outer transition skip a redundant re-fetch (see hasScheduledAppRouterNavigation() at line 1973). In the ported fixture this is masked because the canonical router.push(href) runs after the await and does track, but a userland gesture followed by an immediate refresh() would diverge from push. Worth a short comment explaining why gesture intentionally skips the scheduled-navigation guard, or adding the tracking for parity.
| return; | ||
| } | ||
|
|
||
| if (visibleCommitMode === "synchronous") { |
There was a problem hiding this comment.
The synchronous branch is only reachable when pendingRouterState is null (the early return at line 521 wins otherwise). Today the only synchronous caller is gesture push with programmaticTransition = false, so that invariant holds, but it is implicit. If a future caller ever combined visibleCommitMode: "synchronous" with a programmatic transition, the synchronous commit would be silently dropped in favor of the deferred resolve. A one-line comment noting that synchronous mode assumes a null pendingRouterState would make the coupling explicit.
| }, | ||
| }; | ||
|
|
||
| if (process.env.__NEXT_GESTURE_TRANSITION) { |
There was a problem hiding this comment.
No unit test asserts that experimental_gesturePush is attached when the define is set (the new next-config.test.ts cases only cover config resolution; the e2e spec covers the runtime). Since process.env.__NEXT_GESTURE_TRANSITION is undefined in vitest, appRouterInstance.experimental_gesturePush is always undefined there. Consider a focused unit test that stubs the env define and re-imports the shim to assert the method is (and is not) attached, mirroring how the gating works. Not blocking given the e2e coverage.
|
Review posted successfully. Here's a summary of my findings: Review SummaryThe PR is a clean, well-scoped implementation that correctly adds the experimental gesture push API gated by Verified correct:
Three non-blocking observations posted as inline comments:
No correctness or security issues were found. |
- track scheduled app router navigation in experimental_gesturePush for parity with push/replace, so a refresh() issued in the same task skips its redundant re-fetch - document the synchronous visible-commit mode's null pendingRouterState invariant in the browser navigation controller - add a unit test asserting experimental_gesturePush is only attached when the __NEXT_GESTURE_TRANSITION define is set
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Verified the change end-to-end and it is correct, well-scoped, and faithful to the upstream behavior. I confirmed the items the previous /bigbonk review flagged are now resolved in this diff:
- The scheduled-navigation tracking the earlier review asked for is present (
trackScheduledAppRouterNavigation()/releaseScheduledAppRouterNavigationAfterCurrentTask) atshims/navigation.ts:2063-2065, so a gesture-then-refresh()in the same task now matchespush/replace. - The implicit
synchronousinvariant is now documented atapp-browser-navigation-controller.ts:529-533. - A focused unit test asserting attach/omit based on the define was added (
shims.test.ts:329).
Things I independently verified as correct:
experimental_gesturePushis mutated onto the_appRouter/appRouterInstancesingleton, which is the value fed toAppRouterContext.Provider(app-browser-entry.ts:1098,app-ssr-entry.ts:390) and toinstallWindowNext({ router: appRouterInstance })(app-browser-entry.ts:2264). So it correctly surfaces on bothuseRouter()andwindow.next.router. Because the top-levelifruns at module-load (before any render),useRouter'suseMemo([router, bfcacheId])never goes stale.- The
synchronousbranch is reachable: gesture navigates withprogrammaticTransition = false, sopendingRouterStateis null and the early return at controller:521 does not win.dispatchApprovedVisibleCommitruns after the RSC payloadawait, i.e. outside any React render phase, soflushSyncis safe. - The define injects a raw boolean (
JSON.stringify(nextConfig.gestureTransition)), soif (process.env.__NEXT_GESTURE_TRANSITION)tree-shakes out when disabled. - The new
NextConfig.experimentalobject type keeps its inner[key: string]: unknown, so configs setting otherexperimental.*keys still type-check. - The e2e spec and fixture are a faithful port of
test/e2e/app-dir/gesture-transitions(v16.2.6), with a reasonableexperimental_gesturePush === undefinedthrow added. - URL-safety /
isServerordering in the gesture method matchespush/replace, and the throwing calls (assertSafeNavigationUrl, normalization) run beforetrackScheduledAppRouterNavigation(), so no scheduled-navigation leak on a synchronous throw.
Two minor, non-blocking notes inline. No correctness or security issues found.
| if (isServer) return; | ||
|
|
||
| let appHref = href; | ||
| if (isAbsoluteOrProtocolRelativeUrl(href)) { |
There was a problem hiding this comment.
Minor: this same-origin normalization is redundant with navigateClientSide, which already runs the identical check (isExternalUrl is literally isAbsoluteOrProtocolRelativeUrl, navigation.ts:1562-1564) and normalizes absolute same-origin URLs at navigation.ts:1769-1788.
The one behavioral reason to keep it here is the external-URL case: doing it inline lets the gesture path no-op on external hrefs, whereas falling through to navigateClientSide would trigger a hard window.location.assign. That matches the PR's stated intent, so this is intentional — just worth a one-line comment noting it exists to force the external no-op rather than to normalize (which navigateClientSide would do anyway).
| // early return above never wins. A future caller combining `synchronous` | ||
| // with a programmatic transition would have its synchronous commit | ||
| // silently dropped in favor of the deferred resolve above. | ||
| if (visibleCommitMode === "synchronous") { |
There was a problem hiding this comment.
Worth noting there are now two synchronous-commit paths: the existing dispatchSynchronousVisibleCommit (plain setter(...), used by HMR and history traversal at lines 483/621/801) and this new flushSync-wrapped branch. The distinction is correct — the existing callers already run inside a synchronous event-handler/effect context where React batches and flushes, whereas gesture's commit fires after an await inside a held async transition and must be forced out with flushSync. Not a change request; just flagging the two paths so a future reader doesn't mistake them for duplicates and consolidate them incorrectly.
|
Review posted successfully. Here's a summary: Review SummaryThis PR adds the experimental Confirmed the prior review's concerns are now resolved in this diff:
Independently verified:
Two minor, non-blocking inline notes:
No correctness or security issues found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the full change. It is correct, well-scoped, and a faithful port of test/e2e/app-dir/gesture-transitions (v16.2.6). I independently re-verified the key invariants the two prior /bigbonk reviews flagged, and they all hold:
- Gating / tree-shaking:
defines["process.env.__NEXT_GESTURE_TRANSITION"] = JSON.stringify(nextConfig.gestureTransition)emits a raw boolean literal (true/false), so the module-topif (process.env.__NEXT_GESTURE_TRANSITION)guard innavigation.tstree-shakes out when disabled. Note this differs from the sibling defines that stringify? "true" : "false"(which would be truthy as"false"); the boolean form here is the correct one. - Reachability of the
synchronousbranch: gesture push callsnavigateClientSide(..., programmaticTransition = false, "synchronous"). Inapp-browser-entry.ts:1673shouldUsePendingRouterState = programmaticTransition, sopendingRouterStatestaysnulland the early return at controller:521 does not win — theflushSyncbranch at controller:534 is reached. The new comment documenting that implicit invariant is accurate. - Surface area:
experimental_gesturePushis mutated onto the shared_appRouter/appRouterInstancesingleton, which is fed toAppRouterContext.Provider(app-browser-entry.ts:1098,app-ssr-entry.ts:391) and toinstallWindowNext({ router: appRouterInstance })(passed by reference atwindow-next.ts:189), so it surfaces on bothuseRouter()andwindow.next.router. - Config resolution:
experimental?.gestureTransition === truecoerces theunknownvalue safely; default isfalse; theNextConfig.experimentalobject type retains its inner[key: string]: unknownso otherexperimental.*keys still type-check. - No collateral effect from enabling the flag on the shared
app-basicfixture:gestureTransitiononly controls the method attachment; nothing else branches on it.
One additional non-blocking note on test robustness below. No correctness or security issues found.
|
|
||
| await expect(page.getByTestId("static-content")).toHaveText("This is static content"); | ||
|
|
||
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); |
There was a problem hiding this comment.
Non-blocking (test robustness): this asserts the positive intermediate state (target page + static content visible, URL updated) during the gesture, but never asserts the negative — that the page is still in its intermediate/held state and dynamic-content has not yet resolved. target-page/page.tsx wraps DynamicContent (which await connection()) in <Suspense fallback={loading}> precisely so the dynamic hole is the distinguishing signal between the optimistic gesture commit and the final canonical push.
If connection() resolves quickly in the preview server, the gesture commit and the later router.push() would render identically, and this test could pass even if the synchronous-commit path silently degraded to a normal deferred transition. Consider asserting the held state during the gesture before clicking end-gesture, e.g.:
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); | |
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); | |
| // The gesture commit is the intermediate (held) state: the dynamic hole | |
| // should still be suspended until the canonical push resolves it. This is | |
| // what distinguishes the synchronous gesture commit from a normal push. | |
| await expect(page.getByTestId("loading")).toBeVisible(); | |
| await expect(page.getByTestId("dynamic-content")).toHaveCount(0); |
This matches the upstream fixture's intent (the Suspense boundary exists to observe the intermediate state) and guards against the synchronous lane regressing without detection.
|
Review posted successfully. SummaryThis PR adds the experimental Verified correct (re-confirming the two prior
One additional non-blocking observation I posted (not raised by prior reviews): the e2e spec asserts the positive intermediate state during the gesture but never asserts the negative — that |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the change end-to-end. It is correct, well-scoped, and a faithful port of test/e2e/app-dir/gesture-transitions (v16.2.6). The config plumbing, define injection (raw boolean literal → guard tree-shakes when disabled), gated public method, and the new synchronous/flushSync commit lane are all internally consistent. I independently confirmed:
experimental_gesturePushis attached to the shared_appRouter/appRouterInstancesingleton, which is fed to bothAppRouterContext.ProviderandinstallWindowNext({ router: appRouterInstance })(passed by reference), so it surfaces on bothuseRouter()andwindow.next.router.- The
synchronousbranch is reachable: gesture push usesprogrammaticTransition = false, sopendingRouterStatestays null and the early return at controller:521 does not win. The new comment documenting that implicit invariant is accurate. flushSyncfires after the RSCawait(in a microtask continuation), outside any React render phase, so it is safe.- URL-safety /
isServerordering matchespush/replace, and the throwing calls run beforetrackScheduledAppRouterNavigation(), so there is no scheduled-navigation leak on a synchronous throw. - The
NextConfig.experimentalobject type retains its inner[key: string]: unknown, so otherexperimental.*keys still type-check. - Enabling the flag on the shared
app-basicfixture only controls the method attachment; nothing else branches on it.
The concerns raised by the prior /bigbonk rounds (scheduled-navigation tracking, the synchronous-mode invariant comment, the unit test) are all resolved in this diff. One non-blocking test-robustness item remains.
|
|
||
| await expect(page.getByTestId("static-content")).toHaveText("This is static content"); | ||
|
|
||
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); |
There was a problem hiding this comment.
Non-blocking (test robustness): the test asserts the positive intermediate state (target page + static content visible, URL updated) during the gesture, but never asserts the negative — that the page is still held and dynamic-content has not yet resolved. target-page/page.tsx deliberately wraps DynamicContent (which await connection()) in <Suspense fallback={loading}> so the dynamic hole is the distinguishing signal between the optimistic gesture commit and the final canonical push.
If connection() resolves quickly in the preview server, the gesture commit and the later router.push() would render identically, and this test could pass even if the synchronous-commit lane silently degraded to a normal deferred transition. Consider asserting the held state during the gesture before clicking end-gesture:
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); | |
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); | |
| // The gesture commit is the intermediate (held) state: the dynamic hole | |
| // should still be suspended until the canonical push resolves it. This is | |
| // what distinguishes the synchronous gesture commit from a normal push. | |
| await expect(page.getByTestId("loading")).toBeVisible(); | |
| await expect(page.getByTestId("dynamic-content")).toHaveCount(0); |
|
Review posted successfully. SummaryI traced this PR end-to-end and found no correctness or security issues. It's a clean, well-scoped, faithful port of the upstream Next.js v16.2.6 gesture-transition deploy test. Verified correct:
One non-blocking inline note posted: the e2e spec asserts the positive intermediate state during the gesture but never the negative — that |
…ure spec coverage
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the change end-to-end against the upstream v16.2.6 source. The implementation is correct, well-scoped, and faithful. I verified the items the prior /bigbonk rounds raised and have nothing new that blocks merge. Two notes below, both non-blocking.
Faithful port — the recurring "add a negative held-state assertion" suggestion should NOT be applied. I fetched the upstream test (test/e2e/app-dir/gesture-transitions/gesture-transitions.test.ts, v16.2.6). It asserts only the positive intermediate state (target page + static content visible, URL updated) and never asserts the held/suspended state — identical to this port. The earlier review rounds repeatedly suggested adding loading visible / dynamic-content absent assertions, but that would go beyond upstream and depends on FreshnessPolicy.Gesture shell behavior, which this PR explicitly lists as a non-goal. Declining that suggestion (and documenting why in the test comment) is the right call. The test still has teeth: the layout calls experimental_gesturePush(href) then awaits inside startTransition, so the target-page-visible assertion before end-gesture would fail if the method were a no-op or an ordinary transition-lane push (which would stay held until the pending promise resolves).
Behavioral parity with upstream gesturePush confirmed:
- Upstream resolves the href via
new URL(addBasePath(href), location.href)thenif (isExternalURL(url)) return. vinext'stoSameOriginAppPath-gated no-op produces the same observable result: same-origin absolute hrefs navigate, external/protocol-relative hrefs no-op. - Upstream blocks
javascript:viaisJavaScriptURLString; vinext'sassertSafeNavigationUrlis stricter (alsodata:/vbscript:), matching vinext's ownpush/replace/prefetch. Consistent defense-in-depth.
Verified: config resolution + default, the raw-boolean define tree-shaking out the module-top guard when disabled, the synchronous/flushSync branch reachability (gesture uses programmaticTransition = false), and attachment to the shared appRouterInstance singleton surfacing on both useRouter() and window.next.router. The targeted next-config and shims unit tests pass locally.
No correctness or security issues found.
| ); | ||
| defines["process.env.__NEXT_GESTURE_TRANSITION"] = JSON.stringify( | ||
| nextConfig.gestureTransition, | ||
| ); |
There was a problem hiding this comment.
Non-blocking (clarity): this define intentionally emits a raw boolean literal (JSON.stringify(true) -> true), unlike the sibling defines a few lines up that stringify ? "true" : "false". That distinction matters: the consumer is if (process.env.__NEXT_GESTURE_TRANSITION) in shims/navigation.ts, so the boolean form is what lets the guard tree-shake when disabled — the string "false" would be truthy. This matches Next.js's own define-env.ts, but the divergence from the neighboring defines is easy to "fix" by mistake later. Worth a one-line comment so a future reader doesn't normalize it to the sibling pattern:
| ); | |
| // Emit a raw boolean (not the "true"/"false" string form used by the | |
| // sibling defines above): the consumer guards with | |
| // `if (process.env.__NEXT_GESTURE_TRANSITION)`, so the literal `false` | |
| // tree-shakes the gesture method away when disabled, whereas the | |
| // string "false" would be truthy. Matches Next.js define-env.ts. | |
| defines["process.env.__NEXT_GESTURE_TRANSITION"] = JSON.stringify( | |
| nextConfig.gestureTransition, | |
| ); |
|
|
||
| expect(page.url()).toContain(`${ROUTE_ROOT}/target-page`); | ||
|
|
||
| // Note: like the upstream test, this deliberately does not assert the |
There was a problem hiding this comment.
Nit: the phrase "like the upstream test" is accurate (I confirmed the v16.2.6 test also asserts only the positive state), but the reason given here ("connection() resolves immediately on the dev server") is vinext-specific, not the upstream reason. Upstream omits the held-state assertion because observing the suspended dynamic hole would require FreshnessPolicy.Gesture shell behavior, which is a stated non-goal of this PR. Consider tightening the comment so it doesn't imply upstream omitted it for the same dev-server timing reason — otherwise this reads as if both tests share a flakiness rationale when really vinext is matching upstream's intentional scope.
|
Review posted successfully. Review SummaryI traced PR #1909 end-to-end against the upstream Next.js v16.2.6 source and found no correctness or security issues. The implementation is clean, well-scoped, and faithful. Posted one review with two non-blocking inline notes. Key finding (resolving the recurring Verified correct:
Two non-blocking notes posted:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the change end-to-end and ran the targeted suites locally (vp test run tests/shims.test.ts -t experimental_gesturePush, vp test run tests/next-config.test.ts -t gestureTransition, vp check on the four core source files) — all green. The implementation is correct, well-scoped, and a faithful port of upstream test/e2e/app-dir/gesture-transitions (v16.2.6). No blocking issues.
Independently re-verified the key invariants (matching the prior /bigbonk rounds):
- Surface area:
useRouter()returnsReact.useContext(AppRouterContext), and the provider value isappRouterInstance(=_appRouter) atapp-browser-entry.ts:1098/app-ssr-entry.ts:391, andinstallWindowNext({ router: appRouterInstance })atapp-browser-entry.ts:2264. So the gated method surfaces on bothuseRouter()andwindow.next.routerfrom the same singleton. synchronousbranch reachability: gesture push callsnavigateClientSide(..., programmaticTransition=false, "synchronous");shouldUsePendingRouterState = programmaticTransition(app-browser-entry.ts:1673) sopendingRouterStatestays null and the early return at controller:521 does not win — theflushSyncbranch at controller:541 is reached.- Tree-shaking: the define emits a raw boolean (
JSON.stringify(nextConfig.gestureTransition)), so the module-topif (process.env.__NEXT_GESTURE_TRANSITION)guard drops the method when disabled. The inline comment inindex.tsexplaining the divergence from the sibling"true"/"false"defines is a good guard against accidental "normalization." - Scheduled-navigation parity:
experimental_gesturePushuses the sametrackScheduledAppRouterNavigation()+releaseScheduledAppRouterNavigationAfterCurrentTask()(microtask) window aspush/replace, and the throwing calls (assertSafeNavigationUrl, same-origin normalization) run before tracking — so no scheduled-navigation leak on a synchronous throw. - Config + types: default
false;experimental?.gestureTransition === truecoerces safely;NextConfig.experimentalretains its inner[key: string]: unknownso otherexperimental.*keys still type-check.
One small forward-looking note inline. Otherwise this is good to merge.
| import type { NextConfig } from "vinext"; | ||
|
|
||
| const nextConfig: NextConfig = { | ||
| experimental: { |
There was a problem hiding this comment.
Non-blocking (test-suite scope): enabling experimental.gestureTransition on the shared app-basic fixture turns the gate on for every app-router e2e/integration test that runs against this fixture, not just the gesture spec. I confirmed the only observable effect is the optional experimental_gesturePush method being attached (nothing else branches on the flag), so this is safe today. The latent risk is that if a future test asserts the absence of the method (to verify the off-path of the gate), it would now silently pass against this fixture because the flag is globally on. Worth a one-line comment here noting the flag is enabled fixture-wide solely for the gesture-transitions spec, so a future reader doesn't assume the fixture represents the default (gate-off) config.
|
Review posted successfully. Review SummaryI traced PR #1909 end-to-end and ran the targeted test suites locally — all green. The implementation is correct, well-scoped, and a faithful port of upstream Next.js v16.2.6's gesture-transition deploy test. No blocking issues. Independently verified:
One non-blocking inline note posted (not raised in the six prior The recurring "add a negative held-state assertion to the e2e test" suggestion from earlier rounds was correctly declined (it would exceed upstream scope and depend on |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the change end-to-end against upstream Next.js v16.2.6 (app-router-instance.ts gesturePush and gesture-transitions.test.ts). The config plumbing, raw-boolean define, gated public method, and the new synchronous/flushSync commit lane are correct and internally consistent. The seven prior /bigbonk rounds covered the main invariants (singleton attachment surfacing on useRouter() + window.next.router, synchronous branch reachability via programmaticTransition = false, scheduled-navigation parity, tree-shaking, NextConfig.experimental index-signature preservation). I re-verified those and they hold. No blocking issues.
One structural note worth recording for future readers: vinext's experimental_gesturePush and upstream's diverge in mechanism, not just internals. Upstream forks an optimistic, discardable router state via dispatchGestureState (FreshnessPolicy.Gesture) and does not push history or run a dynamic request during the gesture — the URL/canonical state only lands when the held router.push() resolves. vinext instead performs a real RSC navigation that commits to history synchronously during the gesture, and then the fixture's canonical router.push(href) navigates to the same URL a second time. This reproduces the upstream test's observable contract (target visible + URL updated mid-gesture) but is a meaningfully different runtime path (double navigation to the same href, no optimistic-state discard). The PR documents this as a deliberate non-goal, so it's an acceptable, intentional divergence — flagging it so it's not mistaken for a faithful internal port later.
Two minor, non-blocking notes inline.
| } | ||
|
|
||
| function formatPublicBfcacheId(value: string | null | undefined): string { | ||
| if (!value || value === INITIAL_BFCACHE_ID) return PUBLIC_INITIAL_BFCACHE_ID; |
There was a problem hiding this comment.
Non-blocking (pre-hydration parity): upstream gesturePush early-returns when getCurrentAppRouterState() === null (gesture fired before hydration is a no-op). Here, if the navigation runtime isn't ready yet, navigateClientSide falls through to its non-runtime branch and performs a hard pushHistoryStateWithoutNotify(...) + commitClientNavigationState() — a real history push rather than a no-op. This matches vinext's existing push/replace fallback so it's consistent, but it does differ from upstream's gesture-specific guard. Only reachable if a gesture is dispatched before hydration, which the fixture never does, so not blocking.
| void navigateClientSide(appHref, "push", options?.scroll !== false, false, "synchronous"); | ||
| releaseScheduledAppRouterNavigationAfterCurrentTask(releaseNavigation); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Non-blocking (error handling): void navigateClientSide(...) is fire-and-forget with no .catch. If the RSC navigation rejects (fetch failure mid-gesture), the rejection surfaces as an unhandled promise rejection. push/replace have the same shape (the void navigateClientSide(...) runs inside React.startTransition, also un-awaited), so this is consistent with existing behavior — just noting that the gesture path inherits the same un-awaited-rejection characteristic without the surrounding try/catch that push/replace use for the synchronous startTransition throw.
|
Review posted successfully. SummaryI traced PR #1909 end-to-end against upstream Next.js v16.2.6 and found no blocking correctness or security issues. The implementation is clean and well-scoped. I confirmed the invariants the seven prior One structural finding worth recording (non-blocking): vinext's Two minor inline notes (both consistent with existing
No code was changed — this is a review-only assessment. |
…time is ready Upstream gesturePush early-returns when getCurrentAppRouterState() is null, so a gesture dispatched before hydration is a no-op. Previously vinext fell through navigateClientSide's non-runtime branch and performed a real history push. Guard on the runtime's navigate function (the same readiness check navigateClientSide uses) and document that the un-awaited void navigateClientSide(...) deliberately matches push/replace's fire-and-forget rejection shape.
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Traced the change end-to-end. The implementation is correct, well-scoped, and consistent with the existing push/replace/prefetch patterns in shims/navigation.ts. No blocking issues. The concerns from the eight prior /bigbonk rounds (singleton attachment surfacing on both useRouter() and window.next.router, synchronous-branch reachability via programmaticTransition = false, scheduled-navigation parity, raw-boolean tree-shaking, NextConfig.experimental index-signature preservation, the inline guard comments, and the unit tests) are all resolved in this diff. I re-verified them and they hold.
Independently confirmed details not previously called out:
- basePath round-trip is correct. For an absolute same-origin href,
experimental_gesturePushcallstoSameOriginAppPath(href, __basePath)which strips basePath (url-utils.ts:69), then hands the strippedappHreftonavigateClientSide, which re-applies it viatoBrowserNavigationHref(..., __basePath)(navigation.ts:1791). This matchesprefetch's flow exactly, so basePath apps don't double-apply or drop the prefix. isExternalUrl===isAbsoluteOrProtocolRelativeUrl(navigation.ts:1562-1564), so the inline external no-op in the gesture path and theisExternalUrlcheck insidenavigateClientSideagree — no classification skew.useRouter()returns{ ...router, bfcacheId }(navigation.ts:2140), and the method is mutated onto the singleton at module-load before any render, so the spread always includes it when enabled.- Type/runtime alignment:
AppRouterInstance.experimental_gesturePush?(href, options?: NavigateOptions)withNavigateOptions = { scroll?: boolean }matches the implementation'soptions?: { scroll?: boolean }.
One minor, non-blocking observation inline.
| // navigate function — the same check navigateClientSide uses before its | ||
| // non-runtime fallback, which would otherwise perform a real history push | ||
| // here instead of upstream's no-op. | ||
| if (!getNavigationRuntime()?.functions.navigate) return; |
There was a problem hiding this comment.
Non-blocking (parity nuance): the readiness guard checks getNavigationRuntime()?.functions.navigate, but the body actually only reads getNavigationRuntime() again indirectly through navigateClientSide, which re-fetches the runtime. If the runtime is registered between this guard and the microtask where navigateClientSide's appNavigate = getNavigationRuntime()?.functions.navigate lookup runs, the two reads could see different states. In practice both run synchronously up to the first await in navigateClientSide (the appNavigate lookup at navigation.ts:1847 happens before any await), so there's no real TOCTOU window here, and push/replace have the same shape. Just noting the guard and the actual dispatch read the runtime separately, which is fine but worth keeping in mind if the runtime registration ever becomes async.
|
Review posted successfully. SummaryI traced PR #1909 end-to-end and found no blocking correctness or security issues. The implementation cleanly adds the experimental This PR has already been through eight prior Details I independently confirmed that hadn't been called out before:
One non-blocking inline note: the readiness guard reads the navigation runtime separately from Good to merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: experimental gesture push
Independently traced the change end-to-end. No blocking correctness or security issues. The config plumbing, raw-boolean define, gated public method, and the new synchronous/flushSync commit lane are all internally consistent, and I re-verified the core invariants from the prior /bigbonk rounds:
- Surface area:
experimental_gesturePushis attached to the_appRouter/appRouterInstancesingleton at module load (before any render), and that singleton is passed by reference toAppRouterContext.Provider(app-browser-entry.ts:1098,app-ssr-entry.ts:391) and toinstallWindowNext({ router: appRouterInstance })(assigned by reference atwindow-next.ts:189). So it surfaces on bothuseRouter()(via the...routerspread) andwindow.next.router. synchronousbranch reachability: gesture push callsnavigateClientSide(..., programmaticTransition=false, "synchronous").shouldUsePendingRouterState = programmaticTransition(app-browser-entry.ts:1673), sopendingRouterStatestays null and the early return at controller:521 does not win — theflushSyncbranch at controller:541 is reached.- Tree-shaking:
JSON.stringify(nextConfig.gestureTransition)emits a raw boolean literal, soif (process.env.__NEXT_GESTURE_TRANSITION)drops the method when disabled. The inline comment inindex.ts:1324correctly documents the divergence from the sibling"true"/"false"string defines. - Throw ordering / scheduled-nav parity:
assertSafeNavigationUrland the same-origin normalization (both of which canreturn/throw) run beforetrackScheduledAppRouterNavigation(), so there is no scheduled-navigation leak on a synchronous throw — consistent withpush/replace. - Config/types: default
false;experimental?.gestureTransition === truecoerces safely;NextConfig.experimentalretains its inner[key: string]: unknown.
The items from the earlier rounds (scheduled-nav tracking, the invariant comment, the unit test, the fixture-scope comment, the index.ts define comment) are all resolved in this diff.
One substantive note below (non-blocking, already a documented non-goal).
| router.experimental_gesturePush(href); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| pendingResolvers.add(resolve); |
There was a problem hiding this comment.
Non-blocking, but worth recording explicitly for future readers: vinext's experimental_gesturePush performs a real RSC navigation with a synchronous history push (navigateClientSide(..., "push", ...)), and then this fixture's canonical router.push(href) navigates to the same URL a second time. Upstream's gesture path is mechanically different — it forks an optimistic, discardable router state (FreshnessPolicy.Gesture) and does not push history during the gesture; the URL only lands when the held push() resolves.
The observable contract the deploy test checks (target visible + URL updated mid-gesture) is reproduced, and the PR lists the React Gesture Transition internals as an explicit non-goal, so this is an acceptable, deliberate divergence. The one place it could leak into observable behavior is history/back-button state: two push-mode navigations to the same href. If navigateRsc's commit dedups same-URL pushes into a replace this is a no-op; if it doesn't, a gesture-then-canonical-push leaves a duplicate history entry that upstream would not. Worth confirming the back-button lands on Home (not target-page twice) if/when a follow-up adds traversal coverage — not required for this PR.
|
Review posted successfully. SummaryI independently traced PR #1909 end-to-end and found no blocking correctness or security issues. The implementation cleanly adds the experimental Verified correct:
One substantive non-blocking note posted (not previously raised at this depth): vinext's gesture path does a real RSC navigation with a synchronous history push, then the fixture's canonical This is good to merge. |
2 participants
Overview
experimental.gestureTransition.experimental.gestureTransition, defineprocess.env.__NEXT_GESTURE_TRANSITION, and exposeuseRouter().experimental_gesturePush()only when enabled.packages/vinext/src/shims/navigation.ts,packages/vinext/src/server/app-browser-navigation-controller.ts,packages/vinext/src/config/next-config.ts,tests/e2e/app-router/nextjs-compat/gesture-transitions.spec.tsexperimental_gesturePush is not a function, and the target page becomes visible during the held gesture transition.Why
The App Router public router surface has to be gated by the same config-derived compile-time feature flag as Next.js. The upstream fixture calls
experimental_gesturePush()inside an async transition that does not complete until a later button click, so an ordinary transition-lane navigation is not observable at the intermediate assertion point.next.config.js.experimental.gestureTransitionto config resolution and emitsprocess.env.__NEXT_GESTURE_TRANSITION.useRouter()should expose the same optional method shape under the feature flag.experimental_gesturePushto the App Router instance andwindow.next.routertype.router.push()resolves the held transition.What changed
experimental.gestureTransitionenabledrouter.experimental_gesturePushwas undefined.process.env.__NEXT_GESTURE_TRANSITION.push.Maintainer review path
packages/vinext/src/config/next-config.tsandpackages/vinext/src/index.ts: config resolution and define plumbing for__NEXT_GESTURE_TRANSITION.packages/vinext/src/shims/navigation.ts: gated public method, URL safety, same-origin normalization, and synchronous visible-commit invocation.packages/vinext/src/server/app-browser-navigation-controller.tsandpackages/vinext/src/server/app-browser-entry.ts: shared commit-mode plumbing that keeps normal navigations on the transition lane and gives gesture an immediate approved visible commit.tests/e2e/app-router/nextjs-compat/gesture-transitions.spec.tsand fixture files: faithful port of the upstream held-transition behavior under the shared app-basic route prefix.Validation
vp checkvp test run tests/next-config.test.ts -t gestureTransitionvp run vinext#buildPLAYWRIGHT_PROJECT=app-router npx playwright test tests/e2e/app-router/nextjs-compat/gesture-transitions.spec.ts --project=app-routervp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/gesture-transitions/gesture-transitions.test.tsRisk / compatibility
push,replace,Link, form, refresh, and traversal paths keep the default transition visible-commit mode.experimental.gestureTransition: true; the only exposed difference is the optional router method.Non-goals
FreshnessPolicy.Gestureor partial PPR dynamic-hole behavior.experimentalflags.References
experimental_gesturePushpublic method.process.env.__NEXT_GESTURE_TRANSITION.