codeant-ai-for-open-source[bot] commented on code in PR #36535:
URL: https://github.com/apache/superset/pull/36535#discussion_r2611267881
##########
superset-frontend/src/utils/colorScheme.ts:
##########
@@ -27,6 +27,30 @@ import { intersection, omit, pick } from 'lodash';
import { areObjectsEqual } from 'src/reduxUtils';
const EMPTY_ARRAY: string[] = [];
+const dashboardColorRegistry = {
+ usedColors: new Set<string>(),
+ labelColorMap: {} as Record<string, string>,
+};
+
+export function getDashboardColor(label: string, palette: string[]): string {
+ if (dashboardColorRegistry.labelColorMap[label]) {
Review Comment:
**Suggestion:** When `getDashboardColor` is called with an empty `palette`,
the modulo operation uses `palette.length` (0), producing `NaN` and indexing
into an empty array, which results in `undefined` being returned despite the
function being typed to return a string; adding an explicit guard for empty
palettes avoids this runtime logic error and ensures a valid fallback color is
always returned. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if (!palette.length) {
const defaultColor = '#B3B3B3';
dashboardColorRegistry.labelColorMap[label] =
dashboardColorRegistry.labelColorMap[label] ?? defaultColor;
return dashboardColorRegistry.labelColorMap[label];
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current implementation will break when palette is an empty array: the
for-loop is skipped and the fallback computation does a modulo by
palette.length (0), producing NaN and indexing into palette returns undefined —
violating the string return type and causing downstream consumers to get
undefined. The suggested guard is a correct defensive fix: ensure a
deterministic string fallback when palette is empty. This is a real runtime
bug, not a cosmetic change.
</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:** 36:36
**Comment:**
*Logic Error: When `getDashboardColor` is called with an empty
`palette`, the modulo operation uses `palette.length` (0), producing `NaN` and
indexing into an empty array, which results in `undefined` being returned
despite the function being typed to return a string; adding an explicit guard
for empty palettes avoids this runtime logic error and ensures a valid fallback
color is always returned.
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/visualizations/TimeTable/utils/colorUtils/colorUtils.ts:
##########
@@ -7,7 +7,7 @@
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0color
Review Comment:
**Suggestion:** The Apache license URL in the header comment has an extra
"color" suffix, making the link invalid; this breaks the standard license
notice and should be corrected to the canonical Apache 2.0 URL. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
* http://www.apache.org/licenses/LICENSE-2.0
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a genuine typo in the license header: the "color" suffix corrupts
the canonical Apache 2.0 license URL. Fixing it restores the correct license
notice and a valid link; it's not a mere stylistic change and should be
corrected. There is no functional code impact, but legal/header correctness
matters.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/visualizations/TimeTable/utils/colorUtils/colorUtils.ts
**Line:** 10:10
**Comment:**
*Possible Bug: The Apache license URL in the header comment has an
extra "color" suffix, making the link invalid; this breaks the standard license
notice and should be corrected to the canonical Apache 2.0 URL.
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]
