Copilot commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2590629854
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -277,11 +315,14 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
// Go to new dashboard url
if (gotodash && dashboard) {
+ let url = dashboard.url;
Review Comment:
Tab redirection only works when action is 'saveas', but it should also work
for 'overwrite' action when a user overwrites a chart and wants to go to the
dashboard. Currently, when overwriting a chart and going to the dashboard, the
selectedTab will not navigate to the correct tab even if one was selected.
```suggestion
let url = dashboard.url;
// Always append tab hash if a tab is selected, for both 'saveas'
and 'overwrite'
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -134,6 +142,7 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
this.setState({
dashboard: { label: result.dashboard_title, value: result.id },
});
+ await this.loadTabs(dashboardId);
Review Comment:
Tab selector should only be visible when action is 'saveas' according to
line 622, but it's also loading tabs for the 'overwrite' action at line 145.
This creates unnecessary API calls when tabs won't be used. Consider only
loading tabs when `this.state.action === 'saveas'`.
```suggestion
if (this.state.action === 'saveas') {
await this.loadTabs(dashboardId);
}
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -292,6 +333,115 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
}
}
+ /* Adds a new chart to a new row on the specified dashboard tab.
+ * @param {number} dashboardId - ID of the dashboard.
+ * @param {number} chartId - ID of the chart to add.
+ * @param {string} tabId - ID of the dashboard tab where the chart is added.
+ * @param {string | undefined} sliceName - Chart name
Review Comment:
The comment says "Adds a new chart to a new row" but the function doesn't
always create a new row - it reuses existing rows with available space. Update
the comment to reflect the actual behavior:
```typescript
/* Adds a chart to the specified dashboard tab. If an existing row has
space, the chart is added there; otherwise, a new row is created.
* @param {number} dashboardId - ID of the dashboard.
* @param {number} chartId - ID of the chart to add.
* @param {string} tabId - ID of the dashboard tab where the chart is added.
* @param {string | undefined} sliceName - Chart name
*/
```
```suggestion
/* Adds a chart to the specified dashboard tab. If an existing row has
space, the chart is added there; otherwise, a new row is created.
* @param {number} dashboardId - ID of the dashboard.
* @param {number} chartId - ID of the chart to add.
* @param {string} tabId - ID of the dashboard tab where the chart is
added.
* @param {string | undefined} sliceName - Chart name
```
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -429,3 +441,169 @@ test('dispatches removeChartState when saving and going
to dashboard', async ()
// Clean up
removeChartStateSpy.mockRestore();
});
+
+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' },
+ { value: 'tab2', title: '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 correctly updates selectedTab via forceUpdate', () => {
+ const component = new PureSaveModal(defaultProps);
+
+ component.state = {
+ ...component.state,
+ tabsData: [
+ {
+ value: 'tab1',
+ title: 'Main Tab',
+ key: 'tab1',
+ children: [
+ {
+ value: 'tab2',
+ title: 'Analytics Tab',
+ key: 'tab2',
+ },
+ ],
+ },
+ ],
+ };
+
+ component.setState = function (stateUpdate) {
+ if (typeof stateUpdate === 'function') {
+ this.state = { ...this.state, ...stateUpdate(this.state) };
+ } else {
+ this.state = { ...this.state, ...stateUpdate };
+ }
+ }.bind(component);
+
+ component.onTabChange('tab2');
+
+ expect(component.state.selectedTab).toEqual({
+ value: 'tab2',
+ label: 'Analytics Tab',
+ });
+});
+
+test('chart placement logic finds row with available space', () => {
+ const GRID_COLUMN_COUNT = 12; // From the actual implementation
+ const chartWidth = 4; // From the actual implementation
+
+ // Test case 1: Row has space (8 + 4 = 12 <= 12)
+ const positionJson1 = {
+ tab1: {
+ type: 'TABS',
+ id: 'tab1',
+ children: ['row1'],
+ },
+ row1: {
+ type: 'ROW',
+ id: 'row1',
+ children: ['CHART-1'],
+ meta: {},
+ },
+ 'CHART-1': {
+ type: 'CHART',
+ id: 'CHART-1',
+ meta: { width: 8 },
+ },
+ };
+
+ // Test case 2: Row is full (12 + 4 = 16 > 12)
+ const positionJson2 = {
+ ...positionJson1,
+ 'CHART-1': {
+ ...positionJson1['CHART-1'],
+ meta: { width: 12 },
+ },
+ };
+
+ // Test case 3: Multiple charts in row
+ const positionJson3 = {
+ tab1: {
+ type: 'TABS',
+ id: 'tab1',
+ children: ['row1'],
+ },
+ row1: {
+ type: 'ROW',
+ id: 'row1',
+ children: ['CHART-1', 'CHART-2'],
+ meta: {},
+ },
+ 'CHART-1': {
+ type: 'CHART',
+ id: 'CHART-1',
+ meta: { width: 6 },
+ },
+ 'CHART-2': {
+ type: 'CHART',
+ id: 'CHART-2',
+ meta: { width: 4 },
+ },
+ };
+
+ const findRowWithSpace = (positionJson, tabChildren) => {
+ for (const childKey of tabChildren) {
+ const child = positionJson[childKey];
+ if (child?.type === 'ROW') {
+ const rowChildren = child.children || [];
+ const totalWidth = rowChildren.reduce((sum, key) => {
+ const component = positionJson[key];
+ return sum + (component?.meta?.width || 0);
+ }, 0);
+
+ if (totalWidth + chartWidth <= GRID_COLUMN_COUNT) {
+ return childKey;
+ }
+ }
+ }
+ return null;
+ };
+
+ // Test case 1: Should find row with space
+ expect(findRowWithSpace(positionJson1, ['row1'])).toBe('row1');
+
+ // Test case 2: Should not find row (full)
+ expect(findRowWithSpace(positionJson2, ['row1'])).toBeNull();
+
+ // Test case 3: Should find row (6 + 4 + 4 = 14 > 12, so no space)
Review Comment:
The comment is misleading. It says "6 + 4 + 4 = 14" but the test data only
has two charts (width 6 and 4). The calculation should be: existing charts 6 +
4 = 10, plus new chart width 4 = 14 total, which exceeds GRID_COLUMN_COUNT
(12). Update the comment to:
```javascript
// Test case 3: Should not find row (6 + 4 = 10, adding 4 = 14 > 12)
```
```suggestion
// Test case 3: Should not find row (6 + 4 = 10, adding 4 = 14 > 12)
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -292,6 +333,115 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
}
}
+ /* Adds a new chart to a new row on the specified dashboard tab.
+ * @param {number} dashboardId - ID of the dashboard.
+ * @param {number} chartId - ID of the chart to add.
+ * @param {string} tabId - ID of the dashboard tab where the chart is added.
+ * @param {string | undefined} sliceName - Chart name
+ * */
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ sliceName: string | undefined,
+ ) => {
+ 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 chartKey = `CHART-${chartId}`;
+ const chartWidth = 4;
+
+ // 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 + chartWidth <= 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],
+ meta: {
+ width: chartWidth,
+ height: 50,
+ chartId,
+ sliceName: sliceName ?? `Chart ${chartId}`,
+ },
+ };
+
+ // Add chart to the target row
+ updatedPositionJson[targetRowKey] = {
+ ...updatedPositionJson[targetRowKey],
+ children: [
+ ...(updatedPositionJson[targetRowKey].children || []),
+ chartKey,
+ ],
+ };
+
+ 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);
+ }
+ };
Review Comment:
Missing test coverage for the new `addChartToDashboardTab` method. While
there's a test for the placement logic, there's no integration test that
verifies the full method execution, including API calls, error handling, and
the dashboard update. Consider adding a test that mocks the API responses and
verifies the method works end-to-end.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -292,6 +333,115 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
}
}
+ /* Adds a new chart to a new row on the specified dashboard tab.
+ * @param {number} dashboardId - ID of the dashboard.
+ * @param {number} chartId - ID of the chart to add.
+ * @param {string} tabId - ID of the dashboard tab where the chart is added.
+ * @param {string | undefined} sliceName - Chart name
+ * */
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ sliceName: string | undefined,
+ ) => {
+ 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 chartKey = `CHART-${chartId}`;
+ const chartWidth = 4;
+
+ // 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 + chartWidth <= 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],
+ meta: {
+ width: chartWidth,
+ height: 50,
+ chartId,
+ sliceName: sliceName ?? `Chart ${chartId}`,
+ },
+ };
+
+ // Add chart to the target row
+ updatedPositionJson[targetRowKey] = {
+ ...updatedPositionJson[targetRowKey],
+ children: [
+ ...(updatedPositionJson[targetRowKey].children || []),
+ chartKey,
+ ],
+ };
+
+ 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);
Review Comment:
Incorrect Error constructor syntax. The Error constructor takes a single
message string as its first argument, not a message followed by an error
object. This should be:
```typescript
throw new Error(`Error adding chart to dashboard tab: ${error}`);
```
or wrap the original error:
```typescript
throw error;
```
```suggestion
throw new Error(`Error adding chart to dashboard tab: ${error
instanceof Error ? error.message : String(error)}`);
```
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -292,6 +333,115 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
}
}
+ /* Adds a new chart to a new row on the specified dashboard tab.
+ * @param {number} dashboardId - ID of the dashboard.
+ * @param {number} chartId - ID of the chart to add.
+ * @param {string} tabId - ID of the dashboard tab where the chart is added.
+ * @param {string | undefined} sliceName - Chart name
+ * */
+ addChartToDashboardTab = async (
+ dashboardId: number,
+ chartId: number,
+ tabId: string,
+ sliceName: string | undefined,
+ ) => {
+ 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 chartKey = `CHART-${chartId}`;
+ const chartWidth = 4;
+
+ // 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 + chartWidth <= 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],
+ meta: {
+ width: chartWidth,
+ height: 50,
Review Comment:
Magic numbers 4 (chart width) and 50 (chart height) should be extracted as
named constants for better maintainability. Consider defining these at the top
of the file or importing from a constants file:
```typescript
const DEFAULT_CHART_WIDTH = 4;
const DEFAULT_CHART_HEIGHT = 50;
```
--
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]
