feat: improve chart builder tab management UX#8957
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR improves the DataTable chart builder’s tab UX by making tab creation/selection/closure behavior more consistent and by attempting to prevent duplicate tab names.
Changes:
- Passes an
onCloseChartBuildercallback fromDataTablePlugininto the chart builder panel. - Updates chart tab creation/deletion logic to auto-create an initial chart tab on open, close the chart builder when the last chart tab is removed, and choose the next active tab more like standard tab UIs.
- Replaces the old “tab number” naming mechanism with a counter-based approach intended to avoid duplicate tab names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
frontend/src/plugins/impl/DataTablePlugin.tsx |
Wires onCloseChartBuilder so the chart builder can be programmatically closed when appropriate. |
frontend/src/components/data-table/charts/charts.tsx |
Implements new chart tab lifecycle behavior (auto-create, delete behavior, tab naming counter). |
| // Auto-create a default chart tab when chart builder opens with no tabs | ||
| if ( | ||
| displayHeader && | ||
| !prevDisplayHeader.current && | ||
| tabs.length === 0 && | ||
| cellId | ||
| ) { | ||
| prevDisplayHeader.current = displayHeader; | ||
| const tabName = getChartTabName(0, NEW_CHART_TYPE); | ||
| const newTabs = new Map(tabsMap); | ||
| newTabs.set(cellId, [ | ||
| { tabName, chartType: NEW_CHART_TYPE, config: getChartDefaults() }, | ||
| ]); | ||
| saveTabsMap(newTabs); | ||
| setTabCounter(1); | ||
| setSelectedTab(tabName); | ||
| } |
There was a problem hiding this comment.
The auto-create logic runs during render and calls saveTabsMap, setTabCounter, and setSelectedTab. Updating Jotai/React state during render is unsafe (can break under React 18 concurrent rendering / StrictMode and may trigger render-loop warnings). Move this into a useEffect that runs on the displayHeader transition (closed → open) and only then create the default tab + update selection/counter.
| const [selectedTab, setSelectedTab] = useState(DEFAULT_TAB_NAME); | ||
| const [tabCounter, setTabCounter] = useState(tabs.length); | ||
| const prevDisplayHeader = useRef(displayHeader); |
There was a problem hiding this comment.
Initializing tabCounter with tabs.length does not guarantee unique/monotonically increasing tab names after deletions. Example: if only "Bar Chart 2" remains, tabs.length is 1 so the next add will again produce "Bar Chart 2". Consider deriving the next counter from existing tab names (e.g., max parsed index + 1) or persisting a per-cell monotonic counter in storage so names never collide.
📝 Summary
Before:
Screen.Recording.2026-03-31.at.5.48.26.PM.mov
After:
Screen.Recording.2026-03-31.at.5.49.07.PM.mov