mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
fix(terminal): let hydration heal a rootless layout, without letting it evict
collectLeafIdsInOrder returns nothing for a rootless layout, so the self-heal saw none of their bindings and a duplicate that ownership resolution can see was one hydration could never repair. Both rules now come from one module, which also names why they differ: ownership reads a rootless layout generously, because over-counting only costs a reveal that adopts instead of minting, while the heal reads it narrowly, because a winner takes a binding away and a never-pruned map holds more than it owns. The old fixture for the stranded-binding case built a rootless layout, not a stranded one, so it was not testing the guard it named.
This commit is contained in:
@@ -0,0 +1,37 @@
|
||||
import type { TerminalPaneLayoutNode } from '../../../../shared/terminal-tab-types'
|
||||
import { collectLeafIdsInOrder } from './terminal-layout-leaf-ids'
|
||||
|
||||
export type TerminalLayoutLeafClaims = {
|
||||
root: TerminalPaneLayoutNode | null
|
||||
activeLeafId?: string | null
|
||||
ptyIdsByLeafId?: Record<string, string>
|
||||
}
|
||||
|
||||
/**
|
||||
* Leaf ids this layout holds, read generously: its tree, or — for a rootless layout, which binds
|
||||
* its sole pane off-tree — every leaf it binds. Use this to ask "does some pane already hold
|
||||
* this?", where over-counting only costs a reveal that adopts instead of minting.
|
||||
*
|
||||
* A binding whose leaf has left a rooted tree reattaches nothing, so it is excluded either way.
|
||||
*/
|
||||
export function collectOwnedLeafIds(layout: TerminalLayoutLeafClaims): Set<string> {
|
||||
return new Set(
|
||||
layout.root ? collectLeafIdsInOrder(layout.root) : Object.keys(layout.ptyIdsByLeafId ?? {})
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Leaf ids this layout may take from another tab, read narrowly. Same as the owned set for a
|
||||
* rooted layout, but a rootless one proves only its sole off-tree pane, or the one its
|
||||
* `activeLeafId` names: a never-pruned map holds more than it owns, and claiming those evicts
|
||||
* the live row that really owns them (#13098). Mirrors the `owned`/`claimable` split in
|
||||
* `terminal-session-row-hydration.ts`, which is the guard that caught #13060.
|
||||
*/
|
||||
export function collectClaimableLeafIds(layout: TerminalLayoutLeafClaims): Set<string> {
|
||||
if (layout.root) {
|
||||
return new Set(collectLeafIdsInOrder(layout.root))
|
||||
}
|
||||
const boundLeafIds = Object.keys(layout.ptyIdsByLeafId ?? {})
|
||||
const provenLeafId = boundLeafIds.length === 1 ? boundLeafIds[0] : layout.activeLeafId
|
||||
return new Set(boundLeafIds.filter((leafId) => leafId === provenLeafId))
|
||||
}
|
||||
@@ -1,4 +1,4 @@
|
||||
import { collectLeafIdsInOrder } from '@/components/terminal-pane/terminal-layout-leaf-ids'
|
||||
import { collectOwnedLeafIds } from '@/components/terminal-pane/terminal-layout-leaf-claims'
|
||||
import type { AppState } from '@/store/types'
|
||||
|
||||
/** No `tabsByWorktree`: ownership is tab-keyed, so no worktree key participates. */
|
||||
@@ -21,12 +21,7 @@ export type TerminalPtyPaneOwnerOptions = {
|
||||
preferTabId?: string
|
||||
}
|
||||
|
||||
/**
|
||||
* The leaf a tab's layout binds to `ptyId`, or null.
|
||||
*
|
||||
* A binding whose leaf has left the tree reattaches nothing, so it must not outrank a live
|
||||
* pane (#13098). Rootless layouts bind their sole pane off-tree and keep counting.
|
||||
*/
|
||||
/** The leaf a tab's layout binds to `ptyId`, or null when no leaf it owns holds that binding. */
|
||||
function findLayoutBoundLeafId(
|
||||
state: TerminalPtyPaneOwnerState,
|
||||
tabId: string,
|
||||
@@ -36,9 +31,9 @@ function findLayoutBoundLeafId(
|
||||
if (!layout?.ptyIdsByLeafId) {
|
||||
return null
|
||||
}
|
||||
const leafIdsInTree = layout.root ? new Set(collectLeafIdsInOrder(layout.root)) : null
|
||||
const ownedLeafIds = collectOwnedLeafIds(layout)
|
||||
for (const [leafId, boundPtyId] of Object.entries(layout.ptyIdsByLeafId)) {
|
||||
if (boundPtyId === ptyId && (!leafIdsInTree || leafIdsInTree.has(leafId))) {
|
||||
if (boundPtyId === ptyId && ownedLeafIds.has(leafId)) {
|
||||
return leafId
|
||||
}
|
||||
}
|
||||
|
||||
+45
-5
@@ -11,6 +11,7 @@ const SINGLE_TAB = 'tab-single'
|
||||
const SHARED_LEAF = '10cb5648-8a54-41c0-a6a4-ef0028d93599'
|
||||
const OTHER_LEAF = 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0'
|
||||
const ELSEWHERE_LEAF = '96cdf7ea-9c83-4ba5-a41b-c425955e6606'
|
||||
const CARRIER_LEAF = 'b7f0e2a1-4c6d-4f8a-9b2e-5d3c1a0f7e64'
|
||||
const SHARED_PTY = 'wt-1@@shared'
|
||||
|
||||
function tab(id: string, sortOrder: number, createdAt = 1_000 + sortOrder): TerminalTab {
|
||||
@@ -188,19 +189,58 @@ describe('resolveDuplicateTerminalLayoutBindings', () => {
|
||||
})
|
||||
|
||||
it('ignores a binding whose pane already left the tree', () => {
|
||||
const ghost: TerminalLayoutSnapshot = {
|
||||
root: null,
|
||||
activeLeafId: null,
|
||||
// A rooted layout whose map still names a leaf its tree dropped: the binding reattaches
|
||||
// nothing, so it must not outrank the pane that still mounts the pty.
|
||||
const stranded: TerminalLayoutSnapshot = {
|
||||
root: { type: 'leaf', leafId: CARRIER_LEAF },
|
||||
activeLeafId: CARRIER_LEAF,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [ELSEWHERE_LEAF]: SHARED_PTY }
|
||||
}
|
||||
const healed = heal({ [SINGLE_TAB]: ghost, [SPLIT_TAB]: splitLayout() }, [
|
||||
const healed = heal({ [SINGLE_TAB]: stranded, [SPLIT_TAB]: splitLayout() }, [
|
||||
tab(SINGLE_TAB, 0),
|
||||
tab(SPLIT_TAB, 1)
|
||||
])
|
||||
|
||||
// The ghost reattaches nothing, so it must not outrank the pane that still mounts the pty.
|
||||
expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY)
|
||||
expect(healed[SINGLE_TAB]).toBe(stranded)
|
||||
})
|
||||
|
||||
it('heals a rootless layout, which binds its sole pane off-tree', () => {
|
||||
// Ownership resolution counts these bindings, so the self-heal has to as well, or a
|
||||
// duplicate the resolver can see is one hydration can never repair.
|
||||
const rootless: TerminalLayoutSnapshot = {
|
||||
root: null,
|
||||
activeLeafId: SHARED_LEAF,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [SHARED_LEAF]: SHARED_PTY }
|
||||
}
|
||||
const healed = heal({ [SPLIT_TAB]: splitLayout(), [SINGLE_TAB]: rootless }, [
|
||||
tab(SPLIT_TAB, 0),
|
||||
tab(SINGLE_TAB, 1)
|
||||
])
|
||||
|
||||
expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY)
|
||||
expect(healed[SINGLE_TAB]!.ptyIdsByLeafId).toEqual({})
|
||||
expect(healed[SINGLE_TAB]!.root).toBeNull()
|
||||
})
|
||||
|
||||
it('refuses a rootless layout a second, never-pruned binding it cannot prove', () => {
|
||||
// Only the sole off-tree pane, or the one activeLeafId names, proves a rootless claim.
|
||||
// Claiming the rest would evict the row that really owns the pty, which is #13098.
|
||||
const overreaching: TerminalLayoutSnapshot = {
|
||||
root: null,
|
||||
activeLeafId: ELSEWHERE_LEAF,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [ELSEWHERE_LEAF]: 'wt-1@@own', [SHARED_LEAF]: SHARED_PTY }
|
||||
}
|
||||
const healed = heal({ [SINGLE_TAB]: overreaching, [SPLIT_TAB]: splitLayout() }, [
|
||||
tab(SINGLE_TAB, 0),
|
||||
tab(SPLIT_TAB, 1)
|
||||
])
|
||||
|
||||
expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY)
|
||||
expect(healed[SINGLE_TAB]).toBe(overreaching)
|
||||
})
|
||||
|
||||
it('returns the same object when no tab collides', () => {
|
||||
|
||||
@@ -1,10 +1,8 @@
|
||||
import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types'
|
||||
import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types'
|
||||
import { detachTerminalLayoutLeaf } from '@/components/terminal-pane/terminal-layout-leaf-detach'
|
||||
import {
|
||||
collectLeafIdsInOrder,
|
||||
resolvePtyBoundActiveLeafId
|
||||
} from '@/components/terminal-pane/terminal-layout-leaf-ids'
|
||||
import { resolvePtyBoundActiveLeafId } from '@/components/terminal-pane/terminal-layout-leaf-ids'
|
||||
import { collectClaimableLeafIds } from '@/components/terminal-pane/terminal-layout-leaf-claims'
|
||||
|
||||
type TerminalLayoutOwnerRanking = {
|
||||
canonicalTabIds: ReadonlySet<string>
|
||||
@@ -47,18 +45,19 @@ function compareOwnerTabIds(a: string, b: string, ranking: TerminalLayoutOwnerRa
|
||||
return a < b ? -1 : a > b ? 1 : 0
|
||||
}
|
||||
|
||||
/** Leaf ids this layout claims as pane identities. A binding off the tree reattaches nothing. */
|
||||
// Why the claimable set and not the owned one: a winner here takes a binding away from a loser,
|
||||
// and a rootless layout's never-pruned map would evict the row that really owns it (#13098).
|
||||
function collectHeldLeafIds(layout: TerminalLayoutSnapshot): string[] {
|
||||
return [...new Set(collectLeafIdsInOrder(layout.root))]
|
||||
return [...collectClaimableLeafIds(layout)]
|
||||
}
|
||||
|
||||
/** PTY ids this layout can actually reattach: bound to a leaf its own tree still mounts (#13098). */
|
||||
function collectMountedPtyIds(layout: TerminalLayoutSnapshot): string[] {
|
||||
const leafIdsInTree = new Set(collectLeafIdsInOrder(layout.root))
|
||||
/** PTY ids this layout may claim, bound to a leaf it proves it holds. */
|
||||
function collectClaimablePtyIds(layout: TerminalLayoutSnapshot): string[] {
|
||||
const claimableLeafIds = collectClaimableLeafIds(layout)
|
||||
return [
|
||||
...new Set(
|
||||
Object.entries(layout.ptyIdsByLeafId ?? {})
|
||||
.filter(([leafId]) => leafIdsInTree.has(leafId))
|
||||
.filter(([leafId]) => claimableLeafIds.has(leafId))
|
||||
.map(([, ptyId]) => ptyId)
|
||||
)
|
||||
]
|
||||
@@ -156,7 +155,7 @@ export function resolveDuplicateTerminalLayoutBindings(args: {
|
||||
return resolveDuplicateHolders(
|
||||
resolveDuplicateHolders(args.layoutsByTabId, ranking, collectHeldLeafIds, surrenderLeafId),
|
||||
ranking,
|
||||
collectMountedPtyIds,
|
||||
collectClaimablePtyIds,
|
||||
unbindPtyId
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user