EnxDev commented on code in PR #41285: URL: https://github.com/apache/superset/pull/41285#discussion_r4163409223
########## superset-frontend/src/SqlLab/components/SqlEditor/useNorthPaneView.ts: ########## @@ -0,0 +1,104 @@ +/** + * 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 { useEffect, useRef, useState } from 'react'; +import type { QueryEditor } from 'src/SqlLab/types'; +import { ViewLocations } from 'src/SqlLab/contributions'; +import { useViews } from 'src/core/views'; + +/** Per-tab localStorage key storing the active northPane view ID. */ +const NORTH_PANE_VIEW_KEY = (tabId: string) => `sqllab.northPaneView.${tabId}`; + +// The northPane keys are dynamic per-tab strings rather than members of the +// typed LocalStorageKeys enum, so the typed helpers don't apply. Guard the raw +// access here so a storage-restricted browser can't crash the editor mount. +const readNorthPaneStorage = (key: string): string | null => { + try { + return localStorage.getItem(key); + } catch { + return null; + } +}; + +const writeNorthPaneStorage = (key: string, value: string | null): void => { + try { + if (value === null) { + localStorage.removeItem(key); + } else { + localStorage.setItem(key, value); + } + } catch { + // localStorage may be unavailable (blocked/quota/private mode); ignore. + } +}; + +/** + * Tracks which view (if any) a tab's north pane should render, keeping it in + * sync across reloads and other tabs via a per-tab localStorage entry. + * + * A tab created through the extension API carries the requested view on its + * own query editor state, so it can never be picked up by another tab. + * Editors hydrated from the backend on reload don't carry the field, so this + * falls back to the per-tab localStorage entry kept in sync below. + */ +export default function useNorthPaneView(queryEditor: QueryEditor) { + // Re-renders when an extension registers a northPane view after async load. + const northPaneViews = useViews(ViewLocations.sqllab.northPane) || []; + + // Resolve the per-tab localStorage key the same way every other SQL Lab + // consumer does (`tabViewId ?? id`), so the value written, read back, and + // observed via the `storage` event all agree once a tab is backend-persisted. + const northPaneStorageId = queryEditor.tabViewId ?? queryEditor.id; + + const [northPaneViewId, setNorthPaneViewId] = useState<string | null>( + () => + queryEditor.northPaneViewId ?? + readNorthPaneStorage(NORTH_PANE_VIEW_KEY(northPaneStorageId)), + ); + + // Tracks the storage id last written so that, when a tab syncs to the + // backend and `tabViewId` arrives, the entry under the old id-keyed key is + // removed rather than left orphaned in localStorage. + const northPaneStorageIdRef = useRef(northPaneStorageId); + + useEffect(() => { + if (northPaneStorageIdRef.current !== northPaneStorageId) { + writeNorthPaneStorage( + NORTH_PANE_VIEW_KEY(northPaneStorageIdRef.current), + null, + ); + northPaneStorageIdRef.current = northPaneStorageId; + } + writeNorthPaneStorage( Review Comment: Nit, take it or leave it. Nothing removes `sqllab.northPaneView.<id>` when a tab is closed, so every closed extension tab leaves a key behind in localStorage. Clearing it on the close path (or in `removeQueryEditor`) would keep it tidy. ########## superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx: ########## @@ -219,23 +422,11 @@ function TabbedSqlEditors({ const emptyTab = ( <StyledTab> <TabTitle>{t('Add a new tab')}</TabTitle> - <Tooltip - id="add-tab" - placement="bottom" - title={ - userOS === 'Windows' - ? t('New tab (Ctrl + q)') - : t('New tab (Ctrl + t)') - } - > - <Icons.PlusCircleOutlined - iconSize="s" - css={css` - vertical-align: middle; - `} - data-test="add-tab-icon" - /> - </Tooltip> + {/* Reuses the toolbar's own add-tab control (dropdown when an + extension contributes to sqllab.newTab, plain create otherwise) + instead of a bare icon, so this empty-state entry point offers the + same choices as the "+" in the tab bar once tabs exist. */} + <NewTabButton onAddSqlEditor={newQueryEditor} /> Review Comment: In the empty state there's no antd add `<button>` around this (the Tabs are `card`, not `editable-card`), so `closest('button')` at L250 returns null and the capture listener never attaches. The click just bubbles to `onTabClicked` and creates a plain SQL tab, so the dropdown never shows here, even while extensions are still loading. Could this path get its own click handler that calls `activate()`, and a test that starts with zero query editors? ########## superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx: ########## @@ -94,6 +99,199 @@ 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…') Review Comment: `Loading…` with the ellipsis character isn't in `messages.pot`, which is what's failing babel-extract and `check_pot_drift_test`. The pot already has `Loading...` with three dots and existing translations, so reusing it fixes both checks: ```suggestion ? t('Loading...') ``` ########## superset-frontend/src/extensions/ExtensionsLoader.ts: ########## @@ -100,22 +112,61 @@ class ExtensionsLoader { const results = await Promise.all( extensions.map(ext => this.initializeExtension(ext)), ); - if (results.every(Boolean)) { + const failed = extensions + .filter((_, index) => !results[index]) + .map(ext => ext.name); + if (failed.length === 0) { logging.info('Extensions initialized successfully.'); } else { - const failedCount = results.filter(succeeded => !succeeded).length; logging.info( - `Extensions initialized with ${failedCount} of ` + + `Extensions initialized with ${failed.length} of ` + `${extensions.length} extension(s) failing. See errors above.`, ); } + return failed; } catch (error) { + // Reset so a later call can retry, and rethrow so callers (e.g. + // ExtensionsStartup) can surface the failure instead of it being + // swallowed here and the success path running regardless. + this.initializationPromise = null; logging.error('Error setting up extensions:', error); + throw error; } })(); + // Attached separately (not reassigning this.initializationPromise) so + // ready-tracking is purely a side effect and every caller still sees the + // original promise's own resolve/reject value. + this.initializationPromise.finally(() => this.markReady()); Review Comment: `.finally()` returns a new promise that rejects whenever the original does, and nothing handles it. When the list fetch fails, that surfaces as an unhandled rejection: it's what crashes the worker in sharded-jest-tests (3) at `ExtensionsLoader.test.ts:500`, and in the browser it shows up as an `Uncaught (in promise)` next to the toast. Handling both branches avoids the dangling chain: ```suggestion this.initializationPromise.then( () => this.markReady(), () => this.markReady(), ); ``` -- 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]
