codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481938961
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -71,7 +74,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] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', tabId, targetRowKey],
Review Comment:
**Suggestion:** The inserted chart also uses a hardcoded ancestry
(`ROOT_ID`, `GRID_ID`, `tabId`, `targetRowKey`) instead of deriving parents
from the actual target row. On dashboards where tab ancestry is not rooted at
`GRID_ID`, this creates inconsistent parent chains between row and chart nodes
and can break rendering/filter-scope traversal. Compose chart parents from the
resolved row's real `parents` plus the row id. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Newly saved charts get inconsistent parent chains.
- ⚠️ Tabbed dashboard rendering/filter scopes may misbehave.
- ⚠️ Layout-based features break for affected chart nodes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger the Explore save flow as wired in `ExploreChartHeader`
(`superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:21-28`),
which
dispatches `setSaveChartModalVisibility(true)` to show `SaveModal`
(connected via
`mapStateToProps` in
`superset-frontend/src/explore/components/SaveModal.tsx:917-931`).
2. In `SaveModal` (`renderSaveChartModal` at `SaveModal.tsx:721-841`),
choose a dashboard
and a specific tab via the "Add to dashboard" `AsyncSelect` and "Add to tabs"
`TreeSelect`, which populate `dashboard` and `selectedTab` state
(`SaveModal.tsx:247-253`,
`SaveModal.tsx:414-431`, and `SaveModal.tsx:673-700`), ensuring
`selectedTab.value !==
'OUT_OF_TAB'` so a tab ID will be used.
3. Click "Save as…" and then "Save & go to dashboard" or "Save" to execute
`saveOrOverwrite` (`SaveModal.tsx:463-632`); in the save-as path with a
selected tab,
after creating the slice it computes `selectedTabId` and calls
`addChartToDashboardTab(dashboardResult.id, value.id, selectedTabId,
newSliceName)`
(`SaveModal.tsx:562-569`), which delegates to `addChartToDashboard`
(`SaveModal.tsx:448-461`).
4. Within `addChartToDashboard` (`SaveModal.tsx:97-188`), the function
builds or reuses a
`targetRowKey`; it then assigns the new chart node a parent chain `parents:
['ROOT_ID',
'GRID_ID', tabId, targetRowKey]` (line 166) instead of deriving it from the
actual row's
`parents` plus `targetRowKey`. In canonical tabbed layouts (see the
`position_json`
example in
`superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:10-16`),
chart parents mirror the row ancestry plus the row ID, e.g. `["ROOT_ID",
"TABS-wUKya7eQ0Z", "TAB-BCIJF4NvgQ", "ROW-zvw7luvEL"]`; here, the chart's
`parents` chain
includes `GRID_ID` even when the row's correct ancestry goes through a
`TABS-*` container,
so the chart's lineage diverges from its row container, producing an
inconsistent layout
graph that downstream dashboard logic expects to be aligned for rendering
and filter-scope
traversal.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9cd18623b6d43718878aedc5fa09d9c&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=d9cd18623b6d43718878aedc5fa09d9c&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:** 166:166
**Comment:**
*Logic Error: The inserted chart also uses a hardcoded ancestry
(`ROOT_ID`, `GRID_ID`, `tabId`, `targetRowKey`) instead of deriving parents
from the actual target row. On dashboards where tab ancestry is not rooted at
`GRID_ID`, this creates inconsistent parent chains between row and chart nodes
and can break rendering/filter-scope traversal. Compose chart parents from the
resolved row's real `parents` plus the row 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.
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=b07a39833042dd8333fd21fd7c9d02d4fec0bdd2f3cf7d892e0f0519add6c5a0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=b07a39833042dd8333fd21fd7c9d02d4fec0bdd2f3cf7d892e0f0519add6c5a0&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]
