Open
Conversation
Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
…uit-analysis-loader. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
📝 WalkthroughWalkthroughThis pull request systematically refactors event parsing and notification handling across multiple components and type definitions. The generic type used in Changes
🚥 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: 2
🧹 Nitpick comments (2)
src/types/notification-types.ts (1)
731-735: Type guard does not narrow the type.The function
isSpreadsheetNodeAliasesUpdatedNotificationreturnsnotif is CommonStudyEventData, which is the same as the input type. This makes the type guard ineffective for narrowing purposes. Consider either:
- Creating a specific
SpreadsheetNodeAliasesUpdatedEventDatainterface with appropriate headers, or- Removing the type predicate if narrowing isn't needed.
Suggested fix
-export function isSpreadsheetNodeAliasesUpdatedNotification( - notif: CommonStudyEventData -): notif is CommonStudyEventData { +export function isSpreadsheetNodeAliasesUpdatedNotification(notif: CommonStudyEventData): boolean { return notif.headers?.updateType === NotificationType.SPREADSHEET_NODE_ALIASES_UPDATED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/notification-types.ts` around lines 731 - 735, The type guard is ineffective because isSpreadsheetNodeAliasesUpdatedNotification currently claims the input is the same type (notif is CommonStudyEventData); update it to narrow to a new specific type (e.g., SpreadsheetNodeAliasesUpdatedEventData) that extends CommonStudyEventData and declares the expected headers/updateType shape, then change the function signature to return notif is SpreadsheetNodeAliasesUpdatedEventData and keep the runtime check notif.headers?.updateType === NotificationType.SPREADSHEET_NODE_ALIASES_UPDATED; alternatively, if no narrower type is needed, remove the type predicate and make the function return boolean instead, updating all call sites accordingly (referencing isSpreadsheetNodeAliasesUpdatedNotification, CommonStudyEventData, and NotificationType.SPREADSHEET_NODE_ALIASES_UPDATED).src/components/graph/menus/root-network/use-root-network-notifications.ts (1)
109-117: Consider consolidating notification listeners.Three separate
useNotificationsListenercalls are registered for the sameNotificationsUrlKeys.STUDYchannel. While this works, it could be simplified into a single listener that handles all three notification types. This is a minor optimization that could be addressed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/graph/menus/root-network/use-root-network-notifications.ts` around lines 109 - 117, Consolidate the three useNotificationsListener calls for NotificationsUrlKeys.STUDY into a single call: create one listener callback (e.g., a wrapper function) that inspects the incoming notification payload (type/event field) and delegates to rootNetworksUpdatedNotification, rootNetworksUpdateFailedNotification, or rootNetworkDeletionStartedNotification as appropriate, then pass that wrapper as listenerCallbackMessage to useNotificationsListener(NotificationsUrlKeys.STUDY); this keeps the same behavior but registers only one listener.
🤖 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/types/notification-types.ts`:
- Around line 686-688: Rename the predicate function
isNodSubTreeCreatedNotification to isNodeSubTreeCreatedNotification in the
declaration inside notification-types (update the export name) and update all
imports/usages to the new name (e.g., inside useRootNetworkSearchNotifications
and any other modules importing it); ensure TypeScript types and any type-guard
signatures remain identical so only the identifier changes.
- Around line 384-387: The code in use-get-study-impacts.ts parses
eventData.payload with JSON.parse without guarding for undefined; update the
logic (where JSON.parse(eventData.payload) is called) to first check that
eventData.payload is a non-empty string (or truthy) and only then attempt
JSON.parse, and wrap the parse in a try-catch similar to
use-workspace-notifications.ts so undefined or invalid JSON does not throw —
reference the CommonStudyEventData type, the eventData variable, and the
JSON.parse call in use-get-study-impacts.ts to locate and fix the issue.
---
Nitpick comments:
In `@src/components/graph/menus/root-network/use-root-network-notifications.ts`:
- Around line 109-117: Consolidate the three useNotificationsListener calls for
NotificationsUrlKeys.STUDY into a single call: create one listener callback
(e.g., a wrapper function) that inspects the incoming notification payload
(type/event field) and delegates to rootNetworksUpdatedNotification,
rootNetworksUpdateFailedNotification, or rootNetworkDeletionStartedNotification
as appropriate, then pass that wrapper as listenerCallbackMessage to
useNotificationsListener(NotificationsUrlKeys.STUDY); this keeps the same
behavior but registers only one listener.
In `@src/types/notification-types.ts`:
- Around line 731-735: The type guard is ineffective because
isSpreadsheetNodeAliasesUpdatedNotification currently claims the input is the
same type (notif is CommonStudyEventData); update it to narrow to a new specific
type (e.g., SpreadsheetNodeAliasesUpdatedEventData) that extends
CommonStudyEventData and declares the expected headers/updateType shape, then
change the function signature to return notif is
SpreadsheetNodeAliasesUpdatedEventData and keep the runtime check
notif.headers?.updateType === NotificationType.SPREADSHEET_NODE_ALIASES_UPDATED;
alternatively, if no narrower type is needed, remove the type predicate and make
the function return boolean instead, updating all call sites accordingly
(referencing isSpreadsheetNodeAliasesUpdatedNotification, CommonStudyEventData,
and NotificationType.SPREADSHEET_NODE_ALIASES_UPDATED).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 905ccd1e-82e6-4884-9e84-a5a32eb552bf
📒 Files selected for processing (9)
src/components/dialogs/parameters/use-parameters-notification.tssrc/components/graph/menus/dynamic-simulation/event-modification-scenario-editor.tsxsrc/components/graph/menus/network-modifications/network-modification-node-editor.tsxsrc/components/graph/menus/root-network/use-root-network-notifications.tssrc/components/grid-layout/cards/diagrams/singleLineDiagram/hooks/use-one-bus-shortcircuit-analysis-loader.tsxsrc/components/network/network-map-panel.tsxsrc/hooks/use-optional-loading-parameters.tssrc/hooks/use-study-path.tssrc/types/notification-types.ts
Comment on lines
+384
to
387
| export interface CommonStudyEventData { | ||
| headers: CommonStudyEventDataHeaders; | ||
| payload: undefined; | ||
| payload: string | undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for JSON.parse calls on eventData.payload without guards
rg -n -B2 -A2 'JSON\.parse\(.*\.payload\)' --type tsRepository: gridsuite/gridstudy-app
Length of output: 2310
🏁 Script executed:
#!/bin/bash
# Check the full context of JSON.parse calls with try-catch blocks
echo "=== use-workspace-notifications.ts context ==="
sed -n '36,60p' src/components/workspace/hooks/use-workspace-notifications.ts | cat -n
echo -e "\n=== use-get-study-impacts.ts context ==="
sed -n '55,70p' src/hooks/use-get-study-impacts.ts | cat -n
echo -e "\n=== use-update-equipments-on-notification.ts context ==="
sed -n '110,125p' src/components/spreadsheet-view/hooks/use-update-equipments-on-notification.ts | cat -n
echo -e "\n=== Verify type definition ==="
sed -n '384,387p' src/types/notification-types.ts | cat -nRepository: gridsuite/gridstudy-app
Length of output: 3403
🏁 Script executed:
#!/bin/bash
# Get broader context around the JSON.parse in use-get-study-impacts.ts
sed -n '50,80p' src/hooks/use-get-study-impacts.ts | cat -n
# Also search for any other JSON.parse calls on payload that might have different patterns
echo -e "\n=== Search for any other payload parsing patterns ==="
rg 'payload' src/hooks/use-get-study-impacts.ts -B3 -A3 | cat -nRepository: gridsuite/gridstudy-app
Length of output: 2400
🏁 Script executed:
#!/bin/bash
# Get the full function context - find where this callback starts
# Look backwards from line 50
sed -n '30,80p' src/hooks/use-get-study-impacts.ts | cat -nRepository: gridsuite/gridstudy-app
Length of output: 2799
🏁 Script executed:
#!/bin/bash
# Comprehensive search for all JSON.parse calls involving payload
echo "=== All JSON.parse patterns with payload ==="
rg 'JSON\.parse.*payload' --type ts -n
# Also check if there are try-catch blocks in the full handleStudyNotification function
echo -e "\n=== Full handleStudyNotification function ==="
grep -n 'handleStudyNotification' src/hooks/use-get-study-impacts.ts
# Get the full function using line numbers
sed -n '49,100p' src/hooks/use-get-study-impacts.ts | head -50Repository: gridsuite/gridstudy-app
Length of output: 3182
Add null check for payload in use-get-study-impacts.ts before parsing.
CommonStudyEventData.payload is typed as string | undefined, but use-get-study-impacts.ts line 62 calls JSON.parse(eventData.payload) without a guard. If payload is undefined, this will throw a runtime error. The other call sites in use-workspace-notifications.ts and use-update-equipments-on-notification.ts already have proper try-catch error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/notification-types.ts` around lines 384 - 387, The code in
use-get-study-impacts.ts parses eventData.payload with JSON.parse without
guarding for undefined; update the logic (where JSON.parse(eventData.payload) is
called) to first check that eventData.payload is a non-empty string (or truthy)
and only then attempt JSON.parse, and wrap the parse in a try-catch similar to
use-workspace-notifications.ts so undefined or invalid JSON does not throw —
reference the CommonStudyEventData type, the eventData variable, and the
JSON.parse call in use-get-study-impacts.ts to locate and fix the issue.
Comment on lines
+686
to
688
| export function isNodSubTreeCreatedNotification(notif: CommonStudyEventData): notif is SubtreeCreatedEventData { | ||
| return notif.headers?.updateType === NotificationType.SUBTREE_CREATED; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of the misspelled function name
rg -n "isNodSubTreeCreatedNotification"Repository: gridsuite/gridstudy-app
Length of output: 503
Rename function to fix typo: isNodSubTreeCreatedNotification → isNodeSubTreeCreatedNotification
The function name is missing the letter 'e' in "Node". Update the function definition and its imports/usages across the codebase for consistency.
Changes required
src/types/notification-types.ts:686
-export function isNodSubTreeCreatedNotification(notif: CommonStudyEventData): notif is SubtreeCreatedEventData {
+export function isNodeSubTreeCreatedNotification(notif: CommonStudyEventData): notif is SubtreeCreatedEventData {
return notif.headers?.updateType === NotificationType.SUBTREE_CREATED;
}src/components/graph/menus/root-network/use-root-network-search-notifications.ts:19, :46
- Update the import and usage of this function accordingly
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/notification-types.ts` around lines 686 - 688, Rename the predicate
function isNodSubTreeCreatedNotification to isNodeSubTreeCreatedNotification in
the declaration inside notification-types (update the export name) and update
all imports/usages to the new name (e.g., inside
useRootNetworkSearchNotifications and any other modules importing it); ensure
TypeScript types and any type-guard signatures remain identical so only the
identifier changes.
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.