sadpandajoe commented on code in PR #44101:
URL: https://github.com/apache/superset/pull/44101#discussion_r4130487699
##########
superset/views/core.py:
##########
@@ -797,6 +797,34 @@ def file_handler(self) -> FlaskResponse:
return self.render_app_template(extra_bootstrap_data=payload)
+ @has_access
+ @event_logger.log_this
+ @expose("/extensions/view/<path:view_id>")
+ def extension_view(self, view_id: str) -> FlaskResponse:
Review Comment:
`extension_view` has no test anywhere in the suite, including its
`redirect_to_login()` branch for anonymous access. Given `@has_access` already
gates non-public roles, that branch only matters when anonymous/public-role
access is configured for the app — exactly the kind of narrow authorization
path that regresses unnoticed without a direct test.
Worth a request test asserting an authenticated user gets the SPA shell and
an anonymous user is redirected to login?
##########
superset-frontend/src/core/views/index.ts:
##########
@@ -96,6 +96,44 @@ export const resolveView = (id: string): React.ReactElement
=> {
);
};
+/**
+ * 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 => {
Review Comment:
`useResolveView`'s reactive re-render on late registration — the exact race
this PR's second commit says it fixes, verified only by a manual Playwright run
per the commit message — has no unit test: `core/views/index.test.ts` only
exercises the non-reactive `resolveView`. A future change that breaks the
`useSyncExternalStore` subscription (e.g. an unstable `getSnapshot` reference)
would regress silently with nothing failing in CI.
Worth a test that renders before the view registers, registers it, and
asserts the placeholder gets replaced without a remount?
##########
superset-frontend/src/core/contributions.ts:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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, mirroring the pattern in `src/SqlLab/contributions.ts`.
+ *
+ * @example
+ * // In extension.json:
+ * {
+ * "contributions": {
+ * "views": {
+ * "global": {
+ * "settingsPanel": [{ "id": "my-ext.settings", "name": "My Settings"
}]
Review Comment:
This `extension.json` example (`contributions.views.global.settingsPanel` /
`contributions.menus.global.settingsMenu`) doesn't match the schema the build
tooling actually validates: `ViewContributions`/`MenuContributions` in
`superset-core/src/contributions/index.ts` only accept a `sqllab` scope
(`SqlLabLocation` has no `settingsPanel`/`settingsMenu` member, and there's no
`global` key at all). An extension author following this doc as written would
ship an `extension.json` whose settings contribution is silently ignored by the
manifest-driven path.
Separately, even the working programmatic path
(`registerMenuItem`/`registerView`, as exercised by this PR's own test) doesn't
wire `settings.panel` to anything: `RightMenu.tsx` only reads
`menuItem.command`, never `menuItem.view`, so registering a panel at
`GlobalLocations.settings.panel` has no automatic effect. Unlike the SQL Lab
`ViewListExtension` pattern this mirrors, the extension's own command callback
has to hard-code navigation to `/extensions/view/:viewId`.
Is `settings.panel` meant to be wired automatically, or is the doc comment
ahead of what's implemented?
##########
superset-frontend/packages/superset-core/src/extensions/index.ts:
##########
@@ -48,6 +48,27 @@
import { Extension } from '../common';
import { ExtensionStorage } from '../storage';
+/**
+ * Global, host-level UI surfaces available to an extension, independent of
+ * any specific view or panel it has registered.
+ */
+export interface ExtensionWindow {
+ /**
+ * Show a transient informational toast to the current user.
+ */
+ showInformationMessage(message: string): void;
Review Comment:
`ExtensionWindow.showInformationMessage/showWarningMessage/showErrorMessage`
take only a `message` string, but the underlying
`addInfoToast`/`addWarningToast`/`addDangerToast` action creators already
accept an optional `options` argument that includes `duration`. SIP-96 (linked
in the PR description) describes admin-*configurable* in-app notifications.
Is a fixed, non-configurable toast duration intentional for this POC, or
should the public surface forward an options argument now while the SDK is
still unreleased, to avoid a breaking signature change later?
##########
superset-frontend/src/features/home/RightMenu.test.tsx:
##########
@@ -459,6 +459,41 @@ test('Logs out and clears local storage item redux', async
() => {
}
});
+test('renders an extension-contributed item in the Settings dropdown', async
() => {
+ const { commands, menus } = jest.requireActual('src/core');
+ const { GlobalLocations } = jest.requireActual('src/core/contributions');
+
+ const disposeCommand = commands.registerCommand(
+ { id: 'test-ext.openSettings', title: 'My Extension Settings' },
+ jest.fn(),
Review Comment:
This test registers the extension's command with a `jest.fn()` callback but
only asserts the menu label renders — it never clicks the item, so the actual
`onClick: () => commands.executeCommand(command.id)` wiring in `RightMenu.tsx`
is unverified. A wrong command id or a dropped `executeCommand` call would
leave this test green.
Worth adding a click on the rendered item and asserting the callback fired?
--
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]
