feat: allow drill-to-details to a specific chart (optionaly) configured at the dataset level#34785
feat: allow drill-to-details to a specific chart (optionaly) configured at the dataset level#34785mistercrunch wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/components/Chart/types.ts | ✅ |
| superset/migrations/versions/2025-08-21_01-54_852e99567fe7_.py | ✅ |
| superset-frontend/src/features/datasets/types.ts | ✅ |
| superset/migrations/versions/2025-08-11_12-43_f56ac3accfc9_drill_through_chart_fk.py | ✅ |
| superset-frontend/src/components/Select/ChartSelect.tsx | ✅ |
| superset-frontend/src/explore/components/controls/SelectAsyncControl/index.tsx | ✅ |
| superset-frontend/src/components/Datasource/DatasourceModal.tsx | ✅ |
| superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx | ✅ |
| superset/datasets/schemas.py | ✅ |
| superset/datasets/api.py | ✅ |
| superset-frontend/src/components/Datasource/DatasourceEditor.jsx | ✅ |
| superset/connectors/sqla/models.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@mistercrunch Ephemeral environment spinning up at http://44.243.96.179:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
While testing, you might hit a weird known issues related to this comment: #23454 (comment) Refreshing the page fixes it, so you may need to refresh the page between saving the dataset with the new drill-to-chart for it to work in the dashboard. |
|
🎪 @unknown Creating ephemeral environment for commit Action: View workflow Building and deploying... Watch the labels for progress updates. |
8148a7c to
5ffb332
Compare
|
🎪 Showtime deployed environment on GHA for f0bb7b6 • Environment: http://54.213.17.221:8080 (admin/admin) |
|
Bito Automatic Review Failed - Technical Failure |
|
🎪 Showtime deployed environment on GHA for e8f275d • Environment: http://34.220.106.89:8080 (admin/admin) |
|
Bito Automatic Review Failed - Technical Failure |
| ); | ||
|
|
||
| // Navigate to the explore page | ||
| window.location.href = url; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
The root cause is the processing of the data-bootstrap DOM attribute, which is parsed as JSON and used to set values like application_root. These values then propagate through helpers, are used to construct URLs, and eventually drive navigation. To fix this, we must ensure that any data loaded from the DOM (specifically, anything parsed from data-bootstrap) is sanitized:
- For path segments like
application_root, ensure they only contain valid path characters and do not contain dangerous meta-characters (e.g.,<,>,",',;, and fragments likejavascript:, etc). - The best and lowest-impact fix is to validate and sanitize
application_root(and other similar fields) immediately after parsingdata-bootstrapwithingetBootstrapData(), before any parts of the code base use them for constructing URLs. - If
application_rootis missing or invalid, fall back to the default value. - Implement a helper called
sanitizeAppRoot, which (i) rejects dangerous characters, (ii) ensures it starts with/, (iii) strips fragments, and (iv) returns a safe string. - Use
sanitizeAppRootinsidegetBootstrapDatato guarantee that all uses downstream (e.g., inensureAppRootand all calling code) are safe.
This approach avoids altering callers, only changes the population of bootstrap data, requires a single helper implementation, and does not affect functionality.
| @@ -21,13 +21,49 @@ | ||
|
|
||
| let cachedBootstrapData: BootstrapData | null = null; | ||
|
|
||
| // Helper to sanitize application root paths | ||
| function sanitizeAppRoot(path: string): string { | ||
| if (typeof path !== 'string') return DEFAULT_BOOTSTRAP_DATA.common.application_root; | ||
| // Remove dangerous characters, allow only typical path characters | ||
| // Remove anything after a '?' or '#' (no query/fragments) | ||
| let safePath = path.split(/[?#]/)[0]; | ||
| // Disallow dangerous meta characters | ||
| if (/[^a-zA-Z0-9\/._-]/.test(safePath)) { | ||
| return DEFAULT_BOOTSTRAP_DATA.common.application_root; | ||
| } | ||
| // Ensure starts with / | ||
| if (!safePath.startsWith('/')) { | ||
| safePath = '/' + safePath; | ||
| } | ||
| // Remove trailing slashes except root | ||
| if (safePath.length > 1) { | ||
| safePath = safePath.replace(/\/+$/, ''); | ||
| } | ||
| return safePath; | ||
| } | ||
|
|
||
| export default function getBootstrapData(): BootstrapData { | ||
| if (cachedBootstrapData === null) { | ||
| const appContainer = document.getElementById('app'); | ||
| const dataBootstrap = appContainer?.getAttribute('data-bootstrap'); | ||
| cachedBootstrapData = dataBootstrap | ||
| ? JSON.parse(dataBootstrap) | ||
| : DEFAULT_BOOTSTRAP_DATA; | ||
| if (dataBootstrap) { | ||
| try { | ||
| const parsed = JSON.parse(dataBootstrap); | ||
| // Sanitize the application_root immediately after parsing bootstrap data | ||
| if ( | ||
| parsed && | ||
| parsed.common && | ||
| typeof parsed.common.application_root === 'string' | ||
| ) { | ||
| parsed.common.application_root = sanitizeAppRoot(parsed.common.application_root); | ||
| } | ||
| cachedBootstrapData = parsed; | ||
| } catch (e) { | ||
| cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA; | ||
| } | ||
| } else { | ||
| cachedBootstrapData = DEFAULT_BOOTSTRAP_DATA; | ||
| } | ||
| } | ||
| // Add a fallback to ensure the returned value is always of type BootstrapData | ||
| return cachedBootstrapData ?? DEFAULT_BOOTSTRAP_DATA; |
|
🎪 Showtime deployed environment on GHA for 775063a • Environment: http://52.37.181.94:8080 (admin/admin) |
|
Bito Automatic Review Failed - Technical Failure |
|
Bito Automatic Review Failed - Technical Failure |
There was a problem hiding this comment.
wasn't, now I'm wondering what's not deterministic in our package.json causing these side effects ...
…and Explore paths Fixes two issues with the drill-through chart configuration: 1. **SelectAsyncControl crash on clear**: Fixed isLabeledValue function to handle null values properly when clearing selections 2. **Missing field in Explore path**: Added drill_through_chart_id to both dataset API response and SqlaTable.data property serialization This ensures the drill-through chart field loads and saves correctly whether accessed from: - Datasets CRUD interface - Explore → Edit Dataset flow 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create DashboardContextFormData type for explicit dashboard context contract - Add useDashboardFormData hook to encapsulate complex dashboard filter logic - Simplify DrillDetailPane from 40+ lines to 4 lines using new hook - Fix Edit Chart link to open drill-through chart instead of original chart - Ensure StatefulChart inherits dashboard native filters, colors, and context This ensures drill-through charts get the same dashboard context as regular dashboard charts, providing consistent filter inheritance and user experience. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace manual URL construction in DrillDetailModal with getExploreUrl utility - Refactor ChartSelect to derive props from SelectAsyncControl using ComponentProps - Add comprehensive test coverage for ChartSelect and useDashboardFormData components - Update DatasourceEditor to use allowClear instead of deprecated clearable prop - Use rest pattern for better prop forwarding and type safety 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…archParams Replace manual URL construction with SupersetClient's built-in searchParams support. Rename queryParams prop to searchParams for API alignment and eliminate 12 lines of URL building logic. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…vigation - Only show Edit chart button when drill-through chart is configured - Fix navigation to properly route to explore page instead of dashboard - Use Button href for same-tab navigation (no target="_blank") - Add Dataset.id type definition for URL generation - Update tests to verify link behavior and required dataset properties 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Make Explore button visible only when drill-through chart is configured - Change button text from "Edit chart" to "Explore" for clarity - Fix Explore button to navigate to correct drill-through chart instead of dashboard - Add proper dashboard context resolution using useDashboardFormData hook - Use getFormDataWithDashboardContext for proper filter and context mixing - Simplify StatefulChart rendering with proper formDataOverrides - Generate explore URLs using getExploreUrl with GET method for reliability - Add error handling for URL generation with user-friendly toast messages - Remove duplicate filter conversion logic and use established patterns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…herited filters - Mark drill-to-detail filters with `isExtra: true` in `getFormDataWithDashboardContext` - Ensures context menu filters (e.g., "drill to details by -> Canada") appear as dashboard-inherited filters rather than chart-native filters in explore page - Maintains consistency with filter bar filters which already display correctly - Updated `getFormDataWithDashboardContext` to accept optional `drillToDetailFilters` parameter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplifies repeated postFormData + mountExploreUrl + dashboard_page_id pattern used across 6+ files by providing a clean, type-safe async utility. Benefits: - DrillDetailModal: 15 lines → 5 lines (67% reduction) - Centralized URL generation logic for better maintainability - Type-safe options parameter for chartId, tabId, dashboardPageId - Comprehensive test coverage with 4 test scenarios - Better error handling and promise chain management Files refactored: - DrillDetailModal: Simplified async URL generation - DrillByModal: Clean promise chain with proper error handling - SqlLab ResultSet: Modern async/await pattern - Added comprehensive utility tests All component tests passing ✅ 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Any hope of getting this one through? It's a pretty awesome feature :D |
|
What can i do to make this PR happen? Please help me to begin, i need it dreadfully for my work! |
|
how to enable this function? I got issue with this drill-to-detail table where I can sort the table or resize the row and column width |
This PR introduces customizable drill-to-details functionality that allows dataset owners to configure which chart should be used when users drill through from any chart using their dataset.
Problem: Current drill-to-details uses a one-size-fits-all approach showing all columns in a generic table with no customization options.
Solution: Dataset-level drill-to-details customization allowing association of any existing chart as the drill-to-details target.
Key Benefits:
Implementation:
Walkthrough: