feat(core): add evaluate util#313
Open
harry-whorlow wants to merge 17 commits intoTanStack:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new generic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 913bf8e
☁️ Nx Cloud last updated this comment at |
@tanstack/angular-store
@tanstack/preact-store
@tanstack/react-store
@tanstack/solid-store
@tanstack/store
@tanstack/svelte-store
@tanstack/vue-store
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/store/src/evaluate.ts (3)
44-58: UsehasOwnPropertyinstead ofArray.includesfor O(n) key lookup, and consider including symbol keys.Two concerns on the plain-object branch:
keysB.includes(key)makes the loop O(n·m). The siblingshallowutil inpackages/store/src/shallow.tsusesObject.prototype.hasOwnProperty.call(objB, key)which is O(1) per check.Object.keysignores symbol-keyed properties;shallowuses agetOwnKeyshelper that concatenatesObject.getOwnPropertySymbols. The two utilities should agree on what "the keys of an object" means, otherwise switching betweenshallowandevaluate(e.g. viaAtomOptions.compare) silently changes semantics.♻️ Proposed refactor
- const keysA = Object.keys(objA as object) - const keysB = Object.keys(objB as object) - - if (keysA.length !== keysB.length) { - return false - } - - for (const key of keysA) { - if ( - !keysB.includes(key) || - !evaluate(objA[key as keyof T], objB[key as keyof T]) - ) { - return false - } - } - - return true + const keysA = getOwnKeys(objA as object) + if (keysA.length !== getOwnKeys(objB as object).length) { + return false + } + + for (const key of keysA) { + if ( + !Object.prototype.hasOwnProperty.call(objB, key) || + !evaluate( + (objA as any)[key], + (objB as any)[key], + ) + ) { + return false + } + } + + return true +} + +function getOwnKeys(obj: object): Array<string | symbol> { + return (Object.keys(obj) as Array<string | symbol>).concat( + Object.getOwnPropertySymbols(obj), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 44 - 58, Replace the O(n) includes-based key check and key collection to match shallow.ts: use the same getOwnKeys helper (which concatenates Object.keys and Object.getOwnPropertySymbols) instead of Object.keys for keysA/keysB, and replace keysB.includes(key) with Object.prototype.hasOwnProperty.call(objB, key) (or use the equivalent helper used by shallow) so lookups are O(1) and symbol-keyed properties are considered; keep the existing deep evaluate calls for value comparison.
1-61: Heavy duplication withshallow— consider sharing a single implementation.This file reproduces ~80% of
packages/store/src/shallow.ts(Object.is short-circuit, null/non-object guard, Date/Map/Set branches, key-length + key-membership loop). The only material differences are: (a) recursive descent on object/Map values, (b) addedFilebranch, (c)Object.keysvsgetOwnKeys. A small refactor — e.g. accepting acomparecallback (Object.isfor shallow,evaluateitself for deep) — would let both share one implementation and avoid drift like the inconsistent Map-value handling and symbol-key handling flagged above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 1 - 61, The evaluate function duplicates most logic from shallow; refactor both to share a single implementation by extracting a common compareDeepShallow utility that accepts a compare callback (pass Object.is for shallow and evaluate for deep recursion) and a key enumerator option (use getOwnKeys to include symbol keys instead of Object.keys); update evaluate to reuse this utility, preserve the File branch and recursive descent for Map values, and fix Map handling so both implementations use the same membership/value comparison strategy; adjust shallow to call the new utility with a non-recursive compare callback.
1-1: Add explicit return type annotation.The function returns
booleanbut lacks an explicit return type. Add: booleanto the public signature.Optionally, consider renaming
evaluatetodeepEqual(ordeep, mirroringshallow) for clarity — it better communicates intent at the call site (e.g.,compare: deepEqual) and distinguishes it from the similarly namedshallowfunction sitting next to it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` at line 1, The public function evaluate currently lacks an explicit return type; update the signature of evaluate<T>(objA: T, objB: T) to include a boolean return type (evaluate<T>(objA: T, objB: T): boolean) so callers see the result type; optionally rename the function to deepEqual (or deep) to better reflect its behavior and distinguish it from the neighboring shallow function, updating all call sites and exports accordingly (refer to the evaluate function and the shallow symbol when renaming).packages/store/tests/evaluate.spec.ts (1)
92-151: Add tests that exercise the edge cases of deep equality.The current suite would still pass against the bugs flagged in
evaluate.ts. Consider adding cases that pin down the intended behaviour:
- Cross-type:
evaluate(new Date(), {}),evaluate(new Map(), {}),evaluate(new Set(), {}),evaluate(new Map([['a', 1]]), { a: 1 })— all should befalse.Mapwith structurally-equal-but-non-identical object values, e.g.new Map([['a', { x: 1 }]])vsnew Map([['a', { x: 1 }]])— should betruefor deep equality.Setwith object members (document expected behaviour either way), e.g.new Set([{ a: 1 }])vsnew Set([{ a: 1 }]).- Objects keyed by
Symbol— useful if the symbol-handling refactor is adopted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/tests/evaluate.spec.ts` around lines 92 - 151, Add targeted deep-equality tests that fail current buggy paths in evaluate: add cross-type assertions like expect(evaluate(new Date(), {})).toEqual(false), expect(evaluate(new Map(), {})).toEqual(false), expect(evaluate(new Set(), {})).toEqual(false) and expect(evaluate(new Map([['a',1]]), { a: 1 })).toEqual(false); add Map/Set cases with structurally equal but non-identical object values such as expect(evaluate(new Map([['a',{x:1}]]), new Map([['a',{x:1}]]))).toEqual(true) and a corresponding Set case (decide expected behavior and assert it), and include a test using Symbol keys on an object to verify symbol-key handling — all tests should call the existing evaluate function so failures point to evaluate's logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/store/src/evaluate.ts`:
- Around line 28-34: The Map branch inside the evaluate function compares Map
values shallowly using Object.is (objA/objB Map handling), which breaks deep
equality; change the comparison in the for-loop from Object.is(v, objB.get(k))
to recursively call evaluate(v, objB.get(k)) so nested objects are compared
deeply, and add a unit test in evaluate.spec.ts asserting evaluate(new
Map([['a',{x:1}]]), new Map([['a',{x:1}]])) returns true to cover this case.
- Around line 15-42: The evaluate function's type-specific branches for Date,
File, Map, and Set currently only handle the case where both operands are that
type but silently fall through (and can erroneously return true) when only one
operand is the special type; update each branch in evaluate so it first rejects
mixed-type cases — e.g., if either operand is a Date but not both, immediately
return false, and do the same for File, Map, and Set — keeping the existing
equality logic (objA.getTime() === objB.getTime(), file property comparisons,
Map size/entries check, Set size/contents check) only when both operands are the
same built-in type.
---
Nitpick comments:
In `@packages/store/src/evaluate.ts`:
- Around line 44-58: Replace the O(n) includes-based key check and key
collection to match shallow.ts: use the same getOwnKeys helper (which
concatenates Object.keys and Object.getOwnPropertySymbols) instead of
Object.keys for keysA/keysB, and replace keysB.includes(key) with
Object.prototype.hasOwnProperty.call(objB, key) (or use the equivalent helper
used by shallow) so lookups are O(1) and symbol-keyed properties are considered;
keep the existing deep evaluate calls for value comparison.
- Around line 1-61: The evaluate function duplicates most logic from shallow;
refactor both to share a single implementation by extracting a common
compareDeepShallow utility that accepts a compare callback (pass Object.is for
shallow and evaluate for deep recursion) and a key enumerator option (use
getOwnKeys to include symbol keys instead of Object.keys); update evaluate to
reuse this utility, preserve the File branch and recursive descent for Map
values, and fix Map handling so both implementations use the same
membership/value comparison strategy; adjust shallow to call the new utility
with a non-recursive compare callback.
- Line 1: The public function evaluate currently lacks an explicit return type;
update the signature of evaluate<T>(objA: T, objB: T) to include a boolean
return type (evaluate<T>(objA: T, objB: T): boolean) so callers see the result
type; optionally rename the function to deepEqual (or deep) to better reflect
its behavior and distinguish it from the neighboring shallow function, updating
all call sites and exports accordingly (refer to the evaluate function and the
shallow symbol when renaming).
In `@packages/store/tests/evaluate.spec.ts`:
- Around line 92-151: Add targeted deep-equality tests that fail current buggy
paths in evaluate: add cross-type assertions like expect(evaluate(new Date(),
{})).toEqual(false), expect(evaluate(new Map(), {})).toEqual(false),
expect(evaluate(new Set(), {})).toEqual(false) and expect(evaluate(new
Map([['a',1]]), { a: 1 })).toEqual(false); add Map/Set cases with structurally
equal but non-identical object values such as expect(evaluate(new
Map([['a',{x:1}]]), new Map([['a',{x:1}]]))).toEqual(true) and a corresponding
Set case (decide expected behavior and assert it), and include a test using
Symbol keys on an object to verify symbol-key handling — all tests should call
the existing evaluate function so failures point to evaluate's logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f093f9be-8e7d-4053-aff2-a1c2f39a1a69
📒 Files selected for processing (2)
packages/store/src/evaluate.tspackages/store/tests/evaluate.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/store/src/evaluate.ts (2)
32-38:⚠️ Potential issue | 🟠 Major
Mapvalues are compared shallowly viaObject.is, breaking deep equality.Line 35 uses
Object.is(v, objB.get(k))while the plain-object branch at Line 58 recurses, so e.g.evaluate(new Map([['a', { x: 1 }]]), new Map([['a', { x: 1 }]]))returnsfalse. Replace with a recursive call:🐛 Proposed fix
if (objA instanceof Map && objB instanceof Map) { if (objA.size !== objB.size) return false for (const [k, v] of objA) { - if (!objB.has(k) || !Object.is(v, objB.get(k))) return false + if (!objB.has(k) || !evaluate(v, objB.get(k))) return false } return true }A test in
evaluate.spec.tscovering nested Map values would help lock this in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 32 - 38, Map branch in evaluate compares values using Object.is causing shallow equality; change the value comparison inside the Map loop to call the same recursive equality function (evaluate) instead of Object.is so nested objects/maps are compared deeply (i.e., replace Object.is(v, objB.get(k)) with a recursive call to evaluate(v, objB.get(k))). Ensure you still check key presence and sizes as before and return false on the first non-equal value; add a unit test in evaluate.spec.ts that asserts evaluate(new Map([['a',{x:1}]]), new Map([['a',{x:1}]])) === true to prevent regressions.
15-46:⚠️ Potential issue | 🔴 CriticalCross-type comparisons still fall through to the key-count branch.
When only one operand is a
Date/File/Map/Set, its dedicated branch is skipped because each guard requires both sides to be instances. Execution then reaches Lines 48–62 whereObject.keys()returns[]for these built-ins, so any pairing with a peer that also has zero own enumerable string keys is reported equal. Concretely:
evaluate(new Date(), {})→trueevaluate(new Map([['a', 1]]), {})→true(Maps have 0 own enumerable string keys)evaluate(new Set([1]), {})→trueevaluate(new Date(), new Map())→trueEach branch should also reject when only one side is the special type. Switch from
&&to||in the guards (with a same-type check inside) so mixed-type comparisons returnfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 15 - 46, The equality checks in evaluate (Date/File/Map/Set branches) currently only run when both operands are instances, allowing mixed-type comparisons to fall through; update each guard in evaluate.ts to use disjunctive checks (e.g., typeof File !== 'undefined' && (objA instanceof File || objB instanceof File)) and inside each branch first verify both sides are the same special type (objA instanceof File && objB instanceof File) and return false if not, then keep the existing same-type comparison logic for equality; apply this pattern to the Date, File, Map, and Set branches so mixed-type pairs immediately return false.
🧹 Nitpick comments (2)
packages/store/src/evaluate.ts (2)
1-1: Consider documenting the public API and clarifying the array case.
evaluateis exported but has no JSDoc. Given the PR adds a public utility (and per the relevant snippets it can be passed asAtomOptions.compare), a brief docblock describing supported types, deep-vs-shallow semantics forSet, and circular-ref behavior would help consumers. Also note that arrays are not specially handled and currently flow through the plain-object branch, soevaluate([1,2], { 0: 1, 1: 2 })returnstrue— likely worth either adding anArray.isArraysymmetry check or documenting the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` at line 1, Add a JSDoc for the exported function evaluate<T> that documents its supported input types (primitives, plain objects, Sets), the comparison semantics (deep equality for plain objects, element-wise semantics for Sets or note shallow vs deep), and behavior with circular references; then either update evaluate to explicitly treat arrays symmetrically (use Array.isArray to ensure two arrays are only equal to other arrays and compare by length/items) or document that arrays are treated as plain objects (so [1,2] equals {0:1,1:2}) and that callers must pass arrays in matching shapes; reference the evaluate function and use Array.isArray in the implementation or the docblock to clarify or enforce array symmetry, and mention Set comparison strategy (e.g., element-wise by value) and circular-ref handling.
55-62:keysB.includes(key)makes key lookup O(n²); preferhasOwnProperty.For each key in
objAyou scan all ofkeysB. On wide objects this becomes quadratic. Switching toObject.prototype.hasOwnProperty.call(objB, key)keeps semantics (own-property check) and is O(1) per probe — same approach used inshallow.tsline 46.♻️ Proposed refactor
for (const key of keysA) { if ( - !keysB.includes(key) || + !Object.prototype.hasOwnProperty.call(objB, key) || !evaluate(objA[key as keyof T], objB[key as keyof T]) ) { return false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 55 - 62, The loop that compares keys in evaluate.ts currently uses keysB.includes(key) which yields O(n²); in the for (const key of keysA) block replace the includes check with an own-property check using Object.prototype.hasOwnProperty.call(objB, key) to retain the same own-property semantics and make lookups O(1) per key, keeping the rest of the condition that calls evaluate(objA[key as keyof T], objB[key as keyof T]) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/store/src/evaluate.ts`:
- Around line 40-46: The Set branch in evaluate currently uses objB.has(v) which
compares by reference; replace it with an O(n²) element-wise deep match: convert
objB to a mutable array (or track a used[] boolean), then for each element v in
objA iterate that array to find an element w where the recursive deep comparison
(call evaluate or the internal comparator used elsewhere in this file) returns
true, mark that w as claimed (or remove it) and continue; if any v has no match
return false, otherwise return true. If you prefer not to change complexity,
instead add a JSDoc to the evaluate function above the Set-handling code noting
that Set elements are compared by reference (SameValueZero) and deep equality
for Set members is not supported when used as AtomOptions.compare.
- Around line 1-65: The evaluate function can infinitely recurse on cyclic
structures; add an internal helper (e.g., evaluateInternal) that accepts a
WeakMap or WeakSet to track visited object-pairs and pass that map through every
recursive call from evaluate; before recursing, check the visited structure for
the current (objA, objB) pair and immediately return true if already seen,
otherwise record the pair and proceed, ensuring all recursive branches use the
helper so cyclic references short-circuit instead of causing a stack overflow.
- Around line 48-62: The equality check in evaluate.ts uses Object.keys
(keysA/keysB) which omits Symbol-keyed properties causing mismatches with
shallow; update the key collection to include symbol keys (use the same
getOwnKeys helper used by shallow or replicate its logic:
Object.keys(obj).concat(Object.getOwnPropertySymbols(obj) as any)) and replace
references to keysA/keysB so the length check, includes check and subsequent
evaluate(objA[key], objB[key]) comparisons consider Symbol keys as well; ensure
you reference the existing getOwnKeys helper (or create it) so evaluate and
shallow remain consistent.
---
Duplicate comments:
In `@packages/store/src/evaluate.ts`:
- Around line 32-38: Map branch in evaluate compares values using Object.is
causing shallow equality; change the value comparison inside the Map loop to
call the same recursive equality function (evaluate) instead of Object.is so
nested objects/maps are compared deeply (i.e., replace Object.is(v, objB.get(k))
with a recursive call to evaluate(v, objB.get(k))). Ensure you still check key
presence and sizes as before and return false on the first non-equal value; add
a unit test in evaluate.spec.ts that asserts evaluate(new Map([['a',{x:1}]]),
new Map([['a',{x:1}]])) === true to prevent regressions.
- Around line 15-46: The equality checks in evaluate (Date/File/Map/Set
branches) currently only run when both operands are instances, allowing
mixed-type comparisons to fall through; update each guard in evaluate.ts to use
disjunctive checks (e.g., typeof File !== 'undefined' && (objA instanceof File
|| objB instanceof File)) and inside each branch first verify both sides are the
same special type (objA instanceof File && objB instanceof File) and return
false if not, then keep the existing same-type comparison logic for equality;
apply this pattern to the Date, File, Map, and Set branches so mixed-type pairs
immediately return false.
---
Nitpick comments:
In `@packages/store/src/evaluate.ts`:
- Line 1: Add a JSDoc for the exported function evaluate<T> that documents its
supported input types (primitives, plain objects, Sets), the comparison
semantics (deep equality for plain objects, element-wise semantics for Sets or
note shallow vs deep), and behavior with circular references; then either update
evaluate to explicitly treat arrays symmetrically (use Array.isArray to ensure
two arrays are only equal to other arrays and compare by length/items) or
document that arrays are treated as plain objects (so [1,2] equals {0:1,1:2})
and that callers must pass arrays in matching shapes; reference the evaluate
function and use Array.isArray in the implementation or the docblock to clarify
or enforce array symmetry, and mention Set comparison strategy (e.g.,
element-wise by value) and circular-ref handling.
- Around line 55-62: The loop that compares keys in evaluate.ts currently uses
keysB.includes(key) which yields O(n²); in the for (const key of keysA) block
replace the includes check with an own-property check using
Object.prototype.hasOwnProperty.call(objB, key) to retain the same own-property
semantics and make lookups O(1) per key, keeping the rest of the condition that
calls evaluate(objA[key as keyof T], objB[key as keyof T]) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d7c185a-cfe2-47d3-abff-e3feffbf11df
📒 Files selected for processing (2)
packages/store/src/evaluate.tspackages/store/tests/evaluate.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/store/tests/evaluate.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/store/src/evaluate.ts (2)
101-103:⚠️ Potential issue | 🟡 MinorSymbol-keyed properties are skipped during object comparison.
Line 101 and Line 102 use
Object.keys, so symbol keys are ignored and objects can be reported equal when symbol values differ.Proposed fix
- const keysA = Object.keys(objA as object) - const keysB = Object.keys(objB as object) + const keysA = (Object.keys(objA as object) as Array<string | symbol>).concat( + Object.getOwnPropertySymbols(objA as object), + ) + const keysB = (Object.keys(objB as object) as Array<string | symbol>).concat( + Object.getOwnPropertySymbols(objB as object), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 101 - 103, The object comparison currently uses Object.keys (variables keysA and keysB) which omits symbol-keyed properties; change the key collection to include symbols (e.g., use Reflect.ownKeys(objA) / Reflect.ownKeys(objB) or combine Object.getOwnPropertyNames with Object.getOwnPropertySymbols) so symbol properties are compared as well, and ensure any subsequent sorting/iteration that relied on keysA/keysB still works with the expanded key lists.
81-85:⚠️ Potential issue | 🟠 MajorDeep
Setmatching is not one-to-one, so unequal sets can pass.At Line 83,
.some(...)can match the sameobjBelement for multipleobjAelements. This allows false positives in deep mode.Proposed fix
if (config.mode === 'deep') { - for (const v of objA) { - if (![...objB].some((bv) => _evaluate(v, bv, config, seen))) - return false - } + const remaining = [...objB] + for (const v of objA) { + const matchIndex = remaining.findIndex((bv) => + _evaluate(v, bv, config, seen), + ) + if (matchIndex === -1) return false + remaining.splice(matchIndex, 1) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/evaluate.ts` around lines 81 - 85, The deep Set comparison in _evaluate (when config.mode === 'deep') incorrectly allows multiple objA items to match the same objB element via .some; change the algorithm to perform one-to-one matching by iterating objA and for each element searching objB for the first unused element that satisfies _evaluate, tracking used objB indices/elements (e.g., a Set of used items or a mutable copy of objB with removals) so each objB value can only be matched once; keep using the existing _evaluate signature and the seen recursion tracking.
🧹 Nitpick comments (1)
packages/store/tests/evaluate.spec.ts (1)
368-393: Add a regression case for one-to-one deepSetmatching.Current coverage does not assert that each deep-matched element in
objBis consumed once, so false positives can slip through.Suggested test addition
it('should test equality between Set objects', () => { @@ expect( evaluate( new Set([{ nested: { x: 1 } }]), new Set([{ nested: { x: 1 } }]), { mode: 'deep' }, ), ).toEqual(true) + + // one-to-one deep matching regression + expect( + evaluate( + new Set([{ x: 1 }, { x: 1 }]), + new Set([{ x: 1 }, { x: 2 }]), + { mode: 'deep' }, + ), + ).toEqual(false) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/tests/evaluate.spec.ts` around lines 368 - 393, Add a regression test to assert one-to-one deep matching for Set elements: when calling evaluate with Sets containing duplicate deep objects (use evaluate and the existing test scope 'should test equality between Set objects' or a new it block), include a positive case where both Sets contain two identical deep objects and expect true, and a negative case where one Set has two identical deep objects but the other Set only has one matching deep object (or a different second object) and expect false; this ensures evaluate consumes matched elements one-to-one during deep Set comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/store/src/evaluate.ts`:
- Around line 1-7: The new evaluate<T>(...) function in
packages/store/src/evaluate.ts is not exported from the package barrel, so
consumers cannot import it from the package root; open
packages/store/src/index.ts and add a re-export for evaluate (e.g., export {
evaluate } from './evaluate') so the evaluate function is part of the public API
surface, then run build/type checks to ensure the barrel compiles correctly.
In `@packages/store/tests/evaluate.spec.ts`:
- Around line 31-50: The test stubs the global File using vi.stubGlobal and
currently calls vi.unstubAllGlobals() at the end but not in a finally block;
update the test that calls evaluate(file1, file2) so the global cleanup
(vi.unstubAllGlobals()) runs in a finally block to guarantee the File stub is
removed even if the expect throws—locate the test in
packages/store/tests/evaluate.spec.ts and modify the block that uses
vi.stubGlobal('File', undefined) and evaluate(...) to perform
vi.unstubAllGlobals() inside a finally clause.
---
Duplicate comments:
In `@packages/store/src/evaluate.ts`:
- Around line 101-103: The object comparison currently uses Object.keys
(variables keysA and keysB) which omits symbol-keyed properties; change the key
collection to include symbols (e.g., use Reflect.ownKeys(objA) /
Reflect.ownKeys(objB) or combine Object.getOwnPropertyNames with
Object.getOwnPropertySymbols) so symbol properties are compared as well, and
ensure any subsequent sorting/iteration that relied on keysA/keysB still works
with the expanded key lists.
- Around line 81-85: The deep Set comparison in _evaluate (when config.mode ===
'deep') incorrectly allows multiple objA items to match the same objB element
via .some; change the algorithm to perform one-to-one matching by iterating objA
and for each element searching objB for the first unused element that satisfies
_evaluate, tracking used objB indices/elements (e.g., a Set of used items or a
mutable copy of objB with removals) so each objB value can only be matched once;
keep using the existing _evaluate signature and the seen recursion tracking.
---
Nitpick comments:
In `@packages/store/tests/evaluate.spec.ts`:
- Around line 368-393: Add a regression test to assert one-to-one deep matching
for Set elements: when calling evaluate with Sets containing duplicate deep
objects (use evaluate and the existing test scope 'should test equality between
Set objects' or a new it block), include a positive case where both Sets contain
two identical deep objects and expect true, and a negative case where one Set
has two identical deep objects but the other Set only has one matching deep
object (or a different second object) and expect false; this ensures evaluate
consumes matched elements one-to-one during deep Set comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47ccc531-cdad-44c8-bed9-562167f2a5d7
📒 Files selected for processing (2)
packages/store/src/evaluate.tspackages/store/tests/evaluate.spec.ts
| export function evaluate<T>( | ||
| objA: T, | ||
| objB: T, | ||
| config: { mode: 'shallow' | 'deep' } = { mode: 'shallow' }, |
There was a problem hiding this comment.
This default value is sort of interesting. What is the reasoning behind it?
Author
There was a problem hiding this comment.
To be honest it could be either or... I just defaulted to shallow as that's the use case in store
There was a problem hiding this comment.
Do we want it to give a default? Internally, it'll be convenient to know from a glance, and as a user you can always add a wrapper:
export function shallow<T>(objA: T, objB: T): boolean => {
return evaluate(objA, objB, { mode: 'shallow' })
}
Author
There was a problem hiding this comment.
If it's to be wrapped, then I would go with two functions. But I'm happy providing the config to select which mode.
Whoever is the reviewer is can be the tie-breaker 😆
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
evaluate()utility function for flexible value comparison with configurable shallow and deep comparison modes.Tests
evaluate()covering primitives, objects, dates, maps, sets, and circular reference scenarios.