bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4153056527
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>O(n) editor lookup per event</b></div>
<div id="fix">
`findQueryEditorByAnyId(sqlEditorId ?? undefined)` runs a linear scan of all
`queryEditors` on every START_QUERY/STOP_QUERY/QUERY_SUCCESS/QUERY_FAILED event
via `extractBaseData`, and the `predicate` wrapper already does a second
`findQueryEditor` scan for the same action. On sessions with many tabs and high
query volume this is repeated O(n) work per event; consider a memoized
id→editor map.
</div>
</div>
<small><i>Code Review Run #45b62b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Warning type never asserted</b></div>
<div id="fix">
The test names claim a *warning* toast, but assertions at 295-301 and
333-337 check only `text`. `addWarningToast` sets `toastType` in the ADD_TOAST
payload, so asserting `toastType === ToastType.Warning` would pin the severity;
today downgrading `addWarningToast` to `addInfoToast` in `ExtensionsStartup`
ships these tests green.
</div>
</div>
<small><i>Code Review Run #45b62b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -440,7 +487,7 @@ const onDidCloseTab: typeof sqlLabApi.onDidCloseTab = (
action.queryEditor.dbId ?? 0,
action.queryEditor.catalog,
action.queryEditor.schema,
- true, // closed
+ { closed: true, backendId: resolveBackendId(action.queryEditor) },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Closed tab loses backendId</b></div>
<div id="fix">
The removed `true, // closed` argument passed the editor id as the 7th
positional arg, which `makeTab` mapped to `backendId`. The replacement resolves
`backendId` via `resolveBackendId`, which returns `undefined` for a tab still
in local storage (`inLocalStorage` set, no `tabViewId`), so `onDidCloseTab`
listeners now receive a Tab with no backend id where they previously got the
editor id. Please fall back to `action.queryEditor.id`.
</div>
</div>
<small><i>Code Review Run #45b62b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -557,6 +605,9 @@ const createTab: typeof sqlLabApi.createTab = async (
inheritedValues.queryLimit ?? common?.conf?.DEFAULT_SQLLAB_LIMIT,
autorun: false,
name,
+ ...(options?.northPaneViewId && {
+ northPaneViewId: options.northPaneViewId,
+ }),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>northPaneViewId silently dropped</b></div>
<div id="fix">
`createTab` accepts `options.northPaneViewId` and spreads it into
`newQueryEditor`, but the returned `makeTab(...)` call at lines 624-631 passes
only `backendId` — the `Tab` model has no `northPaneViewId` field, so the
option is accepted and silently discarded. `CreateTabOptions.northPaneViewId`
documents that the tab opens with a registered north-pane view. Please plumb
the value through or surface an error.
</div>
</div>
<small><i>Code Review Run #45b62b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Storage logic inline in component</b></div>
<div id="fix">
This block adds localStorage persistence, per-tab key migration, and
cross-tab `storage`-event sync inline in an already ~1100-line component whose
primary responsibility is rendering the editor. Extracting it into a
`useNorthPaneView(queryEditor)` hook isolates the storage concern, makes it
independently testable, and keeps this file focused on layout. Behavior is
unchanged.
</div>
</div>
<small><i>Code Review Run #45b62b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
