From 7e1598e73fd1db403df916c8f92bf322154b14f0 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:18:57 -0700 Subject: [PATCH] fix(settings): tell a failed load apart from a genuinely empty pane --- .../UsageAccountsCard.test.tsx | 114 +++++++++++++++++ .../UsageAccountsCard.tsx | 50 +++++--- .../bitbucket-integration-card.test.tsx | 117 ++++++++++++++++++ .../settings/bitbucket-integration-card.tsx | 41 ++++-- .../src/components/skills/SkillsPage.test.tsx | 64 ++++++++++ .../src/components/skills/SkillsPage.tsx | 13 +- .../components/skills/skills-page-states.tsx | 22 +++- src/renderer/src/i18n/locales/en.json | 6 +- 8 files changed, 387 insertions(+), 40 deletions(-) create mode 100644 src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.test.tsx create mode 100644 src/renderer/src/components/settings/bitbucket-integration-card.test.tsx diff --git a/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.test.tsx b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.test.tsx new file mode 100644 index 00000000000..05755834f3b --- /dev/null +++ b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.test.tsx @@ -0,0 +1,114 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + claudeList: vi.fn(), + codexList: vi.fn(), + rateLimits: { claude: null, codex: null } as Record +})) + +vi.mock('sonner', () => ({ toast: { error: vi.fn(), success: vi.fn() } })) +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: Record) => unknown) => + selector({ + fetchSettings: async () => {}, + fetchRateLimits: async () => {}, + rateLimits: mocks.rateLimits + }) +})) + +import { UsageAccountsCard } from './UsageAccountsCard' + +const EMPTY_ACCOUNTS = { accounts: [], activeAccountId: null } +const UNKNOWN_TEXT = 'Account status unknown' +const NOT_SET_UP_TEXT = 'Tracking not set up' + +let container: HTMLDivElement +let root: Root + +async function renderCard(): Promise { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + await act(async () => { + root.render() + }) +} + +describe('UsageAccountsCard account-list failures', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.rateLimits = { claude: null, codex: null } + Object.assign(window, { + api: { + claudeAccounts: { list: mocks.claudeList, add: vi.fn() }, + codexAccounts: { list: mocks.codexList, add: vi.fn() } + } + }) + }) + + afterEach(() => { + act(() => root.unmount()) + container.remove() + }) + + it('stops asserting "Tracking not set up" for a provider whose list never loaded', async () => { + mocks.claudeList.mockRejectedValue(new Error('offline')) + mocks.codexList.mockResolvedValue(EMPTY_ACCOUNTS) + + await renderCard() + + const pills = Array.from(container.querySelectorAll('span')).map((node) => node.textContent) + expect(pills).toContain(UNKNOWN_TEXT) + // Why: only the failing provider goes unknown — Codex genuinely answered "none". + expect(pills).toContain(NOT_SET_UP_TEXT) + }) + + it('keeps the real label when the list resolves empty', async () => { + mocks.claudeList.mockResolvedValue(EMPTY_ACCOUNTS) + mocks.codexList.mockResolvedValue(EMPTY_ACCOUNTS) + + await renderCard() + + expect(container.textContent).not.toContain(UNKNOWN_TEXT) + expect(container.textContent).toContain(NOT_SET_UP_TEXT) + }) + + it('prefers the observed connection when rate limits already prove tracking is on', async () => { + mocks.claudeList.mockRejectedValue(new Error('offline')) + mocks.codexList.mockResolvedValue(EMPTY_ACCOUNTS) + mocks.rateLimits = { claude: { status: 'ok', session: null, weekly: null }, codex: null } + + await renderCard() + + expect(container.textContent).not.toContain(UNKNOWN_TEXT) + expect(container.textContent).toContain('Connected · System default') + }) + + it('keeps the quiet label while the read is still in flight, and only then goes unknown', async () => { + // Why: pending is not failed. Painting "unknown" before the IPC settles alarms exactly the + // users this card targets — the ones with no tracking configured, whose rateLimits never fill. + let rejectClaude: (reason: Error) => void = () => {} + mocks.claudeList.mockReturnValue( + new Promise((_resolve, reject) => { + rejectClaude = reject + }) + ) + mocks.codexList.mockResolvedValue(EMPTY_ACCOUNTS) + + await renderCard() + + expect(container.textContent).not.toContain(UNKNOWN_TEXT) + expect(container.textContent).toContain(NOT_SET_UP_TEXT) + + await act(async () => { + rejectClaude(new Error('offline')) + await Promise.resolve() + }) + + expect(container.textContent).toContain(UNKNOWN_TEXT) + }) +}) diff --git a/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx index bda09726912..bbea4447955 100644 --- a/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx +++ b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx @@ -16,6 +16,21 @@ import { translate } from '@/i18n/i18n' type ConnectAction = 'idle' | 'adding' +// Why only 'failed': a read still in flight is not evidence of anything, so it keeps the quiet +// label until it settles. Only a read that actually FAILED may claim the status is unknown. +function usageConnectionLabel( + connection: { connected: boolean; label: string }, + accountsRead: 'pending' | 'failed' | 'loaded' +): string { + if (connection.connected || accountsRead !== 'failed') { + return connection.label + } + return translate( + 'auto.components.feature.wall.agents.orchestration.UsageAccountsCard.accountStatusUnknown', + 'Account status unknown' + ) +} + function ConnectionPill(props: { connected: boolean; label: string }): JSX.Element { const { connected, label } = props return ( @@ -97,14 +112,14 @@ export function UsageAccountsCard(props: { const fetchRateLimits = useAppStore((s) => s.fetchRateLimits) const mountedRef = useMountedRef() - const [claudeAccounts, setClaudeAccounts] = useState({ - accounts: [], - activeAccountId: null - }) - const [codexAccounts, setCodexAccounts] = useState({ - accounts: [], - activeAccountId: null - }) + const [claudeAccounts, setClaudeAccounts] = useState() + const [codexAccounts, setCodexAccounts] = useState() + const [claudeAccountsRead, setClaudeAccountsRead] = useState<'pending' | 'failed' | 'loaded'>( + 'pending' + ) + const [codexAccountsRead, setCodexAccountsRead] = useState<'pending' | 'failed' | 'loaded'>( + 'pending' + ) const [claudeAction, setClaudeAction] = useState('idle') const [codexAction, setCodexAction] = useState('idle') @@ -119,9 +134,13 @@ export function UsageAccountsCard(props: { const next = await window.api.claudeAccounts.list() if (!stale) { setClaudeAccounts(next) + setClaudeAccountsRead('loaded') } } catch { - // Silent — empty list is the right fallback for the inline pitch. + // Why: a failed read is distinct from a confirmed empty account list. + if (!stale) { + setClaudeAccountsRead('failed') + } } })() void (async () => { @@ -129,9 +148,12 @@ export function UsageAccountsCard(props: { const next = await window.api.codexAccounts.list() if (!stale) { setCodexAccounts(next) + setCodexAccountsRead('loaded') } } catch { - // Silent — same reason as above. + if (!stale) { + setCodexAccountsRead('failed') + } } })() return () => { @@ -140,11 +162,11 @@ export function UsageAccountsCard(props: { }, [fetchRateLimits]) const claudeConnection = getFeatureWallUsageProviderConnection({ - managedAccountCount: claudeAccounts.accounts.length, + managedAccountCount: claudeAccounts?.accounts.length ?? 0, provider: rateLimits.claude }) const codexConnection = getFeatureWallUsageProviderConnection({ - managedAccountCount: codexAccounts.accounts.length, + managedAccountCount: codexAccounts?.accounts.length ?? 0, provider: rateLimits.codex }) @@ -240,7 +262,7 @@ export function UsageAccountsCard(props: { 'Track session and weekly usage.' )} connected={claudeConnection.connected} - connectionLabel={claudeConnection.label} + connectionLabel={usageConnectionLabel(claudeConnection, claudeAccountsRead)} isAdding={claudeAction === 'adding'} onSignIn={() => void handleClaudeSignIn()} /> @@ -252,7 +274,7 @@ export function UsageAccountsCard(props: { 'Surface rate limits and swap accounts inline.' )} connected={codexConnection.connected} - connectionLabel={codexConnection.label} + connectionLabel={usageConnectionLabel(codexConnection, codexAccountsRead)} isAdding={codexAction === 'adding'} onSignIn={() => void handleCodexSignIn()} /> diff --git a/src/renderer/src/components/settings/bitbucket-integration-card.test.tsx b/src/renderer/src/components/settings/bitbucket-integration-card.test.tsx new file mode 100644 index 00000000000..28e01c5dbc7 --- /dev/null +++ b/src/renderer/src/components/settings/bitbucket-integration-card.test.tsx @@ -0,0 +1,117 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + refresh: vi.fn(), + status: vi.fn() +})) + +vi.mock('./source-control-preflight-card-status', () => ({ + usePreflightCardStatuses: () => ({ + statuses: { bitbucketStatus: 'not-authenticated', bitbucketAccount: null }, + unavailable: false, + refresh: mocks.refresh + }) +})) +vi.mock('./bitbucket-credentials-dialog', () => ({ + BitbucketCredentialsDialog: () => null +})) + +import { BitbucketIntegrationCard } from './bitbucket-integration-card' + +const LOAD_FAILED_TEXT = 'Could not check for a saved Bitbucket credential.' + +let container: HTMLDivElement +let root: Root + +async function renderCard(): Promise { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + await act(async () => { + root.render() + }) +} + +function recheckButton(): HTMLButtonElement { + const button = Array.from(container.querySelectorAll('button')).find( + (candidate) => candidate.textContent === 'Re-check' + ) + if (!button) { + throw new Error('Re-check button not rendered') + } + return button +} + +describe('BitbucketIntegrationCard credential-read failures', () => { + beforeEach(() => { + vi.clearAllMocks() + Object.assign(window, { + api: { + bitbucket: { status: mocks.status, disconnect: vi.fn(async () => {}) }, + shell: { openUrl: vi.fn() } + } + }) + }) + + afterEach(() => { + act(() => root.unmount()) + container.remove() + }) + + it('says the credential could not be read rather than rendering as "nothing stored"', async () => { + mocks.status.mockRejectedValue(new Error('keychain locked')) + + await renderCard() + + expect(container.textContent).toContain(LOAD_FAILED_TEXT) + }) + + it('does not claim a read failure when the status resolves', async () => { + mocks.status.mockResolvedValue({ source: 'none', account: null }) + + await renderCard() + + expect(container.textContent).not.toContain(LOAD_FAILED_TEXT) + }) + + it('retries the failed credential read from Re-check, not just the preflight', async () => { + mocks.status.mockRejectedValueOnce(new Error('keychain locked')) + mocks.status.mockResolvedValueOnce({ source: 'none', account: null }) + + await renderCard() + expect(container.textContent).toContain(LOAD_FAILED_TEXT) + + await act(async () => { + recheckButton().click() + }) + + expect(mocks.status).toHaveBeenCalledTimes(2) + expect(mocks.refresh).toHaveBeenCalled() + expect(container.textContent).not.toContain(LOAD_FAILED_TEXT) + }) + + it('does not let an older failed read overwrite a newer successful re-check', async () => { + let rejectInitial!: (error: Error) => void + mocks.status + .mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectInitial = reject + }) + ) + .mockResolvedValueOnce({ source: 'none', account: null }) + + await renderCard() + await act(async () => { + recheckButton().click() + }) + await act(async () => { + rejectInitial(new Error('late keychain failure')) + }) + + expect(container.textContent).not.toContain(LOAD_FAILED_TEXT) + }) +}) diff --git a/src/renderer/src/components/settings/bitbucket-integration-card.tsx b/src/renderer/src/components/settings/bitbucket-integration-card.tsx index 5d8809551cb..7933b6ad0da 100644 --- a/src/renderer/src/components/settings/bitbucket-integration-card.tsx +++ b/src/renderer/src/components/settings/bitbucket-integration-card.tsx @@ -1,8 +1,9 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { ExternalLink, GitPullRequestArrow, LoaderCircle, Unlink } from 'lucide-react' import type { BitbucketConnectionStatus } from '../../../../shared/bitbucket-credentials' import { Button } from '@/components/ui/button' import { useMountedRef } from '@/hooks/useMountedRef' +import { readIpcErrorMessage } from '@/lib/ipc-error' import { IntegrationCardDetails, IntegrationCardShell } from './integration-card-shell' import { useIntegrationSubordinateRowClass } from './integration-card-presentation' import type { BitbucketStatus } from './integrations-pane-status' @@ -24,17 +25,24 @@ export function BitbucketIntegrationCard(): React.JSX.Element { const [dialogOpen, setDialogOpen] = useState(false) const [disconnecting, setDisconnecting] = useState(false) const [disconnectError, setDisconnectError] = useState(null) + const [connectionLoadFailed, setConnectionLoadFailed] = useState(false) + const connectionLoadGenerationRef = useRef(0) // Reads plaintext metadata only — never the encrypted secret — so mounting the // pane cannot trigger a keychain prompt. const loadConnection = useCallback(async () => { + const generation = ++connectionLoadGenerationRef.current try { const next = await window.api.bitbucket.status() - if (mountedRef.current) { + if (mountedRef.current && generation === connectionLoadGenerationRef.current) { setConnection(next) + setConnectionLoadFailed(false) } } catch { - // Best-effort: the preflight-driven parts of the card still render. + // Why: without this the card renders exactly like "no credential stored" — say it is unknown. + if (mountedRef.current && generation === connectionLoadGenerationRef.current) { + setConnectionLoadFailed(true) + } } }, [mountedRef]) @@ -61,7 +69,9 @@ export function BitbucketIntegrationCard(): React.JSX.Element { : null const credentialSummary = [authModeLabel, baseUrlOverride].filter(Boolean).join(' · ') - const handleConnected = (): void => { + // Why: Re-check and a fresh connection both need the preflight AND the credential read, so + // Re-check also retries a status() that failed rather than only re-running the preflight. + const reloadCardState = (): void => { void loadConnection() refresh() } @@ -76,12 +86,11 @@ export function BitbucketIntegrationCard(): React.JSX.Element { // Unhandled, the card silently re-renders as still connected. if (mountedRef.current) { setDisconnectError( - error instanceof Error - ? error.message - : translate( - 'auto.components.settings.bitbucket.integration.card.disconnectFailed', - 'Could not remove the saved Bitbucket credential.' - ) + readIpcErrorMessage(error) ?? + translate( + 'auto.components.settings.bitbucket.integration.card.disconnectFailed', + 'Could not remove the saved Bitbucket credential.' + ) ) } } finally { @@ -170,6 +179,14 @@ export function BitbucketIntegrationCard(): React.JSX.Element { ) : null} {disconnectError ?

{disconnectError}

: null} + {connectionLoadFailed ? ( +

+ {translate( + 'auto.components.settings.bitbucket.integration.card.statusLoadFailed', + 'Could not check for a saved Bitbucket credential.' + )} +

+ ) : null} ) : null} - diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index acbd79afd88..6951dca9a16 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -11817,7 +11817,8 @@ "storedAuthFailed": "The saved Bitbucket credential could not authenticate. Edit it, or check that the token still has pull request access.", "storedCredential": "Saved in Orca on this machine. ORCA_BITBUCKET_* environment variables take precedence when set.", "notConfigured": "Connect a Bitbucket Cloud account with an Atlassian API token or an access token. ORCA_BITBUCKET_* environment variables work too and take precedence.", - "disconnectFailed": "Could not remove the saved Bitbucket credential." + "disconnectFailed": "Could not remove the saved Bitbucket credential.", + "statusLoadFailed": "Could not check for a saved Bitbucket credential." } } }, @@ -14310,7 +14311,8 @@ "4e71d72912": "Claude sign-in failed.", "9ddeb558f9": "Claude account added.", "29d0653961": "Sign in", - "945865332e": "Signing in" + "945865332e": "Signing in", + "accountStatusUnknown": "Account status unknown" }, "UsagePage": { "64265cb295": "29% used 5h",