diff --git a/apps/loopover-ui/eslint.config.ts b/apps/loopover-ui/eslint.config.ts index 4a12a6bb72..14903ce5b1 100644 --- a/apps/loopover-ui/eslint.config.ts +++ b/apps/loopover-ui/eslint.config.ts @@ -25,12 +25,10 @@ export default tseslint.config( // because they flagged 24 pre-existing files. #9588 fixed them: purity, refs and static-components are // now clean and back at error, where the recommended preset puts them. // - // set-state-in-effect stays at warn for ONE remaining site: docs-toc reads its headings out of the - // rendered DOM. Sourcing them from the compiled MDX `toc` instead is the real fix, but the component - // lives in the docs LAYOUT while that data lives in the child route -- and it also serves docs pages - // that are not MDX at all and so have no compiled toc. That is its own change, tracked in #9872; - // every other call site in this app is already clean. - "react-hooks/set-state-in-effect": "warn", + // set-state-in-effect is at ERROR as of #9872, which removed the last site: docs-toc built its rail + // by scanning the rendered DOM and reading the result into state from an effect. It now renders the + // page's compiled MDX `toc`, so there is nothing to discover and nothing to set. + "react-hooks/set-state-in-effect": "error", "no-restricted-imports": [ "error", { diff --git a/apps/loopover-ui/src/components/site/docs-page.tsx b/apps/loopover-ui/src/components/site/docs-page.tsx index 4cd9ed723a..cf7baccbbd 100644 --- a/apps/loopover-ui/src/components/site/docs-page.tsx +++ b/apps/loopover-ui/src/components/site/docs-page.tsx @@ -27,7 +27,7 @@ export function DocsPage({

{description}

)} -
+
{children}
diff --git a/apps/loopover-ui/src/components/site/docs-toc.test.tsx b/apps/loopover-ui/src/components/site/docs-toc.test.tsx new file mode 100644 index 0000000000..f821a03e4b --- /dev/null +++ b/apps/loopover-ui/src/components/site/docs-toc.test.tsx @@ -0,0 +1,103 @@ +import { render, screen } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { DocsToc, type TocHeading } from "./docs-toc"; + +// #9872: the rail used to scan the rendered `article.prose-docs` for h2/h3 and read the result into state +// from an effect. It renders the page's COMPILED toc now, so these drive it the way the app does -- by +// handing it items -- and pin the properties the DOM scrape used to provide implicitly. + +vi.mock("@tanstack/react-router", () => ({ + useLocation: () => ({ pathname: "/docs/example" }), + useChildMatches: () => [], +})); + +const items: TocHeading[] = [ + { id: "first", text: "First section", level: 2 }, + { id: "second", text: "Second section", level: 2 }, + { id: "nested", text: "Nested", level: 3 }, +]; + +beforeEach(() => { + window.localStorage.clear(); + vi.stubGlobal( + "IntersectionObserver", + class { + observe() {} + disconnect() {} + unobserve() {} + takeRecords() { + return []; + } + root = null; + rootMargin = ""; + thresholds = []; + }, + ); +}); + +describe("DocsToc (#9872)", () => { + it("renders one link per compiled toc entry, anchored to its id", () => { + render(); + expect(screen.getByRole("link", { name: "First section" }).getAttribute("href")).toBe("#first"); + expect(screen.getByRole("link", { name: "Nested" }).getAttribute("href")).toBe("#nested"); + }); + + it("renders a heading's inline markup instead of flattening it", () => { + // The concrete gain over the DOM scrape, which read `textContent`: 29 of this repo's docs headings + // carry inline code, and the rail used to show them as bare text. + render( + wantedPaths, level: 3 }, + { id: "b", text: "Plain", level: 2 }, + ]} + />, + ); + expect(screen.getByRole("link", { name: "wantedPaths" }).querySelector("code")).not.toBeNull(); + }); + + it("indents depth-3 entries and leaves depth-2 flush", () => { + render(); + expect(screen.getByRole("link", { name: "Nested" }).closest("li")?.className).toContain("pl-3"); + expect( + screen.getByRole("link", { name: "First section" }).closest("li")?.className, + ).not.toContain("pl-3"); + }); + + it("renders nothing for a page with fewer than two headings — a one-item rail is noise", () => { + const { container } = render(); + expect(container.firstChild).toBeNull(); + expect(render().container.firstChild).toBeNull(); + }); + + it("marks the remembered section current on first render, without an effect", () => { + // Restored during render from localStorage rather than set from an effect, which is what lets + // react-hooks/set-state-in-effect go back to `error`. + window.localStorage.setItem("docs-toc:v2:/docs/example", "second"); + render(); + expect(screen.getByRole("link", { name: "Second section" }).getAttribute("aria-current")).toBe( + "location", + ); + expect( + screen.getByRole("link", { name: "First section" }).getAttribute("aria-current"), + ).toBeNull(); + }); + + it("ignores a remembered section that is not on this page", () => { + window.localStorage.setItem("docs-toc:v2:/docs/example", "from-a-different-page"); + render(); + for (const item of items) + expect( + screen.getByRole("link", { name: String(item.text) }).getAttribute("aria-current"), + ).toBeNull(); + }); + + it("survives localStorage throwing (Safari private mode)", () => { + const getItem = vi.spyOn(Storage.prototype, "getItem").mockImplementation(() => { + throw new Error("denied"); + }); + expect(() => render()).not.toThrow(); + getItem.mockRestore(); + }); +}); diff --git a/apps/loopover-ui/src/components/site/docs-toc.tsx b/apps/loopover-ui/src/components/site/docs-toc.tsx index 64831b17cb..a6cfa4a0d9 100644 --- a/apps/loopover-ui/src/components/site/docs-toc.tsx +++ b/apps/loopover-ui/src/components/site/docs-toc.tsx @@ -1,104 +1,119 @@ -import { useEffect, useState } from "react"; -import { useLocation } from "@tanstack/react-router"; +import { use, useEffect, useState, type ReactNode } from "react"; +import { useChildMatches, useLocation } from "@tanstack/react-router"; +import { docsClientLoader } from "@/lib/docs-client-loader"; import { cn } from "@/lib/utils"; -interface Heading { +/** One rail entry. `text` is a ReactNode, not a string: fumadocs compiles a heading's inline markup into + * the toc, so a heading like `### \`wantedPaths\` (string list)` keeps its code formatting here instead of + * being flattened the way reading `textContent` off the DOM did. */ +export interface TocHeading { id: string; - text: string; - level: 2 | 3; + text: ReactNode; + level: number; } -/** A stable empty list, so a route with no headings does not hand the renderer a fresh array each time. */ -const EMPTY_HEADINGS: readonly Heading[] = Object.freeze([]); +/** How many animation frames to keep looking for the page's headings before giving up. ~2s at 60fps: long + * enough for a slow content chunk, short enough that a toc entry whose heading never renders cannot leave + * a frame loop running for the life of the page. */ +const MAX_ATTACH_FRAMES = 120; /** localStorage so the rail recalls the last section across full reloads and tabs. */ const STORE_PREFIX = "docs-toc:v2:"; +/** The last-active section recorded for this route, when it still matches a heading on the page. */ +function restoreActive(storageKey: string, items: readonly TocHeading[]): string { + try { + const saved = window.localStorage.getItem(storageKey); + return saved && items.some((item) => item.id === saved) ? saved : ""; + } catch { + return ""; + } +} + +function remember(storageKey: string, id: string): void { + try { + window.localStorage.setItem(storageKey, id); + } catch { + /* noop */ + } +} + /** - * Right-rail "On this page" table of contents. - * Auto-scans the nearest
for h2 / h3 elements, assigns slug ids - * if missing, and tracks the active section via IntersectionObserver. + * Right-rail "On this page" list. PURE with respect to its items: it renders what it is handed and never + * inspects the document to discover them. + * + * It used to scan the rendered `article.prose-docs` for `h2, h3`, back-fill ids onto any heading missing + * one, and read the result into state from an effect (#9872). That was the app's last + * `react-hooks/set-state-in-effect` site -- the reason the rule sat at `warn` -- and it was fragile + * independently of lint: the rail was a function of the rendered markup, so a styling change to the article + * wrapper silently emptied it. + * + * The effect that remains subscribes an IntersectionObserver, which is real external-system work and + * exactly what an effect is for. It sets state only from the observer CALLBACK (an event), never in the + * effect body. */ -export function DocsToc() { - // Both the headings and the active id belong to ONE route, so both are stored under that route's path - // (#9588). Navigating therefore clears them by derivation -- a previous route's headings can never be - // read, and aria-current can never linger on one -- instead of two setState calls at the top of the - // effect below. The effect still runs: reading the rendered DOM and subscribing an IntersectionObserver - // is external-system work, which is exactly what an effect is for. - const [toc, setToc] = useState<{ path: string; items: readonly Heading[]; active: string }>({ - path: "", - items: [], - active: "", - }); +export function DocsToc({ items }: { items: readonly TocHeading[] }) { const location = useLocation(); const storageKey = `${STORE_PREFIX}${location.pathname}`; - const forThisPath = toc.path === location.pathname ? toc : null; - const items = forThisPath?.items ?? EMPTY_HEADINGS; - const active = forThisPath?.active ?? ""; + // Keyed by pathname so a navigation clears the previous route's active id by derivation rather than by a + // setState at the top of an effect -- the same pattern #9588 used for the headings themselves. + const [activeState, setActiveState] = useState<{ path: string; id: string }>({ + path: "", + id: "", + }); + const active = + activeState.path === location.pathname ? activeState.id : restoreActive(storageKey, items); useEffect(() => { - const article = document.querySelector("article.prose-docs"); - if (!article) return; - const nodes = Array.from(article.querySelectorAll("h2, h3")); - const headings: Heading[] = nodes.map((node) => { - if (!node.id) { - node.id = - (node.textContent ?? "") - .toLowerCase() - .trim() - .replace(/[^a-z0-9]+/g, "-") - .replace(/(^-|-$)/g, "") || `h-${Math.random().toString(36).slice(2, 7)}`; - } - // Scroll-margin so anchored sections clear the sticky header. - node.style.scrollMarginTop = "5rem"; - return { - id: node.id, - text: node.textContent ?? node.id, - level: (node.tagName === "H2" ? 2 : 3) as 2 | 3, - }; - }); - let activeId = ""; - - // No headings: nothing to record. The derived read above already yields an empty list for a path - // this state does not cover, which is exactly the "no TOC on this route" case. - if (headings.length === 0) return; - - // Restore last-active section for this route (display only — does not scroll the page). - // localStorage persists across full reloads and new tabs. - try { - const saved = window.localStorage.getItem(storageKey); - if (saved && headings.some((h) => h.id === saved)) activeId = saved; - } catch { - /* noop */ - } - + if (items.length === 0) return; const observer = new IntersectionObserver( (entries) => { const visible = entries - .filter((e) => e.isIntersecting) + .filter((entry) => entry.isIntersecting) .sort((a, b) => a.boundingClientRect.top - b.boundingClientRect.top); - if (visible[0]) { - const id = visible[0].target.id; - setToc((current) => - current.active === id && current.path === location.pathname - ? current - : { path: location.pathname, items: headings, active: id }, - ); - try { - window.localStorage.setItem(storageKey, id); - } catch { - /* noop */ - } - } + const first = visible[0]; + if (!first) return; + setActiveState({ path: location.pathname, id: first.target.id }); + remember(storageKey, first.target.id); }, { rootMargin: "-80px 0px -65% 0px", threshold: [0, 1] }, ); - setToc({ path: location.pathname, items: headings, active: activeId }); - nodes.forEach((n) => observer.observe(n)); - return () => observer.disconnect(); - }, [storageKey, location.pathname]); + + // The headings are rendered by the PAGE, which resolves in its own Suspense boundary -- independent of + // the one this rail resolves in. A single up-front `getElementById` sweep would therefore observe + // nothing, PERMANENTLY, on any commit where the rail lands before the article: the deps below do not + // change when the content later appears, so the effect never re-runs to pick it up. + // + // In practice both boundaries suspend on the same cached promise and usually commit together, so this + // is a guard against an ordering hazard rather than a fix for an observed failure -- I was not able to + // exercise IntersectionObserver in a headless pane to prove it either way. Attaching across frames + // costs nothing when the headings are already there (one pass, no rAF scheduled) and removes the + // failure mode entirely when they are not. Bounded, so a toc entry whose heading never renders cannot + // leave a frame loop running for the life of the page. + let frame = 0; + let attempts = 0; + const attached = new Set(); + const attach = () => { + for (const item of items) { + if (attached.has(item.id)) continue; + const node = document.getElementById(item.id); + if (node === null) continue; + attached.add(item.id); + observer.observe(node); + } + attempts += 1; + if (attached.size < items.length && attempts < MAX_ATTACH_FRAMES) + frame = requestAnimationFrame(attach); + }; + attach(); + + return () => { + if (frame !== 0) cancelAnimationFrame(frame); + observer.disconnect(); + }; + }, [items, storageKey, location.pathname]); if (items.length < 2) return null; @@ -108,30 +123,26 @@ export function DocsToc() { On this page