From 42cee278c8b24a055121dafdde1a53ff04d00757 Mon Sep 17 00:00:00 2001 From: Waleed Date: Mon, 17 Aug 2026 18:12:09 -0700 Subject: [PATCH] fix(forking): stop a parent re-pick blanking a dependent's stored target value (#6787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(forking): stop a parent re-pick blanking a dependent's stored target value A dependent selector (a sheet under a spreadsheet, a label under a mailbox) is invalidated when its parent is re-picked, because the stored child no longer exists under the new parent. That invalidation was recorded by writing an empty string into the in-session override map — the same value the user's own "clear this field" produces. The two are not the same thing, and the map is submitted verbatim and written into the target workflow's configuration, so an invalidated field cleared the target's real stored value. The sharpest case is an undo. Re-pick a parent away from its original target, then back. The parent nets out unchanged, so nothing is remapped and the remap's own clearing pass never runs — but the child is still blank, and that blank lands on a value the user never touched, with nothing in the UI showing it happened. Record the invalidation with a distinct marker instead. It reads as blank in the selector, the in-block chain context, and the Sync gate, so a required invalidated field still blocks Sync and still renders; but it is omitted from the submitted payload rather than sent as empty, so no override is written and the target keeps what it had. A blank the user picked themselves is still submitted and still clears the target. Also: skip the cascade entirely when a re-pick selects the value the field already had, since the selector fires its change handler either way. Fork file copy: a file whose name is already taken in a reused target folder is de-duplicated with the same allocator the ordinary upload path uses, rather than colliding with the folder-name unique index and being dropped from the fork with its blob deleted. Adds hook-level coverage for the submitted payload, which had none. * fix(forking): preserve dependent-chain semantics * fix(forking): preserve edits during fork sync * fix(workflows): clear stale dependent inputs in Mothership edits --------- Co-authored-by: Vikhyath Mondreti --- .../connector-selector-field.tsx | 2 +- .../[id]/hooks/use-connector-config-fields.ts | 2 +- .../components/combobox/combobox.tsx | 2 +- .../components/dropdown/dropdown.tsx | 2 +- .../components/tool-input/param-dependents.ts | 6 +- .../workflow-search-replace.tsx | 9 +- .../workflow-block/workflow-block.tsx | 2 +- apps/sim/blocks/utils.test.ts | 93 --- apps/sim/blocks/utils.ts | 34 -- .../fork-sync/dependent-value.test.ts | 183 +++++- .../components/fork-sync/dependent-value.ts | 149 ++++- .../components/fork-sync/fork-sync-view.tsx | 28 +- .../fork-sync/use-fork-sync.test.tsx | 558 ++++++++++++++++++ .../components/fork-sync/use-fork-sync.ts | 32 +- .../lib/copy/copy-files.test.ts | 350 ++++++++++- .../workspace-forking/lib/copy/copy-files.ts | 60 +- .../lib/mapping/dependent-reconfigs.ts | 8 +- .../lib/remap/remap-references.test.ts | 50 ++ .../lib/remap/remap-references.ts | 13 +- apps/sim/hooks/use-collaborative-workflow.ts | 2 +- .../workflow/edit-workflow/operations.test.ts | 256 ++++++-- .../workflow/edit-workflow/operations.ts | 50 +- .../search-replace/dependencies.test.ts | 24 - .../workflows/search-replace/dependencies.ts | 33 -- .../lib/workflows/search-replace/indexer.ts | 4 +- .../workflows/subblocks/dependencies.test.ts | 137 +++++ .../lib/workflows/subblocks/dependencies.ts | 66 +++ 27 files changed, 1842 insertions(+), 313 deletions(-) create mode 100644 apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.test.tsx delete mode 100644 apps/sim/lib/workflows/search-replace/dependencies.test.ts delete mode 100644 apps/sim/lib/workflows/search-replace/dependencies.ts create mode 100644 apps/sim/lib/workflows/subblocks/dependencies.test.ts create mode 100644 apps/sim/lib/workflows/subblocks/dependencies.ts diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-selector-field/connector-selector-field.tsx b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-selector-field/connector-selector-field.tsx index 6cdd14d27e..4252b6ec35 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-selector-field/connector-selector-field.tsx +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-selector-field/connector-selector-field.tsx @@ -5,11 +5,11 @@ import { ChipCombobox, type ComboboxOption } from '@sim/emcn' import { Loader } from '@sim/emcn/icons' import { SEARCH_DEBOUNCE_MS } from '@/lib/url-state' import { SELECTOR_CONTEXT_FIELDS } from '@/lib/workflows/subblocks/context' +import { getDependsOnFields } from '@/lib/workflows/subblocks/dependencies' import type { ConfigFieldMap, ConfigFieldValue, } from '@/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-config-fields' -import { getDependsOnFields } from '@/blocks/utils' import type { ConnectorConfigField } from '@/connectors/types' import { getSelectorDefinition } from '@/hooks/selectors/registry' import type { SelectorContext, SelectorKey } from '@/hooks/selectors/types' diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-config-fields.ts b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-config-fields.ts index 955891811d..6972354899 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-config-fields.ts +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-config-fields.ts @@ -1,7 +1,7 @@ 'use client' import { useCallback, useMemo, useState } from 'react' -import { getDependsOnFields } from '@/blocks/utils' +import { getDependsOnFields } from '@/lib/workflows/subblocks/dependencies' import type { ConnectorConfigField, ConnectorMeta } from '@/connectors/types' export type ConfigFieldValue = string | string[] diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/combobox/combobox.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/combobox/combobox.tsx index bea443e2f7..f8ebccf435 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/combobox/combobox.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/combobox/combobox.tsx @@ -2,6 +2,7 @@ import { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react' import { Combobox, type ComboboxOption, cn } from '@sim/emcn' import { Plus } from '@sim/emcn/icons' import { useReactFlow } from 'reactflow' +import { getDependsOnFields } from '@/lib/workflows/subblocks/dependencies' import { SandboxCreateModal } from '@/app/workspace/[workspaceId]/settings/components/sandboxes/components/sandbox-create-modal' import type { SandboxLanguage } from '@/app/workspace/[workspaceId]/settings/components/sandboxes/utils' import { shouldClearMissingOption } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/combobox/missing-option' @@ -13,7 +14,6 @@ import { useSubBlockValue } from '@/app/workspace/[workspaceId]/w/[workflowId]/c import { useActiveSearchTarget } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/providers/active-search-target-provider' import { useAccessibleReferencePrefixes } from '@/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-accessible-reference-prefixes' import type { SubBlockConfig } from '@/blocks/types' -import { getDependsOnFields } from '@/blocks/utils' import { usePermissionConfig } from '@/hooks/use-permission-config' import { useSubBlockStore } from '@/stores/workflows/subblock/store' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx index 996fbf7f3f..b14dfc42c4 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/dropdown/dropdown.tsx @@ -6,6 +6,7 @@ import { NO_DENIED_OPERATIONS, OPERATION_SUBBLOCK_ID, } from '@/lib/permission-groups/operation-access' +import { getDependsOnFields } from '@/lib/workflows/subblocks/dependencies' import { formatDisplayText } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/formatted-text' import { getWorkflowSearchLabelHighlight } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/workflow-search-highlight' import { useFetchedOptions } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/hooks/use-fetched-options' @@ -13,7 +14,6 @@ import { useSubBlockValue } from '@/app/workspace/[workspaceId]/w/[workflowId]/c import { useActiveSearchTarget } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/providers/active-search-target-provider' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' -import { getDependsOnFields } from '@/blocks/utils' import { ResponseBlockHandler } from '@/executor/handlers/response/response-handler' import { useOperationAccess } from '@/hooks/use-operation-access' import { useWorkflowStore } from '@/stores/workflows/workflow/store' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/param-dependents.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/param-dependents.ts index a860066f29..77e44462cf 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/param-dependents.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/param-dependents.ts @@ -1,10 +1,10 @@ -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' +import { getTransitiveSubBlockDependents } from '@/lib/workflows/subblocks/dependencies' import { getBlock } from '@/blocks/registry' /** * Clear every TRANSITIVE `dependsOn` descendant of `changedParamId` in a nested tool's params, * mirroring the top-level block clear (`use-collaborative-workflow`). Reuses the shared - * {@link getWorkflowSearchDependentClears} walk - transitive BFS plus canonical-pair expansion, so a + * {@link getTransitiveSubBlockDependents} walk - transitive BFS plus canonical-pair expansion, so a * basic OR advanced member change clears the dependent - so both surfaces clear identically. Only * descendants that currently hold a non-empty value are reset to `''`; the changed param itself and * non-descendants are untouched. Returns the same reference when nothing changed. @@ -16,7 +16,7 @@ export function clearDependentToolParams( ): Record { const subBlocks = getBlock(toolType)?.subBlocks ?? [] let next: Record | null = null - for (const { subBlockId } of getWorkflowSearchDependentClears(subBlocks, changedParamId)) { + for (const { subBlockId } of getTransitiveSubBlockDependents(subBlocks, [changedParamId])) { if (!params[subBlockId]) continue next ??= { ...params } next[subBlockId] = '' diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/search-replace/workflow-search-replace.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/search-replace/workflow-search-replace.tsx index be31940d6a..90d773f793 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/search-replace/workflow-search-replace.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/search-replace/workflow-search-replace.tsx @@ -5,7 +5,6 @@ import { Button, cn, Input, toast } from '@sim/emcn' import { ChevronDown, ChevronRight, ChevronUp, X } from '@sim/emcn/icons' import { useParams } from 'next/navigation' import { useShallow } from 'zustand/react/shallow' -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' import { indexWorkflowSearchMatches } from '@/lib/workflows/search-replace/indexer' import { buildWorkflowSearchReplacePlan } from '@/lib/workflows/search-replace/replacements' import { @@ -20,6 +19,7 @@ import { import { getWorkflowSearchBlocks } from '@/lib/workflows/search-replace/state' import { WORKFLOW_SEARCH_SUBFLOW_FIELD_IDS } from '@/lib/workflows/search-replace/subflow-fields' import type { WorkflowSearchReplaceSubflowUpdate } from '@/lib/workflows/search-replace/types' +import { getTransitiveSubBlockDependents } from '@/lib/workflows/subblocks/dependencies' import { useRegisterGlobalCommands } from '@/app/workspace/[workspaceId]/providers/global-commands-provider' import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider' import { createCommand } from '@/app/workspace/[workspaceId]/utils/commands-utils' @@ -474,10 +474,9 @@ export function WorkflowSearchReplace() { const blockConfig = block ? getBlock(block.type) : null if (!blockConfig?.subBlocks) continue - const dependentClears = getWorkflowSearchDependentClears( - blockConfig.subBlocks, - update.subBlockId - ) + const dependentClears = getTransitiveSubBlockDependents(blockConfig.subBlocks, [ + update.subBlockId, + ]) for (const clear of dependentClears) { const alreadyUpdated = batchUpdates.some( (candidate) => diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx index 3babe49ba7..bc9714ea20 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx @@ -50,6 +50,7 @@ import { import { resolveSelectedTriggerId } from '@/lib/workflows/blocks/canvas-trigger-sentence' import { calculateWorkflowBlockDimensions } from '@/lib/workflows/blocks/deterministic-dimensions' import { getConditionRows, getRouterRows } from '@/lib/workflows/dynamic-handle-topology' +import { getDependsOnFields } from '@/lib/workflows/subblocks/dependencies' import { getDisplayValue, hasDisplayableRowValue, @@ -96,7 +97,6 @@ import { SELECTOR_TYPES_HYDRATION_REQUIRED, type SubBlockConfig, } from '@/blocks/types' -import { getDependsOnFields } from '@/blocks/utils' import { useKnowledgeBase } from '@/hooks/kb/use-knowledge' import { useCustomTools } from '@/hooks/queries/custom-tools' import { useDeployWorkflow } from '@/hooks/queries/deployments' diff --git a/apps/sim/blocks/utils.test.ts b/apps/sim/blocks/utils.test.ts index fd71606f4a..1cf444caa0 100644 --- a/apps/sim/blocks/utils.test.ts +++ b/apps/sim/blocks/utils.test.ts @@ -67,13 +67,10 @@ vi.mock('@/lib/oauth/utils', () => ({ getScopesForService: vi.fn(() => []), })) -import type { SubBlockConfig } from '@/blocks/types' import { BUILT_IN_TOOL_TYPES, getApiKeyCondition, - getDependsOnFields, getSerializedModelProviderId, - getSubBlocksDependingOnChange, parseOptionalBooleanInput, parseOptionalJsonInput, parseOptionalNumberInput, @@ -377,96 +374,6 @@ describe('parseOptionalBooleanInput', () => { }) }) -describe('getDependsOnFields', () => { - it('returns an empty array when dependsOn is unset', () => { - expect(getDependsOnFields(undefined)).toEqual([]) - }) - - it('returns array dependencies unchanged', () => { - expect(getDependsOnFields(['credential', 'projectId'])).toEqual(['credential', 'projectId']) - }) - - it('flattens all and any dependencies', () => { - expect(getDependsOnFields({ all: ['credential'], any: ['teamId', 'manualTeamId'] })).toEqual([ - 'credential', - 'teamId', - 'manualTeamId', - ]) - }) -}) - -describe('getSubBlocksDependingOnChange', () => { - it('finds direct dependents of a changed subblock', () => { - const subBlocks: SubBlockConfig[] = [ - { id: 'provider', title: 'Provider', type: 'dropdown' }, - { id: 'model', title: 'Model', type: 'dropdown', dependsOn: ['provider'] }, - { id: 'prompt', title: 'Prompt', type: 'long-input' }, - ] - - expect( - getSubBlocksDependingOnChange(subBlocks, 'provider').map((subBlock) => subBlock.id) - ).toEqual(['model']) - }) - - it('matches dependents through canonical basic and advanced siblings', () => { - const subBlocks: SubBlockConfig[] = [ - { - id: 'channel', - title: 'Channel', - type: 'channel-selector', - canonicalParamId: 'channelId', - mode: 'basic', - }, - { - id: 'manualChannel', - title: 'Channel ID', - type: 'short-input', - canonicalParamId: 'channelId', - mode: 'advanced', - }, - { - id: 'messageId', - title: 'Message ID', - type: 'short-input', - dependsOn: ['channelId'], - }, - { - id: 'threadTs', - title: 'Thread Timestamp', - type: 'short-input', - dependsOn: ['otherField'], - }, - ] - - expect( - getSubBlocksDependingOnChange(subBlocks, 'manualChannel').map((subBlock) => subBlock.id) - ).toEqual(['messageId']) - expect( - getSubBlocksDependingOnChange(subBlocks, 'channel').map((subBlock) => subBlock.id) - ).toEqual(['messageId']) - }) - - it('matches object-form dependencies when any listed dependency changes', () => { - const subBlocks: SubBlockConfig[] = [ - { id: 'credential', title: 'Credential', type: 'oauth-input' }, - { id: 'teamId', title: 'Team', type: 'short-input' }, - { - id: 'projectId', - title: 'Project', - type: 'short-input', - dependsOn: { all: ['credential'], any: ['teamId'] }, - }, - ] - - expect( - getSubBlocksDependingOnChange(subBlocks, 'credential').map((subBlock) => subBlock.id) - ).toEqual(['projectId']) - expect( - getSubBlocksDependingOnChange(subBlocks, 'teamId').map((subBlock) => subBlock.id) - ).toEqual(['projectId']) - }) -}) - describe('getSerializedModelProviderId', () => { const resolver = vi.mocked(getProviderFromModel) diff --git a/apps/sim/blocks/utils.ts b/apps/sim/blocks/utils.ts index 909e42a01f..60e4efad52 100644 --- a/apps/sim/blocks/utils.ts +++ b/apps/sim/blocks/utils.ts @@ -8,7 +8,6 @@ import { } from '@/lib/core/config/env-flags' import { getScopesForService } from '@/lib/oauth/utils' import { containsReference } from '@/lib/workflows/sanitization/references' -import { buildCanonicalIndex } from '@/lib/workflows/subblocks/visibility' import type { SubBlockConfig } from '@/blocks/types' import { getBaseModelProviders, @@ -112,39 +111,6 @@ export function getPiModelOptions() { }) } -/** - * Gets all dependency fields as a flat array. - * Handles both simple array format and object format with all/any fields. - */ -export function getDependsOnFields(dependsOn: SubBlockConfig['dependsOn']): string[] { - if (!dependsOn) return [] - if (Array.isArray(dependsOn)) return dependsOn - return [...(dependsOn.all || []), ...(dependsOn.any || [])] -} - -/** - * Finds subblocks that depend on a changed field, accounting for canonical pairs. - */ -export function getSubBlocksDependingOnChange( - allSubBlocks: SubBlockConfig[], - changedSubBlockId: string -): SubBlockConfig[] { - const canonicalIndex = buildCanonicalIndex(allSubBlocks) - const canonicalId = canonicalIndex.canonicalIdBySubBlockId[changedSubBlockId] - const group = canonicalId ? canonicalIndex.groupsById[canonicalId] : undefined - const changedFields = new Set([changedSubBlockId]) - - if (canonicalId) changedFields.add(canonicalId) - if (group?.basicId) changedFields.add(group.basicId) - for (const advancedId of group?.advancedIds || []) { - changedFields.add(advancedId) - } - - return allSubBlocks.filter((subBlock) => - getDependsOnFields(subBlock.dependsOn).some((field) => changedFields.has(field)) - ) -} - function getProviderFromStore(model: string): string | null { const { providers } = useProvidersStore.getState() const normalized = model.toLowerCase() diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.test.ts b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.test.ts index 20a1c079ab..077175f1bd 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.test.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.test.ts @@ -11,6 +11,7 @@ import { getActionableDependentFields, getDisplayedDependentFields, isDependentConfigurationActionable, + isDependentInvalidated, } from '@/ee/workspace-forking/components/fork-sync/dependent-value' const field = (overrides: Partial = {}): ForkDependentReconfig => ({ @@ -31,6 +32,11 @@ const field = (overrides: Partial = {}): ForkDependentRec ...overrides, }) +const mappedRepickContext = (previousValue: string) => ({ + previousValue, + baselineValueFor: (dependent: ForkDependentReconfig) => dependent.currentValue, +}) + describe('dependentKey', () => { it('keys by target workflow + block + subblock', () => { expect( @@ -138,20 +144,45 @@ describe('applyDependentRepick', () => { previous, site, [site, drive, spreadsheet, sheet, unrelated], - 'site-new' + 'site-new', + mappedRepickContext('site-old') ) expect(next).toEqual({ [dependentKey(site)]: 'site-new', - [dependentKey(drive)]: '', - [dependentKey(spreadsheet)]: '', - [dependentKey(sheet)]: '', + [dependentKey(drive)]: null, + [dependentKey(spreadsheet)]: null, + [dependentKey(sheet)]: null, [dependentKey(unrelated)]: 'still-keep-me', }) expect(effectiveDependentValue(drive, next, false)).toBe('') expect(effectiveCopyDependentValue(sheet, next)).toBe('') }) + it('re-picking the value the field already had leaves its descendants alone', () => { + const spreadsheet = field({ + subBlockKey: 'spreadsheetId', + currentValue: 'sheet-doc', + providesContextKey: 'spreadsheetId', + }) + const range = field({ + subBlockKey: 'range', + currentValue: 'Sheet1!A1:D', + consumesContextKeys: ['spreadsheetId'], + }) + + const next = applyDependentRepick( + {}, + spreadsheet, + [spreadsheet, range], + 'sheet-doc', + mappedRepickContext(effectiveDependentValue(spreadsheet, {}, false)) + ) + + expect(next).toEqual({}) + expect(effectiveDependentValue(range, next, false)).toBe('Sheet1!A1:D') + }) + it('only changes the selected field when it provides no selector context', () => { const leaf = field({ subBlockKey: 'issueKey', currentValue: 'ISSUE-1' }) const unrelated = field({ subBlockKey: 'label', currentValue: 'keep-me' }) @@ -161,7 +192,8 @@ describe('applyDependentRepick', () => { { [dependentKey(unrelated)]: 'still-keep-me' }, leaf, [leaf, unrelated], - 'ISSUE-2' + 'ISSUE-2', + mappedRepickContext('ISSUE-1') ) ).toEqual({ [dependentKey(leaf)]: 'ISSUE-2', @@ -200,14 +232,106 @@ describe('applyDependentRepick', () => { previous, projectOne, [projectOne, issueOne, projectTwo, issueTwo], - 'P1-NEW' + 'P1-NEW', + mappedRepickContext('INBOX') ) ).toEqual({ [dependentKey(projectOne)]: 'P1-NEW', - [dependentKey(issueOne)]: '', + [dependentKey(issueOne)]: null, [dependentKey(issueTwo)]: 'P2-1', }) }) + + it('restores the stored chain when a provider is changed and then returned to baseline', () => { + const spreadsheet = field({ + subBlockKey: 'spreadsheetId', + currentValue: 'doc-old', + providesContextKey: 'spreadsheetId', + }) + const range = field({ + subBlockKey: 'range', + currentValue: 'A1:D50', + consumesContextKeys: ['spreadsheetId'], + }) + + const changed = applyDependentRepick( + {}, + spreadsheet, + [spreadsheet, range], + 'doc-new', + mappedRepickContext('doc-old') + ) + const restored = applyDependentRepick( + changed, + spreadsheet, + [spreadsheet, range], + 'doc-old', + mappedRepickContext('doc-new') + ) + + expect(changed).toEqual({ + [dependentKey(spreadsheet)]: 'doc-new', + [dependentKey(range)]: null, + }) + expect(restored).toEqual({}) + expect(effectiveDependentValue(range, restored, false)).toBe('A1:D50') + }) + + it('keeps an intentional empty pick distinct from automatic invalidation', () => { + const label = field({ subBlockKey: 'label', currentValue: 'INBOX' }) + + const next = applyDependentRepick({}, label, [label], '', mappedRepickContext('INBOX')) + + expect(isDependentInvalidated(label, next)).toBe(false) + expect(next[dependentKey(label)]).toBe('') + expect(effectiveDependentValue(label, next, false)).toBe('') + }) + + it('does not restore an Excel sheet while its drive still differs from baseline', () => { + const drive = field({ + subBlockKey: 'driveId', + currentValue: 'drive-old', + providesContextKey: 'driveId', + }) + const spreadsheet = field({ + subBlockKey: 'spreadsheetId', + currentValue: 'workbook-old', + consumesContextKeys: ['driveId'], + providesContextKey: 'spreadsheetId', + }) + const sheet = field({ + subBlockKey: 'sheetName', + currentValue: 'Sheet1', + consumesContextKeys: ['driveId', 'spreadsheetId'], + }) + + const driveChanged = applyDependentRepick( + {}, + drive, + [drive, spreadsheet, sheet], + 'drive-new', + mappedRepickContext('drive-old') + ) + const spreadsheetRepicked = applyDependentRepick( + driveChanged, + spreadsheet, + [drive, spreadsheet, sheet], + 'workbook-new', + mappedRepickContext('') + ) + const spreadsheetRestored = applyDependentRepick( + spreadsheetRepicked, + spreadsheet, + [drive, spreadsheet, sheet], + 'workbook-old', + mappedRepickContext('workbook-new') + ) + + expect(spreadsheetRestored).toEqual({ + [dependentKey(drive)]: 'drive-new', + [dependentKey(sheet)]: null, + }) + }) }) describe('isDependentConfigurationActionable', () => { @@ -281,6 +405,51 @@ describe('isDependentConfigurationActionable', () => { ).toBe(true) }) + it('shows a required field that a parent re-pick blanked (it blocks Sync)', () => { + const spreadsheet = field({ + subBlockKey: 'spreadsheetId', + currentValue: 'doc-old', + providesContextKey: 'spreadsheetId', + }) + const sheet = field({ + subBlockKey: 'sheetName', + required: true, + currentValue: 'Sheet1', + consumesContextKeys: ['spreadsheetId'], + }) + const next = applyDependentRepick( + {}, + spreadsheet, + [spreadsheet, sheet], + 'doc-new', + mappedRepickContext(effectiveDependentValue(spreadsheet, {}, false)) + ) + + // The sync gate reads the same blank the selector shows, so the field gates and is visible. + expect(effectiveDependentValue(sheet, next, false)).toBe('') + expect( + isDependentConfigurationActionable(sheet, next, { + parentResolved: true, + parentChanged: false, + copying: false, + }) + ).toBe(true) + }) + + it('shows a required field the user emptied themselves (it blocks Sync)', () => { + const sheet = field({ subBlockKey: 'sheetName', required: true, currentValue: 'Sheet1' }) + const next = applyDependentRepick({}, sheet, [sheet], '', mappedRepickContext('Sheet1')) + + expect(effectiveDependentValue(sheet, next, false)).toBe('') + expect( + isDependentConfigurationActionable(sheet, next, { + parentResolved: true, + parentChanged: false, + copying: false, + }) + ).toBe(true) + }) + it('hides dependents until their parent is resolved', () => { expect( isDependentConfigurationActionable( diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts index 9ae7ab40d8..f23111dc4e 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts @@ -1,5 +1,8 @@ import type { ForkDependentReconfig } from '@/lib/api/contracts/workspace-fork' +/** In-session dependent values. `null` means an upstream selector invalidated the field. */ +export type DependentReconfigState = Record + /** Stable key for a per-target dependent re-pick (target workflow + block + subblock). */ export function dependentKey(dependent: ForkDependentReconfig): string { return `${dependent.targetWorkflowId}:${dependent.targetBlockId}:${dependent.subBlockKey}` @@ -10,22 +13,62 @@ function sameDependencyScope(left: ForkDependentReconfig, right: ForkDependentRe } /** - * Store a dependent re-pick and clear every selector transitively scoped by it. Empty-string - * overrides are intentional: an absent override means "fall back to the stored value", while a - * changed provider makes every stored descendant stale for both mapped and copied parents. + * Index the selector fields that provide each dependency context within a top-level block or + * nested tool instance. The fork diff has already normalized canonical basic/advanced fields + * into these context keys, so this is the same graph the selectors use to resolve their options. + */ +function indexContextProviders( + fields: ForkDependentReconfig[] +): Map> { + const providersByScope = new Map>() + for (const field of fields) { + if (!field.providesContextKey) continue + let providers = providersByScope.get(field.dependencyScope) + if (!providers) { + providers = new Map() + providersByScope.set(field.dependencyScope, providers) + } + providers.set(field.providesContextKey, field) + } + return providersByScope +} + +interface DependentRepickContext { + /** Effective value displayed immediately before this selection. */ + previousValue: string + /** Value each field falls back to when it has no in-session override. */ + baselineValueFor: (field: ForkDependentReconfig) => string +} + +/** + * Apply one selector choice and invalidate its transitive descendants within the same dependency + * scope. Automatic invalidation is represented by `null`, not `''`, so it can be distinguished + * from a user's intentional clear while the editor is open. + * + * Returning a provider to its baseline restores every descendant whose complete provider chain + * has also returned to baseline. A descendant remains invalidated when another one of its + * providers is still changed. This makes A -> B -> A a true undo without reviving a value under + * a genuinely different scope. */ export function applyDependentRepick( - reconfig: Record, + reconfig: DependentReconfigState, changedField: ForkDependentReconfig, blockFields: ForkDependentReconfig[], - value: string -): Record { + value: string, + context: DependentRepickContext +): DependentReconfigState { const changedKey = dependentKey(changedField) - const nextState = { ...reconfig, [changedKey]: value } + const baselineValue = context.baselineValueFor(changedField) + const nextState = { ...reconfig } + if (value === baselineValue) delete nextState[changedKey] + else nextState[changedKey] = value + + if (context.previousValue === value) return nextState if (!changedField.providesContextKey) return nextState const pendingContextKeys = [changedField.providesContextKey] const visitedFields = new Set([changedKey]) + const descendants: ForkDependentReconfig[] = [] for (let index = 0; index < pendingContextKeys.length; index += 1) { const contextKey = pendingContextKeys[index] if (!contextKey) continue @@ -41,26 +84,71 @@ export function applyDependentRepick( } visitedFields.add(fieldKey) - nextState[fieldKey] = '' + descendants.push(field) if (field.providesContextKey) pendingContextKeys.push(field.providesContextKey) } } + for (const field of descendants) nextState[dependentKey(field)] = null + if (value !== baselineValue) return nextState + + const providersByScope = indexContextProviders(blockFields) + const fieldAndProvidersAtBaseline = ( + field: ForkDependentReconfig, + visiting: Set + ): boolean => { + const fieldKey = dependentKey(field) + if (visiting.has(fieldKey)) return false + const fieldValue = nextState[fieldKey] + const effectiveFieldValue = + fieldValue === undefined ? context.baselineValueFor(field) : fieldValue + if (effectiveFieldValue !== context.baselineValueFor(field)) return false + + visiting.add(fieldKey) + const providers = providersByScope.get(field.dependencyScope) + const providerChainAtBaseline = field.consumesContextKeys.every((contextKey) => { + const provider = providers?.get(contextKey) + return !provider || fieldAndProvidersAtBaseline(provider, visiting) + }) + visiting.delete(fieldKey) + return providerChainAtBaseline + } + + for (let pass = 0; pass < descendants.length; pass += 1) { + let restored = false + for (const field of descendants) { + const fieldKey = dependentKey(field) + if (nextState[fieldKey] !== null) continue + const providers = providersByScope.get(field.dependencyScope) + const providerChainAtBaseline = field.consumesContextKeys.every((contextKey) => { + const provider = providers?.get(contextKey) + return !provider || fieldAndProvidersAtBaseline(provider, new Set()) + }) + if (!providerChainAtBaseline) continue + delete nextState[fieldKey] + restored = true + } + if (!restored) break + } + return nextState } /** * The value sent + displayed for a dependent: the user's in-session re-pick if present, else the - * stored value (`currentValue`). Blank when the parent target changed in-session, since the old - * stored value was for the previous parent and won't resolve against the new one. Shared by the - * sync gate + payload build and the per-block selector so the rule can't drift between them. + * stored value (`currentValue`). Blank when the parent target changed in-session, or when an + * in-block parent re-pick invalidated it, since the old stored value was for the previous parent + * and won't resolve against the new one. Shared by the sync gate and the per-block selector so + * the rule cannot drift between them. The payload submits the same effective blank so stale + * values are removed from top-level fields and nested Agent tool parameters alike. */ export function effectiveDependentValue( field: ForkDependentReconfig, - reconfig: Record, + reconfig: DependentReconfigState, parentChanged: boolean ): string { const repicked = reconfig[dependentKey(field)] + if (repicked === null) return '' if (repicked !== undefined) return repicked return parentChanged ? '' : field.currentValue } @@ -75,13 +163,26 @@ export function effectiveDependentValue( */ export function effectiveCopyDependentValue( field: ForkDependentReconfig, - reconfig: Record + reconfig: DependentReconfigState ): string { const repicked = reconfig[dependentKey(field)] + if (repicked === null) return '' if (repicked !== undefined) return repicked return field.currentValue || field.sourceValue } +/** + * Whether an in-block provider change invalidated this field and the user has not re-picked it. + * It reads and submits as empty while the provider remains changed, preventing a stale top-level + * or nested Agent tool value from surviving under the new scope. + */ +export function isDependentInvalidated( + field: ForkDependentReconfig, + reconfig: DependentReconfigState +): boolean { + return reconfig[dependentKey(field)] === null +} + export interface DependentConfigurationState { parentResolved: boolean parentChanged: boolean @@ -92,10 +193,15 @@ export interface DependentConfigurationState { * Whether a dependent selector needs to be shown. A changed or copied parent requires review * because its children resolve in a different scope. An unchanged mapping only needs a selector * when a required value is missing; its stored values are already valid and sync-ready. + * + * A field a parent re-pick invalidated reads as blank through `effectiveDependentValue`, so a + * REQUIRED one stays on screen here and keeps gating Sync. An optional one drops out of the + * default view - `getDisplayedDependentFields` brings it back under explicit edit mode - and + * hiding it is safe because the effective blank is still submitted to prevent a stale value. */ export function isDependentConfigurationActionable( field: ForkDependentReconfig, - reconfig: Record, + reconfig: DependentReconfigState, state: DependentConfigurationState ): boolean { if (!state.parentResolved) return false @@ -110,22 +216,13 @@ export function isDependentConfigurationActionable( */ export function getActionableDependentFields( fields: ForkDependentReconfig[], - reconfig: Record, + reconfig: DependentReconfigState, state: DependentConfigurationState ): ForkDependentReconfig[] { const actionable = new Set( fields.filter((field) => isDependentConfigurationActionable(field, reconfig, state)) ) - const providersByScope = new Map>() - for (const field of fields) { - if (!field.providesContextKey) continue - let providers = providersByScope.get(field.dependencyScope) - if (!providers) { - providers = new Map() - providersByScope.set(field.dependencyScope, providers) - } - providers.set(field.providesContextKey, field) - } + const providersByScope = indexContextProviders(fields) const pending = Array.from(actionable) for (let index = 0; index < pending.length; index += 1) { @@ -149,7 +246,7 @@ export function getActionableDependentFields( */ export function getDisplayedDependentFields( fields: ForkDependentReconfig[], - reconfig: Record, + reconfig: DependentReconfigState, state: DependentConfigurationState, showConfigured: boolean ): ForkDependentReconfig[] { diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx index a0cc46362e..9a27dbf54e 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx +++ b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx @@ -36,6 +36,7 @@ import { DependentFieldSelector } from '@/ee/workspace-forking/components/fork-s import { applyDependentRepick, type DependentConfigurationState, + type DependentReconfigState, dependentKey, effectiveCopyDependentValue, effectiveDependentValue, @@ -109,7 +110,7 @@ interface WorkflowDependents { function groupDependentsByWorkflow( workflows: ForkResourceUsage['workflows'], dependents: ForkDependentReconfig[], - reconfig: Record, + reconfig: DependentReconfigState, state: DependentConfigurationState, showConfigured: boolean ): WorkflowDependents[] { @@ -181,8 +182,8 @@ interface DependentSelectorProps { copying: boolean workspaceId: string sourceWorkspaceId: string - reconfig: Record - setReconfig: Dispatch>> + reconfig: DependentReconfigState + setReconfig: Dispatch> } /** @@ -204,10 +205,12 @@ function DependentSelector({ reconfig, setReconfig, }: DependentSelectorProps) { - const effectiveValue = (f: ForkDependentReconfig) => + const effectiveValueIn = (f: ForkDependentReconfig, state: DependentReconfigState) => copying - ? effectiveCopyDependentValue(f, reconfig) - : effectiveDependentValue(f, reconfig, parentChanged) + ? effectiveCopyDependentValue(f, state) + : effectiveDependentValue(f, state, parentChanged) + const baselineValueFor = (f: ForkDependentReconfig) => effectiveValueIn(f, {}) + const effectiveValue = (f: ForkDependentReconfig) => effectiveValueIn(f, reconfig) const { providedValues, providedContextKeys } = blockChainState(block, field, effectiveValue) // Disabled until every in-block parent it depends on has a value, so a child never queries // a stale upstream value. @@ -230,7 +233,14 @@ function DependentSelector({ enabled={parentValue !== '' && ready} value={effectiveValue(field)} onChange={(value) => - setReconfig((current) => applyDependentRepick(current, field, block.fields, value)) + setReconfig((current) => + // The pre-pick value comes from the state being updated, so re-selecting the value + // already shown is recognised as the no-op it is and leaves descendants intact. + applyDependentRepick(current, field, block.fields, value, { + previousValue: effectiveValueIn(field, current), + baselineValueFor, + }) + ) } title={field.title} /> @@ -246,8 +256,8 @@ interface DependentWorkflowCardProps { copying: boolean workspaceId: string sourceWorkspaceId: string - reconfig: Record - setReconfig: Dispatch>> + reconfig: DependentReconfigState + setReconfig: Dispatch> } /** diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.test.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.test.tsx new file mode 100644 index 0000000000..dbf6f10d50 --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.test.tsx @@ -0,0 +1,558 @@ +/** + * @vitest-environment jsdom + * + * Coverage for the payload {@link useForkSync} actually submits, not just the helpers it calls. + * The tests distinguish a genuine provider change, which must clear unresolved descendants, + * from a provider undo, which must restore them before Save or Sync derives its payload. + */ +import { act, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { + ForkDependentReconfig, + ForkMappingEntry, + UpdateForkMappingBody, +} from '@/lib/api/contracts/workspace-fork' + +const { + mockUseForkMapping, + mockUseForkDiff, + mockUpdateMutate, + mockUpdateMutateAsync, + mockPromote, +} = vi.hoisted(() => ({ + mockUseForkMapping: vi.fn(), + mockUseForkDiff: vi.fn(), + mockUpdateMutate: vi.fn(), + mockUpdateMutateAsync: vi.fn(), + mockPromote: vi.fn(), +})) + +vi.mock('@sim/emcn', () => ({ + toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn() }, +})) + +vi.mock('@/ee/workspace-forking/hooks/workspace-fork', () => ({ + useForkMapping: mockUseForkMapping, + useForkDiff: mockUseForkDiff, + useUpdateForkMapping: () => ({ + mutate: mockUpdateMutate, + mutateAsync: mockUpdateMutateAsync, + isPending: false, + }), + usePromoteFork: () => ({ mutateAsync: mockPromote }), +})) + +import { + applyDependentRepick, + dependentKey, +} from '@/ee/workspace-forking/components/fork-sync/dependent-value' +import { + type ForkSyncController, + useForkSync, +} from '@/ee/workspace-forking/components/fork-sync/use-fork-sync' + +const WORKSPACE_ID = 'ws-child' +const OTHER_WORKSPACE_ID = 'ws-parent' +const WORKFLOW_ID = 'wf-1' +const BLOCK_ID = 'block-1' + +/** The mapped credential every dependent below hangs off; already mapped, so nothing re-picks it. */ +const CREDENTIAL_ENTRY: ForkMappingEntry = { + kind: 'credential', + resourceType: 'credential', + sourceId: 'cred-src', + sourceLabel: 'Google (source)', + targetId: 'cred-tgt', + suggested: false, + required: true, + sourceDeleted: false, + candidates: [], + candidatesTruncated: false, +} + +function dependent(overrides: Partial): ForkDependentReconfig { + return { + parentKind: 'credential', + parentSourceId: CREDENTIAL_ENTRY.sourceId, + parentContextKey: 'oauthCredential', + targetWorkflowId: WORKFLOW_ID, + targetBlockId: BLOCK_ID, + blockName: 'Google Sheets', + subBlockKey: 'field', + selectorKey: 'sheet', + title: 'Field', + currentValue: '', + sourceValue: '', + required: false, + consumesContextKeys: [], + context: {}, + ...overrides, + } +} + +/** The in-block provider the user re-picks; its change invalidates `CHILD`. */ +const PARENT_FIELD = dependent({ + subBlockKey: 'spreadsheetId', + title: 'Spreadsheet', + currentValue: 'sheet-1', + providesContextKey: 'spreadsheetId', +}) + +/** Optional descendant of `PARENT_FIELD` - the field the P0 used to blank in the target. */ +const CHILD_FIELD = dependent({ + subBlockKey: 'sheetName', + title: 'Sheet', + currentValue: 'Tab A', + consumesContextKeys: ['spreadsheetId'], +}) + +/** Unrelated field the user clears themselves; an intentional clear must still be submitted. */ +const CLEARED_FIELD = dependent({ + subBlockKey: 'folderId', + title: 'Folder', + currentValue: 'folder-9', +}) + +/** Untouched field, submitted with its stored value so the "full stored mapping" contract holds. */ +const UNTOUCHED_FIELD = dependent({ + subBlockKey: 'labelId', + title: 'Label', + currentValue: 'label-3', +}) + +const DEPENDENTS = [PARENT_FIELD, CHILD_FIELD, CLEARED_FIELD, UNTOUCHED_FIELD] + +const SUCCESSFUL_PROMOTE_RESULT = { + promoteRunId: 'run-1', + blockers: [], + unmappedRequired: [], + droppedReferences: [], + triggerUrlChanges: [], + deployFailed: 0, +} + +function createDeferred() { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +const mappedRepickContext = (previousValue: string) => ({ + previousValue, + baselineValueFor: (field: ForkDependentReconfig) => field.currentValue, +}) + +function diffData(dependentReconfigs: ForkDependentReconfig[]) { + return { + sourceWorkspaceId: WORKSPACE_ID, + targetWorkspaceId: OTHER_WORKSPACE_ID, + workflows: [], + dependentReconfigs, + resourceUsages: [], + copyableUnmapped: [], + clearedRefs: [], + triggerMappings: [], + retiringTriggerUrls: [], + excludedSourceWorkflows: [], + excludedTargetWorkflows: [], + mcpReauthServerIds: [], + inlineSecretSources: [], + } +} + +const mountedRoots: Root[] = [] + +function renderForkSync(): { get: () => ForkSyncController } { + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + const container = document.createElement('div') + const root = createRoot(container) + mountedRoots.push(root) + let result: ForkSyncController | undefined + + function Probe() { + result = useForkSync({ + workspaceId: WORKSPACE_ID, + otherWorkspaceId: OTHER_WORKSPACE_ID, + otherWorkspaceName: 'Parent', + direction: 'push', + enabled: true, + }) + return null + } + + act(() => { + root.render(() as ReactNode) + }) + + return { + get: () => { + if (!result) throw new Error('hook result is not ready') + return result + }, + } +} + +/** The `dependentValues` the last Save sent, or `undefined` when it sent none. */ +function savedDependentValues(): UpdateForkMappingBody['dependentValues'] { + expect(mockUpdateMutate).toHaveBeenCalledTimes(1) + const [variables] = mockUpdateMutate.mock.calls[0] as [{ body: UpdateForkMappingBody }] + return variables.body.dependentValues +} + +/** The `dependentValues` the last Sync promoted. */ +function promotedDependentValues(): UpdateForkMappingBody['dependentValues'] { + expect(mockPromote).toHaveBeenCalledTimes(1) + const [variables] = mockPromote.mock.calls[0] as [ + { body: { dependentValues?: UpdateForkMappingBody['dependentValues'] } }, + ] + return variables.body.dependentValues +} + +function valueFor( + submitted: UpdateForkMappingBody['dependentValues'], + field: ForkDependentReconfig +): string | undefined { + return submitted?.find((entry) => entry.subBlockKey === field.subBlockKey)?.value +} + +beforeEach(() => { + vi.clearAllMocks() + mockUseForkMapping.mockReturnValue({ + data: { entries: [CREDENTIAL_ENTRY] }, + isLoading: false, + isError: false, + error: null, + isPlaceholderData: false, + }) + mockUseForkDiff.mockReturnValue({ + data: diffData(DEPENDENTS), + isError: false, + error: null, + isPlaceholderData: false, + }) + mockUpdateMutateAsync.mockResolvedValue({ success: true, updated: 1 }) + mockPromote.mockResolvedValue(SUCCESSFUL_PROMOTE_RESULT) +}) + +afterEach(() => { + act(() => { + for (const root of mountedRoots.splice(0)) root.unmount() + }) +}) + +describe('useForkSync dependent payload', () => { + it('submits an effective blank for an optional dependent invalidated by a real provider change', () => { + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + + expect(get().reconfig[dependentKey(CHILD_FIELD)]).toBeNull() + + act(() => get().save()) + + const submitted = savedDependentValues() + expect(submitted?.map((entry) => entry.subBlockKey)).toEqual([ + PARENT_FIELD.subBlockKey, + CHILD_FIELD.subBlockKey, + CLEARED_FIELD.subBlockKey, + UNTOUCHED_FIELD.subBlockKey, + ]) + expect(valueFor(submitted, PARENT_FIELD)).toBe('sheet-2') + expect(valueFor(submitted, CHILD_FIELD)).toBe('') + expect(valueFor(submitted, UNTOUCHED_FIELD)).toBe('label-3') + }) + + it('submits a field the user cleared themselves, so an intentional clear still clears the target', () => { + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => ({ ...prev, [dependentKey(CLEARED_FIELD)]: '' })) + }) + act(() => get().save()) + + const submitted = savedDependentValues() + expect(submitted?.map((entry) => entry.subBlockKey)).toContain(CLEARED_FIELD.subBlockKey) + expect(valueFor(submitted, CLEARED_FIELD)).toBe('') + }) + + it('restores the dependent chain when the provider re-pick is undone', () => { + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + expect(get().dirty).toBe(true) + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-1', + mappedRepickContext('sheet-2') + ) + ) + }) + + expect(get().reconfig).toEqual({}) + expect(get().dirty).toBe(false) + + act(() => get().save()) + expect(mockUpdateMutate).not.toHaveBeenCalled() + }) + + it('keeps Sync blocked while a REQUIRED dependent is marked by a parent re-pick', () => { + const requiredChild = { ...CHILD_FIELD, required: true } + mockUseForkDiff.mockReturnValue({ + data: diffData([PARENT_FIELD, requiredChild]), + isError: false, + error: null, + isPlaceholderData: false, + }) + const { get } = renderForkSync() + + expect(get().syncDisabled).toBe(false) + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + [PARENT_FIELD, requiredChild], + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + + expect(get().syncDisabled).toBe(true) + expect(get().syncDisabledReason).toBe('Reconfigure all required fields first') + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + [PARENT_FIELD, requiredChild], + 'sheet-1', + mappedRepickContext('sheet-2') + ) + ) + }) + + expect(get().reconfig).toEqual({}) + expect(get().syncDisabled).toBe(false) + }) + + it('submits the invalidated dependent as blank in the promote payload too', async () => { + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + await act(async () => { + await get().sync() + }) + + expect(valueFor(promotedDependentValues(), CHILD_FIELD)).toBe('') + }) + + it('clears a stale nested Agent tool child when its provider changes', async () => { + const project = dependent({ + subBlockKey: 'tools[0].projectId', + dependencyScope: 'tools[0]', + currentValue: 'project-old', + providesContextKey: 'projectId', + }) + const issue = dependent({ + subBlockKey: 'tools[0].issueKey', + dependencyScope: 'tools[0]', + currentValue: 'OLD-1', + consumesContextKeys: ['projectId'], + }) + mockUseForkDiff.mockReturnValue({ + data: diffData([project, issue]), + isError: false, + error: null, + isPlaceholderData: false, + }) + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + project, + [project, issue], + 'project-new', + mappedRepickContext('project-old') + ) + ) + }) + await act(async () => { + await get().sync() + }) + + expect(valueFor(promotedDependentValues(), project)).toBe('project-new') + expect(valueFor(promotedDependentValues(), issue)).toBe('') + }) +}) + +describe('useForkSync post-sync reset', () => { + it('drops the in-session re-picks once the sync commits them', async () => { + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + expect(get().dirty).toBe(true) + + await act(async () => { + await get().sync() + }) + + expect(get().reconfig).toEqual({}) + expect(get().dirty).toBe(false) + }) + + it('keeps the in-session re-picks when the sync is refused by the server gate', async () => { + mockPromote.mockResolvedValue({ + promoteRunId: null, + blockers: [{ kind: 'credential', sourceId: 'cred-src' }], + unmappedRequired: [], + droppedReferences: [], + triggerUrlChanges: [], + deployFailed: 0, + }) + const { get } = renderForkSync() + + act(() => { + get().setReconfig((prev) => + applyDependentRepick( + prev, + PARENT_FIELD, + DEPENDENTS, + 'sheet-2', + mappedRepickContext('sheet-1') + ) + ) + }) + await act(async () => { + await get().sync() + }) + + expect(get().reconfig[dependentKey(PARENT_FIELD)]).toBe('sheet-2') + }) + + it('keeps a newer target mapping selected while Sync was in flight', async () => { + const pendingPromote = createDeferred() + mockPromote.mockReturnValue(pendingPromote.promise) + const { get } = renderForkSync() + let syncPromise!: Promise + + act(() => { + syncPromise = get().sync() + }) + await act(async () => Promise.resolve()) + expect(mockPromote).toHaveBeenCalledTimes(1) + + act(() => get().setTarget(CREDENTIAL_ENTRY, 'cred-newer')) + pendingPromote.resolve(SUCCESSFUL_PROMOTE_RESULT) + await act(async () => syncPromise) + + expect(get().targetFor(CREDENTIAL_ENTRY)).toBe('cred-newer') + expect(get().dirty).toBe(true) + }) + + it('keeps a newer dependent re-pick made while Sync was in flight', async () => { + const pendingPromote = createDeferred() + mockPromote.mockReturnValue(pendingPromote.promise) + const { get } = renderForkSync() + let syncPromise!: Promise + + act(() => { + syncPromise = get().sync() + }) + await act(async () => Promise.resolve()) + expect(mockPromote).toHaveBeenCalledTimes(1) + + act(() => { + get().setReconfig((current) => ({ + ...current, + [dependentKey(PARENT_FIELD)]: 'sheet-newer', + })) + }) + pendingPromote.resolve(SUCCESSFUL_PROMOTE_RESULT) + await act(async () => syncPromise) + + expect(get().reconfig[dependentKey(PARENT_FIELD)]).toBe('sheet-newer') + expect(get().dirty).toBe(true) + }) + + it('keeps newer mapping edits made while Save was in flight', () => { + let finishSave: (() => void) | undefined + mockUpdateMutate.mockImplementation((_variables, options) => { + finishSave = options.onSuccess + }) + const { get } = renderForkSync() + + act(() => { + get().setReconfig((current) => ({ + ...current, + [dependentKey(PARENT_FIELD)]: 'sheet-submitted', + })) + }) + act(() => get().save()) + + act(() => { + get().setTarget(CREDENTIAL_ENTRY, 'cred-newer') + get().setReconfig((current) => ({ + ...current, + [dependentKey(PARENT_FIELD)]: 'sheet-newer', + })) + }) + act(() => finishSave?.()) + + expect(get().targetFor(CREDENTIAL_ENTRY)).toBe('cred-newer') + expect(get().reconfig[dependentKey(PARENT_FIELD)]).toBe('sheet-newer') + expect(get().dirty).toBe(true) + }) +}) diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts index c28d29c371..61aadc7bce 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts @@ -31,9 +31,11 @@ import { isForkRequiredComplete, } from '@/ee/workspace-forking/components/fork-sync/copy-reconciliation' import { + type DependentReconfigState, dependentKey, effectiveCopyDependentValue, effectiveDependentValue, + isDependentInvalidated, } from '@/ee/workspace-forking/components/fork-sync/dependent-value' import { forkDyingTriggerUrls, @@ -145,8 +147,8 @@ export interface ForkSyncController { */ sourceWorkspaceId: string /** In-session dependent re-picks, keyed by `dependentKey`. */ - reconfig: Record - setReconfig: Dispatch>> + reconfig: DependentReconfigState + setReconfig: Dispatch> /** Keys the backend offers as copy candidates, for the entry rows' "Copy instead" affordance. */ copyableKeys: ReadonlySet /** Copyables actually selected for copy (visible + checked), keyed `${kind}:${sourceId}`. */ @@ -286,7 +288,7 @@ export function useForkSync(params: { // `dependentKey`. Folded into the full effective set sent on save/sync, which the server // persists as the stored mapping - so the selection survives every future sync without // re-picking. - const [reconfig, setReconfig] = useState>({}) + const [reconfig, setReconfig] = useState({}) // Referenced-but-unmapped resources the user chose to copy into the target (keyed by // `${kind}:${sourceId}`); default-selected once the diff loads. Selected ones are copied on // sync so their references resolve to the copy instead of being cleared. @@ -685,8 +687,11 @@ export function useForkSync(params: { // effective value: re-pick, stored, or blank-after-change) or copy-selected (re-pick, stored, // or the source reference; promote translates a source document id to its copied counterpart // at write time). The server persists this verbatim as the stored mapping; fields whose - // parent is unresolved are omitted (they can't be configured). This is the whole "what's in - // the mapping goes in" contract, shared by Save and Sync so the two persist identically. + // parent is unresolved are omitted because they can't be configured. A field invalidated by + // an in-block provider re-pick is submitted as `''`: required fields gate Sync until re-picked, + // while optional/LLM-fillable fields must land empty rather than retain a source value scoped + // to the old provider. This is the whole "what's in the mapping goes in" contract, shared by + // Save and Sync so the two persist identically. const buildDependentValues = () => dependentReconfigs.flatMap((field) => { const parent = entryForDependent(field) @@ -723,9 +728,12 @@ export function useForkSync(params: { // A dependent re-pick that differs from its stored value also dirties the editor. A re-pick // under a changed parent is covered by `targetsDirty` (the parent override is the change). + // An automatically invalidated field is covered by the provider override that caused it; it + // must not independently dirty an editor after that provider has been restored to baseline. const reconfigDirty = useMemo( () => dependentReconfigs.some((field) => { + if (isDependentInvalidated(field, reconfig)) return false const repicked = reconfig[dependentKey(field)] return repicked !== undefined && repicked !== field.currentValue }), @@ -736,6 +744,8 @@ export function useForkSync(params: { const save = () => { if (!otherWorkspaceId || !dirty || updateMapping.isPending) return + const submittedTargets = targets + const submittedReconfig = reconfig updateMapping.mutate( { workspaceId, @@ -751,8 +761,8 @@ export function useForkSync(params: { }, { onSuccess: () => { - setTargets({}) - setReconfig({}) + setTargets((current) => (current === submittedTargets ? {} : current)) + setReconfig((current) => (current === submittedReconfig ? {} : current)) toast.success('Mapping saved') }, onError: (error) => toast.error(getErrorMessage(error, 'Failed to save mapping')), @@ -828,6 +838,8 @@ export function useForkSync(params: { const sync = async () => { if (!otherWorkspaceId) return setSubmitting(true) + const submittedTargets = targets + const submittedReconfig = reconfig // Capture every payload from the state at confirm time, before any await - the page's // controls stay mounted during the run (unlike the old modal, which blocked its UI), so a // mid-flight edit must not leak into the promote body. @@ -922,6 +934,12 @@ export function useForkSync(params: { return } + // The run committed the in-session choices: the mapping entries and dependent values are + // stored. Drop only the exact snapshots it submitted; edits made while the request was in + // flight were not committed by this run and must remain available for the next Save/Sync. + setTargets((current) => (current === submittedTargets ? {} : current)) + setReconfig((current) => (current === submittedReconfig ? {} : current)) + const target = otherWorkspaceName || 'the workspace' const label = direction === 'pull' ? `Pulled from "${target}"` : `Pushed to "${target}"` // A sync only commits once every reference is mapped/copied and every required dependent diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts index 622c01cf58..76f2bb315a 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts @@ -4,21 +4,75 @@ import { folder as folderTable } from '@sim/db/schema' import { dbChainMockFns, + type MockCondition, resetDbChainMock, storageServiceMock, storageServiceMockFns, } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +/** The `workspace_files` columns {@link fileRows} enforces its unique indexes on. */ +interface WorkspaceFileRow { + id: string + key: string + workspaceId: string | null + folderId?: string | null + context: string + originalName: string + deletedAt: Date | null + [column: string]: unknown +} + +/** + * `fileRows` is the shared stand-in for the `workspace_files` table: the mocked name + * allocator reads it exactly as the real one queries the DB, and the insert simulation in + * `executeForkFileBlobCopies collisions` enforces the same unique indexes against it. One + * store, so the allocator and the index can never disagree the way two fixtures would. + */ const { + fileRows, + mockAllocateUniqueWorkspaceFileName, mockCopyWorkspaceFileSecretProvenanceInTx, mockIncrementStorageUsageInTx, mockResolveStorageBillingContext, -} = vi.hoisted(() => ({ - mockCopyWorkspaceFileSecretProvenanceInTx: vi.fn(), - mockIncrementStorageUsageInTx: vi.fn(), - mockResolveStorageBillingContext: vi.fn(), -})) +} = vi.hoisted(() => { + const fileRows: WorkspaceFileRow[] = [] + const withCopySuffix = (name: string, n: number) => { + const lastDot = name.lastIndexOf('.') + return lastDot > 0 && lastDot < name.length - 1 + ? `${name.slice(0, lastDot)} (${n})${name.slice(lastDot)}` + : `${name} (${n})` + } + return { + fileRows, + /** + * Mirrors `allocateUniqueWorkspaceFileName`: the taken-name probe matches the columns + * of `workspace_files_workspace_folder_name_active_unique`. + */ + mockAllocateUniqueWorkspaceFileName: vi.fn( + async (workspaceId: string, baseName: string, folderId?: string | null) => { + const taken = (name: string) => + fileRows.some( + (row) => + row.deletedAt === null && + row.context === 'workspace' && + row.workspaceId === workspaceId && + (row.folderId ?? null) === (folderId ?? null) && + row.originalName === name + ) + if (!taken(baseName)) return baseName + for (let n = 1; n <= 1000; n++) { + const candidate = withCopySuffix(baseName, n) + if (!taken(candidate)) return candidate + } + throw new Error(`A file named "${baseName}" already exists in this workspace`) + } + ), + mockCopyWorkspaceFileSecretProvenanceInTx: vi.fn(), + mockIncrementStorageUsageInTx: vi.fn(), + mockResolveStorageBillingContext: vi.fn(), + } +}) vi.mock('@/lib/uploads/core/storage-service', () => storageServiceMock) vi.mock('@/lib/billing/storage', () => ({ @@ -26,6 +80,7 @@ vi.mock('@/lib/billing/storage', () => ({ resolveStorageBillingContext: mockResolveStorageBillingContext, })) vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ + allocateUniqueWorkspaceFileName: mockAllocateUniqueWorkspaceFileName, generateWorkspaceFileKey: vi.fn( (workspaceId: string, fileName: string) => `workspace/${workspaceId}/generated-${fileName}` ), @@ -63,6 +118,7 @@ describe('executeForkFileBlobCopies storage accounting', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() + fileRows.length = 0 storageServiceMockFns.mockHeadObject.mockResolvedValue(null) storageServiceMockFns.mockDownloadFile.mockResolvedValue(Buffer.from('blob-bytes')) storageServiceMockFns.mockUploadFile.mockResolvedValue({ key: 'workspace/child-ws/target' }) @@ -194,6 +250,290 @@ describe('executeForkFileBlobCopies storage accounting', () => { }) }) +/** Rejects a predicate the harness does not model, rather than letting it match everything. */ +function unsupportedPredicate(detail: string): never { + throw new Error(`Unsupported predicate in test harness: ${detail}`) +} + +/** The nested clauses of an `and`/`or` node, or a throw when the node carries none. */ +function predicateClauses(node: MockCondition): unknown[] { + if (!Array.isArray(node.conditions)) + unsupportedPredicate(`${String(node.type)} without conditions`) + return node.conditions +} + +/** + * The row key a predicate node references. The mocked schema tables are column-name maps, so a + * column reference is the column name itself; anything else is a shape this harness cannot read. + */ +function predicateColumn(node: MockCondition, field: 'left' | 'column'): string { + const column = node[field] + if (typeof column !== 'string') + unsupportedPredicate(`${String(node.type)} with a non-column ${field}`) + return column +} + +/** + * Evaluate a mocked drizzle predicate against a row. Real predicate reading, so a chain that + * ignores its `where` clause cannot pass these tests by echoing a fixture back. + */ +function matchesPredicate(row: Record, predicate: unknown): boolean { + if (!predicate) return true + if (typeof predicate !== 'object') unsupportedPredicate(typeof predicate) + const node = predicate as MockCondition + switch (node.type) { + case 'and': + return predicateClauses(node).every((clause) => matchesPredicate(row, clause)) + case 'or': + return predicateClauses(node).some((clause) => matchesPredicate(row, clause)) + case 'eq': + return row[predicateColumn(node, 'left')] === node.right + case 'isNull': { + const value = row[predicateColumn(node, 'column')] + return value === null || value === undefined + } + case 'inArray': { + if (!Array.isArray(node.values)) unsupportedPredicate('inArray without values') + return node.values.includes(row[predicateColumn(node, 'column')]) + } + default: + return unsupportedPredicate(String(node.type)) + } +} + +/** Awaitable stand-in for a drizzle select result, supporting `.limit`/`.for`/`.orderBy`. */ +interface MockSelectResult extends PromiseLike { + catch: Promise['catch'] + finally: Promise['finally'] + limit: (count: number) => MockSelectResult + for: () => MockSelectResult + orderBy: () => MockSelectResult +} + +function selectResult(rows: WorkspaceFileRow[]): MockSelectResult { + const settled = Promise.resolve(rows) + const builder: MockSelectResult = { + then: (onFulfilled, onRejected) => settled.then(onFulfilled, onRejected), + catch: (onRejected) => settled.catch(onRejected), + finally: (onFinally) => settled.finally(onFinally), + limit: (count: number) => selectResult(rows.slice(0, count)), + for: () => builder, + orderBy: () => builder, + } + return builder +} + +/** + * Postgres-faithful `workspace_files` writes against {@link fileRows}: `key` is guarded by + * `workspace_files_key_active_unique` and `(workspace_id, coalesce(folder_id, ''), + * original_name)` by `workspace_files_workspace_folder_name_active_unique`. A bare + * `onConflictDoNothing()` absorbs BOTH plus the primary key (the shipped bug); one targeted at + * the primary key absorbs only a replay of the same row and lets a real name clash raise. + */ +function installFileTableSimulation(): void { + dbChainMockFns.where.mockImplementation((predicate: unknown) => + selectResult(fileRows.filter((row) => matchesPredicate(row, predicate))) + ) + dbChainMockFns.values.mockImplementation((row: WorkspaceFileRow) => { + const attemptInsert = (conflictTarget: unknown) => { + const pkConflict = fileRows.some((existing) => existing.id === row.id) + const activeConflict = fileRows.some( + (existing) => + existing.deletedAt === null && + (existing.key === row.key || + (existing.context === 'workspace' && + existing.workspaceId === row.workspaceId && + (existing.folderId ?? null) === (row.folderId ?? null) && + existing.originalName === row.originalName)) + ) + if (conflictTarget === undefined ? pkConflict || activeConflict : pkConflict) { + return Promise.resolve([]) + } + if (activeConflict) { + return Promise.reject( + Object.assign( + new Error( + 'duplicate key value violates unique constraint ' + + '"workspace_files_workspace_folder_name_active_unique"' + ), + { code: '23505' } + ) + ) + } + fileRows.push({ ...row }) + return Promise.resolve([{ id: row.id }]) + } + return { + onConflictDoNothing: (config?: { target?: unknown }) => { + dbChainMockFns.onConflictDoNothing(config) + return { returning: () => attemptInsert(config?.target) } + }, + onConflictDoUpdate: () => ({ returning: () => attemptInsert(undefined) }), + returning: () => attemptInsert('no-conflict-clause'), + } + }) +} + +describe('executeForkFileBlobCopies target name collisions', () => { + const collidingTask = () => + makeTask({ + fileName: 'budget.xlsx', + sourceKey: 'workspace/src-ws/source-budget.xlsx', + targetKey: 'workspace/child-ws/target-budget.xlsx', + contentType: 'application/vnd.ms-excel', + targetFolderId: 'target-reports', + displayName: 'budget.xlsx', + }) + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + fileRows.length = 0 + storageServiceMockFns.mockHeadObject.mockResolvedValue(null) + storageServiceMockFns.mockDownloadFile.mockResolvedValue(Buffer.from('blob-bytes')) + storageServiceMockFns.mockUploadFile.mockResolvedValue({ + key: 'workspace/child-ws/target-budget.xlsx', + }) + mockResolveStorageBillingContext.mockResolvedValue({ + workspaceId: 'child-ws', + billedAccountUserId: 'target-payer', + billingEntity: { type: 'user', id: 'target-payer' }, + plan: 'pro', + customStorageLimitGB: null, + }) + mockIncrementStorageUsageInTx.mockResolvedValue(100) + installFileTableSimulation() + }) + + it('keeps a file whose name is already taken in the reused target folder', async () => { + // The target already holds `Reports/budget.xlsx`; the fork mirrors `Reports` onto it. + fileRows.push({ + id: 'pre-existing', + key: 'workspace/child-ws/pre-existing-budget.xlsx', + workspaceId: 'child-ws', + folderId: 'target-reports', + context: 'workspace', + originalName: 'budget.xlsx', + deletedAt: null, + }) + + const result = await executeForkFileBlobCopies([collidingTask()], 'test') + + expect(result).toEqual({ copied: 1, failed: 0, failedTargetKeys: [] }) + // Non-destructive: the copy lands beside the target's own file, in the mirrored folder. + expect(fileRows).toHaveLength(2) + expect(fileRows.find((row) => row.id === 'pre-existing')?.originalName).toBe('budget.xlsx') + expect(fileRows.find((row) => row.id === 'target-file-1')).toMatchObject({ + key: 'workspace/child-ws/target-budget.xlsx', + workspaceId: 'child-ws', + folderId: 'target-reports', + originalName: 'budget (1).xlsx', + displayName: 'budget (1).xlsx', + deletedAt: null, + }) + // The blob backing the surviving row must never be swept. + expect(storageServiceMockFns.mockDeleteFile).not.toHaveBeenCalled() + // The de-duplication probe is scoped to the index's exact tuple, folder included. + expect(mockAllocateUniqueWorkspaceFileName).toHaveBeenCalledWith( + 'child-ws', + 'budget.xlsx', + 'target-reports' + ) + expect(mockIncrementStorageUsageInTx).toHaveBeenCalledTimes(1) + }) + + it('absorbs only a primary-key conflict, so a name conflict can never be mistaken for a replay', async () => { + await executeForkFileBlobCopies([collidingTask()], 'test') + + expect(dbChainMockFns.onConflictDoNothing).toHaveBeenCalledWith({ target: 'id' }) + }) + + it('copies a non-colliding file into the mirrored folder unchanged', async () => { + fileRows.push({ + id: 'pre-existing', + key: 'workspace/child-ws/pre-existing-forecast.xlsx', + workspaceId: 'child-ws', + folderId: 'target-reports', + context: 'workspace', + originalName: 'forecast.xlsx', + deletedAt: null, + }) + + const result = await executeForkFileBlobCopies([collidingTask()], 'test') + + expect(result).toEqual({ copied: 1, failed: 0, failedTargetKeys: [] }) + expect(fileRows.find((row) => row.id === 'target-file-1')).toMatchObject({ + folderId: 'target-reports', + originalName: 'budget.xlsx', + displayName: 'budget.xlsx', + }) + expect(storageServiceMockFns.mockDeleteFile).not.toHaveBeenCalled() + }) + + it('de-duplicates each same-named copy against the rows earlier tasks already landed', async () => { + // Two source files share a name inside the folder the target already has a `budget.xlsx` in. + // Each allocation must see the row the previous task committed, so the suffixes advance + // instead of every task racing for the same `budget (1).xlsx`. + fileRows.push({ + id: 'pre-existing', + key: 'workspace/child-ws/pre-existing-budget.xlsx', + workspaceId: 'child-ws', + folderId: 'target-reports', + context: 'workspace', + originalName: 'budget.xlsx', + deletedAt: null, + }) + + const result = await executeForkFileBlobCopies( + [ + collidingTask(), + makeTask({ + fileName: 'budget.xlsx', + sourceKey: 'workspace/src-ws/source-budget-2.xlsx', + targetKey: 'workspace/child-ws/target-budget-2.xlsx', + contentType: 'application/vnd.ms-excel', + targetFolderId: 'target-reports', + targetFileId: 'target-file-2', + }), + ], + 'test' + ) + + expect(result).toEqual({ copied: 2, failed: 0, failedTargetKeys: [] }) + expect(fileRows.map((row) => row.originalName)).toEqual([ + 'budget.xlsx', + 'budget (1).xlsx', + 'budget (2).xlsx', + ]) + expect(storageServiceMockFns.mockDeleteFile).not.toHaveBeenCalled() + }) + + it('replays to the same end state without duplicating the copy or failing differently', async () => { + fileRows.push({ + id: 'pre-existing', + key: 'workspace/child-ws/pre-existing-budget.xlsx', + workspaceId: 'child-ws', + folderId: 'target-reports', + context: 'workspace', + originalName: 'budget.xlsx', + deletedAt: null, + }) + + const first = await executeForkFileBlobCopies([collidingTask()], 'test') + const afterFirst = structuredClone(fileRows) + const replay = await executeForkFileBlobCopies([collidingTask()], 'test') + + expect(first).toEqual({ copied: 1, failed: 0, failedTargetKeys: [] }) + // The replay resolves the existing copy instead of re-copying: still one success, no failure. + expect(replay).toEqual({ copied: 1, failed: 0, failedTargetKeys: [] }) + expect(fileRows).toEqual(afterFirst) + expect(storageServiceMockFns.mockUploadFile).toHaveBeenCalledTimes(1) + expect(dbChainMockFns.transaction).toHaveBeenCalledTimes(1) + expect(mockIncrementStorageUsageInTx).toHaveBeenCalledTimes(1) + expect(storageServiceMockFns.mockDeleteFile).not.toHaveBeenCalled() + }) +}) + describe('planForkFileCopies', () => { it('plans deterministic target metadata without inserting an active row before blob copy', async () => { const sourceMeta = { diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-files.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-files.ts index cfa4888170..22ff96868a 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-files.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-files.ts @@ -9,7 +9,10 @@ import { resolveStorageBillingContext, } from '@/lib/billing/storage' import type { DbOrTx } from '@/lib/db/types' -import { generateWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager' +import { + allocateUniqueWorkspaceFileName, + generateWorkspaceFileKey, +} from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { copyWorkspaceFileSecretProvenanceInTx } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance' import { deleteFile, @@ -112,6 +115,31 @@ async function getFinalizedFileCopies( return new Map(active.map((row) => [row.id, { key: row.key, workspaceId: row.workspaceId }])) } +/** + * Pick the child row's `original_name`, de-duplicating against the partial unique index + * `workspace_files_workspace_folder_name_active_unique` on + * `(workspace_id, coalesce(folder_id, ''), original_name)`. + * + * Since the fork copy started preserving folder structure, a target folder reused by + * {@link resolveForkFolderMapping} can already hold a file of the same name (parent has + * `Reports/budget.xlsx`, the child pushes its own `Reports/budget.xlsx`). Reusing the source + * name would violate that index, so the copy lands as `budget (1).xlsx` instead: both files + * survive, the copy stays visible in the fork under the mirrored folder, and its blob is kept. + * + * Advisory only. The lookup runs outside the finalize transaction (and so cannot see it), so a + * concurrent upload can still claim the name in between. The index stays the authority: such a + * task fails loudly into `failedTargetKeys` instead of being silently dropped. + */ +async function resolveTargetOriginalName(task: BlobCopyTask): Promise { + // The index is partial on durable workspace files; no other context can collide on it. + if (task.context !== 'workspace') return task.fileName + return allocateUniqueWorkspaceFileName( + task.workspaceId, + task.fileName, + task.targetFolderId ?? null + ) +} + /** * Plan child metadata identities and blob copies without inserting an active * `workspace_files` row. The child workspace and source mappings are committed @@ -216,6 +244,10 @@ export async function planForkFileCopies(params: { * `file-upload` references pointing at the now-missing object. A failed task has no * active target metadata; an object uploaded before a failed finalization is deleted * best-effort outside the transaction. + * + * A copy whose name is already taken inside its mirrored target folder is de-duplicated + * (`budget (1).xlsx`) rather than dropped - see {@link resolveTargetOriginalName}. Those + * still count as `copied`. */ export async function executeForkFileBlobCopies( blobTasks: BlobCopyTask[], @@ -292,6 +324,9 @@ export async function executeForkFileBlobCopies( } const billingContext = await resolveStorageBillingContext(task.workspaceId) + const targetOriginalName = await resolveTargetOriginalName(task) + const targetDisplayName = + targetOriginalName === task.fileName ? task.displayName : targetOriginalName await db.transaction(async (tx) => { const [inserted] = await tx .insert(workspaceFiles) @@ -303,16 +338,21 @@ export async function executeForkFileBlobCopies( folderId: task.targetFolderId ?? null, context: task.context, chatId: null, - originalName: task.fileName, - displayName: task.displayName, + originalName: targetOriginalName, + displayName: targetDisplayName, contentType: task.contentType, size: task.size, deletedAt: null, uploadedAt: new Date(), }) - .onConflictDoNothing() + // Targeted at the primary key so ONLY a replay of this same task is absorbed. A + // bare `onConflictDoNothing()` also swallows the `(workspace_id, folder_id, + // original_name)` unique index, whose conflicting row has a DIFFERENT id - the + // recovery below then finds nothing and drops a file that merely shares a name. + .onConflictDoNothing({ target: workspaceFiles.id }) .returning({ id: workspaceFiles.id }) + // Reachable only on a primary-key conflict, so the row is addressable by id. if (!inserted) { const [current] = await tx .select({ @@ -346,8 +386,8 @@ export async function executeForkFileBlobCopies( folderId: task.targetFolderId ?? null, context: task.context, chatId: null, - originalName: task.fileName, - displayName: task.displayName, + originalName: targetOriginalName, + displayName: targetDisplayName, contentType: task.contentType, size: task.size, deletedAt: null, @@ -371,6 +411,14 @@ export async function executeForkFileBlobCopies( await incrementStorageUsageForBillingContextInTx(tx, billingContext, task.size) }) copied += 1 + if (targetOriginalName !== task.fileName) { + logger.warn(`[${requestId}] Copied file renamed to avoid a target name collision`, { + targetKey: task.targetKey, + folderId: task.targetFolderId ?? null, + from: task.fileName, + to: targetOriginalName, + }) + } } catch (error) { failedTargetKeys.push(task.targetKey) logger.warn(`[${requestId}] Failed to copy file blob during fork`, { diff --git a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts index 9d52ec1ce2..f050018cec 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -1,12 +1,15 @@ import { isRecordLike } from '@sim/utils/object' import type { ForkDependentReconfig, ForkResourceUsage } from '@/lib/api/contracts/workspace-fork' import { coerceObjectArray } from '@/lib/workflows/persistence/remap-internal-ids' -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' import { getToolInputParamConfigs } from '@/lib/workflows/search-replace/indexer' import { buildSelectorContextFromBlock, SELECTOR_CONTEXT_FIELDS, } from '@/lib/workflows/subblocks/context' +import { + getDependsOnFields, + getTransitiveSubBlockDependents, +} from '@/lib/workflows/subblocks/dependencies' import { buildCanonicalIndex, buildSubBlockValues, @@ -18,7 +21,6 @@ import { import { resolveToolParamRequired } from '@/lib/workflows/tool-input/param-visibility' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' -import { getDependsOnFields } from '@/blocks/utils' import type { ForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' import { toScannerBlocks } from '@/ee/workspace-forking/lib/remap/reference-scan' import { @@ -163,7 +165,7 @@ function emitAnchoredDependents(params: EmitAnchoredParams): void { if (typeof value === 'string' && value) context[key] = value } - for (const clear of getWorkflowSearchDependentClears(config.subBlocks, anchorCfg.id)) { + for (const clear of getTransitiveSubBlockDependents(config.subBlocks, [anchorCfg.id])) { const dependent = configById.get(clear.subBlockId) if (!dependent?.id || !dependent.selectorKey) continue // Skip fields gated off by their `condition` - a selector under a now-inactive diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts index 4f050b71c5..0334e30e1d 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts @@ -1117,6 +1117,56 @@ describe('applyDependentOverrides', () => { expect(tools[0].params.folder).toBe('Label_99') }) + it('applies an invalidated nested child as empty so it cannot survive under a new provider', () => { + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'jira') { + return blockWith([ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { + id: 'projectId', + title: 'Project', + type: 'project-selector', + dependsOn: ['credential'], + selectorKey: 'jira.projects', + }, + { + id: 'issueKey', + title: 'Issue', + type: 'issue-selector', + dependsOn: ['projectId'], + selectorKey: 'jira.issues', + }, + ]) + } + return undefined as unknown as BlockConfig + }) + const subBlocks: SubBlockRecord = { + tools: entry('tools', 'tool-input', [ + { + type: 'jira', + title: 'Jira', + params: { credential: 'c-new', projectId: 'project-old', issueKey: 'OLD-1' }, + }, + ]), + } + + const result = applyDependentOverrides( + subBlocks, + 'agent', + new Map([ + ['tools[0].projectId', 'project-new'], + ['tools[0].issueKey', ''], + ]) + ) + const tools = ( + result.tools as { + value: Array<{ params: { projectId: string; issueKey: string } }> + } + ).value + expect(tools[0].params).toMatchObject({ projectId: 'project-new', issueKey: '' }) + }) + it('rejects a nested override for a non-allowlisted tool param', () => { vi.mocked(getBlock).mockImplementation((type) => { if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts index 1a71e90e60..ae83c7c6f1 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -10,13 +10,17 @@ import { type SubBlockRecord, } from '@/lib/workflows/persistence/remap-internal-ids' import { CREDENTIAL_SUBBLOCK_IDS } from '@/lib/workflows/persistence/utils' -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' import { getToolInputParamConfigs } from '@/lib/workflows/search-replace/indexer' import { getWorkflowSearchSubBlockResourceDefinition, parseWorkflowSearchSubBlockResources, type StructuredWorkflowSearchResourceKind, } from '@/lib/workflows/search-replace/resources/registry' +import { + getDependsOnFields, + getSubBlocksDependingOnChange, + getTransitiveSubBlockDependents, +} from '@/lib/workflows/subblocks/dependencies' import { buildCanonicalIndex, buildSubBlockValues, @@ -36,7 +40,6 @@ import { import type { ParsedStoredTool } from '@/lib/workflows/tool-input/types' import { getBlock } from '@/blocks/registry' import type { SubBlockConfig } from '@/blocks/types' -import { getDependsOnFields, getSubBlocksDependingOnChange } from '@/blocks/utils' import { collectForkFileUploadKeys, remapForkFileUploadValue, @@ -393,7 +396,7 @@ interface ToolBlockRemapOptions { * fields (handled by the callers / the workflow id map), not block params, so they * pass through here untouched. Returns a new tool object only when something changed. * After remapping, dependent params (via `dependsOn`) of any changed resource are - * cleared with the same {@link getWorkflowSearchDependentClears} walk search-replace + * cleared with the same {@link getTransitiveSubBlockDependents} walk search-replace * uses, so a child scoped to the old parent isn't left stale. */ export function remapToolBlockResources( @@ -592,7 +595,7 @@ export function remapToolBlockResources( const parentCfg = configBySubBlockId.get(subBlockId) const parentRemappedNonEmpty = isNonEmptyValue(readParam(parentCfg, subBlockId)) const parentCopied = parentRemappedNonEmpty && copyRemappedSubBlockIds.has(subBlockId) - for (const clear of getWorkflowSearchDependentClears(toolBlockSubBlocks, subBlockId)) { + for (const clear of getTransitiveSubBlockDependents(toolBlockSubBlocks, [subBlockId])) { const dependentCfg = configBySubBlockId.get(clear.subBlockId) // A verbatim manual-parent dependent is never cleared, even when reachable from a // second (remapped) parent. @@ -1136,7 +1139,7 @@ export function clearDependentsOnRemap( } } - // Same BFS as `getWorkflowSearchDependentClears`, with each preserved dependent's subtree + // Same BFS as `getTransitiveSubBlockDependents`, with each preserved dependent's subtree // pruned (skipping it keeps its own dependents - e.g. a tool's arguments - out of the clear // set). A dependent under an ACTIVE MANUAL parent is verbatim by policy (the manual value is // never remapped), so it is pruned the same way. diff --git a/apps/sim/hooks/use-collaborative-workflow.ts b/apps/sim/hooks/use-collaborative-workflow.ts index 87e37394c0..75516648bc 100644 --- a/apps/sim/hooks/use-collaborative-workflow.ts +++ b/apps/sim/hooks/use-collaborative-workflow.ts @@ -25,10 +25,10 @@ import { type WorkflowSearchSubflowFieldId, workflowSearchSubflowFieldMatchesExpected, } from '@/lib/workflows/search-replace/subflow-fields' +import { getSubBlocksDependingOnChange } from '@/lib/workflows/subblocks/dependencies' import { isSyntheticToolSubBlockId } from '@/lib/workflows/tool-input/synthetic-subblocks' import { useSocket } from '@/app/workspace/providers/socket-provider' import { getBlock } from '@/blocks' -import { getSubBlocksDependingOnChange } from '@/blocks/utils' import { invalidateDeploymentQueries } from '@/hooks/queries/deployments' import { useUndoRedo } from '@/hooks/use-undo-redo' import { diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts index 666d5c4e97..30312b308e 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts @@ -4,22 +4,23 @@ import { describe, expect, it, vi } from 'vitest' import { applyOperationsToWorkflowState } from './engine' -vi.mock('@/blocks/registry', () => ({ - getAllBlocks: () => [ - { +vi.mock('@/blocks/registry', () => { + const blocks: Record = { + condition: { type: 'condition', name: 'Condition', subBlocks: [{ id: 'conditions', type: 'condition-input' }], }, - { + agent: { type: 'agent', name: 'Agent', subBlocks: [ { id: 'systemPrompt', type: 'long-input' }, { id: 'model', type: 'combobox' }, + { id: 'tools', type: 'tool-input' }, ], }, - { + function: { type: 'function', name: 'Function', subBlocks: [ @@ -27,34 +28,54 @@ vi.mock('@/blocks/registry', () => ({ { id: 'language', type: 'dropdown' }, ], }, - ], - getBlock: (type: string) => { - const blocks: Record = { - condition: { - type: 'condition', - name: 'Condition', - subBlocks: [{ id: 'conditions', type: 'condition-input' }], - }, - agent: { - type: 'agent', - name: 'Agent', - subBlocks: [ - { id: 'systemPrompt', type: 'long-input' }, - { id: 'model', type: 'combobox' }, - ], - }, - function: { - type: 'function', - name: 'Function', - subBlocks: [ - { id: 'code', type: 'code' }, - { id: 'language', type: 'dropdown' }, - ], - }, - } - return blocks[type] || undefined - }, -})) + jira: { + type: 'jira', + name: 'Jira', + tools: { access: ['jira_get_issue'] }, + subBlocks: [ + { id: 'credential', type: 'oauth-input' }, + { + id: 'projectId', + type: 'project-selector', + canonicalParamId: 'projectId', + mode: 'basic', + dependsOn: ['credential'], + }, + { + id: 'manualProjectId', + type: 'short-input', + canonicalParamId: 'projectId', + mode: 'advanced', + dependsOn: ['credential'], + }, + { + id: 'issueKey', + type: 'file-selector', + canonicalParamId: 'issueKey', + mode: 'basic', + dependsOn: ['projectId'], + }, + { + id: 'manualIssueKey', + type: 'short-input', + canonicalParamId: 'issueKey', + mode: 'advanced', + dependsOn: ['projectId'], + }, + { + id: 'transitionId', + type: 'short-input', + dependsOn: ['issueKey'], + }, + ], + }, + } + + return { + getAllBlocks: () => Object.values(blocks), + getBlock: (type: string) => blocks[type], + } +}) vi.mock('@/lib/integrations/availability.server', () => ({ isIntegrationDeploymentAvailableForVisibility: () => true, @@ -189,6 +210,175 @@ function makeNestedLoopWorkflow() { } } +function makeDependentWorkflow() { + return { + blocks: { + 'jira-1': { + id: 'jira-1', + type: 'jira', + name: 'Jira 1', + position: { x: 0, y: 0 }, + enabled: true, + subBlocks: { + credential: { id: 'credential', type: 'oauth-input', value: 'credential-old' }, + projectId: { id: 'projectId', type: 'project-selector', value: 'PROJECT-OLD' }, + manualProjectId: { + id: 'manualProjectId', + type: 'short-input', + value: '', + }, + issueKey: { id: 'issueKey', type: 'file-selector', value: 'OLD-123' }, + manualIssueKey: { id: 'manualIssueKey', type: 'short-input', value: '' }, + transitionId: { id: 'transitionId', type: 'short-input', value: 'transition-old' }, + }, + outputs: {}, + data: { + canonicalModes: { + projectId: 'basic', + issueKey: 'basic', + }, + }, + }, + }, + edges: [], + loops: {}, + parallels: {}, + } +} + +describe('handleEditOperation dependent inputs', () => { + it('clears omitted descendants transitively when a parent changes', () => { + const { state } = applyOperationsToWorkflowState(makeDependentWorkflow(), [ + { + operation_type: 'edit', + block_id: 'jira-1', + params: { inputs: { projectId: 'PROJECT-NEW' } }, + }, + ]) + + expect(state.blocks['jira-1'].subBlocks.projectId.value).toBe('PROJECT-NEW') + expect(state.blocks['jira-1'].subBlocks.issueKey.value).toBe('') + expect(state.blocks['jira-1'].subBlocks.transitionId.value).toBe('') + }) + + it('preserves explicitly supplied descendants and clears only their omitted descendants', () => { + const { state } = applyOperationsToWorkflowState(makeDependentWorkflow(), [ + { + operation_type: 'edit', + block_id: 'jira-1', + params: { + inputs: { + projectId: 'PROJECT-NEW', + issueKey: 'NEW-456', + }, + }, + }, + ]) + + expect(state.blocks['jira-1'].subBlocks.projectId.value).toBe('PROJECT-NEW') + expect(state.blocks['jira-1'].subBlocks.issueKey.value).toBe('NEW-456') + expect(state.blocks['jira-1'].subBlocks.transitionId.value).toBe('') + }) + + it('does not clear descendants when the submitted parent is unchanged', () => { + const { state } = applyOperationsToWorkflowState(makeDependentWorkflow(), [ + { + operation_type: 'edit', + block_id: 'jira-1', + params: { inputs: { projectId: 'PROJECT-OLD' } }, + }, + ]) + + expect(state.blocks['jira-1'].subBlocks.issueKey.value).toBe('OLD-123') + expect(state.blocks['jira-1'].subBlocks.transitionId.value).toBe('transition-old') + }) + + it('uses canonical advanced inputs as dependency changes', () => { + const { state } = applyOperationsToWorkflowState(makeDependentWorkflow(), [ + { + operation_type: 'edit', + block_id: 'jira-1', + params: { inputs: { manualProjectId: 'PROJECT-MANUAL' } }, + }, + ]) + + expect(state.blocks['jira-1'].subBlocks.manualProjectId.value).toBe('PROJECT-MANUAL') + expect(state.blocks['jira-1'].data.canonicalModes.projectId).toBe('advanced') + expect(state.blocks['jira-1'].subBlocks.issueKey.value).toBe('') + expect(state.blocks['jira-1'].subBlocks.transitionId.value).toBe('') + }) + + it('clears active manual descendants when their authoring context changes', () => { + const workflow = makeDependentWorkflow() + const jira = workflow.blocks['jira-1'] + jira.subBlocks.projectId.value = '' + jira.subBlocks.manualProjectId.value = 'PROJECT-MANUAL-OLD' + jira.subBlocks.issueKey.value = '' + jira.subBlocks.manualIssueKey.value = 'OLD-123' + jira.data.canonicalModes.projectId = 'advanced' + jira.data.canonicalModes.issueKey = 'advanced' + + const { state } = applyOperationsToWorkflowState(workflow, [ + { + operation_type: 'edit', + block_id: 'jira-1', + params: { inputs: { credential: 'credential-new' } }, + }, + ]) + + expect(state.blocks['jira-1'].subBlocks.manualProjectId.value).toBe('') + expect(state.blocks['jira-1'].subBlocks.manualIssueKey.value).toBe('') + expect(state.blocks['jira-1'].subBlocks.transitionId.value).toBe('') + }) + + it('replaces nested agent tool params instead of retaining omitted dependents', () => { + const workflow = { + blocks: { + 'agent-1': { + id: 'agent-1', + type: 'agent', + name: 'Agent 1', + position: { x: 0, y: 0 }, + enabled: true, + subBlocks: { + tools: { + id: 'tools', + type: 'tool-input', + value: [ + { + type: 'jira', + params: { projectId: 'PROJECT-OLD', issueKey: 'OLD-123' }, + }, + ], + }, + }, + outputs: {}, + data: {}, + }, + }, + edges: [], + loops: {}, + parallels: {}, + } + + const { state } = applyOperationsToWorkflowState(workflow, [ + { + operation_type: 'edit', + block_id: 'agent-1', + params: { + inputs: { + tools: [{ type: 'jira', params: { projectId: 'PROJECT-NEW' } }], + }, + }, + }, + ]) + + expect(state.blocks['agent-1'].subBlocks.tools.value[0].params).toEqual({ + projectId: 'PROJECT-NEW', + }) + }) +}) + describe('handleEditOperation nestedNodes merge', () => { it('preserves existing child block IDs when editing a loop with nestedNodes', () => { const workflow = makeLoopWorkflow() diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts index eda45aa2e6..e2e8bb65f9 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts @@ -1,5 +1,9 @@ import { createLogger } from '@sim/logger' +import { isRecordLike } from '@sim/utils/object' +import { isEqual } from 'es-toolkit' import { isValidKey } from '@/lib/workflows/sanitization/key-validation' +import { getTransitiveSubBlockDependents } from '@/lib/workflows/subblocks/dependencies' +import { isNonEmptyValue } from '@/lib/workflows/subblocks/visibility' import { TriggerUtils } from '@/lib/workflows/triggers/triggers' import { getBlock } from '@/blocks/registry' import { normalizeName, RESERVED_BLOCK_NAMES } from '@/executor/constants' @@ -425,6 +429,14 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon if (params?.inputs) { if (!block.subBlocks) block.subBlocks = {} + const previousSubBlockValues = new Map( + Object.entries(block.subBlocks).map(([key, subBlock]: [string, any]) => [ + key, + structuredClone(subBlock?.value), + ]) + ) + const explicitInputKeys = new Set() + // Validate inputs against block configuration const validationResult = validateInputsForBlock(block.type, params.inputs, block_id) validationErrors.push(...validationResult.errors) @@ -439,6 +451,7 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon if (TRIGGER_RUNTIME_SUBBLOCK_IDS.includes(key)) { return } + explicitInputKeys.add(key) let sanitizedValue = normalizeSubblockValue(key, value) sanitizedValue = normalizeConditionRouterIds(block_id, key, sanitizedValue) @@ -467,12 +480,7 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon } } else { const existingValue = block.subBlocks[key].value - const valuesEqual = - typeof existingValue === 'object' || typeof sanitizedValue === 'object' - ? JSON.stringify(existingValue) === JSON.stringify(sanitizedValue) - : existingValue === sanitizedValue - - if (!valuesEqual) { + if (!isEqual(existingValue, sanitizedValue)) { block.subBlocks[key].value = sanitizedValue } } @@ -481,9 +489,12 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon if ( Object.hasOwn(params.inputs, 'triggerConfig') && block.subBlocks.triggerConfig && - typeof block.subBlocks.triggerConfig.value === 'object' + isRecordLike(block.subBlocks.triggerConfig.value) ) { applyTriggerConfigToBlockSubblocks(block, block.subBlocks.triggerConfig.value) + for (const key of Object.keys(block.subBlocks.triggerConfig.value)) { + explicitInputKeys.add(key) + } } // Update loop/parallel configuration in block.data (strict validation) @@ -538,11 +549,26 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon const editBlockConfig = getBlock(block.type) if (editBlockConfig) { - updateCanonicalModesForInputs( - block, - Object.keys(validationResult.validInputs), - editBlockConfig - ) + updateCanonicalModesForInputs(block, [...explicitInputKeys], editBlockConfig) + + const changedInputKeys = editBlockConfig.subBlocks + .filter((subBlock) => { + const currentSubBlock = block.subBlocks[subBlock.id] + const existed = previousSubBlockValues.has(subBlock.id) + if (!existed || !currentSubBlock) return existed !== Boolean(currentSubBlock) + return !isEqual(previousSubBlockValues.get(subBlock.id), currentSubBlock.value) + }) + .map((subBlock) => subBlock.id) + + for (const { subBlockId } of getTransitiveSubBlockDependents( + editBlockConfig.subBlocks, + changedInputKeys + )) { + if (explicitInputKeys.has(subBlockId)) continue + const dependent = block.subBlocks[subBlockId] + if (!dependent || !isNonEmptyValue(dependent.value)) continue + dependent.value = '' + } } } diff --git a/apps/sim/lib/workflows/search-replace/dependencies.test.ts b/apps/sim/lib/workflows/search-replace/dependencies.test.ts deleted file mode 100644 index f1e2505fa7..0000000000 --- a/apps/sim/lib/workflows/search-replace/dependencies.test.ts +++ /dev/null @@ -1,24 +0,0 @@ -/** - * @vitest-environment node - */ -import { describe, expect, it } from 'vitest' -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' -import type { SubBlockConfig } from '@/blocks/types' - -describe('getWorkflowSearchDependentClears', () => { - it('returns transitive dependents without cycling', () => { - const subBlocks: SubBlockConfig[] = [ - { id: 'credential', title: 'Credential', type: 'oauth-input' }, - { id: 'project', title: 'Project', type: 'project-selector', dependsOn: ['credential'] }, - { id: 'issue', title: 'Issue', type: 'file-selector', dependsOn: ['project'] }, - { id: 'assignee', title: 'Assignee', type: 'user-selector', dependsOn: ['issue'] }, - { id: 'unrelated', title: 'Unrelated', type: 'short-input' }, - ] - - expect(getWorkflowSearchDependentClears(subBlocks, 'credential')).toEqual([ - { subBlockId: 'project', reason: 'project depends on credential' }, - { subBlockId: 'issue', reason: 'issue depends on project' }, - { subBlockId: 'assignee', reason: 'assignee depends on issue' }, - ]) - }) -}) diff --git a/apps/sim/lib/workflows/search-replace/dependencies.ts b/apps/sim/lib/workflows/search-replace/dependencies.ts deleted file mode 100644 index 5059ebc29f..0000000000 --- a/apps/sim/lib/workflows/search-replace/dependencies.ts +++ /dev/null @@ -1,33 +0,0 @@ -import type { SubBlockConfig } from '@/blocks/types' -import { getSubBlocksDependingOnChange } from '@/blocks/utils' - -export interface DependentClear { - subBlockId: string - reason: string -} - -export function getWorkflowSearchDependentClears( - allSubBlocks: SubBlockConfig[], - changedSubBlockId: string -): DependentClear[] { - const clears: DependentClear[] = [] - const visited = new Set([changedSubBlockId]) - const queue = [changedSubBlockId] - - while (queue.length > 0) { - const currentSubBlockId = queue.shift() - if (!currentSubBlockId) continue - - for (const subBlock of getSubBlocksDependingOnChange(allSubBlocks, currentSubBlockId)) { - if (!subBlock.id || visited.has(subBlock.id)) continue - visited.add(subBlock.id) - clears.push({ - subBlockId: subBlock.id, - reason: `${subBlock.id} depends on ${currentSubBlockId}`, - }) - queue.push(subBlock.id) - } - } - - return clears -} diff --git a/apps/sim/lib/workflows/search-replace/indexer.ts b/apps/sim/lib/workflows/search-replace/indexer.ts index 25c2e3ca7b..5fc1a9ffdf 100644 --- a/apps/sim/lib/workflows/search-replace/indexer.ts +++ b/apps/sim/lib/workflows/search-replace/indexer.ts @@ -3,7 +3,6 @@ import { DEFAULT_SUBBLOCK_TYPE } from '@sim/workflow-persistence/subblocks' import type { SubBlockType } from '@sim/workflow-types/blocks' import { isWorkflowBlockProtected } from '@sim/workflow-types/workflow' import { COMPARISON_OPERATORS, LOGICAL_OPERATORS } from '@/lib/table/query-builder/constants' -import { getWorkflowSearchDependentClears } from '@/lib/workflows/search-replace/dependencies' import { getSearchableJsonStringLeaves, isSearchableJsonValueSubBlock, @@ -26,6 +25,7 @@ import type { } from '@/lib/workflows/search-replace/types' import { pathToKey, walkStringValues } from '@/lib/workflows/search-replace/value-walker' import { SELECTOR_CONTEXT_FIELDS } from '@/lib/workflows/subblocks/context' +import { getTransitiveSubBlockDependents } from '@/lib/workflows/subblocks/dependencies' import { resolveStoredToolName } from '@/lib/workflows/subblocks/display' import { buildCanonicalIndex, @@ -784,7 +784,7 @@ export function getToolInputParamConfigs({ ) const allToolSubBlocks = blockConfig?.subBlocks ?? subBlocksResult.subBlocks const getDependentValuePaths = (changedSubBlockId: string): WorkflowSearchValuePath[] => - getWorkflowSearchDependentClears(allToolSubBlocks, changedSubBlockId).map((clear) => [ + getTransitiveSubBlockDependents(allToolSubBlocks, [changedSubBlockId]).map((clear) => [ 'params', clear.subBlockId, ]) diff --git a/apps/sim/lib/workflows/subblocks/dependencies.test.ts b/apps/sim/lib/workflows/subblocks/dependencies.test.ts new file mode 100644 index 0000000000..0a8842ea97 --- /dev/null +++ b/apps/sim/lib/workflows/subblocks/dependencies.test.ts @@ -0,0 +1,137 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + getDependsOnFields, + getSubBlocksDependingOnChange, + getTransitiveSubBlockDependents, +} from '@/lib/workflows/subblocks/dependencies' +import type { SubBlockConfig } from '@/blocks/types' + +describe('getDependsOnFields', () => { + it('returns an empty array when dependsOn is unset', () => { + expect(getDependsOnFields(undefined)).toEqual([]) + }) + + it('returns array dependencies unchanged', () => { + expect(getDependsOnFields(['credential', 'projectId'])).toEqual(['credential', 'projectId']) + }) + + it('flattens all and any dependencies', () => { + expect(getDependsOnFields({ all: ['credential'], any: ['teamId', 'manualTeamId'] })).toEqual([ + 'credential', + 'teamId', + 'manualTeamId', + ]) + }) +}) + +describe('getSubBlocksDependingOnChange', () => { + it('finds direct dependents of a changed subblock', () => { + const subBlocks: SubBlockConfig[] = [ + { id: 'provider', title: 'Provider', type: 'dropdown' }, + { id: 'model', title: 'Model', type: 'dropdown', dependsOn: ['provider'] }, + { id: 'prompt', title: 'Prompt', type: 'long-input' }, + ] + + expect( + getSubBlocksDependingOnChange(subBlocks, 'provider').map((subBlock) => subBlock.id) + ).toEqual(['model']) + }) + + it('matches dependents through canonical basic and advanced siblings', () => { + const subBlocks: SubBlockConfig[] = [ + { + id: 'channel', + title: 'Channel', + type: 'channel-selector', + canonicalParamId: 'channelId', + mode: 'basic', + }, + { + id: 'manualChannel', + title: 'Channel ID', + type: 'short-input', + canonicalParamId: 'channelId', + mode: 'advanced', + }, + { + id: 'messageId', + title: 'Message ID', + type: 'short-input', + dependsOn: ['channelId'], + }, + { + id: 'threadTs', + title: 'Thread Timestamp', + type: 'short-input', + dependsOn: ['otherField'], + }, + ] + + expect( + getSubBlocksDependingOnChange(subBlocks, 'manualChannel').map((subBlock) => subBlock.id) + ).toEqual(['messageId']) + expect( + getSubBlocksDependingOnChange(subBlocks, 'channel').map((subBlock) => subBlock.id) + ).toEqual(['messageId']) + }) + + it('matches object-form dependencies when any listed dependency changes', () => { + const subBlocks: SubBlockConfig[] = [ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { id: 'teamId', title: 'Team', type: 'short-input' }, + { + id: 'projectId', + title: 'Project', + type: 'short-input', + dependsOn: { all: ['credential'], any: ['teamId'] }, + }, + ] + + expect( + getSubBlocksDependingOnChange(subBlocks, 'credential').map((subBlock) => subBlock.id) + ).toEqual(['projectId']) + expect( + getSubBlocksDependingOnChange(subBlocks, 'teamId').map((subBlock) => subBlock.id) + ).toEqual(['projectId']) + }) +}) + +describe('getTransitiveSubBlockDependents', () => { + it('returns transitive dependents without cycling', () => { + const subBlocks: SubBlockConfig[] = [ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { id: 'project', title: 'Project', type: 'project-selector', dependsOn: ['credential'] }, + { id: 'issue', title: 'Issue', type: 'file-selector', dependsOn: ['project'] }, + { id: 'assignee', title: 'Assignee', type: 'user-selector', dependsOn: ['issue'] }, + { id: 'unrelated', title: 'Unrelated', type: 'short-input' }, + ] + + expect(getTransitiveSubBlockDependents(subBlocks, ['credential'])).toEqual([ + { subBlockId: 'project', reason: 'project depends on credential' }, + { subBlockId: 'issue', reason: 'issue depends on project' }, + { subBlockId: 'assignee', reason: 'assignee depends on issue' }, + ]) + }) + + it('walks multiple changed roots once', () => { + const subBlocks: SubBlockConfig[] = [ + { id: 'credential', title: 'Credential', type: 'oauth-input' }, + { id: 'domain', title: 'Domain', type: 'short-input' }, + { + id: 'project', + title: 'Project', + type: 'project-selector', + dependsOn: ['credential', 'domain'], + }, + { id: 'issue', title: 'Issue', type: 'file-selector', dependsOn: ['project'] }, + ] + + expect(getTransitiveSubBlockDependents(subBlocks, ['credential', 'domain'])).toEqual([ + { subBlockId: 'project', reason: 'project depends on credential' }, + { subBlockId: 'issue', reason: 'issue depends on project' }, + ]) + }) +}) diff --git a/apps/sim/lib/workflows/subblocks/dependencies.ts b/apps/sim/lib/workflows/subblocks/dependencies.ts new file mode 100644 index 0000000000..59f2030c0b --- /dev/null +++ b/apps/sim/lib/workflows/subblocks/dependencies.ts @@ -0,0 +1,66 @@ +import { buildCanonicalIndex } from '@/lib/workflows/subblocks/visibility' +import type { SubBlockConfig } from '@/blocks/types' + +export interface DependentSubBlock { + subBlockId: string + reason: string +} + +/** Flattens array and all/any dependency declarations into their referenced field IDs. */ +export function getDependsOnFields(dependsOn: SubBlockConfig['dependsOn']): string[] { + if (!dependsOn) return [] + if (Array.isArray(dependsOn)) return dependsOn + return [...(dependsOn.all || []), ...(dependsOn.any || [])] +} + +/** Finds direct dependents while treating canonical basic/advanced siblings as one field. */ +export function getSubBlocksDependingOnChange( + allSubBlocks: SubBlockConfig[], + changedSubBlockId: string +): SubBlockConfig[] { + const canonicalIndex = buildCanonicalIndex(allSubBlocks) + const canonicalId = canonicalIndex.canonicalIdBySubBlockId[changedSubBlockId] + const group = canonicalId ? canonicalIndex.groupsById[canonicalId] : undefined + const changedFields = new Set([changedSubBlockId]) + + if (canonicalId) changedFields.add(canonicalId) + if (group?.basicId) changedFields.add(group.basicId) + for (const advancedId of group?.advancedIds || []) { + changedFields.add(advancedId) + } + + return allSubBlocks.filter((subBlock) => + getDependsOnFields(subBlock.dependsOn).some((field) => changedFields.has(field)) + ) +} + +/** + * Returns every transitive `dependsOn` descendant of the changed subblocks. + * Canonical basic/advanced siblings are treated as one logical field by the + * shared block dependency resolver. + */ +export function getTransitiveSubBlockDependents( + allSubBlocks: SubBlockConfig[], + changedSubBlockIds: Iterable +): DependentSubBlock[] { + const dependents: DependentSubBlock[] = [] + const visited = new Set(changedSubBlockIds) + const queue = [...visited] + + while (queue.length > 0) { + const currentSubBlockId = queue.shift() + if (!currentSubBlockId) continue + + for (const subBlock of getSubBlocksDependingOnChange(allSubBlocks, currentSubBlockId)) { + if (!subBlock.id || visited.has(subBlock.id)) continue + visited.add(subBlock.id) + dependents.push({ + subBlockId: subBlock.id, + reason: `${subBlock.id} depends on ${currentSubBlockId}`, + }) + queue.push(subBlock.id) + } + } + + return dependents +}