Skip to content

Add PageHeader/PageLayout and migrate Sessions#2282

Open
elizabetdev wants to merge 6 commits into
mainfrom
agent/page-shell
Open

Add PageHeader/PageLayout and migrate Sessions#2282
elizabetdev wants to merge 6 commits into
mainfrom
agent/page-shell

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev commented May 15, 2026

Summary

  • Introduces PageHeader and PageLayout for a consistent sticky page shell (optional breadcrumbs row, toolbar slots, padded content, fill viewport).
  • Migrates Sessions to PageLayout (single-row query toolbar in a custom header), previously in Migrate Sessions page to PageLayout #2287, so this branch is self-contained for review.
  • Documents patterns in agent_docs/page_layout.md and the page-layout Claude skill.

Before

Screenshot 2026-05-15 at 18 16 36

After

Screenshot 2026-05-15 at 18 15 44

Test plan

  • make ci-lint / app typecheck
  • /sessions loads; Run, filters, list, and side panel behave as before
  • yarn knip (root) passes

Made with Cursor

elizabetdev and others added 2 commits May 15, 2026 12:56
Introduces a sticky page shell with optional breadcrumbs, title, and
toolbar slots plus agent documentation and the page-layout skill.

Co-authored-by: Cursor <cursoragent@cursor.com>
Keeps the single-row query toolbar inside a custom PageHeader while using
the shared layout wrapper for scroll and padding.

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

changeset-bot Bot commented May 15, 2026

⚠️ No Changeset found

Latest commit: 5f147e1

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
Copy link
Copy Markdown

vercel Bot commented May 15, 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 15, 2026 5:18pm

Request Review

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

github-actions Bot commented May 15, 2026

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Large diff: 418 production lines changed (threshold: 400)

Additional context: agent branch (agent/page-shell)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 6
  • Production lines changed: 418
  • Branch: agent/page-shell
  • Author: elizabetdev

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

PR Review

  • ⚠️ Padding regression on existing PageHeader callers (AlertsPage, DashboardsListPage, SavedSearchesListPage, TeamPage) — horizontal padding drops from 32px to var(--mantine-spacing-sm) (~12px). The SCSS comment says this is intentional to match Search, but the PR description doesn't flag it and all four pages render differently after merge. → Confirm the visual shift is desired, or scope the new padding to pages that opt into title/leading/actions.
  • ⚠️ Docs describe a state that doesn't exist yet. agent_docs/page_layout.md and .claude/skills/page-layout/SKILL.md list KubernetesDashboardPage, ClickhousePage, and DBServiceMapPage under "Reference implementations" / "Pages using PageHeader / PageLayout today", but none of those files import either primitive (only SessionsPage does in this PR). → Reword to "planned" or land the follow-up migrations before referencing them as canonical examples; otherwise agents will be sent to read non-existent patterns.
  • ⚠️ Sessions form now wraps the entire page instead of only the search inputs (packages/app/src/SessionsPage.tsx:382-470). Stray inputs anywhere in content (e.g. inside SessionSetupInstructions or future children) will trigger onSubmit on Enter. → Consider keeping the <form> scoped to the toolbar inside PageHeader children, or document the wider scope is intentional.
  • ⚠️ <div><header> element change for the same styles.header selector. Low risk (no CSS in the diff targets div.header), but any global rule keyed on the tag would silently break. → Quick grep for div.header / .header selectors outside this module before merge.

Nothing security-relevant. The primitives themselves look clean and the API split (PageLayout vs PageHeader) is reasonable.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

Deep Review

✅ No critical issues found.

🟡 P2 -- recommended

  • packages/app/src/components/PageHeader.module.scss:14 — Reducing the shared header padding from 0 32px to 0 var(--mantine-spacing-sm) shifts every existing consumer (AlertsPage, DashboardsListPage, SavedSearchesListPage, TeamPage) left without migrating those pages, so they ship with new spacing the PR does not touch.
    • Fix: Scope the new padding to a headerStacked / migrated-only class and keep the legacy single-row branch at 0 32px, or migrate the four un-touched pages in this PR and verify the headers visually.
    • project-standards-reviewer, maintainability-reviewer
  • packages/app/src/components/PageHeader.tsx:31 — The four conditional render branches (hasBreadcrumbs && hasToolbar, hasBreadcrumbs && !hasToolbar, !hasBreadcrumbs && hasToolbar, none) ship without any unit tests covering them or the children drop behavior.
    • Fix: Add a PageHeader.test.tsx (or PageLayout.test.tsx) exercising the four prop combinations plus the case where children and title are both passed.
    • testing-reviewer, correctness-reviewer
