fix: the theme belongs in a cookie, and the offline page in the shell

next/script rendered the theme script with a nonce, and browsers hide a nonce attribute from the DOM once they have parsed it, so the server sent nonce="" and the client read undefined. Every page load logged a hydration mismatch. The cookie is the fix rather than a workaround for it. The server reads it and stamps data-theme on the html element, so the first frame is already the right colour and there is no inline script at all. Chasing that turned up worse. /offline sat outside [locale], which made it a sibling of the root layout, so Next gave it a generated one: no stylesheet, no font, no theme, and a second html element. The phase 7 check asserted the text and a button and passed while the page was plainly broken. It is a normal screen now. The service worker keeps one offline copy per language the reader actually visits, learned from their own successful navigations, so no list of locales lives in the worker and a third language stays a catalog file. The e2e warm-up also asks for the dynamic routes, which cost a compile of their own and were being paid for by whichever test reached one first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-05 23:04:05 +00:00
parent e4eb1617d1
commit 8c8463924f
11 changed files with 167 additions and 119 deletions
@@ -681,13 +681,37 @@ Sending retries on an interval as well as on the `online` event: a phone walking
 coverage does not reliably fire that event, and a stranded photograph is the one failure
 this feature exists to prevent.

+### The theme is a cookie, not an inline script
+The first attempt read localStorage in an inline script before the first paint. Under a
+nonce policy that script is one React and the browser disagree about: the browser hides a
+nonce attribute from the DOM, so the server rendered `nonce=""` and the client saw
+`undefined`, and every page load logged a hydration mismatch.
+
+The cookie is better than a workaround for that. The server reads it and stamps
+`data-theme` on the html element, so the first frame is already the right colour and there
+is no inline script at all. `system` writes no attribute, so the reader's own setting keeps
+being followed as it changes.
+
+### The offline page lives under `[locale]`
+It was briefly at `/offline`, outside the locale segment, so that the service worker could
+cache exactly one URL. That was wrong twice over: `[locale]/layout.tsx` is the root layout,
+so a sibling route got Next's generated fallback instead, with no stylesheet, no font, no
+theme and a second `<html>` element; and the page guessed its language from the browser
+rather than knowing it.
+
+It is now a normal screen. The worker keeps one copy per language the reader actually
+visits, learned from their own successful navigations, so the fallback matches the page
+they were trying to reach. No list of locales lives in the worker: a third language stays a
+catalog file and nothing else, which is exactly the sort of place a hardcoded list would be
+forgotten.
+
 ### CSP by nonce, styles still inline
 The proxy mints a nonce per request and Next stamps it on the scripts it renders, with
 `'strict-dynamic'` for the chunks they load. `style-src` keeps `'unsafe-inline'`: React
 writes inline `style` attributes for things like a dragged card and there is no way to
 nonce those.

-This forced one change: `/offline` is rendered per request rather than prerendered. A
+This forced one change: the offline page is rendered per request rather than prerendered. A
 prerendered page carries a build-time nonce that no live policy matches, so its scripts
 were blocked and the page rendered without ever hydrating. The service worker caches
 headers along with the body, so the copy it serves offline stays self consistent.
@@ -6,9 +6,10 @@ import { setRequestLocale } from 'next-intl/server';
 import { getT } from '@/i18n/t';
 import { notFound } from 'next/navigation';
 import type { ReactNode } from 'react';
+import { cookies } from 'next/headers';
 import { Providers } from '@/components/providers';
-import { ThemeScript } from '@/components/theme-script';
 import { routing } from '@/i18n/routing';
+import { THEME_COOKIE } from '@/lib/theme';
 import '../globals.css';

 // Self hosted by next/font: no third party font request at runtime, which keeps the
