codeant-ai-for-open-source[bot] commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2760819629
##########
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts:
##########
@@ -22,27 +22,33 @@
* @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 {
Review Comment:
**Suggestion:** Each ESLint rule module is currently missing the required
`meta` property, which violates the `RuleModule` contract and can cause tooling
or newer ESLint versions to mis-handle the rule (e.g., missing docs, potential
runtime errors when `meta` is assumed to exist); adding a minimal `meta`
definition to the rule fixes this integration issue. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ESLint rule docs missing in dev tooling.
- ⚠️ Editor integrations may omit rule info.
- ⚠️ Rule introspection (fix-type) unavailable.
```
</details>
```suggestion
meta: {
type: 'problem',
docs: {
description:
'Disallow variables in translation string templates for
Flask-babel compatibility',
recommended: false,
},
schema: [],
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the repository contains the plugin at
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts (file
present and
exported via module.exports at bottom of the file; see file end where
module.exports =
plugin).
2. Configure ESLint to load the plugin (for example, add the plugin name to
.eslintrc and
refer to the rule "i18n-strings/no-template-vars"). ESLint will require the
rule module
shape when initializing rules.
3. Run ESLint (e.g., npx eslint .) with a recent ESLint version which
introspects
rule.meta (common in tooling that builds docs, fixers, or rule reports).
During rule
initialization, tooling may access rule.meta for docs/type information.
Because the rule
at superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts:34-35
defines no
meta, tooling that accesses meta will receive undefined.
4. Observable failure modes: some ESLint adapters or UI tools (editor
integrations, doc
generators) will omit documentation for the rule or may throw if they assume
meta exists.
Reproducing locally: run ESLint with an editor plugin that shows rule
descriptions or a
docs generator; the rule will show missing docs or metadata. The existing
code
intentionally exports the rule implementation (create) without meta (line
34-35). Adding a
minimal meta block as in the improved_code at the same location provides the
expected
contract.
```
</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:** 35:35
**Comment:**
*Possible Bug: Each ESLint rule module is currently missing the
required `meta` property, which violates the `RuleModule` contract and can
cause tooling or newer ESLint versions to mis-handle the rule (e.g., missing
docs, potential runtime errors when `meta` is assumed to exist); adding a
minimal `meta` definition to the rule fixes this integration issue.
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:
##########
@@ -53,19 +59,22 @@ module.exports = {
},
},
'sentence-case-buttons': {
- create(context) {
- function isTitleCase(str) {
+ create(context: Rule.RuleContext): Rule.RuleListener {
Review Comment:
**Suggestion:** Similar to the first rule, the `sentence-case-buttons`
ESLint rule is missing a `meta` property, so ESLint and related tooling cannot
reliably introspect the rule (for docs, fix-type, etc.) and may error if `meta`
is assumed to exist; defining `meta` aligns the implementation with ESLint's
expected rule shape. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Rule documentation absent in developer tools.
- ⚠️ Editor rule descriptions not shown.
- ⚠️ CI lint reporting less informative.
```
</details>
```suggestion
meta: {
type: 'suggestion',
docs: {
description:
'Enforce sentence case for translated button text in Superset
UI',
recommended: false,
},
schema: [],
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The rule is defined in
superset-frontend/eslint-rules/eslint-plugin-i18n-strings/index.ts and
exported via
module.exports = plugin (see file end). The rule object key is
'sentence-case-buttons' at
lines 61-62.
2. Add or enable the rule in an ESLint configuration that loads this local
plugin and
requires rule metadata for docs or automatic fixes (common in CI or editor
integrations).
3. Run ESLint (npx eslint .) or trigger the editor plugin which queries
rule.meta to show
description/fixability. Because the rule currently provides create(...) but
lacks meta,
tooling that expects meta will not be able to show description/fix-type or
may treat the
rule as malformed.
4. Confirm effect: absence of rule documentation in editors or docs
generators, or
potential warnings from ESLint about non-conforming rule modules. The
improved_code
inserts a minimal meta block adjacent to the existing create(...) function
at the same
location (lines 61-62) to satisfy the RuleModule contract.
```
</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:** 62:62
**Comment:**
*Possible Bug: Similar to the first rule, the `sentence-case-buttons`
ESLint rule is missing a `meta` property, so ESLint and related tooling cannot
reliably introspect the rule (for docs, fix-type, etc.) and may error if `meta`
is assumed to exist; defining `meta` aligns the implementation with ESLint's
expected rule shape.
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,146 @@
+/**
+ * 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 {
+ type: string;
+ value?: { raw: string };
+ loc?: SourceLocation | null;
+ parent?: {
+ type: string;
+ parent?: { type: string; loc?: SourceLocation | null };
+ };
+}
+
+interface LiteralNode {
+ type: string;
+ value?: unknown;
+ loc?: SourceLocation | null;
+ parent?: { type: string };
+}
+
+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: loc as SourceLocation,
+ message: WARNING_MESSAGE,
+ });
+ if (locId) {
+ warned.push(locId);
+ }
+ }
+ },
+ Literal(node: Node): void {
+ const literalNode = node as LiteralNode;
+ const value = literalNode?.value;
+ // Only process string literals (not numbers, booleans, null, or
RegExp)
+ if (typeof value !== 'string') {
+ return;
+ }
+ const isParentProperty = literalNode?.parent?.type === 'Property';
+ const locId = JSON.stringify(node.loc);
+ const hasWarned = warned.includes(locId);
+
+ if (
+ !hasWarned &&
+ isParentProperty &&
+ (hasLiteralColor(value, true) ||
+ hasHexColor(value) ||
+ hasRgbColor(value))
+ ) {
+ context.report({
+ node,
+ loc: node.loc as SourceLocation,
+ message: WARNING_MESSAGE,
+ });
+ warned.push(locId);
Review Comment:
**Suggestion:** In the Literal handler, `JSON.stringify(node.loc)` is used
without checking for null/undefined and `node.loc` is always cast into `loc:
node.loc as SourceLocation` when reporting; if a Literal node lacks location
information, this will again pass an invalid `loc` to `context.report` and may
cause ESLint to crash rather than simply reporting at the node's default
location. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ESLint runtime crash when Literal lacks location.
- ⚠️ Linting may fail on transformed ASTs.
- ⚠️ CI lint stage could be unstable.
```
</details>
```suggestion
const loc = node.loc;
const locId = loc && JSON.stringify(loc);
const hasWarned = locId ? warned.includes(locId) : false;
if (
!hasWarned &&
isParentProperty &&
(hasLiteralColor(value, true) ||
hasHexColor(value) ||
hasRgbColor(value))
) {
const reportDescriptor: Rule.ReportDescriptor = {
node,
message: WARNING_MESSAGE,
};
if (loc) {
reportDescriptor.loc = loc;
}
context.report(reportDescriptor);
if (locId) {
warned.push(locId);
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Literal handler in
superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts starting
at line 114;
locate the lines that compute locId via JSON.stringify(node.loc) (line 122)
and then call
context.report with loc: node.loc as SourceLocation (lines ~132–136).
2. Execute ESLint using this plugin on an AST where Literal nodes may lack
location
metadata (node.loc is null/undefined), which can happen with some AST
transforms or parser
settings.
3. The code will attempt JSON.stringify(node.loc) producing "undefined" or
"null"
serialization, then call context.report({ node, loc: undefined as
SourceLocation, ... })
(lines ~132–136). Passing an invalid loc may cause ESLint internals to
attempt to read
loc.start/loc.end and throw, turning a linting warning into a runtime
exception that
aborts the lint run.
4. The improved code first guards loc with const loc = node.loc, only builds
locId when
loc exists, and only adds loc to the report descriptor when present —
preventing
context.report from receiving an invalid loc and avoiding crashes.
```
</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:** 122:137
**Comment:**
*Possible Bug: In the Literal handler, `JSON.stringify(node.loc)` is
used without checking for null/undefined and `node.loc` is always cast into
`loc: node.loc as SourceLocation` when reporting; if a Literal node lacks
location information, this will again pass an invalid `loc` to `context.report`
and may cause ESLint to crash rather than simply reporting at the node's
default location.
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/CronPicker/CronPicker.stories.tsx:
##########
@@ -28,22 +28,16 @@ export default {
};
export const InteractiveCronPicker = (props: CronProps) => {
- // @ts-ignore
- const inputRef = useRef<Input>(null);
const [value, setValue] = useState(props.value);
Review Comment:
**Suggestion:** The local `value` state is only initialized from
`props.value` once and never updated when `props.value` changes, so if
Storybook controls or parent code modify the `value` prop after initial render
the input and CronPicker will display stale data that no longer reflects the
current args; syncing state from `props.value` on change avoids this
desynchronization. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Storybook interactive controls show stale cron value.
- ⚠️ Developers test incorrect UI state locally.
```
</details>
```suggestion
import { useState, useCallback, useEffect } from 'react';
import { Divider } from '../Divider';
import { Input } from '../Input';
import { CronPicker } from '.';
import type { CronError, CronProps } from './types';
export const InteractiveCronPicker = (props: CronProps) => {
const [value, setValue] = useState(props.value);
useEffect(() => {
setValue(props.value);
}, [props.value]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Storybook and load the story defined in
superset-frontend/packages/superset-ui-core/src/components/CronPicker/CronPicker.stories.tsx
(file path shown in the story list). The story component is
InteractiveCronPicker declared
at the top of the file (`export const InteractiveCronPicker = (props:
CronProps) => {`),
starting at line 30 in the PR hunk.
2. Confirm initial render uses the incoming prop: InteractiveCronPicker
initializes local
state from props via useState(props.value) (file lines 30–32 in the final
file; referenced
above as lines 19–35 in the hunk). At this point the input and CronPicker
reflect that
initial value.
3. Use Storybook Controls to change the `value` arg at runtime (Storybook
control UI
updates args). Because the component sets local state only on mount
(useState) and does
not synchronize on subsequent prop changes, the Input and CronPicker
continue showing the
previous local `value` instead of the control-updated arg.
4. Observe the desynchronization in the rendered UI: editing the controls
updates
Storybook's args but the visible Input (and CronPicker) remain stale. This
reproduces the
behavior without any external code changes and demonstrates the missing
props-to-state
sync.
Note: This reproduction is based on the final file's explicit
useState(props.value)
initialization and absence of useEffect to update state on props.value
changes (see file
content lines shown above). The suggested useEffect would keep the UI in
sync with
Storybook controls.
```
</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/CronPicker/CronPicker.stories.tsx
**Line:** 19:31
**Comment:**
*Logic Error: The local `value` state is only initialized from
`props.value` once and never updated when `props.value` changes, so if
Storybook controls or parent code modify the `value` prop after initial render
the input and CronPicker will display stale data that no longer reflects the
current args; syncing state from `props.value` on change avoids this
desynchronization.
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/sorters.test.ts:
##########
@@ -45,20 +45,20 @@ const rows = [
* 1 or greater means the first item comes before the second item
*/
test('alphabeticalSort sorts correctly', () => {
- // @ts-ignore
+ // @ts-expect-error
expect(alphabeticalSort('name', rows[0], rows[1])).toBe(-1);
- // @ts-ignore
+ // @ts-expect-error
expect(alphabeticalSort('name', rows[1], rows[0])).toBe(1);
Review Comment:
**Suggestion:** The alphabetical sort tests assert exact values `-1` and
`1`, but the comparator contract (and `localeCompare`) only guarantees
negative/positive values, so these tests are brittle and can fail on compliant
engines that return different negative/positive magnitudes; they should instead
assert `< 0` or `> 0`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Flaky tests across JS engines or comparator implementations.
- ⚠️ Table sorting comparator unit tests unreliable.
```
</details>
```suggestion
expect(alphabeticalSort('name', rows[0], rows[1])).toBeLessThan(0);
// @ts-expect-error
expect(alphabeticalSort('name', rows[1], rows[0])).toBeGreaterThan(0);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Jest against the test file at
superset-frontend/packages/superset-ui-core/src/components/Table/sorters.test.ts
(alphabeticalSort test block starts at line 47).
2. The test asserts exact return values:
expect(alphabeticalSort(...)).toBe(-1) and
toBe(1) (lines 47-54). The comparator contract used by antd and typical
implementations
(e.g., String.prototype.localeCompare) only guarantees
negative/zero/positive results —
not specific -1 or 1 magnitudes.
3. On some JS engines or implementations localeCompare/compare functions may
return
negative values other than -1 (for example -2) while still being correct;
these tests
would then fail even though alphabeticalSort is correct.
4. Replace equality checks with relational checks (.toBeLessThan(0) /
.toBeGreaterThan(0))
at lines 47-54 to correctly validate comparator sign rather than exact
magnitude; re-run
tests to confirm stability across engines.
```
</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/sorters.test.ts
**Line:** 49:51
**Comment:**
*Logic Error: The alphabetical sort tests assert exact values `-1` and
`1`, but the comparator contract (and `localeCompare`) only guarantees
negative/positive values, so these tests are brittle and can fail on compliant
engines that return different negative/positive magnitudes; they should instead
assert `< 0` or `> 0`.
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/generator-superset/test/plugin-chart.test.ts:
##########
@@ -19,6 +19,7 @@
import { dirname, join } from 'path';
import helpers, { result } from 'yeoman-test';
Review Comment:
**Suggestion:** The test imports `result` from `yeoman-test` and calls
`result.assertFile`, but `yeoman-test` does not expose a shared `result`
object; instead, `helpers.run(...)` returns a run result instance on which
`assertFile` should be called, so this misuse will cause `result` to be
undefined or the wrong type and make the test fail at runtime. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ CI test run fails for generator-superset tests.
- ⚠️ Blocks PR verification and merge pipeline.
- ⚠️ Developers cannot rely on generator template tests.
```
</details>
##########
superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts:
##########
@@ -0,0 +1,146 @@
+/**
+ * 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 {
+ type: string;
+ value?: { raw: string };
+ loc?: SourceLocation | null;
+ parent?: {
+ type: string;
+ parent?: { type: string; loc?: SourceLocation | null };
+ };
+}
+
+interface LiteralNode {
+ type: string;
+ value?: unknown;
+ loc?: SourceLocation | null;
+ parent?: { type: string };
+}
+
+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: loc as SourceLocation,
+ message: WARNING_MESSAGE,
+ });
+ if (locId) {
+ warned.push(locId);
+ }
+ }
+ },
+ Literal(node: Node): void {
+ const literalNode = node as LiteralNode;
+ const value = literalNode?.value;
+ // Only process string literals (not numbers, booleans, null, or
RegExp)
+ if (typeof value !== 'string') {
+ return;
+ }
+ const isParentProperty = literalNode?.parent?.type === 'Property';
Review Comment:
**Suggestion:** The Literal handler currently treats any string Literal with
a parent of type `Property` as a color, which means it will incorrectly flag
object *keys* like `{ red: 1 }` as literal color usage even though the color
name appears in the key, not the value; this causes false positives and breaks
the intended semantics of only checking property values. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ False positive lint warnings on object keys named like colors.
- ⚠️ Linter noise during frontend development iterations.
- ⚠️ CI lint stage may fail or require ignore comments.
```
</details>
```suggestion
const parent = literalNode?.parent as
| (LiteralNode['parent'] & { value?: unknown })
| undefined;
const isParentProperty =
parent?.type === 'Property' && parent.value === node;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the ESLint rule file at
superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts and
locate the Literal
handler starting at line 114 (TemplateElement handler ends at ~113 and
Literal handler
begins at 114).
2. Note the existing check at line 121: "const isParentProperty =
literalNode?.parent?.type === 'Property';" — this treats any string Literal
whose parent
is a Property node as a candidate for color checking.
3. Reproduce by running ESLint with this plugin on code containing an object
with a key
that is a color name, e.g. { red: 1 } in any source file. The AST will
create a Property
node where the key is a Literal with value "red" and parent.type ===
'Property'.
4. The rule will treat that key Literal as a property value and run
hasLiteralColor/hasHexColor/hasRgbColor against it, producing a
false-positive report
(warning) for a non-color value used as an object key. This can be observed
as an
unexpected lint warning referencing the location of the object key.
5. The proposed change narrows the check to only consider Property nodes
where the node is
the property's value (parent.value === node), avoiding reporting on keys.
This matches
common AST shapes and prevents the false positive.
```
</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:** 121:121
**Comment:**
*Logic Error: The Literal handler currently treats any string Literal
with a parent of type `Property` as a color, which means it will incorrectly
flag object *keys* like `{ red: 1 }` as literal color usage even though the
color name appears in the key, not the value; this causes false positives and
breaks the intended semantics of only checking property 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>
##########
superset-frontend/eslint-rules/eslint-plugin-icons/no-fontawesome.test.ts:
##########
@@ -22,16 +22,18 @@
* @author Apache
*/
+import type { Rule } from 'eslint';
+
const { RuleTester } = require('eslint');
-const plugin = require('.');
+const plugin: { rules: Record<string, Rule.RuleModule> } = require('.');
//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------
const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
Review Comment:
**Suggestion:** The RuleTester is configured without enabling JSX parsing,
so the JSX snippets in the `valid` and `invalid` test cases (e.g.
`<Icons.Database />`, `<i className="fa fa-database"></i>`) will cause parse
errors instead of exercising the rule, making the tests fail or not correctly
validate the rule behavior. To fix this, enable JSX via
`parserOptions.ecmaFeatures.jsx: true` when instantiating `RuleTester`. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ ESLint rule tests fail due to JSX parse errors.
- ⚠️ CI job running lint/rule-tests will be red.
- ⚠️ Rule behavior not validated in test suite.
```
</details>
```suggestion
const ruleTester = new RuleTester({
parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } },
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test suite (e.g., `npm test` / `yarn test`) which executes the
ESLint rule
tests in
`superset-frontend/eslint-rules/eslint-plugin-icons/no-fontawesome.test.ts`.
2. Test runner loads the file
`superset-frontend/eslint-rules/eslint-plugin-icons/no-fontawesome.test.ts`
and
instantiates RuleTester at the location shown in the file: line 33 (`const
ruleTester =
new RuleTester({ parserOptions: { ecmaVersion: 6 } });`).
3. RuleTester attempts to parse the provided `valid`/`invalid` snippets (for
example
`'<Icons.Database />'` and `'<i className="fa fa-database"></i>'` located
later in the
same file) but JSX syntax is not enabled in parser options, causing the
parser to throw a
syntax/parse error instead of running the rule assertions.
4. The test process reports parsing errors (failing the rule test suite)
rather than
exercising the rule implementation at
`superset-frontend/eslint-rules/eslint-plugin-icons/index.js`
(plugin.rules['no-fa-icons-usage']), so the rule behavior is not validated.
5. Confirm fix: change the instantiation at line 33 to enable JSX via
`parserOptions.ecmaFeatures.jsx: true` (the suggested improved_code). Re-run
tests;
RuleTester will parse JSX and the rule tests will execute normally.
```
</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-icons/no-fontawesome.test.ts
**Line:** 33:33
**Comment:**
*Logic Error: The RuleTester is configured without enabling JSX
parsing, so the JSX snippets in the `valid` and `invalid` test cases (e.g.
`<Icons.Database />`, `<i className="fa fa-database"></i>`) will cause parse
errors instead of exercising the rule, making the tests fail or not correctly
validate the rule behavior. To fix this, enable JSX via
`parserOptions.ecmaFeatures.jsx: true` when instantiating `RuleTester`.
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-icons/index.ts:
##########
@@ -39,19 +55,20 @@ module.exports = {
},
schema: [],
},
- create(context) {
+ create(context: Rule.RuleContext): Rule.RuleListener {
return {
// Check for JSX elements with class names containing "fa"
- JSXElement(node) {
+ JSXElement(node: Node): void {
+ const jsxNode = node as JSXElementNode;
if (
- node.openingElement &&
- node.openingElement.name.name === 'i' &&
- node.openingElement.attributes &&
- node.openingElement.attributes.some(
- attr =>
+ jsxNode.openingElement &&
+ jsxNode.openingElement.name.name === 'i' &&
+ jsxNode.openingElement.attributes &&
+ jsxNode.openingElement.attributes.some(
+ (attr: JSXAttribute) =>
attr.name &&
attr.name.name === 'className' &&
- /fa fa-/.test(attr.value.value),
+ /fa fa-/.test(attr.value?.value ?? ''),
Review Comment:
**Suggestion:** The rule only inspects `className` when it is a simple
literal string, so JSX like `<i className={'fa fa-home'} />` or `<i
className={someVar} />` that resolves to a FontAwesome class is never reported,
which violates the rule's own description of disallowing FontAwesome icons; you
can fix this by also checking `JSXExpressionContainer` values for a string
literal containing the `fa fa-` pattern. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Missed FontAwesome usages in components using expression className.
- ⚠️ Linter allows disallowed icons to slip into code reviews.
- ⚠️ Maintainers may be misled about icon migration progress.
```
</details>
```suggestion
(attr: JSXAttribute) => {
if (!attr.name || attr.name.name !== 'className') {
return false;
}
// Handle both literal and JSX expression container values
const valueNode = attr.value as
| { value?: string }
| { expression?: { value?: string } }
| undefined;
const classNameValue =
valueNode?.value ??
(valueNode as { expression?: { value?: string } }).expression
?.value ??
'';
return /fa fa-/.test(classNameValue);
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the rule implementation at
superset-frontend/eslint-rules/eslint-plugin-icons/index.ts and locate the
JSXElement
visitor starting at line 61 which checks for `<i>` elements (JSXElement at
index.ts:61).
2. Create a test file or a component that uses a JSX expression for
className, for
example: <i className={'fa fa-home'} /> or <i className={someVar} />. These
are common
usages across React components.
3. Run ESLint with this local plugin enabled against that file. The existing
check at
index.ts lines 63-72 only inspects attr.value?.value (literal string) and
will not match
className provided via a JSXExpressionContainer or other expression forms.
4. Observe that the rule does not report a violation even though the
rendered class
contains "fa fa-" (the intended forbidden pattern). This reproduces the
missed-detection
behavior in real code that uses expression-based className values.
5. Confirm by modifying the rule to inspect expression-contained literals
(as in the
improved_code): rerun ESLint and verify the same test files now trigger the
rule.
```
</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-icons/index.ts
**Line:** 65:71
**Comment:**
*Logic Error: The rule only inspects `className` when it is a simple
literal string, so JSX like `<i className={'fa fa-home'} />` or `<i
className={someVar} />` that resolves to a FontAwesome class is never reported,
which violates the rule's own description of disallowing FontAwesome icons; you
can fix this by also checking `JSXExpressionContainer` values for a string
literal containing the `fa fa-` pattern.
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,146 @@
+/**
+ * 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 {
+ type: string;
+ value?: { raw: string };
+ loc?: SourceLocation | null;
+ parent?: {
+ type: string;
+ parent?: { type: string; loc?: SourceLocation | null };
+ };
+}
+
+interface LiteralNode {
+ type: string;
+ value?: unknown;
+ loc?: SourceLocation | null;
+ parent?: { type: string };
+}
+
+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: loc as SourceLocation,
+ message: WARNING_MESSAGE,
Review Comment:
**Suggestion:** In the TemplateElement handler, `loc` is derived from
`templateNode?.parent?.parent?.loc` and then always passed to `context.report`
as `loc: loc as SourceLocation`; if for any reason the parent node lacks
location information (e.g., parser options disable locations), this will pass
`undefined` as a `loc`, which can cause ESLint to throw when trying to access
`loc.start`/`loc.end` instead of simply falling back to the node's own
location. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ESLint runtime error when node.loc absent.
- ⚠️ Linter crash in non-standard parser configs.
- ⚠️ Blocking CI lint runs in some environments.
```
</details>
```suggestion
const reportDescriptor: Rule.ReportDescriptor = {
node,
message: WARNING_MESSAGE,
};
if (loc) {
reportDescriptor.loc = loc;
}
context.report(reportDescriptor);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the TemplateElement handler in
superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts around
lines 83–113.
The code computes loc via templateNode?.parent?.parent?.loc (line 92) and
later always
passes loc into context.report as loc: loc as SourceLocation (line ~104).
2. Run ESLint with this plugin on code parsed with parser options that do
not populate
location data (for example, a custom parser configuration or transformed AST
where loc is
omitted). In that scenario, templateNode?.parent?.parent?.loc will be
undefined.
3. When the rule reaches the reporting branch (conditions at lines 96–103
satisfied), it
will call context.report({ node, loc: undefined as SourceLocation, message:
... }) (lines
~104–108). Passing an undefined loc may cause downstream callers or ESLint
internals to
access loc.start/loc.end and throw a runtime error instead of producing the
intended
warning.
4. The improved code builds the report descriptor and only sets
reportDescriptor.loc when
loc is present, ensuring context.report is called with a valid descriptor
and avoiding
runtime crashes when node location information is absent.
```
</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:** 104:107
**Comment:**
*Possible Bug: In the TemplateElement handler, `loc` is derived from
`templateNode?.parent?.parent?.loc` and then always passed to `context.report`
as `loc: loc as SourceLocation`; if for any reason the parent node lacks
location information (e.g., parser options disable locations), this will pass
`undefined` as a `loc`, which can cause ESLint to throw when trying to access
`loc.start`/`loc.end` instead of simply falling back to the node's own location.
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)
+ : 'Unknown Error'}
Review Comment:
**Suggestion:** The fallback message for unknown errors is a hardcoded
English string that is not passed through the translation function, which
breaks localization and results in untranslated text for users on non-English
locales while other visible strings in this component are localized. [possible
bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart fallback UI displays untranslated text in some locales.
- ⚠️ Inconsistent localization across chart error messages.
- ⚠️ Minor user confusion for non-English users.
```
</details>
```suggestion
: t('Unknown Error')}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build or run the frontend with the PR changes so the file
`superset-frontend/packages/superset-ui-core/src/chart/components/FallbackComponent.tsx`
is used (file present in final state).
2. Trigger a chart rendering path that raises an error but does not provide
a typed Error
message (so the `error` prop is falsy or non-Error but truthy). The
component render path
is in this file at lines 26-51; the fallback UI is rendered from lines 28-36
and the
textual fallback at lines 41-47.
3. Observe the UI text shown inside the <code> element at lines 41-47.
Because the ternary
falls through to the literal 'Unknown Error' (lines 45-46), that exact
English string is
rendered without going through the translation function `t()` used elsewhere
in this file
(e.g., line 39 uses t('Oops! An error occurred!')).
4. In a non-English locale where translations exist for 'Unknown Error', the
literal
'Unknown Error' will remain untranslated, causing inconsistent localization
in the chart
fallback UI — verify by switching the app locale and reproducing the same
error condition;
the string at lines 41-47 stays English while the header at line 39 is
localized.
Note: The existing file already imports and uses the translation function
`t` at line 20;
replacing the literal with `t('Unknown Error')` aligns with established
usage in this
component.
```
</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:** 46:46
**Comment:**
*Possible Bug: The fallback message for unknown errors is a hardcoded
English string that is not passed through the translation function, which
breaks localization and results in untranslated text for users on non-English
locales while other visible strings in this component are localized.
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/sorters.test.ts:
##########
@@ -68,39 +68,39 @@ test('numericalSort sorts correctly', () => {
* In the case the sorter cannot perform the comparison it should return
undefined and the next sort step will proceed without error
*/
test('alphabeticalSort bad inputs no errors', () => {
- // @ts-ignore
+ // @ts-expect-error
expect(alphabeticalSort('name', null, null)).toBe(undefined);
// incorrect non-object values
- // @ts-ignore
+ // @ts-expect-error
expect(alphabeticalSort('name', 3, [])).toBe(undefined);
// incorrect object values without specified key
expect(alphabeticalSort('name', {}, {})).toBe(undefined);
// Object as value for name when it should be a string
expect(
alphabeticalSort(
'name',
- // @ts-ignore
+ // @ts-expect-error
{ name: { title: 'the name attribute should not be an object' } },
{ name: 'Doug' },
),
).toBe(undefined);
});
test('numericalSort bad inputs no errors', () => {
- // @ts-ignore
+ // @ts-expect-error
expect(numericalSort('name', undefined, undefined)).toBe(NaN);
Review Comment:
**Suggestion:** Using `toBe(NaN)` will always fail because Jest's `toBe`
uses strict equality and `NaN === NaN` is false, so these tests will fail even
when `numericalSort` correctly returns `NaN` for bad inputs; use Jest's
`toBeNaN()` matcher instead. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ CI test suite failures blocking PR merges.
- ⚠️ Developer feedback loop slowed by failing tests.
```
</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]
