rusackas commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4141899711
##########
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:
Good catch, switched `closed`/`backendId` to an options object so call sites
aren't left guessing what a bare `false` means.
##########
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:
Fixed both: the prototype restore now lives in `afterEach` so a failed
assertion can't leak the mock into later tests, and pulled the toast-text
unwrapping into a small helper.
##########
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:
Extracted the northPane registration and storage-key derivation into shared
helpers, done.
##########
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:
Good catch, passing `newQueryEditor` directly now instead of wrapping it in
a new arrow function on every render.
--
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]
