codeant-ai-for-open-source[bot] commented on code in PR #37176:
URL: https://github.com/apache/superset/pull/37176#discussion_r2695382414
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -96,8 +100,16 @@ const selectChartCustomizationConfiguration =
createSelector(
state.dashboardInfo.metadata?.chart_customization_config || EMPTY_ARRAY,
selectDashboardChartIds,
Review Comment:
**Suggestion:** The selector accesses `state.dashboardInfo.metadata` without
checking whether `state.dashboardInfo` exists; if `dashboardInfo` is undefined
this will throw at runtime. Add optional chaining on `dashboardInfo` so the
selector safely falls back to `EMPTY_ARRAY` when `dashboardInfo` is not
present. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard render crashes on missing dashboardInfo.
- ❌ Chart customizations sidebar fails to mount.
- ⚠️ Unit tests mocking partial store may fail.
```
</details>
```suggestion
state.dashboardInfo?.metadata?.chart_customization_config ||
EMPTY_ARRAY,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard UI component that uses chart customizations which calls
useChartCustomizationConfiguration (see useChartCustomizationConfiguration at
superset-frontend/src/dashboard/components/nativeFilters/state.ts:127).
2. During initial render the selector selectChartCustomizationConfiguration
is executed
(defined at
superset-frontend/src/dashboard/components/nativeFilters/state.ts:100-101).
3. If Redux state has no dashboardInfo key (state.dashboardInfo ===
undefined) the
expression at
superset-frontend/src/dashboard/components/nativeFilters/state.ts:101
attempts to read `.metadata` on undefined and throws a TypeError.
4. Reproduce in tests by mocking react-redux useSelector to return a store
object without
dashboardInfo and rendering any component that calls
useChartCustomizationConfiguration;
the component render will throw at the selector line shown above.
```
</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/nativeFilters/state.ts
**Line:** 101:101
**Comment:**
*Null Pointer: The selector accesses `state.dashboardInfo.metadata`
without checking whether `state.dashboardInfo` exists; if `dashboardInfo` is
undefined this will throw at runtime. Add optional chaining on `dashboardInfo`
so the selector safely falls back to `EMPTY_ARRAY` when `dashboardInfo` is not
present.
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/dataMask/reducer.ts:
##########
@@ -216,9 +220,17 @@ const dataMaskReducer = produce(
loadedDataMask,
);
- const chartCustomizationConfig =
+ const rawChartCustomizationConfig =
metadata?.chart_customization_config || [];
+ const hasLegacyFormat = rawChartCustomizationConfig.some(item =>
+ isLegacyChartCustomizationFormat(item),
+ );
+
+ const chartCustomizationConfig = hasLegacyFormat
+ ? migrateChartCustomizationArray(rawChartCustomizationConfig)
+ : (rawChartCustomizationConfig as ChartCustomization[]);
Review Comment:
**Suggestion:** Logic bug / unsafe migration: when any legacy item exists
the code calls `migrateChartCustomizationArray` with the entire
`rawChartCustomizationConfig` (including already-modern items), potentially
re-migrating or corrupting items that are already in the new format; instead,
migrate only legacy items and preserve existing new items (while keeping
original order). [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard hydration can corrupt chart customization entries.
- ❌ Sidebar chart customization shows incorrect items.
- ⚠️ Affects dashboards with mixed-format customization arrays.
- ⚠️ Migration may alter saved dashboard configuration.
```
</details>
```suggestion
// Migrate only legacy items and preserve non-legacy items in
original order.
const legacyItems = rawChartCustomizationConfig.filter(item =>
isLegacyChartCustomizationFormat(item),
);
const migratedLegacyItems = legacyItems.length
? migrateChartCustomizationArray(legacyItems)
: [];
// Reconstruct final array preserving order: replace legacy entries
with migrated ones.
let migratedIndex = 0;
const chartCustomizationConfig =
rawChartCustomizationConfig.map(item =>
isLegacyChartCustomizationFormat(item)
? (migratedLegacyItems[migratedIndex++] as ChartCustomization)
: (item as ChartCustomization),
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard page that triggers dashboard hydration. The
HYDRATE_DASHBOARD action
is handled by the reducer at superset-frontend/src/dataMask/reducer.ts in the
HYDRATE_DASHBOARD case (lines ~220-320). The migration code runs at lines
223-232 in that
file.
2. Ensure the dashboard metadata returned from the backend includes
chart_customization_config with a mix of legacy-format and
already-modern-format items
(the reducer reads metadata?.chart_customization_config at line 223).
3. When the HYDRATE_DASHBOARD action is dispatched, the reducer evaluates
hasLegacyFormat
using isLegacyChartCustomizationFormat (imported from
src/dashboard/util/migrateChartCustomization) at lines 226-228.
4. Because at least one legacy item exists, the current code calls
migrateChartCustomizationArray with the entire rawChartCustomizationConfig
array (line
231). If migrateChartCustomizationArray expects legacy-only inputs or
transforms items
generically, this will re-process modern items and can change/corrupt them.
Observe
unexpected/mutated chart customization entries after hydration in the
dashboard UI
(sidebar customizations missing or altered), originating from reducer lines
223-232.
Note: This reproduces concretely by creating a dashboard metadata payload
containing
mixed-format chart_customization_config and loading that dashboard so the
HYDRATE_DASHBOARD reducer branch executes (see reducer.ts lines 220-236).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dataMask/reducer.ts
**Line:** 226:232
**Comment:**
*Possible Bug: Logic bug / unsafe migration: when any legacy item
exists the code calls `migrateChartCustomizationArray` with the entire
`rawChartCustomizationConfig` (including already-modern items), potentially
re-migrating or corrupting items that are already in the new format; instead,
migrate only legacy items and preserve existing new items (while keeping
original order).
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]
