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: a07be63

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 26, 2026 8:17am

[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.

Notes (non-blocking):

• ⚠️ DashboardPage.ts is now 1088 lines; AGENTS.md asks for files under 300. Pre-existing growth, but consider splitting into a dedicated DashboardContainerActions mixin in a follow-up.
• ℹ️ getCollapsedParam/getExpandedParam rely on container ids never containing , (true for nanoid). Fine for now — worth a one-line comment if ids ever change format.
• ℹ️ Production-code surface is intentionally minimal: a data-bordered attribute (DashboardContainer.tsx:244) and a data-testid on the Add Tab menu item (DashboardContainer.tsx:168). Both are pure additive testability hooks, no behavior change.
• ℹ️ waitForDashboardPatch() correctly registered before mutation (avoids the post-hoc waitForResponse race). Good pattern to keep reusing.

[github-actions]

E2E Test Results

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

Status	Count
✅ Passed	187
❌ Failed	0
⚠️ Flaky	2
⏭️ 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

✅ No P0/P1 ship-blockers. The diff is overwhelmingly test infrastructure; the production change is two prose-lint markers, one new data-testid, and one data-bordered attribute. The substantive concerns are all test-flake and test-strength issues, not production regressions.

🟡 P2 -- recommended

• packages/app/tests/e2e/features/dashboard-container.spec.ts:253 -- waitForDashboardPatch() is armed after two addGroupAndGetId calls whose fire-and-forget PATCHes can still be in flight, so the listener can resolve on the wrong mutation and the spec then navigates away before the targeted addTabToGroup PATCH lands, dropping the tab.
  • Fix: Narrow the matcher to assert the request body shape (e.g. group B has 2 tabs), or drain the prior addGroupAndGetId PATCHes before registering the listener.
  • _{correctness, testing, julik-frontend-races}
• packages/app/tests/e2e/features/dashboard-container.spec.ts:312 -- Same race in the drag-to-reorder test: three prior addGroupAndGetId PATCHes can pre-empt the reorderPatch listener since the no-op self-drag at line 307 emits no PATCH of its own.
  • Fix: Wait for all three addGroup PATCHes to settle (or assert on the reorder body shape) before registering reorderPatch.
  • _{correctness, julik-frontend-races}
• packages/app/tests/e2e/features/dashboard-container.spec.ts:290 -- The round-trip test (@full-stack) only asserts toHaveCount(2) on group B after reload, so a backend regression that drops and recreates the second tab with a new id silently passes.
  • Fix: Capture the second tab id from ?activeTabs before navigation and assert the same id is present after reload.
• packages/app/tests/e2e/features/dashboard-container.spec.ts:41 -- addGroupAndGetId returns as soon as the DOM count updates but does not await the underlying setDashboard PATCH, leaving multiple mutations stacked behind subsequent helpers in @full-stack mode.
  • Fix: Await waitForDashboardPatch() inside the helper (or have @full-stack callers sequence each addGroup against its own PATCH) so callers always derive from a confirmed server state.

🔵 P3 nitpicks (4)

• packages/app/tests/e2e/features/dashboard-container.spec.ts:127 -- getActiveTabsParam()[id]! is read synchronously immediately after expect.poll(...).toBeTruthy(), so any nuqs flush between the poll's last tick and the sync read could let the non-null assertion suppress an undefined; the same pattern repeats at line 207.
  • Fix: Capture the polled value inside the expect.poll callback (or use Playwright's matcher return value) instead of asserting then re-reading.
  • _{julik-frontend-races, kieran-typescript}
• packages/app/src/components/DashboardContainer.tsx:244 -- data-bordered="true|false" is a production DOM attribute added purely as a test affordance, permanently coupling internal bordered state to the component's public surface for every consumer.
  • Fix: Assert via toHaveCSS('border-style', ...) against the existing inline border style instead of adding a production attribute, or scope the attribute to test builds.
• packages/app/tests/e2e/page-objects/DashboardPage.ts:381 -- getActiveTabsParam reimplements the double-decode contract from parseAsJsonEncoded in packages/app/src/utils/queryParsers.ts; if the production serializer changes its encoding strategy the helper will silently drift.
  • Fix: Extract the URL-decode step out of parseAsJsonEncoded into a shared helper that both queryParsers.ts and the page object can call.
• packages/app/tests/e2e/page-objects/DashboardPage.ts:277 -- getGroupOrder filters empty ids but the docstring claims the filter is there "to surface DOM regressions" — the implementation in fact hides them.
  • Fix: Either drop the .filter(id => id.length > 0) so missing ids break equality assertions loudly, or rewrite the docstring to match the implementation's true intent (robustness over visibility).

---

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

Testing gaps:

• Bordered state has no save+reload round-trip coverage; test Generic Webhook Option for Alerts #2 only asserts the in-memory DOM flip (acknowledged out-of-scope in the PR body, but worth a follow-up).
• Drag-onto-self verifies the DOM no-op but does not assert that no PATCH was emitted, leaving room for a regressed DnD guard to issue spurious mutations.
• ?activeTabs reload exercises only the second-tab restore path; first-tab restore is unverified.