bito-code-review[bot] commented on code in PR #44345:
URL: https://github.com/apache/superset/pull/44345#discussion_r4065405332
##########
superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx:
##########
@@ -265,6 +265,144 @@ test('Switching tabs', async () => {
expect(props.onChangeTab).toHaveBeenCalled();
});
+test.each([false, true])(
+ 'A childless TABS component does not register an active tab (editMode=%s)',
+ editMode => {
+ // Regression guard for the permalink failure caused by empty tab
containers:
+ // a TABS component with no children has no tab id to activate, so
resolving
+ // `children[tabIndex]` yields `undefined`. Dispatching that into
+ // `dashboardState.activeTabs` puts an `undefined` entry in the array,
which
+ // `JSON.stringify` coerces to `null` in the permalink request body.
+ const props = createProps();
+ props.editMode = editMode;
+ props.component.children = [];
+
+ render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+ },
+);
+
+test.each([false, true])(
+ 'The first child added to an empty TABS component is activated
(editMode=%s)',
+ editMode => {
+ // Counterpart to the guard above: skipping registration while there is no
+ // tab id must not leave the container permanently unregistered.
`activeKey`
+ // is seeded once from state, so an empty container that later receives its
+ // first child has to resolve and register that child, otherwise the tab
+ // renders unselected and never reaches dashboardState.activeTabs.
+ const props = createProps();
+ const tabId = props.component.children[0];
+ props.editMode = editMode;
+ props.component.children = [];
+ const { rerender } = render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+ rerender(
+ <Tabs {...props} component={{ ...props.component, children: [tabId] }}
/>,
+ );
+
+ // Exactly one registration, carrying the resolved id and no stale previous
+ // tab -- never an `undefined` that JSON.stringify would turn into `null`.
+ expect(props.setActiveTab.mock.calls).toEqual([[tabId]]);
+ expect(screen.getByRole('tab')).toHaveAttribute('aria-selected', 'true');
+ },
+);
+
+test('A tab added after deleting the last tab is selected and registered',
async () => {
+ const props = createProps();
+ const [deletedTabId, newTabId] = props.component.children;
+ props.component.children = [deletedTabId];
+ const { rerender } = render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab.mock.calls).toEqual([[deletedTabId]]);
+ expect(screen.getByRole('tab')).toHaveAttribute('aria-selected', 'true');
+
+ await userEvent.click(screen.getByRole('button', { name: 'remove' }));
+ await userEvent.click(screen.getByRole('button', { name: 'Delete' }));
+ expect(props.deleteComponent).toHaveBeenCalledWith(
+ deletedTabId,
+ props.component.id,
+ );
+
+ rerender(
+ <Tabs {...props} component={{ ...props.component, children: [] }} />,
+ );
+ expect(screen.queryByRole('tab')).not.toBeInTheDocument();
+ props.setActiveTab.mockClear();
+
+ await userEvent.click(screen.getByRole('button', { name: 'Add tab' }));
+ expect(props.createComponent).toHaveBeenCalled();
+ expect(props.setActiveTab).not.toHaveBeenCalled();
+
+ rerender(
+ <Tabs
+ {...props}
+ component={{ ...props.component, children: [newTabId] }}
+ />,
+ );
+
+ expect(props.setActiveTab.mock.calls).toEqual([[newTabId, deletedTabId]]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Vacuous async assertion</b></div>
<div id="fix">
The rerender at line 347 is asynchronous for effects: `setActiveTab` fires
from the effect at `Tabs.tsx:177-191` after commit, so the synchronous
`toEqual` at line 354 can run on an empty `mock.calls` and pass vacuously. Wrap
it in `await waitFor(...)` (already imported) so the test fails if registration
never happens.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
await waitFor(() =>
expect(props.setActiveTab.mock.calls).toEqual([[newTabId,
deletedTabId]]),
);
````
</div>
</details>
</div>
<small><i>Code Review Run #6890c9</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]
