From 570890dab6f8d7181ee17331bfceceb5e23d11fc Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 12:31:35 -0700 Subject: [PATCH] chore(simplify): generalize prune helper + add value-identity test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify pass on top of #2070 fix: - Rename pruneStaleSubtreeIds → pruneStaleKeys, generalize to Map so the same shape can absorb other keyed-by-node-id caches (ProvisioningTimeout.tsx tracking map is the obvious next caller — left as a follow-up to keep this PR scoped). - Trim the helper docstring to remove implementation-detail rot (O(map_size), cadence claims). The ref-block comment carries the rationale where it actually matters (at the call site). - Add identity-preservation test: survivors must keep their original Set reference. Guards against a future "rebuild instead of delete" regression that would silently invalidate downstream === checks. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../__tests__/useCanvasViewport.test.ts | 28 ++++++++++++------ .../components/canvas/useCanvasViewport.ts | 29 +++++++++---------- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts b/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts index dcf49526..cd9d518f 100644 --- a/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts +++ b/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { pruneStaleSubtreeIds, shouldFitGrowing } from "../useCanvasViewport"; +import { pruneStaleKeys, shouldFitGrowing } from "../useCanvasViewport"; // Tests cover the auto-fit gate in isolation. The hook itself is // effects + refs + React Flow handles, awkward to exercise directly — @@ -52,37 +52,47 @@ describe("shouldFitGrowing", () => { }); }); -describe("pruneStaleSubtreeIds (#2070)", () => { - it("drops entries whose root is no longer in the live node set", () => { +describe("pruneStaleKeys (#2070)", () => { + it("drops entries whose key is no longer in the live key set", () => { const map = new Map>([ ["root-1", new Set(["root-1", "a"])], ["root-2", new Set(["root-2", "b"])], ["root-3", new Set(["root-3", "c"])], ]); - pruneStaleSubtreeIds(map, new Set(["root-1", "root-3"])); + pruneStaleKeys(map, new Set(["root-1", "root-3"])); expect([...map.keys()].sort()).toEqual(["root-1", "root-3"]); }); - it("is a no-op when every root is still live", () => { + it("is a no-op when every key is still live", () => { const map = new Map>([ ["root-1", new Set(["root-1"])], ["root-2", new Set(["root-2"])], ]); - pruneStaleSubtreeIds(map, new Set(["root-1", "root-2"])); + pruneStaleKeys(map, new Set(["root-1", "root-2"])); expect(map.size).toBe(2); }); - it("clears the map when no live roots remain", () => { + it("clears the map when no live keys remain", () => { const map = new Map>([ ["root-1", new Set(["root-1"])], ]); - pruneStaleSubtreeIds(map, new Set()); + pruneStaleKeys(map, new Set()); expect(map.size).toBe(0); }); it("does not add new entries — only deletes stale ones", () => { const map = new Map>(); - pruneStaleSubtreeIds(map, new Set(["root-1", "root-2"])); + pruneStaleKeys(map, new Set(["root-1", "root-2"])); expect(map.size).toBe(0); }); + + it("preserves value identity for survivors (no rebuild)", () => { + const survivor = new Set(["root-1", "a"]); + const map = new Map>([ + ["root-1", survivor], + ["root-2", new Set(["root-2", "b"])], + ]); + pruneStaleKeys(map, new Set(["root-1"])); + expect(map.get("root-1")).toBe(survivor); + }); }); diff --git a/canvas/src/components/canvas/useCanvasViewport.ts b/canvas/src/components/canvas/useCanvasViewport.ts index 2c3c42e9..b8007f1d 100644 --- a/canvas/src/components/canvas/useCanvasViewport.ts +++ b/canvas/src/components/canvas/useCanvasViewport.ts @@ -41,19 +41,16 @@ export function shouldFitGrowing( } /** - * Drop entries from the last-fit snapshot map whose root id no longer - * exists in the live node set. Called on every fit attempt — the cost - * is O(map_size) and runs only at user-driven cadence (deploys), so - * the map is bounded by "roots present right now" instead of growing - * unbounded across long sessions of import-then-delete cycles. (#2070) + * Drop entries from `map` whose key isn't in `liveKeys`. Generic so the + * same shape can be reused for any keyed-by-node-id cache (#2070). */ -export function pruneStaleSubtreeIds( - map: Map>, - liveNodeIds: ReadonlySet, +export function pruneStaleKeys( + map: Map, + liveKeys: ReadonlySet, ): void { - for (const rootId of map.keys()) { - if (!liveNodeIds.has(rootId)) { - map.delete(rootId); + for (const key of map.keys()) { + if (!liveKeys.has(key)) { + map.delete(key); } } } @@ -269,9 +266,11 @@ export function useCanvasViewport() { // brand-new node landed off-screen. The id-set sees the new id // wasn't in the snapshot and forces the fit. // - // Map is keyed by root id. Pruned at the top of `runFit` against the - // live node set, so deleted roots don't accumulate across long - // sessions of import-then-delete cycles (#2070). + // Map is keyed by root id. Pruned in `runFit` against the live node + // set so deleted roots don't accumulate across long sessions of + // import-then-delete cycles (#2070). Bounded to "roots present right + // now" by that prune; cleanup runs only at user-driven cadence + // (deploys), so the rate naturally tracks growth. const lastFitSubtreeIdsRef = useRef>>(new Map()); useEffect(() => { const runFit = () => { @@ -279,7 +278,7 @@ export function useCanvasViewport() { pendingFitRootRef.current = null; if (!rootCandidate) return; const state = useCanvasStore.getState(); - pruneStaleSubtreeIds( + pruneStaleKeys( lastFitSubtreeIdsRef.current, new Set(state.nodes.map((n) => n.id)), );