@@ -55,11 +56,18 @@ export default async function LocaleLayout({
  if (!isLocale(locale)) notFound();
  setRequestLocale(locale);

+  // Read here rather than in a script the browser runs: the first frame is already the
+  // right colour, and there is no inline script for a nonce policy to argue with.
+  const stored = (await cookies()).get(THEME_COOKIE)?.value;
+  const theme = stored === 'light' || stored === 'dark' ? stored : undefined;
+
  return (
-    <html lang={locale} className={inter.variable} suppressHydrationWarning>
-      <head>
-        <ThemeScript />
-      </head>
+    <html
+      lang={locale}
+      className={inter.variable}
+      {...(theme ? { 'data-theme': theme } : {})}
+      suppressHydrationWarning
+    >
      <body className="min-h-dvh font-sans antialiased">
        <NextIntlClientProvider>
          <Providers>{children}</Providers>
@@ -0,0 +1,35 @@
+import { setRequestLocale } from 'next-intl/server';
+import { Card } from '@/components/ui/card';
+import { getT } from '@/i18n/t';
+import { RetryButton } from './retry-button';
+
+/**
+ * What the service worker serves when a navigation cannot reach the network.
+ *
+ * It lives under `[locale]` like every other screen, so it inherits the shell: the
+ * stylesheet, the font and the theme the reader chose. The worker caches one copy per
+ * language the reader actually visits, so this is already in their language when it is
+ * served (see src/lib/service-worker.js).
+ *
+ * Rendered per request so it carries a live CSP nonce. Prerendered, its scripts would be
+ * stamped at build time and the policy would block them, leaving a page that renders but
+ * never becomes interactive. The worker caches the response headers along with the body,
+ * so the copy it serves offline stays self consistent.
+ */
+export const dynamic = 'force-dynamic';
+
+export default async function OfflinePage({ params }: { params: Promise<{ locale: string }> }) {
+  const { locale } = await params;
+  setRequestLocale(locale);
+  const t = await getT(locale);
+
+  return (
+    <main className="mx-auto flex min-h-dvh w-full max-w-md items-center px-5">
+      <Card className="w-full space-y-3">
+        <h1 className="text-xl font-semibold tracking-tight">{t('offline.title')}</h1>
+        <p className="text-sm text-[var(--text-muted)] text-pretty">{t('offline.body')}</p>
+        <RetryButton label={t('common.retry')} />
+      </Card>
+    </main>
+  );
+}
@@ -0,0 +1,12 @@
+'use client';
+
+import { Button } from '@/components/ui/button';
+
+/** Reloads whatever the reader was trying to reach, which is the only action here. */
+export function RetryButton({ label }: { label: string }) {
+  return (
+    <Button type="button" block onClick={() => window.location.reload()}>
+      {label}
+    </Button>
+  );
+}
@@ -1,24 +0,0 @@
-'use client';
-
-import { DEFAULT_LOCALE, localeFromAcceptLanguage, t } from '@impuestos/i18n';
-import { Button } from '@/components/ui/button';
-import { Card } from '@/components/ui/card';
-import { useBrowserValue } from '@/lib/browser';
-
-export function OfflineNotice() {
-  // This page has no request locale to read, so it takes the browser's own preference.
-  const locale = useBrowserValue(
-    () => localeFromAcceptLanguage(navigator.language),
-    DEFAULT_LOCALE,
-  );
-
-  return (
-    <Card className="w-full space-y-3">
-      <h1 className="text-xl font-semibold tracking-tight">{t(locale, 'offline.title')}</h1>
-      <p className="text-sm text-[var(--text-muted)] text-pretty">{t(locale, 'offline.body')}</p>
-      <Button type="button" block onClick={() => window.location.reload()}>
-        {t(locale, 'common.retry')}
-      </Button>
-    </Card>
-  );
-}
@@ -1,27 +0,0 @@
-import type { Metadata } from 'next';
-import { OfflineNotice } from './offline-notice';
-
-export const metadata: Metadata = { title: 'Impuestos' };
-
-/**
- * Rendered per request so it carries a live CSP nonce like every other page. Prerendered,
- * its scripts would be stamped at build time and the policy would block them, leaving a
- * page that renders but does not work. The service worker caches the response headers
- * along with the body, so the copy it serves offline stays self consistent.
- */
-export const dynamic = 'force-dynamic';
-
-/**
- * What the service worker serves when a navigation cannot reach the network.
- *
- * It lives outside `[locale]` on purpose: the worker caches exactly one URL, and a page
- * that only exists per locale would mean caching one and showing it to everyone. The
- * notice picks its language in the browser instead, from the two catalogs we ship.
- */
-export default function OfflinePage() {
-  return (
-    <main className="mx-auto flex min-h-dvh w-full max-w-md items-center px-5">
-      <OfflineNotice />
-    </main>
-  );
-}
@@ -1,5 +1,5 @@
 import createMiddleware from 'next-intl/middleware';
-import { type NextRequest, NextResponse } from 'next/server';
+import type { NextRequest } from 'next/server';
 import { routing } from './src/i18n/routing';

 const intl = createMiddleware(routing);
@@ -53,14 +53,7 @@ export default function proxy(request: NextRequest) {
  request.headers.set('x-nonce', nonce);
  request.headers.set('content-security-policy', csp);

-  /**
-   * `/offline` is one page for every language: the service worker caches exactly one URL,
-   * so locale routing must leave it alone. It still gets the headers.
-   */
-  const response =
-    request.nextUrl.pathname === '/offline'
-      ? NextResponse.next({ request: { headers: request.headers } })
-      : intl(request);
+  const response = intl(request);

  response.headers.set('content-security-policy', csp);
  for (const [key, value] of Object.entries(STATIC_HEADERS)) response.headers.set(key, value);
@@ -1,19 +0,0 @@
-import Script from 'next/script';
-
-/**
- * Applies the stored theme before the first paint.
- *
- * It has to be inline and it has to run before anything renders: otherwise a reader who
- * chose dark gets a white flash on every navigation. `beforeInteractive` is how the App
- * Router says exactly that. A failure to read storage leaves the system preference in
- * charge, which is the right default anyway.
- */
-const SCRIPT = `try{var t=localStorage.getItem('impuestos-theme');if(t==='dark'||t==='light')document.documentElement.setAttribute('data-theme',t)}catch(e){}`;
-
-export function ThemeScript() {
-  return (
-    <Script id="theme" strategy="beforeInteractive">
-      {SCRIPT}
-    </Script>
-  );
-}
@@ -2,7 +2,7 @@
 * The service worker. Three jobs and no more:
 *
 *   1. show a push notification and open the screen it points at,
- *   2. keep the app shell reachable when the network is not,
+ *   2. keep an explanation reachable when the network is not,
 *   3. get out of the way of everything else.
 *
 * There is no caching of API responses. Tax figures that are quietly out of date are worse
@@ -10,20 +10,15 @@
 * says so.
 */

-const CACHE = 'impuestos-shell-v1';
-const OFFLINE_URL = '/offline';
-
-const PRECACHE = [OFFLINE_URL, '/icon-192.png'];
+const CACHE = 'impuestos-shell-v2';
+const ICON = '/icon-192.png';

 self.addEventListener('install', (event) => {
  event.waitUntil(
    caches
      .open(CACHE)
-      // One at a time rather than addAll: a single miss must not throw away the install
-      // and leave the offline page uncached along with it.
-      .then((cache) =>
-        Promise.all(PRECACHE.map((url) => cache.add(url).catch(() => undefined))),
-      )
+      // A miss must not throw away the install along with it.
+      .then((cache) => cache.add(ICON).catch(() => undefined))
      .then(() => self.skipWaiting()),
  );
 });
@@ -37,6 +32,45 @@ self.addEventListener('activate', (event) => {
  );
 });

+/** The locale is the first path segment, which is where the router always puts it. */
+function localeOf(url) {
+  const segment = new URL(url).pathname.split('/')[1];
+  return segment && /^[a-z]{2}$/.test(segment) ? segment : null;
+}
+
+function offlineUrlFor(locale) {
+  return `/${locale}/offline`;
+}
+
+/**
+ * Keeps one offline page per language the reader actually visits.
+ *
+ * No list of locales lives here on purpose: adding a third language is a catalog file and
+ * nothing else, and a hardcoded list in a service worker is exactly the sort of place a new
+ * locale would be forgotten.
+ */
+async function rememberOfflinePage(locale) {
+  const url = offlineUrlFor(locale);
+  const cache = await caches.open(CACHE);
+  if (await cache.match(url)) return;
+  await cache.add(url).catch(() => undefined);
+}
+
+async function offlineResponse(requestUrl) {
+  const cache = await caches.open(CACHE);
+  const locale = localeOf(requestUrl);
+
+  if (locale) {
+    const exact = await cache.match(offlineUrlFor(locale));
+    if (exact) return exact;
+  }
+
+  // Some language is better than a browser error page.
+  const keys = await cache.keys();
+  const anyOffline = keys.find((request) => request.url.endsWith('/offline'));
+  return anyOffline ? cache.match(anyOffline) : Response.error();
+}
+
 /**
 * Navigations only. A page the network cannot deliver falls back to the offline screen,
 * which explains itself and offers a retry. Everything else goes straight to the network.
@@ -44,11 +78,13 @@ self.addEventListener('activate', (event) => {
 self.addEventListener('fetch', (event) => {
  if (event.request.mode !== 'navigate') return;

+  // Scheduled before responding, not inside the response's promise chain: waitUntil has to
+  // be called while the event is still being dispatched or the work is never kept alive.
+  const locale = localeOf(event.request.url);
+  if (locale) event.waitUntil(rememberOfflinePage(locale));
+
  event.respondWith(
-    fetch(event.request).catch(async () => {
-      const cached = await caches.match(OFFLINE_URL);
-      return cached ?? Response.error();
-    }),
+    fetch(event.request).catch(() => offlineResponse(event.request.url)),
  );
 });

@@ -65,8 +101,8 @@ self.addEventListener('push', (event) => {
  event.waitUntil(
    self.registration.showNotification(payload.title ?? 'Impuestos', {
      body: payload.body ?? '',
-      icon: '/icon-192.png',
-      badge: '/icon-192.png',
+      icon: ICON,
+      badge: ICON,
      // One number and one action (FLOWS.md section 9): the tag collapses a repeat of the
      // same notification rather than stacking it.
      tag: payload.tag ?? 'impuestos',
@@ -1,31 +1,36 @@
-'use client';
-
+/*
+ * No 'use client' directive: this module holds constants and two functions that touch
+ * `document` when called. The cookie name is read by the server layout, and marking the
+ * module client-only would drag it across the boundary for the sake of a string.
+ */
 export const THEMES = ['system', 'light', 'dark'] as const;
 export type Theme = (typeof THEMES)[number];

-export const THEME_KEY = 'impuestos-theme';
+export const THEME_COOKIE = 'impuestos-theme';
+const ONE_YEAR_SECONDS = 60 * 60 * 24 * 365;

 /**
- * Applied by writing `data-theme` on the document element, which the CSS in globals.css
- * treats as beating the system preference in both directions. `system` removes the
- * attribute rather than picking a side, so the reader's own setting is followed as it
- * changes.
+ * The choice lives in a cookie, not in localStorage, so the server can read it and stamp
+ * `data-theme` on the html element before anything is painted. localStorage would mean an
+ * inline script that reads it, which is a white flash on the first frame at best and, under
+ * a nonce policy, a script React and the browser disagree about at worst.
+ *
+ * `system` clears the attribute rather than picking a side, so the reader's own setting is
+ * followed as it changes.
 */
 export function applyTheme(theme: Theme): void {
  if (theme === 'system') document.documentElement.removeAttribute('data-theme');
  else document.documentElement.setAttribute('data-theme', theme);
-  try {
-    window.localStorage.setItem(THEME_KEY, theme);
-  } catch {
-    // A browser with storage blocked keeps the choice for this page and no longer.
-  }
+
+  document.cookie = `${THEME_COOKIE}=${theme}; path=/; max-age=${ONE_YEAR_SECONDS}; samesite=lax`;
+}
+
+export function isTheme(value: string | undefined): value is Theme {
+  return THEMES.includes(value as Theme);
 }

 export function storedTheme(): Theme {
-  try {
-    const value = window.localStorage.getItem(THEME_KEY);
-    return THEMES.includes(value as Theme) ? (value as Theme) : 'system';
-  } catch {
-    return 'system';
-  }
+  const match = document.cookie.match(new RegExp(`(?:^|; )${THEME_COOKIE}=([^;]*)`));
+  const value = match?.[1];
+  return isTheme(value) ? value : 'system';
 }
@@ -28,6 +28,11 @@ const ROUTES = [
  '/es/errores',
  '/es/auditoria',
  '/en/login',
+  // The dynamic routes cost a compile too. The id does not have to exist: reaching the
+  // route is what compiles it, and what the first test would otherwise pay for.
+  '/es/comprobantes/warmup',
+  '/es/declaraciones/warmup',
+  '/es/usuarios/warmup',
 ];

 export default async function globalSetup(): Promise<void> {