codeant-ai-for-open-source[bot] commented on code in PR #41117:
URL: https://github.com/apache/superset/pull/41117#discussion_r3421708787
##########
superset-frontend/src/core/editors/EditorProviders.ts:
##########
@@ -83,15 +47,9 @@ class EditorProviders {
*/
private languageToProvider: Map<EditorLanguage, string> = new Map();
- /**
- * Event emitter for provider registration events.
- */
- private registerEmitter = new EventEmitter<EditorRegisteredEvent>();
+ private registerEmitter = createEventEmitter<EditorRegisteredEvent>();
- /**
- * Event emitter for provider unregistration events.
- */
- private unregisterEmitter = new EventEmitter<EditorUnregisteredEvent>();
+ private unregisterEmitter = createEventEmitter<EditorUnregisteredEvent>();
Review Comment:
**Suggestion:** The new emitter instances keep their own listener sets, but
`reset()` only clears `providers`, `languageToProvider`, and `syncListeners`.
After `reset()`, old register/unregister listeners remain subscribed, so future
registrations can call stale callbacks and retain references unexpectedly.
Extend `reset()` to also clear emitter subscribers (or recreate emitters) so
state is fully reset. [memory leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Editor registration listeners persist across `EditorProviders.reset()`
calls.
- ⚠️ Tests may see events from previous test registrations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. An extension subscribes to editor registration events via
`onDidRegisterEditor` in
`superset-frontend/src/core/editors/index.ts:16-21`, which calls
`EditorProviders.getInstance().onDidRegister(listener)` in
`EditorProviders.ts:187-189`.
2. `EditorProviders.onDidRegister` delegates to
`this.registerEmitter.subscribe(listener)`
(`EditorProviders.ts:187-189`), which uses `createEventEmitter` from
`superset-frontend/src/core/utils.ts:44-54` and stores the bound listener in
an internal
`Set`.
3. A test or consumer resets editor state by calling
`EditorProviders.getInstance().reset()` as intended by the comment in
`EditorProviders.ts:203-205`; this implementation only clears `providers`,
`languageToProvider`, and `syncListeners` (`EditorProviders.ts:206-208`) and
does not
touch `registerEmitter` or `unregisterEmitter`.
4. Subsequent editor registrations through `registerEditor` →
`EditorProviders.registerProvider` (`EditorProviders.ts:89-123`) still call
`this.registerEmitter.fire({ editor })` (`EditorProviders.ts:115-117`),
which delivers
events to the stale listeners retained in the emitter's internal `Set`, so
callbacks from
earlier runs/tests are invoked unexpectedly and accumulate over multiple
resets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eec48b691984411bb793bc4e8cb73c9b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eec48b691984411bb793bc4e8cb73c9b&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/core/editors/EditorProviders.ts
**Line:** 50:52
**Comment:**
*Memory Leak: The new emitter instances keep their own listener sets,
but `reset()` only clears `providers`, `languageToProvider`, and
`syncListeners`. After `reset()`, old register/unregister listeners remain
subscribed, so future registrations can call stale callbacks and retain
references unexpectedly. Extend `reset()` to also clear emitter subscribers (or
recreate emitters) so state is fully reset.
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%2F41117&comment_hash=fe9ab0d52f301edb931e3cb6db2dda7dbee2a419b71b83e4336291961588fd3f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41117&comment_hash=fe9ab0d52f301edb931e3cb6db2dda7dbee2a419b71b83e4336291961588fd3f&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/views/index.ts:
##########
@@ -116,17 +117,11 @@ export const useViews = (location: string): View[] |
undefined =>
export const onDidRegisterView: typeof viewsApi.onDidRegisterView = (
listener: (e: ViewRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
Review Comment:
**Suggestion:** This registration event adapter no longer forwards optional
`thisArgs`, so listeners using method-style callbacks with context will execute
with incorrect `this`. Include `thisArgs` in the function signature and pass it
into `registerEmitter.subscribe`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ View registration listeners lose expected `this` binding.
- ⚠️ Extensions reacting to view registration may mis-handle state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The views API exposes `onDidRegisterView` as `Event<ViewRegisteredEvent>`
in
`superset-frontend/packages/superset-core/src/views/index.ts:25-28`,
following the
`Event<T>` contract `(listener: (e: T) => any, thisArgs?: any) =>
Disposable` defined in
`packages/superset-core/src/common/index.ts:194-26`.
2. The frontend implementation at
`superset-frontend/src/core/views/index.ts:118-120`
defines `onDidRegisterView` as a function that only accepts `listener` and
calls
`registerEmitter.subscribe(listener)`, omitting the optional `thisArgs`
parameter.
3. An extension subscribes using
`views.onDidRegisterView(this.handleViewRegistered,
this)` expecting `handleViewRegistered` to run with `this` bound to the
extension instance
when new views are registered.
4. Since the adapter discards the second argument, `createEventEmitter`'s
`subscribe`
implementation (`superset-frontend/src/core/utils.ts:44-50`) receives
`thisArgs` as
`undefined` and does not bind the desired context, so `this` inside
`handleViewRegistered`
is incorrect and any context-based logic for newly registered views
misbehaves.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6cb95d15104c4a338e78d9eea78c4408&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6cb95d15104c4a338e78d9eea78c4408&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/core/views/index.ts
**Line:** 120:120
**Comment:**
*Api Mismatch: This registration event adapter no longer forwards
optional `thisArgs`, so listeners using method-style callbacks with context
will execute with incorrect `this`. Include `thisArgs` in the function
signature and pass it into `registerEmitter.subscribe`.
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%2F41117&comment_hash=735fe98d714b8940c05e2899d521d1c42fb78b7deab89304597862e340f38603&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41117&comment_hash=735fe98d714b8940c05e2899d521d1c42fb78b7deab89304597862e340f38603&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/menus/index.ts:
##########
@@ -117,16 +118,11 @@ export const useMenu = (location: string): Menu |
undefined =>
export const onDidRegisterMenuItem: typeof menusApi.onDidRegisterMenuItem = (
listener: (e: MenuItemRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
export const onDidUnregisterMenuItem: typeof menusApi.onDidUnregisterMenuItem =
- (listener: (e: MenuItemUnregisteredEvent) => void): Disposable => {
- unregisterListeners.add(listener);
- return new Disposable(() => unregisterListeners.delete(listener));
- };
+ (listener: (e: MenuItemUnregisteredEvent) => void): Disposable =>
+ unregisterEmitter.subscribe(listener);
Review Comment:
**Suggestion:** The unregister event wrapper also ignores the optional
`thisArgs` parameter defined by the `Event<T>` API, which breaks listeners that
rely on bound context. Add `thisArgs` and pass it through to
`unregisterEmitter.subscribe`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Menu unregistration listeners execute with incorrect `this` binding.
- ⚠️ Extensions may fail cleanup when menu items are removed.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public API declares `onDidUnregisterMenuItem` as
`Event<MenuItemUnregisteredEvent>`
in `superset-frontend/packages/superset-core/src/menus/index.ts:36-39`,
again using the
`Event<T>` signature `(listener: (e: T) => any, thisArgs?: any) =>
Disposable` from
`packages/superset-core/src/common/index.ts:194-26`.
2. The core adapter defines `onDidUnregisterMenuItem` in
`superset-frontend/src/core/menus/index.ts:123-125` as a wrapper `(listener)
=>
unregisterEmitter.subscribe(listener)`, which ignores any second `thisArgs`
argument
passed by callers.
3. An extension subscribes with a method-style callback, e.g.
`menus.onDidUnregisterMenuItem(this.onRemoved, this)`, expecting
`this.onRemoved` to run
with `this` bound to the extension instance when menu items are unregistered.
4. Because the adapter never forwards the second argument,
`unregisterEmitter.subscribe`
from `createEventEmitter` (`superset-frontend/src/core/utils.ts:44-50`) is
always called
without `thisArgs`, leading to callbacks bound with the wrong `this` and
potentially
preventing correct cleanup or bookkeeping when menu items are removed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=965b88198dfc4b72afed4c3da14588e7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=965b88198dfc4b72afed4c3da14588e7&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/core/menus/index.ts
**Line:** 123:125
**Comment:**
*Api Mismatch: The unregister event wrapper also ignores the optional
`thisArgs` parameter defined by the `Event<T>` API, which breaks listeners that
rely on bound context. Add `thisArgs` and pass it through to
`unregisterEmitter.subscribe`.
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%2F41117&comment_hash=a0d1ecbad2e956be60e70d1337c2789ab13d41f794f9dccc5a3b8ef26ef5b171&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41117&comment_hash=a0d1ecbad2e956be60e70d1337c2789ab13d41f794f9dccc5a3b8ef26ef5b171&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/menus/index.ts:
##########
@@ -117,16 +118,11 @@ export const useMenu = (location: string): Menu |
undefined =>
export const onDidRegisterMenuItem: typeof menusApi.onDidRegisterMenuItem = (
listener: (e: MenuItemRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
Review Comment:
**Suggestion:** This handler drops the optional `thisArgs` argument from the
`Event<T>` contract, so callbacks registered with a context object will run
with the wrong `this` binding. Accept and forward `thisArgs` to
`registerEmitter.subscribe` to preserve the public API behavior. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Menu registration listeners cannot rely on bound context.
- ⚠️ Extension callbacks may read or mutate wrong instance state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public API defines `onDidRegisterMenuItem` as
`Event<MenuItemRegisteredEvent>` in
`superset-frontend/packages/superset-core/src/menus/index.ts:31-35`, and
`Event<T>` is
specified in `packages/superset-core/src/common/index.ts:194-26` as a
callable `(listener:
(e: T) => any, thisArgs?: any) => Disposable`.
2. The frontend adapter implements `onDidRegisterMenuItem` in
`superset-frontend/src/core/menus/index.ts:119-121` as a function taking
only `listener`
and returning `registerEmitter.subscribe(listener)`, dropping the optional
`thisArgs`
parameter from the contract.
3. An extension built against `@apache-superset/core` calls
`menus.onDidRegisterMenuItem(handler, someContext)` expecting `handler` to
be invoked with
`this === someContext`, which TypeScript allows because the adapter is typed
as `typeof
menusApi.onDidRegisterMenuItem`.
4. At runtime, the adapter never forwards `someContext` to
`registerEmitter.subscribe`, so
`createEventEmitter`'s `subscribe` implementation in
`superset-frontend/src/core/utils.ts:44-50` receives `thisArgs` as
`undefined` and binds
`handler` without context, causing `this` inside the callback to be
incorrect and breaking
any context-dependent menu registration logic.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f07505b31a4543c39459ac969081a77a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f07505b31a4543c39459ac969081a77a&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/core/menus/index.ts
**Line:** 121:121
**Comment:**
*Api Mismatch: This handler drops the optional `thisArgs` argument from
the `Event<T>` contract, so callbacks registered with a context object will run
with the wrong `this` binding. Accept and forward `thisArgs` to
`registerEmitter.subscribe` to preserve the public API behavior.
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%2F41117&comment_hash=f1b83e9aebe038beca75d500424b9b79777e38a6e7ce5eb7abae989376a8b59a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41117&comment_hash=f1b83e9aebe038beca75d500424b9b79777e38a6e7ce5eb7abae989376a8b59a&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/views/index.ts:
##########
@@ -116,17 +117,11 @@ export const useViews = (location: string): View[] |
undefined =>
export const onDidRegisterView: typeof viewsApi.onDidRegisterView = (
listener: (e: ViewRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
export const onDidUnregisterView: typeof viewsApi.onDidUnregisterView = (
listener: (e: ViewUnregisteredEvent) => void,
-): Disposable => {
- unregisterListeners.add(listener);
- return new Disposable(() => unregisterListeners.delete(listener));
-};
+): Disposable => unregisterEmitter.subscribe(listener);
Review Comment:
**Suggestion:** The unregister event wrapper omits the optional context
argument required by the event contract, causing `this` binding regressions for
consumers that pass `thisArgs`. Forward `thisArgs` to
`unregisterEmitter.subscribe`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ View unregistration listeners run with incorrect `this` context.
- ⚠️ Extensions may leak resources during view teardown.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The views core package declares `onDidUnregisterView` as
`Event<ViewUnregisteredEvent>`
in `superset-frontend/packages/superset-core/src/views/index.ts:30-33`,
using the same
`Event<T>` signature with optional `thisArgs` from
`packages/superset-core/src/common/index.ts:194-26`.
2. The adapter in `superset-frontend/src/core/views/index.ts:122-124`
implements
`onDidUnregisterView` as `(listener) =>
unregisterEmitter.subscribe(listener)`, so any
provided `thisArgs` parameter is dropped.
3. An extension subscribes via
`views.onDidUnregisterView(this.handleViewRemoved, this)`
to clean up resources when its views are unregistered, relying on `this`
being bound to
the extension instance.
4. Because the adapter never forwards the `thisArgs` argument,
`unregisterEmitter.subscribe` (from `createEventEmitter` in
`superset-frontend/src/core/utils.ts:44-50`) binds the listener without the
intended
context, leading to incorrect `this` inside `handleViewRemoved` and
potentially leaking
resources or leaving stale state when views are unregistered.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b2ddeb95135e4afab24c0d056e0ad4d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b2ddeb95135e4afab24c0d056e0ad4d3&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/core/views/index.ts
**Line:** 122:124
**Comment:**
*Api Mismatch: The unregister event wrapper omits the optional context
argument required by the event contract, causing `this` binding regressions for
consumers that pass `thisArgs`. Forward `thisArgs` to
`unregisterEmitter.subscribe`.
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%2F41117&comment_hash=9f0e2d70a46f438148cbe2f263a7813e81218543ef469c7d410bd9218b43f0ae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41117&comment_hash=9f0e2d70a46f438148cbe2f263a7813e81218543ef469c7d410bd9218b43f0ae&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]
