msyavuz commented on code in PR #44345:
URL: https://github.com/apache/superset/pull/44345#discussion_r4080648052
##########
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:
Each render has its own `setActiveTab` mock, so this is just the childless
and populated tests run side by side; I think it can go.
##########
docs/docs/using-superset/creating-your-first-dashboard.mdx:
##########
@@ -433,6 +433,11 @@ The dropdown menu is briefly hidden while the screenshot
or PDF is being capture
These menu items respect your permissions: the dashboard export menu only
appears if you can download, and the image/PDF options are disabled if you lack
image-export permission.
+### Editing Dashboard Tabs
Review Comment:
Selecting a newly added tab is expected behavior rather than a feature, so
I'd drop this docs section.
##########
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:
Review Comment:
The multi-paragraph comments on the new tests mostly restate the test names;
one line here would be enough, and the rest could be dropped.
##########
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:
This pins the reducer to emitting `null`, so making the reducer drop falsy
ids later (the correct hardening) turns it red. Could we drop it, or flip it to
assert `undefined` is filtered?
##########
superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.tsx:
##########
@@ -166,6 +166,23 @@ const Tabs = (props: TabsProps): ReactElement => {
const prevTabIds = usePrevious(props.component.children);
useEffect(() => {
+ // Resolve missing or deleted active keys when children become available
+ // so a tab added to an empty container is selected and registered.
+ const tabId = props.component.children[selectedTabIndex];
+ if (!props.component.children.includes(activeKey) && tabId) {
+ setActiveKey(tabId);
+ }
+ }, [activeKey, props.component.children, selectedTabIndex]);
+
+ useEffect(() => {
+ // A TABS component with no children resolves no tab id, so there is
+ // nothing to activate. Dispatching the unresolved id would register an
+ // `undefined` entry in dashboardState.activeTabs, which JSON.stringify
+ // coerces to `null` when the dashboard state is posted to the permalink
+ // endpoint.
+ if (!activeKey) {
Review Comment:
nit: `activeKey` is typed `useState<string>` but can be `undefined`, which
is what this guard handles; `useState<string | undefined>` would make that
explicit.
--
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]
