Skip to content

Commit c5ac424

Browse files
fix(designer): scope Set Variable dropdown for pasted scopes (#9403)
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 `<scope>-#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. Copilot-Session: af9ea0d5-71e4-45d9-9790-0edc869beadd Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4780f85 commit c5ac424

14 files changed

Lines changed: 1554 additions & 84 deletions

File tree

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// @vitest-environment jsdom
2+
import { describe, it, expect } from 'vitest';
3+
import { extendUpstreamNodeIdsForScopePaste } from '../copypaste';
4+
import type { RootState } from '../../..';
5+
import { createWorkflowEdge, createWorkflowNode } from '../../../utils/graph';
6+
import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared';
7+
8+
// Graph shape:
9+
// root
10+
// manual (trigger)
11+
// Init_Variable (source node at root, before the parallel split)
12+
// Init_Parallel (parallel branch A — NOT upstream of branches B/C)
13+
// Condition_outer (parallel branch B; scope with a True subgraph — paste site)
14+
// └── Condition_outer-actions (subgraph paste site inside the condition)
15+
// For_each (parallel branch C; loop with an empty body — paste site)
16+
const buildState = (): RootState => {
17+
const rootGraph = {
18+
id: 'root',
19+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
20+
children: [
21+
createWorkflowNode('manual'),
22+
createWorkflowNode('Init_Variable'),
23+
createWorkflowNode('Init_Parallel'),
24+
{
25+
id: 'Condition_outer',
26+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
27+
children: [
28+
createWorkflowNode('Condition_outer-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE),
29+
{
30+
id: 'Condition_outer-actions',
31+
type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE,
32+
children: [createWorkflowNode('Condition_outer-actions-#subgraph', WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)],
33+
edges: [],
34+
},
35+
],
36+
edges: [createWorkflowEdge('Condition_outer-#scope', 'Condition_outer-actions')],
37+
},
38+
{
39+
id: 'For_each',
40+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
41+
children: [createWorkflowNode('For_each-#scope', WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)],
42+
edges: [],
43+
},
44+
],
45+
edges: [
46+
createWorkflowEdge('manual', 'Init_Variable'),
47+
createWorkflowEdge('Init_Variable', 'Init_Parallel'),
48+
createWorkflowEdge('Init_Variable', 'Condition_outer'),
49+
createWorkflowEdge('Init_Variable', 'For_each'),
50+
],
51+
} as any;
52+
53+
return {
54+
workflow: {
55+
graph: rootGraph,
56+
nodesMetadata: {
57+
manual: { graphId: 'root', isRoot: true, isTrigger: true },
58+
Init_Variable: { graphId: 'root' },
59+
Init_Parallel: { graphId: 'root' },
60+
Condition_outer: { graphId: 'root' },
61+
'Condition_outer-actions': { graphId: 'Condition_outer', parentNodeId: 'Condition_outer', subgraphType: 'CONDITIONAL_TRUE' },
62+
For_each: { graphId: 'root' },
63+
},
64+
},
65+
tokens: {
66+
outputTokens: {},
67+
},
68+
} as unknown as RootState;
69+
};
70+
71+
const nodeMap: Record<string, string> = {
72+
manual: 'manual',
73+
Init_Variable: 'Init_Variable',
74+
Init_Parallel: 'Init_Parallel',
75+
Condition_outer: 'Condition_outer',
76+
'Condition_outer-actions': 'Condition_outer-actions',
77+
For_each: 'For_each',
78+
};
79+
80+
describe('extendUpstreamNodeIdsForScopePaste', () => {
81+
it('returns only the caller ids when there is no enclosing graph or parent', () => {
82+
const state = buildState();
83+
const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], undefined, undefined, state, nodeMap);
84+
expect(result).toEqual(['Init_Variable']);
85+
});
86+
87+
it('surfaces the ancestor variable when pasting into a Condition subgraph', () => {
88+
// Pasting a scope into the True branch of Condition_outer. graphId is the subgraph node.
89+
const state = buildState();
90+
const result = extendUpstreamNodeIdsForScopePaste([], 'Condition_outer-actions', 'Condition_outer', state, nodeMap);
91+
expect(result).toContain('Init_Variable');
92+
expect(result).not.toContain('Init_Parallel');
93+
});
94+
95+
it('surfaces the ancestor variable when pasting into a For each body even if parentId is undefined', () => {
96+
// Regression: pasting a Condition as the first/only action inside a For each. There is no
97+
// predecessor action, so parentId is undefined. The enclosing graphId (For_each) must still
98+
// surface the upstream Init_Variable while excluding the parallel-branch Init_Parallel.
99+
const state = buildState();
100+
const result = extendUpstreamNodeIdsForScopePaste([], 'For_each', undefined, state, nodeMap);
101+
expect(result).toContain('Init_Variable');
102+
expect(result).not.toContain('Init_Parallel');
103+
});
104+
105+
it('deduplicates ids that already appear in the caller-provided upstream list', () => {
106+
const state = buildState();
107+
const result = extendUpstreamNodeIdsForScopePaste(['Init_Variable'], 'For_each', undefined, state, nodeMap);
108+
expect(result.filter((id) => id === 'Init_Variable')).toHaveLength(1);
109+
});
110+
});

libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import type { ActionDefinition } from '@microsoft/logic-apps-shared/src/utils/sr
2424
import { initializeDynamicDataInNodes, initializeOperationMetadata } from './operationdeserializer';
2525
import type { NodesMetadata } from '../../state/workflow/workflowInterfaces';
2626
import { updateAllUpstreamNodes } from './initialize';
27+
import { getUpstreamNodeIds } from '../../utils/graph';
28+
import type { WorkflowNode } from '../../parsers/models/workflowNode';
2729
import type { NodeTokens } from '../../state/tokens/tokensSlice';
2830
import { addDynamicTokens } from '../../state/tokens/tokensSlice';
2931
import { getConnectionReferenceForNodeId } from '../../state/connection/connectionSelector';
@@ -315,6 +317,7 @@ export const pasteOperation = createAsyncThunk('pasteOperation', async (payload:
315317
dispatch(setNodeDescription({ nodeId, description: comment }));
316318
}
317319

320+
updateAllUpstreamNodes(getState() as RootState, dispatch);
318321
dispatch(setIsPanelLoading(false));
319322

320323
return nodeId;
@@ -363,7 +366,13 @@ export const pasteScopeOperation = createAsyncThunk(
363366
true /* shouldAppendAddCase */,
364367
pasteParams
365368
);
366-
actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: parentId, graphId };
369+
// parentNodeId must reference a nodesMetadata key (the containing graph/scope) so the
370+
// parent-chain walk in getUpstreamNodeIds can climb to ancestor scopes. `parentId` from
371+
// relationshipIds is an edge-placement node (e.g. a `<scope>-#subgraph` card) and is not a
372+
// valid metadata key, so use `graphId` (the containing graph) instead, matching how
373+
// pasteScopeInWorkflow finalizes this node's metadata.
374+
const scopeParentNodeId = graphId && graphId !== 'root' ? graphId : undefined;
375+
actionNodesMetadata[nodeId] = { ...actionNodesMetadata[actionId], isRoot: false, parentNodeId: scopeParentNodeId, graphId };
367376
if (Object.keys(allConnectionData).length > 0) {
368377
dispatch(initScopeCopiedConnections(replaceIdsOfExistingNodes(allConnectionData, pasteParams.renamedNodes)));
369378
}
@@ -393,7 +402,8 @@ export const pasteScopeOperation = createAsyncThunk(
393402
for (const id of Object.keys(operations)) {
394403
nodeMap[id] = id;
395404
}
396-
const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, upstreamNodeIds), idReplacements);
405+
const extendedIds = extendUpstreamNodeIdsForScopePaste(upstreamNodeIds, graphId, parentId, state, nodeMap);
406+
const upstreamOutputTokens = replaceIdsOfExistingNodes(filterRecordByArray(state.tokens.outputTokens, extendedIds), idReplacements);
397407
const triggerId = getTriggerNodeId(state.workflow);
398408

