From 5ba5f1324e19dc689a947c7137db79effdf19c04 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Wed, 15 Jul 2026 15:25:49 -0700 Subject: [PATCH] fix(designer): scope Set Variable dropdown for pasted scopes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a Condition (or any scope) containing a Set Variable was copy-pasted into another scope — a Condition branch or a For each body — the pasted Set Variable's Name dropdown showed the wrong set of variables: empty in nested Conditions, or leaking out-of-scope variables (including ones from a parallel branch) when pasted into a For each. Selecting such a variable let the user reference an uninitialized variable and break scope. Root cause (two coordinated defects in the scope-paste path): 1. pasteScopeOperation seeded the pasted top node's `parentNodeId` with the relationship `parentId`, which is an edge-placement node (e.g. a `-#subgraph` card) and is not a nodesMetadata key. The parent-chain walk in getUpstreamNodeIds therefore could not climb to ancestor scopes. 2. pasteScopeInWorkflow recomputed the correct `parentNodeId` (the containing graph id) but assigned it to a detached object: the earlier metadata copy loop had already written the pasted node's entry into state by reference, so the correction never reached state.nodesMetadata. The final upstream recompute (updateAllUpstreamNodes -> getTokenNodeIds) then read the stale placeholder parentNodeId and mis-scoped the dropdown. Fix: - copypaste.ts: set the pasted scope node's `parentNodeId` to the containing `graphId` (undefined at root) instead of the edge-placement `parentId`. - pasteScopeInWorkflow.ts: write the corrected metadata back into state.nodesMetadata so the recomputed graphId/parentNodeId persist. - Restore strict upstream scoping in the Set Variable editor (getEditorAndOptions) and add extendUpstreamNodeIdsForScopePaste so token initialization seeds the enclosing scope's upstream tokens. Tests: - pasteScopeInWorkflow.spec.ts (v1 + v2): the pasted node's parentNodeId is persisted as the containing graph id (not the subgraph card) and stays undefined at root. - copypaste.spec.ts (v1 + v2): extendUpstreamNodeIdsForScopePaste surfaces ancestor variables while excluding parallel-branch variables. - getEditorAndOptions.spec.ts (v1 + v2): strict scoping — empty upstream yields no options; scoped upstream yields only in-scope variables. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: af9ea0d5-71e4-45d9-9790-0edc869beadd --- .../bjsworkflow/__test__/copypaste.spec.ts | 110 ++++ .../lib/core/actions/bjsworkflow/copypaste.ts | 54 +- .../__test__/pasteScopeInWorkflow.spec.ts | 95 ++++ .../parsers/__test__/pasteScopeMatrix.spec.ts | 490 ++++++++++++++++++ .../lib/core/parsers/pasteScopeInWorkflow.ts | 7 + .../__test__/getEditorAndOptions.spec.ts | 44 +- .../tabs/parametersTab/index.tsx | 19 +- .../bjsworkflow/__test__/copypaste.spec.ts | 110 ++++ .../lib/core/actions/bjsworkflow/copypaste.ts | 54 +- .../__test__/pasteScopeInWorkflow.spec.ts | 95 ++++ .../parsers/__test__/pasteScopeMatrix.spec.ts | 490 ++++++++++++++++++ .../lib/core/parsers/pasteScopeInWorkflow.ts | 7 + .../__test__/getEditorAndOptions.spec.ts | 44 +- .../tabs/parametersTab/index.tsx | 19 +- 14 files changed, 1554 insertions(+), 84 deletions(-) create mode 100644 libs/designer-v2/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts create mode 100644 libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts create mode 100644 libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts create mode 100644 libs/designer/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts create mode 100644 libs/designer/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts create mode 100644 libs/designer/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts diff --git a/libs/designer-v2/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts b/libs/designer-v2/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts new file mode 100644 index 00000000000..405e392a21c --- /dev/null +++ b/libs/designer-v2/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts @@ -0,0 +1,110 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { extendUpstreamNodeIdsForScopePaste } from '../copypaste'; +import type { RootState } from '../../..'; +import { createWorkflowEdge, createWorkflowNode } from '../../../utils/graph'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// Graph shape: +// root +// manual (trigger) +// Init_Variable (source node at root, before the parallel split) +// Init_Parallel (parallel branch A — NOT upstream of branches B/C) +// Condition_outer (parallel branch B; scope with a True subgraph — paste site) +// └── Condition_outer-actions (subgraph paste site inside the condition) +// For_each (parallel branch C; loop with an empty body — paste site) +const buildState = (): RootState => { + const rootGraph = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode('manual'), + createWorkflowNode('Init_Variable'), + createWorkflowNode('Init_Parallel'), + { + id: 'Condition_outer', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode('Condition_outer-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: 'Condition_outer-actions', + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode('Condition_outer-actions-#subgraph', WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + }, + ], + edges: [createWorkflowEdge('Condition_outer-#scope', 'Condition_outer-actions')], + }, + { + id: 'For_each', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode('For_each-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + }, + ], + edges: [ + createWorkflowEdge('manual', 'Init_Variable'), + createWorkflowEdge('Init_Variable', 'Init_Parallel'), + createWorkflowEdge('Init_Variable', 'Condition_outer'), + createWorkflowEdge('Init_Variable', 'For_each'), + ], + } as any; + + return { + workflow: { + graph: rootGraph, + nodesMetadata: { + manual: { graphId: 'root', isRoot: true, isTrigger: true }, + Init_Variable: { graphId: 'root' }, + Init_Parallel: { graphId: 'root' }, + Condition_outer: { graphId: 'root' }, + 'Condition_outer-actions': { graphId: 'Condition_outer', parentNodeId: 'Condition_outer', subgraphType: 'CONDITIONAL_TRUE' }, + For_each: { graphId: 'root' }, + }, + }, + tokens: { + outputTokens: {}, + }, + } as unknown as RootState; +}; + +const nodeMap: Record = { + manual: 'manual', + Init_Variable: 'Init_Variable', + Init_Parallel: 'Init_Parallel', + Condition_outer: 'Condition_outer', + 'Condition_outer-actions': 'Condition_outer-actions', + For_each: 'For_each', +}; + +describe('extendUpstreamNodeIdsForScopePaste', () => { + it('returns only the caller ids when there is no enclosing graph or parent', () => { + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], undefined, undefined, state, nodeMap); + expect(result).toEqual(['Init_Variable']); + }); + + it('surfaces the ancestor variable when pasting into a Condition subgraph', () => { + // Pasting a scope into the True branch of Condition_outer. graphId is the subgraph node. + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste([], 'Condition_outer-actions', 'Condition_outer', state, nodeMap); + expect(result).toContain('Init_Variable'); + expect(result).not.toContain('Init_Parallel'); + }); + + it('surfaces the ancestor variable when pasting into a For each body even if parentId is undefined', () => { + // Regression: pasting a Condition as the first/only action inside a For each. There is no + // predecessor action, so parentId is undefined. The enclosing graphId (For_each) must still + // surface the upstream Init_Variable while excluding the parallel-branch Init_Parallel. + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste([], 'For_each', undefined, state, nodeMap); + expect(result).toContain('Init_Variable'); + expect(result).not.toContain('Init_Parallel'); + }); + + it('deduplicates ids that already appear in the caller-provided upstream list', () => { + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], 'For_each', undefined, state, nodeMap); + expect(result.filter((id) => id === 'Init_Variable')).toHaveLength(1); + }); +}); diff --git a/libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts b/libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts index d9a26671ff6..f5dbf9b1dec 100644 --- a/libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts +++ b/libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts @@ -24,6 +24,8 @@ import type { ActionDefinition } from '@microsoft/logic-apps-shared/src/utils/sr import { initializeDynamicDataInNodes, initializeOperationMetadata } from './operationdeserializer'; import type { NodesMetadata } from '../../state/workflow/workflowInterfaces'; import { updateAllUpstreamNodes } from './initialize'; +import { getUpstreamNodeIds } from '../../utils/graph'; +import type { WorkflowNode } from '../../parsers/models/workflowNode'; import type { NodeTokens } from '../../state/tokens/tokensSlice'; import { addDynamicTokens } from '../../state/tokens/tokensSlice'; import { getConnectionReferenceForNodeId } from '../../state/connection/connectionSelector'; @@ -315,6 +317,7 @@ export const pasteOperation = createAsyncThunk('pasteOperation', async (payload: dispatch(setNodeDescription({ nodeId, description: comment })); } + updateAllUpstreamNodes(getState() as RootState, dispatch); dispatch(setIsPanelLoading(false)); return nodeId; @@ -363,7 +366,13 @@ export const pasteScopeOperation = createAsyncThunk( true /* shouldAppendAddCase */, pasteParams ); - actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: parentId, graphId }; + // parentNodeId must reference a nodesMetadata key (the containing graph/scope) so the + // parent-chain walk in getUpstreamNodeIds can climb to ancestor scopes. `parentId` from + // relationshipIds is an edge-placement node (e.g. a `-#subgraph` card) and is not a + // valid metadata key, so use `graphId` (the containing graph) instead, matching how + // pasteScopeInWorkflow finalizes this node's metadata. + const scopeParentNodeId = graphId && graphId !== 'root' ? graphId : undefined; + actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: scopeParentNodeId, graphId }; if (Object.keys(allConnectionData).length > 0) { dispatch(initScopeCopiedConnections(replaceIdsOfExistingNodes(allConnectionData, pasteParams.renamedNodes))); } @@ -393,7 +402,8 @@ export const pasteScopeOperation = createAsyncThunk( for (const id of Object.keys(operations)) { nodeMap[id] = id; } - const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, upstreamNodeIds), idReplacements); + const extendedIds = extendUpstreamNodeIdsForScopePaste(upstreamNodeIds, graphId, parentId, state, nodeMap); + const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, extendedIds), idReplacements); const triggerId = getTriggerNodeId(state.workflow); await Promise.all([ @@ -468,6 +478,46 @@ const filterRecordByArray = (record: Record, upstreamNodeIds return filteredRecord; }; +/** + * Extends the upstream node id set used to seed a pasted scope's existing output tokens. + * + * When we paste a scope (e.g. a Condition) inside another scope, the `upstreamNodeIds` + * we receive from the caller only covers upstream nodes of the paste-site child/parent + * edge. It does not include the enclosing scope container itself or its ancestor chain, + * so nodes declared alongside (or outside) that container — such as an `InitializeVariable` + * at the workflow root — are absent from the fragment's initial upstream token slice. + * + * We fold in the enclosing scope container (`graphId`) and its full upstream context, plus + * the paste-site predecessor (`parentId`) when present. Relying on `graphId` is essential: + * when a scope is pasted as the first/only action inside a container (e.g. a For each body), + * `parentId` is `undefined` or a non-chaining header node, so extending only via `parentId` + * would miss the ancestor variables. The enclosing `graphId` always resolves to a node whose + * parent chain reaches the workflow root, so its upstream reliably surfaces ancestor tokens. + */ +export const extendUpstreamNodeIdsForScopePaste = ( + upstreamNodeIds: string[], + graphId: string | undefined, + parentId: string | undefined, + state: RootState, + operationMap: Record +): string[] => { + const extended = new Set(upstreamNodeIds); + const rootGraph = state.workflow.graph as WorkflowNode | null; + if (!rootGraph) { + return Array.from(extended); + } + for (const containerId of [graphId, parentId]) { + if (!containerId || containerId === 'root') { + continue; + } + extended.add(containerId); + for (const id of getUpstreamNodeIds(containerId, rootGraph, state.workflow.nodesMetadata, operationMap)) { + extended.add(id); + } + } + return Array.from(extended); +}; + type DuplicateOperationsPayload = { nodeIds: string[]; }; diff --git a/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts b/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts new file mode 100644 index 00000000000..2398369d8a4 --- /dev/null +++ b/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts @@ -0,0 +1,95 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { pasteScopeInWorkflow } from '../pasteScopeInWorkflow'; +import { createWorkflowNode } from '../../utils/graph'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// Reproduces the customer bug: a Condition (containing a Set Variable) is pasted into the else +// branch of an existing Condition. The pasted top node arrives with a placeholder parentNodeId +// that points at the subgraph card (`Condition-elseActions-#subgraph`) — an edge-placement node +// that is NOT a nodesMetadata key. The reducer must overwrite that with the containing graph id +// (`Condition-elseActions`) *in state* so the parent-chain walk can climb to the outer scope and +// surface ancestor-scope variables in the Set Variable dropdown. +describe('pasteScopeInWorkflow parentNodeId persistence', () => { + const buildFixture = () => { + const pastedNodeId = 'Condition_2'; + const graphId = 'Condition-elseActions'; + const subgraphCardId = 'Condition-elseActions-#subgraph'; + + const elseSubgraph = { + id: graphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + + const scopeNode = { + id: pastedNodeId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + + // Metadata handed to the reducer for the pasted subtree. The top node carries the broken + // placeholder parentNodeId (the subgraph card id) that must be corrected in state. + const pasteNodesMetadata = { + [pastedNodeId]: { graphId, parentNodeId: subgraphCardId, isRoot: false }, + } as any; + + const state = { + operations: { + [pastedNodeId]: {}, + }, + nodesMetadata: { + Initialize_variables: { graphId: 'root' }, + Condition: { graphId: 'root' }, + [graphId]: { graphId: 'Condition', parentNodeId: 'Condition', subgraphType: 'CONDITIONAL_FALSE' }, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId, parentId: subgraphCardId, childId: undefined } as any; + + return { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds }; + }; + + it('writes the corrected parentNodeId (the containing graph id) into state', () => { + const { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds } = buildFixture(); + + pasteScopeInWorkflow(scopeNode, elseSubgraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false); + + expect(state.nodesMetadata[pastedNodeId]).toBeDefined(); + expect(state.nodesMetadata[pastedNodeId].graphId).toBe(graphId); + // The placeholder subgraph-card parentNodeId must not survive in state. + expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBe(graphId); + expect(state.nodesMetadata[pastedNodeId].parentNodeId).not.toBe(relationshipIds.parentId); + }); + + it('leaves parentNodeId undefined when pasting at the workflow root', () => { + const pastedNodeId = 'Condition_2'; + const rootGraph = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [], + edges: [], + } as any; + const scopeNode = { + id: pastedNodeId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + const state = { + operations: { [pastedNodeId]: {} }, + nodesMetadata: { Initialize_variables: { graphId: 'root' } }, + newlyAddedOperations: {}, + } as any; + const pasteNodesMetadata = { [pastedNodeId]: { graphId: 'root', parentNodeId: undefined } } as any; + const relationshipIds = { graphId: 'root', parentId: undefined, childId: undefined } as any; + + pasteScopeInWorkflow(scopeNode, rootGraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false); + + expect(state.nodesMetadata[pastedNodeId].graphId).toBe('root'); + expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBeUndefined(); + }); +}); diff --git a/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts b/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts new file mode 100644 index 00000000000..7f73058b89e --- /dev/null +++ b/libs/designer-v2/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts @@ -0,0 +1,490 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { pasteScopeInWorkflow } from '../pasteScopeInWorkflow'; +import type { WorkflowNode } from '../models/workflowNode'; +import { createWorkflowNode, createWorkflowEdge, getUpstreamNodeIds } from '../../utils/graph'; +import { getAvailableVariables } from '../../utils/variables'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// End-to-end scope-correctness matrix for the pasted Set Variable dropdown. +// +// Reproduces the customer's screenshots: a Set Variable is nested inside a copied Condition or +// Scope, and that block is pasted into a Condition, a plain Scope, or a For each. The copied top +// node arrives with a placeholder parentNodeId that points at the paste target's edge-placement +// card (`-#subgraph` / `-#scope`) — NOT a nodesMetadata key. Each test drives the +// real `pasteScopeInWorkflow` reducer (which carries the fix), then resolves the nested Set +// Variable's upstream via `getUpstreamNodeIds` and the visible variables via +// `getAvailableVariables`. +// +// A separate-path `Initialize_variables_1` (varB) guards scope: it must NOT leak into targets that +// are not downstream of it, while the in-path `Initialize_variables` (varA) must always resolve. +// WITHOUT the fix the reducer keeps the card-id parentNodeId, the parent-chain walk dead-ends at +// the pasted top node, and these assertions fail. + +const VAR_A = 'Initialize_variables'; +const VAR_B = 'Initialize_variables_1'; +const VAR_A_NAME = 'varA'; +const VAR_B_NAME = 'varB'; + +const variablesMap = { + [VAR_A]: [{ name: VAR_A_NAME, type: 'string' }], + [VAR_B]: [{ name: VAR_B_NAME, type: 'string' }], +} as any; + +type WrapperKind = 'condition' | 'scope'; +type TargetKind = 'condition' | 'scope' | 'foreach'; + +interface TargetBuild { + targetId: string; + container: WorkflowNode; + workflowGraph: WorkflowNode; + graphId: string; + parentId: string; + targetMetadata: Record; +} + +const buildTarget = (kind: TargetKind): TargetBuild => { + if (kind === 'condition') { + const targetId = 'TargetCondition'; + const subgraphId = `${targetId}-elseActions`; + const subgraphCardId = `${subgraphId}-#subgraph`; + const subgraph: WorkflowNode = { + id: subgraphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + const container: WorkflowNode = { + id: targetId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${targetId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), subgraph], + edges: [], + } as any; + return { + targetId, + container, + workflowGraph: subgraph, + graphId: subgraphId, + parentId: subgraphCardId, + targetMetadata: { + [targetId]: { graphId: 'root', parentNodeId: undefined }, + [subgraphId]: { graphId: targetId, parentNodeId: targetId, subgraphType: 'CONDITIONAL_FALSE' }, + }, + }; + } + + // Plain Scope and For each are both single-flow scopes: the graph node id === the scope action + // name, and children paste directly into that graph node's body. + const targetId = kind === 'scope' ? 'TargetScope' : 'TargetForeach'; + const container: WorkflowNode = { + id: targetId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${targetId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + return { + targetId, + container, + workflowGraph: container, + graphId: targetId, + parentId: `${targetId}-#scope`, + targetMetadata: { [targetId]: { graphId: 'root', parentNodeId: undefined } }, + }; +}; + +interface PasteBuild { + topId: string; + setVarId: string; + scopeNode: WorkflowNode; + pasteNodesMetadata: Record; + operations: Record; +} + +// The pasted subtree always ends in a Set Variable nested inside a Condition or a Scope. The top +// node carries the BROKEN placeholder parentNodeId (the target's card id) exactly as copypaste +// seeds it before the reducer corrects it. Nested children carry correct parentNodeIds relative to +// the pasted top node (as buildGraphFromActions produces them). +const buildPastedSubtree = (wrapper: WrapperKind, graphId: string, brokenParentId: string): PasteBuild => { + const setVarId = 'Pasted_SetVariable'; + + if (wrapper === 'condition') { + const topId = 'Pasted_Condition'; + const subId = `${topId}-actions`; + const subCardId = `${subId}-#subgraph`; + const scopeNode: WorkflowNode = { + id: topId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${topId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: subId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(setVarId)], + edges: [], + } as any, + ], + edges: [], + } as any; + return { + topId, + setVarId, + scopeNode, + pasteNodesMetadata: { + [topId]: { graphId, parentNodeId: brokenParentId, isRoot: false }, + [subId]: { graphId: topId, parentNodeId: topId, subgraphType: 'CONDITIONAL_TRUE' }, + [setVarId]: { graphId: subId, parentNodeId: topId }, + }, + operations: { [topId]: {}, [setVarId]: {} }, + }; + } + + // Scope wrapper: the copied Scope contains a Condition, and the Set Variable lives inside that + // Condition (chain Set Variable -> Condition -> Scope -> target). + const topId = 'Pasted_Scope'; + const innerCondId = 'Pasted_InnerCondition'; + const innerSubId = `${innerCondId}-actions`; + const innerSubCardId = `${innerSubId}-#subgraph`; + const scopeNode: WorkflowNode = { + id: topId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${topId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: innerCondId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${innerCondId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: innerSubId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(innerSubCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(setVarId)], + edges: [], + } as any, + ], + edges: [], + } as any, + ], + edges: [], + } as any; + return { + topId, + setVarId, + scopeNode, + pasteNodesMetadata: { + [topId]: { graphId, parentNodeId: brokenParentId, isRoot: false }, + [innerCondId]: { graphId: topId, parentNodeId: topId }, + [innerSubId]: { graphId: innerCondId, parentNodeId: innerCondId, subgraphType: 'CONDITIONAL_TRUE' }, + [setVarId]: { graphId: innerSubId, parentNodeId: innerCondId }, + }, + operations: { [topId]: {}, [innerCondId]: {}, [setVarId]: {} }, + }; +}; + +const resolveVariableNames = (wrapper: WrapperKind, target: TargetKind, varBInPath: boolean): string[] => { + const targetBuild = buildTarget(target); + const paste = buildPastedSubtree(wrapper, targetBuild.graphId, targetBuild.parentId); + + // Root graph: varA is always an ancestor of the target. varB is either sequentially in-path + // (before the target) or on a parallel branch that never reaches the target. + const rootEdges = varBInPath + ? [createWorkflowEdge(VAR_A, VAR_B), createWorkflowEdge(VAR_B, targetBuild.targetId)] + : [createWorkflowEdge(VAR_A, targetBuild.targetId), createWorkflowEdge(VAR_A, VAR_B)]; + + const root: WorkflowNode = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), targetBuild.container], + edges: rootEdges, + } as any; + + const state = { + operations: { ...paste.operations }, + nodesMetadata: { + [VAR_A]: { graphId: 'root' }, + [VAR_B]: { graphId: 'root' }, + ...targetBuild.targetMetadata, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId: targetBuild.graphId, parentId: targetBuild.parentId, childId: undefined } as any; + + pasteScopeInWorkflow( + paste.scopeNode, + targetBuild.workflowGraph, + relationshipIds, + paste.operations, + paste.pasteNodesMetadata, + [paste.topId, paste.setVarId], + state, + false + ); + + const operationMap: Record = Object.fromEntries( + [VAR_A, VAR_B, targetBuild.targetId, paste.topId, paste.setVarId].map((id) => [id, id]) + ); + + const upstream = getUpstreamNodeIds(paste.setVarId, root, state.nodesMetadata, operationMap); + return getAvailableVariables(variablesMap, upstream).map((variable) => variable.name as string); +}; + +// --------------------------------------------------------------------------------------------- +// Deep multi-level nesting (rev 9) +// +// The customer's screenshots bury the paste target many scope layers deep, e.g. +// Scope -> Scope -> Scope -> Scope -> Condition (pure scope chain) +// Scope -> Condition -> For each -> Condition -> Scope -> For each -> Condition (mixed) +// These builders nest an arbitrary chain of scope kinds (outermost first) and paste the +// Set-Variable block into the innermost layer. The in-path root Initialize Variable (varA) must +// still resolve through the whole chain, while a separate-path Initialize Variable (varB) must +// stay excluded even when it lives on a sibling branch high in the tree. +// --------------------------------------------------------------------------------------------- + +interface NestedBuild { + outerId: string; + outerContainer: WorkflowNode; + targetId: string; + workflowGraph: WorkflowNode; + graphId: string; + parentId: string; + nodesMetadata: Record; +} + +// Builds a nested chain of scope containers. `chain[0]` is nested directly in the root graph and +// `chain[chain.length - 1]` is the innermost paste target. When `siblingVarB` is set and the +// outermost layer is a condition, varB is dropped into that condition's opposite (TRUE) branch so +// the deep chain (in the FALSE branch) must not see it. +const buildNestedTarget = (chain: TargetKind[], siblingVarB = false): NestedBuild => { + const meta: Record = {}; + let outerId = ''; + let outerContainer: WorkflowNode | undefined; + let prevInsert: WorkflowNode[] | null = null; + // graphId / parentNodeId assigned to whatever node is placed at the current insertion point. + let childGraphId = 'root'; + let childParentNodeId: string | undefined; + let innermost: { targetId: string; workflowGraph: WorkflowNode; graphId: string; parentId: string } | undefined; + + chain.forEach((kind, i) => { + const containerId = `Nest${i}`; + const isInnermost = i === chain.length - 1; + meta[containerId] = { graphId: childGraphId, parentNodeId: childParentNodeId }; + + let container: WorkflowNode; + let nextInsert: WorkflowNode[]; + let nextGraphId: string; + let nextParentNodeId: string; + let seed: { workflowGraph: WorkflowNode; graphId: string; parentId: string }; + + if (kind === 'condition') { + const subgraphId = `${containerId}-elseActions`; + const subgraphCardId = `${subgraphId}-#subgraph`; + const subgraph: WorkflowNode = { + id: subgraphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + meta[subgraphId] = { graphId: containerId, parentNodeId: containerId, subgraphType: 'CONDITIONAL_FALSE' }; + const children: WorkflowNode[] = [createWorkflowNode(`${containerId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), subgraph]; + + if (i === 0 && siblingVarB) { + const trueSubId = `${containerId}-actions`; + const trueSubCardId = `${trueSubId}-#subgraph`; + const trueSub: WorkflowNode = { + id: trueSubId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(trueSubCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(VAR_B)], + edges: [], + } as any; + meta[trueSubId] = { graphId: containerId, parentNodeId: containerId, subgraphType: 'CONDITIONAL_TRUE' }; + meta[VAR_B] = { graphId: trueSubId, parentNodeId: containerId }; + children.push(trueSub); + } + + container = { id: containerId, type: WORKFLOW_NODE_TYPES.GRAPH_NODE, children, edges: [] } as any; + nextInsert = subgraph.children as WorkflowNode[]; + nextGraphId = subgraphId; + nextParentNodeId = containerId; + seed = { workflowGraph: subgraph, graphId: subgraphId, parentId: subgraphCardId }; + } else { + // Plain Scope and For each are single-flow graphs: children nest directly in the body. + container = { + id: containerId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${containerId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + nextInsert = container.children as WorkflowNode[]; + nextGraphId = containerId; + nextParentNodeId = containerId; + seed = { workflowGraph: container, graphId: containerId, parentId: `${containerId}-#scope` }; + } + + if (i === 0) { + outerId = containerId; + outerContainer = container; + } else { + (prevInsert as WorkflowNode[]).push(container); + } + prevInsert = nextInsert; + childGraphId = nextGraphId; + childParentNodeId = nextParentNodeId; + if (isInnermost) { + innermost = { targetId: containerId, ...seed }; + } + }); + + return { + outerId, + outerContainer: outerContainer as WorkflowNode, + targetId: (innermost as any).targetId, + workflowGraph: (innermost as any).workflowGraph, + graphId: (innermost as any).graphId, + parentId: (innermost as any).parentId, + nodesMetadata: meta, + }; +}; + +type VarBMode = 'parallel' | 'inpath' | 'sibling'; + +const resolveVariableNamesNested = (chain: TargetKind[], wrapper: WrapperKind, varBMode: VarBMode): string[] => { + const nested = buildNestedTarget(chain, varBMode === 'sibling'); + const paste = buildPastedSubtree(wrapper, nested.graphId, nested.parentId); + + let rootChildren: WorkflowNode[]; + let rootEdges: any[]; + const varAMeta = { [VAR_A]: { graphId: 'root' } } as Record; + const rootVarBMeta = varBMode === 'sibling' ? {} : { [VAR_B]: { graphId: 'root' } }; + + if (varBMode === 'inpath') { + rootChildren = [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, VAR_B), createWorkflowEdge(VAR_B, nested.outerId)]; + } else if (varBMode === 'sibling') { + // varB lives inside the outermost condition's opposite branch (built by buildNestedTarget). + rootChildren = [createWorkflowNode(VAR_A), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, nested.outerId)]; + } else { + rootChildren = [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, nested.outerId), createWorkflowEdge(VAR_A, VAR_B)]; + } + + const root: WorkflowNode = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: rootChildren, + edges: rootEdges, + } as any; + + const state = { + operations: { ...paste.operations }, + nodesMetadata: { + ...varAMeta, + ...rootVarBMeta, + ...nested.nodesMetadata, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId: nested.graphId, parentId: nested.parentId, childId: undefined } as any; + + pasteScopeInWorkflow( + paste.scopeNode, + nested.workflowGraph, + relationshipIds, + paste.operations, + paste.pasteNodesMetadata, + [paste.topId, paste.setVarId], + state, + false + ); + + const operationMap: Record = Object.fromEntries( + [VAR_A, VAR_B, paste.topId, paste.setVarId, ...chain.map((_, i) => `Nest${i}`)].map((id) => [id, id]) + ); + + const upstream = getUpstreamNodeIds(paste.setVarId, root, state.nodesMetadata, operationMap); + return getAvailableVariables(variablesMap, upstream).map((variable) => variable.name as string); +}; + +const wrappers: WrapperKind[] = ['condition', 'scope']; +const targets: TargetKind[] = ['condition', 'scope', 'foreach']; + +describe('pasteScopeInWorkflow variable-dropdown scope correctness (paste matrix)', () => { + describe('excludes a separate-path Initialize Variable', () => { + for (const wrapper of wrappers) { + for (const target of targets) { + it(`Set Variable in a copied ${wrapper} pasted into a ${target} sees only in-path variables`, () => { + const names = resolveVariableNames(wrapper, target, false); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + } + } + }); + + describe('includes every Initialize Variable that is genuinely upstream', () => { + it('Set Variable in a copied condition pasted into a condition downstream of both vars sees both', () => { + const names = resolveVariableNames('condition', 'condition', true); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + + it('Set Variable in a copied scope pasted into a for each downstream of both vars sees both', () => { + const names = resolveVariableNames('scope', 'foreach', true); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + }); +}); + +describe('pasteScopeInWorkflow variable-dropdown scope correctness (deep nesting)', () => { + describe('resolves the in-path root variable and excludes a separate-path variable at depth', () => { + const deepChains: { name: string; chain: TargetKind[] }[] = [ + { + name: 'N1 Scope -> Scope -> Scope -> Scope -> Condition (pure scope depth 5)', + chain: ['scope', 'scope', 'scope', 'scope', 'condition'], + }, + { + name: 'N2 Condition -> Condition -> Condition -> Condition (pure subgraph chain depth 4)', + chain: ['condition', 'condition', 'condition', 'condition'], + }, + { + name: 'N3 Scope -> Condition -> For each -> Condition -> Scope -> For each -> Condition (mixed depth 7)', + chain: ['scope', 'condition', 'foreach', 'condition', 'scope', 'foreach', 'condition'], + }, + { name: 'N4 Condition -> Scope -> For each -> Condition (mixed depth 4)', chain: ['condition', 'scope', 'foreach', 'condition'] }, + ]; + + for (const { name, chain } of deepChains) { + it(`${name} sees only the in-path variable`, () => { + const names = resolveVariableNamesNested(chain, 'condition', 'parallel'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + } + + it('N5 deep chain in a condition True branch excludes a variable initialized in the sibling False branch', () => { + // Outermost condition: main chain nests in one branch; Initialize_variables_1 (varB) lives in + // the opposite branch. The pasted Set Variable deep in the chain must not see it. + const names = resolveVariableNamesNested(['condition', 'scope', 'foreach', 'condition'], 'condition', 'sibling'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + }); + + describe('still includes every variable that is genuinely upstream at depth', () => { + it('N6 deep chain downstream of both root variables sees both', () => { + const names = resolveVariableNamesNested(['scope', 'scope', 'condition'], 'condition', 'inpath'); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + }); + + describe('scope-wrapped paste blocks resolve at depth', () => { + it('N7 a copied Scope>Condition>Set Variable pasted into a deeply nested For each sees only the in-path variable', () => { + const names = resolveVariableNamesNested(['scope', 'condition', 'foreach'], 'scope', 'parallel'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + }); +}); diff --git a/libs/designer-v2/src/lib/core/parsers/pasteScopeInWorkflow.ts b/libs/designer-v2/src/lib/core/parsers/pasteScopeInWorkflow.ts index b569ea0048b..38ffb096d5e 100644 --- a/libs/designer-v2/src/lib/core/parsers/pasteScopeInWorkflow.ts +++ b/libs/designer-v2/src/lib/core/parsers/pasteScopeInWorkflow.ts @@ -54,6 +54,13 @@ export const pasteScopeInWorkflow = ( if (getRecordEntry(nodesMetadata, nodeId)?.isRoot === false) { delete nodesMetadata[nodeId].isRoot; } + // The loop above copied the pasted node's metadata into state by reference before this + // correction ran, so the recomputed graphId/parentNodeId must be written back to state. + // Otherwise state keeps the placeholder parentNodeId seeded during paste (an edge-placement + // card id such as `-#subgraph`), which is not a nodesMetadata key. That breaks the + // parent-chain walk in getUpstreamNodeIds and leaves scoped dropdowns (e.g. Set Variable) + // unable to see variables initialized in an ancestor scope. + state.nodesMetadata[nodeId] = nodesMetadata[nodeId]; const parentMetadata = getRecordEntry(state.nodesMetadata, parentId); const isAfterTrigger = (parentMetadata?.isRoot && newGraphId === 'root') ?? false; diff --git a/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts b/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts index 24f39d135cf..6c6a5e7b319 100644 --- a/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts +++ b/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import { getEditorAndOptions } from '..'; import type { VariableDeclaration } from '../../../../../../core/state/tokens/tokensSlice'; import { InitEditorService } from '@microsoft/logic-apps-shared'; @@ -108,7 +109,7 @@ describe('getEditorAndOptions', () => { expect(getEditorAndOptions(operationInfo, parameter, upstreamNodeIds, variables)).toEqual(expectedEditorAndOptions); }); - describe('"variablename" editor fallback when scoped list is empty', () => { + describe('"variablename" editor strictly scopes to upstream variables', () => { const variables: Record = { initInt: [{ name: 'intVar', type: 'integer' }], initFloat: [{ name: 'floatVar', type: 'float' }], @@ -117,30 +118,18 @@ describe('getEditorAndOptions', () => { initBool: [{ name: 'boolVar', type: 'boolean' }], }; - // These supportedTypes values mirror the 5 built-in operations that use the - // 'variablename' editor (see libs/logic-apps-shared/.../manifests/variables.ts): - // setVariable (any), incrementVariable/decrementVariable (float,integer), - // appendToStringVariable (string), appendToArrayVariable (array). - it.each` - operation | supportedTypes | expectedNames - ${'setVariable'} | ${[]} | ${['intVar', 'floatVar', 'strVar', 'arrVar', 'boolVar']} - ${'incrementVariable'} | ${['float', 'integer']} | ${['intVar', 'floatVar']} - ${'decrementVariable'} | ${['float', 'integer']} | ${['intVar', 'floatVar']} - ${'appendToStringVariable'} | ${['string']} | ${['strVar']} - ${'appendToArrayVariable'} | ${['array']} | ${['arrVar']} - `( - 'falls back to all declared variables (respecting supportedTypes) for $operation when upstreamNodeIds is empty', - ({ supportedTypes, expectedNames }) => { - const parameter = getParameterInfo(); - parameter.editor = 'variablename'; - parameter.editorOptions = { supportedTypes }; - - const result = getEditorAndOptions(operationInfo, parameter, /* upstreamNodeIds */ [], variables); - - expect(result.editor).toBe('dropdown'); - expect((result.editorOptions?.options ?? []).map((o: any) => o.value)).toEqual(expectedNames); - } - ); + it('returns an empty options list when no variables are upstream (no all-variables leak)', () => { + const parameter = getParameterInfo(); + parameter.editor = 'variablename'; + parameter.editorOptions = { supportedTypes: [] }; + + // upstreamNodeIds is empty even though variables are declared elsewhere in the workflow. + // Showing all of them would let a Set Variable reference an out-of-scope variable, so the + // dropdown must be empty. + const result = getEditorAndOptions(operationInfo, parameter, /* upstreamNodeIds */ [], variables); + + expect(result).toEqual({ editor: 'dropdown', editorOptions: { options: [] } }); + }); it('returns an empty options list when no variables are declared at all', () => { const parameter = getParameterInfo(); @@ -152,13 +141,12 @@ describe('getEditorAndOptions', () => { expect(result).toEqual({ editor: 'dropdown', editorOptions: { options: [] } }); }); - it('does not fall back when the scoped list already has matches (no regression)', () => { + it('only includes variables whose declaring node is upstream, excluding parallel-branch variables', () => { const parameter = getParameterInfo(); parameter.editor = 'variablename'; parameter.editorOptions = { supportedTypes: ['string'] }; - // upstream includes initStr (matches) but excludes initArr — the fallback must NOT - // add arrVar because scopedOptions is non-empty. + // upstream includes initStr (in scope) but excludes initArr/others (parallel branch). const result = getEditorAndOptions(operationInfo, parameter, ['initStr'], variables); expect(result).toEqual({ diff --git a/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx b/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx index 79e177d7ea3..07a358bd357 100644 --- a/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx +++ b/libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx @@ -46,7 +46,7 @@ import { } from '../../../../../core/utils/parameters/helper'; import type { TokenGroup } from '../../../../../core/utils/tokens'; import { createValueSegmentFromToken, getExpressionTokenSections, getOutputTokenSections } from '../../../../../core/utils/tokens'; -import { getAllVariables, getAvailableVariables } from '../../../../../core/utils/variables'; +import { getAvailableVariables } from '../../../../../core/utils/variables'; import { SettingsSection } from '../../../../settings/settingsection'; import type { Settings } from '../../../../settings/settingsection'; import { ConnectionDisplay } from './connectionDisplay'; @@ -1843,17 +1843,12 @@ export const getEditorAndOptions = ( // Handle variable dropdown editor if (equals(editor, constants.EDITOR.VARIABLE_NAME)) { - const toOption = (variable: VariableDeclaration) => ({ value: variable.name, displayName: variable.name }); - const typeMatches = (variable: VariableDeclaration) => supportedTypes.length === 0 || supportedTypes.includes(variable.type); - - const scopedOptions = getAvailableVariables(variables, upstreamNodeIds).filter(typeMatches).map(toOption); - - // Fallback: when the scoped list is empty but variables are declared elsewhere in the workflow, - // show all declared variables. The runtime does not scope variables by graph position, so any - // declared variable is legally assignable (see: pasteScopeOperation bug where a pasted node's - // upstreamNodeIds does not include the root InitializeVariable). Preserves the ideal UX when - // the scoped list is non-empty; only kicks in to prevent an empty-dropdown dead-end. - const options = scopedOptions.length === 0 ? getAllVariables(variables).filter(typeMatches).map(toOption) : scopedOptions; + const options = getAvailableVariables(variables, upstreamNodeIds) + .filter((variable) => supportedTypes.length === 0 || supportedTypes.includes(variable.type)) + .map((variable) => ({ + value: variable.name, + displayName: variable.name, + })); return { editor: 'dropdown', diff --git a/libs/designer/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts b/libs/designer/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts new file mode 100644 index 00000000000..405e392a21c --- /dev/null +++ b/libs/designer/src/lib/core/actions/bjsworkflow/__test__/copypaste.spec.ts @@ -0,0 +1,110 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { extendUpstreamNodeIdsForScopePaste } from '../copypaste'; +import type { RootState } from '../../..'; +import { createWorkflowEdge, createWorkflowNode } from '../../../utils/graph'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// Graph shape: +// root +// manual (trigger) +// Init_Variable (source node at root, before the parallel split) +// Init_Parallel (parallel branch A — NOT upstream of branches B/C) +// Condition_outer (parallel branch B; scope with a True subgraph — paste site) +// └── Condition_outer-actions (subgraph paste site inside the condition) +// For_each (parallel branch C; loop with an empty body — paste site) +const buildState = (): RootState => { + const rootGraph = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode('manual'), + createWorkflowNode('Init_Variable'), + createWorkflowNode('Init_Parallel'), + { + id: 'Condition_outer', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode('Condition_outer-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: 'Condition_outer-actions', + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode('Condition_outer-actions-#subgraph', WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + }, + ], + edges: [createWorkflowEdge('Condition_outer-#scope', 'Condition_outer-actions')], + }, + { + id: 'For_each', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode('For_each-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + }, + ], + edges: [ + createWorkflowEdge('manual', 'Init_Variable'), + createWorkflowEdge('Init_Variable', 'Init_Parallel'), + createWorkflowEdge('Init_Variable', 'Condition_outer'), + createWorkflowEdge('Init_Variable', 'For_each'), + ], + } as any; + + return { + workflow: { + graph: rootGraph, + nodesMetadata: { + manual: { graphId: 'root', isRoot: true, isTrigger: true }, + Init_Variable: { graphId: 'root' }, + Init_Parallel: { graphId: 'root' }, + Condition_outer: { graphId: 'root' }, + 'Condition_outer-actions': { graphId: 'Condition_outer', parentNodeId: 'Condition_outer', subgraphType: 'CONDITIONAL_TRUE' }, + For_each: { graphId: 'root' }, + }, + }, + tokens: { + outputTokens: {}, + }, + } as unknown as RootState; +}; + +const nodeMap: Record = { + manual: 'manual', + Init_Variable: 'Init_Variable', + Init_Parallel: 'Init_Parallel', + Condition_outer: 'Condition_outer', + 'Condition_outer-actions': 'Condition_outer-actions', + For_each: 'For_each', +}; + +describe('extendUpstreamNodeIdsForScopePaste', () => { + it('returns only the caller ids when there is no enclosing graph or parent', () => { + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], undefined, undefined, state, nodeMap); + expect(result).toEqual(['Init_Variable']); + }); + + it('surfaces the ancestor variable when pasting into a Condition subgraph', () => { + // Pasting a scope into the True branch of Condition_outer. graphId is the subgraph node. + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste([], 'Condition_outer-actions', 'Condition_outer', state, nodeMap); + expect(result).toContain('Init_Variable'); + expect(result).not.toContain('Init_Parallel'); + }); + + it('surfaces the ancestor variable when pasting into a For each body even if parentId is undefined', () => { + // Regression: pasting a Condition as the first/only action inside a For each. There is no + // predecessor action, so parentId is undefined. The enclosing graphId (For_each) must still + // surface the upstream Init_Variable while excluding the parallel-branch Init_Parallel. + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste([], 'For_each', undefined, state, nodeMap); + expect(result).toContain('Init_Variable'); + expect(result).not.toContain('Init_Parallel'); + }); + + it('deduplicates ids that already appear in the caller-provided upstream list', () => { + const state = buildState(); + const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], 'For_each', undefined, state, nodeMap); + expect(result.filter((id) => id === 'Init_Variable')).toHaveLength(1); + }); +}); diff --git a/libs/designer/src/lib/core/actions/bjsworkflow/copypaste.ts b/libs/designer/src/lib/core/actions/bjsworkflow/copypaste.ts index de2f9064037..12022d240c8 100644 --- a/libs/designer/src/lib/core/actions/bjsworkflow/copypaste.ts +++ b/libs/designer/src/lib/core/actions/bjsworkflow/copypaste.ts @@ -17,6 +17,8 @@ import type { ActionDefinition } from '@microsoft/logic-apps-shared/src/utils/sr import { initializeDynamicDataInNodes, initializeOperationMetadata } from './operationdeserializer'; import type { NodesMetadata } from '../../state/workflow/workflowInterfaces'; import { updateAllUpstreamNodes } from './initialize'; +import { getUpstreamNodeIds } from '../../utils/graph'; +import type { WorkflowNode } from '../../parsers/models/workflowNode'; import type { NodeTokens } from '../../state/tokens/tokensSlice'; import { addDynamicTokens } from '../../state/tokens/tokensSlice'; import { getConnectionReferenceForNodeId } from '../../state/connection/connectionSelector'; @@ -169,6 +171,7 @@ export const pasteOperation = createAsyncThunk('pasteOperation', async (payload: dispatch(setNodeDescription({ nodeId, description: comment })); } + updateAllUpstreamNodes(getState() as RootState, dispatch); dispatch(setIsPanelLoading(false)); }); @@ -215,7 +218,13 @@ export const pasteScopeOperation = createAsyncThunk( true /* shouldAppendAddCase */, pasteParams ); - actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: parentId, graphId }; + // parentNodeId must reference a nodesMetadata key (the containing graph/scope) so the + // parent-chain walk in getUpstreamNodeIds can climb to ancestor scopes. `parentId` from + // relationshipIds is an edge-placement node (e.g. a `-#subgraph` card) and is not a + // valid metadata key, so use `graphId` (the containing graph) instead, matching how + // pasteScopeInWorkflow finalizes this node's metadata. + const scopeParentNodeId = graphId && graphId !== 'root' ? graphId : undefined; + actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: scopeParentNodeId, graphId }; if (Object.keys(allConnectionData).length > 0) { dispatch(initScopeCopiedConnections(replaceIdsOfExistingNodes(allConnectionData, pasteParams.renamedNodes))); } @@ -245,7 +254,8 @@ export const pasteScopeOperation = createAsyncThunk( for (const id of Object.keys(operations)) { nodeMap[id] = id; } - const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, upstreamNodeIds), idReplacements); + const extendedIds = extendUpstreamNodeIdsForScopePaste(upstreamNodeIds, graphId, parentId, state, nodeMap); + const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, extendedIds), idReplacements); const triggerId = getTriggerNodeId(state.workflow); await Promise.all([ @@ -317,3 +327,43 @@ const filterRecordByArray = (record: Record, upstreamNodeIds } return filteredRecord; }; + +/** + * Extends the upstream node id set used to seed a pasted scope's existing output tokens. + * + * When we paste a scope (e.g. a Condition) inside another scope, the `upstreamNodeIds` + * we receive from the caller only covers upstream nodes of the paste-site child/parent + * edge. It does not include the enclosing scope container itself or its ancestor chain, + * so nodes declared alongside (or outside) that container — such as an `InitializeVariable` + * at the workflow root — are absent from the fragment's initial upstream token slice. + * + * We fold in the enclosing scope container (`graphId`) and its full upstream context, plus + * the paste-site predecessor (`parentId`) when present. Relying on `graphId` is essential: + * when a scope is pasted as the first/only action inside a container (e.g. a For each body), + * `parentId` is `undefined` or a non-chaining header node, so extending only via `parentId` + * would miss the ancestor variables. The enclosing `graphId` always resolves to a node whose + * parent chain reaches the workflow root, so its upstream reliably surfaces ancestor tokens. + */ +export const extendUpstreamNodeIdsForScopePaste = ( + upstreamNodeIds: string[], + graphId: string | undefined, + parentId: string | undefined, + state: RootState, + operationMap: Record +): string[] => { + const extended = new Set(upstreamNodeIds); + const rootGraph = state.workflow.graph as WorkflowNode | null; + if (!rootGraph) { + return Array.from(extended); + } + for (const containerId of [graphId, parentId]) { + if (!containerId || containerId === 'root') { + continue; + } + extended.add(containerId); + for (const id of getUpstreamNodeIds(containerId, rootGraph, state.workflow.nodesMetadata, operationMap)) { + extended.add(id); + } + } + return Array.from(extended); +}; diff --git a/libs/designer/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts b/libs/designer/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts new file mode 100644 index 00000000000..2398369d8a4 --- /dev/null +++ b/libs/designer/src/lib/core/parsers/__test__/pasteScopeInWorkflow.spec.ts @@ -0,0 +1,95 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { pasteScopeInWorkflow } from '../pasteScopeInWorkflow'; +import { createWorkflowNode } from '../../utils/graph'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// Reproduces the customer bug: a Condition (containing a Set Variable) is pasted into the else +// branch of an existing Condition. The pasted top node arrives with a placeholder parentNodeId +// that points at the subgraph card (`Condition-elseActions-#subgraph`) — an edge-placement node +// that is NOT a nodesMetadata key. The reducer must overwrite that with the containing graph id +// (`Condition-elseActions`) *in state* so the parent-chain walk can climb to the outer scope and +// surface ancestor-scope variables in the Set Variable dropdown. +describe('pasteScopeInWorkflow parentNodeId persistence', () => { + const buildFixture = () => { + const pastedNodeId = 'Condition_2'; + const graphId = 'Condition-elseActions'; + const subgraphCardId = 'Condition-elseActions-#subgraph'; + + const elseSubgraph = { + id: graphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + + const scopeNode = { + id: pastedNodeId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + + // Metadata handed to the reducer for the pasted subtree. The top node carries the broken + // placeholder parentNodeId (the subgraph card id) that must be corrected in state. + const pasteNodesMetadata = { + [pastedNodeId]: { graphId, parentNodeId: subgraphCardId, isRoot: false }, + } as any; + + const state = { + operations: { + [pastedNodeId]: {}, + }, + nodesMetadata: { + Initialize_variables: { graphId: 'root' }, + Condition: { graphId: 'root' }, + [graphId]: { graphId: 'Condition', parentNodeId: 'Condition', subgraphType: 'CONDITIONAL_FALSE' }, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId, parentId: subgraphCardId, childId: undefined } as any; + + return { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds }; + }; + + it('writes the corrected parentNodeId (the containing graph id) into state', () => { + const { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds } = buildFixture(); + + pasteScopeInWorkflow(scopeNode, elseSubgraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false); + + expect(state.nodesMetadata[pastedNodeId]).toBeDefined(); + expect(state.nodesMetadata[pastedNodeId].graphId).toBe(graphId); + // The placeholder subgraph-card parentNodeId must not survive in state. + expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBe(graphId); + expect(state.nodesMetadata[pastedNodeId].parentNodeId).not.toBe(relationshipIds.parentId); + }); + + it('leaves parentNodeId undefined when pasting at the workflow root', () => { + const pastedNodeId = 'Condition_2'; + const rootGraph = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [], + edges: [], + } as any; + const scopeNode = { + id: pastedNodeId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + const state = { + operations: { [pastedNodeId]: {} }, + nodesMetadata: { Initialize_variables: { graphId: 'root' } }, + newlyAddedOperations: {}, + } as any; + const pasteNodesMetadata = { [pastedNodeId]: { graphId: 'root', parentNodeId: undefined } } as any; + const relationshipIds = { graphId: 'root', parentId: undefined, childId: undefined } as any; + + pasteScopeInWorkflow(scopeNode, rootGraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false); + + expect(state.nodesMetadata[pastedNodeId].graphId).toBe('root'); + expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBeUndefined(); + }); +}); diff --git a/libs/designer/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts b/libs/designer/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts new file mode 100644 index 00000000000..7f73058b89e --- /dev/null +++ b/libs/designer/src/lib/core/parsers/__test__/pasteScopeMatrix.spec.ts @@ -0,0 +1,490 @@ +// @vitest-environment jsdom +import { describe, it, expect } from 'vitest'; +import { pasteScopeInWorkflow } from '../pasteScopeInWorkflow'; +import type { WorkflowNode } from '../models/workflowNode'; +import { createWorkflowNode, createWorkflowEdge, getUpstreamNodeIds } from '../../utils/graph'; +import { getAvailableVariables } from '../../utils/variables'; +import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared'; + +// End-to-end scope-correctness matrix for the pasted Set Variable dropdown. +// +// Reproduces the customer's screenshots: a Set Variable is nested inside a copied Condition or +// Scope, and that block is pasted into a Condition, a plain Scope, or a For each. The copied top +// node arrives with a placeholder parentNodeId that points at the paste target's edge-placement +// card (`-#subgraph` / `-#scope`) — NOT a nodesMetadata key. Each test drives the +// real `pasteScopeInWorkflow` reducer (which carries the fix), then resolves the nested Set +// Variable's upstream via `getUpstreamNodeIds` and the visible variables via +// `getAvailableVariables`. +// +// A separate-path `Initialize_variables_1` (varB) guards scope: it must NOT leak into targets that +// are not downstream of it, while the in-path `Initialize_variables` (varA) must always resolve. +// WITHOUT the fix the reducer keeps the card-id parentNodeId, the parent-chain walk dead-ends at +// the pasted top node, and these assertions fail. + +const VAR_A = 'Initialize_variables'; +const VAR_B = 'Initialize_variables_1'; +const VAR_A_NAME = 'varA'; +const VAR_B_NAME = 'varB'; + +const variablesMap = { + [VAR_A]: [{ name: VAR_A_NAME, type: 'string' }], + [VAR_B]: [{ name: VAR_B_NAME, type: 'string' }], +} as any; + +type WrapperKind = 'condition' | 'scope'; +type TargetKind = 'condition' | 'scope' | 'foreach'; + +interface TargetBuild { + targetId: string; + container: WorkflowNode; + workflowGraph: WorkflowNode; + graphId: string; + parentId: string; + targetMetadata: Record; +} + +const buildTarget = (kind: TargetKind): TargetBuild => { + if (kind === 'condition') { + const targetId = 'TargetCondition'; + const subgraphId = `${targetId}-elseActions`; + const subgraphCardId = `${subgraphId}-#subgraph`; + const subgraph: WorkflowNode = { + id: subgraphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + const container: WorkflowNode = { + id: targetId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${targetId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), subgraph], + edges: [], + } as any; + return { + targetId, + container, + workflowGraph: subgraph, + graphId: subgraphId, + parentId: subgraphCardId, + targetMetadata: { + [targetId]: { graphId: 'root', parentNodeId: undefined }, + [subgraphId]: { graphId: targetId, parentNodeId: targetId, subgraphType: 'CONDITIONAL_FALSE' }, + }, + }; + } + + // Plain Scope and For each are both single-flow scopes: the graph node id === the scope action + // name, and children paste directly into that graph node's body. + const targetId = kind === 'scope' ? 'TargetScope' : 'TargetForeach'; + const container: WorkflowNode = { + id: targetId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${targetId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + return { + targetId, + container, + workflowGraph: container, + graphId: targetId, + parentId: `${targetId}-#scope`, + targetMetadata: { [targetId]: { graphId: 'root', parentNodeId: undefined } }, + }; +}; + +interface PasteBuild { + topId: string; + setVarId: string; + scopeNode: WorkflowNode; + pasteNodesMetadata: Record; + operations: Record; +} + +// The pasted subtree always ends in a Set Variable nested inside a Condition or a Scope. The top +// node carries the BROKEN placeholder parentNodeId (the target's card id) exactly as copypaste +// seeds it before the reducer corrects it. Nested children carry correct parentNodeIds relative to +// the pasted top node (as buildGraphFromActions produces them). +const buildPastedSubtree = (wrapper: WrapperKind, graphId: string, brokenParentId: string): PasteBuild => { + const setVarId = 'Pasted_SetVariable'; + + if (wrapper === 'condition') { + const topId = 'Pasted_Condition'; + const subId = `${topId}-actions`; + const subCardId = `${subId}-#subgraph`; + const scopeNode: WorkflowNode = { + id: topId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${topId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: subId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(setVarId)], + edges: [], + } as any, + ], + edges: [], + } as any; + return { + topId, + setVarId, + scopeNode, + pasteNodesMetadata: { + [topId]: { graphId, parentNodeId: brokenParentId, isRoot: false }, + [subId]: { graphId: topId, parentNodeId: topId, subgraphType: 'CONDITIONAL_TRUE' }, + [setVarId]: { graphId: subId, parentNodeId: topId }, + }, + operations: { [topId]: {}, [setVarId]: {} }, + }; + } + + // Scope wrapper: the copied Scope contains a Condition, and the Set Variable lives inside that + // Condition (chain Set Variable -> Condition -> Scope -> target). + const topId = 'Pasted_Scope'; + const innerCondId = 'Pasted_InnerCondition'; + const innerSubId = `${innerCondId}-actions`; + const innerSubCardId = `${innerSubId}-#subgraph`; + const scopeNode: WorkflowNode = { + id: topId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${topId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: innerCondId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [ + createWorkflowNode(`${innerCondId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), + { + id: innerSubId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(innerSubCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(setVarId)], + edges: [], + } as any, + ], + edges: [], + } as any, + ], + edges: [], + } as any; + return { + topId, + setVarId, + scopeNode, + pasteNodesMetadata: { + [topId]: { graphId, parentNodeId: brokenParentId, isRoot: false }, + [innerCondId]: { graphId: topId, parentNodeId: topId }, + [innerSubId]: { graphId: innerCondId, parentNodeId: innerCondId, subgraphType: 'CONDITIONAL_TRUE' }, + [setVarId]: { graphId: innerSubId, parentNodeId: innerCondId }, + }, + operations: { [topId]: {}, [innerCondId]: {}, [setVarId]: {} }, + }; +}; + +const resolveVariableNames = (wrapper: WrapperKind, target: TargetKind, varBInPath: boolean): string[] => { + const targetBuild = buildTarget(target); + const paste = buildPastedSubtree(wrapper, targetBuild.graphId, targetBuild.parentId); + + // Root graph: varA is always an ancestor of the target. varB is either sequentially in-path + // (before the target) or on a parallel branch that never reaches the target. + const rootEdges = varBInPath + ? [createWorkflowEdge(VAR_A, VAR_B), createWorkflowEdge(VAR_B, targetBuild.targetId)] + : [createWorkflowEdge(VAR_A, targetBuild.targetId), createWorkflowEdge(VAR_A, VAR_B)]; + + const root: WorkflowNode = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), targetBuild.container], + edges: rootEdges, + } as any; + + const state = { + operations: { ...paste.operations }, + nodesMetadata: { + [VAR_A]: { graphId: 'root' }, + [VAR_B]: { graphId: 'root' }, + ...targetBuild.targetMetadata, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId: targetBuild.graphId, parentId: targetBuild.parentId, childId: undefined } as any; + + pasteScopeInWorkflow( + paste.scopeNode, + targetBuild.workflowGraph, + relationshipIds, + paste.operations, + paste.pasteNodesMetadata, + [paste.topId, paste.setVarId], + state, + false + ); + + const operationMap: Record = Object.fromEntries( + [VAR_A, VAR_B, targetBuild.targetId, paste.topId, paste.setVarId].map((id) => [id, id]) + ); + + const upstream = getUpstreamNodeIds(paste.setVarId, root, state.nodesMetadata, operationMap); + return getAvailableVariables(variablesMap, upstream).map((variable) => variable.name as string); +}; + +// --------------------------------------------------------------------------------------------- +// Deep multi-level nesting (rev 9) +// +// The customer's screenshots bury the paste target many scope layers deep, e.g. +// Scope -> Scope -> Scope -> Scope -> Condition (pure scope chain) +// Scope -> Condition -> For each -> Condition -> Scope -> For each -> Condition (mixed) +// These builders nest an arbitrary chain of scope kinds (outermost first) and paste the +// Set-Variable block into the innermost layer. The in-path root Initialize Variable (varA) must +// still resolve through the whole chain, while a separate-path Initialize Variable (varB) must +// stay excluded even when it lives on a sibling branch high in the tree. +// --------------------------------------------------------------------------------------------- + +interface NestedBuild { + outerId: string; + outerContainer: WorkflowNode; + targetId: string; + workflowGraph: WorkflowNode; + graphId: string; + parentId: string; + nodesMetadata: Record; +} + +// Builds a nested chain of scope containers. `chain[0]` is nested directly in the root graph and +// `chain[chain.length - 1]` is the innermost paste target. When `siblingVarB` is set and the +// outermost layer is a condition, varB is dropped into that condition's opposite (TRUE) branch so +// the deep chain (in the FALSE branch) must not see it. +const buildNestedTarget = (chain: TargetKind[], siblingVarB = false): NestedBuild => { + const meta: Record = {}; + let outerId = ''; + let outerContainer: WorkflowNode | undefined; + let prevInsert: WorkflowNode[] | null = null; + // graphId / parentNodeId assigned to whatever node is placed at the current insertion point. + let childGraphId = 'root'; + let childParentNodeId: string | undefined; + let innermost: { targetId: string; workflowGraph: WorkflowNode; graphId: string; parentId: string } | undefined; + + chain.forEach((kind, i) => { + const containerId = `Nest${i}`; + const isInnermost = i === chain.length - 1; + meta[containerId] = { graphId: childGraphId, parentNodeId: childParentNodeId }; + + let container: WorkflowNode; + let nextInsert: WorkflowNode[]; + let nextGraphId: string; + let nextParentNodeId: string; + let seed: { workflowGraph: WorkflowNode; graphId: string; parentId: string }; + + if (kind === 'condition') { + const subgraphId = `${containerId}-elseActions`; + const subgraphCardId = `${subgraphId}-#subgraph`; + const subgraph: WorkflowNode = { + id: subgraphId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)], + edges: [], + } as any; + meta[subgraphId] = { graphId: containerId, parentNodeId: containerId, subgraphType: 'CONDITIONAL_FALSE' }; + const children: WorkflowNode[] = [createWorkflowNode(`${containerId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE), subgraph]; + + if (i === 0 && siblingVarB) { + const trueSubId = `${containerId}-actions`; + const trueSubCardId = `${trueSubId}-#subgraph`; + const trueSub: WorkflowNode = { + id: trueSubId, + type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE, + children: [createWorkflowNode(trueSubCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE), createWorkflowNode(VAR_B)], + edges: [], + } as any; + meta[trueSubId] = { graphId: containerId, parentNodeId: containerId, subgraphType: 'CONDITIONAL_TRUE' }; + meta[VAR_B] = { graphId: trueSubId, parentNodeId: containerId }; + children.push(trueSub); + } + + container = { id: containerId, type: WORKFLOW_NODE_TYPES.GRAPH_NODE, children, edges: [] } as any; + nextInsert = subgraph.children as WorkflowNode[]; + nextGraphId = subgraphId; + nextParentNodeId = containerId; + seed = { workflowGraph: subgraph, graphId: subgraphId, parentId: subgraphCardId }; + } else { + // Plain Scope and For each are single-flow graphs: children nest directly in the body. + container = { + id: containerId, + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: [createWorkflowNode(`${containerId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)], + edges: [], + } as any; + nextInsert = container.children as WorkflowNode[]; + nextGraphId = containerId; + nextParentNodeId = containerId; + seed = { workflowGraph: container, graphId: containerId, parentId: `${containerId}-#scope` }; + } + + if (i === 0) { + outerId = containerId; + outerContainer = container; + } else { + (prevInsert as WorkflowNode[]).push(container); + } + prevInsert = nextInsert; + childGraphId = nextGraphId; + childParentNodeId = nextParentNodeId; + if (isInnermost) { + innermost = { targetId: containerId, ...seed }; + } + }); + + return { + outerId, + outerContainer: outerContainer as WorkflowNode, + targetId: (innermost as any).targetId, + workflowGraph: (innermost as any).workflowGraph, + graphId: (innermost as any).graphId, + parentId: (innermost as any).parentId, + nodesMetadata: meta, + }; +}; + +type VarBMode = 'parallel' | 'inpath' | 'sibling'; + +const resolveVariableNamesNested = (chain: TargetKind[], wrapper: WrapperKind, varBMode: VarBMode): string[] => { + const nested = buildNestedTarget(chain, varBMode === 'sibling'); + const paste = buildPastedSubtree(wrapper, nested.graphId, nested.parentId); + + let rootChildren: WorkflowNode[]; + let rootEdges: any[]; + const varAMeta = { [VAR_A]: { graphId: 'root' } } as Record; + const rootVarBMeta = varBMode === 'sibling' ? {} : { [VAR_B]: { graphId: 'root' } }; + + if (varBMode === 'inpath') { + rootChildren = [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, VAR_B), createWorkflowEdge(VAR_B, nested.outerId)]; + } else if (varBMode === 'sibling') { + // varB lives inside the outermost condition's opposite branch (built by buildNestedTarget). + rootChildren = [createWorkflowNode(VAR_A), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, nested.outerId)]; + } else { + rootChildren = [createWorkflowNode(VAR_A), createWorkflowNode(VAR_B), nested.outerContainer]; + rootEdges = [createWorkflowEdge(VAR_A, nested.outerId), createWorkflowEdge(VAR_A, VAR_B)]; + } + + const root: WorkflowNode = { + id: 'root', + type: WORKFLOW_NODE_TYPES.GRAPH_NODE, + children: rootChildren, + edges: rootEdges, + } as any; + + const state = { + operations: { ...paste.operations }, + nodesMetadata: { + ...varAMeta, + ...rootVarBMeta, + ...nested.nodesMetadata, + }, + newlyAddedOperations: {}, + } as any; + + const relationshipIds = { graphId: nested.graphId, parentId: nested.parentId, childId: undefined } as any; + + pasteScopeInWorkflow( + paste.scopeNode, + nested.workflowGraph, + relationshipIds, + paste.operations, + paste.pasteNodesMetadata, + [paste.topId, paste.setVarId], + state, + false + ); + + const operationMap: Record = Object.fromEntries( + [VAR_A, VAR_B, paste.topId, paste.setVarId, ...chain.map((_, i) => `Nest${i}`)].map((id) => [id, id]) + ); + + const upstream = getUpstreamNodeIds(paste.setVarId, root, state.nodesMetadata, operationMap); + return getAvailableVariables(variablesMap, upstream).map((variable) => variable.name as string); +}; + +const wrappers: WrapperKind[] = ['condition', 'scope']; +const targets: TargetKind[] = ['condition', 'scope', 'foreach']; + +describe('pasteScopeInWorkflow variable-dropdown scope correctness (paste matrix)', () => { + describe('excludes a separate-path Initialize Variable', () => { + for (const wrapper of wrappers) { + for (const target of targets) { + it(`Set Variable in a copied ${wrapper} pasted into a ${target} sees only in-path variables`, () => { + const names = resolveVariableNames(wrapper, target, false); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + } + } + }); + + describe('includes every Initialize Variable that is genuinely upstream', () => { + it('Set Variable in a copied condition pasted into a condition downstream of both vars sees both', () => { + const names = resolveVariableNames('condition', 'condition', true); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + + it('Set Variable in a copied scope pasted into a for each downstream of both vars sees both', () => { + const names = resolveVariableNames('scope', 'foreach', true); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + }); +}); + +describe('pasteScopeInWorkflow variable-dropdown scope correctness (deep nesting)', () => { + describe('resolves the in-path root variable and excludes a separate-path variable at depth', () => { + const deepChains: { name: string; chain: TargetKind[] }[] = [ + { + name: 'N1 Scope -> Scope -> Scope -> Scope -> Condition (pure scope depth 5)', + chain: ['scope', 'scope', 'scope', 'scope', 'condition'], + }, + { + name: 'N2 Condition -> Condition -> Condition -> Condition (pure subgraph chain depth 4)', + chain: ['condition', 'condition', 'condition', 'condition'], + }, + { + name: 'N3 Scope -> Condition -> For each -> Condition -> Scope -> For each -> Condition (mixed depth 7)', + chain: ['scope', 'condition', 'foreach', 'condition', 'scope', 'foreach', 'condition'], + }, + { name: 'N4 Condition -> Scope -> For each -> Condition (mixed depth 4)', chain: ['condition', 'scope', 'foreach', 'condition'] }, + ]; + + for (const { name, chain } of deepChains) { + it(`${name} sees only the in-path variable`, () => { + const names = resolveVariableNamesNested(chain, 'condition', 'parallel'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + } + + it('N5 deep chain in a condition True branch excludes a variable initialized in the sibling False branch', () => { + // Outermost condition: main chain nests in one branch; Initialize_variables_1 (varB) lives in + // the opposite branch. The pasted Set Variable deep in the chain must not see it. + const names = resolveVariableNamesNested(['condition', 'scope', 'foreach', 'condition'], 'condition', 'sibling'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + }); + + describe('still includes every variable that is genuinely upstream at depth', () => { + it('N6 deep chain downstream of both root variables sees both', () => { + const names = resolveVariableNamesNested(['scope', 'scope', 'condition'], 'condition', 'inpath'); + expect(names).toContain(VAR_A_NAME); + expect(names).toContain(VAR_B_NAME); + }); + }); + + describe('scope-wrapped paste blocks resolve at depth', () => { + it('N7 a copied Scope>Condition>Set Variable pasted into a deeply nested For each sees only the in-path variable', () => { + const names = resolveVariableNamesNested(['scope', 'condition', 'foreach'], 'scope', 'parallel'); + expect(names).toContain(VAR_A_NAME); + expect(names).not.toContain(VAR_B_NAME); + }); + }); +}); diff --git a/libs/designer/src/lib/core/parsers/pasteScopeInWorkflow.ts b/libs/designer/src/lib/core/parsers/pasteScopeInWorkflow.ts index b569ea0048b..38ffb096d5e 100644 --- a/libs/designer/src/lib/core/parsers/pasteScopeInWorkflow.ts +++ b/libs/designer/src/lib/core/parsers/pasteScopeInWorkflow.ts @@ -54,6 +54,13 @@ export const pasteScopeInWorkflow = ( if (getRecordEntry(nodesMetadata, nodeId)?.isRoot === false) { delete nodesMetadata[nodeId].isRoot; } + // The loop above copied the pasted node's metadata into state by reference before this + // correction ran, so the recomputed graphId/parentNodeId must be written back to state. + // Otherwise state keeps the placeholder parentNodeId seeded during paste (an edge-placement + // card id such as `-#subgraph`), which is not a nodesMetadata key. That breaks the + // parent-chain walk in getUpstreamNodeIds and leaves scoped dropdowns (e.g. Set Variable) + // unable to see variables initialized in an ancestor scope. + state.nodesMetadata[nodeId] = nodesMetadata[nodeId]; const parentMetadata = getRecordEntry(state.nodesMetadata, parentId); const isAfterTrigger = (parentMetadata?.isRoot && newGraphId === 'root') ?? false; diff --git a/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts b/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts index 24f39d135cf..6c6a5e7b319 100644 --- a/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts +++ b/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/__test__/getEditorAndOptions.spec.ts @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import { getEditorAndOptions } from '..'; import type { VariableDeclaration } from '../../../../../../core/state/tokens/tokensSlice'; import { InitEditorService } from '@microsoft/logic-apps-shared'; @@ -108,7 +109,7 @@ describe('getEditorAndOptions', () => { expect(getEditorAndOptions(operationInfo, parameter, upstreamNodeIds, variables)).toEqual(expectedEditorAndOptions); }); - describe('"variablename" editor fallback when scoped list is empty', () => { + describe('"variablename" editor strictly scopes to upstream variables', () => { const variables: Record = { initInt: [{ name: 'intVar', type: 'integer' }], initFloat: [{ name: 'floatVar', type: 'float' }], @@ -117,30 +118,18 @@ describe('getEditorAndOptions', () => { initBool: [{ name: 'boolVar', type: 'boolean' }], }; - // These supportedTypes values mirror the 5 built-in operations that use the - // 'variablename' editor (see libs/logic-apps-shared/.../manifests/variables.ts): - // setVariable (any), incrementVariable/decrementVariable (float,integer), - // appendToStringVariable (string), appendToArrayVariable (array). - it.each` - operation | supportedTypes | expectedNames - ${'setVariable'} | ${[]} | ${['intVar', 'floatVar', 'strVar', 'arrVar', 'boolVar']} - ${'incrementVariable'} | ${['float', 'integer']} | ${['intVar', 'floatVar']} - ${'decrementVariable'} | ${['float', 'integer']} | ${['intVar', 'floatVar']} - ${'appendToStringVariable'} | ${['string']} | ${['strVar']} - ${'appendToArrayVariable'} | ${['array']} | ${['arrVar']} - `( - 'falls back to all declared variables (respecting supportedTypes) for $operation when upstreamNodeIds is empty', - ({ supportedTypes, expectedNames }) => { - const parameter = getParameterInfo(); - parameter.editor = 'variablename'; - parameter.editorOptions = { supportedTypes }; - - const result = getEditorAndOptions(operationInfo, parameter, /* upstreamNodeIds */ [], variables); - - expect(result.editor).toBe('dropdown'); - expect((result.editorOptions?.options ?? []).map((o: any) => o.value)).toEqual(expectedNames); - } - ); + it('returns an empty options list when no variables are upstream (no all-variables leak)', () => { + const parameter = getParameterInfo(); + parameter.editor = 'variablename'; + parameter.editorOptions = { supportedTypes: [] }; + + // upstreamNodeIds is empty even though variables are declared elsewhere in the workflow. + // Showing all of them would let a Set Variable reference an out-of-scope variable, so the + // dropdown must be empty. + const result = getEditorAndOptions(operationInfo, parameter, /* upstreamNodeIds */ [], variables); + + expect(result).toEqual({ editor: 'dropdown', editorOptions: { options: [] } }); + }); it('returns an empty options list when no variables are declared at all', () => { const parameter = getParameterInfo(); @@ -152,13 +141,12 @@ describe('getEditorAndOptions', () => { expect(result).toEqual({ editor: 'dropdown', editorOptions: { options: [] } }); }); - it('does not fall back when the scoped list already has matches (no regression)', () => { + it('only includes variables whose declaring node is upstream, excluding parallel-branch variables', () => { const parameter = getParameterInfo(); parameter.editor = 'variablename'; parameter.editorOptions = { supportedTypes: ['string'] }; - // upstream includes initStr (matches) but excludes initArr — the fallback must NOT - // add arrVar because scopedOptions is non-empty. + // upstream includes initStr (in scope) but excludes initArr/others (parallel branch). const result = getEditorAndOptions(operationInfo, parameter, ['initStr'], variables); expect(result).toEqual({ diff --git a/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx b/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx index f866b034392..520d8d44d60 100644 --- a/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx +++ b/libs/designer/src/lib/ui/panel/nodeDetailsPanel/tabs/parametersTab/index.tsx @@ -46,7 +46,7 @@ import { } from '../../../../../core/utils/parameters/helper'; import type { TokenGroup } from '../../../../../core/utils/tokens'; import { createValueSegmentFromToken, getExpressionTokenSections, getOutputTokenSections } from '../../../../../core/utils/tokens'; -import { getAllVariables, getAvailableVariables } from '../../../../../core/utils/variables'; +import { getAvailableVariables } from '../../../../../core/utils/variables'; import { SettingsSection } from '../../../../settings/settingsection'; import type { Settings } from '../../../../settings/settingsection'; import { ConnectionDisplay } from './connectionDisplay'; @@ -1868,17 +1868,12 @@ export const getEditorAndOptions = ( // Handle variable dropdown editor if (equals(editor, constants.EDITOR.VARIABLE_NAME)) { - const toOption = (variable: VariableDeclaration) => ({ value: variable.name, displayName: variable.name }); - const typeMatches = (variable: VariableDeclaration) => supportedTypes.length === 0 || supportedTypes.includes(variable.type); - - const scopedOptions = getAvailableVariables(variables, upstreamNodeIds).filter(typeMatches).map(toOption); - - // Fallback: when the scoped list is empty but variables are declared elsewhere in the workflow, - // show all declared variables. The runtime does not scope variables by graph position, so any - // declared variable is legally assignable (see: pasteScopeOperation bug where a pasted node's - // upstreamNodeIds does not include the root InitializeVariable). Preserves the ideal UX when - // the scoped list is non-empty; only kicks in to prevent an empty-dropdown dead-end. - const options = scopedOptions.length === 0 ? getAllVariables(variables).filter(typeMatches).map(toOption) : scopedOptions; + const options = getAvailableVariables(variables, upstreamNodeIds) + .filter((variable) => supportedTypes.length === 0 || supportedTypes.includes(variable.type)) + .map((variable) => ({ + value: variable.name, + displayName: variable.name, + })); return { editor: 'dropdown',