codeant-ai-for-open-source[bot] commented on code in PR #38584:
URL: https://github.com/apache/superset/pull/38584#discussion_r3662412118
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -371,7 +371,10 @@ const SaveModal = ({
if (dashboardId) {
try {
const result = (await loadDashboard(dashboardId)) as Dashboard;
- if (canUserEditDashboard(result, user)) {
+ if (
+ canUserEditDashboard(result, user) &&
+ !result.is_managed_externally
+ ) {
setDashboard({ label: result.dashboard_title, value: result.id });
await loadTabs(dashboardId);
}
Review Comment:
**Suggestion:** When `dashboardIdProp` changes to an externally managed or
non-editable dashboard while the modal remains mounted, this condition simply
skips the setters, leaving the previously selected dashboard, tabs, and tab
selection in state. The user can therefore still save using the stale editable
dashboard. Clear `dashboard`, `tabsData`, and `selectedTab` before loading the
new dashboard or when the new dashboard fails this condition, and cancel stale
asynchronous initialization results. [stale reference]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Charts can be saved to a stale dashboard selection.
- ❌ Managed-dashboard protection can be bypassed through stale state.
- ⚠️ Tabs and redirect anchors can reference the old dashboard.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Explore with the save modal visible while `ExploreViewContainer`
renders
`SaveModal` at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:1087-1094`;
the
modal remains mounted while `isSaveModalVisible` stays true.
2. Start with an editable dashboard ID supplied through `props.dashboardId`,
which is
derived from Explore form data at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:1194-1224`.
`SaveModal` loads that dashboard and stores it in `dashboard` at
`superset-frontend/src/explore/components/SaveModal.tsx:373-380`, while
`loadTabs()` also
stores its tabs and selected tab at
`superset-frontend/src/explore/components/SaveModal.tsx:292-356`.
3. Change the Explore dashboard context so `dashboardIdProp` changes to a
dashboard whose
response has `is_managed_externally: true` or which the user cannot edit. The
initialization effect reruns because `dashboardIdProp` is a dependency at
`superset-frontend/src/explore/components/SaveModal.tsx:421-428`, but the
condition at
`superset-frontend/src/explore/components/SaveModal.tsx:374-380` skips both
setters and
performs no cleanup.
4. Observe that the previous dashboard, tabs, and selected tab remain in
component state.
Clicking Save then enters `saveOrOverwrite()` at
`superset-frontend/src/explore/components/SaveModal.tsx:524-563`, which uses
the stale
`dashboard` state and includes that old dashboard ID in
`formData.dashboards`, allowing
the chart to be saved to the previously selected dashboard instead of
clearing the invalid
selection. Concurrent initialization requests can also apply stale results
because the
effect at `superset-frontend/src/explore/components/SaveModal.tsx:360-420`
has no
cancellation or request-generation check.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e1d8904c4b141cf91692b2f7ee9e6ff&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=4e1d8904c4b141cf91692b2f7ee9e6ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.tsx
**Line:** 374:380
**Comment:**
*Stale Reference: When `dashboardIdProp` changes to an externally
managed or non-editable dashboard while the modal remains mounted, this
condition simply skips the setters, leaving the previously selected dashboard,
tabs, and tab selection in state. The user can therefore still save using the
stale editable dashboard. Clear `dashboard`, `tabsData`, and `selectedTab`
before loading the new dashboard or when the new dashboard fails this
condition, and cancel stale asynchronous initialization results.
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%2F38584&comment_hash=12990a333e64d2dc9db3ef399924b06a2da795fc3f46bc89b9681ded8920dae5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=12990a333e64d2dc9db3ef399924b06a2da795fc3f46bc89b9681ded8920dae5&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: [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]
