Copilot commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2578259744
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -331,6 +453,64 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
totalCount: count,
};
};
+ loadTabs = async (dashboardId: number) => {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ const tabTree = result.tab_tree || [];
+
+ const convertToTreeData = (nodes: TabNode[]): TreeDataNode[] =>
+ nodes.map(node => ({
+ value: node.value,
+ title: node.title,
+ key: node.value,
+ children:
+ node.children && node.children.length > 0
+ ? convertToTreeData(node.children)
+ : undefined,
+ }));
+
+ const treeData = convertToTreeData(tabTree);
+ this.setState({ tabsData: treeData });
+ return treeData;
+ } catch (error) {
+ logging.error('Error loading tabs:', error);
+ this.setState({ tabsData: [] });
+ return [];
+ }
+ };
+
+ onTabChange = (value: string) => {
+ if (value) {
+ const findTabInTree = (data: TabNode[]): TabNode | null => {
+ for (const item of data) {
+ if (item.value === value) {
+ return item;
+ }
+ if (item.children) {
+ const found = findTabInTree(item.children);
+ if (found) return found;
+ }
+ }
+ return null;
+ };
+
+ const selectedTab = findTabInTree(this.state.tabsData);
Review Comment:
Type mismatch in `findTabInTree`: the function is defined to accept
`TabNode[]` but is being called with `this.state.tabsData` which is typed as
`Array<{ value: string; title: string; key: string }>` (and should actually be
`TreeDataNode[]`). The parameter type should match the actual state type:
`(data: TreeDataNode[])` or change state type to use `TabNode[]`.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
+ meta: {
+ width: 4,
+ height: 50,
+ chartId: chartId,
+ sliceName: `Chart ${chartId}`,
+ },
+ };
+
+ updatedPositionJson[rowKey] = {
+ type: 'ROW',
+ id: rowKey,
+ children: [chartKey],
+ parents: ['ROOT_ID', 'GRID_ID', tabId],
+ meta: {
+ background: 'BACKGROUND_TRANSPARENT',
+ },
+ };
+
+ if (updatedPositionJson[tabId]) {
+ if (!updatedPositionJson[tabId].children) {
+ updatedPositionJson[tabId].children = [];
+ }
+ updatedPositionJson[tabId].children.push(rowKey);
+ } else {
+ throw new Error(`Tab ${tabId} not found in positionJson`);
+ }
+
+ const response = await SupersetClient.put({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ position_json: JSON.stringify(updatedPositionJson),
+ }),
+ });
+
+ return response;
+ } catch (error) {
+ throw new Error('Error adding chart to dashboard tab:', error);
+ throw error;
Review Comment:
There are two consecutive throw statements where the second one is
unreachable. The first `throw new Error(...)` will prevent execution from
reaching the second `throw error`. Remove line 396 as it will never be executed.
```suggestion
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
Review Comment:
Missing documentation for the `addChartToDashboardTab` method. This complex
method modifies dashboard layout structure and should include JSDoc comments
explaining:
- The purpose of the method
- Parameters and their meanings
- The return value
- The layout structure it creates (parents hierarchy, row/chart relationship)
- Potential errors and when they might occur
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -331,6 +453,64 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
totalCount: count,
};
};
+ loadTabs = async (dashboardId: number) => {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ const tabTree = result.tab_tree || [];
+
+ const convertToTreeData = (nodes: TabNode[]): TreeDataNode[] =>
+ nodes.map(node => ({
+ value: node.value,
+ title: node.title,
+ key: node.value,
+ children:
+ node.children && node.children.length > 0
+ ? convertToTreeData(node.children)
+ : undefined,
+ }));
+
+ const treeData = convertToTreeData(tabTree);
+ this.setState({ tabsData: treeData });
+ return treeData;
+ } catch (error) {
+ logging.error('Error loading tabs:', error);
+ this.setState({ tabsData: [] });
+ return [];
+ }
+ };
Review Comment:
Missing test coverage for the `loadTabs` method. This method handles API
calls and data transformation. Tests should verify:
- Successful loading and conversion of tab data
- Error handling when API call fails
- Proper conversion from TabNode to TreeDataNode structure
- State updates with correct data
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
+ meta: {
+ width: 4,
+ height: 50,
+ chartId: chartId,
+ sliceName: `Chart ${chartId}`,
+ },
+ };
+
+ updatedPositionJson[rowKey] = {
+ type: 'ROW',
+ id: rowKey,
+ children: [chartKey],
+ parents: ['ROOT_ID', 'GRID_ID', tabId],
+ meta: {
+ background: 'BACKGROUND_TRANSPARENT',
+ },
+ };
+
+ if (updatedPositionJson[tabId]) {
+ if (!updatedPositionJson[tabId].children) {
+ updatedPositionJson[tabId].children = [];
+ }
+ updatedPositionJson[tabId].children.push(rowKey);
+ } else {
+ throw new Error(`Tab ${tabId} not found in positionJson`);
+ }
+
+ const response = await SupersetClient.put({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ position_json: JSON.stringify(updatedPositionJson),
+ }),
+ });
+
+ return response;
+ } catch (error) {
+ throw new Error('Error adding chart to dashboard tab:', error);
+ throw error;
+ }
+ };
Review Comment:
Missing test coverage for the `addChartToDashboardTab` method. This is a
critical new method that modifies dashboard layout and makes API calls. Tests
should verify:
- Correct construction of position_json structure
- Proper handling of parent relationships in the layout hierarchy
- Error handling when the tab doesn't exist
- API call with correct parameters
##########
superset-frontend/src/explore/types.ts:
##########
@@ -123,3 +123,16 @@ export interface ExplorePageState {
};
sliceEntities?: JsonObject; // propagated from Dashboard view
}
+
+export interface TabNode {
+ value: string;
+ title: string;
+ children?: TabNode[];
+}
+
+export interface TreeDataNode {
+ value: string;
+ title: string;
+ key: string;
+ children?: TreeDataNode[];
Review Comment:
[nitpick] Inconsistent naming: The type is named `TreeDataNode` but
`TreeSelect` component from Ant Design typically uses `treeData` prop with
nodes that have standard properties. Consider renaming to `TabTreeNode` to
better reflect that this represents tab data in tree structure, making it more
domain-specific and clearer.
```suggestion
export interface TabTreeNode {
value: string;
title: string;
key: string;
children?: TabTreeNode[];
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -331,6 +453,64 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
totalCount: count,
};
};
+ loadTabs = async (dashboardId: number) => {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}/tabs`,
+ });
+
+ const { result } = response.json;
+ const tabTree = result.tab_tree || [];
Review Comment:
Missing error handling: If the API endpoint
`/api/v1/dashboard/${dashboardId}/tabs` doesn't exist or returns an unexpected
response structure, the code assumes `result.tab_tree` will be available. While
there's a catch block, it would be safer to validate the response structure
before accessing `result.tab_tree`:
```typescript
const { result } = response.json;
if (!result || !Array.isArray(result.tab_tree)) {
logging.warn('Invalid tabs response format');
this.setState({ tabsData: [] });
return [];
}
const tabTree = result.tab_tree;
```
```suggestion
if (!result || !Array.isArray(result.tab_tree)) {
logging.warn('Invalid tabs response format');
this.setState({ tabsData: [] });
return [];
}
const tabTree = result.tab_tree;
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
Review Comment:
[nitpick] Unnecessary deep clone: `JSON.parse(JSON.stringify(positionJson))`
creates a deep copy even though you're directly mutating the cloned object
afterward. Consider directly working with the `positionJson` object or use
structured clone if available. This improves performance and readability.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
+ meta: {
+ width: 4,
+ height: 50,
Review Comment:
[nitpick] The hardcoded chart dimensions (width: 4, height: 50) may not be
appropriate for all chart types or dashboard layouts. Consider making these
configurable or deriving them from the dashboard's grid settings or chart
metadata.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -262,6 +282,20 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
}
: null,
);
+ if (dashboard && selectedTabId) {
+ try {
+ await this.addChartToDashboardTab(
+ dashboard.id,
+ value.id,
+ selectedTabId,
+ );
+ } catch (error) {
+ logging.error('Error adding chart to dashboard tab:', error);
+ this.props.addDangerToast(
+ t('Chart was saved but could not be added to the selected tab.'),
+ );
+ }
+ }
Review Comment:
Missing test coverage for integration between save action and tab placement.
Tests should verify:
- Chart is added to selected tab when saving to a dashboard
- URL includes tab hash when navigating to dashboard
- Error toast is shown when tab placement fails but save succeeds
- selectedTabId is correctly extracted and passed to addChartToDashboardTab
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -152,10 +158,20 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ newSliceName: event.target.value });
}
- onDashboardChange(dashboard: { label: string; value: string | number }) {
- this.setState({ dashboard });
- }
+ onDashboardChange = async (dashboard: {
Review Comment:
[nitpick] Inconsistent method binding: The class uses arrow functions for
`onDashboardChange` (line 161) and `onTabChange` (line 486), but uses explicit
binding in the constructor for older methods (lines 104-107). For consistency,
either use arrow functions for all event handlers or bind them all in the
constructor. Arrow functions are the modern React pattern and eliminate the
need for explicit binding.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
+ meta: {
+ width: 4,
+ height: 50,
+ chartId: chartId,
+ sliceName: `Chart ${chartId}`,
+ },
+ };
+
+ updatedPositionJson[rowKey] = {
+ type: 'ROW',
+ id: rowKey,
+ children: [chartKey],
+ parents: ['ROOT_ID', 'GRID_ID', tabId],
+ meta: {
+ background: 'BACKGROUND_TRANSPARENT',
+ },
+ };
+
+ if (updatedPositionJson[tabId]) {
+ if (!updatedPositionJson[tabId].children) {
+ updatedPositionJson[tabId].children = [];
+ }
+ updatedPositionJson[tabId].children.push(rowKey);
+ } else {
+ throw new Error(`Tab ${tabId} not found in positionJson`);
+ }
+
+ const response = await SupersetClient.put({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ position_json: JSON.stringify(updatedPositionJson),
+ }),
+ });
+
+ return response;
+ } catch (error) {
+ throw new Error('Error adding chart to dashboard tab:', error);
+ throw error;
Review Comment:
The error message construction is incorrect. The `Error` constructor expects
a string message as the first parameter, but you're passing the error object as
a second parameter which will be ignored. This should be:
```typescript
throw new Error(`Error adding chart to dashboard tab: ${error}`);
```
or simply re-throw the original error:
```typescript
throw error;
```
```suggestion
throw new Error(`Error adding chart to dashboard tab: ${error}`);
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -152,10 +158,20 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ newSliceName: event.target.value });
}
- onDashboardChange(dashboard: { label: string; value: string | number }) {
- this.setState({ dashboard });
- }
+ onDashboardChange = async (dashboard: {
+ label: string;
+ value: string | number;
+ }) => {
+ this.setState({
+ dashboard,
+ tabsData: [],
+ selectedTab: undefined,
+ });
+ if (typeof dashboard.value === 'number') {
+ await this.loadTabs(dashboard.value);
+ }
+ };
Review Comment:
Potential race condition: If a user quickly changes the dashboard selection
multiple times, multiple `loadTabs` calls could be in flight simultaneously.
The last completed call will set the state, but it may not correspond to the
currently selected dashboard. Consider using an AbortController or tracking the
latest request to ignore stale responses:
```typescript
private currentLoadTabsRequest = 0;
onDashboardChange = async (dashboard) => {
const requestId = ++this.currentLoadTabsRequest;
// ... existing code ...
if (typeof dashboard.value === 'number') {
const tabs = await this.loadTabs(dashboard.value);
if (requestId === this.currentLoadTabsRequest) {
this.setState({ tabsData: tabs });
}
}
};
```
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -345,3 +357,62 @@ test('removes form_data_key from URL parameters after
save', () => {
expect(result.get('slice_id')).toEqual('1');
expect(result.get('save_action')).toEqual('overwrite');
});
+
+test('disables tab selector when no dashboard selected', () => {
+ const { getByRole, getByTestId } = setup();
+ fireEvent.click(getByRole('radio', { name: 'Save as...' }));
+ const tabSelector = getByTestId('mock-tree-select');
+ expect(tabSelector).toBeInTheDocument();
+ expect(tabSelector).toBeDisabled();
+});
+
+test('renders tab selector when saving as ', async () => {
+ const { getByRole, getByTestId } = setup();
+ fireEvent.click(getByRole('radio', { name: 'Save as...' }));
+ const selection = getByTestId('mock-async-select');
+ fireEvent.change(selection, { target: { value: '1' } });
+ const tabSelector = getByTestId('mock-tree-select');
+ expect(tabSelector).toBeInTheDocument();
+ expect(tabSelector).toBeDisabled();
+});
+
+test('onDashboardChange triggers tabs load for existing dashboard', async ()
=> {
+ const dashboardId = mockEvent.value;
+
+ fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}/tabs`, {
+ json: {
+ result: {
+ tab_tree: [{ value: 'tab1', title: 'Main Tab' }],
+ },
+ },
+ });
+ const component = new PureSaveModal(defaultProps);
+ const loadTabsMock = jest
+ .fn()
+ .mockResolvedValue([{ value: 'tab1', title: 'Main Tab' }]);
+ component.loadTabs = loadTabsMock;
+ await component.onDashboardChange({
+ value: dashboardId,
+ label: 'Test Dashboard',
+ });
+ expect(loadTabsMock).toHaveBeenCalledWith(dashboardId);
+});
+
+test('onTabChange updates selectedTab state', () => {
+ const component = new PureSaveModal(defaultProps);
+
+ const setStateMock = jest.fn();
+ component.setState = setStateMock;
+
+ component.state.tabsData = [
+ { value: 'tab1', title: 'Main Tab', key: 'tab1' },
+ { value: 'tab2', title: 'Analytics Tab', key: 'tab2' },
+ ];
Review Comment:
Use `setState` instead of directly modifying component state.
```suggestion
component.setState({
tabsData: [
{ value: 'tab1', title: 'Main Tab', key: 'tab1' },
{ value: 'tab2', title: 'Analytics Tab', key: 'tab2' },
],
});
```
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -345,3 +357,62 @@ test('removes form_data_key from URL parameters after
save', () => {
expect(result.get('slice_id')).toEqual('1');
expect(result.get('save_action')).toEqual('overwrite');
});
+
+test('disables tab selector when no dashboard selected', () => {
+ const { getByRole, getByTestId } = setup();
+ fireEvent.click(getByRole('radio', { name: 'Save as...' }));
+ const tabSelector = getByTestId('mock-tree-select');
+ expect(tabSelector).toBeInTheDocument();
+ expect(tabSelector).toBeDisabled();
+});
+
+test('renders tab selector when saving as ', async () => {
Review Comment:
Typo in test name: "saving as " has a trailing space. Should be "saving as"
or "saving as new chart".
```suggestion
test('renders tab selector when saving as', async () => {
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -231,6 +248,9 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
? sliceDashboards
: [...sliceDashboards, dashboard.id];
formData.dashboards = sliceDashboards;
+ if (this.state.action === 'saveas') {
+ selectedTabId = this.state.selectedTab?.value as string;
Review Comment:
The condition on line 251 only adds the chart to the selected tab when
`action === 'saveas'`, but line 586 shows that the tab selector is only
displayed for 'saveas' action. This means charts saved with the 'overwrite'
action will be added to the dashboard but not to any specific tab. Consider if
this is the intended behavior, or if the condition should check
`this.state.selectedTab` instead.
```suggestion
if (this.state.selectedTab) {
selectedTabId = this.state.selectedTab.value as string;
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -72,6 +75,8 @@ type SaveModalState = {
isLoading: boolean;
saveStatus?: string | null;
dashboard?: { label: string; value: string | number };
+ selectedTab?: { label: string; value: string | number };
+ tabsData: Array<{ value: string; title: string; key: string }>;
Review Comment:
Type inconsistency: `tabsData` state is typed as `Array<{ value: string;
title: string; key: string }>` (line 79), but it's actually populated with
`TreeDataNode[]` which has a `children` property (lines 465-474). The state
type should be `TreeDataNode[]` to match the actual data structure.
```suggestion
tabsData: TreeDataNode[];
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
Review Comment:
The parents array is inconsistent with the actual hierarchy. If this chart
is being added to a specific tab, the parents should be `['ROOT_ID', tabId]`,
not `['ROOT_ID', 'GRID_ID', rowKey]`. The current structure suggests GRID_ID is
a parent, which may not align with the tab-based layout structure.
```suggestion
parents: ['ROOT_ID', tabId],
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +326,91 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({ isLoading: false });
}
}
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ ) => {
+ try {
+ const dashboardResponse = await SupersetClient.get({
+ endpoint: `/api/v1/dashboard/${dashboardId}`,
+ });
+
+ const dashboard = dashboardResponse.json.result;
+
+ let positionJson = dashboard.position_json;
+ if (typeof positionJson === 'string') {
+ positionJson = JSON.parse(positionJson);
+ }
+ positionJson = positionJson || {};
+
+ const updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
+
+ const chartKey = `CHART-${chartId}`;
+ const rowIndex = this.findNextRowPosition(updatedPositionJson);
+ const rowKey = `ROW-${rowIndex}`;
+
+ updatedPositionJson[chartKey] = {
+ type: 'CHART',
+ id: chartKey,
+ children: [],
+ parents: ['ROOT_ID', 'GRID_ID', rowKey],
+ meta: {
+ width: 4,
+ height: 50,
+ chartId: chartId,
+ sliceName: `Chart ${chartId}`,
Review Comment:
The `meta` object is missing the `uuid` property which is required according
to the `LayoutItemMeta` type definition (line 217 in dashboard/types.ts shows
uuid is required). This will cause type inconsistencies and potential runtime
errors. Add a uuid property:
```typescript
meta: {
width: 4,
height: 50,
chartId: chartId,
sliceName: `Chart ${chartId}`,
uuid: `${chartKey}-${Date.now()}`, // or use a proper UUID generator
},
```
```suggestion
sliceName: `Chart ${chartId}`,
uuid: `${chartKey}-${Date.now()}`,
```
--
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]
