Re: [PR] fix(superset-frontend): Fixes for broken functionality when an application root is defined [superset]

Mon, 10 Nov 2025 09:23:04 -0800 


korbit-ai[bot] commented on code in PR #36058:
URL: https://github.com/apache/superset/pull/36058#discussion_r2511375361


##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -26,3 +26,11 @@ import { applicationRoot } from 'src/utils/getBootstrapData';
 export function ensureAppRoot(path: string): string {
   return `${applicationRoot()}${path.startsWith('/') ? path : `/${path}`}`;
 }
+
+/**
+ * Takes a string path to a resource and remove the applicationRoot if it is 
not empty
+ * @param path A string path to a resource
+ */
+export function stripAppRoot(path: string): string {
+  return `${path.startsWith(applicationRoot()) ? 
path.slice(applicationRoot().length) : path}`;

Review Comment:
   ### Empty applicationRoot causes incorrect path stripping <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The stripAppRoot function will fail when applicationRoot() returns an empty 
string, as path.startsWith('') always returns true for any string.
   
   
   ###### Why this matters
   When applicationRoot() is empty, all paths will be processed as if they 
start with the application root, causing the function to incorrectly slice 
characters from the beginning of every path, potentially breaking URL routing 
and resource loading.
   
   ###### Suggested change ∙ *Feature Preview*
   Add a check to ensure applicationRoot is not empty before attempting to 
strip it:
   
   ```typescript
   export function stripAppRoot(path: string): string {
     const appRoot = applicationRoot();
     return appRoot && path.startsWith(appRoot) ? path.slice(appRoot.length) : 
path;
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f973e64-e5f0-4e34-b19e-9dcf9d476514/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f973e64-e5f0-4e34-b19e-9dcf9d476514?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f973e64-e5f0-4e34-b19e-9dcf9d476514?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f973e64-e5f0-4e34-b19e-9dcf9d476514?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f973e64-e5f0-4e34-b19e-9dcf9d476514)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:f3f21509-7279-485a-91c5-aa08be9aee08 -->
   
   
   [](f3f21509-7279-485a-91c5-aa08be9aee08)



##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -292,7 +292,7 @@ const ResultSet = ({
       if (openInNewWindow) {
         window.open(url, '_blank', 'noreferrer');
       } else {
-        history.push(url);
+        history.push(stripAppRoot(url));

Review Comment:
   ### Uncached stripAppRoot call on navigation <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The stripAppRoot function is called on every navigation without caching the 
result, potentially causing unnecessary string processing overhead.
   
   
   ###### Why this matters
   If stripAppRoot performs complex string operations and this navigation 
happens frequently, the repeated function calls could impact performance, 
especially in scenarios with many chart creations.
   
   ###### Suggested change ∙ *Feature Preview*
   Consider caching the result of stripAppRoot(url) in a variable if the same 
URL might be processed multiple times, or ensure stripAppRoot is optimized for 
repeated calls with memoization.
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4954dbf-7488-40c2-a4fa-f7093e37bff3/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4954dbf-7488-40c2-a4fa-f7093e37bff3?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4954dbf-7488-40c2-a4fa-f7093e37bff3?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4954dbf-7488-40c2-a4fa-f7093e37bff3?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4954dbf-7488-40c2-a4fa-f7093e37bff3)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:ec912579-f685-45b0-adcb-ed3c470730c9 -->
   
   
   [](ec912579-f685-45b0-adcb-ed3c470730c9)



-- 
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]

Reply via email to