bito-code-review[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3700596297
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -117,23 +114,50 @@ test('renders the correct input fields based on the
selected operator', async ()
});
test('renders None for operator when Green for increase is selected', async ()
=> {
- render(
+ const { container } = render(
<FormattingPopoverContent
onChange={mockOnChange}
columns={columns}
extraColorChoices={extraColorChoices}
/>,
);
- // Select the 'Green for increase' color scheme
- fireEvent.change(screen.getAllByLabelText(/color scheme/i)[0], {
- target: { value: ColorSchemeEnum.Green },
+ const colorPickerTrigger = container.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-items'),
+ ).toBeInTheDocument();
+ });
+
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = Array.from(presets).find(preset => {
+ const inner = preset.querySelector('.ant-color-picker-color-block-inner');
+ return inner && inner.getAttribute('style')?.includes('0, 150, 0');
});
- fireEvent.click(await screen.findByTitle(/green for increase/i));
+ expect(greenPreset).toBeDefined();
+ expect(greenPreset).toBeInTheDocument();
+ const safeGreenPreset = greenPreset as HTMLElement;
+
+ const innerColorBlock = safeGreenPreset.querySelector(
+ '.ant-color-picker-color-block-inner',
+ );
+ expect(innerColorBlock).toHaveStyle({ background: 'rgba(0, 150, 0, 0.2)' });
+
+ expect(safeGreenPreset).toBeInTheDocument();
+ await userEvent.click(safeGreenPreset);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test searches non-existent RGB string</b></div>
<div id="fix">
The `greenPreset.find()` search for `'0, 150, 0'` in style attributes will
always return undefined. The `colorScheme()` function (constants.ts:68+)
returns theme token names like `'colorSuccess'` that the ColorPickerControl
converts to CSS custom properties, not RGB values. The test should click the
first preset (which is the green theme color) and verify the operator becomes
None. Rule: 6262 (Assert Behavior Logic in Tests).
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const presets =
document.querySelectorAll('.ant-color-picker-presets-color');
````
</div>
</details>
</div>
<small><i>Code Review Run #22271a</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/ColorPickerControl.test.tsx:
##########
@@ -92,3 +97,146 @@ describe('ColorPickerControl', () => {
expect(colorPickerTrigger).toBeInTheDocument();
});
});
+
+test('calls onChange with string key "Green" when resolveThemeTokens is true',
async () => {
+ const onChange = jest.fn();
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens
+ presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
+
+ expect(greenPreset).toBeInTheDocument();
+ await userEvent.click(greenPreset);
+
+ expect(onChange).toHaveBeenCalledWith('Green');
+});
+
+test('calls onChange with RGB object when resolveThemeTokens is false', async
() => {
+ const onChange = jest.fn();
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens={false}
+ presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
+
+ expect(greenPreset).toBeInTheDocument();
+ await userEvent.click(greenPreset);
+
+ expect(onChange).toHaveBeenCalledWith({ r: 0, g: 150, b: 0, a: 0.2 });
+});
+
+test('resolves colorSuccess theme token correctly when matching color is
selected', async () => {
+ const onChange = jest.fn();
+
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ colors: {
+ colorSuccess: 'rgba(82, 196, 26, 1)',
+ },
+ });
+
+ render(
+ <ColorPickerControl
+ {...defaultProps}
+ onChange={onChange}
+ resolveThemeTokens
+ presets={[{ label: 'Theme Tokens', colors: ['colorSuccess'] }]}
+ />,
+ );
+
+ const colorPickerTrigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-items'),
+ ).toBeInTheDocument();
+ });
+
+ const successPreset = document.querySelector(
+ '.ant-color-picker-presets-color [style*="82, 196, 26"]',
+ ) as HTMLElement | null;
+
+ expect(successPreset).toBeInTheDocument();
+
+ await userEvent.click(successPreset!);
+
+ expect(onChange).toHaveBeenCalledWith('colorSuccess');
+});
+
+test('handles theme with nested colors object', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ colors: { primary: '#007bff' },
+ });
+
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ expect(
+ container.querySelector('.ant-color-picker-trigger'),
+ ).toBeInTheDocument();
+});
+
+test('handles theme without colors field', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue({
+ primary: '#007bff',
+ });
+
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ expect(
+ container.querySelector('.ant-color-picker-trigger'),
+ ).toBeInTheDocument();
+});
+
+test('handles undefined theme gracefully', () => {
+ jest
+ .spyOn(require('@apache-superset/core/theme'), 'useTheme')
+ .mockReturnValue(undefined);
+
+ expect(() => render(<ColorPickerControl {...defaultProps} />)).not.toThrow();
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Tests outside describe block</b></div>
<div id="fix">
Tests at lines 101-242 are placed outside the
`describe('ColorPickerControl')` block that closes at line 99. This breaks test
isolation since `beforeAll` (registry setup) and `beforeEach`
(jest.clearAllMocks) only apply to tests inside the block. Stale mocks from
previous runs may cause test failures in CI environments.
</div>
</div>
<small><i>Code Review Run #22271a</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-table/src/controlPanel.tsx:
##########
@@ -773,6 +769,7 @@ const config: ControlPanelConfig = {
(item: ConditionalFormattingConfig, index, array) => {
if (
item.colorScheme &&
+ typeof item.colorScheme === 'string' &&
!['Green', 'Red'].includes(item.colorScheme)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Trend color formatting flags dropped on save</b></div>
<div id="fix">
The filter `!['Green', 'Red'].includes(item.colorScheme)` skips the
migration for trend color configs where `colorScheme` is 'Green' or 'Red'. As a
result, when a user configures a trend color formatter (via the 'Trend colors'
`extraColorChoices` group), the `columnFormatting` and `objectFormatting`
derived from `toTextColor`/`toAllRow` are never written to `array[index]`, so
the flags are silently lost on save. The migration is safe for 'Green'/'Red'
values — it only touches `objectFormatting`/`columnFormatting`, never
`colorScheme` itself.
</div>
</div>
<small><i>Code Review Run #22271a</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]
