codeant-ai-for-open-source[bot] commented on code in PR #41208:
URL: https://github.com/apache/superset/pull/41208#discussion_r3488700699
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx:
##########
@@ -99,6 +103,70 @@ const adhocMetricB = {
optionName: 'def',
};
+test('coerceMetrics regenerates duplicate optionNames so each metric stays
unique', () => {
+ // A saved chart can carry two adhoc metrics with the same optionName (e.g.
+ // born from a duplicated metric). Since edits are matched by optionName, the
+ // duplicates must be split apart on load or editing one overwrites the
other.
+ const dup = 'shared_option';
+ const result = coerceMetrics(
+ [
+ {
+ expressionType: EXPRESSION_TYPES.SIMPLE,
+ column: defaultProps.columns[0],
+ aggregate: AGGREGATES.SUM,
+ optionName: dup,
+ },
+ {
+ expressionType: EXPRESSION_TYPES.SIMPLE,
+ column: defaultProps.columns[1],
+ aggregate: AGGREGATES.AVG,
+ optionName: dup,
+ },
+ ] as any,
Review Comment:
**Suggestion:** Replace this `any` cast with a concrete metric input type
(for example a typed adhoc metric array) so the test data is type-checked.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test code introduces an `any` assertion in TypeScript at this line,
which directly violates the llmcfg-no-any-types rule. A concrete metric input
type should be used instead.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=974a099d3902452ab636aa800c668834&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=974a099d3902452ab636aa800c668834&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
**Line:** 125:125
**Comment:**
*Custom Rule: Replace this `any` cast with a concrete metric input type
(for example a typed adhoc metric array) so the test data is type-checked.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41208&comment_hash=fa3eb8063f9fcb54a0c7c7d98cdea7b81ff7b0e77d45d01b92af77081c90633e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41208&comment_hash=fa3eb8063f9fcb54a0c7c7d98cdea7b81ff7b0e77d45d01b92af77081c90633e&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx:
##########
@@ -99,6 +103,70 @@ const adhocMetricB = {
optionName: 'def',
};
+test('coerceMetrics regenerates duplicate optionNames so each metric stays
unique', () => {
+ // A saved chart can carry two adhoc metrics with the same optionName (e.g.
+ // born from a duplicated metric). Since edits are matched by optionName, the
+ // duplicates must be split apart on load or editing one overwrites the
other.
+ const dup = 'shared_option';
+ const result = coerceMetrics(
+ [
+ {
+ expressionType: EXPRESSION_TYPES.SIMPLE,
+ column: defaultProps.columns[0],
+ aggregate: AGGREGATES.SUM,
+ optionName: dup,
+ },
+ {
+ expressionType: EXPRESSION_TYPES.SIMPLE,
+ column: defaultProps.columns[1],
+ aggregate: AGGREGATES.AVG,
+ optionName: dup,
+ },
+ ] as any,
+ defaultProps.savedMetrics as unknown as Metric[],
+ defaultProps.columns,
+ ) as AdhocMetric[];
+
+ expect(result).toHaveLength(2);
+ // First keeps the optionName, second is regenerated to avoid the collision.
+ expect(result[0].optionName).toBe(dup);
+ expect(result[1].optionName).not.toBe(dup);
+ // Each metric definition is otherwise preserved.
+ expect(result[0].aggregate).toBe(AGGREGATES.SUM);
+ expect(result[1].aggregate).toBe(AGGREGATES.AVG);
+});
+
+test('coerceMetrics regenerates duplicate optionNames for SQL adhoc metrics
too', () => {
+ // The same collision can happen with custom SQL metrics, which take a
+ // different code path than column-backed metrics but must dedupe the same
way.
+ const dup = 'shared_option';
+ const result = coerceMetrics(
+ [
+ {
+ expressionType: EXPRESSION_TYPES.SQL,
+ sqlExpression: 'COUNT(*)',
+ label: 'count',
+ optionName: dup,
+ },
+ {
+ expressionType: EXPRESSION_TYPES.SQL,
+ sqlExpression: 'SUM(value)',
+ label: 'total',
+ optionName: dup,
+ },
+ ] as any,
Review Comment:
**Suggestion:** Remove this `any` assertion and cast the SQL metric fixture
to an explicit adhoc metric type to satisfy strict typing. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The SQL metric test data also uses an `any` assertion in new TypeScript
code, so this is another real instance of the llmcfg-no-any-types violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=58fc65a1bc074cfcbafbe77a7e44fcee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=58fc65a1bc074cfcbafbe77a7e44fcee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
**Line:** 157:157
**Comment:**
*Custom Rule: Remove this `any` assertion and cast the SQL metric
fixture to an explicit adhoc metric type to satisfy strict typing.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41208&comment_hash=e83c15e414d47bdfa7a2880e5dabe35d5b6ac127bde2fc081f438763861a8cb4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41208&comment_hash=e83c15e414d47bdfa7a2880e5dabe35d5b6ac127bde2fc081f438763861a8cb4&reaction=dislike'>👎</a>
--
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]
