Skip to content

feat(dashboard): migrate to PageLayout with sticky query toolbar#2364

Open
elizabetdev wants to merge 1 commit into
mainfrom
page-migrate-dashboard
Open

feat(dashboard): migrate to PageLayout with sticky query toolbar#2364
elizabetdev wants to merge 1 commit into
mainfrom
page-migrate-dashboard

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

Summary

Migrates the Dashboard page to the shared PageLayout / PageHeader shell and introduces a reusable stickyRow slot on PageHeader so any page can pin a single row while the rest of the header scrolls away.

On the dashboard:

  • Sticky row: the query toolbar (SQL/Lucene WHERE, time range, granularity, Live, refresh, edit filters, Run) stays pinned at the top of the scroll container while the user scrolls through tiles.
  • Scrolling chrome: breadcrumbs, the editable dashboard name, dashboard actions (Favorite / Tags / Menu) and the "Created by … Updated …" meta sit above and scroll away with the page.
  • Meta moves into the breadcrumbs row (right side) instead of taking its own body line.
  • Redundant mt="sm" removed from DashboardFiltersPageLayout's padded content already provides the top offset.

The stickyRow slot

<PageHeader
  breadcrumbs={pageBreadcrumbs}    // scrolls away
  stickyRow={queryToolbar}          // pinned to top — the only sticky row
>
  {titleRow}                         // scrolls away
</PageHeader>

Behavior:

  • When stickyRow is provided, the chrome <header> becomes non-sticky and drops its bottom border / bottom padding so it reads as one block with the toolbar at rest.
  • PageHeader returns a Fragment of <header> + sticky <div> so the sticky row's containing block is the page layout root — it stays pinned for the full scroll length rather than being bounded by the chrome's height (the classic position: sticky gotcha).
  • An IntersectionObserver tracks when the row touches the scroll-container top and toggles a .stickyRowAttached class so the row has no padding-top while attached to chrome (tight at rest) and standard padding once stuck (breathing room from the viewport edge). Also exposes data-stuck on the row for tests / debugging.
  • When stickyRow is omitted, PageHeader behaves exactly like before — single sticky block. No other page needs to change.

Test plan

  • yarn ci:unit in packages/app still passes.
  • Open a saved dashboard. Confirm:
    • At rest: chrome (breadcrumbs + name + actions) and toolbar read as one continuous block, no gap and no extra horizontal line between them.
    • Scrolling down: chrome scrolls away; the query toolbar stays pinned at the top of the scroll area with comfortable top padding.
    • Scrolling back up: chrome reappears, toolbar tucks back into "attached" mode (top padding removed).
    • "Created by … Updated …" meta sits on the right side of the breadcrumbs row with the existing tooltip preserved.
  • Open any non-dashboard page that uses PageHeader (Alerts list, Search page, Sessions, Service Map, Kubernetes). Confirm the header still behaves identically to main (fully sticky, no visual regression).
  • Open a dashboard with no defined dashboard variables — DashboardFilters row still sits at the right vertical offset (no doubled top padding).

Made with Cursor

Replaces the custom Dashboard page chrome with the shared
`PageLayout` / `PageHeader` shell and introduces a reusable
`stickyRow` slot on `PageHeader` so any page can pin a single row
while the rest of the header scrolls away.

On the dashboard this is used to keep the query toolbar (SQL/Lucene
WHERE, time range, granularity, Live, refresh, edit filters, Run)
visible while the user scrolls through tiles. The breadcrumbs, the
editable dashboard name, dashboard actions (Favorite, Tags, Menu) and
the "Created by … Updated …" meta all live in the scrolling chrome
above; the meta moves to the right edge of the breadcrumbs row
instead of taking its own body line.

PageHeader changes:
- New `stickyRow?: ReactNode` slot. When provided, the chrome
  `<header>` becomes non-sticky (and drops its bottom border /
  bottom padding so it reads as one block with the toolbar), and a
  sibling `<div>` renders the `stickyRow` at `position: sticky;
  top: 0`. Returning a Fragment of the two siblings means the
  sticky row's containing block is the page layout root, so it
  stays pinned for the full scroll length rather than being bounded
  by the chrome height.
- `IntersectionObserver` tracks when the sticky row touches the
  scroll-container top and toggles a `.stickyRowAttached` class so
  the row has no `padding-top` while attached to chrome (tight
  spacing at rest) and the standard `--mantine-spacing-sm` top
  padding once stuck (breathing room from the viewport edge). The
  state is also exposed as `data-stuck` on the row for tests.
- Pages without a `stickyRow` keep the existing fully-sticky header
  behavior — no migration needed elsewhere.

Also drops a redundant `mt="sm"` from `DashboardFilters`, since the
`PageLayout` `padded` content already provides top padding above it.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 28, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 28, 2026 6:59pm
hyperdx-storybook Error Error May 28, 2026 6:59pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 28, 2026

🦋 Changeset detected

Latest commit: a502c05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label May 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 808 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 4
  • Production lines changed: 808
  • Branch: page-migrate-dashboard
  • Author: elizabetdev

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

@github-actions
Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 191 passed • 3 skipped • 1326s

Status Count
✅ Passed 191
❌ Failed 0
⚠️ Flaky 4
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

