Handle the modification applicability per root network tag - #1281
Handle the modification applicability per root network tag#1281flomillot wants to merge 12 commits into
Conversation
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe table replaces excluded-modification state with tag-specific applicability mappings. Renderers pass this state to Network modification applicability
Sequence Diagram(s)sequenceDiagram
participant NetworkModificationsTable
participant RootNetworkCellRenderer
participant RootNetworkChipCell
participant networkModification
NetworkModificationsTable->>RootNetworkCellRenderer: Pass applicabilities and setApplicabilities
RootNetworkCellRenderer->>RootNetworkChipCell: Pass applicability state and setter
RootNetworkChipCell->>RootNetworkChipCell: Toggle tag-specific applicability optimistically
RootNetworkChipCell->>networkModification: Update modification status with applicable
networkModification-->>RootNetworkChipCell: Return success or failure
RootNetworkChipCell->>RootNetworkChipCell: Restore prior applicability on failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/features/network-modification-table/renderers/root-network-chip-cell.tsx`:
- Around line 80-83: Update the optimistic update in the component’s chip
activation handler to pass a functional updater to setApplicabilities, deriving
the toggled map from the latest state rather than captured applicabilities while
preserving each tag’s independent update. Add a test covering two distinct
root-network tags activated within one batched interaction and verify both
updates remain applied.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d5ece4d-54b0-4364-8d39-e8bf1a8a5940
📒 Files selected for processing (5)
src/features/network-modification-table/network-modifications-table.tsxsrc/features/network-modification-table/renderers/cell-renderers.tsxsrc/features/network-modification-table/renderers/root-network-chip-cell.tsxsrc/module-tanstack.d.tssrc/utils/types/network-modification-types.ts
Follows the study server rename: the endpoint sets the applicability of a modification on one root network, not its global activation flag. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
…sx` for clarity Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
…fication-applicability # Conflicts: # src/features/network-modification-table/renderers/root-network-chip-cell.tsx
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 (2)
src/features/network-modification-table/network-modifications-table.tsx (2)
161-166: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard forced child refreshes against stale responses. If two
force=truerefreshes overlap, an older response can replace newer children becausefetchSubModificationsForExpandedRowshas no request-generation or supersession check. Apply only the latest response, or cancel previous refreshes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/network-modification-table/network-modifications-table.tsx` around lines 161 - 166, Update fetchSubModificationsForExpandedRows and its callers to track refresh generations or otherwise supersede in-flight force=true requests, applying child-modification results only when they belong to the latest refresh. Ensure older overlapping responses cannot overwrite newer children while preserving normal expanded-row loading behavior.
184-185: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove
fetchSubModificationsForExpandedRowsout of the state updater.At
network-modifications-table.tsx:184-185, the updater performs the async fetch. React requires updater functions to be pure and may call them twice in Strict Mode. The helper starts network requests before its firstawait, so duplicate updater calls can start duplicate requests and enqueue duplicate child updates. Trigger the fetch from a post-commit path, and keep the updater limited to returning the next state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/network-modification-table/network-modifications-table.tsx` around lines 184 - 185, The setComposedModifications updater must remain pure: remove fetchSubModificationsForExpandedRows from its callback and have it only return the next state. Trigger the fetch from a post-commit path, such as an effect keyed to the committed expanded-row state, while preserving the existing newlyExpandedRowKeys behavior and child updates.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/features/network-modification-table/network-modifications-table.tsx`:
- Around line 161-166: Update fetchSubModificationsForExpandedRows and its
callers to track refresh generations or otherwise supersede in-flight force=true
requests, applying child-modification results only when they belong to the
latest refresh. Ensure older overlapping responses cannot overwrite newer
children while preserving normal expanded-row loading behavior.
- Around line 184-185: The setComposedModifications updater must remain pure:
remove fetchSubModificationsForExpandedRows from its callback and have it only
return the next state. Trigger the fetch from a post-commit path, such as an
effect keyed to the committed expanded-row state, while preserving the existing
newlyExpandedRowKeys behavior and child updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dcd1460-e57d-4205-b957-d1fdea7040b4
📒 Files selected for processing (4)
src/features/network-modification-table/network-modifications-table.tsxsrc/features/network-modification-table/renderers/cell-renderers.tsxsrc/features/network-modification-table/renderers/root-network-chip-cell.tsxsrc/services/networkModification.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features/network-modification-table/renderers/cell-renderers.tsx
- src/features/network-modification-table/renderers/root-network-chip-cell.tsx
- src/services/networkModification.ts
Also make the optimistic update and its rollback use the value captured at click time instead of blindly toggling the current state. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
Mathieu-Deharbe
left a comment
Mathieu-Deharbe
left a comment
There was a problem hiding this comment.
Ok for me. I didn't approve yet because I have some questions and need to check gridstudy.
| function withApplicability( | ||
| applicabilities: NetworkModificationApplicabilities, | ||
| modificationUuid: UUID, | ||
| tag: string, | ||
| applicable: boolean | ||
| ): NetworkModificationApplicabilities { |
There was a problem hiding this comment.
This function is fine but I am a bit confused by the naming. Why 'with' ?
| function withApplicability( | |
| applicabilities: NetworkModificationApplicabilities, | |
| modificationUuid: UUID, | |
| tag: string, | |
| applicable: boolean | |
| ): NetworkModificationApplicabilities { | |
| function updateAnApplicability( | |
| prevApplicabilities: NetworkModificationApplicabilities, | |
| modificationUuid: UUID, | |
| tag: string, | |
| applicable: boolean | |
| ): NetworkModificationApplicabilities { |
There was a problem hiding this comment.
What about setApplicability ?
| @@ -88,17 +69,8 @@ export function RootNetworkChipCell(props: RootNetworkChipCellProps) { | |||
| data.type === ModificationType.MODIFICATION_REFERENCE || data.childFromShared; | |||
|
|
|||
| const isModificationActivated = useMemo(() => { | |||
There was a problem hiding this comment.
I think this should be called "applicable" or "applied", instead of activated : to avoid any confusion with the activated boolean that all modifications have.
| const isModificationActivated = useMemo(() => { | |
| const isModificationApplied = useMemo(() => { |
There was a problem hiding this comment.
Yeah, I did not know which name to choose...
I don't think applied is correct either. Because the modification is not necessarily applied yet.
And we have the same issue if we choose applicable.
There was a problem hiding this comment.
@flomillot, use isModificationApplicable.
applicable matches the state stored in NetworkModificationApplicabilities and the applicable backend parameter. It also avoids the implication that the modification has already been applied.
const isModificationApplicable = useMemo(() => {
return isApplicableOn(applicabilities, modificationUuid, rootNetwork.rootNetworkUuid);
}, [modificationUuid, applicabilities, rootNetwork.rootNetworkUuid]);isModificationActivated is only required at the ActivableChip API boundary, where it is the component prop name.
You are interacting with an AI system.
| tooltipMessage={rootNetwork.name} | ||
| isActivated={isModificationActivated} | ||
| isDisabled={isLoading || isDisabled || isReferenceModificationOrInsideOne} | ||
| isDisabled={isLoading || isDisabled || isReferenceModificationOrInsideOne || rootNetwork.isCreating} |
There was a problem hiding this comment.
I think that this disable when rootNetwork.isCreating won't be approved by Stephane. It gives the impression that the applicability changed when they are enabled because the loading is over. And given that gridstudy is multi user it will be confusing.
A loading mode/icon for chip will probably be needed. But it could be in a separate ticket.
Every modification carries its own applicability per root network tag, so the table indexes them itself instead of receiving them from its parent, and the two props leave the public API. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
…fication-applicability
The modifications carry their applicability keyed by the root network tag, which renaming a root network changes under the table: the entries in hand then match no tag and every root network reads back as applicable. The tag is resolved to the root network uuid on collect, so what the table holds survives a rename; the tag stays a label. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…fication-applicability Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…fication-applicability
|
3 participants
Depends on gridsuite/study-server#1053.
The applicability of a modification per root network is now held by the modification itself, keyed by root network tag, and study-server exposes it as is instead of a list of exclusions per root network.
ExcludedNetworkModificationsis replaced byNetworkModificationApplicabilities(Record<UUID, Record<UUID, boolean>>), where a root network without an entry is applicable. The modifications carry it keyed by root network tag; it is resolved to the root network uuid on collect, so renaming a root network does not leave the table holding entries under a name nothing matches any more — which would read back as applicable until the modifications are fetched again.RootNetworkChipCellreads and toggles that map onrootNetwork.rootNetworkUuid, which drops the list lookup and the branching that rebuilt the exclusions. The tag stays what the chip is labelled with.toExclude/setToExcludeleave the table meta with no replacement in the public API:NetworkModificationsTableindexes the applicability that each modification carries, and holds it in its own state so a toggle shows immediately, before the refresh the notification triggers.updateModificationStatusByRootNetworksendsapplicableinstead ofactivated, following the study server rename. That parameter discriminates the mapping, so this PR and Use the modification applicability per root network tag study-server#1053 must be released together.Goes with gridsuite/gridstudy-app#4137.