diff --git a/apps/sim/app/api/copilot/chat/resources/route.ts b/apps/sim/app/api/copilot/chat/resources/route.ts index 85b81323a65..52f5c1e6c5e 100644 --- a/apps/sim/app/api/copilot/chat/resources/route.ts +++ b/apps/sim/app/api/copilot/chat/resources/route.ts @@ -16,10 +16,16 @@ import { createNotFoundResponse, createUnauthorizedResponse, } from '@/lib/copilot/request/http' -import type { ChatResource } from '@/lib/copilot/resources/persistence' +import { + type ChatResource, + serializeChatResourceWrite, + setChatResourceTxTimeouts, +} from '@/lib/copilot/resources/persistence' +import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types' import { canonicalizeDesktopSessionResource, - GENERIC_RESOURCE_TITLES, + mergeChatResource, + reorderStoredChatResources, sanitizeChatResources, } from '@/lib/copilot/resources/types' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' @@ -43,59 +49,56 @@ export const POST = withRouteHandler(async (req: NextRequest) => { } ) if (!parsed.success) return parsed.response - const { chatId, resource: requestedResource } = parsed.data.body + const { chatId, resource: requestedResource, clearViewId } = parsed.data.body const resource = canonicalizeDesktopSessionResource(requestedResource) + const resourceUpdate: MothershipResourceUpdate = + clearViewId === true ? { ...resource, clearViewId: true } : resource // Ephemeral UI tab (client does not POST this; guard for old clients / bugs). if (resource.id === 'streaming-file') { return NextResponse.json({ success: true }) } - const [chat] = await db - .select({ resources: copilotChats.resources }) - .from(copilotChats) - .where( - and( + const merged = await serializeChatResourceWrite(chatId, () => + db.transaction(async (tx) => { + await setChatResourceTxTimeouts(tx) + const scope = and( eq(copilotChats.id, chatId), eq(copilotChats.userId, userId), isNull(copilotChats.deletedAt) ) - ) - .limit(1) + const [chat] = await tx + .select({ resources: copilotChats.resources }) + .from(copilotChats) + .where(scope) + .for('update') + .limit(1) - if (!chat) { - return createNotFoundResponse('Chat not found or unauthorized') - } + if (!chat) return null - const existing = sanitizeChatResources( - Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] - ) - const key = `${resource.type}:${resource.id}` - const prev = existing.find((r) => `${r.type}:${r.id}` === key) - - let merged: ChatResource[] - if (prev) { - if (GENERIC_RESOURCE_TITLES.has(prev.title) && !GENERIC_RESOURCE_TITLES.has(resource.title)) { - merged = existing.map((r) => - `${r.type}:${r.id}` === key ? { ...r, title: resource.title } : r + const existing = sanitizeChatResources( + Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] ) - } else { - merged = existing - } - } else { - merged = [...existing, resource] - } + const key = `${resource.type}:${resource.id}` + const prev = existing.find((r) => `${r.type}:${r.id}` === key) + const next: ChatResource[] = prev + ? existing.map((r) => + `${r.type}:${r.id}` === key ? mergeChatResource(r, resourceUpdate) : r + ) + : [...existing, mergeChatResource(undefined, resourceUpdate)] + + await tx + .update(copilotChats) + .set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() }) + .where(scope) + + return next + }) + ) - await db - .update(copilotChats) - .set({ resources: sql`${JSON.stringify(merged)}::jsonb`, updatedAt: new Date() }) - .where( - and( - eq(copilotChats.id, chatId), - eq(copilotChats.userId, userId), - isNull(copilotChats.deletedAt) - ) - ) + if (!merged) { + return createNotFoundResponse('Chat not found or unauthorized') + } logger.info('Added resource to chat', { chatId, resource }) @@ -125,44 +128,45 @@ export const PATCH = withRouteHandler(async (req: NextRequest) => { if (!parsed.success) return parsed.response const { chatId, resources: newOrder } = parsed.data.body - const [chat] = await db - .select({ resources: copilotChats.resources }) - .from(copilotChats) - .where( - and( + const canonicalOrder = await serializeChatResourceWrite(chatId, () => + db.transaction(async (tx): Promise => { + await setChatResourceTxTimeouts(tx) + const scope = and( eq(copilotChats.id, chatId), eq(copilotChats.userId, userId), isNull(copilotChats.deletedAt) ) - ) - .limit(1) + const [chat] = await tx + .select({ resources: copilotChats.resources }) + .from(copilotChats) + .where(scope) + .for('update') + .limit(1) - if (!chat) { - return createNotFoundResponse('Chat not found or unauthorized') - } + if (!chat) return undefined + + const existing = sanitizeChatResources( + Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] + ) + const next = reorderStoredChatResources(existing, newOrder) + if (!next) return null + + await tx + .update(copilotChats) + .set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() }) + .where(scope) - const existing = sanitizeChatResources( - Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] + return next + }) ) - const canonicalOrder = sanitizeChatResources(newOrder) - const existingKeys = new Set(existing.map((r) => `${r.type}:${r.id}`)) - const newKeys = new Set(canonicalOrder.map((r) => `${r.type}:${r.id}`)) - if (existingKeys.size !== newKeys.size || ![...existingKeys].every((k) => newKeys.has(k))) { + if (canonicalOrder === undefined) { + return createNotFoundResponse('Chat not found or unauthorized') + } + if (!canonicalOrder) { return createBadRequestResponse('Reordered resources must match existing resources') } - await db - .update(copilotChats) - .set({ resources: sql`${JSON.stringify(canonicalOrder)}::jsonb`, updatedAt: new Date() }) - .where( - and( - eq(copilotChats.id, chatId), - eq(copilotChats.userId, userId), - isNull(copilotChats.deletedAt) - ) - ) - logger.info('Reordered resources for chat', { chatId, count: canonicalOrder.length }) return NextResponse.json({ success: true, resources: canonicalOrder }) @@ -191,39 +195,45 @@ export const DELETE = withRouteHandler(async (req: NextRequest) => { if (!parsed.success) return parsed.response const { chatId, resourceType, resourceId } = parsed.data.body - // Old builds could persist an inner browser/terminal tab id. Closing the - // singleton panel removes every legacy row of that type so it cannot be - // canonicalized back into view on the next hydration. - const removePredicate = - resourceType === 'browser' || resourceType === 'terminal' - ? sql`elem->>'type' = ${resourceType}` - : sql`elem->>'type' = ${resourceType} AND elem->>'id' = ${resourceId}` - - const [updated] = await db - .update(copilotChats) - .set({ - resources: sql`COALESCE(( - SELECT jsonb_agg(elem) - FROM jsonb_array_elements(${copilotChats.resources}) elem - WHERE NOT (${removePredicate}) - ), '[]'::jsonb)`, - updatedAt: new Date(), - }) - .where( - and( + const merged = await serializeChatResourceWrite(chatId, () => + db.transaction(async (tx) => { + await setChatResourceTxTimeouts(tx) + const scope = and( eq(copilotChats.id, chatId), eq(copilotChats.userId, userId), isNull(copilotChats.deletedAt) ) - ) - .returning({ resources: copilotChats.resources }) + const [chat] = await tx + .select({ resources: copilotChats.resources }) + .from(copilotChats) + .where(scope) + .for('update') + .limit(1) - if (!updated) { + if (!chat) return null + + const existing = sanitizeChatResources( + Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] + ) + const removeAllOfType = resourceType === 'browser' || resourceType === 'terminal' + const next = existing.filter( + (resource) => + resource.type !== resourceType || (!removeAllOfType && resource.id !== resourceId) + ) + + await tx + .update(copilotChats) + .set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() }) + .where(scope) + + return next + }) + ) + + if (!merged) { return createNotFoundResponse('Chat not found or unauthorized') } - const merged = Array.isArray(updated.resources) ? (updated.resources as ChatResource[]) : [] - logger.info('Removed resource from chat', { chatId, resourceType, resourceId }) return NextResponse.json({ success: true, resources: merged }) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.test.tsx new file mode 100644 index 00000000000..d6d37c9e4ed --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.test.tsx @@ -0,0 +1,70 @@ +/** + * @vitest-environment jsdom + */ +import { act, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/app/workspace/[workspaceId]/tables/[tableId]/table', () => ({ + Table: () => null, +})) +vi.mock( + '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/browser-session', + () => ({ BrowserSession: () => null }) +) +vi.mock( + '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/terminal-session/terminal-session', + () => ({ TerminalSession: () => null }) +) + +import { ResourceContent } from '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content' +import type { MothershipResource } from '@/app/workspace/[workspaceId]/home/types' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' + +describe('ResourceContent table view handoff', () => { + let container: HTMLDivElement + let root: Root + + beforeEach(() => { + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + useTableViewPinStore.getState().reset() + container = document.createElement('div') + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + useTableViewPinStore.getState().reset() + }) + + function render(resource: MothershipResource) { + act(() => { + root.render( + ( + + ) as ReactNode + ) + }) + } + + it('hands off a saved view that arrives after the embedded table mounts', () => { + const table: MothershipResource = { + type: 'table', + id: 'table-1', + title: 'Invoices', + } + render(table) + expect(useTableViewPinStore.getState().pins['table-1']).toBeUndefined() + + render({ ...table, viewId: 'view-edited' }) + const pin = useTableViewPinStore.getState().pins['table-1'] + expect(pin?.viewId).toBe('view-edited') + + render({ ...table, viewId: 'view-edited' }) + expect(useTableViewPinStore.getState().pins['table-1']?.seq).toBe(pin?.seq) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx index 82fcd1814e0..b4a6ca4a03a 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx @@ -62,6 +62,7 @@ import { useWorkflows } from '@/hooks/queries/workflows' import { useWorkspaceFiles } from '@/hooks/queries/workspace-files' import { useSettingsNavigation } from '@/hooks/use-settings-navigation' import { useExecutionStore } from '@/stores/execution/store' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' const Workflow = lazy(() => import('@/app/workspace/[workspaceId]/w/[workflowId]/workflow')) @@ -178,6 +179,25 @@ export const ResourceContent = memo(function ResourceContent({ visible = true, onBrowserOverlayControllerChange, }: ResourceContentProps) { + const observedTableViewRef = useRef( + resource.type === 'table' ? { tableId: resource.id, viewId: resource.viewId } : null + ) + + useEffect(() => { + const previous = observedTableViewRef.current + const next = + resource.type === 'table' ? { tableId: resource.id, viewId: resource.viewId } : null + observedTableViewRef.current = next + if (!next?.viewId || (previous?.tableId === next.tableId && previous.viewId === next.viewId)) { + return + } + /** + * `initialViewId` owns the first table adoption. If refreshed chat data + * supplies it later, use the same one-shot handoff as live stream events. + */ + useTableViewPinStore.getState().pin(next.tableId, next.viewId) + }, [resource.id, resource.type, resource.viewId]) + const streamFileName = previewSession?.fileName || 'file.md' const syntheticFile = useMemo(() => { const ext = getFileExtension(streamFileName) diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry/resource-registry.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry/resource-registry.tsx index 527c6d4cf3a..144e3f0fa1e 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry/resource-registry.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry/resource-registry.tsx @@ -300,6 +300,9 @@ const RESOURCE_INVALIDATORS: Record< table: (qc, _wId, id) => { qc.invalidateQueries({ queryKey: tableKeys.lists() }) qc.invalidateQueries({ queryKey: tableKeys.detail(id) }) + // A view the agent just created must be in the list before the embedded + // table can switch to it; see the view-pin store. + qc.invalidateQueries({ queryKey: tableKeys.views(id) }) }, file: (qc, wId, id) => { qc.invalidateQueries({ queryKey: workspaceFilesKeys.lists() }) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.test.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.test.ts index 5eb8df1154d..3bd6a2e6f25 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.test.ts @@ -20,6 +20,8 @@ import type { PersistedStreamEventEnvelope } from '@/lib/copilot/request/session import { handleResourceEvent } from '@/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event' import type { StreamLoopContext } from '@/app/workspace/[workspaceId]/home/hooks/stream/stream-context' import { makeStreamLoopDeps } from '@/app/workspace/[workspaceId]/home/hooks/stream/stream-test-helpers' +import type { MothershipResource } from '@/app/workspace/[workspaceId]/home/types' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' function removeEvent(type: 'workflow' | 'file', id: string): PersistedStreamEventEnvelope { return { @@ -105,3 +107,124 @@ describe('handleResourceEvent removal', () => { expect(onResourceEvent).toHaveBeenCalledWith('browser-session') }) }) + +function tableUpsertEvent( + id: string, + viewId?: string, + clearViewId?: true +): PersistedStreamEventEnvelope { + return { + type: 'resource', + v: 1, + seq: 1, + ts: '', + stream: { streamId: 's', cursor: '1' }, + payload: { + op: 'upsert', + resource: { + type: 'table', + id, + title: 'Invoices', + ...(viewId ? { viewId } : {}), + ...(clearViewId ? { clearViewId } : {}), + }, + }, + } as PersistedStreamEventEnvelope +} + +describe('handleResourceEvent saved-view pins', () => { + beforeEach(() => { + vi.clearAllMocks() + useTableViewPinStore.getState().reset() + }) + + it('opens a closed table on the view and leaves a pin for the table to consume', () => { + const onResourceEvent = vi.fn() + const deps = makeStreamLoopDeps({ onResourceEventRef: { current: onResourceEvent } }) + const ctx = { deps } as StreamLoopContext + + handleResourceEvent(ctx, tableUpsertEvent('tbl-1', 'view-1')) + + expect(deps.addResource).toHaveBeenCalledWith({ + type: 'table', + id: 'tbl-1', + title: 'Invoices', + viewId: 'view-1', + }) + // The pin merge always runs; on a list that lacks the table it is a no-op. + const updater = (deps.setResources as ReturnType).mock.calls[0][0] as ( + current: MothershipResource[] + ) => MothershipResource[] + const others: MothershipResource[] = [{ type: 'file', id: 'file-1', title: 'notes.md' }] + expect(updater(others)).toBe(others) + expect(useTableViewPinStore.getState().pins['tbl-1']?.viewId).toBe('view-1') + expect(mocks.invalidateResourceQueries).toHaveBeenCalledWith( + deps.queryClient, + 'ws-1', + 'table', + 'tbl-1' + ) + expect(onResourceEvent).toHaveBeenCalledWith('tbl-1') + }) + + it('moves the pin on an already-open table so a remount and the live grid both follow', () => { + const open: MothershipResource = { + type: 'table', + id: 'tbl-1', + title: 'Invoices', + viewId: 'view-1', + } + const deps = makeStreamLoopDeps({ + addResource: vi.fn(() => false), + resourcesRef: { current: [open] }, + }) + const ctx = { deps } as StreamLoopContext + + handleResourceEvent(ctx, tableUpsertEvent('tbl-1', 'view-2')) + + const updater = (deps.setResources as ReturnType).mock.calls[0][0] as ( + current: MothershipResource[] + ) => MothershipResource[] + expect(updater([open])).toEqual([{ ...open, viewId: 'view-2' }]) + expect(useTableViewPinStore.getState().pins['tbl-1']?.viewId).toBe('view-2') + }) + + it('ignores a pin on anything but a table and leaves unpinned tables alone', () => { + const deps = makeStreamLoopDeps({ addResource: vi.fn(() => false) }) + const ctx = { deps } as StreamLoopContext + + handleResourceEvent(ctx, tableUpsertEvent('tbl-1')) + + expect(deps.setResources).not.toHaveBeenCalled() + expect(useTableViewPinStore.getState().pins['tbl-1']).toBeUndefined() + }) + + it('clears the stored and pending pin when the agent deletes a saved view', () => { + const open: MothershipResource = { + type: 'table', + id: 'tbl-1', + title: 'Invoices', + viewId: 'view-1', + } + useTableViewPinStore.getState().pin('tbl-1', 'view-1') + const deps = makeStreamLoopDeps({ + addResource: vi.fn(() => false), + resourcesRef: { current: [open] }, + }) + const ctx = { deps } as StreamLoopContext + + handleResourceEvent(ctx, tableUpsertEvent('tbl-1', undefined, true)) + + expect(deps.addResource).toHaveBeenCalledWith({ + type: 'table', + id: 'tbl-1', + title: 'Invoices', + clearViewId: true, + }) + const updater = (deps.setResources as ReturnType).mock.calls[0][0] as ( + current: MothershipResource[] + ) => MothershipResource[] + expect(updater([open])).toEqual([{ type: 'table', id: 'tbl-1', title: 'Invoices' }]) + expect(useTableViewPinStore.getState().pins['tbl-1']).toBeUndefined() + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.ts index 12c1a2d6f94..e9565c7c5ba 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-resource-event.ts @@ -13,6 +13,7 @@ import { import type { StreamLoopContext } from '@/app/workspace/[workspaceId]/home/hooks/stream/stream-context' import type { MothershipResourceType } from '@/app/workspace/[workspaceId]/home/types' import { removeWorkflowFromActiveCache } from '@/hooks/queries/utils/workflow-cache' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' type ResourceEvent = Extract< @@ -44,12 +45,25 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven } = ctx.deps const onResourceEvent = onResourceEventRef.current const payload = parsed.payload + const shouldClearViewId = + payload.resource.type === 'table' && payload.resource.clearViewId === true + // A saved view the agent just created or edited: the table opens on it, and + // an already-open table switches to it. + const pinnedViewId = + !shouldClearViewId && + payload.resource.type === 'table' && + typeof payload.resource.viewId === 'string' && + payload.resource.viewId.trim() + ? payload.resource.viewId + : undefined const resource = canonicalizeDesktopSessionResource({ type: payload.resource.type as MothershipResourceType, id: payload.resource.id, title: typeof payload.resource.title === 'string' ? payload.resource.title : payload.resource.id, + ...(pinnedViewId ? { viewId: pinnedViewId } : {}), }) + const resourceUpdate = shouldClearViewId ? { ...resource, clearViewId: true as const } : resource if (payload.op === MothershipStreamV1ResourceOp.remove) { const resourceType = resource.type @@ -99,7 +113,7 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven !shouldAutoActivatePreviewSession(previewForResource))) const wasAdded = shouldSuppressFileResourceActivation ? !resourcesRef.current.some((r) => r.type === resource.type && r.id === resource.id) - : addResource(resource) + : addResource(resourceUpdate) if (shouldSuppressFileResourceActivation && wasAdded) { setResources((current) => current.some((r) => r.type === resource.type && r.id === resource.id) @@ -111,6 +125,33 @@ export function handleResourceEvent(ctx: StreamLoopContext, parsed: ResourceEven completedPreviewResourceHandoffRef.current.delete(resource.id) previewActivationOwnerRef.current.delete(completedPreviewHandoff.sessionId) } + if (pinnedViewId) { + // Carry the newest pin on an existing tab so a remount adopts it. Not gated + // on `wasAdded`: two upserts in one render both read the stale ref and both + // report "added", while only the first updater actually inserted — the + // updater is idempotent, so it simply runs every time. + setResources((current) => + current.some((r) => r.type === 'table' && r.id === resource.id && r.viewId !== pinnedViewId) + ? current.map((r) => + r.type === 'table' && r.id === resource.id ? { ...r, viewId: pinnedViewId } : r + ) + : current + ) + // Consumed by the embedded table once its views list carries the view — + // which may be after the refetch below lands, or after the tab first opens. + useTableViewPinStore.getState().pin(resource.id, pinnedViewId) + } else if (shouldClearViewId) { + setResources((current) => + current.some((r) => r.type === 'table' && r.id === resource.id && r.viewId !== undefined) + ? current.map((r) => { + if (r.type !== 'table' || r.id !== resource.id) return r + const { viewId: _viewId, ...unpinned } = r + return unpinned + }) + : current + ) + useTableViewPinStore.getState().clear(resource.id) + } invalidateResourceQueries(queryClient, workspaceId, resource.type, resource.id) if (!shouldSuppressFileResourceActivation) onResourceEvent?.(resource.id) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/stream-context.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/stream-context.ts index 5d700ffae64..a10dbca49cf 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/stream-context.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/stream-context.ts @@ -5,6 +5,7 @@ import type { RevealedSimKeysByMessage } from '@/lib/copilot/chat/sim-key-redact import { captureRevealedSimKeys } from '@/lib/copilot/chat/sim-key-redaction' import type { SyntheticFilePreviewPayload } from '@/lib/copilot/request/session' import type { FilePreviewSession } from '@/lib/copilot/request/session/file-preview-session-contract' +import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types' import { createTurnModel, type TurnModel, @@ -95,7 +96,7 @@ export interface StreamLoopDeps { setResources: Dispatch> setActiveResourceId: Dispatch> - addResource: (resource: MothershipResource) => boolean + addResource: (resource: MothershipResourceUpdate) => boolean removeResource: (resourceType: MothershipResourceType, resourceId: string) => void startClientWorkflowTool: (id: string, name: string, args: Record) => void startClientLocalFilesystemTool: (id: string, name: string, args: Record) => void diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx index 48e7b83e235..374620f48b0 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx @@ -42,6 +42,7 @@ vi.mock('@/lib/api/client/request', async (importOriginal) => ({ import { MothershipHandoffStorage } from '@/lib/core/utils/browser-storage' import { useChat } from '@/app/workspace/[workspaceId]/home/hooks/use-chat' +import { type MothershipChatHistory, mothershipChatKeys } from '@/hooks/queries/mothership-chats' import { useMothershipQueueStore } from '@/stores/mothership-queue/store' const DEDUPED_CHAT_ID = 'chat-the-first-attempt-opened' @@ -150,13 +151,18 @@ function renderUseChat(): { * pathname has to match: the hook resets a chat-bound surface back to a fresh * pending key when it finds itself on the home route. */ -function renderUseChatInChat(chatId: string): { +function renderUseChatInChat( + chatId: string, + sharedQueryClient: QueryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }) +): { getResult: () => ReturnType unmount: () => void } { navigationMocks.usePathname.mockReturnValue(`/workspace/ws-1/chat/${chatId}`) ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true - queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + queryClient = sharedQueryClient const container = document.createElement('div') const root = createRoot(container) mountedRoots.push(root) @@ -509,4 +515,66 @@ describe('useChat remount send recovery', () => { // Must NOT have gone to the cross-surface handoff. expect(MothershipHandoffStorage.consume('ws-1')).toBeNull() }) + + it('restores the last edited table view after switching away and back', async () => { + const chatId = 'chat-with-table' + const sharedQueryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }) + const initialHistory: MothershipChatHistory = { + id: chatId, + title: 'Table chat', + messages: [], + activeStreamId: null, + resources: [{ type: 'table', id: 'table-1', title: 'Invoices' }], + } + sharedQueryClient.setQueryData(mothershipChatKeys.detail(chatId), initialHistory) + + const firstSurface = renderUseChatInChat(chatId, sharedQueryClient) + await waitFor(() => firstSurface.getResult().resources.length === 1) + + act(() => { + firstSurface.getResult().addResource({ + type: 'table', + id: 'table-1', + title: 'Invoices', + viewId: 'view-edited', + }) + }) + await waitFor(() => firstSurface.getResult().resources[0]?.viewId === 'view-edited') + firstSurface.unmount() + + const restoredSurface = renderUseChatInChat(chatId, sharedQueryClient) + await waitFor(() => restoredSurface.getResult().resources.length === 1) + + expect(restoredSurface.getResult().resources[0]?.viewId).toBe('view-edited') + }) + + it('hydrates a table view change when resource identity and title stay the same', async () => { + const chatId = 'chat-with-refetched-view' + const sharedQueryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }) + const initialHistory: MothershipChatHistory = { + id: chatId, + title: 'Table chat', + messages: [], + activeStreamId: null, + resources: [{ type: 'table', id: 'table-1', title: 'Invoices' }], + } + sharedQueryClient.setQueryData(mothershipChatKeys.detail(chatId), initialHistory) + + const surface = renderUseChatInChat(chatId, sharedQueryClient) + await waitFor(() => surface.getResult().resources.length === 1) + + act(() => { + sharedQueryClient.setQueryData(mothershipChatKeys.detail(chatId), { + ...initialHistory, + resources: [{ type: 'table', id: 'table-1', title: 'Invoices', viewId: 'view-refetched' }], + }) + }) + + await waitFor(() => surface.getResult().resources[0]?.viewId === 'view-refetched') + expect(surface.getResult().resources[0]?.viewId).toBe('view-refetched') + }) }) 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 8a9a6399493..f19aebfee9b 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -18,6 +18,7 @@ import { isRecordLike } from '@sim/utils/object' import { backoffWithJitter } from '@sim/utils/retry' import { useQueryClient } from '@tanstack/react-query' import { usePathname, useRouter } from 'next/navigation' +import { isApiClientError } from '@/lib/api/client/errors' import { requestJson } from '@/lib/api/client/request' import { addMothershipChatResourceContract, @@ -66,10 +67,13 @@ import { } from '@/lib/copilot/request/session/file-preview-session-contract' import type { StreamBatchEvent } from '@/lib/copilot/request/session/types' import { canDisplayResource } from '@/lib/copilot/resources/availability' +import { ResourcePersistenceQueue } from '@/lib/copilot/resources/client-persistence-queue' import { BROWSER_SESSION_RESOURCE_ID, isAddressableResource, isEphemeralResource, + type MothershipResourceUpdate, + mergeChatResource, sanitizeChatResources, TERMINAL_SESSION_RESOURCE_ID, } from '@/lib/copilot/resources/types' @@ -136,6 +140,7 @@ import type { QueuedSendHandoffSeed, } from '@/stores/mothership-queue/types' import type { ChatContext } from '@/stores/panel' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' import { useTerminalConsoleStore } from '@/stores/terminal' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import type { WorkflowMetadata } from '@/stores/workflows/registry/types' @@ -210,7 +215,7 @@ export interface UseChatReturn { resources: MothershipResource[] activeResourceId: string | null setActiveResourceId: (id: string | null) => void - addResource: (resource: MothershipResource) => boolean + addResource: (resource: MothershipResourceUpdate) => boolean removeResource: (resourceType: MothershipResourceType, resourceId: string) => void reorderResources: (resources: MothershipResource[]) => void messageQueue: QueuedMessage[] @@ -906,10 +911,19 @@ function markMessageStopped(message: PersistedMessage): PersistedMessage { }) } +function buildChatResourceHydrationKey(resource: MothershipResource): string { + return JSON.stringify([ + resource.type, + resource.id, + resource.title, + resource.path ?? null, + resource.viewId ?? null, + resource.executionId ?? null, + ]) +} + function buildChatHistoryHydrationKey(chatHistory: MothershipChatHistory): string { - const resourceKey = chatHistory.resources - .map((resource) => `${resource.type}:${resource.id}:${resource.title}`) - .join('|') + const resourceKey = chatHistory.resources.map(buildChatResourceHydrationKey).join('|') const messageKey = chatHistory.messages.map((message) => message.id).join('|') const streamSnapshot = chatHistory.streamSnapshot const snapshotKey = streamSnapshot @@ -1381,9 +1395,27 @@ export function useChat( * make the tabs disappear for the desktop app too. */ const undisplayableResourcesRef = useRef([]) - const pendingPersistResourceKeysRef = useRef>(new Set()) - const inFlightResourceAddsRef = useRef>>(new Map()) - const reorderNeededAfterFlushRef = useRef(false) + const resourcePersistenceQueueRef = useRef(null) + if (!resourcePersistenceQueueRef.current) { + resourcePersistenceQueueRef.current = new ResourcePersistenceQueue({ + persist: (chatId, update) => { + const { clearViewId, ...resource } = update + return requestJson(addMothershipChatResourceContract, { + body: { + chatId, + resource, + ...(clearViewId === true ? { clearViewId: true as const } : {}), + }, + }) + }, + onError: (error) => { + logger.warn('Failed to persist resource; will retry on next hydration', error) + }, + }) + } + const resourcePersistenceQueue = resourcePersistenceQueueRef.current + const pendingResourceReordersRef = useRef(new Map()) + const pendingResourceReorderFlushesRef = useRef(new Map>()) // Derive the effective active resource ID for rendering without writing a // passive fallback back into the user's URL selection. @@ -1637,10 +1669,9 @@ export function useChat( setTransportIdle() setResources([]) setActiveResourceId(null) + // Pending view pins belong to the chat whose stream issued them. + useTableViewPinStore.getState().reset() undisplayableResourcesRef.current = [] - pendingPersistResourceKeysRef.current.clear() - inFlightResourceAddsRef.current.clear() - reorderNeededAfterFlushRef.current = false resetEphemeralPreviewState() // Editing binds to this hook's composer — release it before rotating chatKey. useMothershipQueueStore.getState().setEditing(chatKeyRef.current, null) @@ -1667,47 +1698,69 @@ export function useChat( workspaceId, ]) - const flushPendingResources = useCallback(async (chatId: string) => { - const pendingKeys = pendingPersistResourceKeysRef.current - if (pendingKeys.size === 0) return - const flushPromises: Array> = [] - for (const resource of resourcesRef.current) { - if (resource.id === 'streaming-file') continue - const key = `${resource.type}:${resource.id}` - if (!pendingKeys.has(key)) continue - pendingKeys.delete(key) - const promise = requestJson(addMothershipChatResourceContract, { - body: { chatId, resource }, + const flushPendingResourceReorder = useCallback( + (chatId: string): Promise => { + const activeFlush = pendingResourceReorderFlushesRef.current.get(chatId) + if (activeFlush) return activeFlush + + const flush = async () => { + while (true) { + const pendingOrder = pendingResourceReordersRef.current.get(chatId) + if (!pendingOrder) return + if (resourcePersistenceQueue.hasPendingIdentityChanges(chatId)) return + + const inFlightWrites = resourcePersistenceQueue.getInFlightWrites(chatId) + if (inFlightWrites.length > 0) { + await Promise.allSettled(inFlightWrites) + continue + } + + pendingResourceReordersRef.current.delete(chatId) + if (pendingOrder.length === 0) return + try { + await requestJson(reorderMothershipChatResourcesContract, { + body: { chatId, resources: pendingOrder }, + }) + } catch (error) { + // 400 is the server rejecting the body's identity set — a tab was + // closed after this order was captured. Replaying it verbatim can + // only fail again, so drop it and let the next reorder or hydration + // re-establish the order. Everything else (offline, 401, 5xx) is + // transient and keeps the body for the next retry. + const unsatisfiable = isApiClientError(error) && error.status === 400 + if (!unsatisfiable && !pendingResourceReordersRef.current.has(chatId)) { + pendingResourceReordersRef.current.set(chatId, pendingOrder) + } + logger.warn( + unsatisfiable + ? 'Discarded a resource reorder the server rejected' + : 'Failed to persist resource reorder; will retry on next hydration', + error + ) + return + } + } + } + const tracked = flush().finally(() => { + if (pendingResourceReorderFlushesRef.current.get(chatId) === tracked) { + pendingResourceReorderFlushesRef.current.delete(chatId) + } }) - .catch((err) => { - pendingPersistResourceKeysRef.current.add(key) - logger.warn('Failed to flush pending resource; will retry on next hydration', err) - }) - .finally(() => { - inFlightResourceAddsRef.current.delete(key) - }) - inFlightResourceAddsRef.current.set(key, promise) - flushPromises.push(promise) - } - if (flushPromises.length === 0) return - await Promise.allSettled(flushPromises) - if (!reorderNeededAfterFlushRef.current) return - reorderNeededAfterFlushRef.current = false - const localOrder = [ - ...resourcesRef.current.filter( - (r) => - r.id !== 'streaming-file' && - !pendingPersistResourceKeysRef.current.has(`${r.type}:${r.id}`) - ), - ...undisplayableResourcesRef.current, - ] - if (localOrder.length === 0) return - requestJson(reorderMothershipChatResourcesContract, { - body: { chatId, resources: localOrder }, - }).catch((err) => { - logger.warn('Failed to sync resource order after flush', err) - }) - }, []) + pendingResourceReorderFlushesRef.current.set(chatId, tracked) + return tracked + }, + [resourcePersistenceQueue] + ) + + const flushPendingResources = useCallback( + async (chatId: string, sourceScopeId: string = chatId) => { + if (resourcePersistenceQueue.getPendingResourceKeys(sourceScopeId).size > 0) { + await resourcePersistenceQueue.flush(chatId, sourceScopeId) + } + await flushPendingResourceReorder(chatId) + }, + [flushPendingResourceReorder, resourcePersistenceQueue] + ) const adoptResolvedChatId = useCallback( (chatId: string, options?: { replaceHomeHistory?: boolean; invalidateList?: boolean }) => { @@ -1784,7 +1837,7 @@ export function useChat( if (options?.invalidateList) { queryClient.invalidateQueries({ queryKey: mothershipChatKeys.list(workspaceId) }) } - flushPendingResources(chatId) + flushPendingResources(chatId, pendingChatKey) }, [flushPendingResources, queryClient, workspaceId] ) @@ -1795,87 +1848,102 @@ export function useChat( const source = chatHistory?.messages.map(toDisplayMessage) ?? pendingMessages return source.map((m) => restoreRevealedSimKeysForMessage(m, revealedSimKeysRef.current)) }, [chatHistory, pendingMessages]) - const addResource = useCallback((resource: MothershipResource): boolean => { - // The single fan-in for tab creation, so the invariant lives here. - if (!isAddressableResource(resource)) { - logger.warn('Ignored a resource with no id', { type: resource.type, title: resource.title }) - return false - } - if (resourcesRef.current.some((r) => r.type === resource.type && r.id === resource.id)) { - return false - } - - setResources((prev) => { - const exists = prev.some((r) => r.type === resource.type && r.id === resource.id) - if (exists) return prev - return [...prev, resource] - }) - // Synthetic result/preview panels are in-memory only. The browser tab - // metadata is persisted even though its live page remains desktop-owned. - if (isEphemeralResource(resource)) { - return true - } + const addResource = useCallback( + (resourceUpdate: MothershipResourceUpdate): boolean => { + // The single fan-in for tab creation, so the invariant lives here. + if (!isAddressableResource(resourceUpdate)) { + logger.warn('Ignored a resource with no id', { + type: resourceUpdate.type, + title: resourceUpdate.title, + }) + return false + } + const existing = resourcesRef.current.find( + (r) => r.type === resourceUpdate.type && r.id === resourceUpdate.id + ) + const resource = mergeChatResource(existing, resourceUpdate) + const persistChatId = chatIdRef.current ?? selectedChatIdRef.current + if (persistChatId && !isEphemeralResource(resource)) { + queryClient.setQueryData( + mothershipChatKeys.detail(persistChatId), + (current) => { + if (!current) return current + const cached = current.resources.find( + (item) => item.type === resource.type && item.id === resource.id + ) + const merged = mergeChatResource(cached, resourceUpdate) + if (cached === merged) return current + return { + ...current, + resources: cached + ? current.resources.map((item) => + item.type === resource.type && item.id === resource.id ? merged : item + ) + : [...current.resources, merged], + } + } + ) + } + if (existing && resource === existing && resourceUpdate.clearViewId !== true) { + return false + } - const persistChatId = chatIdRef.current ?? selectedChatIdRef.current - const key = `${resource.type}:${resource.id}` - // `resourcesRef` is written during render, so adds of the same resource in - // one tick all read the pre-render list and all pass the check above. State - // converges (the updater is idempotent) but each fired its own POST — 5-6 - // per resource in production. - const alreadyPersisting = - inFlightResourceAddsRef.current.has(key) || pendingPersistResourceKeysRef.current.has(key) - if (alreadyPersisting) { - return true - } - if (persistChatId) { - const promise = requestJson(addMothershipChatResourceContract, { - body: { chatId: persistChatId, resource }, + setResources((prev) => { + const current = prev.find((r) => r.type === resource.type && r.id === resource.id) + if (!current) return [...prev, resource] + const merged = mergeChatResource(current, resourceUpdate) + return merged === current + ? prev + : prev.map((r) => (r.type === resource.type && r.id === resource.id ? merged : r)) }) - .catch((err) => { - pendingPersistResourceKeysRef.current.add(key) - logger.warn('Failed to persist resource; will retry on next hydration', err) - }) - .finally(() => { - inFlightResourceAddsRef.current.delete(key) - }) - inFlightResourceAddsRef.current.set(key, promise) - } else { - pendingPersistResourceKeysRef.current.add(key) - } - return true - }, []) + // Synthetic result/preview panels are in-memory only. The browser tab + // metadata is persisted even though its live page remains desktop-owned. + if (isEphemeralResource(resource)) { + return true + } - const removeResource = useCallback((resourceType: MothershipResourceType, resourceId: string) => { - setResources((prev) => prev.filter((r) => !(r.type === resourceType && r.id === resourceId))) - setActiveResourceId((prev) => (prev === resourceId ? null : prev)) + const persistenceScopeId = persistChatId ?? pendingChatKeyRef.current + resourcePersistenceQueue.enqueue(resourceUpdate, persistChatId, persistenceScopeId, existing) + return existing === undefined + }, + [queryClient, resourcePersistenceQueue] + ) - // Ephemeral panels were never persisted; nothing to delete server-side. - if (isEphemeralResource({ type: resourceType, id: resourceId, title: '' })) return + const removeResource = useCallback( + (resourceType: MothershipResourceType, resourceId: string) => { + setResources((prev) => prev.filter((r) => !(r.type === resourceType && r.id === resourceId))) + setActiveResourceId((prev) => (prev === resourceId ? null : prev)) - const key = `${resourceType}:${resourceId}` - const wasPending = pendingPersistResourceKeysRef.current.delete(key) - const inFlightAdd = inFlightResourceAddsRef.current.get(key) - if (wasPending && !inFlightAdd) return + // Ephemeral panels were never persisted; nothing to delete server-side. + if (isEphemeralResource({ type: resourceType, id: resourceId, title: '' })) return - const persistChatId = chatIdRef.current ?? selectedChatIdRef.current - if (!persistChatId) return - const fireDelete = () => { - requestJson(removeMothershipChatResourceContract, { - body: { chatId: persistChatId, resourceType, resourceId }, - }).catch((err) => { - logger.warn('Failed to persist resource removal', err) - }) - } - if (inFlightAdd) { - // Drop the entry now, not when the add settles: an add being deleted must - // not suppress a fresh add of the same resource. The chained delete keeps - // its own reference to the promise. - inFlightResourceAddsRef.current.delete(key) - inFlightAdd.finally(fireDelete) - } else { - fireDelete() - } - }, []) + const existing = resourcesRef.current.find( + (resource) => resource.type === resourceType && resource.id === resourceId + ) + const persistChatId = chatIdRef.current ?? selectedChatIdRef.current + const persistenceScopeId = persistChatId ?? pendingChatKeyRef.current + const { + inFlight: inFlightAdd, + scheduleDelete, + wasPending, + wasPersisted, + } = resourcePersistenceQueue.remove( + resourceType, + resourceId, + persistenceScopeId, + Boolean(existing && persistChatId) + ) + if (wasPending && !inFlightAdd && !wasPersisted) return + + if (!persistChatId) return + scheduleDelete(persistChatId, () => + requestJson(removeMothershipChatResourceContract, { + body: { chatId: persistChatId, resourceType, resourceId }, + }) + ) + }, + [resourcePersistenceQueue] + ) /** * Drops hydrated workflow tabs whose workflow no longer exists, so an old @@ -1908,53 +1976,20 @@ export function useChat( [workspaceId, removeResource] ) - const reorderResources = useCallback((newOrder: MothershipResource[]) => { - setResources(newOrder) - const persistChatId = chatIdRef.current ?? selectedChatIdRef.current - if (!persistChatId) return - const pendingKeys = pendingPersistResourceKeysRef.current - const inFlightAdds = inFlightResourceAddsRef.current - const hasUnsyncedAdds = newOrder.some((r) => { - const key = `${r.type}:${r.id}` - return pendingKeys.has(key) || inFlightAdds.has(key) - }) - if (hasUnsyncedAdds) { - reorderNeededAfterFlushRef.current = true - if (pendingKeys.size === 0 && inFlightAdds.size > 0) { - Promise.allSettled(Array.from(inFlightAdds.values())).then(() => { - if (!reorderNeededAfterFlushRef.current) return - reorderNeededAfterFlushRef.current = false - const chatId = chatIdRef.current ?? selectedChatIdRef.current - if (!chatId) return - const order = [ - ...resourcesRef.current.filter( - (r) => - !isEphemeralResource(r) && - !pendingPersistResourceKeysRef.current.has(`${r.type}:${r.id}`) - ), - ...undisplayableResourcesRef.current, - ] - if (order.length === 0) return - requestJson(reorderMothershipChatResourcesContract, { - body: { chatId, resources: order }, - }).catch((err) => { - logger.warn('Failed to sync resource order after in-flight ADDs', err) - }) - }) - } - return - } - const persistableResources = [ - ...newOrder.filter((r) => !isEphemeralResource(r)), - ...undisplayableResourcesRef.current, - ] - if (persistableResources.length === 0) return - requestJson(reorderMothershipChatResourcesContract, { - body: { chatId: persistChatId, resources: persistableResources }, - }).catch((err) => { - logger.warn('Failed to persist resource reorder', err) - }) - }, []) + const reorderResources = useCallback( + (newOrder: MothershipResource[]) => { + setResources(newOrder) + const persistChatId = chatIdRef.current ?? selectedChatIdRef.current + if (!persistChatId) return + const persistableResources = [ + ...newOrder.filter((resource) => !isEphemeralResource(resource)), + ...undisplayableResourcesRef.current, + ] + pendingResourceReordersRef.current.set(persistChatId, persistableResources) + void flushPendingResourceReorder(persistChatId) + }, + [flushPendingResourceReorder] + ) const ensureWorkflowToolResource = useCallback( (toolArgs: Record): string | undefined => { @@ -2238,11 +2273,8 @@ export function useChat( const streamOwnerId = chatIdRef.current const pendingTurn = activeTurnRef.current const pendingStreamId = streamIdRef.current ?? pendingTurn?.userMessageId - const pendingResources = resourcesRef.current.filter( - (resource) => - !isEphemeralResource(resource) && - pendingPersistResourceKeysRef.current.has(`${resource.type}:${resource.id}`) - ) + const pendingResourceScopeId = + streamOwnerId ?? pendingTurn?.pendingChatKey ?? pendingChatKeyRef.current const navigatedToDifferentChat = sendingRef.current && initialChatId !== streamOwnerId && @@ -2287,13 +2319,7 @@ export function useChat( if (pendingChatKey) { useMothershipQueueStore.getState().migrate(pendingChatKey, resolvedChatId) } - await Promise.allSettled( - pendingResources.map((resource) => - requestJson(addMothershipChatResourceContract, { - body: { chatId: resolvedChatId, resource }, - }) - ) - ) + await resourcePersistenceQueue.flush(resolvedChatId, pendingResourceScopeId) queryClient.invalidateQueries({ queryKey: mothershipChatKeys.detail(resolvedChatId), }) @@ -2339,9 +2365,7 @@ export function useChat( setTransportIdle() setResources([]) setActiveResourceId(null) - pendingPersistResourceKeysRef.current.clear() - inFlightResourceAddsRef.current.clear() - reorderNeededAfterFlushRef.current = false + useTableViewPinStore.getState().reset() resetEphemeralPreviewState() // Rotate the bucket key; the previous chat's queue stays in the store. // Release editing on the chat we're leaving (composer-scoped). @@ -2474,9 +2498,8 @@ export function useChat( mergedResources.length === resourcesRef.current.length && mergedResources.every( (resource, index) => - resourcesRef.current[index].type === resource.type && - resourcesRef.current[index].id === resource.id && - resourcesRef.current[index].title === resource.title + buildChatResourceHydrationKey(resourcesRef.current[index]) === + buildChatResourceHydrationKey(resource) ) if (mergedResources.length > 0) { diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx index d39a6f474d0..a6fbe933f0c 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx @@ -44,6 +44,7 @@ import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/provide import { getTableViewRevision, resolveTableViewConfig, + resolveTableViewPinTransition, resolveTableViewSelection, shouldApplyTableViewRevision, type TableViewRevision, @@ -69,6 +70,7 @@ import { useInlineRename } from '@/hooks/use-inline-rename' import { useSettingsNavigation } from '@/hooks/use-settings-navigation' import { useLogDetailsUIStore } from '@/stores/logs/store' import type { DeletedRowSnapshot } from '@/stores/table/types' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' import { type ColumnConfig, ColumnConfigSidebar, @@ -381,9 +383,13 @@ export function Table({ const updateMetadataMutation = useUpdateTableMetadata({ workspaceId, tableId }) const deleteViewMutation = useDeleteTableView({ workspaceId, tableId }) - /** Resolve the default synchronously so the grid, autosave owner, and menu all - * agree before the URL effect records the adopted view id. */ - const { selectedView, defaultView, activeView } = resolveTableViewSelection(views, activeViewId) + /** Resolve the restored or default view synchronously so the grid, autosave + * owner, and menu agree before the URL effect records the adopted view id. */ + const { selectedView, defaultView, activeView } = resolveTableViewSelection( + views, + activeViewId, + embedded ? initialViewId : undefined + ) const activeViewConfig = useMemo( () => resolveTableViewConfig(tableData?.metadata, activeView?.config ?? null), [tableData?.metadata, activeView?.config] @@ -660,6 +666,13 @@ export function Table({ return } + // Embedded tables record the adopted id BEFORE the revision guard can bail: + // `resolveTableViewSelection` resolves a null param to the restored view, so + // leaving the param unwritten lets a later render drift back to the default. + // Standalone tables have no restored view and keep writing it below. + if (embedded && activeView && activeViewId === null) { + setTableParams({ view: activeView.id }) + } const nextViewRevision = getTableViewRevision(activeView) if ( !shouldApplyTableViewRevision( @@ -703,6 +716,47 @@ export function Table({ tableData?.metadata, ]) + /** + * A view the agent just created or edited (see the view-pin store). Applied + * only once the views list carries it — the pin arrives ahead of the list + * refetch, and writing the URL earlier would name a view the effect above + * resolves to nothing and treats as dead. First adoption is left to that + * effect (it honours `initialViewId` itself); a pin that turns out to be the + * view already applied is consumed without a URL write. + */ + const viewPin = useTableViewPinStore((state) => state.pins[tableId]) + const consumeViewPin = useTableViewPinStore((state) => state.consume) + useEffect(() => { + if (!embedded || !viewPin) return + if (appliedViewRevisionRef.current === undefined) return + if (!views.some((view) => view.id === viewPin.viewId)) return + consumeViewPin(tableId, viewPin.seq) + const transition = resolveTableViewPinTransition( + activeViewId, + appliedViewRevisionRef.current.id, + viewPin.viewId, + pendingCreatedViewIdRef.current + ) + pendingCreatedViewIdRef.current = transition.pendingCreatedViewId + if (!transition.nextViewId) return + preservedViewStateRef.current = null + setTableParams({ view: transition.nextViewId }) + // `viewsAvailable`/`tableAvailable` are what gate first adoption, and + // adoption records itself in a ref, which re-renders nothing. Without them + // a pin that arrives before the table is ready is never reconsidered — the + // restore path has no query invalidation to nudge `views` and rescue it. + }, [ + embedded, + viewPin, + views, + activeViewId, + tableId, + viewsAvailable, + tableAvailable, + consumeViewPin, + setTableParams, + ]) + /** * Live state pruned the same way `pruneViewConfig` prunes the stored config on * read. Without this, deleting a hidden or sorted column leaves the local ids diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.test.ts b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.test.ts index 7cbc4cc5927..b860963115f 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.test.ts @@ -7,6 +7,7 @@ import { ALL_VIEW_PARAM } from '@/app/workspace/[workspaceId]/tables/[tableId]/s import { getTableViewRevision, resolveTableViewConfig, + resolveTableViewPinTransition, resolveTableViewSelection, shouldApplyTableViewRevision, } from '@/app/workspace/[workspaceId]/tables/[tableId]/view-state' @@ -44,6 +45,13 @@ const DEFAULT_VIEW: TableViewWire = { updatedAt: new Date('2026-08-15T01:10:00.000Z'), } +const PINNED_VIEW: TableViewWire = { + ...DEFAULT_VIEW, + id: 'view-pinned', + name: 'Pinned', + isDefault: false, +} + describe('resolveTableViewSelection', () => { it('makes the persisted default active before its URL id is adopted', () => { expect(resolveTableViewSelection([DEFAULT_VIEW], null)).toEqual({ @@ -74,11 +82,58 @@ describe('resolveTableViewSelection', () => { }) }) + it('keeps a restored embedded view active while the host URL is absent', () => { + expect( + resolveTableViewSelection([DEFAULT_VIEW, PINNED_VIEW], null, PINNED_VIEW.id).activeView + ).toBe(PINNED_VIEW) + }) + + it('lets an explicit URL selection override the restored embedded view', () => { + expect( + resolveTableViewSelection([DEFAULT_VIEW, PINNED_VIEW], DEFAULT_VIEW.id, PINNED_VIEW.id) + .activeView + ).toBe(DEFAULT_VIEW) + }) + it('upgrades the legacy All sentinel when a persisted default exists', () => { expect(resolveTableViewSelection([DEFAULT_VIEW], ALL_VIEW_PARAM).activeView).toBe(DEFAULT_VIEW) }) }) +describe('resolveTableViewPinTransition', () => { + it('abandons a pending local creation when an external pin replaces its URL selection', () => { + expect( + resolveTableViewPinTransition('view-old', 'view-created', 'view-pinned', 'view-created') + ).toEqual({ nextViewId: 'view-pinned', pendingCreatedViewId: null }) + }) + + it('clears a different pending creation when the pin is already represented in the URL', () => { + expect( + resolveTableViewPinTransition('view-pinned', 'view-created', 'view-pinned', 'view-created') + ).toEqual({ nextViewId: null, pendingCreatedViewId: null }) + }) + + it('keeps a pending creation when it created the pinned view', () => { + expect( + resolveTableViewPinTransition('view-pinned', 'view-pinned', 'view-pinned', 'view-pinned') + ).toEqual({ nextViewId: null, pendingCreatedViewId: 'view-pinned' }) + }) + + it('replaces a different active URL even if the pin was applied previously', () => { + expect(resolveTableViewPinTransition('view-user', 'view-pinned', 'view-pinned', null)).toEqual({ + nextViewId: 'view-pinned', + pendingCreatedViewId: null, + }) + }) + + it('suppresses a redundant URL update while the applied view has no URL selection', () => { + expect(resolveTableViewPinTransition(null, 'view-pinned', 'view-pinned', null)).toEqual({ + nextViewId: null, + pendingCreatedViewId: null, + }) + }) +}) + describe('shouldApplyTableViewRevision', () => { const cached = { id: 'view-1', diff --git a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.ts b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.ts index 0903d6aa4cd..4386ba88912 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.ts +++ b/apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.ts @@ -21,26 +21,33 @@ export function resolveTableViewConfig( } /** - * Resolves the persisted default synchronously when the URL has not selected a - * view yet. The URL effect still records that choice, but render-time consumers - * all see the same owner while that update is pending. + * Resolves a restored embedded view, then the persisted default, while the URL + * has no selection. The URL effect still records that choice, but render-time + * consumers all see the same owner while that update is pending. */ export function resolveTableViewSelection( views: TableViewWire[], - activeViewId: string | null + activeViewId: string | null, + restoredViewId?: string ): TableViewSelection { let selectedView: TableViewWire | null = null let defaultView: TableViewWire | null = null + let restoredView: TableViewWire | null = null for (const view of views) { if (view.id === activeViewId) selectedView = view if (view.isDefault) defaultView = view + if (view.id === restoredViewId) restoredView = view } return { selectedView, defaultView, activeView: selectedView ?? - (activeViewId === null || activeViewId === ALL_VIEW_PARAM ? defaultView : null), + (activeViewId === null + ? (restoredView ?? defaultView) + : activeViewId === ALL_VIEW_PARAM + ? defaultView + : null), } } @@ -49,6 +56,30 @@ export interface TableViewRevision { updatedAt: number | null } +export interface TableViewPinTransition { + nextViewId: string | null + pendingCreatedViewId: string | null +} + +/** + * Resolves an external saved-view pin without leaving a locally created view + * waiting for a URL selection that the pin is about to replace. + */ +export function resolveTableViewPinTransition( + activeViewId: string | null, + appliedViewId: string | null, + pinnedViewId: string, + pendingCreatedViewId: string | null +): TableViewPinTransition { + if (activeViewId === pinnedViewId || (activeViewId === null && appliedViewId === pinnedViewId)) { + return { + nextViewId: null, + pendingCreatedViewId: pendingCreatedViewId === pinnedViewId ? pendingCreatedViewId : null, + } + } + return { nextViewId: pinnedViewId, pendingCreatedViewId: null } +} + export function getTableViewRevision( view: Pick | null ): TableViewRevision { diff --git a/apps/sim/hooks/queries/mothership-chats.ts b/apps/sim/hooks/queries/mothership-chats.ts index 0c05de3ceb0..7dacc9ce14b 100644 --- a/apps/sim/hooks/queries/mothership-chats.ts +++ b/apps/sim/hooks/queries/mothership-chats.ts @@ -138,6 +138,7 @@ function parseResource(value: unknown, context: string): MothershipResource { type: value.type, id: value.id, title: value.title, + ...(typeof value.viewId === 'string' && value.viewId ? { viewId: value.viewId } : {}), } } diff --git a/apps/sim/lib/api/contracts/copilot.ts b/apps/sim/lib/api/contracts/copilot.ts index 307abaeed5f..58f81aead61 100644 --- a/apps/sim/lib/api/contracts/copilot.ts +++ b/apps/sim/lib/api/contracts/copilot.ts @@ -90,15 +90,45 @@ export type CreateWorkflowCopilotChatBody = z.input { + if (resource.viewId === undefined || resource.type === 'table') return + ctx.addIssue({ + code: 'custom', + path: ['viewId'], + message: 'viewId is only valid for table resources', + }) + }) + +export const addCopilotChatResourceBodySchema = z + .object({ + chatId: requiredFieldSchema('chatId cannot be empty'), + resource: copilotChatResourceItemSchema, + clearViewId: z.literal(true).optional(), + }) + .superRefine((body, ctx) => { + if (body.clearViewId !== true) return + if (body.resource.type !== 'table') { + ctx.addIssue({ + code: 'custom', + path: ['clearViewId'], + message: 'clearViewId is only valid for table resources', + }) + } + if (body.resource.viewId !== undefined) { + ctx.addIssue({ + code: 'custom', + path: ['resource', 'viewId'], + message: 'viewId must be omitted when clearViewId is true', + }) + } + }) export type AddCopilotChatResourceBody = z.input export const removeCopilotChatResourceBodySchema = z.object({ @@ -110,13 +140,7 @@ export type RemoveCopilotChatResourceBody = z.input @@ -338,6 +362,7 @@ const copilotChatResourceSchema = z.object({ type: copilotResourceTypeSchema, id: z.string(), title: z.string(), + viewId: z.string().optional(), }) const copilotChatGetChatSchema = z diff --git a/apps/sim/lib/api/contracts/mothership-chats.test.ts b/apps/sim/lib/api/contracts/mothership-chats.test.ts new file mode 100644 index 00000000000..69d4871ee9c --- /dev/null +++ b/apps/sim/lib/api/contracts/mothership-chats.test.ts @@ -0,0 +1,61 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { addMothershipChatResourceBodySchema } from '@/lib/api/contracts/mothership-chats' + +const TABLE_RESOURCE = { + type: 'table' as const, + id: 'table-1', + title: 'Accounts', +} + +describe('addMothershipChatResourceBodySchema', () => { + it('rejects an empty chat id', () => { + expect( + addMothershipChatResourceBodySchema.safeParse({ + chatId: '', + resource: TABLE_RESOURCE, + }).success + ).toBe(false) + }) + + it('preserves an explicit saved-view pin clear through outbound parsing', () => { + expect( + addMothershipChatResourceBodySchema.parse({ + chatId: 'chat-1', + resource: TABLE_RESOURCE, + clearViewId: true, + }) + ).toEqual({ chatId: 'chat-1', resource: TABLE_RESOURCE, clearViewId: true }) + }) + + it('rejects a clear directive for a non-table resource', () => { + expect( + addMothershipChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { type: 'file', id: 'file-1', title: 'Accounts.csv' }, + clearViewId: true, + }).success + ).toBe(false) + }) + + it('rejects a clear directive paired with a replacement pin', () => { + expect( + addMothershipChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { ...TABLE_RESOURCE, viewId: 'view-1' }, + clearViewId: true, + }).success + ).toBe(false) + }) + + it('rejects a saved-view pin for a non-table resource', () => { + expect( + addMothershipChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { type: 'file', id: 'file-1', title: 'Accounts.csv', viewId: 'view-1' }, + }).success + ).toBe(false) + }) +}) diff --git a/apps/sim/lib/api/contracts/mothership-chats.ts b/apps/sim/lib/api/contracts/mothership-chats.ts index 15052b1323e..a46f4f6ac8f 100644 --- a/apps/sim/lib/api/contracts/mothership-chats.ts +++ b/apps/sim/lib/api/contracts/mothership-chats.ts @@ -1,4 +1,5 @@ import { z } from 'zod' +import { addCopilotChatResourceBodySchema } from '@/lib/api/contracts/copilot' import { scheduleContextSchema } from '@/lib/api/contracts/schedules' import { mountedSecretNamesSchema, @@ -201,6 +202,8 @@ const mothershipChatResourceItemSchema = z.object({ type: z.string(), id: z.string(), title: z.string(), + /** Saved view a table tab is pinned to (type "table" only); dropped here, it would be lost on reorder. */ + viewId: z.string().min(1).optional(), }) const mothershipChatResourcesResponseSchema = z.object({ @@ -208,10 +211,8 @@ const mothershipChatResourcesResponseSchema = z.object({ resources: z.array(mothershipChatResourceItemSchema), }) -const addMothershipChatResourceBodySchema = z.object({ - chatId: z.string().min(1), - resource: mothershipChatResourceItemSchema, -}) +export const addMothershipChatResourceBodySchema = addCopilotChatResourceBodySchema +export type AddMothershipChatResourceBody = z.input const reorderMothershipChatResourcesBodySchema = z.object({ chatId: z.string().min(1), diff --git a/apps/sim/lib/copilot/generated/mothership-stream-v1-schema.ts b/apps/sim/lib/copilot/generated/mothership-stream-v1-schema.ts index e7440fb3d0f..ecf8faf3f82 100644 --- a/apps/sim/lib/copilot/generated/mothership-stream-v1-schema.ts +++ b/apps/sim/lib/copilot/generated/mothership-stream-v1-schema.ts @@ -363,6 +363,9 @@ export const MOTHERSHIP_STREAM_V1_SCHEMA: JsonSchema = { MothershipStreamV1ResourceDescriptor: { additionalProperties: false, properties: { + clearViewId: { + type: 'boolean', + }, id: { type: 'string', }, @@ -372,6 +375,9 @@ export const MOTHERSHIP_STREAM_V1_SCHEMA: JsonSchema = { type: { type: 'string', }, + viewId: { + type: 'string', + }, }, required: ['type', 'id'], type: 'object', diff --git a/apps/sim/lib/copilot/generated/mothership-stream-v1.ts b/apps/sim/lib/copilot/generated/mothership-stream-v1.ts index 3b47c736f7e..41a169a8648 100644 --- a/apps/sim/lib/copilot/generated/mothership-stream-v1.ts +++ b/apps/sim/lib/copilot/generated/mothership-stream-v1.ts @@ -279,9 +279,11 @@ export interface MothershipStreamV1ResourceUpsertPayload { resource: MothershipStreamV1ResourceDescriptor } export interface MothershipStreamV1ResourceDescriptor { + clearViewId?: boolean id: string title?: string type: string + viewId?: string } export interface MothershipStreamV1ResourceRemoveEventEnvelope { payload: MothershipStreamV1ResourceRemovePayload diff --git a/apps/sim/lib/copilot/generated/tool-catalog-v1.ts b/apps/sim/lib/copilot/generated/tool-catalog-v1.ts index 0fa9d09d64a..33a29b3fe25 100644 --- a/apps/sim/lib/copilot/generated/tool-catalog-v1.ts +++ b/apps/sim/lib/copilot/generated/tool-catalog-v1.ts @@ -5807,9 +5807,9 @@ export const TableViews: ToolCatalogEntry = { description: 'Arguments for the operation', properties: { filter: { - type: 'object', + type: ['object', 'null'], description: - 'Saved row predicate, same grammar as query_rows filters: {"all":[...]} / {"any":[...]} of {field, op, value} leaves with exact column NAMES. Omit or null for an unfiltered view.', + 'Saved row predicate, same grammar as query_rows filters: {"all":[...]} / {"any":[...]} of {field, op, value} leaves with exact column NAMES. On update_view, omit to keep the existing filter, pass null to clear it, or pass a predicate to replace it. On create_view, omit or pass null for an unfiltered view.', }, hiddenColumns: { type: 'array', @@ -5828,9 +5828,17 @@ export const TableViews: ToolCatalogEntry = { 'View display name (required for create_view; optional rename on update_view). Free-form label; references always use the view ID, so names are purely display.', }, sort: { - type: 'array', + type: ['array', 'null'], description: - 'Saved ordered sort spec, e.g. [{"field":"due","direction":"asc"}], column NAMES. Omit or null for default ordering.', + 'Saved ordered sort spec, e.g. [{"field":"due","direction":"asc"}], column NAMES. On update_view, omit to keep the existing sort, pass null to clear it, or pass a sort spec to replace it. On create_view, omit or pass null for default ordering.', + items: { + type: 'object', + properties: { + direction: { type: 'string', enum: ['asc', 'desc'] }, + field: { type: 'string' }, + }, + required: ['field', 'direction'], + }, }, tableId: { type: 'string', description: 'Table ID (required for every operation)' }, viewId: { diff --git a/apps/sim/lib/copilot/generated/tool-schemas-v1.ts b/apps/sim/lib/copilot/generated/tool-schemas-v1.ts index 25cd935add7..3e2e9030146 100644 --- a/apps/sim/lib/copilot/generated/tool-schemas-v1.ts +++ b/apps/sim/lib/copilot/generated/tool-schemas-v1.ts @@ -5728,9 +5728,9 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { description: 'Arguments for the operation', properties: { filter: { - type: 'object', + type: ['object', 'null'], description: - 'Saved row predicate, same grammar as query_rows filters: {"all":[...]} / {"any":[...]} of {field, op, value} leaves with exact column NAMES. Omit or null for an unfiltered view.', + 'Saved row predicate, same grammar as query_rows filters: {"all":[...]} / {"any":[...]} of {field, op, value} leaves with exact column NAMES. On update_view, omit to keep the existing filter, pass null to clear it, or pass a predicate to replace it. On create_view, omit or pass null for an unfiltered view.', }, hiddenColumns: { type: 'array', @@ -5751,9 +5751,22 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { 'View display name (required for create_view; optional rename on update_view). Free-form label; references always use the view ID, so names are purely display.', }, sort: { - type: 'array', + type: ['array', 'null'], description: - 'Saved ordered sort spec, e.g. [{"field":"due","direction":"asc"}], column NAMES. Omit or null for default ordering.', + 'Saved ordered sort spec, e.g. [{"field":"due","direction":"asc"}], column NAMES. On update_view, omit to keep the existing sort, pass null to clear it, or pass a sort spec to replace it. On create_view, omit or pass null for default ordering.', + items: { + type: 'object', + properties: { + direction: { + type: 'string', + enum: ['asc', 'desc'], + }, + field: { + type: 'string', + }, + }, + required: ['field', 'direction'], + }, }, tableId: { type: 'string', diff --git a/apps/sim/lib/copilot/request/session/contract.test.ts b/apps/sim/lib/copilot/request/session/contract.test.ts index 86dcbc17fb4..60080c4a533 100644 --- a/apps/sim/lib/copilot/request/session/contract.test.ts +++ b/apps/sim/lib/copilot/request/session/contract.test.ts @@ -227,3 +227,52 @@ describe('stream session contract parser', () => { expect(parsed.reason).toBe('invalid_json') }) }) + +describe('resource event view pins', () => { + it('accepts a table resource pinned to a saved view', () => { + const event = { + ...BASE_ENVELOPE, + type: 'resource' as const, + payload: { + op: 'upsert' as const, + resource: { id: 'tbl-1', type: 'table', title: 'Invoices', viewId: 'view-1' }, + }, + } + + expect(isContractStreamEventEnvelope(event)).toBe(true) + expect(parsePersistedStreamEventEnvelope(event).ok).toBe(true) + }) + + it('rejects a pin that is not a string', () => { + const event = { + ...BASE_ENVELOPE, + type: 'resource' as const, + payload: { + op: 'upsert' as const, + resource: { id: 'tbl-1', type: 'table', title: 'Invoices', viewId: 42 }, + }, + } + + expect(isContractStreamEventEnvelope(event)).toBe(false) + }) + + it('accepts an explicit pin clear and rejects a non-boolean directive', () => { + const event = { + ...BASE_ENVELOPE, + type: 'resource' as const, + payload: { + op: 'upsert' as const, + resource: { id: 'tbl-1', type: 'table', title: 'Invoices', clearViewId: true }, + }, + } + + expect(isContractStreamEventEnvelope(event)).toBe(true) + expect(parsePersistedStreamEventEnvelope(event).ok).toBe(true) + expect( + isContractStreamEventEnvelope({ + ...event, + payload: { ...event.payload, resource: { ...event.payload.resource, clearViewId: 'yes' } }, + }) + ).toBe(false) + }) +}) diff --git a/apps/sim/lib/copilot/request/session/contract.ts b/apps/sim/lib/copilot/request/session/contract.ts index bf8c3ea88d6..a0a4fc6474e 100644 --- a/apps/sim/lib/copilot/request/session/contract.ts +++ b/apps/sim/lib/copilot/request/session/contract.ts @@ -273,7 +273,12 @@ function isValidResourcePayload(payload: JsonRecord): boolean { // Dropping a blank id here is the only guard covering both branches // downstream: the handler adds a suppressed file resource to the tab strip // directly, bypassing the checks in `addResource`. - return hasAddressableId(resource.id) && typeof resource.type === 'string' + return ( + hasAddressableId(resource.id) && + typeof resource.type === 'string' && + (resource.viewId === undefined || typeof resource.viewId === 'string') && + (resource.clearViewId === undefined || typeof resource.clearViewId === 'boolean') + ) } function isValidRunPayload(payload: JsonRecord): boolean { diff --git a/apps/sim/lib/copilot/request/tools/resources.test.ts b/apps/sim/lib/copilot/request/tools/resources.test.ts new file mode 100644 index 00000000000..fefe636c49e --- /dev/null +++ b/apps/sim/lib/copilot/request/tools/resources.test.ts @@ -0,0 +1,82 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + extractResourcesFromToolResult: vi.fn(), + persistChatResources: vi.fn(() => Promise.resolve()), + setAttributes: vi.fn(), +})) + +vi.mock('@/lib/copilot/request/otel', () => ({ + withCopilotSpan: ( + _name: string, + _attributes: Record, + run: (span: { setAttributes: typeof mocks.setAttributes }) => Promise + ) => run({ setAttributes: mocks.setAttributes }), +})) + +vi.mock('@/lib/copilot/resources/persistence', () => ({ + extractDeletedResourcesFromToolResult: vi.fn(() => []), + extractResourcesFromToolResult: mocks.extractResourcesFromToolResult, + hasDeleteCapability: vi.fn(() => false), + isResourceToolName: vi.fn(() => true), + persistChatResources: mocks.persistChatResources, + removeChatResources: vi.fn(() => Promise.resolve()), +})) + +import { + MothershipStreamV1EventType, + MothershipStreamV1ResourceOp, +} from '@/lib/copilot/generated/mothership-stream-v1' +import { handleResourceSideEffects } from '@/lib/copilot/request/tools/resources' + +describe('handleResourceSideEffects', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('persists and emits the explicit saved-view pin clear directive', async () => { + mocks.extractResourcesFromToolResult.mockReturnValue([ + { + type: 'table', + id: 'tbl-1', + title: 'Invoices', + clearViewId: true, + }, + ]) + const onEvent = vi.fn() + + await handleResourceSideEffects( + 'table_views', + { operation: 'delete_view', args: { tableId: 'tbl-1', viewId: 'view-1' } }, + { success: true, output: {} }, + { success: true, output: {} }, + 'chat-1', + onEvent, + () => false + ) + + expect(mocks.persistChatResources).toHaveBeenCalledWith('chat-1', [ + { + type: 'table', + id: 'tbl-1', + title: 'Invoices', + clearViewId: true, + }, + ]) + expect(onEvent).toHaveBeenCalledWith({ + type: MothershipStreamV1EventType.resource, + payload: { + op: MothershipStreamV1ResourceOp.upsert, + resource: { + type: 'table', + id: 'tbl-1', + title: 'Invoices', + clearViewId: true, + }, + }, + }) + }) +}) diff --git a/apps/sim/lib/copilot/request/tools/resources.ts b/apps/sim/lib/copilot/request/tools/resources.ts index 88ee1f01a07..0646f444dcb 100644 --- a/apps/sim/lib/copilot/request/tools/resources.ts +++ b/apps/sim/lib/copilot/request/tools/resources.ts @@ -118,6 +118,9 @@ export async function handleResourceSideEffects( ...(projectedResources[index].path !== undefined ? { path: projectedResources[index].path } : {}), + // An id, never secret material — read from the raw result. + ...(resource.viewId !== undefined ? { viewId: resource.viewId } : {}), + ...(resource.clearViewId === true ? { clearViewId: true as const } : {}), })) : [] @@ -141,7 +144,13 @@ export async function handleResourceSideEffects( type: MothershipStreamV1EventType.resource, payload: { op: MothershipStreamV1ResourceOp.upsert, - resource: { type: resource.type, id: resource.id, title: resource.title }, + resource: { + type: resource.type, + id: resource.id, + title: resource.title, + ...(resource.viewId !== undefined ? { viewId: resource.viewId } : {}), + ...(resource.clearViewId === true ? { clearViewId: true } : {}), + }, }, }) } diff --git a/apps/sim/lib/copilot/resources/client-persistence-queue.test.ts b/apps/sim/lib/copilot/resources/client-persistence-queue.test.ts new file mode 100644 index 00000000000..7335ef7006e --- /dev/null +++ b/apps/sim/lib/copilot/resources/client-persistence-queue.test.ts @@ -0,0 +1,312 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { ResourcePersistenceQueue } from '@/lib/copilot/resources/client-persistence-queue' +import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types' + +function deferred() { + let resolve: (value: T) => void = () => {} + let reject: (error: unknown) => void = () => {} + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise + reject = rejectPromise + }) + return { promise, reject, resolve } +} + +const TABLE_RESOURCE: MothershipResourceUpdate = { + type: 'table', + id: 'table-1', + title: 'Accounts', +} + +describe('ResourcePersistenceQueue', () => { + const onError = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + }) + + it('drains a newer update after the write for the same resource settles', async () => { + const first = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockReturnValueOnce(first.promise) + .mockResolvedValueOnce({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1') + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1') + + await Promise.resolve() + expect(persist).toHaveBeenCalledTimes(1) + const flushed = queue.flush('chat-1') + first.resolve({ success: true }) + await flushed + + expect(persist).toHaveBeenCalledTimes(2) + expect(persist.mock.calls[1]).toEqual(['chat-1', { ...TABLE_RESOURCE, viewId: 'view-b' }]) + expect(queue.pendingKeys.size).toBe(0) + expect(queue.inFlight.size).toBe(0) + }) + + it('retains the newest desired state after a failure for a later retry', async () => { + const first = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockReturnValueOnce(first.promise) + .mockResolvedValueOnce({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, clearViewId: true }, 'chat-1', 'chat-1') + queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1') + first.reject(new Error('offline')) + await Promise.allSettled(Array.from(queue.inFlight.values())) + + await queue.flush('chat-1') + + expect(persist.mock.calls[1]).toEqual(['chat-1', { ...TABLE_RESOURCE, clearViewId: true }]) + expect(onError).toHaveBeenCalledOnce() + }) + + it('does not let a removed write settle over a fresh add of the same resource', async () => { + const stale = deferred() + const fresh = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockReturnValueOnce(stale.promise) + .mockReturnValueOnce(fresh.promise) + const remove = vi.fn().mockResolvedValue({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1') + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + removal.scheduleDelete('chat-1', remove) + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1') + await Promise.resolve() + + expect(persist).toHaveBeenCalledTimes(1) + expect(remove).not.toHaveBeenCalled() + stale.resolve({ success: true }) + await vi.waitFor(() => expect(persist).toHaveBeenCalledTimes(2)) + + expect(remove).not.toHaveBeenCalled() + expect(queue.inFlight.size).toBe(1) + fresh.resolve({ success: true }) + await Promise.allSettled(Array.from(queue.inFlight.values())) + expect(queue.inFlight.size).toBe(0) + }) + + it('deletes a stored resource after its pending update fails', async () => { + const failed = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockReturnValueOnce(failed.promise) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE) + failed.reject(new Error('offline')) + await Promise.allSettled(Array.from(queue.inFlight.values())) + + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + expect(removal.wasPending).toBe(true) + expect(removal.wasPersisted).toBe(true) + const remove = vi.fn().mockResolvedValue({ success: true }) + removal.scheduleDelete('chat-1', remove) + await vi.waitFor(() => expect(remove).toHaveBeenCalledOnce()) + }) + + it('skips deletion after an initial add fails', async () => { + const failed = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockReturnValueOnce(failed.promise) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1') + failed.reject(new Error('offline')) + await Promise.allSettled(Array.from(queue.inFlight.values())) + + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + expect(removal.wasPending).toBe(true) + expect(removal.wasPersisted).toBe(false) + }) + + it('persists a re-add after an already-started deletion settles', async () => { + const deletion = deferred() + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValue({ success: true }) + const remove = vi.fn().mockReturnValue(deletion.promise) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE) + await Promise.allSettled(Array.from(queue.inFlight.values())) + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + removal.scheduleDelete('chat-1', remove) + await vi.waitFor(() => expect(remove).toHaveBeenCalledOnce()) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1') + await Promise.resolve() + expect(persist).toHaveBeenCalledOnce() + + deletion.resolve({ success: true }) + await vi.waitFor(() => expect(persist).toHaveBeenCalledTimes(2)) + expect(persist.mock.calls[1]).toEqual(['chat-1', { ...TABLE_RESOURCE, viewId: 'view-b' }]) + }) + + it('retries a failed deletion on the next flush', async () => { + const first = deferred() + const persist = vi.fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + const remove = vi + .fn<() => Promise>() + .mockReturnValueOnce(first.promise) + .mockResolvedValueOnce({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + removal.scheduleDelete('chat-1', remove) + first.reject(new Error('offline')) + await Promise.allSettled(Array.from(queue.inFlight.values())) + + expect( + queue.getPendingResourceKeys('chat-1').has(`${TABLE_RESOURCE.type}:${TABLE_RESOURCE.id}`) + ).toBe(true) + expect(onError).toHaveBeenCalledOnce() + await queue.flush('chat-1') + + expect(remove).toHaveBeenCalledTimes(2) + expect(queue.pendingKeys.size).toBe(0) + }) + + it('keeps failed writes isolated by chat until that chat is flushed again', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockRejectedValueOnce(new Error('offline')) + .mockResolvedValue({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1') + await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce()) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-2', 'chat-2') + await vi.waitFor(() => expect(persist).toHaveBeenCalledTimes(2)) + + expect(persist.mock.calls[1]).toEqual(['chat-2', { ...TABLE_RESOURCE, viewId: 'view-b' }]) + expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set(['table:table-1'])) + expect(queue.getPendingResourceKeys('chat-2')).toEqual(new Set()) + + await queue.flush('chat-1') + + expect(persist.mock.calls[2]).toEqual(['chat-1', { ...TABLE_RESOURCE, viewId: 'view-a' }]) + expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set()) + }) + + it('adopts provisional writes when a new chat receives its durable id', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValue({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, undefined, 'pending-chat-1') + + expect(queue.getPendingResourceKeys('pending-chat-1')).toEqual(new Set(['table:table-1'])) + await queue.flush('chat-1', 'pending-chat-1') + + expect(persist).toHaveBeenCalledWith('chat-1', { ...TABLE_RESOURCE, viewId: 'view-a' }) + expect(queue.getPendingResourceKeys('pending-chat-1')).toEqual(new Set()) + expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set()) + }) + + it('reports a pending identity change while a deletion has not landed', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValue({ success: true }) + const remove = vi.fn<() => Promise>().mockRejectedValueOnce(new Error('offline')) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1') + await Promise.allSettled(queue.getInFlightWrites('chat-1')) + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(false) + + // The server still holds a resource the client has dropped, so an order + // built from client state would not match and must wait for the delete. + const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + removal.scheduleDelete('chat-1', remove) + await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce()) + + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(true) + }) + + it('does not report an identity change for a failing update to a stored resource', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValueOnce({ success: true }) + .mockRejectedValue(new Error('offline')) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1') + await Promise.allSettled(queue.getInFlightWrites('chat-1')) + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(false) + + // A pin update for the same, already-stored resource keeps failing. The + // resource is on the server, so a reorder naming it stays valid. + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1') + await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce()) + expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set(['table:table-1'])) + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(false) + + await queue.flush('chat-1') + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(false) + }) + + it('reports an unpersisted write while a first add has never succeeded', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockRejectedValue(new Error('offline')) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1') + await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce()) + + expect(queue.hasPendingIdentityChanges('chat-1')).toBe(true) + }) + + it('keeps the newer chat-scoped update when a provisional scope is adopted', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValue({ success: true }) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, undefined, 'pending-chat-1') + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, undefined, 'chat-1') + await queue.flush('chat-1', 'pending-chat-1') + + expect(persist).toHaveBeenCalledTimes(1) + expect(persist).toHaveBeenCalledWith('chat-1', { ...TABLE_RESOURCE, viewId: 'view-b' }) + }) + + it('remembers a stored resource until a failed deletion eventually succeeds', async () => { + const persist = vi + .fn<(chatId: string, update: MothershipResourceUpdate) => Promise>() + .mockResolvedValueOnce({ success: true }) + .mockRejectedValueOnce(new Error('re-add failed')) + const remove = vi.fn().mockRejectedValueOnce(new Error('delete failed')) + const queue = new ResourcePersistenceQueue({ persist, onError }) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE) + await Promise.allSettled(queue.getInFlightWrites('chat-1')) + + const firstRemoval = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + firstRemoval.scheduleDelete('chat-1', remove) + await Promise.allSettled(queue.getInFlightWrites('chat-1')) + + queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1') + await Promise.allSettled(queue.getInFlightWrites('chat-1')) + + const secondRemoval = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1') + expect(secondRemoval.wasPending).toBe(true) + expect(secondRemoval.wasPersisted).toBe(true) + }) +}) diff --git a/apps/sim/lib/copilot/resources/client-persistence-queue.ts b/apps/sim/lib/copilot/resources/client-persistence-queue.ts new file mode 100644 index 00000000000..de031f55559 --- /dev/null +++ b/apps/sim/lib/copilot/resources/client-persistence-queue.ts @@ -0,0 +1,283 @@ +import type { MothershipResource, MothershipResourceUpdate } from '@/lib/copilot/resources/types' +import { mergePendingChatResourceUpdate } from '@/lib/copilot/resources/types' + +interface ResourcePersistenceQueueOptions { + persist: (chatId: string, update: MothershipResourceUpdate) => Promise + onError: (error: unknown) => void +} + +const QUEUE_KEY_SEPARATOR = '\u0000' + +export interface RemovedResourcePersistence { + inFlight: Promise | undefined + scheduleDelete: (chatId: string, remove: () => Promise) => void + wasPersisted: boolean + wasPending: boolean +} + +/** + * Serializes writes per resource while allowing unrelated resources to persist + * concurrently. Each key holds the newest desired state until its write + * succeeds, so an update arriving in flight is drained immediately afterward + * and a failed write remains available for the next hydration retry. + */ +export class ResourcePersistenceQueue { + readonly pendingKeys = new Set() + readonly inFlight = new Map>() + + private readonly desiredUpdates = new Map() + private readonly failedKeys = new Set() + private readonly pendingRemovals = new Map Promise>() + private readonly persistedKeys = new Set() + private readonly removalTokens = new Map() + private readonly writeTokens = new Map() + private readonly persist: ResourcePersistenceQueueOptions['persist'] + private readonly onError: ResourcePersistenceQueueOptions['onError'] + + constructor({ persist, onError }: ResourcePersistenceQueueOptions) { + this.persist = persist + this.onError = onError + } + + /** + * Records the newest desired state for a resource and starts writing it when + * a chat id is known. `scopeId` buckets the write: it is the chat id once the + * chat exists, and the provisional pending-chat key before that, which + * {@link flush} later adopts. `base` is the resource as the server already + * holds it, when the caller knows. + */ + enqueue( + update: MothershipResourceUpdate, + chatId: string | undefined, + scopeId: string, + base?: MothershipResource + ): void { + const key = this.getKey(scopeId, update.type, update.id) + const trackedLocally = + this.desiredUpdates.has(key) || this.pendingKeys.has(key) || this.inFlight.has(key) + if (base && !trackedLocally) this.persistedKeys.add(key) + this.pendingRemovals.delete(key) + this.removalTokens.delete(key) + const previous = this.desiredUpdates.get(key) ?? base + this.desiredUpdates.set(key, mergePendingChatResourceUpdate(previous, update)) + this.failedKeys.delete(key) + + if (!chatId || this.inFlight.has(key)) { + this.pendingKeys.add(key) + return + } + + this.start(key, chatId) + } + + async flush(chatId: string, sourceScopeId: string = chatId): Promise { + this.adoptScope(sourceScopeId, chatId) + for (const key of this.getScopedKeys(this.pendingKeys, chatId)) this.failedKeys.delete(key) + this.startPending(chatId) + + while (true) { + const inFlight = this.getInFlightWrites(chatId) + if (inFlight.length === 0) return + await Promise.allSettled(inFlight) + } + } + + remove( + type: string, + id: string, + scopeId: string, + assumePersisted = false + ): RemovedResourcePersistence { + const key = this.getKey(scopeId, type, id) + const trackedLocally = + this.desiredUpdates.has(key) || this.pendingKeys.has(key) || this.inFlight.has(key) + if (assumePersisted && !trackedLocally) this.persistedKeys.add(key) + const wasPending = this.pendingKeys.delete(key) + const inFlight = this.inFlight.get(key) + const wasPersisted = this.persistedKeys.has(key) + const removalToken = Symbol(key) + this.pendingRemovals.delete(key) + this.removalTokens.set(key, removalToken) + this.desiredUpdates.delete(key) + this.failedKeys.delete(key) + return { + inFlight, + scheduleDelete: (chatId, remove) => { + this.pendingRemovals.set(key, remove) + const startRemoval = () => this.startRemoval(key, chatId, removalToken, remove) + if (inFlight) { + void inFlight.then(startRemoval, startRemoval) + return + } + startRemoval() + }, + wasPending, + wasPersisted, + } + } + + /** + * Whether a pending write would change which resources the chat holds — an + * add the server has not accepted yet, or a delete that has not landed. + * + * Only these may hold a reorder back, because only these make the client's + * identity set disagree with the server's, and the server validates a reorder + * against exactly that. A pending UPDATE to a resource it already stores (a + * saved-view pin) must NOT: such a write can fail indefinitely, and gating on + * it would park tab ordering for the rest of the session. + */ + hasPendingIdentityChanges(scopeId: string): boolean { + return this.getScopedKeys(this.pendingKeys, scopeId).some( + (key) => !this.persistedKeys.has(key) || this.pendingRemovals.has(key) + ) + } + + getPendingResourceKeys(scopeId: string): Set { + const prefix = this.getScopePrefix(scopeId) + return new Set( + this.getScopedKeys(this.pendingKeys, scopeId).map((key) => key.slice(prefix.length)) + ) + } + + getInFlightWrites(scopeId: string): Promise[] { + return this.getScopedKeys(this.inFlight, scopeId).flatMap((key) => { + const write = this.inFlight.get(key) + return write ? [write] : [] + }) + } + + private startPending(chatId: string): void { + for (const key of this.getScopedKeys(this.pendingKeys, chatId)) { + if (this.failedKeys.has(key) || this.inFlight.has(key)) continue + const pendingRemoval = this.pendingRemovals.get(key) + const removalToken = this.removalTokens.get(key) + if (pendingRemoval && removalToken) { + this.startRemoval(key, chatId, removalToken, pendingRemoval) + continue + } + this.start(key, chatId) + } + } + + private startRemoval( + key: string, + chatId: string, + removalToken: symbol, + remove: () => Promise + ): void { + if (this.removalTokens.get(key) !== removalToken) return + + this.pendingKeys.delete(key) + let succeeded = false + const writeToken = Symbol(key) + const tracked = Promise.resolve() + .then(async () => { + if (this.removalTokens.get(key) !== removalToken) return + await remove() + succeeded = true + }) + .catch((error) => { + if (this.removalTokens.get(key) !== removalToken) return + this.pendingKeys.add(key) + this.failedKeys.add(key) + this.onError(error) + }) + .finally(() => { + if (this.writeTokens.get(key) !== writeToken) return + this.writeTokens.delete(key) + this.inFlight.delete(key) + if (succeeded && this.removalTokens.get(key) === removalToken) { + this.pendingRemovals.delete(key) + this.removalTokens.delete(key) + this.persistedKeys.delete(key) + } + if (this.removalTokens.get(key) !== removalToken && this.pendingKeys.has(key)) { + this.start(key, chatId) + } + }) + this.writeTokens.set(key, writeToken) + this.inFlight.set(key, tracked) + } + + private start(key: string, chatId: string): void { + const update = this.desiredUpdates.get(key) + if (!update) { + this.pendingKeys.delete(key) + return + } + + this.pendingKeys.delete(key) + let succeeded = false + const token = Symbol(key) + const tracked = Promise.resolve() + .then(() => this.persist(chatId, update)) + .then((result) => { + succeeded = true + this.persistedKeys.add(key) + return result + }) + .catch((error) => { + // Superseded by a newer write for the same key, which owns the retry. + if (this.writeTokens.get(key) !== token) return + // The resource was removed while this write was in flight. Nothing is + // left to retry, and `onError` would report a retry that never comes. + if (!this.desiredUpdates.has(key)) return + this.pendingKeys.add(key) + this.failedKeys.add(key) + this.onError(error) + }) + .finally(() => { + if (this.writeTokens.get(key) !== token) return + this.writeTokens.delete(key) + this.inFlight.delete(key) + if (!succeeded) return + if (this.pendingKeys.has(key)) { + this.start(key, chatId) + return + } + if (this.desiredUpdates.get(key) === update) this.desiredUpdates.delete(key) + }) + this.writeTokens.set(key, token) + this.inFlight.set(key, tracked) + } + + private adoptScope(sourceScopeId: string, targetScopeId: string): void { + if (sourceScopeId === targetScopeId) return + const sourcePrefix = this.getScopePrefix(sourceScopeId) + for (const sourceKey of this.getScopedKeys(this.pendingKeys, sourceScopeId)) { + const resourceKey = sourceKey.slice(sourcePrefix.length) + const targetKey = `${this.getScopePrefix(targetScopeId)}${resourceKey}` + const sourceUpdate = this.desiredUpdates.get(sourceKey) + const targetUpdate = this.desiredUpdates.get(targetKey) + if (sourceUpdate) { + // The source scope is the provisional pre-chat-id bucket, so its update + // is the OLDER of the two: it is `prev`, and the target's is `next`. + this.desiredUpdates.set( + targetKey, + targetUpdate ? mergePendingChatResourceUpdate(sourceUpdate, targetUpdate) : sourceUpdate + ) + } + this.desiredUpdates.delete(sourceKey) + this.pendingKeys.delete(sourceKey) + this.pendingKeys.add(targetKey) + if (this.failedKeys.delete(sourceKey)) this.failedKeys.add(targetKey) + if (this.persistedKeys.delete(sourceKey)) this.persistedKeys.add(targetKey) + } + } + + private getScopedKeys( + collection: ReadonlySet | ReadonlyMap, + scopeId: string + ): string[] { + const prefix = this.getScopePrefix(scopeId) + return Array.from(collection.keys()).filter((key) => key.startsWith(prefix)) + } + + private getScopePrefix(scopeId: string): string { + return `${scopeId}${QUEUE_KEY_SEPARATOR}` + } + + private getKey(scopeId: string, type: string, id: string): string { + return `${this.getScopePrefix(scopeId)}${type}:${id}` + } +} diff --git a/apps/sim/lib/copilot/resources/extraction.test.ts b/apps/sim/lib/copilot/resources/extraction.test.ts index c47413711f2..4072f69d9e1 100644 --- a/apps/sim/lib/copilot/resources/extraction.test.ts +++ b/apps/sim/lib/copilot/resources/extraction.test.ts @@ -194,3 +194,63 @@ describe('extractDeletedResourcesFromToolResult', () => { ).toEqual([{ type: 'knowledgebase', id: 'kb-1', title: 'Docs' }]) }) }) + +describe('extractResourcesFromToolResult for table_views', () => { + const written = { + success: true, + message: 'Created view "Overdue" (view_1)', + data: { + tableId: 'tbl_1', + tableName: 'Invoices', + viewId: 'view_1', + view: { id: 'view_1', name: 'Overdue', isDefault: false, filter: null, sort: null }, + }, + } + + it.each(['create_view', 'update_view', 'set_default_view'])( + '%s opens the table pinned to the view it wrote', + (operation) => { + expect( + extractResourcesFromToolResult( + 'table_views', + { operation, args: { tableId: 'tbl_1' } }, + written + ) + ).toEqual([{ type: 'table', id: 'tbl_1', title: 'Invoices', viewId: 'view_1' }]) + } + ) + + it('a delete opens the table and explicitly clears its saved pin', () => { + expect( + extractResourcesFromToolResult( + 'table_views', + { operation: 'delete_view', args: { tableId: 'tbl_1', viewId: 'view_1' } }, + { + success: true, + message: 'Deleted view "Overdue"', + data: { tableId: 'tbl_1', tableName: 'Invoices' }, + } + ) + ).toEqual([{ type: 'table', id: 'tbl_1', title: 'Invoices', clearViewId: true }]) + }) + + it.each(['list_views', 'get_view'])('%s opens nothing', (operation) => { + expect( + extractResourcesFromToolResult( + 'table_views', + { operation, args: { tableId: 'tbl_1' } }, + written + ) + ).toEqual([]) + }) + + it('falls back to the argument table id when the result names none', () => { + expect( + extractResourcesFromToolResult( + 'table_views', + { operation: 'update_view', args: { tableId: 'tbl_1', viewId: 'view_1' } }, + { success: true, message: 'Updated view' } + ) + ).toEqual([{ type: 'table', id: 'tbl_1', title: 'Table' }]) + }) +}) diff --git a/apps/sim/lib/copilot/resources/extraction.ts b/apps/sim/lib/copilot/resources/extraction.ts index 2f47680dfaf..80b70e241ec 100644 --- a/apps/sim/lib/copilot/resources/extraction.ts +++ b/apps/sim/lib/copilot/resources/extraction.ts @@ -13,15 +13,17 @@ import { PrepareFileEdit, Rm, RunFunction, + TableViews, UserTable, } from '@/lib/copilot/generated/tool-catalog-v1' -import type { MothershipResource, MothershipResourceType } from './types' +import type { MothershipResourceType, MothershipResourceUpdate } from './types' -type ChatResource = MothershipResource +type ChatResource = MothershipResourceUpdate type ResourceType = MothershipResourceType const RESOURCE_TOOL_NAMES: Set = new Set([ UserTable.id, + TableViews.id, CreateEmptyFile.id, PrepareFileEdit.id, DownloadFile.id, @@ -52,6 +54,7 @@ function getWorkspaceFileTarget( } const READ_ONLY_TABLE_OPS = new Set(['get', 'get_schema', 'get_row', 'query_rows']) +const READ_ONLY_VIEW_OPS = new Set(['list_views', 'get_view']) const READ_ONLY_KB_OPS = new Set(['get', 'query', 'list_tags', 'get_tag_usage']) const READ_ONLY_KNOWLEDGE_ACTIONS = new Set(['listed', 'queried']) @@ -196,6 +199,33 @@ export function extractResourcesFromToolResult( return [] } + // The table agent's view tool. A write names the table it touched and — for + // create/update/set-default — the view, so the panel opens the table pinned + // to that view; a delete opens the table unpinned. Reads open nothing. + case TableViews.id: { + const operation = getOperation(params) ?? '' + if (READ_ONLY_VIEW_OPS.has(operation)) return [] + const args = toRecord(params?.args) + const tableId = (data.tableId as string) ?? (args.tableId as string) + if (!tableId) return [] + const viewId = data.viewId + // Pin and unpin are mutually exclusive: the wire contract rejects the + // pair, and a merge handed both would apply neither. A delete unpins + // regardless of any view id its result happens to carry. + return [ + { + type: 'table', + id: tableId, + title: (data.tableName as string) || 'Table', + ...(operation === 'delete_view' + ? { clearViewId: true as const } + : typeof viewId === 'string' && viewId + ? { viewId } + : {}), + }, + ] + } + case Knowledge.id: { const action = data.action as string | undefined if (READ_ONLY_KNOWLEDGE_ACTIONS.has(action ?? '')) return [] diff --git a/apps/sim/lib/copilot/resources/persistence.test.ts b/apps/sim/lib/copilot/resources/persistence.test.ts new file mode 100644 index 00000000000..76266af9fb2 --- /dev/null +++ b/apps/sim/lib/copilot/resources/persistence.test.ts @@ -0,0 +1,64 @@ +/** + * @vitest-environment node + */ +import { databaseMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + persistChatResources, + serializeChatResourceWrite, +} from '@/lib/copilot/resources/persistence' + +function deferred() { + let resolve: () => void = () => {} + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +const transaction = databaseMock.db.transaction as ReturnType +const TABLE_RESOURCE = { type: 'table' as const, id: 'table-1', title: 'Accounts' } + +describe('persistChatResources ordering', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('starts writes for the same chat in invocation order', async () => { + const first = deferred() + transaction.mockReturnValueOnce(first.promise).mockResolvedValueOnce(undefined) + + const firstWrite = persistChatResources('chat-1', [{ ...TABLE_RESOURCE, viewId: 'view-a' }]) + const secondWrite = persistChatResources('chat-1', [{ ...TABLE_RESOURCE, viewId: 'view-b' }]) + await vi.waitFor(() => expect(transaction).toHaveBeenCalledTimes(1)) + first.resolve() + await Promise.all([firstWrite, secondWrite]) + expect(transaction).toHaveBeenCalledTimes(2) + }) + + it('does not serialize writes for different chats', async () => { + const first = deferred() + const second = deferred() + transaction.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) + + const firstWrite = persistChatResources('chat-1', [TABLE_RESOURCE]) + const secondWrite = persistChatResources('chat-2', [TABLE_RESOURCE]) + await vi.waitFor(() => expect(transaction).toHaveBeenCalledTimes(2)) + first.resolve() + second.resolve() + await Promise.all([firstWrite, secondWrite]) + }) + + it('serializes tool writes behind other resource mutations for the same chat', async () => { + const apiMutation = deferred() + const firstWrite = serializeChatResourceWrite('chat-1', () => apiMutation.promise) + const secondWrite = persistChatResources('chat-1', [TABLE_RESOURCE]) + + await Promise.resolve() + expect(transaction).not.toHaveBeenCalled() + apiMutation.resolve() + await Promise.all([firstWrite, secondWrite]) + + expect(transaction).toHaveBeenCalledOnce() + }) +}) diff --git a/apps/sim/lib/copilot/resources/persistence.ts b/apps/sim/lib/copilot/resources/persistence.ts index f4e0e98251b..7788348c479 100644 --- a/apps/sim/lib/copilot/resources/persistence.ts +++ b/apps/sim/lib/copilot/resources/persistence.ts @@ -3,69 +3,116 @@ import { copilotChats } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { eq, sql } from 'drizzle-orm' -import { GENERIC_RESOURCE_TITLES, type MothershipResource, sanitizeChatResources } from './types' +import { + type MothershipResource, + type MothershipResourceUpdate, + mergeChatResource, + sanitizeChatResources, +} from '@/lib/copilot/resources/types' export { extractDeletedResourcesFromToolResult, extractResourcesFromToolResult, hasDeleteCapability, isResourceToolName, -} from './extraction' +} from '@/lib/copilot/resources/extraction' export type { MothershipResource as ChatResource, MothershipResourceType as ResourceType, -} from './types' +} from '@/lib/copilot/resources/types' const logger = createLogger('CopilotResources') type ChatResource = MothershipResource +const CHAT_RESOURCE_STATEMENT_TIMEOUT_MS = 10_000 +const CHAT_RESOURCE_LOCK_TIMEOUT_MS = 3_000 +const CHAT_RESOURCE_IDLE_TIMEOUT_MS = 5_000 + +/** + * Bounds the `copilot_chats` row-lock wait every resource writer takes. + * + * {@link serializeChatResourceWrite} only serializes writers inside one process. + * Another pod's write — and `finalizeAssistantTurn`, which holds this same row + * `FOR UPDATE` across an assistant-message append — are outside it. Without + * `lock_timeout` a waiter inherits the full statement clock, which the + * deployment does not set either, so one stuck holder can drain the pool. + * + * Safe under pgBouncer transaction pooling: `SET LOCAL` is transaction-scoped + * and clears at COMMIT/ROLLBACK before the session returns to the pool. + */ +export async function setChatResourceTxTimeouts(trx: Pick): Promise { + await trx.execute( + sql.raw(`SET LOCAL statement_timeout = '${CHAT_RESOURCE_STATEMENT_TIMEOUT_MS}ms'`) + ) + await trx.execute(sql.raw(`SET LOCAL lock_timeout = '${CHAT_RESOURCE_LOCK_TIMEOUT_MS}ms'`)) + await trx.execute( + sql.raw(`SET LOCAL idle_in_transaction_session_timeout = '${CHAT_RESOURCE_IDLE_TIMEOUT_MS}ms'`) + ) +} + +const chatResourceWriteChain = new Map>() + +export async function serializeChatResourceWrite( + chatId: string, + write: () => Promise +): Promise { + const tail = chatResourceWriteChain.get(chatId) ?? Promise.resolve() + const run = tail.catch(() => {}).then(write) + chatResourceWriteChain.set(chatId, run) + try { + return await run + } finally { + if (chatResourceWriteChain.get(chatId) === run) chatResourceWriteChain.delete(chatId) + } +} + /** * Appends resources to a chat's JSONB resources column, deduplicating by type+id. * Updates the title of existing resources if the new title is more specific. */ export async function persistChatResources( chatId: string, - newResources: ChatResource[] + newResources: MothershipResourceUpdate[] ): Promise { const toMerge = newResources.filter((r) => r.id !== 'streaming-file') if (toMerge.length === 0) return try { - const [chat] = await db - .select({ resources: copilotChats.resources }) - .from(copilotChats) - .where(eq(copilotChats.id, chatId)) - .limit(1) - - if (!chat) return - - const existing = sanitizeChatResources( - Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] - ) - const map = new Map() - - for (const r of existing) { - map.set(`${r.type}:${r.id}`, r) - } - - for (const r of sanitizeChatResources(toMerge)) { - const key = `${r.type}:${r.id}` - const prev = map.get(key) - if ( - !prev || - (GENERIC_RESOURCE_TITLES.has(prev.title) && !GENERIC_RESOURCE_TITLES.has(r.title)) - ) { - map.set(key, r) - } - } - - const merged = Array.from(map.values()) - - await db - .update(copilotChats) - .set({ resources: sql`${JSON.stringify(merged)}::jsonb` }) - .where(eq(copilotChats.id, chatId)) + await serializeChatResourceWrite(chatId, async () => { + await db.transaction(async (tx) => { + await setChatResourceTxTimeouts(tx) + const [chat] = await tx + .select({ resources: copilotChats.resources }) + .from(copilotChats) + .where(eq(copilotChats.id, chatId)) + .for('update') + .limit(1) + + if (!chat) return + + const existing = sanitizeChatResources( + Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] + ) + const map = new Map() + + for (const r of existing) { + map.set(`${r.type}:${r.id}`, r) + } + + for (const r of sanitizeChatResources(toMerge)) { + const key = `${r.type}:${r.id}` + map.set(key, mergeChatResource(map.get(key), r)) + } + + const merged = Array.from(map.values()) + + await tx + .update(copilotChats) + .set({ resources: sql`${JSON.stringify(merged)}::jsonb` }) + .where(eq(copilotChats.id, chatId)) + }) + }) } catch (err) { logger.warn('Failed to persist chat resources', { chatId, @@ -81,27 +128,33 @@ export async function removeChatResources(chatId: string, toRemove: ChatResource if (toRemove.length === 0) return try { - const [chat] = await db - .select({ resources: copilotChats.resources }) - .from(copilotChats) - .where(eq(copilotChats.id, chatId)) - .limit(1) - - if (!chat) return - - const stored = Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] - const existing = sanitizeChatResources(stored) - const removeKeys = new Set(sanitizeChatResources(toRemove).map((r) => `${r.type}:${r.id}`)) - const filtered = existing.filter((r) => !removeKeys.has(`${r.type}:${r.id}`)) - - const removedSomething = filtered.length !== existing.length - const sanitizedSomething = existing.length !== stored.length - if (!removedSomething && !sanitizedSomething) return - - await db - .update(copilotChats) - .set({ resources: sql`${JSON.stringify(filtered)}::jsonb` }) - .where(eq(copilotChats.id, chatId)) + await serializeChatResourceWrite(chatId, async () => { + await db.transaction(async (tx) => { + await setChatResourceTxTimeouts(tx) + const [chat] = await tx + .select({ resources: copilotChats.resources }) + .from(copilotChats) + .where(eq(copilotChats.id, chatId)) + .for('update') + .limit(1) + + if (!chat) return + + const stored = Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : [] + const existing = sanitizeChatResources(stored) + const removeKeys = new Set(sanitizeChatResources(toRemove).map((r) => `${r.type}:${r.id}`)) + const filtered = existing.filter((r) => !removeKeys.has(`${r.type}:${r.id}`)) + + const removedSomething = filtered.length !== existing.length + const sanitizedSomething = existing.length !== stored.length + if (!removedSomething && !sanitizedSomething) return + + await tx + .update(copilotChats) + .set({ resources: sql`${JSON.stringify(filtered)}::jsonb` }) + .where(eq(copilotChats.id, chatId)) + }) + }) } catch (err) { logger.warn('Failed to remove chat resources', { chatId, diff --git a/apps/sim/lib/copilot/resources/types.test.ts b/apps/sim/lib/copilot/resources/types.test.ts index fa142c19298..7510ad5d36a 100644 --- a/apps/sim/lib/copilot/resources/types.test.ts +++ b/apps/sim/lib/copilot/resources/types.test.ts @@ -8,7 +8,10 @@ import { isEphemeralResource, type MothershipResource, MothershipResourceType, + mergeChatResource, + mergePendingChatResourceUpdate, PERSISTED_RESOURCE_TYPES, + reorderStoredChatResources, sanitizeChatResources, TERMINAL_SESSION_RESOURCE_ID, } from './types' @@ -110,6 +113,30 @@ describe('client and server agree on what can be persisted', () => { } }) + it('accepts an explicit table pin clear and rejects ambiguous or non-table clears', () => { + expect( + addCopilotChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { type: 'table', id: 'tbl-1', title: 'Invoices' }, + clearViewId: true, + }).success + ).toBe(true) + expect( + addCopilotChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { type: 'table', id: 'tbl-1', title: 'Invoices', viewId: 'view-1' }, + clearViewId: true, + }).success + ).toBe(false) + expect( + addCopilotChatResourceBodySchema.safeParse({ + chatId: 'chat-1', + resource: { type: 'file', id: 'file-1', title: 'report.csv' }, + clearViewId: true, + }).success + ).toBe(false) + }) + it('covers every resource type, so a new one has to make the choice explicitly', () => { const all = Object.values(MothershipResourceType) const ephemeral = all.filter((type) => isEphemeralResource(resource({ type }))) @@ -153,3 +180,122 @@ describe('unaddressable resources', () => { expect(parsed.success).toBe(false) }) }) + +describe('mergeChatResource', () => { + const stored = resource({ type: 'table', id: 'tbl-1', title: 'Invoices' }) + + it('adds a resource the chat does not have yet as a copy', () => { + const added = mergeChatResource(undefined, stored) + expect(added).toEqual(stored) + // Copied, not aliased: the result is handed to React state, the query cache + // and the pending-write queue, and the caller keeps mutating its own object. + expect(added).not.toBe(stored) + }) + + it('keeps the stored entry when the newcomer changes nothing', () => { + expect(mergeChatResource(stored, { ...stored })).toBe(stored) + }) + + it('replaces a placeholder title but never a specific one', () => { + const placeholder = resource({ type: 'table', id: 'tbl-1', title: 'Table' }) + expect(mergeChatResource(placeholder, stored).title).toBe('Invoices') + expect(mergeChatResource(stored, placeholder).title).toBe('Invoices') + }) + + it('moves the pin to the view the agent touched last and keeps it across unpinned re-adds', () => { + const pinnedA = mergeChatResource(stored, { ...stored, viewId: 'view-a' }) + expect(pinnedA.viewId).toBe('view-a') + + const pinnedB = mergeChatResource(pinnedA, { ...stored, viewId: 'view-b' }) + expect(pinnedB.viewId).toBe('view-b') + + // A row edit re-adds the table without a view — the tab stays on view-b. + expect(mergeChatResource(pinnedB, stored)).toBe(pinnedB) + }) + + it('clears a pin only when the update carries the explicit clear directive', () => { + const pinned = { ...stored, viewId: 'view-a' } + + expect(mergeChatResource(pinned, { ...stored, clearViewId: true })).toEqual(stored) + expect(mergeChatResource(undefined, { ...stored, clearViewId: true })).toEqual(stored) + }) +}) + +describe('mergeChatResource metadata', () => { + it('takes the metadata a newcomer defines and keeps what it omits', () => { + const placeholder = resource({ type: 'file', id: 'f1', title: 'File' }) + const upgraded = mergeChatResource(placeholder, { + type: 'file', + id: 'f1', + title: 'notes.md', + path: 'files/notes.md', + }) + expect(upgraded).toEqual({ type: 'file', id: 'f1', title: 'notes.md', path: 'files/notes.md' }) + + // A later re-add without a path keeps the stored one. + expect(mergeChatResource(upgraded, { type: 'file', id: 'f1', title: 'notes.md' })).toBe( + upgraded + ) + + const log = resource({ type: 'log', id: 'row-1', title: 'Run' }) + expect( + mergeChatResource(log, { type: 'log', id: 'row-1', title: 'Run', executionId: 'exec-1' }) + .executionId + ).toBe('exec-1') + }) +}) + +describe('mergePendingChatResourceUpdate', () => { + const table = resource({ type: 'table', id: 'tbl-1', title: 'Invoices' }) + + it('retains a pending clear across an unrelated update', () => { + expect(mergePendingChatResourceUpdate({ ...table, clearViewId: true }, table)).toEqual({ + ...table, + clearViewId: true, + }) + }) + + it('lets a newer explicit pin replace a pending clear', () => { + expect( + mergePendingChatResourceUpdate( + { ...table, clearViewId: true }, + { ...table, viewId: 'view-new' } + ) + ).toEqual({ ...table, viewId: 'view-new' }) + }) +}) + +describe('reorderStoredChatResources', () => { + const table = resource({ + type: 'table', + id: 'tbl-1', + title: 'Invoices', + viewId: 'view-new', + }) + const file = resource({ id: 'file-1', title: 'report.csv', path: 'files/report.csv' }) + + it('uses the request only for order and preserves newer stored metadata', () => { + expect( + reorderStoredChatResources( + [table, file], + [ + { ...file, path: 'stale/report.csv' }, + { ...table, viewId: 'view-stale' }, + ] + ) + ).toEqual([file, table]) + }) + + it('rejects missing, extra, and duplicate identities', () => { + expect(reorderStoredChatResources([table, file], [table])).toBeNull() + expect(reorderStoredChatResources([table], [table, file])).toBeNull() + expect(reorderStoredChatResources([table, file], [table, table])).toBeNull() + }) + + it('collapses a duplicated stored row instead of rejecting the reorder', () => { + // Nothing writes a duplicate today, but a chat stored before the writers + // merged by key can hold one. The client sends its deduplicated list, so a + // length comparison would reject every reorder for that chat forever. + expect(reorderStoredChatResources([table, table, file], [file, table])).toEqual([file, table]) + }) +}) diff --git a/apps/sim/lib/copilot/resources/types.ts b/apps/sim/lib/copilot/resources/types.ts index 58d68d6e737..b437f686901 100644 --- a/apps/sim/lib/copilot/resources/types.ts +++ b/apps/sim/lib/copilot/resources/types.ts @@ -30,6 +30,12 @@ export interface MothershipResource { executionId?: string } +/** A resource upsert may explicitly clear metadata that omission preserves. */ +export interface MothershipResourceUpdate extends MothershipResource { + /** Removes a table's saved-view pin instead of preserving it. */ + clearViewId?: true +} + /** * What a chip in an assistant message knows about the resource it points at, * before it has been resolved. The agent writes these tags as text, so a file @@ -202,6 +208,40 @@ export function sanitizeChatResources( return canonicalizeDesktopSessionResources(resources).filter(isAddressableResource) } +/** + * Applies a client-supplied order to the canonical stored entries. Reordering + * carries identity only: metadata echoed by a stale tab must never overwrite a + * newer pin, path, title, or execution id already persisted on the chat. + */ +export function reorderStoredChatResources( + storedResources: readonly MothershipResource[], + requestedOrder: readonly MothershipResource[] +): MothershipResource[] | null { + const stored = sanitizeChatResources(storedResources) + const requested = sanitizeChatResources(requestedOrder) + + // Compared as key SETS, not lengths: a chat that already holds a duplicated + // row (nothing writes one today, but stored data predates the merge-by-key + // writers) sends one fewer entry from the deduplicated client. Matching on + // sets keeps that reorder valid and collapses the duplicate on write, where + // a length check would reject every reorder for that chat forever. + const storedByKey = new Map( + stored.map((resource) => [`${resource.type}:${resource.id}`, resource]) + ) + const requestedKeys = Array.from( + new Set(requested.map((resource) => `${resource.type}:${resource.id}`)) + ) + if (requestedKeys.length !== storedByKey.size) return null + + const reordered: MothershipResource[] = [] + for (const key of requestedKeys) { + const resource = storedByKey.get(key) + if (!resource) return null + reordered.push(resource) + } + return reordered +} + /** Placeholder resource titles that a more specific title may overwrite during dedup. */ export const GENERIC_RESOURCE_TITLES = new Set([ 'Table', @@ -212,6 +252,78 @@ export const GENERIC_RESOURCE_TITLES = new Set([ 'Log', ]) +/** + * Every field {@link mergeChatResource} carries over from the newcomer. `type` + * and `id` identify the entry and can never differ; `title` has its own + * placeholder rule. Declared once so a field added to {@link MothershipResource} + * fails to compile here rather than being silently dropped from both the merge + * and its no-op check. + */ +const MERGED_FIELDS = { + title: true, + path: true, + viewId: true, + executionId: true, +} as const satisfies Record, true> + +const MERGED_FIELD_NAMES = Object.keys(MERGED_FIELDS) as (keyof typeof MERGED_FIELDS)[] + +/** + * Folds a re-added resource into the stored entry with the same type+id. The + * stored title wins unless it was a placeholder. Every other field the + * newcomer defines replaces the stored one — a file's `path`, a log's + * `executionId`, a table's saved-view pin (the tab reopens on the view the + * agent touched last) — while a field the newcomer omits is kept, so an + * unrelated row edit never unpins a table. Returns `prev` itself when nothing + * changes, so callers can skip a no-op write. + */ +export function mergeChatResource( + prev: MothershipResource | undefined, + next: MothershipResourceUpdate +): MothershipResource { + if (!prev) { + // Copied, never aliased: the result lands in React state, the query cache + // and the pending-write queue at once, and `next` is the caller's object. + const { clearViewId: _clearViewId, ...resource } = next + return resource + } + const { viewId: _previousViewId, ...prevWithoutViewId } = prev + const merged: MothershipResource = { + ...(next.clearViewId === true ? prevWithoutViewId : prev), + ...(next.path !== undefined ? { path: next.path } : {}), + ...(next.clearViewId !== true && next.viewId !== undefined ? { viewId: next.viewId } : {}), + ...(next.executionId !== undefined ? { executionId: next.executionId } : {}), + title: + GENERIC_RESOURCE_TITLES.has(prev.title) && !GENERIC_RESOURCE_TITLES.has(next.title) + ? next.title + : prev.title, + } + const unchanged = MERGED_FIELD_NAMES.every((field) => merged[field] === prev[field]) + return unchanged ? prev : merged +} + +/** + * Coalesces durable updates that have not all reached the server yet. Unlike a + * stored resource, the pending value must retain an explicit pin-clear until a + * write succeeds; a later row edit that omits `viewId` must not cancel it. + */ +export function mergePendingChatResourceUpdate( + prev: MothershipResourceUpdate | undefined, + next: MothershipResourceUpdate +): MothershipResourceUpdate { + let previousClearViewId: true | undefined + let previousResource: MothershipResource | undefined + if (prev) { + const { clearViewId, ...resource } = prev + previousClearViewId = clearViewId + previousResource = resource + } + const merged = mergeChatResource(previousResource, next) + const shouldClearViewId = + next.viewId === undefined && (next.clearViewId === true || previousClearViewId === true) + return shouldClearViewId ? { ...merged, clearViewId: true } : merged +} + export const VFS_DIR_TO_RESOURCE: Record = { tables: 'table', files: 'file', diff --git a/apps/sim/lib/copilot/tool-executor/types.ts b/apps/sim/lib/copilot/tool-executor/types.ts index 47db6aa0d95..3bfcfd87da7 100644 --- a/apps/sim/lib/copilot/tool-executor/types.ts +++ b/apps/sim/lib/copilot/tool-executor/types.ts @@ -1,5 +1,5 @@ import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution' -import type { MothershipResource } from '@/lib/copilot/resources/types' +import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types' import type { SecretMountPolicy } from '@/lib/copilot/secret-mount-policy' import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' @@ -84,7 +84,7 @@ export interface ToolExecutionResult { success: boolean output?: unknown error?: string - resources?: MothershipResource[] + resources?: MothershipResourceUpdate[] /** * Declared by tools whose failure a caller cannot otherwise act on. Consumed by * the egress projection and never returned to the model as-is — on a withheld diff --git a/apps/sim/lib/copilot/tools/server/table/table-views.test.ts b/apps/sim/lib/copilot/tools/server/table/table-views.test.ts index 56d4df7e797..89579a4cc0d 100644 --- a/apps/sim/lib/copilot/tools/server/table/table-views.test.ts +++ b/apps/sim/lib/copilot/tools/server/table/table-views.test.ts @@ -26,6 +26,7 @@ vi.mock('@/lib/copilot/application/execute-table-use-case', () => ({ })) import { tableViewsServerTool } from '@/lib/copilot/tools/server/table/table-views' +import { asOrchestrationError } from '@/lib/core/orchestration/types' const context = { userId: 'user-1', workspaceId: 'ws-1', copilotToolExecution: true } as never @@ -33,7 +34,7 @@ const columns = [ { id: 'col_a', name: 'status', type: 'string' }, { id: 'col_b', name: 'due', type: 'date' }, ] -const table = { id: 'tbl-1', schema: { columns } } +const table = { id: 'tbl-1', name: 'Invoices', schema: { columns } } describe('table_views adapter', () => { beforeEach(() => { @@ -91,13 +92,57 @@ describe('table_views adapter', () => { expect(createInput.config.filter).toEqual({ all: [{ field: 'col_a', op: 'eq', value: 'Open' }], }) + expect(createInput).not.toHaveProperty('isDefault') + // What resource extraction reads to open the panel on the new view. + expect(result.data).toMatchObject({ tableId: 'tbl-1', tableName: 'Invoices', viewId: 'view-2' }) + }) + + it('makes the view default inside the same create, with no follow-up write', async () => { + executeUseCase.mockResolvedValueOnce({ table, views: [] }).mockResolvedValueOnce({ + view: { id: 'view-2', name: 'Mine', isDefault: true, config: {} }, + table, + }) + + const result = await tableViewsServerTool.execute( + { operation: 'create_view', args: { tableId: 'tbl-1', name: 'Mine', isDefault: true } }, + context + ) + + expect(executeUseCase).toHaveBeenCalledTimes(2) + expect(executeUseCase.mock.calls[1][2]).toMatchObject({ isDefault: true }) + expect(result.message).toContain('as default') + expect(result.data.view.isDefault).toBe(true) + }) + + it('names the table and view on update, and only the table on delete', async () => { + const stored = { id: 'view-1', name: 'Overdue', isDefault: false, config: {} } + executeUseCase.mockResolvedValueOnce({ table, views: [stored] }).mockResolvedValueOnce({ + view: { ...stored, name: 'Late' }, + table, + }) + const updated = await tableViewsServerTool.execute( + { operation: 'update_view', args: { tableId: 'tbl-1', viewId: 'view-1', name: 'Late' } }, + context + ) + expect(updated.data).toMatchObject({ + tableId: 'tbl-1', + tableName: 'Invoices', + viewId: 'view-1', + }) + + executeUseCase.mockResolvedValueOnce({ viewId: 'view-1', viewName: 'Late', table }) + const deleted = await tableViewsServerTool.execute( + { operation: 'delete_view', args: { tableId: 'tbl-1', viewId: 'view-1' } }, + context + ) + expect(deleted.data).toEqual({ tableId: 'tbl-1', tableName: 'Invoices' }) }) it('rejects unknown column names with the columns spelled out', async () => { executeUseCase.mockResolvedValueOnce({ table, views: [] }) - await expect( - tableViewsServerTool.execute( + const failure = await tableViewsServerTool + .execute( { operation: 'create_view', args: { @@ -108,7 +153,13 @@ describe('table_views adapter', () => { }, context ) - ).rejects.toThrow(/Unknown column/) + .catch((error: unknown) => error) + + // Classified as the caller's mistake, so the model sees the column name + // instead of a masked system error. + expect(asOrchestrationError(failure)?.code).toBe('validation') + expect(asOrchestrationError(failure)?.message).toMatch(/Unknown column/) + expect(executeUseCase).toHaveBeenCalledTimes(1) }) it('rejects unsupported operations without invoking anything', async () => { diff --git a/apps/sim/lib/copilot/tools/server/table/table-views.ts b/apps/sim/lib/copilot/tools/server/table/table-views.ts index 7b004bb1b99..6872f55d4b3 100644 --- a/apps/sim/lib/copilot/tools/server/table/table-views.ts +++ b/apps/sim/lib/copilot/tools/server/table/table-views.ts @@ -1,6 +1,7 @@ import { executeCopilotTableUseCase } from '@/lib/copilot/application/execute-table-use-case' import { TableViews } from '@/lib/copilot/generated/tool-catalog-v1' import type { BaseServerTool, ServerToolContext } from '@/lib/copilot/tools/server/base-tool' +import { OrchestrationError } from '@/lib/core/orchestration/types' import type { SortSpec, TablePredicateInput, TableSchema, TableViewConfig } from '@/lib/table' import { createTableViewUseCase, @@ -9,7 +10,11 @@ import { readTableViewUseCase, updateTableViewUseCase, } from '@/lib/table/application/views' -import { viewConfigIdsToNames, viewConfigNamesToIds } from '@/lib/table/views/service' +import { + TableViewValidationError, + viewConfigIdsToNames, + viewConfigNamesToIds, +} from '@/lib/table/views/service' type TableViewsArgs = { operation: string @@ -22,12 +27,16 @@ type TableViewsResult = { data?: any } +type StoredView = { id: string; name: string; isDefault: boolean; config: TableViewConfig } + /** * Saved-view slice of the split table surface. Unlike the other slices this is * NOT a user_table passthrough — it adapts the dedicated view use cases. * Agents speak column NAMES; stored configs are keyed by stable column id, so * inputs translate names→ids on the way in and every returned view translates - * ids→names on the way out. + * ids→names on the way out. Every write also names the table and the view it + * touched in `data`; resource extraction reads that to open the panel on the + * view that was just written. */ export const tableViewsServerTool: BaseServerTool = { name: TableViews.id, @@ -39,10 +48,7 @@ export const tableViewsServerTool: BaseServerTool { + const presentView = (view: StoredView, columns: TableSchema['columns']) => { const named = viewConfigIdsToNames(view.config, columns) return { id: view.id, @@ -54,16 +60,38 @@ export const tableViewsServerTool: BaseServerTool ({ + tableId: table.id, + tableName: table.name, + viewId: view.id, + view: presentView(view, columns), + }) + // Build the patch from only the keys the caller actually sent: the update // path shallow-merges this into the stored config, so including an absent // part as `null` silently wiped a view's saved sort when only the filter - // changed (and vice versa) — the doc promises "omit to keep". + // changed (and vice versa) — the doc promises "omit to keep, null to clear". + // The name→id translation runs here, outside the use case that would + // classify a bad column name, so it is classified here: unclassified, the + // model gets a masked "system error" instead of the column it got wrong. const namedConfigFromArgs = (columns: TableSchema['columns']): TableViewConfig => { const patch: Record = {} if (args.filter !== undefined) patch.filter = args.filter as TablePredicateInput | null if (args.sort !== undefined) patch.sort = args.sort as SortSpec | null if (args.hiddenColumns !== undefined) patch.hiddenColumns = args.hiddenColumns as string[] - return viewConfigNamesToIds(patch as TableViewConfig, columns) + try { + return viewConfigNamesToIds(patch as TableViewConfig, columns) + } catch (error) { + if (error instanceof TableViewValidationError) { + throw new OrchestrationError('validation', error.message) + } + throw error + } } switch (operation) { @@ -106,26 +134,24 @@ export const tableViewsServerTool: BaseServerTool { } ) + it('createTableView with isDefault demotes the current default in the same transaction', async () => { + queueTableRows(tableViews, [{ total: 2 }]) + dbChainMockFns.returning.mockResolvedValueOnce([{ ...viewRow, isDefault: true }]) + + await createTableView({ + tableId: 'table-1', + workspaceId: 'ws-1', + name: 'My View', + config: {}, + userId: 'user-1', + columns, + isDefault: true, + }) + + expect(dbChainMockFns.set).toHaveBeenCalledWith({ + isDefault: false, + updatedAt: expect.any(Date), + }) + expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ isDefault: true })) + }) + + it('createTableView without isDefault never demotes, even on a first view (which is default anyway)', async () => { + queueTableRows(tableViews, [{ total: 0 }]) + dbChainMockFns.returning.mockResolvedValueOnce([{ ...viewRow, isDefault: true }]) + + await createTableView({ + tableId: 'table-1', + workspaceId: 'ws-1', + name: 'My View', + config: {}, + userId: 'user-1', + columns, + isDefault: false, + }) + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ isDefault: true })) + }) + it('updateTableView signals when the target view exists', async () => { queueTableRows(tableViews, [{ id: 'view-1' }]) // the in-transaction existence pre-check dbChainMockFns.returning.mockResolvedValueOnce([viewRow]) // the update returning @@ -694,3 +733,52 @@ describe('view config column-reference normalization', () => { ).toEqual([{ field: 'createdAt', direction: 'desc' }]) }) }) + +describe('default-view writers share the views lock', () => { + const columns: ColumnDefinition[] = [] + const viewRow = { + id: 'view-1', + tableId: 'table-1', + workspaceId: 'ws-1', + name: 'My View', + config: {}, + isDefault: false, + createdBy: 'user-1', + createdAt: new Date('2026-01-01T00:00:00.000Z'), + updatedAt: new Date('2026-01-01T00:00:00.000Z'), + } + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + it('promoting a view takes the per-table advisory lock the create path holds', async () => { + queueTableRows(tableViews, [{ id: 'view-1' }]) + dbChainMockFns.returning.mockResolvedValueOnce([{ ...viewRow, isDefault: true }]) + + await updateTableView({ viewId: 'view-1', tableId: 'table-1', isDefault: true, columns }) + + // withTableViewsLock issues its SET LOCAL timeouts and the advisory lock + // through execute; the plain-transaction path never calls it. + expect(dbChainMockFns.execute).toHaveBeenCalled() + }) + + it('demoting a view takes the same advisory lock as other default-state writers', async () => { + queueTableRows(tableViews, [{ ...viewRow, isDefault: true }]) + dbChainMockFns.returning.mockResolvedValueOnce([{ ...viewRow, isDefault: false }]) + + await updateTableView({ viewId: 'view-1', tableId: 'table-1', isDefault: false, columns }) + + expect(dbChainMockFns.execute).toHaveBeenCalled() + }) + + it('a rename stays a plain transaction, off the lock', async () => { + queueTableRows(tableViews, [{ id: 'view-1' }]) + dbChainMockFns.returning.mockResolvedValueOnce([{ ...viewRow, name: 'Renamed' }]) + + await updateTableView({ viewId: 'view-1', tableId: 'table-1', name: 'Renamed', columns }) + + expect(dbChainMockFns.execute).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/views/service.ts b/apps/sim/lib/table/views/service.ts index dfa4f339b15..e538454adae 100644 --- a/apps/sim/lib/table/views/service.ts +++ b/apps/sim/lib/table/views/service.ts @@ -418,6 +418,12 @@ export interface CreateTableViewData { config: TableViewConfig userId: string columns: ColumnDefinition[] + /** + * Make the new view the table's default, demoting the previous default in the + * same transaction. The first view on a table is the default regardless — a + * table that has views always keeps one. + */ + isDefault?: boolean /** * Whether to refuse a filter, sort, or column-layout reference naming no live * column. Set by the `/api/v2` surface only, whose caller authored the config @@ -471,6 +477,21 @@ export async function createTableView(data: CreateTableViewData): Promise 0) { + await trx + .update(tableViews) + .set({ isDefault: false, updatedAt: new Date() }) + .where( + and( + eq(tableViews.tableId, data.tableId), + eq(tableViews.workspaceId, data.workspaceId), + eq(tableViews.isDefault, true) + ) + ) + } + const [created] = await trx .insert(tableViews) .values({ @@ -479,7 +500,7 @@ export async function createTableView(data: CreateTableViewData): Promise { - const outcome = await db.transaction(async (tx) => { + const runWrite = (write: (trx: DbTransaction) => Promise): Promise => + data.isDefault !== undefined ? withTableViewsLock(data.tableId, write) : db.transaction(write) + const outcome = await runWrite(async (tx) => { // Confirm the target exists BEFORE demoting. The demotion has to run first — // the partial unique index rejects a second default — but on a PATCH naming a // missing view the target update matches nothing, so without this the demote diff --git a/apps/sim/stores/table/view-pin/store.test.ts b/apps/sim/stores/table/view-pin/store.test.ts new file mode 100644 index 00000000000..c78b32e4c88 --- /dev/null +++ b/apps/sim/stores/table/view-pin/store.test.ts @@ -0,0 +1,73 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it } from 'vitest' +import { useTableViewPinStore } from '@/stores/table/view-pin/store' +import { resetRegisteredUserData } from '@/stores/user-data-reset-registry' + +describe('useTableViewPinStore', () => { + beforeEach(() => { + useTableViewPinStore.getState().reset() + }) + + it('keeps one pending pin per table, the latest winning', () => { + const { pin } = useTableViewPinStore.getState() + pin('tbl-1', 'view-a') + pin('tbl-1', 'view-b') + pin('tbl-2', 'view-c') + + const { pins } = useTableViewPinStore.getState() + expect(pins['tbl-1'].viewId).toBe('view-b') + expect(pins['tbl-2'].viewId).toBe('view-c') + }) + + it('re-pinning the same view is a new request, so a re-edit after the user moved on still switches', () => { + const { pin } = useTableViewPinStore.getState() + pin('tbl-1', 'view-a') + const first = useTableViewPinStore.getState().pins['tbl-1'] + pin('tbl-1', 'view-a') + const second = useTableViewPinStore.getState().pins['tbl-1'] + + expect(second.viewId).toBe(first.viewId) + expect(second.seq).toBeGreaterThan(first.seq) + }) + + it('consume clears only the pin it was handed, never a newer one', () => { + const { pin, consume } = useTableViewPinStore.getState() + pin('tbl-1', 'view-a') + const stale = useTableViewPinStore.getState().pins['tbl-1'] + pin('tbl-1', 'view-b') + + consume('tbl-1', stale.seq) + expect(useTableViewPinStore.getState().pins['tbl-1'].viewId).toBe('view-b') + + consume('tbl-1', useTableViewPinStore.getState().pins['tbl-1'].seq) + expect(useTableViewPinStore.getState().pins['tbl-1']).toBeUndefined() + }) + + it('consuming a table with no pin is a no-op', () => { + const before = useTableViewPinStore.getState().pins + useTableViewPinStore.getState().consume('tbl-none', 1) + expect(useTableViewPinStore.getState().pins).toBe(before) + }) + + it('clear removes a pending pin and leaves other tables alone', () => { + const { pin, clear } = useTableViewPinStore.getState() + pin('tbl-1', 'view-a') + pin('tbl-2', 'view-b') + + clear('tbl-1') + + expect(useTableViewPinStore.getState().pins['tbl-1']).toBeUndefined() + expect(useTableViewPinStore.getState().pins['tbl-2'].viewId).toBe('view-b') + }) + + it('clears pending pins when the authenticated identity changes', () => { + useTableViewPinStore.getState().pin('tbl-1', 'view-a') + + resetRegisteredUserData() + + expect(useTableViewPinStore.getState().pins).toEqual({}) + expect(useTableViewPinStore.getState().nextSeq).toBe(1) + }) +}) diff --git a/apps/sim/stores/table/view-pin/store.ts b/apps/sim/stores/table/view-pin/store.ts new file mode 100644 index 00000000000..75b489c8745 --- /dev/null +++ b/apps/sim/stores/table/view-pin/store.ts @@ -0,0 +1,66 @@ +import { create } from 'zustand' +import { devtools } from 'zustand/middleware' +import { registerUserDataReset } from '@/stores/user-data-reset-registry' + +/** A request that the table switch to one of its saved views. */ +export interface TableViewPin { + viewId: string + /** Distinguishes a repeat pin of the same view — a re-edit after the user moved on — from one already honoured. */ + seq: number +} + +interface TableViewPinState { + /** Pending pins keyed by table id. */ + pins: Record + nextSeq: number + /** Asks the table to open on `viewId`; replaces any pin still pending for it. */ + pin: (tableId: string, viewId: string) => void + /** Clears any pending pin after the referenced view is deleted. */ + clear: (tableId: string) => void + /** Clears a pin the table has applied. A newer pin (higher seq) issued meanwhile is kept. */ + consume: (tableId: string, seq: number) => void + reset: () => void +} + +const initialState = { pins: {} as Record, nextSeq: 1 } + +/** + * Bridges the agent's saved-view work to the embedded table. A view the agent + * just created or edited arrives on the resource stream before the table's + * views query has refetched, so the switch can't be a plain URL write — the + * table would treat the not-yet-listed id as dead and fall back to its default. + * The pin waits here until the table (mounted now or later) sees the view in + * its list, applies it, and consumes the pin. + * + * Ephemeral — no persistence. Reopening a chat restores a pin from the stored + * resource's `viewId` instead. + */ +export const useTableViewPinStore = create()( + devtools( + (set) => ({ + ...initialState, + pin: (tableId, viewId) => + set((state) => ({ + pins: { ...state.pins, [tableId]: { viewId, seq: state.nextSeq } }, + nextSeq: state.nextSeq + 1, + })), + clear: (tableId) => + set((state) => { + if (!state.pins[tableId]) return state + const { [tableId]: _cleared, ...pins } = state.pins + return { pins } + }), + consume: (tableId, seq) => + set((state) => { + const pending = state.pins[tableId] + if (!pending || pending.seq !== seq) return state + const { [tableId]: _consumed, ...pins } = state.pins + return { pins } + }), + reset: () => set(initialState), + }), + { name: 'table-view-pin-store' } + ) +) + +registerUserDataReset('table-view-pin', () => useTableViewPinStore.getState().reset())