Open
Conversation
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughConsolidates column filters into state.tableFilters.columnsFilters keyed by TableType and tab; removes useFilterSelector and legacy spreadsheet/logs store slices; replaces GridApi/update-callback filter persistence with persistSpreadsheetColumnFilters / persistComputationColumnFilters and central updateColumnFiltersAction; adds selectors getColumnFiltersFromState / getColumnFiltersFromStore. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant Hook as useCustomAggridColumnFilter
participant Persist as persistSpreadsheetColumnFilters / persistComputationColumnFilters
participant Dispatcher as updateColumnFiltersAction
participant Reducer
Component->>Hook: user changes filter value (colId, params)
Hook->>Persist: persist...(studyUuid, tabUuid, filters, colDef, onError)
Persist->>Dispatcher: dispatch updateColumnFiltersAction(TableType, tab, filters)
Dispatcher->>Reducer: reducer handles UPDATE_COLUMN_FILTERS
Reducer-->>Component: state.tableFilters.columnsFilters updated
sequenceDiagram
participant ExportBtn
participant StoreSelector as getColumnFiltersFromStore
participant Service as export...AsCsv
ExportBtn->>StoreSelector: onExport -> get filters for TableType/tab
StoreSelector-->>ExportBtn: FilterConfig[] | undefined
ExportBtn->>Service: call export...AsCsv(filters, ...)
Service-->>ExportBtn: CSV response / download or error
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (1)
src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts (1)
107-119: ExcludingupdateFilterfrom debounced callback dependencies is a known pattern but fragile.The comment at lines 107-111 correctly explains the rationale: including
updateFilterwould recreate the debounced function on every filter change, canceling pending updates. However, this relies on the assumption that other dependencies ofupdateFilter(likefilters,studyUuid,type,tab,colId,colDef) remain stable.The comment at line 111 acknowledges this: "PS: it only works because most of the props never change..."
This is acceptable for now, but consider using
useRefto store the latestupdateFiltercallback if stability becomes an issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts` around lines 107 - 119, The debouncedUpdateFilter currently closes over an unstable updateFilter which is excluded from deps; instead capture the latest updateFilter in a ref and have the debounced callback call ref.current(data) so the debounced function can safely remain stable (deps only [debounceMs]); specifically create a ref (e.g., updateFilterRef) and assign updateFilter to updateFilterRef.current whenever updateFilter changes, then in the useCallback/debounce body call updateFilterRef.current(data) and keep isEditingRef.current = false as before to avoid recreating debouncedUpdateFilter on every render.
🤖 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/results/securityanalysis/security-analysis-result-tab.tsx`:
- Line 233: The column filter changes from useComputationColumnFilters are not
triggering pagination reset; add a useEffect in the component that watches the
filters returned by useComputationColumnFilters and calls
resetPaginationIfNKResults when filters change (mirroring how
useComputationGlobalFilters passes a callback), or alternatively update
useComputationColumnFilters to accept an optional onChange callback (like
useComputationGlobalFilters) and invoke that callback on filter updates;
reference useComputationColumnFilters, filters, useEffect,
resetPaginationIfNKResults, and useComputationGlobalFilters to locate where to
add the effect or hook change.
In `@src/redux/reducer.ts`:
- Around line 1600-1628: The UPDATE_COLUMN_FILTERS reducer currently resets
pagination for all tabs in a given analysis type; modify the switch cases inside
builder.addCase(UPDATE_COLUMN_FILTERS, ...) to only reset the pagination for the
specific tab given by action.filterSubType (not all tabs). For each case
(TableType.SecurityAnalysis, TableType.SensitivityAnalysis,
TableType.ShortcircuitAnalysis, TableType.PccMin) replace the loop over *_TABS
with a single assignment to
state[SECURITY_ANALYSIS_PAGINATION_STORE_FIELD][filterSubType].page = 0 (and the
corresponding pagination store field for each case), ensuring you reference the
action's filterSubType variable from UpdateColumnFiltersAction when locating the
tab to reset. Ensure the existing columnsFilters initialization
(state.tableFilters.columnsFilters[filterType] ??= {}) and the assignment
state.tableFilters.columnsFilters[filterType][filterSubType] = filters remain
unchanged.
In `@src/redux/reducer.type.ts`:
- Around line 136-138: Persisted column filters changed shape (previously nested
object wrapping the array) and must be normalized on rehydrate: add a
migration/normalization step that walks the TableFiltersState.columnsFilters
entries, detects legacy values (e.g., an object like { filters: FilterConfig[] }
or similar wrapper) and replaces them with the bare FilterConfig[] expected by
the new code; implement this in the state rehydration path (or a helper like
normalizePersistedTableFilters called from reducer initialization) so
save-spreadsheet-collection-dialog.tsx and use-filtered-row-counter.tsx can
assume columnsFilters contains FilterConfig[] directly.
In `@src/redux/selectors/filter-selectors.ts`:
- Around line 11-16: The selector getColumnFiltersFromState currently always
returns an array (possibly empty) which breaks callers expecting a nullable
value; restore the original "no filters" contract by returning undefined when no
column filters exist instead of an empty array. Update getColumnFiltersFromState
to return state.tableFilters.columnsFilters?.[filterType]?.[filterTab] (no
nullish coalescing) so callers that check for truthiness/optional chaining keep
working, and ensure the function signature still returns FilterConfig[] |
undefined if needed.
---
Nitpick comments:
In
`@src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts`:
- Around line 107-119: The debouncedUpdateFilter currently closes over an
unstable updateFilter which is excluded from deps; instead capture the latest
updateFilter in a ref and have the debounced callback call ref.current(data) so
the debounced function can safely remain stable (deps only [debounceMs]);
specifically create a ref (e.g., updateFilterRef) and assign updateFilter to
updateFilterRef.current whenever updateFilter changes, then in the
useCallback/debounce body call updateFilterRef.current(data) and keep
isEditingRef.current = false as before to avoid recreating debouncedUpdateFilter
on every render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6f4d408-1012-4dc6-b07c-ed47ca773d59
📒 Files selected for processing (44)
src/components/app.jsxsrc/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-autocomplete-filter.tsxsrc/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-boolean-filter.tsxsrc/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-duration-filter.tsxsrc/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-filter.tsxsrc/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.tssrc/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-comparator-filter.tssrc/components/grid-layout/cards/diagrams/singleLineDiagram/single-line-diagram-content.tsxsrc/components/report-viewer/log-table.tsxsrc/components/results/common/column-filter/update-computation-columns-filters.tssrc/components/results/common/column-filter/use-computation-column-filters.tssrc/components/results/common/use-ag-grid-initial-column-filters.tssrc/components/results/dynamicsimulation/dynamic-simulation-result-timeline.tsxsrc/components/results/loadflow/load-flow-result-utils.tssrc/components/results/pccmin/pcc-min-export-button.tsxsrc/components/results/pccmin/pcc-min-result-table.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-result-utils.tssrc/components/results/securityanalysis/use-security-analysis-column-defs.tsxsrc/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.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-export-button.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-result-table.tsxsrc/components/results/shortcircuit/shortcircuit-analysis-result.tsxsrc/components/run-button-container.jsxsrc/components/spreadsheet-view/add-spreadsheet/dialogs/add-spreadsheet-utils.tssrc/components/spreadsheet-view/columns/column-creation-dialog.tsxsrc/components/spreadsheet-view/columns/common-column-definitions.tssrc/components/spreadsheet-view/columns/persist-spreadsheet-column-filters.tssrc/components/spreadsheet-view/spreadsheet/spreadsheet-content/spreadsheet-content.tsxsrc/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/hooks/use-filter-selector.tssrc/redux/actions.tssrc/redux/reducer.tssrc/redux/reducer.type.tssrc/redux/selectors/filter-selectors.tssrc/redux/selectors/filter-store-selectors.tssrc/types/custom-aggrid-types.tssrc/utils/store-sort-filter-fields.ts
💤 Files with no reviewable changes (6)
- src/components/results/shortcircuit/shortcircuit-analysis-result.tsx
- src/components/results/sensitivity-analysis/paged-sensitivity-analysis-result.tsx
- src/types/custom-aggrid-types.ts
- src/components/results/pccmin/pcc-min-result.tsx
- src/hooks/use-filter-selector.ts
- src/utils/store-sort-filter-fields.ts
src/components/results/securityanalysis/security-analysis-result-tab.tsx
Show resolved
Hide resolved
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/services/study/sensitivity-analysis.ts (1)
69-69: Consider applying the same pattern tofetchSensitivityAnalysisResultfor consistency.The
fetchSensitivityAnalysisResultfunction uses optional chaining on line 80 (filters?.length) but declaresfiltersas required (FilterConfig[]). For consistency with the updatedexportSensitivityResultsAsCsv, consider updating this parameter type as well if callers may passundefined.♻️ Suggested change for consistency
export function fetchSensitivityAnalysisResult( studyUuid: UUID, currentNodeUuid: UUID, currentRootNetworkUuid: UUID, selector: any, - filters: FilterConfig[], + filters: FilterConfig[] | undefined, globalFilters: GlobalFilters | undefined ): Promise<SensitivityResult | null> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/study/sensitivity-analysis.ts` at line 69, fetchSensitivityAnalysisResult declares filters as required FilterConfig[] but its code uses optional chaining (filters?.length); update the function signature to accept filters?: FilterConfig[] (or FilterConfig[] | undefined) to match exportSensitivityResultsAsCsv and callers that may pass undefined, and then ensure any internal logic (in fetchSensitivityAnalysisResult and its callers) handles the undefined case consistently with exportSensitivityResultsAsCsv.src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts (1)
179-193: Consider syncingtolerancefrom external filter changes.When syncing from Redux store, only
valueandtypeare synced fromfilterObject. IffilterObject.toleranceexists and differs from local state, subsequent calls tohandleChangeComparatorwill use stale tolerance values (lines 148, 155, 164).♻️ Proposed fix to sync tolerance
if (filterObject) { setSelectedFilterData(filterObject.value); setSelectedFilterComparator(filterObject.type ?? ''); + setTolerance(filterObject.tolerance); } else { setSelectedFilterData(undefined); + setTolerance(undefined); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts` around lines 179 - 193, The effect in use-custom-aggrid-column-filter's useEffect currently syncs only filterObject.value and filterObject.type but ignores filterObject.tolerance, which can leave local tolerance stale when external filters change; update the useEffect (the block that reads filters?.find by colId) to also call the local setter for tolerance (the same state used by handleChangeComparator), i.e. setSelectedTolerance(filterObject.tolerance) when present and clear it (setSelectedTolerance(undefined) or appropriate default) when no filterObject exists, so handleChangeComparator sees the up-to-date tolerance value.
🤖 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/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts`:
- Around line 107-119: The debounced function created in
use-custom-aggrid-column-filter (debouncedUpdateFilter via useCallback +
debounce) lacks cleanup and can fire after unmount; add a useEffect that depends
on debouncedUpdateFilter and returns a cleanup function which calls
debouncedUpdateFilter.clear() (or the appropriate cancel/clear method returned
by debounce) to cancel any pending calls on unmount, keeping the existing
dependency strategy that excludes updateFilter and still referencing debounceMs,
updateFilter, isEditingRef, and debouncedUpdateFilter to locate the code to
modify.
In `@src/components/report-viewer/log-table.tsx`:
- Around line 108-110: The selector fallback currently uses a new array literal
(?? []) which creates a new reference each render and causes the filters
returned by useSelector (where you call getColumnFiltersFromState with
TableType.Logs and reportType) to change unnecessarily and re-trigger the
onFiltersChanged effect; fix this by defining a single module-level constant
like EMPTY_FILTERS: FilterConfig[] = [] (outside the component) and use that
constant as the fallback in the useSelector call so the reference stays stable
when the store value is undefined.
---
Nitpick comments:
In
`@src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts`:
- Around line 179-193: The effect in use-custom-aggrid-column-filter's useEffect
currently syncs only filterObject.value and filterObject.type but ignores
filterObject.tolerance, which can leave local tolerance stale when external
filters change; update the useEffect (the block that reads filters?.find by
colId) to also call the local setter for tolerance (the same state used by
handleChangeComparator), i.e. setSelectedTolerance(filterObject.tolerance) when
present and clear it (setSelectedTolerance(undefined) or appropriate default)
when no filterObject exists, so handleChangeComparator sees the up-to-date
tolerance value.
In `@src/services/study/sensitivity-analysis.ts`:
- Line 69: fetchSensitivityAnalysisResult declares filters as required
FilterConfig[] but its code uses optional chaining (filters?.length); update the
function signature to accept filters?: FilterConfig[] (or FilterConfig[] |
undefined) to match exportSensitivityResultsAsCsv and callers that may pass
undefined, and then ensure any internal logic (in fetchSensitivityAnalysisResult
and its callers) handles the undefined case consistently with
exportSensitivityResultsAsCsv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 467d4eab-07c7-4b4a-93b1-88b941a0370e
📒 Files selected for processing (7)
src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.tssrc/components/report-viewer/log-table.tsxsrc/components/spreadsheet-view/columns/column-creation-dialog.tsxsrc/components/spreadsheet-view/spreadsheet/spreadsheet-content/spreadsheet-content.tsxsrc/redux/selectors/filter-selectors.tssrc/redux/selectors/filter-store-selectors.tssrc/services/study/sensitivity-analysis.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/spreadsheet-view/spreadsheet/spreadsheet-content/spreadsheet-content.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/redux/selectors/filter-selectors.ts
- src/redux/selectors/filter-store-selectors.ts
- src/components/spreadsheet-view/columns/column-creation-dialog.tsx
src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts
Show resolved
Hide resolved
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…efactor-columns-filters
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/spreadsheet-view/columns/column-creation-dialog.tsx (1)
299-303: Consider the implications of fire-and-forget filter persistence.The
persistFilterscall is not awaited, meaning the column update proceeds regardless of whether filter persistence succeeds. If filter persistence fails (network error, etc.) but the column update succeeds, there could be a temporary inconsistency where stale filters reference the old column ID.Given the PR's stated data flow (backend → notification → Redux), this should eventually self-correct via backend notifications. If this is the intended behavior, consider adding a brief comment noting that consistency is restored via the notification mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/spreadsheet-view/columns/column-creation-dialog.tsx` around lines 299 - 303, persistFilters is currently invoked fire-and-forget when updating an existing column (see existingColumn, isUpdate, filters and the call persistFilters(studyUuid, updatedFilters)); either await the persistFilters call and handle errors (e.g., try/catch and surface/log failures) to avoid stale filters if persistence fails, or if the design intentionally relies on backend→notification→Redux to restore consistency, add a concise comment above the persistFilters call explaining that eventual consistency is expected and why the call is not awaited.
🤖 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/spreadsheet-view/columns/column-creation-dialog.tsx`:
- Around line 299-303: persistFilters is currently invoked fire-and-forget when
updating an existing column (see existingColumn, isUpdate, filters and the call
persistFilters(studyUuid, updatedFilters)); either await the persistFilters call
and handle errors (e.g., try/catch and surface/log failures) to avoid stale
filters if persistence fails, or if the design intentionally relies on
backend→notification→Redux to restore consistency, add a concise comment above
the persistFilters call explaining that eventual consistency is expected and why
the call is not awaited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b871c205-c542-4a74-aea1-60203bdc649b
📒 Files selected for processing (4)
src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.tssrc/components/report-viewer/log-table.tsxsrc/components/spreadsheet-view/columns/column-creation-dialog.tsxsrc/components/spreadsheet-view/spreadsheet/spreadsheet-content/spreadsheet-content.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/report-viewer/log-table.tsx
- src/components/spreadsheet-view/spreadsheet/spreadsheet-content/spreadsheet-content.tsx
- src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts
src/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-boolean-filter.tsx
Outdated
Show resolved
Hide resolved
src/components/custom-aggrid/custom-aggrid-filters/custom-aggrid-duration-filter.tsx
Outdated
Show resolved
Hide resolved
…f iterating over tab arrays.
# Conflicts: # src/redux/reducer.type.ts
Contributor
AbdelHedhili
left a comment
AbdelHedhili
left a comment
There was a problem hiding this comment.
just some empty lines to remove. Also prettier error to fix.
src/components/results/dynamicsimulation/dynamic-simulation-result-timeline.tsx
Outdated
Show resolved
Hide resolved
src/components/results/dynamicsimulation/dynamic-simulation-result-timeline.tsx
Outdated
Show resolved
Hide resolved
src/components/results/dynamicsimulation/dynamic-simulation-result-timeline.tsx
Outdated
Show resolved
Hide resolved
src/components/results/dynamicsimulation/dynamic-simulation-result-timeline.tsx
Outdated
Show resolved
Hide resolved
src/components/spreadsheet-view/columns/common-column-definitions.ts
Outdated
Show resolved
Hide resolved
src/components/spreadsheet-view/columns/common-column-definitions.ts
Outdated
Show resolved
Hide resolved
src/components/spreadsheet-view/columns/common-column-definitions.ts
Outdated
Show resolved
Hide resolved
src/components/spreadsheet-view/columns/common-column-definitions.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
AbdelHedhili
approved these changes
Mar 25, 2026
Contributor
AbdelHedhili
left a comment
AbdelHedhili
left a comment
There was a problem hiding this comment.
test : ok
code : ok for me but waiting on @ghazwarhili
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
Simplifying and securing column filters state updates :