codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437435592
##########
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 wrapper drops the optional `thisArgs` parameter from
the `Event` contract, so listeners that rely on context binding will run with
the wrong `this` value. 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 `this` context.
- ⚠️ Extension APIs diverge from documented `Event` type behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the public event type in
`superset-frontend/packages/superset-core/src/common/index.ts:190-24`, where
`Event<T>` is
declared as a callable `(listener: (e: T) => any, thisArgs?: any):
Disposable`, explicitly
supporting an optional `thisArgs` context parameter.
2. Observe the menus API declaration in
`superset-frontend/packages/superset-core/src/menus/index.ts:133-138`, where
`onDidRegisterMenuItem` is declared as `export declare const
onDidRegisterMenuItem:
Event<MenuItemRegisteredEvent>;`, so extension authors are allowed to call
`menus.onDidRegisterMenuItem(listener, thisArgs)`.
3. Inspect the host implementation in
`superset-frontend/src/core/menus/index.ts:119-121`,
where `onDidRegisterMenuItem` is implemented as `(listener: (e:
MenuItemRegisteredEvent)
=> void): Disposable => registerEmitter.subscribe(listener);` — the wrapper
only accepts
`listener` and forwards a single argument to `registerEmitter.subscribe`,
silently
dropping any `thisArgs` passed by the caller.
4. Follow the runtime path: when an extension calls
`menus.registerMenuItem(...)`
(implementation at `superset-frontend/src/core/menus/index.ts:66-80`),
`notifyRegister` at
lines 55-59 fires `registerEmitter.fire(event)`, which in turn invokes all
listeners
registered via `onDidRegisterMenuItem`; because `thisArgs` was never
forwarded, any
extension code that relied on `this` (e.g.,
`menus.onDidRegisterMenuItem(function (e) {
this.handleMenu(e); }, someContext)`) will see `this === undefined` instead
of
`someContext`, violating the `Event` contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=051a0d23ab034c97b5a93849ab58d1da&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=051a0d23ab034c97b5a93849ab58d1da&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:** 119:121
**Comment:**
*Api Mismatch: This wrapper drops the optional `thisArgs` parameter
from the `Event` contract, so listeners that rely on context binding will run
with the wrong `this` value. 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%2F41205&comment_hash=f898a30599758728c272218f9f673621302c82ac31d06e4e262ed35499e54673&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=f898a30599758728c272218f9f673621302c82ac31d06e4e262ed35499e54673&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:** This unregistration event wrapper also omits the optional
`thisArgs` argument, which makes callback context binding inconsistent with the
`Event` type contract and with other APIs that pass through context. Accept and
forward `thisArgs` here as well. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Menu unregistration listeners lose their intended `this` context.
- ⚠️ Extensions observing menu cleanup behave incorrectly on dispose.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. As in suggestion 1, the shared `Event<T>` type in
`superset-frontend/packages/superset-core/src/common/index.ts:190-24`
defines a callable
`(listener: (e: T) => any, thisArgs?: any): Disposable`, and
`superset-frontend/packages/superset-core/src/menus/index.ts:133-138`
declares
`onDidUnregisterMenuItem` as `Event<MenuItemUnregisteredEvent>`, so
consumers may
legitimately call `menus.onDidUnregisterMenuItem(listener, thisArgs)`.
2. In the host implementation at
`superset-frontend/src/core/menus/index.ts:123-125`,
`onDidUnregisterMenuItem` is defined as `(listener: (e:
MenuItemUnregisteredEvent) =>
void): Disposable => unregisterEmitter.subscribe(listener);`, again only
accepting
`listener` and passing a single argument into `unregisterEmitter.subscribe`,
discarding
any supplied `thisArgs`.
3. When a menu item is registered via `registerMenuItem`
(`superset-frontend/src/core/menus/index.ts:66-80`), the returned
`Disposable`'s `dispose`
function removes the item and calls `notifyUnregister` at lines 60-63, which
fires
`unregisterEmitter.fire(event)` to notify all `onDidUnregisterMenuItem`
listeners.
4. If an extension subscribes with context, e.g.
`menus.onDidUnregisterMenuItem(function
(e) { this.onMenuRemoved(e); }, myContext)`, the call goes through the
wrapper in
`src/core/menus/index.ts:123-125` and into `createEventEmitter` in
`src/core/utils.ts:44-50` without the `thisArgs` argument; as a result, the
listener is
stored unbound and will be invoked with `this === undefined` when a menu is
unregistered,
breaking the expected `Event` behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f46056837e284c839d487bafaf2a9687&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=f46056837e284c839d487bafaf2a9687&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: This unregistration event wrapper also omits the
optional `thisArgs` argument, which makes callback context binding inconsistent
with the `Event` type contract and with other APIs that pass through context.
Accept and forward `thisArgs` here as well.
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%2F41205&comment_hash=87e360d7c8eb1ddbacfeeaabc25689d761bbd345651623ee2003c4e1025183dd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=87e360d7c8eb1ddbacfeeaabc25689d761bbd345651623ee2003c4e1025183dd&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]
