refactor(editor): extract history state model#544
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a centralized editor history stack module ( ChangesCentralized Undo/Redo History
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/editorHistory.test.ts (1)
119-127: ⚡ Quick winAdd a direct stack-immutability regression test.
This currently verifies the clone helper, but not that
recordEditorHistorySnapshotstores an isolated copy instack.current/past.🧪 Proposed test addition
+ it("stores cloned snapshots in history stack entries", () => { + const stack = createEditorHistoryStack(); + const snapshot = createSnapshot("first"); + + recordEditorHistorySnapshot(stack, snapshot); + snapshot.selectedZoomId = "mutated"; + + expect(stack.current?.selectedZoomId).toBe("first"); + });🤖 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 `@src/components/video-editor/editorHistory.test.ts` around lines 119 - 127, Add a regression test that ensures recordEditorHistorySnapshot stores an isolated (cloned) copy into the history stack (stack.current and/or past) rather than a reference: call createSnapshot to build an initial snapshot, call recordEditorHistorySnapshot(snapshot) (or whichever API pushes into the history stack), mutate the original snapshot afterwards (e.g., change selectedZoomId), then assert that the snapshot stored in stack.current and/or past remains unchanged and deep-equals the original unmutated snapshot; use cloneEditorHistorySnapshot and areEditorHistorySnapshotsEqual to help locate and compare the stored copy.
🤖 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 `@src/components/video-editor/editorHistory.test.ts`:
- Around line 119-127: Add a regression test that ensures
recordEditorHistorySnapshot stores an isolated (cloned) copy into the history
stack (stack.current and/or past) rather than a reference: call createSnapshot
to build an initial snapshot, call recordEditorHistorySnapshot(snapshot) (or
whichever API pushes into the history stack), mutate the original snapshot
afterwards (e.g., change selectedZoomId), then assert that the snapshot stored
in stack.current and/or past remains unchanged and deep-equals the original
unmutated snapshot; use cloneEditorHistorySnapshot and
areEditorHistorySnapshotsEqual to help locate and compare the stored copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e2980627-1827-4a51-811e-14854619d322
📒 Files selected for processing (3)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/editorHistory.test.tssrc/components/video-editor/editorHistory.ts
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
1 participant
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.
Description
Extracts the editor undo/redo history stack out of
VideoEditor.tsxinto a pure state-transition module with focused unit coverage.Motivation
This is the next small editor architecture slice. Before introducing any external state library, this PR separates the editor history model from React rendering/orchestration so the state boundary is explicit, tested, and reviewable.
Type of Change
Related Issue(s)
None.
Changes Made
src/components/video-editor/editorHistory.tswith the editor history snapshot type, stack creation/reset, record, undo, and redo transitions.src/components/video-editor/editorHistory.test.tscovering initialization, unchanged snapshots, record/redo clearing, undo/redo movement, applying-history updates, max-depth trimming, reset, and cloning behavior.VideoEditor.tsxto delegate history stack mutations to the extracted model while keeping UI state application local to the component.Scope Note
No UI behavior, timeline behavior, export behavior, keyboard shortcut mapping, persistence format, or native/runtime path is intentionally changed. This PR does not introduce Zustand, Redux, or another state dependency; it first carves out a tested pure state boundary.
Testing Guide
Review the new history model and verify that
VideoEditor.tsxstill owns applying snapshots to React state, whileeditorHistory.tsowns only stack transitions.Checklist
Local Checks
npm test -- src/components/video-editor/editorHistory.test.ts(8 tests passed)npm test -- src/components/video-editor/editorHistory.test.ts src/components/video-editor/types.test.ts(25 tests passed)npx tsc --noEmit --pretty false(passed)npx biome check --formatter-enabled=false --assist-enabled=false src/components/video-editor/VideoEditor.tsx src/components/video-editor/editorHistory.ts src/components/video-editor/editorHistory.test.ts(exit 0; existing unrelatedVideoEditor.tsxexhaustive-deps warning remains)git diff --check main...HEAD(passed)Runtime/Repro Evidence
Not run. This PR moves pure editor history stack transitions into a tested module; no Electron, preview rendering, export, or native runtime path changed.
Summary by CodeRabbit