feat(macos): provider connection UX iteration 2#30313
Conversation
Provider connection + Model Profile picker polish (macOS side):
1. **Provider required** — InferenceProfileEditor.swift:
- New `isProviderMissing` validation guard.
- `canSave` now blocks when no provider is selected.
- "Pick a provider" warning badge on the Provider field, mirroring
the existing "Pick a model" badge.
The old inherit-defaults pathway encouraged accidental fallbacks to
the global default model, defeating the point of named profiles.
2. **Status toggle on Model Profile rows** — InferenceProfilesSheet.swift +
SettingsStore.swift:
- Inline VToggle in each profile row + dim opacity on disabled rows.
- `inFlightStatusToggles: Set<String>` guards against overlapping
PATCHes for the same profile (rapid off→on→off race).
- New `SettingsStore.setProfileStatus(name:active:)` does optimistic
update + rollback on failure. Patches `llm.profiles.<name>.status`
using the wire literal ("active"|"disabled") since
`InferenceProfile.toJSON()` omits status when active.
3. **Drop "@" prefix on provider connection key** — ProvidersSheet.swift:
The chip strip already groups the key next to the provider + auth
metadata; the leading @ made it look like a handle.
6. **Hide Advanced section when 0 credentials** — ProvidersSheet.swift:
New `shouldShowAdvancedSection` only renders the disclosure when at
least one credential exists for the provider OR the user is
mid-create. In the empty state the API Key field directly above is
the only path needed (saving a key auto-creates the credential).
Distinct from the previous-iteration regression because here Advanced
contains only the "pick existing" affordance, not the primary create
action.
|
@codex review |
|
@devin review |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private var shouldShowAdvancedSection: Bool { | ||
| let providerCredentials = availableCredentials.filter { $0.service == editorDraft.provider } | ||
| return !providerCredentials.isEmpty || isCreatingNewCredential | ||
| } |
There was a problem hiding this comment.
🔴 shouldShowAdvancedSection hides the only UI path to create named credentials, contradicting explicit PR #30294 warning
The new shouldShowAdvancedSection guard hides the entire Advanced disclosure section when no credentials exist for the provider AND isCreatingNewCredential is false. However, the only way to set isCreatingNewCredential = true is by clicking the "+ New Credential" button at ProvidersSheet.swift:576 — which lives inside the hidden section. This creates a chicken-and-egg problem: the section is hidden because no credentials exist, but the user can't create credentials because the button is hidden.
The comment at ProvidersSheet.swift:340-346 explicitly warns against this:
"Advanced disclosure stays visible even when there are zero credentials — the empty state inside shows a '+ New Credential' button which is the only path to building a named credential before saving an API key. Hiding the whole section regresses that affordance (Codex P2, PR #30294)."
The || isCreatingNewCredential condition in shouldShowAdvancedSection is unreachable from the empty-credentials state, making it dead code in that path.
Prompt for agents
The shouldShowAdvancedSection property at ProvidersSheet.swift:530-533 hides the Advanced section when no credentials exist for the provider. This removes the only UI path to create named credentials (the '+ New Credential' button inside that section), directly contradicting the explicit warning at ProvidersSheet.swift:340-346 from PR #30294.
To fix: either remove the shouldShowAdvancedSection guard entirely (reverting to the old always-visible behavior), or move the '+ New Credential' button outside the conditionally-hidden section so users always have a path to create named credentials regardless of whether existing credentials are loaded. The simplest fix is to remove the shouldShowAdvancedSection check and the @ViewBuilder annotation, restoring the section to always render when authType is api_key.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Thanks for catching the contradiction — pushed c756b4f which keeps the behavior (this is item 6 of Noa's iter2 list — "when 0 keys, simplify to plain text input") and updates the inline comment to reflect the new design: editorApiKeyField (the plain VTextField above) is the empty-state path, not the "+ New Credential" button. Saving auto-creates credential/<provider>/api_key. Users who want a named credential save first with the default ref, then re-open the editor to use Advanced. The named-credential path isn't gone — it's deferred until after the first key exists.
| .font(VFont.bodySmallDefault) | ||
| .foregroundStyle(VColor.contentSecondary) | ||
| } | ||
| .opacity(isActive ? 1.0 : 0.55) | ||
| Spacer(minLength: 0) | ||
| VButton(label: profile.isManaged ? "View" : "Edit", style: .ghost) { | ||
| if profile.isManaged { | ||
| beginView(profile.name) | ||
| } else { | ||
| beginEdit(profile.name) | ||
| HStack(spacing: VSpacing.sm) { | ||
| VToggle( | ||
| isOn: Binding( | ||
| get: { isActive }, | ||
| set: { newActive in | ||
| Task { await setProfileStatus(profile, active: newActive) } | ||
| } | ||
| ) | ||
| ) | ||
| .accessibilityLabel("\(isActive ? "Disable" : "Activate") profile \(profile.displayName)") | ||
| .help(isActive ? "Active — toggle to hide from pickers" : "Disabled — toggle to show in pickers") |
There was a problem hiding this comment.
🚩 Status toggle accessible on managed profiles — verify daemon accepts status PATCHes for managed rows
The comment at InferenceProfilesSheet.swift:247-249 says the inline status toggle works for managed profiles too, reasoning that status is a UI-level concern. The setProfileStatus method at SettingsStore.swift:3430 sends a patchConfig with just the status field. Whether the daemon accepts a status PATCH for a managed profile (source == "managed") depends on the daemon's write-protection logic for managed profiles. If the daemon rejects the PATCH, the rollback at lines 3448-3453 will restore the previous status, and the UI will show an error via actionError. This is a safe degradation, but the team should verify the daemon actually accepts status-only patches on managed profiles — otherwise the toggle will always fail for those rows.
(Refers to lines 245-305)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — the daemon accepts status PATCHes on managed profiles. status is a UI-only concern in the daemon's profile model (it's not part of the managed-immutable contract; only model/provider/auth are). The optimistic update + rollback path in this PR safely degrades if the daemon ever rejects: failure → rollback to previous status → actionError displayed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3440a6e633
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let success = await settingsClient.patchConfig([ | ||
| "llm": ["profiles": [name: ["status": wireStatus]]] | ||
| ]) |
There was a problem hiding this comment.
setProfileStatus always sends a status-only patch for llm.profiles.<name> even if a delete of the same profile is in flight. Because deletion uses NSNull() and this method later sends {status: ...}, a quick “toggle then delete” can be applied out of order and recreate the profile key with only status set (the schema permits status-only profile entries), leaving a ghost profile in config. This race was introduced with the new inline toggle path and needs coordination with delete (e.g., block delete while status update is pending or cancel/serialize per-profile writes).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c756b4f. Added per-name pendingStatusPatches tracking in SettingsStore. setProfileStatus registers its PATCH task in the dict; deleteProfile awaits any in-flight task for the same name before issuing its delete PATCH. All on @MainActor so no extra synchronization needed. The slot-identity check (== on Task) prevents clobbering a newer in-flight PATCH if two toggles race.
Addresses Codex + Devin review on PR #30313: 1. Codex P1 — Prevent status PATCH from resurrecting deleted profiles (SettingsStore.swift): a fast "toggle status → delete" sequence could let a status-only PATCH arrive at the daemon AFTER the delete PATCH, re-creating the profile as `{ status: "disabled" }` via deep-merge. Fix: track in-flight `setProfileStatus` PATCHes per profile name in `pendingStatusPatches`, and have `deleteProfile` await any pending status PATCH for the target name before issuing its delete PATCH. The whole store is `@MainActor` so no extra synchronization is needed; the slot identity check (`==` on Task) avoids clobbering a newer in-flight PATCH if two toggles race. 2. Devin 🔴 (ProvidersSheet.swift) — stale comment that contradicted the new design. The previous comment warned "Advanced disclosure stays visible even when there are zero credentials" (Codex P2, PR #30294). Iter2 item 6 deliberately reverses that: the empty state now shows only the plain `editorApiKeyField`, and saving auto-creates `credential/<provider>/api_key`. Updated the comment to document the new design intent — users who want a *named* credential save first with the default ref, then re-open the editor to use Advanced. Behavior is unchanged; only the comment that misrepresented the new intent was wrong. Devin's 🚩 "managed-profile status PATCH" analysis is informational only — the daemon accepts status PATCHes on managed profiles (status is a UI-only concern); rollback on failure already exists, so a hypothetical daemon-side rejection would degrade safely.
|
@codex review |
|
@devin review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c756b4f86c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return await self.settingsClient.patchConfig([ | ||
| "llm": ["profiles": [name: ["status": wireStatus]]] | ||
| ]) |
There was a problem hiding this comment.
setProfileStatus always sends a status-only PATCH even when the profile is no longer present in profiles. If the row disappears between render and toggle handling (for example, a concurrent config sync deletes it), this payload (llm.profiles.<name>.status) will be deep-merged into config and recreate a partial profile entry containing only status. That can leave orphaned profiles in pickers/lists and is the same resurrection class of bug this change is trying to prevent for delete races. Add an existence guard before issuing the PATCH (or fail fast when the local profile lookup misses).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac4f769. Added an existence guard at the top of setProfileStatus: if the profile is no longer in profiles (concurrent config sync removed it, etc.), log a warning and return false without firing the PATCH. Same defensive shape as the pendingStatusPatches serialization — both are about preventing status-only deep-merges from resurrecting deleted profile keys.
Addresses Codex P2 on iter2 round 2 review. If a concurrent config sync
removes the profile between render and the toggle handler firing, the
status-only PATCH is deep-merged by the daemon and resurrects the
profile key as `{ status: ... }` only — leaving a partial / orphaned
profile in pickers. Same resurrection class of bug as the toggle→delete
race already serialized via `pendingStatusPatches`.
Fix: existence guard at the top of `setProfileStatus`. If the profile
is not in the local `profiles` list, log a warning and return false
without firing the PATCH.
|
@codex review |
|
@devin review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…30423) Marina QA on 0.8.1 Desktop Cloud-hosted: an OpenRouter connection was labeled with the Anthropic tag. The reporter theorized that the system auto-detected provider from the "sk-" key prefix — there is no such heuristic anywhere in the codebase. The actual root cause sits in the macOS provider-connection editor. `ProvidersSheet.beginCreate()` pre-filled `editorDraft.provider = providerCatalog.first?.id`. Anthropic is the first entry in PROVIDER_CATALOG, so opening "New Connection" silently selected Anthropic. If a user pasted an OpenRouter key (or any provider other than Anthropic) into the API Key field and hit Save without scanning the Provider dropdown, the row persisted as `provider=anthropic` with the OpenRouter key stored at `credential/anthropic/api_key`. Downstream, the connection rendered the Anthropic display tag (correct given the bad row) and the Profiles picker filtered it out when the user wanted to bind an OpenRouter profile to it (since `c.provider !== "openrouter"`). Fix — mirrors the `isProviderMissing` pattern that #30313 introduced on the Inference Profile editor: 1. `beginCreate()` starts with an empty `ConnectionDraft`. No pre-filled provider, no pre-filled credential ref. The dropdown placeholder ("Select a provider…") makes the unfilled state visible. Provider's `onChange` already populates the credential ref (`credential/<provider>/api_key`) and the auth-type override (Ollama → none) when the user picks a value, so removing the pre-fill doesn't drop functionality. 2. New `isProviderMissing` + `canSave` computed properties expose the validation state to the view layer. `isProviderMissing` only trips in create mode — `beginEdit` pre-fills from `conn.provider`, and `saveAsNewFromManagedEdit` carries the managed row's provider into the clone, so neither path produces an empty draft. 3. Provider field renders a "Pick a provider" warning badge inline with the label while `isProviderMissing` is true. Same affordance pattern as the "Pick a model" badge in `InferenceProfileEditor.modelField`. 4. Primary footer button gets `isDisabled: !canSave` so the user can't fire Save until the dropdown is satisfied. The button stays enabled for legitimate edit paths (`beginEdit`, `saveAsNewFromManagedEdit`) because both seed a non-empty provider. 5. `commitEditor()` gains a belt-and-suspenders guard that surfaces "Select a provider." inline if a future code path bypasses the disabled button (keyboard submit, programmatic Save, etc.). The daemon's zod schema would reject `provider=""` either way, but this keeps the error message in the editor where the user is. Edit mode is unaffected: `editorProviderField` is only rendered for `.create` state and the dropdown is hidden in `.edit` / `.managedEdit`, so existing connections render their saved provider tag as before. The Save-as-New flow seeds `editorDraft.provider` from the source managed row before flipping to `.create`, so `canSave` returns true on first frame. Cherry-pick candidate for release/v0.8.1. Co-authored-by: apollo-the-bot[bot] <apollo-the-bot[bot]@users.noreply.github.com>
Marina QA on 0.8.1 Desktop Cloud-hosted: an OpenRouter connection was labeled with the Anthropic tag. The reporter theorized that the system auto-detected provider from the "sk-" key prefix — there is no such heuristic anywhere in the codebase. The actual root cause sits in the macOS provider-connection editor. `ProvidersSheet.beginCreate()` pre-filled `editorDraft.provider = providerCatalog.first?.id`. Anthropic is the first entry in PROVIDER_CATALOG, so opening "New Connection" silently selected Anthropic. If a user pasted an OpenRouter key (or any provider other than Anthropic) into the API Key field and hit Save without scanning the Provider dropdown, the row persisted as `provider=anthropic` with the OpenRouter key stored at `credential/anthropic/api_key`. Downstream, the connection rendered the Anthropic display tag (correct given the bad row) and the Profiles picker filtered it out when the user wanted to bind an OpenRouter profile to it (since `c.provider !== "openrouter"`). Fix — mirrors the `isProviderMissing` pattern that #30313 introduced on the Inference Profile editor: 1. `beginCreate()` starts with an empty `ConnectionDraft`. No pre-filled provider, no pre-filled credential ref. The dropdown placeholder ("Select a provider…") makes the unfilled state visible. Provider's `onChange` already populates the credential ref (`credential/<provider>/api_key`) and the auth-type override (Ollama → none) when the user picks a value, so removing the pre-fill doesn't drop functionality. 2. New `isProviderMissing` + `canSave` computed properties expose the validation state to the view layer. `isProviderMissing` only trips in create mode — `beginEdit` pre-fills from `conn.provider`, and `saveAsNewFromManagedEdit` carries the managed row's provider into the clone, so neither path produces an empty draft. 3. Provider field renders a "Pick a provider" warning badge inline with the label while `isProviderMissing` is true. Same affordance pattern as the "Pick a model" badge in `InferenceProfileEditor.modelField`. 4. Primary footer button gets `isDisabled: !canSave` so the user can't fire Save until the dropdown is satisfied. The button stays enabled for legitimate edit paths (`beginEdit`, `saveAsNewFromManagedEdit`) because both seed a non-empty provider. 5. `commitEditor()` gains a belt-and-suspenders guard that surfaces "Select a provider." inline if a future code path bypasses the disabled button (keyboard submit, programmatic Save, etc.). The daemon's zod schema would reject `provider=""` either way, but this keeps the error message in the editor where the user is. Edit mode is unaffected: `editorProviderField` is only rendered for `.create` state and the dropdown is hidden in `.edit` / `.managedEdit`, so existing connections render their saved provider tag as before. The Save-as-New flow seeds `editorDraft.provider` from the source managed row before flipping to `.create`, so `canSave` returns true on first frame. Cherry-pick candidate for release/v0.8.1.
1 participant
Provider connection UX iteration 2 (macOS)
Follow-up polish on the provider connection + Model Profile picker flow,
matched 1:1 by web counterpart at vellum-ai/vellum-assistant-platform#6493.
Changes
Provider required on Model Profile editor —
InferenceProfileEditor.swiftisProviderMissingguard;canSaveblocks when no provider is selected.Inline status toggle on Model Profile rows —
InferenceProfilesSheet.swift+SettingsStore.swiftVToggleper row; dim opacity on disabled rows.inFlightStatusToggles: Set<String>guards rapid off→on→off races.SettingsStore.setProfileStatus(name:active:)does optimistic update + rollback on failure.llm.profiles.<name>.statususing the wire literal ("active"|"disabled") sinceInferenceProfile.toJSON()omits status when active.Drop
@prefix on provider connection key —ProvidersSheet.swift@made it look like a handle.Hide Advanced section when 0 credentials —
ProvidersSheet.swiftshouldShowAdvancedSectiononly renders the disclosure when at least one credential exists for the provider OR the user is mid-create.What's NOT in this PR
Verification
Bot tags