Compare commits
3 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 27d51303ba | |||
| c765483947 | |||
| bfb6a52eea |
@@ -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
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
// 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<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
|
||||
|
||||
@@ -281,10 +281,10 @@ const SpaceTree = forwardRef<SpaceTreeApi, SpaceTreeProps>(function SpaceTree(
|
||||
setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen }));
|
||||
if (isOpen) {
|
||||
const node = treeModel.find(data, id) as SpaceTreeNode | null;
|
||||
if (
|
||||
node?.hasChildren &&
|
||||
(!node.children || node.children.length === 0)
|
||||
) {
|
||||
// Same "unloaded branch" predicate the insert paths use (`isUnloadedBranch`)
|
||||
// so the lazy-load gate and the realtime/DnD inserts can never disagree
|
||||
// about what counts as unloaded (#525).
|
||||
if (treeModel.isUnloadedBranch(node)) {
|
||||
const fetched = await fetchAllAncestorChildren({
|
||||
pageId: id,
|
||||
spaceId: node.spaceId,
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<SpaceTreeNode>);
|
||||
// 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<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 {
|
||||
|
||||
@@ -74,6 +74,48 @@ describe("treeModel.isDescendant", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// #525: the single "is this branch unloaded?" predicate shared by the lazy-load
|
||||
// gate and the insert paths. Unloaded == server says hasChildren but none are
|
||||
// present locally (canonical form `children: []`, also `undefined`). A parent
|
||||
// without hasChildren is genuinely empty, not unloaded.
|
||||
describe("treeModel.isUnloadedBranch", () => {
|
||||
type PH = TreeNode<{ name: string; hasChildren?: boolean }>;
|
||||
it("true for hasChildren + empty array (canonical unloaded form)", () => {
|
||||
const n: PH = { id: "p", name: "P", hasChildren: true, children: [] };
|
||||
expect(treeModel.isUnloadedBranch(n)).toBe(true);
|
||||
});
|
||||
it("true for hasChildren + undefined children", () => {
|
||||
const n: PH = { id: "p", name: "P", hasChildren: true };
|
||||
expect(treeModel.isUnloadedBranch(n)).toBe(true);
|
||||
});
|
||||
it("false for hasChildren + already-loaded children", () => {
|
||||
const n: PH = {
|
||||
id: "p",
|
||||
name: "P",
|
||||
hasChildren: true,
|
||||
children: [{ id: "c", name: "C" }],
|
||||
};
|
||||
expect(treeModel.isUnloadedBranch(n)).toBe(false);
|
||||
});
|
||||
it("false for a genuinely-empty parent (no hasChildren)", () => {
|
||||
expect(
|
||||
treeModel.isUnloadedBranch({
|
||||
id: "p",
|
||||
name: "P",
|
||||
hasChildren: false,
|
||||
children: [],
|
||||
} as PH),
|
||||
).toBe(false);
|
||||
expect(
|
||||
treeModel.isUnloadedBranch({ id: "p", name: "P" } as PH),
|
||||
).toBe(false);
|
||||
});
|
||||
it("false for null/undefined", () => {
|
||||
expect(treeModel.isUnloadedBranch(null)).toBe(false);
|
||||
expect(treeModel.isUnloadedBranch(undefined)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("treeModel.visible", () => {
|
||||
it("returns only root nodes when no openIds", () => {
|
||||
const v = treeModel.visible(fixture, new Set());
|
||||
@@ -197,43 +239,64 @@ describe("treeModel.insertByPosition", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
// #159 #1: inserting/moving a node under a parent whose children are NOT
|
||||
// loaded (`children === undefined`, e.g. a collapsed page) must NOT materialize
|
||||
// a partial `[node]` list — that would defeat the lazy-load gate and hide the
|
||||
// parent's other real children. The node is left to be lazy-loaded; only
|
||||
// `hasChildren` is flagged so the chevron appears.
|
||||
it("does NOT materialize a child under an UNLOADED parent (children undefined)", () => {
|
||||
type PH = TreeNode<{
|
||||
name: string;
|
||||
position?: string;
|
||||
hasChildren?: boolean;
|
||||
}>;
|
||||
type PH = TreeNode<{
|
||||
name: string;
|
||||
position?: string;
|
||||
hasChildren?: boolean;
|
||||
}>;
|
||||
|
||||
// #159 #1 / #525: inserting/moving a node under an UNLOADED parent must NOT
|
||||
// materialize a partial `[node]` list — that would defeat the lazy-load gate and
|
||||
// hide the parent's other real children. The canonical unloaded form here is
|
||||
// `children: []` + `hasChildren: true` (from `pageToTreeNode` /
|
||||
// `pruneCollapsedChildren`), which the pre-#525 `=== undefined` guard MISSED.
|
||||
// The node is left to be lazy-loaded; the chevron stays enabled.
|
||||
it("does NOT materialize a child under an UNLOADED parent (children: [], hasChildren: true)", () => {
|
||||
const tree: PH[] = [
|
||||
{ id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined
|
||||
{ id: "p", name: "P", position: "a0", hasChildren: true, children: [] },
|
||||
];
|
||||
const node: PH = { id: "x", name: "X", position: "a1" };
|
||||
const t = treeModel.insertByPosition(tree, "p", node);
|
||||
const parent = treeModel.find(t, "p");
|
||||
// The node was NOT inserted (children stay unloaded -> lazy-load fetches the
|
||||
// full set, including this node, on expand).
|
||||
expect(parent?.children).toBeUndefined();
|
||||
// full set, including this node, on expand). MUTATION: the pre-#525 predicate
|
||||
// `children === undefined` does not fire for `[]`, so it would insert `[x]`
|
||||
// here and reredden this expectation.
|
||||
expect(parent?.children).toEqual([]);
|
||||
expect(treeModel.find(t, "x")).toBeNull();
|
||||
// ...but the chevron is enabled so the user can expand to load it.
|
||||
// ...and the chevron stays enabled so the user can expand to load it.
|
||||
expect((parent as PH).hasChildren).toBe(true);
|
||||
});
|
||||
|
||||
it("DOES insert under a LOADED-but-empty parent (children: [])", () => {
|
||||
type PH = TreeNode<{
|
||||
name: string;
|
||||
position?: string;
|
||||
hasChildren?: boolean;
|
||||
}>;
|
||||
it("does NOT materialize a child under an UNLOADED parent (children undefined, hasChildren: true)", () => {
|
||||
const tree: PH[] = [
|
||||
{ id: "p", name: "P", position: "a0", hasChildren: true }, // children: undefined
|
||||
];
|
||||
const node: PH = { id: "x", name: "X", position: "a1" };
|
||||
const t = treeModel.insertByPosition(tree, "p", node);
|
||||
const parent = treeModel.find(t, "p");
|
||||
expect(parent?.children).toBeUndefined();
|
||||
expect(treeModel.find(t, "x")).toBeNull();
|
||||
expect((parent as PH).hasChildren).toBe(true);
|
||||
});
|
||||
|
||||
it("DOES insert under a genuinely-empty parent (children: [], hasChildren: false)", () => {
|
||||
const tree: PH[] = [
|
||||
{ id: "p", name: "P", position: "a0", hasChildren: false, children: [] },
|
||||
];
|
||||
const node: PH = { id: "x", name: "X", position: "a1" };
|
||||
const t = treeModel.insertByPosition(tree, "p", node);
|
||||
// A loaded (empty) child list is complete, so the node IS inserted.
|
||||
// No server children (`hasChildren: false`), so materializing the first child
|
||||
// is correct — nothing is hidden.
|
||||
expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]);
|
||||
});
|
||||
|
||||
it("DOES insert under a genuinely-empty parent (children undefined, hasChildren: false)", () => {
|
||||
const tree: PH[] = [
|
||||
{ id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined
|
||||
];
|
||||
const node: PH = { id: "x", name: "X", position: "a1" };
|
||||
const t = treeModel.insertByPosition(tree, "p", node);
|
||||
expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]);
|
||||
});
|
||||
|
||||
|
||||
@@ -43,6 +43,24 @@ export const treeModel = {
|
||||
};
|
||||
},
|
||||
|
||||
// A branch is "unloaded" when the server says it HAS children (`hasChildren`)
|
||||
// but none are present locally. The canonical unloaded form in this codebase
|
||||
// is `children: []` (produced by `pageToTreeNode` and by `pruneCollapsedChildren`
|
||||
// resetting collapsed branches), NOT `children: undefined` — so a predicate that
|
||||
// only checks `=== undefined` misses the real case and materializes a misleading
|
||||
// partial list (#525). This is the SINGLE source of truth for "should a
|
||||
// fetch/materialize be deferred?", shared by the lazy-load gate (`handleToggle`),
|
||||
// the realtime insert path (`insertByPosition`) and the DnD move guard, so they
|
||||
// can never drift apart again. A parent WITHOUT `hasChildren` is genuinely empty
|
||||
// (no server children) — inserting its first child is correct, not deferred.
|
||||
isUnloadedBranch<T extends object>(
|
||||
node: TreeNode<T> | null | undefined,
|
||||
): boolean {
|
||||
if (!node) return false;
|
||||
const hasChildren = (node as { hasChildren?: boolean }).hasChildren === true;
|
||||
return hasChildren && (node.children == null || node.children.length === 0);
|
||||
},
|
||||
|
||||
isDescendant<T extends object>(
|
||||
tree: TreeNode<T>[],
|
||||
ancestorId: string,
|
||||
@@ -127,14 +145,15 @@ export const treeModel = {
|
||||
}
|
||||
const parent = treeModel.find(tree, parentId);
|
||||
// The parent is in the tree but its children have NOT been lazy-loaded yet
|
||||
// (`children === undefined`, distinct from a loaded-but-empty `[]`). Inserting
|
||||
// (`hasChildren` set + children absent/empty — see `isUnloadedBranch`; the
|
||||
// canonical unloaded form is `children: []`, NOT just `undefined`). Inserting
|
||||
// here would MATERIALIZE a misleading partial child list (`[node]`) that
|
||||
// defeats the lazy-load gate — which fetches only when children are
|
||||
// absent/empty — so the parent's OTHER real children would never load and the
|
||||
// moved/added node would be the only one shown (a silent data loss, #159 #1).
|
||||
// Instead, leave the children unloaded and just flag `hasChildren` so the
|
||||
// chevron appears; expanding fetches the FULL set (including this node).
|
||||
if (parent && parent.children === undefined) {
|
||||
if (parent && treeModel.isUnloadedBranch(parent)) {
|
||||
return treeModel.update(
|
||||
tree,
|
||||
parentId,
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -82,17 +82,19 @@ describe("applyMoveTreeNode", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159)", () => {
|
||||
// `dstCollapsed` is in the tree but its children were never lazy-loaded
|
||||
// (children === undefined). The OLD behavior inserted `src` as the ONLY
|
||||
// child ([src]), which defeated the lazy-load gate and HID the parent's
|
||||
// other real children. Now the move leaves children unloaded (so expanding
|
||||
// fetches the FULL set, including src) and just flags hasChildren.
|
||||
it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159 #525)", () => {
|
||||
// `dstCollapsed` is in the tree but its children were never lazy-loaded. The
|
||||
// CANONICAL unloaded form here is `hasChildren: true` + `children: []` (from
|
||||
// `pageToTreeNode` / `pruneCollapsedChildren`), NOT `children: undefined`.
|
||||
// The pre-#525 predicate (`children === undefined`) missed this form and
|
||||
// inserted `src` as the ONLY child ([src]), defeating the lazy-load gate and
|
||||
// HIDING the parent's other real children. Now the move leaves children
|
||||
// unloaded (so expanding fetches the FULL set, including src).
|
||||
const tree: SpaceTreeNode[] = [
|
||||
node("dstCollapsed", {
|
||||
position: "a0",
|
||||
hasChildren: false,
|
||||
children: undefined as unknown as SpaceTreeNode[],
|
||||
hasChildren: true,
|
||||
children: [],
|
||||
}),
|
||||
node("src", { position: "a9" }),
|
||||
];
|
||||
@@ -105,9 +107,10 @@ describe("applyMoveTreeNode", () => {
|
||||
pageData: {},
|
||||
});
|
||||
const dst = treeModel.find(next, "dstCollapsed");
|
||||
// Children stay unloaded -> the lazy-load gate fetches the FULL set (incl.
|
||||
// src) on expand, rather than showing a misleading partial [src] list.
|
||||
expect(dst?.children).toBeUndefined();
|
||||
// Children stay unloaded ([] not materialized to [src]) -> the lazy-load gate
|
||||
// fetches the FULL set (incl. src) on expand. MUTATION: the pre-#525
|
||||
// `=== undefined` predicate would insert [src] here and redden this.
|
||||
expect(dst?.children).toEqual([]);
|
||||
expect(dst?.hasChildren).toBe(true);
|
||||
// src moved away from its old root slot (it lives under dstCollapsed
|
||||
// server-side and reappears when the parent is expanded/loaded).
|
||||
|
||||
Reference in New Issue
Block a user