Fix tree cross-tab navigation : sync tree model globally#3831
Open
Fix tree cross-tab navigation : sync tree model globally#3831
Conversation
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
📝 WalkthroughWalkthroughCentralizes tree-model event routing into a new exported handler ( Changes
Sequence Diagram(s)mermaid 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/network-modification-tree-pane-event-handlers.ts`:
- Around line 121-148: The switch handles notifications for all studies but only
NODE_BUILD_STATUS_UPDATED checks headers.rootNetworkUuid, so other branches
(NODE_CREATED, SUBTREE_CREATED, NODE_MOVED, SUBTREE_MOVED, NODES_DELETED,
NODES_UPDATED, NODE_EDITED) can act on the wrong tree; add a single guard using
eventData.headers.rootNetworkUuid and rootNetworkUuid at the top of the handler
(before the switch) or immediately inside the switch for every root-scoped case
to return/skip when they differ, ensuring functions like
fetchAndDispatchAddedNode, fetchAndHandleSubtree, fetchAndDispatchMovedNode,
fetchAndDispatchUpdatedNodes and fetchNetworkModificationTreeNode only run for
matching rootNetworkUuid.
- Around line 115-152: Add a switch branch for
NotificationType.NODES_COLUMN_POSITION_CHANGED inside handleTreeModelUpdate so
subtree reorder events update the shared Redux tree model even when the pane
component isn't mounted; mirror the existing checks (verify
eventData.headers.rootNetworkUuid === rootNetworkUuid if applicable) and
dispatch the same Redux action used by
src/components/network-modification-tree-pane.jsx for
NODES_COLUMN_POSITION_CHANGED (pass eventData.headers.nodes/parent info exactly
as that pane does) so the global tree order is updated consistently across tabs.
- Around line 104-113: The current fetchAndDispatchUpdatedNodes uses Promise.all
which fails the whole batch if any fetchNetworkModificationTreeNode rejects;
change it to aggregate results with Promise.allSettled (or individually catch
each fetch) so that successful node fetches are collected and passed to
dispatch(networkModificationTreeNodesUpdated(...)) while failed fetches are
logged/ignored; update fetchAndDispatchUpdatedNodes to map nodeIds to settled
results, filter for status === "fulfilled", extract values, dispatch only those
values, and optionally log the rejected reasons including the nodeId.
In `@src/components/study-container.jsx`:
- Around line 265-279: handleStudyUpdate can forward undefined into
handleTreeModelUpdate because useStudy sets studyUuid before fetchRootNetworks
resolves; modify handleStudyUpdate so it only calls handleTreeModelUpdate when
currentRootNetworkUuidRef.current is set (truthy). In the handler (function
handleStudyUpdate) add a guard around the handleTreeModelUpdate call (checking
currentRootNetworkUuidRef.current) and skip or buffer tree sync when it's
undefined, keeping the existing alert and error handling paths intact; this
prevents openStudy/notifications from forwarding undefined into the tree-update
helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2079f08-6758-4b5e-9596-69548bb04237
📒 Files selected for processing (4)
src/components/network-modification-tree-pane-event-handlers.tssrc/components/network-modification-tree-pane.jsxsrc/components/study-container.jsxsrc/hooks/use-study-navigation-sync.ts
Comment on lines
+121
to
+148
| switch (eventData.headers.updateType) { | ||
| case NotificationType.NODE_BUILD_STATUS_UPDATED: | ||
| if (eventData.headers.rootNetworkUuid !== rootNetworkUuid) break; | ||
| fetchAndDispatchUpdatedNodes(dispatch, studyUuid, rootNetworkUuid, eventData.headers.nodes); | ||
| break; | ||
| case NotificationType.NODE_CREATED: | ||
| fetchAndDispatchAddedNode(dispatch, studyUuid, rootNetworkUuid, eventData as NodeCreatedEventData); | ||
| break; | ||
| case NotificationType.SUBTREE_CREATED: | ||
| fetchAndHandleSubtree(dispatch, studyUuid, eventData.headers.newNode, eventData.headers.parentNode); | ||
| break; | ||
| case NotificationType.NODE_MOVED: | ||
| fetchAndDispatchMovedNode(dispatch, studyUuid, rootNetworkUuid, eventData as NodeMovedEventData); | ||
| break; | ||
| case NotificationType.SUBTREE_MOVED: | ||
| fetchAndHandleSubtree(dispatch, studyUuid, eventData.headers.movedNode, eventData.headers.parentNode); | ||
| break; | ||
| case NotificationType.NODES_DELETED: | ||
| dispatch(networkModificationTreeNodesRemoved(eventData.headers.nodes)); | ||
| break; | ||
| case NotificationType.NODES_UPDATED: | ||
| fetchAndDispatchUpdatedNodes(dispatch, studyUuid, rootNetworkUuid, eventData.headers.nodes); | ||
| break; | ||
| case NotificationType.NODE_EDITED: | ||
| fetchNetworkModificationTreeNode(studyUuid, eventData.headers.node, rootNetworkUuid).then((node) => | ||
| dispatch(networkModificationTreeNodesUpdated([node])) | ||
| ); | ||
| break; |
There was a problem hiding this comment.
Filter root-scoped events before mutating the active tree.
The store only holds the tree for the current root network, but this centralized handler now runs for every study notification and only the build-status branch checks headers.rootNetworkUuid. A create/move/update/edit event from another root can now fetch or remove nodes against the wrong model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/network-modification-tree-pane-event-handlers.ts` around lines
121 - 148, The switch handles notifications for all studies but only
NODE_BUILD_STATUS_UPDATED checks headers.rootNetworkUuid, so other branches
(NODE_CREATED, SUBTREE_CREATED, NODE_MOVED, SUBTREE_MOVED, NODES_DELETED,
NODES_UPDATED, NODE_EDITED) can act on the wrong tree; add a single guard using
eventData.headers.rootNetworkUuid and rootNetworkUuid at the top of the handler
(before the switch) or immediately inside the switch for every root-scoped case
to return/skip when they differ, ensuring functions like
fetchAndDispatchAddedNode, fetchAndHandleSubtree, fetchAndDispatchMovedNode,
fetchAndDispatchUpdatedNodes and fetchNetworkModificationTreeNode only run for
matching rootNetworkUuid.
Contributor
Author
There was a problem hiding this comment.
The tree is shared across all root networks by design these events correctly apply globally. Only NODE_BUILD_STATUS_UPDATED is per-root-network-scoped, and it is already filtered
There was a problem hiding this comment.
@ayolab, thank you for the clarification! That makes sense — since the tree model is shared across all root networks, node creation, movement, deletion, and update events correctly apply globally without needing a rootNetworkUuid guard. The existing filter on NODE_BUILD_STATUS_UPDATED is the only root-scoped check needed. I was mistaken in flagging this.
✏️ Learnings added
Learnt from: ayolab
Repo: gridsuite/gridstudy-app PR: 3831
File: src/components/network-modification-tree-pane-event-handlers.ts:121-148
Timestamp: 2026-03-24T11:06:27.690Z
Learning: In gridsuite/gridstudy-app, the network modification tree model (Redux store) is shared across all root networks by design. Tree structure events (NODE_CREATED, SUBTREE_CREATED, NODE_MOVED, SUBTREE_MOVED, NODES_DELETED, NODES_UPDATED, NODE_EDITED) apply globally and do not need to be filtered by rootNetworkUuid. Only NODE_BUILD_STATUS_UPDATED is per-root-network-scoped and must be filtered by rootNetworkUuid. This is relevant to `handleTreeModelUpdate` in `src/components/network-modification-tree-pane-event-handlers.ts`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/network-modification-tree-pane-event-handlers.ts`:
- Around line 149-153: The case handling
NotificationType.NODES_COLUMN_POSITION_CHANGED currently calls
JSON.parse(payload) directly which can throw; wrap the parse in a try-catch
inside that case (the switch branch in
network-modification-tree-pane-event-handlers.ts) so that you catch
SyntaxError/other exceptions from JSON.parse(payload), log or warn the parsing
error and safely skip or provide a sane fallback (e.g. empty array) before
calling dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, ...)),
ensuring dispatch is only called with a valid parsed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa0d9f1d-5ea6-4d88-91c7-e3fbca88778a
📒 Files selected for processing (4)
src/components/graph/network-modification-tree-model.tssrc/components/network-modification-tree-pane-event-handlers.tssrc/components/network-modification-tree-pane.jsxsrc/components/study-container.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/study-container.jsx
Comment on lines
+149
to
+153
| case NotificationType.NODES_COLUMN_POSITION_CHANGED: { | ||
| const { headers, payload } = eventData as NodesColumnPositionsChangedEventData; | ||
| dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, JSON.parse(payload))); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Wrap JSON.parse(payload) in try-catch to avoid unhandled exceptions.
If payload is undefined, empty, or malformed JSON, JSON.parse() throws a SyntaxError, which will propagate unhandled and potentially break subsequent event processing.
🛡️ Proposed defensive fix
case NotificationType.NODES_COLUMN_POSITION_CHANGED: {
const { headers, payload } = eventData as NodesColumnPositionsChangedEventData;
- dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, JSON.parse(payload)));
+ try {
+ dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, JSON.parse(payload)));
+ } catch (e) {
+ console.warn('NODES_COLUMN_POSITION_CHANGED: failed to parse payload', e);
+ }
break;
}📝 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.
Suggested change
| case NotificationType.NODES_COLUMN_POSITION_CHANGED: { | |
| const { headers, payload } = eventData as NodesColumnPositionsChangedEventData; | |
| dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, JSON.parse(payload))); | |
| break; | |
| } | |
| case NotificationType.NODES_COLUMN_POSITION_CHANGED: { | |
| const { headers, payload } = eventData as NodesColumnPositionsChangedEventData; | |
| try { | |
| dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, JSON.parse(payload))); | |
| } catch (e) { | |
| console.warn('NODES_COLUMN_POSITION_CHANGED: failed to parse payload', e); | |
| } | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/network-modification-tree-pane-event-handlers.ts` around lines
149 - 153, The case handling NotificationType.NODES_COLUMN_POSITION_CHANGED
currently calls JSON.parse(payload) directly which can throw; wrap the parse in
a try-catch inside that case (the switch branch in
network-modification-tree-pane-event-handlers.ts) so that you catch
SyntaxError/other exceptions from JSON.parse(payload), log or warn the parsing
error and safely skip or provide a sane fallback (e.g. empty array) before
calling dispatch(reorderNetworkModificationTreeNodes(headers.parentNode, ...)),
ensuring dispatch is only called with a valid parsed value.
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.
No description provided.