-
Notifications
You must be signed in to change notification settings - Fork 16.1k
fix(superset-frontend): Fixes for broken functionality when an application root is defined #36058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The superset/fab_override templates no longer exist but were still referenced. Remove the override classes that reference them and fallback to the FAB templates.
The ReactRouter has basePath set to applicationRoot, therefore all paths must be relative to this
|
Bito Review Skipped - Source Branch Not Found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Empty applicationRoot causes incorrect path stripping ▹ view | ||
| Uncached stripAppRoot call on navigation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/utils/pathUtils.ts | ✅ |
| superset-frontend/src/utils/assetUrl.ts | ✅ |
| superset-frontend/src/features/home/Menu.tsx | ✅ |
| superset-frontend/src/features/home/RightMenu.tsx | ✅ |
| superset-frontend/src/SqlLab/components/ResultSet/index.tsx | ✅ |
| superset/views/base.py | ✅ |
| superset/security/manager.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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36058 +/- ##
===========================================
+ Coverage 0 68.70% +68.70%
===========================================
Files 0 622 +622
Lines 0 45743 +45743
Branches 0 4979 +4979
===========================================
+ Hits 0 31427 +31427
- Misses 0 13070 +13070
- Partials 0 1246 +1246
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2f699f6 to
09fa854
Compare
09fa854 to
97a5ade
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes broken functionality when an application root is defined in Superset's configuration by ensuring proper handling of URL paths throughout the frontend and cleaning up obsolete backend template code.
- Adds
ensureStaticPrefixutility to conditionally apply static asset prefixes to relative paths - Fixes "Create chart" button in SQL Lab to correctly handle app roots when opening in new windows vs navigating in-app
- Removes obsolete FAB template widget classes from backend that were orphaned by a previous PR
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| superset/views/base.py | Removes unused SupersetListWidget class and ListWidget import |
| superset/security/manager.py | Removes unused widget classes (SupersetSecurityListWidget, SupersetRoleListWidget) and their assignments |
| superset-frontend/src/utils/assetUrl.ts | Adds ensureStaticPrefix function to handle static asset URLs |
| superset-frontend/src/utils/assetUrl.test.ts | Adds tests for assetUrl and ensureStaticPrefix functions |
| superset-frontend/src/features/home/RightMenu.tsx | Applies ensureAppRoot to user info URL |
| superset-frontend/src/features/home/Menu.tsx | Applies ensureStaticPrefix to brand images and ensureAppRoot to brand links |
| superset-frontend/src/features/home/Menu.test.tsx | Updates test to verify brand image URLs include static assets prefix |
| superset-frontend/src/explore/exploreUtils/index.js | Adds includeAppRoot parameter to getURIDirectory and mountExploreUrl functions |
| superset-frontend/src/SqlLab/components/ResultSet/index.tsx | Uses includeAppRoot parameter to correctly handle URLs for new window vs in-app navigation |
| expect(assetUrl(absoluteResourcePath)).toBe( | ||
| `${app_root}/${absoluteResourcePath}`, | ||
| ); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for assetUrl with an absolute URL (line 38-40) expects it to prepend the app root and a slash, which would produce invalid URLs like /myapp/https://cdn.domain.com/static/endpoint/img.png. The assetUrl function is designed for relative paths only. This test case should either be removed or moved to a separate test that validates assetUrl should not be used with absolute URLs.
| expect(assetUrl(absoluteResourcePath)).toBe( | |
| `${app_root}/${absoluteResourcePath}`, | |
| ); |
| const absoluteResourcePath = `https://cdn.domain.com/static${resourcePath}`; | ||
|
|
||
| beforeEach(() => { | ||
| // Clear app root |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Clear app root" but the code is actually clearing the static assets prefix mock. The comment should be updated to "Clear static assets prefix" to accurately reflect what the code is doing.
| // Clear app root | |
| // Clear static assets prefix |
| beforeEach(() => { | ||
| // setup a DOM element as a render target | ||
| useSelectorMock.mockClear(); | ||
| // By default use empty app root |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "By default use empty app root" but the code is actually setting the static assets prefix mock to an empty string. The comment should be updated to "By default use empty static assets prefix" to accurately reflect what the code is doing.
| // By default use empty app root | |
| // By default use empty static assets prefix |
SUMMARY
Fixes for the settings menu:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Fixes #35027