You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SBIN2010 (via GitHub)" <gi...@apache.org> on 2025/11/28 19:46:13 UTC
[PR] feat: add tab select with save chart to dashboard [superset]
SBIN2010 opened a new pull request, #36332:
URL: https://github.com/apache/superset/pull/36332
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Users can now select a specific tab when saving a chart to a dashboard. This allows for more precise control over chart placement in complex dashboards with multiple tabs.
Add the ability to select a tab in the chart save modal window when placing it on a dashboard:
- Load and display available tabs when an existing dashboard is selected
- Allow users to choose a specific tab for the new chart
- Automatically position the chart on the selected tab
Added `TreeSelect` from Ant Design for tab selection
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
<img width="638" height="543" alt="Снимок экрана от 2025-11-28 22-43-07" src="https://github.com/user-attachments/assets/3628f439-d8c2-4f0a-b26e-214882e5f079" />
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
run test src/explore/components/SaveModal.test.jsx
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3710606381
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20718041373) for [127cdaf](https://github.com/apache/superset/commit/127cdaf)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3618563458
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2572394493
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -289,6 +305,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 {
+ console.error(`Tab ${tabId} not found in positionJson`);
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Silent Failure in Tab Addition</b></div>
<div id="fix">
When adding a chart to a dashboard tab, if the tab ID is not found in position_json, the chart is not added but no error is thrown, leading to silent failure. The user saves the chart but it doesn't appear in the expected tab. Throwing an error here ensures the catch block in saveOrOverwrite shows a toast notification.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
} else {
throw new Error(`Tab ${tabId} not found in positionJson`);
}
````
</div>
</details>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/8a7e2c9/AGENTS.md#L25">AGENTS.md:25</a>
</li>
</ul>
</details>
<small><i>Code Review Run #393482</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3669493653
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20333338988) for [959563a](https://github.com/apache/superset/commit/959563a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3669552350
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20333338988) for [959563a](https://github.com/apache/superset/commit/959563a)
• **Environment:** http://54.187.220.167:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #36332:
URL: https://github.com/apache/superset/pull/36332
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3606045729
> * In this case, the chart is already saved inside the Sales Dashboard. When I try to save it, the “Add to tabs” field is still disabled. To enable it, I have to select another dashboard and then come back to the Sales Dashboard.
I just noticed this myself. And I've already fixed it.🤝
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3684117094
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20443652329) for [127cdaf](https://github.com/apache/superset/commit/127cdaf)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2661698019
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -236,6 +257,12 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
? sliceDashboards
: [...sliceDashboards, dashboard.id];
formData.dashboards = sliceDashboards;
+ if (
+ this.state.action === 'saveas' &&
Review Comment:
How about centralizing them in a typed constant object or enum?
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -409,6 +657,27 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
}
/>
</FormItem>
+ {this.state.action === 'saveas' && (
Review Comment:
here as well
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3760098914
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21068724480) for [b026c1e](https://github.com/apache/superset/commit/b026c1e)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3760182325
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21068724480) for [b026c1e](https://github.com/apache/superset/commit/b026c1e)
• **Environment:** http://54.218.123.95:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3604113384
CodeAnt AI is reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3598474967
I LOVE THIS FEATURE!
Lots of copilot suggestions... I committed a couple of the "no-brainer" ones on your behalf, but I'm curious what you think of the rest.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2591828730
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -520,3 +520,90 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
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
Review Comment:
I also think we can put those constants inside `superset-frontend/src/dashboard/constants.ts`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3606029654
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3682907893
> > Hey @SBIN2010 how about [this](https://github.com/apache/superset/pull/36332#issuecomment-3606437789)?
> > should we add `"Out of tab"` property as a default insted of the first tab?
>
> Hey @SBIN2010, what do you think about this point?
Hi @EnxDev! I added this a long time ago, you probably missed my comment.
https://github.com/apache/superset/pull/36332#issuecomment-3618566640
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3667212817
> * It seems the the /tabs endpoint is still missing?
- Yes, that's right, there's no such endpoint at the moment.
> * Can you add a test that actually exercises addChartToDashboardTab rather than a conceptual test?
- I tried to create this, but it hasn’t worked out yet.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3615494216
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3674138039
⚠️ **DEPRECATED WORKFLOW** ⚠️
@EnxDev This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "🎪 trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/20364722594). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3629035013
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20042665260) for [959563a](https://github.com/apache/superset/commit/959563a)
• **Environment:** http://52.26.101.174:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3684061257
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3682580348
> Hey @SBIN2010 how about [this](https://github.com/apache/superset/pull/36332#issuecomment-3606437789)?
>
> should we add `"Out of tab"` property as a default insted of the first tab?
Hey @SBIN2010, what do you think about this point?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3590225548
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3606155727
> > * In this case, the chart is already saved inside the Sales Dashboard. When I try to save it, the “Add to tabs” field is still disabled. To enable it, I have to select another dashboard and then come back to the Sales Dashboard.
>
> I just noticed this myself. And I've already fixed it.🤝
Okay, I'm currently testing on your branch with the latest changes pushed, and I’m seeing the same behavior I described earlier. You probably still have some changes to push if you’ve already fixed it.
I'm also wondering whether we should place the new chart in the same row as the last one if there’s available space, instead of creating a new row; @SBIN2010 what do you think about that?
<img width="1683" height="1243" alt="Screenshot 2025-12-03 112259" src="https://github.com/user-attachments/assets/eafbcda7-8b60-4e20-b1ba-baf6da766ae5" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2592076970
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -520,3 +520,90 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
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
Review Comment:
👍I'll move the constants to superset-frontend/src/dashboard/constants.ts
And import them from there for testing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3684254263
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3725937278
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3614486004
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3611009421
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3597256730
@SBIN2010 Ephemeral environment spinning up at http://34.221.129.14:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3605658547
@EnxDev Ephemeral environment spinning up at http://54.213.160.155:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3618566640
> Hey @SBIN2010 how about [this](https://github.com/apache/superset/pull/36332#issuecomment-3606437789)?
>
> should we add `"Out of tab"` property as a default insted of the first tab?
I added the 'Out of tab' option, but only for those cases where tabs are not pages and all tabs are placed on the first page.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3684185915
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20443652329) for [127cdaf](https://github.com/apache/superset/commit/127cdaf)
• **Environment:** http://44.247.48.42:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3667120069
Some things Claude Code says I should ask you @SBIN2010 - just being the human-in-the-middle here ;)
- It seems the the /tabs endpoint is still missing?
- Can you add a test that actually exercises addChartToDashboardTab rather than a conceptual test?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3593216039
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19803981967) for [a339bd9](https://github.com/apache/superset/commit/a339bd9)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3604122551
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36332/files#diff-a1256efcca3d8954c815c26f94d13eb86abd3f52c67258071fed1b3ece4a5b00R161-R174'><strong>Possible Bug</strong></a><br>`onDashboardChange` assumes the dashboard argument is always a non-null object, but `AsyncSelect` is configured with `allowClear`, so it may pass `null`/`undefined` when the selection is cleared. The new implementation immediately dereferences `dashboard.value`, which can now throw at runtime when clearing the field. This path needs a null/undefined guard.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36332/files#diff-a1256efcca3d8954c815c26f94d13eb86abd3f52c67258071fed1b3ece4a5b00R329-R397'><strong>Possible Bug</strong></a><br>In `addChartToDashboardTab`, the `catch` block calls `new Error('Error adding chart to dashboard tab:', error)`. The `Error` constructor only accepts a single message argument, so the second `error` argument is invalid in TypeScript and will be ignored at runtime. This both causes a type error and drops the original stack/context; the function should either rethrow the original error or wrap it properly.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36332/files#diff-9e5762ab474e55120025ca4afd6643fa47dbf2ac56a09f38a0053cf4dc86abc1R47-R50'><strong>Possible Bug</strong></a><br>The mocked `TreeSelect` uses a `data-test` attribute but the tests query using `getByTestId('mock-tree-select')`, which expects `data-testid`. This mismatch will cause all tab selector tests to fail and should be aligned.<br>
</td></tr>
</table>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3606437789
@SBIN2010 this is the current behavior after my changes
Probably we should set the first tab as a default value inside "Add to tabs" field
https://github.com/user-attachments/assets/40d9b7d4-d295-4608-90f4-39b4daeb37ea
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3710676400
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20718041373) for [127cdaf](https://github.com/apache/superset/commit/127cdaf)
• **Environment:** http://35.85.224.165:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2674148589
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -95,9 +102,13 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
this.state = {
newSliceName: props.sliceName,
datasetName: props.datasource?.name,
- action: this.canOverwriteSlice() ? 'overwrite' : 'saveas',
+ action: this.canOverwriteSlice()
+ ? ChartStatusType.overwrite
+ : ChartStatusType.saveas,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent enum vs string usage</b></div>
<div id="fix">
The refactoring introduces ChartStatusType enum but inconsistently mixes it with string literals for action values. While functionally equivalent since enum values are strings, this creates maintenance risk if enum values change. For example, comparisons like `this.state.action === 'overwrite'` should use `ChartStatusType.overwrite` for type safety.
</div>
</div>
<small><i>Code Review Run #bcacb9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3725947255
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | b026c1ec8608d644513636df360e5fa9351a6953 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/696022dcd7673e0008fc0a8d |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-36332--superset-docs-preview.netlify.app](https://deploy-preview-36332--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-36332--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3628989700
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20042665260) for [959563a](https://github.com/apache/superset/commit/959563a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3597209057
⚠️ **DEPRECATED WORKFLOW** ⚠️
@SBIN2010 This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "🎪 trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/19828051867). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3604122672
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2584457209
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -290,6 +330,99 @@ 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 updatedPositionJson = JSON.parse(JSON.stringify(positionJson));
Review Comment:
Do we need this deep clone?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3613103036
CodeAnt AI is running Incremental review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2590741287
##########
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:
When rewriting, there is no choice of tabs - and the redirection occurs only to the toolbar, without taking into account the tabs
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3674174803
@EnxDev Ephemeral environment spinning up at http://34.216.76.155:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2591812561
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -520,3 +520,90 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
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
Review Comment:
Can we import those directly from the SaveModal component?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #36332:
URL: https://github.com/apache/superset/pull/36332#discussion_r2591812561
##########
superset-frontend/src/explore/components/SaveModal.test.jsx:
##########
@@ -520,3 +520,90 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => {
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
Review Comment:
Can we import it directly from the SaveModal component?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3617366468
Hey @SBIN2010 how about [this](https://github.com/apache/superset/pull/36332#issuecomment-3606437789)?
should we add `"Out of tab"` property as a default insted of the first tab?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3604433539
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19877266507) for [8642a16](https://github.com/apache/superset/commit/8642a16)
• **Environment:** http://44.250.241.142:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: add tab select with save chart to dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36332:
URL: https://github.com/apache/superset/pull/36332#issuecomment-3726129470
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org