EnxDev commented on code in PR #44345:
URL: https://github.com/apache/superset/pull/44345#discussion_r4080939545
##########
superset-frontend/src/dashboard/reducers/dashboardState.test.ts:
##########
@@ -274,6 +274,58 @@ describe('DashboardState reducer', () => {
expect.arrayContaining(['TAB-Outer1', 'TAB-Inner1']),
);
});
+
+ // The reported permalink failure was not a wrong tab selection but a
+ // payload the API rejected: an unresolved tab id reached activeTabs and
+ // JSON.stringify coerced it to `null` inside the array. The component
+ // tests assert on a `setActiveTab` mock, so they stop short of the state
+ // that actually gets serialized. These two cases pin the serialization
+ // boundary itself.
+ test('stores a resolved tab id so activeTabs serializes without null', ()
=> {
+ const store = mockStore({
+ dashboardState: { activeTabs: [] },
+ dashboardLayout: { present: { 'TAB-1': { parents: [] } } },
+ });
+ const thunkAction = setActiveTab('TAB-1')(
+ store.dispatch,
+ store.getState as () => RootState,
+ );
+
+ const result = typedDashboardStateReducer(
+ createMockDashboardState({ activeTabs: [] }),
+ thunkAction,
+ );
+
+ expect(result.activeTabs).toEqual(['TAB-1']);
+ expect(JSON.stringify({ activeTabs: result.activeTabs })).toBe(
+ '{"activeTabs":["TAB-1"]}',
+ );
+ });
+
+ test('does not sanitize an unresolved tab id, so callers must not dispatch
one', () => {
Review Comment:
Dropped it. Flipping it to assert filtering would mean asserting behaviour
the reducer does not have, so it would just be red — and the version I had
would indeed block that hardening later. The sibling case (a resolved id
serializes without a `null`) stays.
##########
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]]);
+ expect(screen.getByRole('tab')).toHaveAttribute('aria-selected', 'true');
+});
+
+test.each([false, true])(
+ 'A populated TABS component registers its active tab (editMode=%s)',
+ editMode => {
+ // Positive control for the childless guard. Every other `setActiveTab`
+ // assertion here covers an empty container, so nothing pins down the
+ // ordinary path: a container that mounts with children must still register
+ // its first tab. Without this, a guard that is too broad -- suppressing
+ // registration for populated containers too -- would leave the suite green
+ // while breaking every tabbed dashboard.
+ const props = createProps();
+ props.editMode = editMode;
+
+ render(<Tabs {...props} />, {
+ useRedux: true,
+ useDnd: true,
+ });
+
+ expect(props.setActiveTab.mock.calls).toEqual([['TAB-AsMaxdYL_t']]);
+ },
+);
+
+test('An empty TABS component contributes nothing alongside a populated one',
() => {
Review Comment:
Right — separate mocks, so it was the childless and populated cases re-run
side by side. Removed.
--
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]
