bito-code-review[bot] commented on code in PR #37298:
URL: https://github.com/apache/superset/pull/37298#discussion_r2710438381
##########
superset-frontend/src/components/DatabaseSelector/index.tsx:
##########
@@ -317,6 +376,40 @@ export function DatabaseSelector({
const catalogOptions = catalogData || EMPTY_CATALOG_OPTIONS;
+ const handleModalOk = useCallback(() => {
+ // Apply modal selections to actual state
+ if (modalDb && modalDb.id !== currentDb?.value) {
+ const databaseWithId = { ...modalDb, id: modalDb.id };
+ setCurrentDb(databaseWithId as DatabaseValue);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing DatabaseValue properties</b></div>
<div id="fix">
Ensure `databaseWithId` includes all fields of `DatabaseValue`. For example:
```ts
const databaseWithId = {
...modalDb,
id: modalDb.id,
value: modalDb.id,
label: modalDb.database_name,
};
```
</div>
</div>
<small><i>Code Review Run #9ba2ef</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/SqlEditorLeftBar/SqlEditorLeftBar.test.tsx:
##########
@@ -109,292 +140,295 @@ const renderAndWait = (
}),
);
-test('renders a TableElement', async () => {
- const { findByText, getAllByTestId } = await renderAndWait(
- mockedProps,
- undefined,
- {
+describe('SelectView', () => {
+ test('renders a TableElement', async () => {
+ const { getAllByTestId } = await renderAndWait(mockedProps, undefined, {
...initialState,
sqlLab: {
...initialState.sqlLab,
tables: [table],
databases: { [mockData.database.id]: { ...mockData.database } },
},
- },
- );
- expect(await findByText(/Database/i)).toBeInTheDocument();
- const tableElement = getAllByTestId('table-element');
- expect(tableElement.length).toBeGreaterThanOrEqual(1);
-});
+ });
+ await switchToSelectView();
+ const tableElement = getAllByTestId('table-element');
+ expect(tableElement.length).toBeGreaterThanOrEqual(1);
+ });
-test('table should be visible when expanded is true', async () => {
- const { container, getByText, getByRole, getAllByLabelText } =
- await renderAndWait(mockedProps, undefined, {
- ...initialState,
- sqlLab: {
- ...initialState.sqlLab,
- tables: [table],
- databases: { [mockData.database.id]: { ...mockData.database } },
- },
+ test('table should be visible when expanded is true', async () => {
+ const { container, getByText, getByRole, getAllByLabelText } =
+ await renderAndWait(mockedProps, undefined, {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ tables: [table],
+ databases: { [mockData.database.id]: { ...mockData.database } },
+ },
+ });
+ await switchToSelectView();
+ const dbSelect = getByRole('combobox', {
+ name: 'Select database or type to search databases',
});
+ const schemaSelect = getByRole('combobox', {
+ name: 'Select schema or type to search schemas: main',
+ });
+ const tableSelect = getAllByLabelText(
+ /Select table or type to search tables/i,
+ )[0];
+ const tableOption = within(tableSelect).getByText(/ab_user/i);
- const dbSelect = getByRole('combobox', {
- name: 'Select database or type to search databases',
- });
- const schemaSelect = getByRole('combobox', {
- name: 'Select schema or type to search schemas: main',
+ expect(dbSelect).toBeInTheDocument();
+ expect(schemaSelect).toBeInTheDocument();
+ expect(tableSelect).toBeInTheDocument();
+ expect(tableOption).toBeInTheDocument();
+ expect(
+ container.querySelector('.ant-collapse-content-active'),
+ ).toBeInTheDocument();
+ table.columns.forEach(({ name }) => {
+ expect(getByText(name)).toBeInTheDocument();
+ });
});
- const tableSelect = getAllByLabelText(
- /Select table or type to search tables/i,
- )[0];
- const tableOption = within(tableSelect).getByText(/ab_user/i);
- expect(getByText(/Database/i)).toBeInTheDocument();
- expect(dbSelect).toBeInTheDocument();
- expect(schemaSelect).toBeInTheDocument();
- expect(tableSelect).toBeInTheDocument();
- expect(tableOption).toBeInTheDocument();
- expect(
- container.querySelector('.ant-collapse-content-active'),
- ).toBeInTheDocument();
- table.columns.forEach(({ name }) => {
- expect(getByText(name)).toBeInTheDocument();
+ test('catalog selector should be visible when enabled in the database',
async () => {
+ const { container, getByText, getByRole } = await renderAndWait(
+ mockedProps,
+ undefined,
+ {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: mockedProps.queryEditorId,
+ dbId: mockData.database.id,
+ },
+ tables: [table],
+ databases: {
+ [mockData.database.id]: {
+ ...mockData.database,
+ allow_multi_catalog: true,
+ },
+ },
+ },
+ },
+ );
+ await switchToSelectView();
+
+ const dbSelect = getByRole('combobox', {
+ name: 'Select database or type to search databases',
+ });
+ const catalogSelect = getByRole('combobox', {
+ name: 'Select catalog or type to search catalogs',
+ });
+ const schemaSelect = getByRole('combobox', {
+ name: 'Select schema or type to search schemas',
+ });
+
+ expect(dbSelect).toBeInTheDocument();
+ expect(catalogSelect).toBeInTheDocument();
+ expect(schemaSelect).toBeInTheDocument();
+ expect(
+ container.querySelector('.ant-collapse-content-active'),
+ ).toBeInTheDocument();
+ table.columns.forEach(({ name }) => {
+ expect(getByText(name)).toBeInTheDocument();
+ });
});
-});
-test('catalog selector should be visible when enabled in the database', async
() => {
- const { container, getByText, getByRole } = await renderAndWait(
- mockedProps,
- undefined,
- {
+ test('should toggle the table when the header is clicked', async () => {
+ const { container } = await renderAndWait(mockedProps, undefined, {
...initialState,
sqlLab: {
...initialState.sqlLab,
+ tables: [table],
unsavedQueryEditor: {
id: mockedProps.queryEditorId,
dbId: mockData.database.id,
},
- tables: [table],
databases: {
[mockData.database.id]: {
...mockData.database,
allow_multi_catalog: true,
},
},
},
- },
- );
+ });
+ await switchToSelectView();
- const dbSelect = getByRole('combobox', {
- name: 'Select database or type to search databases',
- });
- const catalogSelect = getByRole('combobox', {
- name: 'Select catalog or type to search catalogs',
- });
- const schemaSelect = getByRole('combobox', {
- name: 'Select schema or type to search schemas',
- });
- const dropdown = getByText(/Select table/i);
- const abUser = getByText(/ab_user/i);
+ const header = container.querySelector('.ant-collapse-header');
+ expect(header).toBeInTheDocument();
- expect(getByText(/Database/i)).toBeInTheDocument();
- expect(dbSelect).toBeInTheDocument();
- expect(catalogSelect).toBeInTheDocument();
- expect(schemaSelect).toBeInTheDocument();
- expect(dropdown).toBeInTheDocument();
- expect(abUser).toBeInTheDocument();
- expect(
- container.querySelector('.ant-collapse-content-active'),
- ).toBeInTheDocument();
- table.columns.forEach(({ name }) => {
- expect(getByText(name)).toBeInTheDocument();
- });
-});
+ if (header) {
+ userEvent.click(header);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing await for async userEvent.click</b></div>
<div id="fix">
The userEvent.click call lacks await, which can cause test flakiness since
userEvent.click is asynchronous in @testing-library/user-event v12.8.3. This
matches the pattern used elsewhere in the file, like in switchToSelectView.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
await userEvent.click(header);
````
</div>
</details>
</div>
<small><i>Code Review Run #9ba2ef</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/AppLayout/index.tsx:
##########
@@ -99,47 +139,59 @@ const AppLayout: React.FC = ({ children }) => {
return (
<StyledContainer>
- <Splitter
- css={css`
- flex: 1;
- `}
- lazy
- onResizeEnd={onSidebarChange}
- onResize={noop}
- >
- <Splitter.Panel
- collapsible={{
- start: true,
- end: true,
- showCollapsibleIcon: true,
- }}
- size={leftWidth}
- min={SQL_EDITOR_LEFTBAR_WIDTH}
+ <StyledSidebarWrapper>
+ <Layout.Sider
+ collapsed
+ collapsedWidth={SQL_EDITOR_LEFTBAR_COLLAPSED_WIDTH}
+ >
+ <StyledMenu
+ mode="vertical"
+ items={collapsedMenuItems}
+ selectable={false}
+ />
+ </Layout.Sider>
+ <Splitter
+ css={css`
+ flex: 1;
+ `}
+ lazy
+ onResizeEnd={onSidebarChange}
+ onResize={noop}
>
- <StyledSidebar>
- <SqlEditorLeftBar
- key={queryEditorId}
- queryEditorId={queryEditorId}
- />
- </StyledSidebar>
- </Splitter.Panel>
- <Splitter.Panel className="sqllab-body">{children}</Splitter.Panel>
- {contributions.length > 0 && (
<Splitter.Panel
collapsible={{
start: true,
end: true,
showCollapsibleIcon: true,
}}
- size={rightWidth}
- min={SQL_EDITOR_RIGHTBAR_WIDTH}
+ size={leftWidth - SQL_EDITOR_LEFTBAR_COLLAPSED_WIDTH}
+ min={SQL_EDITOR_LEFTBAR_WIDTH - SQL_EDITOR_LEFTBAR_COLLAPSED_WIDTH}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Sidebar collapse blocked</b></div>
<div id="fix">
The sidebar panel's min size is set to 344px, but since the layout now
includes a collapsed sider and the panel can shrink to 0 when toggled, this
prevents collapsing. Setting min to 0 enables proper toggle behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
min={0}
````
</div>
</details>
</div>
<small><i>Code Review Run #9ba2ef</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]
