Repository navigation
feat(app): add a row-click preference to expand result rows inline - #3117
Conversation
🦋 Changeset detectedLatest commit: 466bca7 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}
E2E Test Results✅ All tests passed • 387 passed • 1 skipped • 1461s
Tests ran across 4 shards in parallel. |
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 an opt-in preference (default behavior is unchanged: a row click still opens the side panel), the type wiring is sound, and the new 🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (11): correctness, adversarial, testing, maintainability, project-standards, api-contract, kieran-typescript, julik-frontend-races, previous-comments, agent-native, learnings-researcher. Testing gaps:
Note: 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}
| * Click on a specific row. On tables that support inline expansion this | ||
| * expands the row in place; use {@link openRowSidePanel} for the side panel. | ||
| */ | ||
| async clickRow(index: number) { |
There was a problem hiding this comment.
🔵 minor — clickRow / clickFirstRow now have zero callers and silently mean the opposite of what specs used them for
After this diff every call site moved to openRowSidePanel / clickRowBody, so both helpers are unreferenced across the repo (grep for clickRow(/clickFirstRow( outside this file returns only MetricExplorer.test.tsx's unrelated local helper). They are now a trap: a future spec calling clickFirstRow to reach the side panel gets inline expansion instead — or the side panel, depending on whether rowWhere happens to be set. Delete both and let clickRowBody / openRowSidePanel be the API.
There was a problem hiding this comment.
Agreed and done in 85271bb — both helpers are deleted. Confirmed the only remaining clickRow( hits are MetricExplorer.test.tsx's unrelated local helper, so clickRowBody / openRowSidePanel are now the whole API.
| // Once the side panel is open it stays the active surface, so clicks keep | ||
| // moving it from row to row instead of expanding rows behind it. | ||
| const expandInlineOnRowClick = | ||
| prefersInlineExpand && (!canOpenSidePanel || highlightedLineId == null); |
There was a problem hiding this comment.
🔵 minor — highlightedLineId != null is a proxy for "the side panel is open", but it is non-null in states where no panel renders
highlightedLineId comes from DBSqlRowTableWithSidebar as rowId ?? undefined (the URL rowWhere param), while the panel itself only renders when sourceData && (rowSource === sourceId || !rowSource) (DBSqlRowTableWithSidebar.tsx:119). DBSearchPage never clears rowWhere/rowSource when the source changes, so landing on the search page with a stale ?rowWhere=…&rowSource=<other-source> shows no panel yet disables inline expansion — the first row click opens the panel instead of expanding. Derive the flag from whether the panel is actually mounted (e.g. pass an explicit isSidePanelOpen prop from DBSqlRowTableWithSidebar, which already computes that condition) rather than from highlightedLineId.
There was a problem hiding this comment.
Confirmed and fixed in 85271bb. The reachable path is switching source with a row panel open: onCloseSidebar clears both params, but a source switch clears neither, so rowSource goes stale, the panel unmounts, and rowWhere survives.
I fixed it in DBSqlRowTableWithSidebar rather than by adding an isSidePanelOpen prop, since the condition only needs to exist once. The panel is hoisted into a sidePanel element and both the mount and the highlight read from it:
const sidePanel =
sourceData && (rowSource === sourceId || !rowSource) ? (
<DBRowSidePanel ... />
) : null;
...
{sidePanel}
<DBSqlRowTable highlightedLineId={sidePanel ? (rowId ?? undefined) : undefined} ... />This keeps sourceData narrowed (a plain boolean would not), avoids duplicating the predicate, and needs no new prop on DBRowTable. It also fixes the pre-existing half of the bug you did not mention: the table was highlighting a row whose panel was not on screen.
| onClick={() => openSidebar(rowId, source?.id)} | ||
| title="Open in sidebar" | ||
| aria-label="Open in sidebar" | ||
| title="Open in side panel" |
There was a problem hiding this comment.
🔵 minor — Expanded rows still offer "Open in side panel" on tables that have no side panel
This PR deliberately hides the row hover button when onRowDetailsClick is absent so the ClickHouse dashboard's slow-query list doesn't "offer a dead action" (DBRowTable.tsx:1888), but ExpandedLogRow unconditionally renders the maximize button — and ClickhousePage.tsx:800 renders no DBRowSidePanel at all (no DBRowSidePanel/rowWhere reference anywhere in that file), so clicking it just writes rowWhere/rowSource into the URL and nothing opens. Now that a plain row click reaches this expanded row there, gate the button on the same signal — e.g. thread the table's canOpenSidePanel into ExpandedLogRow — instead of the always-truthy {openSidebar && …} guard on line 58.
There was a problem hiding this comment.
Correct, and fixed in 85271bb. {openSidebar && ...} guarded on the return value of a useCallback, so it was always truthy — ExpandedLogRow now takes canOpenSidePanel and DBRowTable passes the same flag that gates the row hover button, so the two agree.
Worth noting this was reachable before this PR too (the chevron already expanded rows on that table), just much harder to stumble into. Extended the no-side-panel unit test to assert the button is absent after expanding, not only before.
| description="What clicking a result row does. The chevron always expands inline." | ||
| > | ||
| <Select | ||
| data-testid="row-click-action-select" |
There was a problem hiding this comment.
🔵 minor — The new preference control is untested and its data-testid has no consumer
The seven new unit tests seed hdx-user-preferences in localStorage directly, so nothing exercises the modal half of the feature: a regression here (wrong key, wrong option value, onChange dropped) would leave every test green. data-testid="row-click-action-select" is referenced nowhere in the repo — it looks like a hook for a test that wasn't written. Either add a test that drives this Select and asserts the stored preference, or drop the orphan test id.
There was a problem hiding this comment.
Right on both counts — the testid was left over from an E2E spec I ended up deleting. Rather than drop it, 85271bb adds src/__tests__/UserPreferencesModal.test.tsx, which drives the Select and covers the wiring you flagged:
- shows
Expand inlinewhen nothing is stored (theDEFAULT_ROW_CLICK_ACTIONfallthrough for existing users) - reflects a stored
sidePanel(thevaluebinding) - picking
Open side panelwritesrowClickAction: 'sidePanel'underhdx-user-preferences(the storage key, option values, andonChange)
A wrong key or option value now fails rather than passing green, and the testid has a consumer.
PR Review
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 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. |
| <IconLink size={16} /> | ||
| </DBRowTableIconButton> | ||
| {onOpenSidePanel && ( | ||
| <DBRowTableIconButton |
There was a problem hiding this comment.
🟠 major — The side panel's only remaining direct entry point is unreachable by keyboard and hidden from screen readers
DBRowTableIconButton renders a div with role="button" and tabIndex={-1} (DBRowTableIconButton.tsx:56), and .rowButtons is opacity: 0 until hover (packages/app/styles/LogTable.module.scss:164). It is also nested inside the row-body <button> (DBRowTable.tsx:1155), whose descendants are presentational per ARIA, so the new aria-label is never exposed. Before this PR the row body itself — a real focusable button — opened the panel; now keyboard/AT users can only reach it via expand-then-maximize. Render the side-panel control outside the row <button> (e.g. alongside the chevron in the expand cell, or as its own <td>) as a focusable element, rather than as another tabIndex={-1} hover affordance.
|
|
||
| // Inline expansion is owned by the expand-button column, so tables that hide | ||
| // it (surrounding context, patterns) always fall back to the side panel. | ||
| const prefersInlineExpand = |
There was a problem hiding this comment.
🔵 minor — A preference labelled "Search results" silently changes row-click behavior for every row table in the app
prefersInlineExpand is computed in RawLogTable, so the new default applies to every DBSqlRowTableWithSideBar consumer — dashboard Search tiles (DBDashboardPage.tsx:1390), ServiceDashboardSlowestEventsTile.tsx:92, PodDetailsSidePanel.tsx:203, NodeDetailsSidePanel.tsx:194, NamespaceDetailsSidePanel.tsx:181, llm/dashboard/ErrorsTab.tsx:113 and ChartPreviewPanel.tsx:315 — several of which are fixed-height tiles where .expandedRowContent's 400px panel (LogTable.module.scss:61) fills the whole tile, and one of which is already inside a side panel. Either scope the default to the search page with an explicit prop on DBSqlRowTableWithSideBar (defaulting the tiles to sidePanel), or drop the "Search results" framing in UserPreferencesModal.tsx:120 so the setting reads as global.
| sourceId={sourceId} | ||
| onRowDetailsClick={onOpenSidebar} | ||
| highlightedLineId={rowId ?? undefined} | ||
| highlightedLineId={sidePanel ? (rowId ?? undefined) : undefined} |
There was a problem hiding this comment.
🔵 minor — The stale-rowWhere guard that gates inline expansion has no test
The seven new unit tests all drive RawLogTable with highlightedLineId passed in directly, so nothing covers the wiring this PR actually changed here: that a rowWhere left over from another source (rowSource !== sourceId) no longer mounts a panel, no longer highlights a row, and no longer suppresses inline expansion. Add a DBSqlRowTableWithSidebar test that mounts with rowWhere set and rowSource pointing at a different source, and asserts the row body still toggles inline expansion.
|
Addressed the review. Three of the four inline findings were real and are fixed in 85271bb; I declined the Fixed — all three were the same root cause, inferring "a side panel is available/open" from a signal that does not mean that:
Declined — Also skipped the P3 about extracting the virtualizer geometry mocks into a shared helper — one consumer does not justify the indirection. Verification: |
| /** What clicking the body of a search result row does. */ | ||
| export type RowClickAction = 'expand' | 'sidePanel'; | ||
|
|
||
| export const DEFAULT_ROW_CLICK_ACTION: RowClickAction = 'sidePanel'; |
There was a problem hiding this comment.
Users without a stored rowClickAction, including existing users whose preferences predate this field, fall back to sidePanel. Because DBRowTable enables inline expansion only when this value is expand, row clicks keep opening the side panel unless users manually opt in. This contradicts the PR's stated behavior that row clicks expand inline by default and that existing users receive the new default.
| export const DEFAULT_ROW_CLICK_ACTION: RowClickAction = 'sidePanel'; | |
| export const DEFAULT_ROW_CLICK_ACTION: RowClickAction = 'expand'; |
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!
|
|
||
| // Once the side panel is open it stays the active surface, so clicks keep | ||
| // moving it from row to row instead of expanding rows behind it. | ||
| const expandInlineOnRowClick = |
There was a problem hiding this comment.
🔵 minor — Opening the side panel strands already-expanded rows: the body button stops collapsing them and drops aria-expanded
expandInlineOnRowClick is global to the table, but isExpanded is per row. With rowClickAction: 'expand', click row A's body (it expands, ExpandedLogRow renders because line 1256 gates only on showExpandButton && isExpanded), then click row B's "Open in side panel" hover button. highlightedLineId becomes non-null, so for row A the body button now reports aria-expanded={undefined} and is labelled "View details for log entry" while row A is still visibly expanded, and clicking it re-targets the panel instead of collapsing. Gate per row — e.g. keep toggling when isExpanded is true (expandInlineOnRowClick || isExpanded) so an expanded row can always be closed by the control that opened it. (Same state breaks the E2E clickRowBody regex in tests/e2e/components/TableComponent.ts:106.)
| /** What clicking the body of a search result row does. */ | ||
| export type RowClickAction = 'expand' | 'sidePanel'; | ||
|
|
||
| export const DEFAULT_ROW_CLICK_ACTION: RowClickAction = 'sidePanel'; |
There was a problem hiding this comment.
🔵 minor — DEFAULT_ROW_CLICK_ACTION is a second source of truth every reader has to remember
storageWithMigration.getItem returns the parsed object as-is (line 182–183) rather than {...DEFAULT_PREFERENCES, ...migrated}, which is why the new field needs ?? DEFAULT_ROW_CLICK_ACTION repeated at every read site (DBRowTable.tsx:458, UserPreferencesModal.tsx:127) — a third reader that forgets it silently gets undefined. Merging DEFAULT_PREFERENCES into the value returned by getItem would let rowClickAction: 'sidePanel' live in DEFAULT_PREFERENCES alone, and would also stop partially-stored preferences from yielding undefined timeFormat/font (which is exactly what the new tests and the new E2E seed do).
| // outlive the panel it belongs to. Deriving the table's highlight from this | ||
| // element keeps it from highlighting a row — and from treating the panel as | ||
| // open, which suppresses inline expansion — while no panel is mounted. | ||
| const sidePanel = |
There was a problem hiding this comment.
🔵 minor — The stale-rowWhere fix — the subtler of the two bug fixes — has no test
Nothing covers that a leftover rowWhere with a rowSource pointing at a different source now yields highlightedLineId={undefined} instead of highlighting a phantom row and suppressing inline expansion. The DBRowTable tests only cover the consumer side (highlightedLineId="some-other-row"). Add a test that renders DBSqlRowTableWithSideBar with sourceId="B" while rowWhere/rowSource are set for source A and asserts the child table receives no highlightedLineId — otherwise a future refactor of that predicate reintroduces the bug green.
|
|
||
| // A fresh jotai store per render keeps the stored preference from leaking | ||
| // into the next test through the module-level preferences atom. | ||
| const renderTable = (ui: ReactElement, rowClickAction?: RowClickAction) => { |
There was a problem hiding this comment.
🔵 minor — renderTable seeds localStorage but never clears it after the describe
window.localStorage.clear() runs only at the start of each render, so after the last "Row click behavior" test the key hdx-user-preferences is left at {colorMode:'dark',rowClickAction:'expand'} for the rest of the file; the "Error state" describe then mounts RawLogTable on jotai's module-level default store, whose atomWithStorage onMount reads that value (so isUTC/timeFormat/font are undefined there). The comment on line 262 says the fresh Provider prevents leakage, but the leak is via localStorage, not the store. Add an afterEach(() => window.localStorage.clear()) to this describe.
cursor
Bot
changed the title
Clicking a row body opened the side panel, leaving the 16px chevron as the only way to expand inline - a poor hit target during an incident, and the opposite of what comparable tools do. Row clicks now toggle inline expansion. The side panel moves to a row hover button, still stays one click away from an expanded row, and keeps receiving row clicks while it is open so browsing row to row is unchanged. Preferences -> Search results -> Row click restores the old behavior. Co-authored-by: Mike Shi <mike@hyperdx.io>
RawLogTable is also used by the ClickHouse dashboard's slow-query list, which passes no onRowDetailsClick. There the side panel preference had nothing to open, so a row click did nothing and the hover button offered a dead action. Fall back to inline expansion when the handler is absent. Co-authored-by: Mike Shi <mike@hyperdx.io>
Three review findings, all the same root cause — the code inferred "a side
panel is available/open" from signals that do not mean that:
- `highlightedLineId` was passed even when the panel was not mounted. Because
`rowWhere`/`rowSource` survive a source switch, a stale `rowWhere` left the
table highlighting a missing row and suppressing inline expansion, so the
first click after switching source opened the panel instead of expanding.
Both now derive from the panel element.
- `ExpandedLogRow` guarded its maximize button on `{openSidebar && ...}`, the
result of a `useCallback` and so always truthy. On the ClickHouse dashboard's
slow-query list, which renders no panel, the button only wrote URL params
nothing reads. It now takes `canOpenSidePanel`, matching the row hover button.
- `clickRow`/`clickFirstRow` lost their last callers and would silently expand
inline for a future spec reaching for the side panel. Removed in favour of
`clickRowBody`/`openRowSidePanel`.
Also covers the preferences Select, whose stored key and option values no test
exercised.
Co-authored-by: Mike Shi <mike@hyperdx.io>
Inline expansion becomes opt-in rather than the new default, so nothing changes for anyone who does not go looking for it. `DBRowTable`'s routing already read the preference, so only the constant moves. The fallout is in the tests: the E2E specs that reach the side panel go back to a plain row click, which restores `clickRow`/`clickFirstRow`, and the inline spec now seeds `rowClickAction: 'expand'` before navigating since the row hover button and the row body's expand target only exist in that mode. Co-authored-by: Mike Shi <mike@hyperdx.io>
Co-authored-by: Mike Shi <mike@hyperdx.io>
1959b18 to
8bafede
Compare
A row and its inline expansion are two `tr`s sharing one virtual index, and both carried `data-index` and the virtualizer's `measureElement` ref. Since virtual-core keys its element cache by index, the expanded `tr` took over that index's ResizeObserver registration and cached its own height there. Collapsing called the ref with `null`, which `measureElement` ignores, so the inflated height stuck and nothing observed the row any more. Each expand/collapse then shrank the render window until scrolling showed a few rows above a screen of empty space. Wrap the pair in a `tbody` and measure that instead, so one element maps to one virtual index. Co-authored-by: Mike Shi <mike@hyperdx.io>
…and-log-inline-on-row-click-d7eb # Conflicts: # packages/app/src/components/DBRowTable.tsx # packages/app/src/components/ExpandableRowTable.tsx # packages/app/tests/e2e/components/TableComponent.ts Co-authored-by: Mike Shi <mike@hyperdx.io>
github-actions
Bot
added
review/tier-3
| // cached there after it unmounted. | ||
| <tbody | ||
| key={virtualRow.key} | ||
| data-index={virtualRow.index} |
There was a problem hiding this comment.
Moving data-index from each virtualized data row to its enclosing tbody breaks the dashboard and chart-editor E2E helpers, which still wait for tbody tr[data-index]. Those helpers can no longer find the data rows, so tests that read cells or interact with rows will time out. Update those locators alongside this DOM change.
| <IconLink size={16} /> | ||
| </DBRowTableIconButton> | ||
| {onOpenSidePanel && ( | ||
| <DBRowTableIconButton |
There was a problem hiding this comment.
🔵 minor — The new "Open in side panel" control is not keyboard reachable
DBRowTableIconButton defaults to tabIndex = -1 and renders component="div" (DBRowTableIconButton.tsx:23,53-56), and this call site doesn't override it. Under rowClickAction: 'expand' the row body no longer opens the side panel, so for a keyboard-only user the direct route to the panel disappears entirely — the only remaining path is Tab to the row body, Enter to expand inline, then Tab to the expanded row's maximize <button>. Pass tabIndex={0} on this one button (it's a primary action, unlike the sibling copy affordances).
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
| onToggleWrap={() => | ||
| setWrapLinesEnabled(!wrapLinesEnabled) | ||
| } | ||
| onOpenSidePanel={ |
There was a problem hiding this comment.
🔵 minor — Hover "Open in side panel" button stays visible when a row click already opens the panel
onOpenSidePanel is gated on prefersInlineExpand && canOpenSidePanel, which ignores highlightedLineId. With the expand preference and the panel already open, expandInlineOnRowClick is false (line 475), so a row click opens/moves the panel — and the hover button does the same thing, which is exactly the duplication the default mode avoids and that DBRowTable.test.tsx:342-346 asserts against. Gate it on expandInlineOnRowClick instead, or accept the duplication deliberately and drop that assertion's rationale.
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
| sourceId={sourceId} | ||
| onRowDetailsClick={onOpenSidebar} | ||
| highlightedLineId={rowId ?? undefined} | ||
| highlightedLineId={sidePanel ? (rowId ?? undefined) : undefined} |
There was a problem hiding this comment.
🔵 minor — The new highlightedLineId derivation has no test; reverting it keeps every test green
This is the fix for the stale rowWhere/rowSource signal, and it is the only thing that returns row clicks to inline expansion after the panel closes — but the unit tests inject highlightedLineId into RawLogTable directly (DBRowTable.test.tsx:465), so changing this line back to highlightedLineId={rowId ?? undefined} still passes. Add a test that renders DBSqlRowTableWithSideBar with rowWhere set and rowSource pointing at a different source, and asserts no row is highlighted.
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
| // Once the side panel is open it stays the active surface, so clicks keep | ||
| // moving it from row to row instead of expanding rows behind it. | ||
| const expandInlineOnRowClick = | ||
| prefersInlineExpand && (!canOpenSidePanel || highlightedLineId == null); |
There was a problem hiding this comment.
🔵 minor — !canOpenSidePanel || in expandInlineOnRowClick is subsumed by prefersInlineExpand
When canOpenSidePanel is false, prefersInlineExpand (line 468) is already true whenever showExpandButton is, so the second !canOpenSidePanel branch can never change the result. Reduce to prefersInlineExpand && highlightedLineId == null and keep the comment about the panel staying the active surface.
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
| /> | ||
| </SettingContainer> | ||
|
|
||
| <Divider label="Search results" labelPosition="left" mt="sm" /> |
There was a problem hiding this comment.
🔵 minor — Preference is scoped as "Search results" but applies to every row table in the app
rowClickAction is read in RawLogTable, which backs dashboard table tiles (DBDashboardPage.tsx:1390), the Kubernetes pod/node/namespace side panels (PodDetailsSidePanel.tsx:203, NodeDetailsSidePanel.tsx:194, NamespaceDetailsSidePanel.tsx:181), the service-dashboard slowest-events tile and the chart preview panel — all of which are short, fixed-height surfaces where a tall inline expansion is a worse fit than the panel. Either retitle the section to something accurate (e.g. "Result rows") or thread an opt-in prop from the search page so the preference only governs the full-height results table.
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
Expanding and then collapsing rows in the search results table permanently shrinks the virtualizer's render window. After a few expand/collapse cycles, scrolling leaves a screen of empty space with only a handful of rows in it — sometimes none at all, while the result count still reads 127.
**Root cause.** A row and its inline expansion are two `<tr>`s but one virtual item. Both carried `data-index={virtualRow.index}` and the virtualizer's `measureElement` ref:
- `virtual-core` keys `measureElementCache` by `item.key`, which is the index. When the expanded `<tr>` mounts, `_measureElement` sees `prevNode !== node`, **unobserves the base row**, observes the expanded row, and caches the expanded row's height as the size of that index.
- Collapsing calls the ref with `null`, and `measureElement` returns early for `null`. The `ResizeObserver` does fire for the detached node, but that path takes the `!node.isConnected` branch, which unobserves and returns *before* resizing.
So the index keeps the expanded height forever, and nothing observes that row any more, so it never re-measures. Every expand/collapse inflates one more index.
**Fix.** Wrap each row and its expansion in their own `<tbody>` and measure that instead, so one DOM element maps to one virtual index. The `ResizeObserver` on the `<tbody>` picks up both the row and the expansion growing or going away, so collapsing restores the correct height on its own. No stylesheet targets this table's `tbody`, and multiple `tbody` elements are valid in a `table`.
This is not a new regression — it reproduces on `main` today via the chevron. The expand/collapse affordance just makes it easy to hit.
### Screenshots or video
Six expand/collapse cycles on rows 0–5, then scrolling to the same offset. Left is `main`, right is this branch:
Measured on the scroll container during that run:
| | rows rendered | space below last row |
| :-- | :-- | :-- |
| baseline, nothing expanded | 55 | −711px |
| after 6 expand/collapse, before fix | 32 | **+941px** at `scrollTop` 1200 |
| after 6 expand/collapse, with fix | 55 | −686px at `scrollTop` 1200 |
Negative means the rendered rows overfill the viewport, which is what you want. The viewport is 555px tall, so a +941px gap means nothing at all is visible.
### Tests
`packages/app/tests/e2e/features/search/virtualized-row-heights.spec.ts` covers both halves: that collapsing restores the render window, and that scrolling afterwards keeps the viewport covered. Both fail on `main` (`Expected: 55, Received: 38`; `Expected: <= 0, Received: 86`) and pass here.
`make ci-lint` and `make ci-unit` are green. The full `search/` E2E directory was re-run: 84 passed, 3 flaky-then-passed, and 2 `search-filters.spec.ts` failures that reproduce identically with `packages/app/src` reverted to `origin/main`, so they are environmental rather than from this change.
### How to test on Vercel preview
**Preview routes:** /search
**Steps:**
1. Wait for the results table to render rows.
2. Click the chevron at the left of the first row to expand it, then click it again to collapse it.
3. Repeat step 2 on the next five rows.
4. Scroll the results table down roughly one screen.
5. Verify the table is still full of log rows, with no blank area between the last row and the bottom of the table.
### References
- Linear Issue:
- Related PRs: #3117 adds an opt-in "expand inline on row click" preference, which makes this bug far easier to trigger. That PR is stacked on this one.
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>
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…ine-on-row-click-d7eb # Conflicts: # packages/app/src/components/DBRowTable.tsx # packages/app/src/components/ExpandableRowTable.tsx # packages/app/tests/e2e/components/TableComponent.ts
| 'position-absolute top-0 end-0 mt-1 me-1 p-1 border-0 bg-transparent text-muted rounded', | ||
| styles.expandButton, | ||
| )} | ||
| onClick={() => openSidebar(rowId, source?.id)} |
There was a problem hiding this comment.
🔵 minor — Expanded row's 'Open in side panel' skips onOpenSidebar and drops aliasWith, unlike the new hover button
The expanded row's button still opens the panel through useSidebarOpener, which only writes the rowWhere/rowSource URL params. The new hover button in DBRowTable goes through _onRowExpandClick → onOpenSidebar (DBSqlRowTableWithSidebar.tsx:93), which also sets aliasWith and fires onSidebarOpen. Opening a row from its expanded view therefore leaves aliasWith stale or empty, and with 'Expand inline' on this button is a main route to the panel. Fix: replace the canOpenSidePanel boolean with an onOpenSidePanel callback, pass () => _onRowExpandClick(row.original) as the hover button already does, and delete useSidebarOpener.
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
github-actions
Bot
added
review/tier-2
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
| 'position-absolute top-0 end-0 mt-1 me-1 p-1 border-0 bg-transparent text-muted rounded', | ||
| styles.expandButton, | ||
| )} | ||
| onClick={() => openSidebar(rowId, source?.id)} |
There was a problem hiding this comment.
🟠 major — The expanded row's 'Open in side panel' button skips aliasWith and live-tail pause that the hover button applies
With rowClickAction: 'expand', the PR offers two ways to open the side panel, and they behave differently. The hover button calls _onRowExpandClick, which reaches onOpenSidebar in DBSqlRowTableWithSidebar.tsx:93. That sets aliasWith and calls onSidebarOpen, which pauses live tail in DBSearchPage.tsx:1495. The maximize button in the expanded row goes through useSidebarOpener instead and only writes rowWhere/rowSource. So the panel opens with whatever aliasWith was left from the last open, and live tail keeps running. Fix: replace the new canOpenSidePanel?: boolean prop with onOpenSidePanel?: () => void, passed from DBRowTable.tsx:1319 as canOpenSidePanel ? () => _onRowExpandClick(row.original) : undefined, and delete useSidebarOpener. That leaves one code path for opening the panel.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| * returned as-is rather than merged with `DEFAULT_PREFERENCES`, so readers must | ||
| * fall back to `DEFAULT_ROW_CLICK_ACTION`. | ||
| */ | ||
| rowClickAction?: RowClickAction; |
There was a problem hiding this comment.
🔵 minor — The rowClickAction default is repeated at every reader instead of applied once when preferences load
rowClickAction is optional, so each reader has to add ?? DEFAULT_ROW_CLICK_ACTION itself (DBRowTable.tsx:471, UserPreferencesModal.tsx:58). Any new reader that forgets it gets undefined. Fix: add rowClickAction: 'sidePanel' to DEFAULT_PREFERENCES and return { ...DEFAULT_PREFERENCES, ...parsed } from the already-migrated branch of migrateUserPreferences (line 151). Then the field can be required and both fallbacks go away.
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
| sourceId={sourceId} | ||
| onRowDetailsClick={onOpenSidebar} | ||
| highlightedLineId={rowId ?? undefined} | ||
| highlightedLineId={sidePanel ? (rowId ?? undefined) : undefined} |
There was a problem hiding this comment.
🔵 minor — No test covers the new rule that hides highlightedLineId when the side panel isn't mounted
Inline expansion is disabled whenever highlightedLineId is set. That value now depends on the sidePanel element, which is null when rowSource points to another source. No test pins this, so dropping the ternary would pass CI while a stale rowWhere silently turns off inline expansion. Add a test for DBSqlRowTableWithSideBar that sets rowWhere plus a rowSource for a different source, then checks that a row click under expand expands inline instead of calling the side-panel opener.
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
![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}
Resolves HDX-5365.
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 toExpand inlineand:Open in side panel, beside the existing copy buttons), and the expanded row's maximize button still opens it;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 toDEFAULT_ROW_CLICK_ACTION.Two table variants need care regardless of the preference:
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:
highlightedLineIdwas passed to the table even when the panel was not mounted.rowWhere/rowSourcesurvive a source switch while closing the panel clears both, so a stalerowWhereleft the table highlighting a row with no panel on screen. The panel is now hoisted into onesidePanelelement that both its own mount andhighlightedLineIdderive from.ExpandedLogRowgated its maximize button on{openSidebar && …}, the return value of auseCallbackand so always truthy. It now takescanOpenSidePanel.DBRowTableIconButtonalso gained anaria-labelfrom 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
maintoday — it reproduces identically through the chevron, with this PR'spackages/app/srcreverted. A row and its inline expansion are twotrs sharing one virtual index, and both were measured, so the expanded one took over that index'sResizeObserverregistration 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.tsxnow renders atbodyper row.Tests
DBRowTable.test.tsx: the side panel on row click by default (and no redundant hover button in that mode), then underexpand— 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.Open in side paneland that clicking it writes therowWhereparam, socanOpenSidePanelis pinned in both directions — hardcoding it tofalsefails the test rather than staying green.UserPreferencesModal.test.tsxdrives the select: the default for users with no stored preference, a stored value round-tripping into the control, and picking an option writingrowClickAction. A wrong storage key or option value now fails instead of passing green.Row click set to expand inlinedescribe 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.tscomes in from fix(app): stop expanded rows corrupting virtualized row heights #3171 and covers the virtualizer regression.Rebase onto
mainRebased 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-warningsbump onmain. The rebase picks that up; the one warning this PR did add (an unsafe cast in the newSelecthandler) is gone, so the app count is back under the ceiling rather than exactly on it.Verification:
make ci-lintandmake ci-unitgreen (244 suites, 3948 tests after the merge). All 94search/E2E tests re-run — 85 passed, 1 flaky-then-passed, and 2search-filters.spec.tsfailures that reproduce identically with this PR'spackages/app/srcreverted toorigin/main, so they are environmental and not from this change.correlated-metric-source,span-linksandtraces-workflowalso 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.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 panelbutton among the row action icons on the right:Linear Issue: HDX-5365