fix(store): replace Object.is with structural equality in CreateHistr…#1762
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthrough
ChangesStructural equality deduplication in createHistoryStore
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/store/src/history.test.ts (1)
125-139: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding a test for the
Object.isfallback path.The default
equalsfunction falls back toObject.iswhenJSON.stringifythrows (e.g., forBigIntvalues). A test exercising this branch would improve confidence in the fallback behavior.it('falls back to Object.is when JSON.stringify throws', () => { const bigIntStore = createHistoryStore({ value: BigInt(1) }); const ref = bigIntStore.present; bigIntStore.set(ref); // same reference expect(bigIntStore.getHistory().past.length).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/store/src/history.test.ts` around lines 125 - 139, Add a new test case in the history.test.ts file to verify the Object.is fallback path in createHistoryStore. Create a test that initializes the store with a BigInt value (which causes JSON.stringify to throw), stores a reference to the present state, sets it back using the same reference, and asserts that the past history remains empty to confirm Object.is comparison is working as the fallback behavior.packages/store/src/history.ts (1)
70-76: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueNote:
JSON.stringifystripsundefinedvalues.Objects like
{ a: undefined }serialize to"{}", making them equal to{}. This is standard JSON behavior but may surprise users expecting strict structural equality. Consider noting this in the JSDoc, or users who need stricter behavior can provide a customequals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/store/src/history.ts` around lines 70 - 76, The default equals function implementation does not document the behavior that JSON.stringify strips undefined values, which can cause unexpected equality results (e.g., { a: undefined } equals {}). Add JSDoc documentation to the equals option or the containing function that clearly explains this JSON.stringify behavior and notes that users requiring stricter structural equality can provide a custom equals function to override the default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/store/src/history.test.ts`:
- Around line 125-139: Add a new test case in the history.test.ts file to verify
the Object.is fallback path in createHistoryStore. Create a test that
initializes the store with a BigInt value (which causes JSON.stringify to
throw), stores a reference to the present state, sets it back using the same
reference, and asserts that the past history remains empty to confirm Object.is
comparison is working as the fallback behavior.
In `@packages/store/src/history.ts`:
- Around line 70-76: The default equals function implementation does not
document the behavior that JSON.stringify strips undefined values, which can
cause unexpected equality results (e.g., { a: undefined } equals {}). Add JSDoc
documentation to the equals option or the containing function that clearly
explains this JSON.stringify behavior and notes that users requiring stricter
structural equality can provide a custom equals function to override the default
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f99c75-ff33-495b-a609-399e67a51446
📒 Files selected for processing (2)
packages/store/src/history.test.tspackages/store/src/history.ts
1 participant
Description
createHistoryStore.set() was using Object.is for deduplication instead of the JSON.stringify structural equality promised by the JSDoc comment. This caused duplicate history entries to be pushed for object and array states that were deeply equal but different references. This PR fixes the equality check and adds an optional equals comparator to HistoryStoreOptions for non-serialisable types.
Related Issue
Closes #1742
Which package(s)?
@termuijs/store
Type of Change
🐛 Bug fix (type:bug)
🧪 Tests (type:testing)
Checklist
⭐ You starred the repo. The needs-star check blocks your merge otherwise.
Tests pass locally: bun vitest run
Build passes: bun run build
Typecheck passes: bun run typecheck
You read CONTRIBUTING.md.
Your PR title follows type: short description.
No new any types without an inline comment explaining why.
No unrelated refactors bundled into this PR.
GSSoC 2026 Participation
You are a GSSoC 2026 contributor.
Your GSSoC profile: https://gssoc.girlscript.org/profile/048e6a97-baaf-4a03-b6e5-bdfc6c38e173
Screenshots / Recordings (UI changes)
No UI changes — pure logic fix in @termuijs/store.
Notes for the Reviewer
Root cause: set() on line 61 of history.ts used Object.is(timeline.present, newState) for its dedup guard. Object.is is reference equality — for primitive types like string this coincidentally works, which is why all existing tests passed. For object or array state, two deeply-equal values with different references always fail the check, causing a duplicate past[] entry on every set() call.
What changed:
Added HistoryStoreOptions interface with an optional equals?: (a: T, b: T) => boolean field
Added options: HistoryStoreOptions = {} parameter to createHistoryStore
Defined a const equals inside the function body that defaults to JSON.stringify structural comparison with an Object.is fallback inside a try/catch (guards against circular references or BigInt which cause JSON.stringify to throw)
Replaced Object.is(timeline.present, newState) with equals(timeline.present, newState) — the only change to existing logic
New tests added (all in history.test.ts):
Object state with same content does not push a duplicate entry
Object state with changed content does push an entry
Array state with same content does not push a duplicate entry
Custom equals function works correctly for non-serialisable types (Map)
All 8 existing tests pass unchanged since they use string state, where JSON.stringify equality is identical in behaviour to Object.is.
No breaking changes — options defaults to {} so all existing createHistoryStore(initial) call sites work without modification.
Summary by CodeRabbit
Bug Fixes
New Features
Tests