fix: address code review findings

- Move server action cache clearing before RSC deserialization so
  invalidation isn't skipped if the payload is malformed
- Document isSameRoute staleness during rapid A->B->A navigations
- Document why reading navigationSnapshotActiveCount during render is
  safe despite being a React anti-pattern
- Fix restoreRscResponse comment: mutation is the risk, not just
  transfer/detach
- Replace waitForTimeout with double-rAF waitForPaintFlush in E2E
  tests for deterministic CI behavior
- Document the self-referencing promise identity pattern in
  prefetchRscResponse

Co-authored-by: Divanshu Chauhan <23524935+Divkix@users.noreply.github.com>

Author: NathanDrake2406

packages/vinext/src/server/app-browser-entry.ts

@@ -459,17 +459,18 @@ function registerServerActionCallback(): void {
       return undefined;
     }
 
+    // Clear navigation caches after the fetch succeeds but before RSC
+    // deserialization. Server actions may have mutated data, making cached
+    // responses stale. We clear immediately after the fetch (not after
+    // deserialization) so cache invalidation isn't skipped if the RSC
+    // payload is malformed.
+    clearClientNavigationCaches();
+
     const result = await createFromFetch<ServerActionResult | ReactNode>(
       Promise.resolve(fetchResponse),
       { temporaryReferences },
     );
 
