Repository navigation
feat(app): reveal search row-selection checkboxes on hover - #3164
Conversation
The checkbox column was permanently visible on every search row, spending attention and a glyph per row on a feature most users will not reach for. Fade it in on row hover or checkbox focus instead; any selection puts the table in selection mode and reveals all of them, so shift-click ranges stay aimable. The cell keeps its width in every state so the data columns never reflow, and touch devices keep the checkbox visible. Co-authored-by: Mike Shi <mike@hyperdx.io>
🦋 Changeset detectedLatest commit: 31cf6df 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}
| test( | ||
| 'reveals a row checkbox only while the row is hovered', | ||
| { tag: ['@local-mode'] }, | ||
| async () => { | ||
| const checkbox = searchPage.table.getRowCheckboxCell(0); | ||
| await expect(checkbox).toHaveCSS('opacity', '0'); | ||
|
|
||
| await searchPage.table.hoverRow(0); | ||
| await expect(checkbox).toHaveCSS('opacity', '1'); | ||
|
|
||
| await searchPage.table.hoverRow(3); | ||
| await expect(checkbox).toHaveCSS('opacity', '0'); | ||
| }, | ||
| ); | ||
|
|
||
| test( | ||
| 'shows every checkbox while a selection exists', | ||
| { tag: ['@local-mode'] }, | ||
| async () => { | ||
| // Row 3 is never hovered here, so it can only be visible because the | ||
| // table is in selection mode — which is what keeps shift-click aimable. | ||
| const unhovered = searchPage.table.getRowCheckboxCell(3); | ||
| await expect(unhovered).toHaveCSS('opacity', '0'); | ||
|
|
||
| await searchPage.table.selectRows([0]); | ||
| await expect(unhovered).toHaveCSS('opacity', '1'); | ||
|
|
||
| await searchPage.clearRowSelection(); | ||
| await expect(unhovered).toHaveCSS('opacity', '0'); |
There was a problem hiding this comment.
The new tests cover mouse hover and active selection, but not the keyboard :focus-within or touch (hover: none) fallbacks added by this change. A regression in either path could leave row-selection controls invisible for keyboard or touch users while this suite remains green. Add coverage that focuses a checkbox by keyboard and checks initial visibility in a touch-capable browser context.
Knowledge Base Used: Development and test workflows
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!
E2E Test Results✅ All tests passed • 377 passed • 1 skipped • 1370s
Tests ran across 4 shards in parallel. |
Clicking a checkbox leaves DOM focus on it, so the :focus-within reveal rule pinned it visible on the row the user had just unchecked. Match :focus-visible instead, which a mouse click does not set. Also drop the glyph from 16px to 14px, trimming the column from 24px to 18px. Co-authored-by: Mike Shi <mike@hyperdx.io>
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
|
Deep Review✅ No critical issues found. This is a self-contained, presentation-only change (opacity-based hover reveal + a slimmer checkbox). The 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Pre-existing (not counted toward verdict)
Reviewers (9): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, previous-comments, kieran-typescript, julik-frontend-races. 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}
| await expect(unhovered).toHaveCSS('opacity', '1'); | ||
|
|
||
| await searchPage.clearRowSelection(); | ||
| await expect(unhovered).toHaveCSS('opacity', '0'); |
There was a problem hiding this comment.
🔵 minor — Final assertion in 'shows every checkbox while a selection exists' depends on where the menu leaves the mouse
clearRowSelection() (SearchPage.ts:233) opens the header menu and clicks row-selection-clear — the 4th Menu.Item in RowSelectionMenu.tsx, in a position="bottom-end" dropdown that overlays the row list. When the menu unmounts the pointer is left resting on whichever row sits at that y, so expect(unhovered).toHaveCSS('opacity', '0') for row 3 passes only because the item happens to land near row 5 rather than row 3; a row-height or menu-padding change silently flips it into a failure. Add await page.mouse.move(0, 0) after clearRowSelection() so the assertion tests selection mode rather than menu geometry.
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
| <Checkbox | ||
| className={styles.selectCheckbox} | ||
| size="xs" | ||
| size={`${CHECKBOX_SIZE}px`} |
There was a problem hiding this comment.
🔵 minor — Pointer target shrinks to 14×14 while the surrounding cell stays inert
Mantine renders the Checkbox root as a plain div (no wrapping <label> when label is undefined), so the only clickable element is the <input> sized by --checkbox-size — the 18px td around it does nothing. The target therefore goes from 16×16 to 14×14 on a control that is now also invisible until hovered, which compounds aiming difficulty. Keep the 14px glyph but make .selectCell itself the hit area (a <label htmlFor> filling the cell, or styles={{ input: { margin: 2 } }}-style padding) so the clickable region matches the reserved column.
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.
^ I feel as though this one is worth addressing as it seems we've lost 2px in the clickable area of the checkbox.
Worth updating:
const CHECKBOX_SIZE = 16; // column: 20px, down from 24px
| // checkbox carries in `.selectCheckbox`. | ||
| const CHECKBOX_SIZE = 14; | ||
|
|
||
| export const ROW_SELECTION_COLUMN_WIDTH = CHECKBOX_SIZE + 4; |
There was a problem hiding this comment.
🔵 minor — Column width hand-syncs a 4px gap that actually lives in SCSS
ROW_SELECTION_COLUMN_WIDTH = CHECKBOX_SIZE + 4 encodes in TS the padding-left: 4px declared on .selectCheckbox in packages/app/styles/LogTable.module.scss:100-104; editing either one alone clips the glyph or leaves dead space, and nothing pins them together. It also means the 18px cell's content box is exactly 14px, so justify-content: center no longer centers anything and the checkbox now sits flush against the expand-chevron cell (previously ~2px of right gap). Drive the gap from one place — drop the padding-left and let the flex centering do the work, or expose it as a CSS custom property the TS constant reads.
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
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
…3117) Resolves HDX-5365. > **Stacked on #3171.** Base is `cursor/fix-virtualized-row-height-d7eb`; it will retarget to `main` once that merges. See "Virtualized row heights" below. ## Why The only way to expand a search result row inline is the 16px chevron, which is a poor hit target when you are scanning logs during an incident. Comparable tools expand on row click. Rather than change what a row click does for everyone, this adds a preference. **The default is unchanged** — a row click still opens the side panel. Users who want the bigger hit target can opt in. ## What changed **Preferences → Search results → Row click**, defaulting to `Open side panel`. Set it to `Expand inline` and: - a row click toggles inline expansion instead of opening the side panel; - the side panel moves to a hover button on the row (`Open in side panel`, beside the existing copy buttons), and the expanded row's maximize button still opens it; - row clicks keep driving the side panel while it is open, so browsing row to row inside the panel is unchanged. Closing the panel returns clicks to inline expansion. Stored as an optional `rowClickAction`. Stored preferences that predate the field are returned as-is rather than merged with the defaults, so readers fall back to `DEFAULT_ROW_CLICK_ACTION`. Two table variants need care regardless of the preference: - Tables that hide the expand column (surrounding context, patterns) have nowhere to put inline details, so they always open the side panel. - The ClickHouse dashboard's slowest-queries list renders inline details but passes no `onRowDetailsClick`, so it always expands inline and offers neither the hover button nor the expanded row's maximize button — both would only write URL params nothing reads. Routing a row click needs to know whether a side panel is available and whether one is already open, and two signals did not actually mean that: - `highlightedLineId` was passed to the table even when the panel was not mounted. `rowWhere`/`rowSource` survive a source switch while closing the panel clears both, so a stale `rowWhere` left the table highlighting a row with no panel on screen. The panel is now hoisted into one `sidePanel` element that both its own mount and `highlightedLineId` derive from. - `ExpandedLogRow` gated its maximize button on `{openSidebar && …}`, the return value of a `useCallback` and so always truthy. It now takes `canOpenSidePanel`. `DBRowTableIconButton` also gained an `aria-label` from its title, which the row hover buttons had been missing. ## Virtualized row heights Reported against this branch: expand and collapse a few rows, scroll, and the table goes blank under the header. That bug is on `main` today — it reproduces identically through the chevron, with this PR's `packages/app/src` reverted. A row and its inline expansion are two `tr`s sharing one virtual index, and both were measured, so the expanded one took over that index's `ResizeObserver` registration and left its height cached there after collapsing. Every expand/collapse shrank the render window. Fixed separately in **#3171** so it lands for chevron users regardless of this PR, and merged in here so this branch does not ship the bigger hit target on top of it. That merge is the only reason `DBRowTable.tsx` now renders a `tbody` per row. ## Tests - Seven unit tests in `DBRowTable.test.tsx`: the side panel on row click by default (and no redundant hover button in that mode), then under `expand` — inline expand/collapse on body click, the hover button, `showExpandButton={false}`, and the open-panel case; plus the no-side-panel table expanding inline regardless of the preference. These are the first tests in that file to render virtualized body rows, hence the geometry mocks. - The inline-expand test also asserts the expanded row exposes `Open in side panel` and that clicking it writes the `rowWhere` param, so `canOpenSidePanel` is pinned in both directions — hardcoding it to `false` fails the test rather than staying green. - `UserPreferencesModal.test.tsx` drives the select: the default for users with no stored preference, a stored value round-tripping into the control, and picking an option writing `rowClickAction`. A wrong storage key or option value now fails instead of passing green. - E2E: the specs that reach the side panel go back to a plain row click, since that is the default again. A new `Row click set to expand inline` describe seeds the preference before navigating and asserts a row body click expands inline without opening the panel, collapses on a second click, and that the hover button opens the panel. - `virtualized-row-heights.spec.ts` comes in from #3171 and covers the virtualizer regression. ## Rebase onto `main` Rebased past #3140 and #3164, which added row multi-select and hover-revealed selection checkboxes to the same row component. No textual conflicts, and the two features compose: the checkbox lives in its own cell and stops propagation, so it does not compete with the row-body click. CI lint was failing only because this branch predated the `--max-warnings` bump on `main`. The rebase picks that up; the one warning this PR did add (an unsafe cast in the new `Select` handler) is gone, so the app count is back under the ceiling rather than exactly on it. Verification: `make ci-lint` and `make ci-unit` green (244 suites, 3948 tests after the merge). All 94 `search/` E2E tests re-run — 85 passed, 1 flaky-then-passed, and 2 `search-filters.spec.ts` failures that reproduce identically with this PR's `packages/app/src` reverted to `origin/main`, so they are environmental and not from this change. `correlated-metric-source`, `span-links` and `traces-workflow` also pass. ## Walkthrough The reported repro, end to end on this branch: two row clicks opening the side panel on the default setting, switching the preference to `Expand inline`, expanding and collapsing six rows, then scrolling — the table stays full. <a href="https://cursor.com/agents/bc-b2c91bdd-e466-447c-942d-dce15159d7eb/artifacts?path=%2Fopt%2Fcursor%2Fartifacts%2Fvirtualized_scroll_repro_fixed.mp4"><img src="https://cursor.com/artifacts/c/art-27481ff4-27fa-4bca-bbe1-3c2abee4f889" alt="virtualized_scroll_repro_fixed.mp4" /></a> Default — a row click opens the side panel, unchanged:  The opt-in:  With it on, a row click expands in place, with no side panel:  Hovering a row shows both features side by side — the selection checkbox on the left, and this PR's `Open in side panel` button among the row action icons on the right:  Linear Issue: [HDX-5365](https://linear.app/clickhouse/issue/HDX-5365/expand-log-inline-on-row-click) <div><a href="https://cursor.com/agents/bc-b2c91bdd-e466-447c-942d-dce15159d7eb?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-b2c91bdd-e466-447c-942d-dce15159d7eb&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> Co-authored-by: Cursor Agent <199161495+cursoragent@users.noreply.github.com>
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
Closes HDX-5429.
Why
#3140 shipped row multi-select with a checkbox on every row, always. Search is a read-and-navigate surface, not a bulk-action one, so the control should be as out of the way as it is on Linear, Notion or Drive rather than as present as it is on Gmail or GitHub issues.
What changed
:has(:focus-visible), so tabbing to it does not land on something invisible. Not:focus-within: a mouse click leaves focus on the checkbox, which would strand it on screen after the row is unchecked..selectionActiveon<table>, driven byselectedCount > 0) and reveals every checkbox — shift-click needs visible endpoints, and this is also what keeps a checked box visible.@media (hover: none): always visible, since touch has no hover.xs, trimming the column from 24px to 18px.Before this change every row looked like the third frame at all times, with a wider checkbox column.
Check, uncheck, walk away — nothing is left stranded on the unchecked row:
(The white dot in both recordings is a synthetic cursor drawn for the capture; the columns hold still throughout.)
Trade-offs
The column keeps its width. The issue floated folding the control into the row's hover affordances to reclaim the column entirely. Any zero-width reveal has to draw over something — the expand chevron sits in the next 32px cell and the timestamp starts at the edge of the one after it — and covering either on hover is worse than the pixels are worth. So this takes the issue's other sanctioned option: reserve the space, fade the glyph, and shrink what is reserved. Nothing reflows on hover.
Discoverability. Hover reveal makes the feature findable by pointing at a row, which is where a user already is. The checkbox now carries
title="Select row, shift-click to select a range"so the range behaviour is not something you have to guess — the reviewer on #3140 did not know it was there.Testing
Three E2E tests added to
row-multi-select.spec.ts: the checkbox is transparent, opaque while its row is hovered, transparent again once the pointer moves on; a never-hovered row's checkbox is visible while a selection exists and hidden after clearing; and a checked-then-unchecked row drops its checkbox once the pointer leaves, while shift-tabbing to it still reveals it. That last one was confirmed to fail against:focus-withinand pass against:focus-visible. All 13 tests in the file pass, along withmake ci-lintand the app unit suite (3938 tests).Linear Issue: HDX-5429