refactor: Unify section/group into single Group with collapsible/bordered options#2015
refactor: Unify section/group into single Group with collapsible/bordered options#2015alex-fedotyev wants to merge 9 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 09831a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Review
|
alex-fedotyev
changed the title
|
@claude code review |
|
Claude finished @alex-fedotyev's task in 2m 11s —— View job Code Review
Overall this is a clean unification with good backward-compat handling. Three issues from the previous automated review remain unaddressed, plus a new bug I found in the rename path. Issues1. Bug:
|
|
@claude code review |
|
Claude finished @alex-fedotyev's task in 3m 12s —— View job Code Review
The previous automated review flagged 4 issues. All 4 have been addressed in this revision — nice cleanup. Findings below. Previously-flagged issues: all resolved ✅
Remaining issues1. Accessibility regression (still unaddressed from original automated review)
// current
<IconChevronRight
onClick={onToggle}
/>
// fix: wrap or add attributes
<IconChevronRight
role="button"
tabIndex={0}
aria-expanded={!isCollapsed}
? 'Expand group' : 'Collapse group'}
onKeyDown={e => (e.key === 'Enter' || e.key === ' ') && onToggle()}
onClick={onToggle}
...
/>2. Keyboard-focusable invisible controls
// add tabIndex alongside the existing opacity/pointerEvents
style={{ opacity: showControls ? 1 : 0, pointerEvents: showControls ? 'auto' : 'none' }}
// + tabIndex={showControls ? 0 : -1}3. Stale URL collapse state when re-enabling collapsible (low severity)
Minor observationsStale
Shared active tab state ( What looks good
|
|
@claude - review code |
|
Claude finished @alex-fedotyev's task in 2m 40s —— View job Code Review
All issues from the previous two review rounds are resolved. Three new findings below. Previously flagged issues: all resolved ✅
New issues1. Tab rename button (pencil icon) still keyboard-focusable when hidden
// current — stays in tab order when invisible
<ActionIcon
variant="subtle"
size="sm"
style={{ opacity: showControls ? 1 : 0, pointerEvents: showControls ? 'auto' : 'none' }}
...
>
// fix — one line addition
tabIndex={showControls ? 0 : -1}2. Spurious alert dot when container has no tabs
In practice this only occurs for containers created before the tab system (legacy containers), but the empty-string sentinel is a code smell. Filtering it out explicitly is cleaner: // current
alertingTabIds.add(tile.tabId ?? firstTabId ?? '');
// fix — only add a real id; skip if there's nothing to attribute to
const attributedTabId = tile.tabId ?? firstTabId;
if (attributedTabId) alertingTabIds.add(attributedTabId);3.
|
alex-fedotyev
force-pushed
the
feat/unified-group
branch
from
027b142 to
d3cf78a
Compare
Add optional fields to DashboardContainerSchema:
- tabs: array of {id, title} for tabbed groups (tab bar at 2+)
- activeTabId: persisted active tab selection
- collapsible: whether the group can be collapsed (default true)
- bordered: whether to show a border (default true)
Remove the dead `type` discriminator field ('section' | 'group') —
containers are now defined by their properties. Old dashboards with
`type: 'section'` still parse (Zod strips unknown keys). The field
can be re-added as a discriminated union if semantically different
container kinds are ever needed.
Also add `tabId` to TileSchema so tiles can reference a specific tab.
- DashboardDndContext: wraps dashboard in DndContext + SortableContext for container drag-and-drop reordering - DashboardDndComponents: SortableContainerWrapper (sortable item with drag handle props), EmptyContainerPlaceholder (shown when a container or tab has no tiles, with an Add button) - DragHandleProps type exported for GroupContainer integration
Unified component for all dashboard container types. Features: - Collapse chevron with keyboard/screen reader support (role, tabIndex, aria-expanded, aria-label, Enter/Space handler) - Optional border toggle - Tab bar (appears at 2+ tabs) with inline rename, delete confirmation - Collapsed state shows pipe-separated tab names (max 4, then "...") - Alert indicators: red dot on tabs/header for active alerts - Overflow menu: Add Tab, Collapse by Default, Disable/Enable Collapse, Hide/Show Border, Delete Group (divider guarded) - Hidden controls removed from keyboard tab order (tabIndex toggle) - Fixed header height prevents UI jump on collapse/expand Deletes SectionHeader.tsx and its tests — all functionality merged into GroupContainer.
useDashboardContainers (301 lines): Container CRUD + tab lifecycle - Add/rename/delete containers - Toggle collapsed, collapsible, bordered - Add/rename/delete tabs with tile migration - Tab change persistence - Container.title synced with tabs[0] on rename and delete useTileSelection (74 lines): Multi-select + Cmd+G grouping - Shift+click tile selection - Cmd+G groups selected tiles into a new container - Assigns tabId to newly grouped tiles
Wire GroupContainer, useDashboardContainers, useTileSelection, and DnD context into DBDashboardPage: - Add @dnd-kit dependencies (core, sortable, utilities) - Single "New Group" in Add menu (replaces Section + Group) - Container rendering with collapsible/bordered/tabbed support - Tile move dropdown with IconCornerDownRight and tab targets - URL-based collapse state (cleared on collapsible toggle) - Alert indicator computation: alertingTabIds per container from tile alert state, passed to GroupContainer - Auto-expand on collapsible disable (prevents stuck-collapsed) - Tile positioning updated for container-aware layout
GroupContainer.test.tsx (329 lines, 18 tests): - Collapsible: chevron show/hide, children toggle, onToggle callback - Bordered: border style present/absent - Collapsed tab summary: pipe-separated names, no summary when expanded or single-tab - Tab bar: renders at 2+ tabs, plain header at 1 tab - Tab delete: confirmation flow, rejected confirmation - Alert indicators: dot on collapsed header, per-tab dot in expanded bar dashboardSections.test.tsx (556 lines, 56 tests): - Schema validation: containers without type field, backward compat with extra fields, tabs, collapsible/bordered options - Tile grouping logic, tab filtering, container authoring operations - Group tab operations: creation, add/delete tabs, rename sync
alex-fedotyev
force-pushed
the
feat/unified-group
branch
from
d3cf78a to
c244076
Compare
alex-fedotyev
changed the base branch from
feat/dashboard-dnd-organization
to
main
github-actions
bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
E2E Test Results✅ All tests passed • 130 passed • 3 skipped • 1029s
Tests ran across 4 shards in parallel. |
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
It's looking good @alex-fedotyev! From a design perspective let's try to stick to semantic tokens.
| background: 'var(--mantine-color-body)', | ||
| border: '1px solid var(--mantine-color-default-border)', |
There was a problem hiding this comment.
We should use semantic tokens instead of mantine css tokens
| background: 'var(--mantine-color-body)', | |
| border: '1px solid var(--mantine-color-default-border)', | |
| background: 'var(--color-bg-body)', | |
| border: '1px solid var(--color-border)', |
There was a problem hiding this comment.
Fixed — switched all Mantine tokens to semantic tokens and removed the dashed border from empty placeholder.
| border: bordered | ||
| ? '1px solid var(--mantine-color-default-border)' | ||
| : undefined, |
There was a problem hiding this comment.
| border: bordered | |
| ? '1px solid var(--mantine-color-default-border)' | |
| : undefined, | |
| border: bordered ? '1px solid var(--color-border)' : undefined, |
| px="sm" | ||
| gap={6} | ||
| style={{ | ||
| borderBottom: '1px solid var(--mantine-color-default-border)', |
There was a problem hiding this comment.
| borderBottom: '1px solid var(--mantine-color-default-border)', | |
| borderBottom: '1px solid var(--color-border)', |
| width: size, | ||
| height: size, | ||
| borderRadius: '50%', | ||
| backgroundColor: 'var(--mantine-color-red-filled)', |
There was a problem hiding this comment.
| backgroundColor: 'var(--mantine-color-red-filled)', | |
| backgroundColor: 'var(--color-bg-danger)', |
| border: isEmpty | ||
| ? '2px dashed var(--mantine-color-default-border)' | ||
| : undefined, |
There was a problem hiding this comment.
I don't think we need this dashed border:
In case you want to keep it, it should be 1px solid var(--color-border). Also we should avoid a border inside another border.
There was a problem hiding this comment.
Agreed — removed the dashed border entirely. Empty placeholders now just show the Add button without a border-inside-border.
Replace mantine-specific CSS variables with semantic design tokens: - var(--mantine-color-body) → var(--color-bg-body) - var(--mantine-color-default-border) → var(--color-border) - var(--mantine-color-red-filled) → var(--color-bg-danger) Addresses review comments from elizabetdev on PR #2015.
- --mantine-color-dimmed → --color-text-muted (chevron, drag handle) - --mantine-color-default-border → --color-border (plain header) - Remove dashed border from empty container placeholder (avoids border-inside-border per review feedback)
| <div | ||
| data-testid={`container-placeholder-${containerId}`} | ||
| style={{ | ||
| minHeight: isEmpty ? 80 : undefined, | ||
| borderRadius: 4, | ||
| border: isEmpty | ||
| ? '2px dashed var(--mantine-color-default-border)' | ||
| : undefined, | ||
| position: 'relative', | ||
| }} | ||
| > |
There was a problem hiding this comment.
I'd prefer if we stuck to mantine components, and only reached for others if there's no other easy way. There must be some mantine container that accomplishes this
| style={{ | ||
| position: 'absolute', | ||
| inset: 0, | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| padding: '0 16px', | ||
| }} |
There was a problem hiding this comment.
style props should be a last resort, we shouldn't use them as a first option. For this one, you could probably avoid style altogether by use the Center component with its props
| confirm, | ||
| alertingTabIds, | ||
| }: GroupContainerProps) { | ||
| const [editing, setEditing] = useState(false); |
There was a problem hiding this comment.
Names here are not clear, I think this is for renaming a group?
| const [editing, setEditing] = useState(false); | ||
| const [editedTitle, setEditedTitle] = useState(container.title); | ||
| const [hovered, setHovered] = useState(false); | ||
| const [editingTabId, setEditingTabId] = useState<string | null>(null); | ||
| const [editedTabTitle, setEditedTabTitle] = useState(''); | ||
| const [hoveredTabId, setHoveredTabId] = useState<string | null>(null); | ||
| const [menuOpen, setMenuOpen] = useState(false); | ||
|
|
||
| const tabs = container.tabs ?? []; | ||
| const hasTabs = tabs.length >= 2; | ||
| const collapsible = container.collapsible !== false; | ||
| const bordered = container.bordered !== false; | ||
| const showControls = hovered || menuOpen; | ||
| const resolvedActiveTabId = activeTabId ?? tabs[0]?.id; | ||
| const isCollapsed = collapsible && collapsed; | ||
|
|
||
| const firstTab = tabs[0]; | ||
| const headerTitle = firstTab?.title ?? container.title; |
There was a problem hiding this comment.
There's a lot of state tracked here, it probably needs to be broken into smaller purposeful components with a callback prop for when that action is performed.
| onClick={e => e.stopPropagation()} | ||
| style={{ display: 'inline' }} | ||
| > | ||
| <input |
| handleRenameTab, | ||
| handleDeleteTab, | ||
| handleTabChange, | ||
| } = useDashboardContainers({ dashboard, setDashboard, confirm }); |
There was a problem hiding this comment.
I just realized everywhere we see setDashboard, that's actually a db call. I don't think we should be doing this everywhere, seems like we should be modifying some local state and then updating after clicking "save" or something
| containerTitle={container.title} | ||
| > | ||
| {(dragHandleProps: DragHandleProps) => ( | ||
| <GroupContainer |
There was a problem hiding this comment.
Can we have a better name? It sounds like a generic framework component, like how mantine has both Group and Container
| <GroupContainer | ||
| container={container} | ||
| collapsed={containerCollapsed} | ||
| defaultCollapsed={container.collapsed ?? false} | ||
| onToggle={() => handleToggleCollapse(container.id)} | ||
| onToggleDefaultCollapsed={() => | ||
| handleToggleDefaultCollapsed(container.id) | ||
| } | ||
| onToggleCollapsible={() => | ||
| handleToggleCollapsible(container.id) | ||
| } | ||
| onToggleBordered={() => | ||
| handleToggleBordered(container.id) | ||
| } | ||
| onDelete={() => handleDeleteContainer(container.id)} | ||
| onAddTile={() => | ||
| onAddTile( | ||
| container.id, | ||
| hasTabs ? groupActiveTabId : undefined, | ||
| ) | ||
| } | ||
| activeTabId={groupActiveTabId} | ||
| onTabChange={tabId => | ||
| handleTabChange(container.id, tabId) | ||
| } | ||
| onAddTab={() => handleAddTab(container.id)} | ||
| onRenameTab={(tabId, newTitle) => | ||
| handleRenameTab(container.id, tabId, newTitle) | ||
| } | ||
| onDeleteTab={tabId => | ||
| handleDeleteTab(container.id, tabId) | ||
| } | ||
| onRename={newTitle => | ||
| handleRenameContainer(container.id, newTitle) | ||
| } | ||
| dragHandleProps={dragHandleProps} | ||
| confirm={confirm} | ||
| alertingTabIds={alertingTabIds} | ||
| > |
There was a problem hiding this comment.
We generally like to follow a principle of components themselves being smart. I feel this violates that, as we have to pass in a ton of callbacks instead of just a few shared contextual props and let the component do these operations itself
| > | ||
| {sectionTiles.map(renderTileComponent)} | ||
| </ReactGridLayout> | ||
| {(currentTabId: string | undefined) => { |
There was a problem hiding this comment.
This whole double-nested-inlined-function-rendering concept is strange. Maybe its unavoidable due to the new dnd plumbing, but if we can avoid it we should. React or the react compiler may have a hard time optimizing this stuff
| containers={containers} | ||
| onReorderContainers={handleReorderContainers} | ||
| > | ||
| {hasContainers ? ( |
There was a problem hiding this comment.
I think this whole conditional is unnecessary. The ungroupedTiles will naturally only render is ungroupedTiles.length > 0 thanks to the other conditional below this
3 participants
Summary
Merges the separate "section" and "group" container types into a single Group concept with configurable options:
This addresses the UX concern from #1972 that "section" and "group" are near-synonyms that force users to choose between two similar concepts.
Screenshots
Dashboard with groups and tabs (expanded):
Group overflow menu (Add Tab, Collapse, Border, Delete):
Collapsed group with pipe-separated tab names (Overview | Latency | Errors):
Commits (review guide)
types.ts: new optional fields (tabs, collapsible, bordered), type field removalDashboardDndContext.tsx,DashboardDndComponents.tsx: sortable wrappers, drag handleGroupContainer.tsx: unified component with tabs, collapse, borders, alerts, a11yuseDashboardContainers.tsx: container/tab CRUD.useTileSelection.ts: multi-select + Cmd+GDBDashboardPage.tsx: wiring, alert computation, DnD, tile positioningGroupContainer.test.tsx(18 tests),dashboardSections.test.tsx(56 tests)Key decisions
typediscriminator — containers defined by properties. Extensible via discriminated union later.AlertState.ALERTtiles. Shows on both expanded tab bar and collapsed header.Backward compatibility
type: 'section'containers parse successfully (Zod strips extra field)collapsible/borderedfields are optional —undefinedtreated astrueBuilds on #1972
Test plan