You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2026/09/14 07:24:25 UTC
[PR] fix(dashboard): don't break the dashboard on a self-referencing layout [superset]
msyavuz opened a new pull request, #44235:
URL: https://github.com/apache/superset/pull/44235
### SUMMARY
A dashboard's stored `position_json` is not guaranteed to be a tree. It can name a component that is
also one of its own ancestors. Nothing on the write path rejects that: `DashboardPutSchema.position_json`
is validated with `validate_json`, which only checks that the string parses.
When such a layout is loaded, two things go wrong in sequence:
1. `updateComponentParentsList` walks the layout's children recursively with no record of what it has
already visited. Following a self-reference recurses until the stack overflows with
`RangeError: Maximum call stack size exceeded`. It runs during hydration
(`actions/hydrate.ts`) and again after every layout action (`reducers/dashboardLayout.ts`), so the
dashboard fails to render and every subsequent edit throws too.
2. With that fixed, hydration succeeds and the cyclic layout is handed to the React tree, where the
components render each other without bound until the renderer dies — so the dashboard still never
appears.
This fixes both:
- `updateComponentParentsList` now tracks visited components and skips one the layout reaches more than
once, logging a warning. This also stops a component reachable from two parents being re-walked once
per path, which was exponential on a deep layout.
- `DashboardComponent` is the single container every nested component renders through, so the repeat is
guarded there: a component cannot be its own ancestor, so when the layout reaches one from its own
descendant it renders nothing and logs a warning.
Both mirror how the backend already degrades on the same data condition — `Dashboard.tabs` in
`models/dashboard.py` carries a `visited` set and warns rather than raising (#43575).
The guard suppresses the cyclic subtree rather than repairing it, so an affected dashboard opens with
those components missing. That is deliberate and is enough to reach edit mode and fix the layout by hand,
which is not possible today. Adding server-side structural validation of `position_json` would stop new
corruption being written, but it turns payloads that are accepted today into 400s, so it is left out of
this change — #43574 called that out as needing its own discussion.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before — dashboard never renders, `Maximum call stack size exceeded`, no way into edit mode:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cd-e8f7-42ac-83d2-d54506368134/tc2-before.webm
After — dashboard renders, no console errors, edit mode works:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cd-5a2c-4b57-8f9d-3260cf529d68/tc2-after.webm
A healthy, acyclic dashboard is unaffected:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cf-9bc1-4a22-9342-a6ee833f9f8e/tc3-after.webm
### TESTING INSTRUCTIONS
Automated:
```bash
cd superset-frontend
npm run test -- src/dashboard/util/updateComponentParentsList.test.ts src/dashboard/containers/DashboardComponent.test.tsx
```
Both suites fail on `master` (the first with `RangeError: Maximum call stack size exceeded`) and pass with
this change.
Manual:
1. Create a dashboard, then `PUT /api/v1/dashboard/{id}` a `position_json` where a component is reachable
from its own descendant, e.g. `GRID_ID -> ROW-a -> ROW-b -> ROW-a`. The API accepts it.
2. Open that dashboard. On `master` the page stays blank and the console shows
`Maximum call stack size exceeded`; with this change the dashboard renders, the console is clean, and
"Edit dashboard" works so the layout can be repaired.
3. Open any normal dashboard and confirm it is unchanged.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): don't break the dashboard on a self-referencing layout [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44235:
URL: https://github.com/apache/superset/pull/44235#discussion_r4003014529
##########
superset-frontend/src/dashboard/containers/DashboardComponent.tsx:
##########
@@ -156,9 +157,25 @@ const DashboardComponent = (
return {};
}, [component, dashboardLayout, props.id]);
- const Component = component
- ? componentLookup[component.type as keyof typeof componentLookup]
- : null;
+ // A stored layout is not guaranteed to be a tree: it can name a component
+ // that is also one of its own ancestors. Every nested component renders
+ // through this container, so rendering that repeat would recurse without
+ // end and the dashboard would never appear. Stop at the repeat instead, so
+ // the rest of the dashboard still renders and the layout can be repaired.
+ const isRepeatedAncestor =
+ props.id === props.parentId ||
+ Boolean(parentComponent?.parents?.includes(props.id));
Review Comment:
**Suggestion:** The repeat check runs after width calculation, so cyclic rows recurse in `getDetailedComponentWidth` before this guard and still overflow the stack during rendering. [logic error]
**Assessment:** 🔴 `Critical` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8f9bc71bdb7f487897d7ee15aaedac39&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8f9bc71bdb7f487897d7ee15aaedac39&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardComponent.tsx
**Line:** 165:167
**Comment:**
*Logic Error: The repeat check runs after width calculation, so cyclic rows recurse in `getDetailedComponentWidth` before this guard and still overflow the stack during rendering.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44235&comment_hash=ecfd40596f9c1bc3b0e4cb630e18414eca4d1e3fed48196078ad54255efdf817&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44235&comment_hash=ecfd40596f9c1bc3b0e4cb630e18414eca4d1e3fed48196078ad54255efdf817&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/updateComponentParentsList.ts:
##########
@@ -44,7 +59,12 @@ export default function updateComponentParentsList({
if (Array.isArray(currentComponent.children)) {
currentComponent.children.forEach(childId => {
- if (layout[childId]) {
+ if (visited.has(childId)) {
+ logging.warn(
+ `The current layout reaches the component with the id: ${childId} more than once. Skipping this component`,
+ );
Review Comment:
**Suggestion:** Skipping later paths leaves a shared component's `parents` set to its first path, so rendering it under another parent can use the wrong tab and navigation ancestry. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=32962b6425554169a4b0f7a957fdf795&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=32962b6425554169a4b0f7a957fdf795&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/updateComponentParentsList.ts
**Line:** 62:65
**Comment:**
*Api Mismatch: Skipping later paths leaves a shared component's `parents` set to its first path, so rendering it under another parent can use the wrong tab and navigation ancestry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44235&comment_hash=d8831e3ee7de976d4114b5882c0e0794dc9d259a2f738fd5245c7d6345f0c6aa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44235&comment_hash=d8831e3ee7de976d4114b5882c0e0794dc9d259a2f738fd5245c7d6345f0c6aa&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): don't break the dashboard on a self-referencing layout [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44235:
URL: https://github.com/apache/superset/pull/44235#issuecomment-5660502569
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | ecd48a15324dfb43d5e0e86961504e3ca22f4a1f |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa7a129092f3b00087d9dea |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44235--superset-docs-preview.netlify.app](https://deploy-preview-44235--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBDkAwEAXQu8yaKBLUDRzj16ASNc2ohYi76-4t3ktRJcREI1FBTnHOPhub6J58qBiXdwLl8kE4yllU75h2OfPmJR7yTJx_B_SoG2tss7bOGDP0bHkBfT8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44235--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): don't break the dashboard on a self-referencing layout [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44235:
URL: https://github.com/apache/superset/pull/44235#issuecomment-5660488734
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): don't break the dashboard on a self-referencing layout [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44235:
URL: https://github.com/apache/superset/pull/44235#issuecomment-5660651040
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44235?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.18%. Comparing base ([`d9b0cb0`](https://app.codecov.io/gh/apache/superset/commit/d9b0cb0453877819c39f614ad906706c8a31a07a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ecd48a1`](https://app.codecov.io/gh/apache/superset/commit/ecd48a15324dfb43d5e0e86961504e3ca22f4a1f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44235 +/- ##
=======================================
Coverage 80.17% 80.18%
=======================================
Files 2925 2925
Lines 172672 172681 +9
Branches 40088 40094 +6
=======================================
+ Hits 138444 138459 +15
+ Misses 31631 31625 -6
Partials 2597 2597
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44235/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44235/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.67% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44235?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org