bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4083080355
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Positional boolean placeholders</b></div>
<div id="fix">
Adding `backendId` as an eighth positional parameter forces every `makeTab`
call site to pass a bare `false` placeholder for `closed` (lines 271, 564,
636), which readers cannot interpret without opening the signature. Consider an
options object (e.g. `{ closed, backendId }`) so arguments are self-describing
and future params stop growing the positional list.
</div>
</div>
<small><i>Code Review Run #f2f26e</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 +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:
<div>
<div id="suggestion">
<div id="issue"><b>Mock leaks on test failure</b></div>
<div id="fix">
Assigning `ExtensionsLoader.prototype.initializeExtensions` is only reverted
on the happy path: if any `waitFor` assertion in this test (or the two sibling
failure tests) fails, the trailing restore is skipped and the mock leaks into
later tests, since `afterEach` resets the loader instance but not the
prototype. Restore in `finally` or in `afterEach`.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Repeated toast assertion boilerplate</b></div>
<div id="fix">
The `store.getState() as unknown as { messageToasts: ... }` plus
regex-search block repeats three times (here, lines 327-334 and 365-374).
Extracting a small helper that returns the toast texts keeps the three
failure-path tests consistent and gives one place to adapt if the toast state
shape or wording changes.
</div>
</div>
<small><i>Code Review Run #f2f26e</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/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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated test setup</b></div>
<div id="fix">
The three new tests repeat an identical `views.registerView({ id:
'test.northPane', ... }, ViewLocations.sqllab.northPane, ...)` block (also at
lines 430-434 and 460-464) and the `sqllab.northPaneView.${queryEditor.id}` key
derivation (lines 407, 429, 459). A shared helper like the existing
`setupWithLatestQuery`, or a `beforeEach` registration, keeps the fixture in
sync when the view id or key format changes.
</div>
</div>
<small><i>Code Review Run #f2f26e</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/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:
<div>
<div id="suggestion">
<div id="issue"><b>Unstable callback defeats memoization</b></div>
<div id="fix">
The inline `() => newQueryEditor()` creates a new prop on every render of
`TabbedSqlEditors`, which re-renders on every keystroke via `queries`. That
defeats the `dropdownItems` memo and re-runs the listener-attaching `useEffect`
whenever `activate` changes. `newQueryEditor` is already a stable `useCallback`
— pass it directly.
</div>
</div>
<small><i>Code Review Run #f2f26e</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]
