fix(#290 review): drop stale cached children on boot + gate size warn (F1/F2/F3)

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) <noreply@anthropic.com>
2026-07-02 22:25:05 +03:00
parent b349676eae
commit e9d5d493d3
6 changed files with 317 additions and 3 deletions
@@ -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`);
@@ -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();

@@ -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);
@@ -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<void>;
+  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<string, boolean>;
+  const base = atom<OpenMap>({});
+  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<SpaceTreeApi>();
+    render(
+      <Provider store={store}>
+        <SpaceTree ref={ref} spaceId="space-1" readOnly={false} />
+      </Provider>,
+    );
+
+    // 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"]);
+    });
+  });
+});
@@ -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<SpaceTreeApi, SpaceTreeProps>(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
@@ -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<string>,
+): 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[] {