feat(dashboards): per-column color on table tiles#2517
Conversation
Bring the number-tile color features to builder table tiles: a per-column static palette color plus ordered conditional color rules, applied per cell at render via the shared resolver. - Add optional `color` and `colorRules` to `DerivedColumnSchema` (the per-column builder config), mirroring the existing per-column `numberFormat`. Move the chart palette tokens and `ColorConditionSchema` above `DerivedColumnSchema` so it can reference them; pure relocation, no logic change (the repo deliberately avoids `z.lazy` forward refs). The legacy-token migration helpers and number-tile schemas stay in place. - Build per-column `colorByColumn` / `rulesByColumn` lookups in DBTableChart alongside `formatByColumn`, and resolve + apply each cell's color in the table renderer, guarding unknown tokens so legacy configs never crash. - Add a per-column color drawer (SeriesColorDrawer) opened from the series row, gated to table tiles, reusing ColorSwatchInput and ColorRulesEditor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: bb0cafe The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
🟡 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 extends the existing number-tile color feature to dashboard table tiles, adding optional
Confidence Score: 4/5Safe to merge after fixing the missing color style on the external-link cell path. The external-link rendering branch in HDXMultiSeriesTableChart.tsx is the only cell variant that omits style={colorStyle}. Every other interactive path — internal Link, error button, and getRowSearchLink Link — correctly applies it. When a configured color rule is in effect and a row resolves to an external URL, the cell renders with the default text color while all surrounding cells in the same column show the chosen palette color. The rest of the implementation is clean: schema, render logic, drawer, and tests are all consistent. packages/app/src/HDXMultiSeriesTableChart.tsx — the external-link cell branch needs style={colorStyle} added to the tag. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DerivedColumnSchema\ncolor? + colorRules?] -->|persisted in select array| B[DBTableChart]
B -->|builds Maps per meta index| C[colorByColumn Map\nrulesByColumn Map]
C -->|passed as column props| D[HDXMultiSeriesTableChart Table]
D -->|per cell| E{coerce value\nto number if numeric string}
E --> F[resolveConditionalColor\nlast-match-wins rules → static color → undefined]
F --> G{isChartPaletteToken?}
G -->|yes| H[getColorFromCSSToken\nCSS var lookup]
G -->|no / unknown token| I[colorStyle = undefined\ndefault text color]
H --> J[colorStyle applied to cell element]
J --> K1[internal Link ✓]
J --> K2[error button ✓]
J --> K3[getRowSearchLink Link ✓]
J --> K4[plain div ✓]
J -.->|missing| K5[external a tag ✗]
Reviews (10): Last reviewed commit: "Merge branch 'main' into alex/table-cell..." | Re-trigger Greptile |
E2E Test Results✅ All tests passed • 242 passed • 1 skipped • 959s
Tests ran across 4 shards in parallel. |
Address review on #2517: move onClose() inside the handleSubmit success callback in SeriesColorDrawer so closing follows a valid submit instead of firing unconditionally. Harmless today (no validation rules) but avoids silently discarding an in-progress edit if a resolver or required field is added later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address testing feedback on #2517: ClickHouse serializes numeric aggregates (count is UInt64, sums) as strings, so the cell value arrives as "15770" not 15770. The numeric color operators (gt / lt / between) require a number, so a rule like "> 100 -> red" never matched and every cell fell back to the static color. Coerce a numeric-looking string to a number before resolving (genuine strings stay as-is for equality / string-match rules), mirroring DBNumberChart. Add a render test that drives string-serialized values so the regression is locked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deep Review✅ No critical issues found. No P0/P1 defects: the schema is backward-compatible and additive, tokens persist as palette tokens, the unknown-token render path is guarded against throwing, and the numeric-string coercion for ClickHouse-serialized counts is correct. The items below are recommended follow-ups. 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (2): correctness, testing. An adversarial reviewer was also dispatched; its scenarios (operator switching persistence, display-type-switch stranding color config, Clear-not-persisting-until-Apply, drag-reorder priority) were independently verified against the code and found to be non-issues (the editor coerces Testing gaps:
|
…tile (#2520) Builds on the number-tile background sparkline (#2489, #2501) and sets up the table-cell trend follow-up tracked in HDX-4604. ## Summary Extracts the chrome-less recharts core out of `NumberTileBackgroundChart` into a reusable `<Sparkline>` component (line / area / bar) and renders the number tile's background trend through it. This is a pure consolidation: the number tile issues the same query and draws the same faint line / area behind the value, so there is no user-visible change. The table-cell trend feature (HDX-4604) needs the identical render shell in every numeric cell. A shared primitive is the alternative to standing up a second chart component next to this one, which is also the reuse question Mike raised on #2489 ("any reason we couldn't reuse/extend the usual time chart instead?"). ## What - New `packages/app/src/components/Sparkline.tsx`: `<Sparkline points type={'line' | 'area' | 'bar'} color height />`. Chrome-less (no axes, grid, legend, or tooltip; dots and animation off), sized to fill its parent via `ResponsiveContainer` (`height` defaults to `100%`). Renders nothing for fewer than two points. The line and area branches are lifted from `NumberTileBackgroundChart` unchanged; the bar branch follows the existing `PatternTrendChart` in `DBRowTable` and is here for the table-cell consumer that lands next. - `NumberTileBackgroundChart.tsx` now renders `<Sparkline points={points} type={backgroundChart.type} color={color} />` inside its unchanged `aria-hidden` wrapper. Its data path (`buildSparklineTimeConfig`, `convertToTimeChartConfig`, `useQueriedChartConfig`, `formatResponseForTimeChart`) and the single-series `sparklinePointsFromGraphResults` helper are unchanged. ## Why the helper stays put `sparklinePointsFromGraphResults` is single-series and specific to the number tile's data path. The table render reconstructs per-group trends differently, so the helper would not be shared; it stays with the number tile. Only the genuinely shared pieces (the component and the `SparklinePoint` type) live in `Sparkline.tsx`. This keeps the existing `NumberTileBackgroundChart` tests in place and avoids a dead re-export. ## No behavior change The render is a verbatim lift, so the recharts output is identical. The diff on `NumberTileBackgroundChart.tsx` shows the same `<Area>` / `<Line>` props (stroke, `strokeOpacity` 0.5, `strokeWidth` 2, `fillOpacity` 0.15, `dataKey`, margin) moving into `<Sparkline>`, wrapped in the same `ResponsiveContainer` inside the same wrapper. The number tile passes the same resolved color and type it computed before, and keeps its own "fewer than two points" guard so the background layer is still absent when there is no trend. ## Test plan - [x] `nx run @hyperdx/app:ci:lint` (eslint + tsc) clean - [x] `nx run @hyperdx/app:ci:unit`: 2091 pass, 0 fail. Includes the existing number-tile render-wiring test and the `DBNumberChart` consumer test, both unchanged and green. - [x] New `Sparkline.test.tsx`: mounts the component and asserts the line / area / bar recharts layers render in the given color, and that it renders nothing below two points. - [x] Dev stack builds and serves the branch (app returns 200), confirming the refactor compiles and runs in the Next.js build, not only under jest. Visual note: this is a verbatim render extraction with no visual delta, so the proof of "renders identically" is the render-block diff plus the unchanged consumer test rather than a before/after screenshot. [ui-check: allow] ## What's not in this PR (follow-up, HDX-4604) - The table-tile "show trend sparklines" display toggle and its schema field. - The per-cell scoped query and grouped cell render in `DBTableChart`, which consumes `<Sparkline>` (lands after #2517 so the per-column color drives the sparkline color). - External API v2 / MCP parity and customer docs for the toggle. No changeset: internal refactor with no user-facing behavior change. [no-changeset: allow]
…tile (#2520) Builds on the number-tile background sparkline (#2489, #2501) and sets up the table-cell trend follow-up tracked in HDX-4604. ## Summary Extracts the chrome-less recharts core out of `NumberTileBackgroundChart` into a reusable `<Sparkline>` component (line / area / bar) and renders the number tile's background trend through it. This is a pure consolidation: the number tile issues the same query and draws the same faint line / area behind the value, so there is no user-visible change. The table-cell trend feature (HDX-4604) needs the identical render shell in every numeric cell. A shared primitive is the alternative to standing up a second chart component next to this one, which is also the reuse question Mike raised on #2489 ("any reason we couldn't reuse/extend the usual time chart instead?"). ## What - New `packages/app/src/components/Sparkline.tsx`: `<Sparkline points type={'line' | 'area' | 'bar'} color height />`. Chrome-less (no axes, grid, legend, or tooltip; dots and animation off), sized to fill its parent via `ResponsiveContainer` (`height` defaults to `100%`). Renders nothing for fewer than two points. The line and area branches are lifted from `NumberTileBackgroundChart` unchanged; the bar branch follows the existing `PatternTrendChart` in `DBRowTable` and is here for the table-cell consumer that lands next. - `NumberTileBackgroundChart.tsx` now renders `<Sparkline points={points} type={backgroundChart.type} color={color} />` inside its unchanged `aria-hidden` wrapper. Its data path (`buildSparklineTimeConfig`, `convertToTimeChartConfig`, `useQueriedChartConfig`, `formatResponseForTimeChart`) and the single-series `sparklinePointsFromGraphResults` helper are unchanged. ## Why the helper stays put `sparklinePointsFromGraphResults` is single-series and specific to the number tile's data path. The table render reconstructs per-group trends differently, so the helper would not be shared; it stays with the number tile. Only the genuinely shared pieces (the component and the `SparklinePoint` type) live in `Sparkline.tsx`. This keeps the existing `NumberTileBackgroundChart` tests in place and avoids a dead re-export. ## No behavior change The render is a verbatim lift, so the recharts output is identical. The diff on `NumberTileBackgroundChart.tsx` shows the same `<Area>` / `<Line>` props (stroke, `strokeOpacity` 0.5, `strokeWidth` 2, `fillOpacity` 0.15, `dataKey`, margin) moving into `<Sparkline>`, wrapped in the same `ResponsiveContainer` inside the same wrapper. The number tile passes the same resolved color and type it computed before, and keeps its own "fewer than two points" guard so the background layer is still absent when there is no trend. ## Test plan - [x] `nx run @hyperdx/app:ci:lint` (eslint + tsc) clean - [x] `nx run @hyperdx/app:ci:unit`: 2091 pass, 0 fail. Includes the existing number-tile render-wiring test and the `DBNumberChart` consumer test, both unchanged and green. - [x] New `Sparkline.test.tsx`: mounts the component and asserts the line / area / bar recharts layers render in the given color, and that it renders nothing below two points. - [x] Dev stack builds and serves the branch (app returns 200), confirming the refactor compiles and runs in the Next.js build, not only under jest. Visual note: this is a verbatim render extraction with no visual delta, so the proof of "renders identically" is the render-block diff plus the unchanged consumer test rather than a before/after screenshot. [ui-check: allow] ## What's not in this PR (follow-up, HDX-4604) - The table-tile "show trend sparklines" display toggle and its schema field. - The per-cell scoped query and grouped cell render in `DBTableChart`, which consumes `<Sparkline>` (lands after #2517 so the per-column color drives the sparkline color). - External API v2 / MCP parity and customer docs for the toggle. No changeset: internal refactor with no user-facing behavior change. [no-changeset: allow]
The per-package eslint warning ceilings added in #2654 make any net-new warning a lint failure. Merging main brought the ceilings onto this branch, and the table color tests added 12 warnings that pushed packages/app over its 740 cap. Clear them at the source without touching any ceiling: - replace empty-arrow mock callbacks with jest.fn() - back the jsdom crypto.randomUUID shim with Object.defineProperty instead of an `as any` cast - mock useSource via jest.mocked() to match the existing pattern in the same file - scope one eslint-disable for the deliberately invalid palette token in the unknown-token fallback test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
karl-power
left a comment
There was a problem hiding this comment.
LGTM but I think the SeriesColorDrawer and ColorRulesEditor could use a review from @elizabetdev to improve UX.
| // Resolve this cell's color from the column config: ordered | ||
| // rules first (last match wins), then the column's static | ||
| // color, else no override. ClickHouse serializes numeric | ||
| // aggregates (count is UInt64, sums, etc.) as strings, so coerce | ||
| // a numeric-looking value to a number first; otherwise the | ||
| // numeric operators (gt / lt / between) never match. Genuine | ||
| // strings (group-by labels, status values) stay as-is so the | ||
| // equality / string-match rules still work. Mirrors the value | ||
| // coercion in DBNumberChart. | ||
| // | ||
| // react-table types the getter as `number`, but the runtime | ||
| // value can be a string (see above), so read it through | ||
| // `unknown` to narrow honestly without an unsafe cast. |
There was a problem hiding this comment.
| // Resolve this cell's color from the column config: ordered | |
| // rules first (last match wins), then the column's static | |
| // color, else no override. ClickHouse serializes numeric | |
| // aggregates (count is UInt64, sums, etc.) as strings, so coerce | |
| // a numeric-looking value to a number first; otherwise the | |
| // numeric operators (gt / lt / between) never match. Genuine | |
| // strings (group-by labels, status values) stay as-is so the | |
| // equality / string-match rules still work. Mirrors the value | |
| // coercion in DBNumberChart. | |
| // | |
| // react-table types the getter as `number`, but the runtime | |
| // value can be a string (see above), so read it through | |
| // `unknown` to narrow honestly without an unsafe cast. | |
| // Resolve this cell's color from the column config: ordered rules first (last match wins). | |
| // react-table types the getter as `number`, but the runtime value can be a string |
Bring the number-tile color features to dashboard table tiles. On a builder table tile you can now set a static color on a column and layer ordered conditional rules (for example
> 500turns the cell red), the table-cell counterpart of the number-tile color picker. This extends the tile color work from #1360 to tables.Summary
colorandcolorRulesto the per-column builder config (DerivedColumnSchema), mirroring the existing per-columnnumberFormat.colorRulesreuses the existingColorConditionSchema(ordered, last matching rule wins, falling back to the static color).DBTableChartbuilds per-columncolorByColumn/rulesByColumnlookups alongsideformatByColumn, andHDXMultiSeriesTableChartresolves and applies each cell's color via the sharedresolveConditionalColorandgetColorFromCSSToken. Numeric-string values (ClickHouse serializes counts as strings) are coerced so numeric rules match, and unknown tokens are guarded so older configs render the default color instead of throwing.SeriesColorDrawer) opens from a palette icon in the series row, shown only on table tiles. It reuses the existingColorSwatchInputandColorRulesEditor.Screenshots
A builder table tile with a static
chart-successbase on the count column plus a> 50000rule mapped tochart-error, over the demo logs. Values above the threshold render red, the rest green; the group-by column stays the default color.Why the
types.tsmoveTo let
DerivedColumnSchemareference the palette and color-condition schemas (defined lower in the file), I relocated those primitive schemas above it. This is a pure move with no logic change. The legacy-token migration helpers and the number-tile schemas stay where they were, so the relocation is kept to the minimum the ordering requires.What's not in this PR (follow-ups)
Test plan
yarn lint:fixmake ci-lint(lint + tsc),yarn knipcolor+colorRulesper operator family, plus rejections for unknown tokens and more than 10 rules); table render (static color, conditional match / no-match, numeric-string coercion, unknown-token renders default without throwing); the color drawer (swatch sets the color, rules round-trip and strip client-side ids, clear); the series editor (color control shown on table tiles, hidden otherwise, opens the drawer).