codeant-ai-for-open-source[bot] commented on code in PR #41352:
URL: https://github.com/apache/superset/pull/41352#discussion_r3464431019
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -132,15 +132,29 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
return typeof dashboard?.value === 'string';
}
- canOverwriteSlice(): boolean {
- return (
- (this.props.can_overwrite ||
- isUserAdmin(this.props.user) ||
- this.props.slice?.owners?.includes(this.props.user.userId)) &&
- !this.props.slice?.is_managed_externally
+ isCurrentUserOwner(): boolean {
+ const userId = this.props.user?.userId;
+ if (userId === undefined) {
+ return false;
+ }
+ // Owners can arrive either as plain ids (number) or as objects depending
on
+ // where the slice was hydrated from (bootstrap vs. SLICE_UPDATED reducer),
+ // so normalize to the numeric id before comparing.
+ return Boolean(
+ this.props.slice?.owners?.some((owner: number | { id?: number }) =>
+ typeof owner === 'number' ? owner === userId : owner?.id === userId,
+ ),
);
}
+ canOverwriteSlice(): boolean {
+ const canEdit =
+ this.props.can_overwrite ||
+ isUserAdmin(this.props.user) ||
+ this.isCurrentUserOwner();
+ return canEdit && !this.props.slice?.is_managed_externally;
Review Comment:
**Suggestion:** `canOverwriteSlice()` no longer requires an existing slice,
so admin users (or any case where `can_overwrite` is true) can get overwrite
enabled even when creating a brand-new chart (`slice` is null). In that state
the modal defaults to overwrite, and `saveOrOverwrite()` will call
`updateSlice(this.props.slice, ...)` with an undefined slice, causing a runtime
failure when `updateSlice` destructures `slice_id`. Require `this.props.slice`
to exist before returning true for overwrite. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Admins creating new charts hit save runtime error.
- ❌ Explore Save chart modal crashes on overwrite selection.
- ⚠️ Confusing default overwrite state for new unsaved charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Log in as a user with Admin role and open Explore on a dataset without a
`slice_id` in
the URL (new unsaved chart). The Explore bootstrap in
`superset-frontend/src/explore/actions/hydrateExplore.ts:21-26,35-37`
initializes
`explore.slice` to `initialSlice`, which is `null` for a brand-new chart,
and sets
`explore.metadata`/`explore.can_overwrite` while the user object carries
admin roles.
2. With this state, `SaveModal` is wired via `mapStateToProps` in
`superset-frontend/src/explore/components/SaveModal.tsx:190-204`, so it
receives
`props.slice = null`, `props.can_overwrite` (from `explore.can_overwrite`),
and an admin
`props.user`. In the constructor at `SaveModal.tsx:108-121`,
`this.canOverwriteSlice()`
(lines 150-155) evaluates `canEdit` as `true` because
`isUserAdmin(this.props.user)`
(defined in `src/dashboard/util/permissionUtils.ts:10-16`) returns true, and
`!this.props.slice?.is_managed_externally` is also `true` because `slice` is
`null`, so
`canOverwriteSlice()` returns `true`.
3. Because `canOverwriteSlice()` is `true`, the constructor initializes
`this.state.action` to `ChartStatusType.overwrite` (lines 111-116), and in
`renderSaveChartModal` (`SaveModal.tsx:222-258`) the "Save (Overwrite)"
radio is enabled
and checked even though `props.slice` is `null`. The Explore header's Save
button, as
exercised in
`superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx:18-50`,
calls `saveModalActions.setSaveChartModalVisibility(true)`, making this
modal visible in
that state.
4. In the modal footer (`renderFooter` at `SaveModal.tsx:113-151`), clicking
the "Save"
button calls `this.saveOrOverwrite(false)` (`SaveModal.tsx:253-154`). Inside
`saveOrOverwrite`, when `this.state.action === 'overwrite'`, it executes
`this.props.actions.updateSlice(this.props.slice, ...)` at lines 84-95 with
`this.props.slice` still `undefined`. The `updateSlice` thunk in
`superset-frontend/src/explore/actions/saveModalActions.ts:7-18` immediately
destructures
`const { slice_id, owners, form_data: formDataFromSlice } = slice;`, causing
a runtime
TypeError ("Cannot destructure property 'slice_id' of 'slice' as it is
undefined") and
aborting the save, so admins cannot save a brand-new chart in this scenario.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af68144a689047ccb2c3fb3d415a36b4&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=af68144a689047ccb2c3fb3d415a36b4&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:** 150:155
**Comment:**
*Incorrect Condition Logic: `canOverwriteSlice()` no longer requires an
existing slice, so admin users (or any case where `can_overwrite` is true) can
get overwrite enabled even when creating a brand-new chart (`slice` is null).
In that state the modal defaults to overwrite, and `saveOrOverwrite()` will
call `updateSlice(this.props.slice, ...)` with an undefined slice, causing a
runtime failure when `updateSlice` destructures `slice_id`. Require
`this.props.slice` to exist before returning true for overwrite.
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%2F41352&comment_hash=17852e8ce3d83ccc47a551153567e8c40f16356a11cd469acb579193d1df72a7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41352&comment_hash=17852e8ce3d83ccc47a551153567e8c40f16356a11cd469acb579193d1df72a7&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]
