refactor: add deepCloneFrontmatterField to prevent input mutation #30
Conversation
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
- Add deepCloneFrontmatterField function for creating safe copies of FrontmatterField - Refactor FrontmatterEditorModal to use local state instead of mutating props - Add comprehensive unit tests for the deep clone function This prevents unintended side effects when editing frontmatter settings in modals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the FrontmatterEditorModal to prevent unintended mutation of props by introducing a deepCloneFrontmatterField utility function. The modal now works with a local copy of the frontmatter field that is only saved when the user explicitly clicks the Save button, improving data integrity and user experience.
Key Changes:
- Added
deepCloneFrontmatterFieldfunction to create safe copies ofFrontmatterFieldobjects - Refactored modal to use
localStateinstead of directly mutatingprops.frontmatterSetting - Added comprehensive unit tests to verify deep cloning behavior and mutation prevention
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/frontmatter/index.ts |
Adds the deepCloneFrontmatterField function that creates deep copies of FrontmatterField objects by cloning nested count objects and refs arrays |
src/ui/modals/FrontmatterEditorModal.ts |
Introduces localState field initialized with a deep clone in the constructor, and consistently replaces all this.props.frontmatterSetting references with this.localState throughout the modal |
__tests__/frontmatter/deepClone.test.ts |
Adds new test suite covering value equality, reference inequality, nested object/array cloning, and mutation isolation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id: field.id, | ||
| name: field.name, | ||
| count: { min: field.count.min, max: field.count.max }, | ||
| refs: [...field.refs], |
There was a problem hiding this comment.
The spread operator [...field.refs] will throw a runtime error if field.refs is null or undefined. The codebase shows evidence that these fields can be undefined (e.g., line 124 in FrontmatterEditorModal uses this.localState.refs || [] and line 79 in main.ts checks !frontmatter.refs || frontmatter.refs.length === 0). Add null/undefined handling to prevent crashes.
Suggested change
| refs: [...field.refs], | |
| refs: [...(field.refs ?? [])], |
Comment on lines
+1
to
+101
| import { deepCloneFrontmatterField } from '../../src/frontmatter'; | ||
| import type { FrontmatterField } from '../../src/frontmatter/types'; | ||
|
|
||
| describe('deepCloneFrontmatterField', () => { | ||
| const createTestField = (): FrontmatterField => ({ | ||
| id: 123, | ||
| name: 'test-field', | ||
| count: { min: 1, max: 5 }, | ||
| refs: ['ref1', 'ref2'], | ||
| overwrite: true, | ||
| linkType: 'WikiLink', | ||
| customQuery: 'custom query', | ||
| }); | ||
|
|
||
| it('should create a copy with identical values', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned).toEqual(original); | ||
| }); | ||
|
|
||
| it('should not be the same reference as original', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned).not.toBe(original); | ||
| }); | ||
|
|
||
| it('should create a new count object', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned.count).not.toBe(original.count); | ||
| expect(cloned.count).toEqual(original.count); | ||
| }); | ||
|
|
||
| it('should create a new refs array', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned.refs).not.toBe(original.refs); | ||
| expect(cloned.refs).toEqual(original.refs); | ||
| }); | ||
|
|
||
| it('should not mutate original when modifying cloned count', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| cloned.count.min = 10; | ||
| cloned.count.max = 20; | ||
|
|
||
| expect(original.count.min).toBe(1); | ||
| expect(original.count.max).toBe(5); | ||
| }); | ||
|
|
||
| it('should not mutate original when modifying cloned refs', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| cloned.refs.push('new-ref'); | ||
| cloned.refs[0] = 'modified'; | ||
|
|
||
| expect(original.refs).toEqual(['ref1', 'ref2']); | ||
| }); | ||
|
|
||
| it('should not mutate original when modifying cloned primitive fields', () => { | ||
| const original = createTestField(); | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| cloned.name = 'modified-name'; | ||
| cloned.overwrite = false; | ||
| cloned.linkType = 'Normal'; | ||
| cloned.customQuery = 'modified query'; | ||
|
|
||
| expect(original.name).toBe('test-field'); | ||
| expect(original.overwrite).toBe(true); | ||
| expect(original.linkType).toBe('WikiLink'); | ||
| expect(original.customQuery).toBe('custom query'); | ||
| }); | ||
|
|
||
| it('should handle empty refs array', () => { | ||
| const original: FrontmatterField = { | ||
| ...createTestField(), | ||
| refs: [], | ||
| }; | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned.refs).toEqual([]); | ||
| expect(cloned.refs).not.toBe(original.refs); | ||
| }); | ||
|
|
||
| it('should handle empty customQuery', () => { | ||
| const original: FrontmatterField = { | ||
| ...createTestField(), | ||
| customQuery: '', | ||
| }; | ||
| const cloned = deepCloneFrontmatterField(original); | ||
|
|
||
| expect(cloned.customQuery).toBe(''); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite should include cases for null or undefined refs arrays, since the runtime code in FrontmatterEditorModal (line 124) and main.ts (line 79) shows these fields can be undefined. Add test cases to verify the deepClone function handles these edge cases gracefully.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
deepCloneFrontmatterFieldfunction for creating safe copies ofFrontmatterFieldFrontmatterEditorModalto use local state instead of mutating props directlyChanges
This PR prevents unintended side effects when editing frontmatter settings in modals by:
Test plan
deepCloneFrontmatterField🤖 Generated with Claude Code