Skip to content

feat: add provider reconfigure and disconnect actions#392

Open
geier wants to merge 6 commits intomainfrom
feature/issue-391
Open

feat: add provider reconfigure and disconnect actions#392
geier wants to merge 6 commits intomainfrom
feature/issue-391

Conversation

@geier
Copy link
Copy Markdown
Contributor

@geier geier commented Apr 23, 2026

Summary

  • add edit and disconnect actions to connected provider entries in Settings
  • reuse the existing provider auth flow for API key reconfiguration and keep OAuth flows intact
  • confirm before disconnecting and refresh provider state after credential removal

Closes #391

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds UI actions on the Settings page to reconfigure (edit) and disconnect already-connected providers, using existing auth flows and refreshing provider state after credential changes.

Changes:

  • Add a disconnectProvider(providerID) action to the providers context (wraps client.auth.remove + instance dispose + refetch).
  • Add reconfigure and disconnect buttons to each connected provider entry in Settings.
  • Reuse the existing “Add Provider” flow for API-key updates, and add a confirmation dialog for disconnect.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
app-prefixable/src/pages/settings.tsx Adds reconfigure/disconnect UI, update wording, and a confirm dialog for provider disconnect.
app-prefixable/src/context/providers.tsx Extends provider context with disconnectProvider implementation that removes credentials and refetches provider state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app-prefixable/src/pages/settings.tsx Outdated
<Show when={connecting()} fallback="Connect">
<Show when={connecting()} fallback={providers.connected.includes(selectedProvider()!) ? "Update" : "Connect"}>
<Spinner class="w-4 h-4" />
Connecting...
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

This loading state text remains "Connecting..." even when the action is an update for an already-connected provider. To match the updated button labels, consider using "Updating..." (or similar) when providers.connected.includes(selectedProvider()!) is true.

Suggested change
Connecting...
{providers.connected.includes(selectedProvider()!) ? "Updating..." : "Connecting..."}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a03025e. The loading label now switches between "Connecting..." and "Updating..." based on whether the selected provider is already connected.

