diff --git a/src/main/updater.check-failure.test.ts b/src/main/updater.check-failure.test.ts index a19c227ce..e12a3bab3 100644 --- a/src/main/updater.check-failure.test.ts +++ b/src/main/updater.check-failure.test.ts @@ -107,7 +107,7 @@ describe('updater check failure handling', () => { vi.unstubAllGlobals() }) - it('treats GitHub release transition errors as idle for user-initiated checks', async () => { + it('surfaces GitHub release transition errors to user-initiated checks', async () => { autoUpdaterMock.checkForUpdates.mockResolvedValueOnce(undefined).mockImplementationOnce(() => { autoUpdaterMock.emit('checking-for-update') queueMicrotask(() => { @@ -127,18 +127,22 @@ describe('updater check failure handling', () => { const statuses = sendMock.mock.calls .filter(([channel]) => channel === 'updater:status') .map(([, status]) => status) - // Why: release transition failures should NOT pretend the user is up to - // date. Sending 'idle' lets the toast controller show an honest - // "currently rolling out" message instead of the misleading "you're on the - // latest version" that auto-dismisses. - expect(statuses).toContainEqual({ state: 'idle' }) + // Why: a user-initiated benign failure must show a visible error. Silently + // sending 'idle' (or 'not-available') makes the button look broken. + expect(statuses).toContainEqual( + expect.objectContaining({ + state: 'error', + userInitiated: true, + message: expect.stringContaining('GitHub may be temporarily unavailable') + }) + ) expect(statuses).not.toContainEqual( expect.objectContaining({ state: 'not-available', userInitiated: true }) ) }) }) - it('treats missing latest-mac.yml during user-initiated checks as idle', async () => { + it('surfaces missing latest-mac.yml to user-initiated checks', async () => { autoUpdaterMock.checkForUpdates.mockResolvedValueOnce(undefined).mockImplementationOnce(() => { autoUpdaterMock.emit('checking-for-update') queueMicrotask(() => { @@ -163,10 +167,43 @@ describe('updater check failure handling', () => { const statuses = sendMock.mock.calls .filter(([channel]) => channel === 'updater:status') .map(([, status]) => status) - expect(statuses).toContainEqual({ state: 'idle' }) + expect(statuses).toContainEqual( + expect.objectContaining({ + state: 'error', + userInitiated: true, + message: expect.stringContaining('GitHub may be temporarily unavailable') + }) + ) expect(statuses).not.toContainEqual( expect.objectContaining({ state: 'not-available', userInitiated: true }) ) }) }) + + it('silently drops background benign failures to idle', async () => { + // Why: background checks must stay quiet; only user-initiated clicks get + // an error card. This prevents noisy nag during a release transition. + autoUpdaterMock.checkForUpdates.mockImplementationOnce(() => { + autoUpdaterMock.emit('checking-for-update') + queueMicrotask(() => { + autoUpdaterMock.emit('error', new Error('Unable to find latest version on GitHub')) + }) + return Promise.reject(new Error('Unable to find latest version on GitHub')) + }) + + const sendMock = vi.fn() + const mainWindow = { webContents: { send: sendMock } } + + const { setupAutoUpdater, checkForUpdates } = await import('./updater') + + setupAutoUpdater(mainWindow as never) + checkForUpdates() + await vi.waitFor(() => { + const statuses = sendMock.mock.calls + .filter(([channel]) => channel === 'updater:status') + .map(([, status]) => status) + expect(statuses).toContainEqual({ state: 'idle' }) + expect(statuses).not.toContainEqual(expect.objectContaining({ state: 'error' })) + }) + }) }) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index 972eefa52..773a84942 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -182,7 +182,7 @@ describe('updater', () => { expect(errorStatuses).toEqual([{ state: 'error', message: 'boom', userInitiated: true }]) }) - it('treats net::ERR_FAILED during checks as a benign idle transition', async () => { + it('surfaces net::ERR_FAILED to user-initiated checks with a friendly message', async () => { autoUpdaterMock.checkForUpdates.mockResolvedValueOnce(undefined).mockImplementationOnce(() => { autoUpdaterMock.emit('checking-for-update') queueMicrotask(() => { @@ -202,7 +202,13 @@ describe('updater', () => { const statuses = sendMock.mock.calls .filter(([channel]) => channel === 'updater:status') .map(([, status]) => status) - expect(statuses).toContainEqual({ state: 'idle' }) + expect(statuses).toContainEqual( + expect.objectContaining({ + state: 'error', + userInitiated: true, + message: expect.stringContaining('GitHub may be temporarily unavailable') + }) + ) }) const statuses = sendMock.mock.calls @@ -210,7 +216,8 @@ describe('updater', () => { .map(([, status]) => status) expect(statuses).toContainEqual({ state: 'checking', userInitiated: true }) - expect(statuses).toContainEqual({ state: 'idle' }) + // Why: the raw electron-updater message is replaced with a user-friendly + // one so we never surface "net::ERR_FAILED" directly to the UI. expect(statuses).not.toContainEqual( expect.objectContaining({ state: 'error', message: 'net::ERR_FAILED' }) ) diff --git a/src/main/updater.ts b/src/main/updater.ts index a2d1cdce1..80b7d1b54 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -202,25 +202,24 @@ async function sendCheckFailureStatus(message: string, userInitiated?: boolean): const handleFailure = async (): Promise => { if (isBenignCheckFailure(message)) { // Why: release transition failures (missing latest.yml while a new - // release is being published) and network blips are transient. The - // previous approach sent 'not-available' for user-initiated checks - // during a release transition, which falsely told the user "you're - // on the latest version" — the toast would flash and auto-dismiss, - // hiding the fact that a newer release is mid-publish. Now all - // benign failures go to 'idle' uniformly: the toast controller - // converts a user-initiated checking→idle transition into an honest - // "currently rolling out" message, and a background retry is - // always scheduled so the update notification arrives once the - // release finishes. + // release is being published) and network blips are transient. Schedule + // a background retry so the notification arrives once the release + // finishes, and intentionally skip persistLastUpdateCheckAt — the check + // didn't truly complete, and recording a timestamp would suppress the + // next startup check. console.warn('[updater] benign check failure:', message) clearAvailableUpdateContext() scheduleAutomaticUpdateCheck(AUTO_UPDATE_RETRY_INTERVAL_MS) - // Why: we intentionally do NOT call persistLastUpdateCheckAt here. - // The check didn't truly complete (the manifest was unreachable due - // to a release transition or network blip), so recording a timestamp - // would suppress the next startup check and delay discovery of the - // new version. - sendStatus({ state: 'idle' }) + if (userInitiated) { + // Why: a user-initiated click expects visible feedback — silently + // dropping to 'idle' makes the button look broken. The card already + // prefixes "Could not check for updates." and Settings prefixes + // "Update check failed.", so the message here only carries the + // actionable cause. + sendErrorStatus('GitHub may be temporarily unavailable. Try again in a minute.', true) + } else { + sendStatus({ state: 'idle' }) + } return } diff --git a/src/renderer/src/components/UpdateCard.tsx b/src/renderer/src/components/UpdateCard.tsx index bbf7f5187..22d31ada8 100644 --- a/src/renderer/src/components/UpdateCard.tsx +++ b/src/renderer/src/components/UpdateCard.tsx @@ -12,9 +12,13 @@ import type { ChangelogData } from '../../../shared/types' // ── Helpers ────────────────────────────────────────────────────────── function releaseUrlForVersion(version: string | null): string { + // Why: when no version is cached (typically a failed check), point at the + // plain releases listing rather than /releases/latest — /latest also breaks + // when GitHub's release API is degraded, and the listing is the most + // reliable manual fallback. return version ? `https://github.com/stablyai/orca/releases/tag/v${version}` - : 'https://github.com/stablyai/orca/releases/latest' + : 'https://github.com/stablyai/orca/releases' } function isAnimatedGif(url: string | undefined): boolean { @@ -312,7 +316,9 @@ export function UpdateCard() { const errorCard: ErrorCardModel | null = status.state === 'error' ? { - title: 'Update Error', + // Why: title is scoped to the operation that failed so check-time + // failures (commonly GitHub-side) don't read as a bug in Orca. + title: cachedVersion ? 'Update Error' : 'Update Check Failed', summary: cachedVersion ? 'Could not complete the update.' : 'Could not check for updates.', diff --git a/src/renderer/src/components/settings/GeneralPane.tsx b/src/renderer/src/components/settings/GeneralPane.tsx index 145b92ebc..2fa2a5ec8 100644 --- a/src/renderer/src/components/settings/GeneralPane.tsx +++ b/src/renderer/src/components/settings/GeneralPane.tsx @@ -1,7 +1,7 @@ /* eslint-disable max-lines -- Why: GeneralPane is the single owner of all general settings UI; splitting individual settings into separate files would scatter related controls without a meaningful abstraction boundary. */ -import { useEffect, useState } from 'react' +import { useEffect, useRef, useState } from 'react' import type { ClaudeRateLimitAccountsState, CodexRateLimitAccountsState, @@ -121,6 +121,30 @@ export function GeneralPane({ settings, updateSettings }: GeneralPaneProps): Rea const searchQuery = useAppStore((s) => s.settingsSearchQuery) const updateStatus = useAppStore((s) => s.updateStatus) const fetchSettings = useAppStore((s) => s.fetchSettings) + // Why: the 'error' variant of UpdateStatus does not carry a `version` field. + // The main process emits `{ state: 'error' }` for both check failures (no + // version known yet) and download/install failures (version was known from + // the preceding 'available'/'downloading'/'downloaded' state). Cache the + // last-known version so the error copy below can distinguish the two cases + // without adding IPC. Mirrors `versionRef` in UpdateCard.tsx. + const updateVersionRef = useRef(null) + if ( + (updateStatus.state === 'available' || + updateStatus.state === 'downloading' || + updateStatus.state === 'downloaded') && + updateStatus.version + ) { + updateVersionRef.current = updateStatus.version + } else if ( + updateStatus.state === 'checking' || + updateStatus.state === 'idle' || + updateStatus.state === 'not-available' + ) { + // Why: a new check cycle has started or completed cleanly. Clear the + // cached version so a subsequent check failure cannot be mis-classified + // as a download failure based on a stale version from a prior cycle. + updateVersionRef.current = null + } const [appVersion, setAppVersion] = useState(null) const [autoSaveDelayDraft, setAutoSaveDelayDraft] = useState( String(settings.editorAutoSaveDelayMs) @@ -1089,7 +1113,15 @@ export function GeneralPane({ settings, updateSettings }: GeneralPaneProps): Rea )} - {updateStatus.state === 'error' && `Update error: ${updateStatus.message}`} + {updateStatus.state === 'error' && + // Why: `{ state: 'error' }` is emitted for both check-time + // failures (no version cached) and download/install failures + // (version cached from a prior 'available'/'downloading'/ + // 'downloaded' state). Label accordingly so a download failure + // isn't mislabeled as a "check" failure. Mirrors UpdateCard.tsx. + (updateVersionRef.current + ? `Update error. ${updateStatus.message}` + : `Update check failed. ${updateStatus.message}`)}