test(e2e): cover unified DashboardContainer (collapse, border, tabs, drag)

[alex-fedotyev]

Summary

Follow-up E2E coverage for PR #2015 (the unified DashboardContainer
that replaced the legacy section/group concept). Drew explicitly asked
for this in his top-level review on #2015 ("Can you confirm we have
followup issues/tickets covering... 2. New E2E tests covering the new
functionality"). Unit coverage landed in #2015 itself; this PR adds
the missing browser-level coverage.

The container UX has four moving pieces, all exercised here:
collapsible (chevron + URL state), bordered (overflow-menu toggle +
inline border style), tabs (tab bar appearance, tab switching, URL
state), and drag-to-reorder via @dnd-kit. Each test step cites the
source line that defines the behavior so a reviewer can double-check
the assertion matches the implementation.

Test cases

1. Group renders with default collapsible chevron and bordered style;
   chevron toggles aria-expanded.
2. Toggling Hide Border / Show Border via the overflow menu flips the
   inline border style and the menu label.
3. Adding a tab brings the tab bar (1-tab groups don't render it),
   switching tabs updates ?activeTabs and aria-selected.
4. ?collapsed and ?expanded URL params survive reload and restore
   per-viewer state.
5. ?activeTabs URL param survives reload and restores the active
   tab.
6. Save-and-reload round-trip preserves the containers list and the
   second tab on group B.
7. Drag-to-reorder: drag-onto-self is a no-op (the DnD guard at
   DashboardDndContext.tsx:67-70); dragging A onto C in [A, B, C]
   yields [B, C, A] (arrayMove semantics) and the new order
   persists across navigation.

What changed since the first push

• Tests feat: introduce usage-stats service + init changeset #3 and chore: bump @hyperdx/node-logger + @hyperdx/node-opentelemetry pkgs #5 wrap getActiveTabsParam()[id] reads in
  expect.poll(...) because nuqs flushes URL state asynchronously
  after Add Tab and tab switches. This is the same race pattern
  the heatmap drag-select e2e (PR test(e2e): cover heatmap drag-select persistence (HDX-4147) #2209) hit.
• Test Update README.md spelling #6 was narrowed to a single mutation per step (addTabToGroup
  on group B) and now waits for networkidle before navigating away.
  The earlier version paired a toggleGroupBordered(idA) with
  addTabToGroup(idB) back-to-back; the resulting concurrent
  setDashboard calls clobber each other because the second
  produce reads the same pre-mutation snapshot. That underlying
  race is tracked in Dashboard: setDashboard mutations clobber each other when fired back-to-back #2216. Bordered round-trip is still covered in
  isolation by test Generic Webhook Option for Alerts #2.

Side fixes pulled in

• DashboardPage.ts page-object selector was still pointing at the
  stale add-new-section-menu-item testid; PR refactor: Unify section/group into single Group with collapsible/bordered options #2015 renamed it to
  add-new-group-menu-item. No existing spec exercised addSection,
  so this hadn't surfaced. Renamed the locator and the helper from
  addSection -> addGroup.
• DashboardContainer.tsx adds data-testid="group-add-tab-${id}"
  on the existing Add Tab menu item so the spec doesn't have to
  match Mantine menu text. One-line non-behavior change.

Tier

Predicted Tier 2 by the local classifier: 1 production file,
10 production lines (the testid addition), no API/router/model
touch, single-layer (packages/app/). Test files are excluded from
the tier calculation per the classifier rules.

Out of scope

• Concurrent-mutation back-pressure on setDashboard (PATCH clobber).
  Tracked in Dashboard: setDashboard mutations clobber each other when fired back-to-back #2216.
• Multi-user URL state isolation (covered by
  dashboardSections.test.tsx unit tests).
• Alert-dot indicators (covered by DashboardContainer.test.tsx).
• Legacy type: "section" migration (one-time data shape; covered
  by dashboardSections.test.tsx).
• Tile drag-reorder (uses react-grid-layout, not the new
  @dnd-kit container DnD).

Test plan

• yarn lint clean.
• yarn tsc --noEmit clean.
• npx playwright test --list tests/e2e/features/dashboard-container.spec.ts lists all 7 cases.
• prose-lint clean against origin/main.
• Local tier prediction: Tier 2.
• CI: full E2E shard run on this branch.

Refs PR #2015. Follow-up race tracked in #2216.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6f66508

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	May 8, 2026 0:32am

[github-actions]

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

• Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats

• Production files changed: 1
• Production lines changed: 11 (+ 601 in test files, excluded from tier calculation)
• Branch: alex/e2e-dashboard-container
• Author: alex-fedotyev

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

PR Review

✅ No critical issues found.

Minor observations (non-blocking):

• ℹ️ Spec file is 342 lines, slightly above AGENTS.md's 300-line guideline (though that rule targets components — test files are routinely longer and this is well-organized into discrete test.step blocks).
• ℹ️ getActiveTabsParam carries dual parse paths (double-decoded + plain JSON.parse fallback) for forward/back compat with the old single-encoded format — comment explains why, fine to keep.
• ℹ️ Two [prose-lint: allow] annotations are added to preserve em-dashes in existing JSDoc — small surface but worth confirming the prose-lint config actually honors that marker (vs. a different disable-line syntax).
• ℹ️ dragGroupTo recomputes the target box after activation (good — @dnd-kit shifts siblings during drag); pattern is consistent with existing SearchPage.ts brush helper, so no concerns.

The PR clearly addresses the follow-up requested on #2015, defers the setDashboard clobber race to the existing tracking issue (#2216), and the new data-bordered attribute is a clean test seam.

[github-actions]

E2E Test Results

✅ All tests passed • 171 passed • 3 skipped • 1225s

Status	Count
✅ Passed	171
❌ Failed	0
⚠️ Flaky	5
⏭️ Skipped	3

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

Compound Engineering Review

PR #2205 — test(e2e): cover unified DashboardContainer (collapse, border, tabs, drag)

Scope: 1 production file (testid + data-bordered attr only), 1 new E2E spec, 1 page-object refactor (addSection → addGroup rename + ~250 lines of helpers). No security, performance, or migration surface — review focused on race conditions, type discipline, simplicity, and pattern consistency.

P1 — Should fix before merge

• packages/app/tests/e2e/page-objects/DashboardPage.ts:298-306 — waitForDashboardPatch predicate matches any PATCH against /api/dashboards/. In the round-trip and drag-reorder tests, the listener is registered after 2–3 prior addGroupAndGetId calls that each emit fire-and-forget PATCHes. A still-in-flight prior PATCH can resolve the listener instead of the targeted one, then the spec navigates while the targeted PATCH is still in flight → the change being tested is dropped. Fix: register listeners before the prior addGroupAndGetId calls (or Promise.all count them), OR tighten the predicate on request body / dashboard id.
• packages/app/tests/e2e/page-objects/DashboardPage.ts:432-468 — dragGroupTo recomputes target.boundingBox() on the inner [data-testid="group-container-${id}"] Box, but @dnd-kit's 250ms translate3d transform is applied to the parent SortableContainerWrapper (DashboardDndComponents.tsx:69-78). The recompute returns the mid-animation projected rect, so on fast machines the pointer can land on a neighbour. Fix: add a testid to the wrapper and sample from there, or wait for the dnd-kit transition to settle before sampling.
• packages/app/tests/e2e/features/dashboard-container.spec.ts:124-132, 204-220 — secondTabId is captured via a synchronous second read of getActiveTabsParam()[id] immediately after an expect.poll. The poll observes the value but the capture races any subsequent nuqs flush. Currently safe by accident; one new mutation step turns this into a stale-id bug with a horrendous diff. Fix: capture the id inside the poll callback.
• packages/app/src/components/DashboardContainer.tsx:38, 283 — // [prose-lint: allow] markers consume nothing — grep -rn "prose-lint" finds zero hits outside this file (no plugin, no Vale config, no CI step). They're inert pragmas masquerading as load-bearing suppressions. Fix: delete the markers and replace the em-dashes with commas (DashboardPage.ts:871,885 already does exactly this in the same PR), or relax the prose-lint rule for source files at the config level.
• packages/app/tests/e2e/page-objects/DashboardPage.ts:343-380 — getActiveTabsParam cargo-cults the production parseAsJsonEncoded parser: double-tryParse, raw fallback, runtime non-array-object validation, per-key string filter. The test owns both ends of this contract; the second tryParse(raw) fallback (line 358) is unreachable because decodeURIComponent on valid nuqs output never throws. Fix: collapse to try { return JSON.parse(decodeURIComponent(raw)) } catch { return {} } (~30 LOC saved).
• packages/app/src/components/DashboardContainer.tsx:244 — data-bordered={bordered ? 'true' : 'false'} always-rendered convention diverges from the established repo pattern (SQLInlineEditor.tsx:305, AutocompleteInput.tsx:144) which uses data-expanded={x ? 'true' : undefined} (presence/absence). Pick one convention. Also: this attr is purely test-only — consider data-test-bordered to signal intent and prevent accidental coupling.
• packages/app/tests/e2e/features/dashboard-container.spec.ts (25 step labels) — All 25 new test.step('lowercase…', …) labels break the repo's Sentence-case convention (469 prior Sentence-case steps in features/, only 2 lowercase outliers, both in dashboard.spec.ts). Fix: capitalize the first letter of each step description.

P2 — Should fix

• packages/app/tests/e2e/features/dashboard-container.spec.ts:171-177, 186-192 — Reload paths read getCollapsedParam() synchronously after waitForLoaded(), which only checks the page-shell testid. Currently safe because the param was written before reload, but if nuqs ever does a mount-time URL reconciliation pass this becomes a flake. Wrap reads in expect.poll for symmetry with the activeTabs reload assertion at line 232.
• packages/app/tests/e2e/page-objects/DashboardPage.ts (lines 95-114, 498, 502, 518, 539, 558, 571) — Locator-style mix: 32 getByTestId vs 3 page.locator('[data-testid="…"]'). New code uses the modern style; pre-existing constructor lines are the inconsistent half. Worth a follow-up cleanup PR while the file is in mind.
• packages/app/tests/e2e/page-objects/DashboardPage.ts:240-249 — getGroupOrder filters empty ids with a 3-line justifying comment, but [data-testid^="group-container-"] already guarantees the prefix and container.id is always non-empty. Defending an unreachable state. Drop the filter.

P3 — Nice to have

• packages/app/tests/e2e/page-objects/DashboardPage.ts:421-422 — Double mouse.move(startX, startY) to identical coords does not "stabilise pointerdown coalescing" — same-coord moves dispatch no event. Magic incantation; remove one or move from a different start.
• Comment noise — Multiple new helpers (getGroupChevron, getGroupBorderedToggle, getGroupTabs, toggleGroupBordered, addTabToGroup) have JSDoc that just restates the function name. Either drop or add the WHY.
• getGroupBorderedAttr (DashboardPage.ts:287-289) — Returns Promise<string|null> from a non-async function while sibling helpers are async. Pick one convention across the new block.

Files reviewed: packages/app/src/components/DashboardContainer.tsx, packages/app/tests/e2e/features/dashboard-container.spec.ts, packages/app/tests/e2e/page-objects/DashboardPage.ts (with DashboardDndContext.tsx, DashboardDndComponents.tsx, queryParsers.ts, useDashboardContainers.tsx consulted for context).

[alex-fedotyev]

P1 flake risks and P2 cleanups addressed in 7796c3a:

• PATCH wait via waitForResponse(...PATCH), registered before the mutation
• dashboardPage.getCurrentDashboardId() (existing helper) instead of inline regex
• @full-stack tag on the two persistence tests
• getActiveTabsParam validates JSON.parse shape and value type
• openGroupMenu waits for pointer-events: none to clear
• dragGroupTo recomputes target box after activation, activation nudge shrunk to 10px
• ?collapsed/?expanded reads polled as a tuple
• New page-object getters: getGroupChevron, getGroupBorderedToggle, getGroupTabs, getGroupBorderedAttr, waitForLoaded, waitForDashboardPatch
• data-bordered attribute on the container shell, asserted via attr instead of inline style.border
• Reverted unrelated em-dash to comma comment edits in DashboardContainer.tsx (em-dash sweep belongs in its own PR)

[github-actions]

Deep Review

Scope: PR #2205 — E2E coverage for the unified DashboardContainer (collapsible/bordered/tabbed/drag-to-reorder), follow-up to PR #2015. Three files: DashboardContainer.tsx (+8/-3, adds data-testid="group-add-tab-${id}" and data-bordered), a new 342-line dashboard-container.spec.ts, and DashboardPage.ts page-object additions (+253/-9).

🔴 P0/P1 -- must fix

• packages/app/tests/e2e/page-objects/DashboardPage.ts:298 -- waitForDashboardPatch() is registered after 2–3 fire-and-forget addGroupAndGetId() calls in the round-trip and drag-persist tests, so the listener can resolve on an in-flight addGroup PATCH and let the spec navigate before the addTab/reorder PATCH lands.
  • Fix: Either await each addGroup's PATCH inside addGroupAndGetId before returning, or accept a discriminator (request postData containing the new tab id / reorder shape) so the helper cannot match the wrong call.
  • _{correctness, julik-frontend-races}
• packages/app/tests/e2e/page-objects/DashboardPage.ts:262 -- waitForLoaded() only awaits the data-testid="dashboard-page" shell, which mounts before the remote dashboard fetch resolves, so post-reload synchronous reads (e.g., getCollapsedParam()) can run against an unhydrated page.
  • Fix: Add a hydration signal to waitForLoaded, e.g., await expect.poll(() => this.getGroups().count()).toBeGreaterThan(0) for tests that expect groups, or wait on the absence of a loading skeleton.
  • _{julik-frontend-races}
• packages/app/src/components/DashboardContainer.tsx:38 -- Two new // [prose-lint: allow] / /* [prose-lint: allow] */ markers target a linter that does not exist in this repo (grep -rn 'prose-lint\|proselint\|vale\|alex' returns only the new sites), and a sibling change at DashboardPage.ts:872,884 rewrote em-dashes to commas instead — the convention is unstable.
  • Fix: Drop both [prose-lint: allow] markers, and either rewrite the em-dashes to a consistent character or wire up a real prose linter; do not leave decorative directives that future readers will copy.
  • _{maintainability, kieran-typescript}
• packages/app/src/components/DashboardContainer.tsx:92 -- The unified container's overflow-menu surface (rename group/tab, Delete Group with tileCount=0 vs tileCount>0 ungroup branch, "Disable Collapse", "Collapse by Default") is the riskiest part of the refactor and has zero behavioral coverage — only chevron, bordered, add-tab, and drag are exercised.
  • Fix: Add tests for: (a) rename via Text click + Enter dispatching to onRenameTab(firstTab.id) vs onRename, (b) Delete Group both branches based on tileCount, (c) Disable Collapse hides the chevron and persists across reload, (d) Collapse by Default flips the saved default.
  • _testing
• packages/app/src/components/DashboardContainer.tsx:111 -- The collapsible=false branch returns null for the chevron and is not exercised, so a regression that breaks the container.collapsible !== false default at line 83 would slip through.
  • Fix: Add a test that toggles group-toggle-collapsible-${id}, asserts getGroupChevron(id) resolves to count 0, then re-enables and asserts the chevron returns.
  • _testing
• packages/app/tests/e2e/page-objects/DashboardPage.ts:356 -- getActiveTabsParam reimplements parseAsJsonEncoded with 13 lines of duplicated explanatory prose; if packages/app/src/utils/queryParsers.ts changes its serialization rule the helper will drift silently while specs keep passing.
  • Fix: Import and use the actual parseAsJsonEncoded parser to read the URL through the same code path the app uses to write it, or shrink the helper to a one-line pointer at queryParsers.ts plus the bare decode-fallback essentials.
  • _{maintainability, kieran-typescript}
• packages/app/tests/e2e/page-objects/DashboardPage.ts:458 -- target.boundingBox() is recomputed after the 5-step activation nudge but before the 15-step move, so the recompute lands a few ms into @dnd-kit's 250ms sortable transform on neighboring containers; the recomputed box is partially-shifted, not at-rest and not at-final-position.
  • Fix: Either drop the recompute (use the original targetBox since at activation the layout has not yet shifted) or await page.waitForTimeout(260) between the activation nudge and the recompute so the transform completes first.
  • _{correctness, julik-frontend-races}

🟡 P2 -- recommended

• packages/app/src/components/DashboardContainer.tsx:144 -- The newly-added data-testid="group-add-tile-${id}" on the in-group "Add Tile" button is never clicked end-to-end, so the path that adds a tile into a specific container (rather than at the dashboard root) is dark.
  • Fix: Add a test that clicks group-add-tile-${id}, configures and saves a tile, and asserts the new tile lands inside [data-testid="group-container-${id}"].
  • _testing
• packages/app/src/components/DashboardContainer.tsx:236 -- The hasContainerAlert branch that renders <AlertDot /> next to the title when alertingTabIds is non-empty is the only signal a user gets that an alert is firing inside a collapsed container, and it has no test seeding alerting state.
  • Fix: Add a focused RTL/integration test on DashboardContainer with alertingTabIds={new Set([tabId])} asserting the dot renders.
  • _testing
• packages/app/tests/e2e/page-objects/DashboardPage.ts:298 -- waitForDashboardPatch's URL filter r.url().includes('/api/dashboards/') matches PATCH requests to any dashboard, which makes it unsafe for any future test that triggers a side-channel PATCH to a different dashboard.
  • Fix: Take dashboardId as an argument and match /api/dashboards/${dashboardId}; document in JSDoc that the helper must be called before triggering the PATCH and matches the next one in flight.
  • _{maintainability, julik-frontend-races}
• packages/app/tests/e2e/page-objects/DashboardPage.ts:356 -- getActiveTabsParam silently returns {} on any parse failure or non-string value, so a serializer regression (e.g., flipping to JSON.stringify([id1, id2])) would surface as expect.poll(...).toBeTruthy() timing out at 5s with no actionable error.
  • Fix: Distinguish "param missing" from "param present but unparseable" — return null for the latter so callers can .not.toBeNull() and surface the underlying decode failure, or stash a lastParseError and console.warn from the empty-object branch.
  • _{correctness, testing, kieran-typescript}

🔵 P3 nitpicks (4)

• packages/app/src/components/DashboardContainer.tsx:227 -- The MAX_COLLAPSED_TABS=4 truncation with ' | …' suffix is untested, so an off-by-one in the slice boundary would not be caught.
  • Fix: Add a quick test that adds 5+ tabs, collapses, and asserts the header text matches /^t1 \| t2 \| t3 \| t4 \| …$/.
• packages/app/tests/e2e/features/dashboard-container.spec.ts:41 -- addGroupAndGetId is defined inline in the spec rather than on DashboardPage, so the next container spec will copy-paste the before/after diff dance.
  • Fix: Hoist to DashboardPage.addGroupAndReturnId(): Promise<string> next to addGroup, where waitForLoaded and getGroupOrder already live.
  • _{maintainability, kieran-typescript}
• packages/app/tests/e2e/page-objects/DashboardPage.ts:303 -- The r.ok() filter inside waitForDashboardPatch causes any 4xx/5xx PATCH response to hang the helper to its 15s timeout, masking real backend regressions as opaque waits.
  • Fix: Drop r.ok() from the predicate and instead await waitForDashboardPatch() followed by an explicit expect(response.ok()).toBe(true) so failures surface as assertion errors.
  • _correctness
• packages/app/tests/e2e/page-objects/DashboardPage.ts:449 -- The double mouse.move(startX, startY) before mouse.down() does not "stabilise pointerdown coalescing" as the comment claims — Playwright dispatches one event per call and the second is at the same point, so it is a no-op the browser may even coalesce away.
  • Fix: Drop the second mouse.move (one line) or rewrite the comment to describe what it actually does.
  • _{julik-frontend-races}

Pre-existing

• packages/app/tests/e2e/page-objects/DashboardPage.ts -- File grows from 712 to 953 lines (+241 of getGroup*, openGroupMenu, dragGroupTo, getActiveTabsParam, waitForDashboardPatch); root AGENTS.md Key Principle feat: add docker prod build stages and publish prod builds #4 says keep files under 300 lines and "break down large components." The principle is phrased around components, but the next surface added (filters, tile menus) tips this toward monolith territory.
  • Fix: Extract a DashboardContainerComponent page-object-component co-located alongside TimePickerComponent / ChartEditorComponent already used at DashboardPage.ts:53-54.
  • _{maintainability, project-standards}

Agent-Native Gaps

Both gaps below are pre-existing in DashboardContainer.tsx / DashboardDndContext.tsx; this PR's net direction is positive (new data-testid and data-bordered hooks make previously orphan affordances reachable).

• Hover-gated overflow menu -- group-menu-${id} sits behind pointer-events: none until React's hovered state commits (DashboardContainer.tsx:106-109). The new page object compensates with group.hover() + a pointer-events poll, but an agent issuing a direct click on the testid silently no-ops. Either always render the trigger as interactive or expose a state attribute the agent can read.
• Drag-only reorder -- Reordering is exclusively mouse-drag with an 8 px activation threshold; no "Move Up/Down" menu items, no keyboard sortable, no programmatic reorder API. Add Move Up / Move Down items (gated on position) to the existing overflow menu so non-pointer agents can reorder.

Learnings & Past Solutions

docs/solutions/ does not exist in this repo (only ce-plugin/docs/solutions/ for the vendored plugin, scope-irrelevant). The patterns this PR establishes — data-testid="group-add-tab-${id}" convention, dragGroupTo nudging past dnd-kit's 8 px threshold, expect.poll for nuqs URL round-trips, double-decoding activeTabs, fire-and-forget PATCH-race hardening — are net-new institutional knowledge. Worth capturing via /ce-compound after merge.

Coverage

• Suppressed below confidence anchor 75: ~14 single-reviewer findings at anchor 50 (e.g., getCollapsedParam empty-array sentinel, getGroupBorderedAttr return-type honesty, expect.poll synchronous re-read race, dragGroupTo viewport-scroll handling, openGroupMenu 5 s pointer-events timeout, toHaveText('Show Border') exact-match brittleness, @full-stack tag asymmetry, dashboardSections.test.tsx lingering vocabulary, comment-citation rot to source line numbers, data-bordered precedent). One P0-anchor-50 finding (waitForLoaded hydration) was retained per the P0 exception. None met the cross-reviewer promotion criteria.
• No mode-aware demotions (no findings matched the testing/maintainability + advisory + P2/P3 trio).
• No CE schema-drift or deployment-verification agents ran (no migration files in scope).

Verdict

Ready with fixes. The two P0s (waitForDashboardPatch race and waitForLoaded hydration) are real flake vectors that will surface intermittently in CI; address them before merge. P1 menu-coverage gaps and the prose-lint markers should be cleaned up but do not block. Net assessment: the spec is unusually thoughtful — the comments cite the actual race conditions it hardens against and the page object documents the dnd-kit/nuqs quirks it works around — but a few specific async sequences still mis-match the asynchrony they claim to handle.

---

Reviewers (8): correctness, testing, maintainability, project-standards, ce-agent-native-reviewer, ce-learnings-researcher, kieran-typescript, julik-frontend-races.

Testing gaps:

• Tab rename and tab delete via GroupTabBar (onRenameTab / onDeleteTab) — only add-tab and switch-tab covered.
• Bordered-toggle reload round-trip — chevron and tabs are round-tripped, bordered is intentionally skipped.
• Backward drag (C → A) exercises the opposite arrayMove direction in handleReorderContainers and is the historically buggier sortable case.
• dragGroupTo "drag onto self" crosses the 8 px activation threshold and asserts a no-op outcome but does not deterministically reach the cited activeData.containerId !== overData.containerId guard at DashboardDndContext.tsx:67-70 — could pass via the !over early-return instead, hiding a regression in the cited guard.