korbit-ai[bot] commented on code in PR #34793:
URL: https://github.com/apache/superset/pull/34793#discussion_r2291052917
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -333,8 +333,9 @@ export const useExploreAdditionalActionsMenu = (
icon: <Icons.FileOutlined />,
disabled: !canDownloadCSV,
onClick: () => {
+ const sliceSelector = `#chart-id-${slice?.slice_id}`;
Review Comment:
### Missing slice_id validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The optional chaining operator (?.) could result in an undefined slice_id,
leading to an invalid selector '#chart-id-undefined'.
###### Why this matters
If slice_id is undefined, the pivot table export would fail because the
selector wouldn't match any element in the DOM.
###### Suggested change ∙ *Feature Preview*
Add a validation check before attempting to export:
```javascript
if (!slice?.slice_id) {
addDangerToast(t('Chart ID is missing. Export cannot be completed.'));
return;
}
const sliceSelector = `#chart-id-${slice.slice_id}`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa894e65-da99-4fd8-ac61-9c8d21b1bdd7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa894e65-da99-4fd8-ac61-9c8d21b1bdd7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa894e65-da99-4fd8-ac61-9c8d21b1bdd7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa894e65-da99-4fd8-ac61-9c8d21b1bdd7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa894e65-da99-4fd8-ac61-9c8d21b1bdd7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:799906e7-3ca6-4386-a043-e84d9fc5e211 -->
[](799906e7-3ca6-4386-a043-e84d9fc5e211)
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -271,7 +271,11 @@ const SliceHeaderControls = (
break;
}
case MenuKeys.ExportPivotXlsx: {
- props.exportPivotExcel?.('.pvtTable', props.slice.slice_name);
+ const sliceSelector = `#chart-id-${props.slice.slice_id}`;
+ props.exportPivotExcel?.(
+ `${sliceSelector} .pvtTable`,
+ props.slice.slice_name,
+ );
Review Comment:
### Unmemoized DOM Selector String <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The DOM selector for the pivot table is being constructed and used during
each export operation. This means the selector string is recreated each time
even though it could be memoized since it only depends on the slice ID which is
unlikely to change.
###### Why this matters
Repeatedly creating the same string during each export operation adds
unnecessary overhead, especially if exports are done frequently.
###### Suggested change ∙ *Feature Preview*
Use useMemo to cache the selector string:
```typescript
const sliceSelector = useMemo(
() => `#chart-id-${props.slice.slice_id} .pvtTable`,
[props.slice.slice_id]
);
// Then in the export handler:
props.exportPivotExcel?.(sliceSelector, props.slice.slice_name);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a174797-1110-4b0c-8df5-7a5071446b6b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a174797-1110-4b0c-8df5-7a5071446b6b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a174797-1110-4b0c-8df5-7a5071446b6b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a174797-1110-4b0c-8df5-7a5071446b6b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a174797-1110-4b0c-8df5-7a5071446b6b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9351d931-cc1d-4855-a15c-7dabb949d6f0 -->
[](9351d931-cc1d-4855-a15c-7dabb949d6f0)
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -271,7 +271,11 @@
break;
}
case MenuKeys.ExportPivotXlsx: {
- props.exportPivotExcel?.('.pvtTable', props.slice.slice_name);
+ const sliceSelector = `#chart-id-${props.slice.slice_id}`;
Review Comment:
### Incorrect Chart Selector for Pivot Export <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The chart selector uses 'chart-id' but the chart container class in the code
uses 'dashboard-chart-id', which will cause the pivot table export to fail.
###### Why this matters
The mismatch between the selector and actual DOM structure means the
exportPivotExcel function won't find the pivot table element, resulting in a
failed export operation.
###### Suggested change ∙ *Feature Preview*
Update the selector to match the dashboard chart container class structure:
```typescript
const sliceSelector = `.dashboard-chart-id-${props.slice.slice_id}`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34e716f5-0f2b-4c55-8d91-a6c1729504d1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34e716f5-0f2b-4c55-8d91-a6c1729504d1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34e716f5-0f2b-4c55-8d91-a6c1729504d1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34e716f5-0f2b-4c55-8d91-a6c1729504d1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/34e716f5-0f2b-4c55-8d91-a6c1729504d1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c6d6bffd-f739-42da-ab86-7d6c7bae1376 -->
[](c6d6bffd-f739-42da-ab86-7d6c7bae1376)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
