fix(explore): let chart owners overwrite when owners come through as objects by rusackas · Pull Request #41352 · apache/superset

rusackas · 2026-06-24T03:48:09Z

SUMMARY

The Save chart modal disables Save (Overwrite) for users who actually own the chart, forcing them into Save as.... That's the path behind the duplicate/stale-chart mess in #38911.

canOverwriteSlice() was checking slice.owners.includes(user.userId). But Slice.owners is typed as { id: number }[], and the Explore bootstrap can hand us owners in object form — so includes() against a numeric userId never matches. Admins squeaked by on the isUserAdmin branch, but plain owners didn't, which is exactly the reporter's scenario.

Fix is just to normalize the owner id before comparing, so it works whether owners arrive as plain ids or as { id } objects. Pulled the check out into a little isCurrentUserOwner() helper to keep canOverwriteSlice() readable.

TESTING INSTRUCTIONS

As a non-admin user who owns a chart, open it in Explore and tweak something (e.g. a description).
Hit Save — Save (Overwrite) should be enabled and selected by default rather than disabled.

Also added a unit test that pins the object-shaped-owners case (it fails on master, passes here).

ADDITIONAL INFORMATION

Has associated issue: Fixes Chart modification - "Save (Overwrite)" not available, "Save As" overwrites original chart and duplicates with stale data (v6) #38911
Changes UI

bito-code-review · 2026-06-24T03:48:18Z

Code Review Agent Run #b437af

Actionable Suggestions - 0

Additional Suggestions - 2

superset-frontend/src/explore/components/SaveModal.tsx - 1
- Missing unit tests for extracted logic · Line 135-156
  
  The new `isCurrentUserOwner()` method at lines 135-147 lacks dedicated unit tests that directly verify its behavior. While line 338's test exercises this path through the UI, rule [6262] requires direct assertion of business logic. Add explicit unit tests covering: undefined userId returns false, numeric owner matches, object owner `{id}` matches, empty owners array, and null slice.
superset-frontend/src/explore/components/SaveModal.test.tsx - 1
- Misleading comment about type · Line 339-341
  
  The comment on line 339 states 'The Slice type declares `owners: { id: number }[]`' but `src/types/Chart.ts` line 76 defines `owners?: number[]`. This creates a misleading reference to the type system. Update the comment to correctly describe the situation: the type expects `number[]` but hydration delivers `{ id: number }[]`.

Review Details

Files reviewed - 2 · Commit Range: 7bfe066..7bfe066
- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

bito-code-review · 2026-06-24T03:56:46Z

The flagged issue is correct. The canOverwriteSlice() method currently evaluates to true for admins even when this.props.slice is null (a new chart), which leads to a runtime error when saveOrOverwrite() attempts to destructure slice_id from an undefined object. To resolve this, you should add a check to ensure this.props.slice exists before allowing an overwrite.

Here is the corrected implementation for canOverwriteSlice in superset-frontend/src/explore/components/SaveModal.tsx:

  canOverwriteSlice(): boolean {
    const canEdit =
      this.props.can_overwrite ||
      isUserAdmin(this.props.user) ||
      this.isCurrentUserOwner();
    return !!this.props.slice && canEdit && !this.props.slice?.is_managed_externally;
  }

There are no other review comments in this pull request to address.

superset-frontend/src/explore/components/SaveModal.tsx

canOverwriteSlice(): boolean {
    const canEdit =
      this.props.can_overwrite ||
      isUserAdmin(this.props.user) ||
      this.isCurrentUserOwner();
    return !!this.props.slice && canEdit && !this.props.slice?.is_managed_externally;
  }

codecov · 2026-06-29T01:30:32Z

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.60%. Comparing base (3dd5e69) to head (7e52419).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...rset-frontend/src/explore/components/SaveModal.tsx	80.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #41352   +/-   ##
=======================================
  Coverage   64.60%   64.60%           
=======================================
  Files        2712     2712           
  Lines      151168   151177    +9     
  Branches    34769    34774    +5     
=======================================
+ Hits        97658    97665    +7     
- Misses      51684    51686    +2     
  Partials     1826     1826

