Merge pull request 'fix(page-tree/dnd): drop-раскрытие + guard от потери детей (#523)' (#532) from fix/523-tree-dnd into fix/525-insert-unloaded

Reviewed-on: #532

apps/client/src/features/page/queries/page-query.ts

@@ -665,6 +665,13 @@ export function updateCacheOnMovePage(
   pageData: Partial<IPage>,
 ) {
   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

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<IPage>);
+
+    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);
+  });
+});

apps/client/src/features/page/tree/components/doc-tree-row.tsx

@@ -55,7 +55,14 @@ type Props<T extends object> = {
 };
 
 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<T extends object>(props: Props<T>) {
   const {
@@ -93,7 +100,11 @@ function DocTreeRowInner<T extends object>(props: Props<T>) {
   const rowRef = useRef<HTMLElement>(null);
   const [isDragging, setIsDragging] = useState(false);
   const [instruction, setInstruction] = useState<Instruction | null>(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<ReturnType<typeof setTimeout> | null>(null);
+  const landedChildTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
 
   const cancelAutoExpand = useCallback(() => {
     if (autoExpandTimerRef.current) {
@@ -249,11 +260,24 @@ function DocTreeRowInner<T extends object>(props: Props<T>) {
               ? getDragLabel(sourceNode)
               : 'item';
             liveRegion.announce(`Moved ${sourceLabel} under ${parentName}.`);
-            // After a make-child drop, expand this row so the user sees the
+            // Do NOT auto-expand the target on drop: a drop must leave the node
-            // just-dropped child — especially important when the row had no
+            // collapsed. Intentional expansion is handled solely by the
-            // children before (chevron just appeared) so the drop would
+            // hover-hold timer (AUTO_EXPAND_MS). Feedback that the drop landed is
-            // otherwise be invisible.
+            // given by the post-move flash + landed-cue highlight + live-region
-            if (op.kind === 'make-child') onToggle(node.id, true);
+            // 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<T extends object>(props: Props<T>) {
 
   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<T extends object>(props: Props<T>) {
         className={styles.node}
         data-dragging={isDragging || undefined}
         data-selected={isSelected || undefined}
+        data-landed-child={landedChild || undefined}
         data-receiving-drop={
           receivingDrop === 'make-child'
             ? blocked

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> = {},
+): 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 }) => (
+    <Provider store={store}>{children}</Provider>
+  );
+  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);
+  });
+});

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
+      // Child-loss guard (#523, twin of #525's realtime `insertByPosition` fix).
-      let optimistic = treeModel.update(after, sourceId, {
+      // We no longer auto-expand the make-child target on drop, so the old
-        position: payload.position,
+      // `onToggle(target, true)` — which was ALSO the only trigger of the
-        parentPageId: payload.parentPageId,
+      // corrective lazy-load — is gone. `treeModel.move` materialized
-      } as Partial<SpaceTreeNode>);
+      // `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<SpaceTreeNode>);
+      } else {
+        // optimistic apply with the new position from the payload
+        optimistic = treeModel.update(after, sourceId, {
+          position: payload.position,
+          parentPageId: payload.parentPageId,
+        } as Partial<SpaceTreeNode>);
+        // 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<SpaceTreeNode>);
+        }
+      }
 
       // 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<SpaceTreeNode>);
-      }
-
       setData(optimistic);
 
       try {

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);