Add bounded workflow loop nodes - #1508
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR introduces first-class loop workflow nodes with bounded template repetition, exit conditions, and editor support. It adds IR types and validation, executor runtime execution, dashboard authoring UI, bidirectional flow mapping, and comprehensive documentation. ChangesWorkflow Loop Nodes
Sequence Diagram(s)sequenceDiagram
participant Author as Workflow Author
participant Editor as WorkflowNodeEditor
participant Mapping as workflow-flow-mapping
participant Executor as WorkflowGraphExecutor
participant LoopRunner as runLoop
participant Context as Workflow Context
Author->>Editor: Configure loop node and template
Editor->>Mapping: flowToIr(loop node)
Mapping->>Executor: Provide IR with loop node
Executor->>LoopRunner: runLoop(loopNode, env)
LoopRunner->>Context: Publish iteration outcomes and values
LoopRunner->>Executor: Return outcome, value, visitedNodeIds
Executor->>Context: Store node:{id}:outcome and node:{id}:value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR introduces first-class
Confidence Score: 5/5Safe to merge; the previously-flagged blocking issue (loop nodes silently permitted inside foreach templates) is fully addressed with both a guard and a test. All execution paths through the loop node are covered by tests (success exit, multi-iteration convergence, exhaustion routing, timeout routing, regex nodeId). The IR validation is thorough, the foreach/loop nesting symmetry is correctly handled in both directions, and the context merge/publish pattern follows the established foreach executor model. packages/engine/src/workflow-graph-loop.ts — local cap constants duplicate core values without the import-from-core guard that the foreach executor uses; worth a follow-up to export a resolver from @fusion/core. Important Files Changed
Sequence DiagramsequenceDiagram
participant E as WorkflowGraphExecutor
participant L as runLoop
participant T as runTemplateNode
participant C as Context
E->>L: runLoop(loopNode, env)
loop for iteration 1..maxIterations
L->>L: check deadline / abort signal
L->>T: runTemplateNode(entry, signal, iterationContext)
T-->>L: WorkflowNodeResult
alt node failure
L->>C: "publishIterationContext + exitReason=node-failure"
L-->>E: "{outcome: failure, value: node.value}"
else walk next edge
L->>T: runTemplateNode(next, ...)
T-->>L: WorkflowNodeResult
end
L->>L: matchesExit(config, sourceNodeValue)
alt exit condition met
L->>C: "context[node:loop:loop] = {exitReason: matched}"
L-->>E: "{outcome: success}"
end
end
L->>C: "context[node:loop:loop] = {exitReason: iteration-exhausted | timeout}"
L-->>E: "{outcome: failure, value: loop-iteration-exhausted | loop-timeout}"
Reviews (2): Last reviewed commit: "fix(FN-0000): address workflow loop revi..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/workflow-flow-mapping.ts (1)
1002-1019:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve persisted child layout when inserting template-group fragments.
Inserted
foreach/loopfragments currently discard the saved positions of their template children and reset every child to the default horizontal layout. The fragmentlayoutalready carries namespaced child keys, so authors will lose any hand-arranged template body every time they insert one of these fragments.Proposed fix
if (groupCfg) { const template = groupCfg.template; const groupKind = editorKind(node); template.nodes.forEach((inner, innerIdx) => { const innerKind = editorKind(inner); + const childPos = + layout?.[foreachChildFlowId(node.id, inner.id)] ?? { + x: FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X, + y: FOREACH_CHILD_Y, + }; childNodes.push({ id: foreachChildFlowId(id, inner.id), type: innerKind, - position: { x: FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X, y: FOREACH_CHILD_Y }, + position: childPos, parentId: id, extent: "parent", data: { kind: innerKind, label: nodeLabel(inner), config: { ...(inner.config ?? {}) } }, deletable: true, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/workflow-flow-mapping.ts` around lines 1002 - 1019, The foreach/loop fragment insertion is overwriting saved child positions by always assigning default FOREACH_CHILD_X/STEP positions; update the creation of child nodes in the block that calls groupTemplateConfigOf(node) and iterates template.nodes (the code that uses foreachChildFlowId, editorKind, nodeLabel) to first look up any persisted per-child layout in node.layout (the namespaced child keys the fragment carries) and, if present, use those x/y (and extent if stored) for position/extent instead of the default horizontal layout; fall back to the existing FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X and FOREACH_CHILD_Y when no saved layout exists. Ensure irEdgeToFlow call that namespaces edge ids continues to use the same id prefix so layout keys match.
🧹 Nitpick comments (1)
docs/workflow-steps.md (1)
122-141: ⚡ Quick winAdd language tag to code fence for proper syntax highlighting.
The fenced code block on line 126 should specify
tsortypescriptfor proper syntax highlighting in rendered documentation.📝 Proposed fix
-``` +```ts { template: { nodes, edges },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/workflow-steps.md` around lines 122 - 141, The fenced code block showing the `loop` node config should include a language tag for TypeScript; update the code fence that begins with "{ template: { nodes, edges }," to use ```ts (or ```typescript) so the snippet for keys like `template`, `exitWhen`, `maxIterations`, and `timeoutMs` is syntax-highlighted in the rendered docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/app/components/nodes/node-summary.ts`:
- Around line 161-177: The loop case in node-summary.ts contains hardcoded
English strings ("until matches", "until contains", "3x") that bypass
localization. Pass these string literals through the translator function `t` to
enable proper localization of loop summaries. This includes the string templates
for the pattern matching phrase and the value contains phrase, as well as the
default iteration count string. Update all three string literals returned in the
conditional expression and the default maxIterations fallback to use the `t`
function for consistency with the rest of the node summary localization.
In `@packages/dashboard/app/components/WorkflowNodeEditor.tsx`:
- Around line 3196-3216: The onChange handler for the exit type select (using
exitType and updateSelectedData) rebuilds exitWhen from only type +
pattern/value and drops other sibling fields (e.g., nodeId, regex flags); update
the handler so when computing the new exitWhen you copy existing keys from the
current exitWhen object and only override type and the pattern/value field
(preserving nodeId and any other fields), e.g., read the current exitWhen via
prev.exitWhen, merge its properties into the new object, and set the new type
plus pattern or value while leaving other keys intact.
In `@packages/engine/src/workflow-graph-loop.ts`:
- Around line 90-96: The matchesExit function currently compiles a RegExp per
iteration; instead precompile and validate the regex once when the loop config
is resolved (the WorkflowLoopConfig.exitWhen object) and store the compiled
RegExp on that config so matchesExit can reuse it; additionally run a safety
check (e.g., use a safe-regex-like heuristic or RE2-compatible validation)
during validation/creation of the loop config (in the workflow config
resolution/validation path that consumes WorkflowLoopConfig) and reject or
sanitize pathological patterns so no hazardous pattern reaches the runtime loop.
---
Outside diff comments:
In `@packages/dashboard/app/components/workflow-flow-mapping.ts`:
- Around line 1002-1019: The foreach/loop fragment insertion is overwriting
saved child positions by always assigning default FOREACH_CHILD_X/STEP
positions; update the creation of child nodes in the block that calls
groupTemplateConfigOf(node) and iterates template.nodes (the code that uses
foreachChildFlowId, editorKind, nodeLabel) to first look up any persisted
per-child layout in node.layout (the namespaced child keys the fragment carries)
and, if present, use those x/y (and extent if stored) for position/extent
instead of the default horizontal layout; fall back to the existing
FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X and FOREACH_CHILD_Y when no
saved layout exists. Ensure irEdgeToFlow call that namespaces edge ids continues
to use the same id prefix so layout keys match.
---
Nitpick comments:
In `@docs/workflow-steps.md`:
- Around line 122-141: The fenced code block showing the `loop` node config
should include a language tag for TypeScript; update the code fence that begins
with "{ template: { nodes, edges }," to use ```ts (or ```typescript) so the
snippet for keys like `template`, `exitWhen`, `maxIterations`, and `timeoutMs`
is syntax-highlighted in the rendered docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb864583-7407-497f-83b1-dd525761f928
📒 Files selected for processing (17)
.changeset/workflow-loop-nodes.mddocs/plans/2026-06-08-002-feat-workflow-loop-nodes-plan.mddocs/workflow-steps.mdpackages/core/src/__tests__/workflow-ir-loop.test.tspackages/core/src/index.tspackages/core/src/workflow-ir-types.tspackages/core/src/workflow-ir.tspackages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/app/components/__tests__/workflow-flow-mapping.test.tspackages/dashboard/app/components/nodes/WorkflowNodeTypes.tsxpackages/dashboard/app/components/nodes/__tests__/node-summary.test.tspackages/dashboard/app/components/nodes/node-summary.tspackages/dashboard/app/components/workflow-flow-mapping.tspackages/engine/src/__tests__/workflow-graph-loop.test.tspackages/engine/src/workflow-graph-executor.tspackages/engine/src/workflow-graph-loop.tspackages/plugin-sdk/src/index.ts
Collaborator
Author
Addressed:
Addressed: updated the loop config fence to
Addressed:
Addressed: updated the bounded-config clamp JSDoc and the approval-bypass stripping comment to reflect loop/template-group behavior. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
loopworkflow IR nodes with bounded template execution, output-based exit conditions, and failure values for timeout/iteration exhaustionValidation
pnpm --filter @fusion/core exec vitest run src/__tests__/workflow-ir-loop.test.ts --silent=passed-only --reporter=dotpnpm --filter @fusion/engine exec vitest run src/__tests__/workflow-graph-loop.test.ts --silent=passed-only --reporter=dotpnpm --filter @fusion/dashboard exec vitest run app/components/__tests__/workflow-flow-mapping.test.ts app/components/nodes/__tests__/node-summary.test.ts --silent=passed-only --reporter=dotpnpm --filter @fusion/core typecheckpnpm --filter @fusion/engine typecheckpnpm --filter @fusion/dashboard typecheckpnpm --filter @fusion/plugin-sdk typecheckpnpm lintpnpm buildpnpm test:gateSummary by CodeRabbit
New Features
Documentation