From b39159148058a9e2e670924eb02000d599a94a40 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:28:23 -0700 Subject: [PATCH 01/13] refactor(ui): clear the react-hooks 7 purity, refs and static-components errors (#9588) useLocalStorage is rebuilt on useSyncExternalStore -- localStorage IS an external store, and subscribing to it directly removes both the mount-time setState-in-effect and the render-phase ref write the old shape needed. The snapshot is the raw string, a primitive, so it is stable by construction; the parse happens once per change. Incidentally removes the first-paint flash, since the stored value is now available on the first client render rather than one effect later. The docs route uses the client loader's useContent(path), which returns react NODES, instead of getComponent(path), which mints a component during render -- a fresh component identity each render remounts the whole page body. RetryCountdown reads a 250ms clock through useSyncExternalStore rather than seeding useState with Date.now() and ticking it from an effect. The snapshot is cached because a bare () => Date.now() is never stable and would re-render forever; one shared interval now serves every mounted countdown. --- apps/loopover-ui/src/lib/api/request.ts | 37 +++- .../src/lib/docs-client-loader.tsx | 6 +- apps/loopover-ui/src/lib/use-local-storage.ts | 181 +++++++++++++----- apps/loopover-ui/src/routes/docs.$slug.tsx | 6 +- 4 files changed, 169 insertions(+), 61 deletions(-) diff --git a/apps/loopover-ui/src/lib/api/request.ts b/apps/loopover-ui/src/lib/api/request.ts index 098a285abd..786ca8e987 100644 --- a/apps/loopover-ui/src/lib/api/request.ts +++ b/apps/loopover-ui/src/lib/api/request.ts @@ -1,4 +1,4 @@ -import { createElement, useEffect, useState } from "react"; +import { createElement, useSyncExternalStore } from "react"; import { toast } from "sonner"; import { @@ -220,6 +220,35 @@ function runRetryWithProgress( }); } +// A 250ms clock as an external store (#9588). Reading `Date.now()` during render is impure, and the +// setState-in-effect tick it replaces is exactly the pattern these rules exist to remove. +// +// The snapshot is CACHED rather than read per call: `useSyncExternalStore` requires a stable snapshot +// between notifications, and a bare `() => Date.now()` returns a new value every single call, which +// re-renders forever. One shared interval serves every mounted countdown and stops with the last one. +let clockNow = Date.now(); +let clockTimer: number | null = null; +const clockListeners = new Set<() => void>(); + +function subscribeToClock(onChange: () => void): () => void { + clockListeners.add(onChange); + if (clockTimer === null) { + clockTimer = window.setInterval(() => { + clockNow = Date.now(); + for (const listener of clockListeners) listener(); + }, 250); + } + return () => { + clockListeners.delete(onChange); + if (clockListeners.size === 0 && clockTimer !== null) { + window.clearInterval(clockTimer); + clockTimer = null; + } + }; +} + +const getClockNow = () => clockNow; + function RetryCountdown({ label, startedAt, @@ -229,11 +258,7 @@ function RetryCountdown({ startedAt: number; timeoutMs: number; }) { - const [now, setNow] = useState(Date.now()); - useEffect(() => { - const i = window.setInterval(() => setNow(Date.now()), 250); - return () => window.clearInterval(i); - }, []); + const now = useSyncExternalStore(subscribeToClock, getClockNow, getClockNow); const elapsed = Math.min(now - startedAt, timeoutMs); const remaining = Math.max(0, Math.ceil((timeoutMs - elapsed) / 1000)); const pct = Math.min(100, Math.round((elapsed / timeoutMs) * 100)); diff --git a/apps/loopover-ui/src/lib/docs-client-loader.tsx b/apps/loopover-ui/src/lib/docs-client-loader.tsx index 2cf861c2f2..e8a8626d55 100644 --- a/apps/loopover-ui/src/lib/docs-client-loader.tsx +++ b/apps/loopover-ui/src/lib/docs-client-loader.tsx @@ -10,7 +10,11 @@ import { docsMdxComponents } from "@/lib/docs-mdx-components"; // docs-source.ts -- never the live MDX component itself -- then this client loader turns that // path into the actual rendered content on both sides. export const docsClientLoader = browserCollections.docs.createClientLoader({ - component({ default: MDXContent }, _props: Record) { + // No props: that leaves the loader's `Props` as `undefined`, which is what makes `useContent(path)` + // callable without a props argument (#9588). `useContent` returns react NODES, where `getComponent` + // returns a component -- and minting a component inside a render is what react-hooks/static-components + // rejects, since a fresh identity each render remounts the whole subtree. + component({ default: MDXContent }) { return ; }, }); diff --git a/apps/loopover-ui/src/lib/use-local-storage.ts b/apps/loopover-ui/src/lib/use-local-storage.ts index e3c8136463..ddc5a06361 100644 --- a/apps/loopover-ui/src/lib/use-local-storage.ts +++ b/apps/loopover-ui/src/lib/use-local-storage.ts @@ -1,70 +1,151 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useState, useSyncExternalStore } from "react"; /** - * Tiny SSR-safe localStorage hook. Reads once on mount; writes are persisted - * synchronously and broadcast via a `storage` event for other tabs. + * One hook instance's view of a localStorage key, as a subscribable cell. * - * `legacyKey`, when given, is read as a one-time fallback if `key` is absent - * (a rebrand key-rename migration) -- the value found there is written - * forward to `key` immediately so every later read hits the new key - * directly. The legacy key is left in place, unremoved. + * The cell's snapshot is the RAW STRING, never the parsed value: a string is a primitive, so the + * snapshot is referentially stable by construction -- which is exactly what `useSyncExternalStore` + * demands (a fresh `JSON.parse` per snapshot read would re-render forever). Parsing happens once + * per change, in the hook. */ -export function useLocalStorage(key: string, initial: T, legacyKey?: string) { - const [value, setValue] = useState(initial); - const [hydrated, setHydrated] = useState(false); - // Call sites often pass a fresh `[]` / `{...}` literal each render; keep the - // listener keyed only on `key`/`legacyKey` and read the latest initial via ref. - const initialRef = useRef(initial); - initialRef.current = initial; +type StorageCell = { + subscribe: (onChange: () => void) => () => void; + getRaw: () => string | null; + /** Same-tab write: persist, update the cell, notify. `storage` never fires in the writing tab. */ + write: (raw: string) => void; + /** Re-read localStorage into the cell and notify, after the legacy migration writes forward. */ + refresh: () => void; +}; - useEffect(() => { +function createStorageCell(key: string, legacyKey: string | undefined): StorageCell { + const listeners = new Set<() => void>(); + let raw: string | null = null; + let loaded = false; + + /** Reads the new key, falling back to the legacy one so the value is right even before the + * migration effect has written it forward. Disabled storage (private mode) reads as absent. */ + const read = (): string | null => { try { - const raw = window.localStorage.getItem(key); - if (raw !== null) { - setValue(JSON.parse(raw) as T); - } else if (legacyKey) { - const legacyRaw = window.localStorage.getItem(legacyKey); - if (legacyRaw !== null) { - setValue(JSON.parse(legacyRaw) as T); - window.localStorage.setItem(key, legacyRaw); - } - } + const own = window.localStorage.getItem(key); + if (own !== null) return own; + return legacyKey === undefined ? null : window.localStorage.getItem(legacyKey); } catch { - /* ignore */ + return null; } - setHydrated(true); + }; + + const set = (next: string | null): void => { + if (loaded && next === raw) return; + raw = next; + loaded = true; + for (const listener of listeners) listener(); + }; - // Cross-tab sync: the browser fires `storage` only in *other* same-origin tabs - // (never the tab that wrote). Same-tab writes already update state via `update()`. - const onStorage = (event: StorageEvent) => { - if (event.key !== key && (!legacyKey || event.key !== legacyKey)) return; - if (event.newValue === null) { - setValue(initialRef.current); - return; + return { + getRaw: () => { + if (!loaded) { + raw = read(); + loaded = true; } + return raw; + }, + subscribe: (onChange) => { + listeners.add(onChange); + // Cross-tab sync: the browser fires `storage` only in *other* same-origin tabs (never the tab + // that wrote). The event's own payload is used rather than a re-read -- the writing tab has + // already persisted it, so the payload IS the value, and using it avoids a read-back race. + const onStorage = (event: StorageEvent) => { + if (event.key !== key && (legacyKey === undefined || event.key !== legacyKey)) return; + set(event.newValue); + }; + window.addEventListener("storage", onStorage); + return () => { + listeners.delete(onChange); + window.removeEventListener("storage", onStorage); + }; + }, + write: (next) => { try { - setValue(JSON.parse(event.newValue) as T); + window.localStorage.setItem(key, next); } catch { - /* ignore */ + /* ignore quota */ } - }; - window.addEventListener("storage", onStorage); - return () => window.removeEventListener("storage", onStorage); - }, [key, legacyKey]); + set(next); + }, + refresh: () => set(read()), + }; +} + +/** Server render (and the hydration pass) sees no storage at all, so every key reads as absent. */ +const getServerRaw = (): string | null => null; + +/** `hydrated` as an external store rather than a `setState` in an effect: it never changes after + * mount, so the subscription is a no-op and the two snapshots carry the whole signal. */ +const subscribeToHydration = () => () => {}; +const isHydrated = () => true; +const isNotHydrated = () => false; + +/** An absent key, or one holding something that is not JSON, both read as the initial value. */ +function parseRaw(raw: string | null, fallback: T): T { + if (raw === null) return fallback; + try { + return JSON.parse(raw) as T; + } catch { + return fallback; + } +} + +/** + * Tiny SSR-safe localStorage hook. Reads synchronously through `useSyncExternalStore`; writes are + * persisted immediately and broadcast via a `storage` event for other tabs. + * + * `legacyKey`, when given, is read as a one-time fallback if `key` is absent (a rebrand key-rename + * migration) -- the value found there is written forward to `key` immediately so every later read + * hits the new key directly. The legacy key is left in place, unremoved. + * + * Built on `useSyncExternalStore` rather than `useState` plus a mount effect (#9588). localStorage + * IS an external store, and subscribing to it directly removes both the mount-time + * `setState`-in-effect and the render-phase ref write the old shape needed -- and, incidentally, + * the first-paint flash of the initial value, since the stored value is now available on the very + * first client render rather than one effect later. + */ +export function useLocalStorage(key: string, initial: T, legacyKey?: string) { + // The fallback for an absent or unparseable value, captured once. Call sites often pass a fresh + // `[]` / `{...}` literal each render, and holding the FIRST one is what keeps the identity of the + // returned value stable across renders. A `useState` initializer rather than a ref: React + // guarantees it runs exactly once, and reading a ref during render is itself disallowed. + const [initialValue] = useState(() => initial); + + const cell = useMemo(() => createStorageCell(key, legacyKey), [key, legacyKey]); + + const raw = useSyncExternalStore(cell.subscribe, cell.getRaw, getServerRaw); + const hydrated = useSyncExternalStore(subscribeToHydration, isHydrated, isNotHydrated); + + const value = useMemo(() => parseRaw(raw, initialValue), [raw, initialValue]); + + // The one-time forward migration. A write is a side effect, so it lives here rather than in the + // cell's read path; that read path's own legacy fallback means the value is already correct + // whether or not this has run yet. + useEffect(() => { + if (legacyKey === undefined) return; + try { + if (window.localStorage.getItem(key) !== null) return; + const legacyRaw = window.localStorage.getItem(legacyKey); + if (legacyRaw === null) return; + window.localStorage.setItem(key, legacyRaw); + cell.refresh(); + } catch { + /* ignore */ + } + }, [cell, key, legacyKey]); const update = useCallback( (next: T | ((prev: T) => T)) => { - setValue((prev) => { - const resolved = typeof next === "function" ? (next as (p: T) => T)(prev) : next; - try { - window.localStorage.setItem(key, JSON.stringify(resolved)); - } catch { - /* ignore quota */ - } - return resolved; - }); + const previous = parseRaw(cell.getRaw(), initialValue); + const resolved = typeof next === "function" ? (next as (p: T) => T)(previous) : next; + cell.write(JSON.stringify(resolved)); }, - [key], + [cell, initialValue], ); return [value, update, hydrated] as const; diff --git a/apps/loopover-ui/src/routes/docs.$slug.tsx b/apps/loopover-ui/src/routes/docs.$slug.tsx index ac613ae80b..ab293ad8d3 100644 --- a/apps/loopover-ui/src/routes/docs.$slug.tsx +++ b/apps/loopover-ui/src/routes/docs.$slug.tsx @@ -42,12 +42,10 @@ export const Route = createFileRoute("/docs/$slug")({ function DocsSlugPage() { const { path, title, description, eyebrow } = Route.useLoaderData(); - const Content = docsClientLoader.getComponent(path); + const content = docsClientLoader.useContent(path); return ( - }> - - + }>{content} ); } From cf6d868450f4c96458e51530f4c801f7d9708b60 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:56:53 -0700 Subject: [PATCH 02/13] refactor(ui): derive loading and progress-bar visibility instead of writing them from effects (#9588) useApiResource keys its settled result by the exact inputs it was fetched for, so both "loading" and the stale-response guard (#7785) fall out of comparing that key against the current inputs -- the request-id ref did the same job by hand, and a disabled resource no longer renders a frame of loading before an effect corrects it. usePreviewDataState derives isLoading from which version has settled, so bumping the version re-enters loading on the same render that requested the refresh. ApiProgressBar shows on the render that sees activity start, via React's documented adjust-state-during-render pattern; only the 240ms trailing fade remains a timer, whose setState runs in the timer callback rather than an effect body. --- .../src/components/site/api-progress-bar.tsx | 17 ++- .../src/components/site/state-views.tsx | 9 +- .../src/lib/api/use-api-resource.ts | 100 +++++++++++------- 3 files changed, 80 insertions(+), 46 deletions(-) diff --git a/apps/loopover-ui/src/components/site/api-progress-bar.tsx b/apps/loopover-ui/src/components/site/api-progress-bar.tsx index ae5688c8b7..9f8291ec34 100644 --- a/apps/loopover-ui/src/components/site/api-progress-bar.tsx +++ b/apps/loopover-ui/src/components/site/api-progress-bar.tsx @@ -11,11 +11,20 @@ export function ApiProgressBar() { const active = inFlight > 0 || status === "loading"; const [visible, setVisible] = useState(false); + // Becoming active shows the bar IMMEDIATELY, adjusted during render rather than from an effect (#9588): + // React's documented pattern for reacting to a changed input, and it renders the bar in the same commit + // that saw the change instead of a frame later. + const [wasActive, setWasActive] = useState(active); + if (wasActive !== active) { + setWasActive(active); + if (active) setVisible(true); + } + + // Only the trailing fade-out is a timer, and its setState runs in the timer callback -- not synchronously + // in the effect body. The bar lingers 240ms after the last request settles so a burst of quick calls does + // not strobe it. useEffect(() => { - if (active) { - setVisible(true); - return; - } + if (active) return; const t = window.setTimeout(() => setVisible(false), 240); return () => clearTimeout(t); }, [active]); diff --git a/apps/loopover-ui/src/components/site/state-views.tsx b/apps/loopover-ui/src/components/site/state-views.tsx index c142ba9c9c..7446949960 100644 --- a/apps/loopover-ui/src/components/site/state-views.tsx +++ b/apps/loopover-ui/src/components/site/state-views.tsx @@ -26,11 +26,14 @@ export function StateBoundary(props: ComponentProps) export function usePreviewDataState(label: string, delay = 220) { const [version, setVersion] = useState(0); - const [isLoading, setIsLoading] = useState(true); + // `isLoading` is DERIVED from which version has finished settling (#9588), rather than written to `true` + // synchronously at the top of the effect. Bumping `version` therefore re-enters the loading state on the + // very same render that requested the refresh, with no cascading second render to correct it. + const [settledVersion, setSettledVersion] = useState(-1); + const isLoading = settledVersion !== version; useEffect(() => { - setIsLoading(true); - const timer = window.setTimeout(() => setIsLoading(false), delay); + const timer = window.setTimeout(() => setSettledVersion(version), delay); return () => window.clearTimeout(timer); }, [delay, version]); diff --git a/apps/loopover-ui/src/lib/api/use-api-resource.ts b/apps/loopover-ui/src/lib/api/use-api-resource.ts index 3061ead78e..837288ff7b 100644 --- a/apps/loopover-ui/src/lib/api/use-api-resource.ts +++ b/apps/loopover-ui/src/lib/api/use-api-resource.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; import { getApiOrigin } from "./origin"; import { apiFetch, type ApiFailureKind } from "./request"; @@ -16,10 +16,30 @@ type ResourceState = loadedAt: null; }; +/** What a finished load produced. "loading" is not a member: it is the ABSENCE of a settled result for the + * inputs currently being asked about, derived at the return below rather than stored. */ +type SettledState = Exclude, { status: "loading" }>; + type UseApiResourceOptions = { enabled?: boolean; }; +/** The synthetic sentinel a disabled resource reports. Frozen at module scope so every disabled resource + * returns the same object rather than a fresh one per render. */ +const DISABLED_STATE: ResourceState = Object.freeze({ + status: "error", + data: null, + error: "disabled", + loadedAt: null, +}); + +const LOADING_STATE: ResourceState = Object.freeze({ + status: "loading", + data: null, + error: null, + loadedAt: null, +}); + export function useApiResource( path: string, label: string, @@ -27,27 +47,17 @@ export function useApiResource( options: UseApiResourceOptions = {}, ) { const enabled = options.enabled ?? true; - const [state, setState] = useState>({ - status: "loading", - data: null, - error: null, - loadedAt: null, - }); - const requestIdRef = useRef(0); + // The exact inputs a stored result belongs to. Comparing this against the CURRENT inputs is what makes + // both "loading" and the stale-response guard (#7785) derivable rather than bookkeeping: when `path` + // changes (pagination offsets, free-text repo input, window selection) a new load starts while an older + // apiFetch is still in flight, and the older one's result carries the OLD key, so it can neither be + // reported nor overwrite the newer request's. The request-id ref this replaces did the same job by hand. + const key = JSON.stringify([path, label, token ?? null]); + const [settled, setSettled] = useState<{ key: string; state: SettledState } | null>(null); const load = useCallback(async () => { - // Guard against out-of-order responses (#7785): when `path` changes (pagination offsets, free-text repo input, - // window selection) a new load starts while an older apiFetch is still in flight. Tag each load and, after the - // await, drop the result if a newer load has since superseded it — otherwise a stale page's response resolves - // last and silently overwrites the current one while the surrounding UI reflects the newer request. - const requestId = requestIdRef.current + 1; - requestIdRef.current = requestId; - if (!enabled) { - setState({ status: "error", data: null, error: "disabled", loadedAt: null }); - return; - } - setState({ status: "loading", data: null, error: null, loadedAt: null }); + if (!enabled) return; const headers: Record = { Accept: "application/json" }; if (token) headers.Authorization = `Bearer ${token}`; const result = await apiFetch(`${getApiOrigin().replace(/\/$/, "")}${path}`, { @@ -55,29 +65,41 @@ export function useApiResource( headers, credentials: "include", }); - // A newer load superseded this one (the path changed mid-flight); drop this stale response entirely. - if (requestId !== requestIdRef.current) return; - if (result.ok) { - setState({ status: "ready", data: result.data, error: null, loadedAt: Date.now() }); - } else { - setState({ - status: "error", - data: null, - error: result.message, - errorKind: result.kind, - errorStatus: result.status, - loadedAt: null, - }); - } - }, [enabled, label, path, token]); + setSettled({ + key, + state: result.ok + ? { status: "ready", data: result.data, error: null, loadedAt: Date.now() } + : { + status: "error", + data: null, + error: result.message, + errorKind: result.kind, + errorStatus: result.status, + loadedAt: null, + }, + }); + }, [enabled, key, label, path, token]); useEffect(() => { - if (!enabled) { - setState({ status: "error", data: null, error: "disabled", loadedAt: null }); - return; - } + if (!enabled) return; void load(); }, [enabled, load]); - return { ...state, reload: load }; + /** Explicit refresh. Clears to `loading` first — a caller who asked for a reload wants to see one, and + * this runs from an event handler rather than an effect, so nothing cascades. */ + const reload = useCallback(async () => { + setSettled(null); + await load(); + }, [load]); + + // Every state here is DERIVED (#9588): nothing is written into state synchronously from an effect, which + // is what the old shape did twice — once for "disabled" and once for "loading". Storing "disabled" also + // meant a disabled resource rendered one frame of `loading` before the effect corrected it. + const state: ResourceState = !enabled + ? (DISABLED_STATE as ResourceState) + : settled !== null && settled.key === key + ? settled.state + : (LOADING_STATE as ResourceState); + + return { ...state, reload }; } From 55622df96376ea952cbc365c87871c28ccb6e7a0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:59:12 -0700 Subject: [PATCH 03/13] refactor(ui): derive banner dismissal, palette highlight and the sidebar cookie (#9588) The api-status-banner effect changed no rendered output: `dismissed` already compares the stored key against the current one, so a stale key reads as not-dismissed by derivation. The effect only scrubbed state nothing consulted. The command palette resets its highlighted index during render, so the reset lands in the same commit as the new filtered list rather than a frame after it. The sidebar's open state is a cookie -- an external store, now read as one instead of copied into state by a mount effect. The server snapshot stays undefined, which is what the hydration gate already keys on, so output is unchanged. --- .../src/components/site/api-status-banner.tsx | 9 ++--- .../src/components/site/app-shell.tsx | 35 +++++++++++++++---- .../src/components/site/command-palette.tsx | 13 ++++--- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/apps/loopover-ui/src/components/site/api-status-banner.tsx b/apps/loopover-ui/src/components/site/api-status-banner.tsx index 9088f31783..f30af03fc7 100644 --- a/apps/loopover-ui/src/components/site/api-status-banner.tsx +++ b/apps/loopover-ui/src/components/site/api-status-banner.tsx @@ -43,12 +43,9 @@ export function ApiStatusBanner() { const visibleKey = severity ? `${connection}:${status}` : null; const dismissed = visibleKey !== null && dismissedKey === visibleKey; - // If status changes to a new failure mode, un-dismiss. - useEffect(() => { - if (visibleKey && dismissedKey && dismissedKey !== visibleKey) { - setDismissedKey(null); - } - }, [visibleKey, dismissedKey]); + // A new failure mode un-dismisses the banner by DERIVATION: `dismissed` above compares the stored key + // against the current one, so a stale key already reads as not-dismissed (#9588). The effect that used to + // null it out changed no rendered output -- it only scrubbed state nothing consulted. if (!severity || dismissed) return null; diff --git a/apps/loopover-ui/src/components/site/app-shell.tsx b/apps/loopover-ui/src/components/site/app-shell.tsx index 2a2fc7ed80..e29dca9236 100644 --- a/apps/loopover-ui/src/components/site/app-shell.tsx +++ b/apps/loopover-ui/src/components/site/app-shell.tsx @@ -14,7 +14,7 @@ import { Workflow, } from "lucide-react"; import type { ComponentType } from "react"; -import { useEffect, useState } from "react"; +import { useEffect, useState, useSyncExternalStore } from "react"; import { PREVIEW_SESSION_ALLOWED, useSession } from "@/lib/api/session"; import { describeApiStatus, pingHealth, useApiStatus } from "@/lib/api/status"; @@ -34,6 +34,24 @@ import { SidebarTrigger, } from "@/components/ui/sidebar"; import { LoopOverMark } from "./mark"; + +/** The sidebar's persisted open/closed state, read straight from its cookie. A primitive snapshot, so it + * is referentially stable for `useSyncExternalStore` by construction. Defaults to open when unset. */ +function readSidebarCookie(): boolean | undefined { + const match = document.cookie.match(/(?:^|; )sidebar_state=([^;]+)/); + return match ? match[1] === "true" : true; +} + +/** No cookie exists during server render or the hydration pass; `undefined` is what the gate below keys on. */ +function readNoSidebarCookie(): boolean | undefined { + return undefined; +} + +/** The cookie is written by the sidebar itself and never changes underneath this component, so there is + * nothing to subscribe to -- the same posture the mount effect this replaces already had. */ +function subscribeToSidebarCookie(): () => void { + return () => {}; +} import { StatusPill, type Status } from "./control-primitives"; import { LoadingState } from "./state-views"; import { cn } from "@/lib/utils"; @@ -103,12 +121,15 @@ export function AppShell() { const loc = useLocation(); const navigate = useNavigate(); const routerState = useRouterState(); - const [sidebarOpen, setSidebarOpen] = useState(undefined); - - useEffect(() => { - const m = document.cookie.match(/(?:^|; )sidebar_state=([^;]+)/); - setSidebarOpen(m ? m[1] === "true" : true); - }, []); + // The sidebar's persisted state lives in a cookie -- an external store, so it is READ as one (#9588) + // rather than copied into state by a mount effect. The server snapshot stays `undefined`, which is what + // the hydration gate below already keys on, so the rendered output is unchanged; the difference is that + // the real value is available on the first client render instead of one cascading render later. + const sidebarOpen = useSyncExternalStore( + subscribeToSidebarCookie, + readSidebarCookie, + readNoSidebarCookie, + ); // Preview deploys: when this is a preview build (VITE_PREVIEW) and the URL carries `?preview=1`, start // the synthetic demo session automatically once hydration confirms there's no real session. This lets the diff --git a/apps/loopover-ui/src/components/site/command-palette.tsx b/apps/loopover-ui/src/components/site/command-palette.tsx index 4f2c63878c..6d6791a616 100644 --- a/apps/loopover-ui/src/components/site/command-palette.tsx +++ b/apps/loopover-ui/src/components/site/command-palette.tsx @@ -94,11 +94,16 @@ export function CommandPalette({ items = DEFAULT_ITEMS }: { items?: PaletteItem[ return items.filter((i) => `${i.label} ${i.group ?? ""}`.toLowerCase().includes(term)); }, [q, items]); - // The highlighted result resets whenever the filtered set changes or the palette re-opens, so a - // stale index from a previous, longer list can never point past the current end. - useEffect(() => { + // The highlighted result resets whenever the filtered set changes or the palette re-opens, so a stale + // index from a previous, longer list can never point past the current end. Adjusted during render + // (React's documented pattern for reacting to changed inputs) rather than from an effect (#9588): the + // reset lands in the same commit as the new list, so no frame ever renders the old index against it. + const resetKey = `${open}:${filtered.length}:${filtered.map((i) => i.to).join("\u0000")}`; + const [highlightResetKey, setHighlightResetKey] = useState(resetKey); + if (highlightResetKey !== resetKey) { + setHighlightResetKey(resetKey); setHighlightedIndex(0); - }, [filtered, open]); + } function selectItem(item: PaletteItem | undefined) { if (!item) return; From 5b7b8bfa1be043beab85cbc82368ec6a983e43ac Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:02:31 -0700 Subject: [PATCH 04/13] refactor(ui): read the stats cache, hydration and motion preference as external stores (#9588) github-stats-chip read sessionStorage into state from a mount effect; sessionStorage IS an external store, so it is read as one. The snapshot memoizes on the raw string, because re-parsing per call returns a fresh object every render and loops forever. useCountUp sampled prefers-reduced-motion and tab visibility inside its effect and then wrote the final value synchronously when either said "no animation". Both are genuinely subscribable -- a matchMedia change list and visibilitychange -- so they are a store, which lets the no-animation case be derived: the value simply IS the target. Only the rAF and safety-net callbacks still set state, which they may. --- .../src/components/site/github-stats-chip.tsx | 58 +++++++++++++---- .../components/site/proof-of-power-stats.tsx | 63 ++++++++++++++----- 2 files changed, 95 insertions(+), 26 deletions(-) diff --git a/apps/loopover-ui/src/components/site/github-stats-chip.tsx b/apps/loopover-ui/src/components/site/github-stats-chip.tsx index 8afe12855b..5652d74cd3 100644 --- a/apps/loopover-ui/src/components/site/github-stats-chip.tsx +++ b/apps/loopover-ui/src/components/site/github-stats-chip.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, useSyncExternalStore } from "react"; import { useQuery } from "@tanstack/react-query"; import { Github, Star, GitFork, RefreshCw } from "lucide-react"; import { toast } from "sonner"; @@ -20,11 +20,47 @@ function finiteCount(value: unknown): number { return typeof value === "number" && Number.isFinite(value) && value >= 0 ? Math.trunc(value) : 0; } -function readCache(): Cached | null { +/** `mounted` as an external store: it never changes after hydration, so the subscription is a no-op and + * the two snapshots carry the whole signal. */ +const subscribeToHydration = () => () => {}; +const isHydrated = () => true; +const isNotHydrated = () => false; + +/** sessionStorage is not observable and is only written by this chip, so there is nothing to subscribe to. + * The snapshot is memoized on the raw string so `useSyncExternalStore` sees a stable object between + * changes -- re-parsing per call would return a fresh object every render and loop forever. */ +const subscribeToStatsCache = () => () => {}; +let cachedStatsRaw: string | null = null; +let cachedStatsValue: Cached | null = null; + +function readCachedStats(): Cached | null { + const raw = readCacheRaw(); + if (raw !== cachedStatsRaw) { + cachedStatsRaw = raw; + cachedStatsValue = readCache(); + } + return cachedStatsValue; +} + +function readNoCachedStats(): Cached | null { + return null; +} + +/** The cache's raw string, which is what the memo above compares -- a primitive, so it is a sound + * identity for "has the cache changed". Disabled storage reads as absent. */ +function readCacheRaw(): string | null { if (typeof window === "undefined") return null; try { - const raw = window.sessionStorage.getItem(CACHE_KEY); - if (!raw) return null; + return window.sessionStorage.getItem(CACHE_KEY); + } catch { + return null; + } +} + +function readCache(): Cached | null { + const raw = readCacheRaw(); + if (!raw) return null; + try { const parsed = JSON.parse(raw) as Cached; return parsed?.stats ? parsed : null; } catch { @@ -79,13 +115,13 @@ async function fetchRepo(): Promise { const compact = new Intl.NumberFormat("en", { notation: "compact", maximumFractionDigits: 1 }); export function GithubStatsChip({ className }: { className?: string }) { - // Avoid SSR/CSR mismatch: only read sessionStorage after mount. - const [mounted, setMounted] = useState(false); - const [cached, setCached] = useState(null); - useEffect(() => { - setCached(readCache()); - setMounted(true); - }, []); + // Avoid SSR/CSR mismatch: sessionStorage is only readable after mount. Both facts are read as external + // stores (#9588) rather than copied into state by a mount effect -- sessionStorage IS one, and "have we + // hydrated yet" is exactly the server-snapshot-vs-client-snapshot distinction useSyncExternalStore + // exists to express. The cached stats are therefore available on the first client render, so the chip + // seeds react-query below without a cascading render in between. + const mounted = useSyncExternalStore(subscribeToHydration, isHydrated, isNotHydrated); + const cached = useSyncExternalStore(subscribeToStatsCache, readCachedStats, readNoCachedStats); const { data, isError, isFetching, isLoading, refetch } = useQuery({ queryKey: ["gh-repo", REPO], diff --git a/apps/loopover-ui/src/components/site/proof-of-power-stats.tsx b/apps/loopover-ui/src/components/site/proof-of-power-stats.tsx index 3415083c71..8ef0ef84f8 100644 --- a/apps/loopover-ui/src/components/site/proof-of-power-stats.tsx +++ b/apps/loopover-ui/src/components/site/proof-of-power-stats.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, useSyncExternalStore } from "react"; import { useQuery } from "@tanstack/react-query"; import { Link } from "@tanstack/react-router"; @@ -31,20 +31,52 @@ async function fetchPublicStats(): Promise { return result.data; } +/** + * Whether a count-up may animate at all: motion is not suppressed, rAF exists, and the tab is visible. + * + * Read as an external store (#9588) rather than sampled inside the effect, because both inputs genuinely + * ARE subscribable -- a `matchMedia` change list and `visibilitychange`. That is what lets the + * no-animation case be DERIVED below instead of written to state synchronously from an effect body. + */ +const MOTION_QUERY = "(prefers-reduced-motion: reduce)"; + +function subscribeToMotionAllowed(onChange: () => void): () => void { + const media = window.matchMedia?.(MOTION_QUERY); + media?.addEventListener("change", onChange); + document.addEventListener("visibilitychange", onChange); + return () => { + media?.removeEventListener("change", onChange); + document.removeEventListener("visibilitychange", onChange); + }; +} + +function motionAllowed(): boolean { + if (typeof window === "undefined" || typeof requestAnimationFrame === "undefined") return false; + if (window.matchMedia?.(MOTION_QUERY)?.matches) return false; + return typeof document === "undefined" || document.visibilityState === "visible"; +} + +/** No animation during server render -- the value is simply the target. */ +function motionAllowedOnServer(): boolean { + return false; +} + /** Count up to `target` once on mount (and on later increases), honoring prefers-reduced-motion. */ function useCountUp(target: number, durationMs = 900): number { - const [value, setValue] = useState(0); + const canAnimate = useSyncExternalStore( + subscribeToMotionAllowed, + motionAllowed, + motionAllowedOnServer, + ); + const shouldAnimate = canAnimate && target > 0; + const [progress, setProgress] = useState(0); const fromRef = useRef(0); + useEffect(() => { - const reduce = - typeof window !== "undefined" && - window.matchMedia?.("(prefers-reduced-motion: reduce)")?.matches; - const canAnimate = - typeof requestAnimationFrame !== "undefined" && - (typeof document === "undefined" || document.visibilityState === "visible"); - if (reduce || target <= 0 || !canAnimate) { + // Not animating: the value IS the target, derived at the return below. Only the ref still needs + // updating, so a later increase counts up from where this one landed rather than from zero. + if (!shouldAnimate) { fromRef.current = target; - setValue(target); return; } const from = fromRef.current; @@ -53,23 +85,24 @@ function useCountUp(target: number, durationMs = 900): number { const tick = (t: number) => { const p = Math.min(1, (t - start) / durationMs); const eased = 1 - Math.pow(1 - p, 3); - setValue(Math.round(from + (target - from) * eased)); + setProgress(Math.round(from + (target - from) * eased)); if (p < 1) raf = requestAnimationFrame(tick); else fromRef.current = target; }; raf = requestAnimationFrame(tick); - // Safety net: rAF is throttled/never fires in a background tab, which would freeze the count at 0. Guarantee + // Safety net: rAF is throttled/never fires in a background tab, which would freeze the count. Guarantee // the real number lands regardless after the animation window. const settle = window.setTimeout(() => { fromRef.current = target; - setValue(target); + setProgress(target); }, durationMs + 250); return () => { cancelAnimationFrame(raf); window.clearTimeout(settle); }; - }, [target, durationMs]); - return value; + }, [target, durationMs, shouldAnimate]); + + return shouldAnimate ? progress : target; } function Num({ value }: { value: number }) { From 5e68ebe9b034669a1361e37d00bf9ace43c4af20 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:04:08 -0700 Subject: [PATCH 05/13] refactor(ui): derive the chat-QA selection and the command preview (#9588) chat-qa-panel back-filled its select with the first eligible PR from an effect; the list arrives asynchronously, so that left one render showing an empty select before correcting itself. The value is now derived from the choice and the list together. commands-panel keys its preview by the exact inputs it was fetched for, so clearing on change is a derivation rather than a synchronous write at the top of the effect -- and a stale in-flight response now lands under the old key and can never be shown. --- .../site/app-panels/chat-qa-panel.tsx | 14 ++++++------- .../site/app-panels/commands-panel.tsx | 20 +++++++++++++++---- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/apps/loopover-ui/src/components/site/app-panels/chat-qa-panel.tsx b/apps/loopover-ui/src/components/site/app-panels/chat-qa-panel.tsx index e0d52ee192..085af3d69d 100644 --- a/apps/loopover-ui/src/components/site/app-panels/chat-qa-panel.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/chat-qa-panel.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState, type ReactNode } from "react"; +import { useMemo, useState, type ReactNode } from "react"; import { MessageCircle, RefreshCw, Send } from "lucide-react"; import { StatusPill, type Status } from "@/components/site/control-primitives"; @@ -29,16 +29,16 @@ type ReviewabilityRow = { pr: string; title: string; chatQaEnabled: boolean }; */ export function ChatQaPanel({ reviewability }: { reviewability: ReviewabilityRow[] }) { const eligible = useMemo(() => reviewability.filter((row) => row.chatQaEnabled), [reviewability]); - const [selectedPr, setSelectedPr] = useState(eligible[0]?.pr ?? ""); + // The chosen PR, or the first eligible one when nothing is chosen yet. DERIVED (#9588) rather than + // back-filled by an effect: the list arrives asynchronously, and the effect used to leave one render + // showing an empty select before correcting itself. + const [chosenPr, setChosenPr] = useState(""); + const selectedPr = chosenPr || (eligible[0]?.pr ?? ""); const [question, setQuestion] = useState(""); const [result, setResult] = useState(null); const [error, setError] = useState(null); const [busy, setBusy] = useState(false); - useEffect(() => { - if (!selectedPr && eligible[0]) setSelectedPr(eligible[0].pr); - }, [selectedPr, eligible]); - if (eligible.length === 0) return null; async function ask() { @@ -98,7 +98,7 @@ export function ChatQaPanel({ reviewability }: { reviewability: ReviewabilityRow