codeant-ai-for-open-source[bot] commented on code in PR #36103:
URL: https://github.com/apache/superset/pull/36103#discussion_r2606738940
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts:
##########
@@ -93,8 +96,7 @@ declare module 'react-table' {
}
export interface ColumnInterface<D extends object>
- extends UseGlobalFiltersColumnOptions<D>,
- UseSortByColumnOptions<D> {
+ extends UseGlobalFiltersColumnOptions<D>, UseSortByColumnOptions<D> {
Review Comment:
**Suggestion:** The `ColumnInterface` was extended with
`UseGlobalFiltersColumnOptions<D>`, which is not imported in this file and will
cause a TypeScript unresolved type error; remove the unreferenced type (or
import the correct column-level global filter type) — minimal fix shown removes
the unimported symbol and keeps the already-imported `UseSortByColumnOptions`.
[type error]
**Severity Level:** Minor ⚠️
```suggestion
extends UseSortByColumnOptions<D> {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, actionable fix: UseGlobalFiltersColumnOptions isn't
imported and likely wasn't intended here. Dropping it and keeping
UseSortByColumnOptions fixes the unresolved type while preserving sort-related
typings.
</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-table/src/DataTable/types/react-table.d.ts
**Line:** 99:99
**Comment:**
*Type Error: The `ColumnInterface` was extended with
`UseGlobalFiltersColumnOptions<D>`, which is not imported in this file and will
cause a TypeScript unresolved type error; remove the unreferenced type (or
import the correct column-level global filter type) — minimal fix shown removes
the unimported symbol and keeps the already-imported `UseSortByColumnOptions`.
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/plugins/legacy-plugin-chart-country-map/src/transformProps.js:
##########
@@ -24,8 +24,31 @@ export default function transformProps(chartProps) {
selectCountry,
colorScheme,
sliceId,
+ customColorRules,
+ customColorScale,
} = formData;
+ let parsedColorRules = [];
+ let parsedColorScale = [];
+
+ try {
+ parsedColorRules = customColorRules ? JSON.parse(customColorRules) : [];
+ } catch (error) {
+ console.warn(
+ 'Invalid JSON in customColorRules. Please check your configuration
syntax:',
+ error && error.message ? error.message : error,
+ );
Review Comment:
**Suggestion:** Passing non-string values (objects/arrays) to JSON.parse
will throw a runtime error or a SyntaxError; ensure you only call JSON.parse on
strings and otherwise accept already-parsed objects/arrays or fallback to an
empty array. Also explicitly reset `parsedColorRules` in the catch so its value
is predictable after a failed parse. [type error]
**Severity Level:** Minor ⚠️
```suggestion
if (!customColorRules) {
parsedColorRules = [];
} else if (typeof customColorRules === 'string') {
parsedColorRules = JSON.parse(customColorRules);
} else {
// already an object/array — accept as-is
parsedColorRules = customColorRules;
}
} catch (error) {
console.warn(
'Invalid JSON in customColorRules. Please check your configuration
syntax:',
error && error.message ? error.message : error,
);
parsedColorRules = [];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code unconditionally calls JSON.parse when a value exists — if
the caller already passes an object/array, JSON.parse will throw. Guarding on
typeof and accepting already-parsed objects avoids unnecessary runtime errors
and makes the behavior predictable. Resetting to [] in the catch also makes the
post-error state explicit (though parsedColorRules is initialized to []
already).
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js
**Line:** 35:40
**Comment:**
*Type Error: Passing non-string values (objects/arrays) to JSON.parse
will throw a runtime error or a SyntaxError; ensure you only call JSON.parse on
strings and otherwise accept already-parsed objects/arrays or fallback to an
empty array. Also explicitly reset `parsedColorRules` in the catch so its value
is predictable after a failed parse.
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/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +64,46 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ sliceId: PropTypes.number,
+ customColorRules: PropTypes.array,
+ minColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ maxColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ customColorScale: PropTypes.array,
};
const maps = {};
+function safeNumber(v) {
+ if (v === null || v === undefined || v === '') return NaN;
+ const n = Number(v);
+ return Number.isFinite(n) ? n : NaN;
+}
+
+function rgbaToHex(rgba) {
+ if (typeof rgba === 'string') return rgba;
+ if (Array.isArray(rgba)) return rgbaToHex(rgba[0]);
+ if (!rgba || typeof rgba !== 'object') return null;
+ const { r, g, b } = rgba;
+ if (r === undefined || g === undefined || b === undefined) return null;
+ const toHex = n => {
+ const hex = Math.round(n).toString(16);
+ return hex.length === 1 ? `0${hex}` : hex;
+ };
+ return `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+}
+
+function normalizeScale(scale) {
+ if (Array.isArray(scale)) return scale;
+ if (typeof scale === 'string') {
+ try {
+ return JSON.parse(scale);
Review Comment:
**Suggestion:** `normalizeScale` currently returns an empty array if the
JSON-parsed value is an object (not an array). If the caller passes a single
scale object serialized as JSON, it will be ignored. Update `normalizeScale` to
accept a JSON string that parses to either an array or a single object by
wrapping a parsed object into an array. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const parsed = JSON.parse(scale);
// If a single object was provided, wrap it into an array for
consistent downstream handling
if (Array.isArray(parsed)) return parsed;
if (parsed && typeof parsed === 'object') return [parsed];
return [];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct. Currently a JSON string that parses to a single object will be
returned as an object and later discarded because callers expect an array
(normalizedScaleWithColors uses Array.isArray). Wrapping a parsed object into
an array makes downstream code behave as intended and fixes a real bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
**Line:** 99:99
**Comment:**
*Logic Error: `normalizeScale` currently returns an empty array if the
JSON-parsed value is an object (not an array). If the caller passes a single
scale object serialized as JSON, it will be ignored. Update `normalizeScale` to
accept a JSON string that parses to either an array or a single object by
wrapping a parsed object into an array.
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/explore/components/controls/VizTypeControl/VizTile.tsx:
##########
@@ -56,10 +56,10 @@ export const VizTile = ({
setShowTooltip(
Boolean(
!isTransitioning &&
- (!isActive ||
- (chartNameRef.current &&
- chartNameRef.current.scrollWidth >
- chartNameRef.current.clientWidth)),
+ (!isActive ||
+ (chartNameRef.current &&
+ chartNameRef.current.scrollWidth >
+ chartNameRef.current.clientWidth)),
),
);
Review Comment:
**Suggestion:** The boolean calculation is repeated and uses `Boolean(...)`
with a complex inline expression; factor out the measured element once, use
optional chaining/explicit check, and set a clear boolean to avoid subtle
coercion and repeated property access. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
{
const el = chartNameRef.current;
const isOverflowing = el ? el.scrollWidth > el.clientWidth : false;
setShowTooltip(!isTransitioning && (!isActive || isOverflowing));
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Factoring out chartNameRef.current into a local variable and using an
explicit boolean improves readability and avoids repeated property access; it
doesn't change semantics and is a safe, beneficial refactor.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/VizTypeControl/VizTile.tsx
**Line:** 56:64
**Comment:**
*Logic Error: The boolean calculation is repeated and uses
`Boolean(...)` with a complex inline expression; factor out the measured
element once, use optional chaining/explicit check, and set a clear boolean to
avoid subtle coercion and repeated property access.
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/packages/superset-ui-core/src/components/Table/VirtualTable.tsx:
##########
@@ -31,8 +31,9 @@ import { useTheme, styled } from '@apache-superset/core/ui';
import { TableSize, ETableAction } from './index';
-export interface VirtualTableProps<RecordType>
- extends AntTableProps<RecordType> {
+export interface VirtualTableProps<
+ RecordType,
Review Comment:
**Suggestion:** Inconsistent generic constraint: the `VirtualTable`
component uses `RecordType extends object` in its generic, but the new
`VirtualTableProps` declaration does not constrain `RecordType`; make the
interface generic consistent by adding `extends object` to avoid allowing
incompatible types and to keep type constraints aligned. [type error]
**Severity Level:** Minor ⚠️
```suggestion
RecordType extends object,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good, actionable suggestion. The component signature uses `RecordType
extends object`
while the props interface currently allows any type. Adding `extends object`
keeps
the API consistent and prevents accidental use with primitive record types
that
would be incompatible with how the component expects row objects.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx
**Line:** 35:35
**Comment:**
*Type Error: Inconsistent generic constraint: the `VirtualTable`
component uses `RecordType extends object` in its generic, but the new
`VirtualTableProps` declaration does not constrain `RecordType`; make the
interface generic consistent by adding `extends object` to avoid allowing
incompatible types and to keep type constraints aligned.
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/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx:
##########
@@ -74,5 +74,10 @@ export default styled(CountryMap)`
cursor: pointer;
stroke: ${theme.colorSplit};
}
+
+ g.text-layer text, g.text-layer text.big-text, g.text-layer
text.result-text {
Review Comment:
**Suggestion:** The selector list `g.text-layer text, g.text-layer
text.big-text, g.text-layer text.result-text` is redundant because
`g.text-layer text` already matches text elements with classes; keep a single
selector to avoid duplication and reduce maintenance risk. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
g.text-layer text {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The selector `g.text-layer text` already matches all descendant <text>
elements, including
those with `big-text` and `result-text` classes. Consolidating to a single
selector reduces
duplication and maintenance burden without changing behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx
**Line:** 78:78
**Comment:**
*Logic Error: The selector list `g.text-layer text, g.text-layer
text.big-text, g.text-layer text.result-text` is redundant because
`g.text-layer text` already matches text elements with classes; keep a single
selector to avoid duplication and reduce maintenance risk.
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]
