From a22fb6064756e0fdfa7fd72c0aa57c97ae2a8ed2 Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Sat, 22 Aug 2026 18:27:19 +0200 Subject: [PATCH] fix: harden widget query persistence and refresh indicator Review follow-ups on the session query cache: - Cache the per-payload size verdict and reuse a single TextEncoder. The persist subscription re-dehydrates the whole cache on every cache event, so every widget payload was being superjson-serialized several times per refresh cycle just to measure it. - Skip the size measurement on the restore path. It runs before any query is allowed to fetch and the payload was already budgeted when it was written. - Reject stored queries whose dataUpdatedAt is not a positive finite number; `undefined <= 0` is false, so those were hydrating with a NaN timestamp. - Add `stop()` and tear the persister down before `queryClient.clear()`, so the clear cannot overwrite the saved snapshot with an empty cache. - Remove every other scope's cache when a persister is created: signing out navigates away before the scope guard can react, which left the previous account's dashboard data in the tab. - Remove the entry instead of storing an empty cache. Refresh indicator: - Make it decorative instead of an aria-live region; every widget refreshes on a timer, so it made assistive technology repeat "loading" forever. Also broadens the e2e hydration-error matcher to production React's minified error codes, hoists the single WidgetTimeProvider in ClientBoard, memoizes the resolved local time zone, and drops the exports this branch made dead. --- .../query-persistence.spec.ts | 68 +++++++++++++- .../_client-providers/query-persistence.ts | 90 +++++++++++++++---- .../app/[locale]/_client-providers/trpc.tsx | 6 +- .../app/[locale]/boards/(content)/_client.tsx | 42 ++++----- .../[locale]/boards/(content)/_creator.tsx | 7 +- .../components/board/items/item-content.tsx | 10 ++- e2e/lazy-widgets.spec.ts | 13 ++- packages/widgets/src/clock/world-clock.ts | 5 -- .../src/common/widget-time-context.spec.tsx | 2 +- .../src/common/widget-time-context.tsx | 33 +++++-- packages/widgets/src/index.tsx | 19 ---- packages/widgets/src/time.ts | 4 +- 12 files changed, 208 insertions(+), 91 deletions(-) diff --git a/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.spec.ts b/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.spec.ts index bbce19bc2..954797a22 100644 --- a/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.spec.ts +++ b/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.spec.ts @@ -71,12 +71,23 @@ describe("widget query persistence", () => { expect(getQueryPersistenceStorageKey(null)).not.toBe(getQueryPersistenceStorageKey("user-a")); const userAKey = getQueryPersistenceStorageKey("user-a"); - const userBKey = getQueryPersistenceStorageKey("user-b"); window.sessionStorage.setItem(userAKey, "a"); - window.sessionStorage.setItem(userBKey, "b"); createSessionQueryPersister("user-a").removeClient(); expect(window.sessionStorage.getItem(userAKey)).toBeNull(); - expect(window.sessionStorage.getItem(userBKey)).toBe("b"); + }); + + test("evicts every other scope so signing out cannot leave data behind", () => { + const userAKey = getQueryPersistenceStorageKey("user-a"); + const anonymousKey = getQueryPersistenceStorageKey(null); + window.sessionStorage.setItem(userAKey, "a"); + window.sessionStorage.setItem(anonymousKey, "anonymous"); + window.sessionStorage.setItem("unrelated", "kept"); + + createSessionQueryPersister("user-b"); + + expect(window.sessionStorage.getItem(userAKey)).toBeNull(); + expect(window.sessionStorage.getItem(anonymousKey)).toBeNull(); + expect(window.sessionStorage.getItem("unrelated")).toBe("kept"); }); test("never restores another session scope", async () => { @@ -89,9 +100,53 @@ describe("widget query persistence", () => { clientState: dehydrate(queryClient), }); userAPersister.flush(); + expect((await userAPersister.restoreClient())?.clientState.queries).toHaveLength(1); expect(createSessionQueryPersister("user-b").restoreClient()).toBeUndefined(); - expect((await userAPersister.restoreClient())?.clientState.queries).toHaveLength(1); + }); + + test("stops persisting once the provider tears the cache down", async () => { + const storage = createMemoryStorage(); + const persister = createSessionQueryPersister("user-a", storage); + const { queryClient } = createSuccessfulQuery([["widget", "weather", "getWeather"], { type: "query" }], { + temp: 21, + }); + + persister.persistClient({ + timestamp: Date.now(), + buster: queryPersistenceBuster, + clientState: dehydrate(queryClient), + }); + persister.flush(); + persister.stop(); + + // A torn down provider clears its query client, which would otherwise be + // persisted as an empty cache on top of the snapshot we just saved. + persister.persistClient({ + timestamp: Date.now(), + buster: queryPersistenceBuster, + clientState: { mutations: [], queries: [] }, + }); + persister.flush(); + + expect((await persister.restoreClient())?.clientState.queries).toHaveLength(1); + }); + + test("removes the cache instead of storing an empty one", () => { + const scope = "user-a"; + const key = getQueryPersistenceStorageKey(scope); + const { queryClient } = createSuccessfulQuery([["board", "getBoardByName"], { type: "query" }], { name: "Home" }); + const persister = createSessionQueryPersister(scope); + window.sessionStorage.setItem(key, "stale"); + + persister.persistClient({ + timestamp: Date.now(), + buster: queryPersistenceBuster, + clientState: dehydrate(queryClient), + }); + persister.flush(); + + expect(window.sessionStorage.getItem(key)).toBeNull(); }); test("persister itself rejects unrelated query payloads", async () => { @@ -179,6 +234,11 @@ describe("widget query persistence", () => { queryKey: [["board", "getBoardByName"], { type: "query" }], }, { queryHash: "malformed", queryKey: [["widget", "weather"]], state: null }, + { + ...validQuery, + queryHash: "missing-timestamp", + state: { ...validQuery.state, dataUpdatedAt: undefined }, + }, ], }, }), diff --git a/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.ts b/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.ts index 76e798cc9..f899a881d 100644 --- a/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.ts +++ b/apps/nextjs/src/app/[locale]/_client-providers/query-persistence.ts @@ -22,20 +22,45 @@ interface PersistableQuery { }; } -export type SessionQueryPersister = Persister & { flush: () => void }; +export type SessionQueryPersister = Persister & { flush: () => void; stop: () => void }; -export const shouldPersistDashboardQuery = (query: PersistableQuery) => { - if (query.state.data === undefined || query.state.dataUpdatedAt <= 0 || !isWidgetDataQueryKey(query.queryKey)) { - return false; - } +const textEncoder = new TextEncoder(); +// React Query re-dehydrates the whole cache on every cache event, so this +// predicate runs for every query several times per refresh cycle. Measuring a +// payload means serializing it, which dwarfs everything else in the pass, so +// remember the verdict per data reference instead of re-encoding every widget +// payload every time. +const budgetVerdictCache = new WeakMap(); +const measureFitsBudget = (data: unknown) => { try { - return new TextEncoder().encode(stringify(query.state.data)).byteLength <= queryPersistenceMaxDataBytes; + return textEncoder.encode(stringify(data)).byteLength <= queryPersistenceMaxDataBytes; } catch { return false; } }; +const fitsStorageBudget = (data: unknown) => { + if (typeof data !== "object" || data === null) return measureFitsBudget(data); + + const cached = budgetVerdictCache.get(data); + if (cached !== undefined) return cached; + + const verdict = measureFitsBudget(data); + budgetVerdictCache.set(data, verdict); + return verdict; +}; + +const holdsDashboardData = (query: PersistableQuery) => + query.state.data !== undefined && + Number.isFinite(query.state.dataUpdatedAt) && + query.state.dataUpdatedAt > 0 && + isWidgetDataQueryKey(query.queryKey); + +/** Write path: dashboard data that is small enough to keep in session storage. */ +export const shouldPersistDashboardQuery = (query: PersistableQuery) => + holdsDashboardData(query) && fitsStorageBudget(query.state.data); + type DehydratedQuery = PersistedClient["clientState"]["queries"][number]; const sanitizePersistedQuery = (query: DehydratedQuery): DehydratedQuery => ({ @@ -66,11 +91,14 @@ const sanitizePersistedClient = (client: PersistedClient): PersistedClient => ({ const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); +// Read path: the payload was already budgeted when it was written, and this runs +// before any query is allowed to fetch, so it deliberately skips the expensive +// size measurement and only rejects anything that is not restorable widget data. const isRestorableQuery = (value: unknown): value is DehydratedQuery => { if (!isRecord(value) || typeof value.queryHash !== "string" || !Array.isArray(value.queryKey)) return false; if (!isRecord(value.state) || typeof value.state.status !== "string") return false; - return shouldPersistDashboardQuery(value as unknown as PersistableQuery); + return holdsDashboardData(value as unknown as PersistableQuery); }; const sanitizeRestoredClient = (value: unknown): PersistedClient | undefined => { @@ -100,7 +128,29 @@ const getSessionStorage = (): Storage | undefined => { const removeStorageItem = (storage: Storage | undefined, key: string) => { try { storage?.removeItem(key); - } catch {} + } catch { + return undefined; + } +}; + +// Signing out navigates away before the session scope guard can observe the +// change, so a previous account's cached dashboard data would otherwise linger +// in the tab. Only the active scope may keep a cache. +const removeForeignScopes = (storage: Storage | undefined, activeKey: string) => { + if (!storage) return; + + try { + const foreignKeys: string[] = []; + for (let index = 0; index < storage.length; index += 1) { + const storedKey = storage.key(index); + if (storedKey === null || storedKey === activeKey) continue; + if (storedKey.startsWith(`${queryPersistenceStoragePrefix}:`)) foreignKeys.push(storedKey); + } + + for (const foreignKey of foreignKeys) storage.removeItem(foreignKey); + } catch { + return undefined; + } }; export const createSessionQueryPersister = ( @@ -108,14 +158,21 @@ export const createSessionQueryPersister = ( storage: Storage | undefined = getSessionStorage(), ): SessionQueryPersister => { const key = getQueryPersistenceStorageKey(scope); + removeForeignScopes(storage, key); let pendingClient: PersistedClient | undefined; let persistTimer: ReturnType | undefined; + let stopped = false; const persistNow = (client: PersistedClient) => { if (!storage) return; let candidate: PersistedClient | undefined = sanitizePersistedClient(client); let errorCount = 0; + if (candidate.clientState.queries.length === 0) { + removeStorageItem(storage, key); + return; + } + while (candidate) { try { storage.setItem(key, stringify(candidate)); @@ -146,7 +203,7 @@ export const createSessionQueryPersister = ( return { persistClient(client) { - if (!storage) return; + if (!storage || stopped) return; pendingClient = client; if (persistTimer !== undefined) return; persistTimer = setTimeout(flush, 0); @@ -157,12 +214,7 @@ export const createSessionQueryPersister = ( if (!storedValue) return undefined; const client = sanitizeRestoredClient(parse(storedValue)); - if (!client || client.timestamp > Date.now()) { - removeStorageItem(storage, key); - return undefined; - } - - if (client.clientState.queries.length === 0) { + if (!client || client.timestamp > Date.now() || client.clientState.queries.length === 0) { removeStorageItem(storage, key); return undefined; } @@ -179,5 +231,13 @@ export const createSessionQueryPersister = ( removeStorageItem(storage, key); }, flush, + // Called when the provider tears down, after the last flush: the teardown + // also clears the query cache, and that clear must not be able to overwrite + // the snapshot we just saved with an empty one. + stop() { + stopped = true; + clearPersistTimer(); + pendingClient = undefined; + }, }; }; diff --git a/apps/nextjs/src/app/[locale]/_client-providers/trpc.tsx b/apps/nextjs/src/app/[locale]/_client-providers/trpc.tsx index 51aefdd43..52bc9ac29 100644 --- a/apps/nextjs/src/app/[locale]/_client-providers/trpc.tsx +++ b/apps/nextjs/src/app/[locale]/_client-providers/trpc.tsx @@ -152,15 +152,19 @@ const ScopedTRPCReactProvider = ({ return client; }); - useEffect(() => () => queryClient.clear(), [queryClient]); + // Declared before the cache teardown below so its cleanup runs first: the + // persister has to save the real cache and shut down before `clear()` empties + // it, otherwise the resulting "empty cache" event would be persisted instead. useEffect(() => { const flush = () => queryPersister.flush(); window.addEventListener("pagehide", flush); return () => { window.removeEventListener("pagehide", flush); flush(); + queryPersister.stop(); }; }, [queryPersister]); + useEffect(() => () => queryClient.clear(), [queryClient]); const trpcClient = useMemo(() => { return clientApi.createClient({ diff --git a/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx b/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx index ce9737984..c0015fcdd 100644 --- a/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx +++ b/apps/nextjs/src/app/[locale]/boards/(content)/_client.tsx @@ -162,31 +162,27 @@ export const ClientBoard = ({ ); - if (representativeWidth === null) { - return ( - - {content} - - ); - } - return ( - - - {tPreview("editorWidthLabel", { layoutName: currentLayout.name, width: representativeWidth })} - - - {content} - - + {representativeWidth === null ? ( + content + ) : ( + + + {tPreview("editorWidthLabel", { layoutName: currentLayout.name, width: representativeWidth })} + + + {content} + + + )} ); }; diff --git a/apps/nextjs/src/app/[locale]/boards/(content)/_creator.tsx b/apps/nextjs/src/app/[locale]/boards/(content)/_creator.tsx index ccd61caca..ce91b82a2 100644 --- a/apps/nextjs/src/app/[locale]/boards/(content)/_creator.tsx +++ b/apps/nextjs/src/app/[locale]/boards/(content)/_creator.tsx @@ -96,9 +96,14 @@ export const createBoardContentPage = >( return ( + {/* + Rendered on the server, so this is the server's clock and time + zone. Widgets paint with it immediately and switch to the browser + values right after hydration. + */} diff --git a/apps/nextjs/src/components/board/items/item-content.tsx b/apps/nextjs/src/components/board/items/item-content.tsx index 5c03f2648..0bf05fb7f 100644 --- a/apps/nextjs/src/components/board/items/item-content.tsx +++ b/apps/nextjs/src/components/board/items/item-content.tsx @@ -2,7 +2,7 @@ import type { ComponentType, CSSProperties, MutableRefObject, PropsWithChildren import { memo, Suspense, use, useCallback, useEffect, useMemo, useRef, useState } from "react"; import { createPortal } from "react-dom"; import dynamic from "next/dynamic"; -import { Box, Button, Center, Loader, Portal, VisuallyHidden } from "@mantine/core"; +import { Box, Button, Center, Loader, Portal } from "@mantine/core"; import { useElementSize, useIsomorphicEffect } from "@mantine/hooks"; import { QueryErrorResetBoundary, useIsFetching, useQueryClient } from "@tanstack/react-query"; import combineClasses from "clsx"; @@ -343,10 +343,14 @@ const LoadedBoardItemContent = ({ /> + {/* + Purely decorative: widgets keep showing their previous data while they + refresh, and every widget refreshes on a timer, so announcing this would + make assistive technology repeat "loading" for the whole board forever. + */} {isWidgetRefreshing && ( - + - {t("common.action.loading")} )} diff --git a/e2e/lazy-widgets.spec.ts b/e2e/lazy-widgets.spec.ts index f3e6b1d03..511dcb966 100644 --- a/e2e/lazy-widgets.spec.ts +++ b/e2e/lazy-widgets.spec.ts @@ -107,8 +107,11 @@ describe("lazy widget application graph", () => { const pageErrors: Error[] = []; const hydrationErrors: string[] = []; page.on("pageerror", (error) => pageErrors.push(error)); + // Production React reports hydration mismatches as minified error codes with + // a react.dev link, so matching the development wording alone never fires. + const hydrationErrorPattern = /hydrat|did not match|react\.dev\/errors\/4(18|2[1235])|react error #4(18|2[1235])/i; page.on("console", (message) => { - if (message.type() === "error" && /hydration|did not match/i.test(message.text())) { + if (message.type() === "error" && hydrationErrorPattern.test(message.text())) { hydrationErrors.push(message.text()); } }); @@ -118,16 +121,12 @@ describe("lazy widget application graph", () => { await page.goto(`${baseUrl}/auth/login`); await page.getByLabel("Username").fill(adminCredentials.username); await page.locator("#password").fill(adminCredentials.password); - const initialDownloadsResponse = page.waitForResponse( - (response) => response.url().includes("widget.downloads.getJobsAndStatuses") && response.ok(), - ); const signedIn = page.waitForURL((url) => url.origin === baseUrl && url.pathname === "/", { waitUntil: "commit", timeout: 60_000, }); await page.locator("css=button[type='submit']").click(); await signedIn; - await initialDownloadsResponse; const visibleBoard = page.locator("[data-homarr-dev-benchmark-board]").filter({ visible: true }).first(); await expect(visibleBoard).toBeVisible({ timeout: 30_000 }); @@ -169,7 +168,7 @@ describe("lazy widget application graph", () => { await expect(clockWidget.locator("time").filter({ visible: true }).first()).not.toHaveText("--:--"); await expect.poll(() => refreshRequestStarted, { timeout: 10_000 }).toBe(true); await expect(cachedDownload).toBeVisible({ timeout: 5_000 }); - await expect(downloadsWidget.locator('output[aria-live="polite"]')).toBeVisible(); + await expect(downloadsWidget.locator("[data-widget-refreshing]")).toBeVisible(); const refreshedDownloadsResponse = page.waitForResponse( (response) => response.url().includes("widget.downloads.getJobsAndStatuses") && response.ok(), @@ -177,7 +176,7 @@ describe("lazy widget application graph", () => { releaseRefresh(); releaseRefresh = undefined; await refreshedDownloadsResponse; - await expect(downloadsWidget.locator('output[aria-live="polite"]')).toHaveCount(0); + await expect(downloadsWidget.locator("[data-widget-refreshing]")).toHaveCount(0); await page.unroute(downloadsRequestPattern, delayDownloadsRefresh); expect(pageErrors).toEqual([]); expect(hydrationErrors).toEqual([]); diff --git a/packages/widgets/src/clock/world-clock.ts b/packages/widgets/src/clock/world-clock.ts index e951fa916..8ae090e87 100644 --- a/packages/widgets/src/clock/world-clock.ts +++ b/packages/widgets/src/clock/world-clock.ts @@ -61,11 +61,6 @@ export interface WorldClockTime { dayDifference: number | null; } -export const getResolvedLocalTimeZone = () => { - const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone; - return timeZone && isTimeZoneSupported(timeZone) ? timeZone : "UTC"; -}; - export const createWorldClockTime = ({ city, now, diff --git a/packages/widgets/src/common/widget-time-context.spec.tsx b/packages/widgets/src/common/widget-time-context.spec.tsx index 286c8f95f..2e091e7b0 100644 --- a/packages/widgets/src/common/widget-time-context.spec.tsx +++ b/packages/widgets/src/common/widget-time-context.spec.tsx @@ -10,7 +10,7 @@ import { useWidgetLocalTimeZone, WidgetTimeProvider } from "./widget-time-contex const TimeProbe = () => ( ); diff --git a/packages/widgets/src/common/widget-time-context.tsx b/packages/widgets/src/common/widget-time-context.tsx index d344d5380..347975369 100644 --- a/packages/widgets/src/common/widget-time-context.tsx +++ b/packages/widgets/src/common/widget-time-context.tsx @@ -1,7 +1,7 @@ "use client"; import type { PropsWithChildren } from "react"; -import { createContext, useContext, useSyncExternalStore } from "react"; +import { createContext, useContext, useMemo, useSyncExternalStore } from "react"; const WidgetTimestampContext = createContext(null); const WidgetTimeZoneContext = createContext("UTC"); @@ -15,15 +15,22 @@ export const WidgetTimeProvider = ({ children, initialTimestamp, initialTimeZone, -}: PropsWithChildren) => ( - - - {children} - - -); +}: PropsWithChildren) => { + const timeZone = useMemo(() => getSupportedTimeZone(initialTimeZone), [initialTimeZone]); + + return ( + + {children} + + ); +}; export const useWidgetInitialTimestamp = () => useContext(WidgetTimestampContext); + +/** + * Resolves to the browser time zone, but renders (and hydrates) with the time + * zone the server used so widgets can paint a time before hydration finishes. + */ export const useWidgetLocalTimeZone = () => { const initialTimeZone = useContext(WidgetTimeZoneContext); return useSyncExternalStore(subscribeToTimeZone, getResolvedLocalTimeZone, () => initialTimeZone); @@ -31,7 +38,15 @@ export const useWidgetLocalTimeZone = () => { const subscribeToTimeZone = () => () => undefined; -const getResolvedLocalTimeZone = () => getSupportedTimeZone(Intl.DateTimeFormat().resolvedOptions().timeZone); +// React calls the snapshot getter on every render of every consumer, and +// resolving a time zone builds `Intl.DateTimeFormat` instances, so keep the +// answer around: it cannot change without a new document. +let resolvedLocalTimeZone: string | undefined; + +const getResolvedLocalTimeZone = () => { + resolvedLocalTimeZone ??= getSupportedTimeZone(Intl.DateTimeFormat().resolvedOptions().timeZone); + return resolvedLocalTimeZone; +}; const getSupportedTimeZone = (timeZone: string | undefined) => { if (!timeZone) return "UTC"; diff --git a/packages/widgets/src/index.tsx b/packages/widgets/src/index.tsx index 86ea2599d..a9ee650fe 100644 --- a/packages/widgets/src/index.tsx +++ b/packages/widgets/src/index.tsx @@ -3,9 +3,7 @@ import type { Loader } from "next/dynamic"; import dynamic from "next/dynamic"; import { Center, Loader as UiLoader } from "@mantine/core"; -import { objectEntries } from "@homarr/common"; import type { IntegrationKind, WidgetKind } from "@homarr/definitions"; -import type { SettingsContextProps } from "@homarr/settings/creator"; import * as anchorNote from "./anchor-note"; import * as audioStats from "./audio-stats"; @@ -41,7 +39,6 @@ import * as notebook from "./notebook"; import * as paperlessNgx from "./paperless-ngx"; import * as patchmon from "./patchmon"; import * as notifications from "./notifications"; -import type { WidgetOptionDefinition } from "./options"; import * as releases from "./releases"; import * as rssFeed from "./rssFeed"; import * as smartHomeEntityState from "./smart-home/entity-state"; @@ -191,19 +188,3 @@ export type inferSupportedIntegrationsStrict = (Widget } ? WidgetImports[TKind]["definition"]["supportedIntegrations"] : never[])[number]; - -export const reduceWidgetOptionsWithDefaultValues = ( - kind: WidgetKind, - settings: Pick, - currentValue: Record = {}, -) => { - const definition = widgetImports[kind].definition; - const options = definition.createOptions(settings) as Record; - return objectEntries(options).reduce( - (prev, [key, value]) => ({ - ...prev, - [key]: currentValue[key] ?? value.defaultValue, - }), - {} as Record, - ); -}; diff --git a/packages/widgets/src/time.ts b/packages/widgets/src/time.ts index 07babe34b..180cb0d18 100644 --- a/packages/widgets/src/time.ts +++ b/packages/widgets/src/time.ts @@ -1,3 +1 @@ -export { WidgetTimeProvider, useWidgetInitialTimestamp, useWidgetLocalTimeZone } from "./common/widget-time-context"; -export { getNextWidgetTickDelay, useWidgetNow } from "./common/use-widget-now"; -export type { WidgetClockPrecision } from "./common/use-widget-now"; +export { WidgetTimeProvider } from "./common/widget-time-context";