feat: add tab select with save chart to dashboard #36332
Conversation
This reverts commit 35aaeb6.
SBIN2010
added
the
🎪 ⚡ showtime-trigger-start
github-actions
bot
added
🎪 a339bd9 🚦 building
github-actions
bot
added
🎪 a339bd9 🚦 failed
SBIN2010
added
the
🎪 🛑 showtime-trigger-stop
github-actions
bot
removed
🎪 🛑 showtime-trigger-stop
|
@SBIN2010 This workflow is deprecated! Please use the new Superset Showtime system instead:
Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@SBIN2010 Ephemeral environment spinning up at http://34.221.129.14:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to select a specific tab when saving a chart to a dashboard, providing users with more precise control over chart placement in multi-tab dashboards.
Key Changes:
- Added TreeSelect component from Ant Design for tab selection in the save modal
- Implemented
loadTabsmethod to fetch available tabs from a selected dashboard - Created
addChartToDashboardTabmethod to programmatically place charts on specific tabs - Added TypeScript interfaces
TabNodeandTreeDataNodefor type safety
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| superset-frontend/src/explore/types.ts | Added TypeScript type definitions for tab tree structures (TabNode and TreeDataNode) |
| superset-frontend/src/explore/components/SaveModal.tsx | Enhanced save modal with tab selection functionality, including state management, API calls, and dashboard layout manipulation |
| superset-frontend/src/explore/components/SaveModal.test.jsx | Added unit tests for tab selector rendering and behavior, including mock setup for TreeSelect component |
| 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); |
There was a problem hiding this 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[].
| addChartToDashboardTab = async ( | ||
| dashboardId: number, | ||
| chartId: number, | ||
| tabId: string, | ||
| ) => { |
There was a problem hiding this 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
| export interface TreeDataNode { | ||
| value: string; | ||
| title: string; | ||
| key: string; | ||
| children?: TreeDataNode[]; |
There was a problem hiding this 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.
| export interface TreeDataNode { | |
| value: string; | |
| title: string; | |
| key: string; | |
| children?: TreeDataNode[]; | |
| export interface TabTreeNode { | |
| value: string; | |
| title: string; | |
| key: string; | |
| children?: TabTreeNode[]; |
| 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 []; | ||
| } | ||
| }; |
There was a problem hiding this 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
| if (this.state.action === 'saveas') { | ||
| selectedTabId = this.state.selectedTab?.value as string; |
There was a problem hiding this 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.
| if (this.state.action === 'saveas') { | |
| selectedTabId = this.state.selectedTab?.value as string; | |
| if (this.state.selectedTab) { | |
| selectedTabId = this.state.selectedTab.value as string; |
| width: 4, | ||
| height: 50, | ||
| chartId: chartId, | ||
| sliceName: `Chart ${chartId}`, |
There was a problem hiding this 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:
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()}`, |
| component.state.tabsData = [ | ||
| { value: 'tab1', title: 'Main Tab', key: 'tab1' }, | ||
| { value: 'tab2', title: 'Analytics Tab', key: 'tab2' }, | ||
| ]; |
There was a problem hiding this comment.
Use setState instead of directly modifying component state.
| component.state.tabsData = [ | |
| { value: 'tab1', title: 'Main Tab', key: 'tab1' }, | |
| { value: 'tab2', title: 'Analytics Tab', key: 'tab2' }, | |
| ]; | |
| component.setState({ | |
| tabsData: [ | |
| { value: 'tab1', title: 'Main Tab', key: 'tab1' }, | |
| { value: 'tab2', title: 'Analytics Tab', key: 'tab2' }, | |
| ], | |
| }); |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
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. |
2 participants
SUMMARY
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:
Added
TreeSelectfrom Ant Design for tab selectionBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
demo2_can_select_tab.webm
TESTING INSTRUCTIONS
run test src/explore/components/SaveModal.test.jsx
ADDITIONAL INFORMATION