feat: Add stopNodeId tracking for improved validation and edge control#9867
feat: Add stopNodeId tracking for improved validation and edge control#9867Cristhianzl merged 2 commits intomainfrom
Conversation
…atement ♻️ (flowStore.ts): Refactor useFlowStore to include a stopNodeId property and setter function for better state management and flow control.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces stopNodeId state and setter in the flow store and its type definition, integrates stopNodeId into buildFlow and edge-running updates, and removes a debug log from the FlowSidebarComponent. No exported signatures removed; new fields added to FlowStoreType. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / FlowPage
participant Store as FlowStore
participant Graph as Graph Utils
UI->>Store: buildFlow({ startNodeId?, stopNodeId? })
alt stopNodeId provided and no startNodeId
Store->>Store: setStopNodeId(stopNodeId)
Store->>Graph: computeUpstreamSubgraph(stopNodeId)
else startNodeId provided
Store->>Graph: computeDownstreamSubgraph(startNodeId)
end
Store->>Store: validate subgraph scope
note over Store: Maintains existing validation logic
Store->>Store: updateEdgesRunningByNodes()
note over Store: Exclude edges where sourceHandle.id === stopNodeId
opt clear stopNodeId when not applicable
Store->>Store: setStopNodeId(undefined)
end
Store-->>UI: build complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
github-actions
bot
added
enhancement
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9867 +/- ##
=======================================
Coverage 22.86% 22.86%
=======================================
Files 1086 1086
Lines 39710 39711 +1
Branches 5418 5418
=======================================
+ Hits 9078 9080 +2
+ Misses 30477 30476 -1
Partials 155 155
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/stores/flowStore.ts (1)
682-695: Fix stale stopNodeId when startNodeId is also provided (always sync at build start, and clear on completion/error/stop).Currently, if both startNodeId and stopNodeId are passed, the store’s stopNodeId is not updated (Line 682 is skipped), and Line 692 only clears when stopNodeId is falsy. This can cause edge animations to use an outdated stopNodeId.
Apply this diff to synchronize at the start of build and clear at the end/error/stop:
@@ - const edges = get().edges; + const edges = get().edges; let errors: string[] = []; + // Keep store in sync with the argument for this build (handles both presence/absence). + get().setStopNodeId(stopNodeId); @@ - } else if (stopNodeId) { - get().setStopNodeId(stopNodeId); + } else if (stopNodeId) { const upstream = getConnectedSubgraph( stopNodeId, get().nodes, edges, "upstream", ); nodesToValidate = upstream.nodes; edgesToValidate = upstream.edges; } - if (!stopNodeId) { - get().setStopNodeId(undefined); - }Also clear it on stop/error/complete:
@@ stopBuilding: () => { - get().buildController.abort(); + get().buildController.abort(); + get().setStopNodeId(undefined); @@ onBuildComplete ); } + // Clear stop marker after a build finishes + get().setStopNodeId(undefined); @@ onBuildError }); } + // Clear stop marker on error + get().setStopNodeId(undefined);
🧹 Nitpick comments (1)
src/frontend/src/stores/flowStore.ts (1)
911-916: Speed up lookups in updateEdgesRunningByNodes with a Set.ids.includes(...) is O(N) per edge. Use a Set for O(1) membership when animating many edges.
updateEdgesRunningByNodes: (ids: string[], running: boolean) => { const edges = get().edges; - - const newEdges = edges.map((edge) => { + const idSet = new Set(ids); + const newEdges = edges.map((edge) => { if ( edge.data?.sourceHandle && - ids.includes(edge.data.sourceHandle.id ?? "") && + idSet.has(edge.data.sourceHandle.id ?? "") && edge.data.sourceHandle.id !== get().stopNodeId ) { edge.animated = running; edge.className = running ? "running" : ""; } else { edge.animated = false; edge.className = "not-running"; } return edge; }); set({ edges: newEdges }); },Also confirm that “past the stop node” means edges whose source is stopNodeId. If it should include all downstream of stopNodeId, we need to filter by that subgraph, not just equality.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx(0 hunks)src/frontend/src/stores/flowStore.ts(5 hunks)src/frontend/src/types/zustand/flow/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/frontend/src/pages/FlowPage/components/flowSidebarComponent/index.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/frontend/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
src/frontend/src/**/*.{ts,tsx,js,jsx}: All frontend TypeScript and JavaScript code should be located under src/frontend/src/ and organized into components, pages, icons, stores, types, utils, hooks, services, and assets directories as per the specified directory layout.
Use React 18 with TypeScript for all UI components in the frontend.
Format all TypeScript and JavaScript code using the make format_frontend command.
Lint all TypeScript and JavaScript code using the make lint command.
Files:
src/frontend/src/types/zustand/flow/index.tssrc/frontend/src/stores/flowStore.ts
src/frontend/src/types/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
All TypeScript type definitions should be placed in the types directory.
Files:
src/frontend/src/types/zustand/flow/index.ts
src/frontend/src/stores/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
Use Zustand for state management in frontend stores.
Files:
src/frontend/src/stores/flowStore.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 10/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 11/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 4/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 9/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 5/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 8/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 6/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 7/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 3/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 2/11
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 1/11
- GitHub Check: Run Frontend Unit Tests / Frontend Jest Unit Tests
- GitHub Check: Test Starter Templates
🔇 Additional comments (1)
src/frontend/src/types/zustand/flow/index.ts (1)
292-294: stopNodeId and setter added — looks good; confirm reset semantics.Type additions align with the store usage. Please confirm the field is cleared on flow resets and after builds complete/abort to avoid stale UI state.
| stopNodeId: undefined, | ||
| setStopNodeId: (nodeId: string | undefined) => { | ||
| set({ stopNodeId: nodeId }); | ||
| }, |
There was a problem hiding this comment.
Ensure stopNodeId resets with flow lifecycle.
stopNodeId isn’t cleared in resetFlow or resetFlowState, which can leave animations disabled for future flows.
Add clearing in both places:
@@ resetFlow: (flow) => {
set({
nodes,
edges: newEdges,
flowState: undefined,
buildInfo: null,
inputs,
outputs,
hasIO: inputs.length > 0 || outputs.length > 0,
flowPool: {},
currentFlow: flow,
positionDictionary: {},
});
+ // Clear any previous stop marker when loading a new/other flow
+ get().setStopNodeId(undefined);
},
@@ resetFlowState: () => {
set({
nodes: [],
edges: [],
flowState: undefined,
hasIO: false,
inputs: [],
outputs: [],
flowPool: {},
currentFlow: undefined,
reactFlowInstance: null,
lastCopiedSelection: null,
verticesBuild: null,
flowBuildStatus: {},
buildInfo: null,
isBuilding: false,
isPending: true,
positionDictionary: {},
componentsToUpdate: [],
});
+ // Clear stop marker on full reset
+ get().setStopNodeId(undefined);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stopNodeId: undefined, | |
| setStopNodeId: (nodeId: string | undefined) => { | |
| set({ stopNodeId: nodeId }); | |
| }, | |
| stopNodeId: undefined, | |
| setStopNodeId: (nodeId: string | undefined) => { | |
| set({ stopNodeId: nodeId }); | |
| }, | |
| resetFlow: (flow) => { | |
| set({ | |
| nodes, | |
| edges: newEdges, | |
| flowState: undefined, | |
| buildInfo: null, | |
| inputs, | |
| outputs, | |
| hasIO: inputs.length > 0 || outputs.length > 0, | |
| flowPool: {}, | |
| currentFlow: flow, | |
| positionDictionary: {}, | |
| }); | |
| // Clear any previous stop marker when loading a new/other flow | |
| get().setStopNodeId(undefined); | |
| }, | |
| resetFlowState: () => { | |
| set({ | |
| nodes: [], | |
| edges: [], | |
| flowState: undefined, | |
| hasIO: false, | |
| inputs: [], | |
| outputs: [], | |
| flowPool: {}, | |
| currentFlow: undefined, | |
| reactFlowInstance: null, | |
| lastCopiedSelection: null, | |
| verticesBuild: null, | |
| flowBuildStatus: {}, | |
| buildInfo: null, | |
| isBuilding: false, | |
| isPending: true, | |
| positionDictionary: {}, | |
| componentsToUpdate: [], | |
| }); | |
| // Clear stop marker on full reset | |
| get().setStopNodeId(undefined); | |
| }, |
🤖 Prompt for AI Agents
In src/frontend/src/stores/flowStore.ts around lines 1084 to 1087, stopNodeId is
defined and setter exists but the value is not cleared when a flow is reset;
update both resetFlow and resetFlowState to explicitly set stopNodeId to
undefined so animations aren't permanently disabled for subsequent flows, i.e.,
add logic in each reset function to call the existing setter or directly set
stopNodeId to undefined as part of the state reset.
github-actions
bot
added
enhancement
|
#9867) ✨ (flowSidebarComponent/index.tsx): Remove unnecessary console.log statement ♻️ (flowStore.ts): Refactor useFlowStore to include a stopNodeId property and setter function for better state management and flow control. Co-authored-by: Carlos Coelho <80289056+carlosrcoelho@users.noreply.github.com>
4 participants
This pull request introduces improvements to the flow state management in the frontend, primarily by adding support for tracking and updating the
stopNodeIdin the flow store. This change enables more precise control over flow validation and edge animation, particularly when a stop node is specified. Additionally, some code cleanup was performed.Flow store enhancements:
stopNodeIdandsetStopNodeIdto theFlowStoreTypeinterface and store implementation, allowing the flow to track which node is designated as the stopping point and update it as needed. [1] [2]stopNodeIdduring flow validation, ensuring the store accurately reflects the current stopping node. [1] [2]Edge animation logic:
stopNodeId, preventing running animations on edges beyond the stop node.Code cleanup:
https://www.loom.com/share/1c3779ff306447998860b5f2a32f359f
Summary by CodeRabbit