Repository navigation
[HDX-5307] Keep multi-line query fields visible when unfocused - #3132
elizabetdev wants to merge 22 commits into
Conversation
…urred The search bar collapsed to a single line whenever it lost focus, so anything past the first line of a multi-line query was hidden until you clicked back into it. Focusing it expanded the bar into an absolutely positioned overlay that covered the results underneath. The bar now sizes to its content and stays that way, growing the row instead of overlaying it, up to four lines in Lucene and 150px in SQL before it scrolls. The language switch stretches to match so the seam between the two stays flush, and the `/` hint stays on the first line. Staying expanded is opt-in via a new `keepExpanded` prop rather than the default for every inline SQL editor: SELECT and ORDER BY sit in narrow boxes where the default value wraps, and expanding them permanently would have tripled the height of the search page header. Co-authored-by: Cursor <cursoragent@cursor.com>
The time picker, live tail refresh interval and Run button were centered against the search bar, so they drifted down the row as a multi-line query made it taller. Align them to the top instead, level with the bar's first line. Same fix on the sessions and services toolbars, which centered for the same reason. The service map and LLM dashboard already used flex-start, and the dashboard toolbar stretches, so both were already correct. Co-authored-by: Cursor <cursoragent@cursor.com>
The label repeated what the SQL placeholder already says while taking
horizontal room the query itself can use, which matters more now that a
long query wraps instead of being cut off.
Most embedders already passed showLabel={false} and labelled the input
themselves, so the props are removed rather than defaulted off. The
multiline E2E spec located the SQL editor by that label; it now goes
through the where-language-switch testid, the same handle the chart
editor page object uses.
Co-authored-by: Cursor <cursoragent@cursor.com>
At the default size the Lucene input reserved a 38px row but drew a 36px bordered box inside it, so the language switch stretched to the taller row and stood 2px proud of the input at both ends. The switch was sized to 36px to match the SQL editor, which is the height the bar actually uses. Both languages now derive their height from one number per size, so the seam is flush either way and switching language no longer resizes the bar. The inner textarea's min-height comes from the same variable that reserves the row, which is what had drifted: it was set in SearchWhereInput while the row height came from AutocompleteInput. Only the default size was wrong; xs already agreed at 30px. The seam is checked in the multiline spec at one line of content, the only state where the reserved height binds — an empty input's placeholder wraps at narrow widths and the content drives the height instead. Co-authored-by: Cursor <cursoragent@cursor.com>
/ and s still focus the input. The keycap was clipping long queries and is already listed in the keyboard shortcuts modal. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9bd39ba 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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 364 passed • 1 skipped • 988s
Tests ran across 4 shards in parallel. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review6 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 6 minor 4 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. |
Deep ReviewScope: PR #3132 — keep multi-line query fields (SQL WHERE/SELECT/ORDER BY, PromQL, Lucene) visible when unfocused; remove the expand-on-focus overlay and grow the layout instead. ~950 lines across This PR has been through 16 prior review passes. The orchestrator independently re-verified every previously-flagged concern against current code before grading. 8 of 9 substantive prior concerns are resolved in the current diff ( ✅ No critical issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (3): previous-comments, agent-native, learnings-researcher (correctness, adversarial, kieran-typescript, frontend-races, testing, maintainability, and project-standards were dispatched but had not returned at finalization; contested areas were independently re-verified by the orchestrator). Testing gaps: Always-expanded-after-blur behavior for embedded (non-query-bar) form WHERE fields is not covered by the new e2e specs, which exercise the search/chart query bars; the |
The app's lint budget is `--max-warnings 568` and this assertion put it at 569. querySelector returns an Element, which already has contains() and getBoundingClientRect(), so the cast bought nothing. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Thread allowMultiline through SearchInputV2 so single-line Lucene filters stay one row. Keep the error border while focused, wait for blur before asserting height, and pin the Apply filters switch to the action bar's first line. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- keep single-line Lucene filters readable without submitting Shift+Enter - align validation borders, indicators, and growing editor rows - remove stale layout styles and position-independent E2E locators Co-authored-by: Cursor <cursoragent@cursor.com>
…tiline-always-visible-search-bar
- keep allowMultiline=false inputs one row focused and blurred - align chart labels with the first editor row - reuse the shared WHERE input page object Co-authored-by: Cursor <cursoragent@cursor.com>
| <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 — The editor's base height is re-asserted by hand at nine call sites
<Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> is repeated verbatim nine times across six files (here, ChartEditorControls.tsx:336/360, ChartSeriesEditor.tsx:525/550/576, HighlightedAttributes.tsx:89/98, MaterializedViews.tsx:576, OrderByFormRow.tsx:92) to keep a label or icon level with the neighbouring editor's first line. All nine hardcode .sm while the editors next to them infer their height from size, so the two must be kept in sync by hand: adding size="xs" to the ORDER BY editor below this line would silently leave the filters toggle 6px low, with nothing to catch it. Extract one small wrapper (taking the same size the editor takes, or a class keyed off the --editor-base-height the editor already publishes) and use it at all nine sites.
allowMultiline={false} stays one row, so wrapping plus overflow:hidden hid
the rest of a long query. Match SQL: no wrap, overflow-x. E2E now types a
wrapping value into the waterfall filter in both languages and asserts the
height after blur.
Co-authored-by: Cursor <cursoragent@cursor.com>
| expect(await settledHeight(visibleBox)).toBeLessThanOrEqual(oneRowHeight + 1); | ||
|
|
||
| await blurActiveElement(page); | ||
| await expect(focusTarget).not.toBeFocused(); | ||
|
|
||
| expect(await settledHeight(visibleBox)).toBeLessThanOrEqual(oneRowHeight + 1); |
There was a problem hiding this comment.
The new helper checks only the visible box's height. Lucene already uses maxRows={1}, so reverting the new no-wrap CSS would still pass these assertions while wrapping and hiding the rest of the query. Add an assertion that the long value does not overflow vertically, alongside the height checks, so the test detects the clipping regression it is intended to prevent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| width, | ||
| maxWidth, | ||
| minWidth, | ||
| ['--editor-base-height' as string]: `${baseHeight}px`, |
There was a problem hiding this comment.
The new CSS custom-property entry widens its key with as string, contrary to the repository directive to avoid casts when type inference is possible. Define the custom-property styles in an inferred object and spread that object into style instead, preserving the property name without an assertion. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| {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 copy-pasted ten times across six files
The same "centre this control on the editor's first line" wrapper is now hand-rolled at ChartSeriesEditor.tsx:525,550,576, ChartEditorControls.tsx:336,360, ChartActionBar.tsx:160, HighlightedAttributes.tsx:89,98, MaterializedViews.tsx:576 and OrderByFormRow.tsx:92, each hard-coding the sm variant. Every one of them silently desyncs if the editor beside it is ever given size="xs" (30px), and the repo conventions make grepping for an existing component before adding a new one REQUIRED. Extract one small component next to editorInputHeights.ts (e.g. <EditorFirstLineSlot size="sm">{children}</EditorFirstLineSlot>, or a shared class keyed off --editor-base-height so it inherits the height the editor already publishes) and use it at all ten sites.
Restore shared headers to horizontal padding only so title-only pages stay a flush 60px bar. Sessions opts into growing so the multi-line search still gets room once it outgrows the toolbar. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the P2 from the deep review: shared |
| wrap="nowrap" | ||
| flex={1} | ||
| // Stays level with the search bar's first line as it grows | ||
| align="flex-start" |
There was a problem hiding this comment.
🔵 minor — align="flex-start" top-aligns the 34px ActionIcons against 36px toolbar controls
Mantine Group defaults to align="center", which was centering the two size="lg" ActionIcons (34px, lines 385 and 400) against the 36px SearchWhereInput/TimePicker/Run button. With flex-start they now sit flush to the top and 2px short at the bottom — the same 2px seam this PR removes elsewhere. KubernetesFilters.tsx:325 has the same shape (28px FilterLinkToggle ActionIcon vs 30px size="xs" selects). Wrap the shorter controls the way ChartActionBar.tsx:160 already does in this diff: <Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center">…</Flex> (.xs for the Kubernetes row).
| } | ||
|
|
||
| private clauseField(label: 'SELECT' | 'ORDER BY'): MultilineField { | ||
| const paper = borderedBox(this.page.getByText(label, { exact: true })); |
There was a problem hiding this comment.
🔵 minor — clauseField() label lookup isn't scoped to the search form and also matches the query-error panel
this.page.getByText(label, { exact: true }) matches page-wide, and DBSearchPage.tsx:2674 / :2683 render a second <Text>SELECT</Text> and <Text>ORDER BY</Text> inside the hasQueryError panel (itself wrapped in a Paper, so borderedBox would resolve to the wrong box). Any test run where the query errors turns the new multiline/input-focus assertions into a strict-mode violation instead of a real failure. Scope it: borderedBox(this.searchForm.getByText(label, { exact: true })).
| const selectBlurred = await searchPage.getSelectBorderColor(); | ||
| await searchPage.getSELECTEditor().focus(); | ||
| await expect | ||
| .poll(() => searchPage.getSelectBorderColor()) |
There was a problem hiding this comment.
🔵 minor — New focus test locates the editors by .cm-content index, which the PR's own helper exists to avoid
It focuses searchPage.getSELECTEditor() (.cm-content first) and getOrderByEditor() (.nth(1)) while asserting on the label-located Paper from selectClauseField()/orderByClauseField(). SearchPage.ts:366-370 documents why the index is wrong: in SQL mode the WHERE input is a CodeMirror editor and comes first — and the search page opens in SQL whenever getStoredLanguage() returns 'sql' (DBSearchPage.tsx:1235, backed by the hdx-search-where-language localStorage key). Then the test focuses the WHERE input and polls a SELECT border that never changes. Focus selectClauseField().focusTarget / orderByClauseField().focusTarget instead.
| </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 36px label/icon row is hand-rolled nine times and duplicates FormRow's existing one
<Flex h={EDITOR_INPUT_HEIGHTS.sm} align="center"> is added nine times across this diff (here, HighlightedAttributes.tsx:89,98, MaterializedViews.tsx:576, ChartEditorControls.tsx:336,360, ChartSeriesEditor.tsx:525,550,576, ChartActionBar.tsx:160) — and this instance sits inside a FormRow, whose label column already implements the identical pattern (FormRow.tsx:16-24: outer align="flex-start" + a height: '36px' label box). Extract one small component (e.g. EditorRowAligned) and change FormRow.tsx:23 to EDITOR_INPUT_HEIGHTS.sm, so the literal and the constant can't drift apart.
## Summary
- Fixes HDX-5307: SQL, Lucene, and PromQL query fields stay fully visible when unfocused, growing the layout instead of collapsing or floating an overlay.
- Shared `EDITOR_INPUT_HEIGHTS` keeps the language switch flush with the input. `allowMultiline={false}` stays one row (Lucene scrolls horizontally).
- Stacked follow-ups: toolbar alignment, search-bar chrome. Split from #3132.
## Test plan
- [ ] On Search, type a two-line SQL and Lucene query, then click away: both lines stay visible and the bar grows the layout.
- [ ] SELECT and ORDER BY stay expanded after blur.
- [ ] On a source or chart form SQL field, a wrapped value stays visible after blur.
- [ ] `allowMultiline={false}` (trace spans filter) stays one row after blur.
- [ ] `make dev-e2e FILE=multiline`
Summary
SQLInlineEditor(search, source and chart forms) andPromQLEditor.position: absolutetook the field out of the layout flow, so in a container sized to its content (a Storybook story, for one) the field collapsed to ~6px on focus and wrapped one character per line.allowMultiline={false}stays single-line in both languages (and hides the empty-placeholder scrollbar).WHERElabel and the/keycap overlay are gone (/andsstill focus the input).Test plan
Components/SQLInlineEditorkeeps its width and renders on one line when focused.WHERElabel;/andsstill focus the input with no keycap overlay.make dev-e2e FILE=multilineandmake dev-e2e FILE=input-focus