EnxDev opened a new pull request, #44345:
URL: https://github.com/apache/superset/pull/44345
### SUMMARY
Empty tab containers were quietly writing a `null` into
`dashboardState.activeTabs`, which then ended up in the dashboard permalink
request body.
`Tabs.tsx` figures out which tab is active by reading
`component.children[tabIndex]`. If the TABS component has no children, that
read returns `undefined` — but the mount effect dispatched it through
`setActiveTab` anyway, so `findTabsToRestore` merged an `undefined` entry into
`activeTabs`. Since `JSON.stringify` turns `undefined` into `null` inside
arrays, the POST to `/api/v1/dashboard/{id}/permalink` went out looking like
this:
```json
"activeTabs": ["TAB-OTSsSEwZ0", null, "TAB-8l5p-0X1HcVN6DBgYoMy6",
"TAB-Zgq03BIpvp"]
```
The fix is a guard in the effect: if there's no tab id to activate, don't
dispatch. `handleClickTab` already had the same check (`if
(tabIds[tabIndex])`), and `getDefaultActiveTabs` already skips childless
containers, so this was the last path left that could seed an unresolved id.
A couple of things about the original report are worth writing down, because
both look mysterious and both turn out to be the same thing:
- **The position of the `null` moved around** between reproductions. That's
just component mount order — nothing meaningful.
- **Only *some* empty TABS containers triggered it**, and it looked like
nesting depth mattered. It isn't depth, it's mounting: tab content deeper in
the tree isn't mounted until its parent tab is selected, so only containers
present on the initial render ever got the chance to dispatch. Same reason
switching to another tab and back "fixed" it — the array got rebuilt.
**One important note on scope.** This was originally reported as an HTTP
400, and on the reporter's version (6.0.0.30) it is. But the backend half is
already fixed on master: #40969 added `allow_none=True` to the inner
`fields.String` in `DashboardPermalinkStateSchema`, so a `null` entry no longer
gets rejected. **This PR does not fix a 400 on master** — the 400 can't
reproduce here. What it fixes is the frontend defect that puts the `null` there
in the first place, which is still writing junk into permalink state today. If
the 400 itself needs addressing for 6.0.x, that's a backport of #40969, not
this change.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no visual change. The difference is in the request payload:
Before:
```json
"activeTabs": ["TAB-OTSsSEwZ0", null, "TAB-8l5p-0X1HcVN6DBgYoMy6"]
```
After:
```json
"activeTabs": ["TAB-OTSsSEwZ0", "TAB-8l5p-0X1HcVN6DBgYoMy6"]
```
### TESTING INSTRUCTIONS
Automated:
```bash
cd superset-frontend
npm run test -- src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx
```
The new test (`A childless TABS component does not register an active tab`)
renders a TABS component with `children: []` and asserts `setActiveTab` is
never called. On unfixed code it fails with `Received number of calls: 1` / `1:
undefined`, which is the bug in one line.
Manual:
1. Build a dashboard with tabs, and inside the tab that's active on load,
drop in a **Tabs** component and delete all of its child tabs so the container
is empty.
2. Save, then open the dashboard fresh via direct URL. Don't click into the
tab first — clicking is the workaround and will mask the issue.
3. Open DevTools → Network, filter Fetch/XHR.
4. `...` → **Share** → **Copy permalink to clipboard**.
5. Check the request payload for `POST /api/v1/dashboard/{id}/permalink`.
`activeTabs` should contain only real tab ids and no `null`.
6. Same check via the inline link icon next to the tab title.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
---
Ran `npm run test` over `src/dashboard/components/gridComponents`,
`src/dashboard/reducers`, and `src/dashboard/actions` — 31 suites, 363 tests,
all passing.
A note for reviewers on pre-commit: the `Type-Checking (Frontend)` hook
currently fails on `master` with 116 pre-existing errors in unrelated files.
None are in the files this PR touches, so I left that baseline alone rather
than dragging unrelated fixes into this diff. Every other hook passes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
