diff --git a/packages/vinext/src/server/app-browser-entry.ts b/packages/vinext/src/server/app-browser-entry.ts index 27589b83..e755f830 100644 --- a/packages/vinext/src/server/app-browser-entry.ts +++ b/packages/vinext/src/server/app-browser-entry.ts @@ -9,7 +9,15 @@ import { encodeReply, setServerCallback, } from "@vitejs/plugin-rsc/browser"; -import { flushSync } from "react-dom"; +import { + createElement, + Fragment, + startTransition, + useEffect, + useRef, + useState, + useTransition, +} from "react"; import { hydrateRoot } from "react-dom/client"; import { PREFETCH_CACHE_TTL, @@ -120,6 +128,65 @@ async function readInitialRscStream(): Promise> { return rscResponse.body; } +// --------------------------------------------------------------------------- +// NavigationRoot — persistent wrapper for concurrent RSC navigation +// +// startTransition(() => root.render(newTree)) does NOT correctly prevent +// Suspense fallbacks from flashing during navigation. When root.render() +// replaces the entire fiber tree, React has no "previously committed content" +// to hold onto — new Suspense boundaries in the incoming tree may flash their +// fallbacks before their content resolves. +// +// The correct fix: hold RSC content in React state inside a persistent +// component. startTransition(() => setState(newContent)) inside a persistent +// component tells React to keep that component's current committed output +// visible until the new render (including all Suspense boundaries) is fully +// resolved, then commit atomically. This is how Next.js App Router prevents +// loading-boundary flashes during client navigation. +// --------------------------------------------------------------------------- + +// Exposed by NavigationRoot. Returns a Promise that resolves once the +// transition commits to the DOM — callers can await it to know when the +// new content is actually visible (used by navigateRsc so that +// __VINEXT_RSC_PENDING__ resolves at the right time for scroll restoration). +let _scheduleRscUpdate: ((content: ReactNode) => Promise) | null = null; + +function NavigationRoot({ initial }: { initial: ReactNode }) { + const [content, setContent] = useState(initial); + // useTransition gives us isPending so we know exactly when a transition + // has committed. We use that to resolve the promise returned to navigateRsc, + // which in turn lets __VINEXT_RSC_PENDING__ resolve at the right moment for + // scroll restoration (restoreScrollPosition in navigation.ts awaits it). + const [isPending, startTransitionHook] = useTransition(); + const resolveRef = useRef<(() => void) | null>(null); + + // After each commit: if a transition just completed, resolve the waiter. + // useEffect runs after the browser has painted the committed tree, which + // is the correct point for scroll restoration to apply. + useEffect(() => { + if (!isPending && resolveRef.current) { + const resolve = resolveRef.current; + resolveRef.current = null; + resolve(); + } + }); + + _scheduleRscUpdate = (newContent: ReactNode): Promise => { + return new Promise((resolve) => { + // Overwrite any prior pending resolve — if a second navigation fires + // before the first commits, the first waiter is abandoned (acceptable). + resolveRef.current = resolve; + startTransitionHook(() => { + setContent(newContent); + }); + }); + }; + + // Fragment wrapper: renders content directly with no extra DOM nodes so + // the hydration output is identical to the server-rendered HTML. + return createElement(Fragment, null, content); +} + function registerServerActionCallback(): void { setServerCallback(async (id, args) => { const temporaryReferences = createTemporaryReferenceSet(); @@ -162,7 +229,13 @@ function registerServerActionCallback(): void { }); if (isServerActionResult(result)) { - getReactRoot().render(result.root); + // Route through NavigationRoot so root.render() doesn't destroy the wrapper. + // Server action results are fully resolved so startTransition commits promptly. + if (_scheduleRscUpdate) { + void _scheduleRscUpdate(result.root); + } else { + getReactRoot().render(result.root); + } if (result.returnValue) { if (!result.returnValue.ok) throw result.returnValue.data; return result.returnValue.data; @@ -170,7 +243,11 @@ function registerServerActionCallback(): void { return undefined; } - getReactRoot().render(result as ReactNode); + if (_scheduleRscUpdate) { + void _scheduleRscUpdate(result as ReactNode); + } else { + getReactRoot().render(result as ReactNode); + } return result; }); } @@ -181,9 +258,12 @@ async function main(): Promise { const rscStream = await readInitialRscStream(); const root = await createFromReadableStream(rscStream); + // Hydrate with NavigationRoot so subsequent navigations go through setState + // transitions rather than root.render() replacements. NavigationRoot's + // Fragment wrapper renders identical DOM to the SSR HTML — no hydration mismatch. reactRoot = hydrateRoot( document, - root as ReactNode, + createElement(NavigationRoot, { initial: root as ReactNode }), import.meta.env.DEV ? { onCaughtError() {} } : undefined, ); @@ -251,10 +331,43 @@ async function main(): Promise { setClientParams({}); } - const rscPayload = await createFromFetch(Promise.resolve(navResponse)); - flushSync(() => { - getReactRoot().render(rscPayload as ReactNode); + // Buffer the full RSC response body before passing it to createFromFetch. + // + // Without buffering, createFromFetch receives a streaming response and + // creates React elements with "lazy" chunks for async server components. + // When React renders these, Suspense boundaries suspend and React commits + // the fallback first (short content), then the resolved content in a + // second pass. This causes two problems: + // 1. Suspense fallback flash — the fallback is briefly visible between + // the shell commit and the resolved-content commit. + // 2. Scroll restoration jank — restoreScrollPosition sees a page that + // is too short to reach the saved scroll position on the first attempt. + // + // By fully buffering the response, all RSC rows are available before the + // flight parser runs. createFromFetch returns a fully-resolved React tree + // with no lazy chunks. React renders and commits the complete content in + // a single pass — no Suspense suspension, no partial commits, no flash. + // + // The tradeoff: the old content stays visible for the full server response + // time (e.g. 400ms for a slow async component). This matches Next.js App + // Router's "keep old UI visible until new content is ready" contract. + const responseBody = await navResponse.arrayBuffer(); + const bufferedResponse = new Response(responseBody, { + headers: navResponse.headers, + status: navResponse.status, + statusText: navResponse.statusText, }); + const rscPayload = await createFromFetch(Promise.resolve(bufferedResponse)); + // Await the transition commit so __VINEXT_RSC_PENDING__ resolves only + // after the new content is painted (needed for scroll restoration). + if (_scheduleRscUpdate) { + await _scheduleRscUpdate(rscPayload as ReactNode); + } else { + // Fallback: shouldn't occur after hydration completes. + startTransition(() => { + getReactRoot().render(rscPayload as ReactNode); + }); + } } catch (error) { console.error("[vinext] RSC navigation error:", error); window.location.href = href; @@ -278,6 +391,7 @@ async function main(): Promise { const rscPayload = await createFromFetch( fetch(toRscUrl(window.location.pathname + window.location.search)), ); + // HMR bypasses NavigationRoot for immediate code-change feedback. getReactRoot().render(rscPayload as ReactNode); } catch (error) { console.error("[vinext] RSC HMR error:", error); diff --git a/packages/vinext/src/shims/navigation.ts b/packages/vinext/src/shims/navigation.ts index 2e23b5b8..06605baf 100644 --- a/packages/vinext/src/shims/navigation.ts +++ b/packages/vinext/src/shims/navigation.ts @@ -495,18 +495,28 @@ function restoreScrollPosition(state: unknown): void { void Promise.resolve().then(() => { const pending: Promise | null = window.__VINEXT_RSC_PENDING__ ?? null; + // Retry scrollTo until the page is tall enough to reach the target. + // + // The first attempt may land short when a Suspense boundary was not + // previously in the committed tree (e.g. navigating back to a page with + // an async list). React commits the Suspense fallback first (short + // content, max-scroll = 0), then commits the resolved content in a + // second pass. Each rAF retry gives newly-committed content a chance to + // be in the DOM so the scroll can reach its target. + const scrollWithRetry = (deadline: number) => { + window.scrollTo(x, y); + if (Math.abs(window.scrollY - y) > 2 && Date.now() < deadline) { + requestAnimationFrame(() => scrollWithRetry(deadline)); + } + }; + if (pending) { - // Wait for the RSC navigation to finish rendering, then scroll. void pending.then(() => { - requestAnimationFrame(() => { - window.scrollTo(x, y); - }); + requestAnimationFrame(() => scrollWithRetry(Date.now() + 1500)); }); } else { // No RSC navigation in flight (Pages Router or already settled). - requestAnimationFrame(() => { - window.scrollTo(x, y); - }); + requestAnimationFrame(() => scrollWithRetry(Date.now() + 1500)); } }); } @@ -866,6 +876,14 @@ export function unauthorized(): never { // Listen for popstate on the client if (!isServer) { + // Disable the browser's built-in scroll restoration so it doesn't override + // our manual restoration. The browser's 'auto' mode fires asynchronously and + // can reset scroll to 0 after our rAF-based restoration already set the + // correct position. We own scroll restoration entirely via restoreScrollPosition. + if ("scrollRestoration" in history) { + history.scrollRestoration = "manual"; + } + window.addEventListener("popstate", (event) => { notifyListeners(); // Restore scroll position for back/forward navigation diff --git a/tests/e2e/app-router/loading.spec.ts b/tests/e2e/app-router/loading.spec.ts index 64b417fc..2c469afd 100644 --- a/tests/e2e/app-router/loading.spec.ts +++ b/tests/e2e/app-router/loading.spec.ts @@ -2,6 +2,13 @@ import { test, expect } from "@playwright/test"; const BASE = "http://localhost:4174"; +async function waitForHydration(page: import("@playwright/test").Page) { + await expect(async () => { + const ready = await page.evaluate(() => !!(window as any).__VINEXT_RSC_ROOT__); + expect(ready).toBe(true); + }).toPass({ timeout: 10_000 }); +} + test.describe("Loading boundaries (loading.tsx)", () => { test("slow page eventually renders content", async ({ page }) => { await page.goto(`${BASE}/slow`); @@ -45,4 +52,35 @@ test.describe("Loading boundaries (loading.tsx)", () => { timeout: 10_000, }); }); + + /** + * Client navigation to a slow page works end-to-end. + * + * Note: showing loading.tsx during client navigation to a new route is + * correct Next.js behavior — React shows the Suspense boundary for a + * route segment that has never been rendered before. The regression we + * guard against (issue #639) is *partial* UI flash where content *outside* + * a Suspense boundary updates before content *inside* it is ready. That + * scenario is covered in navigation-flows.spec.ts. + */ + test("client navigation to slow page completes without full reload", async ({ page }) => { + await page.goto(`${BASE}/`); + await expect(page.locator("h1")).toHaveText("Welcome to App Router"); + await waitForHydration(page); + + await page.evaluate(() => { + (window as any).__NAV_MARKER__ = true; + }); + + // Client-navigate — do NOT use page.goto() (that is a hard/SSR navigation). + await page.click('[data-testid="slow-link"]'); + + // The slow page has a 2s server delay; wait up to 10s total. + await expect(page.locator("h1")).toHaveText("Slow Page", { timeout: 10_000 }); + await expect(page.locator("main > p")).toHaveText("This page has a loading boundary."); + + // Confirm no full-page reload occurred. + const marker = await page.evaluate(() => (window as any).__NAV_MARKER__); + expect(marker).toBe(true); + }); }); diff --git a/tests/e2e/app-router/navigation-flows.spec.ts b/tests/e2e/app-router/navigation-flows.spec.ts index ce9f6eab..a402f33a 100644 --- a/tests/e2e/app-router/navigation-flows.spec.ts +++ b/tests/e2e/app-router/navigation-flows.spec.ts @@ -235,3 +235,135 @@ test.describe("App Router navigation flows", () => { await expect(page.locator('[data-testid="client-search-q"]')).toHaveText("test-param"); }); }); + +// --------------------------------------------------------------------------- +// Regression tests for issue #639: Suspense partial-flash during navigation +// +// The bug: flushSync() caused React to synchronously commit a partial new tree +// where content *outside* a Suspense boundary (e.g. a heading) had updated to +// the new value but content *inside* the Suspense boundary was still showing +// its fallback. This "partial flash" made it look like the heading was wrong. +// +// The fix: NavigationRoot holds RSC content in React state and uses +// startTransition to schedule updates. React's transition semantics keep the +// *entire* previous committed tree visible until all Suspense boundaries in the +// new tree have resolved, then commits atomically. +// +// We test this with /suspense-nav-test: a page where the heading is *outside* +// Suspense but the list is *inside* Suspense. Both depend on the same ?filter +// param. A partial flash would produce heading="All Items" + list fallback at +// the same time. The correct behavior is: old heading stays until list is ready, +// then both update together. +// --------------------------------------------------------------------------- +test.describe("issue #639 — Suspense partial-flash regression", () => { + test("heading and list update together during same-page navigation", async ({ page }) => { + // Start with filter applied so Suspense has resolved content. + await page.goto(`${BASE}/suspense-nav-test?filter=active`); + await waitForHydration(page); + + await expect(page.locator("#page-heading")).toHaveText("Filtered: active"); + await expect(page.locator("#item-list")).toBeVisible(); + + // Instrument: record if we ever observe the NEW heading ("All Items") at + // the same moment as the list loading fallback — that is the partial flash. + await page.evaluate(() => { + (window as any).__PARTIAL_FLASH_SEEN__ = false; + const observer = new MutationObserver(() => { + const heading = document.getElementById("page-heading"); + const loadingFallback = document.getElementById("list-loading"); + // Partial flash: new heading visible while list is still loading + if (heading?.textContent === "All Items" && loadingFallback) { + (window as any).__PARTIAL_FLASH_SEEN__ = true; + } + }); + observer.observe(document.body, { + childList: true, + subtree: true, + characterData: true, + }); + (window as any).__PARTIAL_FLASH_OBSERVER__ = observer; + }); + + // Navigate back to the unfiltered state (clears the filter). + await page.click("#toggle-filter"); + + // Wait for the full transition to complete. + await expect(page.locator("#page-heading")).toHaveText("All Items", { timeout: 10_000 }); + await expect(page.locator("#item-list")).toBeVisible(); + + await page.evaluate(() => { + (window as any).__PARTIAL_FLASH_OBSERVER__?.disconnect(); + }); + + const partialFlashSeen = await page.evaluate(() => (window as any).__PARTIAL_FLASH_SEEN__); + expect(partialFlashSeen).toBe(false); + }); + + test("back/forward navigation completes correctly on suspense page", async ({ page }) => { + await page.goto(`${BASE}/suspense-nav-test`); + await waitForHydration(page); + await expect(page.locator("#page-heading")).toHaveText("All Items"); + + // Navigate to filtered state + await page.click("#toggle-filter"); + await expect(page.locator("#page-heading")).toHaveText("Filtered: active", { + timeout: 10_000, + }); + + // Navigate back + await page.goBack(); + await expect(page.locator("#page-heading")).toHaveText("All Items", { timeout: 10_000 }); + + // Navigate forward + await page.goForward(); + await expect(page.locator("#page-heading")).toHaveText("Filtered: active", { + timeout: 10_000, + }); + }); + + /** + * Regression test for issue #639 — back-button scroll restoration jank. + * + * The bug: flushSync committed an incomplete tree (with Suspense fallbacks) + * before scroll restoration ran. Because fallbacks have different heights + * than resolved content, the restored scroll position landed in the wrong + * place, then jumped again when content resolved — visible stutter. + * + * The fix (NavigationRoot): the old content stays visible during the + * transition, so scroll is set on stable layout. The new content then + * commits at that scroll position. No layout shift, no stutter. + */ + test("back-button restores scroll position after navigating to item detail", async ({ page }) => { + await page.goto(`${BASE}/suspense-nav-test`); + await waitForHydration(page); + + // Wait for list to load (async, 400ms delay) + await expect(page.locator("#item-list")).toBeVisible({ timeout: 10_000 }); + + // Scroll well down the list so there is a meaningful position to restore. + await page.evaluate(() => window.scrollTo(0, 600)); + const scrollBefore = await page.evaluate(() => window.scrollY); + expect(scrollBefore).toBeGreaterThan(0); + + // Navigate via evaluate click (no Playwright auto-scroll). + await page.evaluate(() => { + const link = document.querySelector("#item-list a:first-child") as HTMLAnchorElement; + link?.click(); + }); + await expect(page.locator("#item-heading")).toBeVisible({ timeout: 10_000 }); + + // Press back. + await page.goBack(); + await expect(page.locator("#item-list")).toBeVisible({ timeout: 10_000 }); + + // Allow time for the retry-based scroll restoration to succeed. + // The list has a 400ms load delay; restoration retries until the page + // is tall enough to reach the target position. + await page.waitForTimeout(800); + + const scrollAfter = await page.evaluate(() => window.scrollY); + + // Scroll should be close to where we left off (within 50px tolerance). + expect(Math.abs(scrollAfter - scrollBefore)).toBeLessThan(50); + }); +}); diff --git a/tests/fixtures/app-basic/app/page.tsx b/tests/fixtures/app-basic/app/page.tsx index f0d4978f..1a93cc23 100644 --- a/tests/fixtures/app-basic/app/page.tsx +++ b/tests/fixtures/app-basic/app/page.tsx @@ -9,6 +9,9 @@ export default function HomePage() { Go to About Go to Blog Go to Dashboard + + Go to Slow + Go to Headers Test diff --git a/tests/fixtures/app-basic/app/suspense-nav-test/FilterToggle.tsx b/tests/fixtures/app-basic/app/suspense-nav-test/FilterToggle.tsx new file mode 100644 index 00000000..93ffb59f --- /dev/null +++ b/tests/fixtures/app-basic/app/suspense-nav-test/FilterToggle.tsx @@ -0,0 +1,16 @@ +"use client"; +import { useRouter } from "next/navigation"; + +export function FilterToggle({ current }: { current: string }) { + const router = useRouter(); + return ( + + ); +} diff --git a/tests/fixtures/app-basic/app/suspense-nav-test/item/[id]/page.tsx b/tests/fixtures/app-basic/app/suspense-nav-test/item/[id]/page.tsx new file mode 100644 index 00000000..48ff5534 --- /dev/null +++ b/tests/fixtures/app-basic/app/suspense-nav-test/item/[id]/page.tsx @@ -0,0 +1,28 @@ +import Link from "next/link"; + +// Async detail page — simulates a DB fetch that takes a moment. +// This is important for the scroll-restoration test: back-button scroll +// should only fire once the destination content is ready, not on a partial +// tree with wrong layout heights. +async function ItemDetail({ id }: { id: string }) { + await new Promise((r) => setTimeout(r, 200)); + return ( +
+

Details for item {id}

+

This content loads asynchronously (200ms).

+
+ ); +} + +export default async function ItemPage({ params }: { params: Promise<{ id: string }> }) { + const { id } = await params; + return ( +
+

Item {id}

+ + ← Back to list + + +
+ ); +} diff --git a/tests/fixtures/app-basic/app/suspense-nav-test/page.tsx b/tests/fixtures/app-basic/app/suspense-nav-test/page.tsx new file mode 100644 index 00000000..f1f27ca3 --- /dev/null +++ b/tests/fixtures/app-basic/app/suspense-nav-test/page.tsx @@ -0,0 +1,45 @@ +import Link from "next/link"; +import { Suspense } from "react"; +import { FilterToggle } from "./FilterToggle"; + +const ALL_ITEMS = Array.from({ length: 20 }, (_, i) => ({ + id: i + 1, + text: `Item ${String.fromCharCode(65 + i)}`, // Item A … Item T +})); + +async function ItemList({ filter }: { filter: string }) { + // Artificial delay: long enough for Playwright to catch intermediate state. + await new Promise((r) => setTimeout(r, 400)); + const items = filter ? [{ id: 1, text: `Filtered: ${filter}` }] : ALL_ITEMS; + return ( +
    + {items.map((i) => ( +
  • + {i.text} +
    • + ))} +
+ ); +} + +export default async function SuspenseNavTestPage({ + searchParams, +}: { + searchParams: Promise<{ filter?: string }>; +}) { + const { filter = "" } = await searchParams; + + return ( +
+ {/* Heading is OUTSIDE the Suspense boundary. + With flushSync, it updates before the list is ready — partial flash. + With startTransition (NavigationRoot), it only updates once the list + is also ready — both change together (issue #639). */} +

{filter ? `Filtered: ${filter}` : "All Items"}

+ + Loading items...
}> + + + + ); +}