399409
await Promise.all([
@@ -468,6 +478,46 @@ const filterRecordByArray = (record: Record<string, NodeTokens>, upstreamNodeIds
468478
return filteredRecord;
469479
};
470480

481+
/**
482+
* Extends the upstream node id set used to seed a pasted scope's existing output tokens.
483+
*
484+
* When we paste a scope (e.g. a Condition) inside another scope, the `upstreamNodeIds`
485+
* we receive from the caller only covers upstream nodes of the paste-site child/parent
486+
* edge. It does not include the enclosing scope container itself or its ancestor chain,
487+
* so nodes declared alongside (or outside) that container — such as an `InitializeVariable`
488+
* at the workflow root — are absent from the fragment's initial upstream token slice.
489+
*
490+
* We fold in the enclosing scope container (`graphId`) and its full upstream context, plus
491+
* the paste-site predecessor (`parentId`) when present. Relying on `graphId` is essential:
492+
* when a scope is pasted as the first/only action inside a container (e.g. a For each body),
493+
* `parentId` is `undefined` or a non-chaining header node, so extending only via `parentId`
494+
* would miss the ancestor variables. The enclosing `graphId` always resolves to a node whose
495+
* parent chain reaches the workflow root, so its upstream reliably surfaces ancestor tokens.
496+
*/
497+
export const extendUpstreamNodeIdsForScopePaste = (
498+
upstreamNodeIds: string[],
499+
graphId: string | undefined,
500+
parentId: string | undefined,
501+
state: RootState,
502+
operationMap: Record<string, string>
503+
): string[] => {
504+
const extended = new Set(upstreamNodeIds);
505+
const rootGraph = state.workflow.graph as WorkflowNode | null;
506+
if (!rootGraph) {
507+
return Array.from(extended);
508+
}
509+
for (const containerId of [graphId, parentId]) {
510+
if (!containerId || containerId === 'root') {
511+
continue;
512+
}
513+
extended.add(containerId);
514+
for (const id of getUpstreamNodeIds(containerId, rootGraph, state.workflow.nodesMetadata, operationMap)) {
515+
extended.add(id);
516+
}
517+
}
518+
return Array.from(extended);
519+
};
520+
471521
type DuplicateOperationsPayload = {
472522
nodeIds: string[];
473523
};
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// @vitest-environment jsdom
2+
import { describe, it, expect } from 'vitest';
3+
import { pasteScopeInWorkflow } from '../pasteScopeInWorkflow';
4+
import { createWorkflowNode } from '../../utils/graph';
5+
import { WORKFLOW_NODE_TYPES } from '@microsoft/logic-apps-shared';
6+
7+
// Reproduces the customer bug: a Condition (containing a Set Variable) is pasted into the else
8+
// branch of an existing Condition. The pasted top node arrives with a placeholder parentNodeId
9+
// that points at the subgraph card (`Condition-elseActions-#subgraph`) — an edge-placement node
10+
// that is NOT a nodesMetadata key. The reducer must overwrite that with the containing graph id
11+
// (`Condition-elseActions`) *in state* so the parent-chain walk can climb to the outer scope and
12+
// surface ancestor-scope variables in the Set Variable dropdown.
13+
describe('pasteScopeInWorkflow parentNodeId persistence', () => {
14+
const buildFixture = () => {
15+
const pastedNodeId = 'Condition_2';
16+
const graphId = 'Condition-elseActions';
17+
const subgraphCardId = 'Condition-elseActions-#subgraph';
18+
19+
const elseSubgraph = {
20+
id: graphId,
21+
type: WORKFLOW_NODE_TYPES.SUBGRAPH_NODE,
22+
children: [createWorkflowNode(subgraphCardId, WORKFLOW_NODE_TYPES.SUBGRAPH_CARD_NODE)],
23+
edges: [],
24+
} as any;
25+
26+
const scopeNode = {
27+
id: pastedNodeId,
28+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
29+
children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)],
30+
edges: [],
31+
} as any;
32+
33+
// Metadata handed to the reducer for the pasted subtree. The top node carries the broken
34+
// placeholder parentNodeId (the subgraph card id) that must be corrected in state.
35+
const pasteNodesMetadata = {
36+
[pastedNodeId]: { graphId, parentNodeId: subgraphCardId, isRoot: false },
37+
} as any;
38+
39+
const state = {
40+
operations: {
41+
[pastedNodeId]: {},
42+
},
43+
nodesMetadata: {
44+
Initialize_variables: { graphId: 'root' },
45+
Condition: { graphId: 'root' },
46+
[graphId]: { graphId: 'Condition', parentNodeId: 'Condition', subgraphType: 'CONDITIONAL_FALSE' },
47+
},
48+
newlyAddedOperations: {},
49+
} as any;
50+
51+
const relationshipIds = { graphId, parentId: subgraphCardId, childId: undefined } as any;
52+
53+
return { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds };
54+
};
55+
56+
it('writes the corrected parentNodeId (the containing graph id) into state', () => {
57+
const { pastedNodeId, graphId, elseSubgraph, scopeNode, pasteNodesMetadata, state, relationshipIds } = buildFixture();
58+
59+
pasteScopeInWorkflow(scopeNode, elseSubgraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false);
60+
61+
expect(state.nodesMetadata[pastedNodeId]).toBeDefined();
62+
expect(state.nodesMetadata[pastedNodeId].graphId).toBe(graphId);
63+
// The placeholder subgraph-card parentNodeId must not survive in state.
64+
expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBe(graphId);
65+
expect(state.nodesMetadata[pastedNodeId].parentNodeId).not.toBe(relationshipIds.parentId);
66+
});
67+
68+
it('leaves parentNodeId undefined when pasting at the workflow root', () => {
69+
const pastedNodeId = 'Condition_2';
70+
const rootGraph = {
71+
id: 'root',
72+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
73+
children: [],
74+
edges: [],
75+
} as any;
76+
const scopeNode = {
77+
id: pastedNodeId,
78+
type: WORKFLOW_NODE_TYPES.GRAPH_NODE,
79+
children: [createWorkflowNode(`${pastedNodeId}-#scope`, WORKFLOW_NODE_TYPES.SCOPE_CARD_NODE)],
80+
edges: [],
81+
} as any;
82+
const state = {
83+
operations: { [pastedNodeId]: {} },
84+
nodesMetadata: { Initialize_variables: { graphId: 'root' } },
85+
newlyAddedOperations: {},
86+
} as any;
87+
const pasteNodesMetadata = { [pastedNodeId]: { graphId: 'root', parentNodeId: undefined } } as any;
88+
const relationshipIds = { graphId: 'root', parentId: undefined, childId: undefined } as any;
89+
90+
pasteScopeInWorkflow(scopeNode, rootGraph, relationshipIds, {}, pasteNodesMetadata, [pastedNodeId], state, false);
91+
92+
expect(state.nodesMetadata[pastedNodeId].graphId).toBe('root');
93+
expect(state.nodesMetadata[pastedNodeId].parentNodeId).toBeUndefined();
94+
});
95+
});

0 commit comments

Comments
 (0)