[HDX-4406] Number tile conditional color rules (ordered thresholds)#2386
Merged
kodiakhq[bot] merged 12 commits intoJun 5, 2026
Merged
Conversation
alex-fedotyev
added
the
ai-generated
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
E2E Test Results✅ All tests passed • 197 passed • 3 skipped • 1353s
Tests ran across 4 shards in parallel. |
alex-fedotyev
changed the title
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).
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.
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.
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).
…yout Two fixes: 1. Color not applying: ClickHouse returns UInt64 counts as JSON strings when output_format_json_quote_64bit_integers=1. The rawValue passed to resolveConditionalColor was a string like "1251132", causing all numeric operators (gte, gt, lt, etc.) to short-circuit with a false result. Coerce parseable-as-number strings to numbers before evaluation. Test added for the string-coercion path. 2. Layout: rule rows now use fixed widths and center alignment instead of stretching full-width with flex-start offsets. Operator w=80, value w=120 (single) / w=72 each (between), Add rule button left-aligned.
Replace references to a competitor's threshold model with neutral terminology: 'last-match-wins' and 'higher-priority rules go last'.
#2362 renamed the chart palette tokens from numeric (chart-1..chart-10) to hue names (chart-blue..chart-gray) and made ChartPaletteTokenSchema a strict z.enum that rejects the legacy values. This branch was authored against the old numeric tokens, so the new code's literal 'chart-1' references no longer typecheck or parse. Migrated 'chart-1' to 'chart-blue' across the new code added in this branch: - ColorRulesEditor: default new-rule color and clear-fallback. - ColorRulesEditor.test: makeRule default and the toMatchObject assertion. - utils.test (evaluateColorCondition / resolveConditionalColor describe blocks): the 12 rule literals. - common-utils/types.test (ColorConditionSchema describe block): all 30+ rule literals plus the "parses with all palette tokens" enumeration, which now lists the 10 hue names + the 3 semantic tokens. The 'chart-2' in the neq positive-case test became 'chart-orange' for variety. Legitimate pre-existing legacy-token references were left untouched: - ChartPaletteTokenSchema.parse('chart-1') / .parse('chart-10') in utils.test (still asserts the strict schema rejects legacy). - The render-time migration test in DBNumberChart.test ('chart-1' as any -> getColorFromCSSToken('chart-green')). - ColorSwatchInput.test legacy-coerce case. - dashboard.test / dashboard.remote.test legacy normalizer tests. - LEGACY_CHART_PALETTE_TOKEN_MAP and its guards-test coverage.
alex-fedotyev
force-pushed
the
alex/HDX-4406-number-tile-conditional-color-rules
branch
from
a5d93b8 to
19486c6
Compare
…tile-conditional-color-rules
Replace em-dashes with semicolons in two comment lines added by this branch so the prose-lint check passes for HDX-4406-owned files. No code behavior change. - packages/app/src/components/__tests__/DBNumberChart.test.tsx:445 - packages/app/src/utils.ts:621
alex-fedotyev
changed the base branch from
main
to
alex/HDX-4405-fix-stranded-tooltip-in-virtual-table
alex-fedotyev
changed the base branch from
alex/HDX-4405-fix-stranded-tooltip-in-virtual-table
to
main
Contributor
🟡 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
|
Greptile SummaryThis PR adds ordered conditional color rules to number tiles, letting users define threshold-based color overrides that evaluate last-match-wins against the tile's raw data value, with a fallback to the existing static color.
Confidence Score: 5/5Safe to merge — the feature is well-scoped, the resolver logic is correct, and tests cover all operator branches including the previously-flagged neq type-guard fix. The color resolution pipeline (schema → resolver → UI → chart) is implemented consistently end-to-end. The previously flagged neq cross-type bug and the rawValue duplication are both addressed in this HEAD. All operator branches have unit tests, the DrawerFormValues localId round-trip is clean, and the colorRules section is properly gated behind DisplayType.Number. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DBNumberChart renders] --> B[Compute rawValue from data]
B --> C{rawValue is NaN?}
C -- Yes --> D[rawValue = undefined]
C -- No --> E{rawValue is string?}
E -- Yes --> F{parseable as number?}
F -- Yes --> G[coerce to number]
F -- No --> H[keep as string]
E -- No --> I[keep as number]
D & G & H & I --> J[resolveConditionalColor]
J --> K{rules empty or value null?}
K -- Yes --> L[return fallback = resolveChartPaletteToken config.color]
K -- No --> M[iterate rules in order]
M --> N{rule matches?}
N -- Yes --> O[match = rule.color]
N -- No --> P[match unchanged]
O & P --> Q{more rules?}
Q -- Yes --> M
Q -- No --> R[return last match]
L & R --> S{token defined?}
S -- Yes --> T[getColorFromCSSToken → CSS hex]
S -- No --> U[undefined → default text color]
Reviews (3): Last reviewed commit: "Merge branch 'main' into alex/HDX-4406-n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two fixes from the automated review on #2386: - evaluateColorCondition (neq): guard on typeof so cross-type mismatches return false. Without it, a 'neq' rule with a string value (e.g. 'none') would match every numeric tile value because number !== 'someString' is always true. The contract was already documented in the eq/neq docstring but the implementation only honoured it for eq. Adds two unit tests (number vs string, string vs number) to lock the contract in. - DBNumberChart: reuse the already-computed value instead of re-walking data?.data?.[0] for rawValueRaw. The original duplication shipped because the rules path needs raw input (string/number) while formatNumber needs a numeric fallback; pulling them apart only requires turning Number.NaN into undefined so the coercion IIFE can treat 'no value' uniformly. Note: `value === Number.NaN` is a no-op in JS (NaN never equals itself), so the check uses Number.isNaN(value) instead.
teeohhem
approved these changes
Jun 5, 2026
kodiakhq
Bot
deleted the
alex/HDX-4406-number-tile-conditional-color-rules
branch
alex-fedotyev
added a commit
that referenced
this pull request
Jun 8, 2026
…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.
4 tasks
kodiakhq Bot
pushed a commit
that referenced
this pull request
Jun 10, 2026
…2428) Number tiles let you set a static color and ordered conditional color rules in the editor, but the v2 external dashboards API carried neither field, so dashboards authored through `POST /api/v2/dashboards` could not set them. A raw SQL number tile that set a color in the UI also lost it on a GET then PUT round-trip, because the external conversion never emitted `color`. This adds number-tile color to the external API so the authoring surfaces match. Closes #2359 (that issue predates the conditional rules and only mentioned the static color; this covers both). Builds on #2265 (static color) and #2386 (conditional rules). ## What - Builder number chart config gains `color` (a palette token) and `colorRules` (ordered conditional rules, last match wins, max 10). - Raw SQL number chart config gains `color`. `colorRules` is intentionally left off the raw SQL variant: the editor shows the rules section for raw SQL number tiles, but its save path drops them, so persisted raw SQL configs never carry rules and the API should not invent capability the UI lacks. - Both directions of the internal/external conversion round-trip the new fields. - The OpenAPI spec adds a `ChartPaletteToken` enum and a `ColorCondition` schema, referenced from the number config. `openapi.json` is regenerated. ## Why the palette-token handling looks the way it does Colors persist as palette tokens, not hex, so they reflow across light and dark themes. The canonical tokens were renamed to hue names (`chart-blue`, `chart-red`, and so on) recently, and the older numeric tokens (`chart-1` through `chart-10`) can still sit in stored dashboards. So: - On input, the API accepts hue-named tokens only (the same enum the schema validates). A legacy numeric token in a hand-written payload is rejected with a clear enum error. - On output, a stored legacy token is normalized to its hue name via the shared `resolveChartPaletteToken` helper, matching the app's fetch-time normalizer. Older dashboards keep returning a valid token and keep rendering. I reused the existing `ChartPaletteTokenSchema` and `ColorConditionSchema` from `common-utils` rather than re-declaring them in the external schema (the file already imports leaf enums and the OnClick schemas the same way). That keeps the external surface from drifting from what the editor persists. ## Test plan - [x] `yarn workspace @hyperdx/api tsc --noEmit` - [x] `yarn workspace @hyperdx/api lint` - [x] `yarn workspace @hyperdx/api lint:openapi` - [x] `make dev-int FILE=external-api/__tests__/dashboards.test.ts` (111 passed) - Round-trip: builder `color` plus `colorRules` covering every operator family, and raw SQL `color`. - Rejections: non-palette token, legacy numeric token on input, more than 10 rules, `between` without a two-number tuple, a numeric operator with a string value, an invalid regex, and an over-length label. - Backward compat: a tile with neither field; a stored legacy token normalized to its hue name on read, for both builder and raw SQL number tiles. - The two "round-trip all supported fields" tests (POST and PUT) now include `color` and `colorRules` on the number tile. ## What is not in this PR - MCP `hyperdx_save_dashboard` parity for the same fields. That is the next PR in this series and reuses this conversion. - Customer docs for the number-tile color options (tracked in `ClickHouse/clickhouse-docs#6298`).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
What changed
common-utils (schema)
ColorConditionSchema(discriminated union ofgt,gte,lt,lte,between,eq,neq,contains,startsWith,endsWith,regexoperators)colorRules(optional, max 10) toSharedChartSettingsSchemaalongside the existingcolorfieldapp (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 tofallback(the static color) when nothing matchesUInt64counts as JSON strings) are coerced to numbers before rule evaluationapp (UI)
ColorRulesEditorcomponent: sortable rule list via@dnd-kit/sortable, per-operator value inputs (single number, two-number range forbetween, text-or-number foreq/neq),ColorSwatchInputper rule, add/delete buttons; "Add rule" disables at 10ChartDisplaySettingsDrawer: added "Conditional colors" section gated onDisplayType.Number, placed below the existing static color pickerEditTimeChartForm:colorRuleswired throughuseWatch,displaySettingsmemo, andhandleUpdateDisplaySettingsDBNumberChart: resolves color viaresolveConditionalColor(rawValue, config.colorRules, config.color)at render timeTests
evaluateColorConditionper-operator,resolveConditionalColorincluding last-match-wins, string coercion, and the canonical success/warning/error scenarioColorRulesEditorDBNumberChartwithcolor: 'chart-success'+ two rules confirms value 50 -> success, 200 -> warning, 1000 -> error; also covers string UInt64 inputNo changes to
packages/apischemas or external API (separate follow-up ticket, mirrors HDX-4378).Screenshots or video
How to test on Vercel preview
Preview routes: /dashboards
Steps:
References