Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/renderer/src/components/terminal-pane/TerminalPane.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ import {
} from '@/runtime/runtime-terminal-inspection'
import {
clearWebRuntimeTerminalBuffer,
closeWebRuntimeTerminal,
updateWebRuntimePaneLayout
closeWebRuntimeTerminal
} from '@/runtime/web-runtime-session'
import {
armPrimarySelectionNativePasteSuppression,
Expand All @@ -158,6 +157,10 @@ import {
planTerminalLiveLayoutInsertions
} from './terminal-live-layout-reconciliation'
import type { TerminalQuickCommand, TerminalQuickCommandScope } from '../../../../shared/types'
import {
createRemotePaneLayoutPusher,
type RemotePaneLayoutPusher
} from './remote-pane-layout-push'
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants'
import { isRuntimeOwnedSshTargetId } from '../../../../shared/execution-host'
import { getRepoIdFromWorktreeId } from '../../../../shared/worktree-id'
Expand Down Expand Up @@ -512,6 +515,8 @@ function TerminalPane(
paneTitlesRef.current = paneTitles
const removedTitleLeafIdsRef = useRef<Set<string>>(new Set())
const clearedScrollbackLeafIdsRef = useRef<Set<string>>(new Set())
const remotePaneLayoutPusherRef = useRef<RemotePaneLayoutPusher | null>(null)
remotePaneLayoutPusherRef.current ??= createRemotePaneLayoutPusher()
const [paneTitleOverlayRects, setPaneTitleOverlayRects] = useState<
Record<number, PaneTitleOverlayRect>
>({})
Expand Down Expand Up @@ -1057,13 +1062,7 @@ function TerminalPane(
(ptyId) => typeof ptyId === 'string' && isRemoteRuntimePtyId(ptyId)
)
if (hasRemotePane) {
void updateWebRuntimePaneLayout({
worktreeId,
tabId,
root: layout.root,
expandedLeafId: layout.expandedLeafId,
...(layout.titlesByLeafId ? { titlesByLeafId: layout.titlesByLeafId } : {})
})
remotePaneLayoutPusherRef.current?.push({ worktreeId, tabId, layout })
}
for (const leafId of currentLeafIds) {
clearedScrollbackLeafIds.delete(leafId)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
/**
* Perf regression: layout persists fire on pane-title churn, and every one of
* them used to push a remote-runtime IPC round trip regardless of whether the
* host-visible layout had moved. Counting invocations at the mocked IPC boundary
* pins the before/after: 100 persists with an unchanged layout cost 100 pushes
* before the dedupe and 1 after.
*/
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { TerminalLayoutSnapshot } from '../../../../shared/types'

const updateWebRuntimePaneLayout = vi.fn()
vi.mock('@/runtime/web-runtime-session', () => ({
updateWebRuntimePaneLayout: (...args: unknown[]) => updateWebRuntimePaneLayout(...args)
}))

const { createRemotePaneLayoutPusher } = await import('./remote-pane-layout-push')

const PERSISTS = 100

function deferred<T>(): { promise: Promise<T>; resolve: (value: T) => void } {
let resolve = (_value: T): void => undefined
const promise = new Promise<T>((complete) => {
resolve = complete
})
return { promise, resolve }
}

function makeLayout(overrides: Partial<TerminalLayoutSnapshot> = {}): TerminalLayoutSnapshot {
return {
root: {
type: 'split',
direction: 'vertical',
ratio: 0.5,
first: { type: 'leaf', leafId: 'leaf-a' },
second: { type: 'leaf', leafId: 'leaf-b' }
},
activeLeafId: 'leaf-a',
expandedLeafId: null,
ptyIdsByLeafId: { 'leaf-a': 'remote:pty-a', 'leaf-b': 'remote:pty-b' },
titlesByLeafId: { 'leaf-a': 'build' },
...overrides
}
}

describe('createRemotePaneLayoutPusher', () => {
beforeEach(() => {
updateWebRuntimePaneLayout.mockReset().mockResolvedValue(true)
})

it('pushes once across 100 persists of an unchanged layout', () => {
const pusher = createRemotePaneLayoutPusher()
for (let i = 0; i < PERSISTS; i += 1) {
// Fresh object each time: persistLayoutSnapshot re-serializes on every call.
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
}
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(1)
})

it('sends the same payload the un-deduped path sent', () => {
const pusher = createRemotePaneLayoutPusher()
const layout = makeLayout()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout })
expect(updateWebRuntimePaneLayout).toHaveBeenCalledWith({
worktreeId: 'wt-1',
tabId: 'tab-1',
root: layout.root,
expandedLeafId: layout.expandedLeafId,
titlesByLeafId: layout.titlesByLeafId
})
})

it('omits titlesByLeafId when the layout carries no titles', () => {
const pusher = createRemotePaneLayoutPusher()
pusher.push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout({ titlesByLeafId: undefined })
})
expect(updateWebRuntimePaneLayout.mock.calls[0][0]).not.toHaveProperty('titlesByLeafId')
})

it('pushes again for every host-visible change', () => {
const pusher = createRemotePaneLayoutPusher()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
pusher.push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout({ expandedLeafId: 'leaf-a' })
})
pusher.push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout({
expandedLeafId: 'leaf-a',
titlesByLeafId: { 'leaf-a': 'test' }
})
})
pusher.push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout({
expandedLeafId: 'leaf-a',
titlesByLeafId: { 'leaf-a': 'test' },
root: {
type: 'split',
direction: 'vertical',
ratio: 0.7,
first: { type: 'leaf', leafId: 'leaf-a' },
second: { type: 'leaf', leafId: 'leaf-b' }
}
})
})
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(4)
})

it('pushes again when a remote pane swaps its pty', () => {
// ptyIdsByLeafId is not in the payload, but a swap means a different host session.
const pusher = createRemotePaneLayoutPusher()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
pusher.push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout({ ptyIdsByLeafId: { 'leaf-a': 'remote:pty-c', 'leaf-b': 'remote:pty-b' } })
})
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(2)
})

it('does not carry a cached layout across tabs', () => {
const pusher = createRemotePaneLayoutPusher()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-2', layout: makeLayout() })
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(3)
})

it('does not carry a cached layout across worktrees', () => {
const pusher = createRemotePaneLayoutPusher()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
pusher.push({ worktreeId: 'wt-2', tabId: 'tab-1', layout: makeLayout() })
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(2)
})

it('retries an unchanged layout after a failed push', async () => {
updateWebRuntimePaneLayout.mockResolvedValueOnce(false)
const pusher = createRemotePaneLayoutPusher()
const input = { worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() }

pusher.push(input)
await Promise.resolve()
pusher.push(input)

expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(2)
})

it('does not let a stale failure invalidate a newer in-flight layout', async () => {
const first = deferred<boolean>()
const second = deferred<boolean>()
updateWebRuntimePaneLayout.mockImplementationOnce(() => first.promise)
updateWebRuntimePaneLayout.mockImplementationOnce(() => second.promise)
const pusher = createRemotePaneLayoutPusher()
const changedLayout = makeLayout({ expandedLeafId: 'leaf-a' })

pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: changedLayout })
first.resolve(false)
await Promise.resolve()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: changedLayout })

expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(2)
second.resolve(true)
})

it('re-pushes after a remount re-establishes host geometry', () => {
const pusher = createRemotePaneLayoutPusher()
pusher.push({ worktreeId: 'wt-1', tabId: 'tab-1', layout: makeLayout() })
createRemotePaneLayoutPusher().push({
worktreeId: 'wt-1',
tabId: 'tab-1',
layout: makeLayout()
})
expect(updateWebRuntimePaneLayout).toHaveBeenCalledTimes(2)
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import type { TerminalLayoutSnapshot } from '../../../../shared/types'
import { terminalLayoutEqual } from '@/lib/terminal-layout-equality'
import { updateWebRuntimePaneLayout } from '@/runtime/web-runtime-session'

export type RemotePaneLayoutPusher = {
push: (input: { worktreeId: string; tabId: string; layout: TerminalLayoutSnapshot }) => void
}

/**
* Pane geometry is host-authoritative for remote tabs, so persists must push it — but
* persists also fire on pane-title churn, which leaves the host-visible layout untouched.
* Dedupe against the last push so unchanged layouts cost no remote round trip.
*/
export function createRemotePaneLayoutPusher(): RemotePaneLayoutPusher {
let lastAttempt: {
id: number
worktreeId: string
tabId: string
snapshot: TerminalLayoutSnapshot
} | null = null
let nextAttemptId = 0
return {
push: ({ worktreeId, tabId, layout }) => {
if (
lastAttempt?.worktreeId === worktreeId &&
lastAttempt.tabId === tabId &&
terminalLayoutEqual(lastAttempt.snapshot, layout)
) {
return
}
const attempt = { id: ++nextAttemptId, worktreeId, tabId, snapshot: layout }
lastAttempt = attempt
void updateWebRuntimePaneLayout({
worktreeId,
tabId,
root: layout.root,
expandedLeafId: layout.expandedLeafId,
...(layout.titlesByLeafId ? { titlesByLeafId: layout.titlesByLeafId } : {})
}).then((updated) => {
// Why: a disconnected or timed-out push carried no information, so the next persist must retry it.
if (!updated && lastAttempt?.id === attempt.id) {
lastAttempt = null
}
})
}
}
}
55 changes: 55 additions & 0 deletions src/renderer/src/lib/terminal-layout-equality.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import type { TerminalLayoutSnapshot, TerminalPaneLayoutNode } from '../../../shared/types'

function sameStringRecord(
a: Readonly<Record<string, string>> | undefined,
b: Readonly<Record<string, string>> | undefined
): boolean {
const left = a ?? {}
const right = b ?? {}
const leftKeys = Object.keys(left)
const rightKeys = Object.keys(right)
return (
leftKeys.length === rightKeys.length &&
leftKeys.every(
(key) => Object.prototype.hasOwnProperty.call(right, key) && left[key] === right[key]
)
)
}

export function terminalLayoutNodeEqual(
a: TerminalPaneLayoutNode | null | undefined,
b: TerminalPaneLayoutNode | null | undefined
): boolean {
if (!a || !b) {
return !a && !b
}
if (a.type !== b.type) {
return false
}
if (a.type === 'leaf') {
return b.type === 'leaf' && a.leafId === b.leafId
}
return (
b.type === 'split' &&
a.direction === b.direction &&
a.ratio === b.ratio &&
terminalLayoutNodeEqual(a.first, b.first) &&
terminalLayoutNodeEqual(a.second, b.second)
Comment on lines +32 to +37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Normalize the default split ratio before comparison.

Line 35 treats undefined and 0.5 as different values. The shared type defines an absent ratio as 0.5. A host snapshot that omits the default ratio will bypass store and IPC identity bailouts when the renderer writes 0.5.

Proposed fix
-    a.ratio === b.ratio &&
+    (a.ratio ?? 0.5) === (b.ratio ?? 0.5) &&

Add coverage for an omitted ratio versus 0.5.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return (
b.type === 'split' &&
a.direction === b.direction &&
a.ratio === b.ratio &&
terminalLayoutNodeEqual(a.first, b.first) &&
terminalLayoutNodeEqual(a.second, b.second)
return (
b.type === 'split' &&
a.direction === b.direction &&
(a.ratio ?? 0.5) === (b.ratio ?? 0.5) &&
terminalLayoutNodeEqual(a.first, b.first) &&
terminalLayoutNodeEqual(a.second, b.second)

)
}

/** Structural equality over every persisted layout field; drives store/IPC write bailouts. */
export function terminalLayoutEqual(
a: TerminalLayoutSnapshot | undefined,
b: TerminalLayoutSnapshot
): boolean {
return (
terminalLayoutNodeEqual(a?.root, b.root) &&
(a?.activeLeafId ?? null) === b.activeLeafId &&
(a?.expandedLeafId ?? null) === b.expandedLeafId &&
sameStringRecord(a?.ptyIdsByLeafId, b.ptyIdsByLeafId) &&
sameStringRecord(a?.buffersByLeafId, b.buffersByLeafId) &&
sameStringRecord(a?.scrollbackRefsByLeafId, b.scrollbackRefsByLeafId) &&
sameStringRecord(a?.titlesByLeafId, b.titlesByLeafId)
)
}
Loading