-    // Clear navigation caches after the action succeeds — server actions
-    // may have mutated data, making cached responses stale. We clear here
-    // (not before the fetch) so failed actions don't unnecessarily
-    // invalidate valid caches.
-    clearClientNavigationCaches();
-
     if (isServerActionResult(result)) {
       updateBrowserTree(
         result.root,
@@ -538,6 +539,12 @@ async function main(): Promise<void> {
       // so React keeps the old UI visible during the transition. For cross-route
       // navigations (different pathname), use synchronous updates — React's
       // startTransition hangs in Firefox when replacing the entire tree.
+      //
+      // Note: window.location.pathname reflects the *previous* page here because
+      // the two-phase commit defers history updates to useLayoutEffect. During
+      // rapid A->B->A, the second navigation may see isSameRoute=true while the
+      // user perceives a cross-route nav. This is harmless — it only affects
+      // whether startTransition is used (smoother animation, not correctness).
       const isSameRoute = url.pathname === window.location.pathname;
       const cachedRoute = getVisitedResponse(rscUrl, navigationKind);
       const navigationCommitEffect = createNavigationCommitEffect(href, historyUpdateMode);

packages/vinext/src/shims/navigation.ts

@@ -245,10 +245,10 @@ export async function snapshotRscResponse(response: Response): Promise<CachedRsc
 
 export function restoreRscResponse(snapshot: CachedRscResponse): Response {
   // Uint8Array creates a view over the cached ArrayBuffer without copying.
-  // ReadableStream consumption does not detach the underlying buffer, so
-  // repeated restores from the same snapshot are safe. If a future consumer
-  // ever transfers or detaches the buffer, this assumption breaks — add a
-  // defensive .slice(0) in that case.
+  // ReadableStream consumption does not mutate or detach the underlying
+  // buffer, so repeated restores from the same snapshot are safe. If a
+  // future consumer ever mutates, transfers, or detaches the buffer, this
+  // assumption breaks — add a defensive .slice(0) in that case.
   const body = new ReadableStream<Uint8Array>({
     start(controller) {
       controller.enqueue(new Uint8Array(snapshot.body));
@@ -367,6 +367,11 @@ export function prefetchRscResponse(rscUrl: string, responsePromise: Promise<Res
     evictPrefetchCacheIfNeeded();
   }
 
+  // The `pendingResponse` variable is assigned and then referenced inside its
+  // own .then()/.catch() chain. This is an intentional identity check pattern:
+  // when the promise settles, `cache.get(rscUrl)?.pendingResponse === pendingResponse`
+  // verifies this is still the *current* pending request for that URL (not a
+  // newer one that superseded it).
   let pendingResponse: Promise<CachedRscResponse | null>;
   pendingResponse = responsePromise
     .then(async (response) => {
@@ -660,9 +665,16 @@ export function usePathname(): string {
     getPathnameSnapshot,
     () => _getServerContext()?.pathname ?? "/",
   );
-  // Only use the render snapshot during an active navigation transition.
-  // After commit, fall through to useSyncExternalStore so user
-  // pushState/replaceState calls are immediately reflected.
+  // Prefer the render snapshot during an active navigation transition so
+  // hooks return the pending URL, not the stale committed one. After commit,
+  // fall through to useSyncExternalStore so user pushState/replaceState
+  // calls are immediately reflected.
+  //
+  // Reading navigationSnapshotActiveCount (mutable external state) during
+  // render is technically a React anti-pattern, but safe here: the counter
+  // only changes synchronously before startTransition (activate) and in
+  // useLayoutEffect after commit (deactivate). React's concurrent renders
+  // within a single transition see the same external state snapshot.
   if (renderSnapshot && (getClientNavigationState()?.navigationSnapshotActiveCount ?? 0) > 0) {
     return renderSnapshot.pathname;
   }

tests/e2e/app-router/navigation-regressions.spec.ts

@@ -9,6 +9,13 @@ async function waitForHydration(page: import("@playwright/test").Page) {
   }).toPass({ timeout: 10_000 });
 }
 
+/** Wait for two animation frames to flush all pending paints and microtasks. */
+async function waitForPaintFlush(page: import("@playwright/test").Page) {
+  await page.evaluate(
+    () => new Promise((r) => requestAnimationFrame(() => requestAnimationFrame(r))),
+  );
+}
+
 async function waitForProvidersList(page: import("@playwright/test").Page) {
   await expect(page.locator("#providers-title")).toHaveText("Providers");
   await expect(page.locator("#provider-list")).toBeVisible({ timeout: 10_000 });
@@ -154,7 +161,7 @@ test.describe("App Router navigation regressions", () => {
     await expect(page.locator("#client-param-label")).toHaveText("Client param: beta");
     await expect(page.locator("#param-filter-title")).toHaveText("Server param: beta");
     await expect(page.locator("#param-filter-first-row")).toHaveText("Beta 1");
-    await page.waitForTimeout(100);
+    await waitForPaintFlush(page);
 
     const events = await page.evaluate(() => {
       (
@@ -264,7 +271,7 @@ test.describe("App Router navigation regressions", () => {
     await expect(page.locator("#client-filter-label")).toHaveText("Client filter: beta");
     await expect(page.locator("#query-filter-title")).toHaveText("Server filter: beta");
     await expect(page.locator("#query-filter-first-row")).toHaveText("Beta 1");
-    await page.waitForTimeout(100);
+    await waitForPaintFlush(page);
 
     const events = await page.evaluate(() => {
       (
@@ -362,7 +369,7 @@ test.describe("App Router navigation regressions", () => {
     await page.locator("#query-filter-beta").click();
     await expect(page.locator("#query-filter-title")).toHaveText("Server filter: beta");
     await expect(page.locator("#query-filter-first-row")).toHaveText("Beta 1");
-    await page.waitForTimeout(100);
+    await waitForPaintFlush(page);
 
     const events = await page.evaluate(() => {
       (
@@ -453,7 +460,7 @@ test.describe("App Router navigation regressions", () => {
     await page.locator("#query-filter-beta").click();
     await expect(page.locator("#query-filter-title")).toHaveText("Server filter: beta");
     await expect(page.locator("#query-filter-first-row")).toHaveText("Beta 1");
-    await page.waitForTimeout(100);
+    await waitForPaintFlush(page);
 
     const events = await page.evaluate(() => {
       (
@@ -550,7 +557,7 @@ test.describe("App Router navigation regressions", () => {
     await expect(page.locator("#link-client-filter-label")).toHaveText("Client filter: beta");
     await expect(page.locator("#link-filter-title")).toHaveText("Server filter: beta");
     await expect(page.locator("#link-filter-first-row")).toHaveText("Beta 1");
-    await page.waitForTimeout(100);
+    await waitForPaintFlush(page);
 
     const events = await page.evaluate(() => {
       (
@@ -609,7 +616,7 @@ test.describe("App Router navigation regressions", () => {
     await waitForHydration(page);
 
     await page.locator("#link-filter-beta").hover();
-    await page.waitForTimeout(50);
+    await waitForPaintFlush(page);
 
     await page.locator("#link-filter-beta").click();
     await expect(page.locator("#link-client-filter-label")).toHaveText("Client filter: beta");