diff --git a/apps/client/src/features/page/queries/page-query.ts b/apps/client/src/features/page/queries/page-query.ts index 4d43ab2b..7ac0e3c0 100644 --- a/apps/client/src/features/page/queries/page-query.ts +++ b/apps/client/src/features/page/queries/page-query.ts @@ -665,6 +665,13 @@ export function updateCacheOnMovePage( pageData: Partial, ) { invalidatePageTree(); + // Invalidate the moved page's breadcrumbs (#523). The tree-side child-loss + // guard removes the moved node from the local tree when its new parent is an + // unloaded branch, so `findBreadcrumbPath` misses it and the breadcrumb bar + // falls back to the server `["breadcrumbs", pageId]` query — which this move + // must invalidate, otherwise the crumbs keep showing the OLD parent until a + // refocus/navigation. + queryClient.invalidateQueries({ queryKey: ["breadcrumbs", pageId] }); // Remove page from old parent's cache const oldQueryKey = oldParentId === null diff --git a/apps/client/src/features/page/queries/update-cache-on-move.breadcrumbs.test.ts b/apps/client/src/features/page/queries/update-cache-on-move.breadcrumbs.test.ts new file mode 100644 index 00000000..fbd10443 --- /dev/null +++ b/apps/client/src/features/page/queries/update-cache-on-move.breadcrumbs.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import type { IPage } from "@/features/page/types/page.types"; + +// A fresh QueryClient stands in for the app singleton (importing the real +// @/main.tsx would run ReactDOM.createRoot, which has no DOM root in jsdom). +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +import { queryClient } from "@/main.tsx"; +import { updateCacheOnMovePage } from "./page-query"; + +// #523: the tree-side child-loss guard removes the moved node from the local +// tree when its new parent is an unloaded branch, so `findBreadcrumbPath` misses +// it and the breadcrumb bar falls back to the server `["breadcrumbs", pageId]` +// query. That query MUST be invalidated by a move, or the crumbs keep showing +// the OLD parent until a refocus/navigation. +describe("updateCacheOnMovePage — breadcrumbs invalidation (#523)", () => { + beforeEach(() => { + queryClient.clear(); + vi.restoreAllMocks(); + }); + + it("invalidates the moved page's ['breadcrumbs', pageId] query", () => { + const spy = vi.spyOn(queryClient, "invalidateQueries"); + + updateCacheOnMovePage("s1", "moved-page", "old-parent", "new-parent", { + id: "moved-page", + } as Partial); + + const invalidatedBreadcrumbs = spy.mock.calls.some( + ([arg]) => + Array.isArray((arg as { queryKey?: unknown[] })?.queryKey) && + (arg as { queryKey: unknown[] }).queryKey[0] === "breadcrumbs" && + (arg as { queryKey: unknown[] }).queryKey[1] === "moved-page", + ); + expect(invalidatedBreadcrumbs).toBe(true); + }); +}); diff --git a/apps/client/src/features/page/tree/components/doc-tree-row.tsx b/apps/client/src/features/page/tree/components/doc-tree-row.tsx index 347f1f3e..c04028d0 100644 --- a/apps/client/src/features/page/tree/components/doc-tree-row.tsx +++ b/apps/client/src/features/page/tree/components/doc-tree-row.tsx @@ -55,7 +55,14 @@ type Props = { }; const DRAG_TYPE = 'doc-tree-item'; -const AUTO_EXPAND_MS = 500; +// Hover-hold before a collapsed row auto-expands during a drag. 2s (not ~0.5s) +// so merely dragging the cursor THROUGH the tree never expands rows — only a +// deliberate hold does (#523). +const AUTO_EXPAND_MS = 2000; +// How long the "a page just moved in here" cue stays on a collapsed target after +// a make-child drop. Long enough to notice at a glance during frequent +// collapsed-drops, short enough not to linger. Code-only UX constant (#523). +const DROP_LANDED_HIGHLIGHT_MS = 1800; function DocTreeRowInner(props: Props) { const { @@ -93,7 +100,11 @@ function DocTreeRowInner(props: Props) { const rowRef = useRef(null); const [isDragging, setIsDragging] = useState(false); const [instruction, setInstruction] = useState(null); + // Transient "just received a child" cue: a make-child drop no longer expands + // the (collapsed) target, so flash the row instead so the move isn't invisible. + const [landedChild, setLandedChild] = useState(false); const autoExpandTimerRef = useRef | null>(null); + const landedChildTimerRef = useRef | null>(null); const cancelAutoExpand = useCallback(() => { if (autoExpandTimerRef.current) { @@ -249,11 +260,24 @@ function DocTreeRowInner(props: Props) { ? getDragLabel(sourceNode) : 'item'; liveRegion.announce(`Moved ${sourceLabel} under ${parentName}.`); - // After a make-child drop, expand this row so the user sees the - // just-dropped child — especially important when the row had no - // children before (chevron just appeared) so the drop would - // otherwise be invisible. - if (op.kind === 'make-child') onToggle(node.id, true); + // Do NOT auto-expand the target on drop: a drop must leave the node + // collapsed. Intentional expansion is handled solely by the + // hover-hold timer (AUTO_EXPAND_MS). Feedback that the drop landed is + // given by the post-move flash + landed-cue highlight + live-region + // announce above. When the make-child target is collapsed, flash a + // distinct "child moved in here" cue on the row (it stays collapsed). + if (op.kind === 'make-child' && !isOpen) { + if (landedChildTimerRef.current) { + clearTimeout(landedChildTimerRef.current); + } + setLandedChild(true); + landedChildTimerRef.current = setTimeout(() => { + setLandedChild(false); + landedChildTimerRef.current = null; + }, DROP_LANDED_HIGHLIGHT_MS); + } + // Restore the openness of the MOVED page itself (source) — untouched + // by the above; the target is never expanded here. if (source.data.isOpenOnDragStart) onToggle(sourceId, true); }, }), @@ -281,6 +305,17 @@ function DocTreeRowInner(props: Props) { useEffect(() => () => cancelAutoExpand(), [cancelAutoExpand]); + // Clear the landed-child cue timer on unmount (mirrors autoExpandTimerRef). + useEffect( + () => () => { + if (landedChildTimerRef.current) { + clearTimeout(landedChildTimerRef.current); + landedChildTimerRef.current = null; + } + }, + [], + ); + const effectiveInst = instruction?.type === 'instruction-blocked' ? instruction.desired @@ -317,6 +352,7 @@ function DocTreeRowInner(props: Props) { className={styles.node} data-dragging={isDragging || undefined} data-selected={isSelected || undefined} + data-landed-child={landedChild || undefined} data-receiving-drop={ receivingDrop === 'make-child' ? blocked diff --git a/apps/client/src/features/page/tree/hooks/use-tree-mutation.move-guard.test.tsx b/apps/client/src/features/page/tree/hooks/use-tree-mutation.move-guard.test.tsx new file mode 100644 index 00000000..0e333219 --- /dev/null +++ b/apps/client/src/features/page/tree/hooks/use-tree-mutation.move-guard.test.tsx @@ -0,0 +1,149 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { Provider, createStore } from "jotai"; +import type { ReactNode } from "react"; + +import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts"; +import { treeModel } from "@/features/page/tree/model/tree-model"; +import type { SpaceTreeNode } from "@/features/page/tree/types"; + +// --- Boundary mocks: only the network/query + router/i18n surfaces the hook +// touches. The tree math (treeModel, dropOpToMovePayload) runs for real so the +// child-loss guard is exercised end-to-end. +const moveMutate = vi.fn().mockResolvedValue({}); +const updateCacheOnMovePageMock = vi.fn(); +vi.mock("@/features/page/queries/page-query.ts", () => ({ + useCreatePageMutation: () => ({ mutateAsync: vi.fn() }), + useUpdatePageMutation: () => ({ mutateAsync: vi.fn() }), + useRemovePageMutation: () => ({ mutateAsync: vi.fn() }), + useMovePageMutation: () => ({ mutateAsync: moveMutate }), + updateCacheOnMovePage: (...args: unknown[]) => + updateCacheOnMovePageMock(...args), +})); +vi.mock("react-router-dom", () => ({ + useNavigate: () => vi.fn(), + useParams: () => ({ spaceSlug: "space", pageSlug: undefined }), +})); +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (s: string) => s }), +})); +vi.mock("@mantine/notifications", () => ({ + notifications: { show: vi.fn() }, +})); + +// Import AFTER mocks so the hook binds to them. +import { useTreeMutation } from "./use-tree-mutation"; + +function node( + id: string, + over: Partial = {}, +): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id.toUpperCase(), + position: "a0", + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: false, + children: [], + ...over, + }; +} + +function setup(before: SpaceTreeNode[]) { + const store = createStore(); + store.set(treeDataAtom, before); + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + const { result } = renderHook(() => useTreeMutation("space-1"), { wrapper }); + return { store, result }; +} + +describe("useTreeMutation.handleMove — child-loss guard (#523)", () => { + beforeEach(() => { + vi.clearAllMocks(); + moveMutate.mockResolvedValue({}); + }); + + it("make-child into an UNLOADED folder does NOT materialize [source] — keeps it lazy-loadable", async () => { + // F has children on the server but none are loaded here (canonical unloaded + // form: hasChildren + children:[]). X sits at root. + const before = [ + node("F", { position: "a0", hasChildren: true, children: [] }), + node("X", { position: "a5" }), + ]; + const { store, result } = setup(before); + + await act(async () => { + await result.current.handleMove("X", { + kind: "make-child", + targetId: "F", + }); + }); + + const tree = store.get(treeDataAtom); + const f = treeModel.find(tree, "F"); + // The guard leaves F unloaded (children stay []), so a later expand fetches + // the FULL server set (incl. X) instead of showing a misleading partial [X]. + // MUTATION: dropping the guard (using the `move` result) would put children + // === [X] here and redden this. + expect(f?.children).toEqual([]); + expect(f?.hasChildren).toBe(true); + // X is removed from its old (root) slot; it reappears on expand/load of F. + expect(treeModel.find(tree, "X")).toBeNull(); + // The server move is still persisted. + expect(moveMutate).toHaveBeenCalledTimes(1); + }); + + it("make-child into a LOADED folder appends source and KEEPS the existing children", async () => { + // F is loaded with one child c1; moving X in must preserve c1 (no loss) and + // append X — this path does NOT hit the guard. + const before = [ + node("F", { + position: "a0", + hasChildren: true, + children: [node("c1", { position: "a1", parentPageId: "F" })], + }), + node("X", { position: "a5" }), + ]; + const { store, result } = setup(before); + + await act(async () => { + await result.current.handleMove("X", { + kind: "make-child", + targetId: "F", + }); + }); + + const tree = store.get(treeDataAtom); + const f = treeModel.find(tree, "F"); + expect(f?.children?.map((n) => n.id)).toEqual(["c1", "X"]); + expect(f?.hasChildren).toBe(true); + // X now lives under F. + expect(treeModel.find(tree, "X")?.parentPageId).toBe("F"); + }); + + it("make-child into a genuinely-empty leaf materializes the child (nothing to lose)", async () => { + // Leaf L has no server children (hasChildren:false). Dropping X in should + // show X immediately — the guard must NOT fire here. + const before = [ + node("L", { position: "a0", hasChildren: false, children: [] }), + node("X", { position: "a5" }), + ]; + const { store, result } = setup(before); + + await act(async () => { + await result.current.handleMove("X", { + kind: "make-child", + targetId: "L", + }); + }); + + const tree = store.get(treeDataAtom); + const l = treeModel.find(tree, "L"); + expect(l?.children?.map((n) => n.id)).toEqual(["X"]); + expect(l?.hasChildren).toBe(true); + }); +}); diff --git a/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts b/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts index 5f6a1061..60e9f38f 100644 --- a/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts +++ b/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts @@ -61,11 +61,48 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { if (!source) return; const oldParentId = source.parentPageId ?? null; - // optimistic apply with the new position from the payload - let optimistic = treeModel.update(after, sourceId, { - position: payload.position, - parentPageId: payload.parentPageId, - } as Partial); + // Child-loss guard (#523, twin of #525's realtime `insertByPosition` fix). + // We no longer auto-expand the make-child target on drop, so the old + // `onToggle(target, true)` — which was ALSO the only trigger of the + // corrective lazy-load — is gone. `treeModel.move` materialized + // `target.children = [source]` (only the moved node); if the target is an + // UNLOADED branch (server has children but none are loaded here), keeping + // that partial `[source]` list would defeat the lazy-load gate and hide the + // target's OTHER server children (the #159 #1 data-loss class). So for an + // unloaded make-child target, build the optimistic tree WITHOUT + // materializing source under it: just remove source from its old parent and + // flag the target `hasChildren`. The gate stays armed and a later manual + // expand fetches the FULL set (incl. the moved page, which the awaited + // server move persists). Predicate is the gate's (`isUnloadedBranch`), NOT + // `insertByPosition`'s old `=== undefined` (canonical unloaded is `[]`). + const target = + op.kind === "make-child" + ? (treeModel.find(before, op.targetId) as SpaceTreeNode | null) + : null; + const unloadedMakeChild = + op.kind === "make-child" && treeModel.isUnloadedBranch(target); + + let optimistic: SpaceTreeNode[]; + if (unloadedMakeChild) { + // Do NOT materialize [source] into the unloaded target. + optimistic = treeModel.remove(before, sourceId); + optimistic = treeModel.update(optimistic, op.targetId, { + hasChildren: true, + } as Partial); + } else { + // optimistic apply with the new position from the payload + optimistic = treeModel.update(after, sourceId, { + position: payload.position, + parentPageId: payload.parentPageId, + } as Partial); + // For make-child onto a previously-childless (loaded) target: flip + // hasChildren on so the new parent shows its chevron. + if (op.kind === "make-child") { + optimistic = treeModel.update(optimistic, op.targetId, { + hasChildren: true, + } as Partial); + } + } // If the old parent has no children left, mark hasChildren: false so the // chevron disappears. Without this, the empty parent keeps rendering an @@ -79,14 +116,6 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { } } - // For make-child onto a previously-childless target: flip hasChildren on - // so the new parent shows its chevron. - if (op.kind === "make-child") { - optimistic = treeModel.update(optimistic, op.targetId, { - hasChildren: true, - } as Partial); - } - setData(optimistic); try { diff --git a/apps/client/src/features/page/tree/styles/tree.module.css b/apps/client/src/features/page/tree/styles/tree.module.css index e018af73..10f1ba48 100644 --- a/apps/client/src/features/page/tree/styles/tree.module.css +++ b/apps/client/src/features/page/tree/styles/tree.module.css @@ -150,6 +150,45 @@ ); } +/* "A page just moved in here" cue (#523). A make-child drop no longer expands + the collapsed target, so the moved page is momentarily invisible; pulse the + target row in a distinct teal (NOT the blue make-child highlight, NOT the + neutral post-move flash) so the landing is noticeable while the node stays + collapsed. Two short pulses fit inside DROP_LANDED_HIGHLIGHT_MS (1.8s), after + which the row clears the attribute and the animation stops. */ +@keyframes landedChildPulse { + 0% { + background-color: light-dark( + var(--mantine-color-teal-2), + rgba(45, 212, 191, 0.30) + ); + outline-color: light-dark( + var(--mantine-color-teal-6), + var(--mantine-color-teal-5) + ); + } + 100% { + background-color: transparent; + outline-color: transparent; + } +} + +.node[data-landed-child="true"] { + outline: 2px solid transparent; + outline-offset: -1px; + animation: landedChildPulse 0.9s ease-out 2; +} + +@media (prefers-reduced-motion: reduce) { + .node[data-landed-child="true"] { + animation: none; + background-color: light-dark( + var(--mantine-color-teal-1), + rgba(45, 212, 191, 0.18) + ); + } +} + .dropLine { position: absolute; left: var(--drop-line-indent, 0);