bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4154935420
##########
superset-frontend/src/extensions/ExtensionsStartup.test.tsx:
##########
@@ -260,26 +274,49 @@ test('does not initialize ExtensionsLoader when
EnableExtensions feature flag is
initializeSpy.mockRestore();
});
-test('continues rendering children even when ExtensionsLoader initialization
fails', async () => {
+test('surfaces a warning toast naming the extensions that failed to
initialize', async () => {
+ mockIsFeatureEnabled.mockReturnValue(true);
+
+ // A single extension's remote entry failing does not reject the aggregate;
+ // the loader resolves with the names of the failed extensions instead.
+ ExtensionsLoader.prototype.initializeExtensions = jest
+ .fn()
+ .mockResolvedValue(['Broken Extension']);
+
+ const store = createStore(mockInitialState, reducerIndex);
+
+ render(
+ <ExtensionsStartup>
+ <div data-testid="child" />
+ </ExtensionsStartup>,
+ { store, useRouter: true },
+ );
+
+ await waitFor(() => {
+ expect(
+ getToastTexts(store).some(text =>
+ /Some extensions failed to load: Broken Extension/.test(text),
+ ),
+ ).toBe(true);
+ });
Review Comment:
<!-- Bito Reply -->
The suggestion to assert `toastType === ToastType.Warning` is appropriate.
It improves the test by ensuring that the severity of the toast is explicitly
verified, preventing potential regressions where a warning might be downgraded
to an info toast without failing the test.
**superset-frontend/src/extensions/ExtensionsStartup.test.tsx**
```
await waitFor(() => {
expect(
getToastTexts(store).some(text =>
/Some extensions failed to load: Broken Extension/.test(text),
),
).toBe(true);
});
```
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:
##########
@@ -275,6 +304,56 @@ const SqlEditor: FC<Props> = ({
const logAction = useLogAction({ queryEditorId: queryEditor.id });
const isActive = currentQueryEditorId === queryEditor.id;
+
+ // 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;
+
+ // ID of the northPane view active for this tab, or null for the default
+ // SQL editor layout. 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 fall back to the per-tab localStorage entry that the
+ // effect below keeps in sync.
+ 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(
+ NORTH_PANE_VIEW_KEY(northPaneStorageId),
+ northPaneViewId,
+ );
+ }, [northPaneStorageId, northPaneViewId]);
+
+ useEffect(() => {
+ const handler = (e: StorageEvent) => {
+ if (e.key === NORTH_PANE_VIEW_KEY(northPaneStorageId)) {
+ setNorthPaneViewId(e.newValue || null);
+ }
+ };
+ window.addEventListener('storage', handler);
+ return () => window.removeEventListener('storage', handler);
+ }, [northPaneStorageId]);
Review Comment:
<!-- Bito Reply -->
The implementation of the `useNorthPaneView` hook correctly encapsulates the
storage logic, including persistence, key migration, and cross-tab
synchronization. This change effectively reduces the complexity of the main
component and improves maintainability by isolating the storage concern.
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -211,12 +252,18 @@ function extractBaseData(action: QueryAction): {
queryLimit,
} = query;
+ // Resolve backendId through the same path as every other Tab construction
+ // so query-event listeners can correlate the tab with its tabstateview row.
+ const queryEditor = findQueryEditorByAnyId(sqlEditorId ?? undefined);
Review Comment:
<!-- Bito Reply -->
The suggestion to use a memoized map is a valid optimization for reducing
redundant linear scans during high-frequency events. However, given that query
events occur per lifecycle transition rather than per keystroke, the current
implementation's performance impact is minimal in practice. You may choose to
proceed with the current approach if the number of open tabs remains small, as
it avoids the added complexity of cache invalidation.
**superset-frontend/src/core/sqlLab/index.ts**
```
// Resolve backendId through the same path as every other Tab construction
// so query-event listeners can correlate the tab with its tabstateview
row.
const queryEditor = findQueryEditorByAnyId(sqlEditorId ?? undefined);
```
--
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]
