Skip to content

Commit 81e0abf

Browse files
waleedlatif1claude
andcommitted
fix(desktop): adopt the remembered tab without claiming the user's selection
Review round on the reopen fix. A late first report of the desktop app's active tab carries the tab it remembers, not a switch the user made, so it is adopted rather than claimed and agent activity can still take the view on chat open. A move away from a tab the desktop was already showing stays the user's own. Adoption now waits for the chat history to be applied, so the arrival order of the tab list and the history no longer decides which resource a chat opens on, and it skips a tab the strip has already dropped, so closing the shown tab cannot write the closed id back. Closing the shown tab selects its neighbour the way the desktop app picks the next native tab, instead of flashing through the strip's last tab. The two wrapper hooks now share one options type with the strip, and the adopt rule lives in a single helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139YonWmiZUnPMTHoH4PtAJ
1 parent c84a151 commit 81e0abf

7 files changed

Lines changed: 202 additions & 150 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tabs.tsx

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -408,11 +408,19 @@ export function ResourceTabs({
408408

409409
const handleClose = useCallback(
410410
(id: string) => {
411-
const resource = resources.find((r) => r.id === id)
411+
const index = resources.findIndex((r) => r.id === id)
412+
const resource = resources[index]
412413
if (!resource) return
413414
const isMulti = selectedIds.has(resource.id) && selectedIds.size > 1
414415
const targets = isMulti ? resources.filter((r) => selectedIds.has(r.id)) : [resource]
415416
if (!confirmClosingRunningTerminals(targets, terminalTabs)) return
417+
// Closing the shown tab moves to its neighbour, right then left, the way
418+
// the desktop app picks the next native tab, so the strip does not fall
419+
// back to its last tab and jump once the close lands.
420+
if (!isMulti && activeId === resource.id) {
421+
const nextId = findNearestId(resources, index, null)
422+
if (nextId) selectResource(nextId)
423+
}
416424
// A browser tab's page is closed natively and its resource dropped at
417425
// once; the tab list then confirms the removal. A shell's close answers
418426
// with the tab list, so its resource follows that list instead — a
@@ -451,7 +459,16 @@ export function ResourceTabs({
451459
}
452460
},
453461
// eslint-disable-next-line react-hooks/exhaustive-deps
454-
[chatId, desktopScopeId, onRemoveResource, resources, selectedIds, terminalTabs]
462+
[
463+
activeId,
464+
chatId,
465+
desktopScopeId,
466+
onRemoveResource,
467+
resources,
468+
selectResource,
469+
selectedIds,
470+
terminalTabs,
471+
]
455472
)
456473

457474
/**

apps/sim/app/workspace/[workspaceId]/home/home.tsx

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -334,27 +334,21 @@ export function Home({ chatId, userName, userId }: HomeProps) {
334334
[setActiveResourceId, clearResourceActivity]
335335
)
336336

337-
const desktopTabResourceCallbacks = {
337+
const desktopTabResourceOptions = {
338+
scopeId: desktopScopeId,
339+
resources,
340+
activeResourceId,
341+
selectedResourceId: activeResourceParam,
342+
// A chat without an id has nothing stored to wait for.
343+
hydrated: resolvedChatId === undefined || !isChatHistoryPending,
338344
addResource,
339345
removeResource,
340346
selectResource: selectResourceFromUser,
341347
restoreResource: setActiveResourceId,
342348
onResourceEvent: handleResourceEvent,
343349
}
344-
useBrowserTabResources({
345-
scopeId: desktopScopeId,
346-
resources,
347-
activeResourceId,
348-
selectedResourceId: activeResourceParam,
349-
...desktopTabResourceCallbacks,
350-
})
351-
useTerminalTabResources({
352-
scopeId: desktopScopeId,
353-
resources,
354-
activeResourceId,
355-
selectedResourceId: activeResourceParam,
356-
...desktopTabResourceCallbacks,
357-
})
350+
useBrowserTabResources(desktopTabResourceOptions)
351+
useTerminalTabResources(desktopTabResourceOptions)
358352

359353
const addResourceFromUser = useCallback(
360354
(resource: MothershipResource) => {

apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.test.tsx

Lines changed: 85 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,7 @@ function pushTabs(scopeId: string, tabs: ReturnType<typeof tab>[], activeTabId:
3737
})
3838
}
3939

40-
interface HostProps {
41-
scopeId: string
42-
resources: MothershipResource[]
43-
activeResourceId: string | null
44-
selectedResourceId: string | null
45-
addResource: (resource: MothershipResource) => void
46-
removeResource: (type: MothershipResource['type'], id: string) => void
47-
selectResource: (id: string) => void
48-
restoreResource: (id: string) => void
49-
onResourceEvent: (id: string, options?: { activate?: boolean }) => void
50-
}
40+
type HostProps = Parameters<typeof useBrowserTabResources>[0]
5141

5242
function Host(props: HostProps) {
5343
useBrowserTabResources(props)
@@ -69,6 +59,7 @@ describe('useBrowserTabResources', () => {
6959
resources: [],
7060
activeResourceId: null,
7161
selectedResourceId: null,
62+
hydrated: true,
7263
addResource,
7364
removeResource,
7465
selectResource,
@@ -206,7 +197,6 @@ describe('useBrowserTabResources', () => {
206197
{ type: 'browser', id: '2', title: 'Page 2' },
207198
],
208199
activeResourceId: '2',
209-
selectedResourceId: 'deleted-file',
210200
})
211201

212202
expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1')
@@ -229,20 +219,94 @@ describe('useBrowserTabResources', () => {
229219
expect(sendBrowserPanelAction).not.toHaveBeenCalled()
230220
})
231221

232-
it('switches to a selected page once it lands, as after a reload with the tab in the URL', () => {
233-
render({ selectedResourceId: '2' })
222+
it('adopts the native active page only once the chat history has been applied', () => {
223+
const resources: MothershipResource[] = [
224+
{ type: 'browser', id: '1', title: 'Page 1' },
225+
{ type: 'browser', id: '2', title: 'Page 2' },
226+
]
227+
const rerender = render({ hydrated: false })
228+
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
229+
rerender({ resources, activeResourceId: '2', selectedResourceId: null, hydrated: false })
230+
expect(restoreResource).not.toHaveBeenCalled()
231+
232+
rerender({ resources, activeResourceId: '2', selectedResourceId: null, hydrated: true })
233+
expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1')
234+
})
235+
236+
it('leaves a stored resource the history pinned alone once hydrated', () => {
237+
const rerender = render({ hydrated: false })
238+
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
239+
rerender({
240+
resources: [
241+
{ type: 'browser', id: '1', title: 'Page 1' },
242+
{ type: 'browser', id: '2', title: 'Page 2' },
243+
{ type: 'file', id: 'f', title: 'notes.md' },
244+
],
245+
activeResourceId: 'f',
246+
selectedResourceId: 'f',
247+
hydrated: true,
248+
})
249+
expect(restoreResource).not.toHaveBeenCalled()
250+
})
251+
252+
it('does not adopt a page the user just closed in the strip', () => {
253+
const rerender = render()
254+
pushTabs(SCOPE, [tab('1'), tab('2', true), tab('3')], '2')
255+
rerender({
256+
resources: [
257+
{ type: 'browser', id: '1', title: 'Page 1' },
258+
{ type: 'browser', id: '2', title: 'Page 2' },
259+
{ type: 'browser', id: '3', title: 'Page 3' },
260+
],
261+
activeResourceId: '2',
262+
selectedResourceId: '2',
263+
})
264+
expect(restoreResource).not.toHaveBeenCalled()
265+
266+
// The strip dropped page 2 before the native close landed.
267+
rerender({
268+
resources: [
269+
{ type: 'browser', id: '1', title: 'Page 1' },
270+
{ type: 'browser', id: '3', title: 'Page 3' },
271+
],
272+
activeResourceId: '3',
273+
selectedResourceId: null,
274+
})
275+
expect(restoreResource).not.toHaveBeenCalled()
276+
expect(sendBrowserPanelAction).not.toHaveBeenCalled()
277+
})
278+
279+
it('adopts the first reported active page without claiming it', () => {
280+
const resources: MothershipResource[] = [
281+
{ type: 'browser', id: '1', title: 'Page 1' },
282+
{ type: 'browser', id: '2', title: 'Page 2' },
283+
]
284+
const rerender = render()
285+
// The pages land before the desktop app reports which one it shows.
286+
pushTabs(SCOPE, [tab('1'), tab('2')], null)
287+
rerender({ resources, activeResourceId: '2', selectedResourceId: null })
288+
expect(restoreResource).not.toHaveBeenCalled()
289+
290+
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
291+
expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1')
292+
expect(selectResource).not.toHaveBeenCalled()
234293
expect(sendBrowserPanelAction).not.toHaveBeenCalled()
294+
})
235295

296+
it('claims a native switch away from a page it was already showing', () => {
297+
const resources: MothershipResource[] = [
298+
{ type: 'browser', id: '1', title: 'Page 1' },
299+
{ type: 'browser', id: '2', title: 'Page 2' },
300+
]
301+
const rerender = render()
236302
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
237-
expect(sendBrowserPanelAction).toHaveBeenCalledExactlyOnceWith(
238-
'switch-tab',
239-
{ tabId: '2', claim: false },
240-
SCOPE
241-
)
303+
rerender({ resources, activeResourceId: '1', selectedResourceId: null })
304+
expect(selectResource).not.toHaveBeenCalled()
242305

243-
// The requested switch landing is not a native change to follow.
306+
// A keyboard shortcut in the page moved the desktop app off page 1.
244307
pushTabs(SCOPE, [tab('1'), tab('2', true)], '2')
245-
expect(selectResource).not.toHaveBeenCalled()
308+
expect(selectResource).toHaveBeenCalledExactlyOnceWith('2')
309+
expect(restoreResource).not.toHaveBeenCalled()
246310
})
247311

248312
it('follows a native switch into the strip only while the user is on the browser', () => {

apps/sim/app/workspace/[workspaceId]/home/hooks/use-browser-tab-resources.ts

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,8 @@ import { getErrorMessage } from '@sim/utils/errors'
55
import { onOpenInBrowserPanel } from '@/lib/browser-agent/open-in-panel'
66
import { browserTabTitle } from '@/lib/browser-agent/tab-label'
77
import { openUrlInNewBrowserTab, sendBrowserPanelAction } from '@/lib/browser-agent/transport'
8-
import type { MothershipResource } from '@/lib/copilot/resources/types'
98
import {
10-
type DesktopTabResourceCallbacks,
9+
type DesktopTabStripOptions,
1110
useDesktopTabResources,
1211
} from '@/app/workspace/[workspaceId]/home/hooks/use-desktop-tab-resources'
1312
import { useBrowserSessionStore } from '@/stores/browser-session/store'
@@ -16,14 +15,6 @@ const logger = createLogger('BrowserTabResources')
1615

1716
const EMPTY_BROWSER_TABS: BrowserTabState[] = []
1817

19-
interface UseBrowserTabResourcesOptions extends DesktopTabResourceCallbacks {
20-
/** Desktop browser scope whose pages back this chat's browser tabs. */
21-
scopeId: string
22-
resources: readonly MothershipResource[]
23-
activeResourceId: string | null
24-
selectedResourceId: string | null
25-
}
26-
2718
function switchBrowserTab(tabId: string, scopeId: string): void {
2819
sendBrowserPanelAction('switch-tab', { tabId, claim: false }, scopeId)
2920
}
@@ -32,17 +23,8 @@ function switchBrowserTab(tabId: string, scopeId: string): void {
3223
* Projects the desktop app's live browser pages into `browser` resource tabs,
3324
* one per page. See {@link useDesktopTabResources} for the shared model.
3425
*/
35-
export function useBrowserTabResources({
36-
scopeId,
37-
resources,
38-
activeResourceId,
39-
selectedResourceId,
40-
addResource,
41-
removeResource,
42-
selectResource,
43-
restoreResource,
44-
onResourceEvent,
45-
}: UseBrowserTabResourcesOptions): void {
26+
export function useBrowserTabResources(options: DesktopTabStripOptions): void {
27+
const { scopeId, selectResource } = options
4628
const hasSession = useBrowserSessionStore((state) => state.sessions[scopeId] !== undefined)
4729
const browserTabs = useBrowserSessionStore(
4830
(state) => state.sessions[scopeId]?.tabs ?? EMPTY_BROWSER_TABS
@@ -67,21 +49,13 @@ export function useBrowserTabResources({
6749
selectResourceRef.current = selectResource
6850

6951
useDesktopTabResources({
52+
...options,
7053
type: 'browser',
71-
scopeId,
7254
tabs,
7355
hasSession,
7456
activeTabId,
7557
agentTabId,
7658
switchTab: switchBrowserTab,
77-
resources,
78-
activeResourceId,
79-
selectedResourceId,
80-
addResource,
81-
removeResource,
82-
selectResource,
83-
restoreResource,
84-
onResourceEvent,
8559
})
8660

8761
// Chat links clicked in the desktop app open in a new browser tab. The user

0 commit comments

Comments
 (0)