codeant-ai-for-open-source[bot] commented on code in PR #36314:
URL: https://github.com/apache/superset/pull/36314#discussion_r2629129478
##########
superset-frontend/src/dashboard/types.ts:
##########
@@ -129,6 +129,8 @@ export type DashboardState = {
data: JsonObject;
};
chartStates?: Record<string, any>;
+ lastRefreshTime?: number;
+ tabActivationTimes?: Record<string, number>;
Review Comment:
**Suggestion:** Incorrect `Record` value typing: `tabActivationTimes?:
Record<string, number>` tells TypeScript that any index access returns a
`number`, but at runtime missing keys will yield `undefined`. This mismatch can
cause unsafe assumptions and runtime errors; change the value type to allow
`undefined` so indexed reads are typed correctly. [type error]
**Severity Level:** Minor ⚠️
```suggestion
// Mapping of tab id to activation timestamp in milliseconds; values may
be undefined for tabs not yet activated.
tabActivationTimes?: Record<string, number | undefined>;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion is reasonable: `tabActivationTimes` is optional and even when
present, indexing an arbitrary key can yield `undefined`. Typing the values as
`number | undefined` (or using a `Record<string, number> | undefined` with
careful index usage) more accurately models runtime behavior and prevents
unsafe assumptions. This change improves type-safety.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/types.ts
**Line:** 133:133
**Comment:**
*Type Error: Incorrect `Record` value typing: `tabActivationTimes?:
Record<string, number>` tells TypeScript that any index access returns a
`number`, but at runtime missing keys will yield `undefined`. This mismatch can
cause unsafe assumptions and runtime errors; change the value type to allow
`undefined` so indexed reads are typed correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx:
##########
@@ -28,8 +28,9 @@ import DashboardComponent from
'src/dashboard/containers/DashboardComponent';
import { EditableTitle } from '@superset-ui/core/components';
import { setEditMode, onRefresh } from 'src/dashboard/actions/dashboardState';
-import Tab from './Tab';
+import Tab, { RENDER_TAB, RENDER_TAB_CONTENT } from './Tab';
import Markdown from '../Markdown';
+import { LayoutItem } from 'src/dashboard/types';
Review Comment:
**Suggestion:** Importing a type as a runtime value: `import { LayoutItem }
from 'src/dashboard/types';` performs a real module import at runtime which can
fail in tests if the module only exports TypeScript types; mark it as a
type-only import to avoid emitting a runtime import. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
import type { LayoutItem } from 'src/dashboard/types';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid and useful suggestion. In TSX tests we should prefer
type-only imports to avoid emitting runtime imports for pure types which can
fail in some test bundlers or cause unnecessary module resolution. Changing to
`import type { LayoutItem }` is safe and doesn't affect runtime behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx
**Line:** 33:33
**Comment:**
*Possible Bug: Importing a type as a runtime value: `import {
LayoutItem } from 'src/dashboard/types';` performs a real module import at
runtime which can fail in tests if the module only exports TypeScript types;
mark it as a type-only import to avoid emitting a runtime import.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx:
##########
@@ -440,6 +450,7 @@ const Tab = props => {
props.index,
props.depth,
props.editMode,
+ props.onDropOnTab,
Review Comment:
**Suggestion:** The useCallback dependency array for `renderTab` incorrectly
includes `props.onDropOnTab` and omits the actual props that `renderTab`
destructures and passes to `DragDroppable` (`onDropPositionChange` and
`onDragTab`). This can cause the callback to hold stale references when
`onDropPositionChange` or `onDragTab` change (and unnecessarily re-run when
`onDropOnTab` changes). Replace the wrong dependency with the correct props so
React can properly update the callback. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
props.onDropPositionChange,
props.onDragTab,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct. renderTab destructures and forwards onDropPositionChange and
onDragTab to DragDroppable but the dependency list mistakenly references
props.onDropOnTab (which renderTab doesn't use) and omits
props.onDropPositionChange and props.onDragTab. That can produce stale closures
and subtle bugs; updating the array to include the actual props used is the
right, minimal fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.tsx
**Line:** 453:453
**Comment:**
*Logic Error: The useCallback dependency array for `renderTab`
incorrectly includes `props.onDropOnTab` and omits the actual props that
`renderTab` destructures and passes to `DragDroppable` (`onDropPositionChange`
and `onDragTab`). This can cause the callback to hold stale references when
`onDropPositionChange` or `onDragTab` change (and unnecessarily re-run when
`onDropOnTab` changes). Replace the wrong dependency with the correct props so
React can properly update the callback.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx:
##########
@@ -80,7 +81,9 @@ const createProps = () => ({
parentId: 'TABS-L-d9eyOE-b',
depth: 2,
index: 1,
- renderType: 'RENDER_TAB_CONTENT',
+ renderType: RENDER_TAB_CONTENT as
+ | typeof RENDER_TAB
+ | typeof RENDER_TAB_CONTENT,
Review Comment:
**Suggestion:** Unnecessary and brittle type assertion on the `renderType`
property; the multi-line `as typeof RENDER_TAB | typeof RENDER_TAB_CONTENT`
cast is redundant and can mask mismatches — assign the constant directly to
provide the correct runtime value and let TypeScript infer the type. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
renderType: RENDER_TAB_CONTENT,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The cast is unnecessary — the runtime value is already `RENDER_TAB_CONTENT`.
Removing the `as ...` makes the code simpler and avoids masking mismatches. It
doesn't change behavior and is the cleaner option in tests.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.test.tsx
**Line:** 84:86
**Comment:**
*Type Error: Unnecessary and brittle type assertion on the `renderType`
property; the multi-line `as typeof RENDER_TAB | typeof RENDER_TAB_CONTENT`
cast is redundant and can mask mismatches — assign the constant directly to
provide the correct runtime value and let TypeScript infer the 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>
--
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]
