Move Voltage Level Modification functionality and related components into commons-ui#1060
Open
Move Voltage Level Modification functionality and related components into commons-ui#1060
Conversation
…into commons-ui Signed-off-by: achour94 <berrahmaachour@gmail.com>
…cation-into-commons-ui # Conflicts: # src/components/network-modifications/voltage-level/index.ts
📝 WalkthroughWalkthroughAdds voltage-level modification support: a React modification form, TypeScript DTOs/types, Yup validation and form↔DTO mapping utilities, new barrel exports, a field constant, and English/French translation keys. No other runtime control flow changes. Changes
Sequence Diagram(s)🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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.
🧹 Nitpick comments (3)
src/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsx (2)
57-65: Outdated comment about MUI bug.The comment references a MUI bug that "should be fixed after v5.12.2", but the project uses
@mui/materialv5.16.14. The MUI bug is no longer relevant—the remaining limitation is purely backend-related (Powsybl). Consider simplifying the comment to focus on the actual blocking issue.✏️ Simplified comment
const substationField = ( <AutocompleteInput allowNewValue forcePopupIcon name={FieldConstants.SUBSTATION_ID} label="SUBSTATION" - // Because of a mui/material bug, the disabled attribute does not work properly. - // It should be fixed after v5.12.2. For the moment, instead of fetching the - // substation list to display in this AutocompleteInput, we only show the current substation. + // Only showing current substation since changing substations is not yet supported by the backend (Powsybl). options={[voltageLevelToModify?.substationId ?? '']} inputTransform={(value) => (value === null ? '' : value)} outputTransform={(value) => value} size="small" formProps={filledTextField} - disabled // TODO: to be removed when it is possible to change the substation of a voltage level in the backend (Powsybl) + disabled // TODO: Enable when Powsybl supports changing substation of a voltage level /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsx` around lines 57 - 65, Update the outdated comment in VoltageLevelModificationForm.tsx (around the AutocompleteInput options/disabled props) to remove the MUI bug reference and version number; instead state concisely that the field is disabled due to a backend limitation in Powsybl and keep the TODO about removing the disabled flag once the backend supports changing a voltage level's substation.
27-39: RedundantreadOnlywithdisabled.Setting both
InputProps={{ readOnly: true }}anddisabledis redundant sincedisabledalready prevents user input. You can simplify by removing theInputPropsprop.✨ Simplified TextField
const voltageLevelIdField = ( <TextField size="small" fullWidth label="ID" value={equipmentId ?? ''} - InputProps={{ - readOnly: true, - }} disabled {...filledTextField} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsx` around lines 27 - 39, The TextField instance assigned to voltageLevelIdField currently sets both InputProps={{ readOnly: true }} and disabled, which is redundant; remove the InputProps prop (or the readOnly flag) and keep disabled so the field remains non-editable, i.e. update the TextField used in voltageLevelIdField to drop InputProps while preserving disabled, label, value (equipmentId ?? ''), and {...filledTextField}.src/components/network-modifications/voltage-level/modification/voltageLevelModification.types.ts (1)
34-46: Consider makingtyperequired with a literal type for stronger type safety.The
typefield is defined as optional and generic (type?: ModificationType), but the DTO contract (per the context snippet) expects it to betype: ModificationType.VOLTAGE_LEVEL_MODIFICATION. SincevoltageLevelModificationFormToDtoalways sets this value explicitly, the code works at runtime, but the type definition is more permissive than intended.💡 Suggested type refinement
export interface VoltageLevelModificationDto { uuid?: UUID; equipmentId: string; equipmentName?: AttributeModification<string> | null; substationId?: AttributeModification<string> | null; nominalV?: AttributeModification<number> | null; lowVoltageLimit?: AttributeModification<number> | null; highVoltageLimit?: AttributeModification<number> | null; ipMin?: AttributeModification<number> | null; ipMax?: AttributeModification<number> | null; properties?: Property[] | null; - type?: ModificationType; + type: ModificationType.VOLTAGE_LEVEL_MODIFICATION; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/network-modifications/voltage-level/modification/voltageLevelModification.types.ts` around lines 34 - 46, The VoltageLevelModificationDto currently has an optional, generic type field; change it so the field is required and narrowed to the literal value used by the DTO: replace "type?: ModificationType" with "type: ModificationType.VOLTAGE_LEVEL_MODIFICATION" in the VoltageLevelModificationDto declaration, and update any call sites or converters (e.g., voltageLevelModificationFormToDto) if they rely on the field being optional to ensure they still set that required literal value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/components/network-modifications/voltage-level/modification/voltageLevelModification.types.ts`:
- Around line 34-46: The VoltageLevelModificationDto currently has an optional,
generic type field; change it so the field is required and narrowed to the
literal value used by the DTO: replace "type?: ModificationType" with "type:
ModificationType.VOLTAGE_LEVEL_MODIFICATION" in the VoltageLevelModificationDto
declaration, and update any call sites or converters (e.g.,
voltageLevelModificationFormToDto) if they rely on the field being optional to
ensure they still set that required literal value.
In
`@src/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsx`:
- Around line 57-65: Update the outdated comment in
VoltageLevelModificationForm.tsx (around the AutocompleteInput options/disabled
props) to remove the MUI bug reference and version number; instead state
concisely that the field is disabled due to a backend limitation in Powsybl and
keep the TODO about removing the disabled flag once the backend supports
changing a voltage level's substation.
- Around line 27-39: The TextField instance assigned to voltageLevelIdField
currently sets both InputProps={{ readOnly: true }} and disabled, which is
redundant; remove the InputProps prop (or the readOnly flag) and keep disabled
so the field remains non-editable, i.e. update the TextField used in
voltageLevelIdField to drop InputProps while preserving disabled, label, value
(equipmentId ?? ''), and {...filledTextField}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ffdce5c-0e57-45eb-a69b-c6e9efce4f4e
📒 Files selected for processing (7)
src/components/network-modifications/voltage-level/index.tssrc/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsxsrc/components/network-modifications/voltage-level/modification/index.tssrc/components/network-modifications/voltage-level/modification/voltageLevelModification.types.tssrc/components/network-modifications/voltage-level/modification/voltageLevelModification.utils.tssrc/translations/en/networkModificationsEn.tssrc/translations/fr/networkModificationsFr.ts
Member
|
Tested with gridstudy locally and it worked. (Can you please fix the sonarcloud reliability rating so it can be merged? --- Apparently it's common during refactors and it is ignored) |
Member
|
There is one issue with gridexplore where in the form, the field that contains the value of SubstationId is empty. A fix needs to be implemented either in gridExplore or commons-ui |
Signed-off-by: achour94 <berrahmaachour@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/network-modifications/voltage-level/modification/voltageLevelModification.utils.ts`:
- Around line 60-70: The form defaults object
voltageLevelModificationEmptyFormData is missing substationId which causes the
FieldConstants.SUBSTATION_ID bound control to start empty; add substationId: ''
(or appropriate null/empty type) to voltageLevelModificationEmptyFormData and
update the DTO→form mapping function (the voltage level DTO to form converter
used around lines ~93-104) to copy dto.substationId into the form data so the
substation field is hydrated when a DTO value exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79490040-ed72-46ee-8375-20d62311cdef
📒 Files selected for processing (4)
src/components/network-modifications/voltage-level/modification/VoltageLevelModificationForm.tsxsrc/components/network-modifications/voltage-level/modification/voltageLevelModification.types.tssrc/components/network-modifications/voltage-level/modification/voltageLevelModification.utils.tssrc/utils/constants/fieldConstants.ts
✅ Files skipped from review due to trivial changes (2)
- src/utils/constants/fieldConstants.ts
- src/components/network-modifications/voltage-level/modification/voltageLevelModification.types.ts
Comment on lines
+60
to
+70
| export const voltageLevelModificationEmptyFormData: VoltageLevelModificationFormData = { | ||
| equipmentID: '', | ||
| equipmentName: '', | ||
| hideSubstationField: false, | ||
| nominalV: null, | ||
| lowVoltageLimit: null, | ||
| highVoltageLimit: null, | ||
| lowShortCircuitCurrentLimit: null, | ||
| highShortCircuitCurrentLimit: null, | ||
| AdditionalProperties: [], | ||
| }; |
There was a problem hiding this comment.
Hydrate and initialize substationId in form data to prevent empty Substation field.
Line 60 and Line 93 omit substationId from defaults/DTO→form mapping, so the form control bound to FieldConstants.SUBSTATION_ID starts empty even when DTO carries a value.
💡 Proposed fix
export const voltageLevelModificationEmptyFormData: VoltageLevelModificationFormData = {
equipmentID: '',
equipmentName: '',
hideSubstationField: false,
+ substationId: '',
nominalV: null,
lowVoltageLimit: null,
highVoltageLimit: null,
lowShortCircuitCurrentLimit: null,
highShortCircuitCurrentLimit: null,
AdditionalProperties: [],
};
@@
export const voltageLevelModificationDtoToForm = (
voltageLevelDto: VoltageLevelModificationDto,
includePreviousValues = true
): VoltageLevelModificationFormData => ({
equipmentID: voltageLevelDto.equipmentId,
equipmentName: voltageLevelDto.equipmentName?.value ?? '',
hideSubstationField: false,
+ substationId: voltageLevelDto.substationId ?? '',
nominalV: voltageLevelDto.nominalV?.value ?? null,
lowVoltageLimit: voltageLevelDto.lowVoltageLimit?.value ?? null,
highVoltageLimit: voltageLevelDto.highVoltageLimit?.value ?? null,
lowShortCircuitCurrentLimit:
convertInputValue(FieldType.LOW_SHORT_CIRCUIT_CURRENT_LIMIT, voltageLevelDto.ipMin?.value) ?? null,
highShortCircuitCurrentLimit:
convertInputValue(FieldType.HIGH_SHORT_CIRCUIT_CURRENT_LIMIT, voltageLevelDto.ipMax?.value) ?? null,
...getPropertiesFromModification(voltageLevelDto.properties, includePreviousValues),
});Also applies to: 93-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/network-modifications/voltage-level/modification/voltageLevelModification.utils.ts`
around lines 60 - 70, The form defaults object
voltageLevelModificationEmptyFormData is missing substationId which causes the
FieldConstants.SUBSTATION_ID bound control to start empty; add substationId: ''
(or appropriate null/empty type) to voltageLevelModificationEmptyFormData and
update the DTO→form mapping function (the voltage level DTO to form converter
used around lines ~93-104) to copy dto.substationId into the form data so the
substation field is hydrated when a DTO value exists.
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
2 participants
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.
PR Summary