codeant-ai-for-open-source[bot] commented on code in PR #40441:
URL: https://github.com/apache/superset/pull/40441#discussion_r3304463686
##########
superset-frontend/src/extensions/ExtensionsStartup.tsx:
##########
@@ -53,11 +64,40 @@ const ExtensionsStartup: React.FC<{ children?:
React.ReactNode }> = ({
children,
}) => {
const [initialized, setInitialized] = useState(false);
+ const location = useLocation();
+ const prevPathname = useRef<string | null>(null);
const userId = useSelector<RootState, number | undefined>(
({ user }) => user.userId,
);
+ // Notify the navigation namespace on every route change.
+ useEffect(() => {
+ if (prevPathname.current !== location.pathname) {
+ prevPathname.current = location.pathname;
+ notifyPageChange(location.pathname);
+ }
+ }, [location.pathname]);
+
+ // Isolate unhandled rejections from extension code for the lifetime of the
+ // app — registered once, never removed.
+ useEffect(() => {
+ const handleUnhandledRejection = (event: PromiseRejectionEvent) => {
+ logging.error(
+ '[extensions] Unhandled rejection from extension:',
+ event.reason,
+ );
+ event.preventDefault();
+ };
Review Comment:
**Suggestion:** The global `unhandledrejection` handler unconditionally
calls `preventDefault()`, which suppresses all unhandled promise rejections in
the app, not just extension-originated failures. This can hide host-side
runtime errors and break normal browser error reporting; scope this suppression
to verified extension errors (or only log without preventing default for
non-extension rejections). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Host unhandled rejections mislabelled as extension failures.
- ⚠️ Browser default unhandled-rejection logging is suppressed.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The root `App` component in `superset-frontend/src/views/App.tsx:82-123`
wraps all
routes inside `<ExtensionsStartup>` (line 91), so `ExtensionsStartup` is
mounted for every
authenticated SPA session.
2. Inside `ExtensionsStartup`, the effect at
`superset-frontend/src/extensions/ExtensionsStartup.tsx:82-99` registers a
global
`unhandledrejection` listener on `window` whose `handleUnhandledRejection`
logs
`"[extensions] Unhandled rejection from extension:"` and unconditionally
calls
`event.preventDefault()` for every unhandled Promise rejection.
3. Trigger any unhandled Promise rejection in the running app (for example,
in the browser
DevTools console, evaluate `new Promise((_, reject) => reject(new
Error('test-unhandled')));` without a `.catch()`), which per the browser
event model
raises a `window`-level `unhandledrejection` event that is delivered to all
registered
listeners, including `handleUnhandledRejection`.
4. Observe that the rejection is always logged as an "[extensions]" error by
the handler
in `ExtensionsStartup.tsx:85-90` and that the default browser handling of
unhandled
rejections is suppressed via `event.preventDefault()`, masking whether the
failure
originated in host application code or extension code and reducing the
visibility of real
app runtime errors in normal browser logging.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=840b82d1d1d54dc9a8a198c82e272cb4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=840b82d1d1d54dc9a8a198c82e272cb4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/extensions/ExtensionsStartup.tsx
**Line:** 85:91
**Comment:**
*Logic Error: The global `unhandledrejection` handler unconditionally
calls `preventDefault()`, which suppresses all unhandled promise rejections in
the app, not just extension-originated failures. This can hide host-side
runtime errors and break normal browser error reporting; scope this suppression
to verified extension errors (or only log without preventing default for
non-extension rejections).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=68f00592e5f72f1eecdf1f63aab24abb9d8d16b30694953a12c8c559e721b620&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=68f00592e5f72f1eecdf1f63aab24abb9d8d16b30694953a12c8c559e721b620&reaction=dislike'>👎</a>
##########
superset-frontend/src/extensions/ExtensionsLoader.ts:
##########
@@ -149,8 +172,33 @@ class ExtensionsLoader {
await container.init(__webpack_share_scopes__.default);
const factory = await container.get('./index');
- // Execute the module factory - side effects fire registrations
- factory();
+
+ // Intercept contribution registrations during module activation so we can
+ // collect the Disposables and drive cleanup on deactivation.
+ const collected: (() => void)[] = [];
+ const originalSuperset = window.superset;
+ window.superset = {
+ ...originalSuperset,
+ views: {
+ ...originalSuperset.views,
+ registerView: (
+ ...args: Parameters<typeof originalSuperset.views.registerView>
+ ) => {
+ const disposable = originalSuperset.views.registerView(...args);
+ collected.push(() => disposable.dispose());
+ return disposable;
+ },
+ },
+ };
Review Comment:
**Suggestion:** Deactivation is advertised as disposing extension
contributions, but registration interception only wraps `views.registerView`;
contributions registered through other namespaces (commands, menus, editors,
etc.) are never collected and therefore never disposed. This leaves stale
contributions active after deactivation and can cause duplicated behavior on
reactivation. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Deactivated extensions leave commands and menus still registered.
- ⚠️ Reactivated extensions can accumulate duplicate commands and menus.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe that extension contributions are registered through multiple
namespaces that
all return Disposables: `registerCommand` in
`superset-frontend/src/core/commands/index.ts:29-49`, `registerMenuItem` in
`superset-frontend/src/core/menus/index.ts:41-53`, and `registerView` in
`superset-frontend/src/core/views/index.ts:4-23` all construct `new
Disposable(() => { ...
})`.
2. Note that the host exposes these namespaces to extensions via
`window.superset` in
`superset-frontend/src/extensions/ExtensionsStartup.tsx:110-125`, so
extension bundles can
call `window.superset.commands.registerCommand`,
`window.superset.menus.registerMenuItem`,
and `window.superset.views.registerView` during their module side effects.
3. In the extension loader, `loadModule()` in
`superset-frontend/src/extensions/ExtensionsLoader.ts:176-201` temporarily
overrides only
`window.superset.views.registerView`, wrapping it to collect `Disposable`
instances into
the local `collected` array; commands, menus, editors, and other namespaces
are passed
through unmodified and their Disposables are never recorded.
4. When an extension is deactivated via `deactivateExtension(id)` in
`superset-frontend/src/extensions/ExtensionsLoader.ts:116-123`, the method
looks up
`extensionDisposables.get(id)` and calls each stored disposer; because
`extensionDisposables` is populated solely from `loadModule()`'s `collected`
array, only
view registrations are torn down, while commands and menu items registered
by the same
extension remain in their registries, leading to stale behavior (and
potential
duplication) once deactivation/reactivation wiring starts calling
`deactivateExtension`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31fcf75d333349e1917ec73e252b771a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=31fcf75d333349e1917ec73e252b771a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/extensions/ExtensionsLoader.ts
**Line:** 180:192
**Comment:**
*Incomplete Implementation: Deactivation is advertised as disposing
extension contributions, but registration interception only wraps
`views.registerView`; contributions registered through other namespaces
(commands, menus, editors, etc.) are never collected and therefore never
disposed. This leaves stale contributions active after deactivation and can
cause duplicated behavior on reactivation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=7b4686e279dba8a4a3d969c1e2aa7bfcf7f79fe83c4fefedb8e24ec80f0a61b0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=7b4686e279dba8a4a3d969c1e2aa7bfcf7f79fe83c4fefedb8e24ec80f0a61b0&reaction=dislike'>👎</a>
##########
superset-frontend/src/views/App.tsx:
##########
@@ -112,6 +113,13 @@ const App = () => (
</Route>
))}
</Switch>
+ {/*
+ The singleton chatbot bubble. Rendered as a sibling of the route
+ Switch — inside ExtensionsStartup so chatbot extensions have been
+ loaded and registered, but outside the Switch so the bubble persists
+ across route changes (SIP §3.2).
+ */}
+ <ChatbotMount />
Review Comment:
**Suggestion:** Mounting the chatbot host unconditionally makes every app
load execute chatbot startup work (including its settings API request) even
when extensions/chatbot functionality is disabled. Gate this mount behind the
relevant feature flag to avoid unnecessary network calls and background work on
non-extension deployments. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Extra `/api/v1/extensions/settings` calls when extensions disabled.
- ⚠️ Unused chatbot subscription work on every application load.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. On application startup, the `App` component in
`superset-frontend/src/views/App.tsx:82-123` renders `<ExtensionsStartup>`
containing the
routes `<Switch>` and, as a sibling, `<ChatbotMount />` at line 122, so
`ChatbotMount`
mounts on every authenticated page load.
2. In `ExtensionsStartup`, extension bundle loading is gated by
`isFeatureEnabled(FeatureFlag.EnableExtensions)` at
`superset-frontend/src/extensions/ExtensionsStartup.tsx:132-134`, but this
flag is not
used around `<ChatbotMount />` in `App.tsx`, so the chatbot host mounts
regardless of
whether extensions are enabled.
3. The `ChatbotMount` component in
`superset-frontend/src/components/ChatbotMount/index.tsx:29-54` runs a
`useEffect` on
mount that always calls `SupersetClient.get({ endpoint:
'/api/v1/extensions/settings' })`,
then updates `adminSelectedId`, `enabledMap`, and `activeChatbot` state
based on the
response, even when no extensions are enabled or configured.
4. When the extensions feature flag is disabled (the same
`FeatureFlag.EnableExtensions`
used to hide the extensions admin route in
`superset-frontend/src/views/routes.tsx:367-372`), users still incur the
`/api/v1/extensions/settings` request and associated state updates on every
app load via
`ChatbotMount`, resulting in unnecessary network traffic and background work
on
deployments that do not use the extensions/chatbot feature.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7c48bc9b72364796bfe90abaa9303b0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7c48bc9b72364796bfe90abaa9303b0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/views/App.tsx
**Line:** 122:122
**Comment:**
*Performance: Mounting the chatbot host unconditionally makes every app
load execute chatbot startup work (including its settings API request) even
when extensions/chatbot functionality is disabled. Gate this mount behind the
relevant feature flag to avoid unnecessary network calls and background work on
non-extension deployments.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=b33204da9e9c2e420747d0a08fbbddfb8115899d42da0a08f6b7fb8f9e691f1c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40441&comment_hash=b33204da9e9c2e420747d0a08fbbddfb8115899d42da0a08f6b7fb8f9e691f1c&reaction=dislike'>👎</a>
--
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]
