Skip to content

feat(color-picker): consolidate remaining legacy color selectors onto ColorPickerControl - #42910

Open
rusackas wants to merge 54 commits into
masterfrom
feat/color-picker-consolidation
Open

rusackas wants to merge 54 commits into
masterfrom
feat/color-picker-consolidation

Conversation

@rusackas

@rusackas rusackas commented Aug 8, 2026

Copy link
Copy Markdown
Member

SUMMARY

Follow-up to #42053, which upgraded ColorPickerControl (custom presets, resolveThemeTokens, outputFormat) and used it to fix ConditionalFormattingControl. This PR finds and fixes the four remaining places in the app that still used a legacy/degraded color-selection UI instead of the shared picker, one commit per item.

1. Gauge chart interval colors

Gauge/controlPanel.tsx's "Interval colors" control asked users to type comma-separated 1-indexed positions into the chosen color scheme (e.g. 1,2,4), with zero visual feedback and silent discarding of malformed input.

Replaced with IntervalColorsControl: one ColorPickerControl per interval bound (parsed from the existing intervals control), storing real hex colors in a new interval_colors field, positionally matched to those bounds.

Design decision: bounds stay owned by the existing intervals text control rather than being folded into the new control's own row list (which the row-list add/remove pattern in the task brief technically implied). This keeps a single source of truth for bounds and avoids needing two-way sync between two independent controls — the new control's row count simply tracks whatever intervals currently contains.

Backward compatibility: charts saved before this control existed only have interval_color_indices (the old index strings). getIntervalBoundsAndColors in transformProps.ts still resolves those indices against the categorical scheme at render time whenever interval_colors is empty, so existing dashboards render identically with no migration. The control also resolves legacy indices to real colors for display the first time such a chart's panel is reopened (editor convenience only, not required for correct rendering).

Gauge: new Interval colors control + rendered chart

2. Bullet chart band colors

Bullet chart background bands were hardcoded to a 4-step theme-token ramp with no color control at all — a genuinely new feature, not a swap.

Added an optional range_colors control (BulletRangeColorsControl): one ColorPickerControl per threshold parsed from the existing ranges control, each starting unset ("use default") with a "Use default" link to clear a customization. transformProps.ts captures each range's chosen color by its original (pre-sort) position in ranges before the existing largest-first band sort reorders them for nested drawing, so colors stay pinned to the correct threshold regardless of draw order.

Backward compatible by construction: range_colors is optional and defaults to empty, so charts saved before this control existed have no such field and keep rendering with the exact default ramp.

Bullet: new Range colors control + rendered chart with custom band colors

3. Big Number Period-over-Period comparison colors

The comparison-color control was a 2-choice SelectControl ("Green for increase, red for decrease" / reverse) bound directly to theme.colorSuccess/theme.colorError.

Replaced with two ColorPickerControls, increase_color / decrease_color, using the exact resolveThemeTokens + outputFormat="hex" pattern #42053 introduced: picking the Green/Red preset swatch stores the token name (so it still reads the same as before for users who just want the classic behavior), while any other pick stores a literal hex color. Exported SPECIAL_COLORS/SpecialColorKey from ColorPickerControl.tsx so this call site doesn't redefine the Green/Red mapping.

The color→style resolution moved into two small, independently unit-tested pure functions in utils.ts (resolveComparisonColorKeys, getComparisonColorTokens) rather than living inline in PopKPI's render body — jsdom doesn't reliably expose emotion's injected styles to toHaveStyle for direct component assertions, so the logic needed to be testable on its own.

Backward compatibility: resolveComparisonColorKeys falls back to the legacy comparisonColorScheme field (kept, @deprecated in types.ts) whenever the new fields are absent — including correctly reversing increase/decrease for charts saved with the old "Red for increase, green for decrease" choice, the case a naive default-to-Green migration would have silently broken.

Big Number PoP: new Color for increase/decrease pickers + rendered comparison

4. Admin Theme editor curated colors

ThemeModal.tsx only exposed antd theming as a single JSON textarea, requiring admins to paste in a whole token object from an external tool to change even one color.

Added a "Colors" section (ThemeColorPickers) above the JSON textarea with one ColorPickerControl per curated antd token — the 5 SEED colors (colorPrimary, colorSuccess, colorWarning, colorError, colorInfo) plus 6 load-bearing map/alias tokens (colorLink, colorText, colorTextSecondary, colorBgBase, colorBgContainer, colorBorder). This intentionally does not attempt the full 100+ token surface — everything else stays fully editable via the JSON textarea, which remains the source of truth. Names are taken directly from antd's own SeedToken/MapToken types, not invented.

Sync is two-way, via two small pure functions (tryParseThemeJson, patchThemeJsonToken):

  • Picker → JSON: patches just that key into the JSON's token object and re-serializes with the modal's existing 2-space indent, preserving every other key (curated or not).
  • JSON → pickers: each render re-parses the textarea's current value and re-derives picker values from it.
  • Invalid/mid-edit JSON: tryParseThemeJson returns null instead of throwing (matching the file's existing isValidJson convention); the section shows a small notice and pickers stop persisting edits until the JSON is valid again.

The section is hidden for read-only system themes, matching the existing Format/Apply button visibility.

No backward-compat concern — additive UI over the same JSON, nothing about existing saved themes changes.

Theme editor: curated Colors section synced from JSON

TESTING INSTRUCTIONS

  • npm run test in superset-frontend/ — new/updated suites:
    • plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
    • src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx
    • plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
    • src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx
    • plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/{utils,PopKPI}.test.tsx
    • src/features/themes/{ThemeModal,ThemeColorPickers}.test.tsx
  • Manually: create/edit a Gauge chart, expand Customize → Intervals; create/edit a Bullet chart, expand Customize → Range colors; create/edit a Big Number w/ Time Comparison chart with "Add color for positive/negative change" enabled; open Settings → Themes → + Theme.
  • Backward compatibility: open an existing Gauge chart saved with interval_color_indices only, an existing Bullet chart with no range_colors, and a Big Number PoP chart saved with only comparison_color_scheme (including the Red value) — all three should render identically to before this PR.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov

codecov Bot commented Aug 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.71717% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.64%. Comparing base (1220ec0) to head (cac97c9).
⚠️ Report is 145 commits behind head on master.

Files with missing lines Patch % Lines
superset/mcp_service/chart/chart_utils.py 76.19% 2 Missing and 3 partials ⚠️
...s/plugin-chart-echarts/src/Bullet/controlPanel.tsx 0.00% 3 Missing ⚠️
...igNumber/BigNumberPeriodOverPeriod/controlPanel.ts 50.00% 1 Missing ⚠️
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx 75.00% 1 Missing ⚠️
...s/plugin-chart-echarts/src/Gauge/transformProps.ts 87.50% 1 Missing ⚠️
...explore/components/controls/ColorPickerControl.tsx 98.03% 1 Missing ⚠️
...omponents/controls/IntervalColorsControl/index.tsx 96.77% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #42910       +/-   ##
===========================================
+ Coverage   65.62%   84.64%   +19.02%     
===========================================
  Files        3059     3065        +6     
  Lines      202434   205256     +2822     
  Branches    47448    48125      +677     
===========================================
+ Hits       132852   173746    +40894     
+ Misses      66587    28207    -38380     
- Partials     2995     3303      +308     
Flag Coverage Δ
hive 34.48% <6.52%> (-0.29%) ⬇️
javascript 80.76% <97.71%> (+0.17%) ⬆️
mysql 52.58% <6.52%> (?)
postgres 52.60% <6.52%> (-0.21%) ⬇️
presto 36.21% <6.52%> (-0.29%) ⬇️
python 87.76% <89.13%> (+34.73%) ⬆️
sqlite 52.35% <6.52%> (-0.22%) ⬇️
unit 82.53% <89.13%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sha174n

sha174n commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Nice consolidation, and the one-commit-per-item structure keeps it bisectable with real backward-compat handling on each swap (legacy interval_color_indices / comparison_color_scheme fallbacks, additive optional fields). One process question: this bundles four unrelated areas (Gauge, Bullet, Big Number PoP, Theme editor), each with its own new control dir and backward-compat surface — might be worth confirming maintainers are fine reviewing it as a single PR vs splitting per area. Minor: ThemeColorPickers re-parses the JSON textarea on each render (the useMemo keyed on jsonData mitigates it).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Consolidates legacy chart and theme color selection onto the shared ColorPickerControl.

Changes:

  • Adds per-interval Gauge and per-range Bullet color controls.
  • Adds independent Big Number comparison colors with legacy fallback logic.
  • Adds curated theme-token pickers and related tests.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/features/themes/ThemeModal.tsx Integrates curated theme color pickers.
src/features/themes/ThemeModal.test.tsx Tests modal/picker synchronization.
src/features/themes/ThemeColorPickers.tsx Implements theme token color editing.
src/features/themes/ThemeColorPickers.test.tsx Tests parsing, patching, and interactions.
src/explore/components/controls/IntervalColorsControl/types.ts Defines Gauge color-control props.
src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx Tests Gauge interval color UI.
src/explore/components/controls/IntervalColorsControl/index.tsx Implements Gauge interval color UI.
src/explore/components/controls/index.ts Registers the new controls.
src/explore/components/controls/ColorPickerControl.tsx Exports semantic color definitions.
src/explore/components/controls/BulletRangeColorsControl/types.ts Defines Bullet color-control props.
src/explore/components/controls/BulletRangeColorsControl/index.tsx Implements Bullet range color UI.
src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx Tests Bullet range color UI.
plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts Tests Gauge color rendering and fallback.
plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts Tests Bullet custom band colors.
plugins/plugin-chart-echarts/src/Gauge/types.ts Adds Gauge interval color form data.
plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Applies Gauge colors with legacy fallback.
plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Replaces the legacy Gauge color field.
plugins/plugin-chart-echarts/src/Bullet/types.ts Adds Bullet range color form data.
plugins/plugin-chart-echarts/src/Bullet/transformProps.ts Applies colors to sorted Bullet bands.
plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx Adds the Bullet range color control.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts Adds comparison color resolution helpers.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.test.ts Tests comparison color resolution.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts Adds new comparison color properties.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts Passes comparison colors to the component.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx Applies customizable comparison colors.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx Tests comparison rendering variants.
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts Adds increase/decrease color pickers.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@netlify

netlify Bot commented Aug 29, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 3af5bf5
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6adfeeb256b6618a76e6a67b
😎 Deploy Preview https://deploy-preview-42910--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@rusackas
rusackas force-pushed the feat/color-picker-consolidation branch from 96ba402 to 43e60b5 Compare August 30, 2026 00:46
@rusackas
rusackas marked this pull request as ready for review August 30, 2026 19:51
@dosubot dosubot Bot added change:frontend Requires changing the frontend explore:colors Related to the color scheme in Explore global:theming Related to theming Superset viz:charts:bignumber Related to BigNumber charts viz:charts:gauge Related to the Gauge chart labels Aug 30, 2026
@rusackas

Copy link
Copy Markdown
Member Author

Heya @sha174n, thanks for the read-through! Given each swap already lands as its own bisectable commit with its own backward-compat handling, I'm inclined to keep this bundled rather than split it four ways at this point. Happy to reconsider if another maintainer feels strongly about it.

And yeah, the useMemo already covers the re-parse cost, as you noted.

@bito-code-review

bito-code-review Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #c05116

Actionable Suggestions - 0
Additional Suggestions - 7
  • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx - 1
    • Duplicated color resolution · Line 188-192
      The `colorValue` selection (`percentDifferenceNumber > 0 ? resolvedIncreaseColor : resolvedDecreaseColor`) plus `getComparisonColorTokens` is duplicated in `getArrowIndicatorColor` and the `useMemo`. If the sign logic changes in one place, arrow and pill colors diverge. Consider extracting one shared helper.
  • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts - 1
    • Deprecated tag on required field · Line 53-59
      `intervalColorIndices` is still a required property and is still actively read as the legacy fallback in `getIntervalBoundsAndColors` and `controlPanel.tsx`. Marking a required, still-used field `@deprecated` is contradictory: IDEs will strikethrough and warn on every legitimate usage, and it implies the field can be removed when it cannot. Consider dropping the tag or making the field optional.
  • superset-frontend/src/features/themes/ThemeColorPickers.tsx - 1
    • Unused disabled prop · Line 125-125
      The `disabled` prop is declared, defaulted, and checked in `handleTokenChange`, but the only consumer (`ThemeModal` line 504) renders `` without it and hides the whole Colors section when read-only. This is dead API surface introduced by the diff. Wire it up or drop it.
  • superset-frontend/src/features/themes/ThemeColorPickers.test.tsx - 1
    • Brittle antd-internal test · Line 148-183
      This interaction test depends on antd ColorPicker internal DOM classes (`.ant-color-picker-trigger`, `.ant-color-picker-input input`) and internal event behavior (typing hex + Enter firing `onChangeComplete`). The sibling `ColorPickerControl.test.tsx` explicitly avoids this, noting it "would require more complex mocking as it uses complex internal components." These internals aren't part of antd's public API and can change between versions, making the test brittle/flaky. Consider mocking `ColorPickerControl` to invoke `onChange` directly.
  • superset-frontend/src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx - 1
    • Test coupled to global default · Line 116-142
      This test asserts the picked preset is `#1f77b4`, but `IntervalColorsControl` never forwards `colorScheme` to `ColorPickerControl`, which builds presets from `getCategoricalSchemeRegistry().get()` (no key → global default `SUPERSET_DEFAULT`). The assertion only passes because `testScheme`'s first color coincidentally equals the default's. Set `testScheme` as default in `beforeAll` (as `ColorPickerControl.test.tsx` does) to make the test self-contained.
  • superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx - 1
    • Type-only import · Line 25-25
      `IntervalColorsControlProps` is an interface used only as a type annotation (line 95), yet it is imported as a value while line 24 uses `import type` for the same kind of type-only import. Use `import type { IntervalColorsControlProps } from './types';` for consistency with the codebase's type-safety convention.
  • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts - 1
    • Object injection via bracket notation · Line 153-153
      The use of bracket notation `themeColors[colorValue]` is flagged as a potential object injection sink (CWE-1321). Although `hasOwnProperty.call` is used to validate the key, the linter still flags the access. Consider using a `Map` instead of a plain object to avoid prototype pollution risks. Note: A similar issue exists at line 77 (unchanged code) for `comparisonFontSizesMapping[proportionValue]`.
Review Details
  • Files reviewed - 28 · Commit Range: 8fa21e0..9b6b41e
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/utils.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/types.ts
    • superset-frontend/src/explore/components/controls/ColorPickerControl.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/types.ts
    • superset-frontend/src/explore/components/controls/index.ts
    • superset-frontend/src/features/themes/ThemeColorPickers.test.tsx
    • superset-frontend/src/features/themes/ThemeColorPickers.tsx
    • superset-frontend/src/features/themes/ThemeModal.test.tsx
    • superset-frontend/src/features/themes/ThemeModal.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Sep 16, 2026
@rusackas
rusackas force-pushed the feat/color-picker-consolidation branch from 9b6b41e to 5ec5ad2 Compare September 16, 2026 22:26
@rusackas rusackas removed the requires:rebase Requires rebasing on top of current master label Sep 16, 2026
@bito-code-review

bito-code-review Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #e29137

Actionable Suggestions - 0
Additional Suggestions - 7
  • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx - 1
    • Duplicated tokenize logic · Line 40-46
      `parseRanges` here is functionally identical to `tokenizeToNumericArray` in `plugin-chart-echarts/src/Bullet/utils.ts` (split → trim → filter empty → `Number` → `Number.isFinite`). The utils comment even says it must "match" this function. This PR's whole purpose is keeping them aligned, so divergence risk is real. Consider a single shared helper (or a shared test) to avoid silent color/band misalignment.
  • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx - 1
    • Duplicated color-selection logic · Line 188-192
      The ternary `percentDifferenceNumber > 0 ? resolvedIncreaseColor : resolvedDecreaseColor` is duplicated verbatim in `getArrowIndicatorColor` (188-191) and inside the `useMemo` (206-209). If one is edited without the other, the arrow color and the pill background/text will diverge for the same metric sign. Consider extracting a single `getColorValue()` helper used by both.
  • superset-frontend/src/explore/components/controls/index.ts - 1
    • Missing control type union · Line 61-62
      These two new control types are registered in the control map but not added to the `InternalControlType` union in `packages/superset-ui-chart-controls/src/types.ts`. Control panels reference them as string literals (`type: 'IntervalColorsControl'` in Gauge, `type: 'BulletRangeColorsControl'` in Bullet), so they fall outside the typed union and lose type safety. Add both names to the union.
  • superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx - 1
    • Legacy color logic diverges · Line 58-80
      `resolveLegacyColors` duplicates the fallback in `getIntervalBoundsAndColors` (transformProps.ts), which the comment on line 56 calls "equivalent" — but the two diverge. For legacy index 0, transformProps computes `(0-1) % len = -1` → `colors[-1]` (undefined) → falls back to `colorFn.colors[idx]`, while here `((0-1)%len+len)%len = len-1` yields the last scheme color. Empty-scheme handling also differs (`''` vs `undefined`). A chart with index 0 would render one color but show another in the panel. Consider sharing one helper.
  • superset-frontend/src/features/themes/ThemeColorPickers.tsx - 1
    • Unused disabled prop · Line 125-125
      `disabled` is never passed by the only consumer `ThemeModal` (line 639-642), and `ColorPickerControl` has no `disabled` prop, so the guard at line 139 is unreachable. Worse, a caller passing `disabled` would still see interactive pickers that silently drop `onChange`. Remove the prop or forward it to `ColorPickerControl`.
  • superset-frontend/src/explore/components/controls/ColorPickerControl.tsx - 1
    • Unused exported symbols · Line 30-30
      `SPECIAL_COLORS` and `SpecialColorKey` are exported but never imported anywhere in the repo (grep across src/plugins/packages finds only this file). The intended consumer `BigNumberPeriodOverPeriod` uses `ColorSchemeEnum` from `@superset-ui/chart-controls` instead. Consider reverting these to module-private to avoid dead public API surface.
  • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts - 1
    • Doc contradicts precedence logic · Line 61-66
      The JSDoc says `intervalColors` "Takes precedence over `intervalColorIndices` when present", but `getIntervalBoundsAndColors` (transformProps.ts:76) only prefers it when `Array.isArray(intervalColors) && intervalColors.length > 0`. Since `DEFAULT_FORM_DATA` sets `intervalColors: []`, an empty array is present yet falls back to legacy indices. Consider wording the doc to match the non-empty guard.
Review Details
  • Files reviewed - 28 · Commit Range: 558da7b..5ec5ad2
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Bullet/utils.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx
    • superset-frontend/src/explore/components/controls/BulletRangeColorsControl/types.ts
    • superset-frontend/src/explore/components/controls/ColorPickerControl.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx
    • superset-frontend/src/explore/components/controls/IntervalColorsControl/types.ts
    • superset-frontend/src/explore/components/controls/index.ts
    • superset-frontend/src/features/themes/ThemeColorPickers.test.tsx
    • superset-frontend/src/features/themes/ThemeColorPickers.tsx
    • superset-frontend/src/features/themes/ThemeModal.test.tsx
    • superset-frontend/src/features/themes/ThemeModal.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@rusackas
rusackas force-pushed the feat/color-picker-consolidation branch from 5ec5ad2 to 3d4241f Compare September 21, 2026 16:53
… are unresolved

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…colors and unresolvable legacy indices

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… value array and unresolved legacy indices

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three new issues with unset and legacy color states, inline.

// resolve the correct color from that legacy scheme at render time. A
// hardcoded default here would win over the legacy fallback via
// `applyDefaultFormData` and silently repaint old dashboards.
const comparisonColorControlConfig: CustomControlItem['config'] = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With no increase_color/decrease_color saved (any new chart), both pickers read 'Transparent' while PopKPI paints the theme success/error colors, so the editor never shows what the chart draws.

legacyIndexCount > 0 &&
legacyColors.length > 0 &&
legacyColors.every(color => color === '');
const editable = boundsComplete && !legacyUnresolved;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When an in-use legacy index is past the static palette, every row is disabled with no explanation and the legacy field is hidden, so these bands can't be recolored except by switching scheme, while the help text still says colors follow the scheme until one is edited.

…onsolidation

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ounds; pin non-positional legacy Gauge indices

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One formatting fix inline.

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment thread superset-frontend/src/explore/store.ts Resolved
rusackas and others added 2 commits October 28, 2026 07:42
… opt-in

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating saved Bullet thresholds still drops range_colors when Explore saved them in a different spelling.

rusackas and others added 2 commits October 29, 2026 11:02
…abel contrast, ranges comparison

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…er shorthand hex theme

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One regression in the latest shorthand-hex change; details inline.

…t over translucent bands

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both latest fixes behave as described, but neither has a test that fails when it is removed; suggested tests inline, plus one edge in the Bullet label-fill change.

Comment on lines +165 to +169
let outerCustomFill: string | undefined;
const bandLabelFills = sortedRanges.map(({ color }) => {
if (color && getColorLuminance(color) !== null) outerCustomFill = color;
return outerCustomFill;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: My earlier suggestion missed this: getColorLuminance is also null for opaque colors it can't parse, so an API-saved range_colors entry like yellow inside a black band now gets white text (~1.07:1). Defer outward only for fills known to be translucent.

Suggested change
let outerCustomFill: string | undefined;
const bandLabelFills = sortedRanges.map(({ color }) => {
if (color && getColorLuminance(color) !== null) outerCustomFill = color;
return outerCustomFill;
});
let outerCustomFill: string | undefined;
const bandLabelFills = sortedRanges.map(({ color }) => {
const translucent =
!!color &&
getColorLuminance(color) === null &&
/^(#[0-9a-f]{8}|rgba\()/i.test(color.trim());
if (color && !translucent) outerCustomFill = color;
return outerCustomFill;
});

'#ffffff',
'#ffffff',
]);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Reverting the label-fill change to color ?? outerCustomFill keeps every Bullet test green: the only translucent case has no opaque band outside it. This test pins a translucent band over an opaque one.

Suggested change
});
});
test('labels a translucent band from the opaque custom fill beneath it', () => {
const { echartOptions } = transformProps(
chartProps({ showLabels: true, rangeColors: ['#ffffff33', '#000000', ''] }),
);
const labelColors = (echartOptions as any).series[0].markArea.data.map(
(d: any) => d[0].label.color,
);
// drawn largest-first: unset, black, translucent white over black
expect(labelColors).toEqual([
supersetTheme.colorTextSecondary,
'#ffffff',
'#ffffff',
]);
});

// fallback.
expect(toDisplayHex('Green', {}, true)).toBe('#00960033');
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Removing either normalizeOpaqueAlpha call leaves this suite green, so the opaque four-digit colorError → Red fix is unpinned. This test fails with the shorthand-path call removed.

Suggested change
});
});
test('stores "Red" when the theme colorError is an opaque four-digit hex', async () => {
const onChange = jest.fn();
jest
.spyOn(require('@apache-superset/core/theme'), 'useTheme')
.mockReturnValue({ colors: { colorError: '#F00F' } });
render(
<ColorPickerControl
{...defaultProps}
onChange={onChange}
resolveThemeTokens
presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
/>,
);
await userEvent.click(
document.querySelector('.ant-color-picker-trigger') as HTMLElement,
);
await waitFor(() => {
expect(
document.querySelector('.ant-color-picker-presets-color'),
).toBeInTheDocument();
});
await userEvent.click(
document.querySelectorAll('.ant-color-picker-presets-color')[1],
);
expect(onChange).toHaveBeenCalledWith('Red');
});

…se theme tokens

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ready: in conditional formatting with time comparison, a plain Success/Error swatch pick now saves a Trend sentinel.

Comment on lines 266 to +274
const specialEntry = resolveThemeTokens
? Object.entries(SPECIAL_COLORS).find(
([, rgba]) => rgbaToHex(rgba).toLowerCase() === hex,
)
? Object.keys(SPECIAL_COLORS)
.filter(key => offeredSpecialKeys.has(key))
.map(key => [key, toDisplayHex(key, themeColors, true)] as const)
.find(
([, displayHex]) =>
displayHex !== undefined &&
normalizeColorToHex(displayHex) === hex,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With time comparison on, Table/AG Grid offer both the colorSuccess/colorError swatches and the Trend Green/Red chips; they now share one hex and this match runs first, so picking the plain Success swatch saves Green and hides the operator fields (base saves colorSuccess).

});

test('stores the theme token, not "Green", when Green is not among the presets', async () => {
const onChange = jest.fn();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test sets no theme, so in a full-file run it inherits the colorSuccess: '#0f0' spy from the test above (clearAllMocks keeps mockReturnValue); with shorthand no longer reverse-mapped it gets an RGB object instead of colorSuccess.

Suggested change
const onChange = jest.fn();
const onChange = jest.fn();
jest
.spyOn(require('@apache-superset/core/theme'), 'useTheme')
.mockReturnValue({ colors: { colorSuccess: '#00ff00' } });

Comment on lines +48 to +57
// Parse with the exact same semantics `getIntervalBoundsAndColors` in
// Gauge/transformProps.ts uses for this same legacy fallback (via
// `parseNumbersList`, not reimported here to avoid pulling the whole
// `@superset-ui/plugin-chart-echarts` package -- and every chart plugin it
// eagerly registers -- into `explore/store.ts`'s import graph): one
// unparseable (or blank) token discards every legacy index for the whole
// list, rather than resolving the other tokens independently. A more
// lenient parser here would let this one-time migration (and this
// display-only preview) disagree with how an untouched legacy chart has
// always rendered -- see #42910 review thread on this file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: explore/store.ts no longer imports this module or runs a migration, and its only importer already pulls in @superset-ui/plugin-chart-echarts, so this rationale no longer holds.

Suggested change
// Parse with the exact same semantics `getIntervalBoundsAndColors` in
// Gauge/transformProps.ts uses for this same legacy fallback (via
// `parseNumbersList`, not reimported here to avoid pulling the whole
// `@superset-ui/plugin-chart-echarts` package -- and every chart plugin it
// eagerly registers -- into `explore/store.ts`'s import graph): one
// unparseable (or blank) token discards every legacy index for the whole
// list, rather than resolving the other tokens independently. A more
// lenient parser here would let this one-time migration (and this
// display-only preview) disagree with how an untouched legacy chart has
// always rendered -- see #42910 review thread on this file.
// Parse with the exact same semantics `getIntervalBoundsAndColors` in
// Gauge/transformProps.ts uses for this same legacy fallback: one
// unparseable (or blank) token discards every legacy index for the whole
// list, rather than resolving the other tokens independently. A more
// lenient parser here would let this display-only preview disagree with
// how an untouched legacy chart has always rendered.

…ure black/white

Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit maps shorthand custom-theme tokens back to token names, which Table formatting and the PoP pill cannot render.

Comment on lines 116 to +119
const hex = normalizeColorToHex(value);
// Shorthand pure black/white (e.g. `colorTextBase: '#000'`) would make a
// plain black or white pick match a base token and save the token name
// instead of a usable color value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding shorthand again maps any custom-theme token like colorSuccess: '#0f0' back by name, so that swatch saves colorSuccess (base saves #00ff00); Table formatting then emits invalid #0f087 and the PoP pill text matches its background.

Comment on lines +120 to +125
if (
/^#[0-9a-f]{3,4}$/i.test(value) &&
(hex === '#000000' || hex === '#ffffff')
) {
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The '#000000'/'#ffffff' string literals trip the custom-rules literal-color check; a regex on value matches exactly the same shorthand inputs without them.

Suggested change
if (
/^#[0-9a-f]{3,4}$/i.test(value) &&
(hex === '#000000' || hex === '#ffffff')
) {
return;
}
if (/^#(?:000|fff)f?$/i.test(value)) {
return;
}

This branch has not been deployed

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

Labels

change:frontend Requires changing the frontend explore:colors Related to the color scheme in Explore global:theming Related to theming Superset i18n Namespace | Anything related to localization plugins preset-io risk:chart-metadata size/XXL viz:charts:bignumber Related to BigNumber charts viz:charts:gauge Related to the Gauge chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants