Skip to content

[HDX-1360] feat(app): number tile static color picker#2265

Open
alex-fedotyev wants to merge 6 commits into
mainfrom
alex/HDX-1360-color-swatch-input
Open

[HDX-1360] feat(app): number tile static color picker#2265
alex-fedotyev wants to merge 6 commits into
mainfrom
alex/HDX-1360-color-swatch-input

Conversation

@alex-fedotyev
Copy link
Copy Markdown
Contributor

@alex-fedotyev alex-fedotyev commented May 12, 2026

Summary

First PR in the issue #1360 series. Adds a "Color" section to the number tile's Display Settings drawer that stores a palette token (categorical chart-1 through chart-10, or semantic chart-success / chart-warning / chart-error). The renderer resolves the token at paint time, so the choice reflows correctly across light, dark, and IDE themes without losing contrast.

Re-scoped from the original component-only plan: the picker is now wired into a real product surface (number tile -> Display Settings drawer -> renderer), so reviewers can see it work end-to-end on the Vercel preview instead of through Storybook mocks.

What ships in this PR:

  • Palette tokens (CHART_PALETTE_TOKENS, ChartPaletteToken, CATEGORICAL_PALETTE_TOKENS, SEMANTIC_PALETTE_TOKENS, isChartPaletteToken) move from packages/app/src/utils.ts to packages/common-utils/src/types.ts so the Zod schema and the app share one source of truth. The theme-aware CSS resolver getColorFromCSSToken stays in app (it depends on getComputedStyle(document.documentElement)). app/utils.ts re-exports the moved symbols so existing imports keep working.
  • New ChartPaletteTokenSchema = z.enum(CHART_PALETTE_TOKENS); SharedChartSettingsSchema gains color: ChartPaletteTokenSchema.optional(). Mirrors the existing numberFormat: NumberFormatSchema.optional() pattern at the same level (also a Number-only field stored at shared level and gated in the UI).
  • ColorSwatchInput (palette-only picker, 13 tokens, theme-aware, keyboard-accessible) is unchanged from the previous revision.
  • ChartDisplaySettingsDrawer gains a Color section, gated on displayType === DisplayType.Number, wired via <Controller> around <ColorSwatchInput>.
  • EditTimeChartForm round-trips the field through displaySettings and handleUpdateDisplaySettings.
  • DBNumberChart applies config.color via Mantine <Text c={getColorFromCSSToken(token)}>. Unknown tokens (legacy values, schema drift) fall back to the default text color via the isChartPaletteToken guard.
  • Storybook stories trimmed: kept the 6 isolated stories (Default, Selected, Disabled, WithCustomLabel, AllTokensSelected, KeyboardNav); dropped the 5 in-context mocks since the picker now has a real product surface.

Follow-ups

Filed before merging so the parity work isn't lost:

Per-series color on line / bar / pie, threshold-driven color on number, reference lines, table per-column color, and the background-chart sparkline all ship in their own follow-up PRs.

Test plan

  • yarn workspace @hyperdx/app jest clean (1647 / 1656 pass, 9 pre-existing skips; 3 new tests on DBNumberChart cover positive, unset, and unknown-token paths).
  • yarn workspace @hyperdx/common-utils jest clean (957 / 957).
  • yarn workspace @hyperdx/app lint clean.
  • yarn workspace @hyperdx/app tsc --noEmit clean.
  • yarn workspace @hyperdx/common-utils lint clean.
  • yarn workspace @hyperdx/app knip: no new findings; pre-existing flags unchanged.
  • Prose-lint clean.
  • Tier predictor: Tier 3 (819 prod lines, 4 prod files; Tier 2 max is 250).
  • Vercel preview: drive the number-tile drawer, pick a categorical and a semantic token, save, reload, verify persistence and theme reflow (light + dark).

Resolves part of #1360 (AC3, partial AC6).