🔵 P3 nitpicks (3)
  • packages/app/src/components/PageHeader.tsx:69 — When title/leading/actions are populated, the toolbarInner branches render without children, but PageHeaderProps still accepts children, so passing both silently drops the children.
    • Fix: Narrow the prop type with a discriminated union so children and the toolbar slots are mutually exclusive, or warn in dev when both are supplied.
  • packages/app/src/SessionsPage.tsx:382 — The <form> now wraps the entire PageLayout (header + content) where it previously wrapped only the toolbar; any future descendant <button> without type="button" inside SessionCardList or the empty-state block would submit the toolbar.
    • Fix: Keep the form scoped to the custom header <Group> so submission only covers the toolbar inputs, or audit content-area buttons for explicit type="button".
  • packages/app/src/SessionsPage.tsx:383 — The SessionsPage string added to className is no longer referenced in any SCSS or selector after the migration.
    • Fix: Drop the global SessionsPage literal and keep only styles.pageForm.

Reviewers (4): correctness-reviewer, testing-reviewer, maintainability-reviewer, project-standards-reviewer.

Testing gaps:

  • New shared PageHeader / PageLayout primitives have no unit tests despite non-trivial conditional rendering.
  • No visual or snapshot coverage for the four existing PageHeader consumers whose padding changes in this diff.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

E2E Test Results

All tests passed • 176 passed • 3 skipped • 1232s

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

Tests ran across 4 shards in parallel.

View full report →

Use ignoreFiles for PageLayout until consumer PRs land, document cleanup
in agent_docs, add a TODO on the component, and fix SCSS comment spacing.

Co-authored-by: Cursor <cursoragent@cursor.com>
elizabetdev and others added 2 commits May 15, 2026 15:55
Combine PageLayout consumer so Knip can resolve PageLayout without ignoreFiles.
Remove temporary ignore, PageLayout TODO, and refresh page_layout.md Knip note.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added review/tier-4 Critical — deep review + domain expert sign-off and removed review/tier-3 Standard — full human review required labels May 15, 2026
@elizabetdev elizabetdev changed the title Add shared PageHeader and PageLayout Add PageHeader/PageLayout and migrate Sessions May 15, 2026
Drop `mt-4` on the list wrapper so content sits tighter below the toolbar.

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

PR Review

  • ⚠️ PageHeader.module.scss changes the global .header padding from 0 32px to 0 var(--mantine-spacing-sm) (~12px). This affects every existing consumer (AlertsPage, DashboardsListPage, SavedSearchesListPage, TeamPage), not only Sessions — a ~20px visual shrink on pages the PR claims are out of scope. → Either revert to 32px and only narrow padding for new/migrated pages (e.g. via a compact modifier or the stacked variant), or explicitly verify and call out the unmigrated pages in the PR description.
  • ⚠️ Inconsistent semantics between title prop and children-only pattern: <PageHeader title="Alerts" /> renders an <h1>, but the still-in-tree <PageHeader>Alerts</PageHeader> (Alerts/Dashboards/SavedSearches) renders plain text in a <div>. agent_docs/page_layout.md claims those pages use PageHeader with title, which doesn't match the code. → Either migrate those four consumers to title= in this PR or correct the doc to describe the current state.
  • ⚠️ No unit tests for the new PageHeader branches (children-only, breadcrumbs+toolbar, breadcrumbs-only, toolbar-only) or PageLayout (fillViewport, padded, header override). Multiple render paths landed without coverage. → Add a small __tests__/PageHeader.test.tsx + PageLayout.test.tsx to lock in the branch behavior.
  • 🔎 In SessionsPage, the <form> now wraps PageLayout, so the sticky <header> is inside a form whose data-testid differs from the page root. The page-root data-testid="sessions-page" and sessions-search-form are both preserved, but worth a quick smoke run of packages/app/tests/e2e/features/sessions.spec.ts and core/navigation.spec.ts (which asserts contentTestId: 'sessions-page') before merge.
  • 🔎 PageHeader always renders the .start wrapper even when there's no title/leading (just an empty div in the DOM). Minor — guard it the way .actions is guarded for cleaner markup.

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

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant