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>
📝 WalkthroughWalkthroughReplaced ad-hoc JSON parsing and usages of 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
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/types/notification-types.ts (1)
699-700:⚠️ Potential issue | 🟡 MinorFix typo in exported predicate name (
isNodSubTreeCreatedNotification).Line 699 still exposes the misspelled public function name. Please rename it to
isNodeSubTreeCreatedNotificationand update its imports/usages.✏️ Proposed rename
-export function isNodSubTreeCreatedNotification(notif: CommonStudyEventData): notif is SubtreeCreatedEventData { +export function isNodeSubTreeCreatedNotification(notif: CommonStudyEventData): notif is SubtreeCreatedEventData { return notif.headers?.updateType === NotificationType.SUBTREE_CREATED; }🤖 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 699 - 700, The exported predicate function has a typo: rename isNodSubTreeCreatedNotification to isNodeSubTreeCreatedNotification and keep its implementation and type guard signature (notif: CommonStudyEventData): notif is SubtreeCreatedEventData unchanged; update all references/imports/usages of isNodSubTreeCreatedNotification throughout the codebase to the new name and ensure any exports or re-exports reflect isNodeSubTreeCreatedNotification, leaving the NotificationType.SUBTREE_CREATED comparison intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/types/notification-types.ts`:
- Around line 699-700: The exported predicate function has a typo: rename
isNodSubTreeCreatedNotification to isNodeSubTreeCreatedNotification and keep its
implementation and type guard signature (notif: CommonStudyEventData): notif is
SubtreeCreatedEventData unchanged; update all references/imports/usages of
isNodSubTreeCreatedNotification throughout the codebase to the new name and
ensure any exports or re-exports reflect isNodeSubTreeCreatedNotification,
leaving the NotificationType.SUBTREE_CREATED comparison intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65a95292-a684-4d68-abfe-dd9712a4385d
📒 Files selected for processing (1)
src/types/notification-types.ts
- Consolidated notification callbacks into `rootNetworkNotificationHandler`. - Fixed typos in notification type names (e.g., `isNodSubTreeCreatedNotification` -> `isNodeSubTreeCreatedNotification`). - Improved event data parsing with `parseEventData` utility. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/use-get-study-impacts.ts (1)
56-72:⚠️ Potential issue | 🟠 MajorUnguarded
JSON.parse()can throw and break notification handling.Line 58 guards against missing
payload, but Line 72'sJSON.parse(eventData.payload)remains unprotected. Malformed JSON in a valid payload string throwsSyntaxErroruncaught in the callback, breaking subsequent notification processing.Suggested hardening
- const { - impactedSubstationsIds: substationsIds, - deletedEquipments, - impactedElementTypes, - } = JSON.parse(eventData.payload) as NetworkImpactsInfos; + let impacts: NetworkImpactsInfos; + try { + impacts = JSON.parse(eventData.payload) as NetworkImpactsInfos; + } catch { + return; + } + + const { + impactedSubstationsIds: substationsIds, + deletedEquipments, + impactedElementTypes, + } = impacts;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-get-study-impacts.ts` around lines 56 - 72, The notification handler uses JSON.parse(eventData.payload) unguarded inside the use-get-study-impacts hook (after parseEventData and isStudyNotification checks), which can throw on malformed payloads; wrap the parse in a try/catch around the JSON.parse call that produces impactedSubstationsIds/deletedEquipments/impactedElementTypes (the destructuring from JSON.parse(eventData.payload) as NetworkImpactsInfos), log or handle the parse error (e.g., processLogger.warn or a local logger) and return early so the callback continues safely when payload is invalid, keeping the existing guards for node/rootNetwork (currentRootNetworkUuid and currentNode?.id) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/hooks/use-get-study-impacts.ts`:
- Around line 56-72: The notification handler uses JSON.parse(eventData.payload)
unguarded inside the use-get-study-impacts hook (after parseEventData and
isStudyNotification checks), which can throw on malformed payloads; wrap the
parse in a try/catch around the JSON.parse call that produces
impactedSubstationsIds/deletedEquipments/impactedElementTypes (the destructuring
from JSON.parse(eventData.payload) as NetworkImpactsInfos), log or handle the
parse error (e.g., processLogger.warn or a local logger) and return early so the
callback continues safely when payload is invalid, keeping the existing guards
for node/rootNetwork (currentRootNetworkUuid and currentNode?.id) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef74b0b2-0d93-4deb-98ba-53d4c771def3
📒 Files selected for processing (4)
src/components/graph/menus/root-network/use-root-network-notifications.tssrc/components/graph/menus/root-network/use-root-network-search-notifications.tssrc/hooks/use-get-study-impacts.tssrc/types/notification-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/notification-types.ts
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.