fix(menu): Use app prefix in Menu brand icon url#34935
fix(menu): Use app prefix in Menu brand icon url#34935martyngigg wants to merge 5 commits intoapache:masterfrom
Conversation
dosubot
bot
added
change:frontend
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
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/features/home/Menu.tsx | ✅ |
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.
| <Image | ||
| preview={false} | ||
| src={theme.brandLogoUrl} | ||
| src={assetUrl(theme.brandLogoUrl)} |
There was a problem hiding this comment.
Won't this cause issues when the brandLogoUrl is hosted outside Superset?
There was a problem hiding this comment.
Maybe I have misunderstood how this now works with the new theming?
When assetUrl was originally implemented in #30134 it was assumed that STATIC_ASSETS_PREFIX would be a prefix to all static assets and if using a CDN for example then this would be set as the STATIC_ASSETS_PREFIX and this would then become a prefix to everything.
Similarly we have ensureAppRoot for prefixing non-asset paths.
I think I saw that the default theme.brandLogoUrl value was a relative path much like superset_config[APP_ICON] so I assumed the same rules applied. Maybe assetUrl() could also check the value passed to it is a relative path and only add the prefix then?
There was a problem hiding this comment.
Maybe assetUrl() could also check the value passed to it is a relative path and only add the prefix then?
Yes, that would be my solution as well.
There was a problem hiding this comment.
I've gone with adding a second assetUrlf that checks for the relative path as in the other places it is used the check is not necessary so it avoids wasting time in those places.
Checks if the path is relative and only adds the static prefix if this is true
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #495b24
Actionable Suggestions - 1
-
superset-frontend/src/features/home/Menu.tsx - 1
- Browser compatibility issue · Line 22-22
Additional Suggestions - 1
-
superset-frontend/src/features/home/Menu.test.tsx - 1
-
Path concatenation issue · Line 299-299The test assertion for brand icon src attribute has a path concatenation issue. When `app_root` is '/myapp' and `icon` is '/static/assets/images/superset-logo-horiz.png', the concatenation `${app_root}${icon}` produces '/myapp/static/assets/images/superset-logo-horiz.png' which could lead to double slashes or missing slashes depending on the values. Use proper path joining to ensure consistent asset URLs.
Code suggestion
@@ -299,1 +299,1 @@ - expect(image).toHaveAttribute('src', `${app_root}${icon}`); + expect(image).toHaveAttribute('src', `${app_root}${icon}`.replace(/\/+/g, '/'));
-
Review Details
-
Files reviewed - 4 · Commit Range:
8c48ebb..d4b2fa7- superset-frontend/src/features/home/Menu.test.tsx
- superset-frontend/src/features/home/Menu.tsx
- superset-frontend/src/utils/assetUrl.test.ts
- superset-frontend/src/utils/assetUrl.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
|
/resolve |
|
Closing in favour of #36058. |
2 participants
SUMMARY
Since the theming changes, if an
app_rootis set to a non-empty value, the brand logo failed to render as thesrclink was incorrect. I think this is a bug against 6.0?These changes include the app_root prefix similarly to other places across the frontend.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Add
SUPERSET_APP_ROOT="/workspace/example"todocker/.env-local.Spin up the
docker-compose-lightstack and use the web developer tools to inspect the logo imagesrcattribute. Before the change the asset link was missing the "/workspace/example" prefix but it is included after this change.In development, using the webpack server, the image actually still appears as the image is in the
superset/static/assets/imagesdirectory and the webpack server can still find it. I'm not sure how this can be fixed...ADDITIONAL INFORMATION