Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,21 @@ struct InferenceProfileEditor: View {
/// Provider connections available for the Connection sub-dropdown. The
/// editor reads this list, filters by the currently-selected provider
/// and `.status == .active`, and lets the user route the profile to a
/// specific row. Defaults to empty so test constructions and callers
/// specific row. Defaults to nil so test constructions and callers
/// that don't care about connection routing still compile — daemons
/// older than the `provider_connection`-aware profile schema continue
/// to behave as "pick the first active connection for the provider."
var connections: [ProviderConnection] = []
///
/// `nil` vs `[]` is meaningful:
/// - `nil` → the parent has not yet fetched `listProviderConnections`
/// (pre-load window between `.task` firing and the daemon response).
/// The provider picker falls back to the full catalog so the trigger
/// isn't empty during that gap.
/// - `[]` → the daemon returned zero connections. A fresh workspace
/// with nothing configured. The provider picker filters to empty,
/// the empty-state hint fires, and the user is steered to Providers
/// instead of being allowed to pick a non-dispatchable provider.
var connections: [ProviderConnection]? = nil
let onSave: () -> Void
var onSaveAs: (() -> Void)?
let onCancel: () -> Void
Expand Down Expand Up @@ -317,8 +327,39 @@ struct InferenceProfileEditor: View {
.joined(separator: "-")
}

/// Provider IDs visible in the picker. Filtered to providers that
/// have at least one ACTIVE connection — picking a provider with
/// zero active connections binds the profile to a route the daemon
/// can't dispatch through, leaving the user stuck. The currently-
/// bound `provider` is always kept in the list so editing/viewing a
/// stale profile (whose connection was disabled after the binding
/// was saved) still renders a sensible trigger.
///
/// Pre-load fallback: when `connections` is `nil` (the sheet's
/// `.task` hasn't completed its first `listProviderConnections`
/// fetch yet, or an older daemon that doesn't surface the connection
/// list), return the full catalog so the user doesn't see an empty
/// picker on first open. An EMPTY-but-loaded `connections == []` is
/// distinct: the daemon confirmed zero connections, so the filter
/// runs and yields empty — the empty-state hint fires and steers
/// the user to Providers instead of letting them save a profile
/// bound to a non-dispatchable provider.
///
/// Mirrors web's `visibleProviders` + `providerOptionsSource` in
/// `web/src/app/(app)/assistant/settings/ai/profile-editor-modal.tsx`
/// (PR #6509). The web sibling has the same nil-vs-empty trap and
/// is being addressed in a follow-up.
var availableProviderIds: [String] {
store.dynamicProviderIds
guard let connections else { return store.dynamicProviderIds }

var activeProviderSet = Set<String>()
for connection in connections where connection.status == .active {
activeProviderSet.insert(connection.provider)
}
if let bound = profile.provider, !bound.isEmpty {
activeProviderSet.insert(bound)
}
return store.dynamicProviderIds.filter { activeProviderSet.contains($0) }
Comment on lines 352 to +362
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Stale-provider profiles can still be saved without changing the provider

The availableProviderIds filter at InferenceProfileEditor.swift:336-346 keeps the currently-bound provider in the picker even when it has no active connections. This is intentional for UX (so the user sees the stale binding and can change it). However, note that canSave at InferenceProfileEditor.swift:115-117 does NOT gate on whether the selected provider has active connections — it only checks isProviderMissing, isModelMissing, and isModelInvalid. This means a user can open an existing profile bound to a provider with only disabled connections and hit Save without changing the provider, persisting a profile the daemon can't dispatch through. The PR's stated goal is to prevent picking such providers (which it does for new selections), but the save-side validation gap may warrant a follow-up if the intent is stricter enforcement.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

private var providerField: some View {
Expand Down Expand Up @@ -369,14 +410,21 @@ struct InferenceProfileEditor: View {
(label: store.dynamicProviderDisplayName(provider), value: provider)
}
)
if availableProviderIds.isEmpty && !isReadOnly {
Text("No active provider connections. Open Providers to add or enable one.")
.font(VFont.bodySmallDefault)
.foregroundStyle(VColor.contentTertiary)
}
}
}

/// Active connections that match the currently-selected provider. Used
/// by `connectionField` to populate its dropdown.
/// by `connectionField` to populate its dropdown. During pre-load
/// (`connections == nil`) there's nothing to pick — the connection
/// sub-dropdown stays hidden until the fetch completes.
var availableConnectionsForProvider: [ProviderConnection] {
guard let provider = profile.provider, !provider.isEmpty else { return [] }
return connections.filter { $0.provider == provider && $0.status == .active }
return (connections ?? []).filter { $0.provider == provider && $0.status == .active }
}

