bito-code-review[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2959901266
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +598,502 @@ test('values from the 3rd level of the hierarchy with a
subtotal at the to
},
});
});
+
+test('getCellColor derives readable text from the winning background', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ });
+});
+
+test('getCellColor keeps explicit text color over adaptive contrast', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#ace1c40d' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(172, 225, 196)',
+ });
+});
+
+test('getCellColor treats legacy toTextColor formatters as text color', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ {
+ column: 'revenue',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#ace1c40d' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(172, 225, 196)',
+ });
+});
+
+test('getCellColor ignores cell-bar rules when resolving text color', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#11111199' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: undefined,
+ color: undefined,
+ });
+});
+
+test('renderTableRow keeps subtotal background and readable text in sync', ()
=> {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderTableRow(['revenue'], 0, {
+ rowAttrs: ['metric'],
+ colAttrs: [],
+ rowAttrSpans: [[1]],
+ visibleColKeys: [[]],
+ pivotData: {
+ getAggregator: () => ({
+ value: () => 200,
+ format: (value: number) => String(value),
+ isSubtotal: true,
+ }),
+ } as any,
+ rowTotals: false,
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ cellCallbacks: {},
+ rowTotalCallbacks: {},
+ namesMapping: {},
+ allowRenderHtml: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const valueCell = cells.find(
+ child => React.isValidElement(child) && child.props.className === 'pvtVal',
+ ) as React.ReactElement;
+
+ expect(valueCell.props.style).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ fontWeight: 'bold',
+ });
+});
+
+test('renderColAttrsHeader applies readable text color to formatted headers',
() => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'region',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 'EMEA' ? '#111111' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) && child.props.className === 'pvtColLabel',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ });
+});
+
+test('renderColAttrsHeader uses active header surface for adaptive contrast',
() => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ highlightedHeaderCells: {
+ region: ['EMEA'],
+ },
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'region',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 'EMEA' ? 'rgba(0, 0, 0, 0.4)' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) &&
+ child.props.className === 'pvtColLabel active',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: 'rgba(0, 0, 0, 0.4)',
+ color: getTextColorForBackground(
+ { backgroundColor: 'rgba(0, 0, 0, 0.4)' },
+ supersetTheme.colorPrimaryBg,
+ ),
+ });
+});
+
+test('renderColHeaderRow preserves default header text color without
formatting', () => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: { metric: [] },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#ff00aa',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) && child.props.className === 'pvtColLabel',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: undefined,
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertion</b></div>
<div id="fix">
The test 'renderColHeaderRow preserves default header text color without
formatting' claims to verify preservation of default text color but only checks
backgroundColor. Since the code sets color to undefined in this case, the test
should also assert that color is undefined to fully validate the behavior.
</div>
</div>
<small><i>Code Review Run #24bba0</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-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +598,502 @@ test('values from the 3rd level of the hierarchy with a
subtotal at the to
},
});
});
+
+test('getCellColor derives readable text from the winning background', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ });
+});
+
+test('getCellColor keeps explicit text color over adaptive contrast', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#ace1c40d' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(172, 225, 196)',
+ });
+});
+
+test('getCellColor treats legacy toTextColor formatters as text color', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ {
+ column: 'revenue',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#ace1c40d' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(172, 225, 196)',
+ });
+});
+
+test('getCellColor ignores cell-bar rules when resolving text color', () => {
+ expect(
+ getCellColor(
+ ['revenue'],
+ 200,
+ {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#11111199' : undefined,
+ },
+ ],
+ },
+ '#ffffff',
+ ),
+ ).toEqual({
+ backgroundColor: undefined,
+ color: undefined,
+ });
+});
+
+test('renderTableRow keeps subtotal background and readable text in sync', ()
=> {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'revenue',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 200 ? '#111111' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderTableRow(['revenue'], 0, {
+ rowAttrs: ['metric'],
+ colAttrs: [],
+ rowAttrSpans: [[1]],
+ visibleColKeys: [[]],
+ pivotData: {
+ getAggregator: () => ({
+ value: () => 200,
+ format: (value: number) => String(value),
+ isSubtotal: true,
+ }),
+ } as any,
+ rowTotals: false,
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ cellCallbacks: {},
+ rowTotalCallbacks: {},
+ namesMapping: {},
+ allowRenderHtml: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const valueCell = cells.find(
+ child => React.isValidElement(child) && child.props.className === 'pvtVal',
+ ) as React.ReactElement;
+
+ expect(valueCell.props.style).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ fontWeight: 'bold',
+ });
+});
+
+test('renderColAttrsHeader applies readable text color to formatted headers',
() => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'region',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 'EMEA' ? '#111111' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) && child.props.className === 'pvtColLabel',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: '#111111',
+ color: 'rgb(255, 255, 255)',
+ });
+});
+
+test('renderColAttrsHeader uses active header surface for adaptive contrast',
() => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ highlightedHeaderCells: {
+ region: ['EMEA'],
+ },
+ cellColorFormatters: {
+ metric: [
+ {
+ column: 'region',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 'EMEA' ? 'rgba(0, 0, 0, 0.4)' : undefined,
+ },
+ ],
+ },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#000000',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) &&
+ child.props.className === 'pvtColLabel active',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: 'rgba(0, 0, 0, 0.4)',
+ color: getTextColorForBackground(
+ { backgroundColor: 'rgba(0, 0, 0, 0.4)' },
+ supersetTheme.colorPrimaryBg,
+ ),
+ });
+});
+
+test('renderColHeaderRow preserves default header text color without
formatting', () => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: { metric: [] },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#ff00aa',
+ },
+ });
+
+ const row = tableRenderer.renderColHeaderRow('region', 0, {
+ rowAttrs: [],
+ colAttrs: ['region'],
+ visibleColKeys: [['EMEA']],
+ colAttrSpans: [[1]],
+ colKeys: [['EMEA']],
+ colSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ maxColVisible: 1,
+ maxRowVisible: 0,
+ pivotData: {} as PivotData,
+ namesMapping: {},
+ allowRenderHtml: false,
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ rowTotals: false,
+ colTotals: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) && child.props.className === 'pvtColLabel',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: undefined,
+ });
+});
+
+test('renderTableRow preserves default row-header text color without
formatting', () => {
+ tableRenderer = new TableRenderer({
+ ...mockProps,
+ tableOptions: {
+ cellColorFormatters: { metric: [] },
+ cellBackgroundColor: '#ffffff',
+ cellTextColor: '#ff00aa',
+ },
+ });
+
+ const row = tableRenderer.renderTableRow(['revenue'], 0, {
+ rowAttrs: ['metric'],
+ colAttrs: [],
+ rowAttrSpans: [[1]],
+ visibleColKeys: [[]],
+ pivotData: {
+ getAggregator: () => ({
+ value: () => 200,
+ format: (value: number) => String(value),
+ isSubtotal: false,
+ }),
+ } as any,
+ rowTotals: false,
+ rowSubtotalDisplay: {
+ displayOnTop: false,
+ enabled: false,
+ hideOnExpand: false,
+ },
+ arrowExpanded: null,
+ arrowCollapsed: null,
+ cellCallbacks: {},
+ rowTotalCallbacks: {},
+ namesMapping: {},
+ allowRenderHtml: false,
+ } as any) as React.ReactElement;
+
+ const cells = React.Children.toArray(row.props.children);
+ const headerCell = cells.find(
+ child =>
+ React.isValidElement(child) && child.props.className === 'pvtRowLabel',
+ ) as React.ReactElement;
+
+ expect(headerCell.props.style).toEqual({
+ backgroundColor: undefined,
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertion</b></div>
<div id="fix">
Similar to the column header test, 'renderTableRow preserves default
row-header text color without formatting' only checks backgroundColor but
should also verify that color is undefined to confirm default text color
preservation.
</div>
</div>
<small><i>Code Review Run #24bba0</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]
