fix(table): hide search dropdown when search box is disabled#41772
fix(table): hide search dropdown when search box is disabled#41772durgaprasadml wants to merge 2 commits into
Conversation
dosubot
Bot
added
change:frontend
Code Review Agent Run #28cb9aActionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| describe('Search Select Dropdown conditionally rendering', () => { | ||
| it('does not render "Search by" if include_search is false (server pagination enabled)', () => { | ||
| const props = transformProps({ | ||
| ...testData.raw, | ||
| rawFormData: { | ||
| ...testData.raw.rawFormData, | ||
| server_pagination: true, | ||
| include_search: false, | ||
| }, | ||
| }); | ||
| render( | ||
| ProviderWrapper({ | ||
| children: <TableChart {...props} sticky={false} />, | ||
| }), | ||
| ); | ||
| expect(screen.queryByText('Search by:')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not render "Search by" if there are no search options (server pagination enabled)', () => { | ||
| const props = transformProps({ | ||
| ...testData.raw, | ||
| rawFormData: { | ||
| ...testData.raw.rawFormData, | ||
| server_pagination: true, | ||
| include_search: true, | ||
| }, | ||
| queriesData: [ | ||
| { | ||
| ...testData.raw.queriesData[0], | ||
| colnames: ['num'], | ||
| coltypes: [GenericDataType.Numeric], | ||
| data: [{ num: 1 }, { num: 2 }], | ||
| }, | ||
| ], | ||
| }); | ||
| render( | ||
| ProviderWrapper({ | ||
| children: <TableChart {...props} sticky={false} />, | ||
| }), | ||
| ); | ||
| expect(screen.queryByText('Search by:')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders "Search by" if include_search is true and there are search options (server pagination enabled)', () => { | ||
| const props = transformProps({ | ||
| ...testData.raw, | ||
| rawFormData: { | ||
| ...testData.raw.rawFormData, | ||
| server_pagination: true, | ||
| include_search: true, | ||
| }, | ||
| queriesData: [ | ||
| { | ||
| ...testData.raw.queriesData[0], | ||
| colnames: ['name'], | ||
| coltypes: [GenericDataType.String], | ||
| data: [{ name: 'Michael' }, { name: 'John' }], | ||
| }, | ||
| ], | ||
| }); | ||
| render( | ||
| ProviderWrapper({ | ||
| children: <TableChart {...props} sticky={false} />, | ||
| }), | ||
| ); | ||
| expect(screen.queryByText('Search by:')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Suggestion: Replace this newly introduced grouping block with standalone test() cases (or flatten into existing tests) so the added component test logic does not introduce a new describe() wrapper. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The added block is new component test code and it introduces a nested describe() wrapper around only three related test cases. This matches the rule against using describe() when standalone test() cases would suffice.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
**Line:** 2387:2454
**Comment:**
*Custom Rule: Replace this newly introduced grouping block with standalone `test()` cases (or flatten into existing tests) so the added component test logic does not introduce a new `describe()` wrapper.
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
There was a problem hiding this comment.
@durgaprasadml preferably, new tests to be inline and not nested. Can you help taking a look?
There was a problem hiding this comment.
Yes — this is a valid style nit.
The new describe('Search Select Dropdown conditionally rendering', ...) wrapper isn’t necessary here, and it goes against the “avoid nesting” testing guideline. These three cases can be flattened into standalone test() blocks at the same level as the surrounding tests, while keeping the same assertions and setup.
For example:
test('does not render "Search by" if include_search is false (server pagination enabled)', () => {
// ...
});
test('does not render "Search by" if there are no search options (server pagination enabled)', () => {
// ...
});
test('renders "Search by" if include_search is true and there are search options (server pagination enabled)', () => {
// ...
});That keeps the tests easy to scan and avoids an extra grouping layer for only three cases.
The behavior being tested is still important, so the coverage should stay — just flattened inline. If you want, I can also help adjust the rest of the PR comments in the same style.
|
The review suggestion is correct. Introducing a There are no other comments on this PR to address. superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx |
| expect(screen.queryByText('Search by:')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not render "Search by" if there are no search options (server pagination enabled)', () => { | ||
| const props = transformProps({ | ||
| ...testData.raw, | ||
| rawFormData: { | ||
| ...testData.raw.rawFormData, | ||
| server_pagination: true, | ||
| include_search: true, | ||
| }, | ||
| queriesData: [ | ||
| { | ||
| ...testData.raw.queriesData[0], | ||
| colnames: ['num'], | ||
| coltypes: [GenericDataType.Numeric], | ||
| data: [{ num: 1 }, { num: 2 }], | ||
| }, | ||
| ], | ||
| }); | ||
| render( | ||
| ProviderWrapper({ | ||
| children: <TableChart {...props} sticky={false} />, | ||
| }), | ||
| ); | ||
| expect(screen.queryByText('Search by:')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders "Search by" if include_search is true and there are search options (server pagination enabled)', () => { | ||
| const props = transformProps({ | ||
| ...testData.raw, | ||
| rawFormData: { | ||
| ...testData.raw.rawFormData, | ||
| server_pagination: true, | ||
| include_search: true, | ||
| }, | ||
| queriesData: [ | ||
| { | ||
| ...testData.raw.queriesData[0], | ||
| colnames: ['name'], | ||
| coltypes: [GenericDataType.String], | ||
| data: [{ name: 'Michael' }, { name: 'John' }], | ||
| }, | ||
| ], | ||
| }); | ||
| render( | ||
| ProviderWrapper({ | ||
| children: <TableChart {...props} sticky={false} />, | ||
| }), | ||
| ); | ||
| expect(screen.queryByText('Search by:')).toBeInTheDocument(); |
There was a problem hiding this comment.
Suggestion: The new assertions look for Search by: (with a colon), but this table component renders Search by (without a colon). That makes the positive test fail and allows the negative tests to pass even when the dropdown is actually present. Update the queried text to match the rendered label so the regression tests validate the real behavior. [logic error]
Severity Level: Major ⚠️
- ❌ Search dropdown visibility test fails despite correct behavior.
- ⚠️ Regression tests misvalidate label text and visibility.
- ⚠️ Future UI regressions may pass unnoticed by these tests.Steps of Reproduction ✅
1. Open `superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx` and
locate the search dropdown rendering block at lines 585-591, where the label is rendered
as `t('Search by')` without any colon.
2. Open `superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx` and
inspect the "Search Select Dropdown conditionally rendering" tests at lines 2388-2452,
which all use `screen.queryByText('Search by:')` (with a colon) in their assertions.
3. Run the frontend Jest tests (e.g. `TableChart.test.tsx` via the project's test runner);
the positive test `"renders "Search by" if include_search is true and there are search
options (server pagination enabled)"` at lines 2430-2453 will fail because no element with
exact text `Search by:` exists, even though the dropdown label `Search by` is correctly
rendered.
4. Note that the two negative tests at lines 2388-2428 will still pass even if a dropdown
with label `Search by` is accidentally rendered, because they query `Search by:`
specifically; this mismatch between rendered text and asserted text means the regression
tests do not accurately validate the real UI behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
**Line:** 2402:2452
**Comment:**
*Logic Error: The new assertions look for `Search by:` (with a colon), but this table component renders `Search by` (without a colon). That makes the positive test fail and allows the negative tests to pass even when the dropdown is actually present. Update the queried text to match the rendered label so the regression tests validate the real behavior.
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
durgaprasadml
force-pushed
the
fix/table-search-dropdown-without-search-box
branch
from
a9423f7 to
9f1802a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41772 +/- ##
==========================================
+ Coverage 64.66% 64.68% +0.01%
==========================================
Files 2685 2685
Lines 148534 148538 +4
Branches 34269 34273 +4
==========================================
+ Hits 96055 96087 +32
+ Misses 50720 50692 -28
Partials 1759 1759
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 #c4babcActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
|
@durgaprasadml looks like #36073 (and #35204 before it) already landed the search-box gating on master, so I think what's left here is the empty |
|
Also, mind flattening the new |
durgaprasadml
force-pushed
the
fix/table-search-dropdown-without-search-box
branch
from
9f1802a to
8868e1a
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #652cb8Actionable 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 |
durgaprasadml
force-pushed
the
fix/table-search-dropdown-without-search-box
branch
from
784a929 to
e664052
Compare
Code Review Agent Run #a90223Actionable 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 |
3 participants
Fixes #41747
SUMMARY
This PR fixes an issue where the "Search by" dropdown was displayed in Table charts whenever Server Pagination was enabled, even when the Search Box option was disabled in the Customize tab.
Previously, enabling server-side pagination implicitly caused search-related controls to appear, resulting in confusing behavior for users who intentionally disabled search functionality.
This change updates the rendering logic so that search-related UI elements are only displayed when search functionality is explicitly enabled.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
Server Pagination enabled
Search Box disabled
"Search by" dropdown was still visible
After
Server Pagination enabled
Search Box disabled
"Search by" dropdown is hidden
Search controls only appear when Search Box is enabled
TESTING INSTRUCTIONS
Create a Table chart with one or more text columns.
Enable Server Pagination.
Leave Search Box disabled under the Customize tab.
Save and open the chart.
Verify that the "Search by" dropdown is not displayed.
Verify the following scenarios:
Case 1
Server Pagination = Disabled
Search Box = Disabled
Expected:
No search input
No search dropdown
Case 2
Server Pagination = Disabled
Search Box = Enabled
Expected:
Search input visible
Search dropdown visible
Case 3
Server Pagination = Enabled
Search Box = Disabled
Expected:
No search input
No search dropdown
Case 4
Server Pagination = Enabled
Search Box = Enabled
Expected:
Search input visible
Search dropdown visible
ADDITIONAL INFORMATION
Added regression coverage for search control visibility.
Preserves existing server-side pagination functionality.
Preserves existing search behavior when Search Box is enabled.
No API changes.
No database migrations required.
Backward compatible with existing saved charts.