Repository navigation
[HDX-5307] Drop the search bar WHERE label and / keycap - #3152
elizabetdev merged 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f2401b2 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 |
|
| const selectBlurred = await searchPage.getSelectBorderColor(); | ||
| await searchPage.getSELECTEditor().focus(); | ||
| await expect | ||
| .poll(() => searchPage.getSelectBorderColor()) | ||
| .not.toBe(selectBlurred); | ||
|
|
||
| const orderByBlurred = await searchPage.getOrderByBorderColor(); | ||
| await searchPage.getOrderByEditor().focus(); | ||
| await expect | ||
| .poll(() => searchPage.getOrderByBorderColor()) | ||
| .not.toBe(orderByBlurred); |
There was a problem hiding this comment.
The query language persists in localStorage. When it is SQL, the WHERE field becomes the first CodeMirror editor, so these page-global index locators focus WHERE and SELECT instead of SELECT and ORDER BY. The test then polls borders on different fields and times out rather than validating the intended focus states. Use the existing label-scoped clause fields, whose targets do not depend on editor order.
| const selectBlurred = await searchPage.getSelectBorderColor(); | |
| await searchPage.getSELECTEditor().focus(); | |
| await expect | |
| .poll(() => searchPage.getSelectBorderColor()) | |
| .not.toBe(selectBlurred); | |
| const orderByBlurred = await searchPage.getOrderByBorderColor(); | |
| await searchPage.getOrderByEditor().focus(); | |
| await expect | |
| .poll(() => searchPage.getOrderByBorderColor()) | |
| .not.toBe(orderByBlurred); | |
| const selectBlurred = await searchPage.getSelectBorderColor(); | |
| await searchPage.selectClauseField().focusTarget.focus(); | |
| await expect | |
| .poll(() => searchPage.getSelectBorderColor()) | |
| .not.toBe(selectBlurred); | |
| const orderByBlurred = await searchPage.getOrderByBorderColor(); | |
| await searchPage.orderByClauseField().focusTarget.focus(); | |
| await expect | |
| .poll(() => searchPage.getOrderByBorderColor()) | |
| .not.toBe(orderByBlurred); |
Deep Review✅ No critical issues found. This is a clean, mechanical removal: the 🟡 P2 -- recommended
🔵 P3 nitpicks (1)
Reviewers (6): correctness, testing, maintainability, kieran-typescript, julik-frontend-races, previous-comments. Testing gaps:
|
| renderWithMantine(<TestWrapper defaultLanguage="sql" />); | ||
|
|
||
| expect(screen.getByText('WHERE')).toBeInTheDocument(); | ||
| expect( |
There was a problem hiding this comment.
🔵 minor — No test pins the removal of the WHERE label, the PR's main change
The label only ever rendered in SQL mode (it was passed to SQLInlineEditorControlled), and this diff deletes both tests that covered it (renders SQL input with WHERE label and hides label when showLabel is false). The replacement asserts the language combobox reads SQL and that the Lucene placeholder is absent — both true whether or not a WHERE label renders — so reintroducing the label leaves the suite green. Add expect(screen.queryByText('WHERE')).not.toBeInTheDocument(); to this SQL-mode test. (The new kbd assertion at line 92 lives in the Lucene block, and the keycap rendered in both languages, so it likewise wouldn't catch a SQL-only regression.)
| { name: 'SQL', select: (p: SearchPage) => p.switchToSQLMode() }, | ||
| ]; | ||
|
|
||
| test('marks the WHERE input as focused in both languages', async ({ |
There was a problem hiding this comment.
🔵 minor — New E2E spec never exercises the behaviour this PR claims to preserve
Both tests assert focus border colors on WHERE/SELECT/ORDER BY — the behaviour of the stacked "always-visible query fields" change, not of this diff — while the PR's own test-plan line ("/ and s still focus the input") has no automated coverage in either the unit test or here. Since this diff deletes the keycap that advertised the shortcut, add a case that presses / (and s) on the search page and asserts the WHERE input holds focus: await page.keyboard.press('/') then assert on searchPage.whereInput.luceneInput / .sqlEditor being focused. The hotkey lives in SearchInputV2.tsx:138 and SQLInlineEditor.tsx:230, gated on enableHotkey, and nothing currently tests either.
| await searchPage.goto(); | ||
|
|
||
| const selectBlurred = await searchPage.getSelectBorderColor(); | ||
| await searchPage.getSELECTEditor().focus(); |
There was a problem hiding this comment.
🔵 minor — Focuses the editor by .cm-content index but measures the border by label
getSELECTEditor()/getOrderByEditor() (SearchPage.ts:349,395) resolve .cm-content by position, while getSelectBorderColor()/getOrderByBorderColor() (SearchPage.ts:353,359) resolve the Paper from the SELECT/ORDER BY label — so the element focused and the element measured are located two different ways in the same assertion, and SearchPage.ts:365-371 documents the index lookup as shifting with the WHERE language. Use the label-based field the page object already provides (and that features/shared/multiline.spec.ts:63-66 uses): await searchPage.selectClauseField().focusTarget.focus() and await searchPage.orderByClauseField().focusTarget.focus().
| const searchPage = new SearchPage(page); | ||
| await searchPage.goto(); | ||
|
|
||
| for (const language of languages) { |
There was a problem hiding this comment.
🔵 minor — languages[].name is never read, so a failure can't say which language broke
language.name (lines 12-13) has no reader — only language.select is used — and both languages are asserted inside one test, so a failure in the SQL iteration reports only marks the WHERE input as focused in both languages. Either wrap the body in await test.step(language.name, async () => { ... }), or parametrize with for (const language of languages) test(\marks the WHERE input as focused in ${language.name}`, ...)asmultiline.spec.ts:39-53` does, so the name earns its keep.
PR Review1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor 1 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
The label repeated the SQL placeholder, and the overlay clipped long queries. Keyboard shortcuts still focus the input. Co-authored-by: Cursor <cursoragent@cursor.com>
1709563 to
f2401b2
Compare
|
|
||
| expect(screen.getByText('WHERE')).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByRole('combobox', { name: 'Query language' }), |
There was a problem hiding this comment.
🔵 minor — Rewritten SQL-mode test asserts nothing about the SQL editor it is named for
The old assertion (getByText('WHERE')) proved the SQL branch rendered, because the label lived inside SQLInlineEditor. The replacements do not: the Query language combobox renders in both languages and its value comes straight from the form default the test itself set, and the missing Lucene placeholder is only an absence. Replacing isSql ? <SQLInlineEditorControlled …/> with isSql ? null in SearchWhereInput.tsx:228 keeps this test green. Assert on the SQL editor itself — e.g. expect(container.querySelector('.cm-editor')).toBeInTheDocument() — or fold this case into the neighbouring renders SQL placeholder test, which is currently the only thing covering that branch.
62780c7
into
elizabet/hdx-5307-always-visible-query-fields
## 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
WHERElabel and the/keycap overlay./andsstill focus the input. Adds focus-border E2E coverage.Test plan
WHERElabel./andsstill focus the input with no keycap overlay.make dev-e2e FILE=input-focus