codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3429939864
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -67,7 +70,120 @@ import { CHART_WIDTH, CHART_HEIGHT } from
'src/dashboard/constants';
// Session storage key for recent dashboard
const SK_DASHBOARD_ID = 'save_chart_recent_dashboard';
-interface SaveModalProps extends RouteComponentProps {
+/**
+ * Creates URLSearchParams with save action and slice ID, removing
form_data_key.
+ * Exported for testing purposes.
+ */
+export const createRedirectParams = (
+ windowLocationSearch: string,
+ chart: { id: number },
+ action: string,
+): URLSearchParams => {
+ const searchParams = new URLSearchParams(windowLocationSearch);
+ searchParams.set('save_action', action);
+ searchParams.delete('form_data_key');
+ searchParams.set('slice_id', chart.id.toString());
+ return searchParams;
+};
+
+/**
+ * Adds a chart to a dashboard tab by updating the position_json.
+ * Exported for testing purposes.
+ */
+export const addChartToDashboard = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ sliceNameParam: string | undefined,
+): Promise<void> => {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboardData = dashboardResponse.json.result;
+
+ let positionJson = dashboardData.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const chartKey = `CHART-${chartId}`;
+
+ // Find a row in the tab with available space
+ const tabChildren = positionJson[tabId]?.children || [];
+ let targetRowKey: string | null = null;
+
+ for (const childKey of tabChildren) {
+ const child = positionJson[childKey];
+ if (child?.type === 'ROW') {
+ const rowChildren = child.children || [];
+ const totalWidth = rowChildren.reduce((sum: number, key: string) => {
+ const component = positionJson[key];
+ return sum + (component?.meta?.width || 0);
+ }, 0);
+
+ if (totalWidth + CHART_WIDTH <= GRID_COLUMN_COUNT) {
+ targetRowKey = childKey;
+ break;
+ }
+ }
+ }
+
+ const updatedPositionJson = { ...positionJson };
+
+ // Create a new row if no existing row has space
+ if (!targetRowKey) {
+ targetRowKey = `ROW-${nanoid()}`;
+ updatedPositionJson[targetRowKey] = {
+ type: 'ROW',
+ id: targetRowKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', tabId],
+ meta: {
+ background: 'BACKGROUND_TRANSPARENT',
+ },
+ };
+
+ if (positionJson[tabId]) {
+ updatedPositionJson[tabId] = {
+ ...positionJson[tabId],
+ children: [...(positionJson[tabId].children || []), targetRowKey],
+ };
+ } else {
+ throw new Error(`Tab ${tabId} not found in positionJson`);
+ }
+ }
+
+ updatedPositionJson[chartKey] = {
Review Comment:
**Suggestion:** The dashboard layout parent chain is hardcoded to `ROOT_ID
-> GRID_ID -> tab`, but tabbed dashboards can have different parent paths (for
example `ROOT_ID -> TABS-* -> TAB-*`). This writes invalid `parents` metadata
for new rows/charts and can break placement/rendering in dashboards with nested
tab structures. Build `parents` from the selected tab/row's existing `parents`
arrays instead of hardcoding. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ New charts under tabs get inconsistent layout ancestry metadata.
- ⚠️ Tabbed dashboards may misbehave for newly-added saved charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Explore view, which renders `ExploreViewContainer` and
conditionally mounts
`SaveModal` when `props.isSaveModalVisible` is true
(`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:50-57`),
passing
`dashboardId` and `form_data` into `SaveModal`.
2. In `SaveModal`
(`superset-frontend/src/explore/components/SaveModal.tsx`), select "Save
as..." and choose an existing dashboard with tabbed layout plus a specific
tab in the "Add
to tabs" TreeSelect; this populates `dashboard` and `selectedTab` state and,
on save,
causes `saveOrOverwrite(false)` to be called (lines 24–193 in the
`saveOrOverwrite`
callback).
3. Inside `saveOrOverwrite`, after creating the new slice, the code calls
`addChartToDashboardTab(dashboardResult.id, value.id, selectedTabId,
newSliceName)`
(around lines 122–129 of the `saveOrOverwrite` body), which in turn invokes
`addChartToDashboard(dashboardId, chartId, tabId, sliceNameParam)` (lines
8–21 of the
`addChartToDashboardTab` callback).
4. `addChartToDashboard` fetches the dashboard layout and, when no existing
row under the
selected tab has space, creates a new row and chart entries with hardcoded
`parents`
arrays (`superset-frontend/src/explore/components/SaveModal.tsx:137-158`):
the row gets
`parents: ['ROOT_ID', 'GRID_ID', tabId]` and the chart gets `parents:
['ROOT_ID',
'GRID_ID', tabId, targetRowKey]`. However, real tabbed dashboards use parent
chains like
`["ROOT_ID", "TABS-wUKya7eQ0Z", "TAB-BCIJF4NvgQ", "ROW-zvw7luvEL"]` as seen
in
`position_json` fixtures in
`superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:134-15`,
so the new components have inconsistent/invalid ancestry, which can break
features that
rely on correct `parents` paths for tab navigation and layout logic.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=96729c09ab3945a38ee002c354cbdc36&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=96729c09ab3945a38ee002c354cbdc36&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 137:158
**Comment:**
*Logic Error: The dashboard layout parent chain is hardcoded to
`ROOT_ID -> GRID_ID -> tab`, but tabbed dashboards can have different parent
paths (for example `ROOT_ID -> TABS-* -> TAB-*`). This writes invalid `parents`
metadata for new rows/charts and can break placement/rendering in dashboards
with nested tab structures. Build `parents` from the selected tab/row's
existing `parents` arrays instead of hardcoding.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=d8547c9943519ea4ae9082621bcd6b1e7a0d0823d5338e71527c29fa49694c47&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=d8547c9943519ea4ae9082621bcd6b1e7a0d0823d5338e71527c29fa49694c47&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]
