From d55beb2cf06b1296420dd0ad8fe2372e04f891b0 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 28 May 2026 22:21:48 -0700 Subject: [PATCH] Fix final update review edge cases Co-authored-by: Orca --- src/main/updater.test.ts | 104 ++++++++++++++++++ src/main/updater.ts | 33 +++++- .../src/components/UpdateCard.test.ts | 32 +++++- src/renderer/src/components/UpdateCard.tsx | 15 ++- 4 files changed, 172 insertions(+), 12 deletions(-) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index c2e3b87fdc5..e6c69e6f146 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -786,6 +786,69 @@ describe('updater', () => { expect(autoUpdaterMock.downloadUpdate).toHaveBeenCalledTimes(1) }) + it('does not launch a manual check while an automatic download is active before progress arrives', async () => { + vi.stubGlobal('process', { ...process, platform: 'linux' }) + autoUpdaterMock.checkForUpdates.mockResolvedValue(undefined) + autoUpdaterMock.downloadUpdate.mockImplementation(() => new Promise(() => {})) + + const sendMock = vi.fn() + const mainWindow = { webContents: { send: sendMock } } + + const { setupAutoUpdater, checkForUpdatesFromMenu } = await import('./updater') + + setupAutoUpdater(mainWindow as never, { getLastUpdateCheckAt: () => Date.now() }) + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-available', { version: '1.0.61' }) + + await vi.waitFor(() => { + expect(autoUpdaterMock.downloadUpdate).toHaveBeenCalledTimes(1) + }) + + sendMock.mockClear() + checkForUpdatesFromMenu() + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(autoUpdaterMock.checkForUpdates).not.toHaveBeenCalled() + expect(sendMock).toHaveBeenCalledWith('updater:status', { + state: 'checking', + userInitiated: true + }) + + autoUpdaterMock.emit('update-downloaded', { version: '1.0.61' }) + + expect(sendMock).toHaveBeenCalledWith('updater:status', { + state: 'downloaded', + version: '1.0.61', + releaseUrl: undefined, + userInitiated: true + }) + }) + + it('does not launch a focus-triggered check while an automatic download is active before progress arrives', async () => { + let lastUpdateCheckAt = Date.now() + autoUpdaterMock.downloadUpdate.mockImplementation(() => new Promise(() => {})) + + const mainWindow = { webContents: { send: vi.fn() } } + + const { setupAutoUpdater } = await import('./updater') + + setupAutoUpdater(mainWindow as never, { + getLastUpdateCheckAt: () => lastUpdateCheckAt + }) + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-available', { version: '1.0.61' }) + + await vi.waitFor(() => { + expect(autoUpdaterMock.downloadUpdate).toHaveBeenCalledTimes(1) + }) + + lastUpdateCheckAt = Date.now() - 25 * 60 * 60 * 1000 + appMock.emit('browser-window-focus') + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(autoUpdaterMock.checkForUpdates).not.toHaveBeenCalled() + }) + it('does not start the same active background download again after progress arrives', async () => { autoUpdaterMock.checkForUpdates.mockImplementation(() => { autoUpdaterMock.emit('checking-for-update') @@ -824,6 +887,7 @@ describe('updater', () => { }) return Promise.resolve(undefined) }) + autoUpdaterMock.downloadUpdate.mockImplementation(() => new Promise(() => {})) const sendMock = vi.fn() const mainWindow = { webContents: { send: sendMock } } @@ -1179,6 +1243,46 @@ describe('updater', () => { }) }) + it('keeps a queued replacement across a later checking event before the older download finishes', async () => { + vi.stubGlobal('process', { ...process, platform: 'linux' }) + autoUpdaterMock.downloadUpdate.mockImplementation(() => new Promise(() => {})) + + const sendMock = vi.fn() + const mainWindow = { webContents: { send: sendMock } } + + const { setupAutoUpdater } = await import('./updater') + + setupAutoUpdater(mainWindow as never, { getLastUpdateCheckAt: () => Date.now() }) + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-available', { version: '1.0.61' }) + + await vi.waitFor(() => { + expect(autoUpdaterMock.downloadUpdate).toHaveBeenCalledTimes(1) + }) + + autoUpdaterMock.emit('download-progress', { percent: 10 }) + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-available', { version: '1.0.62' }) + + await vi.waitFor(() => { + expect(sendMock).toHaveBeenCalledWith('updater:status', { + state: 'available', + version: '1.0.62', + changelog: null + }) + }) + + sendMock.mockClear() + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-downloaded', { version: '1.0.61' }) + + expect(autoUpdaterMock.downloadUpdate).toHaveBeenCalledTimes(2) + expect(sendMock).not.toHaveBeenCalledWith( + 'updater:status', + expect.objectContaining({ state: 'downloaded', version: '1.0.61' }) + ) + }) + it('queues a newer replacement while an older download is still active', async () => { vi.stubGlobal('process', { ...process, platform: 'linux' }) autoUpdaterMock.downloadUpdate.mockImplementation(() => new Promise(() => {})) diff --git a/src/main/updater.ts b/src/main/updater.ts index 1aca0025566..7e566c5c0b4 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -104,7 +104,6 @@ function getAutoUpdater(): ElectronAutoUpdater { function clearAvailableUpdateContext(): void { availableVersion = null availableReleaseUrl = null - pendingReplacementDownloadVersion = null } function clearStagedUpdateContext(): void { @@ -306,6 +305,14 @@ function getPendingDownloadVersion(): string | null { ) { return currentStatus.version } + // Why: a follow-up freshness check can clear availableVersion while an older + // artifact is still finishing; the queued replacement remains the target. + if ( + pendingReplacementDownloadVersion && + compareVersions(pendingReplacementDownloadVersion, app.getVersion()) > 0 + ) { + return pendingReplacementDownloadVersion + } return null } @@ -706,7 +713,7 @@ function retryPrereleaseFallbackAfterMissingManifest( function runBackgroundUpdateCheck( nudgeId: string | null = getPersistedPendingUpdateNudgeId() ): void { - if (backgroundCheckLaunchPending || currentStatus.state === 'checking') { + if (backgroundCheckLaunchPending || currentStatus.state === 'checking' || activeDownloadVersion) { return } if (!app.isPackaged || is.dev) { @@ -785,7 +792,8 @@ export function checkForUpdatesFromMenu(options?: { includePrerelease?: boolean enableIncludePrerelease() } - const checkAlreadyInFlight = backgroundCheckLaunchPending || currentStatus.state === 'checking' + const checkAlreadyInFlight = + backgroundCheckLaunchPending || currentStatus.state === 'checking' || activeDownloadVersion userInitiatedCheck = true // Why: a manual check is independent of any active nudge campaign. Reset the // nudge marker so the resulting status is not decorated with activeNudgeId, @@ -859,7 +867,11 @@ async function checkForUpdateNudge(): Promise { return } - if (currentStatus.state === 'checking' || currentStatus.state === 'downloading') { + if ( + currentStatus.state === 'checking' || + currentStatus.state === 'downloading' || + activeDownloadVersion + ) { return } @@ -1034,7 +1046,8 @@ export function setupAutoUpdater( if ( backgroundCheckLaunchPending || currentStatus.state === 'checking' || - currentStatus.state === 'downloading' + currentStatus.state === 'downloading' || + activeDownloadVersion ) { return } @@ -1070,7 +1083,11 @@ function startAvailableUpdateDownload(): void { return } if (activeDownloadVersion) { - if (compareVersions(targetVersion, activeDownloadVersion) > 0) { + if ( + compareVersions(targetVersion, activeDownloadVersion) > 0 && + (!pendingReplacementDownloadVersion || + compareVersions(targetVersion, pendingReplacementDownloadVersion) >= 0) + ) { pendingReplacementDownloadVersion = targetVersion } return @@ -1094,6 +1111,10 @@ function startAvailableUpdateDownload(): void { beginMacUpdateDownload() getAutoUpdater() .downloadUpdate() + .then(() => { + clearActiveUpdateDownload(targetVersion) + startPendingReplacementDownload() + }) .catch((err) => { const message = String(err?.message ?? err) const existingMatchingErrorUserInitiated = diff --git a/src/renderer/src/components/UpdateCard.test.ts b/src/renderer/src/components/UpdateCard.test.ts index f0a2c9cb3b6..08b7c2bc8dd 100644 --- a/src/renderer/src/components/UpdateCard.test.ts +++ b/src/renderer/src/components/UpdateCard.test.ts @@ -355,6 +355,8 @@ function computeVisibility(input: VisibilityInput): VisibilityResult { const isNudgeDriven = 'activeNudgeId' in status && Boolean(status.activeNudgeId) const hasExplicitDownloadIntent = cachedVersion !== null && downloadIntentVersion === cachedVersion + const isLocallyDismissedVersion = + cachedVersion !== null && locallyDismissedVersion === cachedVersion const shouldShowDetailedErrorCard = status.state === 'error' && (isUserInitiated || isNudgeDriven || hasStartedDownload || hasExplicitDownloadIntent) @@ -381,17 +383,26 @@ function computeVisibility(input: VisibilityInput): VisibilityResult { ) { return 'hidden' } + if ( + status.state === 'downloading' && + isLocallyDismissedVersion && + !hasStartedDownload && + !hasExplicitDownloadIntent && + !isNudgeDriven + ) { + return 'hidden' + } if (status.state === 'error' && !shouldShowDetailedErrorCard && !isUserInitiated) { return 'hidden' } const effectiveVersion = 'version' in status ? status.version : cachedVersion - const isLocallyDismissedVersion = + const isEffectiveLocallyDismissedVersion = effectiveVersion !== null && locallyDismissedVersion === effectiveVersion if ( effectiveVersion && - (dismissedVersion === effectiveVersion || isLocallyDismissedVersion) && - (isLocallyDismissedVersion || + (dismissedVersion === effectiveVersion || isEffectiveLocallyDismissedVersion) && + (isEffectiveLocallyDismissedVersion || (!userInitiatedCycle && !isUserInitiated && !hasExplicitDownloadIntent && !isNudgeDriven)) ) { if ( @@ -592,6 +603,18 @@ describe('UpdateCard visibility gates', () => { ).toBe('visible') }) + it('hides user-initiated download progress after the visible available card is dismissed', () => { + expect( + computeVisibility({ + status: { state: 'downloading', percent: 42, version: '1.2.0', userInitiated: true }, + dismissedVersion: '1.2.0', + cachedVersion: '1.2.0', + hasStartedDownload: false, + locallyDismissedVersion: '1.2.0' + }) + ).toBe('hidden') + }) + it('shows settings-initiated download progress even when the version was dismissed', () => { expect( computeVisibility({ @@ -599,7 +622,8 @@ describe('UpdateCard visibility gates', () => { dismissedVersion: '1.2.0', cachedVersion: '1.2.0', hasStartedDownload: false, - downloadIntentVersion: '1.2.0' + downloadIntentVersion: '1.2.0', + locallyDismissedVersion: '1.2.0' }) ).toBe('visible') }) diff --git a/src/renderer/src/components/UpdateCard.tsx b/src/renderer/src/components/UpdateCard.tsx index 0d85a35773d..168612a4df5 100644 --- a/src/renderer/src/components/UpdateCard.tsx +++ b/src/renderer/src/components/UpdateCard.tsx @@ -217,6 +217,8 @@ export function UpdateCard() { const cachedVersion = versionRef.current const hasExplicitDownloadIntent = cachedVersion !== null && downloadIntentVersion === cachedVersion + const isLocallyDismissedVersion = + cachedVersion !== null && locallyDismissedVersionRef.current === cachedVersion const shouldShowDetailedErrorCard = status.state === 'error' && (isUserInitiated || isNudgeDriven || hasStartedDownload.current || hasExplicitDownloadIntent) @@ -271,6 +273,17 @@ export function UpdateCard() { ) { return null } + // Why: closing the available card is also a request to keep the automatic + // pre-download quiet; explicit downloads and nudge campaigns still surface. + if ( + status.state === 'downloading' && + isLocallyDismissedVersion && + !hasStartedDownload.current && + !hasExplicitDownloadIntent && + !isNudgeDriven + ) { + return null + } // Error: show card for user-initiated check failures or for failures tied to // a concrete cached update version (card-initiated and Settings-initiated // download/install flows). Background check failures stay silent. @@ -291,8 +304,6 @@ export function UpdateCard() { // Why: bypass the gate when the current cycle was user-initiated — the user // explicitly asked to check, so they expect to see the result even if they // dismissed the same version earlier. - const isLocallyDismissedVersion = - versionRef.current !== null && locallyDismissedVersionRef.current === versionRef.current if ( versionRef.current && (dismissedVersion === versionRef.current || isLocallyDismissedVersion) &&