mirror of
https://github.com/stablyai/orca.git
synced 2026-11-04 11:07:42 +00:00
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
This commit is contained in:
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 ? (
|
||||
<ClientHostedBrowserPagePane
|
||||
key={`${activeBrowserRuntimeEnvironmentId}:${activeBrowserPage.id}`}
|
||||
browserTab={activeBrowserPage}
|
||||
|
||||
+41
-64
@@ -1,5 +1,5 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { act, cleanup, render } from '@testing-library/react'
|
||||
import { act, cleanup, renderHook } from '@testing-library/react'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const toastMocks = vi.hoisted(() => ({
|
||||
@@ -10,14 +10,19 @@ const toastMocks = vi.hoisted(() => ({
|
||||
|
||||
vi.mock('sonner', () => ({ toast: toastMocks }))
|
||||
|
||||
const store = vi.hoisted(() => {
|
||||
const remoteBrowserPageHandlesByPageId: Record<string, unknown> = {}
|
||||
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<BrowserDownloadRequestedEvent>()
|
||||
let progress = paneChannel<BrowserDownloadProgressEvent>()
|
||||
@@ -27,12 +32,21 @@ beforeEach(() => {
|
||||
requested = paneChannel<BrowserDownloadRequestedEvent>()
|
||||
progress = paneChannel<BrowserDownloadProgressEvent>()
|
||||
finished = paneChannel<BrowserDownloadFinishedEvent>()
|
||||
// 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(
|
||||
<TooltipProvider>
|
||||
<ClientHostedBrowserPagePane
|
||||
browserTab={
|
||||
{
|
||||
id: browserPageId,
|
||||
url: 'https://example.internal/reports',
|
||||
title: 'Reports',
|
||||
loading: false,
|
||||
canGoBack: false,
|
||||
canGoForward: false
|
||||
} as never
|
||||
}
|
||||
workspaceId="workspace-a"
|
||||
chromeShortcutScope="focused"
|
||||
runtimeEnvironmentId="environment-a"
|
||||
worktreeId="worktree-a"
|
||||
placement={{
|
||||
kind: 'client',
|
||||
browserHostClientId: 'client-a',
|
||||
browserHostGeneration: 3,
|
||||
pageHostGeneration: 7
|
||||
}}
|
||||
isActive
|
||||
onUpdatePageState={vi.fn()}
|
||||
onSetUrl={vi.fn()}
|
||||
/>
|
||||
</TooltipProvider>
|
||||
)
|
||||
function renderNotices(): void {
|
||||
renderHook(useBrowserClientHostedDownloadNotices)
|
||||
}
|
||||
|
||||
function emitRequested(overrides: Partial<BrowserDownloadRequestedEvent> = {}): void {
|
||||
@@ -112,9 +98,9 @@ function emitProgress(overrides: Partial<BrowserDownloadProgressEvent> = {}): 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()
|
||||
+14
-12
@@ -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<Map<string, string>>(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<string, string>()
|
||||
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. */
|
||||
|
||||
@@ -42,9 +42,6 @@ export function installClientHostedPaneApi(overrides?: {
|
||||
configurable: true,
|
||||
value: {
|
||||
browser: {
|
||||
onDownloadRequested: inert,
|
||||
onDownloadProgress: inert,
|
||||
onDownloadFinished: inert,
|
||||
onPopup: inert,
|
||||
onPermissionDenied: inert,
|
||||
onContextMenuRequested: inert,
|
||||
|
||||
@@ -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'
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user