From 215bdbd6b014464ba4655dfc1d37247b2a87c558 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:35:16 -0700 Subject: [PATCH] fix(browser): settle client-hosted download toasts after their page closes (#28913) * fix(browser): settle client-hosted download toasts after their page closes The download progress toast for a page this desktop hosts for a remote workspace was driven by a listener inside that page's pane. Switching pages or closing the tab unmounted the listener mid-download, so the finish never reached the toast and its spinner (no close button, no timeout) stayed for the session. An interrupted download had the same result because Chromium sends no finish for it. Own the toasts from one app-level listener that claims a download when it starts, and end the spinner on an interrupt. * test(browser): type the store stub without an assertion * fix(browser): wait for download finish before settling notice --- .../src/app-shell/use-app-shell-services.ts | 2 + .../ClientHostedBrowserPagePane.tsx | 2 - .../browser-workspace-pane.tsx | 11 +- ...r-client-hosted-download-notices.test.tsx} | 105 +++++++----------- .../browser-client-hosted-download-notices.ts | 26 +++-- .../client-hosted-browser-pane-test-rig.ts | 3 - src/renderer/src/store/slices/browser.ts | 5 +- .../slices/browser/browser-host-state.ts | 10 ++ 8 files changed, 77 insertions(+), 87 deletions(-) rename src/renderer/src/components/browser-pane/{ClientHostedBrowserPagePane.download-notices.test.tsx => browser-client-hosted-download-notices.test.tsx} (62%) diff --git a/src/renderer/src/app-shell/use-app-shell-services.ts b/src/renderer/src/app-shell/use-app-shell-services.ts index f0cd00ab6dd..c3ad37a2692 100644 --- a/src/renderer/src/app-shell/use-app-shell-services.ts +++ b/src/renderer/src/app-shell/use-app-shell-services.ts @@ -21,6 +21,7 @@ import { useHostModelCatalogSnapshotsSync } from '../runtime/host-model-catalog- import { useRemoteRuntimeRecoveryTriggers } from '../runtime/use-remote-runtime-recovery-triggers' import { useTerminalViewerColorPublication } from './use-terminal-viewer-color-publication' import { useBrowserIdentityMigrationNotice } from '../components/browser-pane/browser-user-agent-migration-notice' +import { useBrowserClientHostedDownloadNotices } from '../components/browser-pane/browser-client-hosted-download-notices' import { useCodexTerminalServerIsolationNotice } from '../components/terminal-pane/codex-terminal-server-isolation-notice' import { useCodexSharedSettingsNotice } from '../components/terminal-pane/codex-shared-settings-notice' import { useVisibleReviewRefreshReporting } from './use-visible-review-refresh-reporting' @@ -64,6 +65,7 @@ export function useAppShellServices(): void { usePrimarySelectionPaste(primarySelectionMiddleClickPaste) useOsc52ClipboardDefaultOnNotice(persistedUIReady) useBrowserIdentityMigrationNotice() + useBrowserClientHostedDownloadNotices() useCodexTerminalServerIsolationNotice() useCodexSharedSettingsNotice() } diff --git a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx index a956a35a642..b1c49a502be 100644 --- a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx +++ b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx @@ -16,7 +16,6 @@ import { startBrowserClientPageMetadataPublisher } from './browser-client-page-metadata-reporting' import { attachBrowserClientPageToViewport } from './browser-client-page-renderer-installation' -import { useBrowserClientHostedDownloadNotices } from './browser-client-hosted-download-notices' import { useBrowserClientHostedPopupNotices } from './browser-client-hosted-popup-notices' import { useBrowserClientHostedPermissionNotices } from './browser-client-hosted-permission-notices' import { useClientHostedBrowserIntroTour } from './use-client-hosted-browser-intro-tour' @@ -139,7 +138,6 @@ export function ClientHostedBrowserPagePane({ onUpdatePageStateRef }) - useBrowserClientHostedDownloadNotices(browserTab.id) useBrowserClientHostedPopupNotices(browserTab.id) useBrowserClientHostedPermissionNotices(browserTab.id) // Why: the tour points at controls that cannot work yet, and recording the interaction is a diff --git a/src/renderer/src/components/browser-pane/assemble-chrome/browser-workspace-pane.tsx b/src/renderer/src/components/browser-pane/assemble-chrome/browser-workspace-pane.tsx index e473f442a90..6035ef1f486 100644 --- a/src/renderer/src/components/browser-pane/assemble-chrome/browser-workspace-pane.tsx +++ b/src/renderer/src/components/browser-pane/assemble-chrome/browser-workspace-pane.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useMemo } from 'react' import { useAppStore } from '@/store' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' +import { isClientHostedBrowserPageHandle } from '@/store/slices/browser' import type { BrowserWorkspace as BrowserWorkspaceState } from '../../../../../shared/browser-workspace-types' import { destroyPersistentWebview } from '../host-guest/webview-registry' import { useBrowserAutomationVisiblePageIds } from '../host-guest/browser-automation-visibility' @@ -118,14 +119,14 @@ export default function BrowserPane({ // first frame. The host mints the placement, so keying on it would swap component and key at // adoption — a remount that replays open dropdowns and throws away focus and drafts. The pane // renders connecting until adoption fills the placement in. - const stagedClientHosted = - environmentHandle?.staged === true && environmentHandle.stagedClientHosted === true - // Why: a row restored from a previous run has no placement until the relaunched host recovers + // A row restored from a previous run has no placement until the relaunched host recovers // the page, and the streamed pane would meanwhile open a server screencast the host refuses // for a client-placed page. It mounts quiet on the client pane until adoption fills it in. - const restoredClientHosted = environmentHandle?.restoredClientHosted === true + const clientHosted = environmentHandle + ? isClientHostedBrowserPageHandle(environmentHandle) + : false return activeBrowserPage ? ( - clientPlacement || stagedClientHosted || restoredClientHosted ? ( + clientHosted ? ( ({ @@ -10,14 +10,19 @@ const toastMocks = vi.hoisted(() => ({ vi.mock('sonner', () => ({ toast: toastMocks })) +const store = vi.hoisted(() => { + const remoteBrowserPageHandlesByPageId: Record = {} + return { state: { remoteBrowserPageHandlesByPageId } } +}) +vi.mock('@/store', () => ({ useAppStore: { getState: () => store.state } })) + import type { BrowserDownloadFinishedEvent, BrowserDownloadProgressEvent, BrowserDownloadRequestedEvent } from '../../../../shared/browser-guest-events' -import { TooltipProvider } from '@/components/ui/tooltip' -import { installClientHostedPaneApi, paneChannel } from './client-hosted-browser-pane-test-rig' -import { ClientHostedBrowserPagePane } from './ClientHostedBrowserPagePane' +import { paneChannel } from './client-hosted-browser-pane-test-rig' +import { useBrowserClientHostedDownloadNotices } from './browser-client-hosted-download-notices' let requested = paneChannel() let progress = paneChannel() @@ -27,12 +32,21 @@ beforeEach(() => { requested = paneChannel() progress = paneChannel() finished = paneChannel() - // Only the preload boundary is faked: the pane subscribes through the real window.api surface. - installClientHostedPaneApi({ - browser: { - onDownloadRequested: requested.subscribe, - onDownloadProgress: progress.subscribe, - onDownloadFinished: finished.subscribe + store.state.remoteBrowserPageHandlesByPageId = { + 'page-a': { + environmentId: 'environment-a', + remotePageId: 'remote-page-a', + placement: { kind: 'client' } + } + } + Object.defineProperty(window, 'api', { + configurable: true, + value: { + browser: { + onDownloadRequested: requested.subscribe, + onDownloadProgress: progress.subscribe, + onDownloadFinished: finished.subscribe + } } }) }) @@ -42,36 +56,8 @@ afterEach(() => { vi.clearAllMocks() }) -function renderPane(browserPageId = 'page-a'): void { - render( - - - - ) +function renderNotices(): void { + renderHook(useBrowserClientHostedDownloadNotices) } function emitRequested(overrides: Partial = {}): void { @@ -112,9 +98,9 @@ function emitProgress(overrides: Partial = {}): vo act(() => progress.emit(event)) } -describe('ClientHostedBrowserPagePane download notices', () => { +describe('client-hosted browser download notices', () => { it('names the remote workspace destination when the download lands there', () => { - renderPane() + renderNotices() emitRequested() expect(toastMocks.loading).toHaveBeenCalledWith('Downloading report.pdf…', { @@ -135,7 +121,7 @@ describe('ClientHostedBrowserPagePane download notices', () => { }) it('surfaces the fail-closed remote error instead of dropping it', () => { - renderPane() + renderNotices() emitRequested() emitFinished({ @@ -149,9 +135,12 @@ describe('ClientHostedBrowserPagePane download notices', () => { ) }) - it('reports a canceled download rather than leaving the spinner running', () => { - renderPane() + // Why: the page's pane unmounts on a page switch or close while its download keeps running, and + // an unsettled loading toast has no close button. + it('settles the toast after the page that started the download is gone', () => { + renderNotices() emitRequested() + store.state.remoteBrowserPageHandlesByPageId = {} emitFinished({ status: 'canceled' }) @@ -160,11 +149,11 @@ describe('ClientHostedBrowserPagePane download notices', () => { }) }) - it('ignores downloads belonging to another page', () => { - renderPane('page-a') + it('leaves downloads from pages this desktop does not host to their own shelf', () => { + renderNotices() - emitRequested({ browserPageId: 'page-b', downloadId: 'download-2' }) - emitFinished({ browserPageId: 'page-b', downloadId: 'download-2' }) + emitRequested({ browserPageId: 'local-page', downloadId: 'download-2' }) + emitFinished({ browserPageId: 'local-page', downloadId: 'download-2' }) expect(toastMocks.loading).not.toHaveBeenCalled() expect(toastMocks.success).not.toHaveBeenCalled() @@ -173,7 +162,7 @@ describe('ClientHostedBrowserPagePane download notices', () => { // Why: bytes for a client-hosted page land on the remote workspace, so this toast is the only // place a long download shows any movement at all. it('moves the toast forward as bytes arrive', () => { - renderPane() + renderNotices() emitRequested() toastMocks.loading.mockClear() @@ -184,20 +173,8 @@ describe('ClientHostedBrowserPagePane download notices', () => { }) }) - it('keeps the plain line when the server never reported a size', () => { - renderPane() - emitRequested() - toastMocks.loading.mockClear() - - emitProgress({ totalBytes: null }) - - expect(toastMocks.loading).toHaveBeenCalledWith('Downloading report.pdf… 2.1 MB', { - id: 'browser-download:download-1' - }) - }) - - it('ignores progress for a download this pane never started', () => { - renderPane() + it('ignores progress for a download it never claimed', () => { + renderNotices() toastMocks.loading.mockClear() emitProgress() diff --git a/src/renderer/src/components/browser-pane/browser-client-hosted-download-notices.ts b/src/renderer/src/components/browser-pane/browser-client-hosted-download-notices.ts index 081549d5198..83c256ec166 100644 --- a/src/renderer/src/components/browser-pane/browser-client-hosted-download-notices.ts +++ b/src/renderer/src/components/browser-pane/browser-client-hosted-download-notices.ts @@ -1,6 +1,8 @@ -import { useEffect, useRef } from 'react' +import { useEffect } from 'react' import { toast } from 'sonner' import { translate } from '@/i18n/i18n' +import { useAppStore } from '@/store' +import { isClientHostedBrowserPageHandle } from '@/store/slices/browser' import type { BrowserDownloadFinishedEvent, BrowserDownloadProgressEvent, @@ -18,15 +20,18 @@ function downloadToastId(downloadId: string): string { * A client-hosted page has no download shelf and its bytes never land in this desktop's Downloads * folder, so these toasts are the only signal that a download started, landed on the remote * workspace, or was refused. + * + * Why app-level: the page's pane unmounts on a page switch or close while the download keeps + * running, and a loading toast whose finish nobody hears has no close button and never expires. */ -export function useBrowserClientHostedDownloadNotices(browserPageId: string): void { - const filenamesRef = useRef>(new Map()) - +export function useBrowserClientHostedDownloadNotices(): void { useEffect(() => { - const filenames = filenamesRef.current + // Claimed at request time: the finish can arrive after the page's handle is gone. + const filenames = new Map() const releaseRequested = window.api.browser.onDownloadRequested( (event: BrowserDownloadRequestedEvent) => { - if (event.browserPageId !== browserPageId) { + const handle = useAppStore.getState().remoteBrowserPageHandlesByPageId[event.browserPageId] + if (!handle || !isClientHostedBrowserPageHandle(handle)) { return } filenames.set(event.downloadId, event.filename) @@ -38,8 +43,6 @@ export function useBrowserClientHostedDownloadNotices(browserPageId: string): vo ) } ) - // Why: progress carries no page id once the guest is gone, so a filename this pane recorded is - // what proves the download is ours — an unknown id belongs to another page and is ignored. const releaseProgress = window.api.browser.onDownloadProgress( (event: BrowserDownloadProgressEvent) => { const filename = filenames.get(event.downloadId) @@ -53,10 +56,10 @@ export function useBrowserClientHostedDownloadNotices(browserPageId: string): vo ) const releaseFinished = window.api.browser.onDownloadFinished( (event: BrowserDownloadFinishedEvent) => { - if (event.browserPageId !== browserPageId) { + const filename = filenames.get(event.downloadId) + if (filename === undefined) { return } - const filename = filenames.get(event.downloadId) ?? '' filenames.delete(event.downloadId) emitBrowserClientHostedDownloadNotice(event, filename) } @@ -65,9 +68,8 @@ export function useBrowserClientHostedDownloadNotices(browserPageId: string): vo releaseRequested() releaseProgress() releaseFinished() - filenames.clear() } - }, [browserPageId]) + }, []) } /** "Downloading report.pdf… 2.1 MB / 8 MB", falling back to the plain line when size is unknown. */ diff --git a/src/renderer/src/components/browser-pane/client-hosted-browser-pane-test-rig.ts b/src/renderer/src/components/browser-pane/client-hosted-browser-pane-test-rig.ts index b5527404c29..d2b357d92dd 100644 --- a/src/renderer/src/components/browser-pane/client-hosted-browser-pane-test-rig.ts +++ b/src/renderer/src/components/browser-pane/client-hosted-browser-pane-test-rig.ts @@ -42,9 +42,6 @@ export function installClientHostedPaneApi(overrides?: { configurable: true, value: { browser: { - onDownloadRequested: inert, - onDownloadProgress: inert, - onDownloadFinished: inert, onPopup: inert, onPermissionDenied: inert, onContextMenuRequested: inert, diff --git a/src/renderer/src/store/slices/browser.ts b/src/renderer/src/store/slices/browser.ts index 424384e9a42..bb83a79f466 100644 --- a/src/renderer/src/store/slices/browser.ts +++ b/src/renderer/src/store/slices/browser.ts @@ -14,5 +14,8 @@ export type { RemoteBrowserPageHandle } from './browser/browser-slice-contract' export { createBrowserSlice } from './browser/create-browser-slice' -export { isLocalBrowserPageOwner } from './browser/browser-host-state' +export { + isClientHostedBrowserPageHandle, + isLocalBrowserPageOwner +} from './browser/browser-host-state' export { sanitizeBrowserPageAnnotation } from './browser/browser-page-annotation' diff --git a/src/renderer/src/store/slices/browser/browser-host-state.ts b/src/renderer/src/store/slices/browser/browser-host-state.ts index 7ee75ac54a1..5c1865aa1f5 100644 --- a/src/renderer/src/store/slices/browser/browser-host-state.ts +++ b/src/renderer/src/store/slices/browser/browser-host-state.ts @@ -50,6 +50,16 @@ export function buildRestoredRemoteBrowserPageHandles( return handles } +/** Whether this desktop hosts the remote page itself rather than streaming it from the host. Staged + * and restored rows count before the host confirms a placement, so they never mount as streams. */ +export function isClientHostedBrowserPageHandle(handle: RemoteBrowserPageHandle): boolean { + return ( + handle.placement?.kind === 'client' || + (handle.staged === true && handle.stagedClientHosted === true) || + handle.restoredClientHosted === true + ) +} + export function retainCookieImportExecutionHost( result: BrowserCookieImportResult, executionHostId: ExecutionHostId,