mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
fix(terminal): keep host-retired panes out of every layout a paired client writes (#33083)
Exiting a paired host tab's original pane moved the host's active leaf to the surviving split. The client's reconciliation effect followed it with setActivePane, and that persisted every still-mounted pane, writing the retired leaf (whose remote PTY had not ended yet) back into the layout. The leaf was then no longer retired, so its pane stayed forever. Any other persist in that window (insertions, clicks, title edits, divider drags) could do the same. persistLayoutSnapshot now drops the leaves the host retired, using the retireLeavesFromTerminalLayout main already uses, moved to shared. The retired set it reads is the one the reconciliation effect tracks.
This commit is contained in:
@@ -8,7 +8,7 @@ import {
|
||||
} from '../../../shared/terminal-leaf-move'
|
||||
import type { TerminalLayoutSnapshot, TerminalTab } from '../../../shared/terminal-tab-types'
|
||||
import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types'
|
||||
import { retireLeavesFromTerminalLayout } from '../../runtime/mobile-session-terminal-retirement'
|
||||
import { retireLeavesFromTerminalLayout } from '../../../shared/terminal-layout-leaf-retirement'
|
||||
import { createMinimalPersistedTerminalTab } from '../restoring-sessions/session-owner-fields'
|
||||
import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization'
|
||||
import { writeRestorable } from './restorable-write'
|
||||
|
||||
@@ -3,9 +3,9 @@ import type { WorkspaceSessionState } from '../../shared/workspace-session-state
|
||||
import {
|
||||
pruneTabGroupLayoutAfterRetirement,
|
||||
repairMobileSessionTabGroupsAfterRetirement,
|
||||
retireLeavesFromTerminalLayout,
|
||||
type RetiredTerminalSurface
|
||||
} from './mobile-session-terminal-retirement'
|
||||
import { retireLeavesFromTerminalLayout } from '../../shared/terminal-layout-leaf-retirement'
|
||||
import { rebaseWorkspaceSessionTerminalMembership } from './workspace-session-terminal-membership-authority'
|
||||
import { advanceTerminalTopologyRevision } from '../persistence/terminal-topology/terminal-topology-membership'
|
||||
|
||||
|
||||
@@ -6,11 +6,8 @@ import type {
|
||||
RuntimeMobileSessionTerminalTab
|
||||
} from '../../shared/runtime-types'
|
||||
import type { TabGroupLayoutNode } from '../../shared/tab-types'
|
||||
import type {
|
||||
TerminalLayoutSnapshot,
|
||||
TerminalPaneLayoutNode
|
||||
} from '../../shared/terminal-tab-types'
|
||||
import { appendRetiredTerminalSurfaceProofs } from '../../shared/terminal-retirement-proof-ledger'
|
||||
import { retireLeavesFromTerminalLayout } from '../../shared/terminal-layout-leaf-retirement'
|
||||
|
||||
export type RetiredTerminalSurface = {
|
||||
worktreeId: string
|
||||
@@ -21,75 +18,6 @@ export type RetiredTerminalSurface = {
|
||||
retiredAt?: number
|
||||
}
|
||||
|
||||
function pruneTerminalPane(
|
||||
node: TerminalPaneLayoutNode | null,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): TerminalPaneLayoutNode | null {
|
||||
if (!node) {
|
||||
return null
|
||||
}
|
||||
if (node.type === 'leaf') {
|
||||
return retiredLeafIds.has(node.leafId) ? null : node
|
||||
}
|
||||
const first = pruneTerminalPane(node.first, retiredLeafIds)
|
||||
const second = pruneTerminalPane(node.second, retiredLeafIds)
|
||||
if (first && second) {
|
||||
return { ...node, first, second }
|
||||
}
|
||||
return first ?? second
|
||||
}
|
||||
|
||||
function collectTerminalLeafIds(node: TerminalPaneLayoutNode | null): string[] {
|
||||
if (!node) {
|
||||
return []
|
||||
}
|
||||
return node.type === 'leaf'
|
||||
? [node.leafId]
|
||||
: [...collectTerminalLeafIds(node.first), ...collectTerminalLeafIds(node.second)]
|
||||
}
|
||||
|
||||
function omitLeafRecords<T>(
|
||||
values: Record<string, T> | undefined,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): Record<string, T> | undefined {
|
||||
if (!values) {
|
||||
return undefined
|
||||
}
|
||||
const retained = Object.fromEntries(
|
||||
Object.entries(values).filter(([leafId]) => !retiredLeafIds.has(leafId))
|
||||
)
|
||||
return Object.keys(retained).length > 0 ? retained : undefined
|
||||
}
|
||||
|
||||
export function retireLeavesFromTerminalLayout(
|
||||
layout: TerminalLayoutSnapshot,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): TerminalLayoutSnapshot | null {
|
||||
const root = pruneTerminalPane(layout.root, retiredLeafIds)
|
||||
if (!root) {
|
||||
return null
|
||||
}
|
||||
const retainedLeafIds = collectTerminalLeafIds(root)
|
||||
const retainedLeafIdSet = new Set(retainedLeafIds)
|
||||
const activeLeafId =
|
||||
layout.activeLeafId && retainedLeafIdSet.has(layout.activeLeafId)
|
||||
? layout.activeLeafId
|
||||
: retainedLeafIds[0]!
|
||||
return {
|
||||
...layout,
|
||||
root,
|
||||
activeLeafId,
|
||||
expandedLeafId:
|
||||
layout.expandedLeafId && retainedLeafIdSet.has(layout.expandedLeafId)
|
||||
? layout.expandedLeafId
|
||||
: null,
|
||||
ptyIdsByLeafId: omitLeafRecords(layout.ptyIdsByLeafId, retiredLeafIds),
|
||||
buffersByLeafId: omitLeafRecords(layout.buffersByLeafId, retiredLeafIds),
|
||||
scrollbackRefsByLeafId: omitLeafRecords(layout.scrollbackRefsByLeafId, retiredLeafIds),
|
||||
titlesByLeafId: omitLeafRecords(layout.titlesByLeafId, retiredLeafIds)
|
||||
}
|
||||
}
|
||||
|
||||
export function pruneTabGroupLayoutAfterRetirement(
|
||||
layout: TabGroupLayoutNode | undefined,
|
||||
retainedGroupIds: ReadonlySet<string>
|
||||
|
||||
@@ -80,7 +80,7 @@ function renderPersistence() {
|
||||
controller as unknown as Parameters<typeof useTerminalPaneLayoutPersistence>[0]
|
||||
)
|
||||
)
|
||||
return { hook, push }
|
||||
return { hook, push, setTabLayout }
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
@@ -148,3 +148,18 @@ describe('terminal layout title persist', () => {
|
||||
expect(push).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('terminal layout persist with host-retired panes', () => {
|
||||
it('keeps a host-retired pane out of the layout it writes while that pane is still mounted', () => {
|
||||
const { hook, push, setTabLayout } = renderPersistence()
|
||||
hook.result.current.hostRetiredLeafIdsRef.current = new Set([RIGHT])
|
||||
|
||||
hook.result.current.changePaneTitle(1, 'build')
|
||||
|
||||
const written = setTabLayout.mock.lastCall?.[1]
|
||||
expect(written?.root).toEqual({ type: 'leaf', leafId: LEFT })
|
||||
expect(written?.activeLeafId).toBe(LEFT)
|
||||
expect(written?.ptyIdsByLeafId).toEqual({ [LEFT]: 'remote:host:1' })
|
||||
expect(push.mock.lastCall?.[0].layout.root).toEqual({ type: 'leaf', leafId: LEFT })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -15,6 +15,7 @@ import type { TerminalPaneStartupController } from './use-terminal-pane-startup-
|
||||
import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../../shared/layout-owner-switch'
|
||||
import { sendLayoutCommandToOwner } from '@/runtime/layout-owner-command'
|
||||
import { notePendingPaneTitleRename } from '@/runtime/pending-pane-title-renames'
|
||||
import { retireLeavesFromTerminalLayout } from '../../../../shared/terminal-layout-leaf-retirement'
|
||||
|
||||
export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartupController) {
|
||||
const {
|
||||
@@ -37,6 +38,9 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup
|
||||
terminalTab,
|
||||
worktreeId
|
||||
} = controller
|
||||
// Leaves a host-authoritative layout dropped whose panes are still mounted, kept out of every
|
||||
// write so a persist cannot re-add one while its PTY is still ending.
|
||||
const hostRetiredLeafIdsRef = useRef<ReadonlySet<string>>(new Set())
|
||||
const chatOwnerRef = useRef({ chatLeafId, isChatViewMode })
|
||||
useLayoutEffect(() => {
|
||||
chatOwnerRef.current = { chatLeafId, isChatViewMode }
|
||||
@@ -50,7 +54,7 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup
|
||||
}
|
||||
const activePaneId = manager.getActivePane()?.id ?? manager.getPanes()[0]?.id ?? null
|
||||
const leafIdByPaneId = manager.getLeafIdMap()
|
||||
const layout = serializeTerminalLayout(
|
||||
let layout = serializeTerminalLayout(
|
||||
container,
|
||||
activePaneId,
|
||||
expandedPaneIdRef.current,
|
||||
@@ -125,6 +129,11 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup
|
||||
if (Object.keys(titlesByLeafId).length > 0) {
|
||||
layout.titlesByLeafId = titlesByLeafId
|
||||
}
|
||||
const hostRetiredLeafIds = hostRetiredLeafIdsRef.current
|
||||
if (hostRetiredLeafIds.size > 0) {
|
||||
// Why: never retire the last pane here; the host drops a whole tab by itself.
|
||||
layout = retireLeavesFromTerminalLayout(layout, hostRetiredLeafIds) ?? layout
|
||||
}
|
||||
setTabLayout(tabId, layout)
|
||||
const hasRemotePane = Object.values(mergedPtyIds).some(
|
||||
(ptyId) => typeof ptyId === 'string' && isRemoteRuntimePtyId(ptyId)
|
||||
@@ -297,6 +306,7 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup
|
||||
}, [paneCount, paneTitles, persistLayoutSnapshot, terminalTab])
|
||||
|
||||
return {
|
||||
hostRetiredLeafIdsRef,
|
||||
persistLayoutSnapshot,
|
||||
clearPaneScrollback,
|
||||
changePaneTitle,
|
||||
|
||||
@@ -23,6 +23,7 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr
|
||||
closeTerminalLinkActions,
|
||||
containerRef,
|
||||
executeClosePane,
|
||||
hostRetiredLeafIdsRef,
|
||||
isActive,
|
||||
isRendererVisible,
|
||||
isolatedPaneKey,
|
||||
@@ -36,11 +37,9 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr
|
||||
restoredLayout,
|
||||
tabId
|
||||
} = controller
|
||||
// Leaves the last host-authoritative layout named, and the ones it has since
|
||||
// dropped whose panes are still mounted; a removal needs the host to have
|
||||
// named the leaf before it dropped it, and may have to wait for the PTY exit.
|
||||
// Leaves the last host-authoritative layout named; a removal needs the host to
|
||||
// have named the leaf before it dropped it, and may have to wait for the PTY exit.
|
||||
const hostLayoutLeafIdsRef = useRef<ReadonlySet<string>>(new Set())
|
||||
const retiredLeafIdsRef = useRef<ReadonlySet<string>>(new Set())
|
||||
|
||||
useEffect(() => {
|
||||
closeTerminalLinkActions()
|
||||
@@ -51,7 +50,9 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr
|
||||
if (!manager || !restoredLayout.root) {
|
||||
return
|
||||
}
|
||||
// Why the retired check: dropping a retired leaf can drop the layout's only remote PTY id.
|
||||
if (
|
||||
hostRetiredLeafIdsRef.current.size === 0 &&
|
||||
!isHostAuthoritativeLayout({
|
||||
isWebClient: !!(globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__,
|
||||
ptyIdsByLeafId: restoredLayout.ptyIdsByLeafId
|
||||
@@ -62,13 +63,13 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr
|
||||
const layoutLeafIds = new Set(collectLeafIds(restoredLayout.root))
|
||||
const mountedLeafIds = manager.getPanes().map((pane) => pane.leafId)
|
||||
const retiredLeafIds = trackRetiredLeafIds({
|
||||
retiredLeafIds: retiredLeafIdsRef.current,
|
||||
retiredLeafIds: hostRetiredLeafIdsRef.current,
|
||||
previousLayoutLeafIds: hostLayoutLeafIdsRef.current,
|
||||
layoutLeafIds,
|
||||
mountedLeafIds
|
||||
})
|
||||
hostLayoutLeafIdsRef.current = layoutLeafIds
|
||||
retiredLeafIdsRef.current = retiredLeafIds
|
||||
hostRetiredLeafIdsRef.current = retiredLeafIds
|
||||
const insertions = planTerminalLiveLayoutInsertions(restoredLayout.root, mountedLeafIds)
|
||||
const removals = planTerminalLiveLayoutRemovals(
|
||||
restoredLayout.root,
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
import type { TerminalLayoutSnapshot, TerminalPaneLayoutNode } from './terminal-tab-types'
|
||||
import { collectLeafIds } from './terminal-pane-layout-tree'
|
||||
|
||||
function pruneTerminalPane(
|
||||
node: TerminalPaneLayoutNode | null,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): TerminalPaneLayoutNode | null {
|
||||
if (!node) {
|
||||
return null
|
||||
}
|
||||
if (node.type === 'leaf') {
|
||||
return retiredLeafIds.has(node.leafId) ? null : node
|
||||
}
|
||||
const first = pruneTerminalPane(node.first, retiredLeafIds)
|
||||
const second = pruneTerminalPane(node.second, retiredLeafIds)
|
||||
if (first && second) {
|
||||
return { ...node, first, second }
|
||||
}
|
||||
return first ?? second
|
||||
}
|
||||
|
||||
function omitLeafRecords<T>(
|
||||
values: Record<string, T> | undefined,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): Record<string, T> | undefined {
|
||||
if (!values) {
|
||||
return undefined
|
||||
}
|
||||
const retained = Object.fromEntries(
|
||||
Object.entries(values).filter(([leafId]) => !retiredLeafIds.has(leafId))
|
||||
)
|
||||
return Object.keys(retained).length > 0 ? retained : undefined
|
||||
}
|
||||
|
||||
/** Drops leaves and their per-leaf records; null when no leaf remains. */
|
||||
export function retireLeavesFromTerminalLayout(
|
||||
layout: TerminalLayoutSnapshot,
|
||||
retiredLeafIds: ReadonlySet<string>
|
||||
): TerminalLayoutSnapshot | null {
|
||||
const root = pruneTerminalPane(layout.root, retiredLeafIds)
|
||||
if (!root) {
|
||||
return null
|
||||
}
|
||||
const retainedLeafIds = collectLeafIds(root)
|
||||
const retainedLeafIdSet = new Set(retainedLeafIds)
|
||||
const activeLeafId =
|
||||
layout.activeLeafId && retainedLeafIdSet.has(layout.activeLeafId)
|
||||
? layout.activeLeafId
|
||||
: retainedLeafIds[0]!
|
||||
return {
|
||||
...layout,
|
||||
root,
|
||||
activeLeafId,
|
||||
expandedLeafId:
|
||||
layout.expandedLeafId && retainedLeafIdSet.has(layout.expandedLeafId)
|
||||
? layout.expandedLeafId
|
||||
: null,
|
||||
ptyIdsByLeafId: omitLeafRecords(layout.ptyIdsByLeafId, retiredLeafIds),
|
||||
buffersByLeafId: omitLeafRecords(layout.buffersByLeafId, retiredLeafIds),
|
||||
scrollbackRefsByLeafId: omitLeafRecords(layout.scrollbackRefsByLeafId, retiredLeafIds),
|
||||
titlesByLeafId: omitLeafRecords(layout.titlesByLeafId, retiredLeafIds)
|
||||
}
|
||||
}
|
||||
@@ -14,13 +14,15 @@
|
||||
* no exit overlay and no restart control. The refutation-proof shape (verified
|
||||
* here) is that the client's store layout shrinks to one leaf while its DOM
|
||||
* keeps two panes. After the fix the client removes the retired pane and store
|
||||
* + DOM agree at exactly the surviving leaf.
|
||||
* + DOM agree at exactly the surviving leaf. Exiting the original pane also
|
||||
* moves the host's active leaf; the client must not write the retired pane back
|
||||
* into its layout while that PTY is still ending.
|
||||
*
|
||||
* Run:
|
||||
* pnpm exec playwright test tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts \
|
||||
* --config tests/playwright.config.ts --project electron-headless --workers=1
|
||||
*/
|
||||
import type { Page } from '@stablyai/playwright-test'
|
||||
import type { Page, TestInfo } from '@stablyai/playwright-test'
|
||||
import { toWebTerminalSurfaceTabId } from '../../src/shared/terminal-surface-id'
|
||||
import { expect, test } from './helpers/orca-app'
|
||||
import { launchHeadlessPairedRuntimeHost } from './helpers/headless-paired-runtime-host'
|
||||
@@ -56,9 +58,11 @@ async function hostLayoutLeafIds(page: Page, webTabId: string): Promise<string[]
|
||||
}, webTabId)
|
||||
}
|
||||
|
||||
test('removes the pane a paired remote host retired instead of leaving a dead ghost', async ({
|
||||
testRepoPath
|
||||
}, testInfo) => {
|
||||
async function expectHostRetiredPaneRemoved(
|
||||
exitedPane: 'split' | 'original',
|
||||
testRepoPath: string,
|
||||
testInfo: TestInfo
|
||||
): Promise<void> {
|
||||
test.setTimeout(240_000)
|
||||
const host = await launchHeadlessPairedRuntimeHost()
|
||||
let client: Awaited<ReturnType<typeof launchPairedElectronClient>> | null = null
|
||||
@@ -102,9 +106,13 @@ test('removes the pane a paired remote host retired instead of leaving a dead gh
|
||||
.toBe(true)
|
||||
const beforeExit = await mountedLeafPtyIds(client.page, webTabId)
|
||||
|
||||
// The user quits the second shell — the host retires that leaf and
|
||||
// The user quits one shell — the host retires that leaf and
|
||||
// republishes a one-leaf layout.
|
||||
await host.client.call('terminal.send', { terminal: secondHandle, text: 'exit', enter: true })
|
||||
await host.client.call('terminal.send', {
|
||||
terminal: exitedPane === 'split' ? secondHandle : firstHandle,
|
||||
text: 'exit',
|
||||
enter: true
|
||||
})
|
||||
|
||||
// The host-authoritative layout the client holds shrinks to one leaf
|
||||
// (confirms the retirement). This is the refutation-proof signal: before the
|
||||
@@ -139,4 +147,13 @@ test('removes the pane a paired remote host retired instead of leaving a dead gh
|
||||
await client?.dispose()
|
||||
await host.dispose()
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
test('removes the pane a paired remote host retired instead of leaving a dead ghost', async ({
|
||||
testRepoPath
|
||||
}, testInfo) => expectHostRetiredPaneRemoved('split', testRepoPath, testInfo))
|
||||
|
||||
// Why: the original pane is the host's active leaf, so its retirement also moves focus.
|
||||
test('removes the original pane a paired remote host retired after it exits', async ({
|
||||
testRepoPath
|
||||
}, testInfo) => expectHostRetiredPaneRemoved('original', testRepoPath, testInfo))
|
||||
|
||||
Reference in New Issue
Block a user