codeant-ai-for-open-source[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2959473508
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts:
##########
@@ -121,15 +212,619 @@ test('temporal columns use custom TextCellRenderer', ()
=> {
dataType: GenericDataType.Temporal,
});
- const { result } = renderHook(() =>
- useColDefs({
- ...defaultProps,
- columns: [temporalCol],
- data: [{ created_at: '2024-01-01' }],
- }),
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [temporalCol],
+ data: [{ created_at: '2024-01-01' }],
+ }),
+ { wrapper: defaultThemeWrapper },
);
const colDef = result.current[0];
expect(colDef.cellRenderer).toBeInstanceOf(Function);
expect(colDef.cellDataType).toBe('date');
});
+
+test('cellStyle derives readable text color from dark background formatting',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(255, 255, 255)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle keeps explicit text color over adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle treats legacy toTextColor formatters as text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle uses caller-provided surface color for adaptive contrast', ()
=> {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result: lightResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ }),
+ },
+ );
+
+ const { result: darkResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#000000',
+ }),
+ },
+ );
+
+ const lightCellStyle =
getCellStyleFunction(lightResult.current[0].cellStyle);
+ const darkCellStyle = getCellStyleFunction(darkResult.current[0].cellStyle);
+
+ expect(getCellStyleResult(lightCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(getCellStyleResult(darkCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+ expect(getCellStyleResult(lightCellStyle)).not.toEqual(
+ getCellStyleResult(darkCellStyle),
+ );
+});
+
+test('cellStyle uses striped odd-row surface for adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }, { count: 43 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ typeof value === 'number' ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillQuaternary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 0,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 1,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle exposes hover-specific adaptive contrast for formatted cells',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillSecondary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ '--ag-cell-value-hover-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle falls back to cellTextColor when no formatter matches', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: () => undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorPrimaryText: '#123456',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ const cellStyleResult = getCellStyleResult(cellStyle) as {
+ backgroundColor: string;
+ color?: string;
+ textAlign: string;
+ };
+ expect(cellStyleResult).toMatchObject({
+ backgroundColor: '',
+ color: '',
+ '--ag-cell-value-color': '',
+ '--ag-cell-value-hover-color': '',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle preserves invalid explicit text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? 'not-a-color' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'not-a-color',
+ '--ag-cell-value-hover-color': 'not-a-color',
+ });
+});
+
+test('cellStyle ignores cell-bar formatters for text and background
resolution', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#11111199' : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorPrimaryText: '#654321',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ const cellStyleResult = getCellStyleResult(cellStyle) as {
+ backgroundColor: string;
+ color?: string;
+ };
+ expect(cellStyleResult).toMatchObject({
+ backgroundColor: '',
+ color: '',
+ '--ag-cell-value-color': '',
+ '--ag-cell-value-hover-color': '',
Review Comment:
**Suggestion:** This assertion has the same contract mismatch: for
cell-bar-only formatters, no text/background formatter is resolved, so
`getCellStyle` does not include `color`/CSS variable keys. Expecting empty
strings for those keys makes the test assert behavior that the implementation
does not provide. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ False-negative unit test for CELL_BAR scenario.
- ❌ PR validation blocked by failing frontend tests.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute Jest test `cellStyle ignores cell-bar formatters for text and
background
resolution` in `.../useColDefs.test.ts:640-683`.
2. Test passes only `ObjectFormattingEnum.CELL_BAR` formatter (`lines
656-661`) and no
text/background formatter match.
3. In `getCellStyle` (`.../src/utils/getCellStyle.ts:68-76`), CELL_BAR is
explicitly
excluded from background assignment, and no text color is set.
4. Return object includes `backgroundColor: ''` and `textAlign`, while
text-color
variables are conditionally omitted (`lines 102-115`).
5. Assertion at `.../useColDefs.test.ts:677-682` requires `color` and both
CSS variables
as empty strings, causing contract-mismatch failure.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts
**Line:** 679:681
**Comment:**
*Logic Error: This assertion has the same contract mismatch: for
cell-bar-only formatters, no text/background formatter is resolved, so
`getCellStyle` does not include `color`/CSS variable keys. Expecting empty
strings for those keys makes the test assert behavior that the implementation
does not provide.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a08e2496aa3a0cc2be63454a47794d1aab3c373c07b560d36c573a1e322bafea&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a08e2496aa3a0cc2be63454a47794d1aab3c373c07b560d36c573a1e322bafea&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts:
##########
@@ -121,15 +212,619 @@ test('temporal columns use custom TextCellRenderer', ()
=> {
dataType: GenericDataType.Temporal,
});
- const { result } = renderHook(() =>
- useColDefs({
- ...defaultProps,
- columns: [temporalCol],
- data: [{ created_at: '2024-01-01' }],
- }),
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [temporalCol],
+ data: [{ created_at: '2024-01-01' }],
+ }),
+ { wrapper: defaultThemeWrapper },
);
const colDef = result.current[0];
expect(colDef.cellRenderer).toBeInstanceOf(Function);
expect(colDef.cellDataType).toBe('date');
});
+
+test('cellStyle derives readable text color from dark background formatting',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(255, 255, 255)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle keeps explicit text color over adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle treats legacy toTextColor formatters as text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle uses caller-provided surface color for adaptive contrast', ()
=> {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result: lightResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ }),
+ },
+ );
+
+ const { result: darkResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#000000',
+ }),
+ },
+ );
+
+ const lightCellStyle =
getCellStyleFunction(lightResult.current[0].cellStyle);
+ const darkCellStyle = getCellStyleFunction(darkResult.current[0].cellStyle);
+
+ expect(getCellStyleResult(lightCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(getCellStyleResult(darkCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+ expect(getCellStyleResult(lightCellStyle)).not.toEqual(
+ getCellStyleResult(darkCellStyle),
+ );
+});
+
+test('cellStyle uses striped odd-row surface for adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }, { count: 43 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ typeof value === 'number' ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillQuaternary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 0,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 1,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle exposes hover-specific adaptive contrast for formatted cells',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillSecondary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ '--ag-cell-value-hover-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle falls back to cellTextColor when no formatter matches', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: () => undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorPrimaryText: '#123456',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ const cellStyleResult = getCellStyleResult(cellStyle) as {
+ backgroundColor: string;
+ color?: string;
+ textAlign: string;
+ };
+ expect(cellStyleResult).toMatchObject({
+ backgroundColor: '',
+ color: '',
+ '--ag-cell-value-color': '',
+ '--ag-cell-value-hover-color': '',
Review Comment:
**Suggestion:** This expectation asserts that `color` and AG Grid text-color
CSS variables are set to empty strings when no formatter resolves, but
`getCellStyle` omits these keys entirely in that case. Keeping these assertions
will make the test fail even though runtime behavior is correct. Remove those
fields from this `toMatchObject` expectation. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Jest assertion fails against actual getCellStyle contract.
- ❌ Frontend CI can fail on ag-grid test suite.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run frontend Jest via `superset-frontend/package.json:85` (`test` script)
targeting
`plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts`.
2. In test `cellStyle falls back to cellTextColor when no formatter matches`
(`.../useColDefs.test.ts:557-602`), `columnColorFormatters` returns
`undefined` (`lines
571-578`), then `getCellStyleResult` executes `cellStyle`.
3. `useColDefs` wires `cellStyle` to `getCellStyle` in
`.../src/utils/useColDefs.ts:286-307`.
4. `getCellStyle` only spreads `color`/`--ag-cell-value-*` when
`resolvedTextColor`/`hoverResolvedTextColor` are truthy
(`.../src/utils/getCellStyle.ts:104-114`); with no resolved text color,
those keys are
omitted.
5. Assertion at `.../useColDefs.test.ts:595-601` expects empty-string keys
that are not
present, producing a false test failure.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts
**Line:** 597:599
**Comment:**
*Logic Error: This expectation asserts that `color` and AG Grid
text-color CSS variables are set to empty strings when no formatter resolves,
but `getCellStyle` omits these keys entirely in that case. Keeping these
assertions will make the test fail even though runtime behavior is correct.
Remove those fields from this `toMatchObject` expectation.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a5e76ea714618938fc5439aa59d5cc94a32601f58ad77464523358e628e61b88&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a5e76ea714618938fc5439aa59d5cc94a32601f58ad77464523358e628e61b88&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]
