fix(client): stop findBreadcrumbPath mutating the live tree + tests
findBreadcrumbPath set node.name='Untitled' in place, mutating the shared sidebar tree (treeData passed from resolveBreadcrumbNodes). Surface 'Untitled' via a shallow copy on the returned chain only; input nodes stay untouched. Add tests for the non-mutation invariant plus applyUpdateOne reducer, formatRelativeTime buckets, and the pure tree mappers (sortPositionKeys, pageToTreeNode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,7 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import i18n from "@/i18n.ts";
|
||||
import {
|
||||
formatRelativeTime,
|
||||
getTimeGroup,
|
||||
groupNotificationsByTime,
|
||||
} from "@/features/notification/notification.utils.ts";
|
||||
@@ -132,3 +134,59 @@ describe("groupNotificationsByTime", () => {
|
||||
expect(groupNotificationsByTime([], labels)).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("formatRelativeTime — relative buckets and absolute-date fallback", () => {
|
||||
// Distinct fixed clock for the relative formatter (uses Date.now via `new
|
||||
// Date()`), so the bucket boundaries are deterministic under fake timers.
|
||||
const NOW = new Date("2026-06-15T12:00:00.000Z");
|
||||
const MIN = 60_000;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.setSystemTime(NOW);
|
||||
});
|
||||
|
||||
// ISO string `ms` milliseconds before NOW.
|
||||
function ago(ms: number): string {
|
||||
return new Date(NOW.getTime() - ms).toISOString();
|
||||
}
|
||||
|
||||
it("returns the i18n 'now' label for anything under a minute", () => {
|
||||
expect(formatRelativeTime(ago(0))).toBe(i18n.t("now"));
|
||||
expect(formatRelativeTime(ago(59_000))).toBe(i18n.t("now"));
|
||||
});
|
||||
|
||||
it("crosses into the minutes bucket exactly at 1 minute", () => {
|
||||
expect(formatRelativeTime(ago(MIN - 1000))).toBe(i18n.t("now"));
|
||||
expect(formatRelativeTime(ago(MIN))).toBe("1m");
|
||||
expect(formatRelativeTime(ago(5 * MIN))).toBe("5m");
|
||||
expect(formatRelativeTime(ago(59 * MIN))).toBe("59m");
|
||||
});
|
||||
|
||||
it("crosses into the hours bucket exactly at 60 minutes", () => {
|
||||
expect(formatRelativeTime(ago(60 * MIN - 1000))).toBe("59m");
|
||||
expect(formatRelativeTime(ago(HOUR))).toBe("1h");
|
||||
expect(formatRelativeTime(ago(23 * HOUR))).toBe("23h");
|
||||
});
|
||||
|
||||
it("crosses into the days bucket exactly at 24 hours", () => {
|
||||
expect(formatRelativeTime(ago(24 * HOUR - 1000))).toBe("23h");
|
||||
expect(formatRelativeTime(ago(DAY))).toBe("1d");
|
||||
expect(formatRelativeTime(ago(6 * DAY))).toBe("6d");
|
||||
});
|
||||
|
||||
it("falls back to an absolute short date once >= 7 days old", () => {
|
||||
// 6d -> still relative; 7d -> absolute date (no longer N[mhd], and equal to
|
||||
// the localized short-date of the source timestamp).
|
||||
expect(formatRelativeTime(ago(6 * DAY))).toBe("6d");
|
||||
|
||||
const sevenDaysAgo = ago(7 * DAY);
|
||||
const result = formatRelativeTime(sevenDaysAgo);
|
||||
expect(result).not.toMatch(/^\d+[mhd]$/);
|
||||
expect(result).not.toBe(i18n.t("now"));
|
||||
const expected = new Intl.DateTimeFormat(i18n.language, {
|
||||
month: "short",
|
||||
day: "numeric",
|
||||
}).format(new Date(sevenDaysAgo));
|
||||
expect(result).toBe(expected);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { findBreadcrumbPath } from "./utils";
|
||||
import type { SpaceTreeNode } from "@/features/page/tree/types.ts";
|
||||
|
||||
// findBreadcrumbPath walks the live, SHARED sidebar tree. The high-value
|
||||
// invariant: when a node has no usable name it must surface "Untitled" ONLY on
|
||||
// the returned breadcrumb chain via a shallow copy — never by mutating the input
|
||||
// node (which would silently rename the node in the sidebar). Also covers normal
|
||||
// ancestor-chain resolution, the not-found case, and nested children.
|
||||
|
||||
function node(id: string, over: Partial<SpaceTreeNode> = {}): SpaceTreeNode {
|
||||
return {
|
||||
id,
|
||||
slugId: `slug-${id}`,
|
||||
name: id.toUpperCase(),
|
||||
icon: undefined,
|
||||
position: "a0",
|
||||
spaceId: "space-1",
|
||||
parentPageId: null as unknown as string,
|
||||
hasChildren: false,
|
||||
children: [],
|
||||
...over,
|
||||
};
|
||||
}
|
||||
|
||||
describe("findBreadcrumbPath", () => {
|
||||
it("does NOT mutate the input tree when a node has an empty/whitespace name", () => {
|
||||
// A whitespace-only-named node nested under a blank-named root.
|
||||
const target = node("target", { name: " " });
|
||||
const root = node("root", { name: "", hasChildren: true, children: [target] });
|
||||
const tree = [root];
|
||||
|
||||
const result = findBreadcrumbPath(tree, "target");
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
// The RETURNED chain shows "Untitled" for both blank nodes.
|
||||
expect(result!.map((n) => n.name)).toEqual(["Untitled", "Untitled"]);
|
||||
// The original input nodes are untouched (still blank).
|
||||
expect(root.name).toBe("");
|
||||
expect(target.name).toBe(" ");
|
||||
// The renamed breadcrumb entries are fresh copies, not the input objects.
|
||||
expect(result![0]).not.toBe(root);
|
||||
expect(result![1]).not.toBe(target);
|
||||
});
|
||||
|
||||
it("returns the SAME node reference (no copy) when the name is non-empty", () => {
|
||||
// No rename needed -> the node is passed through by reference (cheap path).
|
||||
const target = node("target", { name: "Real Title" });
|
||||
const result = findBreadcrumbPath([target], "target");
|
||||
expect(result![0]).toBe(target);
|
||||
expect(result![0].name).toBe("Real Title");
|
||||
});
|
||||
|
||||
it("resolves the full ancestor chain ending at the target", () => {
|
||||
const target = node("c");
|
||||
const mid = node("b", { hasChildren: true, children: [target] });
|
||||
const root = node("a", { hasChildren: true, children: [mid] });
|
||||
const result = findBreadcrumbPath([root], "c");
|
||||
expect(result!.map((n) => n.id)).toEqual(["a", "b", "c"]);
|
||||
});
|
||||
|
||||
it("finds a target nested under a deeper sibling branch", () => {
|
||||
// Two root branches; the target lives inside the second branch's child.
|
||||
const target = node("deep");
|
||||
const branch2 = node("r2", {
|
||||
hasChildren: true,
|
||||
children: [node("x"), node("y", { hasChildren: true, children: [target] })],
|
||||
});
|
||||
const branch1 = node("r1", { hasChildren: true, children: [node("z")] });
|
||||
const result = findBreadcrumbPath([branch1, branch2], "deep");
|
||||
expect(result!.map((n) => n.id)).toEqual(["r2", "y", "deep"]);
|
||||
});
|
||||
|
||||
it("returns null when the page id is not present in the tree", () => {
|
||||
const root = node("root", { hasChildren: true, children: [node("child")] });
|
||||
expect(findBreadcrumbPath([root], "missing")).toBeNull();
|
||||
expect(findBreadcrumbPath([], "anything")).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -8,6 +8,8 @@ import {
|
||||
closeIds,
|
||||
mergeRootTrees,
|
||||
loadedOpenBranchIds,
|
||||
sortPositionKeys,
|
||||
pageToTreeNode,
|
||||
} from "./utils";
|
||||
import type { IPage } from "@/features/page/types/page.types.ts";
|
||||
import type { SpaceTreeNode } from "@/features/page/tree/types.ts";
|
||||
@@ -60,6 +62,82 @@ function treeNode(id: string, children: SpaceTreeNode[] = []): SpaceTreeNode {
|
||||
};
|
||||
}
|
||||
|
||||
describe("sortPositionKeys", () => {
|
||||
it("orders items ascending by their fractional `position` string", () => {
|
||||
const items = [
|
||||
{ id: "c", position: "a5" },
|
||||
{ id: "a", position: "a1" },
|
||||
{ id: "b", position: "a3" },
|
||||
];
|
||||
expect(sortPositionKeys(items).map((i) => i.id)).toEqual(["a", "b", "c"]);
|
||||
});
|
||||
|
||||
it("is a stable sort: equal positions keep their input order", () => {
|
||||
const items = [
|
||||
{ id: "x", position: "a1" },
|
||||
{ id: "y", position: "a1" },
|
||||
{ id: "z", position: "a1" },
|
||||
];
|
||||
expect(sortPositionKeys(items).map((i) => i.id)).toEqual(["x", "y", "z"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("pageToTreeNode", () => {
|
||||
function pageRow(over: Partial<IPage> = {}): IPage {
|
||||
return {
|
||||
id: "p1",
|
||||
slugId: "slug-p1",
|
||||
title: "My Page",
|
||||
icon: "📄",
|
||||
position: "a1",
|
||||
hasChildren: true,
|
||||
spaceId: "space-1",
|
||||
parentPageId: null as unknown as string,
|
||||
...over,
|
||||
} as IPage;
|
||||
}
|
||||
|
||||
it("maps page.title -> node.name and copies the core fields", () => {
|
||||
const node = pageToTreeNode(pageRow());
|
||||
// The non-trivial transform: a page's `title` becomes the tree node's `name`.
|
||||
expect(node.name).toBe("My Page");
|
||||
expect(node.id).toBe("p1");
|
||||
expect(node.slugId).toBe("slug-p1");
|
||||
expect(node.icon).toBe("📄");
|
||||
expect(node.position).toBe("a1");
|
||||
expect(node.spaceId).toBe("space-1");
|
||||
expect(node.hasChildren).toBe(true);
|
||||
// Always materialized with an empty children array.
|
||||
expect(node.children).toEqual([]);
|
||||
});
|
||||
|
||||
it("derives canEdit from page.permissions.canEdit when the flat field is absent", () => {
|
||||
const node = pageToTreeNode(
|
||||
pageRow({ canEdit: undefined, permissions: { canEdit: true } } as Partial<IPage>),
|
||||
);
|
||||
expect(node.canEdit).toBe(true);
|
||||
});
|
||||
|
||||
it("prefers the flat page.canEdit over permissions.canEdit", () => {
|
||||
const node = pageToTreeNode(
|
||||
pageRow({ canEdit: false, permissions: { canEdit: true } } as Partial<IPage>),
|
||||
);
|
||||
expect(node.canEdit).toBe(false);
|
||||
});
|
||||
|
||||
it("carries temporaryExpiresAt straight off the page", () => {
|
||||
const expiresAt = "2026-06-27T21:00:00.000Z";
|
||||
expect(pageToTreeNode(pageRow({ temporaryExpiresAt: expiresAt })).temporaryExpiresAt).toBe(
|
||||
expiresAt,
|
||||
);
|
||||
});
|
||||
|
||||
it("applies overrides on top of the mapped fields (e.g. optimistic blank name)", () => {
|
||||
const node = pageToTreeNode(pageRow(), { name: "" });
|
||||
expect(node.name).toBe("");
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildTree", () => {
|
||||
it("builds one node per unique page", () => {
|
||||
const tree = buildTree([page("a", "a1"), page("b", "a2")]);
|
||||
|
||||
@@ -70,18 +70,22 @@ export function findBreadcrumbPath(
|
||||
path: SpaceTreeNode[] = [],
|
||||
): SpaceTreeNode[] | null {
|
||||
for (const node of tree) {
|
||||
if (!node.name || node.name.trim() === "") {
|
||||
node.name = "Untitled";
|
||||
}
|
||||
// Never mutate the input tree (it is the live, shared sidebar tree state).
|
||||
// When a node has no usable name, surface "Untitled" via a shallow copy that
|
||||
// only the returned breadcrumb chain sees — the source node stays untouched.
|
||||
const displayNode: SpaceTreeNode =
|
||||
!node.name || node.name.trim() === ""
|
||||
? { ...node, name: "Untitled" }
|
||||
: node;
|
||||
|
||||
if (node.id === pageId) {
|
||||
return [...path, node];
|
||||
return [...path, displayNode];
|
||||
}
|
||||
|
||||
if (node.children) {
|
||||
const newPath = findBreadcrumbPath(node.children, pageId, [
|
||||
...path,
|
||||
node,
|
||||
displayNode,
|
||||
]);
|
||||
if (newPath) {
|
||||
return newPath;
|
||||
|
||||
@@ -3,6 +3,7 @@ import {
|
||||
applyAddTreeNode,
|
||||
applyMoveTreeNode,
|
||||
applyDeleteTreeNode,
|
||||
applyUpdateOne,
|
||||
} from "./tree-socket-reducers";
|
||||
import { treeModel } from "@/features/page/tree/model/tree-model";
|
||||
import { SpaceTreeNode } from "@/features/page/tree/types.ts";
|
||||
@@ -338,3 +339,76 @@ describe("applyAddTreeNode", () => {
|
||||
expect(treeModel.find(next, "temp")?.temporaryExpiresAt).toBe(expiresAt);
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyUpdateOne", () => {
|
||||
// A loaded two-level tree so we can patch both a root and a nested node.
|
||||
const buildTree = (): SpaceTreeNode[] => [
|
||||
node("root", {
|
||||
position: "a0",
|
||||
name: "Root",
|
||||
icon: "📁",
|
||||
hasChildren: true,
|
||||
children: [node("child", { position: "a1", parentPageId: "root", name: "Child", icon: "📄" })],
|
||||
}),
|
||||
];
|
||||
|
||||
// Build the UpdateEvent envelope; only `id`/`payload` matter to the reducer.
|
||||
const ev = (id: string, payload: Record<string, unknown>) =>
|
||||
({
|
||||
operation: "updateOne",
|
||||
spaceId: "space-1",
|
||||
entity: ["pages"],
|
||||
id,
|
||||
payload,
|
||||
}) as unknown as Parameters<typeof applyUpdateOne>[1];
|
||||
|
||||
it("applies a title-only update to the node's name (icon untouched)", () => {
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("child", { title: "Renamed" }));
|
||||
const child = treeModel.find(next, "child");
|
||||
expect(child?.name).toBe("Renamed");
|
||||
// Icon is left as it was.
|
||||
expect(child?.icon).toBe("📄");
|
||||
});
|
||||
|
||||
it("applies an icon-only update to the node's icon (name untouched)", () => {
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("root", { icon: "🔥" }));
|
||||
const root = treeModel.find(next, "root");
|
||||
expect(root?.icon).toBe("🔥");
|
||||
expect(root?.name).toBe("Root");
|
||||
});
|
||||
|
||||
it("applies a combined title + icon update", () => {
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("child", { title: "Both", icon: "⭐" }));
|
||||
const child = treeModel.find(next, "child");
|
||||
expect(child?.name).toBe("Both");
|
||||
expect(child?.icon).toBe("⭐");
|
||||
});
|
||||
|
||||
it("returns prev UNCHANGED (same reference) when the id is not loaded", () => {
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("ghost", { title: "Nope" }));
|
||||
expect(next).toBe(tree);
|
||||
});
|
||||
|
||||
it("returns prev UNCHANGED (same reference) for a no-op payload (no title/icon)", () => {
|
||||
// The node exists, but the payload carries neither title nor icon -> nothing
|
||||
// to patch, so the reducer must hand back the same array reference.
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("child", {}));
|
||||
expect(next).toBe(tree);
|
||||
});
|
||||
|
||||
it("treats an explicit null icon/title as a value to apply (undefined check, not truthiness)", () => {
|
||||
// The reducer guards on `!== undefined`, so a clearing null IS applied.
|
||||
const tree = buildTree();
|
||||
const next = applyUpdateOne(tree, ev("child", { title: "", icon: null }));
|
||||
const child = treeModel.find(next, "child");
|
||||
expect(child?.name).toBe("");
|
||||
expect(child?.icon).toBeNull();
|
||||
// And it did change something -> a fresh reference, not prev.
|
||||
expect(next).not.toBe(tree);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user