diff --git a/e2e/dashboard-ssr-prefetch-hydration.spec.ts b/e2e/dashboard-ssr-prefetch-hydration.spec.ts new file mode 100644 index 000000000..efeec4d01 --- /dev/null +++ b/e2e/dashboard-ssr-prefetch-hydration.spec.ts @@ -0,0 +1,100 @@ +import { expect, test } from "./setup/test"; + +import { STORAGE_STATE_PATH } from "./setup/global-setup"; +import { SSR_PREFETCH_BASE_URL } from "./setup/ssr-prefetch-server"; + +/** + * The dashboard survives hydration in the configuration that ships. + * + * `/` is an RSC wrapper that runs the dashboard's reads server-side and hands + * them to TanStack through a `HydrationBoundary` (`src/app/page.tsx`), so the + * first HTML already carries real tiles. It only does that when + * `DASHBOARD_SSR_PREFETCH` is not `"false"` — which is the default, and + * therefore what every self-hoster runs. + * + * The rest of the suite runs against a server with that flag OFF, because the + * dashboard specs drive their fixtures through `page.route` and a route mock + * only sees client fetches. So nothing here ever exercised the shipped path, + * and a React #418 hydration bailout sat on `/` for a signed-in account on + * every cold load: the app shell hydrates first and its queries answer before + * the streamed dashboard boundary hydrates, so the dashboard's first CLIENT + * render read an account payload (and an achievements payload, and its own + * in-flight tiles) the server never had. React threw the streamed tree away + * and rebuilt it — paying for the prefetch and delivering none of it. + * + * This spec talks to the second server (`SSR_PREFETCH_BASE_URL`, prefetch ON, + * no route mocks, the real seeded account) and asserts two things a mocked + * spec cannot: + * + * 1. the browser reports no hydration error at all; + * 2. the tile node the SERVER streamed is still the same DOM node once the + * page settles. A bailout re-creates it, so node identity is the honest + * witness that the server's work survived instead of being re-done. The + * probe only reads identity — writing a marker attribute onto server HTML + * would be a hydration mismatch of its own making. + * + * Runs on both viewport projects, because the defect reproduced on both. + */ +declare global { + interface Window { + __ssrTileWitness?: { seen: boolean; replaced: number }; + } +} + +test.describe("dashboard hydration with the SSR prefetch on", () => { + test.use({ + storageState: STORAGE_STATE_PATH, + baseURL: SSR_PREFETCH_BASE_URL, + }); + + test("streams the dashboard and keeps it through hydration", async ({ + page, + }) => { + const clientErrors: string[] = []; + page.on("pageerror", (error) => clientErrors.push(error.message)); + page.on("console", (message) => { + if (message.type() === "error") clientErrors.push(message.text()); + }); + + // `addInitScript` runs before any page script, so the poll is armed for + // the very first frame and latches the node React hydrated into. + await page.addInitScript(() => { + const witness = { seen: false, replaced: 0 }; + window.__ssrTileWitness = witness; + let node: Element | null = null; + const poll = () => { + const el = document.querySelector('[data-slot="dashboard-tile-strip"]'); + if (el) { + if (!node) witness.seen = true; + else if (node !== el) witness.replaced += 1; + node = el; + } + requestAnimationFrame(poll); + }; + requestAnimationFrame(poll); + }); + + await page.goto("/"); + + const strip = page.locator('[data-slot="dashboard-tile-strip"]'); + await expect(strip).toBeVisible(); + // The seeded account owns one weight reading, so the strip carries a real + // tile — proof the prefetch populated this render rather than the client + // having filled it in after mount. + await expect( + strip.locator('[data-slot="dashboard-tile-link"]').first(), + ).toBeVisible(); + + // Let every boundary hydrate and every post-mount refetch land before the + // verdict; a bailout that happens late is still a bailout. + await page.waitForLoadState("networkidle"); + + expect( + clientErrors.filter((message) => /418|hydrat/i.test(message)), + ).toEqual([]); + + const witness = await page.evaluate(() => window.__ssrTileWitness); + expect(witness?.seen).toBe(true); + expect(witness?.replaced).toBe(0); + }); +}); diff --git a/e2e/setup/ssr-prefetch-server.ts b/e2e/setup/ssr-prefetch-server.ts new file mode 100644 index 000000000..9bf2bf78b --- /dev/null +++ b/e2e/setup/ssr-prefetch-server.ts @@ -0,0 +1,20 @@ +/** + * The second E2E server: the app as self-hosters actually run it. + * + * `playwright.config.ts` starts the main server with + * `DASHBOARD_SSR_PREFETCH=false`, because the dashboard specs mock `/api/*` + * through `page.route` and a route mock only sees CLIENT fetches — an + * SSR-embedded snapshot would bypass every fixture. That trade is sound for + * those specs and wrong for the product: the flag ships ON, so the whole suite + * was proving a configuration nobody runs. + * + * This module names the second server so the config and the one spec that + * uses it cannot drift on a port number. With `E2E_SKIP_WEB_SERVER=1` neither + * server is started for you and both have to be running — the same contract + * that already applies to the main one. + */ +export const SSR_PREFETCH_PORT = 3100; + +export const SSR_PREFETCH_BASE_URL = + process.env.E2E_SSR_PREFETCH_BASE_URL ?? + `http://localhost:${SSR_PREFETCH_PORT}`; diff --git a/playwright.config.ts b/playwright.config.ts index 35f240032..3a79e9e88 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -1,5 +1,10 @@ import { defineConfig, devices } from "@playwright/test"; +import { + SSR_PREFETCH_BASE_URL, + SSR_PREFETCH_PORT, +} from "./e2e/setup/ssr-prefetch-server"; + /** * Playwright configuration for the HealthLog E2E suite. * @@ -94,28 +99,57 @@ export default defineConfig({ // to point at an already-running dev server. webServer: process.env.E2E_SKIP_WEB_SERVER ? undefined - : { - command: `mkdir -p .next/standalone/.next/static .next/standalone/public && cp -R .next/static/. .next/standalone/.next/static && cp -R public/. .next/standalone/public && PORT=3000 HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, - url: "http://localhost:3000/api/version", - timeout: 60_000, - reuseExistingServer: !process.env.CI, - stdout: "ignore", - stderr: "pipe", - env: { - ...process.env, - // Native document rendering is fail-soft and outside the browser - // suite. Disable it here so queued local thumbnail jobs cannot - // crash the shared server inside Skia during unrelated scenarios. - NATIVE_CANVAS: "off", - // The dashboard RSC wrapper server-prefetches the snapshot into - // the first HTML (HydrationBoundary). Playwright's route mocks - // only see CLIENT fetches, so an SSR-embedded snapshot would - // bypass `mockDashboardSnapshot` and every dashboard spec would - // assert against the seeded account instead of its fixture. - // Disable the prefetch for the e2e server — the suite keeps the - // deterministic client-fetch path; the SSR fast path is - // verified by Lighthouse/manual passes. - DASHBOARD_SSR_PREFETCH: "false", + : [ + { + command: `mkdir -p .next/standalone/.next/static .next/standalone/public && cp -R .next/static/. .next/standalone/.next/static && cp -R public/. .next/standalone/public && PORT=3000 HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, + url: "http://localhost:3000/api/version", + timeout: 60_000, + reuseExistingServer: !process.env.CI, + stdout: "ignore", + stderr: "pipe", + env: { + ...process.env, + // Native document rendering is fail-soft and outside the browser + // suite. Disable it here so queued local thumbnail jobs cannot + // crash the shared server inside Skia during unrelated scenarios. + NATIVE_CANVAS: "off", + // The dashboard RSC wrapper server-prefetches the snapshot into + // the first HTML (HydrationBoundary). Playwright's route mocks + // only see CLIENT fetches, so an SSR-embedded snapshot would + // bypass `mockDashboardSnapshot` and every dashboard spec would + // assert against the seeded account instead of its fixture. + // Disable the prefetch for the e2e server — the suite keeps the + // deterministic client-fetch path. + DASHBOARD_SSR_PREFETCH: "false", + }, }, - }, + // The SHIPPED configuration, on its own port. + // + // Every spec above runs against a server with the dashboard's RSC + // prefetch disabled, which is the one thing self-hosters never run: + // the flag defaults ON. That gap let a React #418 hydration bailout + // reach production on `/` for a signed-in account, on both viewports, + // on every cold load — the client's first render read query cells the + // server never had, so React discarded the streamed dashboard and + // rebuilt it. The suite could not see it, because with the prefetch + // off both sides paint the same skeleton. + // + // One extra server on 3100 closes that. Only + // `dashboard-ssr-prefetch-hydration.spec.ts` talks to it, and it + // asserts the one thing the mocked specs cannot: that the page the + // server streamed is the page the browser keeps. + { + command: `PORT=${SSR_PREFETCH_PORT} HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, + url: `${SSR_PREFETCH_BASE_URL}/api/version`, + timeout: 60_000, + reuseExistingServer: !process.env.CI, + stdout: "ignore", + stderr: "pipe", + env: { + ...process.env, + NATIVE_CANVAS: "off", + DASHBOARD_SSR_PREFETCH: "true", + }, + }, + ], }); diff --git a/scripts/__tests__/ci-determinism.test.ts b/scripts/__tests__/ci-determinism.test.ts index 179478a10..1a8cfac66 100644 --- a/scripts/__tests__/ci-determinism.test.ts +++ b/scripts/__tests__/ci-determinism.test.ts @@ -45,13 +45,31 @@ describe("Playwright CI determinism", () => { expect(config.retries).toBe(2); expect(config.failOnFlakyTests).toBe(true); expect(config.reporter).toEqual([["github"], ["html", { open: "never" }]]); - expect(config.webServer).toMatchObject({ - command: expect.stringContaining( - `${JSON.stringify(process.execPath)} .next/standalone/server.js`, - ), - env: expect.objectContaining({ - NATIVE_CANVAS: "off", - }), + // Two servers, and which is which matters. The suite's own server runs + // with the dashboard's RSC prefetch OFF so `page.route` fixtures govern + // what the dashboard paints; the second runs it ON, which is the default + // and therefore the only configuration self-hosters ever see. A React #418 + // hydration bailout shipped on `/` precisely because nothing in CI ran the + // shipped one. Pin both so that gap cannot reopen by deletion. + const servers = config.webServer; + expect(Array.isArray(servers)).toBe(true); + const webServers = servers as Array>; + expect(webServers).toHaveLength(2); + for (const server of webServers) { + expect(server).toMatchObject({ + command: expect.stringContaining( + `${JSON.stringify(process.execPath)} .next/standalone/server.js`, + ), + env: expect.objectContaining({ + NATIVE_CANVAS: "off", + }), + }); + } + expect(webServers[0]!.env).toMatchObject({ + DASHBOARD_SSR_PREFETCH: "false", + }); + expect(webServers[1]!.env).toMatchObject({ + DASHBOARD_SSR_PREFETCH: "true", }); }); diff --git a/src/__tests__/display-transform-rounding-guard.test.ts b/src/__tests__/display-transform-rounding-guard.test.ts index d0d44e943..7db775374 100644 --- a/src/__tests__/display-transform-rounding-guard.test.ts +++ b/src/__tests__/display-transform-rounding-guard.test.ts @@ -200,9 +200,12 @@ describe("display-transform rounding guard — the bypass freeze", () => { it("every allowlisted surface still converts its rendered values", () => { // The chart formats its own series; anything these files render outside - // the chart must come back through the preference hook. + // the chart must come back through the preference hook. Either reading of + // it counts: a surface that paints inside a streamed page boundary takes + // `useUnitDisplayOnceMounted()`, which is the same sugar with the + // preference withheld from the hydration render. const offenders = RAW_SCALE_SURFACES.filter( - (rel) => !read(rel).includes("useUnitDisplay()"), + (rel) => !/\buseUnitDisplay(OnceMounted)?\(\)/.test(read(rel)), ); expect(offenders).toEqual([]); }); diff --git a/src/app/page-client.tsx b/src/app/page-client.tsx index 9fb46eb06..0174dd042 100644 --- a/src/app/page-client.tsx +++ b/src/app/page-client.tsx @@ -4,8 +4,8 @@ import { LookingAfterCard } from "@/components/dashboard/looking-after-card"; import { useRecordCapabilities } from "@/hooks/use-record-capabilities"; import React, { Suspense, useCallback, useMemo, useState } from "react"; import { useQuery, useQueryClient } from "@tanstack/react-query"; -import { useAuth } from "@/hooks/use-auth"; -import { useUnitDisplay } from "@/hooks/use-unit-display"; +import { useAccountOnceMounted, useAuth } from "@/hooks/use-auth"; +import { useUnitDisplayOnceMounted } from "@/hooks/use-unit-display"; import { useMounted } from "@/hooks/use-mounted"; import { usePullToRefresh } from "@/hooks/use-pull-to-refresh"; import { PullToRefreshIndicator } from "@/components/ui/pull-to-refresh-indicator"; @@ -226,8 +226,18 @@ export default function DashboardPageClient({ */ batchWindow?: BatchWindow; } = {}) { - const { isAuthenticated, user } = useAuth(); + const { isAuthenticated } = useAuth(); const mounted = useMounted(); + // The account payload is withheld until this boundary has mounted, so the + // hydration render paints exactly what the server streamed. The dashboard + // hydrates after the app shell, whose mount already fetched `/api/auth/me`, + // and every account-derived branch below — the BMI chart's `heightCm` gate, + // the personalised weight bands, the module gates, the unit labels — would + // otherwise render one thing on the server and another here. With the RSC + // prefetch on, that costs the whole streamed dashboard: React #418, tree + // discarded, the prefetch paid for and thrown away. See + // `useAccountOnceMounted`. + const user = useAccountOnceMounted(); const { t } = useTranslations(); const fmt = useFormatters(); const queryClient = useQueryClient(); @@ -235,7 +245,7 @@ export default function DashboardPageClient({ // weight chart. `td` converts an ABSOLUTE reading (affine, so a temperature // picks up the °F offset); `tdd` converts a DELTA / slope (factor only, so a // Δ never inherits the offset). Metric users take the identity path. - const unitDisplay = useUnitDisplay(); + const unitDisplay = useUnitDisplayOnceMounted(); const td = (type: string, value: number | null | undefined): number | null => value == null ? null : unitDisplay.toDisplay(type, value); const tdd = ( @@ -302,8 +312,14 @@ export default function DashboardPageClient({ // its own cached route. Gated on the `insights` module (the digest is // the AI-narrative daily layer; the route 403s when insights is off) // and the auth state so a logged-out / gated account never fires it. - const insightsEnabled = useModuleEnabled("insights"); - const digestQuery = useDailyDigest(isAuthenticated && insightsEnabled); + // Two readings of one flag, on purpose. The FETCH gate takes the live + // module map, so an account with insights off never fires the request. The + // RENDER gate below takes the mount-pinned account, so the hydration render + // agrees with the server (which had no account, and therefore the default-on + // answer). See `useAccountOnceMounted`. + const insightsModuleEnabled = useModuleEnabled("insights"); + const insightsEnabled = user?.modules?.insights !== false; + const digestQuery = useDailyDigest(isAuthenticated && insightsModuleEnabled); // v1.29.1 — the v1.29.0 selected-score-ring cluster is removed from the web // hero (Marc, live-use: uneven, wasted tile space). The snapshot still diff --git a/src/components/dashboard/__tests__/looking-after-card.test.tsx b/src/components/dashboard/__tests__/looking-after-card.test.tsx index f39f21da7..ebf1a74fe 100644 --- a/src/components/dashboard/__tests__/looking-after-card.test.tsx +++ b/src/components/dashboard/__tests__/looking-after-card.test.tsx @@ -39,21 +39,28 @@ const mockAccessRef: { value: AccountAccess } = { value: { accounts: [], active: null, canSwitch: false }, }; +const mockAccount = () => ({ + id: "delegate", + username: "delegate", + email: null, + role: "USER", + avatarUrl: null, + modules: {}, + accountAccess: mockAccessRef.value, +}); + vi.mock("@/hooks/use-auth", () => ({ useAuth: () => ({ - user: { - id: "delegate", - username: "delegate", - email: null, - role: "USER", - avatarUrl: null, - modules: {}, - accountAccess: mockAccessRef.value, - }, + user: mockAccount(), isAuthenticated: true, isLoading: false, refetch: vi.fn(), }), + // The card reads the mount-pinned accessor: it paints inside the dashboard's + // streamed boundary, where reading the grants on the hydration render would + // disagree with the server and cost the whole streamed tree. These cases are + // about the MOUNTED render, so the accessor answers the account. + useAccountOnceMounted: () => mockAccount(), })); vi.mock("@/hooks/use-account-switch", () => ({ diff --git a/src/components/dashboard/looking-after-card.tsx b/src/components/dashboard/looking-after-card.tsx index 8e0fce258..704e69433 100644 --- a/src/components/dashboard/looking-after-card.tsx +++ b/src/components/dashboard/looking-after-card.tsx @@ -5,7 +5,7 @@ import { HeartHandshake, Loader2 } from "lucide-react"; import { Card, CardContent } from "@/components/ui/card"; import { TileHeader } from "@/components/insights/tile-header"; import { useAccountSwitch } from "@/hooks/use-account-switch"; -import { useAuth } from "@/hooks/use-auth"; +import { useAccountOnceMounted } from "@/hooks/use-auth"; import { useTranslations } from "@/lib/i18n/context"; import { accountLabel, @@ -49,7 +49,12 @@ import { */ export function LookingAfterCard() { const { t } = useTranslations(); - const { user } = useAuth(); + // Withheld until mounted: this card sits inside the dashboard's streamed + // boundary and renders nothing without grants. The shell has usually answered + // `/api/auth/me` before the boundary hydrates, so reading the grants on the + // hydration render paints a card where the server painted none — a mismatch + // that discards the streamed dashboard. See `useAccountOnceMounted`. + const user = useAccountOnceMounted(); const switchAccount = useAccountSwitch(); const access = user?.accountAccess; diff --git a/src/components/dashboard/recent-workouts-tile.tsx b/src/components/dashboard/recent-workouts-tile.tsx index 2eee9a39d..cd4eab1ab 100644 --- a/src/components/dashboard/recent-workouts-tile.tsx +++ b/src/components/dashboard/recent-workouts-tile.tsx @@ -66,7 +66,15 @@ function renderRow(workout: WorkoutListEntry, sportName: string) { export function RecentWorkoutsTile() { const { t, locale } = useTranslations(); - const { data, isLoading, isError, refetch } = useWorkouts({ limit: 3 }); + const { data, isError, refetch } = useWorkouts({ limit: 3 }); + + // A query's `isLoading` is not a hydration-safe branch: TanStack reports the + // optimistic mount fetch on the client's very first render and cannot on the + // server, so an `isLoading` gate renders one thing in the streamed HTML and + // another in the hydration render — React #418, and the whole streamed + // dashboard is discarded. Gate on what both sides can see instead: whether + // the cell holds an answer yet. + const showLoading = !data && !isError; const workouts = data?.workouts ?? []; return ( @@ -95,7 +103,7 @@ export function RecentWorkoutsTile() { } /> - {isLoading ? ( + {showLoading ? ( // v1.4.43 W11-L6 — reserve roughly the loaded tile height // (3 workouts × ~3 rem rows + spacing) so the surrounding // dashboard layout doesn't reflow once the data lands. diff --git a/src/components/gamification/__tests__/recent-achievements-card.test.tsx b/src/components/gamification/__tests__/recent-achievements-card.test.tsx index 038f844c0..66c8f70e2 100644 --- a/src/components/gamification/__tests__/recent-achievements-card.test.tsx +++ b/src/components/gamification/__tests__/recent-achievements-card.test.tsx @@ -18,8 +18,18 @@ let mockUser: { modules?: Record } | null = {}; vi.mock("@/hooks/use-auth", () => ({ useAuth: () => ({ isAuthenticated: true, user: mockUser }), + useAccountOnceMounted: () => mockUser, })); +// The card paints its skeleton until it is mounted, because the achievements +// cell is shared with `` in the app shell: on a +// cold load of the dashboard that cell is often already full by the time this +// card hydrates, where the server had nothing, and a content branch there is a +// hydration mismatch that costs the whole streamed page. These cases are about +// the MOUNTED render, so the flag is stubbed to what it answers from the first +// client re-render on. +vi.mock("@/hooks/use-mounted", () => ({ useMounted: () => true })); + let mockData: | { summary: unknown; diff --git a/src/components/gamification/recent-achievements-card.tsx b/src/components/gamification/recent-achievements-card.tsx index 61321fcd3..3e7214581 100644 --- a/src/components/gamification/recent-achievements-card.tsx +++ b/src/components/gamification/recent-achievements-card.tsx @@ -19,7 +19,8 @@ import { type LucideIcon, } from "lucide-react"; import Link from "next/link"; -import { useAuth } from "@/hooks/use-auth"; +import { useAccountOnceMounted, useAuth } from "@/hooks/use-auth"; +import { useMounted } from "@/hooks/use-mounted"; import { useTranslations } from "@/lib/i18n/context"; import { formatDate } from "@/lib/format"; import { useAchievementsQuery } from "@/lib/queries/use-achievements-query"; @@ -83,7 +84,12 @@ const RECENT_LIMIT = 3; */ export function RecentAchievementsCard() { const { t } = useTranslations(); - const { user, isAuthenticated } = useAuth(); + const { isAuthenticated } = useAuth(); + // Withheld until mounted for the same reason the skeleton gate below is: + // this card paints inside the dashboard's streamed boundary, and a module + // gate that reads the account on the hydration render disagrees with the + // server, which had none. See `useAccountOnceMounted`. + const user = useAccountOnceMounted(); // v1.18.0 — the achievements module gate. When the account has the // module turned off the dashboard tile disappears entirely (the data @@ -102,6 +108,16 @@ export function RecentAchievementsCard() { enabled: isAuthenticated && achievementsEnabled, }); + // The dashboard is a streamed Suspense boundary and hydrates after the app + // shell. `` mounts with the shell and shares this + // exact cache cell, so by the time this card replays its first render on the + // client the payload is often already there — where the server had nothing + // and painted the skeleton. Pinning the loading branch to the mount snapshot + // keeps the hydration render identical to the server's; the unlocks land on + // the re-render right after. See `useMounted`. + const mounted = useMounted(); + const showSkeleton = !mounted || isPending; + const recent = pickRecentUnlocks(data?.achievements ?? [], RECENT_LIMIT); // Module off ⇒ render nothing so the tile vanishes from the dashboard. @@ -132,7 +148,7 @@ export function RecentAchievementsCard() { } /> - {isPending ? ( + {showSkeleton ? (