From c12a6873b37bc1c8060debd355805293c62cf7e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 2 Aug 2026 18:04:07 +0200 Subject: [PATCH] Hoist the shell banner stack above the sidebar and content row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The offline, shared-record, demo and maintainership strips lived inside the content column, so each one pushed the top bar down while the sidebar's logo band stayed at the top of the window. Both bands read SHELL_HEADER_BAND and are therefore the same height with the same border, and the single line they draw across the top of the app still came apart: measured on the French maintainership strip, the two borders sat 66.5px apart at 768px and 61.0px apart at 1280px, and the full three-banner stack put them 184.5px apart. After the hoist the step is 0.00px at every depth and both widths. A banner is a statement about the whole window, not about the content column, so it belongs above the row. The outer wrapper keeps the h-dvh and the row below takes what the banners leave, which means the stack comes out of the viewport budget instead of being added on top of it: the document still does not scroll at any depth, and
stays the one vertical scroller at both breakpoints. The existing seam guard could not see this class. It asserts that both bands carry the shared band tokens, which stayed true the whole time, and the e2e spec beside it measures a shell with nothing above the header. The new spec forces the banners on through the conditions the components actually read — an unmaintained locale, a patched account payload, a real offline context — and measures the same two borders at four stack depths. Reverting the layout change turns it red on the first banner with the measured offset in the message, while the older seam spec stays green. The maintainership strip gains a data-slot so the shell's chrome can be enumerated from one attribute. --- e2e/chrome-header-seam-banners.spec.ts | 422 ++++++++++++++++++ src/components/i18n/maintainership-banner.tsx | 1 + src/components/layout/auth-shell.tsx | 162 ++++--- 3 files changed, 516 insertions(+), 69 deletions(-) create mode 100644 e2e/chrome-header-seam-banners.spec.ts diff --git a/e2e/chrome-header-seam-banners.spec.ts b/e2e/chrome-header-seam-banners.spec.ts new file mode 100644 index 000000000..a40089772 --- /dev/null +++ b/e2e/chrome-header-seam-banners.spec.ts @@ -0,0 +1,422 @@ +import type { Page } from "@playwright/test"; + +import { expect, test } from "./setup/test"; + +import { STORAGE_STATE_PATH } from "./setup/global-setup"; + +/** + * The seam again, this time with chrome above it. + * + * `chrome-header-seam.spec.ts` measures the two header borders on a shell with + * nothing above them. That is the state almost every session is in, and it is + * also the only state in which the seam cannot break: with no banner painted, + * the sidebar logo band and the top bar both start at y=0 whatever the layout + * does with them. + * + * The banners are the case that breaks it. Each one is a full-width strip that + * paints above the top bar, and while they lived INSIDE the content column they + * pushed the top bar down while the sidebar band stayed where it was — one line + * across the top of the app became two lines a banner-height apart, and three + * can stack at once. Measured before the fix, on the French locale strip: one + * banner offset the two borders by 66.5px at 768px and 61.0px at 1280px, and + * all three together by 184.5px and 143.0px. So this spec forces them on and + * measures the same two borders. + * + * ## How each banner is forced + * + * Honestly, through the condition the component actually reads — no injected + * DOM, no test-only prop: + * + * - **Maintainership** paints on a locale HealthLog does not hand-maintain. + * The context cookie is switched from `en` to `fr`, which is what a + * French-speaking user's browser carries. The swap happens with no page + * alive (`about:blank`): `I18nProvider` rewrites `healthlog-locale` from + * its own active locale on mount, so a cookie written under a live English + * page is reverted within the second. + * - **Shared record** paints from `accountAccess.active` on `/api/auth/me`. + * The real payload is captured first and served back with that block + * filled, rather than performing a real switch: the switch is stamped on + * the SESSION ROW, so switching the shared cookie jar would switch it for + * every other spec holding the same cookie (see + * `DELEGATE_STORAGE_STATE_PATH` in `global-setup.ts` for what that costs). + * A captured-and-patched payload keeps this file's reach inside this file, + * and it keeps working after the offline stage because it never touches the + * network again. + * - **Offline** paints while `navigator.onLine` is false, which + * `context.setOffline(true)` produces for real, event and all. It is armed + * last because it is the only stage a navigation cannot follow. + * + * The demo banner is the one that cannot be forced from a spec: `DEMO_MODE` is + * a server env var read by the root layout, and the suite shares one server + * whose mutations would all be blocked by turning it on. It is the same strip + * as the other three (`border-b px-3 py-2 text-xs`) in the same place in the + * stack, so the three forced here exercise its geometry; nothing in the stack + * is specific to which banner is in it. + * + * ## What is asserted + * + * Two properties, and the second is why the banners were hoisted above the + * shell row rather than nudged inside it: + * + * 1. The seam holds — the two header borders keep sharing one line at every + * stack depth, so chrome above the header cannot displace one band and + * not the other. + * 2. The shell stays inside the viewport — the banners are part of the + * `h-dvh` budget, not added on top of it, so no depth of stack introduces + * a page-level scrollbar. `
` remains the only vertical scroller. + * + * Property 2 runs on mobile too, where there is no sidebar and therefore no + * seam, because the scroll model is the half of the layout that the hoist could + * have broken at that breakpoint. + */ + +const LOCALE_COOKIE = "healthlog-locale"; + +/** + * Every banner that can paint above the top bar, so a stage can assert how many + * are up rather than only that the one it armed is. + */ +const BANNER_SLOTS = [ + "offline-banner", + "shared-record-banner", + "demo-banner", + "maintainership-banner", +] as const; + +type Band = { bottom: number; height: number; borderBottomWidth: number }; + +type ShellGeometry = { + topBar: Band | null; + sidebar: Band | null; + bannerCount: number; + bannerHeight: number; + /** `scrollHeight - clientHeight` of the document's scrolling element. */ + documentOverflow: number; + viewportHeight: number; +}; + +async function readShell(page: Page): Promise { + return page.evaluate( + (slots: string[]) => { + const band = (slot: string) => { + const el = document.querySelector(`[data-slot="${slot}"]`); + if (!el) return null; + const rect = el.getBoundingClientRect(); + return { + bottom: rect.bottom, + height: rect.height, + borderBottomWidth: parseFloat( + getComputedStyle(el).borderBottomWidth || "0", + ), + }; + }; + const banners = slots + .map((slot) => document.querySelector(`[data-slot="${slot}"]`)) + .filter((el): el is Element => el != null); + const scroller = document.scrollingElement ?? document.documentElement; + return { + topBar: band("top-bar"), + sidebar: band("sidebar-header"), + bannerCount: banners.length, + bannerHeight: banners.reduce( + (sum, el) => sum + el.getBoundingClientRect().height, + 0, + ), + documentOverflow: scroller.scrollHeight - scroller.clientHeight, + viewportHeight: window.innerHeight, + }; + }, + BANNER_SLOTS as unknown as string[], + ); +} + +/** + * The live `/api/auth/me` body, read through the page so it carries the page's + * own cookies. Captured before anything else is armed, so the mock that later + * replaces it differs from the real payload in exactly one block. + */ +async function captureAccountPayload(page: Page): Promise { + const raw = await page.evaluate(async () => { + const res = await fetch("/api/auth/me", { credentials: "same-origin" }); + return res.ok ? await res.text() : null; + }); + expect( + raw, + "could not capture /api/auth/me for the sharing mock", + ).not.toBeNull(); + return raw!; +} + +/** + * Serve the captured payload back with the sharing block filled in, so the + * shell believes this session is inside somebody else's record. Everything else + * the shell reads (modules, role, onboarding flags) stays what the server said. + */ +async function armSharedRecord(page: Page, captured: string): Promise { + const payload = JSON.parse(captured) as { data: Record }; + const entry = { + accountId: "seam-fixture-account", + username: "seam-owner", + displayName: null, + access: "read" as const, + canWrite: false, + }; + payload.data.accountAccess = { + accounts: [entry], + active: entry, + canSwitch: true, + }; + const body = JSON.stringify(payload); + await page.route("**/api/auth/me", (route) => + route.fulfill({ status: 200, contentType: "application/json", body }), + ); +} + +/** + * Swap the context's pinned locale for one HealthLog ships as an AI-initial + * translation, so the maintainership strip has a reason to paint. + * + * The page is parked on `about:blank` for the swap. `I18nProvider` mirrors its + * ACTIVE locale into this cookie from an effect, so a cookie written while an + * English page is mounted is reverted before the next navigation can read it — + * measured, not assumed: the value flipped back inside one second. With no page + * running, the next navigation sends `fr` and the provider then mirrors `fr`. + */ +async function armUnmaintainedLocale(page: Page): Promise { + const host = new URL(page.url()).hostname; + await page.goto("about:blank"); + await page.context().addCookies([ + { + name: LOCALE_COOKIE, + value: "fr", + domain: host, + path: "/", + expires: Math.floor(Date.now() / 1000) + 3600, + httpOnly: false, + secure: false, + sameSite: "Lax", + }, + ]); +} + +/** + * Take the context offline and make sure the banner hears about it. + * + * `setOffline` produces the real condition — `navigator.onLine` flips to false, + * which is asserted here rather than assumed. Delivery of the `offline` EVENT + * to an already-loaded page is the part that is not reliable: this spec caught + * it missing once under a fully parallel run while passing three for three on + * its own. `` only reads `navigator.onLine` at mount and + * otherwise waits on the event, so the event is re-dispatched here. That is the + * notification, not the condition, and the condition is verified first. + */ +async function armOffline(page: Page): Promise { + // Let whatever is in flight land first. The Playwright context runs with + // `serviceWorkers: "block"`, so nothing has pre-cached the app shell and a + // lazily-imported chunk cut off mid-download throws the whole tree to the + // global error boundary — "Failed to load chunk … from module …", no top bar, + // no banner. That is a property of the harness rather than of the shell (a + // real install serves those chunks from the worker cache), so it is waited + // out rather than asserted on. Best-effort: a polling surface can keep the + // network from ever going fully idle. + await page.waitForLoadState("networkidle", { timeout: 5000 }).catch(() => {}); + await page.context().setOffline(true); + await expect.poll(() => page.evaluate(() => navigator.onLine)).toBe(false); + await page.evaluate(() => window.dispatchEvent(new Event("offline"))); +} + +test.describe("app-chrome header seam under banners", () => { + test.use({ storageState: STORAGE_STATE_PATH }); + + test("the header seam survives every depth of the banner stack", async ({ + page, + }) => { + test.skip( + test.info().project.name === "chromium-mobile", + "the sidebar is desktop-only (md+), so below md there is no seam to measure", + ); + // Four stages measured at two widths, with a full page load per stage and + // a network settle either side of the offline one. Comfortably inside the + // suite default when it runs alone; the default 30s is not enough headroom + // for it on a loaded machine. + test.setTimeout(60_000); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + + const topBar = page.locator('[data-slot="top-bar"]'); + const sidebarHeader = page.locator('[data-slot="sidebar-header"]'); + await expect(topBar).toBeVisible(); + await expect(sidebarHeader).toBeVisible(); + + const captured = await captureAccountPayload(page); + + // Each stage adds one banner and leaves the previous ones up, so the last + // stage measures the whole stack rather than one banner at a time. + // + // `beforeResize` / `afterResize` exist for the offline stage alone: a + // viewport change can pull a chunk the page has not loaded yet, and a chunk + // fetch that fails with no network takes the whole shell down to the global + // error boundary. Observed once under `--repeat-each=3`, as a missing top + // bar and a "Failed to load chunk" screen. So the network comes back for + // the resize and the stage re-arms after it — the measurement is still + // taken offline, with the banner up. + type Stage = { + label: string; + arm: () => Promise; + beforeResize?: () => Promise; + afterResize?: () => Promise; + }; + const stages: Stage[] = [ + { label: "no banner", arm: async () => {} }, + { + label: "1 banner (maintainership)", + arm: async () => { + await armUnmaintainedLocale(page); + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect( + page.locator('[data-slot="maintainership-banner"]'), + ).toBeVisible(); + }, + }, + { + label: "2 banners (+ shared record)", + arm: async () => { + await armSharedRecord(page, captured); + await page.reload({ waitUntil: "domcontentloaded" }); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toBeVisible(); + }, + }, + { + label: "3 banners (+ offline)", + arm: async () => {}, + beforeResize: async () => { + await page.context().setOffline(false); + await expect + .poll(() => page.evaluate(() => navigator.onLine)) + .toBe(true); + }, + afterResize: async () => { + await armOffline(page); + await expect( + page.locator('[data-slot="offline-banner"]'), + ).toBeVisible(); + }, + }, + ]; + + // Stage outside, width inside: each banner is armed once and the resulting + // stack measured at both widths. 768px is the `md` breakpoint where the + // sidebar first appears — the narrowest shell that has a seam at all, and + // the width where a banner is most likely to wrap onto a second line. + for (const [index, stage] of stages.entries()) { + await stage.arm(); + + for (const width of [768, 1280] as const) { + await stage.beforeResize?.(); + await page.setViewportSize({ width, height: 900 }); + await expect + .poll(() => page.evaluate(() => window.innerWidth)) + .toBe(width); + await stage.afterResize?.(); + await expect(topBar).toBeVisible(); + await expect(sidebarHeader).toBeVisible(); + + const shell = await readShell(page); + const at = `at ${width}px, ${stage.label}`; + + expect(shell.topBar, `top bar missing ${at}`).not.toBeNull(); + expect(shell.sidebar, `sidebar band missing ${at}`).not.toBeNull(); + const top = shell.topBar!; + const side = shell.sidebar!; + + // Two borders that do not exist also "line up". + expect( + top.borderBottomWidth, + `top bar has no bottom border ${at}`, + ).toBeGreaterThan(0); + expect( + side.borderBottomWidth, + `sidebar band has no bottom border ${at}`, + ).toBeGreaterThan(0); + + // The stage armed what it said it armed. Without this a change that + // stopped every banner from painting would make the seam assertion + // below trivially true. + expect( + shell.bannerCount, + `${at}: expected ${index} banner(s) painted, found ${shell.bannerCount}`, + ).toBe(index); + if (index > 0) { + expect( + shell.bannerHeight, + `${at}: the banners paint no height, so this stage proves nothing`, + ).toBeGreaterThan(0); + } + + // The seam. A banner that displaces one band and not the other shows + // up here as a step the size of the stack. + expect( + Math.abs(side.bottom - top.bottom), + `${at}: header seam steps by ${(side.bottom - top.bottom).toFixed( + 2, + )}px — the banner stack (${shell.bannerHeight.toFixed( + 2, + )}px) is displacing one band and not the other`, + ).toBeLessThan(1); + + // The banners come out of the viewport budget, not on top of it. + expect( + shell.documentOverflow, + `${at}: the document scrolls by ${shell.documentOverflow.toFixed( + 2, + )}px — the banner stack pushed the shell past the viewport`, + ).toBeLessThan(1); + } + } + }); + + test("the banner stack never pushes the shell past the viewport", async ({ + page, + }) => { + // Mobile as well as desktop. Below `md` the shell stacks in a column and + // there is no sidebar, so there is no seam to measure — but the scroll + // model is what hoisting the banners could have broken there, and that + // half is measurable at both breakpoints. + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect(page.locator('[data-slot="top-bar"]')).toBeVisible(); + + const captured = await captureAccountPayload(page); + await armSharedRecord(page, captured); + await armUnmaintainedLocale(page); + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect( + page.locator('[data-slot="maintainership-banner"]'), + ).toBeVisible(); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toBeVisible(); + await armOffline(page); + await expect(page.locator('[data-slot="offline-banner"]')).toBeVisible(); + + const shell = await readShell(page); + expect(shell.bannerCount, "the stack did not arm").toBe(3); + expect(shell.bannerHeight).toBeGreaterThan(0); + expect( + shell.documentOverflow, + `the document scrolls by ${shell.documentOverflow.toFixed( + 2, + )}px under a ${shell.bannerHeight.toFixed(2)}px banner stack`, + ).toBeLessThan(1); + + // The top bar is still fully painted below the stack rather than pushed + // off the bottom of the viewport. + const top = shell.topBar!; + expect(top.height).toBeGreaterThan(32); + expect( + top.bottom, + "the banner stack pushed the top bar out of the viewport", + ).toBeLessThanOrEqual(shell.viewportHeight); + }); +}); diff --git a/src/components/i18n/maintainership-banner.tsx b/src/components/i18n/maintainership-banner.tsx index f1ce67ed1..9e092a944 100644 --- a/src/components/i18n/maintainership-banner.tsx +++ b/src/components/i18n/maintainership-banner.tsx @@ -93,6 +93,7 @@ export function MaintainershipBanner() { return (
diff --git a/src/components/layout/auth-shell.tsx b/src/components/layout/auth-shell.tsx index 158eb17e1..01f10881a 100644 --- a/src/components/layout/auth-shell.tsx +++ b/src/components/layout/auth-shell.tsx @@ -297,67 +297,90 @@ export function AuthShell({ {t("nav.skipToContent")} -
- - {/* `min-w-0` lets the content column shrink below its children's - intrinsic min width (e.g. the measurements table); without it - the column widens past the viewport next to the sidebar and - the whole page gains a horizontal scrollbar. Wide children - scroll inside their own `overflow-x-auto` containers. */} -
- {/* - v1.4.43 QoL (M5) — `` paints only when - `navigator.onLine === false`. Sits above the maintainership - banner + top bar so the connection-status hint is always - the first chrome line a user sees in the offline branch. - */} - - {/* v1.36.0 — whose record is open. Sits directly under the - connection strip and above everything else: a person who has - forgotten they are switched is about to write a reading into - somebody else's record, and no other line of chrome is worth - seeing first. Renders nothing when the session is in its own - account, which is every session that has never used sharing. */} - - {/* Demo instances surface a persistent "changes are not - saved" strip — the proxy blocks every mutation, and - without the banner that block only surfaced as a raw API - error after the user already filled in a form. */} - {demoMode ? : null} - - -
` (UI-STANDARDS §9 one-scroll-floor). - // The class is not hypothetical and not confined to app code. A - // Radix `` outside a `
` renders a hidden - // `position:absolute` bubble input on the first pass and drops it - // only once the button ref resolves, so every route carrying a - // switch below the fold (/settings/modules is the long one) grew - // a full-height document scrollbar for the entire pre-hydration - // window. Anchoring the scroll container closes the escape route - // for all of them at once instead of asking each new - // absolutely-positioned child to remember its own wrapper. - className="relative flex-1 [scrollbar-gutter:stable] overflow-y-auto pb-[calc(4rem+env(safe-area-inset-bottom,0px))] md:pb-0" - > - {/* + {/* + The banner stack is PAGE-level, above the sidebar/content row, and that + placement is the whole point rather than a detail of the markup. A + banner is a strip about the whole window — the connection is down, this + is somebody else's record, this instance saves nothing — and none of + those statements is about the content column. + + Inside the column, which is where they lived until v1.36.x, each one + pushed the top bar down while the sidebar's logo band stayed at the + top. Both bands read `SHELL_HEADER_BAND` so they are the same height + with the same border, and the line they draw together still broke: with + the French maintainership strip up, the two borders sat 66.5px apart at + 768px and 61px apart at 1280px, and the full three-banner stack put them + 184.5px apart. Above the row, every banner displaces both bands by the + same amount and the seam cannot come apart however many paint. + + The stack comes OUT of the viewport budget rather than being added on + top of it: the outer wrapper owns the `h-dvh`, the row below takes what + the banners leave (`min-h-0 flex-1`), and `
` stays the one + vertical scroller. Measured in `e2e/chrome-header-seam-banners.spec.ts` + at both breakpoints. + */} +
+ {/* + v1.4.43 QoL (M5) — `` paints only when + `navigator.onLine === false`. Sits above the maintainership + banner + top bar so the connection-status hint is always + the first chrome line a user sees in the offline branch. + */} + + {/* v1.36.0 — whose record is open. Sits directly under the + connection strip and above everything else: a person who has + forgotten they are switched is about to write a reading into + somebody else's record, and no other line of chrome is worth + seeing first. Renders nothing when the session is in its own + account, which is every session that has never used sharing. */} + + {/* Demo instances surface a persistent "changes are not + saved" strip — the proxy blocks every mutation, and + without the banner that block only surfaced as a raw API + error after the user already filled in a form. */} + {demoMode ? : null} + +
+ + {/* `min-w-0` lets the content column shrink below its children's + intrinsic min width (e.g. the measurements table); without it + the column widens past the viewport next to the sidebar and + the whole page gains a horizontal scrollbar. Wide children + scroll inside their own `overflow-x-auto` containers. */} +
+ +
` (UI-STANDARDS §9 one-scroll-floor). + // The class is not hypothetical and not confined to app code. A + // Radix `` outside a `` renders a hidden + // `position:absolute` bubble input on the first pass and drops it + // only once the button ref resolves, so every route carrying a + // switch below the fold (/settings/modules is the long one) grew + // a full-height document scrollbar for the entire pre-hydration + // window. Anchoring the scroll container closes the escape route + // for all of them at once instead of asking each new + // absolutely-positioned child to remember its own wrapper. + className="relative flex-1 [scrollbar-gutter:stable] overflow-y-auto pb-[calc(4rem+env(safe-area-inset-bottom,0px))] md:pb-0" + > + {/* v1.4.33 IW9 — container normalised on `max-w-screen-xl` (1280 px) so dashboard / settings / admin all hit the same content frame. Pre-v1.4.33 the dashboard @@ -371,15 +394,16 @@ export function AuthShell({ the desktop bottom-6 anchor), so the last line of content can always scroll out from under the floating button. */} -
- {outsideSharedRecord ? : children} -
-
+
+ {outsideSharedRecord ? : children} +
+
+
+
-
{/* v1.18.6 — the module-tour launcher lives at the shell level so its