diff --git a/src/main.ts b/src/main.ts index a0ee2609..9a396fba 100644 --- a/src/main.ts +++ b/src/main.ts @@ -53,7 +53,7 @@ export default class QuickAdd extends Plugin { QuickAdd.instance = this; await this.loadSettings(); - settingsStore.setState(this.settings); + settingsStore.replaceState(this.settings); this.unsubscribeSettingsStore = settingsStore.subscribe((settings) => { this.settings = settings; void this.saveSettings(); diff --git a/src/migrations/migrate.test.ts b/src/migrations/migrate.test.ts index c7b3aada..56e6f890 100644 --- a/src/migrations/migrate.test.ts +++ b/src/migrations/migrate.test.ts @@ -1,4 +1,7 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { DEFAULT_SETTINGS } from "src/settings"; +import { settingsStore } from "src/settingsStore"; +import migrate from "./migrate"; // Mock the logger to avoid test output noise vi.mock("src/logger/logManager", () => ({ @@ -11,8 +14,13 @@ vi.mock("src/logger/logManager", () => ({ describe("Migration Re-entrance Safety", () => { let mockPlugin: any; let mockSettings: any; + let unsubscribe: (() => void) | undefined; beforeEach(() => { + settingsStore.replaceState(structuredClone(DEFAULT_SETTINGS)); + unsubscribe?.(); + unsubscribe = undefined; + // Reset settings with minimal structure needed for migration tests mockSettings = { choices: [], @@ -25,6 +33,12 @@ describe("Migration Re-entrance Safety", () => { }; }); + afterEach(() => { + unsubscribe?.(); + unsubscribe = undefined; + settingsStore.replaceState(structuredClone(DEFAULT_SETTINGS)); + }); + describe("Migration safety patterns", () => { it("should verify migrations are tracked correctly", async () => { // Create a simple mock migration function @@ -142,6 +156,130 @@ describe("Migration Re-entrance Safety", () => { }); describe("Specific migration interaction patterns", () => { + it("does not let a later store-backed migration restore stale choices", async () => { + const legacyTemplateChoice = { + id: "daily-note", + name: "Open Daily Note", + type: "Template", + setFileExistsBehavior: true, + fileExistsMode: "Nothing", + }; + const loadedSettings = { + ...structuredClone(DEFAULT_SETTINGS), + choices: [legacyTemplateChoice], + ai: { + ...structuredClone(DEFAULT_SETTINGS.ai), + providers: [ + { + name: "Provider without model source", + type: "openai", + apiKey: "", + defaultModel: "", + models: [], + }, + ], + }, + migrations: { + useQuickAddTemplateFolder: true, + incrementFileNameSettingMoveToDefaultBehavior: true, + consolidateFileExistsBehavior: false, + repairTemplateFileExistsBehavior: true, + mutualExclusionInsertAfterAndWriteToBottomOfFile: true, + setVersionAfterUpdateModalRelease: true, + addDefaultAIProviders: true, + removeMacroIndirection: true, + migrateFileOpeningSettings: true, + backfillFileOpeningDefaults: true, + setProviderModelDiscoveryMode: false, + migrateProviderApiKeysToSecretStorage: true, + }, + }; + + settingsStore.replaceState(loadedSettings as any); + mockPlugin = { + manifest: { version: "2.12.2" }, + settings: structuredClone(loadedSettings), + saveSettings: vi.fn(), + }; + unsubscribe = settingsStore.subscribe((settings) => { + mockPlugin.settings = settings; + void mockPlugin.saveSettings(); + }); + + await migrate(mockPlugin); + + expect(mockPlugin.settings.choices[0]).toMatchObject({ + fileExistsBehavior: { kind: "apply", mode: "doNothing" }, + }); + expect(mockPlugin.settings.choices[0]).not.toHaveProperty( + "setFileExistsBehavior", + ); + expect(mockPlugin.settings.choices[0]).not.toHaveProperty( + "fileExistsMode", + ); + expect( + mockPlugin.settings.migrations.consolidateFileExistsBehavior, + ).toBe(true); + expect( + mockPlugin.settings.migrations.setProviderModelDiscoveryMode, + ).toBe(true); + }); + + it("repairs stale legacy choices when the earlier consolidation is marked complete", async () => { + const loadedSettings = { + ...structuredClone(DEFAULT_SETTINGS), + choices: [ + { + id: "daily-note", + name: "Open Daily Note", + type: "Template", + setFileExistsBehavior: true, + fileExistsMode: "Nothing", + }, + ], + migrations: { + useQuickAddTemplateFolder: true, + incrementFileNameSettingMoveToDefaultBehavior: true, + consolidateFileExistsBehavior: true, + repairTemplateFileExistsBehavior: false, + mutualExclusionInsertAfterAndWriteToBottomOfFile: true, + setVersionAfterUpdateModalRelease: true, + addDefaultAIProviders: true, + removeMacroIndirection: true, + migrateFileOpeningSettings: true, + backfillFileOpeningDefaults: true, + setProviderModelDiscoveryMode: true, + migrateProviderApiKeysToSecretStorage: true, + }, + }; + + settingsStore.replaceState(loadedSettings as any); + mockPlugin = { + manifest: { version: "2.12.2" }, + settings: structuredClone(loadedSettings), + saveSettings: vi.fn(), + }; + unsubscribe = settingsStore.subscribe((settings) => { + mockPlugin.settings = settings; + void mockPlugin.saveSettings(); + }); + + await migrate(mockPlugin); + + expect(mockPlugin.settings.choices[0]).toMatchObject({ + fileExistsBehavior: { kind: "apply", mode: "doNothing" }, + }); + expect(mockPlugin.settings.choices[0]).not.toHaveProperty( + "setFileExistsBehavior", + ); + expect(mockPlugin.settings.choices[0]).not.toHaveProperty( + "fileExistsMode", + ); + expect( + mockPlugin.settings.migrations.repairTemplateFileExistsBehavior, + ).toBe(true); + }); + it("should handle macro-related migration sequence", async () => { // This test verifies the specific pattern we're concerned about: // One migration embeds macros, another removes macro references diff --git a/src/migrations/migrate.ts b/src/migrations/migrate.ts index b0d4ae30..d31a39eb 100644 --- a/src/migrations/migrate.ts +++ b/src/migrations/migrate.ts @@ -4,6 +4,7 @@ import type { Migrations } from "./Migrations"; import useQuickAddTemplateFolder from "./useQuickAddTemplateFolder"; import incrementFileNameSettingMoveToDefaultBehavior from "./incrementFileNameSettingMoveToDefaultBehavior"; import consolidateFileExistsBehavior from "./consolidateFileExistsBehavior"; +import repairTemplateFileExistsBehavior from "./repairTemplateFileExistsBehavior"; import mutualExclusionInsertAfterAndWriteToBottomOfFile from "./mutualExclusionInsertAfterAndWriteToBottomOfFile"; import setVersionAfterUpdateModalRelease from "./setVersionAfterUpdateModalRelease"; import addDefaultAIProviders from "./addDefaultAIProviders"; @@ -13,11 +14,13 @@ import backfillFileOpeningDefaults from "./backfillFileOpeningDefaults"; import setProviderModelDiscoveryMode from "./setProviderModelDiscoveryMode"; import { deepClone } from "src/utils/deepClone"; import migrateProviderApiKeysToSecretStorage from "./migrateProviderApiKeysToSecretStorage"; +import { settingsStore } from "src/settingsStore"; const migrations: Migrations = { useQuickAddTemplateFolder, incrementFileNameSettingMoveToDefaultBehavior, consolidateFileExistsBehavior, + repairTemplateFileExistsBehavior, mutualExclusionInsertAfterAndWriteToBottomOfFile, setVersionAfterUpdateModalRelease, addDefaultAIProviders, @@ -39,6 +42,8 @@ async function migrate(plugin: QuickAdd): Promise { return; } + settingsStore.replaceState(deepClone(plugin.settings)); + // Could batch-run with Promise.all, but we want to log each migration as it runs. for (const migration of migrationsToRun as (keyof Migrations)[]) { log.logMessage( @@ -46,11 +51,17 @@ async function migrate(plugin: QuickAdd): Promise { ); const backup = deepClone(plugin.settings); + const storeBeforeMigration = settingsStore.getState(); try { await migrations[migration].migrate(plugin); + if (settingsStore.getState() !== storeBeforeMigration) { + plugin.settings = deepClone(settingsStore.getState()); + } + plugin.settings.migrations[migration] = true; + settingsStore.replaceState(deepClone(plugin.settings)); log.logMessage(`Migration ${migration} successful.`); } catch (error) { @@ -59,6 +70,7 @@ async function migrate(plugin: QuickAdd): Promise { ); plugin.settings = backup; + settingsStore.replaceState(deepClone(plugin.settings)); } } diff --git a/src/migrations/repairTemplateFileExistsBehavior.ts b/src/migrations/repairTemplateFileExistsBehavior.ts new file mode 100644 index 00000000..b618a311 --- /dev/null +++ b/src/migrations/repairTemplateFileExistsBehavior.ts @@ -0,0 +1,10 @@ +import consolidateFileExistsBehavior from "./consolidateFileExistsBehavior"; +import type { Migration } from "./Migrations"; + +const repairTemplateFileExistsBehavior: Migration = { + description: + "Repair template file collision settings that may have been left in legacy format", + migrate: consolidateFileExistsBehavior.migrate, +}; + +export default repairTemplateFileExistsBehavior; diff --git a/src/settings.ts b/src/settings.ts index 241d093e..d07354a8 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -43,6 +43,7 @@ export interface QuickAddSettings { useQuickAddTemplateFolder: boolean; incrementFileNameSettingMoveToDefaultBehavior: boolean; consolidateFileExistsBehavior: boolean; + repairTemplateFileExistsBehavior: boolean; mutualExclusionInsertAfterAndWriteToBottomOfFile: boolean; setVersionAfterUpdateModalRelease: boolean; addDefaultAIProviders: boolean; @@ -86,6 +87,7 @@ export const DEFAULT_SETTINGS: QuickAddSettings = { useQuickAddTemplateFolder: false, incrementFileNameSettingMoveToDefaultBehavior: false, consolidateFileExistsBehavior: false, + repairTemplateFileExistsBehavior: false, mutualExclusionInsertAfterAndWriteToBottomOfFile: false, setVersionAfterUpdateModalRelease: false, addDefaultAIProviders: false, diff --git a/src/settingsStore.ts b/src/settingsStore.ts index 67b86ef5..cabd671c 100644 --- a/src/settingsStore.ts +++ b/src/settingsStore.ts @@ -14,6 +14,7 @@ export const settingsStore = (() => { return { getState, + replaceState: (state: SettingsState) => setState(state, true), setState, subscribe, diff --git a/tests/e2e/file-exists-behavior.test.ts b/tests/e2e/file-exists-behavior.test.ts index f8480f4f..6518a6d3 100644 --- a/tests/e2e/file-exists-behavior.test.ts +++ b/tests/e2e/file-exists-behavior.test.ts @@ -364,3 +364,45 @@ describe("migration: consolidateFileExistsBehavior", () => { expect(data.migrations.consolidateFileExistsBehavior).toBe(true); }); }); + +describe("migration: repairTemplateFileExistsBehavior", () => { + beforeAll(async () => { + const root = sandbox.root; + const migrationChoice = (id: string) => templateChoice(id, `${root}/repair-test`); + + await qa.data().patch((data) => { + clearTestChoices(data); + + data.choices.push( + { ...migrationChoice("__qa-test-r1-stale-nothing"), setFileExistsBehavior: true, fileExistsMode: "Nothing" }, + { ...migrationChoice("__qa-test-r2-already"), fileExistsBehavior: { kind: "apply", mode: "overwrite" } }, + ); + + data.migrations.consolidateFileExistsBehavior = true; + data.migrations.repairTemplateFileExistsBehavior = false; + }); + + await qa.reload(); + await qa.waitForData( + (data) => data.migrations.repairTemplateFileExistsBehavior === true, + { timeoutMs: 10_000, intervalMs: 300 }, + ); + }, 15_000); + + async function readChoice(id: string) { + const data = await qa.data().read(); + return findChoice(data, id); + } + + it("repairs stale legacy do-nothing behavior", async () => { + const choice = await readChoice("__qa-test-r1-stale-nothing"); + expect(choice?.fileExistsBehavior).toEqual(behavior.doNothing); + expect(choice).not.toHaveProperty("setFileExistsBehavior"); + expect(choice).not.toHaveProperty("fileExistsMode"); + }); + + it("preserves already-migrated behavior", async () => { + const choice = await readChoice("__qa-test-r2-already"); + expect(choice?.fileExistsBehavior).toEqual(behavior.overwrite); + }); +});