EnxDev commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4329640995
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -94,6 +99,202 @@ const TabTitle = styled.span`
// Get the user's OS
const userOS = detectOS();
+const PlusIcon = (
+ <Icons.PlusOutlined
+ iconSize="l"
+ css={css`
+ vertical-align: middle;
+ `}
+ data-test="add-tab-icon"
+ />
+);
+
+// Shown in place of PlusIcon while extensions are still loading — see the
+// extensionsReady guard in NewTabButton below.
+const LoadingIcon = (
+ <Icons.LoadingOutlined
+ iconSize="l"
+ css={css`
+ vertical-align: middle;
+ `}
+ data-test="add-tab-icon"
+ />
+);
+
+function NewTabButton({ onAddSqlEditor }: { onAddSqlEditor: () => void }) {
+ const [open, setOpen] = useState(false);
+ // Until this is true, menus.getMenu(sqllab.newTab) can't be trusted to
+ // reflect the final set of contributions — an extension that hasn't
+ // finished loading yet looks identical to "no extensions at all". Without
+ // this, clicking "+" during that window fell back to adding a plain SQL
+ // tab instead of waiting to show the real dropdown.
+ const extensionsReady = useExtensionsReady();
+
+ // Resolved at render time rather than module load so `t()` runs after the
+ // translator has been configured.
+ const newTabTooltip = !extensionsReady
+ ? t('Loading...')
+ : userOS === 'Windows'
+ ? t('New tab (Ctrl + q)')
+ : t('New tab (Ctrl + t)');
+
+ const dropdownItems = useMemo<MenuItemType[]>(() => {
+ if (!open) return [];
+ const primaryItems =
+ menus.getMenu(ViewLocations.sqllab.newTab)?.primary ?? [];
+ return [
+ {
+ key: 'sql-editor',
+ label: t('SQL Editor'),
+ icon: <Icons.TableOutlined iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ onAddSqlEditor();
+ },
+ },
+ ...primaryItems.flatMap(item => {
+ const command = commands.getCommand(item.command);
+ if (!command) {
+ // An extension contributed this menu item but its command isn't
+ // registered (load is still pending or failed). Skip it so clicking
+ // can't throw "Command not found" and break the add-tab flow.
+ return [];
+ }
+ const Icon = command.icon
+ ? ((Icons as Record<string, typeof Icons.FileOutlined>)[
+ command.icon
+ ] ?? Icons.FileOutlined)
+ : Icons.FileOutlined;
+ return [
+ {
+ key: command.id,
+ label: command.title ?? item.command,
+ icon: <Icon iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ commands.executeCommand(item.command).catch(error => {
+ logging.error(
+ `Failed to execute command ${item.command}`,
+ error,
+ );
+ });
+ },
+ } as MenuItemType,
+ ];
+ }),
+ ];
+ }, [open, onAddSqlEditor]);
+
+ const activate = useCallback(() => {
+ if (!extensionsReady) {
Review Comment:
Until every extension's remote entry settles, the `+` is dead: the capture
listener swallows the click and `activate()` returns early. Readiness waits on
the `Promise.all` at `ExtensionsLoader.ts:112` and the script load has no
timeout, so one slow bundle, even from an extension that contributes nothing to
SQL Lab, leaves users looking at a spinner and a "Loading..." tooltip.
Could we cap the wait and fall back to a plain tab after a few seconds, or
open the dropdown right away with SQL Editor and let contributed items fill in
as they register?
##########
superset-frontend/src/extensions/useExtensionsReady.ts:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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 { useSyncExternalStore } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import ExtensionsLoader from './ExtensionsLoader';
+
+/**
+ * Host-internal only (not part of the @apache-superset/core extension-author
+ * API): whether extension loading has settled, so UI that reads contributed
+ * menus/views/commands — e.g. the sqllab.newTab dropdown — can distinguish
+ * "still loading" (show a spinner, don't fall back yet) from "loaded, and
+ * there's genuinely nothing registered" (the fallback is correct).
+ *
+ * When the EnableExtensions feature flag is off, ExtensionsStartup never
+ * calls initializeExtensions() at all, so there's nothing to wait for —
+ * this returns true immediately in that case rather than reporting an
+ * indefinite pending state.
+ */
+export function useExtensionsReady(): boolean {
Review Comment:
Nothing tests the readiness path. Every TabbedSqlEditors test runs with
`EnableExtensions` off, so this hook is always true there, and the loader tests
never touch `isReady`/`onReady`.
A loader test that `markReady` fires on both resolve and reject, plus one
NewTabButton test with the flag on and a pending init (click does nothing, then
the dropdown opens once it resolves), would cover it.
--
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]