/// The currently-saved binding when it does NOT resolve to any active
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ struct InferenceProfilesSheet: View {
/// for the currently-selected provider. Refreshed on appear and after
/// each editor commit so users who add a connection in another surface
/// see it without a manual reload.
@State private var connections: [ProviderConnection] = []
///
/// `nil` until the first `listProviderConnections` response lands —
/// the editor uses this to distinguish "still loading" from "loaded,
/// daemon returned zero connections." Without the distinction, a
/// fresh workspace would see the full provider catalog and could
/// bind a profile to a non-dispatchable provider.
@State private var connections: [ProviderConnection]? = nil

@State private var editorState: EditorState?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,112 @@ final class InferenceProfileEditorTests: XCTestCase {
XCTAssertNotNil(editor.body)
}

// MARK: - Provider picker filter (iter3 QA issue #1, parity with web PR #6509)

/// Only providers with at least one ACTIVE connection are surfaced in
/// the picker. A provider whose only connection is disabled (openai
/// below) must not appear, because binding a profile to it would
/// route through a credential the daemon will skip on dispatch.
func testAvailableProviderIdsHidesProvidersWithoutActiveConnection() {
let connections: [ProviderConnection] = [
Self.makeConnection(name: "active-anthropic", provider: "anthropic", status: .active),
Self.makeConnection(name: "disabled-openai", provider: "openai", status: .disabled),
]
let editor = InferenceProfileEditor(
store: store,
profile: .constant(InferenceProfile(name: "draft")),
connections: connections,
onSave: {},
onCancel: {}
)
XCTAssertEqual(editor.availableProviderIds, ["anthropic"])
}

/// Stale binding recovery: the editor is opened on a profile whose
/// `provider` value no longer has any active connection. The bound
/// provider must still appear in the picker so the user can see it
/// (and pick a different one) instead of finding an empty trigger.
func testAvailableProviderIdsKeepsCurrentBoundProvider() {
let connections: [ProviderConnection] = [
Self.makeConnection(name: "active-anthropic", provider: "anthropic", status: .active),
Self.makeConnection(name: "disabled-openai", provider: "openai", status: .disabled),
]
let editor = InferenceProfileEditor(
store: store,
profile: .constant(InferenceProfile(
name: "draft",
provider: "openai",
model: "gpt-5"
)),
connections: connections,
onSave: {},
onCancel: {}
)
// Order follows store.dynamicProviderIds catalog order, which
// happens to put anthropic before openai in the test fixture.
XCTAssertEqual(editor.availableProviderIds, ["anthropic", "openai"])
}

/// Pre-load fallback: when `connections` is nil (the parent sheet's
/// `.task` hasn't completed its first `listProviderConnections`
/// fetch yet, or the daemon predates the connections API), the
/// picker shows the full catalog so the user isn't faced with an
/// empty trigger during the network round-trip. Once connections
/// load — even to `[]` — the active-only filter kicks in.
///
/// This is the half of the "nil vs []" distinction. The other half
/// is `testAvailableProviderIdsIsEmptyWhenConnectionsLoadedButEmpty`
/// below.
func testAvailableProviderIdsFallsBackToFullCatalogWhenConnectionsAreNil() {
let editor = InferenceProfileEditor(
store: store,
profile: .constant(InferenceProfile(name: "draft")),
onSave: {},
onCancel: {}
)
// Default `connections` is nil — the pre-load state.
XCTAssertEqual(editor.availableProviderIds, store.dynamicProviderIds)
XCTAssertFalse(editor.availableProviderIds.isEmpty)
}

/// Loaded-but-empty: the daemon confirmed zero connections (fresh
/// workspace). This MUST NOT fall back to the full catalog — that
/// would let the user save a profile bound to a non-dispatchable
/// provider, which is the exact trap this PR is closing. The picker
/// renders empty; the empty-state hint elsewhere in the editor
/// steers the user to the Providers surface.
///
/// Codex P1 (PR #30330): the original `guard !connections.isEmpty`
/// fallback conflated this case with pre-load and re-introduced the
/// QA trap for fresh workspaces. The fix is the nil/empty split.
func testAvailableProviderIdsIsEmptyWhenConnectionsLoadedButEmpty() {
let editor = InferenceProfileEditor(
store: store,
profile: .constant(InferenceProfile(name: "draft")),
connections: [],
onSave: {},
onCancel: {}
)
XCTAssertTrue(editor.availableProviderIds.isEmpty)
}

/// All-disabled connections + no bound provider → the filter yields
/// empty. The picker still renders (the empty-state hint below it
/// drives the user to Providers), but no provider rows appear.
func testAvailableProviderIdsIsEmptyWhenOnlyDisabledConnectionsAndNoBoundProvider() {
let connections = [
Self.makeConnection(name: "disabled-openai", provider: "openai", status: .disabled),
]
let editor = InferenceProfileEditor(
store: store,
profile: .constant(InferenceProfile(name: "draft")),
connections: connections,
onSave: {},
onCancel: {}
)
XCTAssertTrue(editor.availableProviderIds.isEmpty)
}

/// Test helper mirroring `ProvidersSheetTests.makeConnection` so the
/// two surfaces use identical fixture shapes.
private static func makeConnection(
Expand Down
Loading