codeant-ai-for-open-source[bot] commented on code in PR #36721:
URL: https://github.com/apache/superset/pull/36721#discussion_r2629415854
##########
superset-frontend/src/dashboard/util/logging/findNonTabChildChartIds.ts:
##########
@@ -49,9 +58,12 @@ function findNonTabChildChartIds({ id, layout }) {
}
// This method is called frequently, so cache results
-let cachedLayout;
-let cachedIdsLookup = {};
-export default function findNonTabChildChartIdsWithCache({ id, layout }) {
+let cachedLayout: Layout | undefined;
+let cachedIdsLookup: Record<string, number[]> = {};
+export default function findNonTabChildChartIdsWithCache({
+ id,
+ layout,
+}: FindNonTabChildChartIdsParams): number[] {
if (cachedLayout === layout && cachedIdsLookup[id]) {
Review Comment:
**Suggestion:** The cache check only returns cached values when the cached
array is truthy, so calls that legitimately produce an empty list of chart IDs
will never hit the cache and will recompute on every invocation, defeating the
intended caching behavior for those cases and causing unnecessary work. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
if (cachedLayout === layout && id in cachedIdsLookup) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current cache hit check uses the truthiness of cachedIdsLookup[id], so
an existing cached empty array ([]) is treated as a miss. That causes repeated
recomputation for legitimate cases where no chart IDs exist. Changing the check
to test for the presence of the key (e.g., `id in cachedIdsLookup`) correctly
recognizes cached empty results and preserves the intended caching 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/util/logging/findNonTabChildChartIds.ts
**Line:** 67:67
**Comment:**
*Logic Error: The cache check only returns cached values when the
cached array is truthy, so calls that legitimately produce an empty list of
chart IDs will never hit the cache and will recompute on every invocation,
defeating the intended caching behavior for those cases and causing unnecessary
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/src/explore/reducers/saveModalReducer.ts:
##########
@@ -20,8 +20,26 @@
import * as actions from '../actions/saveModalActions';
import { HYDRATE_EXPLORE } from '../actions/hydrateExplore';
-export default function saveModalReducer(state = {}, action) {
- const actionHandlers = {
+interface SaveModalState {
+ isVisible?: boolean;
+ dashboards?: unknown[];
+ saveModalAlert?: string;
+ data?: unknown;
+}
+
+interface SaveModalAction {
+ type: string;
+ isVisible?: boolean;
+ choices?: unknown[];
+ userId?: string;
+ data?: { saveModal: SaveModalState };
Review Comment:
**Suggestion:** The `SaveModalAction` interface incorrectly constrains the
`data` field to always be an object with a `saveModal` property, but in reality
this field is reused for different payload shapes (e.g., `SAVE_SLICE_SUCCESS`
passes the chart save response while `HYDRATE_EXPLORE` passes an explore
bootstrap payload). This mismatch makes the type contract wrong and can cause
incorrect assumptions or require unsafe casts wherever these actions are used
together. Loosening the type of `data` to reflect that it can carry different
payloads avoids misleading consumers while keeping current runtime behavior
unchanged. [type error]
**Severity Level:** Minor ⚠️
```suggestion
// This field is reused by multiple actions with different payload shapes
data?: any;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The reducer reads action.data in at least two different ways:
SAVE_SLICE_SUCCESS expects the full chart-save response on action.data, while
HYDRATE_EXPLORE expects a bootstrap payload where action.data.saveModal exists.
The current type (data?: { saveModal: SaveModalState }) is too narrow and
misrepresents runtime shapes, forcing unsafe casts or confusing consumers.
Loosening the type (or better: introducing a discriminated union of action
types) fixes a real typing mismatch rather than being a mere cosmetic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/reducers/saveModalReducer.ts
**Line:** 35:35
**Comment:**
*Type Error: The `SaveModalAction` interface incorrectly constrains the
`data` field to always be an object with a `saveModal` property, but in reality
this field is reused for different payload shapes (e.g., `SAVE_SLICE_SUCCESS`
passes the chart save response while `HYDRATE_EXPLORE` passes an explore
bootstrap payload). This mismatch makes the type contract wrong and can cause
incorrect assumptions or require unsafe casts wherever these actions are used
together. Loosening the type of `data` to reflect that it can carry different
payloads avoids misleading consumers while keeping current runtime behavior
unchanged.
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/dnd/handleHover.ts:
##########
@@ -17,13 +17,27 @@
* under the License.
*/
import { throttle } from 'lodash';
+import { DropTargetMonitor } from 'react-dnd';
import { DASHBOARD_ROOT_TYPE } from 'src/dashboard/util/componentTypes';
import getDropPosition from 'src/dashboard/util/getDropPosition';
import handleScroll from './handleScroll';
const HOVER_THROTTLE_MS = 100;
-function handleHover(props, monitor, Component) {
+interface DragDropComponent {
+ mounted: boolean;
+ props: {
+ component?: { type: string };
+ onHover?: () => void;
+ };
+ setState: (fn: () => { dropIndicator: unknown }) => void;
+}
+
+function handleHover(
+ props: unknown,
+ monitor: DropTargetMonitor,
+ Component: DragDropComponent,
Review Comment:
**Suggestion:** The `handleHover` function defines its own
`DragDropComponent` interface and types its `props` parameter as `unknown`
instead of reusing the existing `DragDroppableComponent`/`DragDroppableProps`
types from `dragDroppableConfig.ts`, which weakens type safety and risks this
function silently drifting out of sync with the actual drag/drop component
contract over time. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
import type {
DragDroppableComponent,
DragDroppableProps,
} from './dragDroppableConfig';
function handleHover(
props: DragDroppableProps,
monitor: DropTargetMonitor,
Component: DragDroppableComponent,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The project already exports strongly-typed interfaces (DragDroppableProps
and DragDroppableComponent) in dragDroppableConfig.ts that precisely describe
the drag/drop contract used across the DnD code. The current file declares a
local, weaker interface and types props as `unknown`, which reduces type-safety
and risks drifting from the canonical contract. Reusing the shared types is a
safe, correct improvement that prevents future mismatch and improves
IDE/type-checker feedback. This is not fixing a runtime bug but it is a
meaningful correctness/maintenance improvement tied directly to the diff.
</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/dnd/handleHover.ts
**Line:** 21:39
**Comment:**
*Possible Bug: The `handleHover` function defines its own
`DragDropComponent` interface and types its `props` parameter as `unknown`
instead of reusing the existing `DragDroppableComponent`/`DragDroppableProps`
types from `dragDroppableConfig.ts`, which weakens type safety and risks this
function silently drifting out of sync with the actual drag/drop component
contract over time.
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]