🟡 P2 -- recommended

  • packages/app/src/components/PageHeader.tsx:91 -- IntersectionObserver is constructed with no root, so it observes the viewport, but the real scroll container is the #app-content-scroll-container div from layout.tsx:62; when the ClickStack banner is open, the row pins at viewport-y = banner height, intersectionRatio stays 1, and isStuck never flips, so .stickyRowAttached is permanently applied and the toolbar's intended top-padding swap never fires.
    • Fix: Pass the scroll container (document.getElementById('app-content-scroll-container'), ideally via a ref or context) as root, or use a sentinel-div above the row observed with threshold: 0 and no root.
    • correctness, julik-frontend-races, adversarial
  • packages/app/src/components/PageHeader.module.scss:30 -- the new .notSticky rule sets padding-block-end: 0 (explicit comment: "doubling up creates a visible gap"), but .headerStacked later in the file declares padding-block: var(--mantine-spacing-xs) at equal specificity, so source-order wins and the chrome retains a bottom padding, producing exactly the doubled gap the rule was meant to remove on the dashboard's breadcrumbs-plus-children branch.
    • Fix: Reorder so .notSticky follows .headerStacked, or raise specificity (.header.notSticky / .headerStacked.notSticky), or split into padding-block-start/padding-block-end longhands in .headerStacked.
    • adversarial
  • packages/app/src/DBDashboardPage.tsx:2560 -- the "This is a temporary dashboard and can not be saved" Paper banner with the create-dashboard-button is now rendered inside dashboardBody after the sticky queryToolbar, whereas previously it sat immediately under the breadcrumbs above the title row, pushing the only "Create New Saved Dashboard" affordance below the fold on shorter viewports for users who land on a temporary dashboard.
    • Fix: Render the local-dashboard banner inside the chrome (e.g. as part of pageBreadcrumbs or before titleRow), or replace the body banner with an inline "Save as Dashboard" affordance in dashboardActions for isLocalDashboard.
    • adversarial
  • packages/app/src/components/PageHeader.tsx:62 -- the new stickyRow slot adds a useEffect + IntersectionObserver lifecycle, the isStuck class swap, a Fragment return shape, and a new chrome-null branch, but no unit test exercises any of it and the PR's test plan is entirely manual; with no PageHeader.test.tsx in the repo, any future regression in the observer wiring or class toggling lands silently.
    • Fix: Add a PageHeader.test.tsx covering the stickyRow Fragment render, .notSticky application on chrome, and an IntersectionObserver-mocked isStuck toggle of .stickyRowAttached / data-stuck.
    • testing, kieran-typescript, maintainability, project-standards, julik-frontend-races
🔵 P3 nitpicks (7)
  • packages/app/src/setupTests.tsx:13 -- jsdom does not implement IntersectionObserver and setupTests.tsx only polyfills ResizeObserver and matchMedia, so any future test that mounts a component with stickyRow will throw ReferenceError: IntersectionObserver is not defined at effect time.
    • Fix: Add a minimal class polyfill (observe/unobserve/disconnect no-ops) to setupTests.tsx, or guard the effect with typeof IntersectionObserver === 'undefined'.
  • packages/app/src/DBDashboardPage.tsx:2274 -- dashboardActions Group changed from default wrap="wrap" to explicit wrap="nowrap", so on narrow widths with a long dashboard name (capped at maw={500} on EditablePageName) and several tags, the action cluster can push past the chrome's horizontal padding instead of wrapping inside its own column.
    • Fix: Drop wrap="nowrap", or add style={{ minWidth: 0 }} / truncate to EditablePageName so the title yields before the actions overflow.
  • packages/app/src/DBDashboardPage.tsx:2219 -- the new dashboardMeta Text carries style={{ flexShrink: 0 }}, which is new vs the prior layout and prevents the "Created by … Updated …" string from compressing, so on narrow viewports with verbose creator/updater names the meta can push the breadcrumbs trail off-screen.
    • Fix: Remove flexShrink: 0 (or add maw + truncate="end") so the meta yields under width pressure the same way the dashboard-name segment already does.
  • packages/app/src/components/PageHeader.tsx:127 -- when stickyRow is provided and every other slot is empty, chrome is null, so the consumer's className and data-testid are silently dropped and only the hardcoded page-header-sticky-row test id survives; no current caller hits this, but it's a quiet contract change as the API spreads.
    • Fix: Forward className and the consumer data-testid onto the sticky-row <div> when chrome is null, or assert in the prop docs that stickyRow-only usage is unsupported.
  • packages/app/src/components/PageHeader.tsx:179 -- data-stuck={isStuck ? 'true' : undefined} uses a string sentinel where the codebase's conventional pattern for "present/absent" is data-stuck={isStuck || undefined}, forcing any future SCSS consumer to write [data-stuck='true'] instead of the simpler [data-stuck].
    • Fix: Change to data-stuck={isStuck || undefined} (or data-stuck={isStuck}) for present/absent semantics.
  • packages/app/src/components/PageHeader.module.scss:25 -- .notSticky is named for the property it cancels rather than the role the chrome takes on, requiring the next reader to read the comment block to understand why a class called "not sticky" is applied to the chrome.
    • Fix: Rename to .chromeScrolling (or similar role-based name) and update the single reference in chromeStickyClass.
  • packages/app/src/components/PageHeader.tsx:93 -- threshold: [1] plus intersectionRatio < 1 is brittle at fractional device-pixel ratios, where the browser can report 0.9997 for a geometrically-fully-visible element, so on 1.25x / 1.5x zoom the at-rest padding can flicker on resize.
    • Fix: Compare against a small epsilon (e.g. intersectionRatio < 0.99) or use a sentinel-element pattern instead of the top: -1px rootMargin trick.

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

Testing gaps:

  • No automated coverage for the new stickyRow Fragment shape, the .notSticky / .stickyRowAttached class toggling, the IntersectionObserver stuck-transition, or the chrome-null branch.
  • No coverage for the isLocalDashboard layout regression (banner now below the sticky toolbar).
  • No visual / E2E assertion that the dashboard query toolbar actually stays pinned while breadcrumbs and title scroll away — the PR's central user-visible behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant