bito-code-review[bot] commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4163008240
##########
superset-frontend/src/explore/store.ts:
##########
@@ -89,6 +104,45 @@ export function handleDeprecatedControls(formData:
FormData): void {
formData.matrixify_mode_columns = 'disabled';
}
}
+
+ // #42910: migrate the legacy BigNumberPeriodOverPeriod
+ // `comparison_color_scheme` ('Green' | 'Red', where 'Red' reverses
+ // increase/decrease colors) into the `increase_color` / `decrease_color`
+ // ColorPickerControls that replaced it. `comparison_color_scheme` is no
+ // longer a registered control, so `getFormDataFromControls` drops it the
+ // next time the chart is saved -- without this migration, that silently
+ // discards a reversed-color choice the first time an old chart is resaved.
+ if (
+ formData.viz_type === VizType.BigNumberPeriodOverPeriod &&
+ formData.comparison_color_scheme &&
+ formData.increase_color === undefined &&
+ formData.decrease_color === undefined
+ ) {
+ const legacyReversed = formData.comparison_color_scheme === 'Red';
+ formData.increase_color = legacyReversed ? 'Red' : 'Green';
+ formData.decrease_color = legacyReversed ? 'Green' : 'Red';
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated legacy color scheme rule</b></div>
<div id="fix">
This block re-implements the legacy-reversal rule already encoded in
`resolveComparisonColorKeys` (BigNumberPeriodOverPeriod/utils.ts) using
hardcoded 'Red'/'Green' literals instead of the shared `ColorSchemeEnum` from
'@superset-ui/chart-controls'. If the legacy scheme semantics ever change, the
two copies can diverge and migrated charts would disagree with the render-time
fallback. Reusing `ColorSchemeEnum` keeps migration and render fallback in
lockstep.
</div>
</div>
<small><i>Code Review Run #ec4479</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -74,3 +76,124 @@ export const getHeaderFontSize = (proportionValue: number)
=>
export const getComparisonFontSize = (proportionValue: number) =>
comparisonFontSizesMapping[proportionValue] ??
sharedFontSizes[sharedFontSizes.length - 1];
+
+export interface ComparisonColorTokens {
+ /** Color for the arrow indicator and (when the symbol is index 0) text. */
+ text: string;
+ /** Background color for the increase/decrease pill. */
+ background: string;
+ /** Foreground color for the increase/decrease pill's text. */
+ strongText: string;
+}
+
+/**
+ * Resolves the increase/decrease colors to use for rendering, given the
+ * chart's current `increaseColor` / `decreaseColor` (from the
+ * `ColorPickerControl`s added after this became customizable) and the
+ * legacy `comparisonColorScheme` field.
+ *
+ * Charts saved before `increaseColor` / `decreaseColor` existed only have
+ * `comparisonColorScheme`, a 2-choice select ('Green' | 'Red') where 'Green'
+ * meant "green for increase, red for decrease" and 'Red' meant the reverse.
+ * Both legacy choices map onto the same 'Green' | 'Red' semantic token names
+ * used by the new controls' presets, so resolving through it here
+ * reproduces the exact old behavior (including the reversed case) without a
+ * data migration.
+ */
+export const resolveComparisonColorKeys = (
+ comparisonColorScheme: string | undefined,
+ increaseColor: string | undefined,
+ decreaseColor: string | undefined,
+): { increaseColor: string; decreaseColor: string } => {
+ const legacyReversed = comparisonColorScheme === ColorSchemeEnum.Red;
+ return {
+ increaseColor:
+ increaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Red : ColorSchemeEnum.Green),
+ decreaseColor:
+ decreaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Green : ColorSchemeEnum.Red),
+ };
+};
+
+/**
+ * Hex alpha suffix appended to a custom comparison color to build the pill
+ * background tint: 0x1A / 0xFF is roughly 10% opacity.
+ */
+export const COMPARISON_TINT_ALPHA_HEX = '1A';
+
+/**
+ * Resolves a single color value (semantic token name or literal hex from
+ * the color picker) to the (arrow/text, background, strong-text) triad used
+ * across the comparison pills. 'Green' / 'Red' keep using the paired
+ * success/error theme tokens exactly as before these colors were
+ * customizable; any other value is either a theme token name (e.g.
+ * 'colorPrimary', emitted by the picker's `resolveThemeTokens` option) or a
+ * literal hex -- 6-digit, or 8-digit when the alpha-enabled picker is used
+ * -- in which case the background is a light (~10% opacity) tint of that
+ * same color.
+ */
+export const getComparisonColorTokens = (
+ colorValue: string,
+ theme: SupersetTheme,
+): ComparisonColorTokens => {
+ if (colorValue === ColorSchemeEnum.Green) {
+ return {
+ text: theme.colorSuccess,
+ background: theme.colorSuccessBg,
+ strongText: theme.colorSuccessText,
+ };
+ }
+ if (colorValue === ColorSchemeEnum.Red) {
+ return {
+ text: theme.colorError,
+ background: theme.colorErrorBg,
+ strongText: theme.colorErrorText,
+ };
+ }
+ const themeColors = theme as unknown as Record<string, string>;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsound double type assertion</b></div>
<div id="fix">
The double assertion `theme as unknown as Record<string, string>` is
unsound: `SupersetTheme` also holds non-string fields (e.g. `sizeUnit`), so
`themeColors[colorValue]` can yield a non-string typed as `string`, discarding
type checks contrary to dev-standard.mdc's type-safety rule. Cast to
`Record<string, unknown>` and guard with `typeof === 'string'` so the
`colorValue` fallback stays honest.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/3b4bb77/.cursor/rules/dev-standard.mdc#L16";>dev-standard.mdc:16</a>
</li>
</ul>
</details>
<small><i>Code Review Run #ec4479</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t } from '@apache-superset/core/translation';
+import { Button } from '@superset-ui/core/components';
+import {
+ tokenizeToNumericArray,
+ isRangesInputComplete,
+} from '@superset-ui/plugin-chart-echarts';
+import ControlHeader from '../../ControlHeader';
+import ColorPickerControl from '../ColorPickerControl';
+import type { ColorPickerValue } from '../ColorPickerControl';
+import {
+ RangeRow,
+ RangeLabel,
+ replaceColorAtIndex,
+} from '../shared/RangeColorRow';
+import { BulletRangeColorsControlProps } from './types';
+
+/**
+ * Per-range color editor for the Bullet chart. Row *count* is driven by the
+ * sibling `ranges` control (one row per parsed threshold) so range values
+ * keep a single source of truth; this control only owns colors, stored as
+ * an array of hex strings positionally matched to those thresholds. An
+ * empty string entry (or a missing entry past the end of the array) means
+ * "no custom color" -- `Bullet/transformProps.ts` falls back to the default
+ * theme-token band ramp for that range, so existing saved Bullet charts
+ * (which have no `range_colors` at all) render identically to before this
+ * control existed.
+ */
+export default function BulletRangeColorsControl({
+ value,
+ onChange,
+ ranges,
+ ...headerProps
+}: BulletRangeColorsControlProps) {
+ const rangeValues = tokenizeToNumericArray(ranges) ?? [];
+ // While `ranges` is mid-edit (e.g. "20,,60"), tokenizeToNumericArray's
+ // leniency silently compacts it to a shorter list, which would otherwise
+ // shift colors to the wrong positions. Block commits until it's complete
+ // again so no edit can be positionally misaligned once it is.
+ const rangesComplete = isRangesInputComplete(ranges);
+
+ const colorAt = (index: number): string => value?.[index] || '';
+
+ const handleColorChange = (index: number) => (color: ColorPickerValue) => {
+ if (typeof color !== 'string' || !rangesComplete) return;
+ onChange?.(replaceColorAtIndex(rangeValues.length, index, color, colorAt));
+ };
+
+ const handleReset = (index: number) => {
+ if (!rangesComplete) return;
+ onChange?.(replaceColorAtIndex(rangeValues.length, index, '', colorAt));
+ };
+
+ return (
+ <div>
+ <ControlHeader {...headerProps} />
+ {rangeValues.length === 0 ? (
+ <RangeLabel>
+ {t('Add ranges above to configure their colors here.')}
+ </RangeLabel>
+ ) : (
+ rangeValues.map((range, index) => (
+ // eslint-disable-next-line react/no-array-index-key
+ <RangeRow key={index}>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fragmented i18n label</b></div>
<div id="fix">
Line 83 fragments the translatable string: `t('Up to')` is a standalone
message concatenated with `{range}` as a separate JSX node, so translators
cannot reorder the number inside the phrase. The `ariaLabel` on line 86 already
uses the interpolated form `t('Color for range up to %s', range)`; use `t('Up
to %s', range)` here too for consistent, locallyizable output.
</div>
</div>
<small><i>Code Review Run #ec4479</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx:
##########
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+ render,
+ screen,
+ userEvent,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import BulletRangeColorsControl from
'src/explore/components/controls/BulletRangeColorsControl';
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('renders a hint and no rows when no ranges are configured', () => {
+ render(<BulletRangeColorsControl onChange={jest.fn()} />);
+
+ expect(
+ screen.getByText('Add ranges above to configure their colors here.'),
+ ).toBeInTheDocument();
+ expect(document.querySelectorAll('.ant-color-picker-trigger')).toHaveLength(
+ 0,
+ );
+});
+
+test('renders one color picker row per parsed range, unset by default', () => {
+ render(
+ <BulletRangeColorsControl onChange={jest.fn()} ranges="100,200,300" />,
+ );
+
+ expect(screen.getByText(/Up to 100/)).toBeInTheDocument();
+ expect(screen.getByText(/Up to 200/)).toBeInTheDocument();
+ expect(screen.getByText(/Up to 300/)).toBeInTheDocument();
+ expect(document.querySelectorAll('.ant-color-picker-trigger')).toHaveLength(
+ 3,
+ );
+ // no row has a custom color yet, so no "Use default" reset link shows
+ expect(screen.queryByText('Use default')).not.toBeInTheDocument();
+});
+
+test('shows a "Use default" reset link only for rows with a custom color', ()
=> {
+ render(
+ <BulletRangeColorsControl
+ onChange={jest.fn()}
+ ranges="100,200"
+ value={['#ff0000', '']}
+ />,
+ );
+
+ expect(screen.getAllByText('Use default')).toHaveLength(1);
+});
+
+test('calls onChange with the full color array when a row color is picked',
async () => {
+ const onChange = jest.fn();
+ render(
+ <BulletRangeColorsControl
+ onChange={onChange}
+ ranges="100,200"
+ value={['', '']}
+ />,
+ );
+
+ const triggers = document.querySelectorAll('.ant-color-picker-trigger');
+ expect(triggers).toHaveLength(2);
+
+ await userEvent.click(triggers[0]);
+ await waitFor(() => {
+ expect(document.querySelector('.ant-color-picker')).toBeInTheDocument();
+ });
+
+ const hexInput = document.querySelector<HTMLInputElement>(
+ '.ant-color-picker-input input',
+ );
+ expect(hexInput).toBeInTheDocument();
+ await userEvent.clear(hexInput!);
+ await userEvent.type(hexInput!, '00ff00{enter}');
+
+ await waitFor(() => {
+ expect(onChange).toHaveBeenCalled();
+ });
+ const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1][0];
+ expect(lastCall[0].toLowerCase()).toBe('#00ff00');
+ expect(lastCall[1]).toBe('');
+});
+
+test('resets a single row back to "no custom color" without touching the
others', async () => {
+ const onChange = jest.fn();
+ render(
+ <BulletRangeColorsControl
+ onChange={onChange}
+ ranges="100,200"
+ value={['#ff0000', '#00ff00']}
+ />,
+ );
+
+ const resetButtons = screen.getAllByText('Use default');
+ expect(resetButtons).toHaveLength(2);
+ await userEvent.click(resetButtons[0]);
+
+ await waitFor(() => {
+ expect(onChange).toHaveBeenCalledWith(['', '#00ff00']);
+ });
+});
+
+test('disables editing and resets while ranges has a blank token between
numbers', () => {
+ const onChange = jest.fn();
+ render(
+ <BulletRangeColorsControl
+ onChange={onChange}
+ ranges="20,,60"
+ value={['#ff0000', '#00ff00', '#0000ff']}
+ />,
+ );
+
+ // The blank middle token means the list is still being edited; don't
+ // collapse row count or allow commits that would misalign colors once
+ // the blank is filled back in.
+ document.querySelectorAll('.ant-color-picker-trigger').forEach(trigger => {
+ expect(trigger).toHaveClass('ant-color-picker-trigger-disabled');
+ });
+ // AntD's Button always wraps its text child in an inner <span>, so
+ // querying by text here would match that span -- which `toBeDisabled`
+ // never recognizes (it only supports actual form-control elements).
+ // Query by role to get the real <button>, where `disabled` lives.
+ screen.getAllByRole('button', { name: 'Use default' }).forEach(button => {
+ expect(button).toBeDisabled();
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing negative onChange assertion</b></div>
<div id="fix">
This test pins the disabled UI state but never asserts `onChange` stays
silent, though it creates and passes the `onChange` mock (line 125).
`BulletRangeColorsControl`'s `handleColorChange`/`handleReset` guard on
`rangesComplete`, so `expect(onChange).not.toHaveBeenCalled()` would pass today
and would catch a regression that drops that guard. Per BITO.md rule 6262,
assert the behavior the test name claims.
</div>
</div>
<small><i>Code Review Run #ec4479</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
