diff --git a/mobile/src/session/mobile-session-route-parity.test.ts b/mobile/src/session/mobile-session-route-parity.test.ts index 3a6b04661de..e315b353ee3 100644 --- a/mobile/src/session/mobile-session-route-parity.test.ts +++ b/mobile/src/session/mobile-session-route-parity.test.ts @@ -69,8 +69,11 @@ const HEAD_CALLBACK_IDENTITY_SHA256 = const HEAD_CALLBACK_BODY_SHA256 = 'af7f3c62954250d4be7ee432ecd10dc2689792aad8230fed2d1d68bbc892d776' const HEAD_EFFECT_SHA256 = 'd9ebfaabc1e79773cdada7ab370b20459ed972f1f8edce1652199f4d0391cd13' const HEAD_CONTENT_HOOK_SHA256 = '9c3b612fef3f370d66873aefdbe1d701f20cb64ded31fef5cc45fde6f8189581' +// Moved once for the create-tab placement fix: handleCreateTerminal now paints the created tab +// after the anchor it sent the host instead of appending. Only that body changed; the other 11 +// extracted functions are byte-identical. const HEAD_NESTED_FUNCTION_SHA256 = - 'fde6679349ab2b8c30c7e627841ff99bd1dd24441ee95323d0aa70230422ae24' + '90b88dfc168c664d87b240af14f6c4e19fa506a3d1005fcd9ef29df1befc810c' const HEAD_NATIVE_REGISTRATION_SHA256 = 'cab85e4e4a3f43289ba93ddea9ccce57aea83e0bf14fd1620a965aad0c1cb49e' const HEAD_NATIVE_REMOVAL_SHA256 = diff --git a/mobile/src/session/use-mobile-session-terminal-create-actions.test.ts b/mobile/src/session/use-mobile-session-terminal-create-actions.test.ts index e46bf087f38..847ad1316c0 100644 --- a/mobile/src/session/use-mobile-session-terminal-create-actions.test.ts +++ b/mobile/src/session/use-mobile-session-terminal-create-actions.test.ts @@ -262,3 +262,78 @@ describe('mobile + Codex tab creation routing', () => { } ) }) + +describe('optimistic placement of a created tab', () => { + let renderer: ReactTestRenderer | undefined + afterEach(() => renderer?.unmount()) + + async function createLegacyTerminal(scope: ReturnType) { + let actions: ReturnType | undefined + function Harness() { + actions = useMobileSessionTerminalCreateActions(scope as never) + return null + } + await act(async () => { + renderer = create(createElement(Harness)) + }) + await act(async () => { + await actions?.handleCreateTerminal() + }) + } + + function tabIdsAfterCreate( + scope: ReturnType, + prior: { id: string }[] + ): string[] { + const updater = (scope.setSessionTabs as ReturnType).mock.calls.at(-1)?.[0] as ( + prev: { id: string }[] + ) => { id: string }[] + return updater(prior).map((tab) => tab.id) + } + + it('paints the created tab after the anchor it asked the host for, not at the end', async () => { + const scope = createScope(clientReturning(terminalCreateResponse())) + await createLegacyTerminal(scope) + + expect(scope.setSessionTabs).toHaveBeenCalled() + // The request anchored on the active tab, so the paint must land in the same slot the host + // splices into; appending here is what made the tab jump on the next snapshot. + expect(tabIdsAfterCreate(scope, [{ id: 'existing-tab' }, { id: 'trailing-tab' }])).toEqual([ + 'existing-tab', + 'terminal-tab-1', + 'trailing-tab' + ]) + }) + + it('sends the same anchor it paints with', async () => { + const scope = createScope(clientReturning(terminalCreateResponse())) + await createLegacyTerminal(scope) + + expect(scope.client.sendRequest).toHaveBeenCalledWith( + 'session.tabs.createTerminal', + expect.objectContaining({ afterTabId: 'existing-tab' }) + ) + }) + + it('appends when the anchor is not in the client list, matching the host fallback', async () => { + const scope = createScope(clientReturning(terminalCreateResponse())) + await createLegacyTerminal(scope) + + expect(tabIdsAfterCreate(scope, [{ id: 'unrelated-tab' }])).toEqual([ + 'unrelated-tab', + 'terminal-tab-1' + ]) + }) + + it('leaves the list alone when the host snapshot already placed the tab', async () => { + const scope = createScope(clientReturning(terminalCreateResponse())) + await createLegacyTerminal(scope) + + const prior = [{ id: 'existing-tab' }, { id: 'terminal-tab-1' }, { id: 'trailing-tab' }] + expect(tabIdsAfterCreate(scope, prior)).toEqual([ + 'existing-tab', + 'terminal-tab-1', + 'trailing-tab' + ]) + }) +}) diff --git a/mobile/src/session/use-mobile-session-terminal-create-actions.ts b/mobile/src/session/use-mobile-session-terminal-create-actions.ts index cf6e9441d10..11548876f60 100644 --- a/mobile/src/session/use-mobile-session-terminal-create-actions.ts +++ b/mobile/src/session/use-mobile-session-terminal-create-actions.ts @@ -12,6 +12,7 @@ import type { Terminal, TerminalCreateResult } from './mobile-session-route-type import type { MobileSessionAttachmentsModel } from './use-mobile-session-attachments' import { isAgentSessionHandleProvider } from '../../../src/shared/agent-session-provider-handle' import { createMobileStructuredAgentSession } from './mobile-structured-agent-session-launch' +import { placeCreatedSessionTab } from '../../../src/shared/session-tab-placement' export function useMobileSessionTerminalCreateActions(scope: MobileSessionAttachmentsModel) { const { @@ -93,9 +94,12 @@ export function useMobileSessionTerminalCreateActions(scope: MobileSessionAttach return } } + // Why: one anchor for both the request and the optimistic paint below; when they disagreed the + // new tab painted at the end and jumped to its real slot on the next host snapshot. + const afterTabId = activeSessionTabId ?? undefined const response = await client.sendRequest('session.tabs.createTerminal', { worktree: `id:${worktreeId}`, - afterTabId: activeSessionTabId ?? undefined, + afterTabId, clientMutationId, ...(options?.startupCommand ? { command: options.startupCommand } : {}), ...(options?.startupCommandDelivery @@ -123,7 +127,7 @@ export function useMobileSessionTerminalCreateActions(scope: MobileSessionAttach if (prev.some((tab) => tab.id === created.id)) { return prev } - return [...prev, { ...created, isActive: true }] + return placeCreatedSessionTab(prev, { ...created, isActive: true }, afterTabId) }) if (typeof created.terminal === 'string') { const createdHandle = created.terminal diff --git a/src/main/runtime/orca-runtime-create-runtime-owned-mobile-session-terminal.ts b/src/main/runtime/orca-runtime-create-runtime-owned-mobile-session-terminal.ts index 1bb45f838e0..49c7bbf766c 100644 --- a/src/main/runtime/orca-runtime-create-runtime-owned-mobile-session-terminal.ts +++ b/src/main/runtime/orca-runtime-create-runtime-owned-mobile-session-terminal.ts @@ -10,6 +10,7 @@ import type { } from '../../shared/runtime-types' import { randomUUID } from 'node:crypto' import { parsePaneKey } from '../../shared/stable-pane-id' +import { placeCreatedSessionTab } from '../../shared/session-tab-placement' import { buildHeadlessMobileSessionTabGroups, buildMaterializedHeadlessParentLayout, @@ -103,21 +104,17 @@ export class OrcaRuntimeWithCreateRuntimeOwnedMobileSessionTerminal extends Orca parentLayout, isActive: activate } - const tabs = (existing?.tabs ?? []) - .filter((candidate) => candidate.id !== tab.id) - .map((candidate) => ({ + const tabs = placeCreatedSessionTab( + (existing?.tabs ?? []).map((candidate) => ({ ...candidate, ...(candidate.type === 'terminal' && candidate.parentTabId === parentTabId ? { parentLayout } : {}), isActive: activate ? false : candidate.isActive - })) - const insertAfter = afterTabId ? tabs.findIndex((candidate) => candidate.id === afterTabId) : -1 - if (insertAfter >= 0) { - tabs.splice(insertAfter + 1, 0, tab) - } else { - tabs.push(tab) - } + })), + tab, + afterTabId + ) const next: RuntimeMobileSessionTabsSnapshot = { worktree: worktreeId, // Why: a fresh epoch retires the current publisher, so clients drop its later tab updates. diff --git a/src/shared/session-tab-placement.test.ts b/src/shared/session-tab-placement.test.ts new file mode 100644 index 00000000000..f57d2ebf9cb --- /dev/null +++ b/src/shared/session-tab-placement.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest' +import { placeCreatedSessionTab } from './session-tab-placement' + +const created = { id: 'new' } + +describe('placeCreatedSessionTab', () => { + it('inserts directly after the anchor', () => { + expect( + placeCreatedSessionTab([{ id: 'a' }, { id: 'b' }, { id: 'c' }], created, 'a').map((t) => t.id) + ).toEqual(['a', 'new', 'b', 'c']) + }) + + it('appends when the anchor is the last tab', () => { + expect( + placeCreatedSessionTab([{ id: 'a' }, { id: 'b' }], created, 'b').map((t) => t.id) + ).toEqual(['a', 'b', 'new']) + }) + + it('appends when the anchor is absent', () => { + expect( + placeCreatedSessionTab([{ id: 'a' }, { id: 'b' }], created, 'missing').map((t) => t.id) + ).toEqual(['a', 'b', 'new']) + }) + + it('appends when no anchor is given', () => { + for (const anchor of [undefined, null, '']) { + expect(placeCreatedSessionTab([{ id: 'a' }], created, anchor).map((t) => t.id)).toEqual([ + 'a', + 'new' + ]) + } + }) + + it('re-places a tab that is already in the list instead of duplicating it', () => { + expect( + placeCreatedSessionTab([{ id: 'a' }, { id: 'new' }, { id: 'b' }], created, 'b').map( + (t) => t.id + ) + ).toEqual(['a', 'b', 'new']) + }) + + it('does not mutate the input list', () => { + const tabs = [{ id: 'a' }, { id: 'b' }] + placeCreatedSessionTab(tabs, created, 'a') + expect(tabs.map((t) => t.id)).toEqual(['a', 'b']) + }) +}) diff --git a/src/shared/session-tab-placement.ts b/src/shared/session-tab-placement.ts new file mode 100644 index 00000000000..7409414daf3 --- /dev/null +++ b/src/shared/session-tab-placement.ts @@ -0,0 +1,22 @@ +/** + * Where a newly created session tab lands in a tab list. + * + * Shared because a client that optimistically paints a created tab and the host that publishes + * the authoritative snapshot must agree. When they disagreed — the client appending while the + * host spliced after `afterTabId` — the new tab painted at the end and then visibly jumped to its + * real slot as soon as the host frame landed. + */ +export function placeCreatedSessionTab( + tabs: readonly T[], + created: T, + afterTabId: string | null | undefined +): T[] { + const next = tabs.filter((tab) => tab.id !== created.id) + const anchor = afterTabId ? next.findIndex((tab) => tab.id === afterTabId) : -1 + if (anchor < 0) { + next.push(created) + return next + } + next.splice(anchor + 1, 0, created) + return next +}