From 2e150e97cf401c2d392fef8c9896dec4efce9c74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 4 Oct 2026 15:08:42 +0200 Subject: [PATCH 1/7] fix(nav): stop the sidebar flashing scrollbars while it expands The rail animates its width from w-16 to w-64, and the labels laid out against the growing width: every row overran the nav and multi-word labels wrapped, so the nav's overflow-y-auto painted a vertical and a horizontal scrollbar for the length of the transition. Lay the expanded column out at its final width from the first frame and let the rail clip it. The final layout is unchanged; the column fills the expanded rail exactly. A unit test pins the structure and an e2e spec samples the nav on every frame of a real expand and collapse. --- e2e/sidebar-expand-no-scrollbar.spec.ts | 89 +++++++ .../layout/__tests__/sidebar-nav.test.tsx | 61 +++++ src/components/layout/sidebar-nav.tsx | 222 ++++++++++-------- 3 files changed, 275 insertions(+), 97 deletions(-) create mode 100644 e2e/sidebar-expand-no-scrollbar.spec.ts diff --git a/e2e/sidebar-expand-no-scrollbar.spec.ts b/e2e/sidebar-expand-no-scrollbar.spec.ts new file mode 100644 index 000000000..c8bfe58df --- /dev/null +++ b/e2e/sidebar-expand-no-scrollbar.spec.ts @@ -0,0 +1,89 @@ +import { expect, test } from "./setup/test"; + +import { STORAGE_STATE_PATH } from "./setup/global-setup"; + +/** + * Expanding the sidebar must never flash a scrollbar on the nav list. + * + * The rail animates `width` from `w-16` to `w-64`. When the labels laid out + * against the growing width, every row overran the 48 px nav and the + * multi-word labels wrapped, so the nav's `overflow-y-auto` painted a + * vertical and a horizontal scrollbar until the transition ended. The fix + * lays the column out at its final width from the first frame and lets the + * rail clip it (see `sidebar-column` in `src/components/layout/sidebar-nav.tsx`). + * + * This samples the nav on every animation frame of a real expand and a real + * collapse, so it fails on the overflow itself rather than on a class name. + * Desktop only: the sidebar is hidden below `md`. + */ +test.describe("sidebar width transition", () => { + test.use({ storageState: STORAGE_STATE_PATH }); + + test.beforeEach(({}, testInfo) => { + test.skip( + testInfo.project.name === "chromium-mobile", + "global sidebar is desktop-only (md+)", + ); + }); + + test("the nav list never overflows while the rail expands or collapses", async ({ + page, + }) => { + await page.goto("/", { waitUntil: "domcontentloaded" }); + const toggle = page.locator('[data-slot="sidebar-collapse-toggle"]'); + await expect(toggle).toBeVisible(); + const nav = page.locator("aside nav").first(); + await expect(nav).toBeVisible(); + + async function sampleToggle() { + const heightBefore = await nav.evaluate((el) => el.scrollHeight); + await page.evaluate(() => { + const el = document.querySelector("aside nav") as HTMLElement; + const samples: { sw: number; cw: number; sh: number }[] = []; + (window as unknown as { __navSamples: typeof samples }).__navSamples = + samples; + const t0 = performance.now(); + const tick = () => { + samples.push({ + sw: el.scrollWidth, + cw: el.clientWidth, + sh: el.scrollHeight, + }); + if (performance.now() - t0 < 600) requestAnimationFrame(tick); + }; + requestAnimationFrame(tick); + }); + await toggle.click(); + await page.waitForTimeout(700); + const samples = await page.evaluate( + () => + ( + window as unknown as { + __navSamples: { sw: number; cw: number; sh: number }[]; + } + ).__navSamples, + ); + const heightAfter = await nav.evaluate((el) => el.scrollHeight); + // Expanded rows are taller than rail rows, so the list may legitimately + // be either rest height while the width moves, never more. + return { samples, restHeight: Math.max(heightBefore, heightAfter) }; + } + + // Two round trips, so the run covers both directions whichever state + // the stored preference started in. + for (let i = 0; i < 4; i++) { + const { samples, restHeight } = await sampleToggle(); + // A 200 ms transition yields several frames; too few means the + // sampler never ran and the assertions below would prove nothing. + expect(samples.length).toBeGreaterThan(5); + for (const s of samples) { + // No horizontal overflow: no row is wider than the nav. + expect(s.sw).toBeLessThanOrEqual(s.cw + 1); + } + // No vertical growth: no label wraps mid-transition, so the list is + // never taller than at rest and cannot summon a scrollbar. + const tallest = Math.max(...samples.map((s) => s.sh)); + expect(tallest).toBeLessThanOrEqual(restHeight + 1); + } + }); +}); diff --git a/src/components/layout/__tests__/sidebar-nav.test.tsx b/src/components/layout/__tests__/sidebar-nav.test.tsx index a0dd34873..cbc5566a8 100644 --- a/src/components/layout/__tests__/sidebar-nav.test.tsx +++ b/src/components/layout/__tests__/sidebar-nav.test.tsx @@ -57,6 +57,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { I18nProvider } from "@/lib/i18n/context"; import { TooltipProvider } from "@/components/ui/tooltip"; import { + SIDEBAR_EXPANDED_CONTENT_WIDTH, SidebarCollapseToggle, SidebarNav, readSidebarCollapsedPref, @@ -498,3 +499,63 @@ describe(" collapse preference persistence", () => { expect(html.indexOf('href="/admin"')).toBeGreaterThan(toggle); }); }); + +/** + * The rail animates `width` between `w-16` and `w-64`. Expanding used to lay + * the labels out against the growing width: multi-word labels wrapped, the + * rows overran the nav, and its `overflow-y-auto` painted a vertical and a + * horizontal scrollbar for the length of the transition. The content now + * lays out at its final width from the first frame and the rail clips it, so + * nothing inside can overflow a scroll container while the width moves. + */ +describe(" width transition never overflows the nav", () => { + const KEY = "healthlog-sidebar-collapsed"; + + afterEach(() => { + vi.unstubAllGlobals(); + mockMountedRef.value = false; + }); + + function asideClass(html: string): string { + return /]*class="([^"]*)"/.exec(html)?.[1] ?? ""; + } + function columnClass(html: string): string { + return ( + /data-slot="sidebar-column"[^>]*class="([^"]*)"|class="([^"]*)"[^>]*data-slot="sidebar-column"/ + .exec(html) + ?.slice(1) + .find(Boolean) ?? "" + ); + } + + it("clips the content to the animating rail", () => { + const aside = asideClass(render()); + expect(aside).toContain("transition-[width]"); + expect(aside.split(" ")).toContain("overflow-hidden"); + }); + + it("lays the expanded column out at the expanded rail's content width", () => { + const column = columnClass(render()); + expect(column.split(" ")).toContain(SIDEBAR_EXPANDED_CONTENT_WIDTH); + // `w-64` minus the rail's 1 px `border-r`: a wider floor would push the + // right padding under the clip, a narrower one would let the labels + // re-flow while the width grows. + expect(SIDEBAR_EXPANDED_CONTENT_WIDTH).toBe("min-w-[calc(16rem-1px)]"); + expect(asideClass(render()).split(" ")).toContain("border-r"); + }); + + it("drops the floor on the collapsed rail, so the icons follow the narrowing width", () => { + const store = new Map([[KEY, "true"]]); + vi.stubGlobal("window", globalThis); + vi.stubGlobal("localStorage", { + getItem: (k: string) => store.get(k) ?? null, + setItem: (k: string, v: string) => void store.set(k, v), + }); + mockMountedRef.value = true; + const html = render(); + expect(asideClass(html).split(" ")).toContain("w-16"); + expect(columnClass(html).split(" ")).not.toContain( + SIDEBAR_EXPANDED_CONTENT_WIDTH, + ); + }); +}); diff --git a/src/components/layout/sidebar-nav.tsx b/src/components/layout/sidebar-nav.tsx index b0c6baae4..1e8fe6969 100644 --- a/src/components/layout/sidebar-nav.tsx +++ b/src/components/layout/sidebar-nav.tsx @@ -54,6 +54,14 @@ import { const SIDEBAR_COLLAPSED_STORAGE_KEY = "healthlog-sidebar-collapsed"; +/** + * The expanded sidebar's content width: `w-64` minus the rail's 1 px + * `border-r`, so the column fills the expanded rail exactly. Applied as a + * floor while expanded, it keeps the content at its final layout during the + * width transition (see the `sidebar-column` comment in the render). + */ +export const SIDEBAR_EXPANDED_CONTENT_WIDTH = "min-w-[calc(16rem-1px)]"; + /** * The stored collapse choice: `true` / `false`, or `null` when this browser * has none (or storage is unavailable), which lets the viewport default decide. @@ -554,119 +562,139 @@ export function SidebarNav() { ); From 1b1da652d3d26964171e214d1dc884269f19f186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 4 Oct 2026 15:13:52 +0200 Subject: [PATCH 2/7] fix(monitoring): route Umami events to the endpoint the tracker was built with The tracker posts to `${data-host-url}${collect path}`, and the collect path is compiled into the operator's Umami build: /api/send by default, anything else when the instance sets COLLECT_API_ENDPOINT. The tag pointed data-host-url at the app origin and the proxy listened only on /api/send, so a renamed endpoint sent every page view and event to an app path that does not exist and got a full 404 page back each time. Point data-host-url at /api/monitoring/umami and answer every path under it with one proxy route. The route reads the collect path from the configured tracker (cached for an hour), forwards only that path to the Umami host, and answers anything else, or an unreadable tracker, with an empty response. The admin test event posts to the same learnt path. The send-URL resolver lives in one module instead of two copies. --- .../openapi-route-coverage-guard.test.ts | 2 +- .../api/admin/monitoring/umami-test/route.ts | 36 ++-- src/app/api/monitoring/umami-script/route.ts | 47 +---- .../umami/[...path]/__tests__/route.test.ts | 166 ++++++++++++++++++ .../api/monitoring/umami/[...path]/route.ts | 107 +++++++++++ src/app/api/send/__tests__/route.test.ts | 103 ----------- src/app/api/send/route.ts | 104 ----------- src/components/monitoring/umami-script.tsx | 12 +- src/lib/monitoring/__tests__/umami.test.ts | 59 +++++++ src/lib/monitoring/umami-paths.ts | 19 ++ src/lib/monitoring/umami.ts | 113 ++++++++++++ src/proxy.ts | 1 - 12 files changed, 494 insertions(+), 275 deletions(-) create mode 100644 src/app/api/monitoring/umami/[...path]/__tests__/route.test.ts create mode 100644 src/app/api/monitoring/umami/[...path]/route.ts delete mode 100644 src/app/api/send/__tests__/route.test.ts delete mode 100644 src/app/api/send/route.ts create mode 100644 src/lib/monitoring/__tests__/umami.test.ts create mode 100644 src/lib/monitoring/umami-paths.ts create mode 100644 src/lib/monitoring/umami.ts diff --git a/src/__tests__/openapi-route-coverage-guard.test.ts b/src/__tests__/openapi-route-coverage-guard.test.ts index d46bb5b8c..3b23a8804 100644 --- a/src/__tests__/openapi-route-coverage-guard.test.ts +++ b/src/__tests__/openapi-route-coverage-guard.test.ts @@ -320,7 +320,7 @@ const UNPUBLISHED: Readonly> = { "/api/monitoring/glitchtip": { kind: "internalOps", methods: ["POST"] }, "/api/monitoring/settings": { kind: "internalOps", methods: ["GET"] }, "/api/monitoring/umami-script": { kind: "internalOps", methods: ["GET"] }, - "/api/send": { kind: "internalOps", methods: ["POST"] }, + "/api/monitoring/umami/{path}": { kind: "internalOps", methods: ["POST"] }, "/api/mcp/oauth/authorize": { kind: "mcpTransport", methods: ["GET", "POST"], diff --git a/src/app/api/admin/monitoring/umami-test/route.ts b/src/app/api/admin/monitoring/umami-test/route.ts index e3a9d6fc1..52db57a1c 100644 --- a/src/app/api/admin/monitoring/umami-test/route.ts +++ b/src/app/api/admin/monitoring/umami-test/route.ts @@ -4,6 +4,10 @@ import { apiHandler, requireAdmin } from "@/lib/api-handler"; import { annotate, getEvent } from "@/lib/logging/context"; import { apiError, apiSuccess } from "@/lib/api-response"; import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; +import { + resolveUmamiCollectPath, + resolveUmamiSendUrls, +} from "@/lib/monitoring/umami"; import { safeFetch } from "@/lib/safe-fetch"; export const dynamic = "force-dynamic"; @@ -12,30 +16,6 @@ function redact(text: string): string { return text.replace(/https?:\/\/\S+/gi, "[url]"); } -function resolveUmamiSendUrls(scriptUrl: string | null): string[] { - if (!scriptUrl) return []; - try { - const parsed = new URL(scriptUrl); - const origin = parsed.origin; - const pathSegments = parsed.pathname.split("/").filter(Boolean); - const segments = [...pathSegments]; - - if (segments.length > 0 && segments[segments.length - 1]?.includes(".")) { - segments.pop(); - } - - const prefix = segments.length > 0 ? `/${segments.join("/")}` : ""; - const candidates = [ - `${origin}${prefix}/api/send`, - `${origin}/api/send`, - `${origin}/umami/api/send`, - ]; - return Array.from(new Set(candidates)); - } catch { - return []; - } -} - function resolveAppUrl(request: NextRequest): URL { const configured = envValue("APP_URL") ?? @@ -63,7 +43,13 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Umami script URL and website ID must be configured", 422); } - const targetUrls = resolveUmamiSendUrls(settings.umamiScriptUrl); + // The collect path is compiled into the operator's tracker build + // (`COLLECT_API_ENDPOINT`), so the test posts where the tracker would. + const collectPath = await resolveUmamiCollectPath(settings.umamiScriptUrl); + if (collectPath === null) { + return apiError("Umami script could not be fetched", 502); + } + const targetUrls = resolveUmamiSendUrls(settings.umamiScriptUrl, collectPath); if (targetUrls.length === 0) { return apiError("Umami script URL is invalid", 422); } diff --git a/src/app/api/monitoring/umami-script/route.ts b/src/app/api/monitoring/umami-script/route.ts index 4482c9c71..d625bb995 100644 --- a/src/app/api/monitoring/umami-script/route.ts +++ b/src/app/api/monitoring/umami-script/route.ts @@ -2,7 +2,7 @@ import { NextResponse } from "next/server"; import { apiHandler } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; -import { safeFetch } from "@/lib/safe-fetch"; +import { fetchUmamiScript } from "@/lib/monitoring/umami"; export const dynamic = "force-dynamic"; @@ -26,35 +26,14 @@ export const GET = apiHandler(async () => { }); } - // Fail-soft: an unreachable / slow / rebinding-refused upstream must - // degrade to the noop script, not 500 the proxy route — `safeFetch` - // THROWS on its 15 s timeout and on the connect-time public-host - // refusal, and an uncaught throw here surfaced as a 500 on every - // mount whenever the operator's Umami host was down or unresolvable. - let response: Response; - try { - response = await safeFetch( - settings.umamiScriptUrl, - { - next: { revalidate: 3600 }, - }, - // Operator-configured host — pin the connect-time IP so a low-TTL - // DNS record cannot rebind the fetch at an internal range between - // the input-time `isPublicUrl` accept and the socket connect. - { requirePublicHost: true }, - ); - } catch (err) { - // Fail-soft stays a 200, but the failure must stay observable: - // a proper action name keeps the degraded state visible on the - // wide-event dashboards instead of hiding behind a bare meta flag. + // Fail-soft: an unreachable / slow / rebinding-refused upstream degrades + // to the noop script, never a 500 on every mount. The failure stays + // observable under its own action name on the wide-event dashboards. + const fetched = await fetchUmamiScript(settings.umamiScriptUrl); + if (!fetched.ok) { annotate({ action: { name: "monitoring.umami_script.fetch_failed" }, - meta: { - reason: - err instanceof Error - ? err.message.slice(0, 200) - : String(err).slice(0, 200), - }, + meta: { reason: fetched.reason }, }); return new NextResponse(NOOP_SCRIPT, { status: 200, @@ -64,18 +43,8 @@ export const GET = apiHandler(async () => { }, }); } - if (!response.ok) { - return new NextResponse(NOOP_SCRIPT, { - status: 200, - headers: { - "Content-Type": "application/javascript; charset=utf-8", - "Cache-Control": "public, max-age=60", - }, - }); - } - const script = await response.text(); - return new NextResponse(script, { + return new NextResponse(fetched.script, { status: 200, headers: { "Content-Type": "application/javascript; charset=utf-8", diff --git a/src/app/api/monitoring/umami/[...path]/__tests__/route.test.ts b/src/app/api/monitoring/umami/[...path]/__tests__/route.test.ts new file mode 100644 index 000000000..8093f0fe0 --- /dev/null +++ b/src/app/api/monitoring/umami/[...path]/__tests__/route.test.ts @@ -0,0 +1,166 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +// safeFetch's requirePublicHost path runs through undici's own `fetch` +// (version-locked with its dispatcher). Delegate it to the global `fetch` +// stub these tests install so the interception still applies. +vi.mock("undici", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetch: (input: unknown, init?: unknown) => + (globalThis.fetch as unknown as (i: unknown, n?: unknown) => unknown)( + input, + init, + ), + }; +}); + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, +})); + +vi.mock("@/lib/logging/context", () => ({ + annotate: vi.fn(), +})); + +vi.mock("@/lib/monitoring-settings", () => ({ + getPublicMonitoringSettings: vi.fn(), +})); + +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(), +})); + +import { POST } from "../route"; +import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; +import { resetUmamiCollectPathCache } from "@/lib/monitoring/umami"; +import { checkRateLimit } from "@/lib/rate-limit"; + +const SCRIPT_URL = "https://analytics.example.com/script.js"; + +/** A built tracker's endpoint template, as Umami compiles it. */ +function tracker(collectPath: string): string { + return `!function(){const R=\`\${(x||""||u.src.split("/").slice(0,-1).join("/")).replace(/\\/$/,"")}${collectPath}\`;}();`; +} + +function post(path: string[], ip = "203.0.113.11") { + return POST( + new NextRequest(`http://localhost/api/monitoring/umami/${path.join("/")}`, { + method: "POST", + body: JSON.stringify({ type: "event" }), + headers: { "content-type": "application/json", "x-real-ip": ip }, + }), + { params: Promise.resolve({ path }) }, + ); +} + +/** Upstream: the tracker script on GET, `status` on the event POST. */ +function upstream(collectPath: string, status = 202) { + vi.mocked(fetch).mockImplementation(async (input, init) => { + const url = String(input instanceof Request ? input.url : input); + if (url === SCRIPT_URL && (!init?.method || init.method === "GET")) { + return new Response(tracker(collectPath), { status: 200 }); + } + return new Response(null, { status }); + }); +} + +function eventPosts(): string[] { + return vi + .mocked(fetch) + .mock.calls.filter(([, init]) => init?.method === "POST") + .map(([input]) => String(input)); +} + +beforeEach(() => { + vi.resetAllMocks(); + resetUmamiCollectPathCache(); + vi.stubGlobal("fetch", vi.fn()); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + remaining: 119, + resetAt: Date.now() + 60_000, + } as never); + vi.mocked(getPublicMonitoringSettings).mockResolvedValue({ + umamiEnabled: true, + umamiScriptUrl: SCRIPT_URL, + umamiWebsiteId: "site-1", + } as never); +}); + +describe("POST /api/monitoring/umami/[...path]", () => { + it("rate-limits the public Umami proxy by client IP before anything else", async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + + const response = await post(["api", "send"], "203.0.113.10"); + + expect(response.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledWith( + "umami-proxy:203.0.113.10", + 120, + 60 * 1000, + ); + expect(getPublicMonitoringSettings).not.toHaveBeenCalled(); + expect(fetch).not.toHaveBeenCalled(); + }); + + it("forwards a default build's /api/send to the Umami host", async () => { + upstream("/api/send"); + const response = await post(["api", "send"]); + expect(response.status).toBe(202); + expect(eventPosts()).toEqual(["https://analytics.example.com/api/send"]); + }); + + it("forwards a renamed collect endpoint to the same path on the Umami host", async () => { + // A build with COLLECT_API_ENDPOINT=/api/insight posts to + // `/api/insight`. Before the proxy prefix, that landed on + // the app's own origin and 404'd with a full page on every page view. + upstream("/api/insight"); + const response = await post(["api", "insight"]); + expect(response.status).toBe(202); + expect(eventPosts()).toEqual(["https://analytics.example.com/api/insight"]); + }); + + it("refuses any path that is not the tracker's collect path, with an empty body", async () => { + upstream("/api/insight"); + const response = await post(["api", "websites"]); + expect(response.status).toBe(404); + expect(await response.text()).toBe(""); + expect(eventPosts()).toEqual([]); + }); + + it("learns the collect path once, not on every event", async () => { + upstream("/api/insight"); + await post(["api", "insight"]); + await post(["api", "insight"]); + const scriptFetches = vi + .mocked(fetch) + .mock.calls.filter(([input]) => String(input) === SCRIPT_URL); + expect(scriptFetches).toHaveLength(1); + expect(eventPosts()).toHaveLength(2); + }); + + it("answers an empty 502 when the tracker cannot be read", async () => { + vi.mocked(fetch).mockResolvedValue(new Response(null, { status: 500 })); + const response = await post(["api", "send"]); + expect(response.status).toBe(502); + expect(await response.text()).toBe(""); + expect(eventPosts()).toEqual([]); + }); + + it("answers 204 without touching the network when Umami is off", async () => { + vi.mocked(getPublicMonitoringSettings).mockResolvedValue({ + umamiEnabled: false, + umamiScriptUrl: SCRIPT_URL, + umamiWebsiteId: "site-1", + } as never); + const response = await post(["api", "send"]); + expect(response.status).toBe(204); + expect(fetch).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/monitoring/umami/[...path]/route.ts b/src/app/api/monitoring/umami/[...path]/route.ts new file mode 100644 index 000000000..70e83ad3b --- /dev/null +++ b/src/app/api/monitoring/umami/[...path]/route.ts @@ -0,0 +1,107 @@ +import { NextRequest, NextResponse } from "next/server"; +import { apiHandler } from "@/lib/api-handler"; +import { apiError, getClientIp } from "@/lib/api-response"; +import { annotate } from "@/lib/logging/context"; +import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; +import { + resolveUmamiCollectPath, + resolveUmamiSendUrls, +} from "@/lib/monitoring/umami"; +import { checkRateLimit } from "@/lib/rate-limit"; +import { safeFetch } from "@/lib/safe-fetch"; + +export const dynamic = "force-dynamic"; + +/** An empty answer: a refused event must not cost the tracker a 404 page. */ +function empty(status: number): NextResponse { + return new NextResponse(null, { status }); +} + +/** + * Same-origin proxy for the browser's Umami tracker. + * + * The tracker is loaded with `data-host-url` set to `UMAMI_PROXY_PREFIX`, so + * it posts to ``, where the collect path is whatever + * the operator's Umami build calls its endpoint (`/api/send` by default). + * The path after the prefix must be exactly the collect path compiled into + * the configured tracker; anything else is refused, so the proxy forwards + * events and nothing else to the operator's host. + */ +export const POST = apiHandler( + async ( + request: NextRequest, + context: { params: Promise<{ path: string[] }> }, + ) => { + annotate({ action: { name: "umami.proxy" } }); + + const ip = getClientIp(request) ?? "unknown"; + const rl = await checkRateLimit(`umami-proxy:${ip}`, 120, 60 * 1000); + if (!rl.allowed) return apiError("Rate limit exceeded", 429); + + const settings = await getPublicMonitoringSettings(); + if ( + !settings.umamiEnabled || + !settings.umamiScriptUrl || + !settings.umamiWebsiteId + ) { + return empty(204); + } + + const { path } = await context.params; + const requested = `/${path.join("/")}`; + const collectPath = await resolveUmamiCollectPath(settings.umamiScriptUrl); + if (collectPath === null) { + annotate({ + action: { name: "umami.proxy.script_unavailable" }, + }); + return empty(502); + } + if (requested !== collectPath) { + annotate({ + action: { name: "umami.proxy.path_refused" }, + meta: { requested: requested.slice(0, 128), collectPath }, + }); + return empty(404); + } + + const targetUrls = resolveUmamiSendUrls( + settings.umamiScriptUrl, + collectPath, + ); + if (targetUrls.length === 0) return empty(204); + + const body = await request.arrayBuffer(); + // Cap the proxied body at 64 KB. + if (body.byteLength > 65536) return empty(413); + + let lastStatus = 404; + for (const targetUrl of targetUrls) { + let upstream: Response; + try { + upstream = await safeFetch( + targetUrl, + { + method: "POST", + headers: { + "content-type": + request.headers.get("content-type") || "application/json", + "user-agent": + request.headers.get("user-agent") || "healthlog-proxy", + }, + body, + cache: "no-store", + }, + // Operator-configured Umami host: pin the connect-time IP. + { requirePublicHost: true }, + ); + } catch { + annotate({ action: { name: "umami.proxy.upstream_failed" } }); + return empty(502); + } + lastStatus = upstream.status; + // Only a path miss moves on to the next candidate. + if (upstream.status !== 404) return empty(upstream.status); + } + return empty(lastStatus); + }, +); diff --git a/src/app/api/send/__tests__/route.test.ts b/src/app/api/send/__tests__/route.test.ts deleted file mode 100644 index 874cca183..000000000 --- a/src/app/api/send/__tests__/route.test.ts +++ /dev/null @@ -1,103 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { NextRequest } from "next/server"; - -// safeFetch's requirePublicHost path runs through undici's own `fetch` -// (version-locked with its dispatcher). Delegate it to the global `fetch` -// stub these tests install so the existing interception still applies. -vi.mock("undici", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - fetch: (input: unknown, init?: unknown) => - (globalThis.fetch as unknown as (i: unknown, n?: unknown) => unknown)( - input, - init, - ), - }; -}); - -vi.mock("@/lib/api-handler", () => ({ - apiHandler: unknown>(fn: T) => fn, -})); - -vi.mock("@/lib/logging/context", () => ({ - annotate: vi.fn(), -})); - -vi.mock("@/lib/monitoring-settings", () => ({ - getPublicMonitoringSettings: vi.fn(), -})); - -vi.mock("@/lib/rate-limit", () => ({ - checkRateLimit: vi.fn(), -})); - -import { POST } from "../route"; -import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; -import { checkRateLimit } from "@/lib/rate-limit"; - -beforeEach(() => { - vi.resetAllMocks(); - vi.stubGlobal("fetch", vi.fn()); -}); - -describe("POST /api/send", () => { - it("rate-limits the public Umami proxy by client IP", async () => { - vi.mocked(checkRateLimit).mockResolvedValue({ - allowed: false, - remaining: 0, - resetAt: Date.now() + 60_000, - } as never); - - const response = await POST( - new NextRequest("http://localhost/api/send", { - method: "POST", - body: JSON.stringify({ type: "event" }), - headers: { - "content-type": "application/json", - "x-forwarded-for": "203.0.113.10", - }, - }), - ); - - expect(response.status).toBe(429); - expect(checkRateLimit).toHaveBeenCalledWith( - "umami-proxy:203.0.113.10", - 120, - 60 * 1000, - ); - expect(getPublicMonitoringSettings).not.toHaveBeenCalled(); - expect(fetch).not.toHaveBeenCalled(); - }); - - it("continues to proxy when the request is within quota", async () => { - vi.mocked(checkRateLimit).mockResolvedValue({ - allowed: true, - remaining: 119, - resetAt: Date.now() + 60_000, - } as never); - vi.mocked(getPublicMonitoringSettings).mockResolvedValue({ - umamiEnabled: true, - umamiScriptUrl: "https://analytics.example.com/script.js", - umamiWebsiteId: "site-1", - } as never); - vi.mocked(fetch).mockResolvedValue(new Response(null, { status: 202 })); - - const response = await POST( - new NextRequest("http://localhost/api/send", { - method: "POST", - body: JSON.stringify({ type: "event" }), - headers: { - "content-type": "application/json", - "x-real-ip": "203.0.113.11", - }, - }), - ); - - expect(response.status).toBe(202); - expect(fetch).toHaveBeenCalledWith( - "https://analytics.example.com/api/send", - expect.objectContaining({ method: "POST" }), - ); - }); -}); diff --git a/src/app/api/send/route.ts b/src/app/api/send/route.ts deleted file mode 100644 index e8f8e13af..000000000 --- a/src/app/api/send/route.ts +++ /dev/null @@ -1,104 +0,0 @@ -import { NextRequest, NextResponse } from "next/server"; -import { apiHandler } from "@/lib/api-handler"; -import { apiError, getClientIp } from "@/lib/api-response"; -import { annotate } from "@/lib/logging/context"; -import { getPublicMonitoringSettings } from "@/lib/monitoring-settings"; -import { checkRateLimit } from "@/lib/rate-limit"; -import { isPublicUrl } from "@/lib/validations/notifications"; -import { safeFetch } from "@/lib/safe-fetch"; - -export const dynamic = "force-dynamic"; - -// Use the canonical `isPublicUrl` guard rather than a hand-rolled -// allowlist. The previous local implementation predated the central -// helper and was missing CGNAT (100.64/10), IPv6 ULA (fc00::/7), the -// octal/hex/decimal IPv4 alt-notations, and the IPv4-mapped IPv6 -// shapes — every one of which the shared helper catches. Dev mode is -// no longer special-cased here: a localhost-or-RFC1918 Umami URL was -// never a valid configuration anyway (the proxy ships the request to -// the configured host with the visitor's user-agent + body, which -// only makes sense against an externally-reachable analytics target). -function isAllowedUmamiUrl(url: string): boolean { - return isPublicUrl(url); -} - -function resolveUmamiSendUrls(scriptUrl: string | null): string[] { - if (!scriptUrl) return []; - try { - const parsed = new URL(scriptUrl); - const origin = parsed.origin; - const pathSegments = parsed.pathname.split("/").filter(Boolean); - const segments = [...pathSegments]; - - // Remove script file segment (e.g. script.js, umami.js) if present. - if (segments.length > 0 && segments[segments.length - 1]?.includes(".")) { - segments.pop(); - } - - const prefix = segments.length > 0 ? `/${segments.join("/")}` : ""; - const candidates = [ - `${origin}${prefix}/api/send`, - `${origin}/api/send`, - `${origin}/umami/api/send`, - ]; - - return Array.from(new Set(candidates.filter(isAllowedUmamiUrl))); - } catch { - return []; - } -} - -export const POST = apiHandler(async (request: NextRequest) => { - annotate({ action: { name: "umami.proxy" } }); - - const ip = getClientIp(request) ?? "unknown"; - const rl = await checkRateLimit(`umami-proxy:${ip}`, 120, 60 * 1000); - if (!rl.allowed) return apiError("Rate limit exceeded", 429); - - const settings = await getPublicMonitoringSettings(); - if (!settings.umamiEnabled) { - return NextResponse.json({}, { status: 204 }); - } - - const targetUrls = resolveUmamiSendUrls(settings.umamiScriptUrl); - if (targetUrls.length === 0 || !settings.umamiWebsiteId) { - return NextResponse.json({}, { status: 204 }); - } - - const body = await request.arrayBuffer(); - // Limit proxy body size to 64 KB to prevent abuse - if (body.byteLength > 65536) { - return NextResponse.json({}, { status: 413 }); - } - - let lastResponseStatus = 404; - - for (const targetUrl of targetUrls) { - const upstream = await safeFetch( - targetUrl, - { - method: "POST", - headers: { - "content-type": - request.headers.get("content-type") || "application/json", - "user-agent": request.headers.get("user-agent") || "healthlog-proxy", - }, - body, - cache: "no-store", - }, - // Operator-configured Umami host — pin the connect-time IP. - { requirePublicHost: true }, - ); - - lastResponseStatus = upstream.status; - - // Try next candidate only for path misses. - if (upstream.status === 404) { - continue; - } - - return new NextResponse(null, { status: upstream.status }); - } - - return new NextResponse(null, { status: lastResponseStatus }); -}); diff --git a/src/components/monitoring/umami-script.tsx b/src/components/monitoring/umami-script.tsx index 00265cbd6..264c65d69 100644 --- a/src/components/monitoring/umami-script.tsx +++ b/src/components/monitoring/umami-script.tsx @@ -2,6 +2,8 @@ import { useEffect } from "react"; +import { UMAMI_PROXY_PREFIX } from "@/lib/monitoring/umami-paths"; + interface UmamiScriptProps { enabled: boolean; websiteId: string | null; @@ -22,8 +24,14 @@ export function UmamiScript({ enabled, websiteId }: UmamiScriptProps) { script.defer = true; script.src = "/api/monitoring/umami-script"; script.setAttribute("data-website-id", websiteId); - // Force same-origin tracking endpoint so CSP stays strict. - script.setAttribute("data-host-url", window.location.origin); + // Send events to the same-origin proxy so CSP stays strict. The tracker + // appends its own collect path (`/api/send`, or whatever the operator's + // Umami build renamed it to), and every path under the prefix reaches + // the proxy route. + script.setAttribute( + "data-host-url", + `${window.location.origin}${UMAMI_PROXY_PREFIX}`, + ); if (existing) { existing.replaceWith(script); diff --git a/src/lib/monitoring/__tests__/umami.test.ts b/src/lib/monitoring/__tests__/umami.test.ts new file mode 100644 index 000000000..bf9827bac --- /dev/null +++ b/src/lib/monitoring/__tests__/umami.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; + +import { + extractUmamiCollectPath, + resolveUmamiSendUrls, +} from "@/lib/monitoring/umami"; +import { UMAMI_PROXY_PREFIX } from "@/lib/monitoring/umami-paths"; + +/** The endpoint template of a built Umami tracker. */ +function tracker(collectPath: string): string { + return `const{currentScript:u}=c;const R=\`\${(x||""||u.src.split("/").slice(0,-1).join("/")).replace(/\\/$/,"")}${collectPath}\`,_=\`\${e}x\${a}\`;`; +} + +describe("extractUmamiCollectPath", () => { + it("reads Umami's default endpoint", () => { + expect(extractUmamiCollectPath(tracker("/api/send"))).toBe("/api/send"); + }); + + it("reads an endpoint renamed through COLLECT_API_ENDPOINT", () => { + expect(extractUmamiCollectPath(tracker("/api/insight"))).toBe( + "/api/insight", + ); + expect(extractUmamiCollectPath(tracker("/collect"))).toBe("/collect"); + }); + + it("falls back to the default for an unrecognised build", () => { + expect(extractUmamiCollectPath("/* umami disabled */")).toBe("/api/send"); + }); + + it("never accepts a traversal as the collect path", () => { + expect(extractUmamiCollectPath(tracker("/../admin"))).toBe("/api/send"); + }); +}); + +describe("resolveUmamiSendUrls", () => { + it("tries the script directory, the host root and the /umami mount", () => { + expect( + resolveUmamiSendUrls( + "https://stats.example.com/umami/script.js", + "/api/insight", + ), + ).toEqual([ + "https://stats.example.com/umami/api/insight", + "https://stats.example.com/api/insight", + ]); + }); + + it("refuses a private host", () => { + expect( + resolveUmamiSendUrls("http://10.0.0.5/script.js", "/api/send"), + ).toEqual([]); + }); +}); + +describe("UMAMI_PROXY_PREFIX", () => { + it("sits under the public monitoring family the proxy gate admits", () => { + expect(UMAMI_PROXY_PREFIX.startsWith("/api/monitoring/")).toBe(true); + }); +}); diff --git a/src/lib/monitoring/umami-paths.ts b/src/lib/monitoring/umami-paths.ts new file mode 100644 index 000000000..edd6a6f44 --- /dev/null +++ b/src/lib/monitoring/umami-paths.ts @@ -0,0 +1,19 @@ +// Client-safe: imported by the tracker tag, so it carries no server code. + +/** + * Where the browser's Umami tracker sends its events, relative to the app. + * + * The tracker posts to `${data-host-url}${collectPath}`, and the collect + * path is compiled into the operator's tracker build: `/api/send` by + * default, anything at all when the Umami instance sets + * `COLLECT_API_ENDPOINT` (a common ad-blocker workaround). Pointing + * `data-host-url` at the app origin therefore only worked for default + * builds; a renamed endpoint posted every page view to an app path that did + * not exist. Every path under this prefix is answered by the one proxy route + * (`src/app/api/monitoring/umami/[...path]/route.ts`), so whatever the build + * calls its endpoint, the request lands somewhere that forwards it. + */ +export const UMAMI_PROXY_PREFIX = "/api/monitoring/umami"; + +/** Umami's own default collect endpoint. */ +export const UMAMI_DEFAULT_COLLECT_PATH = "/api/send"; diff --git a/src/lib/monitoring/umami.ts b/src/lib/monitoring/umami.ts new file mode 100644 index 000000000..a973dde5a --- /dev/null +++ b/src/lib/monitoring/umami.ts @@ -0,0 +1,113 @@ +import { safeFetch } from "@/lib/safe-fetch"; +import { isPublicUrl } from "@/lib/validations/notifications"; + +import { UMAMI_DEFAULT_COLLECT_PATH } from "./umami-paths"; + +/** + * The endpoint template in a built tracker: + * `` `${(hostUrl || …).replace(/\/$/,"")}/api/send` ``. The capture is the + * path the build appends to the host. + */ +const COLLECT_PATH_PATTERN = + /\.replace\(\/\\\/\$\/,\s*(?:""|'')\)\}(\/[\w\-./~]{1,128})`/; + +/** A collect path is a plain absolute path: no traversal, no query. */ +function isPlainPath(path: string): boolean { + return /^\/[\w\-./~]{1,128}$/.test(path) && !path.split("/").includes(".."); +} + +/** + * The collect path compiled into a tracker build, or Umami's default when + * the build does not carry a recognisable endpoint template. + */ +export function extractUmamiCollectPath(script: string): string { + const match = COLLECT_PATH_PATTERN.exec(script)?.[1]; + return match && isPlainPath(match) ? match : UMAMI_DEFAULT_COLLECT_PATH; +} + +/** + * Upstream URLs to try for an event, in order: the collect path under the + * script's own directory (Umami served below a path prefix), at the host + * root, and under the conventional `/umami` mount. Only public URLs survive. + */ +export function resolveUmamiSendUrls( + scriptUrl: string | null, + collectPath: string = UMAMI_DEFAULT_COLLECT_PATH, +): string[] { + if (!scriptUrl || !isPlainPath(collectPath)) return []; + try { + const parsed = new URL(scriptUrl); + const origin = parsed.origin; + const segments = parsed.pathname.split("/").filter(Boolean); + // Drop the script file itself (script.js, umami.js, …). + if (segments.length > 0 && segments[segments.length - 1]?.includes(".")) { + segments.pop(); + } + const prefix = segments.length > 0 ? `/${segments.join("/")}` : ""; + const candidates = [ + `${origin}${prefix}${collectPath}`, + `${origin}${collectPath}`, + `${origin}/umami${collectPath}`, + ]; + return Array.from(new Set(candidates.filter((url) => isPublicUrl(url)))); + } catch { + return []; + } +} + +const COLLECT_PATH_TTL_MS = 60 * 60 * 1000; +const collectPathCache = new Map(); + +export type UmamiScriptFetch = + { ok: true; script: string } | { ok: false; reason: string }; + +/** + * Fetch the operator's tracker. Fail-soft: an unreachable, slow, refused or + * non-2xx upstream comes back as `{ ok: false, reason }`, never a throw. A + * successful fetch also records the build's collect path, so the event proxy + * learns it without a fetch of its own. + */ +export async function fetchUmamiScript( + scriptUrl: string, +): Promise { + try { + const response = await safeFetch( + scriptUrl, + { next: { revalidate: 3600 } }, + // Operator-configured host: pin the connect-time IP so a low-TTL DNS + // record cannot rebind the fetch at an internal range. + { requirePublicHost: true }, + ); + if (!response.ok) return { ok: false, reason: `HTTP ${response.status}` }; + const script = await response.text(); + collectPathCache.set(scriptUrl, { + path: extractUmamiCollectPath(script), + expiresAt: Date.now() + COLLECT_PATH_TTL_MS, + }); + return { ok: true, script }; + } catch (err) { + return { + ok: false, + reason: (err instanceof Error ? err.message : String(err)).slice(0, 200), + }; + } +} + +/** + * The collect path of the configured tracker, from the cache when fresh, + * otherwise by fetching the script. `null` when the script cannot be read, + * so the caller can refuse rather than guess. + */ +export async function resolveUmamiCollectPath( + scriptUrl: string, +): Promise { + const cached = collectPathCache.get(scriptUrl); + if (cached && cached.expiresAt > Date.now()) return cached.path; + const fetched = await fetchUmamiScript(scriptUrl); + return fetched.ok ? extractUmamiCollectPath(fetched.script) : null; +} + +/** Test seam: forget every learnt collect path. */ +export function resetUmamiCollectPathCache(): void { + collectPathCache.clear(); +} diff --git a/src/proxy.ts b/src/proxy.ts index b514ab5a4..d2bd8b033 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -41,7 +41,6 @@ export const PUBLIC_PATHS = [ "/api/version", "/api/notifications/vapid", "/api/monitoring/", - "/api/send", "/api/withings/webhook", // v1.11.0 — WHOOP webhook (`recovery.updated` / `sleep.updated` / // `workout.updated`, + `*.deleted`). Authenticated by the path-segment From 2c1369f01be3fd6760d5e4629dc9434f42cfe7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 4 Oct 2026 15:28:28 +0200 Subject: [PATCH 3/7] perf(charts): measure chart text outside Recharts sizes every tick and label by writing it into one off-screen span and reading getBoundingClientRect(). Under each read forced a document-wide style recalc, because the stylesheet's descendant-subject :has() rules leave Chromium no way to scope the invalidation. A seeded dashboard made 149 such reads per cold load at about 2.8 ms each, about 1.8 s of main-thread time under a 4x CPU slowdown. The chart runtime now parks that span in a zero-size, contain: strict host under , which Recharts finds by id and keeps using. Each read drops to about 0.01 ms; the dashboard's total blocking time under the same slowdown falls from about 2.5 s to about 0.75 s. The host copies 's classes, where the app font is set, so every string measures exactly as before; an e2e spec checks that parity on the drawn labels and that the host survives a client-side round trip. --- e2e/chart-text-measurement-host.spec.ts | 104 ++++++++++++++++++ src/components/charts/chart-runtime.ts | 9 ++ .../charts/text-measurement-host.ts | 73 ++++++++++++ 3 files changed, 186 insertions(+) create mode 100644 e2e/chart-text-measurement-host.spec.ts create mode 100644 src/components/charts/text-measurement-host.ts diff --git a/e2e/chart-text-measurement-host.spec.ts b/e2e/chart-text-measurement-host.spec.ts new file mode 100644 index 000000000..8da0ea448 --- /dev/null +++ b/e2e/chart-text-measurement-host.spec.ts @@ -0,0 +1,104 @@ +import { expect, test } from "./setup/test"; + +import { STORAGE_STATE_PATH } from "./setup/global-setup"; + +/** + * Recharts measures every tick label in one shared off-screen span + * (`#recharts_measurement_span`). Under `` each read cost a + * document-wide style recalc, because the stylesheet's descendant-subject + * `:has()` rules leave Chromium no way to scope the invalidation; on a + * seeded dashboard that was about 1.8 s of main-thread time under a 4x CPU + * slowdown. The chart runtime now parks the span in a contained host under + * `` (`src/components/charts/text-measurement-host.ts`). + * + * Pinned here, in a real engine: + * - the span Recharts uses is the one in the host, not a second one it + * created under ``, including after a client-side round trip; + * - every string the charts drew measures exactly as it does under + * ``, so the move cannot change a single tick or label. + */ +test.describe("chart text measurement host", () => { + test.use({ storageState: STORAGE_STATE_PATH }); + + async function assertHostInPlace(page: import("@playwright/test").Page) { + const placement = await page.evaluate(() => { + const spans = document.querySelectorAll("#recharts_measurement_span"); + const span = spans[0] as HTMLElement | undefined; + return { + count: spans.length, + hostSlot: span?.parentElement?.dataset.slot ?? null, + hostParent: span?.parentElement?.parentElement?.tagName ?? null, + }; + }); + expect(placement).toEqual({ + count: 1, + hostSlot: "chart-text-measurement-host", + hostParent: "HTML", + }); + } + + test("the dashboard charts measure text outside , with identical results", async ({ + page, + }) => { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await expect(page.locator(".recharts-wrapper").first()).toBeVisible({ + timeout: 20_000, + }); + await assertHostInPlace(page); + + const parity = await page.evaluate(() => { + const host = document.getElementById( + "recharts_measurement_span", + ) as HTMLElement; + const reference = document.createElement("span"); + document.body.appendChild(reference); + // Recharts' own measurement style (`getStringSize`). + const base = { + position: "absolute", + top: "-20000px", + left: "0", + padding: "0", + margin: "0", + border: "none", + whiteSpace: "pre", + }; + const strings = [ + ...new Set( + [...document.querySelectorAll(".recharts-wrapper text")] + .map((el) => el.textContent ?? "") + .filter(Boolean), + ), + ]; + const mismatches: string[] = []; + for (const fontSize of ["10px", "12px", "14px"]) { + for (const text of strings) { + Object.assign(host.style, base, { fontSize }); + Object.assign(reference.style, base, { fontSize }); + host.textContent = text; + reference.textContent = text; + const a = host.getBoundingClientRect(); + const b = reference.getBoundingClientRect(); + if (a.width !== b.width || a.height !== b.height) { + mismatches.push(`${text}@${fontSize}`); + } + } + } + reference.remove(); + return { measured: strings.length, mismatches }; + }); + // A chart with no labels would make the parity check vacuous. + expect(parity.measured).toBeGreaterThan(0); + expect(parity.mismatches).toEqual([]); + + // A client-side round trip re-renders the shell; the host must survive + // it, or Recharts quietly recreates its span under . + await page.locator('a[href="/insights"]:visible').first().click(); + await page.waitForURL((url) => url.pathname === "/insights"); + await page.locator('a[href="/"]:visible').first().click(); + await page.waitForURL((url) => url.pathname === "/"); + await expect(page.locator(".recharts-wrapper").first()).toBeVisible({ + timeout: 20_000, + }); + await assertHostInPlace(page); + }); +}); diff --git a/src/components/charts/chart-runtime.ts b/src/components/charts/chart-runtime.ts index 56dd4ab60..93a51dcfc 100644 --- a/src/components/charts/chart-runtime.ts +++ b/src/components/charts/chart-runtime.ts @@ -23,8 +23,17 @@ * "recharts" directly. * - Types stay importable from the component files directly (type-only * imports are value-free and don't drag the chunk in). + * + * Loading this module also moves Recharts' text-measurement span out of + * `` before the first chart measures anything; see + * `text-measurement-host.ts` for why that read was the dashboard's largest + * main-thread cost. */ +import { installTextMeasurementHost } from "./text-measurement-host"; + +installTextMeasurementHost(); + export { HealthChart } from "./health-chart"; export { MoodChart } from "./mood-chart"; export { MedicationComplianceChart } from "./medication-compliance-chart"; diff --git a/src/components/charts/text-measurement-host.ts b/src/components/charts/text-measurement-host.ts new file mode 100644 index 000000000..f27bc2332 --- /dev/null +++ b/src/components/charts/text-measurement-host.ts @@ -0,0 +1,73 @@ +/** + * Where Recharts measures text, moved out of ``. + * + * Recharts sizes every axis tick and label by writing the string into one + * shared, off-screen `` and reading its + * `getBoundingClientRect()`. It looks the span up by id and creates it under + * `` only when it is missing, so whichever element carries that id is + * the one it uses. + * + * Under `` each of those reads cost a document-wide style recalc. The + * stylesheet carries `:has()` rules whose subject is any descendant of an + * anchor (`:is(:where(.group/avatar-group):has(…) *)`, the card and dialog + * slot rules), so Chromium cannot scope the invalidation a text change in a + * `` descendant causes. Measured on a seeded dashboard: 149 reads per + * cold load, about 2.8 ms each unthrottled (about 1.8 s of the 2.5 s of + * blocking time under a 4x CPU slowdown). The same read on a span outside + * ``, inside a `contain: strict` box, costs about 0.01 ms. + * + * So the span lives under ``, next to ``, inside a zero-size + * contained host. The host copies ``'s class list, which is where the + * app font is set (`--font-sans` + `font-sans`), so every string measures in + * the same font at the same size as before and the charts lay out exactly as + * they did. + */ + +export const RECHARTS_MEASUREMENT_SPAN_ID = "recharts_measurement_span"; + +const HOST_STYLE = [ + "position:absolute", + "top:0", + "left:0", + "width:0", + "height:0", + "overflow:hidden", + "contain:strict", + "visibility:hidden", + "pointer-events:none", +].join(";"); + +/** + * Put the measurement span into its contained host under ``. Moves a + * span Recharts already created under ``; a no-op once installed. + * Returns the span, or `null` where there is no DOM. + */ +export function installTextMeasurementHost( + doc: Document | undefined = typeof document === "undefined" + ? undefined + : document, +): HTMLElement | null { + if (!doc?.documentElement || !doc.body) return null; + const existing = doc.getElementById(RECHARTS_MEASUREMENT_SPAN_ID); + const parent = existing?.parentElement; + if ( + existing && + parent?.dataset.slot === "chart-text-measurement-host" && + parent.parentElement === doc.documentElement + ) { + return existing; + } + + const host = doc.createElement("div"); + host.dataset.slot = "chart-text-measurement-host"; + host.setAttribute("aria-hidden", "true"); + host.className = doc.body.className; + host.style.cssText = HOST_STYLE; + + const span = existing ?? doc.createElement("span"); + span.id = RECHARTS_MEASUREMENT_SPAN_ID; + span.setAttribute("aria-hidden", "true"); + host.appendChild(span); + doc.documentElement.appendChild(host); + return span; +} From 23961d2280fd4f5b145c407d4ceb6ca4e891e9c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 4 Oct 2026 15:29:30 +0200 Subject: [PATCH 4/7] perf(medications): stop re-sending the read a nav prefetch just made The medications list and the compliance summary refetch on every mount, so a take or skip logged on another device shows on return (#316). The nav link's hover / touch / focus intent and the route-commit preload prefetch both reads too, so every visit through a link sent each of them twice: the prefetch, then the identical request from the mount a few hundred milliseconds after the first answer landed. The mount now skips its refetch only when the cached data answers a prefetch that went to the network for this visit: within the last 10 s, and since the route last left /medications. A direct load, a return with an older entry, or a quick return whose prefetch found the entry still fresh keep the refetch on mount. --- src/app/medications/page-client.tsx | 5 +- src/app/medications/page.tsx | 4 +- src/components/providers.tsx | 9 +- .../__tests__/prefetch-medications.test.ts | 129 +++++++++++++++++- src/lib/queries/prefetch-medications.ts | 92 +++++++++++-- .../use-medication-compliance-summary.ts | 8 +- 6 files changed, 229 insertions(+), 18 deletions(-) diff --git a/src/app/medications/page-client.tsx b/src/app/medications/page-client.tsx index 7d13852ec..b37d4ef51 100644 --- a/src/app/medications/page-client.tsx +++ b/src/app/medications/page-client.tsx @@ -55,6 +55,7 @@ import { Wrench, } from "lucide-react"; import { apiGet } from "@/lib/api/api-fetch"; +import { refetchMedicationsOnMount } from "@/lib/queries/prefetch-medications"; import { useMedicationComplianceSummaryAll } from "@/lib/queries/use-medication-compliance-summary"; import { useMedicationListLayout } from "@/lib/queries/use-medication-list-layout"; import { applyMedicationOrder } from "@/lib/medications/medication-order"; @@ -345,7 +346,9 @@ export default function MedicationsPageClient() { // client cache can't know to invalidate a cross-device write. "always" // makes a return-visit fetch fresh; the per-user server cache keeps it // cheap. Mutation-driven invalidation still covers same-tab writes. - refetchOnMount: "always", + // A prefetch this same navigation fired (nav-link intent, route commit) + // already is that fresh fetch, so it is not sent twice. + refetchOnMount: refetchMedicationsOnMount("list"), }); // v1.30.1 M12 — pull-to-refresh parity with labs/checkups/mood/measurements. diff --git a/src/app/medications/page.tsx b/src/app/medications/page.tsx index 201e4e239..a240df37f 100644 --- a/src/app/medications/page.tsx +++ b/src/app/medications/page.tsx @@ -39,7 +39,9 @@ import MedicationsPageClient from "./page-client"; * event, so the list must re-verify on every mount. With the prefetch in * place the hydrated cards paint immediately and that refetch runs in the * background — the "empty then fills" flash is gone WITHOUT dropping the - * freshness guarantee. + * freshness guarantee. The one exception is data a prefetch fired by this + * same navigation just fetched (`refetchMedicationsOnMount`): that already + * is the re-verification, so it is not sent twice. * - Record identity: the session comes from `getUnswitchedSession()`, which * answers null while the browser is acting on somebody else's record. * `/api/medications` IS delegable, so the client cell corrects itself on its diff --git a/src/components/providers.tsx b/src/components/providers.tsx index 2b626370c..e482e04d9 100644 --- a/src/components/providers.tsx +++ b/src/components/providers.tsx @@ -34,7 +34,10 @@ import { getRecordSessionTransition, subscribeToRecordSessionTransition, } from "@/lib/query-keys/record-session-transition"; -import { prefetchMedicationsList } from "@/lib/queries/prefetch-medications"; +import { + forgetMedicationsPrefetch, + prefetchMedicationsList, +} from "@/lib/queries/prefetch-medications"; import { getRecordScope, subscribeToRecordScope, @@ -245,6 +248,10 @@ function DashboardSnapshotPreloader() { // intent, so this is the fallback for direct loads + reloads. if (pathname === "/medications") { prefetchMedicationsList(queryClient, controller.signal); + } else { + // Off the page: a later prefetch starts a new visit, and an earlier one + // must not excuse the next mount's re-verification. + forgetMedicationsPrefetch(); } // A transition to an unresolved, refused, or different record scope must // abort a preloader before its response can be adopted by the next route. diff --git a/src/lib/queries/__tests__/prefetch-medications.test.ts b/src/lib/queries/__tests__/prefetch-medications.test.ts index bf718c69c..22ee0f750 100644 --- a/src/lib/queries/__tests__/prefetch-medications.test.ts +++ b/src/lib/queries/__tests__/prefetch-medications.test.ts @@ -1,12 +1,17 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { QueryClient } from "@tanstack/react-query"; +import { QueryClient, QueryObserver } from "@tanstack/react-query"; import { QUERY_CLIENT_DEFAULT_OPTIONS } from "@/lib/pwa/query-client-options"; import { queryKeys } from "@/lib/query-keys"; import { __resetRecordScopeForTests, setRecordScope, } from "@/lib/query-keys/record-scope"; -import { prefetchMedicationsList } from "../prefetch-medications"; +import { + MEDICATIONS_PREFETCH_COVERS_MOUNT_MS, + forgetMedicationsPrefetch, + prefetchMedicationsList, + refetchMedicationsOnMount, +} from "../prefetch-medications"; function envelopeResponse(data: unknown): Response { return new Response(JSON.stringify({ data, error: null }), { @@ -17,7 +22,9 @@ function envelopeResponse(data: unknown): Response { afterEach(() => { __resetRecordScopeForTests(); + forgetMedicationsPrefetch(); vi.unstubAllGlobals(); + vi.useRealTimers(); }); describe("prefetchMedicationsList", () => { @@ -70,3 +77,121 @@ describe("prefetchMedicationsList", () => { }); }); }); + +/** + * The page cells re-verify on every mount (#316), but a prefetch fired by the + * same navigation already is that fresh read. Before, the hover / route-commit + * prefetch landed and the mount sent the identical request again, on every + * visit, for both the list and the compliance summary. + */ +describe("refetchMedicationsOnMount", () => { + function setup() { + const fetchMock = vi.fn(async () => envelopeResponse([])); + vi.stubGlobal("fetch", fetchMock); + const client = new QueryClient({ + defaultOptions: QUERY_CLIENT_DEFAULT_OPTIONS, + }); + const calls = (path: string) => + fetchMock.mock.calls.filter((call) => (call as unknown[])[0] === path) + .length; + return { client, calls }; + } + + /** Mount a page observer the way the medications page does. */ + async function mountList(client: QueryClient) { + const mountFetch = vi.fn(async () => []); + const observer = new QueryObserver(client, { + queryKey: queryKeys.medications(), + queryFn: mountFetch, + refetchOnMount: refetchMedicationsOnMount("list"), + }); + const unsubscribe = observer.subscribe(() => {}); + await Promise.resolve(); + return { refetched: mountFetch.mock.calls.length > 0, unsubscribe }; + } + + it("does not repeat a prefetch this navigation just answered", async () => { + const { client, calls } = setup(); + prefetchMedicationsList(client); + await vi.waitFor(() => expect(calls("/api/medications")).toBe(1)); + await vi.waitFor(() => + expect(client.getQueryState(queryKeys.medications())?.status).toBe( + "success", + ), + ); + + const { refetched, unsubscribe } = await mountList(client); + expect(refetched).toBe(false); + unsubscribe(); + }); + + it("still refetches on mount when the cache holds an older read", async () => { + const { client } = setup(); + const queryFn = vi.fn(async () => []); + await client.fetchQuery({ queryKey: queryKeys.medications(), queryFn }); + + const { refetched, unsubscribe } = await mountList(client); + // No prefetch answered this visit: the #316 re-verification runs. + expect(refetched).toBe(true); + unsubscribe(); + }); + + it("refetches once the prefetch is older than the visit window", async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + const { client, calls } = setup(); + prefetchMedicationsList(client); + await vi.waitFor(() => + expect(client.getQueryState(queryKeys.medications())?.status).toBe( + "success", + ), + ); + expect(calls("/api/medications")).toBe(1); + + vi.setSystemTime(Date.now() + MEDICATIONS_PREFETCH_COVERS_MOUNT_MS + 1); + const { refetched, unsubscribe } = await mountList(client); + expect(refetched).toBe(true); + unsubscribe(); + }); + + it("a prefetch skipped as fresh does not excuse the mount refetch", async () => { + // The return-visit case: the list was read moments ago by a mount, so + // the nav-link prefetch does not go to the network, and the remount + // must still re-verify. + const { client, calls } = setup(); + await client.fetchQuery({ + queryKey: queryKeys.medications(), + queryFn: async () => [], + }); + prefetchMedicationsList(client); + await Promise.resolve(); + expect(calls("/api/medications")).toBe(0); + + const { refetched, unsubscribe } = await mountList(client); + expect(refetched).toBe(true); + unsubscribe(); + }); + + it("a prefetch from an earlier visit does not excuse a later mount", async () => { + // Load the page (route-commit prefetch), leave, come straight back: the + // nav-link prefetch finds the entry fresh and stays off the network, so + // only the mount can re-verify, and it must. + const { client, calls } = setup(); + prefetchMedicationsList(client); + await vi.waitFor(() => + expect(client.getQueryState(queryKeys.medications())?.status).toBe( + "success", + ), + ); + const first = await mountList(client); + expect(first.refetched).toBe(false); + first.unsubscribe(); + + forgetMedicationsPrefetch(); // the route left /medications + prefetchMedicationsList(client); // hover on the way back: still fresh + expect(calls("/api/medications")).toBe(1); + + const second = await mountList(client); + expect(second.refetched).toBe(true); + second.unsubscribe(); + }); +}); diff --git a/src/lib/queries/prefetch-medications.ts b/src/lib/queries/prefetch-medications.ts index b9902f6e3..6058fc137 100644 --- a/src/lib/queries/prefetch-medications.ts +++ b/src/lib/queries/prefetch-medications.ts @@ -29,20 +29,92 @@ import { apiGet } from "@/lib/api/api-fetch"; */ export const MEDICATIONS_LIST_STALE_TIME_MS = 15_000; +/** + * How long after a prefetch STARTED its data still counts as this visit's + * fresh read. Wide enough to span hover → click → route commit → mount on a + * slow device; far below any gap in which another surface could have + * written something the user would expect to see. + */ +export const MEDICATIONS_PREFETCH_COVERS_MOUNT_MS = 10_000; + +type MedicationsRead = "list" | "compliance"; + +/** When the last prefetch that actually went to the network started. */ +const prefetchStartedAt = new Map(); + +function prefetchRead( + queryClient: QueryClient, + read: MedicationsRead, + queryKey: readonly unknown[], + path: string, + signal?: AbortSignal, +): void { + const state = queryClient.getQueryState(queryKey); + const willFetch = + state?.fetchStatus !== "fetching" && + (!state || + state.dataUpdatedAt < Date.now() - MEDICATIONS_LIST_STALE_TIME_MS); + if (willFetch) prefetchStartedAt.set(read, Date.now()); + void queryClient.prefetchQuery({ + queryKey, + queryFn: () => apiGet(path, { signal }), + staleTime: MEDICATIONS_LIST_STALE_TIME_MS, + }); +} + export function prefetchMedicationsList( queryClient: QueryClient, signal?: AbortSignal, ): void { - void queryClient.prefetchQuery({ - queryKey: queryKeys.medications(), - queryFn: () => apiGet("/api/medications", { signal }), - staleTime: MEDICATIONS_LIST_STALE_TIME_MS, - }); - void queryClient.prefetchQuery({ - queryKey: queryKeys.medicationComplianceSummary(), - queryFn: () => apiGet("/api/medications/compliance", { signal }), - staleTime: MEDICATIONS_LIST_STALE_TIME_MS, - }); + prefetchRead( + queryClient, + "list", + queryKeys.medications(), + "/api/medications", + signal, + ); + prefetchRead( + queryClient, + "compliance", + queryKeys.medicationComplianceSummary(), + "/api/medications/compliance", + signal, + ); +} + +/** + * `refetchOnMount` for the medications reads that must re-verify on every + * visit (#316: a take or skip on another device produces no event here). + * + * Still "always", except when the cached data is the answer to a prefetch + * that this same navigation fired moments ago: the hover / touch / focus + * intent on the nav link, or the route-commit preload. Leaving the page ends + * that navigation (`forgetMedicationsPrefetch`). Re-asking then sent + * the identical request a second time, a few hundred milliseconds after the + * first answer landed, on every visit to the page. A return to the page with + * an older cache entry, or a direct load, still refetches on mount. + */ +export function refetchMedicationsOnMount( + read: MedicationsRead, +): (query: { state: { dataUpdatedAt: number } }) => boolean | "always" { + return (query) => { + const startedAt = prefetchStartedAt.get(read); + const answeredThisVisit = + startedAt !== undefined && + query.state.dataUpdatedAt >= startedAt && + Date.now() - startedAt < MEDICATIONS_PREFETCH_COVERS_MOUNT_MS; + return answeredThisVisit ? false : "always"; + }; +} + +/** + * End the visit a recorded prefetch belongs to. Called whenever the route is + * anything but `/medications`, so a prefetch only ever excuses the mounts of + * the visit it was fired for: leave the page and come back within seconds, + * and the remount still re-verifies. + */ +export function forgetMedicationsPrefetch(): void { + prefetchStartedAt.clear(); } /** diff --git a/src/lib/queries/use-medication-compliance-summary.ts b/src/lib/queries/use-medication-compliance-summary.ts index a3aa4c03b..4a8b6d9e8 100644 --- a/src/lib/queries/use-medication-compliance-summary.ts +++ b/src/lib/queries/use-medication-compliance-summary.ts @@ -4,6 +4,7 @@ import { useQuery } from "@tanstack/react-query"; import { apiGet } from "@/lib/api/api-fetch"; import { queryKeys } from "@/lib/query-keys"; +import { refetchMedicationsOnMount } from "@/lib/queries/prefetch-medications"; import type { ComplianceDisplay } from "@/lib/analytics/compliance"; /** @@ -69,9 +70,10 @@ export function useMedicationComplianceSummary(medicationId: string): { // another surface (the iOS app) leaves this stale for up to the // window, so refetch on every mount: returning to the page reflects // cross-device intakes without a manual reload. The batched read is - // one request per visit, server-cached. + // one request per visit, server-cached. A prefetch fired by this same + // navigation already is that request (see `refetchMedicationsOnMount`). staleTime: 5 * 60 * 1000, - refetchOnMount: "always", + refetchOnMount: refetchMedicationsOnMount("compliance"), select: (rows: MedicationComplianceSummaryEntry[]) => rows.find((row) => row.medicationId === medicationId) ?? null, }); @@ -92,7 +94,7 @@ export function useMedicationComplianceSummaryAll(): { queryFn: fetchComplianceSummary, staleTime: 5 * 60 * 1000, // v1.16.12 (#316) — fresh on return-navigation (see the per-row hook). - refetchOnMount: "always", + refetchOnMount: refetchMedicationsOnMount("compliance"), }); return { data }; } From fa481108753e27317e0943c81e39fd861c500a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 4 Oct 2026 15:37:26 +0200 Subject: [PATCH 5/7] fix(nav): mark the scroll edge when the sidebar list overflows On a short window (around 785 px and below) the nav list is taller than the room between the logo band and the footer group, so it scrolls. With nothing marking the edge, the last visible entry was cut off right at the Collapse row and read as the footer covering it; macOS hides the scrollbar that would have said otherwise. While the list overflows, the footer group draws the same hairline the user section does. A ResizeObserver on the nav and its list keeps the state current as the window or the entries change; with room to spare the rail is unchanged. The e2e spec checks the edge, that the last entry scrolls fully clear of the footer, and that a tall window draws no edge. --- e2e/sidebar-expand-no-scrollbar.spec.ts | 50 +++++++++++++++++++++++++ src/components/layout/sidebar-nav.tsx | 43 ++++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/e2e/sidebar-expand-no-scrollbar.spec.ts b/e2e/sidebar-expand-no-scrollbar.spec.ts index c8bfe58df..71b8d73e8 100644 --- a/e2e/sidebar-expand-no-scrollbar.spec.ts +++ b/e2e/sidebar-expand-no-scrollbar.spec.ts @@ -86,4 +86,54 @@ test.describe("sidebar width transition", () => { expect(tallest).toBeLessThanOrEqual(restHeight + 1); } }); + + test("on a short window the list scrolls behind a visible edge, never under the footer", async ({ + page, + }) => { + // Short enough that the list cannot fit between the logo band and the + // footer group. The rail used to cut the last visible entry off right at + // the Collapse row with nothing marking the edge, which read as the + // footer covering the entry. + await page.setViewportSize({ width: 1280, height: 600 }); + await page.goto("/", { waitUntil: "domcontentloaded" }); + const nav = page.locator("aside nav").first(); + const footer = page.locator('aside [data-slot="sidebar-footer"]'); + await expect(nav).toBeVisible(); + await expect(footer).toHaveAttribute("data-nav-overflows", "true"); + const borderTop = await footer.evaluate( + (el) => getComputedStyle(el).borderTopWidth, + ); + expect(borderTop).toBe("1px"); + + // Scrolled to its end, the last entry sits wholly inside the list, + // above the footer, and is the element under its own centre. + const end = await nav.evaluate((el) => { + el.scrollTop = el.scrollHeight; + const links = [...el.querySelectorAll("a")]; + const last = links[links.length - 1] as HTMLElement; + const r = last.getBoundingClientRect(); + const navBox = el.getBoundingClientRect(); + const footerTop = ( + document.querySelector('aside [data-slot="sidebar-footer"]') as Element + ).getBoundingClientRect().top; + const hit = document.elementFromPoint( + r.left + r.width / 2, + r.top + r.height / 2, + ); + return { + insideList: r.bottom <= navBox.bottom + 0.5, + aboveFooter: r.bottom <= footerTop + 0.5, + hitsItself: last.contains(hit), + }; + }); + expect(end).toEqual({ + insideList: true, + aboveFooter: true, + hitsItself: true, + }); + + // With room to spare there is no edge: the default rail is unchanged. + await page.setViewportSize({ width: 1280, height: 1400 }); + await expect(footer).not.toHaveAttribute("data-nav-overflows", "true"); + }); }); diff --git a/src/components/layout/sidebar-nav.tsx b/src/components/layout/sidebar-nav.tsx index 1e8fe6969..033882f4d 100644 --- a/src/components/layout/sidebar-nav.tsx +++ b/src/components/layout/sidebar-nav.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { Bell, LogOut, @@ -310,6 +310,27 @@ function SidebarUserSection({ collapsed }: { collapsed: boolean }) { ); } +/** + * Whether the nav list is taller than the room the rail leaves it, i.e. + * whether it scrolls. Re-measured when the nav (viewport height) or the list + * (entries arriving as modules resolve) changes size. + */ +function useNavOverflows(navRef: React.RefObject): boolean { + const [overflows, setOverflows] = useState(false); + useEffect(() => { + const nav = navRef.current; + if (!nav || typeof ResizeObserver === "undefined") return; + // Strict: any overflow, the same test that gives the list a scrollbar. + const update = () => setOverflows(nav.scrollHeight > nav.clientHeight); + const observer = new ResizeObserver(update); + observer.observe(nav); + if (nav.firstElementChild) observer.observe(nav.firstElementChild); + update(); + return () => observer.disconnect(); + }, [navRef]); + return overflows; +} + /** * The sidebar's collapse / expand control, drawn as a footer nav row. * @@ -426,6 +447,9 @@ export function SidebarNav() { ); const collapsed = mounted ? (collapsedPref ?? tabletOrBelow) : false; + const navRef = useRef(null); + const navOverflows = useNavOverflows(navRef); + function toggleCollapsed() { const next = !collapsed; setCollapsedPref(next); @@ -611,6 +635,7 @@ export function SidebarNav() {