Feat/store history undo redo - #3038
palak170306-design wants to merge 5 commits into
Conversation
github-actions
Bot
added
type:docs
📝 WalkthroughWalkthroughThe store adds optional undo/redo history with batching, coalescing, limits, and reset support. Snapshot history now strips nested functions during cloning, preserves special values, uses defensive copies, and updates cursors after successful state changes. Tests cover both behaviors. ChangesStore history
Snapshot history
Contributor record
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Store
participant HistoryStacks
Caller->>Store: setState update
Store->>HistoryStacks: record previous state
Caller->>Store: undo()
Store->>HistoryStacks: move state from past to future
Store-->>Caller: notify restored state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 8
🧹 Nitpick comments (2)
packages/store/src/store.ts (2)
683-689: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
getHistory()exposes live state objects by reference; consider defensive cloning likesnapshot-history.ts.
past/futureentries are the actual application state objects, not clones. SincesetStatemerges via spread (unchanged nested properties keep the same reference across entries and the live state), a consumer mutating a returned history entry could corrupt other history entries or the current live state.snapshot-history.ts'screateSnapshotHistorydeep-clones state on capture, restore, andlist()for exactly this reason; this file already importssafeDeepClone(used at Lines 565/568/572) and could apply the same pattern at thegetHistory()boundary.♻️ Proposed fix
- const getHistory = (): { past: T[]; future: T[] } => ({ past: [...past], future: [...future] }); + const getHistory = (): { past: T[]; future: T[] } => ({ + past: past.map(s => safeDeepClone(s) as T), + future: future.map(s => safeDeepClone(s) as T), + });🤖 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/store.ts` around lines 683 - 689, Update getHistory to defensively deep-clone both past and future entries before returning them, using the existing safeDeepClone utility consistent with snapshot-history.ts. Keep resetHistory unchanged and ensure callers cannot mutate stored history or live state through the returned snapshot.
665-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-null assertions lack the required inline justification comment.
past.pop()!andfuture.pop()!are safe here because of the preceding length checks, but the coding guideline requires an inline comment explaining why a type assertion is safe.As per coding guidelines: "No type assertions without an inline comment explaining why."
✏️ Proposed fix
- const target = past.pop()!; + const target = past.pop()!; // safe: past.length === 0 already returned above- const target = future.pop()!; + const target = future.pop()!; // safe: future.length === 0 already returned aboveAlso applies to: 677-677
🤖 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/store.ts` at line 665, Update the pop operations in the surrounding undo/redo logic to add inline comments explaining that the non-null assertions are safe because preceding length checks guarantee an available element. Apply the same justification to both past.pop()! and future.pop()! without changing the control flow.Source: Coding guidelines
🤖 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.
Inline comments:
In `@packages/store/src/history-option.test.ts`:
- Around line 49-65: Move the timer cleanup for the coalesceMs test out of its
body and into an afterEach hook that calls vi.useRealTimers(), ensuring cleanup
runs even when an assertion fails. Remove the inline vi.useRealTimers() call
while preserving the existing fake-timer setup and timestamp-based assertions.
In `@packages/store/src/snapshot-history.test.ts`:
- Around line 74-76: Update the restore failure test around history.restore and
undo to use a valid snapshot ID whose setState application is forced to throw,
rather than an unknown ID that fails before application. Then assert undo still
targets the expected prior snapshot and preserves the cursor position after the
failed restore.
- Line 113: In the snapshot mutation test, replace the unannotated any assertion
on snap.state.nested.count with direct property access, preserving the
assignment of 999 without introducing another type assertion.
In `@packages/store/src/snapshot-history.ts`:
- Around line 27-45: Update the strip function to detect function values before
Date, RegExp, Array, Map, or Set handling, and consistently omit or replace them
across object properties, array elements, Set entries, and Map keys/values so
structuredClone never receives a function.
- Around line 80-82: Validate the history limit before the capture logic in the
snapshot history implementation, rejecting negative values so the `while
(snapshots.length > limit)` loop cannot run indefinitely. Preserve the existing
behavior for zero and positive limits, and anchor the change to the `capture()`
method and its `limit` handling.
- Around line 26-52: Introduce a recursive data-only snapshot state type that
represents the result of cloneState without action/function fields, and use it
for StoreSnapshot<T>.state across capture(), restore(), and list(). Update
cloneState and related return types so snapshots consistently expose this type
rather than claiming they are T, and remove existing type assertions unless
their purpose is documented immediately beside each cast.
In `@packages/store/src/store.ts`:
- Around line 659-669: Reset lastRecordAt to 0 in both undo() and redo() after
applying the history state, matching resetHistory(), so subsequent edits cannot
coalesce across an undo/redo boundary.
- Line 661: Update the error messages in the undo-related paths at lines 661 and
673 to reference the actual exported API name, createStore(), instead of
CreativeStore() or CreateStore().
---
Nitpick comments:
In `@packages/store/src/store.ts`:
- Around line 683-689: Update getHistory to defensively deep-clone both past and
future entries before returning them, using the existing safeDeepClone utility
consistent with snapshot-history.ts. Keep resetHistory unchanged and ensure
callers cannot mutate stored history or live state through the returned
snapshot.
- Line 665: Update the pop operations in the surrounding undo/redo logic to add
inline comments explaining that the non-null assertions are safe because
preceding length checks guarantee an available element. Apply the same
justification to both past.pop()! and future.pop()! without changing the control
flow.
🪄 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 Plus
Run ID: e3c4b64c-725a-4309-a6b2-01623559d159
📒 Files selected for processing (5)
CONTRIBUTORS.mdpackages/store/src/history-option.test.tspackages/store/src/snapshot-history.test.tspackages/store/src/snapshot-history.tspackages/store/src/store.ts
| it('coalesceMs merges updates that occur within the window (timestamp-based, not timer-based)', () => { | ||
| vi.useFakeTimers(); | ||
| vi.setSystemTime(0); | ||
| const store = createStore(() => ({ n: 0 }), { history: { coalesceMs: 300 } }); | ||
|
|
||
| store.setState({ n: 1 }); | ||
| vi.setSystemTime(100); // still inside the 300ms window | ||
| store.setState({ n: 2 }); | ||
|
|
||
| expect(store.getHistory().past.length).toBe(1); | ||
|
|
||
| vi.setSystemTime(500); // window has elapsed | ||
| store.setState({ n: 3 }); | ||
| expect(store.getHistory().past.length).toBe(2); | ||
|
|
||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore real timers via afterEach, not inline at test end.
vi.useRealTimers() is only called at the end of the test body (Line 64). If an assertion (Lines 58 or 62) throws first, fake timers leak into subsequent tests.
As per coding guidelines: "Use vi.useFakeTimers() for debounce or persist tests and vi.useRealTimers() in afterEach hooks; real timers should be off by default."
🐛 Proposed fix
-import { describe, expect, it, vi } from 'vitest';
+import { afterEach, describe, expect, it, vi } from 'vitest';
import { createStore, batch } from './store';
describe('createStore history option', () => {
+ afterEach(() => {
+ vi.useRealTimers();
+ });
+
it('undo()/redo() round-trip a plain setState', () => { vi.setSystemTime(500); // window has elapsed
store.setState({ n: 3 });
expect(store.getHistory().past.length).toBe(2);
-
- vi.useRealTimers();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('coalesceMs merges updates that occur within the window (timestamp-based, not timer-based)', () => { | |
| vi.useFakeTimers(); | |
| vi.setSystemTime(0); | |
| const store = createStore(() => ({ n: 0 }), { history: { coalesceMs: 300 } }); | |
| store.setState({ n: 1 }); | |
| vi.setSystemTime(100); // still inside the 300ms window | |
| store.setState({ n: 2 }); | |
| expect(store.getHistory().past.length).toBe(1); | |
| vi.setSystemTime(500); // window has elapsed | |
| store.setState({ n: 3 }); | |
| expect(store.getHistory().past.length).toBe(2); | |
| vi.useRealTimers(); | |
| }); | |
| import { afterEach, describe, expect, it, vi } from 'vitest'; | |
| import { createStore, batch } from './store'; | |
| describe('createStore history option', () => { | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| }); | |
| it('coalesceMs merges updates that occur within the window (timestamp-based, not timer-based)', () => { | |
| vi.useFakeTimers(); | |
| vi.setSystemTime(0); | |
| const store = createStore(() => ({ n: 0 }), { history: { coalesceMs: 300 } }); | |
| store.setState({ n: 1 }); | |
| vi.setSystemTime(100); // still inside the 300ms window | |
| store.setState({ n: 2 }); | |
| expect(store.getHistory().past.length).toBe(1); | |
| vi.setSystemTime(500); // window has elapsed | |
| store.setState({ n: 3 }); | |
| expect(store.getHistory().past.length).toBe(2); | |
| }); | |
| }); |
🤖 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-option.test.ts` around lines 49 - 65, Move the
timer cleanup for the coalesceMs test out of its body and into an afterEach hook
that calls vi.useRealTimers(), ensuring cleanup runs even when an assertion
fails. Remove the inline vi.useRealTimers() call while preserving the existing
fake-timer setup and timestamp-based assertions.
Source: Coding guidelines
| expect(() => history.restore(9999)).toThrow(/Unknown store snapshot/); | ||
| // cursor should be exactly where it was before the failed restore | ||
| expect(history.undo()?.state.count).toBe(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exercise the state-application failure path.
An unknown ID throws before apply() and would pass even if the cursor were advanced before setState(). Make setState() throw for a valid snapshot, then assert undo() still targets the expected prior snapshot.
🤖 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/snapshot-history.test.ts` around lines 74 - 76, Update the
restore failure test around history.restore and undo to use a valid snapshot ID
whose setState application is forced to throw, rather than an unknown ID that
fails before application. Then assert undo still targets the expected prior
snapshot and preserves the cursor position after the failed restore.
| const history = createSnapshotHistory(store); | ||
| const snap = history.capture('a'); | ||
|
|
||
| (snap.state.nested as any).count = 999; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --lang ts --pattern '$EXPR as any' packages/store/src/snapshot-history.test.tsRepository: Karanjot786/TermUI
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '90,125p' packages/store/src/snapshot-history.test.ts
echo
echo "== AST outline around test file =="
ast-grep outline packages/store/src/snapshot-history.test.ts || true
echo
echo "== package/type settings snippets =="
for f in tsconfig.json tsconfig.base.json packages/store/tsconfig.json packages/store/src/vitest.config.ts vitest.config.ts pnpm-lock.yaml package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,140p' "$f"
fi
done
echo
echo "== snapshot-history source candidates =="
fd -a 'snapshot-history' packages/store/src | while read -r f; do
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" || true
sed -n '1,220p' "$f"
doneRepository: Karanjot786/TermUI
Length of output: 13162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,85p' packages/store/src/snapshot-history.tsRepository: Karanjot786/TermUI
Length of output: 3026
Remove the unannotated any assertion.
snap.state.nested.count is inferred as mutable here, so the assertion is unnecessary. As per coding guidelines, use no any and no type assertions without an inline reason.
Proposed fix
- (snap.state.nested as any).count = 999;
+ snap.state.nested.count = 999;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (snap.state.nested as any).count = 999; | |
| snap.state.nested.count = 999; |
🤖 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/snapshot-history.test.ts` at line 113, In the snapshot
mutation test, replace the unannotated any assertion on snap.state.nested.count
with direct property access, preserving the assignment of 999 without
introducing another type assertion.
Source: Coding guidelines
| function cloneState<T>(state: T): T { | ||
| const strip = (value: unknown): unknown => { | ||
| if (value === null || typeof value !== 'object') return value; | ||
| if (value instanceof Date || value instanceof RegExp) return value; | ||
| if (Array.isArray(value)) return value.map(strip); | ||
| if (value instanceof Map) { | ||
| const m = new Map(); | ||
| for (const [k, v] of value) m.set(k, strip(v)); | ||
| return m; | ||
| } | ||
| if (value instanceof Set) { | ||
| const s = new Set(); | ||
| for (const v of value) s.add(strip(v)); | ||
| return s; | ||
| } | ||
| const out: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(value as Record<string, unknown>)) { | ||
| if (typeof v === 'function') continue; | ||
| out[k] = strip(v); | ||
| } | ||
| return out; | ||
| }; | ||
|
|
||
| const stripped = strip(state); | ||
| return typeof structuredClone === 'function' | ||
| ? structuredClone(stripped as T) | ||
| : (JSON.parse(JSON.stringify(stripped)) as T); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --lang ts --pattern '$EXPR as $TYPE' packages/store/src/snapshot-history.tsRepository: Karanjot786/TermUI
Length of output: 663
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File size and relevant contents:"
wc -l packages/store/src/snapshot-history.ts
cat -n packages/store/src/snapshot-history.ts
echo
echo "Type assertion occurrences with inline comments:"
python3 - <<'PY'
from pathlib import Path
p=Path("packages/store/src/snapshot-history.ts")
text=p.read_text()
for i,line in enumerate(text.splitlines(),1):
if " as " in line:
print(f"{i}: {line}")
PYRepository: Karanjot786/TermUI
Length of output: 5473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Store.ts outline:"
ast-grep outline packages/store/src/store.ts --view expanded || true
echo
echo "Relevant Store<T> contents:"
wc -l packages/store/src/store.ts
sed -n '1,180p' packages/store/src/store.ts | cat -n
echo
echo "Behavioral probe for cloneState stripping and clone behavior:"
node - <<'JS'
function probeClone(fn) {
const original = {
label: 'x',
value: 1,
action: () => 'action',
nested: { fnField: () => 'nested', list: [{ fnField: () => 'nested item' }, 1], map: new Map([['fnField', () => 'map fn']]), set: new Set([() => 'set fn', 1]) },
};
const stripped = fn(original.state || original ?? original);
return {
originalKeys: Object.keys(original),
returnedKeys: Object.keys(stripped),
topActionAbsent: typeof stripped.action === 'function' ? 'present' : 'absent',
nestedFnAbsent: typeof stripped.nested?.fnField === 'function' ? 'present' : 'absent',
listFnCount: (stripped.nested?.list ?? []).filter(i => typeof i === 'function').length,
mapEntries: [...(stripped.nested?.map ?? [])].filter(([k, v]) => typeof k === 'string' && k.endsWith('Field')).length,
};
}
const hasStructuredClone = typeof globalThis.structuredClone === 'function';
console.log(JSON.stringify({
hasStructuredClone,
stripJson: probeClone(s => (s && typeof s === 'object' && !Array.isArray(s) && !(s instanceof Date || s instanceof RegExp || s instanceof Map || s instanceof Set))
? JSON.parse(JSON.stringify(s)) : s),
stripObjectEntries: probeClone(s => (() => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return out;
})()),
cloneResult: hasStructuredClone ? probeClone((s) => globalThis.structuredClone(s)) : null,
}));
JS
echo
echo "Check Store<T> action typing/usages:"
rg -n "Store<[^>]+>.*:|Store\\(" packages --glob '*.ts' --glob '*.tsx' | head -120Repository: Karanjot786/TermUI
Length of output: 9639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant Store<T> contents:"
sed -n '188,820p' packages/store/src/store.ts | cat -n
echo
echo "Behavioral probe for cloneState stripping and clone behavior:"
node - <<'JS'
function probeClone(fn) {
const original = {
label: 'x',
value: 1,
action: () => 'action',
nested: { fnField: () => 'nested', list: [{ fnField: () => 'nested item' }, 1], map: new Map([['fnField', () => 'map fn']]), set: new Set([() => 'set fn', 1]) },
};
const stripped = fn(original);
return {
originalKeys: Object.keys(original),
returnedKeys: Object.keys(stripped),
topActionAbsent: typeof stripped.action === 'function' ? 'present' : 'absent',
nestedFnAbsent: typeof stripped.nested?.fnField === 'function' ? 'present' : 'absent',
listFnCount: (stripped.nested?.list ?? []).filter(i => typeof i === 'function').length,
mapEntries: [...(stripped.nested?.map ?? [])].filter(([k, v]) => typeof k === 'string' && k.endsWith('Field')).length,
};
}
const hasStructuredClone = typeof globalThis.structuredClone === 'function';
console.log(JSON.stringify({
hasStructuredClone,
stripJson: probeClone(s => ((s && typeof s === 'object' && !Array.isArray(s) && !(s instanceof Date || s instanceof RegExp || s instanceof Map || s instanceof Set))
? JSON.parse(JSON.stringify(s)) : s)),
stripObjectEntries: probeClone(s => (() => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return out;
})()),
cloneResult: hasStructuredClone ? probeClone((s) => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return globalThis.structuredClone(out);
}) : null
}, null, 2));
JS
echo
echo "Check Store<T> action factory examples/usages:"
rg -n "createStore\\([^\\n]+=>|set\\([^\\n]+=>\\s*\\({|increment:|reset:|setState\\(" packages --glob '*.ts' --glob '*.tsx' | head -200Repository: Karanjot786/TermUI
Length of output: 27949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Behavioral probe for cloneState stripping and clone behavior:"
node - <<'JS'
function probeClone(fn, input) {
const stripped = fn(input);
const arr = stripped?.nested?.list ?? [];
const map = stripped?.nested?.map ?? new Map();
const mapEntries = [...map.entries()].filter(([k, v]) => typeof k === 'string' && k.endsWith('Field'));
return {
originalKeys: Object.keys(input),
returnedKeys: Object.keys(stripped),
topActionAbsent: typeof stripped.action === 'function' ? 'present' : 'absent',
nestedFnAbsent: typeof stripped.nested?.fnField === 'function' ? 'present' : 'absent',
listFnCount: arr.reduce((n, i) => n + (typeof i === 'function' ? 1 : 0), 0),
mapValuesWithFnKey: mapEntries.filter(([, v]) => typeof v === 'function').length,
};
}
const hasStructuredClone = typeof globalThis.structuredClone === 'function';
const input = {
label: 'x',
value: 1,
action: () => 'action',
nested: {
fnField: () => 'nested',
list: [{ fnField: () => 'nested item' }, 1],
map: new Map([['fnField', () => 'map fn']]),
set: new Set([() => 'set fn', 1]),
},
};
console.log(JSON.stringify({
hasStructuredClone,
stripJson: probeClone(s => ((s && typeof s === 'object' && !Array.isArray(s) && !(s instanceof Date || s instanceof RegExp || s instanceof Map || s instanceof Set))
? JSON.parse(JSON.stringify(s)) : s), input),
stripObjectEntries: probeClone(s => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return out;
}, input),
cloneResult: hasStructuredClone ? probeClone((s) => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return globalThis.structuredClone(out);
}, input) : null
}, null, 2));
console.log("JSON.stringify preserves custom Date RegExp:");
const obj = { date: new Date("2000-01-01T12:34:56Z"), re: /abc/gi, func: () => {} };
console.log(JSON.stringify(JSON.parse(JSON.stringify(obj))));
JSRepository: Karanjot786/TermUI
Length of output: 943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Behavioral probe for cloneState stripping and clone behavior:"
node - <<'JS'
function probeClone(fn, input) {
const stripped = fn(input);
const arr = stripped && stripped.nested && stripped.nested.list ? stripped.nested.list : [];
const map = stripped && stripped.nested && stripped.nested.map ? stripped.nested.map : new Map();
const mapEntries = Array.from(map.entries()).filter(([k, v]) => typeof k === 'string' && k.endsWith('Field'));
return {
originalKeys: Object.keys(input),
returnedKeys: stripped ? Object.keys(stripped) : [],
topActionAbsent: typeof (stripped && stripped.action) === 'function' ? 'present' : 'absent',
nestedFnAbsent: typeof (stripped && stripped.nested && stripped.nested.fnField) === 'function' ? 'present' : 'absent',
listFnCount: arr.reduce((n, i) => n + (typeof i === 'function' ? 1 : 0), 0),
mapValuesWithFnKey: mapEntries.filter(([, v]) => typeof v === 'function').length,
};
}
const hasStructuredClone = typeof globalThis.structuredClone === 'function';
const input = {
label: 'x',
value: 1,
action: () => 'action',
nested: {
fnField: () => 'nested',
list: [{ fnField: () => 'nested item' }, 1],
map: new Map([['fnField', () => 'map fn']]),
set: new Set([() => 'set fn', 1]),
},
};
console.log(JSON.stringify({
hasStructuredClone,
stripJson: probeClone(s => ((s && typeof s === 'object' && !Array.isArray(s) && !(s instanceof Date || s instanceof RegExp || s instanceof Map || s instanceof Set))
? JSON.parse(JSON.stringify(s)) : s), input),
strictObjectEntries: probeClone(s => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return out;
}, input),
cloneResult: hasStructuredClone ? probeClone((s) => {
const out = {};
for (const [k, v] of Object.entries(s)) {
if (typeof v === 'function') continue;
out[k] = v;
}
return globalThis.structuredClone(out);
}, input) : null
}, null, 2));
console.log("JSON.stringify preserves custom Date RegExp:");
const obj = { date: new Date("2000-01-01T12:34:56Z"), re: /abc/gi, func: () => {} };
console.log(JSON.stringify(JSON.parse(JSON.stringify(obj))));
JSRepository: Karanjot786/TermUI
Length of output: 921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
console.log(process.version, typeof globalThis.structuredClone);
const input = {
action: function action() { return 'action'; },
nested: {
fnField: function fnField() { return 'nested'; },
list: [function nestedFn() { return 'nested item'; }, 1],
set: new Set([function setFn() {}, 1]),
},
};
console.log("Object.keys(input):", Object.keys(input));
console.log("JSON.stringify(JSON.parse(input)):");
console.log(JSON.stringify(JSON.parse(JSON.stringify(input))));
const stripped = {};
for (const [k, v] of Object.entries(input)) {
if (typeof v === 'function') continue;
stripped[k] = v;
}
console.log("Object.keys(stripped):", Object.keys(stripped));
const clonedMap = new Map();
clonedMap.set('fnField', function mapFn() {});
console.log("cloneWithMapObjectKeys:", Object.keys(clonedMap));
console.log("cloneWithMapMapEntries:", [...clonedMap.entries()]);
console.log("delete function cloneWithMap:");
const clonedDeleteMap = new Map();
clonedDeleteMap.set('fnField', function mapFn() {});
setDeleteMap(clonedDeleteMap, 'fnField');
console.log([...clonedDeleteMap.entries()]);
JSRepository: Karanjot786/TermUI
Length of output: 1050
Return non-action state from snapshot history.
cloneState() removes top-level action fields and JSON encodes nested functions as null, including Map values indexed by function names, so StoreSnapshot<T>.state no longer matches T. Introduce a data-only snapshot state type for capture(), restore(), and list(), and remove these assertions unless their motivation is documented immediately beside each cast.
🤖 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/snapshot-history.ts` around lines 26 - 52, Introduce a
recursive data-only snapshot state type that represents the result of cloneState
without action/function fields, and use it for StoreSnapshot<T>.state across
capture(), restore(), and list(). Update cloneState and related return types so
snapshots consistently expose this type rather than claiming they are T, and
remove existing type assertions unless their purpose is documented immediately
beside each cast.
Source: Coding guidelines
| const strip = (value: unknown): unknown => { | ||
| if (value === null || typeof value !== 'object') return value; | ||
| if (value instanceof Date || value instanceof RegExp) return value; | ||
| if (Array.isArray(value)) return value.map(strip); | ||
| if (value instanceof Map) { | ||
| const m = new Map(); | ||
| for (const [k, v] of value) m.set(k, strip(v)); | ||
| return m; | ||
| } | ||
| if (value instanceof Set) { | ||
| const s = new Set(); | ||
| for (const v of value) s.add(strip(v)); | ||
| return s; | ||
| } | ||
| const out: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(value as Record<string, unknown>)) { | ||
| if (typeof v === 'function') continue; | ||
| out[k] = strip(v); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Strip functions before handling container types.
Functions in arrays, Sets, Map values, or Map keys currently reach structuredClone() unchanged and cause capture to throw. Handle function values first, then omit or replace them consistently in every container type.
🤖 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/snapshot-history.ts` around lines 27 - 45, Update the
strip function to detect function values before Date, RegExp, Array, Map, or Set
handling, and consistently omit or replace them across object properties, array
elements, Set entries, and Map keys/values so structuredClone never receives a
function.
| while (snapshots.length > limit) { | ||
| snapshots.shift(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Reject negative history limits.
With { limit: -1 }, capture() shifts an already-empty array forever because 0 > -1 remains true, blocking the event loop.
Proposed fix
const limit = options.limit ?? 50;
+if (!Number.isSafeInteger(limit) || limit < 0) {
+ throw new RangeError('Snapshot history limit must be a non-negative integer');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (snapshots.length > limit) { | |
| snapshots.shift(); | |
| } | |
| if (!Number.isSafeInteger(limit) || limit < 0) { | |
| throw new RangeError('Snapshot history limit must be a non-negative integer'); | |
| } | |
| while (snapshots.length > limit) { | |
| snapshots.shift(); | |
| } |
🤖 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/snapshot-history.ts` around lines 80 - 82, Validate the
history limit before the capture logic in the snapshot history implementation,
rejecting negative values so the `while (snapshots.length > limit)` loop cannot
run indefinitely. Preserve the existing behavior for zero and positive limits,
and anchor the change to the `capture()` method and its `limit` handling.
| const undo = (): void => { | ||
| if (!historyOpts) { | ||
| throw new Error('undo() requires the "history" option to be set on CreativeStore()'); | ||
| } | ||
| if (_batchDepth > 0) throw new Error('undo() cannot be called inside batch()'); | ||
| if (past.length === 0) return; | ||
| const target = past.pop()!; | ||
| future.push(state); | ||
| if (future.length > historyLimit) future.shift(); | ||
| applyHistoryState(target); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale lastRecordAt lets coalescing swallow history entries across an undo/redo boundary.
Neither undo() nor redo() resets lastRecordAt. If a new edit happens shortly after an undo/redo, within coalesceMs of the pre-undo timestamp, the recording listener at Line 638 wrongly treats it as a continuation of the earlier coalescing burst and skips pushing the post-undo state onto past — that state is then permanently lost from the undo stack. resetHistory() already resets lastRecordAt = 0 for the same reason (Line 688); undo/redo should do the same.
🐛 Proposed fix
const target = past.pop()!;
future.push(state);
if (future.length > historyLimit) future.shift();
+ lastRecordAt = 0; // prevent a subsequent edit from merging into a now-stale coalesce window
applyHistoryState(target);
};and symmetrically in redo():
const target = future.pop()!;
past.push(state);
if (past.length > historyLimit) past.shift();
+ lastRecordAt = 0;
applyHistoryState(target);
};Also applies to: 671-681
🤖 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/store.ts` around lines 659 - 669, Reset lastRecordAt to 0
in both undo() and redo() after applying the history state, matching
resetHistory(), so subsequent edits cannot coalesce across an undo/redo
boundary.
|
|
||
| const undo = (): void => { | ||
| if (!historyOpts) { | ||
| throw new Error('undo() requires the "history" option to be set on CreativeStore()'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in error messages: wrong function name.
Line 661 says "CreativeStore()" and Line 673 says "CreateStore()" — both should read createStore(), matching the actual exported API.
✏️ Proposed fix
- throw new Error('undo() requires the "history" option to be set on CreativeStore()');
+ throw new Error('undo() requires the "history" option to be set on createStore()');- throw new Error ('redo() requires the "history" option to be set on CreateStore()');
+ throw new Error('redo() requires the "history" option to be set on createStore()');Also applies to: 673-673
🤖 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/store.ts` at line 661, Update the error messages in the
undo-related paths at lines 661 and 673 to reference the actual exported API
name, createStore(), instead of CreativeStore() or CreateStore().
|
No activity on this PR for 14 days. Rebase, resolve conflicts, or comment to keep it open. It closes in 7 days otherwise. |
|
Blocking — duplicate + does not build. Undo/redo already exists on main via createSnapshotHistory() (snapshot-history.ts #2919, on top of #2005). This bolts a second overlapping API onto createStore(); build-and-test fails and it is CONFLICTING; error messages reference a nonexistent CreateStore(). Extend snapshot-history.ts. |
|
No activity on this PR for 14 days. Rebase, resolve conflicts, or comment to keep it open. It closes in 7 days otherwise. |
|
Thanks for the contribution. There are issues to fix before this merges. Issues:
Checklist before re-requesting review:
|
2 participants
Description
Adds a
historyoption tocreateStore()with nativeundo()/redo()/getHistory()/resetHistory()on the returned store. Abatch()call collapses to exactly one history entry,coalesceMsmerges rapid consecutive updates via a timestamp-based sliding window, andundo()/redo()now throw a clear error instead of silently no-op'ing whenhistoryisn't configured on that store.Related Issue
Closes #2772
Which package(s)?
@termuijs/storeType of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/048e6a97-baaf-4a03-b6e5-bdfc6c38e173Screenshots / Recordings (UI changes)
N/A — non-visual,
@termuijs/storeAPI change only.Notes for the Reviewer
createSnapshotHistory()(snapshot-history.ts), not as a replacement. That's an explicit-checkpoint model (capture()/restore(id)); this PR'shistoryoption is automatic, tracking everysetState/batch(). Both can technically be used on the same store but would run as two independent, uncoordinated undo stacks — documented in a code comment, worth a maintainer opinion on whether that should be actively discouraged/guarded against.undo()/redo()bypasssetStateand any configured middleware, restoring the captured state object directly — intentional (avoids re-running transforms on a historical snapshot), but means middleware-enforced invariants aren't re-validated on undo/redo. Flagged in a comment; open to feedback if this tradeoff should be reconsidered.coalesceMsis timestamp-based (comparesDate.now()gaps between calls), notsetTimeout-based — there's no deferred flush, so tests drive it viavi.setSystemTime()between calls rather thanvi.advanceTimersByTime()after them.undo()/redo()now throw whenhistoryisn't set on that store, rather than silently doing nothing — this is a small API decision worth a second look, since it's a behavior change from the initial draft (which no-op'd).Summary by CodeRabbit
New Features
Bug Fixes