From d1502d1d53cc2f373283fbfc13e6bd06dd317002 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Fri, 11 Sep 2026 00:42:34 -0700 Subject: [PATCH] fix(mobile): stop a created tab from jumping when the host snapshot lands Creating a tab from the mobile session strip painted the new tab at the end of the strip and then visibly jumped it to a different slot a beat later. The client asked the host to insert the tab after the active tab, but then predicted a different placement for its own optimistic paint: afterTabId: activeSessionTabId ?? undefined // host: splice(insertAfter + 1) ... return [...prev, { ...created, isActive: true }] // client: append Two independent placements that disagree, so the optimistic frame is wrong by construction and the tab snaps to its real slot on the next published snapshot. The disagreement dates to 57a70d2ac0 ("Fix mobile session tab authority"), which introduced afterTabId and left the append in place. Before that the client used terminal.create with no anchor, so both sides appended and agreed. Rather than teach the client to re-derive the host's rule, both sides now call one shared placeCreatedSessionTab, and the client captures a single afterTabId for the request and the paint so they cannot drift apart again. The host change is a pure refactor onto the shared helper; the 1260-test runtime characterization suite is unchanged. The mobile route-parity hash pin moves once because handleCreateTerminal's body changed - it is the only one of the 12 extracted functions that differs. --- .../mobile-session-route-parity.test.ts | 5 +- ...le-session-terminal-create-actions.test.ts | 75 +++++++++++++++++++ ...-mobile-session-terminal-create-actions.ts | 8 +- ...e-runtime-owned-mobile-session-terminal.ts | 17 ++--- src/shared/session-tab-placement.test.ts | 47 ++++++++++++ src/shared/session-tab-placement.ts | 22 ++++++ 6 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 src/shared/session-tab-placement.test.ts create mode 100644 src/shared/session-tab-placement.ts 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 +}