codeant-ai-for-open-source[bot] commented on code in PR #36541:
URL: https://github.com/apache/superset/pull/36541#discussion_r2611552667
##########
superset-frontend/src/utils/colorScheme.ts:
##########
@@ -103,8 +103,26 @@ export const getFreshLabelsColorMapEntries = (
Object.keys(customLabelsColor).forEach(label => {
delete allEntries[label];
});
+// Ensure no duplicate colors
+const usedColors = new Set<string>();
Review Comment:
**Suggestion:** The `usedColors` set is initialized empty, but custom label
colors were removed from `allEntries` earlier; those custom colors must be
treated as already-used to avoid reassigning them to other labels. Initialize
`usedColors` with the values from `customLabelsColor` so reserved colors are
not reassigned. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const usedColors = new Set<string>(Object.values(customLabelsColor));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
customLabelsColor keys were removed from allEntries, but their color values
remain part of the global pool and could be reassigned to other labels. Seeding
usedColors with Object.values(customLabelsColor) prevents reusing those
reserved colors and preserves developer/user expectations for explicit custom
colors.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/colorScheme.ts
**Line:** 107:107
**Comment:**
*Logic Error: The `usedColors` set is initialized empty, but custom
label colors were removed from `allEntries` earlier; those custom colors must
be treated as already-used to avoid reassigning them to other labels.
Initialize `usedColors` with the values from `customLabelsColor` so reserved
colors are not reassigned.
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>
##########
superset-frontend/src/utils/colorScheme.ts:
##########
@@ -103,8 +103,26 @@ export const getFreshLabelsColorMapEntries = (
Object.keys(customLabelsColor).forEach(label => {
delete allEntries[label];
});
+// Ensure no duplicate colors
+const usedColors = new Set<string>();
+const updatedEntries: Record<string, string> = {};
+
+Object.entries(allEntries).forEach(([label, color]) => {
+ let finalColor = color;
+
+ // If this color is already used, pick the next available color
+ if (usedColors.has(color)) {
+ const availableColors =
Object.values(labelsColorMapInstance.getColorMap());
Review Comment:
**Suggestion:** The code calls
Object.values(labelsColorMapInstance.getColorMap()) inside the loop, but
`getColorMap()` returns an iterable of entries (used earlier with
Object.fromEntries). Using Object.values on that iterable will not produce the
expected array of color values; this can yield an empty array and the
duplicate-avoidance logic will fail. Use the already-built `allEntries` object
to derive available colors (Object.values(allEntries)) so you get the actual
color values. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const availableColors = Object.values(allEntries);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
getColorMap() is used earlier with Object.fromEntries, which implies it
returns an entries iterable/array of [label, color] pairs — calling
Object.values(...) on that will not yield the plain color values you want.
Using Object.values(allEntries) (where allEntries is an object label->color)
returns the actual color values and fixes the bug where the duplicate-avoidance
could fail or return an empty list.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/colorScheme.ts
**Line:** 115:115
**Comment:**
*Possible Bug: The code calls
Object.values(labelsColorMapInstance.getColorMap()) inside the loop, but
`getColorMap()` returns an iterable of entries (used earlier with
Object.fromEntries). Using Object.values on that iterable will not produce the
expected array of color values; this can yield an empty array and the
duplicate-avoidance logic will fail. Use the already-built `allEntries` object
to derive available colors (Object.values(allEntries)) so you get the actual
color values.
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>
--
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]
