From 5fa3d65fe352b2b5ae8525e4bdeb7b5089607c86 Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Sat, 9 May 2026 20:52:15 +0700 Subject: [PATCH] refactor(file-manager): replace mutating addAllToSet/removeAllFromSet with pure addAll/removeAll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old helpers mutated the Set passed in, but the type signature (`Set`) did not communicate that — only the JSDoc did. Every existing caller already cloned `prev` before calling, so the migration to pure `(prev, paths) => new Set` variants is zero-cost and removes a foot-gun. `toggleSubtreeOnSet` and `computeRowClickSelection` now compose the pure helpers directly instead of clone-then-mutate. Co-authored-by: Cursor --- .../file-manager/file-manager-utils.test.ts | 74 +++++++++++-------- .../shared/file-manager/file-manager-utils.ts | 52 ++++++------- 2 files changed, 66 insertions(+), 60 deletions(-) diff --git a/frontend/src/components/shared/file-manager/file-manager-utils.test.ts b/frontend/src/components/shared/file-manager/file-manager-utils.test.ts index 59e72ff5..dfbd0bbe 100644 --- a/frontend/src/components/shared/file-manager/file-manager-utils.test.ts +++ b/frontend/src/components/shared/file-manager/file-manager-utils.test.ts @@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { FileManagerInternalNode, FileManagerRootGroup, FileNode } from './file-manager-types'; import { - addAllToSet, + addAll, buildFileManagerGridTemplate, buildFileManagerTree, clamp, @@ -27,7 +27,7 @@ import { isEverySelected, normalizeRootGroups, pluralizeItemsEnglish, - removeAllFromSet, + removeAll, resolveSelectionModifier, sortFileManagerTree, toggleSubtreeOnSet, @@ -491,47 +491,59 @@ describe('pluralizeItemsEnglish', () => { }); }); -describe('addAllToSet', () => { - it('adds every path from the iterable into the set', () => { - const set = new Set(['a']); +describe('addAll', () => { + it('returns a new set containing every path from prev plus the iterable', () => { + const prev = new Set(['a']); + const next = addAll(prev, ['b', 'c']); - addAllToSet(set, ['b', 'c']); - - expect([...set].sort()).toEqual(['a', 'b', 'c']); + expect([...next].sort()).toEqual(['a', 'b', 'c']); }); - it('is a no-op for an empty iterable', () => { - const set = new Set(['a']); + it('does not mutate the input set', () => { + const prev = new Set(['a']); + const next = addAll(prev, ['b', 'c']); - addAllToSet(set, []); + expect(next).not.toBe(prev); + expect([...prev]).toEqual(['a']); + }); - expect([...set]).toEqual(['a']); + it('returns a freshly cloned set even for an empty iterable', () => { + const prev = new Set(['a']); + const next = addAll(prev, []); + + expect(next).not.toBe(prev); + expect([...next]).toEqual(['a']); }); it('accepts another Set as the source', () => { - const set = new Set(); + const next = addAll(new Set(), new Set(['x', 'y'])); - addAllToSet(set, new Set(['x', 'y'])); - - expect([...set].sort()).toEqual(['x', 'y']); + expect([...next].sort()).toEqual(['x', 'y']); }); }); -describe('removeAllFromSet', () => { - it('removes every present path and ignores missing ones', () => { - const set = new Set(['a', 'b', 'c']); +describe('removeAll', () => { + it('returns a new set without the requested paths and ignores missing ones', () => { + const prev = new Set(['a', 'b', 'c']); + const next = removeAll(prev, ['b', 'missing']); - removeAllFromSet(set, ['b', 'missing']); - - expect([...set].sort()).toEqual(['a', 'c']); + expect([...next].sort()).toEqual(['a', 'c']); }); - it('is a no-op for an empty iterable', () => { - const set = new Set(['a']); + it('does not mutate the input set', () => { + const prev = new Set(['a', 'b']); + const next = removeAll(prev, ['b']); - removeAllFromSet(set, []); + expect(next).not.toBe(prev); + expect([...prev].sort()).toEqual(['a', 'b']); + }); - expect([...set]).toEqual(['a']); + it('returns a freshly cloned set even for an empty iterable', () => { + const prev = new Set(['a']); + const next = removeAll(prev, []); + + expect(next).not.toBe(prev); + expect([...next]).toEqual(['a']); }); }); @@ -1060,11 +1072,11 @@ describe('computeRowClickSelection — purity', () => { }); it('never mutates the input prev Set on a range click that expands directories', () => { - // Defensive: the additive range branch builds `next` via `addAllToSet` - // on a clone of `prev`. If the clone step ever regressed, every - // directory expansion would silently corrupt React state. Shaped after - // the user's three-folder scenario so a regression here always lights - // up a real-world failure mode. + // Defensive: the additive range branch funnels every expansion through + // `addAll`, which clones internally. If that contract ever regressed, + // every directory expansion would silently corrupt React state. + // Shaped after the user's three-folder scenario so a regression here + // always lights up a real-world failure mode. const prev = new Set(['f1', 'f1/a', 'f1/b']); const snapshot = [...prev]; diff --git a/frontend/src/components/shared/file-manager/file-manager-utils.ts b/frontend/src/components/shared/file-manager/file-manager-utils.ts index f2caa02f..5ac1cd60 100644 --- a/frontend/src/components/shared/file-manager/file-manager-utils.ts +++ b/frontend/src/components/shared/file-manager/file-manager-utils.ts @@ -672,24 +672,33 @@ export const computeSelectionTotalBytes = ( }; /** - * Mutates `set` in place by adding every path from `paths`. Designed for "build a - * fresh `next` from `prev`, then add a batch" patterns inside `setState` updaters - * — never call on a state-owned Set directly. + * Returns a freshly cloned Set with every path from `paths` added. Pure — + * safe to call on a state-owned Set inside a `setState` updater because the + * input is never mutated. Empty `paths` still produces a fresh clone. */ -export const addAllToSet = (set: Set, paths: Iterable): void => { +export const addAll = (prev: ReadonlySet, paths: Iterable): Set => { + const next = new Set(prev); + for (const path of paths) { - set.add(path); + next.add(path); } + + return next; }; /** - * Mutates `set` in place by removing every path from `paths`. Same locality - * contract as `addAllToSet`: only meaningful on a freshly cloned Set. + * Returns a freshly cloned Set with every path from `paths` removed. Same + * purity contract as `addAll`: input is never mutated, missing paths are + * silently ignored. */ -export const removeAllFromSet = (set: Set, paths: Iterable): void => { +export const removeAll = (prev: ReadonlySet, paths: Iterable): Set => { + const next = new Set(prev); + for (const path of paths) { - set.delete(path); + next.delete(path); } + + return next; }; /** @@ -731,8 +740,6 @@ export const toggleSubtreeOnSet = ( paths: readonly string[], rootPath?: string, ): Set => { - const next = new Set(prev); - let allSelected: boolean; if (rootPath !== undefined && paths.length > 1) { @@ -743,22 +750,16 @@ export const toggleSubtreeOnSet = ( continue; } - if (!next.has(p)) { + if (!prev.has(p)) { allSelected = false; break; } } } else { - allSelected = isEverySelected(paths, next); + allSelected = isEverySelected(paths, prev); } - if (allSelected) { - removeAllFromSet(next, paths); - } else { - addAllToSet(next, paths); - } - - return next; + return allSelected ? removeAll(prev, paths) : addAll(prev, paths); }; interface ComputeRowClickSelectionArgs { @@ -881,15 +882,8 @@ export const computeRowClickSelection = ({ // unchecked because its descendants stayed out of the selection. const expandDir = (p: string): readonly string[] => dirSubtreePaths?.get(p) ?? [p]; - const addRangeToPrev = (paths: Iterable): Set => { - const next = new Set(prev); - - for (const p of paths) { - addAllToSet(next, expandDir(p)); - } - - return next; - }; + const addRangeToPrev = (paths: Iterable): Set => + addAll(prev, Array.from(paths).flatMap((p) => [...expandDir(p)])); const buildAdditiveFallback = (): Set => addRangeToPrev(hasSubtree && subtreePaths ? subtreePaths : [path]);