Performance: Decouple state.flow.nodes into state.flow.nodes and state.flow.nodeConfigs#68
Performance: Decouple state.flow.nodes into state.flow.nodes and state.flow.nodeConfigs#68srijanpatel merged 74 commits intomainfrom
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fc545ef in 58 seconds
More details
- Looked at
459lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/components/canvas/RunViewFlowCanvas.tsx:218
- Draft comment:
Excessive use ofkeyattributes on non-list elements can lead to unnecessary re-renders. Consider removingkeyattributes from elements that do not require them, such as single instances of components likeBackground,Operator, andNodeSidebar. This comment applies to similar instances in this file and others. - Reason this comment was not posted:
Comment was on unchanged code.
2. frontend/src/components/nodes/BaseNode.tsx:334
- Draft comment:
Excessive use ofkeyattributes on non-list elements can lead to unnecessary re-renders. Consider removingkeyattributes from elements that do not require them, such as single instances of components likeAlert,Handle,Card, andButton. This comment applies to similar instances in this file and others. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/components/nodes/DynamicNode.tsx:130
- Draft comment:
Excessive use ofkeyattributes on non-list elements can lead to unnecessary re-renders. Consider removingkeyattributes from elements that do not require them, such as single instances of components likeInputandNodeOutputDisplay. This comment applies to similar instances in this file and others. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/components/nodes/nodeSidebar/NodeSidebar.tsx:247
- Draft comment:
Excessive use ofkeyattributes on non-list elements can lead to unnecessary re-renders. Consider removingkeyattributes from elements that do not require them, such as single instances of components likeSelect,Input, andTextEditor. This comment applies to similar instances in this file and others. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_v98NVtagFZov1lxE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 63bc8a4 in 46 seconds
More details
- Looked at
112lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_ltyz5bpJOOCVh4SF
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| } | ||
| }; | ||
|
|
||
| export const getWorkflowInputVariables = ( |
There was a problem hiding this comment.
The function getWorkflowInputVariables should return an object, not an array, as it is used to access keys in the DeployModal and RunModal components.
…width calculation
…pyspur_dev into perf/flowSlicev2
Bugfix/conditional node
… in DeployModal and RunModal components
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5a23fbb in 55 seconds
More details
- Looked at
69lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/components/modals/RunModal.tsx:23
- Draft comment:
ThegetWorkflowInputVariablesfunction is removed from imports but still used in the code. This will cause a reference error. Ensure to update the code to use the new logic for obtaining workflow input variables. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/components/modals/DeployModal.tsx:15
- Draft comment:
ThegetWorkflowInputVariablesfunction is removed from imports but still used in the code. This will cause a reference error. Ensure to update the code to use the new logic for obtaining workflow input variables. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_3VzfW8gK31qQKg1e
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b798799 in 41 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/components/modals/DeployModal.tsx:31
- Draft comment:
getApiEndpoint()should be called within the code example strings to ensure the latest endpoint is used. Consider using a function to generate the code examples dynamically. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_cA0UwFhP9XFCzsac
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0092249 in 50 seconds
More details
- Looked at
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_MYT3mhQwI6ZerxWd
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| node = nodes.find(node => node.data?.config?.title === task.node_id); | ||
| // find the node by title in nodeConfigs | ||
| const state = store.getState(); | ||
| const correspondingNodeId = Object.keys(state.flow.nodeConfigs).find(key => state.flow.nodeConfigs[key].title === nodeId); |
There was a problem hiding this comment.
Consider refactoring the logic for finding a node by title in nodeConfigs into a utility function to avoid code duplication and improve maintainability.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3cf89e4 in 42 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_R96cWnN00jC0Ks7T
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| // Update source handle if this is the source node | ||
| if (edge.source === nodeId && edge.sourceHandle === oldTitle) { | ||
| updatedEdge.sourceHandle = newTitle; | ||
| updatedEdge.targetHandle = newTitle; |
There was a problem hiding this comment.
The line updatedEdge.targetHandle = newTitle; is incorrectly placed inside the condition for updating the source handle. It should be in a separate condition for updating the target handle.
There was a problem hiding this comment.
this is the intended behaviour
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f69c557 in 47 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/components/nodes/DynamicNode.tsx:75
- Draft comment:
The change fromedge.source + '_' + edge.sourceHandletoedge.targetHandlemight lead to loss of context or uniqueness in thetitle. Consider whether this change is intentional and if it maintains the necessary information. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_vFzHqsj29TWyAWkv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e71f972 in 24 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/components/nodes/InputNode.tsx:238
- Draft comment:
EnsurenodeConfig?.titleis unique across nodes to prevent potential issues with theHandlecomponent'sidprop. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_7XUFxGHkOjjKRnPa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 19f9fed in 43 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/components/Header.tsx:267
- Draft comment:
UsinginputNode?.data?.titleas an identifier may lead to unexpected behavior iftitleis not unique or intended for identification. Consider usinginputNode?.idfor a more reliable identifier. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code actually has a fallback to id if title is not available. Looking at the broader context, this change seems intentional - there's code elsewhere (L127-131) that shows the codebase sometimes needs to look up nodes by title. The comment misses that id is already used as a fallback. The change appears to be making the code more robust, not less.
I could be missing some edge cases where using title, even with an id fallback, could cause issues. There might be a good reason why using title at all is problematic.
The code's fallback pattern and the existence of title-based lookups elsewhere in the file strongly suggest this is an intentional design choice that the team has already thought through.
Delete this comment. The code already handles the concern by using id as a fallback, and the broader context shows that using title as an identifier is an intentional pattern in this codebase.
Workflow ID: wflow_4qKH7cHQIHlHH9vV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c39e4ee in 33 seconds
More details
- Looked at
138lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. frontend/src/components/nodes/logic/CoalesceNode.tsx:55
- Draft comment:
Consider extracting the logic inside theuseStateinitializer function to a separate function for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The use ofuseStatewith a function initializer is a good practice for performance optimization. However, the logic inside the initializer function is complex and could be extracted to a separate function for better readability and maintainability.
2. frontend/src/components/nodes/logic/CoalesceNode.tsx:82
- Draft comment:
The logic for computingupdatedPredecessorsis duplicated from theuseStateinitializer. Consider refactoring to avoid code duplication. - Reason this comment was not posted:
Confidence changes required:50%
The use ofuseEffectto recompute predecessor nodes is correct, but the logic is duplicated from theuseStateinitializer. This could be refactored to avoid code duplication.
3. frontend/src/components/nodes/logic/CoalesceNode.tsx:144
- Draft comment:
Consider extracting the logic inside themapfunction inuseMemoto a separate function for better readability. - Reason this comment was not posted:
Confidence changes required:33%
TheuseMemohook is used correctly to memoize theinputVariablesarray. However, the logic inside themapfunction is complex and could be extracted to a separate function for better readability.
4. frontend/src/components/nodes/logic/CoalesceNode.tsx:223
- Draft comment:
Consider extracting the logic for computingnodeWidthto a separate function for better readability. - Reason this comment was not posted:
Confidence changes required:33%
TheuseEffectfor settingnodeWidthis well-structured, but the logic for computing widths is complex and could be extracted to a separate function for better readability.
5. frontend/src/components/nodes/logic/CoalesceNode.tsx:298
- Draft comment:
Ensure thathandleIdis unique for each node to avoid potential issues with React's key prop. - Reason this comment was not posted:
Confidence changes required:33%
Thekeyprop for the input handle divs is constructed using bothnode.idandhandleId, which is a good practice to ensure unique keys. However, ensure thathandleIdis always unique for each node to avoid potential issues.
Workflow ID: wflow_z6apHbWtLnnf6cSj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
3 participants
This pull request focuses on refactoring the flow management components in the frontend, particularly by decoupling the
updateNodeDatafunction intoupdateNodeDataOnlyandupdateNodeConfigOnlyto optimize component re-renders. Additionally, there are several improvements, eg., usingReact.memo.Refactoring and Optimization:
frontend/src/components/Header.tsx: ReplacedupdateNodeDatawithupdateNodeDataOnlyin thedispatchcalls to ensure only necessary node data updates are dispatched. [1] [2]frontend/src/components/nodes/BaseNode.tsx: IntroducedReact.memowith a custom comparator functionbaseNodeComparatorto optimize re-renders by comparing only the relevant props. [1] [2] [3]frontend/src/components/nodes/DynamicNode.tsx: AppliedReact.memoand removed redundant node data lookups to improve performance. [1] [2] [3]Important
Refactor frontend flow management by decoupling node data and configuration updates, optimizing re-renders, and improving performance.
updateNodeDataintoupdateNodeDataOnlyandupdateNodeConfigOnlyinflowSlice.tsto separate node data and configuration updates.nodeConfigsinflowSlice.tsto store node configurations separately from node data.Header.tsx,BaseNode.tsx, andDynamicNode.tsxto useupdateNodeDataOnlyandupdateNodeConfigOnly.React.memotoBaseNode.tsxandDynamicNode.tsxwith custom comparators to optimize re-renders.DeployModal.tsxandRunModal.tsxto usenodeConfigsfor input variables.NodeSidebar.tsxto handle node configuration changes and title updates.createNodefunction innodeFactory.tsto generate nodes with separate configurations.flowUtils.tsxto support new node creation and connection logic.This description was created by
for c39e4ee. It will automatically update as commits are pushed.