feat(sqllab): treeview table selection ui#37298
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
dosubot
bot
added
change:frontend
justinpark
requested review from
betodealmeida,
eschutho,
kasiazjc,
michael-s-molina and
rusackas
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
superset-frontend/packages/superset-ui-core/src/components/Segmented/types.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #9ba2ef
Actionable Suggestions - 3
-
superset-frontend/src/SqlLab/components/AppLayout/index.tsx - 1
- Sidebar collapse blocked · Line 168-168
-
superset-frontend/src/components/DatabaseSelector/index.tsx - 1
- Missing DatabaseValue properties · Line 382-383
-
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx - 1
- Missing await for async userEvent.click · Line 261-261
Additional Suggestions - 5
-
superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx - 1
-
Incomplete loading UI test · Line 213-234The test 'shows loading skeleton while fetching schemas' mocks a delayed API response but only verifies the post-loading state by waiting for 'public' text. It does not assert that the Skeleton component is shown during loading, making the test incomplete for UI loading behavior. Consider adding a check for the Skeleton before the waitFor, or restructuring to verify the loading indicator explicitly.
-
-
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx - 1
-
RTL Query Best Practice · Line 49-69The test helper uses document.querySelectorAll('.ant-segmented-item') to find and click the SelectView button, which bypasses React Testing Library's accessibility-focused queries. Consider using screen.getByRole('button', { name: 'SelectView' }) instead for better test reliability and alignment with RTL guidelines.
Code suggestion
@@ -50,19 +50,17 @@ - // Find all segmented items and click the second one (SelectView) - const segmentedItems = document.querySelectorAll('.ant-segmented-item'); - if (segmentedItems.length >= 2) { - await userEvent.click(segmentedItems[1]); - // Wait for the view to switch - await waitFor(() => { - expect( - screen.queryByTestId('mock-table-explore-tree'), - ).not.toBeInTheDocument(); - }); - const changeButton = screen.getByRole('button', { name: 'Change' }); - // Click Change button to open database selector modal - await userEvent.click(changeButton); - - // Verify modal is opened - await waitFor(() => { - expect(screen.getByRole('dialog')).toBeInTheDocument(); - }); - } + // Click the SelectView button + await userEvent.click(screen.getByRole('button', { name: 'SelectView' })); + // Wait for the view to switch + await waitFor(() => { + expect( + screen.queryByTestId('mock-table-explore-tree'), + ).not.toBeInTheDocument(); + }); + const changeButton = screen.getByRole('button', { name: 'Change' }); + // Click Change button to open database selector modal + await userEvent.click(changeButton); + + // Verify modal is opened + await waitFor(() => { + expect(screen.getByRole('dialog')).toBeInTheDocument(); + }); +};
-
-
superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx - 1
-
Incorrect pinned tables filtering · Line 179-188The pinnedTables logic includes tables from all query editors but uses an empty string key for non-matching ones, which won't match table keys later. This could lead to incorrect column data display for pinned tables. Consider filtering tables first to ensure only relevant pinned data is included.
Code suggestion
diff --git a/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx b/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx index 0000000..0000000 100644 --- a/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx +++ b/superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx @@ -179,10 +179,10 @@ const TableExploreTree: React.FC<Props> = ({ queryEditorId }) => { const pinnedTables = useMemo( () => Object.fromEntries( - tables.map(({ queryEditorId, dbId, schema, name, persistData }) => [ - queryEditor.id === queryEditorId ? `${dbId}:${schema}:${name}` : '', - persistData, - ]), + tables + .filter(({ queryEditorId: tableQueryEditorId }) => tableQueryEditorId === queryEditor.id) + .map(({ dbId, schema, name, persistData }) => [`${dbId}:${schema}:${name}`, persistData]), ), [tables, queryEditor.id], );
-
-
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx - 1
-
Custom CSS Violation · Line 65-65The addition of 'white-space: nowrap;' introduces custom CSS that violates the guideline in AGENTS.md to avoid custom styles whenever possible. This may affect text wrapping in the SaveQuery component, but following antd best practices could achieve similar effects without custom CSS.
Code suggestion
@@ -64,3 +64,2 @@ - const Styles = styled.span` - white-space: nowrap; - span[role='img']:not([aria-label='down']) { + const Styles = styled.span` + span[role='img']:not([aria-label='down']) {
-
-
superset-frontend/src/SqlLab/components/TableElement/index.tsx - 1
-
Unused Styled Component · Line 75-85StyledCollapse is defined but never used in the component, creating dead code that should be cleaned up.
Code suggestion
@@ -75,11 +75,0 @@ - const StyledCollapse = styled(Collapse)` - & .ant-collapse-header { - padding-left: 0 !important; - padding-right: 0 !important; - } - - & .ant-collapse-content-box { - padding-top: 0 !important; - padding-bottom: 0 !important; - } - `; -
-
Review Details
-
Files reviewed - 21 · Commit Range:
21267e0..21267e0- superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/packages/superset-ui-core/src/components/Segmented/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Segmented/types.ts
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/src/SqlLab/components/AppLayout/index.tsx
- superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/SqlEditorTopBar.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.ts
- superset-frontend/src/SqlLab/components/TableElement/index.tsx
- superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
- superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx
- superset-frontend/src/SqlLab/constants.ts
- superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
- superset-frontend/src/components/DatabaseSelector/index.tsx
- superset-frontend/src/components/TableSelector/TableSelector.test.tsx
- superset-frontend/src/components/TableSelector/index.tsx
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| const databaseWithId = { ...modalDb, id: modalDb.id }; | ||
| setCurrentDb(databaseWithId as DatabaseValue); |
There was a problem hiding this comment.
Ensure databaseWithId includes all fields of DatabaseValue. For example:
const databaseWithId = {
...modalDb,
id: modalDb.id,
value: modalDb.id,
label: modalDb.database_name,
};Code Review Run #9ba2ef
The 'databaseWithId' object lacks required 'value' and 'label' properties for 'DatabaseValue' type, which can cause rendering issues when setting the current database from modal selection.
Code Review Run #699bcf
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| }); | ||
| }); | ||
| if (header) { | ||
| userEvent.click(header); |
There was a problem hiding this comment.
The userEvent.click call lacks await, which can cause test flakiness since userEvent.click is asynchronous in @testing-library/user-event v12.8.3. This matches the pattern used elsewhere in the file, like in switchToSelectView.
Code suggestion
Check the AI-generated fix before applying
| userEvent.click(header); | |
| await userEvent.click(header); |
Code Review Run #9ba2ef
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #876bbaActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
justinpark
force-pushed
the
feat--sqllab-treeview-left-sidebar
branch
from
467a10d to
d1d4051
Compare
|
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 · |
| jest.mock('../TableExploreTree', () => ({ | ||
| __esModule: true, | ||
| default: () => ( | ||
| <div data-test="mock-table-explore-tree">TableExploreTree</div> |
There was a problem hiding this comment.
Suggestion: The mock component uses the attribute data-test while the tests query data-testid via screen.queryByTestId. This mismatch means the mock won't be found by queryByTestId and the test logic that waits for or asserts on that element will be incorrect or flaky; change the attribute to data-testid so the test queries can reliably find the mock. [possible bug]
Severity Level: Critical 🚨
- ❌ SelectView tests fail or become flaky in CI.
- ⚠️ Developer local test runs unreliable, causing reruns.| <div data-test="mock-table-explore-tree">TableExploreTree</div> | |
| <div data-testid="mock-table-explore-tree">TableExploreTree</div> |
Steps of Reproduction ✅
1. Run the test suite that includes this file (file:
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx). The
mock is defined at lines 40-46 (jest.mock of TableExploreTree).
2. A test (for example the test at ~line 144 "renders a TableElement") calls await
switchToSelectView() (helper defined at lines 48-69) which, after clicking the segmented
item, executes:
- waitFor assertion at lines 55-59 that checks
screen.queryByTestId('mock-table-explore-tree') is not in the document.
3. Because the mock component renders a div with data-test="mock-table-explore-tree" (line
44) instead of data-testid, screen.queryByTestId('mock-table-explore-tree') (the query
used in the waitFor at lines 55-59) will always return null immediately — even when the
mocked tree is still present in the DOM. The waitFor condition passes prematurely.
4. After the premature pass, the helper proceeds to call screen.getByRole('button', {
name: 'Change' }) at line 60. If the UI has not actually finished switching views,
getByRole will throw (element not found) and the test fails or becomes flaky.
Explanation: The current mismatch (data-test vs data-testid) at lines 40-46 makes the
test's query (lines 55-59) invalid. Replacing data-test with data-testid at the mock (line
44) aligns the mock with the query and prevents the waitFor from passing before the actual
UI update completes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx
**Line:** 44:44
**Comment:**
*Possible Bug: The mock component uses the attribute `data-test` while the tests query `data-testid` via `screen.queryByTestId`. This mismatch means the mock won't be found by `queryByTestId` and the test logic that waits for or asserts on that element will be incorrect or flaky; change the attribute to `data-testid` so the test queries can reliably find the mock.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
Code Review Agent Run #699bcfActionable Suggestions - 1
Additional Suggestions - 4
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
This looks amazing!! Thank you so much @justinpark. As we talked over zoom, here are some of the feedback points:
These are just changes to make the screen even cleaner but I love the overall feature. |
|
@justinpark thank you for the PR! Some small initial thoughts below (@michael-s-molina will need your brain here too) **Top right corner schema picker ** 
I think the modal is an additional step and in a way harsh visual change that we can avoid by using dropdowns in the fields itself, so:
Pinned/Preview tab
Tell me what you think! |
justinpark
force-pushed
the
feat--sqllab-treeview-left-sidebar
branch
from
396a850 to
fac96ef
Compare
| ({ children }: { children: (params: { height: number }) => ReactChild }) => | ||
| children({ height: 500 }), |
There was a problem hiding this comment.
Suggestion: The AutoSizer mock only provides height to the child render function but many consumers expect both width and height; if the real component destructures width this will be undefined and can cause layout issues or runtime errors. Mock the same shape the real AutoSizer provides by passing both width and height. [possible bug]
Severity Level: Major ⚠️
- ⚠️ TableExploreTree test rendering may differ from real layout.
- ⚠️ SQLLab treeview layout assertions could be flaky.
- ⚠️ Real AutoSizer-related regressions may be invisible in tests.| ({ children }: { children: (params: { height: number }) => ReactChild }) => | |
| children({ height: 500 }), | |
| ({ children }: { children: (params: { width: number; height: number }) => ReactChild }) => | |
| children({ width: 500, height: 500 }), |
Steps of Reproduction ✅
1. Run the tests in this file: execute Jest for
superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx.
The test file sets up a mock for AutoSizer at lines 27-32 (jest.mock(...) returning
children({ height: 500 })).
2. The render flow starts at renderComponent() (lines 97-101) which calls
render(<TableExploreTree ... />). TableExploreTree will be mounted inside the test
with the mocked AutoSizer in place (mock replaces the real module).
3. If TableExploreTree (the component under test) expects both width and height
from AutoSizer (common pattern), its internal code will receive params.width ===
undefined
because the mock only supplies height. This can cause layout calculations or style
logic that destructures width to use undefined, producing unexpected layout or runtime
errors during rendering.
4. Observed behaviour when this occurs: tests may render differently, assertions about
visible nodes (tree layout) can fail, or the component may throw if it assumes numeric
width.
If the component does not need width, the tests still pass — this mock omission is a
subtle mismatch with the real AutoSizer API and may mask regressions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
**Line:** 30:31
**Comment:**
*Possible Bug: The AutoSizer mock only provides `height` to the child render function but many consumers expect both `width` and `height`; if the real component destructures `width` this will be undefined and can cause layout issues or runtime errors. Mock the same shape the real AutoSizer provides by passing both `width` and `height`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { | ||
| status: 200, | ||
| body: { | ||
| columns: mockColumns, | ||
| }, | ||
| }); | ||
| fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { | ||
| status: 200, | ||
| body: {}, |
There was a problem hiding this comment.
Suggestion: The generic table_metadata/* mock is registered before the more specific table_metadata/extra/* mock, causing the generic route to match and shadow the specific one; register the specific extra/* route first (or use a more specific pattern) so the intended response for the extra endpoint is returned. [logic error]
Severity Level: Major ⚠️
- ❌ Tests relying on extra metadata receive wrong payload.
- ⚠️ TableExploreTree behavior under extra metadata affected.
- ⚠️ Mock mismatch can hide API contract regressions.| fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { | |
| status: 200, | |
| body: { | |
| columns: mockColumns, | |
| }, | |
| }); | |
| fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { | |
| status: 200, | |
| body: {}, | |
| // register the more specific `extra/*` route before the generic `table_metadata/*` | |
| fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { | |
| status: 200, | |
| body: {}, | |
| }); | |
| fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { | |
| status: 200, | |
| body: { | |
| columns: mockColumns, | |
| }, |
Steps of Reproduction ✅
1. Open the test file
superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx.
The beforeEach at line 53 registers fetch-mock routes starting at line 54.
2. The generic route for table metadata is registered at line 62:
fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', ...) returning columns
(lines 62-67).
3. A more specific route for extra metadata is registered afterwards at lines 68-71:
fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', ...) returning {}.
4. When the component under test issues a request to
/api/v1/database/1/table_metadata/extra/<...>, fetch-mock matches routes in
registration order,
so the earlier generic 'table_metadata/*' (registered at line 62) will match first and
be used,
causing the extra/* request to receive the generic response (columns) instead of the
intended {}.
5. Reproduce by running the test that triggers the extra metadata path (e.g., expand
actions that cause
TableExploreTree to fetch extra metadata) and observe the response body not matching
expectations.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
**Line:** 62:70
**Comment:**
*Logic Error: The generic `table_metadata/*` mock is registered before the more specific `table_metadata/extra/*` mock, causing the generic route to match and shadow the specific one; register the specific `extra/*` route first (or use a more specific pattern) so the intended response for the extra endpoint is returned.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| fetchMock.clearHistory(); |
There was a problem hiding this comment.
Suggestion: The test teardown calls only fetchMock.clearHistory() which leaves registered routes in place across tests; this can accumulate duplicate routes or cause tests in other files to see stale mocks. Restore/reset fetch-mock in afterEach to fully remove registered handlers and avoid cross-test interference. [resource leak]
Severity Level: Critical 🚨
- ❌ Other tests observe stale network mocks.
- ❌ Test suite flakiness and false failures.
- ⚠️ CI runs may become unstable intermittently.| fetchMock.clearHistory(); | |
| // fully restore/reset fetch-mock so routes do not leak between tests | |
| // `restore` removes registered mocks; `resetHistory` clears call history | |
| fetchMock.restore(); | |
| fetchMock.resetHistory(); |
Steps of Reproduction ✅
1. Run the full Jest suite including this file. The afterEach at lines 74-77 currently
only calls
jest.clearAllMocks() and fetchMock.clearHistory(), leaving registered fetch-mock routes
in place.
2. Another test file (for example,
packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx)
runs after this test file and performs network interactions; because routes remain
registered,
those tests may match the mocks defined here and receive unexpected mocked responses.
3. Observe cross-test interference: unrelated tests see network calls intercepted by this
file's mocks,
causing flaky failures or unexpected pass/fail behavior across the suite.
4. Fix reproduction: change afterEach to call fetchMock.restore() and
fetchMock.resetHistory() so registered
handlers are removed and call history cleared, preventing leakage when running the test
suite.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx
**Line:** 76:76
**Comment:**
*Resource Leak: The test teardown calls only `fetchMock.clearHistory()` which leaves registered routes in place across tests; this can accumulate duplicate routes or cause tests in other files to see stale mocks. Restore/reset fetch-mock in afterEach to fully remove registered handlers and avoid cross-test interference.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Code Review Agent Run #4e21ebActionable Suggestions - 0Additional Suggestions - 4
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Hey @justinpark, Thanks for working on this update, SQL Lab is looking pretty nice! One thing I wanted to check with you is about the OAuth2 error flow. In the old layout, we would display a warning in the catalog/schema/table selectors and/or in the Results pane. With the new UI, the error message might take too much space: OAuth2.auth.prompt.movAlso, after clicking on the link and going through the auth flow, the user gets redirected back but triggering a refresh (which should clear the error message) is difficult due to the message length. Also, clearing the message at the popup doesn't clear it at the DB selector: Screen.Recording.2026-01-30.at.8.20.02.PM.movAny ideas on how to better display this error? Thank you! |
I wasn't even aware of this flow. Thanks for pointing out @Vitor-Avila! To better understand the requirement, is the authorization required before listing the catalog/schemas or can you read them but not query the tables? I'm asking because the solution would be different depending on this requirement. |
|
@michael-s-molina it depends. Typically, you would need a token before doing anything, so this could happen at the catalog selector (if Also, for any OAuth connection the token might be manually revoked at the DB side, which would prompt users to create a new token in Superset, so we must support this error message in any of these dropdowns. Also, the error would show up in the Results pane if the user tried to execute a SQL query, or in the Preview pane when selecting a table and the token was revoked. In case it's helpful, I can record a video of how this would show up in the old SQL Lab UI. Thank you! |
|
@Vitor-Avila If I'm understanding this correctly, given that the token can be revoked at any time, multiple operations in SQL Lab would fail if a user does not have a valid token. This means that this error message should not be associated with any panel/selector. It's more of a modal/toast type of error message. cc @kasiazjc |
It depends on the implementation, previously we would show it relative/in/under the component that returned the error (example from when schema load failed): 
We can definitely have it in a central place right now (or even make it trigger the dialog "expanded" directly: |
@Vitor-Avila I really think a central place would be better given that this error might affect multiple parts of the screen and it's not related to a specific feature. My suggestion would be to always display a modal similar to the second screenshot you shared. |
Sure, I'm definitely not opposed to it! We might want to think about other errors as well. Like if the connection is broken and the user tries to refresh the list of tables, should this error also show in this new/centralized place? |
That's probably a good question for @kasiazjc. My personal opinion is that "global" errors should use the modal or toast pattern and inline errors should be used when they affect a particular part of the screen but the remaining features still work. Losing connection is a global error, all features will be affected. |
So for the more global errors I am thinking:
so: combination of both if that makes sense, banner only if simply global not triggered by a specific field Agree on the contextual inline for errors that only affect part of the screen. wdyt @michael-s-molina @Vitor-Avila ? |
|
@kasiazjc My concern with displaying global errors inline is that it introduces unnecessary complexity and can result in users seeing the same error message in multiple places. For instance, if there’s a lost connection or missing authentication credentials, showing inline errors would mean that every section relying on a connection or authentication—potentially 10+ locations—would display a "Connection error" or "Failed to authenticate" message. I believe this is exactly why Ant Design offers an error Notification component. In my view, inline errors make sense for non-global issues only. 
|
That makes sense to me - let's ignore my combined error idea :D With notification error I am worried though that it's not that "in your face" - that's why my personal preference and sweet spot is banner/alert. Hard to miss and details are on hand (also if text will be longer) |
Aren't banners and alerts inline components? 
Do you mean toast? Which would be similar to Notification. |
|
Also, I think it's really hard to not see the notifications. Especially because they are animated. You can also position them at the center. Screen.Recording.2026-02-05.at.11.27.38.mov |
I actually meant alert/banner, but true that technically they are inline (and in terms of inline - I was thinking more of field outline + metadata error text). Alert banners are commonly used for global/system-level errors though - there has to be right placement, so that's why I am thinking above SQL area and it's the most central option (at the top of the major section). The errors that were mentioned are blocking, and these two types of components we were talking about have different purposes/feel:
users are more likely to dismiss the notification - I think authorization flow is critical, because otherwise they cannot do anything and with dismiss they are left with nothing - especially if they forgot to click on a link that was shown there (to complete it) |
Yes, I completely agree that we have these 2 types of global notifications depending on requiring user interaction 👍🏼
Exactly. In this case, we can use a banner at the top/center or a modal. Your call. |
|
hey @kasiazjc @michael-s-molina did we decide on something? If we do a banner, should we do under the SQL text box? |
No. A banner would be a global message not tied to any particular feature just like a modal. It would be something like this but with a better design. |
|
Ok, so I did a bunch of digging in terms of UX best practises and patterns in other BI apps to figure out error patterns that we could use and I am going back to my previous idea of combined error (validation on field + banner in this specific case). A little backstory and context (and maybe a start on design guidelines) - in terms of an error types I think these three types make sense for us in general: Validation Error OnlyWhen: Standard form validation, doesn't block entire workflow.
Why field-only: Error and fix are in the same place. Alert Banner OnlyWhen: Not tied to a specific field, or system-level issue.
Why banner-only: No field caused it, no field can fix it. Validation Error + Alert BannerWhen: Field-specific issue blocks the workflow, user can resolve it.
Why both: Field shows what's wrong in context (which specific selection broke the flow), banner shows impact and how to fix. Persists when modal closes. @michael-s-molina I know you are not a fan of combining both, but this is also a pattern that other BI tools use (like Looker or PowerBI) - so showing the error both at the source (the field) and at the impact point (where work is blocked). For our database auth case, the key issue is the flow: user picks a database, sees the auth error in the modal, but might close it without authorizing. If the error only lives in that modal, it disappears. Then they try to run a query and get a confusing failure. With a banner above the SQL editor + field indicator, the error stays visible and they don't have to remember what went wrong or where to fix it. Visually it would look like this (also this would be a placement for the global banner in general) 
This is also aligned with UX principles like:
Wdyt? |
This makes sense. The banner shows the global problem, which avoids repeated error messages, but you're still indicating to the user what field caused the global problem. |
|
@kasiazjc Just one last comment. If you look at this, there's a need for these global banners. We can design them:
WDYT? |
Yeah I think the second option makes more sense, especially from an admin setup perspective. If we're talking about app-level announcements, warnings, or errors that admins need to configure, having a consistent global placement (full-width below header I think would work best) is way easier to manage than trying to figure out module-specific positioning for each message (also would be a pain to implement especially before full app pluginisation :D). The module-level banners we designed for SQL Lab still make sense for workflow-specific validation and errors, but for admin-managed messaging, the global approach is cleaner both for setup and for users. So this is what I would go with for now. |
@michael-s-molina for now - added a note on error types and the mockup I pasted above in our figma file |
5 participants
SUMMARY
Based on the SIP-111 proposal and improvements to the previous #31591 PR, this commit introduces a new SQLLab sidebar treeview UI.
The differences from the previous PR are as follows:
Performance Upgrade with react-arborist
For large schema/table lists, severe scrolling jank occurred due to Ant Design's TreeView performance issues. To resolve this, the treeview has been newly implemented using the open-source library react-arborist.
Segmented UI for Table Selector
By utilizing a segmented UI, users can now switch to the table selector using the traditional select dropdown method.
Modal-Based Selector for Database/catalog/schema selector
With the new design, the dropdown options for the database/catalog/schema selector at the top of the editor were being cut off due to the overflow:hidden property of the editor's top panel. This has now been reworked to open as a modal, allowing users to make selections without any cutoff.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
after--treeview-ui.mov
TESTING INSTRUCTIONS
specs and locally tested
ADDITIONAL INFORMATION