Improve network modifications table rendering performance#3931
Conversation
Stop unmounting cells (and resetting their local state like SwitchCell's modificationActivated) on every columns rebuild by hoisting all cell/header renderers to module scope and routing dynamic values through react-table meta instead of closure capture. - Hoist renderers to a new cell-renderers.tsx (DragHandle, Select, Name, Description, Switch, RootNetwork) - Replace createBaseColumns factory with a static BASE_COLUMNS constant - Build the columns array directly in the editor and pass it as a `columns` prop instead of a createAllColumns callback - Augment react-table TableMeta/ColumnMeta to type the meta channel - Drop the optimistic global update from SwitchCell; the row state is refreshed by the server notification, and a local useState handles the immediate checkbox feedback with rollback on error - Replace useState/useEffect in useIsAnyNodeBuilding with a direct selector - Remove the now-broken inline debounce around handleCellClick Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR extracts table cell/header renderers, exports a constant BASE_COLUMNS, updates root-network column creation to pass per-column meta, expands react-table metadata types, moves switch activation handling to local state with API calls, simplifies a hook to use a selector, and wires computed columns into the node editor without debouncing. ChangesNetwork Modification Table Rendering & State Refactor
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/graph/menus/network-modifications/network-modification-node-editor.tsx (1)
1082-1095:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-null assertion on
currentRootNetworkUuidcould mask a runtime null.
currentRootNetworkUuid!is only evaluated whenisMonoRootStudyisfalse, but there's a potential initial-state window whereisMonoRootStudyis alreadyfalsewhilecurrentRootNetworkUuidhasn't been populated yet. If that occurs,createRootNetworksColumnsreceivesnullcast asstring, and therootNetworkUuid === currentRootNetworkUuidcomparison inside the function would silently fail (allisCurrentRootNetworkvalues would befalse).Consider guarding the call:
🔧 Proposed guard
- : createRootNetworksColumns( - rootNetworks, - currentRootNetworkUuid!, - modificationsToExclude, - setModificationsToExclude - )), + : currentRootNetworkUuid + ? createRootNetworksColumns( + rootNetworks, + currentRootNetworkUuid, + modificationsToExclude, + setModificationsToExclude + ) + : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/graph/menus/network-modifications/network-modification-node-editor.tsx` around lines 1082 - 1095, The code currently passes currentRootNetworkUuid with a non-null assertion into createRootNetworksColumns inside the columns useMemo; instead, guard the call so createRootNetworksColumns is only invoked when isMonoRootStudy is false AND currentRootNetworkUuid is defined (truthy), e.g. branch so that when currentRootNetworkUuid is null/undefined the spread is an empty array; reference the constants/identifiers columns, useMemo, isMonoRootStudy, currentRootNetworkUuid and createRootNetworksColumns when applying this check.
🤖 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/graph/menus/network-modifications/network-modification-table/renderers/switch-cell.tsx`:
- Around line 52-58: The Promise chain uses optional chaining (?.catch) which
can short-circuit and cause a TypeError that prevents the finally block from
running; change the call to invoke the Promise directly (remove the ? operator)
so that setModificationMetadata(...) returns a Promise and you call .catch(...)
and .finally(...) on it; keep the existing rollback via
setModificationActivated(data?.activated), the error reporting via
snackWithFallback(snackError, error, { headerId:
'networkModificationActivationError' }), and the setIsLoading(false) in finally
to guarantee isLoading is always cleared.
---
Outside diff comments:
In
`@src/components/graph/menus/network-modifications/network-modification-node-editor.tsx`:
- Around line 1082-1095: The code currently passes currentRootNetworkUuid with a
non-null assertion into createRootNetworksColumns inside the columns useMemo;
instead, guard the call so createRootNetworksColumns is only invoked when
isMonoRootStudy is false AND currentRootNetworkUuid is defined (truthy), e.g.
branch so that when currentRootNetworkUuid is null/undefined the spread is an
empty array; reference the constants/identifiers columns, useMemo,
isMonoRootStudy, currentRootNetworkUuid and createRootNetworksColumns when
applying this check.
🪄 Autofix (Beta)
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
Run ID: ee78b7ad-c00f-46cf-ac9f-34d80fabc9f7
📒 Files selected for processing (7)
src/components/graph/menus/network-modifications/PERF-ANALYSIS.mdsrc/components/graph/menus/network-modifications/network-modification-node-editor.tsxsrc/components/graph/menus/network-modifications/network-modification-table/createColumns.tsxsrc/components/graph/menus/network-modifications/network-modification-table/renderers/cell-renderers.tsxsrc/components/graph/menus/network-modifications/network-modification-table/renderers/switch-cell.tsxsrc/components/utils/is-any-node-building-hook.tssrc/module-tanstack.d.ts
- Clarify the importance of stable references for cell/header renderers. - Update explanatory comments about renderers' scope and usage in react-table. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
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/components/graph/menus/network-modifications/network-modification-table/renderers/switch-cell.tsx`:
- Around line 32-37: The local state modificationActivated and its updater
setModificationActivated are initialized and synced from data?.activated which
may be undefined and cause a controlled/uncontrolled Switch; coerce values to a
strict boolean when setting state and in the useEffect (e.g. wrap
data?.activated with !!) and also update the other occurrence at the second
state sync (around line referenced 53) so the Switch always receives true/false.
🪄 Autofix (Beta)
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
Run ID: e029946e-ff44-4265-a792-b73be09bcb75
📒 Files selected for processing (2)
src/components/graph/menus/network-modifications/network-modification-table/renderers/cell-renderers.tsxsrc/components/graph/menus/network-modifications/network-modification-table/renderers/switch-cell.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/graph/menus/network-modifications/network-modification-table/renderers/cell-renderers.tsx
|
|
||
| const toggleModificationActive = useCallback(() => { | ||
| setIsLoading(true); | ||
| setModifications((oldModifications) => { |
There was a problem hiding this comment.
No risk of stale data if we remove the dataset update ? Although that may have been part of the perf issues I guess
There was a problem hiding this comment.
What do you mean exactly ? Do you have an example ? If you're talking about this set modification, it was useless because it was already updated right after by server notification.
There was a problem hiding this comment.
I mean up until now the state of the component was directly derived from the value contained in the dataset and with this change it is now driven by a state which replicates the dataset value. That said maybe it was part of the issue
| // Read at runtime via `column.columnDef.meta` (per-column). | ||
| // TData / TValue must match the original generic signature of ColumnMeta for the | ||
| // module-augmentation merge to apply. They aren't referenced in this body, hence the disable. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
It may be relevant to ignore d.ts files for no-unused-vars rule in eslint config
The per-row cells, the cell-renderers adapter, the two services (setModificationMetadata, updateModificationStatusByRootNetwork) and the @tanstack/react-table module augmentation that they relied on have moved to commons-ui. Local files are deleted and createColumns now imports the renderers from commons-ui. createRootNetworksColumns is simplified: it only needs the root-networks list since the rest of the per-column meta (rootNetwork, modificationsToExclude, isCurrentRootNetwork, currentRootNetworkTag) is now derived from TableMeta. The node editor passes studyUuid, currentNodeId, currentRootNetworkUuid, rootNetworks, modificationsToExclude, setModificationsToExclude and a combined isDisabled (isAnyNodeBuilding || mapDataLoading) to NetworkModificationsTable. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
This reverts commit 6c47b85.
…rendering # Conflicts: # src/components/graph/menus/network-modifications/network-modification-node-editor.tsx # src/components/graph/menus/network-modifications/network-modification-table/createColumns.tsx
Thread the optimistic name edit handler through the NAME column's ColumnMeta (same pattern as createRootNetworksColumns) instead of the previously removed createBaseColumns parameter. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
2 participants
Summary
Stop unmounting cells (and resetting their local state like
SwitchCell'smodificationActivated) on every columns rebuild by hoisting all cell/header renderers to module scope and routing dynamic values through react-tablemetainstead of closure capture.cell-renderers.tsx(DragHandle, Select, Name, Description, Switch, RootNetwork).- ReplacePOSTPONED due to merge conflictscreateBaseColumnsfactory with a staticBASE_COLUMNSconstant.columnsprop instead of acreateAllColumnscallback (depends on commons-ui PR).TableMeta/ColumnMetato type the meta channel.SwitchCell; the row state is refreshed by the server notification, and a localuseStatehandles the immediate checkbox feedback with rollback on error.useState/useEffectinuseIsAnyNodeBuildingwith a direct selector.debouncearoundhandleCellClick.Depends on gridsuite/commons-ui#perf/network-modifications-table-meta.