Flag	Coverage Δ
javascript	`69.78% <80.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bito-code-review · 2026-06-29T04:28:48Z

Code Review Agent Run #b9c28c

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: e8c7d66..e8c7d66
- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

bito-code-review · 2026-07-08T14:36:13Z

Code Review Agent Run #b28adf

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: 29cc01d..29cc01d
- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
Files skipped - 0

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

netlify · 2026-07-08T16:10:34Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`7e52419`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a4fcc89259f640008ce348d
😎 Deploy Preview	https://deploy-preview-41352--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

bito-code-review · 2026-07-08T19:14:12Z

Code Review Agent Run #4ad84c

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: 29cc01d..1abcf62
- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

rusackas · 2026-07-09T14:20:37Z

Tried rebasing onto master and this isn't a simple textual conflict — #38831 landed and replaced slice.owners with the Subject-based editors/viewers model in canOverwriteSlice(), so this PR is normalizing a field that no longer exists on that codepath. Worth noting the object-shape bug itself already seems to be handled over there (same typeof === 'number' ternary on editors). Aborted the rebase rather than force something through — need to check whether #38911 still reproduces against the new editors model before reworking this.

The Save chart modal gates "Save (Overwrite)" on canOverwriteSlice(), which checked slice.owners.includes(user.userId). The Slice type declares owners as { id: number }[], and the Explore bootstrap can hydrate owners in object form, so includes() against a numeric userId never matched. A chart owner who wasn't also an admin was therefore forced into "Save as...", which is the path that produces the duplicate/stale-chart behavior reported in the issue. Normalize owner identity to the numeric id before comparing so owners are recognized whether they arrive as plain ids or as objects. Fixes #38911 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

bito-code-review · 2026-07-09T19:33:55Z

Code Review Agent Run #0cdea2

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: 0034fcc..7e52419
- superset-frontend/src/explore/components/SaveModal.test.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

kgabryje

LGTM

…objects (#41352) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

…objects (apache#41352) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

…r test Three findings from the consolidated review, each verified against the code and each with a control run showing the new test fails against the pre-fix source. Restore was offered on externally managed dashboards. The header menu ANDs !is_managed_externally, but the history panel derived canRestore from dash_edit_perm alone -- and the panel also opens from ?version_history=true, so the menu's gate was not a gate at all. version_restore.py checks editorship and nothing else (is_managed_externally appears nowhere under superset/commands/), which left the UI as the only guard. Both consumers now share selectCanRestoreDashboard, mirroring canOverwriteSlice on the chart side; keeping it in one place is the point, since this bug was two copies of one rule drifting apart. The Save toolbar stayed live during a version preview. userCanEdit and userCanShare are both gated on !isVersionPreviewActive; userCanSaveAs was not, and the toolbar acts on whatever is hydrated -- which during a preview is the snapshot's position_data. Gated it, and made the preview hydrate pass editMode: false explicitly rather than letting hydrate re-derive it from the `edit` URL param, which outlives the navigation that set it. Note the related "in-progress edits vanish" concern does not apply: handlePreview already refuses to preview while hasUnsavedChanges and toasts. The owner overwrite test asserted nothing. SaveModal's extraction to canOverwriteSlice dropped the owner route from apache#41352, and this is the one change here outside VERSION_HISTORY. The direction is right -- UpdateChartCommand gates on raise_for_editorship, and is_editor is admin or subjects intersected with editors/extra_editors, with no owners fallback, so such an owner is answered 403 by the API and the old modal only surfaced that after the request. But the test named for it passed via the shared fixture's editors: [{id: 1}] and would have stayed green either way. Rewritten to assert the behaviour that ships, plus a companion for an owner who is an editor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pull-request-size Bot added the size/M label Jun 24, 2026

dosubot Bot added the explore:save Related to saving changes in Explore label Jun 24, 2026

codeant-ai-for-open-source Bot reviewed Jun 24, 2026

View reviewed changes

Comment thread superset-frontend/src/explore/components/SaveModal.tsx Outdated

eschutho requested a review from kgabryje June 24, 2026 07:10

rusackas requested review from betodealmeida, michael-s-molina, villebro and yousoph June 24, 2026 17:33

rusackas force-pushed the fix/issue-38911-overwrite-owner-objects branch from 7bfe066 to e8c7d66 Compare June 29, 2026 01:20

rusackas requested review from dpgaspar and sha174n June 29, 2026 18:06

rusackas force-pushed the fix/issue-38911-overwrite-owner-objects branch from e8c7d66 to 29cc01d Compare July 8, 2026 11:28

codeant-ai-for-open-source Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread superset-frontend/src/explore/components/SaveModal.test.tsx Outdated

Comment thread superset-frontend/src/explore/components/SaveModal.tsx Outdated

rusackas and others added 2 commits July 9, 2026 09:22

fix: correct inaccurate owner-shape comments in SaveModal

7e52419

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rusackas force-pushed the fix/issue-38911-overwrite-owner-objects branch from 1abcf62 to 7e52419 Compare July 9, 2026 16:29

kgabryje approved these changes Jul 13, 2026

View reviewed changes

rusackas merged commit c0e5f52 into master Jul 13, 2026
67 checks passed

rusackas deleted the fix/issue-38911-overwrite-owner-objects branch July 13, 2026 10:10

hainenber pushed a commit that referenced this pull request Jul 18, 2026

fix(explore): let chart owners overwrite when owners come through as …

3807670

…objects (#41352) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

rdg0 pushed a commit to rdg0/superset that referenced this pull request Jul 20, 2026

fix(explore): let chart owners overwrite when owners come through as …

bd3c029

…objects (apache#41352) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

mikebridge mentioned this pull request Jul 30, 2026

feat(versioning): version-history UI #41551

Merged

10 tasks

Uh oh!

Conversation

rusackas commented Jun 24, 2026

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #b437af

Uh oh!

Uh oh!

bito-code-review Bot commented Jun 24, 2026

Uh oh!

codecov Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bito-code-review Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #b9c28c

Uh oh!

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #b28adf

Uh oh!

netlify Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

bito-code-review Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #4ad84c

Uh oh!

rusackas commented Jul 9, 2026

Uh oh!

bito-code-review Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #0cdea2

Uh oh!

kgabryje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bito-code-review Bot commented Jun 24, 2026 •

edited

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading

bito-code-review Bot commented Jun 29, 2026 •

edited

Loading

bito-code-review Bot commented Jul 8, 2026 •

edited

Loading

netlify Bot commented Jul 8, 2026 •

edited

Loading

bito-code-review Bot commented Jul 8, 2026 •

edited

Loading

bito-code-review Bot commented Jul 9, 2026 •

edited

Loading