From 4bcdc6736985fb763adc300c5d41f2e3e2d37830 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 14 Sep 2026 16:44:30 -0700 Subject: [PATCH] Distinguish pane load failures from empty states (#20735) * refactor(renderer): give the IPC error reader a clamped and an unclamped shape * fix(settings): tell a failed load apart from a genuinely empty pane * refactor: consolidate import types and simplify failure handling - Move filesystem import types to shared for renderer use - Add compactIpcErrorMessage for single-line error display - Consolidate entry failure toasts to single global slot - Simplify account tracking and discard retry logic * fix type * fix: clear stale state when pane loads fail Credential reads, account fetches, and skill scans can fail, leaving stale data on screen. This change clears previous state when a load fails, distinguishing load failures from genuinely empty results, and prevents stale controls from appearing after failed re-checks. Use readIpcErrorMessage for consistent error handling and track runtime targets to invalidate results from old targets. * fix(settings): show credential action when bitbucket status read fails When the credential-read operation fails, allow users to retry by showing "Add or replace credentials" button. Initialize the credentials dialog with the current (confirmed) connection state instead of stale data from a failed read, preventing outdated information from pre-populating the form. --- .../UsageAccountsCard.test.tsx | 111 ++++++++++++ .../UsageAccountsCard.tsx | 23 +-- .../feature-wall-usage-tracking.test.ts | 18 ++ .../feature-wall-usage-tracking.ts | 16 +- .../bitbucket-integration-card.test.tsx | 167 ++++++++++++++++++ .../settings/bitbucket-integration-card.tsx | 95 ++++++---- .../src/components/skills/SkillsPage.test.tsx | 94 ++++++++++ .../src/components/skills/SkillsPage.tsx | 32 ++-- .../components/skills/skills-page-states.tsx | 19 +- src/renderer/src/i18n/locales/en.json | 7 +- 10 files changed, 515 insertions(+), 67 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..659409f0643 --- /dev/null +++ b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.test.tsx @@ -0,0 +1,111 @@ +// @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(() => { + const rateLimits: Record = { claude: null, codex: null } + return { claudeList: vi.fn(), codexList: vi.fn(), rateLimits } +}) + +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('does not claim tracking is unset while the account read is pending', async () => { + let rejectClaude: (reason: Error) => void = () => {} + mocks.claudeList.mockReturnValue( + new Promise((_resolve, reject) => { + rejectClaude = reject + }) + ) + mocks.codexList.mockResolvedValue(EMPTY_ACCOUNTS) + + await renderCard() + + expect(container.textContent).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..14cd1f67b6f 100644 --- a/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx +++ b/src/renderer/src/components/feature-wall/agents-orchestration/UsageAccountsCard.tsx @@ -6,6 +6,7 @@ import { useAppStore } from '@/store' import { Button } from '@/components/ui/button' import { ClaudeIcon, OpenAIIcon } from '@/components/status-bar/icons' import { cn } from '@/lib/utils' +import { readIpcErrorMessage } from '@/lib/ipc-error' import { useMountedRef } from '@/hooks/useMountedRef' import type { ClaudeRateLimitAccountsState, @@ -97,14 +98,8 @@ 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 [claudeAction, setClaudeAction] = useState('idle') const [codexAction, setCodexAction] = useState('idle') @@ -121,7 +116,7 @@ export function UsageAccountsCard(props: { setClaudeAccounts(next) } } catch { - // Silent — empty list is the right fallback for the inline pitch. + // Leave the account state unknown. } })() void (async () => { @@ -131,7 +126,7 @@ export function UsageAccountsCard(props: { setCodexAccounts(next) } } catch { - // Silent — same reason as above. + // Leave the account state unknown. } })() return () => { @@ -140,11 +135,11 @@ export function UsageAccountsCard(props: { }, [fetchRateLimits]) const claudeConnection = getFeatureWallUsageProviderConnection({ - managedAccountCount: claudeAccounts.accounts.length, + managedAccountCount: claudeAccounts?.accounts.length, provider: rateLimits.claude }) const codexConnection = getFeatureWallUsageProviderConnection({ - managedAccountCount: codexAccounts.accounts.length, + managedAccountCount: codexAccounts?.accounts.length, provider: rateLimits.codex }) @@ -178,7 +173,7 @@ export function UsageAccountsCard(props: { 'Claude sign-in failed.' ), { - description: String((error as Error)?.message ?? error) + description: readIpcErrorMessage(error) } ) } @@ -219,7 +214,7 @@ export function UsageAccountsCard(props: { 'Codex sign-in failed.' ), { - description: String((error as Error)?.message ?? error) + description: readIpcErrorMessage(error) } ) } diff --git a/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.test.ts b/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.test.ts index 27c4784e122..fd07b2fe198 100644 --- a/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.test.ts +++ b/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.test.ts @@ -52,6 +52,24 @@ describe('feature wall usage tracking state', () => { ).toEqual({ connected: false, label: 'Tracking not set up' }) }) + it('returns unknown when the account list has not loaded', () => { + expect( + getFeatureWallUsageProviderConnection({ + managedAccountCount: undefined, + provider: null + }) + ).toEqual({ connected: false, label: 'Account status unknown' }) + }) + + it('prefers observed provider usage over an unknown account list', () => { + expect( + getFeatureWallUsageProviderConnection({ + managedAccountCount: undefined, + provider: rateLimits() + }) + ).toEqual({ connected: true, label: 'Connected · System default' }) + }) + it('marks the usage step complete from system-default provider data', () => { expect( hasFeatureWallUsageTracking({ diff --git a/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.ts b/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.ts index 8702b72336f..8b389216546 100644 --- a/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.ts +++ b/src/renderer/src/components/feature-wall/feature-wall-usage-tracking.ts @@ -19,16 +19,17 @@ export function hasFeatureWallProviderUsageTracking(provider: ProviderRateLimits } export function getFeatureWallUsageProviderConnection(args: { - managedAccountCount: number + managedAccountCount: number | undefined provider: ProviderRateLimits | null }): FeatureWallUsageProviderConnection { - if (args.managedAccountCount > 0) { + const { managedAccountCount } = args + if (managedAccountCount !== undefined && managedAccountCount > 0) { return { connected: true, label: translate( 'auto.components.feature.wall.feature.wall.usage.tracking.00087eecb2', 'Connected · {{value0}}', - { value0: args.managedAccountCount } + { value0: managedAccountCount } ) } } @@ -41,6 +42,15 @@ export function getFeatureWallUsageProviderConnection(args: { ) } } + if (managedAccountCount === undefined) { + return { + connected: false, + label: translate( + 'auto.components.feature.wall.agents.orchestration.UsageAccountsCard.accountStatusUnknown', + 'Account status unknown' + ) + } + } return { connected: false, label: translate( 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..fed4922add1 --- /dev/null +++ b/src/renderer/src/components/settings/bitbucket-integration-card.test.tsx @@ -0,0 +1,167 @@ +// @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(), + statuses: { bitbucketStatus: 'not-authenticated', bitbucketAccount: null } +})) + +vi.mock('./source-control-preflight-card-status', () => ({ + usePreflightCardStatuses: () => ({ + statuses: mocks.statuses, + unavailable: false, + refresh: mocks.refresh + }) +})) +vi.mock('./bitbucket-credentials-dialog', () => ({ + BitbucketCredentialsDialog: ({ open, initialEmail }: { open: boolean; initialEmail?: string }) => + open ?
Credential dialog open {initialEmail}
: 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() + mocks.statuses = { bitbucketStatus: 'not-authenticated', bitbucketAccount: null } + 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) + expect(container.textContent).not.toContain('Connect') + expect(container.textContent).toContain('Add or replace credentials') + expect(container.textContent).not.toContain('credentials are configured') + + await act(async () => { + Array.from(container.querySelectorAll('button')) + .find((button) => button.textContent === 'Add or replace credentials') + ?.click() + }) + expect(container.textContent).toContain('Credential dialog open') + }) + + 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) + }) + + it('does not expose stale credential controls after a re-check fails', async () => { + mocks.statuses = { bitbucketStatus: 'connected', bitbucketAccount: null } + mocks.status + .mockResolvedValueOnce({ + configured: true, + source: 'stored', + account: 'stale-account', + authMode: 'token', + email: 'stale@example.com', + baseUrl: null + }) + .mockRejectedValueOnce(new Error('keychain locked')) + + await renderCard() + expect(container.textContent).toContain('stale-account') + expect(container.textContent).toContain('Edit credentials') + expect(container.querySelector('[aria-label="Disconnect Bitbucket"]')).not.toBeNull() + + await act(async () => { + recheckButton().click() + }) + + expect(container.textContent).toContain(LOAD_FAILED_TEXT) + expect(container.textContent).not.toContain('stale-account') + expect(container.textContent).not.toContain('Edit credentials') + expect(container.textContent).toContain('Add or replace credentials') + expect(container.querySelector('[aria-label="Disconnect Bitbucket"]')).toBeNull() + + await act(async () => { + Array.from(container.querySelectorAll('button')) + .find((button) => button.textContent === 'Add or replace credentials') + ?.click() + }) + expect(container.textContent).toContain('Credential dialog open') + expect(container.textContent).not.toContain('stale@example.com') + }) +}) diff --git a/src/renderer/src/components/settings/bitbucket-integration-card.tsx b/src/renderer/src/components/settings/bitbucket-integration-card.tsx index 5d8809551cb..a2630e31f39 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]) @@ -42,14 +50,18 @@ export function BitbucketIntegrationCard(): React.JSX.Element { void loadConnection() }, [loadConnection]) - const envManaged = connection?.source === 'environment' - const storedCredential = connection?.source === 'stored' - const account = connection?.account ?? statuses.bitbucketAccount + const currentConnection = connectionLoadFailed ? null : connection + const credentialStatusKnown = currentConnection !== null + const envManaged = currentConnection?.source === 'environment' + const storedCredential = currentConnection?.source === 'stored' + const account = currentConnection?.account ?? statuses.bitbucketAccount // Only surface a base URL the user actually overrode; the default is noise. const baseUrlOverride = - connection?.baseUrl && connection.baseUrl !== DEFAULT_API_BASE_URL ? connection.baseUrl : null - const authModeLabel = connection?.authMode - ? connection.authMode === 'token' + currentConnection?.baseUrl && currentConnection.baseUrl !== DEFAULT_API_BASE_URL + ? currentConnection.baseUrl + : null + const authModeLabel = currentConnection?.authMode + ? currentConnection.authMode === 'token' ? translate( 'auto.components.settings.bitbucket.integration.card.authModeToken', 'Access token' @@ -61,7 +73,8 @@ export function BitbucketIntegrationCard(): React.JSX.Element { : null const credentialSummary = [authModeLabel, baseUrlOverride].filter(Boolean).join(' · ') - const handleConnected = (): void => { + // A fresh connection and Re-check both refresh preflight and credential state. + const reloadCardState = (): void => { void loadConnection() refresh() } @@ -76,20 +89,18 @@ 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 { if (mountedRef.current) { setDisconnecting(false) } - void loadConnection() - refresh() + reloadCardState() } } @@ -118,18 +129,26 @@ export function BitbucketIntegrationCard(): React.JSX.Element { statusTone={connected ? 'connected' : 'attention'} statusLabel={tokenProviderStatusLabel({ configured: connected, status })} actions={ - status !== 'checking' && !envManaged ? ( + status !== 'checking' && !envManaged && (credentialStatusKnown || connectionLoadFailed) ? ( ) : null } @@ -170,11 +189,21 @@ 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} + {credentialStatusKnown ? ( + + ) : null}
{!connected ? (
diff --git a/src/renderer/src/components/skills/skills-page-states.tsx b/src/renderer/src/components/skills/skills-page-states.tsx index 1db52379e8f..0a61078e665 100644 --- a/src/renderer/src/components/skills/skills-page-states.tsx +++ b/src/renderer/src/components/skills/skills-page-states.tsx @@ -84,11 +84,11 @@ export function SkillsEmptyState({ } export function SkillsScanErrorBand({ - message, + detail, disabled, onRetry }: { - message: string + detail?: string disabled: boolean onRetry: () => void }): React.JSX.Element { @@ -97,9 +97,18 @@ export function SkillsScanErrorBand({
-

- {message} -

+ {/* Announce the detail with the headline. */} +
+

+ {translate('auto.components.skills.SkillsPage.ea72d6185b', 'Could not scan skills')} +

+ {detail ? ( + // Preserve multi-line git and SSH errors. +

+ {detail} +

+ ) : null} +
diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 4d30c7941a7..ebca3398176 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -11816,7 +11816,9 @@ "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.", + "replaceCredentials": "Add or replace credentials" } } }, @@ -14309,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",