Skip to content

fix provider auth bridge calls and implement provider settings view#68

Merged
akemmanuel merged 3 commits into
masterfrom
improving-provider-view
May 25, 2026
Merged

fix provider auth bridge calls and implement provider settings view#68
akemmanuel merged 3 commits into
masterfrom
improving-provider-view

Conversation

@akemmanuel
Copy link
Copy Markdown
Owner

  • Fix setProviderAuth parameter shape to match SDK AuthSetData
  • Fix removeProviderAuth to call DELETE /auth/{id} via raw client
  • Implement SettingsProviders with search, overlay dialogs, disconnect confirmation
  • Create DialogSelectProvider showing all providers with connection checkmarks
  • Create DialogCustomProvider with optional model fields

- Fix setProviderAuth parameter shape to match SDK AuthSetData
- Fix removeProviderAuth to call DELETE /auth/{id} via raw client
- Implement SettingsProviders with search, overlay dialogs, disconnect confirmation
- Create DialogSelectProvider showing all providers with connection checkmarks
- Create DialogCustomProvider with optional model fields
@github-actions
Copy link
Copy Markdown

opencode session  |  github run

@github-actions
Copy link
Copy Markdown

Confidence Score: 4

Issues detected:

  1. removeProviderAuth uses internal _client — fragile (opencode-bridge.ts:399)

    const res = await this._client.auth._client.delete({
      path: { id: providerID },
      url: "/auth/{id}",
    });

    Taps into the private _client property of the SDK auth sub-client. If the SDK restructures its internals this will break silently at runtime. A safer approach: either add a proper endpoint to the SDK's public API or use a raw fetch call with the base URL directly.

  2. disconnectError state is global but rendered per-provider (SettingsProviders.tsx:127)

    const [disconnectError, setDisconnectError] = useState<string | null>(null);

    The showError flag references this single state variable inside .map() loops. If disconnecting provider-a fails, the error banner renders for every provider in the list (not just the failed one), since disconnectError isn't scoped by provider ID.

  3. Stale useMemo dependency (DialogSelectProvider.tsx:70)

    }, [providers, connectedIds, lowerSearch]);

    The inner logic no longer references connectedIds (the early if (connectedIds.has(p.id)) continue; was removed), yet it remains in the dependency array. This will trigger unnecessary recomputations when connectedIds changes. Remove it from deps.

  4. Duplicate provider row rendering (SettingsProviders.tsx, search vs. connected sections)
    The same provider row markup (with disconnect confirmation, env badge, error banner, etc.) is duplicated for the search results path and the normal "Connected" section. Approximately 100 lines of identical JSX. Extract a <ProviderRow> component.

  5. loading state causes flash on every refresh() (SettingsProviders.tsx:101)

    setLoading(true);

    Called both on mount and after connect/disconnect actions. After the initial load, this resets the view to a spinner briefly even though the data is already cached. Consider a separate "refreshing" state for subsequent calls.

New%20session%20-%202026-05-25T17%3A51%3A45.007Z
opencode session  |  github run

@github-actions
Copy link
Copy Markdown

Confidence Score: 4

The follow-up commit (547159b) already addressed several concerns from the prior automated review: disconnectError is now scoped by providerID, connectedIds was removed from stale useMemo deps, and the loading spinner no longer flashes on reconnect. The removeProviderAuth in opencode-bridge.ts uses the correct public SDK API (this._client.auth.remove), not internal properties.

Remaining issues:

  1. Duplicate provider row JSX — inconsistent SourceBadge (SettingsProviders.tsx:228-312 vs :327-411)
    Both the search-results path and the connected-section path render ~85 lines of nearly identical provider row markup. They differ in small but meaningful ways: the search path omits SourceBadge entirely and wraps the name in a plain <span> instead of a min-w-0 flex container. This means users in search mode won't see whether a connected provider's auth source is "custom", "api key", or "config" — only "from env" is shown. Extract a <ProviderRow> component to eliminate the duplication and ensure consistent presentation.

    // search results path (lines 241-243)
    {isConnected ? (
      isEnv ? (
        <span>from env</span>
      ) : ...
    // connected section path (lines 344-350)
    <SourceBadge source={provider.source} />
    {isEnv ? (
      <span title="Connected from your environment variables">from env</span>
    ) : ...
  2. No error handling for initial load failures (SettingsProviders.tsx:106-113)
    The refresh() function has no catch block. If listAll() or getAuthMethods() throws during the initial mount, the component settles permanently into the "Could not load providers" state with no recovery path. Adding a dedicated error state and perhaps a retry mechanism would improve resilience.

  3. confirmingDisconnect isn't cleared when search filter changes (SettingsProviders.tsx:70)
    If a user clicks Disconnect on a provider (entering confirmation state), then starts typing in the search box, the confirmation persists but may reference a provider that is no longer visible. Dismissing it via setConfirmingDisconnect(null) won't happen until the user interacts with the now-hidden row.

  4. handleDisconnect calls dispose before refresh (SettingsProviders.tsx:131-134)

    await providersApi.disconnect(target, providerID);
    await providersApi.dispose(target);
    await refresh();

    If dispose() has side effects on the provider API instance (e.g., closing a connection pool), the subsequent refresh() call may fail. Confirm the semantics of dispose — if it's a per-operation cleanup it should be reordered after refresh().

  5. filteredProviders computed on every render (SettingsProviders.tsx:188-194)
    Unlike the memoized providerBackendIds, filteredProviders is derived inline. For small provider lists this is fine, but it's an inconsistency that could cause unnecessary work if the list grows.

New%20session%20-%202026-05-25T18%3A02%3A32.037Z
opencode session  |  github run

@akemmanuel akemmanuel merged commit 49b9c75 into master May 25, 2026
1 check passed
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.

1 participant