[WIP] Prepare global filter migration#4059
Open
klesaulnier wants to merge 7 commits into
Open
Conversation
… into the component Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
📝 WalkthroughWalkthroughThe global filter feature is refactored from a Redux-context hybrid into a typed, nullable React context with explicit callbacks. A new adapter provider fetches/validates substation-property filters. State mutations move to pure utilities, backend sync moves to a debounced helper, and consuming files are repointed to relocated modules. ChangesGlobal Filter Context-to-Props Refactor
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 |
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
…ider props creation Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/results/common/global-filter/global-filter-context-types.ts (1)
11-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: preserve
FilterType[]typing forfilterCategories.
GlobalFilterProvidersuppliesfilterCategories: FilterType[], but the context value widens it tostring[]. SinceFilterTypeis a string enum it compiles, yet downstream consumers reading from the context lose the enum typing and any exhaustiveness/IDE help it provides. Consider typing this field asFilterType[](or a shared alias) to keep the contract precise.🤖 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/results/common/global-filter/global-filter-context-types.ts` around lines 11 - 23, The `GlobalFilterContextValue` contract widens `filterCategories` to `string[]`, which loses the intended `FilterType` enum typing used by `GlobalFilterProvider`. Update the `GlobalFilterContextValue` definition to use `FilterType[]` (or a shared alias) for `filterCategories`, and make sure any related context/provider typings stay aligned so consumers of `useGlobalFilterContext` keep the precise enum-based type information.
🤖 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.
Nitpick comments:
In `@src/components/results/common/global-filter/global-filter-context-types.ts`:
- Around line 11-23: The `GlobalFilterContextValue` contract widens
`filterCategories` to `string[]`, which loses the intended `FilterType` enum
typing used by `GlobalFilterProvider`. Update the `GlobalFilterContextValue`
definition to use `FilterType[]` (or a shared alias) for `filterCategories`, and
make sure any related context/provider typings stay aligned so consumers of
`useGlobalFilterContext` keep the precise enum-based type information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d92b0b8c-f995-47cb-916f-d0c5ff5f5eab
📒 Files selected for processing (8)
src/components/results/common/global-filter/global-filter-autocomplete.tsxsrc/components/results/common/global-filter/global-filter-context-provider.tsxsrc/components/results/common/global-filter/global-filter-context-types.tssrc/components/results/common/global-filter/global-filter-context.tssrc/components/results/common/global-filter/global-filter-paper.tsxsrc/components/results/common/global-filter/global-filter-provider.tsxsrc/components/results/common/global-filter/global-filter-selector.tsxsrc/components/results/common/global-filter/selected-global-filters.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/results/common/global-filter/global-filter-selector.tsx
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/results/common/global-filter/global-filter-context-types.ts (1)
8-10: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse type-only imports here.
PropsWithChildren,GlobalFilter, andRecentGlobalFilterare only used in type positions, soimport typemakes the intent explicit and keeps the file safe if module-preserving transpilation is introduced.♻️ Proposed import cleanup
-import { PropsWithChildren } from 'react'; -import { GlobalFilter, RecentGlobalFilter } from './global-filter-types'; +import type { PropsWithChildren } from 'react'; +import type { GlobalFilter, RecentGlobalFilter } from './global-filter-types'; import type { UUID } from 'node:crypto';🤖 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/results/common/global-filter/global-filter-context-types.ts` around lines 8 - 10, Update the imports in global-filter-context-types so they are type-only: PropsWithChildren, GlobalFilter, and RecentGlobalFilter are only used in type positions, so switch their imports to import type while leaving UUID as-is if needed. Use the existing symbols PropsWithChildren, GlobalFilter, and RecentGlobalFilter in this file to verify the cleanup and keep the module safe for module-preserving transpilation.
🤖 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/results/common/global-filter/global-filter-paper.tsx`:
- Around line 123-133: The add-selected-filters flow in
addSelectedFiltersToGlobalFiltersOptions leaves the directory selector open when
values is undefined and can also skip cleanup if
addFiltersToGlobalFiltersOptions rejects. Update this callback so the
directoryItemSelectorOpen state is cleared on every exit path, including the
no-values cancel case and a finally block around the async add operation, while
keeping the existing setOpenedDropdown behavior intact.
In `@src/components/results/common/global-filter/global-filter-provider.tsx`:
- Around line 82-86: The initial metadata load in the useEffect inside
GlobalFilterProvider does not handle rejections, so
fetchSubstationPropertiesGlobalFilters() can leave an unhandled promise
rejection and no fallback state. Update the effect to catch errors from
fetchSubstationPropertiesGlobalFilters(), and ensure
setSubstationPropertiesGlobalFilters is still driven to a defined fallback value
when the request fails so the provider remains in a recoverable state.
- Around line 166-194: The duplicate-check in addFiltersToGlobalFiltersOptions
is using selectedGlobalFilters, which can miss IDs that are already selected but
not currently resolvable from globalFilterOptions. Update the membership test to
use the source-of-truth selectedFilterIds for this table instead, so
fetchElementsInfos results are only added when their id is not already present.
Keep the rest of the flow in GlobalFilterProvider the same, including
dispatch(addToGlobalFilterOptions) and the subsequent addToSelectedGlobalFilters
calls.
In `@src/components/results/common/global-filter/global-filter-state.utils.ts`:
- Around line 91-116: The helper markNotFoundGlobalFiltersAsDeletedInState
currently marks matching GlobalFilter entries as deleted and prunes them from
tableState.recents, but it leaves tableState.selected untouched, so deleted
filters remain selected and can still flow into backend sync. Update
markNotFoundGlobalFiltersAsDeletedInState to also filter the
notFoundGlobalFilters IDs out of tableState.selected (and keep the existing
recents cleanup), or ensure the same exclusion happens wherever
GlobalFilterTableState is consumed.
---
Nitpick comments:
In `@src/components/results/common/global-filter/global-filter-context-types.ts`:
- Around line 8-10: Update the imports in global-filter-context-types so they
are type-only: PropsWithChildren, GlobalFilter, and RecentGlobalFilter are only
used in type positions, so switch their imports to import type while leaving
UUID as-is if needed. Use the existing symbols PropsWithChildren, GlobalFilter,
and RecentGlobalFilter in this file to verify the cleanup and keep the module
safe for module-preserving transpilation.
🪄 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: a19a0054-4772-4091-8f53-df0b9a67c758
📒 Files selected for processing (15)
src/components/results/common/global-filter/global-filter-app-data.tssrc/components/results/common/global-filter/global-filter-autocomplete.tsxsrc/components/results/common/global-filter/global-filter-context-provider.tsxsrc/components/results/common/global-filter/global-filter-context-types.tssrc/components/results/common/global-filter/global-filter-context.tssrc/components/results/common/global-filter/global-filter-paper.tsxsrc/components/results/common/global-filter/global-filter-provider.tsxsrc/components/results/common/global-filter/global-filter-selector.tsxsrc/components/results/common/global-filter/global-filter-state.utils.tssrc/components/results/common/global-filter/global-filter-sync.utils.tssrc/components/results/common/global-filter/global-filter-utils.tssrc/components/results/common/global-filter/selected-global-filters.tsxsrc/components/results/common/global-filter/use-global-filter-options.tssrc/redux/globalFiltersMiddleware.tssrc/redux/reducer.ts
💤 Files with no reviewable changes (1)
- src/components/results/common/global-filter/global-filter-utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/results/common/global-filter/global-filter-selector.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/results/common/global-filter/global-filter-context.ts
- src/components/results/common/global-filter/selected-global-filters.tsx
- src/components/results/common/global-filter/global-filter-context-provider.tsx
- src/components/results/common/global-filter/global-filter-autocomplete.tsx
Comment on lines
+123
to
+133
| const addSelectedFiltersToGlobalFiltersOptions = useCallback( | ||
| async (values: TreeViewFinderNodeProps[] | undefined) => { | ||
| if (!values) { | ||
| return; | ||
| } | ||
|
|
||
| setOpenedDropdown(true); | ||
|
|
||
| const elements: ElementAttributes[] = await fetchElementsInfos(values.map((value) => value.id)); | ||
| const newlySelectedFilters: GlobalFilter[] = []; | ||
| elements.forEach((element: ElementAttributes) => { | ||
| // ignore already selected filters and non-generic filters : | ||
| if (!selectedGlobalFilters.find((filter) => filter.uuid && filter.uuid === element.elementUuid)) { | ||
| // add the others | ||
| const substationOrVoltageLevel = | ||
| element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION || | ||
| element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL; | ||
| newlySelectedFilters.push({ | ||
| id: element.elementUuid, | ||
| uuid: element.elementUuid, | ||
| equipmentType: element.specificMetadata?.equipmentType, | ||
| label: element.elementName, | ||
| filterType: substationOrVoltageLevel ? FilterType.SUBSTATION_OR_VL : FilterType.GENERIC_FILTER, | ||
| filterTypeFromMetadata: element.specificMetadata?.type, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| dispatch(addToGlobalFilterOptions(newlySelectedFilters)); | ||
| dispatch( | ||
| addToSelectedGlobalFilters( | ||
| tableType, | ||
| tableUuid, | ||
| newlySelectedFilters.map((f) => f.id) | ||
| ) | ||
| ); | ||
| await addFiltersToGlobalFiltersOptions(values.map((value) => value.id)); | ||
| setDirectoryItemSelectorOpen(false); | ||
| }, | ||
| [selectedGlobalFilters, setDirectoryItemSelectorOpen, setOpenedDropdown, dispatch, tableType, tableUuid] | ||
| [addFiltersToGlobalFiltersOptions, setDirectoryItemSelectorOpen, setOpenedDropdown] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Always close the directory selector on cancel or failure.
When values is undefined, Line 126 returns before clearing directoryItemSelectorOpen; a cancel/close path can leave the controlled selector stuck open. Also close in finally so a rejected add operation does not skip cleanup.
🐛 Proposed close-state fix
const addSelectedFiltersToGlobalFiltersOptions = useCallback(
async (values: TreeViewFinderNodeProps[] | undefined) => {
- if (!values) {
- return;
- }
-
- setOpenedDropdown(true);
- await addFiltersToGlobalFiltersOptions(values.map((value) => value.id));
- setDirectoryItemSelectorOpen(false);
+ try {
+ if (values?.length) {
+ setOpenedDropdown(true);
+ await addFiltersToGlobalFiltersOptions(values.map((value) => value.id));
+ }
+ } finally {
+ setDirectoryItemSelectorOpen(false);
+ }
},
[addFiltersToGlobalFiltersOptions, setDirectoryItemSelectorOpen, setOpenedDropdown]
);📝 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
| const addSelectedFiltersToGlobalFiltersOptions = useCallback( | |
| async (values: TreeViewFinderNodeProps[] | undefined) => { | |
| if (!values) { | |
| return; | |
| } | |
| setOpenedDropdown(true); | |
| const elements: ElementAttributes[] = await fetchElementsInfos(values.map((value) => value.id)); | |
| const newlySelectedFilters: GlobalFilter[] = []; | |
| elements.forEach((element: ElementAttributes) => { | |
| // ignore already selected filters and non-generic filters : | |
| if (!selectedGlobalFilters.find((filter) => filter.uuid && filter.uuid === element.elementUuid)) { | |
| // add the others | |
| const substationOrVoltageLevel = | |
| element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION || | |
| element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL; | |
| newlySelectedFilters.push({ | |
| id: element.elementUuid, | |
| uuid: element.elementUuid, | |
| equipmentType: element.specificMetadata?.equipmentType, | |
| label: element.elementName, | |
| filterType: substationOrVoltageLevel ? FilterType.SUBSTATION_OR_VL : FilterType.GENERIC_FILTER, | |
| filterTypeFromMetadata: element.specificMetadata?.type, | |
| }); | |
| } | |
| }); | |
| dispatch(addToGlobalFilterOptions(newlySelectedFilters)); | |
| dispatch( | |
| addToSelectedGlobalFilters( | |
| tableType, | |
| tableUuid, | |
| newlySelectedFilters.map((f) => f.id) | |
| ) | |
| ); | |
| await addFiltersToGlobalFiltersOptions(values.map((value) => value.id)); | |
| setDirectoryItemSelectorOpen(false); | |
| }, | |
| [selectedGlobalFilters, setDirectoryItemSelectorOpen, setOpenedDropdown, dispatch, tableType, tableUuid] | |
| [addFiltersToGlobalFiltersOptions, setDirectoryItemSelectorOpen, setOpenedDropdown] | |
| const addSelectedFiltersToGlobalFiltersOptions = useCallback( | |
| async (values: TreeViewFinderNodeProps[] | undefined) => { | |
| try { | |
| if (values?.length) { | |
| setOpenedDropdown(true); | |
| await addFiltersToGlobalFiltersOptions(values.map((value) => value.id)); | |
| } | |
| } finally { | |
| setDirectoryItemSelectorOpen(false); | |
| } | |
| }, | |
| [addFiltersToGlobalFiltersOptions, setDirectoryItemSelectorOpen, setOpenedDropdown] |
🤖 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/results/common/global-filter/global-filter-paper.tsx` around
lines 123 - 133, The add-selected-filters flow in
addSelectedFiltersToGlobalFiltersOptions leaves the directory selector open when
values is undefined and can also skip cleanup if
addFiltersToGlobalFiltersOptions rejects. Update this callback so the
directoryItemSelectorOpen state is cleared on every exit path, including the
no-values cancel case and a finally block around the async add operation, while
keeping the existing setOpenedDropdown behavior intact.
Comment on lines
+82
to
+86
| useEffect(() => { | ||
| fetchSubstationPropertiesGlobalFilters().then(({ substationPropertiesGlobalFilters }) => { | ||
| setSubstationPropertiesGlobalFilters(substationPropertiesGlobalFilters); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the initial metadata fetch failure path.
If fetchSubstationPropertiesGlobalFilters() rejects here, the effect leaves an unhandled promise rejection and the provider never recovers to a defined fallback state.
Suggested fix
useEffect(() => {
- fetchSubstationPropertiesGlobalFilters().then(({ substationPropertiesGlobalFilters }) => {
- setSubstationPropertiesGlobalFilters(substationPropertiesGlobalFilters);
- });
- }, []);
+ let cancelled = false;
+
+ fetchSubstationPropertiesGlobalFilters()
+ .then(({ substationPropertiesGlobalFilters }) => {
+ if (!cancelled) {
+ setSubstationPropertiesGlobalFilters(substationPropertiesGlobalFilters ?? new Map());
+ }
+ })
+ .catch((error) => {
+ snackWithFallback(snackError, error as Error, {
+ headerId: 'ComputationFilterResultsError',
+ });
+ if (!cancelled) {
+ setSubstationPropertiesGlobalFilters(new Map());
+ }
+ });
+
+ return () => {
+ cancelled = true;
+ };
+ }, [snackError]);📝 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
| useEffect(() => { | |
| fetchSubstationPropertiesGlobalFilters().then(({ substationPropertiesGlobalFilters }) => { | |
| setSubstationPropertiesGlobalFilters(substationPropertiesGlobalFilters); | |
| }); | |
| }, []); | |
| useEffect(() => { | |
| let cancelled = false; | |
| fetchSubstationPropertiesGlobalFilters() | |
| .then(({ substationPropertiesGlobalFilters }) => { | |
| if (!cancelled) { | |
| setSubstationPropertiesGlobalFilters(substationPropertiesGlobalFilters ?? new Map()); | |
| } | |
| }) | |
| .catch((error) => { | |
| snackWithFallback(snackError, error as Error, { | |
| headerId: 'ComputationFilterResultsError', | |
| }); | |
| if (!cancelled) { | |
| setSubstationPropertiesGlobalFilters(new Map()); | |
| } | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [snackError]); |
🤖 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/results/common/global-filter/global-filter-provider.tsx`
around lines 82 - 86, The initial metadata load in the useEffect inside
GlobalFilterProvider does not handle rejections, so
fetchSubstationPropertiesGlobalFilters() can leave an unhandled promise
rejection and no fallback state. Update the effect to catch errors from
fetchSubstationPropertiesGlobalFilters(), and ensure
setSubstationPropertiesGlobalFilters is still driven to a defined fallback value
when the request fails so the provider remains in a recoverable state.
Comment on lines
+166
to
+194
| const addFiltersToGlobalFiltersOptions = useCallback( | ||
| async (elementIds: UUID[]) => { | ||
| const elements: ElementAttributes[] = await fetchElementsInfos(elementIds); | ||
| const newlySelectedFilters: GlobalFilter[] = []; | ||
| elements.forEach((element: ElementAttributes) => { | ||
| // ignore already selected filters and non-generic filters : | ||
| if (!selectedGlobalFilters.find((filter) => filter.uuid && filter.uuid === element.elementUuid)) { | ||
| // add the others | ||
| const substationOrVoltageLevel = | ||
| element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION || | ||
| element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL; | ||
| newlySelectedFilters.push({ | ||
| id: element.elementUuid, | ||
| uuid: element.elementUuid, | ||
| equipmentType: element.specificMetadata?.equipmentType, | ||
| label: element.elementName, | ||
| filterType: substationOrVoltageLevel ? FilterType.SUBSTATION_OR_VL : FilterType.GENERIC_FILTER, | ||
| filterTypeFromMetadata: element.specificMetadata?.type, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| dispatch(addToGlobalFilterOptions(newlySelectedFilters)); | ||
| newlySelectedFilters.forEach((filter) => | ||
| dispatch(addToSelectedGlobalFilters(tableType, tableUuid, [filter.id])) | ||
| ); | ||
| }, | ||
| [dispatch, selectedGlobalFilters, tableType, tableUuid] | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Deduplicate against selectedFilterIds, not selectedGlobalFilters.
selectedGlobalFilters is built by resolving selectedFilterIds through globalFilterOptions and dropping misses on Lines 74-78. If Redux still has a selected ID whose option is temporarily absent, Line 172 treats it as “not selected” and re-dispatches addToSelectedGlobalFilters for the same filter. Use the ID list itself for membership checks.
Suggested fix
const addFiltersToGlobalFiltersOptions = useCallback(
async (elementIds: UUID[]) => {
const elements: ElementAttributes[] = await fetchElementsInfos(elementIds);
+ const selectedIds = new Set(selectedFilterIds ?? []);
const newlySelectedFilters: GlobalFilter[] = [];
elements.forEach((element: ElementAttributes) => {
// ignore already selected filters and non-generic filters :
- if (!selectedGlobalFilters.find((filter) => filter.uuid && filter.uuid === element.elementUuid)) {
+ if (!selectedIds.has(element.elementUuid)) {
// add the others
const substationOrVoltageLevel =
element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION ||
element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL;
newlySelectedFilters.push({
@@
dispatch(addToSelectedGlobalFilters(tableType, tableUuid, [filter.id]))
);
},
- [dispatch, selectedGlobalFilters, tableType, tableUuid]
+ [dispatch, selectedFilterIds, tableType, tableUuid]
);📝 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
| const addFiltersToGlobalFiltersOptions = useCallback( | |
| async (elementIds: UUID[]) => { | |
| const elements: ElementAttributes[] = await fetchElementsInfos(elementIds); | |
| const newlySelectedFilters: GlobalFilter[] = []; | |
| elements.forEach((element: ElementAttributes) => { | |
| // ignore already selected filters and non-generic filters : | |
| if (!selectedGlobalFilters.find((filter) => filter.uuid && filter.uuid === element.elementUuid)) { | |
| // add the others | |
| const substationOrVoltageLevel = | |
| element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION || | |
| element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL; | |
| newlySelectedFilters.push({ | |
| id: element.elementUuid, | |
| uuid: element.elementUuid, | |
| equipmentType: element.specificMetadata?.equipmentType, | |
| label: element.elementName, | |
| filterType: substationOrVoltageLevel ? FilterType.SUBSTATION_OR_VL : FilterType.GENERIC_FILTER, | |
| filterTypeFromMetadata: element.specificMetadata?.type, | |
| }); | |
| } | |
| }); | |
| dispatch(addToGlobalFilterOptions(newlySelectedFilters)); | |
| newlySelectedFilters.forEach((filter) => | |
| dispatch(addToSelectedGlobalFilters(tableType, tableUuid, [filter.id])) | |
| ); | |
| }, | |
| [dispatch, selectedGlobalFilters, tableType, tableUuid] | |
| ); | |
| const addFiltersToGlobalFiltersOptions = useCallback( | |
| async (elementIds: UUID[]) => { | |
| const elements: ElementAttributes[] = await fetchElementsInfos(elementIds); | |
| const selectedIds = new Set(selectedFilterIds ?? []); | |
| const newlySelectedFilters: GlobalFilter[] = []; | |
| elements.forEach((element: ElementAttributes) => { | |
| // ignore already selected filters and non-generic filters : | |
| if (!selectedIds.has(element.elementUuid)) { | |
| // add the others | |
| const substationOrVoltageLevel = | |
| element.specificMetadata?.equipmentType === EquipmentType.SUBSTATION || | |
| element.specificMetadata?.equipmentType === EquipmentType.VOLTAGE_LEVEL; | |
| newlySelectedFilters.push({ | |
| id: element.elementUuid, | |
| uuid: element.elementUuid, | |
| equipmentType: element.specificMetadata?.equipmentType, | |
| label: element.elementName, | |
| filterType: substationOrVoltageLevel ? FilterType.SUBSTATION_OR_VL : FilterType.GENERIC_FILTER, | |
| filterTypeFromMetadata: element.specificMetadata?.type, | |
| }); | |
| } | |
| }); | |
| dispatch(addToGlobalFilterOptions(newlySelectedFilters)); | |
| newlySelectedFilters.forEach((filter) => | |
| dispatch(addToSelectedGlobalFilters(tableType, tableUuid, [filter.id])) | |
| ); | |
| }, | |
| [dispatch, selectedFilterIds, tableType, tableUuid] | |
| ); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 172-172: Prefer .some(…) over .find(…).
🤖 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/results/common/global-filter/global-filter-provider.tsx`
around lines 166 - 194, The duplicate-check in addFiltersToGlobalFiltersOptions
is using selectedGlobalFilters, which can miss IDs that are already selected but
not currently resolvable from globalFilterOptions. Update the membership test to
use the source-of-truth selectedFilterIds for this table instead, so
fetchElementsInfos results are only added when their id is not already present.
Keep the rest of the flow in GlobalFilterProvider the same, including
dispatch(addToGlobalFilterOptions) and the subsequent addToSelectedGlobalFilters
calls.
Comment on lines
+91
to
+116
| export function markNotFoundGlobalFiltersAsDeletedInState( | ||
| globalFilterOptions: GlobalFilter[], | ||
| tableState: GlobalFilterTableState | undefined, | ||
| notFoundGlobalFilters: GlobalFilter[] | ||
| ): MarkNotFoundGlobalFiltersAsDeletedResult { | ||
| const ids = new Set(notFoundGlobalFilters.map((filter) => filter.id)); | ||
| const updatedGlobalFilterOptions = globalFilterOptions.map((globalFilter) => | ||
| ids.has(globalFilter.id) ? { ...globalFilter, deleted: true } : globalFilter | ||
| ); | ||
|
|
||
| if (!tableState) { | ||
| return { | ||
| globalFilterOptions: updatedGlobalFilterOptions, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| globalFilterOptions: updatedGlobalFilterOptions, | ||
| tableState: { | ||
| selected: tableState.selected, | ||
| recents: tableState.recents?.length | ||
| ? tableState.recents.filter((recentFilter) => !ids.has(recentFilter.id)) | ||
| : tableState.recents, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect prior reducer behavior for MARK_NOT_FOUND_GLOBAL_FILTERS_AS_DELETED to confirm whether `selected` was pruned.
rg -nP -C5 'MARK_NOT_FOUND_GLOBAL_FILTERS_AS_DELETED' src/redux/reducer.ts
git log -p --follow -S 'deleted = true' -- src/redux/reducer.ts | sed -n '1,120p'Repository: gridsuite/gridstudy-app
Length of output: 1169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline helper ==\n'
ast-grep outline src/components/results/common/global-filter/global-filter-state.utils.ts --view expanded || true
printf '\n== outline reducer ==\n'
ast-grep outline src/redux/reducer.ts --view expanded || true
printf '\n== reducer slice around MARK_NOT_FOUND_GLOBAL_FILTERS_AS_DELETED ==\n'
sed -n '1658,1698p' src/redux/reducer.ts
printf '\n== helper slice around markNotFoundGlobalFiltersAsDeletedInState ==\n'
sed -n '1,180p' src/components/results/common/global-filter/global-filter-state.utils.tsRepository: gridsuite/gridstudy-app
Length of output: 7934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search global filter table selected usage ==\n'
rg -n "tableState\.selected|selected: .*tableState\.selected|selected\)" src/components src/redux src -g '*global-filter*' -g '!**/node_modules/**' || true
printf '\n== search deleted handling in global filter code ==\n'
rg -n "deleted" src/components/results/common/global-filter src/redux src -g '!**/node_modules/**' || true
printf '\n== search selectors/helpers for tableFilters.globalFilters ==\n'
rg -n "tableFilters\.globalFilters|globalFilterOptions" src/components src/redux -g '!**/node_modules/**' || trueRepository: gridsuite/gridstudy-app
Length of output: 21273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== provider slice ==\n'
sed -n '100,140p' src/components/results/common/global-filter/global-filter-provider.tsx
printf '\n== hook slice ==\n'
sed -n '1,80p' src/components/results/common/global-filter/use-selected-global-filters.ts
printf '\n== middleware slice ==\n'
sed -n '1,120p' src/redux/globalFiltersMiddleware.tsRepository: gridsuite/gridstudy-app
Length of output: 7818
Prune deleted filters from selected The helper only removes not-found IDs from recents; selected is left unchanged, so deleted filters still count as selected and are picked up by the backend sync path. Remove those IDs from selected here too, or explicitly filter out deleted options at the consumer boundary.
🤖 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/results/common/global-filter/global-filter-state.utils.ts`
around lines 91 - 116, The helper markNotFoundGlobalFiltersAsDeletedInState
currently marks matching GlobalFilter entries as deleted and prunes them from
tableState.recents, but it leaves tableState.selected untouched, so deleted
filters remain selected and can still flow into backend sync. Update
markNotFoundGlobalFiltersAsDeletedInState to also filter the
notFoundGlobalFilters IDs out of tableState.selected (and keep the existing
recents cleanup), or ensure the same exclusion happens wherever
GlobalFilterTableState is consumed.
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
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 (1)
src/components/results/common/global-filter/utils/global-filter-sync.utils.ts (1)
23-48: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftOverlapping in-flight saves for the same
indexare not guarded.The
!debouncedSyncTimers[index]check in.finally()only detects a newer pending timer, not a previous save still in flight. If a new debounced call fires while an earlier save for the sameindexis still awaiting its network response, both saves proceed concurrently with no ordering guarantee — the earlier (stale) save can resolve after the later one and overwrite it on the backend. It also causesunmarkEditingGlobalFilterto run while an older save is still pending.This logic is carried over from the previous inline implementation in
globalFiltersMiddleware.ts, but as it's now a shared, reusable utility it's worth hardening here.🔧 Suggested fix: serialize saves per index
-const debouncedSyncTimers: Record<string, ReturnType<typeof setTimeout>> = {}; +const debouncedSyncTimers: Record<string, ReturnType<typeof setTimeout>> = {}; +const pendingSyncs: Record<string, Promise<void>> = {}; export function syncGlobalFilters({ index, globalFilters, save, onError, debounceMs = DEFAULT_GLOBAL_FILTER_SYNC_DEBOUNCE_MS, }: SyncGlobalFiltersParameters): void { markEditingGlobalFilter(index); if (debouncedSyncTimers[index]) { clearTimeout(debouncedSyncTimers[index]); } debouncedSyncTimers[index] = setTimeout(() => { - save(globalFilters) - .catch(onError) - .finally(() => { - // Only end the sync if no new debounce timer was started while the save was in flight. - if (!debouncedSyncTimers[index]) { - unmarkEditingGlobalFilter(index); - } - }); - - delete debouncedSyncTimers[index]; + delete debouncedSyncTimers[index]; + const previous = pendingSyncs[index] ?? Promise.resolve(); + const current: Promise<void> = previous + .then(() => save(globalFilters)) + .catch(onError) + .finally(() => { + // Only end the sync if this is still the latest chained save and no new timer was scheduled. + if (pendingSyncs[index] === current && !debouncedSyncTimers[index]) { + unmarkEditingGlobalFilter(index); + } + }); + pendingSyncs[index] = current; }, debounceMs); }🤖 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/results/common/global-filter/utils/global-filter-sync.utils.ts` around lines 23 - 48, `syncGlobalFilters` only tracks pending debounce timers, so overlapping saves for the same `index` can still run concurrently and finish out of order. Harden `syncGlobalFilters` in `global-filter-sync.utils.ts` by serializing saves per `index` (for example, track an in-flight save/promise or a version token and skip/chain newer calls until the prior `save(globalFilters)` settles), and only call `unmarkEditingGlobalFilter` when both the debounce timer and any in-flight save for that `index` are done.
♻️ Duplicate comments (1)
src/components/results/common/global-filter/ui/global-filter-dropdown-panel.tsx (1)
123-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDirectory selector can get stuck open on cancel/failure — unresolved from prior review.
When
valuesisundefined(cancel), the function returns at Line 126 beforesetDirectoryItemSelectorOpen(false)runs at Line 131, leaving the selector controlled-open. There's also notry/finally, so a rejectedaddFiltersToGlobalFiltersOptionscall skips cleanup too. This is the same code flagged in a previous review round and still isn't fixed.🐛 Proposed fix
const addSelectedFiltersToGlobalFiltersOptions = useCallback( async (values: TreeViewFinderNodeProps[] | undefined) => { - if (!values) { - return; - } - - setOpenedDropdown(true); - await addFiltersToGlobalFiltersOptions(values.map((value) => value.id)); - setDirectoryItemSelectorOpen(false); + try { + if (values?.length) { + setOpenedDropdown(true); + await addFiltersToGlobalFiltersOptions(values.map((value) => value.id)); + } + } finally { + setDirectoryItemSelectorOpen(false); + } }, [addFiltersToGlobalFiltersOptions, setDirectoryItemSelectorOpen, setOpenedDropdown] );🤖 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/results/common/global-filter/ui/global-filter-dropdown-panel.tsx` around lines 123 - 134, In addSelectedFiltersToGlobalFiltersOptions, the directory selector can stay open on cancel or when addFiltersToGlobalFiltersOptions fails because the early return skips cleanup and there is no guaranteed close path. Update this callback in global-filter-dropdown-panel so setDirectoryItemSelectorOpen(false) always runs, using a try/finally around the async addFiltersToGlobalFiltersOptions call and handling the undefined values case before or within that flow; keep setOpenedDropdown and the callback dependencies intact.
🤖 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/components/results/common/global-filter/utils/global-filter-sync.utils.ts`:
- Around line 23-48: `syncGlobalFilters` only tracks pending debounce timers, so
overlapping saves for the same `index` can still run concurrently and finish out
of order. Harden `syncGlobalFilters` in `global-filter-sync.utils.ts` by
serializing saves per `index` (for example, track an in-flight save/promise or a
version token and skip/chain newer calls until the prior `save(globalFilters)`
settles), and only call `unmarkEditingGlobalFilter` when both the debounce timer
and any in-flight save for that `index` are done.
---
Duplicate comments:
In
`@src/components/results/common/global-filter/ui/global-filter-dropdown-panel.tsx`:
- Around line 123-134: In addSelectedFiltersToGlobalFiltersOptions, the
directory selector can stay open on cancel or when
addFiltersToGlobalFiltersOptions fails because the early return skips cleanup
and there is no guaranteed close path. Update this callback in
global-filter-dropdown-panel so setDirectoryItemSelectorOpen(false) always runs,
using a try/finally around the async addFiltersToGlobalFiltersOptions call and
handling the undefined values case before or within that flow; keep
setOpenedDropdown and the callback dependencies intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bb9bb48-d81f-4e2d-902d-92c88bf343c8
📒 Files selected for processing (53)
src/components/app.jsxsrc/components/results/common/global-filter/adapter/global-filter-app-data.tssrc/components/results/common/global-filter/adapter/global-filter-provider.tsxsrc/components/results/common/global-filter/context/global-filter-context-provider.tsxsrc/components/results/common/global-filter/context/global-filter-context.tssrc/components/results/common/global-filter/context/global-filter-context.type.tssrc/components/results/common/global-filter/global-filter-selector.tsxsrc/components/results/common/global-filter/global-filter.style.tssrc/components/results/common/global-filter/global-filter.type.tssrc/components/results/common/global-filter/hooks/use-computation-global-filters.tssrc/components/results/common/global-filter/hooks/use-global-filter-options.tssrc/components/results/common/global-filter/hooks/use-global-filter-results.tssrc/components/results/common/global-filter/hooks/use-selected-global-filters.tssrc/components/results/common/global-filter/ui/global-filter-dropdown-panel.tsxsrc/components/results/common/global-filter/ui/global-filter.tsxsrc/components/results/common/global-filter/ui/label-with-info-tooltip.tsxsrc/components/results/common/global-filter/ui/selected-global-filters.tsxsrc/components/results/common/global-filter/utils/build-valid-global-filters.tssrc/components/results/common/global-filter/utils/global-filter-state.utils.tssrc/components/results/common/global-filter/utils/global-filter-sync.utils.tssrc/components/results/common/global-filter/utils/global-filter-utils.tssrc/components/results/common/utils.tssrc/components/results/loadflow/load-flow-result-tab.tsxsrc/components/results/pccmin/pcc-min-export-button.tsxsrc/components/results/pccmin/pcc-min-result-tab.tsxsrc/components/results/pccmin/pcc-min-result.tsxsrc/components/results/pccmin/pcc-min-result.type.tssrc/components/results/securityanalysis/security-analysis-result-tab.tsxsrc/components/results/securityanalysis/security-analysis.type.tssrc/components/results/sensitivity-analysis/paged-sensitivity-analysis-result.tsxsrc/components/results/sensitivity-analysis/sensitivity-analysis-export-button.tsxsrc/components/results/sensitivity-analysis/sensitivity-analysis-result-tab.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-export-button.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-result-tab.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-result.tsxsrc/components/spreadsheet-view/add-spreadsheet/dialogs/add-spreadsheet-utils.tssrc/components/spreadsheet-view/spreadsheet/spreadsheet-content/hooks/use-spreadsheet-gs-filter.tssrc/components/spreadsheet-view/spreadsheet/spreadsheet-toolbar/row-counter/use-filtered-row-counter.tsxsrc/components/spreadsheet-view/spreadsheet/spreadsheet-toolbar/save/save-spreadsheet-collection-dialog.tsxsrc/components/spreadsheet-view/spreadsheet/spreadsheet-toolbar/save/save-spreadsheet-dialog.tsxsrc/components/spreadsheet-view/types/spreadsheet.type.tssrc/components/voltage-init-result-tab.tsxsrc/redux/actions.tssrc/redux/globalFiltersMiddleware.tssrc/redux/reducer.tssrc/redux/reducer.type.tssrc/services/study/filter.tssrc/services/study/loadflow.tssrc/services/study/pcc-min.tssrc/services/study/sensitivity-analysis.tssrc/services/study/short-circuit-analysis.tssrc/services/study/study-config.tssrc/services/study/voltage-init.ts
💤 Files with no reviewable changes (1)
- src/components/results/common/global-filter/adapter/global-filter-app-data.ts
✅ Files skipped from review due to trivial changes (25)
- src/components/results/securityanalysis/security-analysis.type.ts
- src/components/results/shortcircuit/shortcircuit-analysis-result-tab.tsx
- src/redux/reducer.type.ts
- src/components/results/sensitivity-analysis/sensitivity-analysis-result-tab.tsx
- src/components/results/common/global-filter/hooks/use-selected-global-filters.ts
- src/components/spreadsheet-view/spreadsheet/spreadsheet-toolbar/save/save-spreadsheet-dialog.tsx
- src/components/results/shortcircuit/shortcircuit-analysis-result.tsx
- src/services/study/pcc-min.ts
- src/services/study/short-circuit-analysis.ts
- src/services/study/study-config.ts
- src/redux/actions.ts
- src/components/results/securityanalysis/security-analysis-result-tab.tsx
- src/components/results/common/global-filter/context/global-filter-context.type.ts
- src/components/results/common/global-filter/hooks/use-computation-global-filters.ts
- src/components/spreadsheet-view/spreadsheet/spreadsheet-toolbar/save/save-spreadsheet-collection-dialog.tsx
- src/components/results/common/global-filter/hooks/use-global-filter-options.ts
- src/components/results/common/global-filter/hooks/use-global-filter-results.ts
- src/components/spreadsheet-view/add-spreadsheet/dialogs/add-spreadsheet-utils.ts
- src/components/results/shortcircuit/shortcircuit-analysis-export-button.tsx
- src/components/results/pccmin/pcc-min-export-button.tsx
- src/services/study/sensitivity-analysis.ts
- src/components/results/pccmin/pcc-min-result.type.ts
- src/components/voltage-init-result-tab.tsx
- src/services/study/filter.ts
- src/components/results/sensitivity-analysis/sensitivity-analysis-export-button.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/redux/reducer.ts
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.
PR Summary