You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/10/20 17:46:56 UTC
[PR] fix(dashboard): don't clone charts a copied layout leaves out [superset]
mikebridge opened a new pull request, #45528:
URL: https://github.com/apache/superset/pull/45528
### SUMMARY
**What users saw.** Copying a dashboard with “duplicate charts” could create unused charts when the copied layout left out charts from the source dashboard, including when opening an older version as a new dashboard. With history capture enabled, the copy could also fail.
**Why, in plain terms.** The copy created every source chart first, then removed charts that the copied layout did not use. Those charts had already been saved.
**What this changes.** Only charts present in the copied layout are duplicated. Existing handling of archived or deleted charts and legacy chart IDs is preserved.
The clone loop uses the reconciled layout's normalized chart IDs before creating any chart rows. Regression tests assert total chart counts and orphan counts for subset and empty layouts.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: this corrects backend copy behavior without a UI change.
### TESTING INSTRUCTIONS
1. Create a dashboard with two charts.
2. Copy it with chart duplication enabled and a layout containing only one source chart, then repeat with an empty layout.
3. Confirm the copies contain one and zero charts respectively, no unused chart copies are created, and the source keeps both charts.
Validated locally: 311 DAO/dashboard unit tests, all 11 dashboard-copy API tests using isolated SQLite, and changed-file pre-commit checks including MyPy. The new duplicate-copy regressions fail before the fix.
### 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
--
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 clone charts a copied layout leaves out [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45528:
URL: https://github.com/apache/superset/pull/45528#issuecomment-6299876099
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; chart duplication is limited to charts retained in the copied dashboard layout.
- Tests check that dropped charts are not cloned or left orphaned, while retained charts preserve the expected copy behavior.
**Assessed commit:** `ca3aabef0332`
--
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 clone charts a copied layout leaves out [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #45528:
URL: https://github.com/apache/superset/pull/45528#discussion_r4330793742
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
+ chart_id
+ for node in metadata["positions"].values()
+ if (chart_id := _layout_chart_id(node)) is not None
+ }
# Duplicating slices as well, mapping old ids to new ones
for slc in original_dash.slices:
- if slc.id in archived_ids:
+ if slc.id in archived_ids or slc.id not in layout_chart_ids:
Review Comment:
Non-blocking. A side effect of skipping omitted charts: `old_to_new_slice_ids` no longer has entries for them, so `_remap_filter_scopes` leaves their IDs as the source's IDs. Before this change they were rewritten to orphan-clone IDs. Example: source has A=1 and B=2, the layout keeps only A, and A is cloned to 4. The copy's `json_metadata` ends up with:
- `chart_configuration` keys `"2"` and `"4"`. The `"2"` entry belongs to the source's live chart B.
- `chart_configuration["4"].crossFilters.chartsInScope == [2]`.
- A native filter's `chartsInScope == [4, 2]` and `scope.excluded == [2]`.
The references were dangling before this change too. Now they point at a live chart on another dashboard. If B is later added to the copy, it picks up the stale cross-filter config. The follow-up suggested on #45472 (prune metadata to the layout's charts) is now the natural complement: after the remap, drop any ID in these structures that isn't in `old_to_new_slice_ids.values()`.
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
Review Comment:
Non-blocking, pre-existing. `timed_refresh_immune_slices` and `expanded_slices` are never remapped on the duplicate path, even for charts that *are* cloned. In the A=1 to clone 4 case above, the copy keeps `timed_refresh_immune_slices: [1, 2]` and `expanded_slices: {"1": true, "2": true}`. As a result, every duplicated copy loses refresh immunity and expanded state for its charts. `_remap_filter_scopes` covers neither key. Out of scope here, but worth handling in the same follow-up: remap through `old_to_new_slice_ids` and prune to the layout. (`default_filters` has a similar problem: its old-ID keys are filtered against the new `slice_ids`, so they are all dropped.)
--
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 clone charts a copied layout leaves out [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #45528:
URL: https://github.com/apache/superset/pull/45528
--
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 clone charts a copied layout leaves out [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45528:
URL: https://github.com/apache/superset/pull/45528#discussion_r4380583003
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
+ chart_id
+ for node in metadata["positions"].values()
+ if (chart_id := _layout_chart_id(node)) is not None
+ }
# Duplicating slices as well, mapping old ids to new ones
for slc in original_dash.slices:
- if slc.id in archived_ids:
+ if slc.id in archived_ids or slc.id not in layout_chart_ids:
Review Comment:
Thanks. Updated the version-history docs to say that only live charts in the copied layout are cloned, so opening an older version won't bring along charts added later.
--
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 clone charts a copied layout leaves out [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45528:
URL: https://github.com/apache/superset/pull/45528#discussion_r4331234897
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
+ chart_id
+ for node in metadata["positions"].values()
+ if (chart_id := _layout_chart_id(node)) is not None
+ }
# Duplicating slices as well, mapping old ids to new ones
for slc in original_dash.slices:
- if slc.id in archived_ids:
+ if slc.id in archived_ids or slc.id not in layout_chart_ids:
Review Comment:
Thanks, Matt. Confirmed: the fallback keeps omitted IDs in the source namespace on this branch. The stacked #45530 prunes chart configuration and filter scopes to retained chart IDs in 46b2623bf2, so I'll keep that cleanup there rather than expand this clone-count fix.
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
Review Comment:
Thanks, confirmed these are pre-existing. The stacked #45530 remaps and prunes expanded charts and refresh exemptions in 46b2623bf2. Legacy default_filters still needs a separate follow-up; I'm leaving that behavior out of this fix.
--
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 clone charts a copied layout leaves out [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45528:
URL: https://github.com/apache/superset/pull/45528#issuecomment-6423361672
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f0854f85b487e1a96cb43ac5284447451347db90 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6adf92895350d36f0483da8c |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45528--superset-docs-preview.netlify.app](https://deploy-preview-45528--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#Dco7DoAgDADQu3SWROUT9AYeA1sIA9IGXYjx7rK94b0gjS95YAeY4GyhYh6-US2rd0YrZOmKm-RQFRau8R6PohTuB43pAqVt9ZvVdibt0my8puARvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45528--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 clone charts a copied layout leaves out [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45528:
URL: https://github.com/apache/superset/pull/45528#issuecomment-6297788339
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45528?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 65.03%. Comparing base ([`ad30c89`](https://app.codecov.io/gh/apache/superset/commit/ad30c898f8645d6333fb0f0051adc72804a789fd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`292e593`](https://app.codecov.io/gh/apache/superset/commit/292e5930e907c6ef792a2bbc15da257092566273?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (ad30c89) and HEAD (292e593). Click for more details.
>
> <details><summary>HEAD has 5 uploads less than BASE</summary>
>
>| Flag | BASE (ad30c89) | HEAD (292e593) |
>|------|------|------|
>|python|7|3|
>|postgres|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45528 +/- ##
===========================================
- Coverage 83.65% 65.03% -18.62%
===========================================
Files 3053 3053
Lines 200647 200648 +1
Branches 46823 46823
===========================================
- Hits 167851 130492 -37359
- Misses 29555 67207 +37652
+ Partials 3241 2949 -292
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.91% <0.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.88% <100.00%> (-0.01%)` | :arrow_down: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.66% <0.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.96% <100.00%> (-34.22%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45528/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/45528?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
Re: [PR] fix(dashboard): don't clone charts a copied layout leaves out [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45528:
URL: https://github.com/apache/superset/pull/45528#discussion_r4366064739
##########
superset/daos/dashboard.py:
##########
@@ -969,9 +969,15 @@ def copy_dashboard(
# the raw-layout markdown placeholder while preserving geometry;
# the remap below skips that slot because it has no chartId.
reconcile_position_json(metadata.get("positions"), original_dash.id)
+ chart_id: int | None
+ layout_chart_ids: set[int] = {
+ chart_id
+ for node in metadata["positions"].values()
+ if (chart_id := _layout_chart_id(node)) is not None
+ }
# Duplicating slices as well, mapping old ids to new ones
for slc in original_dash.slices:
- if slc.id in archived_ids:
+ if slc.id in archived_ids or slc.id not in layout_chart_ids:
Review Comment:
This changes what "Open as new" does for a version whose layout predates some of the current charts, and the docs still describe the old behaviour.
`docs/docs/using-superset/version-history.mdx:321` says "Live charts are still cloned" under copying with charts. Worth a clause there saying only the charts in the copied layout are cloned, so a user opening an older version isn't surprised when newer charts don't come along.
--
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