korbit-ai[bot] commented on code in PR #35897:
URL: https://github.com/apache/superset/pull/35897#discussion_r2475634709
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts:
##########
@@ -58,4 +59,5 @@ export type FormattingPopoverProps = PopoverProps & {
export type ConditionalFormattingFlag = {
toAllRowCheck?: boolean;
toColorTextCheck?: boolean;
+ toCellBarCheck?: boolean;
};
Review Comment:
### Inconsistent Property Naming Pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The naming convention between ConditionalFormattingConfig and
ConditionalFormattingFlag is inconsistent. The Config uses 'toAllRow',
'toTextColor', while the Flag uses 'toAllRowCheck', 'toColorTextCheck'.
###### Why this matters
Inconsistent naming patterns increase cognitive load and make the codebase
harder to maintain. It also violates the principle of least surprise.
###### Suggested change ∙ *Feature Preview*
Align the naming patterns either by using the 'Check' suffix consistently or
removing it entirely. Example:
```typescript
export type ConditionalFormattingFlag = {
toAllRow?: boolean;
toTextColor?: boolean;
toCellBar?: boolean;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c653ed-c948-498f-b9da-bff9fcae3005/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c653ed-c948-498f-b9da-bff9fcae3005?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c653ed-c948-498f-b9da-bff9fcae3005?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c653ed-c948-498f-b9da-bff9fcae3005?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c653ed-c948-498f-b9da-bff9fcae3005)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cd7fd92e-3e3e-4815-9374-794c1599f600 -->
[](cd7fd92e-3e3e-4815-9374-794c1599f600)
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -300,6 +306,8 @@ export const FormattingPopoverContent = ({
[columns, column],
);
+ const columnTypeNumeric = columnType === GenericDataType.Numeric;
Review Comment:
### Missing memoization for columnTypeNumeric computation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The columnTypeNumeric computation is performed on every render even when the
columnType hasn't changed.
###### Why this matters
This causes unnecessary re-computation on each render cycle, potentially
impacting performance in components that render frequently or have many
conditional formatting controls.
###### Suggested change ∙ *Feature Preview*
Wrap the computation in useMemo with columnType as dependency:
```typescript
const columnTypeNumeric = useMemo(
() => columnType === GenericDataType.Numeric,
[columnType]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/88496038-3737-4ca8-a6ab-5c76e6dc7c09/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/88496038-3737-4ca8-a6ab-5c76e6dc7c09?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/88496038-3737-4ca8-a6ab-5c76e6dc7c09?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/88496038-3737-4ca8-a6ab-5c76e6dc7c09?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/88496038-3737-4ca8-a6ab-5c76e6dc7c09)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3d235b75-b9df-4d77-ac1f-bec979b487ea -->
[](3d235b75-b9df-4d77-ac1f-bec979b487ea)
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -883,8 +884,12 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
if (formatter.toTextColor) {
color = formatterResult.slice(0, -2);
+ } else if (formatter.toCellBar) {
+ if (showCellBars)
+ backgroundColorCellBar = formatterResult.slice(0, -2);
Review Comment:
### Inconsistent showCellBars condition between formatter and renderer
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The toCellBar formatter logic only applies when showCellBars is true, but
the cell bar rendering logic checks both valueRange and valueRangeFlag
conditions independently.
###### Why this matters
This creates inconsistent behavior where a formatter can set
backgroundColorCellBar but it won't be used if showCellBars is false at render
time, or conversely, if showCellBars changes after the formatter runs, the
stored color may be used unexpectedly.
###### Suggested change ∙ *Feature Preview*
Remove the showCellBars condition from the formatter logic and let the
renderer handle the display decision consistently:
```typescript
else if (formatter.toCellBar) {
backgroundColorCellBar = formatterResult.slice(0, -2);
}
```
This ensures the color is always captured when a toCellBar formatter is
present, and the renderer will decide whether to use it based on the valueRange
and valueRangeFlag conditions.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b458437-9be0-4730-bbe8-c8d0e0cabe90/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b458437-9be0-4730-bbe8-c8d0e0cabe90?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b458437-9be0-4730-bbe8-c8d0e0cabe90?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b458437-9be0-4730-bbe8-c8d0e0cabe90?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b458437-9be0-4730-bbe8-c8d0e0cabe90)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1d0653ae-248c-490f-abb0-a60e146c9bb0 -->
[](1d0653ae-248c-490f-abb0-a60e146c9bb0)
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -776,12 +776,17 @@ const config: ControlPanelConfig = {
item.colorScheme &&
!['Green', 'Red'].includes(item.colorScheme)
) {
- if (!item.toAllRow || !item.toTextColor) {
+ if (
+ !item.toAllRow ||
+ !item.toTextColor ||
+ !item.toCellBar
+ ) {
// eslint-disable-next-line no-param-reassign
array[index] = {
...item,
toAllRow: item.toAllRow ?? false,
toTextColor: item.toTextColor ?? false,
+ toCellBar: item.toCellBar ?? false,
};
}
Review Comment:
### Inefficient conditional property assignment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition checks if any of the three properties are falsy, but then
always sets all three properties to their current values or false, regardless
of which specific property triggered the condition.
###### Why this matters
This logic will unnecessarily overwrite properties that are already properly
set. For example, if `toAllRow` is true and `toTextColor` is true, but
`toCellBar` is undefined, the code will still reset `toAllRow` and
`toTextColor` to their current values, creating unnecessary object mutations
and potential performance issues.
###### Suggested change ∙ *Feature Preview*
Only set properties that are actually undefined or need defaulting:
```typescript
if (
item.toAllRow === undefined ||
item.toTextColor === undefined ||
item.toCellBar === undefined
) {
// eslint-disable-next-line no-param-reassign
array[index] = {
...item,
...(item.toAllRow === undefined && { toAllRow: false }),
...(item.toTextColor === undefined && { toTextColor: false }),
...(item.toCellBar === undefined && { toCellBar: false }),
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72599b00-f0cd-4872-acb0-f8b4bcd0f951/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72599b00-f0cd-4872-acb0-f8b4bcd0f951?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72599b00-f0cd-4872-acb0-f8b4bcd0f951?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72599b00-f0cd-4872-acb0-f8b4bcd0f951?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72599b00-f0cd-4872-acb0-f8b4bcd0f951)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:99d56e8d-32e8-4a9c-9942-0aabffa82d61 -->
[](99d56e8d-32e8-4a9c-9942-0aabffa82d61)
--
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]
