-
Notifications
You must be signed in to change notification settings - Fork 16.3k
feat: add tab select with save chart to dashboard #36332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
19151d1
35aaeb6
db3afda
9eea2e6
3930c95
b355212
7200d6c
3049a3d
247aef8
b4678ec
0ae844d
49fc529
601dde2
66823a5
4f59629
a7ce4e9
1bf4fdd
1241867
045cf6b
acdba8a
4fb89c2
f47d28b
3bdf337
780a5d6
3c339df
798b7a6
2db564e
a58cfad
9ea0d2f
c6bbf53
5214e7a
d98105c
30b1576
4aaa27c
25c1987
c3854cb
e1c4909
c60bda8
964307e
df97cb7
6ef82fa
d0ae96c
725039c
727c59f
eeb04da
ca12644
45c1a1a
44708bf
e3336eb
8a7e2c9
b3d39d8
5de507e
f56c2c1
bc0c095
3a82246
a339bd9
9967088
2444e7e
ccdf86b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ import { | |||||||||||||||
| Input, | ||||||||||||||||
| Loading, | ||||||||||||||||
| Divider, | ||||||||||||||||
| TreeSelect, | ||||||||||||||||
| } from '@superset-ui/core/components'; | ||||||||||||||||
| import { | ||||||||||||||||
| DatasourceType, | ||||||||||||||||
|
|
@@ -42,11 +43,13 @@ import { | |||||||||||||||
| } from '@superset-ui/core'; | ||||||||||||||||
| import { css, styled, Alert } from '@apache-superset/core/ui'; | ||||||||||||||||
| import { Radio } from '@superset-ui/core/components/Radio'; | ||||||||||||||||
| import { Layout } from 'src/dashboard/types'; | ||||||||||||||||
| import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils'; | ||||||||||||||||
| import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; | ||||||||||||||||
| import { SaveActionType } from 'src/explore/types'; | ||||||||||||||||
| import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; | ||||||||||||||||
| import { Dashboard } from 'src/types/Dashboard'; | ||||||||||||||||
| import { TabNode, TreeDataNode } from '../types'; | ||||||||||||||||
|
|
||||||||||||||||
| // Session storage key for recent dashboard | ||||||||||||||||
| const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; | ||||||||||||||||
|
|
@@ -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 }>; | ||||||||||||||||
|
||||||||||||||||
| tabsData: Array<{ value: string; title: string; key: string }>; | |
| tabsData: TreeDataNode[]; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 });
}
}
};
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| if (this.state.action === 'saveas') { | |
| selectedTabId = this.state.selectedTab?.value as string; | |
| if (this.state.selectedTab) { | |
| selectedTabId = this.state.selectedTab.value as string; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| parents: ['ROOT_ID', 'GRID_ID', rowKey], | |
| parents: ['ROOT_ID', tabId], |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
meta: {
width: 4,
height: 50,
chartId: chartId,
sliceName: `Chart ${chartId}`,
uuid: `${chartKey}-${Date.now()}`, // or use a proper UUID generator
},| sliceName: `Chart ${chartId}`, | |
| sliceName: `Chart ${chartId}`, | |
| uuid: `${chartKey}-${Date.now()}`, |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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;| const tabTree = result.tab_tree || []; | |
| if (!result || !Array.isArray(result.tab_tree)) { | |
| logging.warn('Invalid tabs response format'); | |
| this.setState({ tabsData: [] }); | |
| return []; | |
| } | |
| const tabTree = result.tab_tree; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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[].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
setStateinstead of directly modifying component state.