Simplify NetworkModificationsTable column API#1121
Open
flomillot wants to merge 3 commits into
Open
Conversation
Accept a stable `columns` array prop instead of a createAllColumns factory called inside an internal useMemo, so the parent owns when columns are rebuilt. - Memoize nameHeaderProps from the individual fields and forward it through react-table meta along with isRowDragDisabled and modificationsCount so column renderers can read them via table.options.meta - Augment TableMeta with the new fields and ColumnMeta with cellStyle 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 selected for processing (2)
📝 WalkthroughWalkthroughThe pull request refactors NetworkModificationsTable to decouple column definition from the component and extends the table with root network context. The table now accepts precomputed columns, root network metadata, and exclusion filters via props; consolidates all context into a memoized ChangesRoot Network Filtering Integration
🚥 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Brings the per-row cells used by NetworkModificationsTable (description, switch, root-network chip and the cell-renderers adapter) into commons-ui alongside the existing renderers. Cells receive study/node identifiers, disabled state and root-network metadata via the table's TableMeta rather than reading them from the gridstudy Redux store, so the lib stays state-agnostic. ColumnMeta is reduced back to cellStyle only; everything that was shared across columns (rootNetworks list, currentRootNetworkUuid, modificationsToExclude pair) lives in TableMeta now. Also ports the setModificationMetadata and updateModificationStatusByRootNetwork services and adds a minimal RootNetworkRowInfo type so consumers can pass their own richer root-network objects (structurally compatible). Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/network-modification-table/network-modifications-table.tsx (1)
34-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import.
The
NameHeaderPropsimport is not used in this file (flagged by pipeline). ThenameHeaderPropsobject at lines 149-154 is constructed inline without referencing this type.🧹 Proposed fix
-import { AUTO_EXTENSIBLE_COLUMNS, NameHeaderProps } from './columns-definition'; +import { AUTO_EXTENSIBLE_COLUMNS } from './columns-definition';🤖 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/components/network-modification-table/network-modifications-table.tsx` at line 34, Remove the unused import NameHeaderProps from the import statement that currently reads "import { AUTO_EXTENSIBLE_COLUMNS, NameHeaderProps } from './columns-definition';" since the file constructs the nameHeaderProps object inline and never references the NameHeaderProps type; keep AUTO_EXTENSIBLE_COLUMNS, and update the import to only include the symbols actually used (e.g., "AUTO_EXTENSIBLE_COLUMNS") to satisfy the pipeline.
🤖 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/network-modification-table/renderers/cell-renderers.tsx`:
- Around line 100-105: currentRootNetworkTag can be undefined and will break
FormattedMessage; update the code that computes or passes currentRootNetworkTag
(the variable defined via meta?.rootNetworks?.find(...)? .tag) to provide a safe
fallback (e.g. empty string or a localized "N/A") before it's used in the
Tooltip/FormattedMessage call so the {tag} placeholder is never undefined;
change the expression that sets currentRootNetworkTag or the values prop passed
to FormattedMessage to use the fallback.
In `@src/components/network-modification-table/renderers/description-cell.tsx`:
- Line 10: The import in description-cell.tsx uses a deep import ('import
IconButton from "@mui/material/IconButton"') which violates project rules;
replace it with a root-level named import by importing IconButton from
'@mui/material' (e.g., import { IconButton } from '@mui/material') so the module
is pulled from the package root and the pipeline will stop failing.
- Line 75: The call to createEditDescriptionStyle currently uses
data.description directly and can throw if data is undefined; change the
invocation in description-cell.tsx to pass a null-safe value (e.g., use optional
chaining or a default) such as createEditDescriptionStyle(data?.description ??
'') so createEditDescriptionStyle always receives a defined string; update any
similar calls in the same file (e.g., renderers that use data.description) to
follow the same pattern.
- Around line 26-31: The DescriptionCell component is currently defined as a
const arrow function which violates the project's ESLint rule requiring function
declarations; change the declaration "export const DescriptionCell:
FunctionComponent<DescriptionCellProps> = ({ data, studyUuid, currentNodeId,
isDisabled = false, }) => { ... }" into a named function declaration "export
function DescriptionCell(props: DescriptionCellProps) { ... }" (or destructure
parameters inside the function signature) preserving the same props (data,
studyUuid, currentNodeId, isDisabled with its default) and all existing
implementation details and exports so ESLint accepts the component.
In
`@src/components/network-modification-table/renderers/root-network-chip-cell.tsx`:
- Around line 69-77: The component RootNetworkChipCell is currently declared as
a const arrow function; change it to a named function declaration to satisfy
ESLint: replace the const declaration export const RootNetworkChipCell:
FunctionComponent<RootNetworkChipCellProps> = (...) => { ... } with an exported
function declaration export function RootNetworkChipCell(props:
RootNetworkChipCellProps) { const { data, studyUuid, currentNodeId, rootNetwork,
modificationsToExclude, setModificationsToExclude, isDisabled = false } = props;
... } so the signature and default for isDisabled remain the same and all
internal references to RootNetworkChipCell continue to work.
- Around line 112-117: The optimistic update to modificationsToExclude needs to
be rolled back if updateModificationStatusByRootNetwork fails: capture the
previous modificationsToExclude (e.g., const prev = modificationsToExclude)
before calling getUpdatedExcludedModifications / updateStatus, and in the
.catch(error) handler restore it by calling setModificationsToExclude(prev) (and
keep the snackWithFallback call). Update
updateStatus/getUpdatedExcludedModifications signatures if needed to return both
the new state and a rollback closure or previous state so the catch block can
reliably revert; ensure setIsLoading(false) remains in .finally().
In `@src/components/network-modification-table/renderers/switch-cell.tsx`:
- Around line 23-28: The component SwitchCell is declared as an exported const
arrow function which violates the project's ESLint rule requiring function
declarations for React components; change the declaration to a named function
declaration that accepts a single parameter of type SwitchCellProps (preserve
the prop names data, studyUuid, currentNodeId, isDisabled with its default
false) and export it (e.g., export function SwitchCell(props: SwitchCellProps) {
... }), leaving the rest of the implementation unchanged and keeping the
SwitchCellProps type annotation.
In `@src/services/networkModification.ts`:
- Around line 154-167: The query parameter serialization is wrong:
updateModificationStatusByRootNetwork currently does
urlSearchParams.append('uuids', String([modificationUuid])) which yields the
array string instead of the raw UUID; change this to append the UUID directly
(e.g., urlSearchParams.append('uuids', modificationUuid)) or build a
comma-separated string if multiple UUIDs are expected, ensuring the API receives
plain UUID text rather than String([..]).
- Around line 135-152: The UUID query serialization is wrong: replace the unsafe
String([modificationUuid]) pattern in setModificationMetadata and
updateModificationStatusByRootNetwork with proper URLSearchParams
handling—either append the single UUID only when defined
(urlSearchParams.append('uuids', String(modificationUuid)) guarded by a
null/undefined check) or, if multiple UUIDs are intended, accept an array and
iterate like the existing forEach pattern to append each defined UUID
individually; ensure you never append an empty string so the backend never
receives an empty uuids= parameter.
---
Outside diff comments:
In `@src/components/network-modification-table/network-modifications-table.tsx`:
- Line 34: Remove the unused import NameHeaderProps from the import statement
that currently reads "import { AUTO_EXTENSIBLE_COLUMNS, NameHeaderProps } from
'./columns-definition';" since the file constructs the nameHeaderProps object
inline and never references the NameHeaderProps type; keep
AUTO_EXTENSIBLE_COLUMNS, and update the import to only include the symbols
actually used (e.g., "AUTO_EXTENSIBLE_COLUMNS") to satisfy the pipeline.
🪄 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: 552f14f8-4228-4dd0-b364-e5f949ed0989
📒 Files selected for processing (9)
src/components/network-modification-table/network-modifications-table.tsxsrc/components/network-modification-table/renderers/cell-renderers.tsxsrc/components/network-modification-table/renderers/description-cell.tsxsrc/components/network-modification-table/renderers/index.tssrc/components/network-modification-table/renderers/root-network-chip-cell.tsxsrc/components/network-modification-table/renderers/switch-cell.tsxsrc/module-tanstack.d.tssrc/services/networkModification.tssrc/utils/types/network-modification-types.ts
Comment on lines
+26
to
+31
| export const DescriptionCell: FunctionComponent<DescriptionCellProps> = ({ | ||
| data, | ||
| studyUuid, | ||
| currentNodeId, | ||
| isDisabled = false, | ||
| }) => { |
There was a problem hiding this comment.
Fix function component definition to use function declaration.
The component is defined as a const arrow function, but the project's ESLint configuration requires function declarations for components. This is causing the pipeline to fail.
🔧 Proposed fix
-export const DescriptionCell: FunctionComponent<DescriptionCellProps> = ({
+export function DescriptionCell({
data,
studyUuid,
currentNodeId,
isDisabled = false,
-}) => {
+}: DescriptionCellProps) {📝 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
| export const DescriptionCell: FunctionComponent<DescriptionCellProps> = ({ | |
| data, | |
| studyUuid, | |
| currentNodeId, | |
| isDisabled = false, | |
| }) => { | |
| export function DescriptionCell({ | |
| data, | |
| studyUuid, | |
| currentNodeId, | |
| isDisabled = false, | |
| }: DescriptionCellProps) { |
🧰 Tools
🪛 ESLint
[error] 26-83: Function component is not a function declaration
(react/function-component-definition)
🪛 GitHub Actions: CI / 1_build _ build.txt
[error] 26-26: ESLint react/function-component-definition: Function component is not a function declaration
🪛 GitHub Actions: CI / build _ build
[error] 26-26: ESLint (react/function-component-definition): Function component is not a function declaration.
🪛 GitHub Check: build / build
[failure] 26-26:
Function component is not a function declaration
🤖 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/components/network-modification-table/renderers/description-cell.tsx`
around lines 26 - 31, The DescriptionCell component is currently defined as a
const arrow function which violates the project's ESLint rule requiring function
declarations; change the declaration "export const DescriptionCell:
FunctionComponent<DescriptionCellProps> = ({ data, studyUuid, currentNodeId,
isDisabled = false, }) => { ... }" into a named function declaration "export
function DescriptionCell(props: DescriptionCellProps) { ... }" (or destructure
parameters inside the function signature) preserving the same props (data,
studyUuid, currentNodeId, isDisabled with its default) and all existing
implementation details and exports so ESLint accepts the component.
Comment on lines
+112
to
+117
| .catch((error) => { | ||
| snackWithFallback(snackError, error, { headerId: 'modificationActivationByRootNetworkError' }); | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false); | ||
| }); |
There was a problem hiding this comment.
Roll back optimistic state update on API failure.
When updateModificationStatusByRootNetwork fails (line 112), the error handler shows a snack message but does not revert the optimistic state change applied on line 129-131. This leaves modificationsToExclude in an inconsistent state, where the UI shows the modification as excluded/included but the server state differs. Users may see incorrect activation state until the page is refreshed or server notifications arrive.
♻️ Suggested approach
Capture the previous state before the optimistic update, then restore it in the catch block:
const handleModificationActivationByRootNetwork = useCallback(() => {
if (!modificationUuid) {
return;
}
setIsLoading(true);
+
+ // Capture previous state for rollback
+ let previousState: ExcludedNetworkModifications[] | null = null;
+ setModificationsToExclude((prev) => {
+ previousState = prev;
+ return getUpdatedExcludedModifications(prev, rootNetwork.rootNetworkUuid, modificationUuid, updateStatus);
+ });
- setModificationsToExclude((prev) =>
- getUpdatedExcludedModifications(prev, rootNetwork.rootNetworkUuid, modificationUuid, updateStatus)
- );
}, [modificationUuid, rootNetwork.rootNetworkUuid, setModificationsToExclude, updateStatus]);Then modify updateStatus to accept and use the previous state:
const updateStatus = useCallback(
- (newStatus: boolean) => {
+ (newStatus: boolean, rollback?: () => void) => {
if (!studyUuid || !modificationUuid || !currentNodeId) {
setIsLoading(false);
return;
}
updateModificationStatusByRootNetwork(
studyUuid,
currentNodeId,
rootNetwork.rootNetworkUuid,
modificationUuid,
newStatus
)
.catch((error) => {
+ rollback?.();
snackWithFallback(snackError, error, { headerId: 'modificationActivationByRootNetworkError' });
})
.finally(() => {
setIsLoading(false);
});
},
[studyUuid, currentNodeId, modificationUuid, rootNetwork.rootNetworkUuid, snackError]
);Note: This requires refactoring getUpdatedExcludedModifications to pass the rollback closure.
🤖 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/components/network-modification-table/renderers/root-network-chip-cell.tsx`
around lines 112 - 117, The optimistic update to modificationsToExclude needs to
be rolled back if updateModificationStatusByRootNetwork fails: capture the
previous modificationsToExclude (e.g., const prev = modificationsToExclude)
before calling getUpdatedExcludedModifications / updateStatus, and in the
.catch(error) handler restore it by calling setModificationsToExclude(prev) (and
keep the snackWithFallback call). Update
updateStatus/getUpdatedExcludedModifications signatures if needed to return both
the new state and a rollback closure or previous state so the catch block can
reliably revert; ensure setIsLoading(false) remains in .finally().
Comment on lines
+23
to
+28
| export const SwitchCell: FunctionComponent<SwitchCellProps> = ({ | ||
| data, | ||
| studyUuid, | ||
| currentNodeId, | ||
| isDisabled = false, | ||
| }) => { |
There was a problem hiding this comment.
Fix function component definition to use function declaration.
The component is defined as a const arrow function, but the project's ESLint configuration requires function declarations for components. This is causing the pipeline to fail.
🔧 Proposed fix
-export const SwitchCell: FunctionComponent<SwitchCellProps> = ({
+export function SwitchCell({
data,
studyUuid,
currentNodeId,
isDisabled = false,
-}) => {
+}: SwitchCellProps) {📝 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
| export const SwitchCell: FunctionComponent<SwitchCellProps> = ({ | |
| data, | |
| studyUuid, | |
| currentNodeId, | |
| isDisabled = false, | |
| }) => { | |
| export function SwitchCell({ | |
| data, | |
| studyUuid, | |
| currentNodeId, | |
| isDisabled = false, | |
| }: SwitchCellProps) { |
🧰 Tools
🪛 ESLint
[error] 23-80: Function component is not a function declaration
(react/function-component-definition)
🪛 GitHub Check: build / build
[failure] 23-23:
Function component is not a function declaration
🤖 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/components/network-modification-table/renderers/switch-cell.tsx` around
lines 23 - 28, The component SwitchCell is declared as an exported const arrow
function which violates the project's ESLint rule requiring function
declarations for React components; change the declaration to a named function
declaration that accepts a single parameter of type SwitchCellProps (preserve
the prop names data, studyUuid, currentNodeId, isDisabled with its default
false) and export it (e.g., export function SwitchCell(props: SwitchCellProps) {
... }), leaving the rest of the implementation unchanged and keeping the
SwitchCellProps type annotation.
This reverts commit d1caf40.
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.
Summary
Accept a stable
columnsarray prop instead of acreateAllColumnsfactory called inside an internaluseMemo, so the parent owns when columns are rebuilt.nameHeaderPropsfrom the individual fields and forward it through react-tablemetaalong withisRowDragDisabledandmodificationsCountso column renderers can read them viatable.options.meta.TableMetawith the new fields andColumnMetawithcellStyle.Paired with gridsuite/gridstudy-app#3931, which adopts this API to stop unmounting cells on every columns rebuild.