Repository navigation
[HDX-5307] Top-align toolbars next to growing query fields - #3151
Conversation
🦋 Changeset detectedLatest commit: 7623e3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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.
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** | ||
| * Add block padding that only shows once the toolbar outgrows `min-height`. | ||
| * Use for a header that can grow (Sessions' multi-line search); omit on | ||
| * title-only pages so they keep a flush 60px bar. | ||
| */ | ||
| growing?: boolean; |
There was a problem hiding this comment.
This user-facing @hyperdx/app layout change does not include a changeset. The repository guide requires a changeset for behavior changes to published packages, so this requirement must be satisfied before merging by adding a patch changeset.
Context Used: CLAUDE.md (source)
There was a problem hiding this comment.
Added a patch changeset.
Deep ReviewScope: Pure UI/CSS layout change across 13 ✅ No critical issues found. No P0/P1 or P2 findings cleared the bar. Verification highlights:
Prior review comments (all resolved in the current tree):
🔵 P3 nitpicks (1)
Reviewers (7): correctness, kieran-typescript, julik-frontend-races, testing, maintainability, project-standards, previous-comments. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Use for a header that can grow (Sessions' multi-line search); omit on | ||
| * title-only pages so they keep a flush 60px bar. | ||
| */ | ||
| growing?: boolean; |
There was a problem hiding this comment.
🔵 minor — New growing prop is unreachable via PageLayout and renders identically to the default on every current header
PageLayoutProps picks only 'title' | 'leading' | 'actions' | 'breadcrumbs' | 'children' (packages/app/src/components/PageLayout.tsx:27-30), so the structured-slot pages — the only ones that could ever need it (DBServiceMapPage, ClickhousePage, KubernetesDashboardPage) — cannot opt in without switching to the header escape hatch; add 'growing' to that Pick. Separately, the stated rationale in PageHeader.module.scss:25-28 and the prop doc ("omit on title-only pages so they keep a flush 60px bar") does not hold: box-sizing: border-box is global (packages/app/styles/globals.css:12) and .header has min-height: 60px, so padding-block: 10px leaves a 39px content box and every current header's at-rest content is ≤36px (a sm input/button) — title-only pages would not shift. If the opt-in isn't wanted, put padding-block on .header and delete the flag; that also removes a latent conflict, since .header.growing (two classes) outranks .notSticky { padding-block-end: 0 } (one class), so a future header passing both growing and stickyRow would keep the bottom padding .notSticky exists to drop.
There was a problem hiding this comment.
Sessions already uses the header escape hatch, which is the only page that needs growing. Structured-slot pages stay flush; forwarding through PageLayout can wait until one of them actually grows.
| <Flex gap="sm" mb="sm" align="flex-start" justify="end"> | ||
| {filtersToggle != null && tabQueriesData(activeTab) && ( | ||
| <DashboardFiltersToggle {...filtersToggle} /> | ||
| <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> |
There was a problem hiding this comment.
🔵 minor — <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> first-line box is copy-pasted 8 times, and prior art already exists
The same wrapper is repeated at ChartActionBar.tsx:160, ChartEditorControls.tsx:337 and :361, ChartSeriesEditor.tsx:525, :551 and :577, HighlightedAttributes.tsx:89 and :98, MaterializedViews.tsx:576, OrderByFormRow.tsx:92 — and FormRow already implements the identical idiom with a hardcoded height: '36px' label box (packages/app/src/components/Sources/SourceForm/FormRow.tsx:18-24). Per the repo's REQUIRED DRY rule ("shared logic between two components → factor it into a sibling module"), extract one component next to editorInputHeights.ts (e.g. <EditorFirstLineRow size="sm">{children}</EditorFirstLineRow>), use it at all eight sites, and have FormRow use it too so the 36px constant has one source of truth.
There was a problem hiding this comment.
Leaving the wrappers inline. Extracting a shared cell and switching FormRow would widen this PR; Sessions is the only growing header today.
PR Review
4 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 4 minor 2 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
0a3497a to
48cafdf
Compare
| Block padding is absorbed at rest and only shows once the row outgrows the | ||
| header — otherwise the query sits flush against the edges. Other pages keep | ||
| the zero block padding above so a title-only bar does not shift. */ | ||
| .header.growing { |
There was a problem hiding this comment.
🔵 minor — growing opt-in guards against a shift that cannot happen, and inverts .notSticky's specificity
.header is box-sizing: border-box (packages/app/styles/globals.css:12) with min-height: 60px and a 1px bottom border, so 10px block padding leaves 39px of content box — more than the tallest control any current .header consumer renders (36px input-sm: AlertsPage/DashboardsListPage/SavedSearchesListPage are title-only, TeamPage and DBServiceMapPage use size="xs", DashboardKioskHeader is a title + small group). Moving padding-block: var(--mantine-spacing-xs) onto .header itself leaves every one of them at exactly 60px and lets you drop the prop, the class, and the three doc edits. It also removes a latent trap: .header.growing (0,2,0) out-specifies .notSticky { padding-block-end: 0 } (0,1,0), so the first page that combines growing with stickyRow silently gets back the bottom padding whose own comment says it "creates a visible gap" — with the declaration on .header, .notSticky wins on source order instead.
There was a problem hiding this comment.
Keeping the opt-in. Title-only bars wouldn't shift at rest, but the earlier PageHeader review asked not to put block padding on every header. Specificity vs .notSticky is fixed with :not(.notSticky).
| {showWhere && ( | ||
| <> | ||
| <Text size="sm">Where</Text> | ||
| <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> |
There was a problem hiding this comment.
🔵 minor — <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> is inlined 10 times; FormRow already implements it
The same three-prop wrapper is repeated at ChartSeriesEditor.tsx:525,551,577, ChartEditorControls.tsx:337,361, ChartActionBar.tsx:160, HighlightedAttributes.tsx:89,98, MaterializedViews.tsx:576, and OrderByFormRow.tsx:92. packages/app/src/components/Sources/SourceForm/FormRow.tsx:16-24 is the existing implementation of exactly this idiom (a Flex align="flex-start" row whose label cell is pinned to a height: '36px' box) — except it hardcodes the literal, giving the number three homes (FormRow, EDITOR_INPUT_HEIGHTS, --editor-base-height). Extract one component next to editorInputHeights.ts (e.g. <EditorFirstLineCell size="sm">), use it at the ten sites, and switch FormRow's '36px' to EDITOR_INPUT_HEIGHTS.sm.
There was a problem hiding this comment.
Same as the ActionBar thread: not extracting a helper in this PR.
Keep date pickers, Run, and row actions level with the first line of a wrapping SQL or Lucene field. Sessions opts into PageHeader block padding so a taller search bar still has room. Co-authored-by: Cursor <cursoragent@cursor.com>
48cafdf to
f7ec30f
Compare
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
| <Flex gap="sm" mb="sm" align="flex-start" justify="end"> | ||
| {filtersToggle != null && tabQueriesData(activeTab) && ( | ||
| <DashboardFiltersToggle {...filtersToggle} /> | ||
| <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> |
There was a problem hiding this comment.
🔵 minor — New alignment wrappers pass EDITOR_INPUT_HEIGHTS.sm to Mantine h=, which converts it to rem while the editors hard-code px
Every existing consumer of the constant emits a literal pixel value — SQLInlineEditor.tsx:343, SearchWhereInput.tsx:210, AutocompleteInput.tsx:203, PromQLEditor.tsx:208 all set --editor-base-height: ${baseHeight}px, and .paper uses it as min-height. The eight new wrappers instead pass the bare number to Mantine's h style prop, which renders calc(2.25rem * var(--mantine-scale)). Identical at a 16px root font size, but a user who raises the browser's default font size (or a non-1 --mantine-scale) gets a taller wrapper than the 36px editor beside it, so the label/icon drops below the first line — the exact misalignment this PR fixes. Use style={{ height: EDITOR_INPUT_HEIGHTS.sm }} (React renders numeric height as px) or h={${EDITOR_INPUT_HEIGHTS.sm}px} at all eight sites (ChartActionBar.tsx:160, ChartEditorControls.tsx:337/361, ChartSeriesEditor.tsx:442/468/494, HighlightedAttributes.tsx:89/98, MaterializedViews.tsx:576, OrderByFormRow.tsx:92).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fixed: the wrappers now pass h={${EDITOR_INPUT_HEIGHTS.sm}px} so they stay on the same pixel height as the editors.
| Block padding is absorbed at rest and only shows once the row outgrows the | ||
| header — otherwise the query sits flush against the edges. Other pages keep | ||
| the zero block padding above so a title-only bar does not shift. */ | ||
| .header.growing { |
There was a problem hiding this comment.
🔵 minor — .header.growing outranks .notSticky, silently restoring the bottom padding that stickyRow mode removes
.header.growing is specificity (0,2,0); .notSticky (line 38) is (0,1,0), so its deliberate padding-block-end: 0 loses regardless of source order. The comment on .notSticky says the sticky row owns the separator and the breathing room and that doubling up creates a visible gap — so <PageHeader growing stickyRow={…}> would get exactly that gap. No caller combines them today, but nothing stops one. Scope the rule as .header.growing:not(.notSticky), or re-assert padding-block-end: 0 in a .growing.notSticky rule.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fixed: the rule is now .header.growing:not(.notSticky) so sticky-row chrome still drops the bottom padding.
| </ActionIcon> | ||
| </Tooltip> | ||
| {/* Stays level with the editor's first line as it grows */} | ||
| <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> |
There was a problem hiding this comment.
🔵 minor — The "hold a label level with the editor's first line" wrapper is re-implemented eight times; prior art already exists
FormRow (packages/app/src/components/Sources/SourceForm/FormRow.tsx:16-24) — the component this very row renders inside — already does this: an align="flex-start" row whose label column is a fixed height: '36px' box with centered content. SearchWhereInput.module.scss:68 (.languageSwitchRow) solves it a third way, off --editor-base-height. The DRY rule in the repository conventions is marked REQUIRED. Extract one small component (e.g. <EditorFirstLineAlign> next to editorInputHeights.ts), use it at the eight new sites, and switch FormRow's literal 36px to EDITOR_INPUT_HEIGHTS.sm so there is one source of truth for the first-line height.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Same as the other DRY threads: not pulling FormRow into this change.
E2E Test Results✅ All tests passed • 379 passed • 1 skipped • 1185s
Tests ran across 4 shards in parallel. |
Mantine's `h` number is rem, so pass an explicit px height to stay aligned with the 36px editors. Growing headers skip the sticky-row padding reset. Adds a patch changeset. Co-authored-by: Cursor <cursoragent@cursor.com>
| // Staged group-bys replace the chart's, same as the filters above: they | ||
| // were chosen against this metric's tags. Only when something was staged | ||
| // though — group by is chart-level, so clearing it on every apply would | ||
| // though � group by is chart-level, so clearing it on every apply would |
There was a problem hiding this comment.
🔵 minor — Comment rewritten with an invalid UTF-8 byte, so the file is no longer valid UTF-8
The em dash in // though — group by is chart-level was replaced with a raw non-UTF-8 byte (though � group by); rg 'though — group by' and rg 'though . group by' both fail to match because the byte isn't decodable. Nothing else in the hunk changed, so this is an unintended save-encoding artifact — restore the em dash (or use a plain -) and re-save the file as UTF-8.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| <IconCheck size={16} /> | ||
| </ActionIcon> | ||
| </Tooltip> | ||
| <Flex h={`${EDITOR_INPUT_HEIGHTS.sm}px`} align="center"> |
There was a problem hiding this comment.
🔵 minor — <Flex h={EDITOR_INPUT_HEIGHTS.sm}px align="center"> is repeated 8× and restates alignment logic that already exists three ways
The same wrapper is pasted at OrderByFormRow.tsx:91, MaterializedViews.tsx:575, HighlightedAttributes.tsx:88 and :97, ChartActionBar.tsx:160, ChartEditorControls.tsx:336 and :360, and ChartSeriesEditor.tsx:441/:467/:493 — while the codebase already solves "keep this control level with the editor's first line" in FormRow.tsx:16-33 (the very wrapper OrderByFormRow renders inside: align="flex-start" + a label column with a hardcoded height: '36px'), in SearchWhereInput.module.scss:68 (.languageSwitchRow { height: calc(var(--editor-base-height) - …) }), and in FilterTemplateList.tsx:55 (mt={3}). Per the REQUIRED DRY rule in CLAUDE.md, extract one component (e.g. <EditorFirstLineRow size?> next to editorInputHeights.ts) that takes the editor size and maps it through EDITOR_INPUT_HEIGHTS, use it at all eight call sites, and switch FormRow's literal '36px' to the constant — the hardcoded .sm is also a silent trap, since SQLInlineEditor/SearchWhereInput accept size="xs" (30px, as DBTracePanel.tsx:406 uses) and any such row would leave the label sitting 3px low with no type error.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
A previous commit rewrote the file through a non-UTF-8 locale, turning the em dash into a raw 0x97 byte. swc refused to read the file, breaking the app build, the Vercel deploy and every E2E shard.
| <IconCheck size={16} /> | ||
| </ActionIcon> | ||
| </Tooltip> | ||
| <Flex h={`${EDITOR_INPUT_HEIGHTS.sm}px`} align="center"> |
There was a problem hiding this comment.
🔵 minor — New <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> wrapper is repeated 9× and re-derives what FormRow already does
FormRow (packages/app/src/components/Sources/SourceForm/FormRow.tsx:16-24) already implements this exact idiom — <Flex align="flex-start"> around a justify="center" block pinned to height: '36px' — and both OrderByFormRow and HighlightedAttributes render inside a FormRow, so the same trick now exists twice in one subtree. The repo's REQUIRED DRY rule asks you to grep for the operation before adding a new one. Extract one small component (e.g. FirstLineRow next to editorInputHeights.ts) and use it at all nine call sites (ChartActionBar.tsx:160, ChartEditorControls.tsx:336/360, ChartSeriesEditor.tsx:441/467/493, HighlightedAttributes.tsx:88/97, MaterializedViews.tsx:575, OrderByFormRow.tsx:91), and switch FormRow's literal '36px' to EDITOR_INPUT_HEIGHTS.sm so the first-line height has one source of truth.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| Block padding is absorbed at rest and only shows once the row outgrows the | ||
| header — otherwise the query sits flush against the edges. Other pages keep | ||
| the zero block padding above so a title-only bar does not shift. */ | ||
| .header.growing:not(.notSticky) { |
There was a problem hiding this comment.
🔵 minor — The growing prop and its :not(.notSticky) guard can be replaced by one unconditional line
.header is box-sizing: border-box with min-height: 60px, so padding-block: var(--mantine-spacing-xs) (20px total) is absorbed for any content ≤40px tall — which covers every current non-stacked header: title-only pages, TeamPage.tsx:274 (xs input, 30px), DashboardKioskHeader.tsx:14, DBServiceMapPage.tsx:343 (Text/Slider/TimePicker, 36px). Applying it unconditionally to .header lets .notSticky's later padding-block-end: 0 win naturally, drops the growing prop, the :not(.notSticky) specificity workaround, and the doc edits in agent_docs/page_layout.md and .claude/skills/page-layout/SKILL.md. It also removes a latent trap: as written, growing + stickyRow loses the top padding too, not just the bottom.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
PageHeadergrowingso block padding only appears once the search bar outgrows the 60px bar.Test plan