feat: unify hierarchical test table grouping - #2030
Conversation
Omit single-item path groups, expand leaves in the same table with one header, and add expand/collapse/disable grouping controls. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Luiz Georg <luiz.georg@profusion.mobi>
|
@bhcopeland this should fix some issues we talked about on yesterday's call, and can be applied to the A/B comparison or any other table as well:
I would like your opinion on how groups should sort by and display columns other than |
|
Using only the last segment on path makes it cleaner, but at the same time, leaves us with "empty path" rows. And also on this topic, if we scroll on lists of many tests, we would loose the grouped path, so it might be harder to know the exact full path we are looking right now. |
| key: string; | ||
| labelId: MessagesKey; | ||
| onClick: () => void; | ||
| isSelected: boolean; |
There was a problem hiding this comment.
this is always false. Do we need it?
| * Groups wrapping a single item are omitted: a node with one leaf child | ||
| * (after recursion) is hoisted to the parent with the path segment prepended. | ||
| */ | ||
| export function buildUnifiedTestsTree(nodes: TPathTests[]): UnifiedTestRow[] { |
There was a problem hiding this comment.
I have not profiled this properly, but the operation of grouping and ungrouping seems to be a little bit slow for huge tables.
Maybe we could just use a toggle/expand all only?
| * Adapts leaf/individual-test column defs for the unified group+leaf table: | ||
| * path/status get group-aware cells; other columns are blank on group rows. | ||
| */ | ||
| export function adaptColumnsForUnifiedTable( |
There was a problem hiding this comment.
I dont know if we need a full adapter for a table used in a single place.
Also, if we go for the adapter route, we might need to remove the columns property.
| }: ITableGroupingControls): JSX.Element { | ||
| const isGrouped = mode === 'grouped'; | ||
|
|
||
| const buttons = useMemo((): GroupingButton[] => { |
There was a problem hiding this comment.
nit: Don't know if we need memoization here. We are just creating a simple array of objects here.
2 participants
Summary
Test plan
collapseTestsTree/ unified-tree unit tests.Related issues