mikebridge commented on PR #41509: URL: https://github.com/apache/superset/pull/41509#issuecomment-5104734710
## Review **What it does:** Adds two capability flags, `supports_samples` and `supports_drill_to_detail`, defaulted `True` on `BaseDatasource` and set `False` on `SemanticView` (since semantic views expose only pre-defined metrics/dimensions, not raw rows). Uses these to: - Hide the "Samples" tab in Explore and disable the "Drill to detail" context-menu item on the frontend - Hard-block the same operations server-side (`/datasource/samples` returns 400; `_get_drill_detail` raises `QueryObjectValidationError`) - Swap three raw `<pre>` error blocks (DrillDetailPane, SamplesPane, useResultsPane) for a proper `Alert` component **Correctness — solid:** - Defense in depth is done right: UI hides the affordance *and* the backend independently rejects it, so there's no bypass via direct API calls. - `DatasourceDAO.sources` mapping includes `SEMANTIC_VIEW: SemanticView`, so the views.py class-level lookup works as intended. - Backward compatibility handled carefully — frontend checks are `!== false` / `getattr(..., True)`, so datasources/backends that don't send the flag keep current behavior. - Nice catch: a `useEffect` falls back the active tab to "Results" if a datasource swap makes the currently-active "Samples" tab disappear (avoids an orphaned/blank panel) — addresses a prior automated-review finding on this same PR. - `Alert` import path (`@apache-superset/core/components`) matches the pattern used elsewhere in the codebase. **Nits:** - The `<pre>` → `Alert` UX refactor in three files is unrelated to the stated PR purpose (hiding the Samples tab). Not wrong, just worth calling out in the description. - `DISABLED_REASONS.DATASOURCE` is a vague key name next to siblings like `DATABASE`/`NO_AGGREGATIONS` — something like `DATASOURCE_TYPE` would read clearer. - `supports_samples`/`supports_drill_to_detail` are duplicated on the `Datasource` type across both `dashboard/types.ts` and `explore/types.ts` (pre-existing duplication pattern, not introduced here). - PR checklist is unfilled — "Changes UI" isn't checked despite adding an Alert component and hiding a tab, and testing instructions are blank. **Test coverage:** Good — backend unit tests for the query_actions gate, the views.py 400 path, and SemanticView flags; frontend tests for tab-hiding, the mid-session fallback, and the disabled drill-to-detail states. No blocking issues — looks mergeable pending the checklist being filled in. --- *Automated review* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
