From c3c9c2cd5d60ed0d4a2c81fe0f1be927a508ad44 Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Wed, 20 May 2026 22:28:23 +0000 Subject: [PATCH 1/3] feat(app): hover hint and native link affordance for dashboard table tile row click When a dashboard table tile is configured with an `onClick` action (search or dashboard link), the row click destination is now visible before the user clicks. After ~250ms of hover, a small card appears with text like `Search HyperDX Logs` or `Open dashboard "API Latency Drilldown"`. The cell wrapper switches from a manual `
` with hand-rolled cmd-click / middle-click / Enter handlers to a Next.js ``. The browser now handles cmd-click and middle-click opening in a new tab natively, right-click shows "Open in New Tab" / "Copy Link Address" in the context menu, the destination URL appears in the status bar on hover, and keyboard users can Tab to a cell and press Enter to navigate with a visible focus ring. The cell wrapper now mirrors the pattern in `DBListBarChart.tsx` (the only other dashboard chart with a row drilldown HoverCard). A new `describeOnClick` helper in common-utils renders the one-line hint. The hook returns the description alongside the URL so future hint surfaces (chart-header subtitle, scope preview) can reuse the same string. --- .changeset/onclick-hover-hint.md | 6 + packages/app/src/HDXMultiSeriesTableChart.tsx | 104 +++++++++++----- packages/app/src/components/DBTableChart.tsx | 28 +---- .../__tests__/useOnClickLinkBuilder.test.tsx | 116 ++++++++++++++++++ .../app/src/hooks/useOnClickLinkBuilder.ts | 106 +++++++++++----- .../tests/e2e/page-objects/DashboardPage.ts | 8 +- .../src/core/__tests__/linkUrlBuilder.test.ts | 74 +++++++++++ .../common-utils/src/core/linkUrlBuilder.ts | 41 +++++++ 8 files changed, 390 insertions(+), 93 deletions(-) create mode 100644 .changeset/onclick-hover-hint.md create mode 100644 packages/app/src/hooks/__tests__/useOnClickLinkBuilder.test.tsx diff --git a/.changeset/onclick-hover-hint.md b/.changeset/onclick-hover-hint.md new file mode 100644 index 0000000000..c529280ac2 --- /dev/null +++ b/.changeset/onclick-hover-hint.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/app": patch +--- + +Dashboard table tiles configured with a row-click action now show a hover hint describing where the click will go (for example, `Search HyperDX Logs` or `Open dashboard "API Latency Drilldown"`). The cell wrapper is now a real link, so cmd-click and middle-click open the destination in a new tab, right-click shows the browser context menu with "Open in New Tab" and "Copy Link Address", and the destination URL appears in the browser status bar on hover. Keyboard users can Tab to a cell and press Enter to navigate, with a visible focus ring. diff --git a/packages/app/src/HDXMultiSeriesTableChart.tsx b/packages/app/src/HDXMultiSeriesTableChart.tsx index 6d449c6c58..8012cbb781 100644 --- a/packages/app/src/HDXMultiSeriesTableChart.tsx +++ b/packages/app/src/HDXMultiSeriesTableChart.tsx @@ -1,6 +1,7 @@ import { useCallback, useMemo, useRef, useState } from 'react'; +import Link from 'next/link'; import cx from 'classnames'; -import { UnstyledButton } from '@mantine/core'; +import { HoverCard, Text, UnstyledButton } from '@mantine/core'; import { IconDownload, IconTextWrap } from '@tabler/icons-react'; import { flexRender, @@ -20,11 +21,14 @@ import { useVirtualizer } from '@tanstack/react-virtual'; import { CsvExportButton } from './components/CsvExportButton'; import TableHeader from './components/DBTable/TableHeader'; import { useCsvExport } from './hooks/useCsvExport'; +import type { RowAction } from './hooks/useOnClickLinkBuilder'; import { useBrandDisplayName } from './theme/ThemeProvider'; import { UNDEFINED_WIDTH } from './tableUtils'; import type { NumberFormat } from './types'; import { formatNumber } from './utils'; +import focusStyles from '../styles/focus.module.scss'; + export type TableVariant = 'default' | 'muted'; // Arbitrary limit to prevent OOM crashes for very large result sets. Most result sets should be paginated anyway. @@ -34,7 +38,8 @@ export const Table = ({ data, groupColumnName, columns, - onRowClick, + getRowAction, + getRowSearchLink, tableBottom, enableClientSideSorting = false, sorting, @@ -53,7 +58,18 @@ export const Table = ({ sortingFn?: SortingFnOption; }[]; groupColumnName?: string; - onRowClick?: (row: any, e?: React.MouseEvent) => void; + // Returns the row click destination + a hover-hint description. When + // set, the cell becomes an wrapped in a HoverCard. The resolved + // URL goes straight in the href so the browser handles cmd-click, + // middle-click, right-click, status bar preview, and keyboard + // activation natively. Rows whose templates fail (`url: null`) fall + // back to a click handler that fires a notification, preserving the + // pre-existing #2140 / #2141 / #2146 / #2148 behavior. + getRowAction?: (row: any) => RowAction; + // Legacy single-tile drilldown: bare URL, no hint, no HoverCard. + // Used outside the dashboard onClick path (event side panel, services + // dashboard, etc.). Only consulted when getRowAction is not provided. + getRowSearchLink?: (row: any) => string | null; tableBottom?: React.ReactNode; enableClientSideSorting?: boolean; sorting: SortingState; @@ -146,40 +162,64 @@ export const Table = ({ 'text-truncate': !wrapLinesEnabled, }); - if (onRowClick) { - return ( -
onRowClick(row.original, e)} - // Middle-click (button === 1) fires onAuxClick but NOT - // onClick on non-anchor elements. - onAuxClick={e => { - if (e.button === 1) { - e.preventDefault(); - onRowClick(row.original, e); - } - }} - // Suppress the browser's middle-click autoscroll cursor on non-anchor elements. - onMouseDown={e => { - if (e.button === 1) e.preventDefault(); - }} - onKeyDown={e => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - onRowClick(row.original); - } - }} + // Native covers cmd-click (new tab), middle-click + // (new tab), right-click ("Open in New Tab" / "Copy Link + // Address"), Enter key activation, and the browser status + // bar URL preview. No manual handlers required. + const linkClassName = cx( + className, + 'd-block text-reset text-decoration-none', + focusStyles.focusRing, + ); + + if (getRowAction) { + const action = getRowAction(row.original); + const anchor = action.url ? ( + + {formattedValue} + + ) : ( + // Row's templates failed to resolve. Render a real anchor + // so the hover hint still appears and the click fires the + // existing error toast. preventDefault inside onClickError + // stops navigation including for cmd-click / middle-click. + // The proper "muted row + warning icon" preempt state is + // tracked as AC8 in the discoverability outcome. + {formattedValue} -
+ + ); + + return ( + + {anchor} + + {action.description} + + ); } + if (getRowSearchLink) { + const url = getRowSearchLink(row.original); + if (url) { + return ( + + {formattedValue} + + ); + } + } + return
{formattedValue}
; }, size: diff --git a/packages/app/src/components/DBTableChart.tsx b/packages/app/src/components/DBTableChart.tsx index 3862424c20..708f63e1da 100644 --- a/packages/app/src/components/DBTableChart.tsx +++ b/packages/app/src/components/DBTableChart.tsx @@ -1,5 +1,4 @@ import { useCallback, useMemo, useState } from 'react'; -import { useRouter } from 'next/router'; import { ClickHouseQueryError } from '@hyperdx/common-utils/dist/clickhouse'; import { isRatioChartConfig } from '@hyperdx/common-utils/dist/core/renderChartConfig'; import { @@ -217,33 +216,11 @@ export default function DBTableChart({ queriedConfig, ]); - const getOnClickLink = useOnClickLinkBuilder({ + const getRowAction = useOnClickLinkBuilder({ onClick: config.onClick, dateRange: queriedConfig.dateRange, }); - const router = useRouter(); - const hasOnRowClick = !!getOnClickLink || !!getRowSearchLink; - const onRowClick = useCallback( - (row: Record, e?: React.MouseEvent) => { - const url = getOnClickLink - ? getOnClickLink(row) - : getRowSearchLink - ? getRowSearchLink(row) - : null; - - // getOnClickLink will surface any errors notifications - if (!url) return; - - if (e?.metaKey || e?.ctrlKey || e?.button === 1) { - window.open(url, '_blank'); - } else { - router.push(url); - } - }, - [getOnClickLink, getRowSearchLink, router], - ); - return ( {isLoading && !data ? ( @@ -285,7 +262,8 @@ export default function DBTableChart({ ({ + useSources: jest.fn(), +})); + +jest.mock('@/dashboard', () => ({ + useDashboards: jest.fn(), +})); + +jest.mock('@mantine/notifications', () => ({ + notifications: { show: jest.fn() }, +})); + +const dateRange: [Date, Date] = [ + new Date('2026-01-01T00:00:00Z'), + new Date('2026-01-01T01:00:00Z'), +]; + +describe('useOnClickLinkBuilder', () => { + beforeEach(() => { + jest.clearAllMocks(); + jest.mocked(useSources).mockReturnValue({ + data: [ + { + id: 'src_1', + name: 'HyperDX Logs', + kind: 'log', + connection: 'c', + from: { databaseName: 'd', tableName: 't' }, + timestampValueExpression: 'Timestamp', + }, + ], + } as any); + jest.mocked(useDashboards).mockReturnValue({ + data: [{ id: 'dash_1', name: 'API Latency Drilldown', tiles: [] }], + } as any); + }); + + it('returns null when no onClick is configured', () => { + const { result } = renderHook(() => + useOnClickLinkBuilder({ onClick: undefined, dateRange }), + ); + expect(result.current).toBeNull(); + }); + + it('returns a row resolver that includes the resolved source name in the description', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + }; + const { result } = renderHook(() => + useOnClickLinkBuilder({ onClick, dateRange }), + ); + expect(result.current).not.toBeNull(); + const action = result.current!({}); + expect(action.description).toBe('Search HyperDX Logs'); + expect(action.url).toMatch(/^\/search\?source=src_1&/); + expect(action.onClickError).toBeUndefined(); + }); + + it('returns a row resolver that includes the resolved dashboard name in the description', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + }; + const { result } = renderHook(() => + useOnClickLinkBuilder({ onClick, dateRange }), + ); + const action = result.current!({}); + expect(action.description).toBe('Open dashboard "API Latency Drilldown"'); + expect(action.url).toMatch(/^\/dashboards\/dash_1\?/); + }); + + it('encodes a row resolution failure as url: null with a click handler that fires a notification', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'template', template: '{{MissingColumn}}' }, + whereLanguage: 'sql', + }; + const { result } = renderHook(() => + useOnClickLinkBuilder({ onClick, dateRange }), + ); + const action = result.current!({ Src: 'Logs' }); + expect(action.url).toBeNull(); + expect(action.description).toBe('Open in search'); + expect(action.onClickError).toBeInstanceOf(Function); + + // Render time is silent: no toast fires while computing the URL. + expect(jest.mocked(notifications.show)).not.toHaveBeenCalled(); + + // Clicking the failing row fires the toast. + const preventDefault = jest.fn(); + action.onClickError!({ preventDefault } as any); + expect(preventDefault).toHaveBeenCalledTimes(1); + expect(jest.mocked(notifications.show)).toHaveBeenCalledWith( + expect.objectContaining({ + color: 'red', + title: 'Link error', + message: "Row has no column 'MissingColumn'", + }), + ); + }); +}); diff --git a/packages/app/src/hooks/useOnClickLinkBuilder.ts b/packages/app/src/hooks/useOnClickLinkBuilder.ts index 6ce705621d..978c3efbf0 100644 --- a/packages/app/src/hooks/useOnClickLinkBuilder.ts +++ b/packages/app/src/hooks/useOnClickLinkBuilder.ts @@ -1,5 +1,6 @@ import { useMemo } from 'react'; import { + describeOnClick, renderOnClickDashboard, renderOnClickSearch, } from '@hyperdx/common-utils/dist/core/linkUrlBuilder'; @@ -10,10 +11,36 @@ import { useDashboards } from '@/dashboard'; import { useSources } from '@/source'; /** - * Returns a function that, given some row data, produces a URL for the - * configured onClick action. Errors (unresolved names, malformed templates, - * unknown sources) surface as Mantine toast notifications; the function - * returns null in error cases. + * The action a row click should take. `url` is set when the row's + * templates resolved successfully; the cell wrapper renders a real + * `` so the browser handles cmd-click, middle-click, + * right-click, status bar preview, and keyboard activation natively. + * + * When `url` is null the row's templates failed (missing column, stale + * source ID, etc.). The cell still renders as a clickable anchor so the + * hover hint and click-shows-error UX from the original onClick work + * (#2140 / #2141 / #2146 / #2148) keep working. `onClickError` is the + * click handler that fires the notification. + */ +export type RowAction = { + url: string | null; + description: string; + onClickError?: (e: React.MouseEvent) => void; +}; + +/** + * Returns a function that, given some row data, produces a `RowAction` + * describing the configured onClick: the destination URL (when the row + * resolves), a one-line description for the hover hint, and an + * on-click error handler (when the row's templates failed). + * + * Returns null when no `onClick` is configured on the chart, so callers + * can fall back to the legacy `getRowSearchLink` drilldown. + * + * Render-time calls to the returned function are silent: a failing row + * produces `url: null` without raising a Mantine notification. The + * notification fires only when the user clicks the failing row (via + * `onClickError`), mirroring the pre-existing behavior. */ export function useOnClickLinkBuilder({ onClick, @@ -21,49 +48,50 @@ export function useOnClickLinkBuilder({ }: { onClick: OnClick | undefined; dateRange: [Date, Date]; -}): ((row: Record) => string | null) | null { +}): ((row: Record) => RowAction) | null { const { data: sources } = useSources(); const { data: dashboards } = useDashboards(); - const [sourceIdsByName, sourceIds] = useMemo(() => { - const map = new Map(); - const set = new Set(); + const [sourceIdsByName, sourceIds, sourceNamesById] = useMemo(() => { + const idsByName = new Map(); + const ids = new Set(); + const namesById = new Map(); for (const s of sources?.filter(isSearchableSource) ?? []) { - set.add(s.id); + ids.add(s.id); + namesById.set(s.id, s.name); - const existing = map.get(s.name); + const existing = idsByName.get(s.name); if (existing) existing.push(s.id); - else map.set(s.name, [s.id]); + else idsByName.set(s.name, [s.id]); } - return [map, set]; + return [idsByName, ids, namesById]; }, [sources]); - const [dashboardIdsByName, dashboardIds] = useMemo(() => { - const map = new Map(); - const set = new Set(); + const [dashboardIdsByName, dashboardIds, dashboardNamesById] = useMemo(() => { + const idsByName = new Map(); + const ids = new Set(); + const namesById = new Map(); for (const d of dashboards ?? []) { - set.add(d.id); + ids.add(d.id); + namesById.set(d.id, d.name); - const existing = map.get(d.name); + const existing = idsByName.get(d.name); if (existing) existing.push(d.id); - else map.set(d.name, [d.id]); + else idsByName.set(d.name, [d.id]); } - return [map, set]; + return [idsByName, ids, namesById]; }, [dashboards]); return useMemo(() => { if (!onClick) return null; - return (row: Record) => { - const showError = (message: string) => { - notifications.show({ - id: message, - color: 'red', - title: 'Link error', - message, - }); - }; + const description = describeOnClick({ + onClick, + sourceNamesById, + dashboardNamesById, + }); + return (row: Record): RowAction => { const renderResult = onClick.type === 'search' ? renderOnClickSearch({ @@ -81,19 +109,33 @@ export function useOnClickLinkBuilder({ dateRange, }); - if (!renderResult.ok) { - showError(renderResult.error); - return null; + if (renderResult.ok) { + return { url: renderResult.url, description }; } - return renderResult.url; + const errorMessage = renderResult.error; + return { + url: null, + description, + onClickError: (e: React.MouseEvent) => { + e.preventDefault(); + notifications.show({ + id: errorMessage, + color: 'red', + title: 'Link error', + message: errorMessage, + }); + }, + }; }; }, [ onClick, sourceIds, sourceIdsByName, + sourceNamesById, dateRange, dashboardIds, dashboardIdsByName, + dashboardNamesById, ]); } diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 7f962520ac..6ed68891e6 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -689,15 +689,15 @@ export class DashboardPage { } /** - * Click the first row's first cell of a table tile. Each cell contains a - * div[role="link"] that owns the onRowClick handler — click that directly - * to trigger the configured action. + * Click the first row's first cell of a table tile. Each cell contains an + * `` (Next.js Link) carrying the configured action's URL. Click the + * anchor directly to trigger the navigation. */ async clickFirstTableRow(tileIndex = 0) { await this.getTile(tileIndex) .locator('table tbody tr') .first() - .locator('div[role="link"]') + .locator('a[href]') .first() .click(); } diff --git a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts index 9305dac1c5..ee7b64b7f5 100644 --- a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts +++ b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts @@ -1,5 +1,6 @@ import type { OnClickDashboard, OnClickSearch } from '../../types'; import { + describeOnClick, renderOnClickDashboard, renderOnClickSearch, validateOnClickTemplate, @@ -953,3 +954,76 @@ describe('validateOnClickTemplate', () => { expect(() => validateOnClickTemplate(onClick)).not.toThrow(); }); }); + +describe('describeOnClick', () => { + const sourceNamesById = new Map([['src_1', 'HyperDX Logs']]); + const dashboardNamesById = new Map([ + ['dash_1', 'API Latency Drilldown'], + ]); + + it('describes a search action targeting a known source by ID with the resolved name', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Search HyperDX Logs'); + }); + + it('falls back to a generic verb form when the search source ID is not in the lookup', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_missing' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Open in search'); + }); + + it('falls back to a generic verb form for template-mode search targets', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'template', template: '{{Src}}' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Open in search'); + }); + + it('describes a dashboard action targeting a known dashboard by ID with the resolved name', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Open dashboard "API Latency Drilldown"'); + }); + + it('falls back to a generic verb form when the dashboard ID is not in the lookup', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_missing' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Open dashboard'); + }); + + it('falls back to a generic verb form for template-mode dashboard targets', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'template', template: '{{Dashboard}}' }, + whereLanguage: 'sql', + }; + expect( + describeOnClick({ onClick, sourceNamesById, dashboardNamesById }), + ).toBe('Open dashboard'); + }); +}); diff --git a/packages/common-utils/src/core/linkUrlBuilder.ts b/packages/common-utils/src/core/linkUrlBuilder.ts index 62c16daef3..bab6f41c56 100644 --- a/packages/common-utils/src/core/linkUrlBuilder.ts +++ b/packages/common-utils/src/core/linkUrlBuilder.ts @@ -238,6 +238,47 @@ export function renderOnClickDashboard({ return { ok: true, url: `/dashboards/${dashboardId}?${params.toString()}` }; } +/** + * Build a one-line, row-independent description of an OnClick action, + * suitable for a hover hint shown before the user commits to the click. + * + * Returns one of four shapes: + * - `Search ` when targeting a known source by ID. + * - `Open in search` when targeting a source by template, or when the + * ID does not resolve to a known source. + * - `Open dashboard ""` when targeting a known dashboard by ID. + * - `Open dashboard` when targeting a dashboard by template, or when + * the ID does not resolve to a known dashboard. + * + * Template-mode targets resolve a different name per row (the name + * is itself templated); the generic verb form keeps the hint stable + * across rows. Per-row scope preview belongs to a separate hint + * surface and is not the responsibility of this helper. + */ +export function describeOnClick({ + onClick, + sourceNamesById, + dashboardNamesById, +}: { + onClick: OnClick; + sourceNamesById: Map; + dashboardNamesById: Map; +}): string { + if (onClick.type === 'search') { + if (onClick.target.mode === 'id') { + const name = sourceNamesById.get(onClick.target.id); + if (name) return `Search ${name}`; + } + return 'Open in search'; + } + + if (onClick.target.mode === 'id') { + const name = dashboardNamesById.get(onClick.target.id); + if (name) return `Open dashboard "${name}"`; + } + return 'Open dashboard'; +} + /** Throws if the given OnClick includes a template with invalid syntax */ export function validateOnClickTemplate(onClick: OnClick) { if (onClick.target.mode === 'template') { From 5def1437937eaf42a9ed0470d9cdceac289087e5 Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Wed, 20 May 2026 23:11:54 +0000 Subject: [PATCH 2/3] fix(app): address deep review feedback on table tile onClick P0 fix: - Failure rows now render as in the body loop instead of mounting one HoverCard per cell. Hovering anywhere on the row shows a stable hint; cursor movement between cells no longer flickers a different dropdown per column. - getRowAction is now computed once per row and the hook caches by row reference (WeakMap). The handlebars + URLSearchParams build no longer reruns for every cell on every render. - now uses prefetch={false}. Virtualized scroll no longer triggers an N-row prefetch storm against /search? and /dashboards routes the user usually never opens in bulk. - Added a component-level test (HDXMultiSeriesTableChart.test.tsx) that exercises both success and failure branches end to end: asserts the anchor / button shape, the onClickError wiring, the row-level HoverCard hint visibility, and the legacy getRowSearchLink fallback. Smaller cleanups: - describeOnClick now does an explicit exhaustiveness check on the OnClick discriminator so adding a third variant fails type-check. - onClickError is typed as React.MouseEvent. - The cell wrapper carries data-testid="dashboard-table-row-action" on both branches; e2e page object resolves by testid so future inline column links don't steal the click. - Hook test asserts URL params via URLSearchParams instead of position-coupled regex. --- packages/app/src/HDXMultiSeriesTableChart.tsx | 99 ++++++---- .../HDXMultiSeriesTableChart.test.tsx | 180 ++++++++++++++++++ .../__tests__/useOnClickLinkBuilder.test.tsx | 21 +- .../app/src/hooks/useOnClickLinkBuilder.ts | 20 +- .../tests/e2e/page-objects/DashboardPage.ts | 9 +- .../common-utils/src/core/linkUrlBuilder.ts | 15 +- 6 files changed, 297 insertions(+), 47 deletions(-) create mode 100644 packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx diff --git a/packages/app/src/HDXMultiSeriesTableChart.tsx b/packages/app/src/HDXMultiSeriesTableChart.tsx index 8012cbb781..6ae6158237 100644 --- a/packages/app/src/HDXMultiSeriesTableChart.tsx +++ b/packages/app/src/HDXMultiSeriesTableChart.tsx @@ -166,46 +166,49 @@ export const Table = ({ // (new tab), right-click ("Open in New Tab" / "Copy Link // Address"), Enter key activation, and the browser status // bar URL preview. No manual handlers required. - const linkClassName = cx( + const interactiveClassName = cx( className, - 'd-block text-reset text-decoration-none', + 'd-block text-reset text-decoration-none w-100 text-start', focusStyles.focusRing, ); if (getRowAction) { + // The hook memoizes per-row results internally, so calling + // this once per cell is cheap and the row-level HoverCard + // in the loop below sees the same identity. const action = getRowAction(row.original); - const anchor = action.url ? ( - - {formattedValue} - - ) : ( - // Row's templates failed to resolve. Render a real anchor - // so the hover hint still appears and the click fires the - // existing error toast. preventDefault inside onClickError - // stops navigation including for cmd-click / middle-click. - // The proper "muted row + warning icon" preempt state is - // tracked as AC8 in the discoverability outcome. - + {formattedValue} + + ); + } + // Row's templates failed to resolve. Use a real ); } @@ -213,7 +216,12 @@ export const Table = ({ const url = getRowSearchLink(row.original); if (url) { return ( - + {formattedValue} ); @@ -367,7 +375,11 @@ export const Table = ({ )} {items.map(virtualRow => { const row = rows[virtualRow.index] as TableRow; - return ( + // Compute the action once per row so the row-level HoverCard + // sees the same description and per-cell renders share the + // memoized result from useOnClickLinkBuilder. + const rowAction = getRowAction ? getRowAction(row.original) : null; + const tr = ( ); + // Row-level HoverCard so the hint position stays stable as the + // cursor moves between cells in the same row. Mantine's + // HoverCard.Target clones the and merges its hover ref + // with the virtualizer's measureElement ref. + if (rowAction) { + return ( + + {tr} + + {rowAction.description} + + + ); + } + return tr; })} {paddingBottom > 0 && ( diff --git a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx new file mode 100644 index 0000000000..482ef7b4d3 --- /dev/null +++ b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx @@ -0,0 +1,180 @@ +import React from 'react'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; + +import { Table } from '@/HDXMultiSeriesTableChart'; + +// Next.js Link needs a router context; the router isn't exercised +// because we never trigger client-side navigation in these tests. +jest.mock('next/router', () => ({ + useRouter: () => ({ + push: jest.fn(), + prefetch: jest.fn(), + pathname: '/', + query: {}, + }), +})); + +// JSDOM gives the table container 0×0 dimensions, so the real +// virtualizer renders no rows. Stub it to render every row as if +// visible. The hint / button / link assertions don't depend on +// virtualization correctness. +jest.mock('@tanstack/react-virtual', () => ({ + useVirtualizer: ({ count }: { count: number }) => ({ + getVirtualItems: () => + Array.from({ length: count }, (_, index) => ({ + index, + key: index, + start: index * 58, + end: (index + 1) * 58, + size: 58, + })), + getTotalSize: () => count * 58, + measureElement: () => {}, + options: { scrollMargin: 0 }, + }), +})); + +const baseColumns = [ + { + id: 'service', + dataKey: 'ServiceName', + displayName: 'Service', + }, + { + id: 'count', + dataKey: 'Count', + displayName: 'Count', + }, +]; + +const baseData = [{ ServiceName: 'web', Count: 10 }]; + +describe('HDXMultiSeriesTableChart
', () => { + describe('getRowAction success path', () => { + it('renders the row click cell as a real with the resolved URL', () => { + const getRowAction = jest.fn().mockReturnValue({ + url: '/search?source=src_1&where=', + description: 'Search HyperDX Logs', + }); + + renderWithMantine( +
{}} + />, + ); + + const links = screen.getAllByTestId('dashboard-table-row-action'); + expect(links.length).toBeGreaterThan(0); + links.forEach(link => { + expect(link.tagName).toBe('A'); + expect(link.getAttribute('href')).toContain('/search?source=src_1'); + }); + }); + }); + + describe('getRowAction failure path', () => { + it('renders the cell as a
{}} + />, + ); + + const buttons = screen.getAllByTestId('dashboard-table-row-action'); + expect(buttons.length).toBeGreaterThan(0); + buttons.forEach(btn => { + expect(btn.tagName).toBe('BUTTON'); + // No href anywhere, so cmd-click / middle-click / right-click + // "Open in New Tab" can't silently open the page. + expect(btn.hasAttribute('href')).toBe(false); + }); + + fireEvent.click(buttons[0]); + expect(onClickError).toHaveBeenCalledTimes(1); + }); + + it('reveals the row-level HoverCard hint when the cursor hovers the row', async () => { + const getRowAction = jest.fn().mockReturnValue({ + url: null, + description: 'Search HyperDX Logs', + onClickError: jest.fn(), + }); + + renderWithMantine( +
{}} + />, + ); + + const row = screen.getByText('web').closest('tr')!; + fireEvent.mouseEnter(row); + + await waitFor( + () => { + expect(screen.getByText('Search HyperDX Logs')).toBeInTheDocument(); + }, + { timeout: 1000 }, + ); + }); + }); + + describe('legacy getRowSearchLink fallback', () => { + it('renders the cell as a Link without a HoverCard when only getRowSearchLink is provided', () => { + const getRowSearchLink = jest + .fn() + .mockReturnValue('/search?source=legacy&where='); + + renderWithMantine( +
{}} + />, + ); + + const links = screen.getAllByTestId('dashboard-table-row-action'); + links.forEach(link => { + expect(link.tagName).toBe('A'); + expect(link.getAttribute('href')).toContain('/search?source=legacy'); + }); + }); + }); + + describe('no action configured', () => { + it('renders plain cells with no Link, button, or HoverCard', () => { + renderWithMantine( +
{}} + />, + ); + + expect( + screen.queryAllByTestId('dashboard-table-row-action'), + ).toHaveLength(0); + }); + }); +}); diff --git a/packages/app/src/hooks/__tests__/useOnClickLinkBuilder.test.tsx b/packages/app/src/hooks/__tests__/useOnClickLinkBuilder.test.tsx index 14370407e4..2fbc4abb8b 100644 --- a/packages/app/src/hooks/__tests__/useOnClickLinkBuilder.test.tsx +++ b/packages/app/src/hooks/__tests__/useOnClickLinkBuilder.test.tsx @@ -66,7 +66,9 @@ describe('useOnClickLinkBuilder', () => { expect(result.current).not.toBeNull(); const action = result.current!({}); expect(action.description).toBe('Search HyperDX Logs'); - expect(action.url).toMatch(/^\/search\?source=src_1&/); + const params = new URLSearchParams(action.url!.split('?')[1]); + expect(action.url!.startsWith('/search?')).toBe(true); + expect(params.get('source')).toBe('src_1'); expect(action.onClickError).toBeUndefined(); }); @@ -81,7 +83,22 @@ describe('useOnClickLinkBuilder', () => { ); const action = result.current!({}); expect(action.description).toBe('Open dashboard "API Latency Drilldown"'); - expect(action.url).toMatch(/^\/dashboards\/dash_1\?/); + expect(action.url!.startsWith('/dashboards/dash_1?')).toBe(true); + }); + + it('caches results per row reference so repeated calls share the same RowAction', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + }; + const { result } = renderHook(() => + useOnClickLinkBuilder({ onClick, dateRange }), + ); + const row = { ServiceName: 'web' }; + const first = result.current!(row); + const second = result.current!(row); + expect(first).toBe(second); }); it('encodes a row resolution failure as url: null with a click handler that fires a notification', () => { diff --git a/packages/app/src/hooks/useOnClickLinkBuilder.ts b/packages/app/src/hooks/useOnClickLinkBuilder.ts index 978c3efbf0..6b4841ffbd 100644 --- a/packages/app/src/hooks/useOnClickLinkBuilder.ts +++ b/packages/app/src/hooks/useOnClickLinkBuilder.ts @@ -25,7 +25,7 @@ import { useSources } from '@/source'; export type RowAction = { url: string | null; description: string; - onClickError?: (e: React.MouseEvent) => void; + onClickError?: (e: React.MouseEvent) => void; }; /** @@ -91,7 +91,13 @@ export function useOnClickLinkBuilder({ dashboardNamesById, }); - return (row: Record): RowAction => { + // Cache results by row reference so the same row producing N cells + // doesn't re-run handlebars rendering + URLSearchParams construction + // N times per render. The WeakMap is scoped to this closure, so a new + // memo (different onClick / sources / dashboards) starts fresh. + const cache = new WeakMap, RowAction>(); + + const compute = (row: Record): RowAction => { const renderResult = onClick.type === 'search' ? renderOnClickSearch({ @@ -117,7 +123,7 @@ export function useOnClickLinkBuilder({ return { url: null, description, - onClickError: (e: React.MouseEvent) => { + onClickError: (e: React.MouseEvent) => { e.preventDefault(); notifications.show({ id: errorMessage, @@ -128,6 +134,14 @@ export function useOnClickLinkBuilder({ }, }; }; + + return (row: Record): RowAction => { + const cached = cache.get(row); + if (cached) return cached; + const result = compute(row); + cache.set(row, result); + return result; + }; }, [ onClick, sourceIds, diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 6ed68891e6..652e9c5332 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -689,15 +689,16 @@ export class DashboardPage { } /** - * Click the first row's first cell of a table tile. Each cell contains an - * `` (Next.js Link) carrying the configured action's URL. Click the - * anchor directly to trigger the navigation. + * Click the first row's first cell of a table tile. Each cell carries + * `data-testid="dashboard-table-row-action"` on either the success + * anchor (Next.js Link) or the failure button, so a single selector + * matches both branches. */ async clickFirstTableRow(tileIndex = 0) { await this.getTile(tileIndex) .locator('table tbody tr') .first() - .locator('a[href]') + .locator('[data-testid="dashboard-table-row-action"]') .first() .click(); } diff --git a/packages/common-utils/src/core/linkUrlBuilder.ts b/packages/common-utils/src/core/linkUrlBuilder.ts index bab6f41c56..89dfcccd04 100644 --- a/packages/common-utils/src/core/linkUrlBuilder.ts +++ b/packages/common-utils/src/core/linkUrlBuilder.ts @@ -271,12 +271,17 @@ export function describeOnClick({ } return 'Open in search'; } - - if (onClick.target.mode === 'id') { - const name = dashboardNamesById.get(onClick.target.id); - if (name) return `Open dashboard "${name}"`; + if (onClick.type === 'dashboard') { + if (onClick.target.mode === 'id') { + const name = dashboardNamesById.get(onClick.target.id); + if (name) return `Open dashboard "${name}"`; + } + return 'Open dashboard'; } - return 'Open dashboard'; + // Exhaustiveness check: adding a new OnClickSchema variant must + // also extend describeOnClick. + const _exhaustive: never = onClick; + return _exhaustive; } /** Throws if the given OnClick includes a template with invalid syntax */ From 19c0204e28ae7859cc9df745cfcd4d27534e1767 Mon Sep 17 00:00:00 2001 From: Alex Fedotyev <61838744+alex-fedotyev@users.noreply.github.com> Date: Fri, 22 May 2026 00:29:41 +0000 Subject: [PATCH 3/3] fix(app): cell-level hover hint placement and visual parity for failure rows Addresses three rendering issues Drew flagged on #2321 after approval: - Failure rows now share a single .cellButton SCSS reset with the success-row , so the cell wrapper renders identically across branches. The user-agent button defaults (padding, font, color, text-align, line-height) were leaking through. - Hover hint is suppressed when action.url === null. The previous state showed an "Open in search" hint on a row whose click would only fire an error toast, which lied to the user. - Row-level HoverCard (position="top") replaced with Tooltip.Floating wrapping the . The hint now follows the cursor at the cell rather than anchoring at row-center-top. Tests updated: the failure-row hint test now asserts the hint is absent; a success-row positive test was added. Both branches get a data-shape attribute so the test assertions don't couple to the CSS-module hash. --- packages/app/src/HDXMultiSeriesTableChart.tsx | 42 ++++++++++--------- .../HDXMultiSeriesTableChart.test.tsx | 41 ++++++++++++++---- packages/app/styles/focus.module.scss | 22 ++++++++++ 3 files changed, 79 insertions(+), 26 deletions(-) diff --git a/packages/app/src/HDXMultiSeriesTableChart.tsx b/packages/app/src/HDXMultiSeriesTableChart.tsx index 6ae6158237..78641fe7e9 100644 --- a/packages/app/src/HDXMultiSeriesTableChart.tsx +++ b/packages/app/src/HDXMultiSeriesTableChart.tsx @@ -1,7 +1,7 @@ import { useCallback, useMemo, useRef, useState } from 'react'; import Link from 'next/link'; import cx from 'classnames'; -import { HoverCard, Text, UnstyledButton } from '@mantine/core'; +import { Tooltip, UnstyledButton } from '@mantine/core'; import { IconDownload, IconTextWrap } from '@tabler/icons-react'; import { flexRender, @@ -187,6 +187,7 @@ export const Table = ({ prefetch={false} className={interactiveClassName} data-testid="dashboard-table-row-action" + data-shape="link" > {formattedValue} @@ -197,15 +198,17 @@ export const Table = ({ // New Tab" stay disabled (a # anchor would silently open // a meaningless new tab on auxclick before our onClick // handler runs). The button still surfaces the existing - // notification toast on left-click; the proper "muted row - // + warning icon" preempt state is tracked as AC8. + // notification toast on left-click. focusStyles.cellButton + // resets the user-agent button defaults (padding, font, + // color, text-align, line-height) so the wrapper renders + // identically to the success-row . return ( @@ -221,6 +224,7 @@ export const Table = ({ prefetch={false} className={interactiveClassName} data-testid="dashboard-table-row-action" + data-shape="link" > {formattedValue} @@ -398,24 +402,24 @@ export const Table = ({ })} ); - // Row-level HoverCard so the hint position stays stable as the - // cursor moves between cells in the same row. Mantine's - // HoverCard.Target clones the and merges its hover ref - // with the virtualizer's measureElement ref. - if (rowAction) { + // Row-level Tooltip.Floating so the hint follows the cursor + // and anchors near the cell the user is over, not at the row's + // center-top. Tooltip.Floating tracks the cursor via floating-ui + // and stays within the row's bounding box; one tooltip per row + // means no flicker as the cursor moves between cells. + // + // The hint is suppressed when rowAction.url === null because + // the click only fires an error toast on those rows, so showing + // "Open in search" would mislead the user. + if (rowAction && rowAction.url) { return ( - - {tr} - - {rowAction.description} - - + {tr} + ); } return tr; diff --git a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx index 482ef7b4d3..c63e72941b 100644 --- a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx +++ b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx @@ -71,9 +71,37 @@ describe('HDXMultiSeriesTableChart
', () => { expect(links.length).toBeGreaterThan(0); links.forEach(link => { expect(link.tagName).toBe('A'); + expect(link.getAttribute('data-shape')).toBe('link'); expect(link.getAttribute('href')).toContain('/search?source=src_1'); }); }); + + it('reveals the hint at the cursor when hovering a success row', async () => { + const getRowAction = jest.fn().mockReturnValue({ + url: '/search?source=src_1&where=', + description: 'Search HyperDX Logs', + }); + + renderWithMantine( +
{}} + />, + ); + + const row = screen.getByText('web').closest('tr')!; + fireEvent.mouseEnter(row); + + await waitFor( + () => { + expect(screen.getByText('Search HyperDX Logs')).toBeInTheDocument(); + }, + { timeout: 1000 }, + ); + }); }); describe('getRowAction failure path', () => { @@ -99,6 +127,7 @@ describe('HDXMultiSeriesTableChart
', () => { expect(buttons.length).toBeGreaterThan(0); buttons.forEach(btn => { expect(btn.tagName).toBe('BUTTON'); + expect(btn.getAttribute('data-shape')).toBe('button'); // No href anywhere, so cmd-click / middle-click / right-click // "Open in New Tab" can't silently open the page. expect(btn.hasAttribute('href')).toBe(false); @@ -108,7 +137,7 @@ describe('HDXMultiSeriesTableChart
', () => { expect(onClickError).toHaveBeenCalledTimes(1); }); - it('reveals the row-level HoverCard hint when the cursor hovers the row', async () => { + it('does not reveal a hint when the row action has no url', async () => { const getRowAction = jest.fn().mockReturnValue({ url: null, description: 'Search HyperDX Logs', @@ -128,12 +157,9 @@ describe('HDXMultiSeriesTableChart
', () => { const row = screen.getByText('web').closest('tr')!; fireEvent.mouseEnter(row); - await waitFor( - () => { - expect(screen.getByText('Search HyperDX Logs')).toBeInTheDocument(); - }, - { timeout: 1000 }, - ); + // Give the tooltip a chance to mount; assert it never does. + await new Promise(resolve => setTimeout(resolve, 250)); + expect(screen.queryByText('Search HyperDX Logs')).not.toBeInTheDocument(); }); }); @@ -156,6 +182,7 @@ describe('HDXMultiSeriesTableChart
', () => { const links = screen.getAllByTestId('dashboard-table-row-action'); links.forEach(link => { expect(link.tagName).toBe('A'); + expect(link.getAttribute('data-shape')).toBe('link'); expect(link.getAttribute('href')).toContain('/search?source=legacy'); }); }); diff --git a/packages/app/styles/focus.module.scss b/packages/app/styles/focus.module.scss index 0f21db49cb..a7501701ca 100644 --- a/packages/app/styles/focus.module.scss +++ b/packages/app/styles/focus.module.scss @@ -3,3 +3,25 @@ outline-offset: 2px; border-radius: var(--mantine-radius-xs); } + +// Used by the failure-row fallback