Skip to content

feat(app): add a row-click preference to expand result rows inline #3117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kodiakhq merged 10 commits into from
Sep 29, 2026
Merged

feat(app): add a row-click preference to expand result rows inline #3117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d59684
feat(app): expand result rows inline on row click
cursoragent Sep 14, 2026
89e61ab
fix(app): keep inline expansion when a table has no side panel
cursoragent Sep 14, 2026
eae6338
fix(app): tie row-click routing to a side panel that actually exists
cursoragent Sep 14, 2026
6358e05
fix(app): keep the side panel as the default row click action
cursoragent Sep 16, 2026
8bafede
fix(app): avoid a new unsafe cast in the row click preference select
cursoragent Sep 21, 2026
50a3a00
test(app): assert the expanded row offers the side panel button
cursoragent Sep 21, 2026
8667e3a
fix(app): stop expanded rows corrupting virtualized row heights
cursoragent Sep 21, 2026
27143f3
Merge branch 'cursor/fix-virtualized-row-height-d7eb' into cursor/exp…
cursoragent Sep 21, 2026
472b3bc
Merge remote-tracking branch 'origin/main' into cursor/expand-log-inl…
pulpdrew Sep 29, 2026
466bca7
Merge branch 'main' into cursor/expand-log-inline-on-row-click-d7eb
kodiakhq[bot] Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .changeset/witty-mangos-listen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'@hyperdx/app': minor
---

Add a Preferences → Search results → Row click setting. Leave it at `Open side
panel` (the default) for the existing behavior, or set it to `Expand inline` to
have a row click expand the row in place — the chevron's 16px hit target is hard
to aim at while scanning logs. With inline expansion on, the side panel moves to
a hover button on the row, stays one click away from an expanded row, and keeps
receiving row clicks while it is open.
33 changes: 32 additions & 1 deletion packages/app/src/UserPreferencesModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,24 @@ import { IconFlask } from '@tabler/icons-react';
import { OPTIONS_FONTS } from './config/fonts';
import { useAppTheme } from './theme/ThemeProvider';
import { isValidThemeName, themes } from './theme';
import { UserPreferences, useUserPreferences } from './useUserPreferences';
import {
DEFAULT_ROW_CLICK_ACTION,
RowClickAction,
UserPreferences,
useUserPreferences,
} from './useUserPreferences';

const OPTIONS_COLOR_MODE = [
{ label: 'System', value: 'system' },
{ label: 'Light', value: 'light' },
{ label: 'Dark', value: 'dark' },
];

const OPTIONS_ROW_CLICK_ACTION: { label: string; value: RowClickAction }[] = [
{ label: 'Open side panel', value: 'sidePanel' },
{ label: 'Expand inline', value: 'expand' },
];

