-
Notifications
You must be signed in to change notification settings - Fork 16k
fix: no fs logging of extensions unless flag is set #35612
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?
fix: no fs logging of extensions unless flag is set #35612
Conversation
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 |
---|---|---|
Unconditional API call when extensions disabled ▹ view |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/extensions/ExtensionsStartup.tsx | ✅ |
superset-frontend/src/extensions/ExtensionsManager.ts | ✅ |
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.
public async initializeExtensions(): Promise<void> { | ||
if (!isFeatureEnabled(FeatureFlag.EnableExtensions)) { | ||
return; | ||
} | ||
const response = await SupersetClient.get({ | ||
endpoint: '/api/v1/extensions/', | ||
}); |
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.
Unconditional API call when extensions disabled 
Tell me more
What is the issue?
The initializeExtensions method now unconditionally makes an API call to fetch extensions, even when the EnableExtensions feature flag is disabled.
Why this matters
This will cause unnecessary network requests and potential processing of extension data when extensions are disabled, contradicting the developer's intent to avoid extension-related operations when the flag is off.
Suggested change ∙ Feature Preview
The feature flag check should be moved to the caller (ExtensionsStartup) or restored in this method to prevent the API call when extensions are disabled:
public async initializeExtensions(): Promise<void> {
if (!isFeatureEnabled(FeatureFlag.EnableExtensions)) {
return;
}
const response = await SupersetClient.get({
endpoint: '/api/v1/extensions/',
});
// ... rest of the method
}
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
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.
No, the check was moved one level up, so that this is never triggered if the flag is unset.
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.
I see, thank you for clarifying. The higher-level check addresses the concern about unnecessary API calls when extensions are disabled.
Good catch @villebro. You need to move the tests that check for single initialization from |
SUMMARY
Currently we're logging that extensions are initialized successfully even if the feature flag isn't set. This moves the feature flag check one level up to avoid triggering this log unless the FF is enabled.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION