diff --git a/CHANGELOG.md b/CHANGELOG.md index 49765f849..3f19edaa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Multi-window tab persistence dropped all but one tab on relaunch. Three save paths each wrote only the current window's tabs, racing the `willTerminate` aggregate save: `handleTabSelectionChange` (every time you switched tabs), the `mainWindowWillClose` notification observer, and the per-window `handleWindowWillClose` close path. On Cmd+Q, AppKit closes windows LIFO so the last writer left only the first-opened window's tab in the file. The tab-selection path now writes aggregated tabs from all windows of the connection. Per-window close-time saves no-op when the app is terminating, leaving the canonical `willTerminate` aggregate save intact. The `mainWindowWillClose` observer also routes through the aggregated save path. - Filter value autocomplete popover stole keyboard focus from the text field after the first keystroke when Full Keyboard Access was enabled (System Settings → Keyboard → Keyboard Navigation). The popover content used SwiftUI `Button` rows, which become focus targets under FKA, so SwiftUI auto-focused the first row when the popover appeared. Replaced the rows with `Text` + `.onTapGesture` (non-focusable) and marked the dropdown as `.focusable(false)`. - Toolbar database name was empty after relaunching with a connection that had no database configured but a last-used database restored via `selectDatabaseFromLastSession`. The window opened (and the toolbar resolved its initial name) before the post-connect actions populated `session.currentDatabase`, so the toolbar fell back to the empty `connection.database`. Sidebar and Cmd+K both worked because they read the session directly. The toolbar now re-syncs its database name on every `connectionStatusDidChange`, picking up the restored value once the session settles. +- AI provider settings showed `unsupported URL` on Test Connection and Reload Models while editing a draft provider's endpoint. `AIProviderFactory` cached the created provider by `(id, apiKey)` only, so a debounced model fetch that fired during a momentarily empty endpoint (cleared before retyping) stashed a provider built with `endpoint = ""`. Subsequent calls under the same draft id returned that stale instance regardless of the now-correct endpoint, and `URL(string: "/v1/models")` was rejected with `URLError.unsupportedURL`. Cache invalidation only happened on save. The cache now compares the full `AIProviderConfig` along with `apiKey`, so any field change rebuilds the provider. ### Changed diff --git a/TablePro/Core/AI/AIProviderFactory.swift b/TablePro/Core/AI/AIProviderFactory.swift index 9c40f1c29..adf64e6e6 100644 --- a/TablePro/Core/AI/AIProviderFactory.swift +++ b/TablePro/Core/AI/AIProviderFactory.swift @@ -17,12 +17,12 @@ enum AIProviderFactory { } private static let cacheLock = OSAllocatedUnfairLock( - initialState: [UUID: (apiKey: String?, provider: AIProvider)]() + initialState: [UUID: (config: AIProviderConfig, apiKey: String?, provider: AIProvider)]() ) static func createProvider(for config: AIProviderConfig, apiKey: String?) -> AIProvider { cacheLock.withLock { cache in - if let cached = cache[config.id], cached.apiKey == apiKey { + if let cached = cache[config.id], cached.apiKey == apiKey, cached.config == config { return cached.provider } let provider: AIProvider @@ -36,7 +36,7 @@ enum AIProviderFactory { maxOutputTokens: config.maxOutputTokens ) } - cache[config.id] = (apiKey, provider) + cache[config.id] = (config, apiKey, provider) return provider } } diff --git a/TableProTests/Core/AI/AIProviderFactoryCacheTests.swift b/TableProTests/Core/AI/AIProviderFactoryCacheTests.swift new file mode 100644 index 000000000..783a6c2f2 --- /dev/null +++ b/TableProTests/Core/AI/AIProviderFactoryCacheTests.swift @@ -0,0 +1,175 @@ +// +// AIProviderFactoryCacheTests.swift +// TableProTests +// +// Verifies AIProviderFactory.createProvider returns a fresh instance +// whenever any field of AIProviderConfig changes — not only id/apiKey. +// Regression coverage for the bug where mid-edit endpoint changes were +// ignored because the cache key was just (id, apiKey). +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("AIProviderFactory cache") +@MainActor +struct AIProviderFactoryCacheTests { + private func makeConfig( + id: UUID, + type: AIProviderType = .openAI, + name: String = "Test", + model: String = "gpt-x", + endpoint: String = "https://api.openai.com", + maxOutputTokens: Int? = nil + ) -> AIProviderConfig { + AIProviderConfig( + id: id, + name: name, + type: type, + model: model, + endpoint: endpoint, + maxOutputTokens: maxOutputTokens + ) + } + + // MARK: - Cache hit + + @Test("Identical config + apiKey returns the same cached instance") + func cacheHitReturnsSameInstance() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let config = makeConfig(id: id) + let first = AIProviderFactory.createProvider(for: config, apiKey: "k") + let second = AIProviderFactory.createProvider(for: config, apiKey: "k") + #expect(first === second) + } + + // MARK: - Cache miss on config changes (the bug) + + @Test("Endpoint change rebuilds the provider (the regression)") + func endpointChangeBypassesCache() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let original = makeConfig(id: id, endpoint: "https://api.openai.com") + let mutated = makeConfig(id: id, endpoint: "https://api.deepseek.com") + let first = AIProviderFactory.createProvider(for: original, apiKey: "k") + let second = AIProviderFactory.createProvider(for: mutated, apiKey: "k") + #expect(first !== second) + } + + @Test("Model change rebuilds the provider") + func modelChangeBypassesCache() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let original = makeConfig(id: id, model: "gpt-4") + let mutated = makeConfig(id: id, model: "gpt-4o") + let first = AIProviderFactory.createProvider(for: original, apiKey: "k") + let second = AIProviderFactory.createProvider(for: mutated, apiKey: "k") + #expect(first !== second) + } + + @Test("maxOutputTokens change rebuilds the provider") + func maxOutputTokensChangeBypassesCache() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let original = makeConfig(id: id, maxOutputTokens: nil) + let mutated = makeConfig(id: id, maxOutputTokens: 2_048) + let first = AIProviderFactory.createProvider(for: original, apiKey: "k") + let second = AIProviderFactory.createProvider(for: mutated, apiKey: "k") + #expect(first !== second) + } + + @Test("Name change rebuilds the provider (full-config equality)") + func nameChangeBypassesCache() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let original = makeConfig(id: id, name: "A") + let mutated = makeConfig(id: id, name: "B") + let first = AIProviderFactory.createProvider(for: original, apiKey: "k") + let second = AIProviderFactory.createProvider(for: mutated, apiKey: "k") + #expect(first !== second) + } + + @Test("apiKey change rebuilds the provider") + func apiKeyChangeBypassesCache() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let config = makeConfig(id: id) + let first = AIProviderFactory.createProvider(for: config, apiKey: "old") + let second = AIProviderFactory.createProvider(for: config, apiKey: "new") + #expect(first !== second) + } + + @Test("nil → non-nil apiKey transition rebuilds the provider") + func apiKeyNilToValueRebuilds() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let config = makeConfig(id: id) + let first = AIProviderFactory.createProvider(for: config, apiKey: nil) + let second = AIProviderFactory.createProvider(for: config, apiKey: "k") + #expect(first !== second) + } + + // MARK: - Mid-edit scenario + + @Test("Stale cached provider after empty-endpoint debounce is replaced when endpoint is finalized") + func midEditEndpointBecomesActive() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + // Mimic the SwiftUI flow: scheduleFetchModels fires while user is mid-typing + // with an empty/partial endpoint, then again once the field is filled in. + let mid = makeConfig(id: id, endpoint: "") + let final = makeConfig(id: id, endpoint: "https://api.deepseek.com") + let staleProvider = AIProviderFactory.createProvider(for: mid, apiKey: "k") + let liveProvider = AIProviderFactory.createProvider(for: final, apiKey: "k") + #expect(staleProvider !== liveProvider) + } + + // MARK: - Cache isolation by id + + @Test("Different provider ids never share cached instances") + func differentIdsAreIndependent() { + let firstID = UUID() + let secondID = UUID() + defer { + AIProviderFactory.invalidateCache(for: firstID) + AIProviderFactory.invalidateCache(for: secondID) + } + let firstConfig = makeConfig(id: firstID) + let secondConfig = makeConfig(id: secondID) + let first = AIProviderFactory.createProvider(for: firstConfig, apiKey: "k") + let second = AIProviderFactory.createProvider(for: secondConfig, apiKey: "k") + #expect(first !== second) + } + + // MARK: - Invalidation + + @Test("invalidateCache(for:) forces the next call to rebuild") + func invalidateForIdRebuilds() { + let id = UUID() + defer { AIProviderFactory.invalidateCache(for: id) } + let config = makeConfig(id: id) + let first = AIProviderFactory.createProvider(for: config, apiKey: "k") + AIProviderFactory.invalidateCache(for: id) + let second = AIProviderFactory.createProvider(for: config, apiKey: "k") + #expect(first !== second) + } + + @Test("invalidateCache(for:) only affects the targeted id") + func invalidateForIdLeavesOthersIntact() { + let targetID = UUID() + let bystanderID = UUID() + defer { + AIProviderFactory.invalidateCache(for: targetID) + AIProviderFactory.invalidateCache(for: bystanderID) + } + let target = makeConfig(id: targetID) + let bystander = makeConfig(id: bystanderID) + _ = AIProviderFactory.createProvider(for: target, apiKey: "k") + let bystanderFirst = AIProviderFactory.createProvider(for: bystander, apiKey: "k") + AIProviderFactory.invalidateCache(for: targetID) + let bystanderSecond = AIProviderFactory.createProvider(for: bystander, apiKey: "k") + #expect(bystanderFirst === bystanderSecond) + } +}