Feat/view groups fast follow #9513
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
This PR implements significant changes to record groups functionality and fixes various UI issues, focusing on entity counting, aggregation, and visual improvements.
- Replaces
entityCountInCurrentViewComponentStatewith newrecordIndexEntityCountstates to support per-group counting and aggregation - Adds 32px height standardization for aggregate footers and improves vertical alignment of tags/counts in record groups
- Implements conditional rendering of aggregate calculations on hover and "None" option in group dropdown
- Fixes text overflow handling for long view titles with responsive max-width constraints
- Removes sticky positioning from aggregate footer and simplifies shadow effects for better visual consistency
26 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| useEffect(() => { | ||
| setOnEntityCountChange( | ||
| () => (entityCount: number) => setRecordCountInCurrentView(entityCount), | ||
| () => (entityCount: number, currentRecordGroupId?: string) => | ||
| setRecordIndexEntityCount(entityCount, currentRecordGroupId), | ||
| ); | ||
| }, [setRecordCountInCurrentView, setOnEntityCountChange]); | ||
| }, [setRecordIndexEntityCount, setOnEntityCountChange]); |
There was a problem hiding this comment.
logic: setOnEntityCountChange is called on every render due to missing memoization of the callback function. Consider using useCallback here.
| ); | ||
| }, [setRecordCountInCurrentView, setOnEntityCountChange]); | ||
| }, [setRecordIndexEntityCount, setOnEntityCountChange]); | ||
|
|
||
| const setViewFieldAggregateOperation = useRecoilCallback( |
There was a problem hiding this comment.
logic: setViewFieldAggregateOperation has empty dependency array but uses snapshot and set from useRecoilCallback. These should be added to dependencies or explicitly marked as intentionally omitted.
There was a problem hiding this comment.
logic: Removing this hook without a clear replacement could break record loading functionality. Need to verify where this logic was moved to ensure no functionality gaps.
|
|
||
| const setRecordIndexEntityCount = useRecoilCallback( | ||
| ({ set }) => | ||
| (count: number, recordGroupId?: string) => { |
There was a problem hiding this comment.
logic: count parameter should be validated to ensure it's non-negative
| set(recordIndexEntityCountByGroupFamilyState(recordGroupId), count); | ||
| } else { | ||
| set(recordIndexEntityCountNoGroupFamilyState, count); | ||
| } |
There was a problem hiding this comment.
style: consider wrapping state updates in try-catch to handle potential Recoil errors gracefully
| @@ -93,7 +96,7 @@ export const useSetRecordTableData = ({ | |||
| set(recordIndexAllRecordIdsSelector, recordIds); | |||
| } | |||
|
|
|||
| onEntityCountChange(totalCount); | |||
| onEntityCountChange(totalCount, currentRecordGroupId); | |||
There was a problem hiding this comment.
logic: onEntityCountChange is called only when currentRowIds and recordIds are different, which could miss updates when only totalCount changes
| /> | ||
| {isDefined(onClick) ? ( | ||
| <LightIconButton | ||
| testId="dropdown-menu-header-end-icon" |
There was a problem hiding this comment.
logic: testId 'dropdown-menu-header-end-icon' is misleading since this is for a start icon
| @@ -76,13 +96,22 @@ export const DropdownMenuHeader = ({ | |||
| )} | |||
| {StartIcon && ( | |||
| <StyledHeader data-testid={testId} className={className}> | |||
There was a problem hiding this comment.
logic: onClick prop is not passed to StyledHeader when StartIcon is present, making the header non-clickable even when onClick is defined
| setOnEntityCountChange( | ||
| () => (entityCount: number) => setRecordCountInCurrentView(entityCount), | ||
| () => (entityCount: number) => setRecordIndexEntityCount(entityCount), | ||
| ); |
There was a problem hiding this comment.
style: unnecessary double arrow function nesting - could be simplified to setOnEntityCountChange(setRecordIndexEntityCount)
| @@ -42,8 +42,7 @@ export const SignInBackgroundMockContainerEffect = ({ | |||
| setViewObjectMetadataId, | |||
| } = useInitViewBar(viewId); | |||
|
|
|||
| const { setRecordCountInCurrentView } = | |||
| useSetRecordCountInCurrentView(viewId); | |||
| const { setRecordIndexEntityCount } = useSetRecordIndexEntityCount(viewId); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
style: effect has multiple unrelated responsibilities - consider splitting into separate effects for metadata setup and column setup
|
Please take care of QA for this PR ! |
Fix #9512
The current weight is the same as in Figma, waiting for confirmation
This component is used in lot of places, removing the padding left can brake other places
Can't find a good way to fix it, seems more related to the fact it's running in debug mode.
Can't reproduce the issue