codeant-ai-for-open-source[bot] commented on code in PR #36923:
URL: https://github.com/apache/superset/pull/36923#discussion_r2664543395
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/isDerivedSeries.ts:
##########
@@ -17,22 +17,13 @@
* specific language governing permissions and limitationsxw
* under the License.
*/
-import {
- ensureIsArray,
- JsonObject,
- QueryFormData,
- ComparisonType,
-} from '@superset-ui/core';
+import { ensureIsArray, JsonObject, QueryFormData } from '@superset-ui/core';
import { hasTimeOffset } from './timeOffset';
export const isDerivedSeries = (
series: JsonObject,
formData: QueryFormData,
): boolean => {
- const comparisonType = formData.comparison_type;
- if (comparisonType !== ComparisonType.Values) {
- return false;
- }
const timeCompare: string[] = ensureIsArray(formData?.time_compare);
Review Comment:
**Suggestion:** `ensureIsArray` can return non-string elements; asserting
the result as `string[]` without filtering may pass non-strings into
`hasTimeOffset` and cause logic/runtime issues—filter the array to only strings
to guarantee the expected type. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const timeCompare = ensureIsArray(formData?.time_compare).filter((v): v is
string => typeof v === 'string');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion tightens runtime/type-safety: ensureIsArray can legally
return values of any type, so narrowing/filtering to strings before passing to
hasTimeOffset avoids surprising runtime behavior or type-unsafe assumptions.
The proposed type guard `.filter((v): v is string => typeof v === 'string')` is
correct and improves robustness.
</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-chart-controls/src/operators/utils/isDerivedSeries.ts
**Line:** 27:27
**Comment:**
*Type Error: `ensureIsArray` can return non-string elements; asserting
the result as `string[]` without filtering may pass non-strings into
`hasTimeOffset` and cause logic/runtime issues—filter the array to only strings
to guarantee the expected type.
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-chart-controls/src/operators/utils/timeOffset.ts:
##########
@@ -36,7 +36,7 @@ export const hasTimeOffset = (
timeCompare: string[],
): boolean =>
typeof series.name === 'string'
- ? !!getTimeOffset(series, timeCompare)
+ ? !!getTimeOffset(series, timeCompare) || timeCompare.includes(series.name)
: false;
Review Comment:
**Suggestion:** Performance bug: calling `getTimeOffset` (which scans
`timeCompare`) and then calling `timeCompare.includes(...)` performs two
separate linear scans over `timeCompare`; compute the offset once and reuse the
result to avoid the duplicate O(n) work. [performance]
**Severity Level:** Minor ⚠️
```suggestion
): boolean => {
if (typeof series.name !== 'string') return false;
const foundOffset = getTimeOffset(series, timeCompare);
if (foundOffset) return true;
return Array.isArray(timeCompare) && timeCompare.includes(series.name);
};
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Calling getTimeOffset (which iterates timeCompare) and then
timeCompare.includes(...) results in two linear scans. Caching the result of
getTimeOffset and reusing it avoids the duplicate O(n) work with no behavioral
change, so this is a valid micro-optimization.
</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-chart-controls/src/operators/utils/timeOffset.ts
**Line:** 37:40
**Comment:**
*Performance: Performance bug: calling `getTimeOffset` (which scans
`timeCompare`) and then calling `timeCompare.includes(...)` performs two
separate linear scans over `timeCompare`; compute the offset once and reuse the
result to avoid the duplicate O(n) work.
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-chart-controls/src/operators/utils/isDerivedSeries.ts:
##########
@@ -17,22 +17,13 @@
* specific language governing permissions and limitationsxw
* under the License.
*/
-import {
- ensureIsArray,
- JsonObject,
- QueryFormData,
- ComparisonType,
-} from '@superset-ui/core';
+import { ensureIsArray, JsonObject, QueryFormData } from '@superset-ui/core';
Review Comment:
**Suggestion:** Importing type-only symbols (`JsonObject`, `QueryFormData`)
as runtime values can cause bundlers or runtimes to attempt to import
non-existent value exports and break at runtime; change them to type-only
imports so only `ensureIsArray` is imported as a runtime value. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
import { ensureIsArray } from '@superset-ui/core';
import type { JsonObject, QueryFormData } from '@superset-ui/core';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, low-risk improvement. JsonObject and QueryFormData are
types — changing them to `import type` avoids the (rare) situation where a
bundler/transform tries to resolve non-existent runtime exports.
TypeScript will erase types on emit, but using `import type` is the correct,
explicit form and prevents problems with transpilers that don't strip types the
same way. There's no functional change and it's a safe compile-time hygiene fix.
</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-chart-controls/src/operators/utils/isDerivedSeries.ts
**Line:** 20:20
**Comment:**
*Possible Bug: Importing type-only symbols (`JsonObject`,
`QueryFormData`) as runtime values can cause bundlers or runtimes to attempt to
import non-existent value exports and break at runtime; change them to
type-only imports so only `ensureIsArray` is imported as a runtime value.
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]
