From 4e34db1c7f48d97c547ad5914b6f8122374e3f74 Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Fri, 29 May 2026 23:35:34 +0000 Subject: [PATCH 1/7] fix(app): prevent stranded tooltip in virtualised table rows [HDX-4405] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-row Tooltip.Floating instances got stranded in their Portal when a virtual row unmounted before onMouseLeave fired — a race that occurs when the mouse moves rapidly across a TanStack Virtual list. Fix: replace one Tooltip.Floating per virtual row with a single shared Tooltip.Floating wrapping the whole . The floating tooltip now lives on , which never unmounts, so its Portal-rendered content can never be left open after the triggering element disappears. Row-level onMouseEnter/onMouseLeave handlers update a shared hoveredRowDescription state; the tooltip's disabled prop gates visibility so rows without a resolved URL (error-toast branch) never show a hint. A tbody-level onMouseLeave acts as a safety net to clear the description if a rapid mouse move causes a row to unmount before its own leave handler fires. Test: adds a regression test that verifies the tooltip disappears on mouseLeave (the stranded-tooltip scenario). --- packages/app/src/HDXMultiSeriesTableChart.tsx | 129 ++++++++++-------- .../HDXMultiSeriesTableChart.test.tsx | 38 ++++++ 2 files changed, 113 insertions(+), 54 deletions(-) diff --git a/packages/app/src/HDXMultiSeriesTableChart.tsx b/packages/app/src/HDXMultiSeriesTableChart.tsx index 78641fe7e9..7e90df44dd 100644 --- a/packages/app/src/HDXMultiSeriesTableChart.tsx +++ b/packages/app/src/HDXMultiSeriesTableChart.tsx @@ -307,6 +307,15 @@ export const Table = ({ ); const [wrapLinesEnabled, setWrapLinesEnabled] = useState(false); + // Single shared tooltip state for all virtual rows. Maintaining one + // Tooltip.Floating at the level (rather than one per virtual row) + // prevents the tooltip from getting stranded when a row unmounts before + // onMouseLeave fires — a race that happens when the mouse moves rapidly + // across a virtualised list. See HDX-4405. + const [hoveredRowDescription, setHoveredRowDescription] = useState< + string | null + >(null); + const { csvData } = useCsvExport( truncatedData, columns.map(col => ({ @@ -371,65 +380,77 @@ export const Table = ({ ))} - - {paddingTop > 0 && ( - - - - )} - {items.map(virtualRow => { - const row = rows[virtualRow.index] as TableRow; - // 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.getVisibleCells().map(cell => { - return ( - - {flexRender( - cell.column.columnDef.cell, - cell.getContext(), - )} - - ); - })} + {/* Single Tooltip.Floating wrapping the whole so the hint + follows the cursor without being tied to the lifecycle of any + individual virtual row. Per-row Tooltip.Floating instances get + stranded in the Portal when a row unmounts before onMouseLeave + fires (rapid mouse movement in a virtualised list). With this + approach the tooltip state lives on , which never unmounts, + and the label is updated by onMouseEnter/onMouseLeave handlers + on each . See HDX-4405. */} + + {/* onMouseLeave on is a safety net: if a virtual row + unmounts before its own onMouseLeave fires (rapid movement), + the cursor leaving the table body still clears the description. */} + setHoveredRowDescription(null)}> + {paddingTop > 0 && ( + + - ); - // 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) { + )} + {items.map(virtualRow => { + const row = rows[virtualRow.index] as TableRow; + // Compute the action once per row so per-cell renders share + // the memoized result from useOnClickLinkBuilder. + const rowAction = getRowAction + ? getRowAction(row.original) + : null; + // Only surface a hint when the URL resolved successfully. + // Rows whose templates failed only fire an error toast on + // click, so showing "Open in search" would mislead the user. + const hintDescription = + rowAction?.url != null ? rowAction.description : null; return ( - setHoveredRowDescription(hintDescription) + : undefined + } + onMouseLeave={ + hintDescription != null + ? () => setHoveredRowDescription(null) + : undefined + } > - {tr} - + {row.getVisibleCells().map(cell => { + return ( + + {flexRender( + cell.column.columnDef.cell, + cell.getContext(), + )} + + ); + })} + ); - } - return tr; - })} - {paddingBottom > 0 && ( - - - - )} - + })} + {paddingBottom > 0 && ( + + + + )} + + {isTruncated && (
diff --git a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx index c63e72941b..f56a880623 100644 --- a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx +++ b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx @@ -102,6 +102,44 @@ describe('HDXMultiSeriesTableChart ', () => { { timeout: 1000 }, ); }); + + it('hides the hint after mouseLeave so tooltip cannot get stranded (HDX-4405)', async () => { + // Regression test: the tooltip must disappear when the row is left. + // Previously each virtual row mounted its own Tooltip.Floating; if the + // row unmounted before onMouseLeave fired (rapid mouse movement), the + // Portal-rendered tooltip stayed visible. The fix moves the single + // Tooltip.Floating to so its state never gets stranded. + const getRowAction = jest.fn().mockReturnValue({ + url: '/search?source=src_1&where=', + description: 'Search HyperDX Logs', + }); + + renderWithMantine( +
{}} + />, + ); + + const row = screen.getByText('web').closest('tr')!; + + // Show tooltip + fireEvent.mouseEnter(row); + await waitFor(() => + expect(screen.getByText('Search HyperDX Logs')).toBeInTheDocument(), + ); + + // Leave the row — tooltip must disappear + fireEvent.mouseLeave(row); + await waitFor(() => + expect( + screen.queryByText('Search HyperDX Logs'), + ).not.toBeInTheDocument(), + ); + }); }); describe('getRowAction failure path', () => { From 56727f03a0dbb36c83499db2b1638537fc473929 Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Fri, 29 May 2026 23:49:52 +0000 Subject: [PATCH 2/7] test(e2e): add tooltip regression test for HDX-4405 Verifies that the Tooltip.Floating hint appears on row hover and disappears when the mouse moves away, covering the stranded-tooltip regression introduced by the per-row Tooltip.Floating in PR #2321. Changes: - dashboard-table-linking.spec.ts: new 'Tooltip hint appears on hover and disappears on mouse-leave' test. Creates a table tile with a Search row-click action, hovers the first row to confirm the hint appears, then moves the mouse away to confirm it hides cleanly. - DashboardPage.ts: adds getFirstTableRow() and hoverFirstTableRowAndGetTooltip() page-object helpers. --- .../features/dashboard-table-linking.spec.ts | 44 ++++++++++++++++++ .../tests/e2e/page-objects/DashboardPage.ts | 45 +++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index d4f127b2aa..c13d6d6507 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -751,5 +751,49 @@ test.describe( await expect(dashboardPage.ignoredUrlFiltersBanner).toBeHidden(); }); }); + + test('Tooltip hint appears on hover and disappears on mouse-leave — no stranded tooltip (HDX-4405)', async ({ + page, + }) => { + // Regression test for HDX-4405: Tooltip.Floating instances mounted + // per virtual row were getting stranded in their Portal when the row + // unmounted before onMouseLeave fired (rapid mouse movement). The fix + // moves a single shared Tooltip.Floating to so its state is + // never tied to a virtual row's lifecycle. + const ts = Date.now(); + + await test.step('Create a table tile with a Search row-click action', async () => { + await addTableTile(`E2E Tooltip ${ts}`); + await dashboardPage.chartEditor.openRowClickDrawer(); + await dashboardPage.chartEditor.setRowClickMode('Search'); + await dashboardPage.chartEditor.fillRowClickTemplate( + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardPage.chartEditor.applyRowClickDrawer(); + await dashboardPage.saveTile(); + }); + + await test.step('Set time range to Last 6 hours so rows render', async () => { + await dashboardPage.timePicker.selectRelativeTime('Last 6 hours'); + }); + + await dashboardPage.waitForTableTileRows(0); + + await test.step('Hover over first row — tooltip hint must appear', async () => { + await dashboardPage.hoverFirstTableRowAndGetTooltip(0); + // hoverFirstTableRowAndGetTooltip already asserts visibility before + // returning; reaching here means the tooltip appeared successfully. + }); + + await test.step('Move mouse away from the table — tooltip must disappear', async () => { + // Move to a neutral area well outside the table (top-left corner of + // the viewport). The tbody onMouseLeave clears hoveredRowDescription, + // which sets disabled=true on the single shared Tooltip.Floating, + // hiding it via inline display:none. + await page.mouse.move(10, 10); + const tooltipText = page.getByText(/Open in search/, { exact: false }); + await expect(tooltipText).toBeHidden({ timeout: 3000 }); + }); + }); }, ); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 93c14f1e6a..bfb585f192 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -970,6 +970,51 @@ export class DashboardPage { .click(); } + /** + * Return the first data row () of the table in the + * given tile. Used for hover-based interactions (e.g. tooltip tests). + */ + getFirstTableRow(tileIndex = 0): Locator { + return this.getTile(tileIndex) + .locator('table tbody tr[data-index]') + .first(); + } + + /** + * Hover over the first data row of a table tile and wait for the + * floating tooltip to appear. Returns the tooltip locator so callers + * can make further assertions. + * + * Tooltip.Floating renders its content inside a Portal at the document + * body level. Mantine uses CSS modules (hashed classes) so we locate the + * floating popup by text content that matches the known hint patterns used + * by describeOnClick (e.g. "Open in search", "Search ", + * "Open dashboard"). The tooltip is shown via `display: block` on the + * Portal div, so Playwright's `toBeVisible` checks that correctly. + */ + async hoverFirstTableRowAndGetTooltip(tileIndex = 0): Promise { + const row = this.getFirstTableRow(tileIndex); + await row.hover(); + // Trigger a mousemove inside the row so Tooltip.Floating's internal + // mousemove handler has a coordinate to position against. + const box = await row.boundingBox(); + if (box) { + await this.page.mouse.move( + box.x + box.width / 2, + box.y + box.height / 2, + ); + } + // Tooltip.Floating renders a div inside a Portal (appended to document + // body). When open, Mantine sets `display: block` on it inline; when + // closed, `display: none`. We can't rely on a stable Mantine class name + // (CSS modules hash them), so match by the tooltip text content that + // describeOnClick produces: "Search ", "Open in search", etc. + // These phrases don't appear in the table cells themselves. + const tooltip = this.page.getByText(/Open in search/, { exact: false }); + await tooltip.waitFor({ state: 'visible', timeout: 5000 }); + return tooltip; + } + /** * Locator for the Mantine toast raised by useOnClickLinkBuilder when the * configured onClick action fails (unknown source, missing row column, etc). From c87549819a62d2f8991eb600c85994ebf064feaa Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Fri, 29 May 2026 23:59:58 +0000 Subject: [PATCH 3/7] style: fix prettier formatting in DashboardPage.ts --- packages/app/tests/e2e/page-objects/DashboardPage.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index bfb585f192..2aece518cd 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -999,10 +999,7 @@ export class DashboardPage { // mousemove handler has a coordinate to position against. const box = await row.boundingBox(); if (box) { - await this.page.mouse.move( - box.x + box.width / 2, - box.y + box.height / 2, - ); + await this.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); } // Tooltip.Floating renders a div inside a Portal (appended to document // body). When open, Mantine sets `display: block` on it inline; when From 24d8552cde285de19b4e6ae4b94736481d2d6eda Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Sat, 30 May 2026 01:04:27 +0000 Subject: [PATCH 4/7] fix(app): address code review feedback on HDX-4405 tooltip fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2 — correctness: - Store hoveredVirtualIndex (row index) instead of hoveredRowDescription (string) so the label re-derives via useMemo on every render. If the virtualiser drops or replaces the hovered row (scroll, auto-refetch, rapid cursor movement) the new row's action is shown immediately; stale text from the unmounted row can never persist. - Rows with url:null or empty description now correctly disable the tooltip regardless of what the prior hover state was. P2 — testing: - Replaced the simple mouseLeave regression test with one that exercises the actual race: hover index 0 (URL row), then enter index 1 (no-URL row) without firing mouseLeave on index 0. Asserts tooltip hides by inspecting the Mantine inline display style on the Portal container. P3 — maintainability: - label={hoveredRowDescription} — drop the ?? '' fallback that obscured the disabled gating relationship (Mantine accepts null as ReactNode). - disabled={!hoveredRowDescription} — guards against empty-string descriptions that would mount a zero-width floating tooltip. - Unconditional onMouseEnter/onMouseLeave on each with a hoisted clearHovered useCallback, replacing the conditional handler pattern that forked JSX unnecessarily. - Collapse dual comment blocks into one rationale at the Tooltip.Floating call site; the state declaration now has a single-line pointer. - Add data-testid="row-action-hint" to the Tooltip.Floating label span so E2E tests locate the tooltip by stable testid rather than by hard-coupled copy strings. --- packages/app/src/HDXMultiSeriesTableChart.tsx | 73 ++++++++++--------- .../HDXMultiSeriesTableChart.test.tsx | 65 +++++++++++------ .../features/dashboard-table-linking.spec.ts | 13 ++-- .../tests/e2e/page-objects/DashboardPage.ts | 11 +-- 4 files changed, 90 insertions(+), 72 deletions(-) diff --git a/packages/app/src/HDXMultiSeriesTableChart.tsx b/packages/app/src/HDXMultiSeriesTableChart.tsx index 7e90df44dd..604e0c3140 100644 --- a/packages/app/src/HDXMultiSeriesTableChart.tsx +++ b/packages/app/src/HDXMultiSeriesTableChart.tsx @@ -307,14 +307,32 @@ export const Table = ({ ); const [wrapLinesEnabled, setWrapLinesEnabled] = useState(false); - // Single shared tooltip state for all virtual rows. Maintaining one - // Tooltip.Floating at the level (rather than one per virtual row) - // prevents the tooltip from getting stranded when a row unmounts before - // onMouseLeave fires — a race that happens when the mouse moves rapidly - // across a virtualised list. See HDX-4405. - const [hoveredRowDescription, setHoveredRowDescription] = useState< - string | null - >(null); + // Store the virtual index of the hovered row (not its description string) + // so the label re-derives on every render. If the virtualiser replaces the + // row at that index (scroll re-virtualisation, auto-refetch) the label + // reflects the new row immediately rather than showing stale text. Storing + // the index also makes the safety-net `onMouseLeave` on correct: + // it sets null rather than the prior row's description. See HDX-4405. + const [hoveredVirtualIndex, setHoveredVirtualIndex] = useState( + null, + ); + + // Derive the label from whichever row currently occupies hoveredVirtualIndex. + // Returns null when no row is hovered, the index is out of range, or the + // row's action has no URL (error-toast rows show no hint). + const hoveredRowDescription = useMemo(() => { + if (hoveredVirtualIndex == null) return null; + const virtualRow = items.find(v => v.index === hoveredVirtualIndex); + if (!virtualRow) return null; + const row = rows[virtualRow.index] as TableRow | undefined; + if (!row) return null; + const rowAction = getRowAction ? getRowAction(row.original) : null; + return rowAction?.url != null && rowAction.description + ? rowAction.description + : null; + }, [hoveredVirtualIndex, items, rows, getRowAction]); + + const clearHovered = useCallback(() => setHoveredVirtualIndex(null), []); const { csvData } = useCsvExport( truncatedData, @@ -386,17 +404,20 @@ export const Table = ({ stranded in the Portal when a row unmounts before onMouseLeave fires (rapid mouse movement in a virtualised list). With this approach the tooltip state lives on , which never unmounts, - and the label is updated by onMouseEnter/onMouseLeave handlers - on each . See HDX-4405. */} + and the label is re-derived from hoveredVirtualIndex each render so + scroll re-virtualisation never shows stale text. See HDX-4405. */} {hoveredRowDescription} + } withinPortal - disabled={hoveredRowDescription == null} + disabled={!hoveredRowDescription} > {/* onMouseLeave on is a safety net: if a virtual row - unmounts before its own onMouseLeave fires (rapid movement), - the cursor leaving the table body still clears the description. */} - setHoveredRowDescription(null)}> + unmounts before its own onMouseLeave fires (rapid cursor + movement or re-virtualisation), leaving the table body still + clears the hovered index. */} + {paddingTop > 0 && ( setHoveredRowDescription(hintDescription) - : undefined - } - onMouseLeave={ - hintDescription != null - ? () => setHoveredRowDescription(null) - : undefined - } + onMouseEnter={() => setHoveredVirtualIndex(virtualRow.index)} + onMouseLeave={clearHovered} > {row.getVisibleCells().map(cell => { return ( diff --git a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx index f56a880623..8947a731dc 100644 --- a/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx +++ b/packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx @@ -103,20 +103,31 @@ describe('HDXMultiSeriesTableChart
@@ -404,32 +425,14 @@ export const Table = ({ )} {items.map(virtualRow => { const row = rows[virtualRow.index] as TableRow; - // Compute the action once per row so per-cell renders share - // the memoized result from useOnClickLinkBuilder. - const rowAction = getRowAction - ? getRowAction(row.original) - : null; - // Only surface a hint when the URL resolved successfully. - // Rows whose templates failed only fire an error toast on - // click, so showing "Open in search" would mislead the user. - const hintDescription = - rowAction?.url != null ? rowAction.description : null; return (
', () => { ); }); - it('hides the hint after mouseLeave so tooltip cannot get stranded (HDX-4405)', async () => { - // Regression test: the tooltip must disappear when the row is left. - // Previously each virtual row mounted its own Tooltip.Floating; if the - // row unmounted before onMouseLeave fired (rapid mouse movement), the - // Portal-rendered tooltip stayed visible. The fix moves the single - // Tooltip.Floating to so its state never gets stranded. - const getRowAction = jest.fn().mockReturnValue({ - url: '/search?source=src_1&where=', - description: 'Search HyperDX Logs', - }); + it('hides the hint when the hovered virtual index maps to a no-URL row (HDX-4405)', async () => { + // Regression for the virtualiser race: the hovered can unmount + // before its onMouseLeave fires (rapid movement / data refresh). The + // old per-row Tooltip.Floating was stranded in the Portal; the fix + // stores a virtual index and re-derives the label via useMemo each + // render. When the row at that index no longer has a URL, the tooltip + // hides without a leave event from the (now-gone) element. + // + // We verify the key invariant structurally: hovering index 0 (URL row) + // shows the hint; hovering index 1 (no-URL row) hides it — no + // mouseLeave fires between the two enterevents, simulating the + // cursor jumping over the table faster than leave events dispatch. + const multiRowData = [ + { ServiceName: 'web', Count: 10 }, + { ServiceName: 'api', Count: 5 }, + ]; + const getRowAction = jest.fn((row: { ServiceName: string }) => + row.ServiceName === 'web' + ? { url: '/search?source=src_1&where=', description: 'Search Logs' } + : { url: null, description: '', onClickError: jest.fn() }, + ); renderWithMantine(
', () => { />, ); - const row = screen.getByText('web').closest('tr')!; + const webRow = screen.getByText('web').closest('tr')!; + const apiRow = screen.getByText('api').closest('tr')!; - // Show tooltip - fireEvent.mouseEnter(row); - await waitFor(() => - expect(screen.getByText('Search HyperDX Logs')).toBeInTheDocument(), - ); + // Hover the URL row — tooltip must appear + fireEvent.mouseEnter(webRow); + await waitFor(() => { + const hint = screen.getByTestId('row-action-hint'); + const tooltipBox = hint.closest('[style*="display"]'); + expect(tooltipBox?.style.display).toBe('block'); + }); - // Leave the row — tooltip must disappear - fireEvent.mouseLeave(row); - await waitFor(() => - expect( - screen.queryByText('Search HyperDX Logs'), - ).not.toBeInTheDocument(), - ); + // Hover the no-URL row WITHOUT firing mouseLeave on the first row. + // This simulates the cursor jumping faster than leave events dispatch. + fireEvent.mouseEnter(apiRow); + + // The label must derive to null (apiRow has url:null) so tooltip hides. + await waitFor(() => { + const hint = screen.getByTestId('row-action-hint'); + const tooltipBox = hint.closest('[style*="display"]'); + expect(tooltipBox?.style.display).toBe('none'); + }); }); }); diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index c13d6d6507..1fb89a64f2 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -786,13 +786,14 @@ test.describe( }); await test.step('Move mouse away from the table — tooltip must disappear', async () => { - // Move to a neutral area well outside the table (top-left corner of - // the viewport). The tbody onMouseLeave clears hoveredRowDescription, - // which sets disabled=true on the single shared Tooltip.Floating, - // hiding it via inline display:none. + // Move to a neutral area well outside the table. The + // onMouseLeave safety net clears hoveredVirtualIndex, which makes + // hoveredRowDescription derive to null, disabling the shared + // Tooltip.Floating (display:none in the Portal). await page.mouse.move(10, 10); - const tooltipText = page.getByText(/Open in search/, { exact: false }); - await expect(tooltipText).toBeHidden({ timeout: 3000 }); + await expect(page.getByTestId('row-action-hint')).toBeHidden({ + timeout: 3000, + }); }); }); }, diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 2aece518cd..4f5d2778c4 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -1001,13 +1001,10 @@ export class DashboardPage { if (box) { await this.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); } - // Tooltip.Floating renders a div inside a Portal (appended to document - // body). When open, Mantine sets `display: block` on it inline; when - // closed, `display: none`. We can't rely on a stable Mantine class name - // (CSS modules hash them), so match by the tooltip text content that - // describeOnClick produces: "Search ", "Open in search", etc. - // These phrases don't appear in the table cells themselves. - const tooltip = this.page.getByText(/Open in search/, { exact: false }); + // The Tooltip.Floating label is a . + // The portal div uses display:none when disabled, so Playwright's + // toBeVisible() correctly reflects the open/closed state. + const tooltip = this.page.getByTestId('row-action-hint'); await tooltip.waitFor({ state: 'visible', timeout: 5000 }); return tooltip; } From b5f91a3c349cc394874be85da4dfd66d9dc8098b Mon Sep 17 00:00:00 2001 From: Alex Fedotyev Date: Sat, 30 May 2026 03:48:49 +0000 Subject: [PATCH 5/7] feat: number tile conditional color rules (HDX-4406) Add Grafana-style threshold color rules to number tiles. Users can define an ordered list of conditions in Display Settings; the last matching rule's color wins, falling back to the static tile color, then to the default text color. Schema (common-utils): - Add ColorConditionSchema (discriminated union: gt/gte/lt/lte/between/ eq/neq/contains/startsWith/endsWith/regex) - Add colorRules to SharedChartSettingsSchema (optional, max 10) Resolver (app): - evaluateColorCondition: per-rule evaluation with type guards - resolveConditionalColor: last-match-wins, null-safe UI (app): - ColorRulesEditor: sortable rule list via @dnd-kit/sortable, per-operator value inputs, ColorSwatchInput, add/delete controls - ChartDisplaySettingsDrawer: conditional colors section gated on DisplayType.Number, below existing static color picker - EditTimeChartForm: wire colorRules through displaySettings and handleUpdateDisplaySettings - DBNumberChart: evaluate rules against raw numeric value at render time Tests: - Schema positive/negative tests (common-utils) - evaluateColorCondition + resolveConditionalColor unit tests (app) - ColorRulesEditor RTL tests (add/delete/operator/render) - DBNumberChart integration test: success/warning/error scenario No API schema or external-API changes (separate follow-up ticket). --- packages/app/src/__tests__/utils.test.ts | 303 +++++++++++++- .../components/ChartDisplaySettingsDrawer.tsx | 52 ++- .../app/src/components/ColorRulesEditor.tsx | 385 ++++++++++++++++++ .../DBEditTimeChartForm/EditTimeChartForm.tsx | 6 + packages/app/src/components/DBNumberChart.tsx | 30 +- .../__tests__/ColorRulesEditor.test.tsx | 175 ++++++++ .../__tests__/DBNumberChart.test.tsx | 109 +++++ packages/app/src/utils.ts | 78 ++++ .../common-utils/src/__tests__/types.test.ts | 284 +++++++++++++ packages/common-utils/src/types.ts | 71 ++++ 10 files changed, 1478 insertions(+), 15 deletions(-) create mode 100644 packages/app/src/components/ColorRulesEditor.tsx create mode 100644 packages/app/src/components/__tests__/ColorRulesEditor.test.tsx create mode 100644 packages/common-utils/src/__tests__/types.test.ts diff --git a/packages/app/src/__tests__/utils.test.ts b/packages/app/src/__tests__/utils.test.ts index bedea9c851..fc986f06e1 100644 --- a/packages/app/src/__tests__/utils.test.ts +++ b/packages/app/src/__tests__/utils.test.ts @@ -1,4 +1,8 @@ -import { NumericUnit, TSource } from '@hyperdx/common-utils/dist/types'; +import { + ColorCondition, + NumericUnit, + TSource, +} from '@hyperdx/common-utils/dist/types'; import { SortingState } from '@tanstack/react-table'; import { act, renderHook } from '@testing-library/react'; @@ -6,6 +10,7 @@ import { MetricsDataType, NumberFormat } from '../types'; import * as utils from '../utils'; import { COLORS, + evaluateColorCondition, formatAttributeClause, formatDurationMs, formatDurationMsCompact, @@ -16,6 +21,7 @@ import { mapKeyBy, orderByStringToSortingState, parseTimestampToMs, + resolveConditionalColor, sortingStateToOrderByString, stripTrailingSlash, useQueryHistory, @@ -1265,3 +1271,298 @@ describe('getColorFromCSSToken', () => { } }); }); + +// ─── evaluateColorCondition ─────────────────────────────────────────────────── + +describe('evaluateColorCondition', () => { + describe('numeric ordered operators', () => { + it('gt: returns true when value > rule.value', () => { + const rule: ColorCondition = { + operator: 'gt', + value: 10, + color: 'chart-1', + }; + expect(evaluateColorCondition(11, rule)).toBe(true); + expect(evaluateColorCondition(10, rule)).toBe(false); + expect(evaluateColorCondition(9, rule)).toBe(false); + }); + + it('gte: returns true when value >= rule.value', () => { + const rule: ColorCondition = { + operator: 'gte', + value: 10, + color: 'chart-1', + }; + expect(evaluateColorCondition(10, rule)).toBe(true); + expect(evaluateColorCondition(11, rule)).toBe(true); + expect(evaluateColorCondition(9, rule)).toBe(false); + }); + + it('lt: returns true when value < rule.value', () => { + const rule: ColorCondition = { + operator: 'lt', + value: 10, + color: 'chart-1', + }; + expect(evaluateColorCondition(9, rule)).toBe(true); + expect(evaluateColorCondition(10, rule)).toBe(false); + }); + + it('lte: returns true when value <= rule.value', () => { + const rule: ColorCondition = { + operator: 'lte', + value: 10, + color: 'chart-1', + }; + expect(evaluateColorCondition(10, rule)).toBe(true); + expect(evaluateColorCondition(9, rule)).toBe(true); + expect(evaluateColorCondition(11, rule)).toBe(false); + }); + + it('numeric operators return false for string values', () => { + const rule: ColorCondition = { + operator: 'gt', + value: 10, + color: 'chart-1', + }; + expect(evaluateColorCondition('15', rule)).toBe(false); + }); + }); + + describe('between operator', () => { + it('returns true when value is within [lo, hi]', () => { + const rule: ColorCondition = { + operator: 'between', + value: [10, 100], + color: 'chart-1', + }; + expect(evaluateColorCondition(50, rule)).toBe(true); + expect(evaluateColorCondition(10, rule)).toBe(true); + expect(evaluateColorCondition(100, rule)).toBe(true); + expect(evaluateColorCondition(9, rule)).toBe(false); + expect(evaluateColorCondition(101, rule)).toBe(false); + }); + + it('handles inverted range (first > second) by normalising to [lo, hi]', () => { + const rule: ColorCondition = { + operator: 'between', + value: [100, 10], + color: 'chart-1', + }; + expect(evaluateColorCondition(50, rule)).toBe(true); + expect(evaluateColorCondition(5, rule)).toBe(false); + }); + + it('returns false for string values', () => { + const rule: ColorCondition = { + operator: 'between', + value: [10, 100], + color: 'chart-1', + }; + expect(evaluateColorCondition('50', rule)).toBe(false); + }); + }); + + describe('eq / neq operators', () => { + it('eq: returns true on strict equality (number)', () => { + const rule: ColorCondition = { + operator: 'eq', + value: 5, + color: 'chart-1', + }; + expect(evaluateColorCondition(5, rule)).toBe(true); + expect(evaluateColorCondition(6, rule)).toBe(false); + }); + + it('eq: returns true on strict equality (string)', () => { + const rule: ColorCondition = { + operator: 'eq', + value: 'CRIT', + color: 'chart-1', + }; + expect(evaluateColorCondition('CRIT', rule)).toBe(true); + expect(evaluateColorCondition('crit', rule)).toBe(false); + }); + + it('eq: cross-type mismatch returns false ("5" vs 5)', () => { + const rule: ColorCondition = { + operator: 'eq', + value: '5', + color: 'chart-1', + }; + expect(evaluateColorCondition(5, rule)).toBe(false); + }); + + it('neq: returns true when value differs', () => { + const rule: ColorCondition = { + operator: 'neq', + value: 0, + color: 'chart-1', + }; + expect(evaluateColorCondition(1, rule)).toBe(true); + expect(evaluateColorCondition(0, rule)).toBe(false); + }); + }); + + describe('string operators', () => { + it('contains: returns true when string includes value', () => { + const rule: ColorCondition = { + operator: 'contains', + value: 'error', + color: 'chart-error', + }; + expect(evaluateColorCondition('fatal error occurred', rule)).toBe(true); + expect(evaluateColorCondition('warning', rule)).toBe(false); + }); + + it('contains: returns false for number values', () => { + const rule: ColorCondition = { + operator: 'contains', + value: 'error', + color: 'chart-error', + }; + expect(evaluateColorCondition(42, rule)).toBe(false); + }); + + it('startsWith: matches prefix', () => { + const rule: ColorCondition = { + operator: 'startsWith', + value: 'ERR', + color: 'chart-error', + }; + expect(evaluateColorCondition('ERR_500', rule)).toBe(true); + expect(evaluateColorCondition('WARN_ERR', rule)).toBe(false); + }); + + it('endsWith: matches suffix', () => { + const rule: ColorCondition = { + operator: 'endsWith', + value: 'CRIT', + color: 'chart-error', + }; + expect(evaluateColorCondition('ALERT_CRIT', rule)).toBe(true); + expect(evaluateColorCondition('CRIT_OK', rule)).toBe(false); + }); + + it('regex: matches valid pattern', () => { + const rule: ColorCondition = { + operator: 'regex', + value: '^err.*', + color: 'chart-error', + }; + expect(evaluateColorCondition('error123', rule)).toBe(true); + expect(evaluateColorCondition('warning', rule)).toBe(false); + }); + + it('regex: bad pattern returns false without throwing', () => { + const rule = { + operator: 'regex' as const, + value: '[invalid', + color: 'chart-error' as const, + }; + expect(() => evaluateColorCondition('test', rule)).not.toThrow(); + expect(evaluateColorCondition('test', rule)).toBe(false); + }); + }); +}); + +// ─── resolveConditionalColor ────────────────────────────────────────────────── + +describe('resolveConditionalColor', () => { + it('returns fallback when rules is undefined', () => { + expect(resolveConditionalColor(50, undefined, 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('returns fallback when rules is empty', () => { + expect(resolveConditionalColor(50, [], 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('returns fallback when value is null', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 0, color: 'chart-warning' }, + ]; + expect(resolveConditionalColor(null, rules, 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('returns fallback when value is undefined', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 0, color: 'chart-warning' }, + ]; + expect(resolveConditionalColor(undefined, rules, 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('returns the matching rule color when one rule matches', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 100, color: 'chart-warning' }, + ]; + expect(resolveConditionalColor(200, rules, 'chart-success')).toBe( + 'chart-warning', + ); + }); + + it('returns the LAST matching rule color (last-match-wins)', () => { + // value 1000: both rules match; last (chart-error) wins + const rules: ColorCondition[] = [ + { operator: 'gte', value: 100, color: 'chart-warning' }, + { operator: 'gte', value: 500, color: 'chart-error' }, + ]; + expect(resolveConditionalColor(1000, rules, 'chart-success')).toBe( + 'chart-error', + ); + }); + + it('returns fallback when no rule matches', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 100, color: 'chart-warning' }, + { operator: 'gte', value: 500, color: 'chart-error' }, + ]; + // value 50: no rule matches, return fallback + expect(resolveConditionalColor(50, rules, 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('covers the DBNumberChart success/warning/error scenario', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 100, color: 'chart-warning' }, + { operator: 'gte', value: 500, color: 'chart-error' }, + ]; + // 50 → no match → static color + expect(resolveConditionalColor(50, rules, 'chart-success')).toBe( + 'chart-success', + ); + // 200 → rule 1 matches, rule 2 doesn't → chart-warning + expect(resolveConditionalColor(200, rules, 'chart-success')).toBe( + 'chart-warning', + ); + // 1000 → both match → last match = chart-error + expect(resolveConditionalColor(1000, rules, 'chart-success')).toBe( + 'chart-error', + ); + }); + + it('string rules do not match numeric values', () => { + const rules: ColorCondition[] = [ + { operator: 'contains', value: 'err', color: 'chart-error' }, + ]; + // numeric value, string rule: no match + expect(resolveConditionalColor(42, rules, 'chart-success')).toBe( + 'chart-success', + ); + }); + + it('returns undefined fallback when fallback is undefined and no rule matches', () => { + const rules: ColorCondition[] = [ + { operator: 'gte', value: 100, color: 'chart-warning' }, + ]; + expect(resolveConditionalColor(50, rules, undefined)).toBeUndefined(); + }); +}); diff --git a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx index 5f5f8005bc..12cb3eba8f 100644 --- a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx +++ b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx @@ -20,6 +20,12 @@ import { import { shouldFillNullsWithZero } from '@/ChartUtils'; import { FormatTime } from '@/useFormatTime'; +import { + attachLocalIds, + ColorRulesEditor, + ColorRuleWithId, + stripLocalIds, +} from './ColorRulesEditor'; import { ColorSwatchInput } from './ColorSwatchInput'; import { CheckBoxControlled } from './InputControlled'; import { DEFAULT_NUMBER_FORMAT, NumberFormatForm } from './NumberFormat'; @@ -31,10 +37,19 @@ export type ChartConfigDisplaySettings = Pick< | 'fillNulls' | 'compareToPreviousPeriod' | 'color' + | 'colorRules' > & { groupByColumnsOnLeft?: boolean; }; +/** + * Internal form shape: `colorRules` is stored with `localId`s for dnd-kit + * stability; they are stripped before the settings are passed to `onChange`. + */ +type DrawerFormValues = Omit & { + colorRules?: ColorRuleWithId[]; +}; + interface ChartDisplaySettingsDrawerProps { opened: boolean; settings: ChartConfigDisplaySettings; @@ -53,7 +68,7 @@ interface ChartDisplaySettingsDrawerProps { function applyDefaultSettings( settings: ChartConfigDisplaySettings, fallbackNumberFormat?: NumberFormat, -): ChartConfigDisplaySettings { +): DrawerFormValues { return { numberFormat: settings.numberFormat ?? fallbackNumberFormat ?? DEFAULT_NUMBER_FORMAT, @@ -65,6 +80,9 @@ function applyDefaultSettings( compareToPreviousPeriod: settings.compareToPreviousPeriod ?? false, groupByColumnsOnLeft: settings.groupByColumnsOnLeft ?? false, color: settings.color, + colorRules: settings.colorRules + ? attachLocalIds(settings.colorRules) + : undefined, }; } @@ -84,10 +102,9 @@ export default function ChartDisplaySettingsDrawer({ [settings, defaultNumberFormat], ); - const { control, handleSubmit, reset, setValue } = - useForm({ - defaultValues: appliedDefaults, - }); + const { control, handleSubmit, reset, setValue } = useForm({ + defaultValues: appliedDefaults, + }); useEffect(() => { reset(appliedDefaults); @@ -102,12 +119,24 @@ export default function ChartDisplaySettingsDrawer({ }, [onClose, reset, appliedDefaults]); const applyChanges = useCallback(() => { - handleSubmit(onChange)(); + handleSubmit(formValues => { + // Strip client-side localIds before passing rules to the config. + const { colorRules, ...rest } = formValues; + onChange({ + ...rest, + colorRules: colorRules ? stripLocalIds(colorRules) : undefined, + }); + })(); onClose(); }, [onChange, handleSubmit, onClose]); const resetToDefaults = useCallback(() => { - reset(applyDefaultSettings({}, defaultNumberFormat)); + reset( + applyDefaultSettings( + {} as ChartConfigDisplaySettings, + defaultNumberFormat, + ), + ); }, [reset, defaultNumberFormat]); const isTimeChart = @@ -199,6 +228,15 @@ export default function ChartDisplaySettingsDrawer({ )} /> + + ( + + )} + /> + )} diff --git a/packages/app/src/components/ColorRulesEditor.tsx b/packages/app/src/components/ColorRulesEditor.tsx new file mode 100644 index 0000000000..2de588e924 --- /dev/null +++ b/packages/app/src/components/ColorRulesEditor.tsx @@ -0,0 +1,385 @@ +import React, { useCallback } from 'react'; +import { + DndContext, + DragEndEvent, + MouseSensor, + TouchSensor, + useSensor, + useSensors, +} from '@dnd-kit/core'; +import { + arrayMove, + SortableContext, + useSortable, + verticalListSortingStrategy, +} from '@dnd-kit/sortable'; +import { CSS } from '@dnd-kit/utilities'; +import type { + ChartPaletteToken, + ColorCondition, +} from '@hyperdx/common-utils/dist/types'; +import { + ActionIcon, + Box, + Button, + Group, + NumberInput, + Select, + Stack, + Text, + TextInput, +} from '@mantine/core'; +import { IconGripVertical, IconTrash } from '@tabler/icons-react'; + +import { ColorSwatchInput } from './ColorSwatchInput'; + +// ─── Types ─────────────────────────────────────────────────────────────────── + +/** A ColorCondition with a client-side `localId` used as a stable dnd-kit key. */ +export type ColorRuleWithId = ColorCondition & { localId: string }; + +type ColorRulesEditorProps = { + value: ColorRuleWithId[]; + onChange: (rules: ColorRuleWithId[]) => void; +}; + +// ─── Operator options (number-tile subset) ──────────────────────────────────── + +const OPERATOR_OPTIONS = [ + { value: 'gt', label: '>' }, + { value: 'gte', label: '>=' }, + { value: 'lt', label: '<' }, + { value: 'lte', label: '<=' }, + { value: 'between', label: 'between' }, + { value: 'eq', label: '=' }, + { value: 'neq', label: '≠' }, +] as const; + +type NumericTileOperator = (typeof OPERATOR_OPTIONS)[number]['value']; + +/** Default rule added when the user clicks "Add rule". */ +function makeDefaultRule(): ColorRuleWithId { + return { + localId: crypto.randomUUID(), + operator: 'gt', + value: 0, + color: 'chart-1', + }; +} + +// ─── Single sortable rule row ───────────────────────────────────────────────── + +function SortableRuleRow({ + rule, + index, + onUpdate, + onDelete, +}: { + rule: ColorRuleWithId; + index: number; + onUpdate: (index: number, next: ColorRuleWithId) => void; + onDelete: (index: number) => void; +}) { + const { + attributes, + listeners, + setNodeRef, + transform, + transition, + isDragging, + } = useSortable({ id: rule.localId }); + + const style: React.CSSProperties = { + transform: CSS.Transform.toString(transform), + transition, + opacity: isDragging ? 0.5 : 1, + }; + + const handleOperatorChange = useCallback( + (op: string | null) => { + if (!op) return; + const operator = op as NumericTileOperator; + // When switching to/from 'between', reset the value to a valid shape. + if (operator === 'between') { + onUpdate(index, { + ...rule, + operator: 'between', + value: [0, 100], + } as ColorRuleWithId); + } else if (operator === 'eq' || operator === 'neq') { + // eq/neq accept number or string; default to number 0 + const currentVal = + rule.operator !== 'between' && typeof rule.value === 'number' + ? rule.value + : 0; + onUpdate(index, { + ...rule, + operator, + value: currentVal, + } as ColorRuleWithId); + } else { + // Numeric ordered: gt/gte/lt/lte need a number + const currentVal = + rule.operator !== 'between' && typeof rule.value === 'number' + ? rule.value + : 0; + onUpdate(index, { + ...rule, + operator, + value: currentVal, + } as ColorRuleWithId); + } + }, + [index, onUpdate, rule], + ); + + const handleColorChange = useCallback( + (color?: ChartPaletteToken) => { + onUpdate(index, { ...rule, color: color ?? 'chart-1' }); + }, + [index, onUpdate, rule], + ); + + const handleDelete = useCallback(() => onDelete(index), [index, onDelete]); + + // Value inputs differ by operator + let valueInputs: React.ReactNode; + if (rule.operator === 'between') { + const [lo, hi] = rule.value; + valueInputs = ( + + + onUpdate(index, { + ...rule, + operator: 'between', + value: [typeof v === 'number' ? v : 0, hi], + } as ColorRuleWithId) + } + aria-label={`Rule ${index + 1} lower bound`} + style={{ width: 72 }} + /> + + to + + + onUpdate(index, { + ...rule, + operator: 'between', + value: [lo, typeof v === 'number' ? v : 0], + } as ColorRuleWithId) + } + aria-label={`Rule ${index + 1} upper bound`} + style={{ width: 72 }} + /> + + ); + } else if (rule.operator === 'eq' || rule.operator === 'neq') { + // Accept text for eq/neq; if parseable as number convert it, else keep string + const displayVal = + typeof rule.value === 'number' + ? String(rule.value) + : (rule.value as string); + valueInputs = ( + { + const raw = e.currentTarget.value; + const num = Number(raw); + const coerced = + raw !== '' && !Number.isNaN(num) && Number.isFinite(num) + ? num + : raw; + onUpdate(index, { + ...rule, + operator: rule.operator, + value: coerced, + } as ColorRuleWithId); + }} + aria-label={`Rule ${index + 1} value`} + style={{ flex: 1 }} + /> + ); + } else { + // Numeric ordered: gt/gte/lt/lte + valueInputs = ( + + onUpdate(index, { + ...rule, + value: typeof v === 'number' ? v : 0, + } as ColorRuleWithId) + } + aria-label={`Rule ${index + 1} value`} + style={{ flex: 1 }} + /> + ); + } + + return ( + + + {/* Drag handle */} + + + + + {/* Operator */} +