codeant-ai-for-open-source[bot] commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2771264355
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -630,25 +644,26 @@ const Chart = props => {
>
<SliceHeader
ref={headerRef}
- slice={slice}
+ slice={sliceForHeader}
isExpanded={isExpanded}
- isCached={isCached}
- cachedDttm={cachedDttm}
- queriedDttm={queriedDttm}
- updatedDttm={chartUpdateEndTime}
+ isCached={isCached as boolean[]}
+ cachedDttm={cachedDttm as string[]}
+ queriedDttm={queriedDttm as string | null | undefined}
+ updatedDttm={chartUpdateEndTime ?? null}
toggleExpandSlice={boundActionCreators.toggleExpandSlice}
forceRefresh={forceRefresh}
editMode={editMode}
annotationQuery={annotationQuery}
logExploreChart={logExploreChart}
logEvent={boundActionCreators.logEvent}
- onExploreChart={onExploreChart}
exportCSV={exportCSV}
exportPivotCSV={exportPivotCSV}
exportXLSX={exportXLSX}
exportFullCSV={exportFullCSV}
exportFullXLSX={exportFullXLSX}
- updateSliceName={props.updateSliceName}
+ updateSliceName={
Review Comment:
**Suggestion:** The `updateSliceName` function passed down to `SliceHeader`
has the wrong call signature: `SliceHeader` expects a callback that only takes
the new title string, but you're passing `props.updateSliceName` (which expects
`(sliceId: number, name: string)`), cast to the wrong type. At runtime
`EditableTitle` will invoke this with a single string argument, causing the
slice ID to be set to the title string and the name argument to be `undefined`,
so slice renaming will either fail or target the wrong slice. Wrap the prop so
it adapts from `(name: string)` to `(sliceId: number, name: string)` using
`props.id`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Slice rename mis-targets charts in dashboard UI.
- ❌ User-facing chart title editing broken in edit mode.
- ⚠️ Dashboard audit logs may record wrong slice IDs.
- ⚠️ Tests for slice rename may start failing.
```
</details>
```suggestion
updateSliceName={(newSliceName: string) =>
props.updateSliceName(props.id, newSliceName)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard UI and enable edit mode for a chart. The Chart
component passes its
updateSliceName prop into SliceHeader at Chart.tsx:664-665
(`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx`).
2. SliceHeader renders EditableTitle and wires the onSaveTitle prop to its
`updateSliceName` parameter (see SliceHeader at
`superset-frontend/src/dashboard/components/SliceHeader/index.tsx` lines
253-262,
specifically line 261 where `onSaveTitle={updateSliceName}`).
3. EditableTitle will call the onSaveTitle callback with a single argument:
the new title
string (invoked by the editable title component when user confirms a
rename). Because
Chart passed `props.updateSliceName` cast to `(arg0: string) => void`
(Chart.tsx:664-665),
that single string will be forwarded to the dashboard-level updateSliceName
function which
actually expects `(sliceId: number, name: string)`.
4. The dashboard-level handler will receive the title string as the first
argument
(sliceId), and undefined (or no second argument) for the name parameter.
This either
attempts to rename the wrong slice (treating the title string as an id) or
fails
silently/throws depending on the implementation of the handler. Confirm by
performing a
rename in the UI and observing incorrect slice renaming or no-op.
Explanation: The existing Chart code intentionally casts a two-arg updater
to a one-arg
signature. SliceHeader (index.tsx) exposes updateSliceName as `(arg0:
string) => void` and
EditableTitle calls it with a single string; Chart should adapt the call to
include the
slice id (props.id). The proposed wrapper fixes the mismatch by converting
`(name:
string)` to `(sliceId, name)` using the chart's id.
```
</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/Chart/Chart.tsx
**Line:** 664:664
**Comment:**
*Logic Error: The `updateSliceName` function passed down to
`SliceHeader` has the wrong call signature: `SliceHeader` expects a callback
that only takes the new title string, but you're passing
`props.updateSliceName` (which expects `(sliceId: number, name: string)`), cast
to the wrong type. At runtime `EditableTitle` will invoke this with a single
string argument, causing the slice ID to be set to the title string and the
name argument to be `undefined`, so slice renaming will either fail or target
the wrong slice. Wrap the prop so it adapts from `(name: string)` to `(sliceId:
number, name: string)` using `props.id`.
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/types.ts:
##########
@@ -280,6 +344,13 @@ export type Slice = {
created_by: { id: number };
};
+export interface SliceEntitiesState {
+ slices: Record<string, Slice>;
Review Comment:
**Suggestion:** The `SliceEntitiesState.slices` map is typed as
`Record<string, Slice>`, but existing consumers (e.g., the Deckgl layer
visibility plugin) treat slice IDs as numbers (`Record<number, Slice>`), which
creates a type-level inconsistency that can lead to incorrect assumptions about
key types and subtle bugs when indexing or merging slices by numeric ID.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ DeckGL layer visibility customization may misuse slice IDs.
- ❌ Drill detail modal indexing by chart id may be unsafe.
- ⚠️ Native filter scope utilities assume numeric keys.
- ⚠️ TypeScript cross-file type mismatches create silent bugs.
```
</details>
```suggestion
slices: Record<number, Slice>;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the DeckGL layer visibility customization which reads slice
entities. The file
superset-frontend/src/chartCustomizations/components/DeckglLayerVisibility/DeckglLayerVisibilityCustomizationPlugin.tsx
references state.sliceEntities?.slices at lines reported by Grep (lines ~38
and ~55 in
that file) and explicitly treats slices as Record<number, Slice> (see Grep
hit:
"...slices: Record<number, Slice>;").
2. The Dashboard root state type (superset-frontend/src/dashboard/types.ts)
currently
defines SliceEntitiesState.slices as Record<string, Slice> (lines 347-352 in
the PR),
creating a mismatch between the declared root type and several consumers.
3. A consumer that types its selector as expecting numeric keys (e.g.,
DrillDetailModal at
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx
which uses (state:
{ sliceEntities: { slices: Record<number, Slice> } }) => ... at the lines
found by Grep)
will compile against a different shape than the RootState's declaration,
permitting
incorrect indexing like sliceEntities.slices[chartId] where chartId is a
number. At
runtime JavaScript coerces numeric keys to strings, so this mismatch
produces silent type
unsafety — TypeScript may not catch misuse across files due to the
conflicting
declarations.
4. Reproduce locally: run TypeScript/IDE checks and navigate to a consumer
typed with
Record<number, Slice> (for example Deckgl plugin or DrillDetailModal).
Observe the
inconsistency in inferred types between the consumer and RootState (found at
superset-frontend/src/dashboard/types.ts:347-352). Attempt a code edit that
merges or
indexes slices with numeric IDs (e.g., sliceEntities.slices[42]) —
editors/tsc may not
surface the mismatch reliably because of the conflicting declarations,
leading to
potential runtime confusion when keys are stringified.
5. Conclusion / why this is concrete: Grep shows many callers treating
slices as
Record<number, Slice> (Deckgl plugin, DrillDetailModal, FilterScope utils,
Chart selector,
tests and reducers). The current RootState declaration (this file) is the
canonical type
and must match those callers to avoid cross-file type inconsistency and
silent runtime
assumptions.
```
</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:** 348:348
**Comment:**
*Possible Bug: The `SliceEntitiesState.slices` map is typed as
`Record<string, Slice>`, but existing consumers (e.g., the Deckgl layer
visibility plugin) treat slice IDs as numbers (`Record<number, Slice>`), which
creates a type-level inconsistency that can lead to incorrect assumptions about
key types and subtle bugs when indexing or merging slices by numeric ID.
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:
##########
@@ -116,23 +123,46 @@ const TitleDropIndicator = styled.div`
}
`;
-const renderDraggableContent = dropProps =>
- dropProps.dropIndicatorProps && <div {...dropProps.dropIndicatorProps} />;
-
-const Tab = props => {
+interface DropIndicatorChildProps {
+ dropIndicatorProps?: {
+ className: string;
+ } | null;
+}
+
+const renderDraggableContent = (
+ dropProps: DropIndicatorChildProps,
+): ReactElement | null =>
+ dropProps.dropIndicatorProps ? (
+ <div {...dropProps.dropIndicatorProps} />
+ ) : null;
+
+interface DragDropChildProps {
+ dropIndicatorProps?: {
+ className: string;
+ } | null;
+ dragSourceRef?: Ref<HTMLDivElement>;
+ draggingTabOnTab?: boolean;
+}
+
+const Tab = (props: TabProps): ReactElement => {
const dispatch = useDispatch();
- const canEdit = useSelector(state => state.dashboardInfo.dash_edit_perm);
- const dashboardLayout = useSelector(state => state.dashboardLayout.present);
+ const canEdit = useSelector(
+ (state: RootState) => state.dashboardInfo.dash_edit_perm,
+ );
+ const dashboardLayout = useSelector(
+ (state: RootState) => state.dashboardLayout.present,
+ );
const lastRefreshTime = useSelector(
- state => state.dashboardState.lastRefreshTime,
+ (state: RootState) => state.dashboardState.lastRefreshTime,
);
const tabActivationTime = useSelector(
- state => state.dashboardState.tabActivationTimes?.[props.id] || 0,
+ (state: RootState) =>
+ state.dashboardState.tabActivationTimes?.[props.id] || 0,
Review Comment:
**Suggestion:** The selector for `tabActivationTime` forces a default of `0`
and the subsequent `if` condition guards on `tabActivationTime`'s truthiness,
so for tabs that don't yet have an entry in `tabActivationTimes` (or where it
is legitimately `0`) the condition will never pass even when `lastRefreshTime`
is more recent, meaning the tab's charts will not be refreshed after a
dashboard refresh and may display stale data; using a numeric comparison
instead of a truthiness check avoids this logic bug. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard tab charts may remain stale.
- ⚠️ Tab activation UX inconsistent after refresh.
- ⚠️ Embedded dashboards showing outdated data.
```
</details>
##########
superset-frontend/src/dashboard/types.ts:
##########
@@ -45,6 +46,15 @@ export interface ExtendedNativeFilterScope extends
NativeFilterScope {
selectedLayers?: string[];
}
+/** Configuration item for native filters and chart customizations */
+export interface FilterConfigItem extends JsonObject {
+ id: string;
+ chartsInScope?: number[];
+ tabsInScope?: string[];
+ type?: string;
+ targets?: { datasetId?: number; [key: string]: unknown }[];
Review Comment:
**Suggestion:** The `FilterConfigItem.targets` field is typed as an array of
loosely defined objects with only an optional `datasetId`, but elsewhere filter
and chart customization configuration relies on the full `NativeFilterTarget`
shape; using this overly generic type can hide mistakes (like misspelled
fields) and break consumers that expect strongly typed targets, leading to
misconfigured filters or customizations at runtime. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Native filter configuration UI may misconfigure filters.
- ⚠️ Filter scope utilities may process invalid targets.
- ⚠️ Chart customization transformers risk silent errors.
- ⚠️ Suggestion tightens typing; prevents runtime misconfigs.
```
</details>
```suggestion
targets?: NativeFilterTarget[];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect native filter configuration consumers. Grep shows multiple native
filter
components under superset-frontend/src/dashboard/components/nativeFilters/*
which operate
on filter target shapes (e.g., FilterScope utils and transformers referenced
in Grep
results). Those consumers expect the structured NativeFilterTarget shape.
2. In the current types file (superset-frontend/src/dashboard/types.ts),
FilterConfigItem.targets is declared as an array of loose objects with only
an optional
datasetId (lines 49-56), which permits misspellings or missing required
fields that other
code relies on.
3. Reproduce: build or run type-checked code paths involving
FiltersConfigForm/FilterScope
components (files reported by Grep:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/*).
Create a filters configuration object with targets that omit fields present
on
NativeFilterTarget (for example missing 'datasetId' vs 'dataSourceId' or
misspelled keys).
With the loose typing, the compiler will not flag the issue, but at runtime
the native
filter code will look for specific fields and may behave incorrectly
(mis-scoped filters,
no target charts).
4. Because NativeFilterTarget is already exported/used elsewhere (Grep
returned files that
reference NativeFilterTarget), replacing the loose object with
NativeFilterTarget[] aligns
the shape across consumers and exposes misuses at compile time, preventing
runtime
misconfiguration.
5. Conclusion: this step traces actual consumers (FiltersConfigForm,
FilterScope utils,
transformer files shown in Grep) which rely on a concrete target shape —
tightening the
type to NativeFilterTarget produces earlier and concrete detection of
incorrect target
shapes.
```
</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:** 55:55
**Comment:**
*Type Error: The `FilterConfigItem.targets` field is typed as an array
of loosely defined objects with only an optional `datasetId`, but elsewhere
filter and chart customization configuration relies on the full
`NativeFilterTarget` shape; using this overly generic type can hide mistakes
(like misspelled fields) and break consumers that expect strongly typed
targets, leading to misconfigured filters or customizations at runtime.
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]
