codeant-ai-for-open-source[bot] commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2757462267
##########
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts:
##########
@@ -22,21 +22,26 @@
* @author Apache
*/
+import type { Rule } from 'eslint';
+import type { Node } from 'estree';
+
//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
-/** @type {import('eslint').Rule.RuleModule} */
-module.exports = {
+const plugin: { rules: Record<string, Rule.RuleModule> } = {
rules: {
'no-template-vars': {
- create(context) {
- function handler(node) {
- if (node.arguments.length) {
- const firstArgs = node.arguments[0];
+ create(context: Rule.RuleContext): Rule.RuleListener {
+ function handler(node: Node): void {
+ const callNode = node as Node & {
+ arguments: Array<Node & { expressions?: Node[] }>;
+ };
+ if (callNode.arguments.length) {
+ const firstArgs = callNode.arguments[0];
if (
Review Comment:
**Suggestion:** The `no-template-vars` rule only inspects the first argument
of the translation call, so template literals with variables in later arguments
(e.g. the plural string in `tn`) are never reported, which breaks the rule's
intent of banning variables in any translation template string. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Translation rule misses variable-containing plural templates.
- ⚠️ Linter allows translation strings with runtime variables.
- ⚠️ Developers may ship non-extractable translations.
```
</details>
```suggestion
arguments: Array<Node & { type: string; expressions?: Node[] }>;
};
for (const arg of callNode.arguments ?? []) {
if (
arg.type === 'TemplateLiteral' &&
(arg as Node & { expressions?: Node[] }).expressions?.length
) {
context.report({
node,
message:
"Don't use variables in translation string templates.
Flask-babel is a static translation service, so it can't handle strings that
include variables",
});
break;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run ESLint with this plugin enabled on the repo (plugin file at
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts). The
rule's handler is
defined at lines ~36-45 in that file.
2. Lint a source file that calls tn('singular template', 'plural template',
n) where the
plural template (2nd argument) is a TemplateLiteral containing expressions
(e.g. tn(`You
have ${count} item`, `You have ${count} items`, count)). The current handler
only inspects
the first argument (lines ~40-44) and ignores the second argument.
3. Observe that ESLint does not report a violation for the plural
TemplateLiteral with
embedded expressions, allowing variable-containing translation templates to
pass linting.
4. Confirm by searching the codebase for tn( occurrences (e.g. rg "tn\\(") —
plural
translation calls are common in UI labels/buttons — demonstrating this path
is reachable
by normal lint runs. The proposed change iterates over all arguments and
will catch
template literals in any argument.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts
**Line:** 38:42
**Comment:**
*Logic Error: The `no-template-vars` rule only inspects the first
argument of the translation call, so template literals with variables in later
arguments (e.g. the plural string in `tn`) are never reported, which breaks the
rule's intent of banning variables in any translation template string.
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-calendar/src/Calendar.ts:
##########
@@ -96,12 +98,13 @@ function Calendar(element, props) {
}
const timestamps = metricsData[metric];
const extents = d3Extent(Object.keys(timestamps), key => timestamps[key]);
- const step = (extents[1] - extents[0]) / (steps - 1);
+ const step =
Review Comment:
**Suggestion:** The code assumes that `d3Extent` always returns numeric
bounds, but when a metric has no data (or only non-numeric values)
`extents[0]`/`extents[1]` will be `undefined`, so using them in arithmetic and
as the domain for `createLinearScale` will yield `NaN` and can cause the
legend/color scale setup and downstream rendering to fail for that metric.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Calendar chart breaks for metrics with no data.
- ⚠️ Dashboard tiles may show empty/errored calendar visuals.
- ⚠️ Data edge-cases produce inconsistent colors.
```
</details>
```suggestion
if (extents[0] == null || extents[1] == null) {
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the per-metric loop in
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.ts
beginning near line
99 inside function Calendar. The code computes `extents` from timestamps via
d3Extent on
line 100.
2. Trigger the Calendar rendering with a metric whose timestamps object is
empty (e.g.,
metricsData[metric] === {}), or where timestamp values are non-numeric so
d3Extent returns
[undefined, undefined]. This is a realistic dataset scenario when a metric
has no samples
or upstream aggregation produced no buckets.
3. On an empty metric, `extents[0]` and/or `extents[1]` will be
null/undefined. The code
then executes the arithmetic on line 101 which uses those undefined values
and produces
NaN, which propagates into color scale creation and legend calculation.
4. Reproduce by mounting Calendar with props.data.data containing a metric
key that maps
to an empty object, then observe that legend/color calculations fail or
produce invalid
values and the calendar either renders incorrectly or throws at runtime.
Note: The suggested early-return (or alternatively skipping this metric)
prevents
downstream arithmetic on undefined extents.
```
</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-calendar/src/Calendar.ts
**Line:** 101:101
**Comment:**
*Logic Error: The code assumes that `d3Extent` always returns numeric
bounds, but when a metric has no data (or only non-numeric values)
`extents[0]`/`extents[1]` will be `undefined`, so using them in arithmetic and
as the domain for `createLinearScale` will yield `NaN` and can cause the
legend/color scale setup and downstream rendering to fail for that metric.
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/chart/components/FallbackComponent.tsx:
##########
@@ -39,7 +38,13 @@ export default function FallbackComponent({ error, height,
width }: Props) {
<div>
<b>{t('Oops! An error occurred!')}</b>
</div>
- <code>{error ? getErrorMessage(error) : 'Unknown Error'}</code>
+ <code>
+ {error instanceof Error
+ ? error.message
+ : error
+ ? String(error)
Review Comment:
**Suggestion:** Rendering the raw error message directly in the UI can
unintentionally expose internal implementation details or sensitive backend
error information to end users; instead, this fallback component should display
only a generic, non-sensitive error message. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart fallback UI may expose internal error messages.
- ⚠️ Unit tests depend on FallbackComponent rendering.
- ⚠️ Dashboard charts use SuperChart fallback component.
```
</details>
```suggestion
{'Unknown Error'}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a chart that triggers the fallback path. SuperChart imports the
default fallback
at `src/chart/components/SuperChart.tsx:39` (import
DefaultFallbackComponent) and passes
props here: `src/chart/components/SuperChart.tsx:221-222` where it renders
`<FallbackComponent width={width} height={height} {...props} />`.
2. Cause a chart render error (for example, a chart plugin throws during
render). The
error propagates to SuperChart's error handling and it renders the default
FallbackComponent at `src/chart/components/SuperChart.tsx:221-222`.
3. When FallbackComponent renders, it executes the code at
`src/chart/components/FallbackComponent.tsx:41-46` (the existing code block)
and will
display the raw `error.message` or `String(error)` directly in the UI.
4. Observe the UI showing the raw error text inside a <code> block (file and
lines:
`superset-frontend/packages/superset-ui-core/src/chart/components/FallbackComponent.tsx:41-46`).
This can reveal internal stack/messages returned by chart plugins or backend
errors. Note:
tests reference this component at
`test/chart/components/FallbackComponent.test.tsx:23-28`, so changing
display behavior
will affect those tests.
```
</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/chart/components/FallbackComponent.tsx
**Line:** 42:45
**Comment:**
*Security: Rendering the raw error message directly in the UI can
unintentionally expose internal implementation details or sensitive backend
error information to end users; instead, this fallback component should display
only a generic, non-sensitive error message.
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-map-box/src/MapBox.tsx:
##########
@@ -29,24 +28,46 @@ const NOOP = () => {};
export const DEFAULT_MAX_ZOOM = 16;
export const DEFAULT_POINT_RADIUS = 60;
-const propTypes = {
- width: PropTypes.number,
- height: PropTypes.number,
- aggregatorName: PropTypes.string,
- clusterer: PropTypes.object,
- globalOpacity: PropTypes.number,
- hasCustomMetric: PropTypes.bool,
- mapStyle: PropTypes.string,
- mapboxApiKey: PropTypes.string.isRequired,
- onViewportChange: PropTypes.func,
- pointRadius: PropTypes.number,
- pointRadiusUnit: PropTypes.string,
- renderWhileDragging: PropTypes.bool,
- rgb: PropTypes.array,
- bounds: PropTypes.array,
-};
+interface Viewport {
+ longitude: number;
+ latitude: number;
+ zoom: number;
+ isDragging?: boolean;
+}
+
+interface Clusterer {
+ getClusters(bbox: number[], zoom: number): GeoJSONLocation[];
+}
+
+interface GeoJSONLocation {
+ geometry: {
+ coordinates: [number, number];
+ };
+ properties: Record<string, number | string | boolean | null | undefined>;
+}
-const defaultProps = {
+interface MapBoxProps {
+ width?: number;
+ height?: number;
+ aggregatorName?: string;
+ clusterer?: Clusterer;
+ globalOpacity?: number;
+ hasCustomMetric?: boolean;
+ mapStyle?: string;
+ mapboxApiKey: string;
+ onViewportChange?: (viewport: Viewport) => void;
+ pointRadius?: number;
+ pointRadiusUnit?: string;
+ renderWhileDragging?: boolean;
+ rgb?: (string | number)[];
+ bounds?: [[number, number], [number, number]];
Review Comment:
**Suggestion:** The `bounds` prop is declared optional but is immediately
used with non-null assertions in `fitBounds` and bbox computation, so a caller
omitting `bounds` (which the type currently allows) will trigger a runtime
error instead of being caught by TypeScript. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Component initialization fails without bounds.
- ⚠️ Map viewport computation (fitBounds) affected.
- ⚠️ No fallback bounds defined in defaultProps.
```
</details>
```suggestion
bounds: [[number, number], [number, number]];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open MapBox.tsx and inspect MapBoxProps: bounds is declared optional at
line 63
(bounds?: [[number, number], [number, number]];).
2. Locate constructor usage at MapBox.tsx lines ~82-94 where
fitBounds(bounds!) is called:
new WebMercatorViewport(...).fitBounds(bounds!). The code uses bounds with a
non-null
assertion during construction.
3. Also locate render-time usage at MapBox.tsx lines ~135-142 where bbox is
computed from
bounds using bounds! to build array entries; both sites assume bounds exists.
4. Reproduce by rendering <MapBox ... /> without a bounds prop. Since
defaultProps does
not supply bounds, the constructor will call fitBounds(undefined) (due to
bounds!), which
will throw (or produce invalid mercator values) causing
initialization/runtime failure.
The exact failing lines are visible in the file at the constructor and bbox
computation
sites.
5. This is a realistic failure for any consumer that omits bounds (type
allows omission).
Making bounds required or adding runtime guards will prevent the crash.
```
</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-map-box/src/MapBox.tsx
**Line:** 63:63
**Comment:**
*Type Error: The `bounds` prop is declared optional but is immediately
used with non-null assertions in `fitBounds` and bbox computation, so a caller
omitting `bounds` (which the type currently allows) will trigger a runtime
error instead of being caught by TypeScript.
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.ts:
##########
@@ -213,8 +228,8 @@ function CountryMap(element, props) {
if (map) {
drawMap(map);
} else {
- const url = countries[country];
- d3.json(url, (error, mapData) => {
+ const url = (countries as Record<string, string>)[country];
+ d3.json(url, (error: unknown, mapData: GeoData) => {
if (error) {
Review Comment:
**Suggestion:** The code assumes that `country` always maps to a valid URL
in `countries`, but `transformProps` can pass `null` or an unmapped value, so
`url` may be `undefined` and `d3.json` will be called with an invalid URL,
leading to a failed request or runtime error instead of a controlled
user-facing error message. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Missing map data for unmapped country causes request failure.
- ⚠️ User may not see controlled error message.
- ⚠️ Console shows d3/json request errors.
```
</details>
```suggestion
if (!url) {
const countryName =
countryOptions.find(x => x[0] === country)?.[1] || country;
d3.select(element).html(
`<div class="alert alert-danger">Could not load map data for
${countryName}</div>`,
);
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a CountryMap chart (file:
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts).
The
map-loading logic is at the hunk starting line 228 (`const map =
maps[country];`) in the
PR diff.
2. Provide props where the `country` prop is null, undefined, or a value not
present in
the countries lookup (this can happen if transformProps or upstream control
passes an
unmapped ISO key). The code then computes `const url = (countries as
Record<string,
string>)[country];` at lines shown in the hunk.
3. If `url` is undefined, the current code calls `d3.json(url, ...)` with an
invalid URL.
This results in a failed network request or runtime error inside d3.json
before the
existing in-callback error branch can run, leading to either console errors
or no
user-facing message.
4. Observable result: no graceful "Could not load map data" alert is shown
in some failure
modes; instead the request may fail silently or throw earlier. Reproduce by
invoking the
legacy country map with an unmapped `country` value (e.g., via chart control
or by editing
the props passed to the renderer) and observe network and console 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/CountryMap.ts
**Line:** 233:233
**Comment:**
*Possible Bug: The code assumes that `country` always maps to a valid
URL in `countries`, but `transformProps` can pass `null` or an unmapped value,
so `url` may be `undefined` and `d3.json` will be called with an invalid URL,
leading to a failed request or runtime error instead of a controlled
user-facing error message.
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/eslint-rules/eslint-plugin-theme-colors/index.ts:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "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
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * @fileoverview Rule to warn about literal colors
+ * @author Apache
+ */
+
+import type { Rule } from 'eslint';
+import type { Node, SourceLocation } from 'estree';
+
+const COLOR_KEYWORDS: string[] = require('./colors');
+
+function hasHexColor(quasi: string): boolean {
+ const regex = /#([a-f0-9]{3}|[a-f0-9]{4}(?:[a-f0-9]{2}){0,2})\b/gi;
+ return !!quasi.match(regex);
+}
+
+function hasRgbColor(quasi: string): boolean {
+ const regex = /rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*(\d+(?:\.\d+)?))?\)/i;
+ return !!quasi.match(regex);
+}
+
+function hasLiteralColor(quasi: string, strict: boolean = false): boolean {
+ // matches literal colors at the start or end of a CSS prop
+ return COLOR_KEYWORDS.some((color: string) => {
+ const regexColon = new RegExp(`: ${color}`);
+ const regexSemicolon = new RegExp(` ${color};`);
+ return (
+ !!quasi.match(regexColon) ||
+ !!quasi.match(regexSemicolon) ||
+ (strict && quasi === color)
+ );
+ });
+}
+
+const WARNING_MESSAGE: string =
+ 'Theme color variables are preferred over rgb(a)/hex/literal colors';
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+interface TemplateElementNode extends Node {
+ value?: { raw: string };
+ parent?: Node & {
+ parent?: Node & { loc?: SourceLocation | null };
+ };
+}
+
+interface LiteralNode extends Node {
+ value?: string;
+ parent?: Node;
+}
+
+const plugin: { rules: Record<string, Rule.RuleModule> } = {
+ rules: {
+ 'no-literal-colors': {
+ create(context: Rule.RuleContext): Rule.RuleListener {
+ const warned: string[] = [];
+ return {
+ TemplateElement(node: Node): void {
+ const templateNode = node as TemplateElementNode;
+ const rawValue = templateNode?.value?.raw;
+ const isChildParentTagged =
+ templateNode?.parent?.parent?.type ===
'TaggedTemplateExpression';
+ const isChildParentArrow =
+ templateNode?.parent?.parent?.type === 'ArrowFunctionExpression';
+ const isParentTemplateLiteral =
+ templateNode?.parent?.type === 'TemplateLiteral';
+ const loc = templateNode?.parent?.parent?.loc;
+ const locId = loc && JSON.stringify(loc);
+ const hasWarned = locId ? warned.includes(locId) : false;
+ if (
+ !hasWarned &&
+ (isChildParentTagged ||
+ (isChildParentArrow && isParentTemplateLiteral)) &&
+ rawValue &&
+ (hasLiteralColor(rawValue) ||
+ hasHexColor(rawValue) ||
+ hasRgbColor(rawValue))
+ ) {
+ context.report(node, loc as SourceLocation, WARNING_MESSAGE);
+ if (locId) {
+ warned.push(locId);
+ }
+ }
+ },
+ Literal(node: Node): void {
+ const literalNode = node as LiteralNode;
+ const value = literalNode?.value;
+ const isParentProperty = literalNode?.parent?.type === 'Property';
+ const locId = JSON.stringify(node.loc);
+ const hasWarned = warned.includes(locId);
+
+ if (
Review Comment:
**Suggestion:** The `Literal` visitor assumes the literal `value` is always
a string and passes it directly into helper functions that call `.match`, but
in ESTree/ESLint ASTs `Literal.value` can be numbers, booleans, null, RegExp,
etc., so encountering a non-zero number or a RegExp literal in an object
property will cause a runtime `TypeError` when `.match` is called on a
non-string value; adding a type check to ensure only string literals are
processed avoids the crash while preserving the intended behavior. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ ESLint process can crash during lint runs.
- ⚠️ CI lint job may fail unexpectedly.
- ⚠️ Affects files with numeric/RegExp property literals.
```
</details>
```suggestion
if (typeof value !== 'string') {
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run ESLint that loads this plugin (file:
superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts).
2. During linting, the rule's visitor method Literal at index.ts:105 is
invoked for AST
Literal nodes.
3. If a property's value is a non-string literal (e.g. property: 42 or
property: /abc/),
the code at index.ts:107 assigns value to a non-string (number or RegExp).
4. The helper functions hasLiteralColor / hasHexColor / hasRgbColor (called
at
index.ts:116-118) call .match on the passed value; calling .match on a
non-string throws
TypeError at runtime.
5. Observe ESLint process crashing during lint run with a TypeError stack
trace
originating from index.ts lines 116-118 when processing such a file.
6. Reproduce locally by adding a test file with an object property using a
numeric literal
(e.g. { color: 42 }) and running eslint against that file; the visitor at
index.ts:105-123
will execute and trigger the crash.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts
**Line:** 112:112
**Comment:**
*Type Error: The `Literal` visitor assumes the literal `value` is
always a string and passes it directly into helper functions that call
`.match`, but in ESTree/ESLint ASTs `Literal.value` can be numbers, booleans,
null, RegExp, etc., so encountering a non-zero number or a RegExp literal in an
object property will cause a runtime `TypeError` when `.match` is called on a
non-string value; adding a type check to ensure only string literals are
processed avoids the crash while preserving the intended behavior.
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-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -186,7 +231,7 @@ class ScatterPlotGlowOverlay extends PureComponent {
if (location.properties.cluster) {
let clusterLabel = clusterLabelMap[i];
const scaledRadius = roundDecimal(
- (clusterLabel / maxLabel) ** 0.5 * radius,
+ ((clusterLabel as number) / maxLabel) ** 0.5 * radius,
Review Comment:
**Suggestion:** The cluster radius scaling divides by `maxLabel` without
guarding against it being zero or non‑finite; if all cluster aggregation values
are zero (e.g., `sum` of metrics that are all 0), `maxLabel` becomes 0 and the
expression `(clusterLabel / maxLabel) ** 0.5` yields `NaN`, causing the arc
radius to be `NaN` and cluster circles to fail to render. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Cluster markers disappear for zero-valued aggregations.
- ⚠️ Map visualization inaccurate for zero-sum datasets.
- ⚠️ Affects cluster rendering in ScatterPlotGlowOverlay.
```
</details>
```suggestion
const safeMaxLabel =
Number.isFinite(maxLabel) && maxLabel > 0 ? maxLabel : 1;
const scaledRadius = roundDecimal(
((clusterLabel as number) / safeMaxLabel) ** 0.5 * radius,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build and run the frontend with this PR applied so the modified component
is used.
2. Trigger rendering of the map visualization that uses
ScatterPlotGlowOverlay (the
component in
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx).
The
redraw logic executes in the redraw method (final file) which iterates
clusterLabelMap and
reaches the cluster rendering branch around lines 232-236.
3. Provide cluster data where every cluster aggregation value is 0 (for
example, the
server returns cluster properties with point_count > 0 but sum = 0 for a
'sum'
aggregation). The code computes maxLabel from clusterLabelMap (earlier in
redraw) and if
all values are zero maxLabel == 0.
4. When execution reaches the code at lines 232-236, the expression
((clusterLabel as
number) / maxLabel) ** 0.5 performs division by zero, producing NaN. The
computed
scaledRadius becomes NaN and is passed to ctx.arc(...), causing the cluster
circle drawing
to fail or be invisible. This reproduces a missing cluster markers situation
in the map UI
(observed at the paths and line numbers above).
```
</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-map-box/src/ScatterPlotGlowOverlay.tsx
**Line:** 233:234
**Comment:**
*Logic Error: The cluster radius scaling divides by `maxLabel` without
guarding against it being zero or non‑finite; if all cluster aggregation values
are zero (e.g., `sum` of metrics that are all 0), `maxLabel` becomes 0 and the
expression `(clusterLabel / maxLabel) ** 0.5` yields `NaN`, causing the arc
radius to be `NaN` and cluster circles to fail to render.
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-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -234,23 +279,28 @@ class ScatterPlotGlowOverlay extends PureComponent {
}
} else {
const defaultRadius = radius / 6;
- const radiusProperty = location.properties.radius;
- const pointMetric = location.properties.metric;
- let pointRadius =
- radiusProperty === null ? defaultRadius : radiusProperty;
- let pointLabel;
+ const radiusProperty = location.properties.radius as number | null;
+ const pointMetric = location.properties.metric as
+ | number
+ | string
+ | null;
+ let pointRadius: number =
+ radiusProperty === null
+ ? defaultRadius
+ : (radiusProperty as number);
+ let pointLabel: string | number | undefined;
if (radiusProperty !== null) {
Review Comment:
**Suggestion:** The radius handling treats only `null` as "missing" but not
`undefined`, so when `properties.radius` is absent (undefined) the code
initializes `pointRadius` to `undefined` and still enters the conversion block
for Kilometers/Miles/Pixels, leading to operations like `kmToPixels(undefined,
...)` and further math on `undefined`, which will produce `NaN` radii and
inconsistent or invisible points instead of cleanly falling back to the default
radius. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Some point markers render incorrectly or not at all.
- ⚠️ Point-size scaling for datasets missing radius values.
- ⚠️ Affects non-cluster point rendering logic in the component.
```
</details>
```suggestion
radiusProperty == null
? defaultRadius
: (radiusProperty as number);
let pointLabel: string | number | undefined;
if (radiusProperty != null) {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the app with the PR's ScatterPlotGlowOverlay component included.
2. Use a dataset where some point features omit the radius property entirely
(properties.radius is undefined rather than explicitly null). The component
reads
radiusProperty at line 282 in the file and currently narrows it as "as
number | null".
3. Execution reaches the non-cluster branch (around lines 279-293). Because
the code only
treats null as "missing", an undefined radiusProperty causes pointRadius to
be set to
(radiusProperty as number) — effectively undefined — rather than
defaultRadius.
4. The code then enters the conversion block (lines starting 293) and can
call
kmToPixels(undefined, ...), perform Number(undefined) or other math,
producing NaN for
pointRadius and resulting in invisible or incorrectly sized points on the
map. Observing
map points missing for rows where properties.radius was omitted reproduces
the issue;
relevant code locations are in the file at the lines above.
```
</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-map-box/src/ScatterPlotGlowOverlay.tsx
**Line:** 289:293
**Comment:**
*Logic Error: The radius handling treats only `null` as "missing" but
not `undefined`, so when `properties.radius` is absent (undefined) the code
initializes `pointRadius` to `undefined` and still enters the conversion block
for Kilometers/Miles/Pixels, leading to operations like `kmToPixels(undefined,
...)` and further math on `undefined`, which will produce `NaN` radii and
inconsistent or invisible points instead of cleanly falling back to the default
radius.
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-map-box/src/MapBox.tsx:
##########
@@ -29,24 +28,46 @@ const NOOP = () => {};
export const DEFAULT_MAX_ZOOM = 16;
export const DEFAULT_POINT_RADIUS = 60;
-const propTypes = {
- width: PropTypes.number,
- height: PropTypes.number,
- aggregatorName: PropTypes.string,
- clusterer: PropTypes.object,
- globalOpacity: PropTypes.number,
- hasCustomMetric: PropTypes.bool,
- mapStyle: PropTypes.string,
- mapboxApiKey: PropTypes.string.isRequired,
- onViewportChange: PropTypes.func,
- pointRadius: PropTypes.number,
- pointRadiusUnit: PropTypes.string,
- renderWhileDragging: PropTypes.bool,
- rgb: PropTypes.array,
- bounds: PropTypes.array,
-};
+interface Viewport {
+ longitude: number;
+ latitude: number;
+ zoom: number;
+ isDragging?: boolean;
+}
+
+interface Clusterer {
+ getClusters(bbox: number[], zoom: number): GeoJSONLocation[];
+}
+
+interface GeoJSONLocation {
+ geometry: {
+ coordinates: [number, number];
+ };
+ properties: Record<string, number | string | boolean | null | undefined>;
+}
-const defaultProps = {
+interface MapBoxProps {
+ width?: number;
+ height?: number;
+ aggregatorName?: string;
+ clusterer?: Clusterer;
Review Comment:
**Suggestion:** The `clusterer` prop is typed as optional even though the
component unconditionally dereferences it with a non-null assertion and calls
`getClusters`, which means any consumer that omits `clusterer` (and TypeScript
currently permits that) will cause a runtime crash instead of a compile-time
error. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Runtime crash during MapBox render (clusters computation).
- ⚠️ Any parent component that omits clusterer triggers failure.
- ⚠️ No default clusterer provided in defaultProps.
```
</details>
```suggestion
clusterer: Clusterer;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect MapBoxProps in
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx and
find clusterer
declared optional at line 53 (clusterer?: Clusterer;). This defines allowed
consumer
usage.
2. Find the render-time dereference at MapBox.tsx lines ~137-144 (clusters
computation):
clusterer!.getClusters(bbox, Math.round(viewport.zoom)); The code uses a
non-null
assertion and assumes clusterer is present.
3. Reproduce by mounting <MapBox ... /> without providing a clusterer prop.
Because
defaultProps does not provide a clusterer, the runtime value will be
undefined.
4. When render runs (initial mount or re-render), execution reaches
clusterer!.getClusters(...), invoking undefined.getClusters and producing a
TypeError. The
call site is visible in the file at the clusters assignment (MapBox.tsx
around lines
137-144).
5. This is a realistic crash path because many consumers might forget to
pass clusterer
(the type currently permits omission), and there is no runtime guard before
calling
getClusters.
```
</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-map-box/src/MapBox.tsx
**Line:** 53:53
**Comment:**
*Type Error: The `clusterer` prop is typed as optional even though the
component unconditionally dereferences it with a non-null assertion and calls
`getClusters`, which means any consumer that omits `clusterer` (and TypeScript
currently permits that) will cause a runtime crash instead of a compile-time
error.
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-horizon/src/transformProps.ts:
##########
@@ -16,7 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
-export default function transformProps(chartProps) {
+import { ChartProps } from '@superset-ui/core';
+
+export default function transformProps(chartProps: ChartProps) {
const { height, width, formData, queriesData } = chartProps;
const { horizonColorScale, seriesHeight } = formData;
Review Comment:
**Suggestion:** The code destructures `horizonColorScale` and `seriesHeight`
from `formData` using camelCase keys that don't match the control names
(`horizon_color_scale` and `series_height`), so both values will be
`undefined`, causing `colorScale` to ignore user configuration and
`parseInt(seriesHeight, 10)` to yield `NaN`, breaking the chart's layout.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Horizon chart color scale ignored by user configuration.
- ❌ Chart layout broken due to NaN series height.
- ⚠️ Affects all dashboards using Horizon chart plugin.
- ⚠️ Visualization regressions visible to end users.
```
</details>
```suggestion
const {
horizon_color_scale: horizonColorScale,
series_height: seriesHeight,
} = formData;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build and run the frontend with the PR changes applied so the chart
plugin code at
superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts
is used.
2. Open a dashboard containing the Horizon chart and edit its controls: set
a custom color
scale and change the series height in the chart controls UI (these controls
are the
sources of formData and are wired in snake_case like `horizon_color_scale` /
`series_height` in the control definitions).
3. When the chart renders, the transformProps function at
superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts:23
executes
the line `const { horizonColorScale, seriesHeight } = formData;` (line
referenced above),
which looks for camelCase keys that do not exist in the incoming formData
object.
4. As a result, horizonColorScale and seriesHeight are undefined. The
returned object sets
colorScale to undefined and calls `parseInt(seriesHeight, 10)` producing NaN
for
seriesHeight. Observe the chart either ignoring the configured color scale
or rendering
incorrectly (height/spacing broken) in the UI where Horizon charts are used.
5. Repeatable: every time the Horizon chart is used with non-default control
values, the
mis-mapping will occur because control values are provided under snake_case
keys by the
form system.
```
</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-horizon/src/transformProps.ts
**Line:** 23:23
**Comment:**
*Logic Error: The code destructures `horizonColorScale` and
`seriesHeight` from `formData` using camelCase keys that don't match the
control names (`horizon_color_scale` and `series_height`), so both values will
be `undefined`, causing `colorScale` to ignore user configuration and
`parseInt(seriesHeight, 10)` to yield `NaN`, breaking the chart's layout.
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/eslint-rules/eslint-plugin-i18n-strings/index.ts:
##########
@@ -76,29 +84,39 @@ module.exports = {
// Check for Button components
if (parent.type === 'JSXExpressionContainer') {
- const jsx = parent.parent;
+ const jsx = (parent as Node & { parent?: Node }).parent as
+ | (Node & {
+ openingElement?: { name: { name: string } };
+ })
+ | undefined;
if (jsx?.type === 'JSXElement') {
- const elementName = jsx.openingElement.name.name;
+ const elementName = jsx.openingElement?.name.name;
return elementName === 'Button';
}
}
return false;
}
- function handler(node) {
- if (node.arguments.length) {
- const firstArg = node.arguments[0];
+ function handler(node: Node): void {
+ const callNode = node as Node & {
+ arguments: Array<Node & { value?: string }>;
+ };
+ if (callNode.arguments.length) {
+ const firstArg = callNode.arguments[0];
if (
firstArg.type === 'Literal' &&
typeof firstArg.value === 'string'
) {
Review Comment:
**Suggestion:** The `sentence-case-buttons` rule only examines the first
argument of a `t`/`tn` call, so button labels passed as additional string
literal arguments (for example the plural label in `tn`) can still be in Title
Case without being flagged, which undermines the rule's goal of enforcing
sentence case for all button text. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Button labels in plural args remain Title Case.
- ⚠️ UI text inconsistency across buttons.
- ⚠️ Linter enforcement weakened for button text.
```
</details>
```suggestion
arguments: Array<Node & { type: string; value?: string }>;
};
for (const arg of callNode.arguments ?? []) {
if (
arg.type === 'Literal' &&
typeof arg.value === 'string'
) {
const text = arg.value;
if (
isButtonContext(node as Node & { parent?: Node }) &&
isTitleCase(text)
) {
const sentenceCase = text
.toLowerCase()
.replace(/^\w/, (c: string) => c.toUpperCase());
context.report({
node: arg,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run ESLint using the plugin (file:
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts). The
sentence-case-buttons handler is defined around lines ~101-116.
2. Lint a file where a Button label is provided via tn('singular', 'Plural
Title Case', n)
or where a component's button prop receives multiple string args and the
second/other
literal is Title Case (e.g. tn('Delete', 'Delete Datasets', count)).
3. Because the current handler only checks the first argument (firstArg at
line ~103), the
Title Case label in the second argument is not reported. The lint run shows
no violation
whereas the proposed rule should flag the Title Case plural/button label.
4. Verify prevalence by searching the codebase for tn( and multi-argument
t()/tn() usages
(e.g. rg "tn\\(") — these plural forms are used in UI where button labels
and plural
strings appear, making this a realistic lint gap.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts
**Line:** 103:110
**Comment:**
*Logic Error: The `sentence-case-buttons` rule only examines the first
argument of a `t`/`tn` call, so button labels passed as additional string
literal arguments (for example the plural label in `tn`) can still be in Title
Case without being flagged, which undermines the rule's goal of enforcing
sentence case for all button text.
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.ts:
##########
@@ -60,17 +73,17 @@ function CountryMap(element, props) {
const container = element;
const format = getNumberFormatter(numberFormat);
const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
+ .get(linearColorScheme)!
+ .createLinearScale(d3Extent(data, v => v.metric) as [number, number]);
Review Comment:
**Suggestion:** The sequential color scheme lookup uses a non-null assertion
and immediately calls `createLinearScale`, so if `linearColorScheme` is missing
or refers to a scheme that has been removed,
`getSequentialSchemeRegistry().get(linearColorScheme)` will return `undefined`
and the code will throw at runtime when trying to call `createLinearScale` on
it; this should fall back gracefully instead of crashing the chart. [possible
bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Country map chart crashes on invalid color scheme.
- ⚠️ Chart initialization halts before map drawing.
- ⚠️ Browser console shows uncaught TypeError.
```
</details>
```suggestion
const sequentialScheme =
getSequentialSchemeRegistry().get(linearColorScheme);
const linearColorScale =
sequentialScheme?.createLinearScale(
d3Extent(data, v => v.metric) as [number, number],
) ?? (() => '');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a CountryMap chart in the frontend with the PR code deployed
(component
implementation at
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts).
The code
that creates the sequential color scale is at the hunk starting line 73
(`const
linearColorScale = ...`) in the PR diff.
2. Trigger chart rendering with props where linearColorScheme is either
undefined, null,
or an invalid key (this happens when the chart control sends a scheme name
that no longer
exists). The call flow is: chart renderer -> legacy plugin loader -> this
CountryMap
function defined in the file above, which immediately executes the lines at
73–78.
3. At runtime getSequentialSchemeRegistry().get(linearColorScheme) returns
undefined for a
missing/invalid key; the code uses a non-null assertion (`!`) and then calls
`.createLinearScale(...)` on undefined. This causes a TypeError thrown
synchronously
during chart initialization, preventing the rest of CountryMap from running.
4. Observable result: the chart fails to render and an uncaught error is
visible in
browser console; no graceful user-facing error message is shown because the
exception
occurs before map drawing or error-handling branches. Confirm reproduction
by supplying an
invalid linearColorScheme value in the chart controls or inspecting
network/props for
charts using this legacy plugin.
```
</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.ts
**Line:** 75:77
**Comment:**
*Possible Bug: The sequential color scheme lookup uses a non-null
assertion and immediately calls `createLinearScale`, so if `linearColorScheme`
is missing or refers to a scheme that has been removed,
`getSequentialSchemeRegistry().get(linearColorScheme)` will return `undefined`
and the code will throw at runtime when trying to call `createLinearScale` on
it; this should fall back gracefully instead of crashing the chart.
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-horizon/src/HorizonChart.tsx:
##########
@@ -96,13 +97,13 @@ class HorizonChart extends PureComponent {
offsetX,
} = this.props;
- let yDomain;
+ let yDomain: [number, number] | undefined;
if (colorScale === 'overall') {
- const allValues = data.reduce(
+ const allValues = data.reduce<DataValue[]>(
(acc, current) => acc.concat(current.values),
[],
);
- yDomain = d3Extent(allValues, d => d.y);
+ yDomain = d3Extent(allValues, d => d.y) as [number, number];
Review Comment:
**Suggestion:** When `colorScale` is set to "overall" and the aggregated
`data` contains no numeric values, `d3Extent` can return `[undefined,
undefined]`, which is then cast to `[number, number]` and passed down as
`yDomain`, causing `HorizonRow` to build a y-scale with an invalid domain and
potentially producing NaN-based rendering behavior. Guard the extent result by
checking that there are values and that both min and max are non-null before
assigning `yDomain`, otherwise leave it undefined so each row can fall back to
its own extent logic. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Charts with colorScale='overall' may render incorrectly.
- ⚠️ Empty series produce invalid y-domain values.
- ⚠️ HorizonRow scale computation receives invalid input.
```
</details>
```suggestion
if (allValues.length > 0) {
const [min, max] = d3Extent(allValues, d => d.y);
if (min != null && max != null) {
yDomain = [min, max];
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file at
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonChart.tsx
and locate the
renderer at the render() method (lines ~86-120 in the PR hunk). The code
that computes
yDomain starts at line 100 in the PR diff: "let yDomain..." (see
existing_code_start_line).
2. Provide chart data where colorScale prop is set to 'overall' (the
component reads this
from this.props at lines ~86-96). In practice, any caller that renders
<HorizonChart
colorScale="overall" data={...} /> will follow this branch.
3. Supply data such that data.reduce produces an allValues array that is
empty or where
all DataValue.y entries are undefined/null (for example, data = [{ key:
['a'], values: []
}, { key: ['b'], values: [] }]). With the current code, d3Extent(allValues,
d => d.y) will
return [undefined, undefined].
4. The existing code casts the result to [number, number] (line 106),
assigning yDomain =
[undefined, undefined]. HorizonRow receives yDomain (mapped in the render
loop) and will
attempt to build a d3 scale with invalid domain leading to NaN coordinates
or no
rendering.
5. Observed behavior: rows render incorrectly (gaps, no bars, console
warnings from d3
scale usage) when all series are empty or contain non-numeric y values. The
proposed
improved_code guards the extent result by checking length and min/max
non-null before
setting yDomain, preventing passing an invalid domain to HorizonRow.
```
</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-horizon/src/HorizonChart.tsx
**Line:** 106:106
**Comment:**
*Possible Bug: When `colorScale` is set to "overall" and the aggregated
`data` contains no numeric values, `d3Extent` can return `[undefined,
undefined]`, which is then cast to `[number, number]` and passed down as
`yDomain`, causing `HorizonRow` to build a y-scale with an invalid domain and
potentially producing NaN-based rendering behavior. Guard the extent result by
checking that there are values and that both min and max are non-null before
assigning `yDomain`, otherwise leave it undefined so each row can fall back to
its own extent logic.
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]
