Skip to content

Commit 7943024

Browse files
waleedlatif1claude
andcommitted
refactor(desktop): align the two tab-adoption paths and drop dead plumbing
Quality pass on the reopen fix. The late-arrival adoption now carries the same guards as the hydrated one, so a first report of the desktop app's active tab can no longer override a selection the user made before the tab list arrived. Both guards are pinned by tests that fail when either is removed. The predicate the adopt and claim paths share moved into one helper, so the single difference between them — adoption needs the tab to still be in the strip, following the user does not — is stated once. Removes a ref nothing read and an options interface with no second consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139YonWmiZUnPMTHoH4PtAJ
1 parent 6eba8d2 commit 7943024

3 files changed

Lines changed: 90 additions & 53 deletions

File tree

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

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -249,19 +249,21 @@ describe('useBrowserTabResources', () => {
249249
expect(restoreResource).toHaveBeenCalledExactlyOnceWith('1')
250250
})
251251

252-
it('leaves a stored resource the history pinned alone once hydrated', () => {
253-
const rerender = render({ hydrated: false })
254-
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
255-
rerender({
256-
resources: [
257-
{ type: 'browser', id: '1', title: 'Page 1' },
258-
{ type: 'browser', id: '2', title: 'Page 2' },
259-
{ type: 'file', id: 'f', title: 'notes.md' },
260-
],
261-
activeResourceId: 'f',
262-
selectedResourceId: 'f',
263-
hydrated: true,
252+
it('leaves an explicitly selected page alone when the history is applied', () => {
253+
const resources: MothershipResource[] = [
254+
{ type: 'browser', id: '1', title: 'Page 1' },
255+
{ type: 'browser', id: '2', title: 'Page 2' },
256+
]
257+
// The user is on page 2 by choice while the desktop app shows page 1.
258+
const rerender = render({
259+
resources,
260+
activeResourceId: '2',
261+
selectedResourceId: '2',
262+
hydrated: false,
264263
})
264+
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
265+
266+
rerender({ resources, activeResourceId: '2', selectedResourceId: '2', hydrated: true })
265267
expect(restoreResource).not.toHaveBeenCalled()
266268
})
267269

@@ -309,6 +311,29 @@ describe('useBrowserTabResources', () => {
309311
expect(sendBrowserPanelAction).not.toHaveBeenCalled()
310312
})
311313

314+
it('does not let a first report override a selection made before the pages landed', () => {
315+
const resources: MothershipResource[] = [
316+
{ type: 'browser', id: '1', title: 'Page 1' },
317+
{ type: 'browser', id: '2', title: 'Page 2' },
318+
]
319+
// The pages land first, with the desktop app not yet reporting which it shows.
320+
const rerender = render({ selectedResourceId: '2', activeResourceId: '2' })
321+
pushTabs(SCOPE, [tab('1'), tab('2')], null)
322+
rerender({ resources, selectedResourceId: '2', activeResourceId: '2' })
323+
// The selection is honoured by switching the native page to it.
324+
expect(sendBrowserPanelAction).toHaveBeenCalledWith(
325+
'switch-tab',
326+
{ tabId: '2', claim: false },
327+
SCOPE
328+
)
329+
330+
// The desktop app then reports the page it was already on. The strip must
331+
// not move onto it, or the selection the user made would be lost.
332+
pushTabs(SCOPE, [tab('1', true), tab('2')], '1')
333+
expect(restoreResource).not.toHaveBeenCalled()
334+
expect(selectResource).not.toHaveBeenCalled()
335+
})
336+
312337
it('claims a native switch away from a page it was already showing', () => {
313338
const resources: MothershipResource[] = [
314339
{ type: 'browser', id: '1', title: 'Page 1' },

apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2490,6 +2490,9 @@ export function useChat(
24902490
// Replacing the array with an identical one still re-renders the tab
24912491
// strip and panel — skip the no-op so open panels don't flash.
24922492
if (!resourcesUnchanged) {
2493+
// The ref keeps an eager fallback so a request sent in this commit
2494+
// still attaches a resource; the selection itself stays empty so the
2495+
// desktop app's remembered tab can win.
24932496
activeResourceIdRef.current =
24942497
hydratedActiveResourceId ?? mergedResources[mergedResources.length - 1].id
24952498
setResources(mergedResources)

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

Lines changed: 50 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -8,24 +8,8 @@ export interface DesktopTab {
88
title: string
99
}
1010

11-
export interface DesktopTabResourceCallbacks {
12-
/** Adds a tab without activating it; activation goes through {@link onResourceEvent}. */
13-
addResource: (resource: MothershipResource) => void
14-
removeResource: (resourceType: MothershipResourceType, resourceId: string) => void
15-
/** Explicit user selection, which claims the strip's selection for the user. */
16-
selectResource: (resourceId: string) => void
17-
/**
18-
* Adopts the desktop app's remembered tab as the shown resource without
19-
* claiming the selection for the user, so agent activity can still take the
20-
* view the way it does on any chat open.
21-
*/
22-
restoreResource: (resourceId: string) => void
23-
/** Agent activity on a tab, subject to the panel's user-ownership policy. */
24-
onResourceEvent: ResourceEventHandler
25-
}
26-
2711
/** What the strip shares with every kind of desktop-backed resource tab. */
28-
export interface DesktopTabStripOptions extends DesktopTabResourceCallbacks {
12+
export interface DesktopTabStripOptions {
2913
/** Desktop scope whose live tabs back this chat's resource tabs. */
3014
scopeId: string
3115
resources: readonly MothershipResource[]
@@ -35,15 +19,24 @@ export interface DesktopTabStripOptions extends DesktopTabResourceCallbacks {
3519
selectedResourceId: string | null
3620
/**
3721
* Whether the chat's stored resources have been applied to the strip.
38-
*
39-
* Adopting a tab writes it to `activeResourceId`, which is the one place the
40-
* rest of the surface reads as the shown resource, so adopting on top of a
22+
* Adopting a tab writes it to `activeResourceId`, so adopting on top of a
4123
* provisional fallback would let the arrival order of the tab list and the
42-
* chat history decide what the chat opens on. Waiting makes the outcome the
43-
* same either way: the history pins a stored resource, or it pins nothing
44-
* and the desktop app's remembered tab stands.
24+
* chat history decide what the chat opens on.
4525
*/
4626
hydrated: boolean
27+
/** Adds a tab without activating it; activation goes through {@link onResourceEvent}. */
28+
addResource: (resource: MothershipResource) => void
29+
removeResource: (resourceType: MothershipResourceType, resourceId: string) => void
30+
/** Explicit user selection, which claims the strip's selection for the user. */
31+
selectResource: (resourceId: string) => void
32+
/**
33+
* Adopts the desktop app's remembered tab as the shown resource without
34+
* claiming the selection for the user, so agent activity can still take the
35+
* view the way it does on any chat open.
36+
*/
37+
restoreResource: (resourceId: string) => void
38+
/** Agent activity on a tab, subject to the panel's user-ownership policy. */
39+
onResourceEvent: ResourceEventHandler
4740
}
4841

4942
interface UseDesktopTabResourcesOptions extends DesktopTabStripOptions {
@@ -65,21 +58,40 @@ interface UseDesktopTabResourcesOptions extends DesktopTabStripOptions {
6558
}
6659

6760
/**
68-
* The desktop app's active tab to adopt in place of the strip's fallback: one
69-
* the strip does not show yet, of the same kind as the fallback, and still in
70-
* the strip — a tab just closed there stays the desktop app's active tab until
71-
* the close lands.
61+
* The desktop app's active tab when it is not the tab the strip shows, and the
62+
* strip is on one of this kind. Null when the two already agree or the strip
63+
* is showing something else entirely.
7264
*/
73-
function nativeTabToAdopt(
65+
function nativeTabOffStrip(
7466
resources: readonly MothershipResource[],
7567
activeResourceId: string | null,
7668
activeTabId: string | null,
7769
type: MothershipResourceType
7870
): string | null {
7971
if (!activeTabId || activeTabId === activeResourceId) return null
80-
if (resources.find((resource) => resource.id === activeResourceId)?.type !== type) return null
81-
const live = resources.some((resource) => resource.type === type && resource.id === activeTabId)
82-
return live ? activeTabId : null
72+
return resources.find((resource) => resource.id === activeResourceId)?.type === type
73+
? activeTabId
74+
: null
75+
}
76+
77+
/**
78+
* The same tab, narrowed to one the strip still holds as a resource: a tab
79+
* just closed there stays the desktop app's active tab until the close lands,
80+
* and adopting it would show a tab that is gone. Following a switch the user
81+
* made needs no such check — a brand-new tab is followed before the strip has
82+
* projected it.
83+
*/
84+
function nativeTabToAdopt(
85+
resources: readonly MothershipResource[],
86+
activeResourceId: string | null,
87+
activeTabId: string | null,
88+
type: MothershipResourceType
89+
): string | null {
90+
const tabId = nativeTabOffStrip(resources, activeResourceId, activeTabId, type)
91+
if (!tabId) return null
92+
return resources.some((resource) => resource.type === type && resource.id === tabId)
93+
? tabId
94+
: null
8395
}
8496

8597
/**
@@ -131,8 +143,6 @@ export function useDesktopTabResources({
131143
const requestedTabIdRef = useRef<string | null>(null)
132144
const scopeIdRef = useRef(scopeId)
133145
scopeIdRef.current = scopeId
134-
const tabsRef = useRef(tabs)
135-
tabsRef.current = tabs
136146
const activeTabIdRef = useRef(activeTabId)
137147
activeTabIdRef.current = activeTabId
138148
const resourcesRef = useRef(resources)
@@ -141,6 +151,8 @@ export function useDesktopTabResources({
141151
activeResourceIdRef.current = activeResourceId
142152
/** Whether the strip shows an explicit selection rather than its fallback. */
143153
const explicitSelection = selectedResourceId !== null && selectedResourceId === activeResourceId
154+
const explicitSelectionRef = useRef(explicitSelection)
155+
explicitSelectionRef.current = explicitSelection
144156
const hydratedRef = useRef(hydrated)
145157
hydratedRef.current = hydrated
146158
/**
@@ -187,7 +199,6 @@ export function useDesktopTabResources({
187199
}
188200
}, [addResource, hasSession, removeResource, resources, scopeId, tabs, type])
189201

190-
/** Whether the selected resource is one of this kind's live tabs. */
191202
const selectedTabIsLive =
192203
selectedResourceId !== null && tabs.some((tab) => tab.id === selectedResourceId)
193204

@@ -232,15 +243,13 @@ export function useDesktopTabResources({
232243
}
233244
const activeResourceId = activeResourceIdRef.current
234245
if (previousActiveTabId !== null) {
235-
const activeResource = resourcesRef.current.find(
236-
(resource) => resource.id === activeResourceId
237-
)
238-
if (activeTabId && activeResource?.type === type && activeResource.id !== activeTabId) {
239-
selectResourceRef.current(activeTabId)
240-
}
246+
const tabId = nativeTabOffStrip(resourcesRef.current, activeResourceId, activeTabId, type)
247+
if (tabId) selectResourceRef.current(tabId)
241248
return
242249
}
243-
if (!hydratedRef.current) return
250+
// Same guards as the adopt effect above: a first report must not override
251+
// a selection the user made before the tab list arrived.
252+
if (!hydratedRef.current || explicitSelectionRef.current) return
244253
const tabId = nativeTabToAdopt(resourcesRef.current, activeResourceId, activeTabId, type)
245254
if (tabId) restoreResourceRef.current(tabId)
246255
}, [activeTabId, type])

0 commit comments

Comments
 (0)