You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ju...@apache.org on 2026/03/27 21:54:52 UTC
(superset) branch master updated: fix(sqllab): invalid treeview folder expansion (#38897)
This is an automated email from the ASF dual-hosted git repository.
justinpark pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new a5d2324e216 fix(sqllab): invalid treeview folder expansion (#38897)
a5d2324e216 is described below
commit a5d2324e21652de64f1f25716efbe2eed1ecc175
Author: JUST.in DO IT <ju...@airbnb.com>
AuthorDate: Fri Mar 27 14:54:45 2026 -0700
fix(sqllab): invalid treeview folder expansion (#38897)
---
.../TableExploreTree/TableExploreTree.test.tsx | 82 ++++++++++++++++++++++
.../SqlLab/components/TableExploreTree/index.tsx | 55 ++++++++++++---
2 files changed, 127 insertions(+), 10 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx b/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
index 58e9193c649..8a1cacdf6d8 100644
--- a/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
+++ b/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
@@ -264,3 +264,85 @@ test('renders contributed toolbar action in leftSidebar slot', async () => {
screen.getByRole('button', { name: 'Left Sidebar Action' }),
).toBeInTheDocument();
});
+
+test('shows columns immediately on first toggle when searchTerm is active', async () => {
+ // Regression: treeData change after async fetch was resetting react-arborist's
+ // internal open state, so children only appeared after toggling twice.
+ // The useEffect([treeData]) fix ensures manually-opened nodes stay open.
+ renderComponent();
+
+ await waitFor(() => {
+ expect(screen.getByText('public')).toBeInTheDocument();
+ });
+
+ // Expand schema to load tables
+ await userEvent.click(screen.getByText('public'));
+ await waitFor(() => {
+ expect(screen.getByText('users')).toBeInTheDocument();
+ });
+
+ // Activate search while schema is already expanded
+ const searchInput = screen.getByPlaceholderText(
+ 'Enter a part of the object name',
+ );
+ await userEvent.type(searchInput, 'pub');
+
+ // Tables remain visible under the search-matched schema
+ expect(screen.getByText('users')).toBeInTheDocument();
+
+ // Toggle the table with searchTerm active — columns must appear on the FIRST click
+ await userEvent.click(screen.getByText('users'));
+
+ await waitFor(() => {
+ expect(screen.getByText('id')).toBeInTheDocument();
+ });
+ expect(screen.getByText('name')).toBeInTheDocument();
+ expect(screen.getByText('created_at')).toBeInTheDocument();
+});
+
+test('closes a schema while searchTerm is active and keeps it closed', async () => {
+ // Regression: manuallyOpenedNodes must record the schema as closed when the
+ // user collapses it with a searchTerm active. If it remains true, the treeData
+ // effect would immediately re-open the schema on the next data change.
+ //
+ // Note: searchTerm 'users' is intentional. A term that matches the schema name
+ // (e.g. 'pub') would cause highlightText to split 'public' across two DOM nodes,
+ // making screen.getByText('public') unreliable. 'users' matches a table name so
+ // the schema label is left as a plain text node and remains queryable.
+ renderComponent();
+
+ await waitFor(() => {
+ expect(screen.getByText('public')).toBeInTheDocument();
+ });
+
+ // Expand schema to load tables
+ await userEvent.click(screen.getByText('public'));
+ await waitFor(() => {
+ expect(screen.getByText('users')).toBeInTheDocument();
+ });
+
+ // Activate search with a term that matches a table name; 'public' schema is
+ // visible as an ancestor and its text is not highlighted (safe to query by text)
+ const searchInput = screen.getByPlaceholderText(
+ 'Enter a part of the object name',
+ );
+ await userEvent.type(searchInput, 'users');
+ expect(screen.getByText('public')).toBeInTheDocument();
+
+ // Load columns to trigger a treeData change that could accidentally reopen the schema
+ await userEvent.click(screen.getByText('users'));
+ await waitFor(() => {
+ expect(screen.getByText('id')).toBeInTheDocument();
+ });
+
+ // Close the schema while searchTerm is active
+ await userEvent.click(screen.getByText('public'));
+
+ // Tables and columns must disappear and stay gone — the treeData effect must not
+ // reopen the schema because manuallyOpenedNodes was updated to false on close
+ await waitFor(() => {
+ expect(screen.queryByText('id')).not.toBeInTheDocument();
+ });
+ // The schema node itself remains visible as a matching ancestor (just collapsed)
+ expect(screen.getByText('public')).toBeInTheDocument();
+});
diff --git a/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx b/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx
index 13b6e700ad5..5a505355506 100644
--- a/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx
+++ b/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx
@@ -18,6 +18,7 @@
*/
import {
useCallback,
+ useEffect,
useState,
useRef,
type ChangeEvent,
@@ -165,6 +166,21 @@ const TableExploreTree: React.FC<Props> = ({ queryEditorId }) => {
Record<string, boolean>
>({});
+ // Keep a ref so the treeData effect below always reads the latest value
+ // without needing it as a dependency (we only want to re-open on data change).
+ const manuallyOpenedNodesRef = useRef(manuallyOpenedNodes);
+ manuallyOpenedNodesRef.current = manuallyOpenedNodes;
+
+ // When treeData changes (e.g., children arrive after an async fetch),
+ // react-arborist may reset the open state of nodes that just received children.
+ // Explicitly re-open every node the user has manually opened so children
+ // become visible immediately without requiring a second toggle.
+ useEffect(() => {
+ Object.entries(manuallyOpenedNodesRef.current)
+ .filter(([, isOpen]) => isOpen)
+ .forEach(([id]) => treeRef.current?.open(id));
+ }, [treeData]);
+
// Custom search match function for react-arborist
const searchMatch = useCallback(
(node: NodeApi<TreeNodeData>, term: string): boolean => {
@@ -320,18 +336,37 @@ const TableExploreTree: React.FC<Props> = ({ queryEditorId }) => {
return;
}
- // Track manually opened/closed state
- setManuallyOpenedNodes(prev => {
- const wasOpen = prev[id] ?? false;
- const isNowOpen = !wasOpen;
+ // Determine the previous open state to compute the new state.
+ //
+ // When searchTerm is active, react-arborist auto-expands nodes whose
+ // descendants match the query. Those nodes have isOpen=true in the tree
+ // but are absent from manuallyOpenedNodes, so the original check
+ // (`manuallyOpenedNodes[id] ?? false`) misidentifies them as closed and
+ // inverts the toggle direction. Reading from treeRef (which holds the
+ // actual pre-toggle state because onToggle fires before the state change)
+ // fixes this.
+ //
+ // When searchTerm is empty, searchMatch returns true for every node and
+ // react-arborist marks all schemas as open (isOpen=true) even before any
+ // user interaction. Using treeRef in that case would treat every first
+ // click as a close action, so fall back to manuallyOpenedNodes instead.
+ const wasOpen = searchTerm
+ ? (treeRef.current?.get(id)?.isOpen ??
+ manuallyOpenedNodes[id] ??
+ false)
+ : (manuallyOpenedNodes[id] ?? false);
+ const isNowOpen = !wasOpen;
- // Trigger data fetch when opening
- if (isNowOpen) {
- handleToggle(id, true);
- }
+ // Trigger data fetch when opening
+ if (isNowOpen) {
+ handleToggle(id, true);
+ }
- return { ...prev, [id]: isNowOpen };
- });
+ // Track manually opened/closed state
+ setManuallyOpenedNodes(prev => ({
+ ...prev,
+ [id]: isNowOpen,
+ }));
}}
>
{renderNode}