bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4141901842
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -161,19 +191,37 @@ const makeTab = (
catalog: string | null = null,
schema: string | null = null,
closed: boolean = false,
+ backendId?: string,
Review Comment:
<!-- Bito Reply -->
The suggestion to use an options object for `makeTab` is appropriate. It
improves code readability and maintainability by making the arguments
self-describing, which avoids the ambiguity of positional boolean placeholders.
##########
superset-frontend/src/extensions/ExtensionsStartup.test.tsx:
##########
@@ -260,26 +261,56 @@ 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.
+ const originalInitialize = ExtensionsLoader.prototype.initializeExtensions;
+ 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(() => {
+ const { messageToasts } = store.getState() as unknown as {
+ messageToasts: { text: string }[];
+ };
+ expect(
+ messageToasts.some(toast =>
+ /Some extensions failed to load: Broken Extension/.test(toast.text),
+ ),
+ ).toBe(true);
+ });
+
+ ExtensionsLoader.prototype.initializeExtensions = originalInitialize;
Review Comment:
<!-- Bito Reply -->
The changes correctly address the identified issues. Moving the prototype
restoration to `afterEach` ensures it runs regardless of test success or
failure, preventing mock leakage, and extracting the toast-text assertion into
a helper function improves maintainability and consistency across the test
suite.
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -265,25 +424,7 @@ function TabbedSqlEditors({
onEdit={handleEdit}
popupClassName={SQLLAB_TAB_OVERFLOW_POPUP_CLASS}
type={queryEditors?.length === 0 ? 'card' : 'editable-card'}
- addIcon={
- <Tooltip
- id="add-tab"
- placement="left"
- title={
- userOS === 'Windows'
- ? t('New tab (Ctrl + q)')
- : t('New tab (Ctrl + t)')
- }
- >
- <Icons.PlusOutlined
- iconSize="l"
- css={css`
- vertical-align: middle;
- `}
- data-test="add-tab-icon"
- />
- </Tooltip>
- }
+ addIcon={<NewTabButton onAddSqlEditor={() => newQueryEditor()} />}
Review Comment:
<!-- Bito Reply -->
The suggestion is correct. Passing the stable `newQueryEditor` callback
directly instead of creating a new anonymous function on every render prevents
unnecessary re-renders of the `NewTabButton` component and ensures that
memoized dependencies remain stable.
**superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx**
```
addIcon={<NewTabButton onAddSqlEditor={newQueryEditor} />}
```
##########
superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx:
##########
@@ -397,6 +400,94 @@ describe('SqlEditor', () => {
).toBeInTheDocument();
});
+ test('renders a registered northPane view in place of the editor', async ()
=> {
+ const { queryEditor } = mockedProps;
+ // The fixture has no tabViewId, so the component falls back to the id;
+ // mirror that here to derive the same persistence key.
+ const storageKey = `sqllab.northPaneView.${queryEditor.id}`;
+ localStorage.setItem(storageKey, 'test.northPane');
+ const disposable = views.registerView(
+ { id: 'test.northPane', name: 'Test North Pane' },
+ ViewLocations.sqllab.northPane,
+ () => <div data-test="np-view">NorthPane content</div>,
+ );
Review Comment:
<!-- Bito Reply -->
The suggestion to extract the `northPane` registration and storage-key
derivation into shared helpers is a valid and recommended improvement. It
addresses the code duplication identified in the review, ensuring that the
fixture remains consistent if the view ID or key format changes in the future.
Applying this refactoring will improve maintainability and reduce the risk of
synchronization errors across the test suite.
--
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]
