From 388894c25797a9cec1b3f81777e91e57b047c0f8 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Mon, 29 Jun 2026 04:49:48 +0300 Subject: [PATCH] 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) --- .../notification/notification.utils.test.ts | 58 ++++++++++++++ .../tree/utils/find-breadcrumb-path.test.ts | 79 +++++++++++++++++++ .../features/page/tree/utils/utils.test.ts | 78 ++++++++++++++++++ .../src/features/page/tree/utils/utils.ts | 14 ++-- .../websocket/tree-socket-reducers.test.ts | 74 +++++++++++++++++ 5 files changed, 298 insertions(+), 5 deletions(-) create mode 100644 apps/client/src/features/page/tree/utils/find-breadcrumb-path.test.ts diff --git a/apps/client/src/features/notification/notification.utils.test.ts b/apps/client/src/features/notification/notification.utils.test.ts index 14d99d0e..2cfd7f55 100644 --- a/apps/client/src/features/notification/notification.utils.test.ts +++ b/apps/client/src/features/notification/notification.utils.test.ts @@ -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); + }); +}); diff --git a/apps/client/src/features/page/tree/utils/find-breadcrumb-path.test.ts b/apps/client/src/features/page/tree/utils/find-breadcrumb-path.test.ts new file mode 100644 index 00000000..0dff0f79 --- /dev/null +++ b/apps/client/src/features/page/tree/utils/find-breadcrumb-path.test.ts @@ -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 { + 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(); + }); +}); 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 4ea181b5..0eced376 100644 --- a/apps/client/src/features/page/tree/utils/utils.test.ts +++ b/apps/client/src/features/page/tree/utils/utils.test.ts @@ -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 { + 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), + ); + 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), + ); + 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")]); diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 11600162..d5084b00 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -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; diff --git a/apps/client/src/features/websocket/tree-socket-reducers.test.ts b/apps/client/src/features/websocket/tree-socket-reducers.test.ts index 81c62b56..ae93a714 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -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) => + ({ + operation: "updateOne", + spaceId: "space-1", + entity: ["pages"], + id, + payload, + }) as unknown as Parameters[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); + }); +});