From 85159b8698b29ea8d8085edcbdd8ac4fd0de9b61 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 30 Jul 2026 00:12:35 +0900 Subject: [PATCH] fix: address ultrareview findings on profile persistence - A blank Name no longer deletes the profile: the binding path stops running the name-dropping normalization on every keystroke, and the persistence boundary substitutes the last persisted name for an in-progress blank one (rename-revert semantics). Decode-time normalization could otherwise silently drop the profile and orphan a bound home. - Repository settings writebacks no longer clobber the externally written per-repo launch memory: setDefaultAgentProfileID and setGlobalCommandEnabled mutate only their own field in the shared store, and the whole-struct binding writeback preserves (and refreshes) lastLaunchedAgentProfileID. Regression tests for both. Claude-Session: https://claude.ai/code/session_011y9A9ZLzhQ84EWXiQ5La5b --- .../Reducer/RepositorySettingsFeature.swift | 12 ++- .../Reducer/AgentProfilesFeature.swift | 32 +++++++- supacodeTests/AgentProfilesFeatureTests.swift | 80 +++++++++++++++++++ 3 files changed, 119 insertions(+), 5 deletions(-) diff --git a/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift b/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift index 06195749..d47bdaa2 100644 --- a/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift +++ b/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift @@ -236,8 +236,11 @@ struct RepositorySettingsFeature { } state.userSettings.setGlobalCommandEnabled(isEnabled, id: commandID) let rootURL = state.rootURL + // Targeted write: `state.userSettings` is a load-time snapshot, and + // `lastLaunchedAgentProfileID` is written externally by profile + // launches — a whole-struct writeback would clobber it. @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings - $userRepositorySettings.withLock { $0 = state.userSettings } + $userRepositorySettings.withLock { $0.setGlobalCommandEnabled(isEnabled, id: commandID) } return .send(.delegate(.settingsChanged(rootURL))) case .setDefaultAgentProfileID(let profileID): @@ -245,7 +248,7 @@ struct RepositorySettingsFeature { state.userSettings.defaultAgentProfileID = profileID let rootURL = state.rootURL @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings - $userRepositorySettings.withLock { $0 = state.userSettings } + $userRepositorySettings.withLock { $0.defaultAgentProfileID = profileID } return .send(.delegate(.settingsChanged(rootURL))) case .appearanceLoaded(let appearance): @@ -388,6 +391,11 @@ struct RepositorySettingsFeature { @Shared(.repositorySettings(rootURL)) var repositorySettings @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings $repositorySettings.withLock { $0 = normalizedSettings } + // `state.userSettings` is a load-time snapshot; preserve the + // externally-written launch memory instead of clobbering it with the + // stale copy (profile launches update it while Settings stays open). + let lastLaunched = userRepositorySettings.lastLaunchedAgentProfileID + state.userSettings.lastLaunchedAgentProfileID = lastLaunched $userRepositorySettings.withLock { $0 = state.userSettings } return .send(.delegate(.settingsChanged(rootURL))) diff --git a/supacode/Features/Settings/Reducer/AgentProfilesFeature.swift b/supacode/Features/Settings/Reducer/AgentProfilesFeature.swift index f40ec553..ba6fb828 100644 --- a/supacode/Features/Settings/Reducer/AgentProfilesFeature.swift +++ b/supacode/Features/Settings/Reducer/AgentProfilesFeature.swift @@ -76,7 +76,10 @@ struct AgentProfilesFeature { state.alert = Self.unrestrictedAlert(profileID: pendingID) return .none } - state.settings = state.settings.normalized() + // State deliberately stays as typed — normalizing here would fight + // the Name field (trim trailing spaces mid-word) and drop the profile + // outright the moment the field is cleared. Blank names are handled + // at the persistence boundary instead. refreshHomeStatus(&state) return persist(state.settings) @@ -155,9 +158,32 @@ struct AgentProfilesFeature { private func persist(_ settings: UserGlobalSettings) -> Effect { .run { send in @Shared(.userGlobalSettings) var storedSettings - $storedSettings.withLock { $0 = settings } - await send(.delegate(.settingsChanged(settings))) + let sanitized = Self.sanitizedForPersistence(settings, persisted: storedSettings) + $storedSettings.withLock { $0 = sanitized } + await send(.delegate(.settingsChanged(sanitized))) + } + } + + /// A blank name must never reach disk: decode-time normalization drops + /// blank-named profiles, which would silently delete the profile (and + /// orphan a bound home) on the next load. An in-progress blank name keeps + /// its last persisted name — standard rename-revert semantics — while the + /// editor state keeps whatever the user is typing. + nonisolated static func sanitizedForPersistence( + _ edited: UserGlobalSettings, + persisted: UserGlobalSettings + ) -> UserGlobalSettings { + var settings = edited + settings.agentProfiles = settings.agentProfiles.map { profile in + var profile = profile + if profile.name.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + profile.name = + persisted.agentProfiles.first { $0.id == profile.id }?.name + ?? AgentRuntimeAdapterRegistry.displayName(for: profile.runtime.agent) + } + return profile } + return settings.normalized() } private func removeProfile(_ state: inout State, id: AgentProfile.ID) { diff --git a/supacodeTests/AgentProfilesFeatureTests.swift b/supacodeTests/AgentProfilesFeatureTests.swift index 8738272e..20009b78 100644 --- a/supacodeTests/AgentProfilesFeatureTests.swift +++ b/supacodeTests/AgentProfilesFeatureTests.swift @@ -85,6 +85,43 @@ struct AgentProfilesFeatureTests { #expect(persisted.wrappedValue.agentProfiles.first?.executionMode == .unrestricted) } + @Test(.dependencies) func blankNameNeverPersistsOrDeletesTheProfile() async { + let profile = AgentProfile(name: "Codex", runtime: .codex) + let storage = SettingsTestStorage() + let (store, persisted) = withDependencies { + $0.settingsFileStorage = storage.storage + } operation: { + @Shared(.userGlobalSettings) var settings + $settings.withLock { $0.agentProfiles = [profile] } + var initial = AgentProfilesFeature.State() + initial.settings = settings + initial.selectedProfileID = profile.id + let store = TestStore(initialState: initial) { + AgentProfilesFeature() + } + return (store, $settings) + } + + // Clearing the Name field keeps the profile editable in state and keeps + // the last persisted name on disk — never a silent deletion. + var edited = store.state.settings + edited.agentProfiles[0].name = " " + await store.send(.binding(.set(\.settings, edited))) { + $0.settings.agentProfiles[0].name = " " + } + await store.receive(\.delegate.settingsChanged) + #expect(persisted.wrappedValue.agentProfiles.count == 1) + #expect(persisted.wrappedValue.agentProfiles.first?.name == "Codex") + + // Typing the new name persists it normally. + edited.agentProfiles[0].name = "Codex · Deep" + await store.send(.binding(.set(\.settings, edited))) { + $0.settings.agentProfiles[0].name = "Codex · Deep" + } + await store.receive(\.delegate.settingsChanged) + #expect(persisted.wrappedValue.agentProfiles.first?.name == "Codex · Deep") + } + @Test(.dependencies) func removingBoundProfileConfirmsAndCanTrashHome() async { var bound = AgentProfile(name: "Codex · Work", runtime: .codex) bound.bindsDedicatedHome = true @@ -198,4 +235,47 @@ struct AgentProfilesFeatureTests { } await store.receive(\.delegate.settingsChanged) } + + @Test(.dependencies) func repositorySettingsWritebacksPreserveExternalLaunchMemory() async { + let rootURL = URL(fileURLWithPath: "/tmp/repo-\(UUID().uuidString)") + let localStorage = RepositoryLocalSettingsTestStorage() + let launched = UUID() + let designated = UUID() + let (store, shared) = withDependencies { + $0.repositoryLocalSettingsStorage = localStorage.storage + } operation: { + let store = TestStore( + initialState: RepositorySettingsFeature.State( + rootURL: rootURL, + repositoryKind: .plain, + settings: .default, + userSettings: .default + ) + ) { + RepositorySettingsFeature() + } + @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings + // A profile launch writes the memory while Settings stays open with a + // stale snapshot. + $userRepositorySettings.withLock { $0.lastLaunchedAgentProfileID = launched } + return (store, $userRepositorySettings) + } + + await store.send(.setDefaultAgentProfileID(designated)) { + $0.userSettings.defaultAgentProfileID = designated + } + await store.receive(\.delegate.settingsChanged) + #expect(shared.wrappedValue.lastLaunchedAgentProfileID == launched) + #expect(shared.wrappedValue.defaultAgentProfileID == designated) + + // The whole-struct binding writeback must also preserve (and refresh) it. + var user = store.state.userSettings + user.disabledGlobalCommandIDs = ["global-build"] + await store.send(.binding(.set(\.userSettings, user))) { + $0.userSettings.disabledGlobalCommandIDs = ["global-build"] + $0.userSettings.lastLaunchedAgentProfileID = launched + } + await store.receive(\.delegate.settingsChanged) + #expect(shared.wrappedValue.lastLaunchedAgentProfileID == launched) + } } -- 2.51.2