Skip to content

Commit 8530364

Browse files
committed
fix(tables): keep a model's folder destination, and clear a hidden table selection
Two review findings, both on the block surface. A block used as an Agent tool is handed the TOOL's schema, not this block's subblock ids - `createLLMToolSchema(toolConfig, ...)` in `providers/utils.ts` - so a model answers with `path` / `folderPath` while the canvas stores `folderRef` / `moveTargetRef`. Every folder transformer read only the canvas id and dropped the model's answer. Five of them then failed schema validation, which is loud and recoverable. `move` did not: the tool substitutes the workspace root for an absent destination, so "move into /Reports" became "move to the root" with a success result. The canvas value still wins where the author set one; the model's answer is the fallback, matching what `columns` already does in this file. `update_folder` branches on the canvas source instead, because its destination is composed rather than picked and an unset parent is a real destination (the root), not a missing one. The delete cascade deliberately gets no model fallback - it is `user-only` on both the tool param and the subblock. Narrowing the folder scope filtered the selected table out of the picker without touching the stored value, so the combobox showed its placeholder while the block kept executing against the hidden table. The selection is now cleared when it falls out of scope - but only for a table that is actually loaded and actually out of scope, since a table missing from the list is still-loading or deleted, and clearing there would destroy a valid config over a transient cache state.
1 parent 2f10911 commit 8530364

4 files changed

Lines changed: 224 additions & 21 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/table-selector/scope.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,45 @@ describe('parseFolderScope', () => {
106106
expect(isTableInFolderScope({ folderId: 'archive' }, FOLDERS, null)).toBe(true)
107107
})
108108
})
109+
110+
/*
111+
* The clearing rule the component applies when a scope change hides the current
112+
* selection. Expressed here as the predicate the effect guards on, so the
113+
* "don't clear a table that merely has not loaded" case is pinned.
114+
*/
115+
describe('a selection hidden by a scope change', () => {
116+
const TABLES = [
117+
{ id: 'in-scope', folderId: 'q3' },
118+
{ id: 'out-of-scope', folderId: 'archive' },
119+
]
120+
121+
function shouldClear(tableId: string, scope: string): boolean {
122+
const loaded = TABLES.some((t) => t.id === tableId)
123+
if (!loaded) return false
124+
const segments = parseFolderScope(scope)
125+
return !TABLES.filter((t) => isTableInFolderScope(t, FOLDERS, segments)).some(
126+
(t) => t.id === tableId
127+
)
128+
}
129+
130+
it('clears a loaded table the new scope excludes', () => {
131+
expect(shouldClear('out-of-scope', '/Reports')).toBe(true)
132+
})
133+
134+
it('keeps a table the scope still admits', () => {
135+
expect(shouldClear('in-scope', '/Reports')).toBe(false)
136+
})
137+
138+
it('keeps everything when the scope is cleared', () => {
139+
expect(shouldClear('out-of-scope', '')).toBe(false)
140+
})
141+
142+
it('never clears a table that is not in the loaded list', () => {
143+
/* Still loading, or deleted - clearing there would destroy a valid config. */
144+
expect(shouldClear('not-loaded', '/Reports')).toBe(false)
145+
})
146+
147+
it('never clears on a scope that cannot be parsed', () => {
148+
expect(shouldClear('out-of-scope', 'Reports')).toBe(false)
149+
})
150+
})

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/table-selector/table-selector.tsx‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client'
22

3-
import { useCallback, useMemo } from 'react'
3+
import { useCallback, useEffect, useMemo } from 'react'
44
import { Combobox, type ComboboxOption } from '@sim/emcn'
55
import { useParams } from 'next/navigation'
66
import { formatDisplayText } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/formatted-text'
@@ -76,6 +76,31 @@ export function TableSelector({
7676
const value = isPreview ? previewValue : storeValue
7777
const tableId = typeof value === 'string' ? value : null
7878

79+
const scoped = useMemo(() => {
80+
/* Decode the scope once for the whole list rather than per row. */
81+
const scopeSegments = parseFolderScope(folderScopePath)
82+
return scopeSegments?.length
83+
? tables.filter((table) => isTableInFolderScope(table, tableFolders, scopeSegments))
84+
: tables
85+
}, [tables, tableFolders, folderScopePath])
86+
87+
/*
88+
* Narrowing the folder must not leave a selection the picker no longer shows.
89+
* Filtering alone would render the placeholder while the block kept running
90+
* against the hidden table — the config would say one thing and the run do
91+
* another. Dropping the value makes the empty picker true.
92+
*
93+
* Only a table that IS loaded and IS out of scope is cleared. A table absent
94+
* from the list is a different situation (still loading, or deleted) and
95+
* clearing there would destroy a valid config over a transient cache state.
96+
*/
97+
useEffect(() => {
98+
if (isPreview || disabled || isLoading || !tableId) return
99+
if (!tables.some((table) => table.id === tableId)) return
100+
if (scoped.some((table) => table.id === tableId)) return
101+
setStoreValue('')
102+
}, [isPreview, disabled, isLoading, tableId, tables, scoped, setStoreValue])
103+
79104
/**
80105
* Two tables can share a name in different folders, so a colliding name is
81106
* suffixed with its folder path. Table names are lowercased for display (the
@@ -84,11 +109,6 @@ export function TableSelector({
84109
* disambiguated too. The folder path keeps its authored casing.
85110
*/
86111
const options = useMemo<ComboboxOption[]>(() => {
87-
/* Decode the scope once for the whole list rather than per row. */
88-
const scopeSegments = parseFolderScope(folderScopePath)
89-
const scoped = scopeSegments?.length
90-
? tables.filter((table) => isTableInFolderScope(table, tableFolders, scopeSegments))
91-
: tables
92112
const duplicateNames = collectDuplicateNames(scoped.map((table) => table.name.toLowerCase()))
93113
return scoped.map((table) => ({
94114
label: disambiguateLabelByFolder(
@@ -99,7 +119,7 @@ export function TableSelector({
99119
),
100120
value: table.id,
101121
}))
102-
}, [tables, tableFolders, folderScopePath])
122+
}, [scoped, tableFolders])
103123

104124
const handleChange = useCallback(
105125
(selectedValue: string) => {

‎apps/sim/blocks/blocks/table-folders.test.ts‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,3 +258,92 @@ describe('table_v2 names every operation on the canvas', () => {
258258
}
259259
})
260260
})
261+
262+
/*
263+
* A block used as an Agent tool is handed the TOOL's schema, not this block's
264+
* subblock ids, so a model answers with `path` / `folderPath` while the canvas
265+
* stores `folderRef` / `moveTargetRef`. Reading only the canvas id dropped the
266+
* model's answer — and on move the tool's root fallback then turned "move into
267+
* /Reports" into "move to the root", silently.
268+
*/
269+
describe('table_v2 keeps the destination a model asked for', () => {
270+
it('moves the table where the model said, not to the workspace root', () => {
271+
const params = paramsFor('move', { tableId: 'table-1', folderPath: '/Reports' })
272+
273+
expect(params.folderPath).toBe('/Reports')
274+
expect(tableMoveSchemas.body.safeParse(params).success).toBe(true)
275+
})
276+
277+
it('lets the canvas destination win when the author picked one', () => {
278+
const params = paramsFor('move', {
279+
tableId: 'table-1',
280+
moveTargetRef: '/Archive',
281+
folderPath: '/Reports',
282+
})
283+
284+
expect(params.folderPath).toBe('/Archive')
285+
})
286+
287+
it('still reads an unset canvas destination as the workspace root', () => {
288+
/* No model answer either, so the tool's root fallback is the only signal. */
289+
expect(paramsFor('move', { tableId: 'table-1', moveTargetRef: '' }).folderPath).toBeUndefined()
290+
})
291+
292+
it.each([
293+
['list_folders', { path: '/Reports' }, 'path'],
294+
['create_folder', { path: '/Reports/Q3' }, 'path'],
295+
['delete_folder', { path: '/Reports' }, 'path'],
296+
['restore_folder', { path: '/Reports/Q3' }, 'path'],
297+
] as const)('carries a model-supplied path through %s', (operation, modelParams, field) => {
298+
expect(paramsFor(operation, modelParams)[field]).toBe(modelParams.path)
299+
})
300+
301+
it('carries both ends of a model-driven folder move', () => {
302+
const params = paramsFor('update_folder', {
303+
path: '/Reports/Q3',
304+
destinationPath: '/Archive/Q3',
305+
})
306+
307+
expect(params).toMatchObject({ path: '/Reports/Q3', destinationPath: '/Archive/Q3' })
308+
expect(tableUpdateFolderSchemas.body.safeParse(params).success).toBe(true)
309+
})
310+
311+
it('keeps composing from the canvas when the author picked a source folder', () => {
312+
/* Canvas mode still moves to the root when no destination parent is set. */
313+
const params = paramsFor('update_folder', {
314+
folderRef: '/Reports/Q3',
315+
destinationParentRef: '',
316+
destinationPath: '/Ignored/Q3',
317+
})
318+
319+
expect(params).toMatchObject({ path: '/Reports/Q3', destinationPath: '/Q3' })
320+
})
321+
322+
it('carries model-supplied listing options', () => {
323+
const params = paramsFor('list_folders', {
324+
path: '/Reports',
325+
recursive: true,
326+
depth: 2,
327+
search: 'Q3',
328+
limit: 25,
329+
})
330+
331+
expect(params).toMatchObject({ recursive: true, depth: 2, search: 'Q3', limit: 25 })
332+
expect(tableListFoldersSchemas.body.safeParse(params).success).toBe(true)
333+
})
334+
335+
it('never takes the delete cascade from a model', () => {
336+
/*
337+
* `recursive` is user-only on the tool param AND on the subblock, so a model
338+
* answer must not reach the use case even if one is somehow present.
339+
*/
340+
const params = paramsFor('delete_folder', { path: '/Reports', recursive: true })
341+
342+
expect(params.recursive).toBe(false)
343+
})
344+
345+
it('marks the delete cascade user-only on the subblock, not just the tool', () => {
346+
/* The block's own schema is what an Agent tool surface is built from. */
347+
expect(subBlock('deleteFolderRecursive')?.paramVisibility).toBe('user-only')
348+
})
349+
})

‎apps/sim/blocks/blocks/table_v2.ts‎

Lines changed: 66 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,12 @@ interface TableBlockParams {
7474
folderSearch?: string
7575
folderLimit?: string
7676
deleteFolderRecursive?: unknown
77+
/* Model-supplied tool params, present when the block runs as an Agent tool. */
78+
path?: string
79+
destinationPath?: string
80+
search?: string
81+
depth?: string
82+
limit?: string
7783
}
7884

7985
/**
@@ -202,13 +208,40 @@ function folderPathPrefix(parentPath: string | undefined): string {
202208
return parentPath && parentPath !== ROOT_FOLDER_PATH ? parentPath : ''
203209
}
204210

211+
/**
212+
* The canvas value if the author set one, else the value the model supplied.
213+
*
214+
* A block used as an Agent tool is handed the TOOL's schema, not this block's
215+
* subblock ids (`createLLMToolSchema(toolConfig, ...)` in `providers/utils.ts`),
216+
* so a model answers with `path` / `folderPath` while the canvas stores
217+
* `folderRef` / `moveTargetRef`. Reading only the canvas id drops the model's
218+
* answer — and on move the tool then substitutes the workspace root, turning
219+
* "move into /Reports" into "move to the root". Same precedent as `columns`
220+
* above.
221+
*/
222+
function canvasOrModel<T>(canvas: T | undefined, model: T | undefined): T | undefined {
223+
return canvas !== undefined ? canvas : model
224+
}
225+
226+
/** A switch the author never touched arrives empty; only then does the model's answer count. */
227+
function switchOrModel(canvas: unknown, model: unknown): boolean {
228+
const authored = canvas !== undefined && canvas !== null && canvas !== ''
229+
return switchValue(authored ? canvas : model)
230+
}
231+
205232
const paramTransformers: Record<string, (params: TableBlockParams) => ParsedParams> = {
206233
list_folders: (params) => ({
207-
path: folderRefPath(params.folderRef),
208-
recursive: switchValue(params.folderRecursive),
209-
depth: parseOptionalNumberInput(params.folderDepth, 'Max Depth', { integer: true, min: 1 }),
210-
search: optionalText(params.folderSearch),
211-
limit: parseOptionalNumberInput(params.folderLimit, 'Limit', { integer: true, min: 1 }),
234+
path: canvasOrModel(folderRefPath(params.folderRef), optionalText(params.path)),
235+
recursive: switchOrModel(params.folderRecursive, params.recursive),
236+
depth: canvasOrModel(
237+
parseOptionalNumberInput(params.folderDepth, 'Max Depth', { integer: true, min: 1 }),
238+
parseOptionalNumberInput(params.depth, 'Max Depth', { integer: true, min: 1 })
239+
),
240+
search: canvasOrModel(optionalText(params.folderSearch), optionalText(params.search)),
241+
limit: canvasOrModel(
242+
parseOptionalNumberInput(params.folderLimit, 'Limit', { integer: true, min: 1 }),
243+
parseOptionalNumberInput(params.limit, 'Limit', { integer: true, min: 1 })
244+
),
212245
}),
213246

214247
/*
@@ -221,7 +254,10 @@ const paramTransformers: Record<string, (params: TableBlockParams) => ParsedPara
221254
const name = optionalText(params.folderName)
222255
const parentPrefix = folderPathPrefix(folderRefPath(params.createParentRef))
223256
return {
224-
path: name ? `${parentPrefix}/${encodeFolderPathSegment(name)}` : undefined,
257+
path: canvasOrModel(
258+
name ? `${parentPrefix}/${encodeFolderPathSegment(name)}` : undefined,
259+
optionalText(params.path)
260+
),
225261
}
226262
},
227263

@@ -233,27 +269,43 @@ const paramTransformers: Record<string, (params: TableBlockParams) => ParsedPara
233269
* and is not offered here.
234270
*/
235271
update_folder: (params) => {
236-
const sourcePath = folderRefPath(params.folderRef)
237-
const segment = sourcePath?.split('/').pop()
238-
const parentPrefix = folderPathPrefix(folderRefPath(params.destinationParentRef))
272+
const canvasSource = folderRefPath(params.folderRef)
273+
if (canvasSource) {
274+
/*
275+
* Canvas mode: the destination is composed, because the path the folder
276+
* WILL have does not exist yet and cannot be picked. An unset parent is
277+
* the workspace root, which is a real destination, not a missing one.
278+
*/
279+
const segment = canvasSource.split('/').pop()
280+
const parentPrefix = folderPathPrefix(folderRefPath(params.destinationParentRef))
281+
return {
282+
path: canvasSource,
283+
destinationPath: segment ? `${parentPrefix}/${segment}` : undefined,
284+
}
285+
}
286+
/* Agent mode: the model addresses both ends directly. */
239287
return {
240-
path: sourcePath,
241-
destinationPath: segment ? `${parentPrefix}/${segment}` : undefined,
288+
path: optionalText(params.path),
289+
destinationPath: optionalText(params.destinationPath),
242290
}
243291
},
244292

245293
delete_folder: (params) => ({
246-
path: folderRefPath(params.folderRef),
294+
path: canvasOrModel(folderRefPath(params.folderRef), optionalText(params.path)),
295+
/*
296+
* No model fallback: the cascade is `user-only` on both the tool param and
297+
* the subblock, so a model must never be able to supply it.
298+
*/
247299
recursive: switchValue(params.deleteFolderRecursive),
248300
}),
249301

250302
restore_folder: (params) => ({
251-
path: optionalText(params.restoreFolderPath),
303+
path: canvasOrModel(optionalText(params.restoreFolderPath), optionalText(params.path)),
252304
}),
253305

254306
move: (params) => ({
255307
tableId: params.tableId,
256-
folderPath: folderRefPath(params.moveTargetRef),
308+
folderPath: canvasOrModel(folderRefPath(params.moveTargetRef), optionalText(params.folderPath)),
257309
}),
258310

259311
insert_row: (params) => ({

0 commit comments

Comments
 (0)