From e9d5d493d35e99b807acea6e73822b67e49fbbaa Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 22:25:05 +0300 Subject: [PATCH] fix(#290 review): drop stale cached children on boot + gate size warn (F1/F2/F3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (MEDIUM regression): a collapsed-but-cached branch showed STALE children on re-expand after reload (the cache keeps children of any ever-expanded branch; refreshOpenBranches only refreshes OPEN branches, but the fetch guard skips a branch that has cached children). New pruneCollapsedChildren(tree, openIds) resets children to [] (keeps hasChildren) for every node NOT in the persisted open-set, recursing into open nodes — a once-per-mount boot effect. A pruned collapsed branch is then the 'unloaded' shape handleToggle re-fetches, so its first expand reconciles fresh (as pre-cache). Open branches keep their children (refreshOpenBranches handles them, no double fetch). Test: a collapsed cached branch with a stale child fetches fresh on first expand after boot. F2: gate the >4MB size-guard console.warn behind the writeFailureWarned once-flag (like the quota branch) so editing a huge tree no longer re-warns every ~500ms; test that an oversized tree is not persisted + warns exactly once. F3: narrow the use-auth privacy comment (only tree caches are swept; other localStorage entries remain). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/features/auth/hooks/use-auth.ts | 6 +- .../page/tree/atoms/tree-data-atom.test.ts | 33 +++ .../page/tree/atoms/tree-data-atom.ts | 8 +- .../components/space-tree.boot-cache.test.tsx | 222 ++++++++++++++++++ .../page/tree/components/space-tree.tsx | 16 ++ .../src/features/page/tree/utils/utils.ts | 35 +++ 6 files changed, 317 insertions(+), 3 deletions(-) create mode 100644 apps/client/src/features/page/tree/components/space-tree.boot-cache.test.tsx diff --git a/apps/client/src/features/auth/hooks/use-auth.ts b/apps/client/src/features/auth/hooks/use-auth.ts index 93fbd212..cd082e2c 100644 --- a/apps/client/src/features/auth/hooks/use-auth.ts +++ b/apps/client/src/features/auth/hooks/use-auth.ts @@ -123,8 +123,10 @@ export default function useAuth() { const handleLogout = async () => { setCurrentUser(RESET); - // Purge the persisted sidebar tree caches (they contain page titles) so - // nothing readable is left in localStorage on a shared machine. + // Purge the persisted sidebar tree caches (they contain page titles) so the + // cached page titles aren't left readable in localStorage on a shared + // machine. (Only the tree caches are swept; other localStorage entries + // remain.) clearPersistedTreeCaches(); await logout(); window.location.replace(`${APP_ROUTE.AUTH.LOGIN}?logout=1`); diff --git a/apps/client/src/features/page/tree/atoms/tree-data-atom.test.ts b/apps/client/src/features/page/tree/atoms/tree-data-atom.test.ts index 79e6f237..62397ff1 100644 --- a/apps/client/src/features/page/tree/atoms/tree-data-atom.test.ts +++ b/apps/client/src/features/page/tree/atoms/tree-data-atom.test.ts @@ -205,6 +205,39 @@ describe("treeDataAtom (localStorage-persisted)", () => { expect(persistedTreeDataKeys()).toEqual([]); }); + it("skips persisting a tree over the size cap and warns exactly once", async () => { + vi.useFakeTimers(); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const setItemSpy = vi.spyOn(localStorage, "setItem"); + + const { treeDataAtom, flushPendingTreeDataWrites } = await freshImport(); + const store = createStore(); + + // One node whose name alone serializes to > MAX_SERIALIZED_LENGTH (~4M). + const huge = node("big"); + huge.name = "x".repeat(4_000_001); + + store.set(treeDataAtom, [huge]); + vi.advanceTimersByTime(DEBOUNCE_MS + 100); + + // The oversized serialization is skipped: the key is never written. + expect(localStorage.getItem(ANON_KEY)).toBeNull(); + expect(setItemSpy).not.toHaveBeenCalled(); + + // Editing the still-oversized tree fires another debounced write, but the + // "too large" warn is gated by the once-flag — no per-tick console spam. + store.set(treeDataAtom, [huge, node("big2")]); + vi.advanceTimersByTime(DEBOUNCE_MS + 100); + flushPendingTreeDataWrites(); + + expect(localStorage.getItem(ANON_KEY)).toBeNull(); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy).toHaveBeenCalledWith( + "[tree] cached tree too large to persist; skipping", + ANON_KEY, + ); + }); + it("disables persistence after clearPersistedTreeCaches: NEW writes never reach storage", async () => { vi.useFakeTimers(); diff --git a/apps/client/src/features/page/tree/atoms/tree-data-atom.ts b/apps/client/src/features/page/tree/atoms/tree-data-atom.ts index 554a491f..411336aa 100644 --- a/apps/client/src/features/page/tree/atoms/tree-data-atom.ts +++ b/apps/client/src/features/page/tree/atoms/tree-data-atom.ts @@ -49,7 +49,13 @@ function writeNow(key: string, value: SpaceTreeNode[]): void { try { const serialized = JSON.stringify(value); if (serialized.length > MAX_SERIALIZED_LENGTH) { - console.warn("[tree] cached tree too large to persist; skipping", key); + // Warn ONCE, like the quota branch below: a >4M-char tree re-serializes on + // every ~500ms debounce tick while it's edited, so an un-gated warn would + // spam the console on each flush. + if (!writeFailureWarned) { + writeFailureWarned = true; + console.warn("[tree] cached tree too large to persist; skipping", key); + } return; } localStorage.setItem(key, serialized); diff --git a/apps/client/src/features/page/tree/components/space-tree.boot-cache.test.tsx b/apps/client/src/features/page/tree/components/space-tree.boot-cache.test.tsx new file mode 100644 index 00000000..4f248c81 --- /dev/null +++ b/apps/client/src/features/page/tree/components/space-tree.boot-cache.test.tsx @@ -0,0 +1,222 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { createRef } from "react"; +import { render, act, waitFor, cleanup } from "@testing-library/react"; + +// --- Mocks for the heavy / networked module graph --------------------------- +// Same isolation strategy as space-tree.expand-all.test.tsx: everything that +// would otherwise need a real server / router / DnD stack is mocked. Here we +// additionally CAPTURE the DocTree props (onToggle + data) so the test can +// drive a lazy-load expand exactly as a row click would, and we control +// fetchAllAncestorChildren to assert the fresh fetch happens. + +const fetchAllAncestorChildrenMock = vi.fn(); + +// Holder mutated by the DocTree stub each render so the test can read the +// latest tree it was handed and invoke its onToggle callback. +const docTree: { + onToggle?: (id: string, isOpen: boolean) => void | Promise; + data: unknown[]; +} = { data: [] }; + +vi.mock("@/features/page/services/page-service.ts", () => ({ + getSpaceTree: vi.fn(), + getPageBreadcrumbs: vi.fn(), +})); + +vi.mock("@/features/page/queries/page-query.ts", () => ({ + // No root pages and no further pages — the server data-load effect stays + // inert (isDataLoaded never flips), so refreshOpenBranches never runs and the + // test exercises ONLY the boot-prune + handleToggle lazy-load path against + // the hydrated cache we seed into the atom below. + useGetRootSidebarPagesQuery: () => ({ + data: undefined, + hasNextPage: false, + fetchNextPage: vi.fn(), + isFetching: false, + }), + usePageQuery: () => ({ data: undefined }), + fetchAllAncestorChildren: (...args: unknown[]) => + fetchAllAncestorChildrenMock(...args), +})); + +vi.mock("@/features/page/tree/hooks/use-tree-mutation.ts", () => ({ + useTreeMutation: () => ({ handleMove: vi.fn() }), +})); + +vi.mock("@mantine/notifications", () => ({ + notifications: { show: vi.fn() }, +})); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("react-router-dom", () => ({ + useParams: () => ({ pageSlug: undefined }), +})); + +vi.mock("@/lib", () => ({ + extractPageSlugId: () => undefined, +})); + +vi.mock("@/lib/config.ts", () => ({ + isCompactPageTreeEnabled: () => false, +})); + +// Capture the props DocTree is rendered with instead of rendering anything. +vi.mock("./doc-tree", () => ({ + DocTree: (props: { onToggle: (id: string, isOpen: boolean) => void; data: unknown[] }) => { + docTree.onToggle = props.onToggle; + docTree.data = props.data; + return null; + }, + ROW_HEIGHT_COMPACT: 28, + ROW_HEIGHT_STANDARD: 32, +})); +vi.mock("./space-tree-row", () => ({ + SpaceTreeRow: () => null, +})); + +vi.mock("@mantine/core", () => ({ + Text: ({ children }: { children?: unknown }) => children ?? null, +})); + +// In-memory open-map (the real one is localStorage-backed and crashes under the +// jsdom shim). Empty at start of each test -> every branch is COLLAPSED, which +// is exactly the state we need to prove the boot-prune. `scopeKeyAtom` is +// re-exported because the persisted tree-data atom resolves its scope through it. +vi.mock("@/features/page/tree/atoms/open-tree-nodes-atom.ts", async () => { + const { atom } = await import("jotai"); + type OpenMap = Record; + const base = atom({}); + const openTreeNodesAtom = atom( + (get) => get(base), + (get, set, update: OpenMap | ((prev: OpenMap) => OpenMap)) => { + const next = + typeof update === "function" + ? (update as (prev: OpenMap) => OpenMap)(get(base)) + : update; + set(base, next); + }, + ); + const scopeKeyAtom = atom(() => "test-workspace:test-user"); + return { openTreeNodesAtom, scopeKeyAtom }; +}); + +import SpaceTree, { SpaceTreeApi } from "./space-tree"; +import { + treeDataAtom, + flushPendingTreeDataWrites, +} from "@/features/page/tree/atoms/tree-data-atom.ts"; +import { createStore, Provider } from "jotai"; +import type { SpaceTreeNode } from "@/features/page/tree/types.ts"; + +// The scopeKeyAtom mock resolves to this fixed scope, so the persisted +// tree-data atom hydrates from exactly this localStorage key at mount +// (getOnInit + atomWithStorage's onMount both read it). +const CACHE_KEY = "treeData:v1:test-workspace:test-user"; + +function child( + id: string, + parentPageId: string, + hasChildren = false, +): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id, + position: "a0", + spaceId: "space-1", + parentPageId, + hasChildren, + children: [], + }; +} + +// A hydrated boot cache: a COLLAPSED branch (not in the open-map) that still +// carries a stale cached child — the exact shape a previous session left behind +// after the branch was expanded then collapsed then persisted. +function cachedTreeWithCollapsedBranch(): SpaceTreeNode[] { + return [ + { + id: "branch", + slugId: "slug-branch", + name: "branch", + position: "a0", + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: true, + children: [child("stale", "branch")], + }, + ]; +} + +beforeEach(() => { + fetchAllAncestorChildrenMock.mockReset(); + docTree.onToggle = undefined; + docTree.data = []; + // Flush any pending debounced write from a previous test before clearing. + flushPendingTreeDataWrites(); + try { + localStorage.clear?.(); + } catch { + /* fresh store per test isolates state */ + } +}); + +afterEach(() => { + cleanup(); +}); + +describe("SpaceTree boot-cache prune (#159 #8 stale collapsed children)", () => { + it("drops a collapsed cached branch's children on boot and fetches fresh on first expand", async () => { + // Server returns FRESH children on the lazy-load: the stale cached child is + // gone, a renamed/new one takes its place. + fetchAllAncestorChildrenMock.mockResolvedValue([child("fresh", "branch")]); + + // Simulate the localStorage-hydrated boot cache: seed the persisted key + // BEFORE mount so the atom hydrates it (store.set would be clobbered by + // atomWithStorage's onMount re-reading storage — this is the real path). + localStorage.setItem( + CACHE_KEY, + JSON.stringify(cachedTreeWithCollapsedBranch()), + ); + + const store = createStore(); + const ref = createRef(); + render( + + + , + ); + + // Boot-prune ran at mount: the COLLAPSED branch's cached children were + // dropped to the unloaded shape ([]), so the stale child is no longer there. + const branchAfterBoot = docTree.data.find( + (n) => (n as SpaceTreeNode).id === "branch", + ) as SpaceTreeNode; + expect(branchAfterBoot.children).toEqual([]); + expect(branchAfterBoot.hasChildren).toBe(true); + + // First expand of the collapsed branch after boot must lazy-load fresh + // children (before this fix the cached children were kept and the fetch + // was skipped, showing stale data). + await act(async () => { + await docTree.onToggle!("branch", true); + }); + + expect(fetchAllAncestorChildrenMock).toHaveBeenCalledTimes(1); + expect(fetchAllAncestorChildrenMock).toHaveBeenCalledWith({ + pageId: "branch", + spaceId: "space-1", + }); + + // The fresh children replaced the stale cache in the live tree. + await waitFor(() => { + const branch = store + .get(treeDataAtom) + .find((n) => n.id === "branch")!; + expect(branch.children.map((c) => c.id)).toEqual(["fresh"]); + }); + }); +}); diff --git a/apps/client/src/features/page/tree/components/space-tree.tsx b/apps/client/src/features/page/tree/components/space-tree.tsx index 125888f0..85e4cdae 100644 --- a/apps/client/src/features/page/tree/components/space-tree.tsx +++ b/apps/client/src/features/page/tree/components/space-tree.tsx @@ -30,6 +30,7 @@ import { openBranches, closeIds, loadedOpenBranchIds, + pruneCollapsedChildren, } from "@/features/page/tree/utils/utils.ts"; import { SpaceTreeNode } from "@/features/page/tree/types.ts"; import { treeModel } from "@/features/page/tree/model/tree-model"; @@ -199,6 +200,21 @@ const SpaceTree = forwardRef(function SpaceTree( const openIdsRef = useRef(openIds); openIdsRef.current = openIds; + // Boot-cache hygiene (#159 #8): the localStorage-hydrated tree carries the + // children of every branch ever expanded, including ones now COLLAPSED. Their + // first expand would skip the lazy-load and render stale children (a + // rename/move/delete missed while offline). Drop the cached children of every + // COLLAPSED branch ONCE at mount so its first expand fetches fresh via + // handleToggle — exactly as it did before the tree was cached. OPEN branches + // keep their children and are refreshed by refreshOpenBranches instead, so + // this runs before any expand and never double-fetches an open branch. + const prunedBootCacheRef = useRef(false); + useEffect(() => { + if (prunedBootCacheRef.current) return; + prunedBootCacheRef.current = true; + setData((prev) => pruneCollapsedChildren(prev, openIdsRef.current)); + }, [setData]); + // Re-fetch and reconcile the children of every currently-open, already-loaded // branch of THIS space. Shared by the socket reconnect handler and the // post-load cache refresh below. The ROOT level is reconciled separately by diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index d5084b00..1c61d5cf 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -293,6 +293,41 @@ export function loadedOpenBranchIds( return ids; } +/** + * Boot-cache hygiene (#159 #8): the persisted tree keeps the children of EVERY + * branch ever expanded — collapsing a branch never prunes them. So on reload a + * COLLAPSED branch hydrates with its old cached children, and `handleToggle` + * skips the lazy-load on first expand (children already present) → it shows + * STALE children (renamed / moved / deleted while the user was offline) with no + * reconcile. `refreshOpenBranches` only refreshes OPEN branches, so collapsed + * ones slip through. + * + * Fix: drop the cached children of every node NOT in the persisted open-set, + * resetting it to the canonical UNLOADED shape (`children: []`, `hasChildren` + * untouched — see pageToTreeNode). Its first expand then lazy-loads fresh, just + * as it did before the tree was cached to localStorage. OPEN branches keep + * their children (refreshOpenBranches reconciles those, so they must not be + * dropped here) and are recursed into so a collapsed branch nested under an + * open one is pruned too. + */ +export function pruneCollapsedChildren( + tree: SpaceTreeNode[], + openIds: ReadonlySet, +): SpaceTreeNode[] { + return tree.map((node) => { + const hasLoadedChildren = !!node.children && node.children.length > 0; + if (!openIds.has(node.id)) { + // Collapsed: drop the whole cached subtree so it reads as unloaded. + return hasLoadedChildren ? { ...node, children: [] } : node; + } + // Open: keep it, but recurse into its children (a nested collapsed branch + // must still be pruned). + return hasLoadedChildren + ? { ...node, children: pruneCollapsedChildren(node.children, openIds) } + : node; + }); +} + // Collect every node id in the tree (roots, branches, leaves). Used by // collapseAll to clear the open-state map for all current-space nodes. export function collectAllIds(nodes: SpaceTreeNode[]): string[] {