codeant-ai-for-open-source[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3678117319
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,151 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const getReverseThemeColorMap = (
+ themeColors: Record<string, any>,
+): Record<string, string> => {
+ const reverseMap: Record<string, string> = {};
+ if (!themeColors) return reverseMap;
+
+ Object.entries(themeColors).forEach(([name, value]) => {
+ if (typeof value === 'string') {
+ reverseMap[value.toLowerCase()] = name;
+ }
+ });
+
+ return reverseMap;
+};
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ themeColors: Record<string, string>,
+): string | undefined {
+ if (!value) return undefined;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ if (typeof value === 'string') {
+ if (value in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[value as SpecialColorKey]).toLowerCase();
+ }
+ if (themeColors && value in themeColors) {
+ return themeColors[value].toLowerCase();
+ }
+ return value.toLowerCase();
Review Comment:
**Suggestion:** `ColorPickerValue` allows arbitrary strings, but unknown
strings are passed directly to Ant Design as the controlled `value`. A saved
token that is no longer present in the active theme, or another legacy non-CSS
color name, is therefore supplied as an invalid picker value, so the picker
cannot reliably display the saved color and may reset or render an empty value.
Resolve only valid color strings or provide a valid fallback while preserving
the original value for submission. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Conditional-formatting editors lose unresolved saved colors.
- ⚠️ Ant Design picker display becomes unreliable.
- ⚠️ Theme changes can expose legacy invalid color tokens.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=89f8df69681b483e91ef4897ad546046&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=89f8df69681b483e91ef4897ad546046&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx
**Line:** 75:78
**Comment:**
*Possible Bug: `ColorPickerValue` allows arbitrary strings, but unknown
strings are passed directly to Ant Design as the controlled `value`. A saved
token that is no longer present in the active theme, or another legacy non-CSS
color name, is therefore supplied as an invalid picker value, so the picker
cannot reliably display the saved color and may reset or render an empty value.
Resolve only valid color strings or provide a valid fallback while preserving
the original value for submission.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=951344df516bd4bf97bc1ec40fe952c710d586628e6dfece936bb4b043f0cae2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=951344df516bd4bf97bc1ec40fe952c710d586628e6dfece936bb4b043f0cae2&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,151 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const getReverseThemeColorMap = (
+ themeColors: Record<string, any>,
+): Record<string, string> => {
+ const reverseMap: Record<string, string> = {};
+ if (!themeColors) return reverseMap;
+
+ Object.entries(themeColors).forEach(([name, value]) => {
+ if (typeof value === 'string') {
+ reverseMap[value.toLowerCase()] = name;
+ }
+ });
+
+ return reverseMap;
+};
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ themeColors: Record<string, string>,
+): string | undefined {
+ if (!value) return undefined;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ if (typeof value === 'string') {
+ if (value in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[value as SpecialColorKey]).toLowerCase();
+ }
+ if (themeColors && value in themeColors) {
+ return themeColors[value].toLowerCase();
+ }
+ return value.toLowerCase();
}
- return hexColor;
+ return rgbaToHex(value).toLowerCase();
}
export default function ColorPickerControl({
onChange,
value,
+ presets: customPresets,
+ ariaLabel,
...headerProps
}: ColorPickerControlProps) {
const categoricalScheme = getCategoricalSchemeRegistry().get();
- const presetColors = categoricalScheme?.colors.slice(0, 9) || [];
+ const defaultPresets = categoricalScheme?.colors.slice(0, 9) || [];
+ const theme = useTheme();
+
+ const themeColors = useMemo<Record<string, string>>(
+ () => (theme as any)?.colors || theme || {},
+ [theme],
+ );
+
+ const reverseMap = useMemo(
+ () => getReverseThemeColorMap(themeColors),
+ [themeColors],
+ );
+
+ const presets = useMemo(() => {
+ if (customPresets) {
+ return customPresets.map(item => ({
+ label: item.label,
+ colors: item.colors.map(color => {
+ if (color in SPECIAL_COLORS) {
+ return rgbaToHex(
+ SPECIAL_COLORS[color as SpecialColorKey],
+ ).toLowerCase();
+ }
+ if (themeColors && color in themeColors) {
+ return themeColors[color].toLowerCase();
+ }
+ return String(color).toLowerCase();
+ }),
+ }));
+ }
+
+ return [
+ {
+ label: 'Theme colors',
+ colors: defaultPresets.map(c => String(c).toLowerCase()),
+ },
+ ];
+ }, [customPresets, themeColors, defaultPresets]);
const handleChange = (color: ColorValue) => {
- if (onChange) {
- const rgb = color.toRgb();
- onChange({
- r: rgb.r,
- g: rgb.g,
- b: rgb.b,
- a: rgb.a,
- });
+ if (!onChange) return;
+
+ const rgb = color.toRgb();
+ const hex = rgbaToHex(rgb).toLowerCase();
+
+ const specialEntry = Object.entries(SPECIAL_COLORS).find(
+ ([, rgba]) => rgbaToHex(rgba).toLowerCase() === hex,
+ );
+
+ if (specialEntry) {
+ onChange(specialEntry[0] as SpecialColorKey);
+ return;
}
+
+ if (reverseMap[hex]) {
+ onChange(reverseMap[hex]);
+ return;
+ }
+
+ onChange(rgb);
Review Comment:
**Suggestion:** The reverse lookup emits a theme-token string whenever the
selected literal color matches a theme value. Existing callers such as the
contour and color-breakpoint controls expect `onChange` to receive an RGB
object and store that value directly; receiving a token string makes their
color validation fail and can persist an invalid color. Restrict token-name
conversion to conditional-formatting usage or preserve the existing RGB
callback contract for these callers. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Color breakpoint selections can become invalid strings.
- ❌ Breakpoint validation prevents saving selected theme colors.
- ⚠️ Generic color controls receive inconsistent callback value types.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=102138408dc3485e904b04452b6a6140&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=102138408dc3485e904b04452b6a6140&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx
**Line:** 146:151
**Comment:**
*Api Mismatch: The reverse lookup emits a theme-token string whenever
the selected literal color matches a theme value. Existing callers such as the
contour and color-breakpoint controls expect `onChange` to receive an RGB
object and store that value directly; receiving a token string makes their
color validation fail and can persist an invalid color. Restrict token-name
conversion to conditional-formatting usage or preserve the existing RGB
callback contract for these callers.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=8d55ac9833701d078cf091d49c764a248b49c7c517e75b264469e7d59c5b91be&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=8d55ac9833701d078cf091d49c764a248b49c7c517e75b264469e7d59c5b91be&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -117,20 +114,31 @@ 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();
});
- fireEvent.click(await screen.findByTitle(/green for increase/i));
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
Review Comment:
**Suggestion:** The first preset is not the Green trend preset: the
component receives the default `colors` group before `extraColorChoices`, so
`presets[0]` selects the first theme color (`colorSuccess`) rather than
`ColorSchemeEnum.Green`. The test therefore does not verify the intended
behavior; locate the trend-color preset explicitly or select the corresponding
swatch after accounting for the default presets. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Conditional-formatting test selects the wrong preset.
- ⚠️ Green-specific operator behavior remains unverified.
- ⚠️ CI can fail or provide misleading regression coverage.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a73cbb7775394c9f9a68d7fb2a56219c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a73cbb7775394c9f9a68d7fb2a56219c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx
**Line:** 137:138
**Comment:**
*Incorrect Condition Logic: The first preset is not the Green trend
preset: the component receives the default `colors` group before
`extraColorChoices`, so `presets[0]` selects the first theme color
(`colorSuccess`) rather than `ColorSchemeEnum.Green`. The test therefore does
not verify the intended behavior; locate the trend-color preset explicitly or
select the corresponding swatch after accounting for the default presets.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=a73e5bb8c3ca937940a7107c48efafbd9bb4d08a3b1b80da52defd4e6673bff4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42053&comment_hash=a73e5bb8c3ca937940a7107c48efafbd9bb4d08a3b1b80da52defd4e6673bff4&reaction=dislike'>👎</a>
--
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]
