Skip to content

Commit a7d12e5

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(tables): address view review feedback
1 parent 2703ce9 commit a7d12e5

5 files changed

Lines changed: 143 additions & 19 deletions

File tree

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -264,17 +264,35 @@ export function Table({
264264
* panel's Columns section edits it and the active view persists it. */
265265
const [hiddenColumns, setHiddenColumns] = useState<string[]>([])
266266

267-
const [{ sort: sortColumn, dir: sortDirection, view: activeViewId }, setTableParams] =
267+
const [{ sort: sortColumn, dir: sortDirection, view: urlActiveViewId }, setTableParams] =
268268
useQueryStates(tableDetailParsers, tableDetailUrlKeys)
269269

270-
/** A chat View resource owns its initial selection. After seeding, normal
271-
* selector and URL behavior take over so users can switch Views as usual. */
272-
const seededPropViewRef = useRef<string | undefined>(undefined)
270+
/**
271+
* An embedded View must own the first render synchronously. Seeding only the
272+
* host URL in an effect races the view resolver: it can adopt Default view
273+
* before the URL write lands and clear the requested chat resource.
274+
*
275+
* Keep a local selection only for View-resource embeds; page Tables and plain
276+
* Table resources continue to use the URL as their sole source of truth.
277+
*/
278+
const [embeddedActiveViewId, setEmbeddedActiveViewId] = useState<string | null>(
279+
() => propViewId ?? null
280+
)
281+
const activeViewId = propViewId ? embeddedActiveViewId : urlActiveViewId
282+
const syncedPropViewRef = useRef<string | undefined>(undefined)
273283
useEffect(() => {
274-
if (!propViewId || seededPropViewRef.current === propViewId) return
275-
seededPropViewRef.current = propViewId
284+
if (!propViewId || syncedPropViewRef.current === propViewId) return
285+
syncedPropViewRef.current = propViewId
286+
setEmbeddedActiveViewId(propViewId)
276287
void setTableParams({ view: propViewId })
277288
}, [propViewId, setTableParams])
289+
const setActiveViewId = useCallback(
290+
(viewId: string) => {
291+
if (propViewId) setEmbeddedActiveViewId(viewId)
292+
void setTableParams({ view: viewId })
293+
},
294+
[propViewId, setTableParams]
295+
)
278296

279297
// Read-only mirrors for the resolve effect: it must know whether the user has
280298
// already applied a filter / hidden columns without re-running when they change.
@@ -552,7 +570,7 @@ export function Table({
552570
const keep = inheritedParams ? { ...localWork(), sort: false } : localWork()
553571
if (defaultView) {
554572
seededViewIdRef.current = defaultView.id
555-
setTableParams({ view: defaultView.id })
573+
setActiveViewId(defaultView.id)
556574
applyViewConfig(defaultView.config, keep)
557575
resolvePendingLayout(true)
558576
return
@@ -561,7 +579,10 @@ export function Table({
561579
// would clear a deep-linked `?sort=` on mount. Inherited params are the
562580
// exception: nothing about them refers to this table, so they're cleared.
563581
seededViewIdRef.current = null
564-
if (inheritedParams) setTableParams({ view: ALL_VIEW_PARAM, sort: null, dir: null })
582+
if (inheritedParams) {
583+
setActiveViewId(ALL_VIEW_PARAM)
584+
setTableParams({ sort: null, dir: null })
585+
}
565586
resolvePendingLayout(false)
566587
return
567588
}
@@ -581,7 +602,7 @@ export function Table({
581602
// Nothing to apply, but the URL still names a view that no longer exists.
582603
// Rewrite it so a stale bookmark can't be copied on, and so the param
583604
// matches the All the UI is already showing.
584-
setTableParams({ view: ALL_VIEW_PARAM })
605+
setActiveViewId(ALL_VIEW_PARAM)
585606
}
586607
return
587608
}
@@ -599,7 +620,7 @@ export function Table({
599620
if (activeViewId !== null && activeViewId !== ALL_VIEW_PARAM && !activeView) {
600621
if (pendingCreatedViewIdRef.current === activeViewId) return
601622
seededViewIdRef.current = null
602-
setTableParams({ view: ALL_VIEW_PARAM })
623+
setActiveViewId(ALL_VIEW_PARAM)
603624
applyViewConfig(null)
604625
return
605626
}
@@ -623,7 +644,7 @@ export function Table({
623644
embedded,
624645
sortColumn,
625646
applyViewConfig,
626-
setTableParams,
647+
setActiveViewId,
627648
resolvePendingLayout,
628649
])
629650

@@ -707,9 +728,9 @@ export function Table({
707728

708729
const handleSelectView = useCallback(
709730
(viewId: string | null) => {
710-
setTableParams({ view: viewId ?? ALL_VIEW_PARAM })
731+
setActiveViewId(viewId ?? ALL_VIEW_PARAM)
711732
},
712-
[setTableParams]
733+
[setActiveViewId]
713734
)
714735

715736
const handleRenameView = useCallback((viewId: string) => {
@@ -816,7 +837,7 @@ export function Table({
816837
// seeded — it can't tell a just-created view from a dead id otherwise.
817838
seededViewIdRef.current = view.id
818839
pendingCreatedViewIdRef.current = view.id
819-
setTableParams({ view: view.id })
840+
setActiveViewId(view.id)
820841
// Which means the blank config must be applied here; nuqs batches this
821842
// sort write with the `view` write above into one URL update.
822843
if (blank) applyViewConfig(view.config)
@@ -830,12 +851,12 @@ export function Table({
830851
(viewId: string) => {
831852
deleteViewMutation.mutate(viewId, {
832853
onSuccess: () => {
833-
if (viewId === activeViewId) setTableParams({ view: ALL_VIEW_PARAM })
854+
if (viewId === activeViewId) setActiveViewId(ALL_VIEW_PARAM)
834855
},
835856
onError: (error) => toast.error(getErrorMessage(error, 'Failed to delete view')),
836857
})
837858
},
838-
[activeViewId, setTableParams]
859+
[activeViewId, setActiveViewId]
839860
)
840861

841862
const runColumnMutation = useRunColumn({ workspaceId, tableId })

apps/sim/lib/copilot/resources/extraction.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,44 @@ import { describe, expect, it } from 'vitest'
55
import { extractDeletedResourcesFromToolResult, extractResourcesFromToolResult } from './extraction'
66

77
describe('extractResourcesFromToolResult', () => {
8+
it('auto-opens the created View instead of its source Table', () => {
9+
const resources = extractResourcesFromToolResult(
10+
'user_table',
11+
{
12+
operation: 'create_view',
13+
args: { tableId: 'tbl_123' },
14+
},
15+
{
16+
success: true,
17+
message: 'Created View "Qualified leads"',
18+
data: {
19+
view: {
20+
id: 'view_456',
21+
tableId: 'tbl_123',
22+
name: 'Qualified leads',
23+
},
24+
},
25+
// Server tools are wrapped in ToolExecutionResult.output, so this
26+
// nested descriptor is not available as result.resources.
27+
resources: [
28+
{
29+
type: 'view',
30+
id: 'tbl_123:view_456',
31+
title: 'Qualified leads',
32+
},
33+
],
34+
}
35+
)
36+
37+
expect(resources).toEqual([
38+
{
39+
type: 'view',
40+
id: 'tbl_123:view_456',
41+
title: 'Qualified leads',
42+
},
43+
])
44+
})
45+
846
it('extracts file resources from create_file results', () => {
947
const resources = extractResourcesFromToolResult(
1048
'create_file',

apps/sim/lib/copilot/resources/extraction.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
UserTable,
1616
WorkspaceFile,
1717
} from '@/lib/copilot/generated/tool-catalog-v1'
18-
import type { MothershipResource, MothershipResourceType } from './types'
18+
import { type MothershipResource, type MothershipResourceType, tableViewResourceId } from './types'
1919

2020
type ChatResource = MothershipResource
2121
type ResourceType = MothershipResourceType
@@ -79,6 +79,22 @@ export function extractResourcesFromToolResult(
7979
case UserTable.id: {
8080
if (READ_ONLY_TABLE_OPS.has(getOperation(params) ?? '')) return []
8181

82+
const args = asRecord(params?.args)
83+
if (getOperation(params) === 'create_view') {
84+
const view = asRecord(data.view)
85+
const tableId = (view.tableId as string | undefined) ?? (args.tableId as string | undefined)
86+
if (tableId && view.id) {
87+
return [
88+
{
89+
type: 'view',
90+
id: tableViewResourceId(tableId, view.id as string),
91+
title: (view.name as string) || 'View',
92+
},
93+
]
94+
}
95+
return []
96+
}
97+
8298
if (result.tableId) {
8399
return [
84100
{
@@ -101,7 +117,6 @@ export function extractResourcesFromToolResult(
101117
if (table.id) {
102118
return [{ type: 'table', id: table.id as string, title: (table.name as string) || 'Table' }]
103119
}
104-
const args = asRecord(params?.args)
105120
const tableId =
106121
(data.tableId as string) ?? (args.tableId as string) ?? (params?.tableId as string)
107122
if (tableId) {

apps/sim/lib/copilot/tools/handlers/resources.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,51 @@ describe('executeOpenResource', () => {
133133
})
134134
})
135135

136+
it('reopens a View from its persisted composite resource id', async () => {
137+
getTableViewMock.mockResolvedValue({
138+
id: 'view_1',
139+
tableId: 'tbl_1',
140+
name: 'Qualified leads',
141+
})
142+
getTableByIdMock.mockResolvedValue({
143+
id: 'tbl_1',
144+
workspaceId: 'workspace-1',
145+
name: 'Leads',
146+
})
147+
148+
const result = await executeOpenResource(
149+
{ resources: [{ type: 'view', id: 'tbl_1:view_1' }] },
150+
{ userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'workspace-1' }
151+
)
152+
153+
expect(getTableViewMock).toHaveBeenCalledWith('view_1')
154+
expect(result).toMatchObject({
155+
success: true,
156+
output: { opened: 1, errors: [] },
157+
resources: [{ type: 'view', id: 'tbl_1:view_1', title: 'Qualified leads' }],
158+
})
159+
})
160+
161+
it('refuses a composite View id with the wrong source Table', async () => {
162+
getTableViewMock.mockResolvedValue({
163+
id: 'view_1',
164+
tableId: 'tbl_actual',
165+
name: 'Qualified leads',
166+
})
167+
168+
const result = await executeOpenResource(
169+
{ resources: [{ type: 'view', id: 'tbl_claimed:view_1' }] },
170+
{ userId: 'user-1', workflowId: 'workflow-1', workspaceId: 'workspace-1' }
171+
)
172+
173+
expect(getTableByIdMock).not.toHaveBeenCalled()
174+
expect(result).toMatchObject({
175+
success: false,
176+
output: { opened: 0, errors: ['View does not belong to the specified Table.'] },
177+
resources: [],
178+
})
179+
})
180+
136181
it('refuses a View whose source Table belongs to another workspace', async () => {
137182
getTableViewMock.mockResolvedValue({
138183
id: 'view_1',

apps/sim/lib/copilot/tools/handlers/resources.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/typ
55
import {
66
type MothershipResource,
77
MothershipResourceType,
8+
parseTableViewResourceId,
89
tableViewResourceId,
910
} from '@/lib/copilot/resources/types'
1011
import { canonicalWorkspaceFilePath } from '@/lib/copilot/vfs/path-utils'
@@ -68,8 +69,12 @@ async function resolveResource(
6869
}
6970
if (resourceType === 'view') {
7071
if (!item.id) return { error: 'view resources require `id`.' }
71-
const view = await getTableView(item.id)
72+
const parsedId = parseTableViewResourceId(item.id)
73+
const view = await getTableView(parsedId?.viewId ?? item.id)
7274
if (!view) return { error: `No View with id "${item.id}".` }
75+
if (parsedId && parsedId.tableId !== view.tableId) {
76+
return { error: 'View does not belong to the specified Table.' }
77+
}
7378
const table = await getTableById(view.tableId)
7479
if (!table || (context.workspaceId && table.workspaceId !== context.workspaceId)) {
7580
return { error: 'View not found in the current workspace.' }

0 commit comments

Comments
 (0)