bito-code-review[bot] commented on code in PR #38346:
URL: https://github.com/apache/superset/pull/38346#discussion_r2873754422
##########
superset-frontend/src/components/ViewListExtension/ViewListExtension.test.tsx:
##########
@@ -16,88 +16,34 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ReactElement } from 'react';
+import React, { ReactElement } from 'react';
import { render, screen } from 'spec/helpers/testing-library';
-import type { contributions, core } from '@apache-superset/core';
-import ExtensionsManager from 'src/extensions/ExtensionsManager';
+import * as ExtensionsContextUtils from
'src/extensions/ExtensionsContextUtils';
+import { views } from 'src/core';
import { ExtensionsProvider } from 'src/extensions/ExtensionsContext';
import ViewListExtension from '.';
-function createMockView(
- id: string,
- overrides: Partial<contributions.ViewContribution> = {},
-): contributions.ViewContribution {
- return {
- id,
- name: `${id} View`,
- ...overrides,
- };
-}
-
-function createMockExtension(
- options: Partial<core.Extension> & {
- views?: Record<string, Record<string, contributions.ViewContribution[]>>;
- } = {},
-): core.Extension {
- const {
- id = 'test-extension',
- name = 'Test Extension',
- views = {},
- } = options;
-
- return {
- id,
- name,
- description: 'A test extension',
- version: '1.0.0',
- dependencies: [],
- remoteEntry: '',
- exposedModules: [],
- extensionDependencies: [],
- contributions: {
- commands: [],
- menus: {},
- views,
- },
- activate: jest.fn(),
- deactivate: jest.fn(),
- };
-}
-
-function setupActivatedExtension(
- manager: ExtensionsManager,
- extension: core.Extension,
-) {
- const context = { disposables: [] };
- (manager as any).contextIndex.set(extension.id, context);
- (manager as any).extensionContributions.set(extension.id, {
- commands: extension.contributions.commands,
- menus: extension.contributions.menus,
- views: extension.contributions.views,
+const mockRegisterViewProvider = jest.fn();
+const mockUnregisterViewProvider = jest.fn();
+
+jest
+ .spyOn(ExtensionsContextUtils, 'getExtensionsContextValue')
+ .mockReturnValue({
+ registerViewProvider: mockRegisterViewProvider,
+ unregisterViewProvider: mockUnregisterViewProvider,
+ getView: jest.fn(),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken test mocking causes render failure</b></div>
<div id="fix">
The test mocks getView as jest.fn() without implementation, causing it to
return undefined. The component renders getView(view.id), which fails since
undefined is not a valid React element. Mock getView to return identifiable
elements like <div>{id}</div> to verify the correct views are rendered.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
getView: jest.fn((id) => React.createElement('div', null, id)),
````
</div>
</details>
</div>
<small><i>Code Review Run #22288d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/commands/index.ts:
##########
@@ -50,15 +60,20 @@ const executeCommand: typeof commandsApi.executeCommand =
async <T>(
return callback(...args) as T;
};
-const getCommands: typeof commandsApi.getCommands = filterInternal => {
- const commands = Array.from(commandRegistry.keys());
- return Promise.resolve(
- filterInternal ? commands.filter(cmd => !cmd.startsWith('_')) : commands,
- );
+const getCommands: typeof commandsApi.getCommands = (): Command[] =>
+ Array.from(commandsMap.values());
+
+const getCommand: typeof commandsApi.getCommand = (
+ id: string,
+): Command | undefined => commandsMap.get(id);
+
+export const resetContributions = (): void => {
+ commandsMap.clear();
};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent state in resetContributions</b></div>
<div id="fix">
The resetContributions function only clears commandsMap but not
commandRegistry, which could lead to inconsistent state. If resetContributions
is called, getCommands will return an empty array, but executeCommand might
still work for previously registered commands since commandRegistry retains
entries. This inconsistency may cause confusion in testing or cleanup scenarios.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
export const resetContributions = (): void => {
commandsMap.clear();
commandRegistry.clear();
};
````
</div>
</details>
</div>
<small><i>Code Review Run #22288d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/editors/index.ts:
##########
@@ -24,45 +24,33 @@
* and resolution functions declared in the API types.
*/
-import type { contributions } from '@apache-superset/core';
import { editors as editorsApi } from '@apache-superset/core';
-import ExtensionsManager from 'src/extensions/ExtensionsManager';
import { Disposable } from '../models';
import EditorProviders from './EditorProviders';
-type EditorLanguage = contributions.EditorLanguage;
+type EditorLanguage = editorsApi.EditorLanguage;
+type Editor = editorsApi.Editor;
type EditorProvider = editorsApi.EditorProvider;
type EditorComponent = editorsApi.EditorComponent;
type EditorProviderRegisteredEvent = editorsApi.EditorProviderRegisteredEvent;
type EditorProviderUnregisteredEvent =
editorsApi.EditorProviderUnregisteredEvent;
/**
- * Register an editor provider for specific languages.
- * The contribution metadata is resolved from the extension manifest by ID.
+ * 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 id The editor contribution ID declared in extension.json
- * @param component The React component implementing EditorProps
- * @returns A Disposable to unregister the provider
+ * @param editor The editor descriptor.
+ * @param component The React component implementing the editor.
+ * @returns A Disposable to unregister the provider.
*/
-export const registerEditorProvider = (
- id: string,
+export const registerEditor = (
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>API Breaking Change</b></div>
<div id="fix">
The API has changed from `registerEditorProvider(id, component)` to
`registerEditor(editor, component)`, but the documentation still shows the old
usage. This breaks the documented contract for extension developers and may
cause runtime errors if extensions follow the docs.
</div>
</div>
<small><i>Code Review Run #22288d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/ViewListExtension/ViewListExtension.test.tsx:
##########
@@ -149,46 +92,38 @@ test('renders nothing for viewId with no matching
contributions', () => {
expect(container.firstChild?.childNodes.length ?? 0).toBe(0);
});
-test('handles multiple extensions with views for same viewId', async () => {
- const manager = ExtensionsManager.getInstance();
-
- await createActivatedExtension(manager, {
- id: 'extension-1',
- views: {
- test: {
- view: [createMockView('ext1-view')],
- },
- },
- });
-
- await createActivatedExtension(manager, {
- id: 'extension-2',
- views: {
- test: {
- view: [createMockView('ext2-view')],
- },
- },
- });
+test('handles multiple views registered at the same location', () => {
+ views.registerView(
+ { id: 'ext1-view', name: 'ext1-view View' },
+ TEST_VIEW_ID,
+ dummyProvider,
+ );
+ views.registerView(
+ { id: 'ext2-view', name: 'ext2-view View' },
+ TEST_VIEW_ID,
+ dummyProvider,
+ );
renderWithExtensionsProvider(<ViewListExtension viewId={TEST_VIEW_ID} />);
expect(screen.getByText(/ext1-view/)).toBeInTheDocument();
expect(screen.getByText(/ext2-view/)).toBeInTheDocument();
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertions mismatch rendered content</b></div>
<div id="fix">
The test registers two views with the same dummyProvider, which renders
'placeholder' for both, but expects specific view IDs ('ext1-view' and
'ext2-view') in the rendered text. This mismatch causes the test to fail since
the IDs are not present in the output. To fix, create separate providers that
render the view IDs, e.g., const provider1 = () => <div>ext1-view</div>; and
use them in registerView calls.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset-frontend/src/components/ViewListExtension/ViewListExtension.test.tsx
+++
superset-frontend/src/components/ViewListExtension/ViewListExtension.test.tsx
@@ -37,7 +37,9 @@ const TEST_VIEW_ID = 'test.view';
const dummyProvider = () => React.createElement('div', null,
'placeholder');
+const ext1Provider = () => React.createElement('div', null, 'ext1-view');
+const ext2Provider = () => React.createElement('div', null, 'ext2-view');
+
const renderWithExtensionsProvider = (ui: ReactElement) =>
render(ui, { wrapper: ExtensionsProvider as any });
beforeEach(() => {
@@ -95,7 +97,7 @@ test('handles multiple views registered at the same
location', () => {
views.registerView(
{ id: 'ext1-view', name: 'ext1-view View' },
TEST_VIEW_ID,
- dummyProvider,
+ ext1Provider,
);
views.registerView(
{ id: 'ext2-view', name: 'ext2-view View' },
TEST_VIEW_ID,
- dummyProvider,
+ ext2Provider,
);
renderWithExtensionsProvider(<ViewListExtension viewId={TEST_VIEW_ID}
/>);
expect(screen.getByText(/ext1-view/)).toBeInTheDocument();
expect(screen.getByText(/ext2-view/)).toBeInTheDocument();
});
```
</div>
</details>
</div>
<small><i>Code Review Run #22288d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
