Merge pull request #308 from bnema/fix/config-migrate-idempotent

fix(config): make migration idempotent

commit: a450a83c8a06362476507a21c959b8e1dbe79a66
author: bnema · 4mo ago · 2026-06-18 05:56 UTC
committer: GitHub · 4mo ago · 2026-06-18 05:56 UTC
merge of: 6a62fb1 + 74b2a9b · diff against the first parent
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

2 files · +69 -5

M internal/infrastructure/config/migrate.go +21 -5


@@ -195,7 +195,7 @@ func (m *Migrator) DetectChanges() ([]port.KeyChange, error) {
195	195		func (m *Migrator) findDeprecatedKeys(userKeys map[string]any, defaultKeys map[string]bool) []string {
196	196		var deprecated []string
197	197		for userKey := range userKeys {
198		-	if userKey == databasePathKey {
	198	+	if m.isMigrationIgnoredDeprecatedKey(userKey) {
199	199		continue
200	200		}
201	201		if !m.keyOrRelatedExistsInDefaults(userKey, defaultKeys) {

@@ -670,8 +670,7 @@ func (m *Migrator) getAllDefaultKeys() []string {
670	670		// Filter out keys that should not be migrated
671	671		filtered := make([]string, 0, len(keys))
672	672		for _, key := range keys {
673		-	// Skip database.path as it's set dynamically
674		-	if key == databasePathKey {
	673	+	if m.isMigrationIgnoredDeprecatedKey(key) {
675	674		continue
676	675		}
677	676		filtered = append(filtered, key)

@@ -813,9 +812,12 @@ func (m *Migrator) flattenMap(data map[string]any, prefix string, keys map[strin
813	812		// isUserDataSection checks if a key path represents user-defined data
814	813		// that should not be compared key-by-key (e.g., search shortcuts, action bindings).
815	814		func (*Migrator) isUserDataSection(keyPath string) bool {
816		-	// User data sections - treat entire section as one key
	815	+	// User data sections - treat entire section as one key.
	816	+	// Some of these sections legitimately default to an empty map/table; keeping
	817	+	// them as leaf keys makes detection agree with TOML writing.
817	818		switch keyPath {
818		-	case "search_shortcuts":
	819	+	case "search_shortcuts",
	820	+	"workspace.floating_pane.profiles":
819	821		return true
820	822		}
821	823

@@ -917,6 +919,20 @@ func appendUniqueKeys(base, extra []string) []string {
917	919		return base
918	920		}
919	921
	922	+	func (*Migrator) isMigrationIgnoredDeprecatedKey(key string) bool {
	923	+	switch key {
	924	+	case databasePathKey:
	925	+	return true
	926	+	case "appearance.external_theme.path":
	927	+	// Runtime may derive this path when it is omitted, so it is not a Viper
	928	+	// default. It remains a valid explicit config key and should not be
	929	+	// removed as deprecated.
	930	+	return true
	931	+	default:
	932	+	return false
	933	+	}
	934	+	}
	935	+
920	936		func excludeKeys(keys []string, excluded map[string]bool) []string {
921	937		if len(excluded) == 0 {
922	938		return keys

 func (m *Migrator) findDeprecatedKeys(userKeys map[string]any, defaultKeys map[string]bool) []string {
 	var deprecated []string
 	for userKey := range userKeys {
-		if userKey == databasePathKey {
+		if m.isMigrationIgnoredDeprecatedKey(userKey) {
 			continue
 		}
 		if !m.keyOrRelatedExistsInDefaults(userKey, defaultKeys) {
 	// Filter out keys that should not be migrated
 	filtered := make([]string, 0, len(keys))
 	for _, key := range keys {
-		// Skip database.path as it's set dynamically
+		if m.isMigrationIgnoredDeprecatedKey(key) {
-		if key == databasePathKey {
 			continue
 		}
 		filtered = append(filtered, key)
 // isUserDataSection checks if a key path represents user-defined data
 // that should not be compared key-by-key (e.g., search shortcuts, action bindings).
 func (*Migrator) isUserDataSection(keyPath string) bool {
-	// User data sections - treat entire section as one key
+	// User data sections - treat entire section as one key.
+	// Some of these sections legitimately default to an empty map/table; keeping
+	// them as leaf keys makes detection agree with TOML writing.
 	switch keyPath {
-	case "search_shortcuts":
+	case "search_shortcuts",
+		"workspace.floating_pane.profiles":
 		return true
 	}
 	return base
 }
+func (*Migrator) isMigrationIgnoredDeprecatedKey(key string) bool {
+	switch key {
+	case databasePathKey:
+		return true
+	case "appearance.external_theme.path":
+		// Runtime may derive this path when it is omitted, so it is not a Viper
+		// default. It remains a valid explicit config key and should not be
+		// removed as deprecated.
+		return true
+	default:
+		return false
+	}
+}
 func excludeKeys(keys []string, excluded map[string]bool) []string {
 	if len(excluded) == 0 {
 		return keys

M internal/infrastructure/config/migrate_test.go +48 -0


@@ -974,6 +974,54 @@ func TestMigrator_DeleteNestedKey(t *testing.T) {
974	974		})
975	975		}
976	976
	977	+	func TestMigrator_Migrate_IsIdempotentForExternalThemePathAndEmptyProfiles(t *testing.T) {
	978	+	home := t.TempDir()
	979	+	t.Setenv("HOME", home)
	980	+	t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
	981	+	t.Setenv("XDG_DATA_HOME", filepath.Join(home, ".local", "share"))
	982	+	t.Setenv("XDG_STATE_HOME", filepath.Join(home, ".local", "state"))
	983	+	t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache"))
	984	+
	985	+	configFile, err := GetConfigFile()
	986	+	require.NoError(t, err)
	987	+	require.NoError(t, os.MkdirAll(filepath.Dir(configFile), 0o755))
	988	+
	989	+	content, err := encodeConfigToTOML(DefaultConfig())
	990	+	require.NoError(t, err)
	991	+	require.Contains(t, content, "path = '")
	992	+	require.Contains(t, content, "[workspace.floating_pane.profiles]")
	993	+	content = strings.Replace(content, "\n [workspace.floating_pane.profiles]\n", "", 1)
	994	+	require.NoError(t, os.WriteFile(configFile, []byte(content), 0o644))
	995	+
	996	+	m := NewMigrator()
	997	+	firstChanges, err := m.DetectChanges()
	998	+	require.NoError(t, err)
	999	+	assertContainsChange(t, firstChanges, port.KeyChangeAdded, "", "workspace.floating_pane.profiles")
	1000	+
	1001	+	appliedKeys, err := m.Migrate()
	1002	+	require.NoError(t, err)
	1003	+	require.NotEmpty(t, appliedKeys, "first migration should apply the missing profiles default")
	1004	+
	1005	+	secondChanges, err := m.DetectChanges()
	1006	+	require.NoError(t, err)
	1007	+	assert.Empty(t, secondChanges, "second migration detection should report no changes after a successful migration")
	1008	+
	1009	+	secondAppliedKeys, err := m.Migrate()
	1010	+	require.NoError(t, err)
	1011	+	assert.Empty(t, secondAppliedKeys, "second migration should be a no-op")
	1012	+	}
	1013	+
	1014	+	func assertContainsChange(t *testing.T, changes []port.KeyChange, changeType port.KeyChangeType, oldKey, newKey string) {
	1015	+	t.Helper()
	1016	+
	1017	+	for _, change := range changes {
	1018	+	if change.Type == changeType && change.OldKey == oldKey && change.NewKey == newKey {
	1019	+	return
	1020	+	}
	1021	+	}
	1022	+	assert.Failf(t, "missing expected change", "type=%v old=%q new=%q changes=%v", changeType, oldKey, newKey, changes)
	1023	+	}
	1024	+
977	1025		// --- Migrate-path popup → browsing_contexts transform tests ---
978	1026
979	1027		func TestMigrator_GetUserConfigKeysWithValues_TransformsLegacyPopups(t *testing.T) {


@@ -974,6 +974,54 @@ func TestMigrator_DeleteNestedKey(t *testing.T) {
974	})	974		})
975	}	975		}
976		976
		977	+	func TestMigrator_Migrate_IsIdempotentForExternalThemePathAndEmptyProfiles(t *testing.T) {
		978	+	home := t.TempDir()
		979	+	t.Setenv("HOME", home)
		980	+	t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
		981	+	t.Setenv("XDG_DATA_HOME", filepath.Join(home, ".local", "share"))
		982	+	t.Setenv("XDG_STATE_HOME", filepath.Join(home, ".local", "state"))
		983	+	t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache"))
		984	+
		985	+	configFile, err := GetConfigFile()
		986	+	require.NoError(t, err)
		987	+	require.NoError(t, os.MkdirAll(filepath.Dir(configFile), 0o755))
		988	+
		989	+	content, err := encodeConfigToTOML(DefaultConfig())
		990	+	require.NoError(t, err)
		991	+	require.Contains(t, content, "path = '")
		992	+	require.Contains(t, content, "[workspace.floating_pane.profiles]")
		993	+	content = strings.Replace(content, "\n [workspace.floating_pane.profiles]\n", "", 1)
		994	+	require.NoError(t, os.WriteFile(configFile, []byte(content), 0o644))
		995	+
		996	+	m := NewMigrator()
		997	+	firstChanges, err := m.DetectChanges()
		998	+	require.NoError(t, err)
		999	+	assertContainsChange(t, firstChanges, port.KeyChangeAdded, "", "workspace.floating_pane.profiles")
		1000	+
		1001	+	appliedKeys, err := m.Migrate()
		1002	+	require.NoError(t, err)
		1003	+	require.NotEmpty(t, appliedKeys, "first migration should apply the missing profiles default")
		1004	+
		1005	+	secondChanges, err := m.DetectChanges()
		1006	+	require.NoError(t, err)
		1007	+	assert.Empty(t, secondChanges, "second migration detection should report no changes after a successful migration")
		1008	+
		1009	+	secondAppliedKeys, err := m.Migrate()
		1010	+	require.NoError(t, err)
		1011	+	assert.Empty(t, secondAppliedKeys, "second migration should be a no-op")
		1012	+	}
		1013	+
		1014	+	func assertContainsChange(t *testing.T, changes []port.KeyChange, changeType port.KeyChangeType, oldKey, newKey string) {
		1015	+	t.Helper()
		1016	+
		1017	+	for _, change := range changes {
		1018	+	if change.Type == changeType && change.OldKey == oldKey && change.NewKey == newKey {
		1019	+	return
		1020	+	}
		1021	+	}
		1022	+	assert.Failf(t, "missing expected change", "type=%v old=%q new=%q changes=%v", changeType, oldKey, newKey, changes)
		1023	+	}
		1024	+
977	// --- Migrate-path popup → browsing_contexts transform tests ---	1025		// --- Migrate-path popup → browsing_contexts transform tests ---
978		1026
979	func TestMigrator_GetUserConfigKeysWithValues_TransformsLegacyPopups(t *testing.T) {	1027		func TestMigrator_GetUserConfigKeysWithValues_TransformsLegacyPopups(t *testing.T) {