Conversation
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
📝 WalkthroughWalkthroughConsolidates column filters into state.tableFilters.columnsFilters keyed by TableType and tab; removes useFilterSelector and GridApi-based update callbacks; adds selectors Changes
Sequence Diagram(s)mermaid mermaid 🚥 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
| builder.addCase(UPDATE_COLUMN_FILTERS, (state, action: UpdateColumnFiltersAction) => { | ||
| const { filterType, filterSubType, filters } = action; | ||
| state.tableFilters.columnsFilters[filterType] ??= {}; | ||
| state.tableFilters.columnsFilters[filterType][filterSubType] ??= { | ||
| columns: [], | ||
| }; | ||
| state.tableFilters.columnsFilters[filterType][filterSubType].columns = filters; | ||
| state.tableFilters.columnsFilters[filterType][filterSubType] = filters; | ||
|
|
||
| // Reset pagination to page 0 when column filters change | ||
| switch (filterType) { | ||
| case TableType.SecurityAnalysis: | ||
| SECURITY_ANALYSIS_TABS.forEach((tab) => { | ||
| state[SECURITY_ANALYSIS_PAGINATION_STORE_FIELD][tab].page = 0; | ||
| }); | ||
| break; | ||
| case TableType.SensitivityAnalysis: | ||
| SENSITIVITY_ANALYSIS_TABS.forEach((tab) => { | ||
| state[SENSITIVITY_ANALYSIS_PAGINATION_STORE_FIELD][tab].page = 0; | ||
| }); | ||
| break; | ||
| case TableType.ShortcircuitAnalysis: | ||
| SHORTCIRCUIT_ANALYSIS_TABS.forEach((tab) => { | ||
| state[SHORTCIRCUIT_ANALYSIS_PAGINATION_STORE_FIELD][tab].page = 0; | ||
| }); | ||
| break; | ||
| case TableType.PccMin: | ||
| PCCMIN_ANALYSIS_TABS.forEach((tab) => { | ||
| state[PCCMIN_ANALYSIS_PAGINATION_STORE_FIELD][tab].page = 0; | ||
| }); | ||
| break; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is resetting pagination for all tabs when filtering one tab a common UX pattern in data grids?
💡 Result:
No, resetting pagination for all tabs when filtering one tab is not a common UX pattern in data grids. In tabbed data grids, each tab typically maintains independent pagination, filtering, and sorting state to preserve user context and avoid disorienting switches between tabs. Sources confirm independent state management is standard: - AG Grid multi-tab implementations use separate grid instances per tab, each managing its own pagination and filters independently (23). - Tabulator persists state per table instance, implying separate handling for multiple tables/tabs (16,18). - Shadcn and Filament table tabs treat tabs as pre-configured filters on a single table, not separate grids with cross-tab resets (19,22). - MUI X and other grids provide per-grid pagination reset props/options for filter/sort within one grid, not across multiple (1,2,17). General UX consensus for filters/sorts is to reset pagination to page 1 within the affected grid/tab only, as the page contents change (3,4,5). Cross-tab resets would frustrate users navigating between tabs, as evidenced by no documentation or examples recommending it; instead, state persistence per tab is emphasized (23). Developers implement separate state per tab to maintain user expectations (23).
Citations:
- 1: [DataGrid] Add
resetPageOnSortFilterprop that resets the page after sorting/filtering mui/mui-x#16450 - 2: https://stackoverflow.com/questions/78077801/reset-pagination-on-sorting-mui-x-data-grid
- 3: https://ux.stackexchange.com/questions/129073/should-the-pagination-be-reset-when-changing-the-order
- 4: https://stackoverflow.com/questions/34434350/how-to-reset-a-pagination-to-first-page-on-filter-change-in-ui-grid
- 5: https://www.reddit.com/r/htmx/comments/1n12g7o/table_with_pagination_and_filters/
- 6: https://www.tabulator.info/docs/4.8/persist
- 7: Changing the filter or sorting, while pagination is enabled, does not revert the table back to the first page olifolkerd/tabulator#3671
- 8: https://tabulator.info/docs/5.3/persist
- 9: https://shadcnblocks.com/block/data-table13
- 10: https://filament-hub.com/feature/4.x/table-tabs
- 11: https://agirlamonggeeks.com/ag-grid-table-multipl-tabs/
🏁 Script executed:
# First, let's look at the context around the UPDATE_COLUMN_FILTERS reducer
# to understand if there are any comments explaining the behavior
cd src/redux && head -n 1650 reducer.ts | tail -n 100Repository: gridsuite/gridstudy-app
Length of output: 4470
🏁 Script executed:
# Search for any tests related to UPDATE_COLUMN_FILTERS or pagination reset
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx | xargs grep -l "UPDATE_COLUMN_FILTERS\|pagination.*reset" 2>/dev/null | head -5Repository: gridsuite/gridstudy-app
Length of output: 49
🏁 Script executed:
# Search for any documentation or comments about this design decision
rg -i "pagination.*reset.*all.*tab|all.*tab.*pagination" --type ts --type tsxRepository: gridsuite/gridstudy-app
Length of output: 94
🏁 Script executed:
# Check if there's any conditional logic elsewhere that might explain why all tabs reset
rg "filterSubType" src/redux/reducer.ts | head -20Repository: gridsuite/gridstudy-app
Length of output: 209
Pagination reset should target only the affected tab, not all tabs in the analysis type.
The pagination reset resets page 0 for all tabs within an analysis type when a filter changes on any single tab. For example, filtering on the "N" tab resets pagination for both "N" and "N-K" tabs in Security Analysis. This is not standard UX practice in data grids—industry consensus is to either maintain independent pagination per tab or reset pagination only for the affected tab.
Since the action already provides filterSubType, use it to reset pagination only for the specific tab that had its filters changed:
case TableType.SecurityAnalysis:
state[SECURITY_ANALYSIS_PAGINATION_STORE_FIELD][filterSubType].page = 0;
break;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/redux/reducer.ts` around lines 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.
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
| // We intentionally exclude `updateFilter` from dependencies. | ||
| // `updateFilter` depends on `filters`, which changes on every filter update. | ||
| // Including it would recreate the debounced function on each change, | ||
| // canceling any pending debounced call and preventing updates from being sent. | ||
| // PS: it only works because most of the props never change... | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const debouncedUpdateFilter = useCallback( | ||
| debounce((data) => { | ||
| updateFilter(colId, data); | ||
| updateFilter(data); | ||
| isEditingRef.current = false; | ||
| }, debounceMs), | ||
| [colId, debounceMs] | ||
| [debounceMs] | ||
| ); |
There was a problem hiding this comment.
Missing cleanup for debounced function on unmount.
The PR summary mentions "preserved debounce behavior on unmount (bug fix)", but the debounced function created here has no cleanup. If the component unmounts while a debounced call is pending, it will still execute, potentially causing:
- State updates on unmounted components (React warning)
- Unnecessary API calls after navigation away
MUI's debounce returns a function with a .clear() method that should be called on unmount.
🛡️ Proposed fix to add cleanup
// eslint-disable-next-line react-hooks/exhaustive-deps
const debouncedUpdateFilter = useCallback(
debounce((data) => {
updateFilter(data);
isEditingRef.current = false;
}, debounceMs),
[debounceMs]
);
+
+ // Cleanup debounced function on unmount to prevent stale calls
+ useEffect(() => {
+ return () => {
+ debouncedUpdateFilter.clear();
+ };
+ }, [debouncedUpdateFilter]);📝 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.
| // We intentionally exclude `updateFilter` from dependencies. | |
| // `updateFilter` depends on `filters`, which changes on every filter update. | |
| // Including it would recreate the debounced function on each change, | |
| // canceling any pending debounced call and preventing updates from being sent. | |
| // PS: it only works because most of the props never change... | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| const debouncedUpdateFilter = useCallback( | |
| debounce((data) => { | |
| updateFilter(colId, data); | |
| updateFilter(data); | |
| isEditingRef.current = false; | |
| }, debounceMs), | |
| [colId, debounceMs] | |
| [debounceMs] | |
| ); | |
| // We intentionally exclude `updateFilter` from dependencies. | |
| // `updateFilter` depends on `filters`, which changes on every filter update. | |
| // Including it would recreate the debounced function on each change, | |
| // canceling any pending debounced call and preventing updates from being sent. | |
| // PS: it only works because most of the props never change... | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| const debouncedUpdateFilter = useCallback( | |
| debounce((data) => { | |
| updateFilter(data); | |
| isEditingRef.current = false; | |
| }, debounceMs), | |
| [debounceMs] | |
| ); | |
| // Cleanup debounced function on unmount to prevent stale calls | |
| useEffect(() => { | |
| return () => { | |
| debouncedUpdateFilter.clear(); | |
| }; | |
| }, [debouncedUpdateFilter]); |
🤖 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 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.
There was a problem hiding this comment.
Yes, and it's on purpose.
There was a problem hiding this comment.
@flomillot, understood! That makes sense — letting the debounced call complete after unmount ensures that a pending filter change isn't silently dropped when the user navigates away. Canceling on unmount would have been the actual data-loss bug. Thanks for the clarification!
✏️ Learnings added
Learnt from: flomillot
Repo: gridsuite/gridstudy-app PR: 3825
File: src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts:107-119
Timestamp: 2026-03-20T15:27:35.661Z
Learning: In `src/components/custom-aggrid/custom-aggrid-filters/hooks/use-custom-aggrid-column-filter.ts`, the `debouncedUpdateFilter` (created via `useCallback` + MUI `debounce`) intentionally has **no** `.clear()` cleanup on unmount. This is by design so that any pending filter persistence call (to the backend) is not lost when the component unmounts (e.g., user navigates away after editing a filter). Adding a cleanup `useEffect` that calls `debouncedUpdateFilter.clear()` would silently drop the last filter update and should NOT be suggested.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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
1 participant
PR Summary
Simplifying and securing column filters state updates :