PR-1 of the issue #1360 series. The existing ColorSwatchInput
(orphan in OSS since #440) used a free-form ColorInput plus a
hand-picked 9-value hex array. Refactor it to a palette-only
picker driven by the unified 13-token palette landed in #1627:
ten categorical tokens (chart-1..chart-10) and three semantic
tokens (chart-success, chart-warning, chart-error).

Storing tokens (not hex) lets user color choices reflow across
themes and color modes without losing contrast or breaking the
WCAG ratios baked into the palette.

The component remains internal in this PR. PRs 2b through 6b
in the issue #1360 series wire it into ChartSeriesEditor,
ChartDisplaySettingsDrawer, and the number-tile thresholds /
reference-line editors. PR 7 mirrors the schema fields on the
external dashboards API. PR 8 documents the feature.

Highlights:
- New ChartPaletteToken type + CHART_PALETTE_TOKENS,
  CATEGORICAL_PALETTE_TOKENS, SEMANTIC_PALETTE_TOKENS arrays,
  isChartPaletteToken type guard in utils.ts.
- New getColorFromCSSToken(token) reads the matching
  --color-chart-* CSS variable, with SSR-safe fallbacks.
- New resolveChartColor(token, fallbackIndex, level) for PR 2b
  to plug into setLineColors / formatResponseForPieChart.
- ColorSwatchInput popover split into categorical / semantic
  sections; trigger uses --color-outline-focus for focus-visible;
  swatch buttons use aria-pressed; legacy non-token values are
  guarded to treat as unset.
- Storybook stories matrix: six isolated stories (picker
  mechanics) and five in-context mocks (InSeriesRow,
  InLineChart, InStackedBar, InNumberTile,
  InReferenceLineEditor). The in-context stories are
  storybook-only mocks; they do not import the real editor
  components.
- 13 RTL tests cover the rejection rules and keyboard nav.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 12, 2026

🦋 Changeset detected

Latest commit: 1bf59d5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 28, 2026 3:57am

Request Review

resolveChartColor and ColorSwatchInputProps had no consumers on
this branch and tripped the knip CI check. Drop resolveChartColor
entirely (the next PR in the issue #1360 series adds it back
alongside its first consumer in ChartUtils.setLineColors), and
make ColorSwatchInputProps an internal type alias (consumers
infer the prop shape from typeof ColorSwatchInput).
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 12, 2026

E2E Test Results

All tests passed • 192 passed • 3 skipped • 1252s

Status Count
✅ Passed 192
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

Wire the palette picker into a real product surface. The
number tile's Display Settings drawer now exposes a "Color"
section that stores a palette token on the tile config; the
renderer resolves the token at paint time so the choice
reflows correctly across light, dark, and IDE themes.

Re-scope of PR #2265:

- Move the palette tokens (CHART_PALETTE_TOKENS,
  ChartPaletteToken, CATEGORICAL_PALETTE_TOKENS,
  SEMANTIC_PALETTE_TOKENS, isChartPaletteToken) from
  packages/app/src/utils.ts to
  packages/common-utils/src/types.ts so the Zod schema and
  the app share a single source of truth. The theme-aware
  CSS resolver (getColorFromCSSToken) stays in app because
  it depends on getComputedStyle(document.documentElement).
  app/utils.ts re-exports the moved symbols so existing
  imports keep working unchanged.
- Add ChartPaletteTokenSchema = z.enum(CHART_PALETTE_TOKENS)
  and color: ChartPaletteTokenSchema.optional() on
  SharedChartSettingsSchema. Mirrors the existing
  numberFormat: NumberFormatSchema.optional() pattern at the
  same level (also a number-only field stored at shared
  level and gated in the UI).
- Add the Color section to ChartDisplaySettingsDrawer, gated
  on displayType === DisplayType.Number, wired via
  <Controller> around <ColorSwatchInput>.
- Round-trip the color through EditTimeChartForm's
  displaySettings memo and handleUpdateDisplaySettings.
- Apply config.color in DBNumberChart via Mantine
  <Text c={getColorFromCSSToken(token)}>. Unknown tokens
  (legacy values, schema drift) fall back to the default
  text color via isChartPaletteToken guard.
- Trim the 5 in-context Storybook stories (InSeriesRow,
  InLineChart, InStackedBar, InNumberTile,
  InReferenceLineEditor, TokensReference). The picker now
  has a real product surface, so the mocks are redundant.
  The 6 isolated stories (Default, Selected, Disabled,
  WithCustomLabel, AllTokensSelected, KeyboardNav) stay
  because they exercise picker mechanics that the in-product
  surface only triggers one row at a time.

Adds 3 RTL tests on DBNumberChart: positive (color resolved
through getColorFromCSSToken), unset (no resolution), unknown
token (renderer falls back gracefully).

Schema parity for the external dashboards API
(packages/api/src/utils/zod.ts +
convertToExternal/InternalTileChartConfig + OpenAPI regen)
and customer docs on docs/use/dashboards.md ship in
follow-up PRs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@alex-fedotyev alex-fedotyev changed the title [HDX-1360] feat(app): refactor ColorSwatchInput to palette tokens [HDX-1360] feat(app): number tile static color picker May 28, 2026
…watch-input

# Conflicts:
#	packages/app/src/components/DBNumberChart.tsx
@alex-fedotyev alex-fedotyev marked this pull request as ready for review May 28, 2026 03:10
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label May 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 28, 2026

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 557 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 8
  • Production lines changed: 557 (+ 464 in test files, excluded from tier calculation)
  • Branch: alex/HDX-1360-color-swatch-input
  • Author: alex-fedotyev

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions
Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

✅ No critical issues found. The diff is tightly scoped, the palette-token store is additive-optional, the renderer is defensively guarded against legacy values, and ColorSwatchInput's only existing caller is the drawer that this PR also updates — so the breaking API change on the component carries no orphaned consumers.

🟡 P2 -- recommended

  • packages/app/src/utils.ts:499 -- getColorFromCSSToken and paletteTokenSSRFallback add four new branches (SSR fallback, getComputedStyle throw, empty CSS-var, populated CSS-var) with semantic-token cases plus a categorical Number(token.slice(...)) parse, and none are exercised by a direct unit test; the only coverage is via a jest.mock in DBNumberChart.test.tsx that replaces the function entirely.
    • Fix: Add unit tests in packages/app/src/__tests__/utils.test.ts covering each semantic token, a categorical token, the SSR branch, the getComputedStyle throw, and the empty-string fallback.
    • ce-testing-reviewer, ce-kieran-typescript-reviewer
  • packages/common-utils/src/types.ts:839 -- isChartPaletteToken is the runtime safety net for legacy values at two render-time call sites (ColorSwatchInput.safeValue, DBNumberChart.tileColor) but has no direct tests, so a regression that loosens the guard (e.g. dropping the typeof value === 'string' check) would not be caught.
    • Fix: Add tests in packages/common-utils/src/__tests__ covering valid tokens, hex strings, undefined/null/numbers, empty string, and case-sensitive non-matches.
    • ce-testing-reviewer
  • packages/app/src/components/ChartDisplaySettingsDrawer.tsx:124 -- The new showTileColor = displayType === DisplayType.Number gate and the Controller-wrapped ColorSwatchInput, plus the round-trip through EditTimeChartForm.tsx:226-543 (useWatchdisplaySettings memo → handleUpdateDisplaySettingssetValue('color', ...)), have no integration test; a refactor that drops 'color' from any of those three lists would silently break the picker.
    • Fix: Add an integration test that mounts the drawer with displayType=Number, picks a token, asserts onChange fires with {color: 'chart-…', …}, and a negative test that the picker is absent for displayType=Table.
    • ce-testing-reviewer, ce-kieran-typescript-reviewer
  • packages/app/src/components/__tests__/DBNumberChart.test.tsx:307 -- The three new color tests only assert that the mocked getColorFromCSSToken was (or was not) called and that '1234' renders; they never verify the resolved color reaches the <Text c={...}> DOM element, so a regression that drops the c prop or passes undefined would still pass.
    • Fix: Add an assertion against the rendered element's color (e.g. toHaveStyle({color: 'resolved(chart-success)'}) or the Mantine data-* attribute) to lock in the contract that the resolved token is applied.
    • ce-testing-reviewer
🔵 P3 nitpicks (4)
  • packages/common-utils/src/types.ts:828 -- CATEGORICAL_PALETTE_TOKENS = CHART_PALETTE_TOKENS.slice(0, 10) and SEMANTIC_PALETTE_TOKENS = CHART_PALETTE_TOKENS.slice(10) rely on magic indices that silently desync if CHART_PALETTE_TOKENS is reordered or extended; the cast also widens to a plain readonly ChartPaletteToken[] (losing tuple-literal narrowing).
    • Fix: Define the two subset tuples literally and compose CHART_PALETTE_TOKENS = [...CATEGORICAL_PALETTE_TOKENS, ...SEMANTIC_PALETTE_TOKENS] as const so the split is self-describing.
    • ce-maintainability-reviewer, ce-kieran-typescript-reviewer
  • packages/app/src/components/ColorSwatchInput.test.tsx:184 -- The 'keyboard activates the trigger and selects a swatch with Enter' test fires fireEvent.keyDown(trigger, {key: 'Enter'}) and then fireEvent.click(trigger); the click does all the work, so the test passes even if Enter activation is removed entirely.
    • Fix: Drop fireEvent.click and use userEvent.keyboard('{Enter}') after focusing, or rename the test to reflect that it asserts click activation.
    • ce-testing-reviewer, ce-correctness-reviewer
  • packages/app/src/components/ColorSwatchInput.test.tsx:174 -- 'closes the popover after a selection' asserts only that the swatch test-id is absent, which would also be true if the dropdown re-mounted for another reason.
    • Fix: Also assert expect(trigger).toHaveAttribute('aria-expanded', 'false') to verify the popover-closed semantics directly.
  • packages/app/src/utils.ts:519 -- paletteTokenSSRFallback's default branch parses Number(token.slice('chart-'.length)); a future non-numeric categorical token (e.g. chart-neutral) would produce NaN, COLORS[NaN] is undefined, and the ?? COLORS[0] silently masks the gap.
    • Fix: Replace the loose default with an exhaustiveness check (e.g. default: const _: never = token; return COLORS[0];) so a future token forces a deliberate update.
    • ce-kieran-typescript-reviewer

Reviewers (5): ce-correctness-reviewer, ce-testing-reviewer, ce-maintainability-reviewer, ce-kieran-typescript-reviewer, ce-api-contract-reviewer.

Testing gaps:

  • paletteTokenSSRFallback semantic and categorical branches have no direct coverage.
  • The drawer color section and the form's color round-trip are untested end-to-end.
  • No regression test that an external-API payload carrying color is either accepted (post-parity follow-up [HDX-1360] External API parity for number-tile color #2359) or rejected explicitly — today it is silently stripped.

- Add getColorFromCSSToken unit tests covering the browser happy path,
  empty CSS var fallback, getComputedStyle throw fallback, and SSR
  fallback for all categorical indices (chart-1 through chart-10).
  Tests note that jsdom prevents simulating window===undefined so the
  throw branch covers the same paletteTokenSSRFallback code path.
- Add isChartPaletteToken tests for valid tokens, hex strings,
  undefined/null/numbers, empty string, case-sensitive non-matches,
  out-of-range indices, and lookalike non-tokens.
- Add ChartDisplaySettingsDrawer integration tests: assert the color
  picker is present for displayType=Number and absent for Table; assert
  onChange receives the selected token (categorical and semantic) when
  Apply is clicked.
- Strengthen DBNumberChart color test: update mock to return a valid
  hex (#00ff00) so Mantine applies it as an inline color style, then
  assert toHaveStyle({color: 'rgb(0, 255, 0)'}) to lock in that the
  resolved token reaches the <Text> DOM element.
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

P2 test coverage from deep review: added direct unit tests for getColorFromCSSToken (browser happy path, empty-CSS-var fallback, getComputedStyle-throw fallback, and all categorical indices), isChartPaletteToken guard tests (13 cases covering valid tokens, hex strings, null/undefined/numbers, case-sensitive mismatches, and out-of-range indices), a new ChartDisplaySettingsDrawer integration test verifying the picker appears for Number tiles and absent for Table tiles plus that onChange fires with the selected token, and a strengthened DBNumberChart color test that asserts the resolved color reaches the DOM element inline style. Commit: 1bf59d5.

@github-actions
Copy link
Copy Markdown
Contributor

Deep Review

🔴 P0/P1 -- must fix

  • packages/app/src/components/ChartEditor/utils.ts:101 -- the raw-SQL and PromQL save and preview paths build the saved/runtime chart config via pick(form, [...]) lists that omit color, while ChartDisplaySettingsDrawer.tsx:124 shows the new picker for every Number tile (showTileColor = displayType === DisplayType.Number), so a user on a raw-SQL or PromQL Number tile picks a swatch, sees no preview change, clicks Save, and the value is silently stripped before reaching setChartConfig.
    • Fix: add 'color' to the pick(...) arrays in convertFormStateToSavedChartConfig (sql branch ~:101, promql branch ~:79) and convertFormStateToChartConfig (sql branch :167, promql branch :146), then add a focused test for the sql/promql Number-tile round-trip.
    • correctness

🟡 P2 -- recommended

  • packages/common-utils/src/types.ts:872 -- color is added to the internal SharedChartSettingsSchema, but externalDashboardNumberChartConfigSchema and externalDashboardNumberRawSqlChartConfigSchema in packages/api/src/utils/zod.ts do not declare it, and the external dashboard tile config re-parses via .transform, so any color written through the UI is silently stripped on external read and any color posted via the external API is rejected on write -- the PR notes this as a follow-up, but a user who sets a color via the app and reads through the external API today sees no color.
    • Fix: track the follow-up so the external schemas land before the feature is broadly used, or document the temporary asymmetry in the external API release notes; an internal-only lock-in test that asserts the external schema currently strips color would prevent accidental partial fixes.
    • api-contract, correctness
🔵 P3 nitpicks (6)
  • packages/app/src/components/ChartDisplaySettingsDrawer.tsx:92 -- the useEffect(() => reset(appliedDefaults), [appliedDefaults, reset]) re-runs whenever the parent's settings reference churns, so an in-flight color (or numberFormat) edit inside an already-open drawer can be clobbered if EditTimeChartForm's memoized displaySettings rebuilds before Apply; pre-existing pattern but now also affects the new color field.
    • Fix: gate the reset on !opened (or on formState.isDirty === false), or shallow-compare appliedDefaults before resetting; add a regression test that picks a color, rerenders the parent with a referentially-new settings whose values match, and asserts the picker still reflects the user choice.
    • adversarial
  • packages/common-utils/src/types.ts:828 -- CATEGORICAL_PALETTE_TOKENS = CHART_PALETTE_TOKENS.slice(0, 10) and SEMANTIC_PALETTE_TOKENS = CHART_PALETTE_TOKENS.slice(10) enforce the categorical/semantic split with a magic numeric index, so inserting a future categorical token at the wrong position would silently route it into the semantic group with no compile-time signal.
    • Fix: define CATEGORICAL_PALETTE_TOKENS and SEMANTIC_PALETTE_TOKENS as the as const sources of truth and derive CHART_PALETTE_TOKENS = [...CATEGORICAL_PALETTE_TOKENS, ...SEMANTIC_PALETTE_TOKENS] as const, then add a unit test asserting the categorical entries all match /^chart-\d+$/.
    • kieran-typescript, adversarial
  • packages/app/src/utils.ts:519 -- paletteTokenSSRFallback's default branch parses the token suffix as a number, so a future semantic token (say chart-info) would silently compute Number('info') - 1 = NaN and fall back to COLORS[0] (brand green) on SSR with no compile-time signal.
    • Fix: narrow the default branch to a typed categorical-only union (or split on CATEGORICAL_PALETTE_TOKENS membership) and assert exhaustiveness with a never check so future palette additions force an explicit choice.
    • kieran-typescript
  • packages/app/src/components/ColorSwatchInput.tsx:77 -- when value is a non-palette string the trigger renders as empty (IconCircleOff) and the Clear button is gated on safeValue, so a user whose saved tile carries an unknown token from a future server-side migration sees an apparently-empty picker but has no UI affordance to clear the stale value other than picking a new one.
    • Fix: render the Clear button whenever the raw value is truthy, regardless of isChartPaletteToken, so unknown tokens can be cleared without first picking a known one.
    • adversarial
  • packages/app/src/components/ColorSwatchInput.test.tsx:171 -- "keyboard activates the trigger and selects a swatch with Enter" calls fireEvent.keyDown(trigger, {key: 'Enter'}) followed by fireEvent.click(trigger); jsdom does not synthesize a click from keyDown, and the component has no explicit onKeyDown handler, so the test passes on the click and gives false confidence about keyboard activation.
    • Fix: switch to userEvent.setup() plus user.keyboard('{Enter}') after focusing the trigger and each swatch, so the test actually exercises native-button activation.
    • testing
  • packages/app/src/components/ColorSwatchInput.tsx:65 -- the JSDoc in ColorSwatchInput.tsx and the comment in packages/common-utils/src/types.ts:805 both reference notes/repo-conventions/hyperdx/tile-styling.md, but that file does not exist in the repository.
    • Fix: either add the referenced doc or drop the references so future readers don't chase a missing file.
    • project-standards

Reviewers (7): correctness, testing, maintainability, project-standards, kieran-typescript, api-contract, adversarial.

Testing gaps:

  • No assertion that convertFormStateToSavedChartConfig/convertFormStateToChartConfig preserve color for {configType:'sql', displayType:Number} -- would catch the P1 directly.
  • No form-level test for EditTimeChartForm round-tripping color through useWatch -> displaySettings -> handleUpdateDisplaySettings.
  • No ChartDisplaySettingsDrawer "Reset to Defaults" test asserting color is unset and the subsequent Apply forwards color: undefined.
  • No ColorSwatchInput test for a non-palette saved value still surfacing a Clear affordance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant