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 20 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: c01ae19 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 ReviewSummary: Merges
|
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
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.
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)
| 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
There was a problem hiding this comment.
Acknowledged — this is a significant design change (local state + save button) that goes beyond this PR's scope. Worth tracking as a follow-up. The immediate DB writes match the existing dashboard pattern elsewhere in the codebase.
There was a problem hiding this comment.
Good point. I classified all 15 setDashboard call sites and found one that was genuinely UI-ephemeral: activeTabId (which tab this viewer is looking at). Moved it to URL state in dc153b9, matching the existing per-viewer pattern already in place for collapse state.
- URL param
activeTabsstorescontainerId:tabIdentries per viewer container.activeTabIdremains as the DB-stored default for new viewers (and gets updated when tabs are added/deleted so the new/first tab is selected by default)- Resolution order: URL override → DB default → first tab
The other setDashboard calls are structural (tile/container/tab CRUD, filters, layout, alerts) and should persist immediately — they match the existing pattern across DBDashboardPage (e.g. RGL layout changes, filter edits, tile saves have always been immediate writes). Moving those to a local-state + save-button flow is a much larger change (navigation warnings, filter/query coupling since filters drive live queries, RGL drag batching, multi-user concurrency). Happy to open a follow-up ticket if you want to pursue that separately — this PR isn't the right scope.
| 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
There was a problem hiding this comment.
Open to suggestions on naming. The render in DBDashboardPage now uses DashboardGroupItem as the wrapper component. GroupContainer is the reusable UI component. Happy to rename if there's a preferred convention.
There was a problem hiding this comment.
Renamed GroupContainer → DashboardContainer in 9c39efa. DashboardContainer matches the schema type name in common-utils/types.ts, so the component name aligns with the data model. The schema type is aliased as DashboardContainerSchema locally where both are needed in the same file (component file + DBDashboardPage). The wrapper previously named DashboardGroupItem is now DashboardContainerRow for consistency.
Note: we discussed the future variable-repeat feature (repeat as tabs vs. sections linked to a filter) and this name works cleanly for both static and derived containers — Group / Tabs / Sections are rendering modes, DashboardContainer is the thing that has them.
…naming - Replace raw divs/spans with Mantine Box, Flex, Center, TextInput components - Extract GroupTabBar sub-component from GroupContainer (~180 lines) - Extract DashboardGroupItem to eliminate double-nested inline rendering - Pre-compute alertingTabIdsByContainer as useMemo outside render loop - Rename state: editing→isRenamingGroup, editedTitle→groupRenameValue, etc. - Merge 1-tab and 2+-tab cases in handleAddTab - Remove default params from calculateNextTilePosition (callers pass explicit) - Use semantic token --color-outline-focus for tile selection outline - Remove unnecessary hasContainers ternary in render - Add comment explaining tab/title sync semantics
knudtty
left a comment
knudtty
left a comment
There was a problem hiding this comment.
Just this and fixing the delete behavior
| // Extracted component for rendering a single dashboard group/container. | ||
| // Eliminates the double-nested inline function rendering pattern. |
There was a problem hiding this comment.
Comments like these aren't necessary
There was a problem hiding this comment.
Audited all PR-added comments and removed the WHAT-style ones in 08593ae (DashboardGroupItem banner, 'Update container assignment', section banners like '--- Tab management ---', 'Legacy container with no tabs: create 2 tabs', 'Move tiles to first remaining tab', etc.). Kept comments that explain WHY / non-obvious invariants (tab/title sync semantics, URL-precedence-over-DB rationale, default tile size reasoning).
| // Whether the group can be collapsed (default true) | ||
| collapsible: z.boolean().optional(), | ||
| // Whether to show a border around the group (default true) | ||
| bordered: z.boolean().optional(), | ||
| // Optional tabs: 2+ entries → tab bar renders, 0-1 → plain group header. | ||
| // Tiles reference a specific tab via tabId. | ||
| tabs: z.array(DashboardContainerTabSchema).optional(), | ||
| activeTabId: z.string().optional(), |
There was a problem hiding this comment.
Has this been tested in a non-preview environment? I worry if there are migrations needed and if the mongoose model needs to be updated.
There was a problem hiding this comment.
No migration needed — the Mongoose model stores containers as Schema.Types.Array (schemaless), so MongoDB accepts any shape. The only validation is the Zod schema on the frontend, which handles the upgrade cleanly:
- Old
type: 'section'field → silently stripped byz.object()(default behavior) - New fields (
tabs,collapsible,bordered,activeTabId,tabId) → all.optional(), default toundefined
Added functional tests that exercise the upgrade path — start with a realistic old-format dashboard (no tabs, no tabId, no collapsible/bordered) and run all hook operations on it:
useDashboardContainers.test.tsx — "legacy dashboard upgrade path"
Existing Zod-level test also covers backward compat:
dashboardSections.test.tsx:273 — "old dashboards with type field still parse"
Addresses PR feedback: tab deletion now shows a modal with two options instead of silently merging tiles. Also removes unnecessary comment and adds hook-level tests covering delete/move actions and the legacy dashboard upgrade path.
When selecting tiles into a new group empties a source container, the now-empty container is removed. Unrelated empty containers are preserved. Addresses review feedback on useTileSelection.ts:53.
Per review feedback, GroupContainer is too generic (resembles Mantine's Group/Container). DashboardContainer matches the schema type in common-utils/types.ts, making the component name align with the data model. The schema type is aliased locally as DashboardContainerSchema where both are needed in the same file. The wrapper component previously named DashboardGroupItem is renamed to DashboardContainerRow for consistency.
Tab selection is now per-viewer rather than persisted to the dashboard.
Matches the existing URL-based pattern for per-viewer collapse state.
- Adds `activeTabs` URL param (`containerId:tabId,...` entries)
- Removes `handleTabChange` from useDashboardContainers (DB write); the
page-level handler now updates URL state only
- Resolution order: URL override → container.activeTabId (DB default) → first tab
- Stale URL entries are safely ignored when a tabId no longer exists
Addresses review feedback on DBDashboardPage.tsx:1604 ("setDashboard is a
db call") for the one call site that was genuinely UI-ephemeral.
…fallback After the URL migration for activeTabId, clicking 'Add Tab' could leave the local viewer on their old URL-selected tab because the URL override wins over the new DB default. Now we clear the URL entry for that container on Add Tab so the viewer is taken to the newly created tab. Also hardened getActiveTabId to validate DB-stored activeTabId against the actual tabs array before using it (pre-existing issue carried over from the old resolver).
Matches the tab-delete pattern: deleting a group now opens a modal letting the user choose whether to preserve tiles (ungroup to top level) or delete them along with the group. Uniform across plain groups, legacy sections, and multi-tab groups — the filter operates on container.id and handles all tiles regardless of their tab assignment. - Ungroup Tiles (primary, only offered when tileCount > 0): strips containerId + tabId, shifts tiles below any existing ungrouped tiles - Delete Group & Tiles (danger): removes tiles belonging to this container - Cancel: no-op Removed the `confirm` prop from useDashboardContainers since the hook no longer prompts; callers render their own modal.
|
@claude - review code |
|
Claude finished @alex-fedotyev's task in 3m 43s —— View job Code Review
All 3 issues from the previous review round are resolved. Two bugs from rounds 1–2 remain unaddressed, plus one new finding. Previously flagged issues: all resolved ✅
Remaining issues1. Spurious
|
1. Spurious setDashboard on every tile drag in multi-tab containers (DBDashboardPage.tsx). The layout-change handler was built with all container tiles across every tab, but RGL only renders visible-tab tiles. newLayout.length != currentLayout.length was always true for multi-tab containers, triggering a DB write on every drag event even when nothing had moved. Fix: build the handler from visibleTiles so both layouts align. 2. Remove unused handleToggleCollapsed from useDashboardContainers hook (plus its test). The hook exposed a toggle-DB-collapsed handler that was discarded at the only call site as _handleToggleCollapsedDB — the page-level handleToggleDefaultCollapsed covers the same DB toggle. Dead API surface, now removed. 3. Document the ':' separator invariant in the activeTabs URL param. Safe today because makeId() is base-36, but comment flags the dependency in case the ID source ever changes.
Previously the container-level alert dot was guarded by `isCollapsed`, which meant plain (single-tab or legacy) containers in their expanded state had no group-header indicator — users had to scan tiles to spot alerts, which fails when tiles are below the fold. Now the dot shows whenever the plain-header render path is taken and the container has alerting tiles (both collapsed and expanded). The multi-tab expanded path is unaffected since it renders the tab bar (with per-tab dots) instead of the plain header.
3 participants
Summary
Merges the separate "section" and "group" container types into a single DashboardContainer concept with configurable options:
AlertState.ALERT, so an alerting group is visible at a glance even when the alerting tile is below the foldThis 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):
Overflow menu (Add Tab, Collapse, Border, Delete):
Collapsed with pipe-separated tab names (Overview | Latency | Errors):
Commits (review guide)
types.tstabs,collapsible,bordered,activeTabId,tabId);typediscriminator removedDashboardDndContext.tsx,DashboardDndComponents.tsx: sortable wrappers, drag handleDashboardContainerDashboardContainer.tsx+GroupTabBar.tsx: unified component with tabs, collapse, borders, alerts, a11yuseDashboardContainers.tsx: container/tab CRUD.useTileSelection.ts: multi-select + Cmd+G (auto-deletes emptied source containers)DBDashboardPage.tsx: wiring, alert computation, DnD, tile positioning, URL-based active-tab + collapse stateDashboardContainer.test.tsx,useDashboardContainers.test.tsx,useTileSelection.test.tsx,dashboardSections.test.tsxKey decisions
typediscriminator — containers are defined by properties. Extensible via additional fields later (e.g., planned variable-repeat feature could addrepeat: { filterId; as: 'tabs' \| 'sections' }cleanly onto the same container).AlertState.ALERTtiles. Also shown at the group-header level when no tab bar is rendered (plain/collapsed), so a single alerting tile in a large dashboard isn't hidden.Backward compatibility
type: 'section'containers parse successfully (Zod strips extra field)collapsible,bordered,tabs,activeTabId,tabId) —undefinedtreated as defaultscontainersfield is a schemalessSchema.Types.Array; all validation happens at the Zod layeruseDashboardContainers.test.tsx"legacy dashboard upgrade path" anddashboardSections.test.tsx:273"old dashboards with type field still parse"Review feedback addressed
stylepropsGroupTabBarextracted from the main component;DashboardContainerRowextracted from inline render inDBDashboardPageactiveTabId+ collapse/expand) — the only non-structuralsetDashboardcall migrated off the serverGroupContainer→DashboardContainerto match the schema typesetDashboardon every tile drag in multi-tab containers (layout handler was built with all-tab tiles but RGL only renders visible-tab tiles)handleToggleCollapsedexport from the hook):separator invariant in theactiveTabsURL paramTest plan
make ci-lint)Builds on #1972