Comment on lines +605 to +612
async function confirmProviderDelete() {
const providerID = providerToDelete()
if (!providerID) return
setProviderToDelete(null)
setError(null)
setSuccess(null)
const ok = await providers.disconnectProvider(providerID)
if (ok) {
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Provider disconnect flow doesn’t track any in-flight state (unlike MCP delete/connect in this file), so users can trigger multiple disconnect requests (e.g., double-click confirm / click trash again before refetch completes) and there’s no UI feedback while the async disconnect is running. Consider adding a providerDeleting/providerLoading signal (or reusing connecting) and using it to disable the trash button + ConfirmDialog confirm/cancel while the request is in progress, and optionally show a spinner.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a03025e. Provider disconnect now tracks in-flight state, disables the provider actions and confirm dialog buttons while the request is running, and surfaces disconnect errors inside the dialog.

Comment thread app-prefixable/src/pages/settings.tsx Outdated
Comment on lines 1437 to 1440
<Show when={connecting()} fallback={providers.connected.includes(selectedProvider()!) ? "Update API Key" : "Connect with API Key"}>
<Spinner class="w-4 h-4" />
Connecting...
</Show>
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The button loading text is always "Connecting..." even when the action is an update (reconfigure). This makes the UI feedback inconsistent with the updated labels (Update / Update API Key). Consider switching the loading copy to "Updating..." when the selected provider is already connected.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a03025e alongside the matching loading-copy update above. The API-key and generic connect/update buttons now show "Updating..." when reconfiguring an already connected provider.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +451 to +468
async function disconnectProvider(providerID: string): Promise<boolean> {
try {
await client.auth.remove({ providerID })
setStore("modelsByAgent", produce((state) => {
for (const [agent, model] of Object.entries(state)) {
if (model.providerID !== providerID) continue
delete state[agent]
}
}))
setStore("sessionModels", produce((state) => {
for (const [sessionID, model] of Object.entries(state)) {
if (model.providerID !== providerID) continue
delete state[sessionID]
}
}))
await client.instance.dispose()
await refetchProviders()
return true
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

disconnectProvider returns false for any failure in the post-remove cleanup (client.instance.dispose() / refetchProviders()), even though the provider credentials may already have been removed successfully. That can leave the UI showing a disconnect failure (and possibly still showing the provider as connected) while the backend state has already changed, leading to confusing retries/404s and inconsistent in-memory state. Consider treating client.auth.remove success as the primary success criterion (return true once it succeeds), and run dispose/refetch in a best-effort try/catch (or surface a separate "refresh failed" warning) so the disconnect outcome isn’t misreported.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d91ccb7. client.auth.remove is now treated as the primary success condition, while the dispose/refetch refresh step is best-effort and only logs if it fails, so the UI no longer misreports a successful disconnect as a failure.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1111 to +1114
type="button"
onClick={() => startProviderEdit(providerID)}
disabled={!!providerDeleting()}
class="p-1.5 rounded transition-colors"
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The connected-provider action buttons are only disabled during providerDeleting(), not while a connect/update OAuth/API-key flow is in-flight (connecting() / oauthPending()). This allows starting a reconfigure while another provider connect/update is running, which can race with the in-flight handlers and lead to confusing state resets (e.g., clearing oauthPending while the callback continues). Consider also disabling this edit button when connecting() (and possibly when oauthPending() is set).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 617074f. Provider reconfigure now uses a shared busy guard, so the edit action is disabled while connect/update, OAuth, or disconnect work is already in flight.

Comment thread app-prefixable/src/pages/settings.tsx Outdated
Comment on lines +1126 to +1131
disabled={!!providerDeleting()}
onClick={() => setProviderToDelete(providerID)}
class="p-1.5 rounded transition-colors"
style={{
color: "var(--interactive-critical)",
...(providerDeleting() ? { opacity: "0.6", cursor: "not-allowed" } : {}),
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The disconnect (trash) button is disabled only during providerDeleting(). While a connect/update flow is in progress (connecting() / oauthPending()), users can still open the disconnect dialog and fire a disconnect concurrently with a connect/update, which can create races (dispose/refetch ordering, success/error messages overwriting state). Consider disabling the disconnect button whenever connecting() (and/or oauthPending()) is true, similar to how the connect/update buttons are disabled.

Suggested change
disabled={!!providerDeleting()}
onClick={() => setProviderToDelete(providerID)}
class="p-1.5 rounded transition-colors"
style={{
color: "var(--interactive-critical)",
...(providerDeleting() ? { opacity: "0.6", cursor: "not-allowed" } : {}),
disabled={!!providerDeleting() || !!connecting() || !!oauthPending()}
onClick={() => setProviderToDelete(providerID)}
class="p-1.5 rounded transition-colors"
style={{
color: "var(--interactive-critical)",
...(providerDeleting() || connecting() || oauthPending() ? { opacity: "0.6", cursor: "not-allowed" } : {}),

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 617074f. The disconnect action and dialog controls now use the same shared busy state, preventing concurrent disconnects while connect/update or OAuth flows are already running.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 640 to 642
async function handleOAuthStart(providerID: string, methodIndex: number) {
setError(null)
setSuccess(null)
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

With provider reconfiguration now supported, the OAuth connect/update path should also adjust its user-facing copy for the "already connected" case. Currently the OAuth flow still reports success as "Connected to …" even when it’s effectively a re-auth/update, which is inconsistent with the updated API-key path ("Updated …").

Consider capturing wasConnected before starting OAuth (e.g., providers.connected.includes(providerID)) and using it for the success message (and any other "Connecting" copy shown during the OAuth pending state).

Copilot uses AI. Check for mistakes.
Comment on lines 1325 to +1333
<button
type="button"
onClick={() => setProviderSearch("")}
class="absolute right-2 top-1/2 -translate-y-1/2 p-1"
onClick={() => setSelectedProvider(null)}
class="text-sm"
style={{ color: "var(--text-weak)" }}
>
<X class="w-4 h-4" />
Choose a different provider
</button>
</Show>
</div>

{/* Provider grid - max height with scroll */}
<div class="grid grid-cols-2 gap-2 max-h-64 overflow-y-auto">
<For each={filteredProviders()}>
{(provider) => (
}
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The new "Choose a different provider" button can be clicked while a connect/update request is in-flight (connecting()), which allows changing selectedProvider mid-request and can lead to confusing UI state (success/errors for one provider while another/no provider is selected).

Consider disabling this button when providerActionBusy() (or at least when connecting()), similar to how the edit/disconnect actions are disabled.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add edit and delete actions for connected providers

2 participants