diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tabs.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tabs.tsx index 66d91c0e0be..19627b45254 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tabs.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tabs.tsx @@ -408,11 +408,19 @@ export function ResourceTabs({ const handleClose = useCallback( (id: string) => { - const resource = resources.find((r) => r.id === id) + const index = resources.findIndex((r) => r.id === id) + const resource = resources[index] if (!resource) return const isMulti = selectedIds.has(resource.id) && selectedIds.size > 1 const targets = isMulti ? resources.filter((r) => selectedIds.has(r.id)) : [resource] if (!confirmClosingRunningTerminals(targets, terminalTabs)) return + // Closing the shown tab moves to its neighbour, right then left, the way + // the desktop app picks the next native tab, so the strip does not fall + // back to its last tab and jump once the close lands. + if (!isMulti && activeId === resource.id) { + const nextId = findNearestId(resources, index, null) + if (nextId) selectResource(nextId) + } // A browser tab's page is closed natively and its resource dropped at // once; the tab list then confirms the removal. A shell's close answers // with the tab list, so its resource follows that list instead — a @@ -451,7 +459,16 @@ export function ResourceTabs({ } }, // eslint-disable-next-line react-hooks/exhaustive-deps - [chatId, desktopScopeId, onRemoveResource, resources, selectedIds, terminalTabs] + [ + activeId, + chatId, + desktopScopeId, + onRemoveResource, + resources, + selectResource, + selectedIds, + terminalTabs, + ] ) /** diff --git a/apps/sim/app/workspace/[workspaceId]/home/home.tsx b/apps/sim/app/workspace/[workspaceId]/home/home.tsx index c169af084e8..1721d8e61b4 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/home.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/home.tsx @@ -334,24 +334,21 @@ export function Home({ chatId, userName, userId }: HomeProps) { [setActiveResourceId, clearResourceActivity] ) - const desktopTabResourceCallbacks = { + const desktopTabResourceOptions = { + scopeId: desktopScopeId, + resources, + activeResourceId, + selectedResourceId: activeResourceParam, + // A chat without an id has nothing stored to wait for. + hydrated: resolvedChatId === undefined || !isChatHistoryPending, addResource, removeResource, selectResource: selectResourceFromUser, + restoreResource: setActiveResourceId, onResourceEvent: handleResourceEvent, } - useBrowserTabResources({ - scopeId: desktopScopeId, - resources, - activeResourceId, - ...desktopTabResourceCallbacks, - }) - useTerminalTabResources({ - scopeId: desktopScopeId, - resources, - activeResourceId, - ...desktopTabResourceCallbacks, - }) + useBrowserTabResources(desktopTabResourceOptions) + useTerminalTabResources(desktopTabResourceOptions) const addResourceFromUser = useCallback( (resource: MothershipResource) => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.test.tsx index 2563e158eac..44ea000b42e 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.test.tsx @@ -6,6 +6,7 @@ import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { MothershipResource } from '@/lib/copilot/resources/types' import { useBrowserTabResources } from '@/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources' +import type { DesktopTabStripOptions } from '@/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources' import { useBrowserSessionStore } from '@/stores/browser-session/store' const { sendBrowserPanelAction, openUrlInNewBrowserTab, openInPanelListeners } = vi.hoisted(() => ({ @@ -37,17 +38,7 @@ function pushTabs(scopeId: string, tabs: ReturnType[], activeTabId: }) } -interface HostProps { - scopeId: string - resources: MothershipResource[] - activeResourceId: string | null - addResource: (resource: MothershipResource) => void - removeResource: (type: MothershipResource['type'], id: string) => void - selectResource: (id: string) => void - onResourceEvent: (id: string, options?: { activate?: boolean }) => void -} - -function Host(props: HostProps) { +function Host(props: DesktopTabStripOptions) { useBrowserTabResources(props) return null } @@ -58,21 +49,26 @@ describe('useBrowserTabResources', () => { const addResource = vi.fn() const removeResource = vi.fn() const selectResource = vi.fn() + const restoreResource = vi.fn() const onResourceEvent = vi.fn() - function render(overrides: Partial = {}) { - const props: HostProps = { + function render(overrides: Partial = {}) { + const props: DesktopTabStripOptions = { scopeId: SCOPE, resources: [], activeResourceId: null, + selectedResourceId: null, + hydrated: true, addResource, removeResource, selectResource, + restoreResource, onResourceEvent, ...overrides, } act(() => root.render()) - return (next: Partial) => act(() => root.render()) + return (next: Partial) => + act(() => root.render()) } beforeEach(() => { @@ -153,11 +149,27 @@ describe('useBrowserTabResources', () => { { type: 'browser', id: '1', title: 'Page 1' }, { type: 'browser', id: '2', title: 'Page 2' }, ] - const rerender = render({ resources, activeResourceId: '1' }) + const rerender = render({ resources, activeResourceId: '1', selectedResourceId: '1' }) pushTabs(SCOPE, [tab('1', true), tab('2')], '1') expect(sendBrowserPanelAction).not.toHaveBeenCalled() - rerender({ activeResourceId: '2' }) + rerender({ activeResourceId: '2', selectedResourceId: '2' }) + expect(sendBrowserPanelAction).toHaveBeenCalledExactlyOnceWith( + 'switch-tab', + { tabId: '2', claim: false }, + SCOPE + ) + + // The requested switch landing is not a native change to follow. + pushTabs(SCOPE, [tab('1'), tab('2', true)], '2') + expect(selectResource).not.toHaveBeenCalled() + }) + + it('shows a page selected before the pages landed, once it arrives', () => { + render({ selectedResourceId: '2', activeResourceId: '2' }) + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') expect(sendBrowserPanelAction).toHaveBeenCalledExactlyOnceWith( 'switch-tab', { tabId: '2', claim: false }, @@ -169,13 +181,182 @@ describe('useBrowserTabResources', () => { expect(selectResource).not.toHaveBeenCalled() }) + it('adopts the native active page on reopen instead of pushing the fallback tab', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + { type: 'browser', id: '3', title: 'Page 3' }, + ] + // The user left this chat on page 2. On reopen the strip starts empty, the + // pages land, and it falls back to its last tab until it learns better. + const rerender = render() + pushTabs(SCOPE, [tab('1'), tab('2', true), tab('3')], '2') + rerender({ resources, activeResourceId: '3', selectedResourceId: null }) + + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + expect(restoreResource).toHaveBeenCalledExactlyOnceWith('2') + expect(selectResource).not.toHaveBeenCalled() + + // The adopted tab is now both the selection and the native page: settled. + restoreResource.mockClear() + rerender({ activeResourceId: '2', selectedResourceId: '2' }) + expect(restoreResource).not.toHaveBeenCalled() + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + }) + + it('adopts the native active page when the selection is stale', () => { + const rerender = render({ selectedResourceId: 'deleted-file' }) + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + rerender({ + resources: [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ], + activeResourceId: '2', + }) + + expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1') + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + }) + + it('leaves a fallback that is not a browser tab alone', () => { + const rerender = render() + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + rerender({ + resources: [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'file', id: 'f', title: 'notes.md' }, + ], + activeResourceId: 'f', + selectedResourceId: null, + }) + + expect(restoreResource).not.toHaveBeenCalled() + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + }) + + it('adopts the native active page only once the chat history has been applied', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ] + const rerender = render({ hydrated: false }) + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + rerender({ resources, activeResourceId: '2', selectedResourceId: null, hydrated: false }) + expect(restoreResource).not.toHaveBeenCalled() + + rerender({ resources, activeResourceId: '2', selectedResourceId: null, hydrated: true }) + expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1') + }) + + it('leaves an explicitly selected page alone when the history is applied', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ] + // The user is on page 2 by choice while the desktop app shows page 1. + const rerender = render({ + resources, + activeResourceId: '2', + selectedResourceId: '2', + hydrated: false, + }) + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + + rerender({ resources, activeResourceId: '2', selectedResourceId: '2', hydrated: true }) + expect(restoreResource).not.toHaveBeenCalled() + }) + + it('does not adopt a page the user just closed in the strip', () => { + const rerender = render() + pushTabs(SCOPE, [tab('1'), tab('2', true), tab('3')], '2') + rerender({ + resources: [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + { type: 'browser', id: '3', title: 'Page 3' }, + ], + activeResourceId: '2', + selectedResourceId: '2', + }) + expect(restoreResource).not.toHaveBeenCalled() + + // The strip dropped page 2 before the native close landed. + rerender({ + resources: [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '3', title: 'Page 3' }, + ], + activeResourceId: '3', + selectedResourceId: null, + }) + expect(restoreResource).not.toHaveBeenCalled() + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + }) + + it('adopts the first reported active page without claiming it', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ] + const rerender = render() + // The pages land before the desktop app reports which one it shows. + pushTabs(SCOPE, [tab('1'), tab('2')], null) + rerender({ resources, activeResourceId: '2', selectedResourceId: null }) + expect(restoreResource).not.toHaveBeenCalled() + + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1') + expect(selectResource).not.toHaveBeenCalled() + expect(sendBrowserPanelAction).not.toHaveBeenCalled() + }) + + it('does not let a first report override a selection made before the pages landed', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ] + // The pages land first, with the desktop app not yet reporting which it shows. + const rerender = render({ selectedResourceId: '2', activeResourceId: '2' }) + pushTabs(SCOPE, [tab('1'), tab('2')], null) + rerender({ resources, selectedResourceId: '2', activeResourceId: '2' }) + // The selection is honoured by switching the native page to it. + expect(sendBrowserPanelAction).toHaveBeenCalledWith( + 'switch-tab', + { tabId: '2', claim: false }, + SCOPE + ) + + // The desktop app then reports the page it was already on. The strip must + // not move onto it, or the selection the user made would be lost. + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + expect(restoreResource).not.toHaveBeenCalled() + expect(selectResource).not.toHaveBeenCalled() + }) + + it('claims a native switch away from a page it was already showing', () => { + const resources: MothershipResource[] = [ + { type: 'browser', id: '1', title: 'Page 1' }, + { type: 'browser', id: '2', title: 'Page 2' }, + ] + const rerender = render() + pushTabs(SCOPE, [tab('1', true), tab('2')], '1') + rerender({ resources, activeResourceId: '1', selectedResourceId: null }) + expect(selectResource).not.toHaveBeenCalled() + + // A keyboard shortcut in the page moved the desktop app off page 1. + pushTabs(SCOPE, [tab('1'), tab('2', true)], '2') + expect(selectResource).toHaveBeenCalledExactlyOnceWith('2') + expect(restoreResource).not.toHaveBeenCalled() + }) + it('follows a native switch into the strip only while the user is on the browser', () => { const resources: MothershipResource[] = [ { type: 'browser', id: '1', title: 'Page 1' }, { type: 'browser', id: '2', title: 'Page 2' }, { type: 'file', id: 'f', title: 'notes.md' }, ] - const rerender = render({ resources, activeResourceId: '1' }) + const rerender = render({ resources, activeResourceId: '1', selectedResourceId: '1' }) pushTabs(SCOPE, [tab('1', true), tab('2')], '1') pushTabs(SCOPE, [tab('1'), tab('2', true)], '2') @@ -183,7 +364,7 @@ describe('useBrowserTabResources', () => { expect(sendBrowserPanelAction).not.toHaveBeenCalled() selectResource.mockClear() - rerender({ activeResourceId: 'f' }) + rerender({ activeResourceId: 'f', selectedResourceId: 'f' }) pushTabs(SCOPE, [tab('1', true), tab('2')], '1') expect(selectResource).not.toHaveBeenCalled() }) @@ -192,6 +373,7 @@ describe('useBrowserTabResources', () => { render({ resources: [{ type: 'browser', id: '1', title: 'Page 1' }], activeResourceId: '1', + selectedResourceId: '1', }) pushTabs(SCOPE, [tab('1', true)], '1') act(() => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.ts index 51885bd161f..98983b756dd 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.ts @@ -5,9 +5,8 @@ import { getErrorMessage } from '@sim/utils/errors' import { onOpenInBrowserPanel } from '@/lib/browser-agent/open-in-panel' import { browserTabTitle } from '@/lib/browser-agent/tab-label' import { openUrlInNewBrowserTab, sendBrowserPanelAction } from '@/lib/browser-agent/transport' -import type { MothershipResource } from '@/lib/copilot/resources/types' import { - type DesktopTabResourceCallbacks, + type DesktopTabStripOptions, useDesktopTabResources, } from '@/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources' import { useBrowserSessionStore } from '@/stores/browser-session/store' @@ -16,13 +15,6 @@ const logger = createLogger('BrowserTabResources') const EMPTY_BROWSER_TABS: BrowserTabState[] = [] -interface UseBrowserTabResourcesOptions extends DesktopTabResourceCallbacks { - /** Desktop browser scope whose pages back this chat's browser tabs. */ - scopeId: string - resources: readonly MothershipResource[] - activeResourceId: string | null -} - function switchBrowserTab(tabId: string, scopeId: string): void { sendBrowserPanelAction('switch-tab', { tabId, claim: false }, scopeId) } @@ -31,15 +23,8 @@ function switchBrowserTab(tabId: string, scopeId: string): void { * Projects the desktop app's live browser pages into `browser` resource tabs, * one per page. See {@link useDesktopTabResources} for the shared model. */ -export function useBrowserTabResources({ - scopeId, - resources, - activeResourceId, - addResource, - removeResource, - selectResource, - onResourceEvent, -}: UseBrowserTabResourcesOptions): void { +export function useBrowserTabResources(options: DesktopTabStripOptions): void { + const { scopeId, selectResource } = options const hasSession = useBrowserSessionStore((state) => state.sessions[scopeId] !== undefined) const browserTabs = useBrowserSessionStore( (state) => state.sessions[scopeId]?.tabs ?? EMPTY_BROWSER_TABS @@ -64,19 +49,13 @@ export function useBrowserTabResources({ selectResourceRef.current = selectResource useDesktopTabResources({ + ...options, type: 'browser', - scopeId, tabs, hasSession, activeTabId, agentTabId, switchTab: switchBrowserTab, - resources, - activeResourceId, - addResource, - removeResource, - selectResource, - onResourceEvent, }) // Chat links clicked in the desktop app open in a new browser tab. The user diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index 0ac26689ac1..5ba0de06c40 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -2476,22 +2476,25 @@ export function useChat( ) if (mergedResources.length > 0) { - // An explicit selection wins. Otherwise fall back to the last resource - // the server holds, not the last on screen: local-only browser tabs can - // land before the history does, and which side arrives first must not - // decide which tab the chat opens on. + // An explicit selection wins. Otherwise pin the last resource the server + // holds, not the last on screen: local-only browser tabs can land before + // the history does, and which side arrives first must not decide which + // tab the chat opens on. When the server holds nothing, hydration writes + // no fallback: the desktop app remembers which of its tabs the user was + // on, and the desktop tab hooks adopt that tab instead of the last one. const selectedResourceId = selectedResourceIdRef.current const hydratedActiveResourceId = selectedResourceId && mergedResources.some((resource) => resource.id === selectedResourceId) ? selectedResourceId - : ( - restorableResources[restorableResources.length - 1] ?? - mergedResources[mergedResources.length - 1] - ).id + : (restorableResources[restorableResources.length - 1]?.id ?? null) // Replacing the array with an identical one still re-renders the tab // strip and panel — skip the no-op so open panels don't flash. if (!resourcesUnchanged) { - activeResourceIdRef.current = hydratedActiveResourceId + // The ref keeps an eager fallback so a request sent in this commit + // still attaches a resource; the selection itself stays empty so the + // desktop app's remembered tab can win. + activeResourceIdRef.current = + hydratedActiveResourceId ?? mergedResources[mergedResources.length - 1].id setResources(mergedResources) setActiveResourceId(hydratedActiveResourceId) } diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources.ts index 865ad133f15..5d47b8355cd 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources.ts @@ -8,20 +8,39 @@ export interface DesktopTab { title: string } -export interface DesktopTabResourceCallbacks { +/** What the strip shares with every kind of desktop-backed resource tab. */ +export interface DesktopTabStripOptions { + /** Desktop scope whose live tabs back this chat's resource tabs. */ + scopeId: string + resources: readonly MothershipResource[] + /** The resource the strip shows: the explicit selection or its fallback. */ + activeResourceId: string | null + /** The explicit selection alone, without the strip's fallback. */ + selectedResourceId: string | null + /** + * Whether the chat's stored resources have been applied to the strip. + * Adopting a tab writes it to `activeResourceId`, so adopting on top of a + * provisional fallback would let the arrival order of the tab list and the + * chat history decide what the chat opens on. + */ + hydrated: boolean /** Adds a tab without activating it; activation goes through {@link onResourceEvent}. */ addResource: (resource: MothershipResource) => void removeResource: (resourceType: MothershipResourceType, resourceId: string) => void /** Explicit user selection, which claims the strip's selection for the user. */ selectResource: (resourceId: string) => void + /** + * Adopts the desktop app's remembered tab as the shown resource without + * claiming the selection for the user, so agent activity can still take the + * view the way it does on any chat open. + */ + restoreResource: (resourceId: string) => void /** Agent activity on a tab, subject to the panel's user-ownership policy. */ onResourceEvent: ResourceEventHandler } -interface UseDesktopTabResourcesOptions extends DesktopTabResourceCallbacks { +interface UseDesktopTabResourcesOptions extends DesktopTabStripOptions { type: 'browser' | 'terminal' - /** Desktop scope whose live tabs back this chat's resource tabs. */ - scopeId: string /** The desktop app's live tab list for the scope, in its order. */ tabs: readonly DesktopTab[] /** @@ -36,8 +55,43 @@ interface UseDesktopTabResourcesOptions extends DesktopTabResourceCallbacks { agentTabId: string | null /** Shows a tab natively without claiming it for the user. */ switchTab: (tabId: string, scopeId: string) => void - resources: readonly MothershipResource[] - activeResourceId: string | null +} + +/** + * The desktop app's active tab when it is not the tab the strip shows, and the + * strip is on one of this kind. Null when the two already agree or the strip + * is showing something else entirely. + */ +function nativeTabOffStrip( + resources: readonly MothershipResource[], + activeResourceId: string | null, + activeTabId: string | null, + type: MothershipResourceType +): string | null { + if (!activeTabId || activeTabId === activeResourceId) return null + return resources.find((resource) => resource.id === activeResourceId)?.type === type + ? activeTabId + : null +} + +/** + * The same tab, narrowed to one the strip still holds as a resource: a tab + * just closed there stays the desktop app's active tab until the close lands, + * and adopting it would show a tab that is gone. Following a switch the user + * made needs no such check — a brand-new tab is followed before the strip has + * projected it. + */ +function nativeTabToAdopt( + resources: readonly MothershipResource[], + activeResourceId: string | null, + activeTabId: string | null, + type: MothershipResourceType +): string | null { + const tabId = nativeTabOffStrip(resources, activeResourceId, activeTabId, type) + if (!tabId) return null + return resources.some((resource) => resource.type === type && resource.id === tabId) + ? tabId + : null } /** @@ -49,7 +103,10 @@ interface UseDesktopTabResourcesOptions extends DesktopTabResourceCallbacks { * a resource tab closes its native tab at the strip, which then comes back * through the same list. Visible selection is routed the same way — choosing * a resource tab switches the native tab, and a native switch follows into the - * strip while the user is on that kind of tab. + * strip while the user is on that kind of tab. Without an explicit selection + * the desktop app's own active tab wins: it remembers the tab the user left a + * chat on, so reopening the chat lands there instead of on the strip's + * last-tab fallback. * * The agent never moves the visible tab itself. Its tab is announced as * resource activity, so the existing view policy decides whether to show it or @@ -65,9 +122,12 @@ export function useDesktopTabResources({ switchTab, resources, activeResourceId, + selectedResourceId, + hydrated, addResource, removeResource, selectResource, + restoreResource, onResourceEvent, }: UseDesktopTabResourcesOptions): void { /** @@ -83,18 +143,29 @@ export function useDesktopTabResources({ const requestedTabIdRef = useRef(null) const scopeIdRef = useRef(scopeId) scopeIdRef.current = scopeId - const tabsRef = useRef(tabs) - tabsRef.current = tabs const activeTabIdRef = useRef(activeTabId) activeTabIdRef.current = activeTabId const resourcesRef = useRef(resources) resourcesRef.current = resources const activeResourceIdRef = useRef(activeResourceId) activeResourceIdRef.current = activeResourceId + /** Whether the strip shows an explicit selection rather than its fallback. */ + const explicitSelection = selectedResourceId !== null && selectedResourceId === activeResourceId + const explicitSelectionRef = useRef(explicitSelection) + explicitSelectionRef.current = explicitSelection + const hydratedRef = useRef(hydrated) + hydratedRef.current = hydrated + /** + * The tab the desktop app showed last, to tell a change of the shown tab + * from the scope's first report. Starts unset, like the scope itself. + */ + const previousActiveTabIdRef = useRef(null) const switchTabRef = useRef(switchTab) switchTabRef.current = switchTab const selectResourceRef = useRef(selectResource) selectResourceRef.current = selectResource + const restoreResourceRef = useRef(restoreResource) + restoreResourceRef.current = restoreResource const onResourceEventRef = useRef(onResourceEvent) onResourceEventRef.current = onResourceEvent @@ -105,6 +176,7 @@ export function useDesktopTabResources({ knownScopeRef.current = scopeId known.clear() requestedTabIdRef.current = null + previousActiveTabIdRef.current = null } const resourceTabIds = new Set( resources.filter((resource) => resource.type === type).map((resource) => resource.id) @@ -127,30 +199,59 @@ export function useDesktopTabResources({ } }, [addResource, hasSession, removeResource, resources, scopeId, tabs, type]) - // Selecting a resource tab shows its native tab. Keyed on the selection - // alone: a native push must not re-assert a selection it just moved away - // from, or the two sides would trade switches forever. + const selectedTabIsLive = + selectedResourceId !== null && tabs.some((tab) => tab.id === selectedResourceId) + + // Selecting a resource tab shows its native tab. Keyed on the explicit + // selection alone — the strip's fallback is not a choice to impose on the + // desktop app, and a native push must not re-assert a selection it just + // moved away from, or the two sides would trade switches forever — and on + // that tab being live, so a selection made before the desktop app published + // its tab list is shown once the tab arrives rather than dropped. + useEffect(() => { + if (!selectedResourceId || !selectedTabIsLive) return + if (selectedResourceId === activeTabIdRef.current) return + requestedTabIdRef.current = selectedResourceId + switchTabRef.current(selectedResourceId, scopeIdRef.current) + }, [selectedResourceId, selectedTabIsLive]) + + // With no effective selection the strip falls back to a tab of its own + // choosing. The desktop app still shows the tab the user was last on, so the + // strip adopts that one rather than showing a page the user did not pick. useEffect(() => { - if (!activeResourceId || activeResourceId === activeTabIdRef.current) return - if (!tabsRef.current.some((tab) => tab.id === activeResourceId)) return - requestedTabIdRef.current = activeResourceId - switchTabRef.current(activeResourceId, scopeIdRef.current) - }, [activeResourceId]) + if (!hydrated || explicitSelection) return + const tabId = nativeTabToAdopt( + resourcesRef.current, + activeResourceId, + activeTabIdRef.current, + type + ) + if (tabId) restoreResourceRef.current(tabId) + }, [activeResourceId, explicitSelection, hydrated, type]) // A native switch while the user is on this kind of tab follows into the - // strip. The switch this hook requested itself is not a native change of mind. + // strip. The switch this hook requested itself is not a native change of + // mind, and neither is the scope's first report: that one carries the tab + // the desktop app remembers, so it is adopted rather than claimed. A move + // away from a tab it was already showing is the user's own. useEffect(() => { + const previousActiveTabId = previousActiveTabIdRef.current + previousActiveTabIdRef.current = activeTabId if (requestedTabIdRef.current === activeTabId) { requestedTabIdRef.current = null return } - const activeResource = resourcesRef.current.find( - (resource) => resource.id === activeResourceIdRef.current - ) - if (!activeTabId || activeResource?.type !== type || activeResource.id === activeTabId) { + const activeResourceId = activeResourceIdRef.current + if (previousActiveTabId !== null) { + const tabId = nativeTabOffStrip(resourcesRef.current, activeResourceId, activeTabId, type) + if (tabId) selectResourceRef.current(tabId) return } - selectResourceRef.current(activeTabId) + // Same guards as the adopt effect above: a first report must not override + // a selection the user made before the tab list arrived. + if (!hydratedRef.current || explicitSelectionRef.current) return + const tabId = nativeTabToAdopt(resourcesRef.current, activeResourceId, activeTabId, type) + if (tabId) restoreResourceRef.current(tabId) }, [activeTabId, type]) // The agent's tab surfaces like any other agent activity. diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.test.tsx index ef79b3ed7c2..2b69e762e77 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.test.tsx @@ -6,6 +6,7 @@ import type { TerminalTabState } from '@sim/terminal-protocol' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { MothershipResource } from '@/lib/copilot/resources/types' +import type { DesktopTabStripOptions } from '@/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources' import { useTerminalTabResources } from '@/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources' import { useCopilotTerminalStore } from '@/stores/copilot-terminal/store' @@ -38,17 +39,7 @@ function pushTabs(scopeId: string, tabs: TerminalTabState[], activeTerminalId: s }) } -interface HostProps { - scopeId: string - resources: MothershipResource[] - activeResourceId: string | null - addResource: (resource: MothershipResource) => void - removeResource: (type: MothershipResource['type'], id: string) => void - selectResource: (id: string) => void - onResourceEvent: (id: string, options?: { activate?: boolean }) => void -} - -function Host(props: HostProps) { +function Host(props: DesktopTabStripOptions) { useTerminalTabResources(props) return null } @@ -59,21 +50,26 @@ describe('useTerminalTabResources', () => { const addResource = vi.fn() const removeResource = vi.fn() const selectResource = vi.fn() + const restoreResource = vi.fn() const onResourceEvent = vi.fn() - function render(overrides: Partial = {}) { - const props: HostProps = { + function render(overrides: Partial = {}) { + const props: DesktopTabStripOptions = { scopeId: SCOPE, resources: [], activeResourceId: null, + selectedResourceId: null, + hydrated: true, addResource, removeResource, selectResource, + restoreResource, onResourceEvent, ...overrides, } act(() => root.render()) - return (next: Partial) => act(() => root.render()) + return (next: Partial) => + act(() => root.render()) } beforeEach(() => { @@ -120,31 +116,56 @@ describe('useTerminalTabResources', () => { { type: 'terminal', id: 'terminal:1', title: 'dir-1' }, { type: 'terminal', id: 'terminal:2', title: 'dir-2' }, ] - const rerender = render({ resources, activeResourceId: 'terminal:1' }) + const rerender = render({ + resources, + activeResourceId: 'terminal:1', + selectedResourceId: 'terminal:1', + }) pushTabs(SCOPE, [shell('1', true), shell('2')], '1') expect(switchTerminal).not.toHaveBeenCalled() - rerender({ activeResourceId: 'terminal:2' }) + rerender({ activeResourceId: 'terminal:2', selectedResourceId: 'terminal:2' }) expect(switchTerminal).toHaveBeenCalledExactlyOnceWith('2', SCOPE, { claim: false }) pushTabs(SCOPE, [shell('1'), shell('2', true)], '2') expect(selectResource).not.toHaveBeenCalled() }) + it('adopts the native active shell on reopen instead of pushing the fallback tab', () => { + const rerender = render() + pushTabs(SCOPE, [shell('1', true), shell('2')], '1') + rerender({ + resources: [ + { type: 'terminal', id: 'terminal:1', title: 'dir-1' }, + { type: 'terminal', id: 'terminal:2', title: 'dir-2' }, + ], + activeResourceId: 'terminal:2', + selectedResourceId: null, + }) + + expect(switchTerminal).not.toHaveBeenCalled() + expect(restoreResource).toHaveBeenCalledExactlyOnceWith('terminal:1') + expect(selectResource).not.toHaveBeenCalled() + }) + it('follows a native switch into the strip only while the user is on a terminal', () => { const resources: MothershipResource[] = [ { type: 'terminal', id: 'terminal:1', title: 'dir-1' }, { type: 'terminal', id: 'terminal:2', title: 'dir-2' }, { type: 'file', id: 'f', title: 'notes.md' }, ] - const rerender = render({ resources, activeResourceId: 'terminal:1' }) + const rerender = render({ + resources, + activeResourceId: 'terminal:1', + selectedResourceId: 'terminal:1', + }) pushTabs(SCOPE, [shell('1', true), shell('2')], '1') pushTabs(SCOPE, [shell('1'), shell('2', true)], '2') expect(selectResource).toHaveBeenCalledExactlyOnceWith('terminal:2') selectResource.mockClear() - rerender({ activeResourceId: 'f' }) + rerender({ activeResourceId: 'f', selectedResourceId: 'f' }) pushTabs(SCOPE, [shell('1', true), shell('2')], '1') expect(selectResource).not.toHaveBeenCalled() }) @@ -156,6 +177,7 @@ describe('useTerminalTabResources', () => { { type: 'terminal', id: 'terminal:2', title: 'dir-2' }, ], activeResourceId: 'terminal:1', + selectedResourceId: 'terminal:1', }) pushTabs(SCOPE, [shell('1', true), shell('2')], '1') act(() => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.ts index e794b0debd7..b9cb6826138 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-terminal-tab-resources.ts @@ -1,23 +1,15 @@ import { useMemo } from 'react' import type { TerminalTabState } from '@sim/terminal-protocol' -import type { MothershipResource } from '@/lib/copilot/resources/types' import { terminalIdFromResourceId, terminalResourceId } from '@/lib/terminal/resource-id' import { switchTerminal } from '@/lib/terminal/transport' import { - type DesktopTabResourceCallbacks, + type DesktopTabStripOptions, useDesktopTabResources, } from '@/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources' import { useCopilotTerminalStore } from '@/stores/copilot-terminal/store' const EMPTY_TERMINAL_TABS: TerminalTabState[] = [] -interface UseTerminalTabResourcesOptions extends DesktopTabResourceCallbacks { - /** Desktop terminal scope whose shells back this chat's terminal tabs. */ - scopeId: string - resources: readonly MothershipResource[] - activeResourceId: string | null -} - function showTerminal(resourceId: string, scopeId: string): void { void switchTerminal(terminalIdFromResourceId(resourceId), scopeId, { claim: false }).catch( () => {} @@ -28,15 +20,8 @@ function showTerminal(resourceId: string, scopeId: string): void { * Projects the desktop app's live shells into `terminal` resource tabs, one * per shell. See {@link useDesktopTabResources} for the shared model. */ -export function useTerminalTabResources({ - scopeId, - resources, - activeResourceId, - addResource, - removeResource, - selectResource, - onResourceEvent, -}: UseTerminalTabResourcesOptions): void { +export function useTerminalTabResources(options: DesktopTabStripOptions): void { + const { scopeId } = options const hasSession = useCopilotTerminalStore((state) => state.sessions[scopeId] !== undefined) const terminalTabs = useCopilotTerminalStore( (state) => state.sessions[scopeId]?.tabs.tabs ?? EMPTY_TERMINAL_TABS @@ -60,18 +45,12 @@ export function useTerminalTabResources({ ) useDesktopTabResources({ + ...options, type: 'terminal', - scopeId, tabs, hasSession, activeTabId: activeTerminalId && terminalResourceId(activeTerminalId), agentTabId: agentTerminalId && terminalResourceId(agentTerminalId), switchTab: showTerminal, - resources, - activeResourceId, - addResource, - removeResource, - selectResource, - onResourceEvent, }) }