Add details for errors in the security analysis parameters contingency count#1215
Add details for errors in the security analysis parameters contingency count#1215FranckLecuyer wants to merge 11 commits into
Conversation
…y count Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
FranckLecuyer
requested review from
TheMaskedTurtle,
antoinebhs,
ayolab,
flomillot and
klesaulnier
as code owners
|
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:
📝 WalkthroughWalkthroughContingency count data now uses per-list structures, is enriched with contingency-list names, and is converted into success or failure simulation states before rendering. The contingency messages in English and French now distinguish simulated counts from non-simulatable contingencies. ChangesContingency Count Enrichment and Simulation Result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
🤖 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/parameters/common/contingency-table/contingency-table.tsx`:
- Around line 231-239: The contingency-table alert branch in
contingency-table.tsx is treating a null simulatedContingencyCount as “No
contingency,” which incorrectly shows the empty-state message after fetch
failures. Update the conditional around the alert in the contingency-table
component so it only renders when simulatedContingencyCount indicates a
successful request with nbContingencies === 0, and keep the null/error path
separate from the successful zero-result path.
- Around line 96-102: The enrichment map in contingency-table.tsx is using the
display name as the key, which can overwrite entries when multiple contingency
lists share the same name. Update the enrichment logic around the
contingencyCount.countByContingencyList loop so enriched stays keyed by
contingencyListUuid (or equivalent UUID key) and attach listName as a field on
the stored value instead of using it as the record key. Make sure any downstream
code that reads the enriched data preserves the UUID-based lookup and uses the
human-readable name only for display.
- Around line 132-135: The contingency-table missing-elements check is using
`.length` on `contingencyEquipments`, but this value is a Set stored in
`notFoundElements`, so the branch never runs. Update the conditional in
`contingency-table.tsx` to use `Set.size` instead, keeping the surrounding
`contingencyEquipments` check and the failure-alert path in place. Reference the
`notFoundElements` lookup and the `contingencyEquipments` guard so the
missing-elements alert becomes reachable again.
In `@src/translations/fr/parameters.ts`:
- Line 238: The French translation entry contingenciesWillNotBeSimulated
contains a typo in the message text. Update the string so “velide” is corrected
to “valide” in the parameters translation object.
🪄 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: 9d8d9983-e3ab-4367-8273-1d99c31e6cd5
📒 Files selected for processing (4)
src/features/parameters/common/contingency-table/contingency-table.tsxsrc/features/parameters/common/contingency-table/types.tssrc/translations/en/parameters.tssrc/translations/fr/parameters.ts
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
There was a problem hiding this comment.
Review — contingency count error details
Scope: branch review against main (4 files, +130/−21).
Goal: surface which contingency in which list is invalid, instead of just a count + "N not found".
The discriminated-union refactor (SuccessCountType | FailureCountType) is a clear improvement over the old loose optional-field shape, and the Grid→Grid2 switch is aligned with the ongoing migration. Translations are in sync across EN/FR with matching ICU placeholders, and the removed key is no longer referenced. 👍
Main concern is one likely-blocking type/runtime mismatch (the Set vs .length cast), plus a fragile helper round-trip and a couple of minor cleanups — all inline below.
Validation: not run. Suggest type-check + lint; removing the as unknown as string[] cast should let tsc surface the Set/.length mismatch directly.
Generated with the commons-ui code-review skill.
|
|
||
| if ( | ||
| contingencyEquipments !== undefined && | ||
| (contingencyEquipments as unknown as string[]).length > 0 |
There was a problem hiding this comment.
[blocker] Set<string> typed but read as an array. notFoundElements is declared Record<string, Set<string>> (types.ts), but here the deciding condition uses (contingencyEquipments as unknown as string[]).length. A Set has no .length — only .size — and this data comes from fetchContingencyCount resolving backend JSON, which cannot carry a real Set.
So either:
- the runtime value is actually an array → change the type to
Record<string, string[]>and drop the cast; or - it really is a
Set→.lengthis alwaysundefined,undefined > 0is alwaysfalse, and no invalid contingency is ever detected (the feature silently no-ops).
The as unknown as string[] double cast is what hides this. Please align the declared type with the real payload and remove the cast so the compiler validates the check.
| for (let i = 0; i < Object.entries(contingencyCount.countByContingencyList).length; i++) { | ||
| const [contingencyListUuid] = Object.entries(contingencyCount.countByContingencyList)[i]; | ||
| const listName = namesById[contingencyListUuid as UUID]; | ||
| if (listName !== undefined) { |
There was a problem hiding this comment.
[major] Silent undercount when a list name is unresolved. When listName === undefined, the entry is dropped from enriched, so contingencies from any list whose name can't be resolved are later excluded from total in analyzeContingencies — the displayed count can be wrong. Please keep unresolved lists in the total (e.g. fall back to the UUID) or justify dropping them.
| ) { | ||
| return { | ||
| success: false, | ||
| contingencyListName: contingencyListName.includes('_') |
There was a problem hiding this comment.
[major] Fragile string-key round-trip. enrichContingencyCount encodes the name into the map key as `${contingencyListUuid}_${listName}`, and here it's parsed back out with slice(indexOf('_') + 1). Encoding structured data into a string key only to split it apart again is fragile. Prefer carrying a structured value (e.g. { listName, count }) so no decode step is needed.
| }); | ||
| }); | ||
| const enriched: Record<string, ContingencyCountByContingencyList> = {}; | ||
| for (let i = 0; i < Object.entries(contingencyCount.countByContingencyList).length; i++) { |
There was a problem hiding this comment.
[minor] Object.entries() recomputed and re-indexed each iteration. for (let i = 0; i < Object.entries(x).length; i++) then indexing Object.entries(x)[i] rebuilds the entries array on every iteration — O(n²) allocations for plain iteration. Same pattern at L121 and L128. Use for (const [k, v] of Object.entries(x)) (or .forEach); faster and clearer.
| const styles = { | ||
| alert: { color: 'text.primary', paddingTop: 0, paddingBottom: 0 }, | ||
| alert: { color: 'text.primary', paddingTop: 0, paddingBottom: 0, width: '100%' }, | ||
| error: { color: 'red', paddingTop: 0, paddingBottom: 0, width: '100%' }, |
There was a problem hiding this comment.
[minor] Hardcoded color: 'red' instead of a theme token. The sibling alert style uses color: 'text.primary', and <Alert severity="error"> already provides error coloring, so this override is likely redundant. If a color is needed, use a theme token (error.main). Also re-check width: '100%' on both alert and error — it may now be redundant with the width: '100%' parent Grid.
|
|
||
| return ( | ||
| <Grid container direction="column"> | ||
| <Grid container sx={{ width: '100%' }}> |
There was a problem hiding this comment.
The Grid→Grid2 switch is fine and aligned with the ongoing migration. Just confirming: the root went from <Grid container direction="column"> to <Grid container sx={{ width: '100%' }}> — please verify dropping direction="column" is intentional and the rows still stack as before (it reads like a deliberate layout change rather than a migration requirement).
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
…_contingency_count
…_contingency_count
…_contingency_count
…_contingency_count
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
|
2 participants
PR Summary