fix(migrations): traverse nested template choices#1152
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors the consolidateFileExistsBehavior migration to use a traversal-based normalization (walkAllChoices) and expands tests to cover nested template choices in macro commands, conditionals, legacy macros, and malformed persisted collections; tests also assert removal of obsolete file-exists fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Deploying quickadd with
|
| Latest commit: |
9ee2acf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4aa22f76.quickadd.pages.dev |
| Branch Preview URL: | https://codex-fix-file-exists-migrat.quickadd.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/migrations/consolidateFileExistsBehavior.ts (1)
15-18: Harden migration input before deep-cloning settings arrays.If persisted settings are malformed (e.g.,
choicesmissing/non-array), this can throw before traversal. Consider normalizing both collections withArray.isArray(...)before cloning.Suggested defensive patch
- plugin.settings.choices = deepClone(plugin.settings.choices); - (plugin.settings as any).macros = deepClone( - (plugin.settings as any).macros || [], - ); + const choices = Array.isArray(plugin.settings.choices) + ? plugin.settings.choices + : []; + const macros = Array.isArray((plugin.settings as any).macros) + ? (plugin.settings as any).macros + : []; + + plugin.settings.choices = deepClone(choices); + (plugin.settings as any).macros = deepClone(macros);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/consolidateFileExistsBehavior.ts` around lines 15 - 18, The migration currently deep-clones plugin.settings.choices and (plugin.settings as any).macros without validating their types, which can throw if persisted data is malformed; before calling deepClone, ensure both are normalized to arrays by replacing non-array values with empty arrays (e.g., set plugin.settings.choices = Array.isArray(plugin.settings.choices) ? deepClone(plugin.settings.choices) : []; and similarly for (plugin.settings as any).macros), preserving the existing cast to (plugin.settings as any) for macros and using deepClone for the safe copy.src/migrations/consolidateFileExistsBehavior.test.ts (1)
87-92: Optional: assert legacy field cleanup on nested template nodes too.You already assert normalized
fileExistsBehavior; addingtoBeUndefined()checks for nestedsetFileExistsBehaviorandfileExistsModewould better lock in full normalization behavior.Also applies to: 171-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/consolidateFileExistsBehavior.test.ts` around lines 87 - 92, Add assertions to ensure legacy fields are removed on nested template nodes by checking that setFileExistsBehavior and fileExistsMode are undefined wherever you already assert normalized fileExistsBehavior; for example augment the existing expectation on plugin.settings.choices[0].macro.commands[0].choice to also include toBeUndefined() checks for choice.setFileExistsBehavior and choice.fileExistsMode, and do the same for the other nested node assertions around the second test block (the nodes currently asserting normalized fileExistsBehavior) so the test verifies both normalization and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/migrations/consolidateFileExistsBehavior.test.ts`:
- Around line 87-92: Add assertions to ensure legacy fields are removed on
nested template nodes by checking that setFileExistsBehavior and fileExistsMode
are undefined wherever you already assert normalized fileExistsBehavior; for
example augment the existing expectation on
plugin.settings.choices[0].macro.commands[0].choice to also include
toBeUndefined() checks for choice.setFileExistsBehavior and
choice.fileExistsMode, and do the same for the other nested node assertions
around the second test block (the nodes currently asserting normalized
fileExistsBehavior) so the test verifies both normalization and cleanup.
In `@src/migrations/consolidateFileExistsBehavior.ts`:
- Around line 15-18: The migration currently deep-clones plugin.settings.choices
and (plugin.settings as any).macros without validating their types, which can
throw if persisted data is malformed; before calling deepClone, ensure both are
normalized to arrays by replacing non-array values with empty arrays (e.g., set
plugin.settings.choices = Array.isArray(plugin.settings.choices) ?
deepClone(plugin.settings.choices) : []; and similarly for (plugin.settings as
any).macros), preserving the existing cast to (plugin.settings as any) for
macros and using deepClone for the safe copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85a9d2fd-2121-4c80-a1c4-19ca910be176
📒 Files selected for processing (2)
src/migrations/consolidateFileExistsBehavior.test.tssrc/migrations/consolidateFileExistsBehavior.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.
Handle nested Template choices during \ by reusing the shared choice traversal helper.
The previous migration only normalized top-level Template choices, Multi children, and direct legacy macro commands. That missed Template choices embedded inside Macro choice command lists and inside Conditional then/else branches, which left legacy file-exists settings behind and degraded runtime behavior to prompt mode.
This switches the migration to use , which already traverses Multi choices, Macro choice commands, legacy macros, and Conditional branches in one place. The tests now cover nested Macro-choice templates, legacy macros, and Conditional branches.
This came from follow-up review comments on #1151 after the original PR was merged.
Summary by CodeRabbit
Tests
Refactor