[Aggregate queries for table views - #1] Introduce aggregateOperationForViewFieldState #9010
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Introduces a new Recoil state management system for aggregate operations (SUM, COUNT, etc.) in table views, enabling per-field aggregation functionality.
- Added
aggregateOperationForViewFieldStatefamily state in/record-table-footer/states/aggregateOperationForViewFieldState.tsto store field-specific aggregation settings - Implemented state synchronization in
RecordIndexTableContainerEffect.tsxusinguseRecoilCallbackto efficiently update aggregate operations - Modified
RecordIndexContainer.tsxto handle aggregate operation changes during view field updates - Removed
usePortalprop fromDropdowncomponent, suggesting architectural changes in dropdown rendering
5 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| import { createFamilyState } from '@/ui/utilities/state/utils/createFamilyState'; | ||
|
|
||
| export const aggregateOperationForViewFieldState = createFamilyState< | ||
| AGGREGATE_OPERATIONS | null | undefined, |
There was a problem hiding this comment.
style: Consider whether undefined is needed in the type union since defaultValue is null
| currentViewWithSavedFiltersAndSorts?.viewFields.forEach((viewField) => { | ||
| setViewFieldsAggregateOperations(viewField); | ||
| }); |
There was a problem hiding this comment.
logic: Missing null check on viewFields before forEach could cause runtime error
...-front/src/modules/object-record/record-index/components/RecordIndexTableContainerEffect.tsx
Outdated
Show resolved
Hide resolved
| const aggregateOperationForViewField = snapshot | ||
| .getLoadable( | ||
| aggregateOperationForViewFieldState({ | ||
| viewFieldId: viewField.id, | ||
| }), | ||
| ) | ||
| .getValue(); |
There was a problem hiding this comment.
style: getLoadable could throw if state is not initialized - wrap in try/catch
...ages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx
Outdated
Show resolved
Hide resolved
| if (aggregateOperationForViewField !== viewField.aggregateOperation) { | ||
| set( | ||
| aggregateOperationForViewFieldState({ | ||
| viewFieldId: viewField.id, | ||
| }), | ||
| viewField.aggregateOperation, | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: Missing null check for viewField.aggregateOperation. If undefined, this could set null state unnecessarily.
| viewField.aggregateOperation, | ||
| ); | ||
| } | ||
| }); | ||
| }, | ||
| [columnDefinitions, setTableColumns], |
There was a problem hiding this comment.
logic: Dependencies array is missing aggregateOperationForViewFieldState, which this callback uses internally.
| const aggregateOperationForViewField = snapshot | ||
| .getLoadable( | ||
| aggregateOperationForViewFieldState({ | ||
| viewFieldId: viewField.id, | ||
| }), | ||
| ) | ||
| .getValue(); |
There was a problem hiding this comment.
style: getLoadable() call could throw if state is not initialized. Consider using loadable.state === 'hasValue' check.
...ages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx
Show resolved
Hide resolved
...-front/src/modules/object-record/record-index/components/RecordIndexTableContainerEffect.tsx
Show resolved
Hide resolved
| @@ -68,5 +74,37 @@ export const RecordIndexTableContainerEffect = () => { | |||
| ); | |||
| }, [setRecordCountInCurrentView, setOnEntityCountChange]); | |||
|
|
|||
There was a problem hiding this comment.
setAggregateOperationForViewField ? Why using plural ?
There was a problem hiding this comment.
because it is setting the aggregate operation for every view fields we have
There was a problem hiding this comment.
It only sets one view field operation for the view field passed in parameters, that naming would be adequate if the function would indeed contain a loop.
| currentViewWithSavedFiltersAndSorts?.viewFields, | ||
| setViewFieldsAggregateOperations, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Or
currentViewWithSavedFiltersAndSorts?.viewFields.forEach(
setViewFieldAggregateOperation,
);
Works in that case, why not ?
There was a problem hiding this comment.
Here we can see why having the naming in singular fits with the fact that we apply a single operation on each item, it's coherent if we read it : For each view field, set aggregate operation
Introducing aggregateOperationForViewFieldState to add a state storing the aggregate operation for each view field