fix(migrations): preserve migrated settings state#1213
Conversation
📝 WalkthroughWalkthroughThis PR introduces a repair migration that re-runs legacy template file-exists setting normalization, with updated store API to synchronize migration execution, startup initialization using full state replacement, and comprehensive unit and end-to-end validation. ChangesTemplate File-Exists Behavior Repair Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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)
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: |
61e19a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9b9f0daf.quickadd.pages.dev |
| Branch Preview URL: | https://fix-migration-settings-store.quickadd.pages.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.ts (1)
56-56: ⚡ Quick winAvoid reference aliasing in
settingsStore.replaceState(this.settings)
settingsStore.replaceStateis implemented assetState(state, true), soreplaceState(this.settings)stores the exact same object reference in the Zustand store (no cloning). Ifthis.settingsis later mutated in-place without going throughsettingsStore.setState/replaceState, store subscribers won’t be notified.In this file the only in-place mutation found is
this.settings.version = currentVersioninannounceUpdate(), which runs aftermigrate(this). Migrations usually replace the store state withdeepClone(...), which breaks the aliasing—but ifmigrationsToRun.length === 0, the aliasing can persist and theversionupdate will be “silent”.Consider cloning on startup to make this robust:
Proposed fix
+import { deepClone } from "./utils/deepClone"; @@ - settingsStore.replaceState(this.settings); + settingsStore.replaceState(deepClone(this.settings));🤖 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/main.ts` at line 56, The call settingsStore.replaceState(this.settings) stores the same object reference, so later in-place edits like this.settings.version = currentVersion (in announceUpdate) can mutate the stored state silently; fix by cloning before replacing the store state (e.g., use a deep clone/structuredClone/JSON parse-stringify) and/or switch the in-place mutation in announceUpdate to update via settingsStore.setState/replaceState so subscribers are notified; update the code paths around settingsStore.replaceState, this.settings, and announceUpdate to perform a clone or use store setters instead.
🤖 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/main.ts`:
- Line 56: The call settingsStore.replaceState(this.settings) stores the same
object reference, so later in-place edits like this.settings.version =
currentVersion (in announceUpdate) can mutate the stored state silently; fix by
cloning before replacing the store state (e.g., use a deep
clone/structuredClone/JSON parse-stringify) and/or switch the in-place mutation
in announceUpdate to update via settingsStore.setState/replaceState so
subscribers are notified; update the code paths around
settingsStore.replaceState, this.settings, and announceUpdate to perform a clone
or use store setters instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d81bbabb-471c-4265-8a28-47426a9845c8
📒 Files selected for processing (7)
src/main.tssrc/migrations/migrate.test.tssrc/migrations/migrate.tssrc/migrations/repairTemplateFileExistsBehavior.tssrc/settings.tssrc/settingsStore.tstests/e2e/file-exists-behavior.test.ts
Preserve migration results when startup migrations mix direct
plugin.settingsupdates withsettingsStoreupdates.The file-exists behavior consolidation could mark
consolidateFileExistsBehaviorcomplete while stale store state restored legacy template choice fields. That left choices withsetFileExistsBehavior/fileExistsModebut nofileExistsBehavior, causing runtime fallback to the collision prompt.This PR keeps
settingsStoreandplugin.settingssynchronized around each migration, uses full store replacement for loaded settings, and adds a repair migration for users who already have the stale legacy shape with the earlier consolidation flag marked complete.Rollout notes:
repairTemplateFileExistsBehavior.Validation:
bun run test -- src/migrations/migrate.test.ts src/migrations/consolidateFileExistsBehavior.test.tsbun run testbun run test:e2e -- tests/e2e/file-exists-behavior.test.tsbun run test:e2ebun run lintbun run buildSummary by CodeRabbit
New Features
Bug Fixes
Tests