From 64f0f61cf8f95f5809038895eb42e7ccfe9fd905 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 27 Aug 2026 22:02:39 -0400 Subject: [PATCH] Harden document preview lifecycle and permissions --- ...browser-session-partition-policies.test.ts | 58 +++++++++- .../browser-session-partition-policies.ts | 102 ++++++++++-------- .../doc-preview-download-block-notice.test.ts | 21 +++- .../doc-preview-download-block-notice.ts | 3 + src/main/browser/doc-preview-protocol.test.ts | 12 +-- src/main/browser/doc-preview-protocol.ts | 6 +- .../profile-project-session-state.test.ts | 52 +++++++++ .../profile-project-session-transfer.ts | 24 ++++- ...ence-cohort-and-identity-migration.test.ts | 37 ++++++- .../worktree-identity-migration.ts | 31 +++++- .../browser-workspace-doc-location.test.ts | 34 ++++++ src/renderer/src/store/slices/browser.ts | 17 ++- .../worktrees-identity-migration.test.ts | 38 ++++++- .../session/worktree-identity-rename-state.ts | 48 +++++++-- src/shared/browser-page-doc-location.ts | 24 +++++ 15 files changed, 422 insertions(+), 85 deletions(-) diff --git a/src/main/browser/browser-session-partition-policies.test.ts b/src/main/browser/browser-session-partition-policies.test.ts index a3667154c9b..78ce34d95fd 100644 --- a/src/main/browser/browser-session-partition-policies.test.ts +++ b/src/main/browser/browser-session-partition-policies.test.ts @@ -3,7 +3,9 @@ import type { BrowserSessionProfile } from '../../shared/browser-workspace-types const mocks = vi.hoisted(() => ({ handleGuestWillDownload: vi.fn(), - noticeDocPreviewDownloadBlocked: vi.fn() + noticeDocPreviewDownloadBlocked: vi.fn(), + clearBrowserWebAuthnAccessHandlers: vi.fn(), + installBrowserWebAuthnAccessHandlers: vi.fn() })) type WillDownloadListener = ( @@ -88,13 +90,13 @@ vi.mock('./browser-session-user-agent-mode', () => ({ })) vi.mock('./browser-webauthn-access', () => ({ allowsBrowserWebAuthnPermission: () => false, - clearBrowserWebAuthnAccessHandlers: vi.fn(), - installBrowserWebAuthnAccessHandlers: vi.fn() + clearBrowserWebAuthnAccessHandlers: mocks.clearBrowserWebAuthnAccessHandlers, + installBrowserWebAuthnAccessHandlers: mocks.installBrowserWebAuthnAccessHandlers })) type PartitionPolicyInstaller = ( profile: BrowserSessionProfile, - options?: { downloads?: 'route' | 'deny' } + options?: { downloads?: 'route' | 'deny'; permissions?: 'browser' | 'deny' } ) => void // Why imported per test rather than at the top: the installer remembers which partitions it has @@ -183,3 +185,51 @@ describe('partition download policy', () => { expect(mocks.handleGuestWillDownload).toHaveBeenCalledTimes(1) }) }) + +describe('partition permission policy', () => { + it('keeps ordinary browser partitions on the browser permission policy', async () => { + const install = await loadInstaller() + install(profileFor('persist:browsing-1')) + + expect(mocks.installBrowserWebAuthnAccessHandlers).toHaveBeenCalledWith( + sessionsByPartition.get('persist:browsing-1') + ) + expect(mocks.clearBrowserWebAuthnAccessHandlers).not.toHaveBeenCalled() + }) + + it('denies every request and check on a strict partition without WebAuthn handlers', async () => { + const install = await loadInstaller() + install(profileFor('orca-doc-preview'), { permissions: 'deny' }) + const sess = sessionsByPartition.get('orca-doc-preview') + if (!sess) { + throw new Error('Expected the preview session') + } + const requestHandler = sess.setPermissionRequestHandler.mock.calls[0]?.[0] as ( + webContents: Electron.WebContents, + permission: string, + callback: (allowed: boolean) => void + ) => void + const checkHandler = sess.setPermissionCheckHandler.mock.calls[0]?.[0] as ( + webContents: Electron.WebContents, + permission: string + ) => boolean + const displayMediaHandler = sess.setDisplayMediaRequestHandler.mock.calls[0]?.[0] as ( + request: Electron.DisplayMediaRequestHandlerHandlerRequest, + callback: (streams: { video?: Electron.WebFrameMain; audio?: 'loopback' }) => void + ) => void + + for (const permission of ['media', 'clipboard-read', 'notifications', 'fullscreen']) { + let decision: boolean | null = null + requestHandler({} as Electron.WebContents, permission, (allowed) => (decision = allowed)) + expect(decision).toBe(false) + expect(checkHandler({} as Electron.WebContents, permission)).toBe(false) + } + expect(mocks.installBrowserWebAuthnAccessHandlers).not.toHaveBeenCalled() + expect(mocks.clearBrowserWebAuthnAccessHandlers).toHaveBeenCalledWith(sess) + let displayMediaDecision: { video?: Electron.WebFrameMain; audio?: 'loopback' } | null = null + displayMediaHandler({} as Electron.DisplayMediaRequestHandlerHandlerRequest, (decision) => { + displayMediaDecision = decision + }) + expect(displayMediaDecision).toEqual({ video: undefined, audio: undefined }) + }) +}) diff --git a/src/main/browser/browser-session-partition-policies.ts b/src/main/browser/browser-session-partition-policies.ts index cf542665ae4..11f1f741e5e 100644 --- a/src/main/browser/browser-session-partition-policies.ts +++ b/src/main/browser/browser-session-partition-policies.ts @@ -57,10 +57,14 @@ function resolvePermissionNoticeUrl( /** `route` hands the item to the owning page's download flow; `deny` cancels it before it starts. */ export type BrowserPartitionDownloadPolicy = 'route' | 'deny' +export type BrowserPartitionPermissionPolicy = 'browser' | 'deny' export function installBrowserSessionPartitionPolicies( profile: BrowserSessionProfile, - options?: { downloads?: BrowserPartitionDownloadPolicy } + options?: { + downloads?: BrowserPartitionDownloadPolicy + permissions?: BrowserPartitionPermissionPolicy + } ): void { const { partition } = profile const sess = session.fromPartition(partition) @@ -75,57 +79,63 @@ export function installBrowserSessionPartitionPolicies( sess.setUserAgent(cleanUA) setupClientHintsOverride(sess, cleanUA) } - sess.setPermissionRequestHandler((webContents, permission, callback, details) => { - // Why: defer media to macOS TCC; denying at the session layer throws NotAllowedError even after the user granted Camera/Mic to the OS. - if (permission === 'media') { - // Capture before async handling; opaque frames cannot be attributed to a named site. - const rawUrl = resolvePermissionNoticeUrl(webContents, details) - void requestSystemMediaAccess( - details as Electron.MediaAccessPermissionRequest | undefined - ).then( - (granted) => { - if (!granted) { + if (options?.permissions === 'deny') { + sess.setPermissionRequestHandler((_webContents, _permission, callback) => callback(false)) + sess.setPermissionCheckHandler(() => false) + clearBrowserWebAuthnAccessHandlers(sess) + } else { + sess.setPermissionRequestHandler((webContents, permission, callback, details) => { + // Why: defer media to macOS TCC; denying at the session layer throws NotAllowedError even after the user granted Camera/Mic to the OS. + if (permission === 'media') { + // Capture before async handling; opaque frames cannot be attributed to a named site. + const rawUrl = resolvePermissionNoticeUrl(webContents, details) + void requestSystemMediaAccess( + details as Electron.MediaAccessPermissionRequest | undefined + ).then( + (granted) => { + if (!granted) { + browserManager.notifyPermissionDenied({ + guestWebContentsId: webContents.id, + permission, + rawUrl + }) + } + callback(granted) + }, + (error: unknown) => { + console.error('[permissions] Browser media access failed:', error) browserManager.notifyPermissionDenied({ guestWebContentsId: webContents.id, permission, rawUrl }) + callback(false) } - callback(granted) - }, - (error: unknown) => { - console.error('[permissions] Browser media access failed:', error) - browserManager.notifyPermissionDenied({ - guestWebContentsId: webContents.id, - permission, - rawUrl - }) - callback(false) - } - ) - return - } - const allowed = isAutoGrantedBrowserSessionPermission(permission) - if (!allowed) { - const rawUrl = resolvePermissionNoticeUrl(webContents, details) - browserManager.notifyPermissionDenied({ - guestWebContentsId: webContents.id, - permission, - rawUrl - }) - } - callback(allowed) - }) - sess.setPermissionCheckHandler((_webContents, permission, _origin, details) => { - if (permission === 'media') { - return hasSystemMediaAccess(details?.mediaType) - } - if (allowsBrowserWebAuthnPermission(permission, details)) { - return true - } - return isAutoGrantedBrowserSessionPermission(permission) - }) - installBrowserWebAuthnAccessHandlers(sess) + ) + return + } + const allowed = isAutoGrantedBrowserSessionPermission(permission) + if (!allowed) { + const rawUrl = resolvePermissionNoticeUrl(webContents, details) + browserManager.notifyPermissionDenied({ + guestWebContentsId: webContents.id, + permission, + rawUrl + }) + } + callback(allowed) + }) + sess.setPermissionCheckHandler((_webContents, permission, _origin, details) => { + if (permission === 'media') { + return hasSystemMediaAccess(details?.mediaType) + } + if (allowsBrowserWebAuthnPermission(permission, details)) { + return true + } + return isAutoGrantedBrowserSessionPermission(permission) + }) + installBrowserWebAuthnAccessHandlers(sess) + } sess.setDisplayMediaRequestHandler((_request, callback) => { callback({ video: undefined, audio: undefined }) }) diff --git a/src/main/browser/doc-preview-download-block-notice.test.ts b/src/main/browser/doc-preview-download-block-notice.test.ts index 9fc4396b329..c57998ea4e5 100644 --- a/src/main/browser/doc-preview-download-block-notice.test.ts +++ b/src/main/browser/doc-preview-download-block-notice.test.ts @@ -2,7 +2,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ publishDocPreviewFailure: vi.fn(), - boundGrantIdByGuest: new Map() + boundGrantIdByGuest: new Map(), + revocationListener: null as null | ((grant: { id: string }) => void) })) vi.mock('./doc-preview-failure-notice', () => ({ @@ -11,6 +12,12 @@ vi.mock('./doc-preview-failure-notice', () => ({ vi.mock('./doc-preview-guest-policy', () => ({ readDocPreviewGuestBoundGrantId: (guest: object) => mocks.boundGrantIdByGuest.get(guest) ?? null })) +vi.mock('./doc-preview-grant-registry', () => ({ + onDocPreviewGrantRevoked: (listener: (grant: { id: string }) => void) => { + mocks.revocationListener = listener + return vi.fn() + } +})) const GRANT_ID = 'a'.repeat(32) const OTHER_GRANT_ID = 'b'.repeat(32) @@ -32,6 +39,7 @@ async function loadNotifier(): Promise<(guest: Electron.WebContents) => void> { beforeEach(() => { vi.clearAllMocks() mocks.boundGrantIdByGuest.clear() + mocks.revocationListener = null // Why per test: the module remembers which grants it has already told the reader about. vi.resetModules() vi.useFakeTimers() @@ -94,6 +102,17 @@ describe('noticeDocPreviewDownloadBlocked', () => { }) }) + it('forgets a preview as soon as its grant is revoked', async () => { + const notice = await loadNotifier() + const guest = guestBoundTo(GRANT_ID) + + notice(guest) + mocks.revocationListener?.({ id: GRANT_ID }) + notice(guest) + + expect(mocks.publishDocPreviewFailure).toHaveBeenCalledTimes(2) + }) + // The absence half of the first test: no shell is showing this contents, so there is no preview // to put a notice on. Without the presence tests above, this would pass on a module that never // published anything at all. diff --git a/src/main/browser/doc-preview-download-block-notice.ts b/src/main/browser/doc-preview-download-block-notice.ts index 2aefb2824a7..13cebef49b7 100644 --- a/src/main/browser/doc-preview-download-block-notice.ts +++ b/src/main/browser/doc-preview-download-block-notice.ts @@ -1,4 +1,5 @@ import { publishDocPreviewFailure } from './doc-preview-failure-notice' +import { onDocPreviewGrantRevoked } from './doc-preview-grant-registry' import { readDocPreviewGuestBoundGrantId } from './doc-preview-guest-policy' /** @@ -12,6 +13,8 @@ import { readDocPreviewGuestBoundGrantId } from './doc-preview-guest-policy' const NOTICE_MIN_INTERVAL_MS = 2_000 const noticedAtByGrantId = new Map() +onDocPreviewGrantRevoked((grant) => noticedAtByGrantId.delete(grant.id)) + export function noticeDocPreviewDownloadBlocked(guest: Electron.WebContents): void { const grantId = readDocPreviewGuestBoundGrantId(guest) // Nothing to route a notice to: no shell is showing this contents as a preview. diff --git a/src/main/browser/doc-preview-protocol.test.ts b/src/main/browser/doc-preview-protocol.test.ts index b87e7f9e942..32700be32fd 100644 --- a/src/main/browser/doc-preview-protocol.test.ts +++ b/src/main/browser/doc-preview-protocol.test.ts @@ -261,8 +261,8 @@ describe('installDocPreviewProtocolHandler', () => { expect(cancelled(`orca-preview://${'a'.repeat(32)}/index.html`)).toBe(false) }) - // Why: preview guests are webviews like any other, so they must not skip the deny-by-default - // permission and display-media policy every browser partition gets. + // Why: preview guests still use the shared installer for certificate, UA, permission and + // download hooks, with a stricter decision than ordinary browsing partitions. it('applies the shared browser partition policies to the preview session', () => { installDocPreviewProtocolHandler() @@ -272,14 +272,12 @@ describe('installDocPreviewProtocolHandler', () => { ) }) - // Why downloads and nothing else: the browser download flow attributes a file to the page that - // asked for it, and a previewed document is no page. Routed, it would write remote-authored bytes - // into this desktop's Downloads folder with no prompt and no tab to name as the source. - it('asks for downloads to be denied on the preview partition', () => { + it('denies downloads and ambient browser permissions on the preview partition', () => { installDocPreviewProtocolHandler() expect(mocks.installBrowserSessionPartitionPolicies).toHaveBeenCalledWith(expect.anything(), { - downloads: 'deny' + downloads: 'deny', + permissions: 'deny' }) }) }) diff --git a/src/main/browser/doc-preview-protocol.ts b/src/main/browser/doc-preview-protocol.ts index 7ed1507c5b8..e5d2c906f54 100644 --- a/src/main/browser/doc-preview-protocol.ts +++ b/src/main/browser/doc-preview-protocol.ts @@ -124,8 +124,8 @@ export function installDocPreviewProtocolHandler(): void { previewSession.webRequest.onBeforeRequest((details, callback) => { callback({ cancel: !isAllowedDocPreviewRequestUrl(details.url) }) }) - // Why: preview guests are webviews like any other, so they inherit the same deny-by-default - // permission, display-media and user-agent policy every browser partition gets. + // Why: the shared installer still owns certificate, UA, download and permission hooks, while + // preview content receives no ambient browser/device permissions. installBrowserSessionPartitionPolicies( { id: DOC_PREVIEW_PARTITION, @@ -139,6 +139,6 @@ export function installDocPreviewProtocolHandler(): void { // page to attribute the file to, and a previewed document is not one. Routed here it would // reserve a name in this desktop's Downloads folder and write remote-authored bytes into it // with nothing in the UI naming the tab that asked, and no prompt in front of it. - { downloads: 'deny' } + { downloads: 'deny', permissions: 'deny' } ) } diff --git a/src/main/orca-profiles/profile-project-session-state.test.ts b/src/main/orca-profiles/profile-project-session-state.test.ts index c60622ac9ba..1d9a64870c3 100644 --- a/src/main/orca-profiles/profile-project-session-state.test.ts +++ b/src/main/orca-profiles/profile-project-session-state.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { getDefaultWorkspaceSession } from '../../shared/constants' +import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' import { mergeWorkspaceSessions, removeRepoFromWorkspaceSession @@ -84,6 +85,57 @@ describe('profile project session state', () => { ) }) + it('rekeys document-preview ownership during project transfer', () => { + const session = { + ...getDefaultWorkspaceSession(), + browserTabsByWorktree: { + [REMOVED_WORKTREE_ID]: [ + { + id: 'browser-1', + worktreeId: REMOVED_WORKTREE_ID, + docLocation: { + kind: 'workspace-doc', + worktreeId: REMOVED_WORKTREE_ID, + filePath: '/removed/docs/report.html' + } + } + ] + }, + browserPagesByWorkspace: { + 'browser-1': [ + { + id: 'page-1', + workspaceId: 'browser-1', + worktreeId: REMOVED_WORKTREE_ID, + docLocation: { + kind: 'workspace-doc', + worktreeId: REMOVED_WORKTREE_ID, + filePath: '/removed/docs/report.html' + } + } + ] + } + } + + const result = extractSessionForTransfer( + session as unknown as WorkspaceSessionState, + REMOVED_REPO_ID, + 'repo-c' + ) + const transferredWorktreeId = 'repo-c::/removed' + + expect(result.browserTabsByWorktree?.[transferredWorktreeId]?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: transferredWorktreeId, + filePath: '/removed/docs/report.html' + }) + expect(result.browserPagesByWorkspace?.['browser-1']?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: transferredWorktreeId, + filePath: '/removed/docs/report.html' + }) + }) + it('prunes terminal membership authority records with a removed repo', () => { const session = { ...getDefaultWorkspaceSession(), diff --git a/src/main/orca-profiles/profile-project-session-transfer.ts b/src/main/orca-profiles/profile-project-session-transfer.ts index 0cf15a02ef2..44f83680b40 100644 --- a/src/main/orca-profiles/profile-project-session-transfer.ts +++ b/src/main/orca-profiles/profile-project-session-transfer.ts @@ -1,6 +1,11 @@ import { getDefaultWorkspaceSession } from '../../shared/constants' import type { ExecutionHostId } from '../../shared/execution-host' -import type { BrowserPage, BrowserWorkspace } from '../../shared/browser-workspace-types' +import type { + BrowserPage, + BrowserPageDocLocation, + BrowserWorkspace +} from '../../shared/browser-workspace-types' +import { remapBrowserPageDocLocation } from '../../shared/browser-page-doc-location' import type { Tab, TabGroup } from '../../shared/tab-types' import type { TerminalTab } from '../../shared/terminal-tab-types' import type { @@ -175,6 +180,9 @@ function rekeyBrowserWorkspace( return { ...structuredClone(workspace), worktreeId: rekeyWorktreeId(oldRepoId, newRepoId, workspace.worktreeId), + ...(workspace.docLocation + ? { docLocation: rekeyBrowserDocLocation(workspace.docLocation, oldRepoId, newRepoId) } + : {}), // Why: both the session profile and the resolved partition string are // source-profile-scoped; carrying either across would point the restored // pane at a partition the target profile's allowlist rejects. @@ -186,10 +194,22 @@ function rekeyBrowserWorkspace( function rekeyBrowserPage(page: BrowserPage, oldRepoId: string, newRepoId: string): BrowserPage { return { ...structuredClone(page), - worktreeId: rekeyWorktreeId(oldRepoId, newRepoId, page.worktreeId) + worktreeId: rekeyWorktreeId(oldRepoId, newRepoId, page.worktreeId), + ...(page.docLocation + ? { docLocation: rekeyBrowserDocLocation(page.docLocation, oldRepoId, newRepoId) } + : {}) } } +function rekeyBrowserDocLocation( + location: BrowserPageDocLocation, + oldRepoId: string, + newRepoId: string +): BrowserPageDocLocation { + const nextWorktreeId = rekeyWorktreeId(oldRepoId, newRepoId, location.worktreeId) + return remapBrowserPageDocLocation(location, location.worktreeId, nextWorktreeId) +} + function copyBrowserPages( pagesByWorkspace: Record | undefined, workspaceIds: ReadonlySet, diff --git a/src/main/persistence-cohort-and-identity-migration.test.ts b/src/main/persistence-cohort-and-identity-migration.test.ts index 9d931a1bbd4..4081672c821 100644 --- a/src/main/persistence-cohort-and-identity-migration.test.ts +++ b/src/main/persistence-cohort-and-identity-migration.test.ts @@ -243,10 +243,33 @@ describe('Store.migrateWorktreeIdentity', () => { }, activeFileIdByWorktree: { [OLD]: '/ws/cunner/a.ts' }, browserTabsByWorktree: { - [OLD]: [{ id: 'browser1', worktreeId: OLD, title: 'Browser', url: 'about:blank' }] + [OLD]: [ + { + id: 'browser1', + worktreeId: OLD, + title: 'Browser', + url: 'about:blank', + docLocation: { + kind: 'workspace-doc', + worktreeId: OLD, + filePath: '/ws/cunner/docs/report.html' + } + } + ] }, browserPagesByWorkspace: { - browser1: [{ id: 'page1', workspaceId: 'browser1', worktreeId: OLD }] + browser1: [ + { + id: 'page1', + workspaceId: 'browser1', + worktreeId: OLD, + docLocation: { + kind: 'workspace-doc', + worktreeId: OLD, + filePath: '/ws/cunner/docs/report.html' + } + } + ] }, activeBrowserTabIdByWorktree: { [OLD]: 'browser1' }, activeTabTypeByWorktree: { [OLD]: 'browser' }, @@ -327,6 +350,16 @@ describe('Store.migrateWorktreeIdentity', () => { expect(session.browserTabsByWorktree?.[OLD]).toBeUndefined() expect(session.browserTabsByWorktree?.[NEW]?.[0]?.worktreeId).toBe(NEW) expect(session.browserPagesByWorkspace?.browser1?.[0]?.worktreeId).toBe(NEW) + expect(session.browserTabsByWorktree?.[NEW]?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: NEW, + filePath: '/ws/worktree-creation-spinner/docs/report.html' + }) + expect(session.browserPagesByWorkspace?.browser1?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: NEW, + filePath: '/ws/worktree-creation-spinner/docs/report.html' + }) expect(session.activeBrowserTabIdByWorktree?.[NEW]).toBe('browser1') expect(session.activeTabTypeByWorktree?.[NEW]).toBe('browser') expect(session.activeWorktreeId).toBe(NEW) diff --git a/src/main/persistence/tracking-repos/worktree-identity-migration.ts b/src/main/persistence/tracking-repos/worktree-identity-migration.ts index d9451258634..2d2fa484ad6 100644 --- a/src/main/persistence/tracking-repos/worktree-identity-migration.ts +++ b/src/main/persistence/tracking-repos/worktree-identity-migration.ts @@ -1,4 +1,6 @@ import type { WorkspaceKey } from '../../../shared/folder-workspace-types' +import type { BrowserPage, BrowserWorkspace } from '../../../shared/browser-workspace-types' +import { remapBrowserPageDocLocation } from '../../../shared/browser-page-doc-location' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' import type { PersistedState } from '../../../shared/persisted-state-types' @@ -6,6 +8,7 @@ import { getWorktreeIdFromHostIdentity, isWorktreeHostIdentity } from '../../../shared/worktree/host-qualified-identity' +import { splitWorktreeIdForFilesystem } from '../../../shared/worktree/id' /** * Re-keys every worktreeId-keyed record in `state` from `oldWorktreeId` to `newWorktreeId`. Mutates `state` in place; @@ -35,6 +38,23 @@ export function migrateWorktreeIdentity( } const withNewWorktreeId = (value: T): T => value.worktreeId === oldWorktreeId ? { ...value, worktreeId: newWorktreeId } : value + const oldWorktreePath = splitWorktreeIdForFilesystem(oldWorktreeId)?.worktreePath + const newWorktreePath = splitWorktreeIdForFilesystem(newWorktreeId)?.worktreePath + const withNewBrowserWorktreeId = (value: T): T => { + const renamedValue = withNewWorktreeId(value) + return value.docLocation?.worktreeId === oldWorktreeId + ? { + ...renamedValue, + docLocation: remapBrowserPageDocLocation( + value.docLocation, + oldWorktreeId, + newWorktreeId, + oldWorktreePath, + newWorktreePath + ) + } + : renamedValue + } const migrateSession = (session: WorkspaceSessionState | undefined): boolean => { if (!session) { return false @@ -72,16 +92,21 @@ export function migrateWorktreeIdentity( sessionChanged = moveSessionKey(session.activeFileIdByWorktree) || sessionChanged sessionChanged = moveSessionKey(session.browserTabsByWorktree, (workspaces) => - workspaces.map(withNewWorktreeId) + workspaces.map(withNewBrowserWorktreeId) ) || sessionChanged if (session.browserPagesByWorkspace) { let pagesChanged = false const nextPagesByWorkspace = { ...session.browserPagesByWorkspace } for (const [workspaceId, pages] of Object.entries(nextPagesByWorkspace)) { - if (!pages.some((page) => page.worktreeId === oldWorktreeId)) { + if ( + !pages.some( + (page) => + page.worktreeId === oldWorktreeId || page.docLocation?.worktreeId === oldWorktreeId + ) + ) { continue } - nextPagesByWorkspace[workspaceId] = pages.map(withNewWorktreeId) + nextPagesByWorkspace[workspaceId] = pages.map(withNewBrowserWorktreeId) pagesChanged = true } if (pagesChanged) { diff --git a/src/renderer/src/store/slices/browser-workspace-doc-location.test.ts b/src/renderer/src/store/slices/browser-workspace-doc-location.test.ts index 1c621752d19..d311216a807 100644 --- a/src/renderer/src/store/slices/browser-workspace-doc-location.test.ts +++ b/src/renderer/src/store/slices/browser-workspace-doc-location.test.ts @@ -280,6 +280,40 @@ describe('a browser page that shows a workspace document', () => { expect(store.getState().browserPagesByWorkspace[tab.id]).toHaveLength(1) }) + it('reopens a closed document tab with its document identity', () => { + const store = createStoreWithWorktree() + const tab = store.getState().createBrowserTab(WORKTREE_ID, LIVE_GRANT_URL, { + docLocation: DOC_LOCATION, + browserRuntimeEnvironmentId: null + }) + + store.getState().closeBrowserTab(tab.id) + const reopened = store.getState().reopenClosedBrowserTab(WORKTREE_ID) + const page = reopened ? store.getState().browserPagesByWorkspace[reopened.id]?.[0] : undefined + + expect(page?.docLocation).toEqual(DOC_LOCATION) + expect(page?.url).toBe(ORCA_BROWSER_BLANK_URL) + expect(reopened?.docLocation).toEqual(DOC_LOCATION) + }) + + it('reopens a closed document page with its document identity', () => { + const store = createStoreWithWorktree() + const tab = store.getState().createBrowserTab(WORKTREE_ID, 'https://example.com/') + const documentPage = store.getState().createBrowserPage(tab.id, LIVE_GRANT_URL, { + docLocation: DOC_LOCATION, + browserRuntimeEnvironmentId: null + }) + if (!documentPage) { + throw new Error('Expected a document page') + } + + store.getState().closeBrowserPage(documentPage.id) + const reopened = store.getState().reopenClosedBrowserPage(tab.id) + + expect(reopened?.docLocation).toEqual(DOC_LOCATION) + expect(reopened?.url).toBe(ORCA_BROWSER_BLANK_URL) + }) + it('writes the document and not the grant to the session', () => { const store = createStoreWithWorktree() const tab = store.getState().createBrowserTab(WORKTREE_ID, LIVE_GRANT_URL, { diff --git a/src/renderer/src/store/slices/browser.ts b/src/renderer/src/store/slices/browser.ts index 081ea469a45..21394e8ff36 100644 --- a/src/renderer/src/store/slices/browser.ts +++ b/src/renderer/src/store/slices/browser.ts @@ -97,6 +97,8 @@ type CreateBrowserPageOptions = { activate?: boolean title?: string browserRuntimeEnvironmentId?: string | null + /** Creates a page that shows a workspace document instead of a URL. */ + docLocation?: BrowserPageDocLocation } type BrowserTabPageState = { @@ -1187,6 +1189,7 @@ export const createBrowserSlice: StateCreator = activate: true, sessionProfileId, sessionPartition, + ...(snap.docLocation ? { docLocation: snap.docLocation } : {}), targetGroupId: entryToRestore.position?.groupId }) restoreRecentlyClosedTabPosition(get, worktreeId, restored.id, entryToRestore.position) @@ -1201,14 +1204,16 @@ export const createBrowserSlice: StateCreator = sessionProfileId, sessionPartition, targetGroupId: entryToRestore.position?.groupId, - browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId + browserRuntimeEnvironmentId: firstPage.browserRuntimeEnvironmentId, + ...(firstPage.docLocation ? { docLocation: firstPage.docLocation } : {}) }) for (const p of restPages) { get().createBrowserPage(restored.id, p.url, { activate: false, title: p.title, - browserRuntimeEnvironmentId: p.browserRuntimeEnvironmentId + browserRuntimeEnvironmentId: p.browserRuntimeEnvironmentId, + ...(p.docLocation ? { docLocation: p.docLocation } : {}) }) } @@ -1288,7 +1293,9 @@ export const createBrowserSlice: StateCreator = workspace.worktreeId, url, options?.title, - options?.browserRuntimeEnvironmentId + options?.browserRuntimeEnvironmentId, + undefined, + options?.docLocation ) set((s) => { @@ -1309,6 +1316,7 @@ export const createBrowserSlice: StateCreator = s.activeBrowserTabIdByWorktree[workspace.worktreeId] === workspaceId const shouldFocusAddressBar = shouldUpdateGlobalActiveSurface && + !page.docLocation && (page.url === 'about:blank' || page.url === ORCA_BROWSER_BLANK_URL) return { @@ -1476,7 +1484,8 @@ export const createBrowserSlice: StateCreator = return get().createBrowserPage(workspaceId, pageToRestore.url, { title: pageToRestore.title, activate: true, - browserRuntimeEnvironmentId: pageToRestore.browserRuntimeEnvironmentId + browserRuntimeEnvironmentId: pageToRestore.browserRuntimeEnvironmentId, + ...(pageToRestore.docLocation ? { docLocation: pageToRestore.docLocation } : {}) }) }, diff --git a/src/renderer/src/store/slices/worktrees-identity-migration.test.ts b/src/renderer/src/store/slices/worktrees-identity-migration.test.ts index fc36935dd5c..7d709f33ac7 100644 --- a/src/renderer/src/store/slices/worktrees-identity-migration.test.ts +++ b/src/renderer/src/store/slices/worktrees-identity-migration.test.ts @@ -74,8 +74,32 @@ describe('migrateWorktreeIdentity', () => { [OLD]: { resultOwner: { worktreeId: OLD, runtimeEnvironmentId: 'runtime-a' } } }, activeTabIdByWorktree: { [OLD]: 'tab1' }, - browserTabsByWorktree: { [OLD]: [{ id: 'browser1', worktreeId: OLD }] }, - browserPagesByWorkspace: { browser1: [{ id: 'page1', worktreeId: OLD }] }, + browserTabsByWorktree: { + [OLD]: [ + { + id: 'browser1', + worktreeId: OLD, + docLocation: { + kind: 'workspace-doc', + worktreeId: OLD, + filePath: '/ws/cunner/docs/report.html' + } + } + ] + }, + browserPagesByWorkspace: { + browser1: [ + { + id: 'page1', + worktreeId: OLD, + docLocation: { + kind: 'workspace-doc', + worktreeId: OLD, + filePath: '/ws/cunner/docs/report.html' + } + } + ] + }, recentlyClosedBrowserTabsByWorktree: { [OLD]: [ { workspace: { id: 'closed-browser', worktreeId: OLD }, pages: [{ worktreeId: OLD }] } @@ -127,6 +151,16 @@ describe('migrateWorktreeIdentity', () => { expect(s.activeTabIdByWorktree[NEW]).toBe('tab1') expect(s.browserTabsByWorktree[NEW]?.[0]?.worktreeId).toBe(NEW) expect(s.browserPagesByWorkspace.browser1?.[0]?.worktreeId).toBe(NEW) + expect(s.browserTabsByWorktree[NEW]?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: NEW, + filePath: '/ws/worktree-creation-spinner/docs/report.html' + }) + expect(s.browserPagesByWorkspace.browser1?.[0]?.docLocation).toEqual({ + kind: 'workspace-doc', + worktreeId: NEW, + filePath: '/ws/worktree-creation-spinner/docs/report.html' + }) expect(s.recentlyClosedBrowserTabsByWorktree[NEW]?.[0]?.workspace.worktreeId).toBe(NEW) expect(s.recentlyClosedBrowserTabsByWorktree[NEW]?.[0]?.pages[0]?.worktreeId).toBe(NEW) expect(s.recentlyClosedBrowserPagesByWorkspace.browser1?.[0]?.worktreeId).toBe(NEW) diff --git a/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-state.ts b/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-state.ts index 1a841fbdfc9..dee9ebb7f84 100644 --- a/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-state.ts +++ b/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-state.ts @@ -1,4 +1,9 @@ import type { AppState } from '../../../types' +import type { + BrowserPage, + BrowserWorkspace +} from '../../../../../../shared/browser-workspace-types' +import { remapBrowserPageDocLocation } from '../../../../../../shared/browser-page-doc-location' import { splitWorktreeIdForFilesystem } from '../../../../../../shared/worktree/id' import { worktreeWorkspaceKey } from '../../../../../../shared/workspace-scope' import { getWorktreeIdFromVisitKey } from '@/lib/worktree-visit-recency' @@ -76,17 +81,34 @@ export function buildWorktreeRenameState( } const withNewWorktreeId = (value: T): T => value.worktreeId === oldWorktreeId ? { ...value, worktreeId: newWorktreeId } : value + const oldWorktreePath = splitWorktreeIdForFilesystem(oldWorktreeId)?.worktreePath + const newWorktreePath = splitWorktreeIdForFilesystem(newWorktreeId)?.worktreePath + const withNewBrowserWorktreeId = (value: T): T => { + const renamedValue = withNewWorktreeId(value) + return value.docLocation?.worktreeId === oldWorktreeId + ? { + ...renamedValue, + docLocation: remapBrowserPageDocLocation( + value.docLocation, + oldWorktreeId, + newWorktreeId, + oldWorktreePath, + newWorktreePath + ) + } + : renamedValue + } const renameValueByKey: Partial> = { tabsByWorktree: (tabs: { worktreeId: string }[]) => tabs.map(withNewWorktreeId), - browserTabsByWorktree: (workspaces: { worktreeId: string }[]) => - workspaces.map(withNewWorktreeId), + browserTabsByWorktree: (workspaces: BrowserWorkspace[]) => + workspaces.map(withNewBrowserWorktreeId), recentlyClosedBrowserTabsByWorktree: ( - snapshots: { workspace: { worktreeId: string }; pages: { worktreeId: string }[] }[] + snapshots: { workspace: BrowserWorkspace; pages: BrowserPage[] }[] ) => snapshots.map((snapshot) => ({ ...snapshot, - workspace: withNewWorktreeId(snapshot.workspace), - pages: snapshot.pages.map(withNewWorktreeId) + workspace: withNewBrowserWorktreeId(snapshot.workspace), + pages: snapshot.pages.map(withNewBrowserWorktreeId) })), fileSearchStateByWorktree: (searchState: AppState['fileSearchStateByWorktree'][string]) => ({ ...searchState, @@ -121,8 +143,6 @@ export function buildWorktreeRenameState( files.map(withNewWorktreeId) ) // Why: terminal reopen snapshots hold absolute startupCwd paths under the old folder; remap or Cmd+Shift+T respawns into a directory that no longer exists after the rename. - const oldWorktreePath = splitWorktreeIdForFilesystem(oldWorktreeId)?.worktreePath - const newWorktreePath = splitWorktreeIdForFilesystem(newWorktreeId)?.worktreePath renameKey('recentlyClosedTerminalTabsByWorktree', (snapshots: ClosedTerminalTabSnapshot[]) => oldWorktreePath && newWorktreePath ? remapClosedTerminalTabSnapshotCwds(snapshots, oldWorktreePath, newWorktreePath) @@ -137,23 +157,29 @@ export function buildWorktreeRenameState( : s.openFiles const currentBrowserPagesByWorkspace = s.browserPagesByWorkspace ?? {} const browserPagesByWorkspace = Object.values(currentBrowserPagesByWorkspace).some((pages) => - pages.some((page) => page.worktreeId === oldWorktreeId) + pages.some( + (page) => page.worktreeId === oldWorktreeId || page.docLocation?.worktreeId === oldWorktreeId + ) ) ? Object.fromEntries( Object.entries(currentBrowserPagesByWorkspace).map(([workspaceId, pages]) => [ workspaceId, - pages.map(withNewWorktreeId) + pages.map(withNewBrowserWorktreeId) ]) ) : s.browserPagesByWorkspace const currentRecentlyClosedBrowserPagesByWorkspace = s.recentlyClosedBrowserPagesByWorkspace ?? {} const recentlyClosedBrowserPagesByWorkspace = Object.values( currentRecentlyClosedBrowserPagesByWorkspace - ).some((pages) => pages.some((page) => page.worktreeId === oldWorktreeId)) + ).some((pages) => + pages.some( + (page) => page.worktreeId === oldWorktreeId || page.docLocation?.worktreeId === oldWorktreeId + ) + ) ? Object.fromEntries( Object.entries(currentRecentlyClosedBrowserPagesByWorkspace).map(([workspaceId, pages]) => [ workspaceId, - pages.map(withNewWorktreeId) + pages.map(withNewBrowserWorktreeId) ]) ) : s.recentlyClosedBrowserPagesByWorkspace diff --git a/src/shared/browser-page-doc-location.ts b/src/shared/browser-page-doc-location.ts index 3ef30c29952..97a9dcc6e27 100644 --- a/src/shared/browser-page-doc-location.ts +++ b/src/shared/browser-page-doc-location.ts @@ -3,6 +3,7 @@ import type { BrowserPageDocLocation, BrowserWorkspace } from './browser-workspace-types' +import { relativePathInsideRoot, resolveRuntimePath } from './cross-platform-path' /** * Why an explicit comparison and not object identity: the mirror rebuilds the workspace's copy of @@ -25,3 +26,26 @@ export function isWorkspaceDocSurface( ): boolean { return Boolean(surface.docLocation) } + +export function remapBrowserPageDocLocation( + location: BrowserPageDocLocation, + oldWorktreeId: string, + newWorktreeId: string, + oldWorktreePath?: string, + newWorktreePath?: string +): BrowserPageDocLocation { + if (location.worktreeId !== oldWorktreeId) { + return location + } + const relativePath = + oldWorktreePath && newWorktreePath + ? relativePathInsideRoot(oldWorktreePath, location.filePath) + : null + return { + ...location, + worktreeId: newWorktreeId, + ...(relativePath !== null && newWorktreePath + ? { filePath: resolveRuntimePath(newWorktreePath, relativePath) } + : {}) + } +}