[HDX-4405] fix(app): prevent stranded tooltip in virtualised table rows#2380
Conversation
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 <tbody>. The floating tooltip now lives on <tbody>, 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).
alex-fedotyev
added
the
ai-generated
🦋 Changeset detectedLatest commit: 53d12f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
E2E Test Results✅ All tests passed • 199 passed • 3 skipped • 1335s
Tests ran across 4 shards in parallel. |
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.
github-actions
Bot
added
the
review/tier-2
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Deep Review✅ No critical issues found. This is a self-contained UI fix: the cursor-following 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (9): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, adversarial, agent-native, learnings-researcher. Testing gaps:
|
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 <tr> 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.
MikeShi42
left a comment
MikeShi42
left a comment
There was a problem hiding this comment.
fix makes sense - though i think it's a bit "aggressive" that the tooltip follows the cursor so closely, would it make more sense to have the action on the right side of the table?
Replace the tbody-level Tooltip.Floating that tracks the cursor
with a hover-revealed trailing chevron icon anchored to the row's
last cell, wrapped in an anchored Mantine Tooltip. Mirrors the
.rowButtons pattern used by Search and Patterns.
Eliminates the stranded-tooltip bug class by construction: the
anchored Tooltip's open state is tied to the icon's lifecycle, so
a virtual row unmounting mid-hover can no longer leave a popup
behind. Addresses Mike's design feedback that the cursor-following
tooltip felt "a bit aggressive".
Drop hoveredVirtualIndex / hoveredRowDescription state, the
tbody-level Tooltip.Floating, the tbody onMouseLeave safety net,
and per-row onMouseEnter / onMouseLeave handlers. Add a per-row
trailing IconChevronRight in the last <td>, wrapped in a Next.js
Link with prefetch=false, tabIndex=-1, and aria-hidden so cmd /
middle / right-click semantics carry over but sequential focus
and screen-reader traversal aren't double-counted.
New HDXMultiSeriesTableChart.module.scss owns .tableRow,
.lastCell, .rowActionHint. The icon is opacity 0 by default and
fades in on .tableRow:hover; the last <td> gets position: relative
so the icon's absolute positioning anchors to the cell, not the
row (which is not a reliable positioning context across browsers).
Tooltip is anchored position=left with the established
DBRowTableIconButton conventions (openDelay 300, closeDelay 100,
withArrow, fz xs).
Suppressed on failure rows (rowAction.url null): the description
("Open in search", "Open dashboard X") would promise a destination
the click cannot deliver.
Tests:
- Unit tests rewritten: trailing chevron rendering, dashboard
variant tooltip, failure-row chevron absent, legacy
getRowSearchLink path chevron absent. Dropped the inline-style
closest() walk flagged in the deep review.
- E2E spec replaces the stranded-tooltip dismiss test with
opacity-gated visibility, anchored tooltip text on hover, and
click-on-chevron navigation.
- DashboardPage hint helper hovers the row to reveal the icon,
then hovers the icon to open the anchored Tooltip.
|
Pivoted the PR per Mike's review feedback. The cursor-following Tooltip.Floating is gone; the hint is now a hover-revealed trailing chevron in the last cell, with an anchored Mantine Tooltip describing the destination on hover. Pushed as Walking through Mike's concern: instead of patching the original surface I replaced it with one that doesn't have the same failure mode. The anchored Tooltip's open state is tied to the icon's lifecycle, so a virtual row unmounting mid-hover can't leave a popup behind by construction. Notes on the design choices, in case they help review:
PR-tier prediction: Tier 2 (132 production lines changed, single layer, no critical-path files). The previous commit on this branch ( Always-visible chevron column (Option A from the chat) is a richer surface and is tracked as a followup. It would add a permanent 32 px column on the right edge of the table, plus action-type-specific icons ( Validation locally:
|
|
@MikeShi42 - what do you think of this? I changed it to use less aggressive UX, while using |
|
@elizabetdev - what do you think about the |
|
@alex-fedotyev I think all the actionable rows should have a hover state And we already using the chevron right for the following (so I think we shouldn't be reuisng the same icon to indicate a different thing: 
The solutionSo the solution would be, on hover |
Address elizabetdev review feedback on PR #2380: - Hover state on actionable rows: switched the trailing-icon row from the global `bg-muted-hover` utility (`--color-bg-muted`) to a module-scoped `.actionableRow` class that hovers to `--color-bg-highlighted`. Non-actionable rows (failure or no action configured) keep `bg-muted-hover` as-is, so the visual delta between the two reinforces interactivity before the user sees the icon fade in. - Icon swap: `IconChevronRight` -> `IconArrowUpRight`. The chevron-right is already used elsewhere (sidebar group collapse affordances among others) for an unrelated interaction, so reusing it here was a collision. Arrow-up-right reads as "navigate elsewhere" without that collision. Tests: - Unit: two new positive / negative tests assert the row gains `actionableRow` on a resolved URL and falls back to `bg-muted-hover` otherwise. Existing trailing-icon tests rename to "arrow" wording; the `data-testid="row-action-hint"` selector is unchanged so test infra carries over. - E2E: step labels in `dashboard-table-linking.spec.ts` renamed to reference the arrow icon; selector is unchanged. - Changeset filename renamed `hdx-4405-trailing-chevron-hint.md` -> `hdx-4405-trailing-action-hint.md` and the patch note updated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Both fixed in d5d06c7. Icon: swapped Hover state: new Tests:
Vercel preview should update with the new icon + hover behavior shortly. |
|
@elizabetdev - is this how you imagined it? |
…2386) ## Summary This PR adds conditional color rules to number tiles. Define an ordered list of conditions; the last matching rule's color wins, so list rules in ascending priority order with the highest-priority rule last. If no rule matches, the tile falls back to its static color (set previously in #2265), then to the default text color. The schema is intentionally generic at the shared level so a future table-tile slice can attach per-column rules without a schema change. > **Stacking note:** the branch was cut from `alex/HDX-4405-fix-stranded-tooltip-in-virtual-table` (PR #2380), so the file list includes 4 unrelated files from that PR: `HDXMultiSeriesTableChart.tsx`, `HDXMultiSeriesTableChart.test.tsx`, `dashboard-table-linking.spec.ts`, `DashboardPage.ts`. Those files belong to #2380 and will drop out of this diff once #2380 merges. The HDX-4406 review surface is the other 10 files. ### What changed **common-utils (schema)** - Added `ColorConditionSchema` (discriminated union of `gt`, `gte`, `lt`, `lte`, `between`, `eq`, `neq`, `contains`, `startsWith`, `endsWith`, `regex` operators) - Added `colorRules` (optional, max 10) to `SharedChartSettingsSchema` alongside the existing `color` field **app (resolver)** - `evaluateColorCondition`: evaluates a single rule against a runtime value with proper type guards (numeric ops reject strings, string ops reject numbers, bad regex returns false) - `resolveConditionalColor`: iterates rules in order, last match wins, falls back to `fallback` (the static color) when nothing matches - String data values (ClickHouse returns `UInt64` counts as JSON strings) are coerced to numbers before rule evaluation **app (UI)** - New `ColorRulesEditor` component: sortable rule list via `@dnd-kit/sortable`, per-operator value inputs (single number, two-number range for `between`, text-or-number for `eq`/`neq`), `ColorSwatchInput` per rule, add/delete buttons; "Add rule" disables at 10 - `ChartDisplaySettingsDrawer`: added "Conditional colors" section gated on `DisplayType.Number`, placed below the existing static color picker - `EditTimeChartForm`: `colorRules` wired through `useWatch`, `displaySettings` memo, and `handleUpdateDisplaySettings` - `DBNumberChart`: resolves color via `resolveConditionalColor(rawValue, config.colorRules, config.color)` at render time **Tests** - Schema: positive + negative cases for all operators and array length constraints - Resolver: `evaluateColorCondition` per-operator, `resolveConditionalColor` including last-match-wins, string coercion, and the canonical success/warning/error scenario - UI: add/delete/operator shape/color swatch/render cases for `ColorRulesEditor` - Integration: `DBNumberChart` with `color: 'chart-success'` + two rules confirms value 50 -> success, 200 -> warning, 1000 -> error; also covers string UInt64 input No changes to `packages/api` schemas or external API (separate follow-up ticket, mirrors HDX-4378). ### Screenshots or video | Before | After | | :----- | :---- | | Number tile has only a static color picker | Number tile shows a "Conditional colors" section with add/reorder/delete rule controls below the static color picker | ### How to test on Vercel preview **Preview routes:** /dashboards **Steps:** 1. Open or create a dashboard and add a number tile (or edit an existing one). 2. Click the "Display Settings" button to open the drawer. 3. Confirm the "Conditional colors" section appears below the "Color" picker. 4. Click "Add rule" and verify a rule row appears with operator >, a value input, a color swatch, and a delete button. 5. Change the operator to >= and set value to 100; pick the Warning color swatch. 6. Click "Add rule" again; set operator >=, value 500; pick the Error color swatch. 7. Click "Apply". Verify the tile persists the rules (re-open Display Settings and confirm the two rules are still there). 8. With a tile value below 100, confirm it renders in the static (or default) color. 9. With a value between 100 and 499, confirm warning color. 10. With a value >= 500, confirm error color. 11. Click "Add rule" 8 more times to reach 10 rules total. Verify the button becomes disabled. 12. Drag a rule handle to reorder and click Apply; confirm the new order is reflected on re-open. ### References - Linear Issue: https://linear.app/clickhouse/issue/HDX-4406 - Related PRs: #2265 (static number-tile color picker, precedent); #2380 (tooltip fix; this branch is stacked on it)
…l-table #2386 (HDX-4406 number tile color rules) was branched from this PR while it still had the Tooltip.Floating intermediate, and was merged to main first. That intermediate is now obsolete: review feedback on this PR drove the pivot to a trailing-arrow hint anchored to a per-row icon, removing the hoveredVirtualIndex/hoveredRowDescription state, the tbody-level Tooltip.Floating, and the inline-style walk the deep review flagged. Resolution: take this branch's version of the four HDX-4405 files wholesale. packages/app/src/HDXMultiSeriesTableChart.tsx packages/app/src/__tests__/HDXMultiSeriesTableChart.test.tsx packages/app/tests/e2e/features/dashboard-table-linking.spec.ts packages/app/tests/e2e/page-objects/DashboardPage.ts The auto-merge would otherwise have silently re-introduced the pre-pivot state code (kept by 3-way merge because HEAD's deletion predated the branch base) and a unit test that walked inline styles on a no-longer-present Tooltip.Floating. All other files merge cleanly from main.
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…-import.spec.ts The merge resolution took HEAD's DashboardPage.ts wholesale to drop the obsolete Tooltip.Floating hint helper. That also dropped the exportDashboard / addTileWithSource / exportDashboardMenuItem additions from main #2410 (dashboard-export filters), which the materialized-views and dashboard-template-import e2e specs depend on. Re-add the three members verbatim from main, leaving the HDX-4405 pivot helpers (getRowActionHint, hoverFirstTableRowAndGetTooltip arrow-icon flow) untouched.
Greptile SummaryThis PR replaces the
Confidence Score: 5/5Safe to merge. The stranded-tooltip bug is eliminated by construction and no regressions are introduced in the row-action render path. All changed logic is additive or a clean replacement with no shared mutable state. The WeakMap memoization in No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Row as TR.tableRow
participant CSS as CSS (:hover rule)
participant Icon as Link.rowActionHint
participant Tooltip as Mantine Tooltip (portal)
User->>Row: mouseenter
Row->>CSS: :hover activates
CSS->>Icon: opacity 0 to 1, pointer-events none to auto
User->>Icon: mouseenter (openDelay 300ms)
Icon->>Tooltip: open
Tooltip-->>User: label rendered in portal
User->>User: mouse leaves row/icon
Icon->>Tooltip: mouseleave, closeDelay 100ms
Tooltip-->>User: label hidden
CSS->>Icon: opacity 1 to 0, pointer-events auto to none
Reviews (2): Last reviewed commit: "fix(test): align row-click filter assert..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const row = rows[virtualRow.index] as TableRow<any>; | ||
| // Compute the action once per row so the trailing-icon hint | ||
| // shares the memoized result from useOnClickLinkBuilder with | ||
| // the per-cell renders. | ||
| const rowAction = getRowAction ? getRowAction(row.original) : null; | ||
| const isActionable = Boolean(rowAction && rowAction.url); |
There was a problem hiding this comment.
getRowAction invocation per row added in render loop
The column-def cell function (line 184) already calls getRowAction(row.original) once per cell, so for a row with N visible columns the hook is called N times during flexRender. This new outer call makes it N+1 per row. The inline comment asserts the hook "memoizes per-row results internally" — if that memoization is keyed by row.original reference and the reference is stable across the same render, the overhead is negligible. If data refreshes produce new row-object references each cycle the cache misses, and every row now incurs one extra uncached computation. Worth confirming that useOnClickLinkBuilder uses a reference-stable cache (e.g. WeakMap or useMemo with a stable dep).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ress Greptile feedback
Three pre-existing e2e tests in dashboard-table-linking.spec.ts
expected the row-click filter template to render as a Lucene
condition (`ServiceName:"X"`), but main now emits a SQL IN clause
(`ServiceName IN ('X')`). The merge resolution took this branch's
version of the spec file wholesale to keep the HDX-4405 trailing
arrow test, which dropped main's matching test updates. Re-apply
them here verbatim.
Also addresses two Greptile P2 findings:
- HDXMultiSeriesTableChart.tsx: drop `rowAction.url as string` on
the trailing-icon Link. Narrow `rowAction?.url` to a non-null
string once per row so the guard and the Link sink stay
type-safe under future changes to `RowAction`.
- DashboardPage.ts + dashboard-table-linking.spec.ts: narrow
`page.getByRole('tooltip')` by name (matches /Search|Open/) so
the open/close assertions cannot collide with header-cell or
resize-handle tooltips that may be in the portal at the moment
of the check.
Local validation: app lint clean, 10/10 HDXMultiSeriesTableChart
unit tests pass.
github-actions
Bot
added
review/tier-3
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
LGTM!
Nit: All the row click actions (default, search and dashboard) should have the same hover effect.
For example, when I hover a row that goes to a search page I don't see any hover effect and hint. I only see when it goes to another dashboard.
kodiakhq
Bot
deleted the
alex/HDX-4405-fix-stranded-tooltip-in-virtual-table
branch
3 participants
Summary
Pivot of the original HDX-4405 fix per review feedback.
The first round of this PR fixed the stranded-tooltip bug by hoisting
Tooltip.Floatingto the<tbody>. That dismissed the bug but the resulting surface still felt off: the tooltip followed the cursor too closely. I've replaced it with a hover-revealed trailing chevron icon anchored to the row's last cell, wrapped in an anchored MantineTooltip. Mirrors the.rowButtonspattern from Search and Patterns.This eliminates the stranded-tooltip bug class by construction: the anchored Tooltip's open state is tied to the icon's lifecycle, so a virtual row unmounting mid-hover can no longer leave a popup behind.
What changed (vs the previous commit on this branch)
<tbody>-levelTooltip.Floating, thehoveredVirtualIndex/hoveredRowDescriptionstate, the<tbody>onMouseLeavesafety net, and per-rowonMouseEnter/onMouseLeavehandlers.IconChevronRightin the last<td>of clickable rows, wrapped in a Next.js<Link href={rowAction.url} prefetch={false} tabIndex={-1} aria-hidden="true">. The Link reuses the row's existing URL so cmd-click / middle-click / right-click "Open in New Tab" / status-bar URL preview all work on the icon.tabIndex={-1}andaria-hidden="true"keep sequential focus order and screen-reader traversal anchored to the per-cell Links so the icon doesn't double-count.HDXMultiSeriesTableChart.module.scssowns.tableRow,.lastCell,.rowActionHint. The icon isopacity: 0by default and fades in on.tableRow:hovervia a 0.15s transition..lastCelladdsposition: relativeto the last<td>only (the<tr>does not reliably form a positioning context across browsers).position="left",withArrow,openDelay={300},closeDelay={100},fz="xs"to match the establishedDBRowTableIconButtonconvention.rowAction.url === null) so the icon never promises a destination the click cannot deliver.Tests
HDXMultiSeriesTableChart.test.tsxrewritten: trailing-chevron rendering on success rows, dashboard-variant tooltip text, no chevron on failure rows, no chevron on the legacygetRowSearchLinkpath, no chevron when no action is configured. Dropped the inline-styleclosest('[style*="display"]')walk flagged in the previous deep review.Trailing chevron hint appears on row hover...now asserts opacity-gated visibility, tooltip text on row hover, and click-on-chevron navigation. Replaces the prior stranded-tooltip dismiss test.DashboardPage.tshint helper hovers the row to reveal the icon, then hovers the icon to open the anchored Tooltip. Docstring rewritten.How to test on Vercel preview
Preview route:
/dashboardsonClickaction (Search or Dashboard).Search HyperDX LogsorOpen dashboard "API Latency").onClickwhose template references an unknown column. Hover the resulting row. The chevron is not rendered.References
24d8552c) which fixed the stranding bug via tbody-Tooltip.Floating.