// Brand theme options (generated from theme registry)
const OPTIONS_BRAND_THEMES = Object.values(themes).map(t => ({
label: t.displayName,
Expand Down Expand Up @@ -108,6 +118,27 @@ export const UserPreferencesModal = ({
/>
</SettingContainer>

<Divider label="Search results" labelPosition="left" mt="sm" />

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

<SettingContainer
label="Row click"
description="What clicking a result row does. The chevron always expands inline."
>
<Select
data-testid="row-click-action-select"

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@cursor cursor Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 inline when nothing is stored (the DEFAULT_ROW_CLICK_ACTION fallthrough for existing users)
  • reflects a stored sidePanel (the value binding)
  • picking Open side panel writes rowClickAction: 'sidePanel' under hdx-user-preferences (the storage key, option values, and onChange)

A wrong key or option value now fails rather than passing green, and the testid has a consumer.

value={userPreferences.rowClickAction ?? DEFAULT_ROW_CLICK_ACTION}
onChange={value => {
const option = OPTIONS_ROW_CLICK_ACTION.find(
o => o.value === value,
);
if (option) {
setUserPreference({ rowClickAction: option.value });
}
}}
data={OPTIONS_ROW_CLICK_ACTION}
allowDeselect={false}
/>
</SettingContainer>

<Divider
label={
<Group align="center" gap="xs">
Expand Down
64 changes: 64 additions & 0 deletions packages/app/src/__tests__/UserPreferencesModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { Provider } from 'jotai';
import { screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { UserPreferencesModal } from '@/UserPreferencesModal';
import type { UserPreferences } from '@/useUserPreferences';

const STORAGE_KEY = 'hdx-user-preferences';

const renderModal = (stored?: Partial<UserPreferences>) => {
window.localStorage.clear();
if (stored) {
window.localStorage.setItem(
STORAGE_KEY,
// `colorMode` is what marks stored preferences as already migrated.
JSON.stringify({ colorMode: 'dark', ...stored }),
);
}
return renderWithMantine(
<Provider>
<UserPreferencesModal opened onClose={() => {}} />
</Provider>,
);
};

const readStoredPreferences = (): UserPreferences =>
JSON.parse(window.localStorage.getItem(STORAGE_KEY) ?? '{}');

describe('UserPreferencesModal', () => {
describe('Row click', () => {
it('shows the default action for users with no stored preference', () => {
renderModal();

expect(screen.getByTestId('row-click-action-select')).toHaveValue(
'Open side panel',
);
});

it('reflects a stored preference', () => {
renderModal({ rowClickAction: 'expand' });

expect(screen.getByTestId('row-click-action-select')).toHaveValue(
'Expand inline',
);
});

it('stores the selected action', async () => {
renderModal();

await userEvent.click(screen.getByTestId('row-click-action-select'));
await userEvent.click(
await screen.findByRole('option', {
name: 'Expand inline',
hidden: true,
}),
);

expect(screen.getByTestId('row-click-action-select')).toHaveValue(
'Expand inline',
);
expect(readStoredPreferences().rowClickAction).toBe('expand');
});
});
});
53 changes: 47 additions & 6 deletions packages/app/src/components/DBRowTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ import {
UNDEFINED_WIDTH,
} from '@/tableUtils';
import { FormatTime } from '@/useFormatTime';
import { useUserPreferences } from '@/useUserPreferences';
import {
DEFAULT_ROW_CLICK_ACTION,
useUserPreferences,
} from '@/useUserPreferences';
import {
getChartColorInfo,
getLogLevelClass,
Expand Down Expand Up @@ -385,7 +388,7 @@ export const RawLogTable = memo(
rows: Record<string, any>[];
isLoading?: boolean;
fetchNextPage?: (options?: FetchNextPageOptions | undefined) => any;
onRowDetailsClick: (row: Record<string, any>) => void;
onRowDetailsClick?: (row: Record<string, any>) => void;
generateRowId: (row: Record<string, any>) => RowWhereResult;
hasNextPage?: boolean;
highlightedLineId?: string;
Expand Down Expand Up @@ -453,9 +456,25 @@ export const RawLogTable = memo(
);

const {
userPreferences: { isUTC },
userPreferences: { isUTC, rowClickAction },
} = useUserPreferences();

// The ClickHouse dashboard's slow-query list has no side panel to open, so
// inline expansion is the only thing a row click can do there.
const canOpenSidePanel = onRowDetailsClick != null;

// 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 =

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — A preference labelled "Search results" changes row clicks in every table, including small dashboard tiles

RawLogTable reads rowClickAction itself, so expand applies to every table that renders it with the expand column: dashboard tiles (DBDashboardPage.tsx:1390, ServiceDashboardSlowestEventsTile.tsx:92 — a fixed-height ChartCard), the k8s detail drawers (PodDetailsSidePanel.tsx:203, NodeDetailsSidePanel.tsx:194) and the chart preview panel (ChartPreviewPanel.tsx:315). In a 300–400px tile, replacing the drawer with an inline RowOverviewPanelWrapper (tabs + full column list) is strictly worse, and the hover button is the only remaining route to the panel. Scope it: pass the routing choice in as a prop from the search results table (DBSearchPage.tsx:2787) instead of reading the global preference in the leaf.

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

@cursor cursor Bot Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberate, and I would rather not scope it to the search page in this PR.

The preference is opt-in and off by default, so nothing changes for anyone who does not go looking for it. For someone who does turn it on, "a row click expands the row" reading the same everywhere is the behavior I would expect from a global preference; having it silently revert to the drawer in a dashboard tile is the more surprising outcome. The drawer stays reachable in those tiles through the hover button and the expanded row's maximize button.

The tile-height concern is real, but it applies equally to the chevron, which already expands inline in those same tiles today and is unaffected by this preference. If small tiles turn out to be the wrong surface for inline details, the fix belongs with the expand column rather than with this preference.

Happy to thread the choice down from DBSearchPage.tsx:2787 as a prop if you would prefer it scoped — flag it and I will.

showExpandButton &&
(!canOpenSidePanel ||
(rowClickAction ?? DEFAULT_ROW_CLICK_ACTION) === 'expand');

// 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 =

@github-actions github-actions Bot Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.)

prefersInlineExpand && (!canOpenSidePanel || highlightedLineId == null);

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@cursor cursor Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — "Side panel is open" is inferred from highlightedLineId, which does not mean that for every caller

ContextSidePanel.tsx:315 passes highlightedLineId={rowId} to mean "the row this surrounding-context view is centred on" while also passing onRowDetailsClick; the inference only happens to be harmless there because that table sets showExpandButton={false}. Any caller that highlights a row for a reason other than an open panel silently disables the preference. Have DBSqlRowTableWithSidebar — the component that owns the panel — pass an explicit isSidePanelOpen prop rather than deriving panel state from a display prop.

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

@cursor cursor Bot Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the derivation is indirect, but leaving it as is — the fix would widen the PR past what it already touches.

As you note, ContextSidePanel.tsx:315 is harmless today because that table passes showExpandButton={false}, and prefersInlineExpand requires showExpandButton, so the inference never runs there. Every other caller that passes highlightedLineId gets it from DBSqlRowTableWithSidebar, where it now derives from the sidePanel element actually being mounted — that was the stale-rowWhere fix in this PR.

Threading an explicit isSidePanelOpen would mean a new prop on DBSqlRowTable and RawLogTable plus updating the pass-through callers, for no behavior change. Worth doing if a caller ever highlights a row for a reason other than an open panel while also showing the expand column; there is no such caller now.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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


const [columnSizeStorage, setColumnSizeStorage] = useLocalStorage<
Record<string, number>
>(`${tableId}-column-sizes`, {});
Expand Down Expand Up @@ -1195,9 +1214,23 @@ export const RawLogTable = memo(
[styles.isTruncated]: !wrapLinesEnabled,
})}
onClick={() => {
_onRowExpandClick(row.original);
if (expandInlineOnRowClick) {
toggleRowExpansion(rowId);
} else {
_onRowExpandClick(row.original);
}
}}
aria-label="View details for log entry"
aria-expanded={
expandInlineOnRowClick ? isExpanded : undefined
}
// Deliberately distinct from the chevron's "Expand log
// details" so the two hit targets stay addressable
// apart in tests and assistive tech.
aria-label={
expandInlineOnRowClick
? `${isExpanded ? 'Collapse' : 'Expand'} log row`
: 'View details for log entry'
}
>
{row
.getVisibleCells()
Expand Down Expand Up @@ -1265,6 +1298,11 @@ export const RawLogTable = memo(
onToggleWrap={() =>
setWrapLinesEnabled(!wrapLinesEnabled)
}
onOpenSidePanel={

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

prefersInlineExpand && canOpenSidePanel
? () => _onRowExpandClick(row.original)
: undefined
}
/>
)}
</button>
Expand All @@ -1278,6 +1316,7 @@ export const RawLogTable = memo(
virtualKey={virtualRow.key.toString()}
source={source}
rowId={rowId}
canOpenSidePanel={canOpenSidePanel}
>
{renderRowDetails?.({
id: rowId,
Expand Down Expand Up @@ -1909,7 +1948,9 @@ function DBSqlRowTableComponent({
fetchNextPage={fetchNextPage}
// onPropertySearchClick={onPropertySearchClick}
hasNextPage={hasNextPage}
onRowDetailsClick={_onRowDetailsClick}
// Passed through conditionally so tables with no side panel (the
// ClickHouse dashboard's slow-query list) don't offer to open one.
onRowDetailsClick={onRowDetailsClick ? _onRowDetailsClick : undefined}
onScroll={onScroll}
generateRowId={getRowWhere}
isError={isError}
Expand Down
29 changes: 18 additions & 11 deletions packages/app/src/components/DBSqlRowTableWithSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,23 +120,30 @@ export default function DBSqlRowTableWithSideBar({
[sourceData],
);

// `rowWhere`/`rowSource` survive a source switch, so a stale `rowWhere` can
// 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 =

@github-actions github-actions Bot Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

sourceData && (rowSource === sourceId || !rowSource) ? (
<DBRowSidePanel
source={sourceData}
rowId={rowId ?? undefined}
aliasWith={aliasWith}
onClose={onCloseSidebar}
closeOnClickOutside={closeOnClickOutside}
keepOpenSelector={keepOpenSelector}
/>
) : null;

return (
<RowSidePanelContext value={context ?? {}}>
{sourceData && (rowSource === sourceId || !rowSource) && (
<DBRowSidePanel
source={sourceData}
rowId={rowId ?? undefined}
aliasWith={aliasWith}
onClose={onCloseSidebar}
closeOnClickOutside={closeOnClickOutside}
keepOpenSelector={keepOpenSelector}
/>
)}
{sidePanel}
<DBSqlRowTable
config={config}
sourceId={sourceId}
onRowDetailsClick={onOpenSidebar}
highlightedLineId={rowId ?? undefined}
highlightedLineId={sidePanel ? (rowId ?? undefined) : undefined}

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The PR's central fix — gating highlightedLineId on a mounted side panel — has no test

Every new unit test drives RawLogTable directly and passes highlightedLineId as a literal prop (DBRowTable.test.tsx:464), so reverting line 146 back to highlightedLineId={rowId ?? undefined} leaves all seven new tests and the new E2E green while restoring exactly the bug the PR describes: after a source switch, a stale rowWhere makes the table treat a non-existent panel as open and suppresses inline expansion. There is no test file for DBSqlRowTableWithSidebar at all (packages/app/src/components/__tests__/ has none), and no E2E exercises rowSource mismatch. Add a test that renders DBSqlRowTableWithSideBar with rowSource set to a different source id and asserts the table still expands inline on row click.

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 github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

@github-actions github-actions Bot Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

enabled={enabled}
isLive={isLive ?? true}
queryKeyPrefix={'dbSqlRowTable'}
Expand Down
1 change: 1 addition & 0 deletions packages/app/src/components/DBTable/DBRowTableIconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export const DBRowTableIconButton: React.FC<DBRowTableIconButtonProps> = ({
className={baseClasses}
tabIndex={tabIndex}
role="button"
aria-label={title}
>
{isActive ? <IconCheck size={iconSize} /> : children}
</UnstyledButton>
Expand Down
19 changes: 18 additions & 1 deletion packages/app/src/components/DBTable/DBRowTableRowButtons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import React, { useState } from 'react';
import { notifications } from '@mantine/notifications';
import { IconCopy, IconLink, IconTextWrap } from '@tabler/icons-react';
import {
IconCopy,
IconLayoutSidebarRightExpand,
IconLink,
IconTextWrap,
} from '@tabler/icons-react';

import { RowWhereResult } from '@/hooks/useRowWhere';
import {
Expand All @@ -19,6 +24,8 @@ interface DBRowTableRowButtonsProps {
sourceId?: string;
isWrapped: boolean;
onToggleWrap: () => void;
/** Omitted when clicking the row already opens the side panel. */
onOpenSidePanel?: () => void;
}

const DBRowTableRowButtons: React.FC<DBRowTableRowButtonsProps> = ({
Expand All @@ -27,6 +34,7 @@ const DBRowTableRowButtons: React.FC<DBRowTableRowButtonsProps> = ({
sourceId,
isWrapped,
onToggleWrap,
onOpenSidePanel,
}) => {
const [isCopied, setIsCopied] = useState(false);
const [isUrlCopied, setIsUrlCopied] = useState(false);
Expand Down Expand Up @@ -105,6 +113,15 @@ const DBRowTableRowButtons: React.FC<DBRowTableRowButtonsProps> = ({
>
<IconLink size={16} />
</DBRowTableIconButton>
{onOpenSidePanel && (
<DBRowTableIconButton

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

onClick={onOpenSidePanel}
variant="copy"
title="Open in side panel"
>
<IconLayoutSidebarRightExpand size={16} />
</DBRowTableIconButton>
)}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
</div>
);
};
Expand Down
3 changes: 2 additions & 1 deletion packages/app/src/components/DBTable/RowSelectionCells.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ export const RowSelectionCell = memo(
aria-label="Select row"
title="Select row, shift-click to select a range"
data-testid="row-select-checkbox"
// The row body is a single button that opens the side panel.
// The row body is a single button that opens the side panel or expands
// the row, depending on the `rowClickAction` preference.
onClick={e => e.stopPropagation()}
onChange={e =>
onToggle(rowId, {
Expand Down
9 changes: 6 additions & 3 deletions packages/app/src/components/ExpandableRowTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@ export const ExpandedLogRow = memo(
virtualKey,
source,
rowId,
canOpenSidePanel,
}: {
children: React.ReactNode;
columnsLength: number;
virtualKey: string;
source?: TSource;
rowId: string;
/** False on tables that render no side panel, so the URL params go nowhere. */
canOpenSidePanel?: boolean;
}) => {
const openSidebar = useSidebarOpener();

Expand All @@ -51,16 +54,16 @@ export const ExpandedLogRow = memo(
<div className={cx('mx-2 mb-2 rounded', styles.expandedRowContent)}>
<div className="position-relative">
<div className="px-3 pt-2 position-relative">
{openSidebar && (
{canOpenSidePanel && (
<button
type="button"
className={cx(
'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)}

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Expanded row's side-panel button drops aliasWith, unlike the new hover button

The diff now offers two routes from a row to the panel: the new hover button goes through onRowDetailsClick → DBSqlRowTableWithSidebar.onOpenSidebar, which sets both rowWhere and the aliasWith state the panel queries with; the expanded row's button calls openSidebar(rowId, source?.id), which writes only the URL params. aliasWith in DBSqlRowTableWithSidebar.tsx:91 starts as [], so on a fresh page load — expand a row inline, click its maximize button as the first panel open — the panel runs useRowData with no WITH clauses, while rowWhere may reference an alias defined by one (useRowWhere.tsx:154-172, where valueExpr is the aliased expression). Route the expanded row's button through the same onRowDetailsClick path rather than the URL params.

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 github-actions Bot Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 github-actions Bot Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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

title="Open in sidebar"
aria-label="Open in sidebar"
title="Open in side panel"

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@cursor cursor Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

aria-label="Open in side panel"
style={{
zIndex: 1,
fontSize: '12px',
Expand Down
Loading
Loading