EnxDev commented on code in PR #44101: URL: https://github.com/apache/superset/pull/44101#discussion_r4263773552
########## superset-frontend/src/core/contributions.ts: ########## @@ -0,0 +1,61 @@ +/** + * 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. + */ +/** + * Global (non-SQL-Lab-scoped) view/menu locations for extension + * integration. Unlike `src/SqlLab/contributions.ts`, these locations + * aren't declared via the manifest `contributions.views`/`contributions.menus` + * field -- `ViewContributions`/`MenuContributions` in `superset-core` + * only accept a `sqllab` scope today -- so extensions register against + * them imperatively instead. + * + * This constant is internal to the Superset app; it isn't exported from + * `@apache-superset/core`, so extensions reference these locations by + * their literal string values instead of importing `GlobalLocations`. Review Comment: With `GlobalLocations` kept internal, the two literal strings are the public contract, and this JSDoc in a host-only file is the one place they're written down. Could we add a page under `docs/developer_docs/extensions/extension-points/` next to `sqllab.md`? The two locations, the `primary`-only rule and `ctx.window` would cover it. ########## superset-frontend/packages/superset-core/src/extensions/index.ts: ########## @@ -65,6 +100,12 @@ export interface ExtensionContext { * All keys are automatically namespaced to prevent collisions. */ storage: ExtensionStorage; + + /** + * Global, host-level UI surfaces (e.g. toast notifications) not scoped to + * any single view or panel. + */ + window: ExtensionWindow; Review Comment: The other host capabilities are top-level namespaces (`commands`, `menus`, `views`, `navigation`), and the context has so far only carried things scoped to one extension, like its namespaced storage. Nothing in the toast implementation reads `this.extension`, and on `ctx` a component registered through `views.registerView` can only reach it if `activate` closes over the context. Would a namespace export fit better? I might be missing a reason it has to hang off the context. ########## superset-frontend/src/extensions/ExtensionContext.ts: ########## @@ -37,10 +43,33 @@ class ExtensionContext implements ExtensionContextType { private _storage?: ExtensionContextType['storage']; + private _window?: ExtensionContextType['window']; + constructor(extension: Extension) { this.extension = extension; } + get window(): ExtensionContextType['window'] { + if (!this._window) { + // Dispatched against the app-level `store` singleton directly, not + // via a `useDispatch()` hook, since this surface must also work from + // non-component extension code (e.g. a registered command's + // callback), not just from within a React render. + this._window = { + showInformationMessage: (message: string, options) => { + store.dispatch(addInfoToast(message, options)); Review Comment: `options` goes through whole, so `allowHtml`, `noDuplicate` and `action` all work from an extension even though the SDK type only declares `duration`. An explicit `{ duration: undefined }` also overwrites the 4000 default in the spread and falls through to `addToast`'s 8000. Mind picking `duration` out and only passing it when it's set? Same for the other two methods. ########## superset-frontend/src/features/home/RightMenu.tsx: ########## @@ -521,6 +527,31 @@ const RightMenu = ({ } }); + const extensionSettingsItems = (settingsMenuExtension?.primary ?? []) + .map(menuItem => { + const command = commands.getCommand(menuItem.command); + if (!command) { + return null; + } + return { + key: command.id, + label: command.title, + title: command.description, + onClick: () => commands.executeCommand(command.id), Review Comment: I know the panel wiring was left manual on purpose, but `menuItem.view` is already sitting on the item. Without it each extension hand-rolls `window.location.assign('/extensions/view/...')`: a full reload per click, no help from the router's `basename` under a non-root app root, and the id encoding becomes every extension's problem. Could the host render a `Link` to the view route when `view` resolves to something registered at `settings.panel`, and fall back to `executeCommand` otherwise? ########## superset-frontend/src/core/views/index.ts: ########## @@ -89,11 +89,53 @@ export const resolveView = (id: string): React.ReactElement => { if (!entry) { return React.createElement(ExtensionPlaceholder, { id }); } - return React.createElement( - ErrorBoundary, - null, - React.createElement(entry.component), + return React.createElement(ErrorBoundary, { + key: id, + // No JSX in a .ts file; `key` requires a props object, so `children` + // has to travel through it too. + // oxlint-disable-next-line no-children-prop + children: React.createElement(entry.component), + }); +}; + +/** + * Reactive counterpart to `resolveView`, for hosting a single + * extension-registered view (see `src/pages/ExtensionView`). + * + * `resolveView` reads the registry once, synchronously, at call time. + * That's a real gap for a host page reached by direct/full navigation + * (a bookmark, a page refresh, or an extension's own menu command using + * `window.location.assign` -- see `ExtensionView`'s own docstring): the + * extension providing the view loads asynchronously (its remote entry is + * fetched over the network), so on first render the registry is still + * empty and a plain `resolveView` call permanently commits to the + * "could not be loaded" placeholder, with nothing to trigger a re-render + * once the extension actually finishes loading and registers. Subscribing + * via `useSyncExternalStore` (the same registry-change events `useViews` + * already subscribes to) re-renders once that registration lands. + * + * Snapshots the registered `component` itself, not a freshly + * `React.createElement`-ed result -- `useSyncExternalStore` requires a + * `getSnapshot` that returns a stable reference when nothing changed, and + * `entry.component` is exactly that (unlike a new element object created + * fresh on every call). + */ +export const useResolveView = (id: string): React.ReactElement => { + const component = useSyncExternalStore( + subscribe, + () => viewRegistry.get(id)?.component, + () => undefined, ); + if (!component) { + return React.createElement(ExtensionPlaceholder, { id }); Review Comment: On a hard load this shows "could not be loaded" until the remote entry registers, and shows the same thing forever when the id is just wrong. A user can't tell slow from broken. `ExtensionsLoader.initializeExtensions()` already returns a memoized promise, so the page could render `Loading` until it settles and only then fall back to the placeholder. ########## superset-frontend/src/pages/ExtensionView/index.tsx: ########## @@ -0,0 +1,76 @@ +/** + * 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. + */ +import { useParams } from 'react-router-dom'; +import { t } from '@apache-superset/core/translation'; +import SubMenu, { SubMenuProps } from 'src/features/home/SubMenu'; +import { useResolveView } from 'src/core/views'; + +/** + * Generic full-page host for a single extension-registered view, reached at + * `/extensions/view/:viewId+`. Extensions cannot render their own views + * directly (`resolveView`/`useResolveView` are host-internal, not part of + * the public `@apache-superset/core` SDK) -- they register a view at + * `GlobalLocations.settings.panel` and a matching command at + * `GlobalLocations.settings.menu` whose callback navigates here, and the + * host resolves and renders it. + * + * Uses the reactive `useResolveView`, not the plain `resolveView`: this + * page is reachable by a direct/full navigation (a bookmark, a page + * refresh, or an extension's own menu command falling back to + * `window.location.assign` in the absence of an SDK-level SPA-navigation + * primitive), and the providing extension's own code loads asynchronously + * -- a non-reactive resolve would permanently commit to the "could not be + * loaded" placeholder from before that load finishes. + */ +const decodeViewId = (viewId: string | undefined): string => { + if (!viewId) { + return ''; + } + try { + return decodeURIComponent(viewId); + } catch { + return viewId; + } +}; + +const ExtensionView = () => { + const { viewId: rawViewId } = useParams<{ viewId: string }>(); + // The router leaves percent-encoded separators (`%2F`) intact, so decode + // to match the registry key whether a command used a literal or encoded `/`. + const viewId = decodeViewId(rawViewId); + + const menuData: SubMenuProps = { + name: t('Extension'), Review Comment: Nit, take it or leave it. Every extension page gets the heading "Extension", and the registry already holds the view's `name`. `useResolveView` could hand it back with the component so the header says which screen this is. ########## superset-frontend/src/features/home/RightMenu.tsx: ########## @@ -521,6 +527,31 @@ const RightMenu = ({ } }); + const extensionSettingsItems = (settingsMenuExtension?.primary ?? []) Review Comment: Every signed-in user who loads the extension gets this entry, and the SDK's `authentication` namespace only exposes `getCSRFToken`, so there's no SDK way for an extension to check roles before it registers. For SIP-96 that puts an admin screen in a Gamma user's Settings menu. Is the plan for extensions to fetch roles themselves, or should a menu item be able to carry a permission the host checks? -- 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]
