bito-code-review[bot] commented on code in PR #38346: URL: https://github.com/apache/superset/pull/38346#discussion_r2874427497
########## superset-frontend/src/core/views/index.ts: ########## @@ -0,0 +1,83 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview Standalone views registry implementation. + * + * Stores view metadata and providers as module-level state. + * Extensions register views as side effects at import time. + */ + +import React, { ReactElement } from 'react'; +import type { views as viewsApi } from '@apache-superset/core'; +import { ErrorBoundary } from 'src/components/ErrorBoundary'; +import ExtensionPlaceholder from 'src/extensions/ExtensionPlaceholder'; +import { Disposable } from '../models'; + +type View = viewsApi.View; + +const viewRegistry: Map< + string, + { view: View; location: string; provider: () => ReactElement } +> = new Map(); + +const locationIndex: Map<string, Set<string>> = new Map(); + +const registerView: typeof viewsApi.registerView = ( + view: View, + location: string, + provider: () => ReactElement, +): Disposable => { + const { id } = view; + + viewRegistry.set(id, { view, location, provider }); + + const ids = locationIndex.get(location) ?? new Set(); + ids.add(id); + locationIndex.set(location, ids); + + return new Disposable(() => { + viewRegistry.delete(id); + locationIndex.get(location)?.delete(id); + }); +}; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>State inconsistency on duplicate view ids</b></div> <div id="fix"> It looks like registering the same view id multiple times could lead to inconsistency between viewRegistry and locationIndex. For instance, if two registrations occur for the same id, disposing the first one removes the view from both maps, but the second registration's view is lost while locationIndex still lists the id, causing getViews to return an empty array instead of undefined. This might affect view resolution reliability. </div> </div> <small><i>Code Review Run #21516c</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/packages/superset-core/src/api/editors.ts: ########## @@ -489,33 +504,29 @@ export interface EditorProviderRegisteredEvent { * Event fired when an editor provider is unregistered. */ export interface EditorProviderUnregisteredEvent { - /** The contribution that was unregistered */ - contribution: EditorContribution; + /** The descriptor of the editor that was unregistered */ + editor: Editor; } /** - * Register an editor provider for specific languages. - * When an extension registers an editor, it replaces the default for those languages. + * Registers a custom editor provider as a module-level side effect. * - * The contribution metadata (name, languages, description) is read from the - * extension's manifest (extension.json), so only the contribution ID and - * component are needed at registration time. + * When an extension registers an editor, it replaces the default for those languages. * - * @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 including id, name, and languages. + * @param component The React component implementing the editor. + * @returns Disposable which unregisters this editor provider on disposal. * * @example * ```typescript - * const disposable = registerEditorProvider( - * 'acme.monaco-sql', - * MonacoSQLEditor + * editors.registerEditor( + * { id: 'editors_bundle.monaco_sql', name: 'Monaco SQL Editor', languages: ['sql'] }, + * MonacoSQLEditor, * ); - * context.disposables.push(disposable); * ``` */ -export declare function registerEditorProvider( - id: string, +export declare function registerEditor( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Outdated API Documentation</b></div> <div id="fix"> The API refactor changes registerEditorProvider to registerEditor and updates related types, but the documentation still shows the old function name and types. This will mislead developers reading the docs. Update the docs to reflect the new API. </div> </div> <small><i>Code Review Run #21516c</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]
