From c2acb3c11bc281f64d69f2e31352b32275d11dc7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 18 Jun 2026 09:15:53 +0800 Subject: [PATCH] fix(mobile): open the file tab when a terminal file path is tapped --- .../app/h/[hostId]/session/[worktreeId].tsx | 181 +++++++++---- .../session/opened-mobile-session-tab.test.ts | 238 ++++++++++++++++++ .../src/session/opened-mobile-session-tab.ts | 85 +++++++ 3 files changed, 461 insertions(+), 43 deletions(-) create mode 100644 mobile/src/session/opened-mobile-session-tab.test.ts create mode 100644 mobile/src/session/opened-mobile-session-tab.ts diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index 2d9b1c1ee..fce16261e 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -126,6 +126,11 @@ import { mobileSessionTabsEqual, terminalRecordsEqual } from '../../../../src/session/mobile-terminal-records' +import { + activateOpenedMobileSessionTab, + refreshOpenedMobileSessionTabs, + shouldActivateOpenedMobileSessionTab +} from '../../../../src/session/opened-mobile-session-tab' import { buildMobileNewTabAgentOptions, type MobileNewTabAgentOption, @@ -191,6 +196,15 @@ import type { TerminalGestureInputQueue } from './mobile-session-route-types' +type PendingBrowserFocus = { + pageId: string + shouldFocus?: () => boolean +} + +type CreateBrowserOptions = { + shouldFocus?: () => boolean +} + function getActiveTabIdForHandle( tabs: MobileSessionTab[], terminalHandle: string | null @@ -922,7 +936,7 @@ export default function SessionScreen() { // the app-level active tab). We remember the page id and, once its session tab // syncs, activate it through the normal switchSessionTab path (which also makes // switching back to the terminal work). A ref breaks the callback dep cycle. - const pendingBrowserFocusPageIdRef = useRef(null) + const pendingBrowserFocusRef = useRef(null) const switchSessionTabRef = useRef<((tab: MobileSessionTab) => void) | null>(null) const initialEmptySessionAutoCreateRef = useRef(null) const markdownSaveSeqRef = useRef>(new Map()) @@ -931,6 +945,7 @@ export default function SessionScreen() { // Why: post-RPC refresh timers capture this screen and must not survive // route reuse or unmount. const delayedActionTimersRef = useRef>>(new Set()) + const terminalFileTapActivationSeqRef = useRef(0) // Why: server-side layout state machine emits a monotonic seq on every // applyLayout. Track the highest seq we've observed per handle and drop // any scrollback/resized event with a strictly older seq — these are @@ -2145,44 +2160,65 @@ export default function SessionScreen() { [client, markdownDocs, showToast, worktreeId] ) - const fetchSessionTabsInFlightRef = useRef(false) + // Why: activation callers need the fresh tab snapshot; await an existing + // refresh instead of reading stale refs while another list request is running. + const fetchSessionTabsInFlightRef = useRef | null>(null) const fetchSessionTabs = useCallback(async () => { if (!client) { return } if (fetchSessionTabsInFlightRef.current) { + await fetchSessionTabsInFlightRef.current return } - fetchSessionTabsInFlightRef.current = true - try { - const response = await client.sendRequest('session.tabs.list', { - worktree: `id:${worktreeId}` - }) - if (!response.ok) { - return - } - const result = (response as RpcSuccess).result as SessionTabsResult - applySessionTabs(result) - // Focus a just-opened browser tab once it appears in the snapshot, via the - // normal activate path so it sticks and the user can still switch away. - const pendingPageId = pendingBrowserFocusPageIdRef.current - if (pendingPageId) { - const browserTab = result.tabs.find( - (tab) => tab.type === 'browser' && tab.browserPageId === pendingPageId - ) - if (browserTab) { - pendingBrowserFocusPageIdRef.current = null - switchSessionTabRef.current?.(browserTab) + const request = (async () => { + try { + const response = await client.sendRequest('session.tabs.list', { + worktree: `id:${worktreeId}` + }) + if (!response.ok) { + return } + const result = (response as RpcSuccess).result as SessionTabsResult + applySessionTabs(result) + // Focus a just-opened browser tab once it appears in the snapshot, via the + // normal activate path so it sticks and the user can still switch away. + const pendingBrowserFocus = pendingBrowserFocusRef.current + if (pendingBrowserFocus?.shouldFocus && !pendingBrowserFocus.shouldFocus()) { + pendingBrowserFocusRef.current = null + } else if (pendingBrowserFocus) { + const browserTab = result.tabs.find( + (tab) => tab.type === 'browser' && tab.browserPageId === pendingBrowserFocus.pageId + ) + if (browserTab) { + pendingBrowserFocusRef.current = null + switchSessionTabRef.current?.(browserTab) + } + } + } catch { + // Keep the last tab snapshot visible during reconnect/backoff. } - } catch { - // Keep the last tab snapshot visible during reconnect/backoff. + })() + fetchSessionTabsInFlightRef.current = request + try { + await request } finally { - fetchSessionTabsInFlightRef.current = false + if (fetchSessionTabsInFlightRef.current === request) { + fetchSessionTabsInFlightRef.current = null + } } }, [applySessionTabs, client, worktreeId]) + const refreshOpenedTabs = useCallback( + async () => + refreshOpenedMobileSessionTabs({ + getCurrentRefresh: () => fetchSessionTabsInFlightRef.current, + refreshSessionTabs: fetchSessionTabs + }), + [fetchSessionTabs] + ) + useEffect(() => { if (connState === 'connected') { return @@ -2376,7 +2412,7 @@ export default function SessionScreen() { activeSessionTabTypeRef.current = null pendingActiveSessionTabIdRef.current = null pendingActiveTerminalHandleRef.current = null - pendingBrowserFocusPageIdRef.current = null + pendingBrowserFocusRef.current = null initialEmptySessionAutoCreateRef.current = null for (const queued of terminalGestureInputQueuesRef.current.values()) { if (queued.timer) { @@ -2868,6 +2904,19 @@ export default function SessionScreen() { if (handle !== activeHandleRef.current || !client) { return } + const activationSeq = terminalFileTapActivationSeqRef.current + 1 + terminalFileTapActivationSeqRef.current = activationSeq + const sourceTerminalHandle = handle + let activated = false + const shouldActivate = (): boolean => + shouldActivateOpenedMobileSessionTab({ + activated, + activationSeq, + latestActivationSeq: terminalFileTapActivationSeqRef.current, + sourceTerminalHandle, + activeTerminalHandle: activeHandleRef.current, + activeTabType: activeSessionTabTypeRef.current + }) void (async () => { try { const worktree = `id:${worktreeId}` @@ -2883,12 +2932,17 @@ export default function SessionScreen() { if (!resolved.exists || resolved.isDirectory || !resolved.relativePath) { return } + if (!shouldActivate()) { + return + } // Confirm the tap landed on something openable before giving feedback. triggerSelection() // Why: HTML opens in a browser pane (streamed from the desktop), // matching desktop's terminal-click behavior, instead of a file view. if (classifyMobileArtifact(resolved.relativePath) === 'html' && resolved.absolutePath) { - void handleCreateBrowser('file://' + resolved.absolutePath) + void handleCreateBrowser('file://' + resolved.absolutePath, { + shouldFocus: shouldActivate + }) return } const openResponse = await client.sendRequest( @@ -2899,18 +2953,48 @@ export default function SessionScreen() { if (!openResponse.ok) { return } - // Why: the desktop creates the file tab asynchronously; a single poll - // can race it, so refresh a few times to reliably pick it up and - // switch to it (the file browser gets this for free via router.back). - scheduleDelayedAction(() => void fetchSessionTabs(), 300) - scheduleDelayedAction(() => void fetchSessionTabs(), 900) - scheduleDelayedAction(() => void fetchSessionTabs(), 1800) + // Why: the host creates the file tab asynchronously, and from a terminal + // the active tab stays on the terminal — so we must explicitly switch to + // the new file tab once it syncs in (the file browser gets this for free + // by popping back to an already-active tab). Poll a few times since the + // tab may take a moment to appear. + const openedPath = resolved.relativePath + const activateOpenedFile = async (): Promise => { + const didActivate = await activateOpenedMobileSessionTab({ + relativePath: openedPath, + fetchSessionTabs: refreshOpenedTabs, + getTabs: () => sessionTabsRef.current, + getActiveTabId: () => activeSessionTabIdRef.current, + getActivationState: () => ({ + activated, + activationSeq, + latestActivationSeq: terminalFileTapActivationSeqRef.current, + sourceTerminalHandle, + activeTerminalHandle: activeHandleRef.current, + activeTabType: activeSessionTabTypeRef.current + }), + switchSessionTab: (opened) => { + const switchSessionTab = switchSessionTabRef.current + if (!switchSessionTab) { + return false + } + switchSessionTab(opened) + return true + } + }) + if (didActivate) { + activated = true + } + } + scheduleDelayedAction(() => void activateOpenedFile(), 300) + scheduleDelayedAction(() => void activateOpenedFile(), 900) + scheduleDelayedAction(() => void activateOpenedFile(), 1800) } catch { // Resolution/open is best-effort; a failed tap silently no-ops. } })() }, - [client, worktreeId, scheduleDelayedAction, fetchSessionTabs] + [client, worktreeId, scheduleDelayedAction, refreshOpenedTabs] ) const handleTerminalOpenUrl = useCallback( @@ -3685,7 +3769,10 @@ export default function SessionScreen() { } } - async function handleCreateBrowser(rawUrl = 'about:blank'): Promise { + async function handleCreateBrowser( + rawUrl = 'about:blank', + options?: CreateBrowserOptions + ): Promise { if (!client || creatingBrowser) { return false } @@ -3702,33 +3789,41 @@ export default function SessionScreen() { showToast(message, 1400) return false } + if (options?.shouldFocus && !options.shouldFocus()) { + return false + } setCreatingBrowser(true) setCreateError('') + const hasFocusGuard = options?.shouldFocus != null try { const response = await client.sendRequest( 'browser.tabCreate', { worktree: `id:${worktreeId}`, url, - // The user opened this tab (tapped HTML / address bar) → focus it. - activate: true + // Why: terminal HTML taps may become stale while browser creation is + // in flight. Defer activation until the guarded mobile focus path runs. + activate: !hasFocusGuard }, { timeoutMs: 30_000 } ) if (!response.ok) { throw new Error((response as RpcFailure).error.message) } - // Focus the new browser tab once it syncs (fetchSessionTabs activates it + // Focus the new browser tab once it syncs (refreshOpenedTabs activates it // via the normal path). Refresh a few times since the desktop registers // the tab asynchronously. const created = (response as RpcSuccess).result as { browserPageId?: string } - if (created.browserPageId) { - pendingBrowserFocusPageIdRef.current = created.browserPageId + if (created.browserPageId && (!options?.shouldFocus || options.shouldFocus())) { + pendingBrowserFocusRef.current = { + pageId: created.browserPageId, + shouldFocus: options?.shouldFocus + } } - void fetchSessionTabs() - scheduleDelayedAction(() => void fetchSessionTabs(), 400) - scheduleDelayedAction(() => void fetchSessionTabs(), 1200) + void refreshOpenedTabs() + scheduleDelayedAction(() => void refreshOpenedTabs(), 400) + scheduleDelayedAction(() => void refreshOpenedTabs(), 1200) return true } catch (err) { const message = err instanceof Error ? err.message : 'Failed to create browser' diff --git a/mobile/src/session/opened-mobile-session-tab.test.ts b/mobile/src/session/opened-mobile-session-tab.test.ts new file mode 100644 index 000000000..104ee312b --- /dev/null +++ b/mobile/src/session/opened-mobile-session-tab.test.ts @@ -0,0 +1,238 @@ +import { describe, expect, it } from 'vitest' +import { + activateOpenedMobileSessionTab, + findOpenedMobileSessionTab, + refreshOpenedMobileSessionTabs, + shouldActivateOpenedMobileSessionTab, + type OpenedMobileSessionTabCandidate +} from './opened-mobile-session-tab' + +describe('findOpenedMobileSessionTab', () => { + it('matches a file tab by relative path', () => { + const tabs: OpenedMobileSessionTabCandidate[] = [ + { type: 'terminal', id: 'term' }, + { type: 'file', id: 'file-1', relativePath: 'src/app.ts' } + ] + + expect(findOpenedMobileSessionTab(tabs, 'src/app.ts')?.id).toBe('file-1') + }) + + it('matches a markdown tab by relative path', () => { + const tabs: OpenedMobileSessionTabCandidate[] = [ + { type: 'file', id: 'file-1', relativePath: 'src/app.ts' }, + { type: 'markdown', id: 'md-1', relativePath: 'README.md' } + ] + + expect(findOpenedMobileSessionTab(tabs, 'README.md')?.id).toBe('md-1') + }) + + it('ignores non-openable tabs even if they carry path-shaped data', () => { + const tabs: OpenedMobileSessionTabCandidate[] = [ + { type: 'browser', id: 'browser-1', relativePath: 'README.md' }, + { type: 'terminal', id: 'term-1', relativePath: 'README.md' } + ] + + expect(findOpenedMobileSessionTab(tabs, 'README.md')).toBeNull() + }) + + it('matches future file-like tab types that carry a relative path', () => { + const tabs: OpenedMobileSessionTabCandidate[] = [ + { type: 'image', id: 'image-1', relativePath: 'assets/logo.png' } + ] + + expect(findOpenedMobileSessionTab(tabs, 'assets/logo.png')?.id).toBe('image-1') + }) + + it('skips diff tabs when an edit tab has the same relative path', () => { + const tabs: OpenedMobileSessionTabCandidate[] = [ + { type: 'file', id: 'diff-1', mode: 'diff', relativePath: 'src/app.ts' }, + { type: 'file', id: 'edit-1', mode: 'edit', relativePath: 'src/app.ts' } + ] + + expect(findOpenedMobileSessionTab(tabs, 'src/app.ts')?.id).toBe('edit-1') + }) +}) + +describe('refreshOpenedMobileSessionTabs', () => { + it('waits for a current refresh before requesting a post-open snapshot', async () => { + const order: string[] = [] + let resolveCurrentRefresh: () => void = () => {} + const currentRefresh = new Promise((resolve) => { + resolveCurrentRefresh = resolve + }) + + const refresh = refreshOpenedMobileSessionTabs({ + getCurrentRefresh: () => currentRefresh, + refreshSessionTabs: async () => { + order.push('fresh') + } + }) + + await Promise.resolve() + expect(order).toEqual([]) + + order.push('current') + resolveCurrentRefresh() + await refresh + + expect(order).toEqual(['current', 'fresh']) + }) +}) + +describe('activateOpenedMobileSessionTab', () => { + const fileTab = { type: 'file', id: 'file-1', relativePath: 'src/app.ts' } + + function createActivationHarness() { + let tabs: OpenedMobileSessionTabCandidate[] = [] + let activeTabId: string | null = 'terminal-1' + let activated = false + const activationSeq = 1 + let latestActivationSeq = 1 + let activeTerminalHandle: string | null = 'pty-1' + let activeTabType: string | null = 'terminal' + const switched: string[] = [] + return { + setTabs(nextTabs: OpenedMobileSessionTabCandidate[]) { + tabs = nextTabs + }, + markActivated() { + activated = true + }, + supersedeActivation() { + latestActivationSeq += 1 + }, + leaveSourceTerminal() { + activeTerminalHandle = null + activeTabType = 'file' + }, + options(fetchSessionTabs: () => Promise) { + return { + relativePath: 'src/app.ts', + fetchSessionTabs, + getTabs: () => tabs, + getActiveTabId: () => activeTabId, + getActivationState: () => ({ + activated, + activationSeq, + latestActivationSeq, + sourceTerminalHandle: 'pty-1', + activeTerminalHandle, + activeTabType + }), + switchSessionTab: (tab: OpenedMobileSessionTabCandidate) => { + activeTabId = tab.id + activeTabType = tab.type + activeTerminalHandle = null + switched.push(tab.id) + return true + } + } + }, + switched + } + } + + it('refreshes tabs and switches to the opened file tab', async () => { + const harness = createActivationHarness() + + const activated = await activateOpenedMobileSessionTab( + harness.options(async () => { + harness.setTabs([fileTab]) + }) + ) + + expect(activated).toBe(true) + expect(harness.switched).toEqual(['file-1']) + }) + + it('does not switch when a newer tap supersedes this attempt during refresh', async () => { + const harness = createActivationHarness() + + const activated = await activateOpenedMobileSessionTab( + harness.options(async () => { + harness.setTabs([fileTab]) + harness.supersedeActivation() + }) + ) + + expect(activated).toBe(false) + expect(harness.switched).toEqual([]) + }) + + it('does not switch when the user leaves the source terminal during refresh', async () => { + const harness = createActivationHarness() + + const activated = await activateOpenedMobileSessionTab( + harness.options(async () => { + harness.setTabs([fileTab]) + harness.leaveSourceTerminal() + }) + ) + + expect(activated).toBe(false) + expect(harness.switched).toEqual([]) + }) + + it('stops retrying after an earlier attempt already activated the tab', async () => { + const harness = createActivationHarness() + harness.markActivated() + + const activated = await activateOpenedMobileSessionTab( + harness.options(async () => { + harness.setTabs([fileTab]) + }) + ) + + expect(activated).toBe(false) + expect(harness.switched).toEqual([]) + }) +}) + +describe('shouldActivateOpenedMobileSessionTab', () => { + const currentTerminalState = { + activationSeq: 2, + latestActivationSeq: 2, + sourceTerminalHandle: 'pty-1', + activeTerminalHandle: 'pty-1', + activeTabType: 'terminal' + } + + it('allows the latest tap while the source terminal is still active', () => { + expect( + shouldActivateOpenedMobileSessionTab({ + ...currentTerminalState, + activated: false + }) + ).toBe(true) + }) + + it('stops later retries after the first successful activation', () => { + expect( + shouldActivateOpenedMobileSessionTab({ + ...currentTerminalState, + activated: true + }) + ).toBe(false) + }) + + it('prevents an older tap from stealing focus from a newer tap', () => { + expect( + shouldActivateOpenedMobileSessionTab({ + ...currentTerminalState, + activated: false, + activationSeq: 1 + }) + ).toBe(false) + }) + + it('does not activate after the user leaves the source terminal', () => { + expect( + shouldActivateOpenedMobileSessionTab({ + ...currentTerminalState, + activated: false, + activeTabType: 'file', + activeTerminalHandle: null + }) + ).toBe(false) + }) +}) diff --git a/mobile/src/session/opened-mobile-session-tab.ts b/mobile/src/session/opened-mobile-session-tab.ts new file mode 100644 index 000000000..16cecb0f3 --- /dev/null +++ b/mobile/src/session/opened-mobile-session-tab.ts @@ -0,0 +1,85 @@ +export type OpenedMobileSessionTabCandidate = { + id: string + type: string + mode?: unknown + relativePath?: unknown +} + +export type OpenedMobileSessionTabActivationState = { + activated: boolean + activationSeq: number + latestActivationSeq: number + sourceTerminalHandle: string + activeTerminalHandle: string | null + activeTabType: string | null +} + +export type ActivateOpenedMobileSessionTabOptions = { + relativePath: string + fetchSessionTabs: () => Promise + getTabs: () => readonly T[] + getActiveTabId: () => string | null + getActivationState: () => OpenedMobileSessionTabActivationState + switchSessionTab: (tab: T) => boolean +} + +export type RefreshOpenedMobileSessionTabsOptions = { + getCurrentRefresh: () => Promise | null + refreshSessionTabs: () => Promise +} + +export async function refreshOpenedMobileSessionTabs( + options: RefreshOpenedMobileSessionTabsOptions +): Promise { + const currentRefresh = options.getCurrentRefresh() + if (currentRefresh) { + await currentRefresh + } + await options.refreshSessionTabs() +} + +export function findOpenedMobileSessionTab( + tabs: readonly T[], + relativePath: string +): T | null { + return ( + tabs.find( + (tab) => + tab.type !== 'browser' && + tab.type !== 'terminal' && + tab.mode !== 'diff' && + tab.relativePath === relativePath + ) ?? null + ) +} + +export function shouldActivateOpenedMobileSessionTab( + state: OpenedMobileSessionTabActivationState +): boolean { + return ( + !state.activated && + state.activationSeq === state.latestActivationSeq && + state.activeTabType === 'terminal' && + state.activeTerminalHandle === state.sourceTerminalHandle + ) +} + +export async function activateOpenedMobileSessionTab( + options: ActivateOpenedMobileSessionTabOptions +): Promise { + if (!shouldActivateOpenedMobileSessionTab(options.getActivationState())) { + return false + } + await options.fetchSessionTabs() + if (!shouldActivateOpenedMobileSessionTab(options.getActivationState())) { + return false + } + const opened = findOpenedMobileSessionTab(options.getTabs(), options.relativePath) + if (!opened) { + return false + } + if (options.getActiveTabId() === opened.id) { + return true + } + return options.switchSessionTab(opened) +}