Repository navigation
[HDX-5307] Keep multi-line query fields visible when unfocused - #3150
Conversation
Grow SQL, Lucene, and PromQL fields in layout instead of collapsing to one line or floating an overlay. Shared editor heights keep the language switch flush with the input. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: f0f46bb 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.
|
🟡 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}
| placeholder?: string; | ||
| onLanguageChange?: (language: 'sql' | 'lucene') => void; | ||
| language?: 'sql' | 'lucene'; | ||
| onSubmit?: () => void; |
There was a problem hiding this comment.
Removing language from SQLInlineEditorProps leaves the unchanged QueryExpressionFilterEditForm caller passing language="sql" to SQLInlineEditorControlled. Because the controlled component derives its accepted props from this interface, TypeScript reports the extra property and the app type-check cannot pass until the caller is updated.
E2E Test Results✅ All tests passed • 364 passed • 1 skipped • 1302s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. This PR replaces the focus-time 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (2): previous-comments, orchestrator diff analysis (correctness, maintainability, and CSS-cascade tracing performed directly against the diff). Testing gaps:
Coverage note: The correctness, kieran-typescript, julik-frontend-races, maintainability, and testing reviewer sub-agents were still running when output was requested; the findings above come from the completed previous-comments reviewer and direct orchestrator analysis of the full production diff, CSS specificity, prop flow, and the e2e/unit test files. No P0/P1 defects were found in that analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Border colors of the two halves of the WHERE control. They sit flush | ||
| * against each other, so a focus state on only one reads as half-focused. | ||
| */ | ||
| async getWhereBorderColors(): Promise<{ |
There was a problem hiding this comment.
🔵 minor — Border-color test helpers have no callers, and the focus/validation recolor they were written for is untested
SearchPage.focusWhereInput (331), getWhereBorderColors (339), getSelectBorderColor (353), getOrderByBorderColor (359), WhereInputComponent.borderColors() and WhereInputComponent.focus() are referenced nowhere in packages/app/tests/e2e — readSeam's borderColor half exists only to feed them, and expectSeamFlush uses only the heights. Meanwhile the changeset's headline claim ("Focus recolors the whole control, including the language switch, without overriding an error or warning border") is exactly what the new .root:has(.inputWrapper :focus) .languageSwitch / [data-validation-state] rules in SearchWhereInput.module.scss:55-65 implement, and nothing asserts it. Either add a spec that focuses the WHERE input and asserts languageSwitch and input border colors match (and that an error/warning state wins over focus), or delete the helpers.
| await page.keyboard.press('ControlOrMeta+A'); | ||
| await page.keyboard.type(WRAPPING_SINGLE_LINE_QUERY); | ||
|
|
||
| expect(await settledHeight(visibleBox)).toBeLessThanOrEqual(oneRowHeight + 1); |
There was a problem hiding this comment.
🔵 minor — expectStaysOneRow cannot fail in SQL mode — it measures a box with a hard max-height
For SQL, WhereInputComponent.field('SQL') sets visibleBox to borderedBox(this.sqlEditor) — the Paper, which SQLInlineEditor.tsx:356 now gives styles.clamped (max-height: var(--editor-base-height), overflow: hidden, SQLInlineEditor.module.scss:27-30). So settledHeight(visibleBox) <= oneRowHeight + 1 holds by construction even if a regression made the content wrap to two lines and clipped the second. Also assert the growth box stays one row (e.g. .cm-line count === 1 / growthBox height unchanged), which is the property allowMultiline={false} is actually about.
PR Review
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor 1 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines
1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
…ders The dashboard filter expression editor still passed the removed `language` prop, which failed the app type-check. Removing the mirror dead switch in AutocompleteInput/SearchInputV2 too, folding the validation-state ladder into variableValidation, and moving the focus-border spec here so the recolor it asserts ships with its test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review in 483d5d2. P1 (build break) — P2 (unused border helpers) — P3 / minor — also in this commit:
Not doing: a Lucene Stacked PRs #3151 and #3152 were rebased on this. 3905 app unit tests pass. |
| getSELECTEditor() { | ||
| return this.page.locator('.cm-content').first(); |
There was a problem hiding this comment.
In SQL mode, the WHERE field adds a CodeMirror editor before SELECT and ORDER BY. These global .cm-content indices therefore make getSELECTEditor() return WHERE and getOrderByEditor() return SELECT. The new focus and multiline tests exercise the wrong fields and can pass without covering ORDER BY, weakening this regression coverage.
| autosize | ||
| minRows={1} | ||
| maxRows={isSearchInputFocused ? 4 : 1} | ||
| maxRows={allowMultiline ? maxVisibleRows : 1} |
There was a problem hiding this comment.
🟠 major — An empty field now grows to fit its wrapped placeholder, so a narrow search box renders 2+ rows tall before the user types anything
react-textarea-autosize measures node.value || node.placeholder, so with maxRows no longer clamped to 1 while unfocused, an empty input sizes to the placeholder's wrapped height. KubernetesFilters.tsx:338 gives the Lucene input minWidth: 200 with the placeholder "Search your events w/ Lucene ex. column:foo", which wraps at that width — the empty k8s filter bar is now double height. The same applies to SQL: SQLInlineEditor has EditorView.lineWrapping on whenever allowMultiline (line 282) and the old .paper:not(.expanded) { max-height; overflow: hidden } clamp is gone, so DBTracePanel.tsx:404 ("Log Trace ID Column (ex. trace_id)") and DBSearchPage.tsx:2282 (SELECT, placeholder = the full default select list) grow while still empty. The PR's own helper documents the mechanism (tests/e2e/utils/multiline-input.ts: "the placeholder wraps at narrow widths and the content then drives the height"), which is why onSingleLine types a character first — so no test covers the empty state. Gate growth on there being content: maxRows={allowMultiline && value ? maxVisibleRows : 1}, and for SQL re-apply styles.clamped when the value is empty.
| height: calc(var(--editor-base-height) - var(--editor-border-inset, 2px)); | ||
| } | ||
|
|
||
| .shortcutHint { |
There was a problem hiding this comment.
🔵 minor — The / hotkey hint floats to the vertical middle of a multi-line query instead of staying on the first row
.shortcutHint is position: absolute; top: 50%; transform: translateY(-50%) inside .inputWrapper, which used to be one row tall whenever the hint was visible (the expanded editor was an absolute overlay). Now that the input grows in flow, the 22px Kbd centres on the whole box: on the search page (DBSearchPage.tsx:2362, enableHotkey, size="xs" → 30px base) a two-line WHERE puts it straddling both lines at the right edge, on top of the query text. Pin it to the first row the same way variableValidation.module.scss does — replace the centring with top: 0; height: var(--editor-base-height); display: flex; align-items: center (drop the transform).
| field(language: QueryLanguage): MultilineField { | ||
| if (language === 'Lucene') { | ||
| const input = this.luceneInput; | ||
| return { focusTarget: input, growthBox: input, visibleBox: input }; |
There was a problem hiding this comment.
🔵 minor — For Lucene, visibleBox is the textarea rather than the bordered box, so the growth assertion never checks the element this PR resized
MultilineField.visibleBox is documented as "the bordered box the user sees, which must not clip the content", and expectExpandsAndStaysExpanded asserts it grows separately from growthBox. For Lucene both are the same textarea, making that assertion a duplicate of the growthBox one; the Mantine .mantine-InputWrapper-root — the box whose reserved-38px/drawn-36px mismatch this PR fixes, and the only thing that could clip the second line — is never measured after it grows (expectSeamFlush only runs at one line, via onSingleLine). Point visibleBox at the wrapper, e.g. input.locator('xpath=ancestor::div[contains(@class, "mantine-InputWrapper-root")][1]'), mirroring borderedBox on the SQL side.
|
|
||
| /* Single-line Lucene: one visual row, like SQL without line wrapping. A long | ||
| query scrolls horizontally instead of wrapping under the clip. */ | ||
| .root[data-single-line='true'] .textarea textarea { |
There was a problem hiding this comment.
🔵 minor — overflow: auto hidden on a 28px single-line textarea lets a classic horizontal scrollbar eat the text
With white-space: pre and overflow-x: auto, a query wider than the box shows a real horizontal scrollbar on Chrome/Linux and Windows (~15px). The height is pinned by min-height: calc(var(--editor-base-height) - 2px) !important — 28px at the xs size used by the trace spans/logs filters (DBTraceWaterfallChart.tsx:1292) — so the scrollbar leaves ~13px for a ~19px text row, and overflow-y: hidden clips the rest. The new expectStaysOneRow check only compares heights, so it passes either way. Use overflow: hidden instead: a textarea still scrolls horizontally to follow the caret without reserving scrollbar space.
| field(language: QueryLanguage): MultilineField { | ||
| if (language === 'Lucene') { | ||
| const input = this.luceneInput; | ||
| return { focusTarget: input, growthBox: input, visibleBox: input }; |
There was a problem hiding this comment.
🔵 minor — Lucene growth check compares the textarea against itself, so the anti-clipping assertion is vacuous
For Lucene, field() returns the same locator as both growthBox and visibleBox, but the bordered box the user sees is the Mantine InputWrapper root — .textarea carries the border and textarea { border: none !important } (AutocompleteInput.module.scss:16,23). So expectExpandsAndStaysExpanded's "The visible box has to actually grow, not clip the second line" check (utils/multiline-input.ts:143-146) compares the textarea to itself and would still pass if the wrapper clamped to one row — only the SQL arm of the test actually guards against clipping. Use the textarea's bordered wrapper (e.g. this.row.locator('.mantine-InputWrapper-root').first()) as visibleBox.
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 - Drop the redundant `WHERE` label and the `/` keycap overlay from the search bar. - `/` and `s` still focus the input. - Re-opened against `main` after #3152 merged into the stacked base branch instead of `main` (#3150 had already landed). ## Test plan - [ ] Empty SQL bar shows the placeholder and no `WHERE` label. - [ ] `/` and `s` still focus the input with no keycap overlay.
Summary
EDITOR_INPUT_HEIGHTSkeeps the language switch flush with the input.allowMultiline={false}stays one row (Lucene scrolls horizontally).Test plan
allowMultiline={false}(trace spans filter) stays one row after blur.make dev-e2e FILE=multiline