fix: show warning when table list is truncated in TableSelector#41798
fix: show warning when table list is truncated in TableSelector#41798AdarshJ173 wants to merge 3 commits into
Conversation
Code Review Agent Run #352b8fActionable Suggestions - 0Additional Suggestions - 1
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 |
The useTables hook computes a hasMore flag (json.count > json.result.length) but the TableSelector component never reads it. When a database has more tables than the API page limit, users see a truncated list with no indication that tables are missing. Changes: - Import Alert from @superset-ui/core/components - Display an Alert warning when data?.hasMore is true, informing users that some tables are not shown - Wrap in aria-live='polite' div with role='status' for accessibility - Style the warning using the theme warning color - Add data-test attribute for testability Tests: - Verify warning renders when hasMore is true (count > result.length) - Verify warning is absent when hasMore is false (count === result.length) Fixes apache#40407 Signed-off-by: AdarshJ173 <adarshj173@gmail.com>
AdarshJ173
force-pushed
the
fix/table-selector-has-more-warning
branch
from
ec1054b to
6875d70
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| {t( | ||
| 'Some tables are not shown. Refine your search.', | ||
| )} |
There was a problem hiding this comment.
Suggestion: The warning text instructs users to “Refine your search,” but this component only performs client-side filtering on the already-truncated options list (no server-side search request is triggered), so refining search cannot reveal missing tables. Update the message to describe an action that actually changes the fetched result set (for example schema/catalog change or refresh behavior), otherwise users are guided to a non-working path. [comment mismatch]
Severity Level: Major ⚠️
- ⚠️ TableSelector warning suggests ineffective search for missing tables.
- ⚠️ Datasource editing UX confusing when table list truncated.Steps of Reproduction ✅
1. Configure a database with more tables than the tables API returns per request so that
`/api/v1/database/<id>/tables/` responds with `json.count > json.result.length`, which
sets `hasMore: true` in the transform (see
`superset-frontend/src/hooks/apiResources/tables.ts:103-118`, especially lines 115-118
where `hasMore` is computed).
2. Open the physical dataset editor where `TableSelector` is used by `DatasourceEditor`
(see
`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:20-61`,
lines 20-61 in the snippet where the `Field` with `fieldKey="tableSelector"` renders a
`TableSelector` control wired to the current database, catalog, and schema).
3. In the rendered `TableSelector` component, observe that it calls `useTables` with the
current `dbId`, `catalog`, and `schema` and receives `data.hasMore = true` when the
response is truncated (see
`superset-frontend/src/components/TableSelector/index.tsx:185-207` where `useTables` is
invoked, and lines 209-223 where `tableOptions` are derived from `data.options`). Because
`data.hasMore` is truthy, the new `helpText` at lines 338-343 renders the message `Some
tables are not shown. Refine your search.` below the select.
4. Try to follow the instruction by typing into the table select search box. The `Select`
component’s `filterOption` prop is wired to `handleFilterOption` (see `index.tsx:304-328`
for the `Select` and `index.tsx:295-302` for `handleFilterOption`), which only filters the
already-fetched `tableOptions` array client-side and never triggers `refetch` or a new
query. As a result, tables omitted from `data.options` due to truncation can never be
surfaced by refining the search, even though the warning tells the user that refining the
search will help, confirming the suggestion’s mismatch between guidance and actual
behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/TableSelector/index.tsx
**Line:** 340:342
**Comment:**
*Comment Mismatch: The warning text instructs users to “Refine your search,” but this component only performs client-side filtering on the already-truncated `options` list (no server-side search request is triggered), so refining search cannot reveal missing tables. Update the message to describe an action that actually changes the fetched result set (for example schema/catalog change or refresh behavior), otherwise users are guided to a non-working path.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current warning message suggests that users can "Refine your search" to find missing tables, but the To resolve this, the message should be updated to guide users toward actions that trigger a new fetch, such as changing the schema or catalog, or refreshing the data. Here is a suggested update for the warning message in <Alert
type="warning"
showIcon
message={t(
'Some tables are not shown. Try changing the schema or catalog to find the table you need.',
)}
/>I have checked the PR and there are no other review comments to address. Would you like me to proceed with any other changes? superset-frontend/src/components/TableSelector/index.tsx |
The useTables hook computes a hasMore flag (json.count > json.result.length)
but the TableSelector component never consumes it. When a database has more
tables than the API page limit, users see a truncated list with no indication.
Changes:
- Import Alert from @apache-superset/core/components (the Alert wrapper
component is not part of @superset-ui/core/components)
- Render an Alert type="warning" with role="alert" and aria-live="polite"
when data?.hasMore is true, using antd's built-in accessibility
- Restored .table-length CSS to original (use design system, avoid custom CSS)
Tests:
- Added positive test: mock with count=100 / result=4 items triggers alert
- Added negative test: mock with count=4 / result=4 items does not show alert
- Replaced "as any" mocks with typed interfaces (TableApiResponse)
- Use getByRole('alert') / queryByRole('alert') per project testing patterns
Closes apache#40407
Signed-off-by: AdarshJ173 <adarshj173@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41798 +/- ##
=======================================
Coverage 64.74% 64.74%
=======================================
Files 2687 2687
Lines 148743 148745 +2
Branches 34329 34331 +2
=======================================
+ Hits 96302 96304 +2
Misses 50677 50677
Partials 1764 1764
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #1911cbActionable 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 |
|
@AdarshJ173 a little more PR description couldn't hurt, but I'm mainly curious what you think of Bito's copy suggestion. |
2 participants
Closes #40407