feat(semantic layers): don't show samples tab in explore#41509
feat(semantic layers): don't show samples tab in explore#41509betodealmeida wants to merge 5 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41509 +/- ##
==========================================
- Coverage 64.73% 64.69% -0.05%
==========================================
Files 2687 2688 +1
Lines 148743 148976 +233
Branches 34329 34382 +53
==========================================
+ Hits 96295 96379 +84
- Misses 50681 50824 +143
- Partials 1767 1773 +6
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:
|
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // (e.g. semantic views). The check is intentionally ``=== false`` so that | ||
| // datasources from older backends that don't send the flag still show the | ||
| // tab and preserve current behavior. | ||
| const showSamplesTab = datasource?.supports_samples !== false; |
There was a problem hiding this comment.
Suggestion: Conditionally removing the Samples tab can leave the currently selected tab pointing to a key that no longer exists when the datasource changes from supporting samples to not supporting them. In that state the panel can render with no active pane and no results request, so users see an empty data panel until they manually recover. Add a synchronization step that forces the active tab back to Results whenever samples become unsupported. [logic error]
Severity Level: Major ⚠️
❌ Explore data panel can show blank content after switch.
⚠️ Samples request flag set but SamplesPane no longer mounted.
❌ Users must manually recover by clicking a Results tab.Steps of Reproduction ✅
1. Open the Explore view for a chart whose datasource supports samples; this renders
`ExploreChartPanel` at
`superset-frontend/src/explore/components/ExploreChartPanel/index.tsx:553-564`, which
includes `DataTablesPane` with the current `datasource` prop.
2. In the Explore data panel, click the “Samples” tab; this triggers `handleTabClick` in
`DataTablesPane` at `DataTablesPane.tsx:143-152`, setting `activeTabKey` to
`ResultTypes.Samples` (`'samples'`) and causing the samples request branch in the effect
at `DataTablesPane.tsx:107-133` to set `isRequest.samples` to `true`.
3. While the panel remains open on the Samples tab, switch the datasource in Explore to a
semantic view whose `datasource.supports_samples` is `false`; `ExploreChartPanel`
re-renders and passes the new `datasource` into `DataTablesPane` (index.tsx:553-564),
where `showSamplesTab` at `DataTablesPane.tsx:204` becomes `false` and `tabItems` at
`DataTablesPane.tsx:205-228` is recomputed without any item whose `key` is
`ResultTypes.Samples`.
4. Observe that `activeTabKey` remains `'samples'` because there is no effect syncing it
to `ResultTypes.Results` when `showSamplesTab` changes, and `Tabs` at
`DataTablesPane.tsx:232-237` is rendered with `activeKey={panelOpen ? activeTabKey : ''}`
but `items={tabItems}` containing only results tabs. With an `activeKey` that no longer
matches any tab item, the data panel renders without a visible active tab or content, and
since `SamplesPane` is no longer mounted, no new results request is issued until the user
manually clicks a results tab to recover.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx
**Line:** 204:204
**Comment:**
*Logic Error: Conditionally removing the Samples tab can leave the currently selected tab pointing to a key that no longer exists when the datasource changes from supporting samples to not supporting them. In that state the panel can render with no active pane and no results request, so users see an empty data panel until they manually recover. Add a synchronization step that forces the active tab back to Results whenever samples become unsupported.
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. When the Here is the suggested fix for useEffect(() => {
if (!showSamplesTab && activeTabKey === ResultTypes.Samples) {
setActiveTabKey(ResultTypes.Results);
}
}, [showSamplesTab, activeTabKey]);I have checked the PR comments and there are no other pending review comments to address. Would you like me to implement any other changes? superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx |
41e74cd to
7dac6c1
Compare
Code Review Agent Run #3e87cfActionable 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 |
Code Review Agent Run #087886Actionable 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 |
mikebridge
left a comment
mikebridge
left a comment
There was a problem hiding this comment.
Review: Approve
Clean, well-scoped change — the capability-flag modeling reads nicely and the backward-compat story (server defaults True, strict === false frontend checks) is airtight. Two minor notes, both optional:
- Drill-to-detail lacks a symmetric server-side gate.
supports_samplesgets a real refusal in the samples endpoint, butsupports_drill_to_detailis only surfaced in thedata()payload and enforced in the frontend menu — drill-detail requests flow through the chart-data API (superset/common/query_actions.py:227) with no matching check. Fine for this PR's scope, but the flags are advertised as symmetric while only one is server-enforced. Worth a follow-up if drill must be hard-blocked. - Fill the empty PR template sections (TESTING INSTRUCTIONS, ADDITIONAL INFORMATION are blank; UI checkbox unchecked despite the UI behaviour change), and drop the stray
package-lock.jsonlibcchurn if easy.
This review was generated by Claude (AI) on behalf of @mikebridge.
|
@betodealmeida should we worry about the codeant finding? We might want to make sure we have tests that assert the Samples tab doesn't show as blank, and re-appears when switching back to a SQL tab. |
mikebridge
left a comment
mikebridge
left a comment
There was a problem hiding this comment.
Approving — LGTM. Any notes in my earlier review are non-blocking.
This review was generated by Claude (AI) on behalf of @mikebridge.
48958cc to
a664294
Compare
Code Review Agent Run #b0733fActionable Suggestions - 0Additional Suggestions - 3
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 |
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| @patch("superset.views.datasource.views._", lambda s: s) |
There was a problem hiding this comment.
Suggestion: Replace the inline lambda with a named helper function that includes explicit parameter and return type annotations, and patch using that typed helper instead. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new code introduces an inline lambda without type hints in a modified Python file. Since the rule requires type hints for new or modified Python code where annotations can be added, replacing it with a named helper annotated with explicit parameter and return types is a valid match.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/views/datasource/views_test.py
**Line:** 322:322
**Comment:**
*Custom Rule: Replace the inline lambda with a named helper function that includes explicit parameter and return type annotations, and patch using that typed helper instead.
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
Code Review Agent Run #f6a2dbActionable 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 |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
thanks for addressing the comments, LGTM
There was a problem hiding this comment.
are this changes intended?
|
Obviously we need a rebase here, but does anyone want to sweep through the bot review comments (many are not marked as resolved) and take/reject/resolve them? (the branch is on origin, so any Committer should be able to do this) |
|
Happy to tackle the rebase here if @betodealmeida doesn't have time and nobody else wants to volunteer :) Just say the word. |
76ed4cd to
38254af
Compare
| expect(screen.getByText('Results')).toBeVisible(); | ||
| // Panel stays expanded and renders Results content rather than going blank. | ||
| expect(screen.getByLabelText('Collapse data panel')).toBeVisible(); |
There was a problem hiding this comment.
Suggestion: This regression test does not actually verify that the pane switched away from an orphaned tab and rendered fallback content; it only checks that the tab label and collapse button are visible, which can still be true when the content area is blank. Add an assertion on concrete Results-pane content (or active tab state) after rerender so the original blank-panel bug cannot pass unnoticed. [logic error]
Severity Level: Major ⚠️
- ⚠️ Explore Data panel regression may bypass this specific test.
- ⚠️ Blank Results content on datasource swap might reoccur undetected.
- ⚠️ Regression coverage for semantic view datasources remains incomplete.Steps of Reproduction ✅
1. Open
`superset-frontend/src/explore/components/DataTablesPane/test/DataTablesPane.test.tsx` and
locate the test `Falls back to Results when active Samples tab disappears mid-session` at
lines 108–137, which documents a prior bug leaving `activeTabKey === 'samples'` and
rendering a blank panel (comment at lines 109–112).
2. Assume the original bug in `DataTablesPane` reappears so that, after a datasource swap
that removes the Samples tab, the tab strip (including the `Results` label) and the panel
chrome (including the `Collapse data panel` button) still render, but the content area
remains blank because the active tab key no longer matches any rendered tab (as described
in the test comment).
3. Run the frontend test suite (e.g. `npm test -- DataTablesPane.test.tsx`) so this
regression test mounts `DataTablesPane`, clicks `Expand data panel` and `Samples`, then
rerenders with `supports_samples: false`, following the exact sequence coded at lines
113–133.
4. Observe that the assertions at lines 134–136 only verify that the `Results` tab label
and `Collapse data panel` button are visible, without asserting any concrete Results
content (such as table rows) or the active tab state; in the bug scenario described in
step 2 these UI chrome elements can still be present while the content area is blank, so
the test still passes and fails to catch the regression.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/DataTablesPane/test/DataTablesPane.test.tsx
**Line:** 134:136
**Comment:**
*Logic Error: This regression test does not actually verify that the pane switched away from an orphaned tab and rendered fallback content; it only checks that the tab label and collapse button are visible, which can still be true when the content area is blank. Add an assertion on concrete Results-pane content (or active tab state) after rerender so the original blank-panel bug cannot pass unnoticed.
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
Code Review Agent Run #2038baActionable Suggestions - 0Additional Suggestions - 3
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 |
4 participants
SUMMARY
In Explore, hide the "Samples" tab when visualizing a semantic view, since it doesn't make sense for them.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION