codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437440708
##########
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:** These emitter instances are long-lived singleton fields, but
`reset()` does not clear or recreate them, so listeners registered in prior
test/runtime phases remain retained and can fire unexpectedly later.
Reinitialize emitters (or add clear support) during reset to avoid stale
listener retention. [memory leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ EditorProviders tests can leak listeners between cases.
- ⚠️ Future tests using registration events risk flaky behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/core/utils.ts:44-54`, `createEventEmitter<T>()`
creates a
closure over a `listeners` `Set` and returns `{ fire, subscribe }`, with
`subscribe`
adding listeners to that `Set` but no way to clear it.
2. In `superset-frontend/src/core/editors/EditorProviders.ts:50-52`,
`EditorProviders`
constructs `registerEmitter` and `unregisterEmitter` once as instance fields
using
`createEventEmitter`, so their internal `listeners` sets are long-lived for
the lifetime
of the singleton.
3. In the same file, `onDidRegister()` and `onDidUnregister()` at
`EditorProviders.ts:187-199` invoke
`this.registerEmitter.subscribe(listener)` /
`this.unregisterEmitter.subscribe(listener)` and return the resulting
`Disposable`, but
`reset()` at `EditorProviders.ts:205-209` only clears `providers`,
`languageToProvider`,
and `syncListeners` — it does not recreate the emitters or clear their
internal
`listeners` sets.
4. As shown in
`superset-frontend/src/core/editors/EditorProviders.test.ts:7-11`, tests
call `EditorProviders.getInstance().reset()` in `beforeEach`; if a test
registers
listeners via `onDidRegister()` or `onDidUnregister()`, those listeners will
remain in the
emitter's internal `Set` after `reset()`, and subsequent tests that register
providers
(calling `registerProvider()` at `EditorProviders.ts:89-123`, which fires
`registerEmitter.fire(...)` at line 116) will still invoke the stale
listeners,
demonstrating the listener leak across resets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=247bf606834e4d189ba95038a43e15d3&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=247bf606834e4d189ba95038a43e15d3&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: These emitter instances are long-lived singleton fields,
but `reset()` does not clear or recreate them, so listeners registered in prior
test/runtime phases remain retained and can fire unexpectedly later.
Reinitialize emitters (or add clear support) during reset to avoid stale
listener retention.
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=e2680bfb015b270190bbf1a849f9ad779a16198f826b38e4d4898035f311f828&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=e2680bfb015b270190bbf1a849f9ad779a16198f826b38e4d4898035f311f828&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/editors/index.ts:
##########
@@ -18,130 +18,39 @@
*/
/**
- * @fileoverview Implementation of the editors API for Superset.
+ * @fileoverview Host implementation of the `editors` contribution type.
*
- * This module provides the runtime implementation of the editor registration
- * and resolution functions declared in the API types.
+ * Extensions register via the public `editors.registerEditor()` and the host
+ * resolves the appropriate provider per language, falling back to the built-in
+ * AceEditorProvider when no extension is registered.
+ *
+ * The public namespace (`editors`) is exposed to extensions on
`window.superset`.
+ * `EditorHost` is the host-internal component for rendering editors and is NOT
+ * part of the public `@apache-superset/core` API.
*/
import { useSyncExternalStore } from 'react';
import { editors as editorsApi } from '@apache-superset/core';
-import { Disposable } from '../models';
import EditorProviders from './EditorProviders';
-type EditorLanguage = editorsApi.EditorLanguage;
-type Editor = editorsApi.Editor;
-type EditorProvider = editorsApi.EditorProvider;
-type EditorComponent = editorsApi.EditorComponent;
-type EditorRegisteredEvent = editorsApi.EditorRegisteredEvent;
-type EditorUnregisteredEvent = editorsApi.EditorUnregisteredEvent;
-
-/**
- * Register an editor provider as a module-level side effect.
- * Takes the editor descriptor directly rather than looking it up
- * from a manifest by ID.
- *
- * @param editor The editor descriptor.
- * @param component The React component implementing the editor.
- * @returns A Disposable to unregister the provider.
- */
-export const registerEditor = (
- editor: Editor,
- component: EditorComponent,
-): Disposable => {
- const providers = EditorProviders.getInstance();
- return providers.registerProvider(editor, component);
-};
-
-/**
- * Get the editor provider for a specific language.
- * Returns the extension's editor if registered, otherwise undefined.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const getEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return manager.getProvider(language);
-};
-
-/**
- * Check if an extension has registered an editor for a language.
- *
- * @param language The language to check
- * @returns True if an extension provides an editor for this language
- */
-export const hasEditor = (language: EditorLanguage): boolean => {
- const manager = EditorProviders.getInstance();
- return manager.hasProvider(language);
-};
-
-/**
- * Get all registered editor providers.
- *
- * @returns Array of all registered editor providers
- */
-export const getAllEditors = (): EditorProvider[] => {
- const manager = EditorProviders.getInstance();
- return manager.getAllProviders();
-};
-
-/**
- * Event fired when an editor is registered.
- * Subscribe to this event to react when extensions register new editors.
- */
-export const onDidRegisterEditor = (
- listener: (e: EditorRegisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidRegister(listener);
-};
+export type { EditorHostProps } from './EditorHost';
+export { default as EditorHost } from './EditorHost';
+export { default as AceEditorProvider } from './AceEditorProvider';
-/**
- * Event fired when an editor is unregistered.
- * Subscribe to this event to react when extensions unregister editors.
- */
-export const onDidUnregisterEditor = (
- listener: (e: EditorUnregisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidUnregister(listener);
-};
+const provider = EditorProviders.getInstance();
-/**
- * Hook that returns the editor provider for a specific language and
re-renders when it changes.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const useEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return useSyncExternalStore(
- manager.subscribe,
- () => manager.getProvider(language),
+export const useEditor = (language: editorsApi.EditorLanguage) =>
+ useSyncExternalStore(
+ provider.subscribe,
+ () => provider.getProvider(language),
() => undefined,
);
-};
-/**
- * Editors API object for use in the extension system.
- */
export const editors: typeof editorsApi = {
- registerEditor,
- getEditor,
- hasEditor,
- getAllEditors,
- onDidRegisterEditor,
- onDidUnregisterEditor,
+ registerEditor: provider.registerProvider.bind(provider),
+ getEditor: provider.getProvider.bind(provider),
+ hasEditor: provider.hasProvider.bind(provider),
+ getAllEditors: provider.getAllProviders.bind(provider),
+ onDidRegisterEditor: provider.onDidRegister.bind(provider),
+ onDidUnregisterEditor: provider.onDidUnregister.bind(provider),
Review Comment:
**Suggestion:** This has the same contract break for unregister events: the
optional `thisArgs` from the `Event` API is ignored, so context-bound listeners
won't behave correctly. Add a wrapper that accepts and forwards both listener
and context. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions listening for editor unregistration lose bound context.
- ⚠️ Breaks symmetry with other Event<T>-based APIs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public API in
`superset-frontend/packages/superset-core/src/editors/index.ts:618-621`
declares
`onDidUnregisterEditor` as `Event<EditorUnregisteredEvent>`, where
`Event<T>` (from
`packages/superset-core/src/common/index.ts:35-40,14-15`) supports an
optional `thisArgs`
parameter for binding listener context.
2. The host implementation in
`superset-frontend/src/core/editors/index.ts:49-56` wires
`onDidUnregisterEditor` as `provider.onDidUnregister.bind(provider)`,
mirroring the
register event wiring.
3. Inside `superset-frontend/src/core/editors/EditorProviders.ts:192-199`,
`onDidUnregister` is defined as `public onDidUnregister(listener:
Listener<EditorUnregisteredEvent>): Disposable { return
this.unregisterEmitter.subscribe(listener); }`, only accepting `listener`
and not
propagating any `thisArgs` value to `subscribe`.
4. If extension code subscribes using
`editors.onDidUnregisterEditor(function (e) {
this.handleCleanup(e); }, myContext)`, the implementation discards
`myContext` and
`createEventEmitter.subscribe` (see `src/core/utils.ts:44-49`) binds the raw
listener
without a `this` context; when `unregisterProvider()` fires the event at
`EditorProviders.ts:148`, `this` inside the listener is `undefined`,
breaking any listener
that expects the documented context binding.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48d9db4e30864b8ba132d86577a27d7e&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=48d9db4e30864b8ba132d86577a27d7e&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/index.ts
**Line:** 55:55
**Comment:**
*Api Mismatch: This has the same contract break for unregister events:
the optional `thisArgs` from the `Event` API is ignored, so context-bound
listeners won't behave correctly. Add a wrapper that accepts and forwards both
listener and context.
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=2c4429d957254e8509fc410fc2a6c8453e62400938112751a4dc31e1520d22f6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=2c4429d957254e8509fc410fc2a6c8453e62400938112751a4dc31e1520d22f6&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/editors/index.ts:
##########
@@ -18,130 +18,39 @@
*/
/**
- * @fileoverview Implementation of the editors API for Superset.
+ * @fileoverview Host implementation of the `editors` contribution type.
*
- * This module provides the runtime implementation of the editor registration
- * and resolution functions declared in the API types.
+ * Extensions register via the public `editors.registerEditor()` and the host
+ * resolves the appropriate provider per language, falling back to the built-in
+ * AceEditorProvider when no extension is registered.
+ *
+ * The public namespace (`editors`) is exposed to extensions on
`window.superset`.
+ * `EditorHost` is the host-internal component for rendering editors and is NOT
+ * part of the public `@apache-superset/core` API.
*/
import { useSyncExternalStore } from 'react';
import { editors as editorsApi } from '@apache-superset/core';
-import { Disposable } from '../models';
import EditorProviders from './EditorProviders';
-type EditorLanguage = editorsApi.EditorLanguage;
-type Editor = editorsApi.Editor;
-type EditorProvider = editorsApi.EditorProvider;
-type EditorComponent = editorsApi.EditorComponent;
-type EditorRegisteredEvent = editorsApi.EditorRegisteredEvent;
-type EditorUnregisteredEvent = editorsApi.EditorUnregisteredEvent;
-
-/**
- * Register an editor provider as a module-level side effect.
- * Takes the editor descriptor directly rather than looking it up
- * from a manifest by ID.
- *
- * @param editor The editor descriptor.
- * @param component The React component implementing the editor.
- * @returns A Disposable to unregister the provider.
- */
-export const registerEditor = (
- editor: Editor,
- component: EditorComponent,
-): Disposable => {
- const providers = EditorProviders.getInstance();
- return providers.registerProvider(editor, component);
-};
-
-/**
- * Get the editor provider for a specific language.
- * Returns the extension's editor if registered, otherwise undefined.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const getEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return manager.getProvider(language);
-};
-
-/**
- * Check if an extension has registered an editor for a language.
- *
- * @param language The language to check
- * @returns True if an extension provides an editor for this language
- */
-export const hasEditor = (language: EditorLanguage): boolean => {
- const manager = EditorProviders.getInstance();
- return manager.hasProvider(language);
-};
-
-/**
- * Get all registered editor providers.
- *
- * @returns Array of all registered editor providers
- */
-export const getAllEditors = (): EditorProvider[] => {
- const manager = EditorProviders.getInstance();
- return manager.getAllProviders();
-};
-
-/**
- * Event fired when an editor is registered.
- * Subscribe to this event to react when extensions register new editors.
- */
-export const onDidRegisterEditor = (
- listener: (e: EditorRegisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidRegister(listener);
-};
+export type { EditorHostProps } from './EditorHost';
+export { default as EditorHost } from './EditorHost';
+export { default as AceEditorProvider } from './AceEditorProvider';
-/**
- * Event fired when an editor is unregistered.
- * Subscribe to this event to react when extensions unregister editors.
- */
-export const onDidUnregisterEditor = (
- listener: (e: EditorUnregisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidUnregister(listener);
-};
+const provider = EditorProviders.getInstance();
-/**
- * Hook that returns the editor provider for a specific language and
re-renders when it changes.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const useEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return useSyncExternalStore(
- manager.subscribe,
- () => manager.getProvider(language),
+export const useEditor = (language: editorsApi.EditorLanguage) =>
+ useSyncExternalStore(
+ provider.subscribe,
+ () => provider.getProvider(language),
() => undefined,
);
-};
-/**
- * Editors API object for use in the extension system.
- */
export const editors: typeof editorsApi = {
- registerEditor,
- getEditor,
- hasEditor,
- getAllEditors,
- onDidRegisterEditor,
- onDidUnregisterEditor,
+ registerEditor: provider.registerProvider.bind(provider),
+ getEditor: provider.getProvider.bind(provider),
+ hasEditor: provider.hasProvider.bind(provider),
+ getAllEditors: provider.getAllProviders.bind(provider),
+ onDidRegisterEditor: provider.onDidRegister.bind(provider),
Review Comment:
**Suggestion:** This wiring drops the `thisArgs` part of the `Event`
contract because `onDidRegister` only forwards the listener and ignores the
optional context argument. Callers using `editors.onDidRegisterEditor(listener,
ctx)` will get an unbound callback and `this`-dependent listeners will break.
Expose a wrapper matching the full `Event` signature and forward `thisArgs`.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions using thisArgs on editor registration will break.
- ⚠️ Violates Event<T> contract advertised in superset-core.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public editors API in
`superset-frontend/packages/superset-core/src/editors/index.ts:610-617`
declares
`onDidRegisterEditor` as `Event<EditorRegisteredEvent>`, and the `Event<T>`
type in
`superset-frontend/packages/superset-core/src/common/index.ts:35-40,14-15`
explicitly has
signature `(listener: (e: T) => any, thisArgs?: any): Disposable`,
supporting an optional
`thisArgs` context.
2. The host implementation wires this in
`superset-frontend/src/core/editors/index.ts:49-56` as `export const
editors: typeof
editorsApi = { ..., onDidRegisterEditor:
provider.onDidRegister.bind(provider), ... }`,
where `provider` is the singleton from `EditorProviders.getInstance()` at
line 40.
3. `EditorProviders.onDidRegister` in
`superset-frontend/src/core/editors/EditorProviders.ts:187-189` is defined
as `public
onDidRegister(listener: Listener<EditorRegisteredEvent>): Disposable { return
this.registerEmitter.subscribe(listener); }`, which only accepts `listener`
and does not
accept or forward the optional `thisArgs` parameter.
4. An extension using the documented API via `window.superset.editors`
(populated in
`superset-frontend/src/extensions/ExtensionsStartup.tsx:38-54`) can
legitimately call
`editors.onDidRegisterEditor(function (e) { this.handle(e); }, myContext)`,
but because
the implementation is the bound method `provider.onDidRegister` that ignores
the second
argument, `createEventEmitter.subscribe` (in `src/core/utils.ts:44-49`)
never receives
`thisArgs`, so the listener is invoked with `this === undefined` when
`registerProvider()`
fires the event at `EditorProviders.ts:116`. Any listener relying on `this`
will then
throw (e.g., `TypeError: Cannot read properties of undefined`), violating
the `Event`
contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83e9d5506a894404838c1940202a8b48&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=83e9d5506a894404838c1940202a8b48&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/index.ts
**Line:** 54:54
**Comment:**
*Api Mismatch: This wiring drops the `thisArgs` part of the `Event`
contract because `onDidRegister` only forwards the listener and ignores the
optional context argument. Callers using `editors.onDidRegisterEditor(listener,
ctx)` will get an unbound callback and `this`-dependent listeners will break.
Expose a wrapper matching the full `Event` signature and forward `thisArgs`.
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=098e7e96eb55ae6c8ebc5efa0b4953602384932f4ecac32b354b933b02c05cbb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=098e7e96eb55ae6c8ebc5efa0b4953602384932f4ecac32b354b933b02c05cbb&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]
