codeant-ai-for-open-source[bot] commented on code in PR #37902:
URL: https://github.com/apache/superset/pull/37902#discussion_r2868387585
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -99,645 +201,578 @@ export const StyledModal = styled(Modal)`
}
`;
-class SaveModal extends Component<SaveModalProps, SaveModalState> {
- constructor(props: SaveModalProps) {
- super(props);
- this.state = {
- newSliceName: props.sliceName,
- datasetName: props.datasource?.name,
- action: this.canOverwriteSlice()
- ? ChartStatusType.overwrite
- : ChartStatusType.saveas,
- isLoading: false,
- dashboard: undefined,
- tabsData: [],
- selectedTab: undefined,
- };
- this.onDashboardChange = this.onDashboardChange.bind(this);
- this.onSliceNameChange = this.onSliceNameChange.bind(this);
- this.changeAction = this.changeAction.bind(this);
- this.saveOrOverwrite = this.saveOrOverwrite.bind(this);
- this.isNewDashboard = this.isNewDashboard.bind(this);
- this.onHide = this.onHide.bind(this);
- }
+const SaveModal = ({
+ addDangerToast,
+ actions,
+ form_data,
+ user,
+ alert: alertProp,
+ sliceName = '',
+ slice,
+ datasource,
+ dashboardId: dashboardIdProp,
+ isVisible,
+}: SaveModalProps) => {
+ const dispatch = useDispatch();
+ const history = useHistory();
+ const theme = useTheme();
+
+ const canOverwriteSlice = useCallback(
+ (): boolean =>
+ slice?.owners?.includes(user.userId) && !slice?.is_managed_externally,
+ [slice, user.userId],
+ );
- isNewDashboard(): boolean {
- const { dashboard } = this.state;
- return typeof dashboard?.value === 'string';
- }
+ const [newSliceName, setNewSliceName] = useState<string | undefined>(
+ sliceName,
+ );
+ const [datasetName, setDatasetName] = useState<string>(datasource?.name);
+ const [action, setAction] = useState<SaveActionType>(
+ canOverwriteSlice() ? ChartStatusType.overwrite : ChartStatusType.saveas,
+ );
+ const [isLoading, setIsLoading] = useState<boolean>(false);
+ const [dashboard, setDashboard] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+ const [tabsData, setTabsData] = useState<TabTreeNode[]>([]);
+ const [selectedTab, setSelectedTab] = useState<
+ { label: string; value: string | number } | undefined
+ >(undefined);
+
+ const isNewDashboard = useCallback(
+ (): boolean => typeof dashboard?.value === 'string',
+ [dashboard?.value],
+ );
- canOverwriteSlice(): boolean {
- return (
- this.props.slice?.owners?.includes(this.props.user.userId) &&
- !this.props.slice?.is_managed_externally
- );
- }
+ const loadDashboard = useCallback(async (id: number) => {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${id}`,
+ });
+ return response.json.result;
+ }, []);
- async componentDidMount() {
- let { dashboardId } = this.props;
- if (!dashboardId) {
- let lastDashboard = null;
- try {
- lastDashboard = sessionStorage.getItem(SK_DASHBOARD_ID);
- } catch (error) {
- // continue regardless of error
- }
- dashboardId = lastDashboard && parseInt(lastDashboard, 10);
- }
- if (dashboardId) {
+ const loadTabs = useCallback(
+ async (dashboardId: number) => {
try {
- const result = (await this.loadDashboard(dashboardId)) as Dashboard;
- if (canUserEditDashboard(result, this.props.user)) {
- this.setState({
- dashboard: { label: result.dashboard_title, value: result.id },
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ if (!result || !Array.isArray(result.tab_tree)) {
+ logging.warn('Invalid tabs response format');
+ setTabsData([]);
+ return [];
+ }
+ const tabTree = result.tab_tree;
+ const gridTabIds = new Set<string>();
+ const convertToTreeData = (nodes: TabNode[]): TabTreeNode[] =>
+ nodes.map(node => {
+ const isGridTab =
+ Array.isArray(node.parents) && node.parents.includes('GRID_ID');
+ if (isGridTab) {
+ gridTabIds.add(node.value);
+ }
+ return {
+ value: node.value,
+ title: node.title,
+ key: node.value,
+ children:
+ node.children && node.children.length > 0
+ ? convertToTreeData(node.children)
+ : undefined,
+ };
});
- await this.loadTabs(dashboardId);
+
+ const treeData = convertToTreeData(tabTree);
+
+ // Add "Out of tab" option at the beginning
+ if (gridTabIds.size > 0) {
+ const tabsDataWithOutOfTab = [
+ {
+ value: 'OUT_OF_TAB',
+ title: 'Out of tab',
+ key: 'OUT_OF_TAB',
+ children: undefined,
+ },
+ ...treeData,
+ ];
+
+ setTabsData(tabsDataWithOutOfTab);
+ setSelectedTab({ value: 'OUT_OF_TAB', label: 'Out of tab' });
+ } else {
+ const firstTab = treeData[0];
+ setTabsData(treeData);
+ setSelectedTab({ value: firstTab.value, label: firstTab.title });
}
Review Comment:
**Suggestion:** When loading tabs, the code assumes that `treeData` always
contains at least one element and unconditionally accesses `treeData[0]`; if
the API returns an empty `tab_tree` array, this will cause a runtime error when
reading `firstTab.value` and `firstTab.title`, breaking the save modal for such
dashboards. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Save modal crashes for dashboards without any tab definitions.
- ⚠️ Users cannot save charts to dashboards lacking tabs.
```
</details>
```suggestion
} else if (treeData.length > 0) {
const firstTab = treeData[0];
setTabsData(treeData);
setSelectedTab({ value: firstTab.value, label: firstTab.title });
} else {
// No tabs returned; keep tabs data empty and do not preselect a
tab
setTabsData([]);
setSelectedTab(undefined);
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Explore UI, open any chart so that `ExploreViewContainer` renders,
which
includes `SaveModal` (see
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:90`
importing
`SaveModal` and usage around line 1055).
2. Click the "Save" button in the chart header; this dispatches
`setSaveChartModalVisibility(true)` from `ExploreChartHeader` (see
`superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:252`),
causing
Redux state `saveModal.isVisible` to become true and `SaveModal` (connected
via
`mapStateToProps` in `SaveModal.tsx:842-853`) to mount.
3. When `SaveModal` mounts, its `useEffect` at `SaveModal.tsx:319-347` runs
`initializeDashboard`, which calls `loadTabs(dashboardId)` (line 336) for
the selected or
last-used dashboard ID (stored under `SK_DASHBOARD_ID`).
4. If `/api/v1/dashboard/{dashboardId}/tabs` returns a valid payload with an
empty
`tab_tree` array (per backend schema `TabsPayloadSchema.tab_tree` in
`superset/dashboards/schemas.py:331-334` and computation in `Dashboard.tabs`
at
`superset/models/dashboard.py:300-345`), then in `loadTabs` the `gridTabIds`
set remains
empty and `treeData` is `[]`. Execution reaches the `else` branch at
`SaveModal.tsx:303-307`, evaluates `const firstTab = treeData[0];` where
`firstTab` is
`undefined`, and immediately tries to read `firstTab.value` and
`firstTab.title`, causing
a runtime TypeError (cannot read properties of undefined). This breaks the
Save modal for
dashboards whose `tab_tree` is empty.
```
</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/components/SaveModal.tsx
**Line:** 303:307
**Comment:**
*Logic Error: When loading tabs, the code assumes that `treeData`
always contains at least one element and unconditionally accesses
`treeData[0]`; if the API returns an empty `tab_tree` array, this will cause a
runtime error when reading `firstTab.value` and `firstTab.title`, breaking the
save modal for such dashboards.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=f606bbd533bdc49c78a0c5b2287ed04bea9220b2a0be1e4f58f1eb708827fa11&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=f606bbd533bdc49c78a0c5b2287ed04bea9220b2a0be1e4f58f1eb708827fa11&reaction=dislike'>👎</a>
--
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]
