From 8218c1a8efe222b5ca24aad9b2afce4ab78940fe Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 11:15:07 +0300 Subject: [PATCH] fix(tree): refresh loaded branches on reconnect so they don't go stale (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third tree-sync finding (#8). On a socket reconnect after a missed-events gap (laptop sleep / wifi blip), the resync only invalidated the ROOT sidebar query; a move/rename/delete that happened INSIDE an already-loaded, expanded branch was never reflected — the branch stayed stale until the user manually interacted. (The #2 fix reconciles the root level; this covers the deeper loaded branches.) - `treeModel.reconcileChildren(tree, parentId, fresh)`: replace a loaded branch's DIRECT children with the authoritative fresh set (drop removed, add new, reorder to server) while PRESERVING each surviving child's already-loaded grandchildren, so deeper expansion is not collapsed. An unloaded branch (children === undefined) is left untouched (lazy-load fetches it fresh). - `loadedOpenBranchIds(tree, openIds)`: the branches a reconnect should refresh (open AND loaded). `fetchAllAncestorChildren(..., { fresh: true })` bypasses the 30-min sidebar cache so the reconcile sees current data (handler-order independent). - space-tree: on socket `connect`, re-fetch + reconcile each open loaded branch of the active space (space-switch-guarded; an unloaded branch is skipped). Tests: reconcileChildren (drop/add/reorder + preserve grandchildren + unloaded no-op) and loadedOpenBranchIds (open+loaded only, skip unloaded, nested). The pure logic is unit-tested; the live socket-reconnect round-trip is not browser-automated (simulating a reconnect gap is impractical) — sidebar render + expand were smoke-tested with no regression. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/features/page/queries/page-query.ts | 37 +++++++--- .../page/tree/components/space-tree.tsx | 69 +++++++++++++++---- .../page/tree/model/tree-model.test.ts | 45 ++++++++++++ .../features/page/tree/model/tree-model.ts | 42 +++++++++++ .../features/page/tree/utils/utils.test.ts | 39 +++++++++++ .../src/features/page/tree/utils/utils.ts | 23 +++++++ 6 files changed, 232 insertions(+), 23 deletions(-) diff --git a/apps/client/src/features/page/queries/page-query.ts b/apps/client/src/features/page/queries/page-query.ts index 4e279621..ee44b775 100644 --- a/apps/client/src/features/page/queries/page-query.ts +++ b/apps/client/src/features/page/queries/page-query.ts @@ -274,7 +274,10 @@ export function useRestorePageMutation() { queryClient.setQueryData(["pages", restoredPage.slugId], merge); }, onError: (error) => { - notifications.show({ message: t("Failed to restore page"), color: "red" }); + notifications.show({ + message: t("Failed to restore page"), + color: "red", + }); }, }); } @@ -285,10 +288,10 @@ export function useGetSidebarPagesQuery( return useInfiniteQuery({ queryKey: ["sidebar-pages", data], enabled: !!data?.pageId || !!data?.spaceId, - queryFn: ({ pageParam }) => getSidebarPages({ ...data, cursor: pageParam, limit: 100 }), + queryFn: ({ pageParam }) => + getSidebarPages({ ...data, cursor: pageParam, limit: 100 }), initialPageParam: undefined, - getNextPageParam: (lastPage) => - lastPage.meta?.nextCursor ?? undefined, + getNextPageParam: (lastPage) => lastPage.meta?.nextCursor ?? undefined, }); } @@ -296,11 +299,14 @@ export function useGetRootSidebarPagesQuery(data: SidebarPagesParams) { return useInfiniteQuery({ queryKey: ["root-sidebar-pages", data.spaceId], queryFn: async ({ pageParam }) => { - return getSidebarPages({ spaceId: data.spaceId, cursor: pageParam, limit: 100 }); + return getSidebarPages({ + spaceId: data.spaceId, + cursor: pageParam, + limit: 100, + }); }, initialPageParam: undefined, - getNextPageParam: (lastPage) => - lastPage.meta?.nextCursor ?? undefined, + getNextPageParam: (lastPage) => lastPage.meta?.nextCursor ?? undefined, }); } @@ -323,12 +329,17 @@ export function usePageBreadcrumbsQuery( }); } -export async function fetchAllAncestorChildren(params: SidebarPagesParams) { +export async function fetchAllAncestorChildren( + params: SidebarPagesParams, + // `fresh: true` forces a server refetch (staleTime 0) — used by the reconnect + // refresh (#159 #8), which must NOT receive the 30-min-cached children. + opts?: { fresh?: boolean }, +) { // not using a hook here, so we can call it inside a useEffect hook const response = await queryClient.fetchQuery({ queryKey: ["sidebar-pages", params], queryFn: () => getAllSidebarPages(params), - staleTime: 30 * 60 * 1000, + staleTime: opts?.fresh ? 0 : 30 * 60 * 1000, }); const allItems = response.pages.flatMap((page) => page.items); @@ -347,11 +358,15 @@ export function useRecentChangesQuery(spaceId?: string) { }); } -export function useCreatedByQuery(params?: { userId?: string; spaceId?: string }) { +export function useCreatedByQuery(params?: { + userId?: string; + spaceId?: string; +}) { const { userId, spaceId } = params ?? {}; return useInfiniteQuery({ queryKey: ["pages-created-by-user", { userId, spaceId }], - queryFn: ({ pageParam }) => getCreatedByPages({ userId, spaceId, cursor: pageParam, limit: 15 }), + queryFn: ({ pageParam }) => + getCreatedByPages({ userId, spaceId, cursor: pageParam, limit: 15 }), initialPageParam: undefined as string | undefined, getNextPageParam: (lastPage) => lastPage.meta.hasNextPage ? lastPage.meta.nextCursor : undefined, 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 b2cfb994..affcbac3 100644 --- a/apps/client/src/features/page/tree/components/space-tree.tsx +++ b/apps/client/src/features/page/tree/components/space-tree.tsx @@ -29,9 +29,11 @@ import { collectBranchIds, openBranches, closeIds, + loadedOpenBranchIds, } from "@/features/page/tree/utils/utils.ts"; import { SpaceTreeNode } from "@/features/page/tree/types.ts"; import { treeModel } from "@/features/page/tree/model/tree-model"; +import { socketAtom } from "@/features/websocket/atoms/socket-atom.ts"; import { getPageBreadcrumbs, getSpaceTree, @@ -39,11 +41,7 @@ import { import { IPage } from "@/features/page/types/page.types.ts"; import { extractPageSlugId } from "@/lib"; import { isCompactPageTreeEnabled } from "@/lib/config.ts"; -import { - DocTree, - ROW_HEIGHT_COMPACT, - ROW_HEIGHT_STANDARD, -} from "./doc-tree"; +import { DocTree, ROW_HEIGHT_COMPACT, ROW_HEIGHT_STANDARD } from "./doc-tree"; import { SpaceTreeRow } from "./space-tree-row"; interface SpaceTreeProps { @@ -193,6 +191,54 @@ const SpaceTree = forwardRef(function SpaceTree( [openTreeNodes], ); + // Latest tree + open-state for the reconnect handler (its closure would + // otherwise read stale snapshots). + const [socket] = useAtom(socketAtom); + const dataRef = useRef(data); + dataRef.current = data; + const openIdsRef = useRef(openIds); + openIdsRef.current = openIds; + + // Reconnect refresh (#159 #8): on a socket reconnect, re-fetch and reconcile + // the children of every currently-open, already-loaded branch of THIS space, + // so a move/rename/delete that happened INSIDE a loaded branch while events + // were missed (laptop sleep / wifi gap) is reflected instead of left stale. + // The ROOT level is reconciled separately by the root-query refetch + + // mergeRootTrees; an UNLOADED branch is skipped (lazy-load fetches it fresh on + // expand). No first-connect guard is needed: space-tree usually mounts AFTER + // the initial connect, so every `connect` it sees is a reconnect; the rare + // initial-connect case has an empty tree, so the refresh is a harmless no-op. + useEffect(() => { + if (!socket) return; + const onConnect = async () => { + const effectSpaceId = spaceIdRef.current; + const branchIds = loadedOpenBranchIds( + dataRef.current.filter((n) => n?.spaceId === effectSpaceId), + openIdsRef.current, + ); + if (branchIds.length === 0) return; + for (const id of branchIds) { + try { + // `fresh: true` bypasses the 30-min sidebar-pages cache so the + // reconcile sees the server's CURRENT children (handler-order + // independent — no reliance on the global reconnect invalidation). + const fresh = await fetchAllAncestorChildren( + { pageId: id, spaceId: effectSpaceId }, + { fresh: true }, + ); + if (spaceIdRef.current !== effectSpaceId) return; // space switched + setData((prev) => treeModel.reconcileChildren(prev, id, fresh)); + } catch (err) { + console.error("[tree] reconnect branch refresh failed", err); + } + } + }; + socket.on("connect", onConnect); + return () => { + socket.off("connect", onConnect); + }; + }, [socket, setData]); + const handleToggle = useCallback( async (id: string, isOpen: boolean) => { setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen })); @@ -245,8 +291,7 @@ const SpaceTree = forwardRef(function SpaceTree( notifications.show({ color: "red", message: t("Couldn't expand the tree: {{reason}}", { - reason: - err?.response?.data?.message ?? err?.message ?? String(err), + reason: err?.response?.data?.message ?? err?.message ?? String(err), }), }); } finally { @@ -262,11 +307,11 @@ const SpaceTree = forwardRef(function SpaceTree( setOpenTreeNodes((prev) => closeIds(prev, ids)); }, [filteredData, setOpenTreeNodes]); - useImperativeHandle( - ref, - () => ({ expandAll, collapseAll, isExpanding }), - [expandAll, collapseAll, isExpanding], - ); + useImperativeHandle(ref, () => ({ expandAll, collapseAll, isExpanding }), [ + expandAll, + collapseAll, + isExpanding, + ]); // Stable callbacks for DocTree. Without these, every parent render recreates // the props and tears down every row's draggable/dropTarget subscription, diff --git a/apps/client/src/features/page/tree/model/tree-model.test.ts b/apps/client/src/features/page/tree/model/tree-model.test.ts index b726155e..a2dbd6b9 100644 --- a/apps/client/src/features/page/tree/model/tree-model.test.ts +++ b/apps/client/src/features/page/tree/model/tree-model.test.ts @@ -255,6 +255,51 @@ describe("treeModel.insertByPosition", () => { }); }); +// reconcileChildren (#159 #8): on a socket-reconnect refresh, an already-loaded +// branch is reconciled against a fresh server fetch — removed children drop, +// new ones appear, order follows the server, and surviving children keep their +// own loaded grandchildren (deeper expansion is not collapsed). +describe("treeModel.reconcileChildren", () => { + type N = TreeNode<{ name: string }>; + const leaf = (id: string): N => ({ id, name: id.toUpperCase() }); + + it("drops removed children, adds new ones, and follows the fresh order", () => { + const tree: N[] = [ + { id: "p", name: "P", children: [leaf("a"), leaf("b")] }, + ]; + // Server now has b, c (a was deleted/moved away; c is new) in this order. + const next = treeModel.reconcileChildren(tree, "p", [leaf("b"), leaf("c")]); + expect(treeModel.find(next, "p")?.children?.map((n) => n.id)).toEqual([ + "b", + "c", + ]); + expect(treeModel.find(next, "a")).toBeNull(); + }); + + it("preserves a surviving child's loaded grandchildren", () => { + const tree: N[] = [ + { + id: "p", + name: "P", + children: [{ id: "a", name: "A", children: [leaf("a1")] }, leaf("b")], + }, + ]; + // Fresh fetch returns only top-level children (no grandchildren). + const next = treeModel.reconcileChildren(tree, "p", [leaf("a"), leaf("b")]); + // 'a' keeps its previously loaded grandchild 'a1'. + expect(treeModel.find(next, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + ]); + }); + + it("leaves an UNLOADED parent (children undefined) untouched", () => { + const tree: N[] = [{ id: "p", name: "P" }]; // children: undefined + const next = treeModel.reconcileChildren(tree, "p", [leaf("a")]); + expect(next).toBe(tree); // no-op: lazy-load handles an unloaded branch + expect(treeModel.find(next, "p")?.children).toBeUndefined(); + }); +}); + // addTreeNode idempotency: the receiver early-returns when the node id already // exists, so re-delivery (or the author's optimistic node) is never duplicated. // This guards the find-then-skip contract insertByPosition relies on. diff --git a/apps/client/src/features/page/tree/model/tree-model.ts b/apps/client/src/features/page/tree/model/tree-model.ts index 922305e7..aa13d8b4 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -223,6 +223,48 @@ export const treeModel = { return touched ? out : tree; }, + // Replace a parent's DIRECT children with the authoritative `fresh` set while + // PRESERVING each surviving child's already-loaded grandchildren (deeper + // expansion). Unlike `appendChildren` (add-only), this DROPS children that are + // no longer present and reorders to `fresh` — so a move/delete/rename that + // happened inside a loaded branch while events were missed (a socket reconnect + // gap) is reflected, not left stale (#159 #8). Only used to reconcile an + // already-loaded branch against a fresh fetch; a parent with no loaded children + // (`children === undefined`) is left untouched (lazy-load handles it). + reconcileChildren( + tree: TreeNode[], + parentId: string, + fresh: TreeNode[], + ): TreeNode[] { + let touched = false; + const walk = (nodes: TreeNode[]): TreeNode[] => + nodes.map((n) => { + if (n.id === parentId) { + // Only reconcile a branch whose children were actually loaded; an + // unloaded parent stays unloaded (lazy-load fetches it fresh later). + if (n.children === undefined) return n; + const prevById = new Map(n.children.map((c) => [c.id, c])); + const merged = fresh.map((f) => { + const prev = prevById.get(f.id); + // Preserve the surviving child's previously loaded grandchildren so + // deeper expansion is not collapsed by the reconcile. + return prev?.children !== undefined + ? { ...f, children: prev.children } + : f; + }); + touched = true; + return { ...n, children: merged }; + } + if (n.children) { + const next = walk(n.children); + if (next !== n.children) return { ...n, children: next }; + } + return n; + }); + const out = walk(tree); + return touched ? out : tree; + }, + place( tree: TreeNode[], sourceId: string, diff --git a/apps/client/src/features/page/tree/utils/utils.test.ts b/apps/client/src/features/page/tree/utils/utils.test.ts index f10ab55e..4ea181b5 100644 --- a/apps/client/src/features/page/tree/utils/utils.test.ts +++ b/apps/client/src/features/page/tree/utils/utils.test.ts @@ -7,6 +7,7 @@ import { openBranches, closeIds, mergeRootTrees, + loadedOpenBranchIds, } from "./utils"; import type { IPage } from "@/features/page/types/page.types.ts"; import type { SpaceTreeNode } from "@/features/page/tree/types.ts"; @@ -321,3 +322,41 @@ describe("mergeRootTrees (#159 #2 reconnect reconcile)", () => { expect(merged[0].name).toBe("NEW"); }); }); + +describe("loadedOpenBranchIds (#159 #8 reconnect refresh targets)", () => { + function n(id: string, children?: SpaceTreeNode[]): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id.toUpperCase(), + icon: undefined, + position: "a0", + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: !!children, + children: children as SpaceTreeNode[], + }; + } + + it("returns OPEN branches whose children are loaded (array)", () => { + const tree = [n("a", [n("a1")]), n("b", [n("b1")])]; + const ids = loadedOpenBranchIds(tree, new Set(["a"])); + expect(ids).toEqual(["a"]); // b is closed; a is open+loaded + }); + + it("skips an open branch whose children are NOT loaded (undefined)", () => { + const tree = [n("a")]; // children undefined + expect(loadedOpenBranchIds(tree, new Set(["a"]))).toEqual([]); + }); + + it("includes a loaded-but-empty open branch (a child may have been added during the gap)", () => { + const tree = [n("a", [])]; + expect(loadedOpenBranchIds(tree, new Set(["a"]))).toEqual(["a"]); + }); + + it("walks nested open+loaded branches (deep chain refreshes every level)", () => { + const tree = [n("a", [n("a1", [n("a1a")])])]; + const ids = loadedOpenBranchIds(tree, new Set(["a", "a1"])); + expect(ids.sort()).toEqual(["a", "a1"]); + }); +}); diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 04cb51df..56f6ab02 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -246,6 +246,29 @@ export function mergeRootTrees( return sortPositionKeys(reconciled); } +/** + * Ids of branches a socket-reconnect refresh should re-fetch and reconcile + * (#159 #8): a node that is currently OPEN and whose children are LOADED + * (`children` is an array — possibly empty). An unloaded branch (`children === + * undefined`) is skipped because lazy-load fetches it fresh on the next expand, + * so there is nothing stale to reconcile. Walks the whole tree (a deep open + * chain refreshes every loaded level). + */ +export function loadedOpenBranchIds( + tree: SpaceTreeNode[], + openIds: ReadonlySet, +): string[] { + const ids: string[] = []; + const walk = (nodes: SpaceTreeNode[]) => { + for (const n of nodes) { + if (openIds.has(n.id) && Array.isArray(n.children)) ids.push(n.id); + if (n.children) walk(n.children); + } + }; + walk(tree); + return ids; +} + // 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[] {