fix: delete Chart under "All" in home page doesn't refresh after dele…#39471
fix: delete Chart under "All" in home page doesn't refresh after dele…#39471AoLiGei1221 wants to merge 25 commits intoapache:masterfrom
Conversation
Code Review Agent Run #8ad1a5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39471 +/- ##
=======================================
Coverage 64.58% 64.58%
=======================================
Files 2564 2564
Lines 133558 133571 +13
Branches 31031 31037 +6
=======================================
+ Hits 86254 86272 +18
+ Misses 45812 45807 -5
Partials 1492 1492
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It looks like not my fault. Could someone help me re-run the CI/CD to check? Thank you! |
|
It seems that things work for dashboards but not for charts in terms of what you're expecting here.Let's see if we can add a failing test for the deletion of charts that gets fixed by this patch.We should also make sure that we're following the same React code patterns that we're using for dashboards in this chart implementation.We might also want to look at saved queries while we're at it if that's going to have a similar problem.If there's any code that can be reused across any and all deletions in the homepage, that would be ideal. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #232926Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Hey @rusackas , Thanks for the feedback! I’ve aligned the chart deletion flow with the dashboard pattern. I also added a regression test covering deletion on ALL tabs to ensure the list refreshes correctly. Please check again. Thank you so much! Screen.Recording.2026-04-20.at.16.35.04.mov |
Code Review Agent Run #c2332bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| menuItems.push({ | ||
| key: 'delete', | ||
| label: ( | ||
| label: onDelete ? ( |
There was a problem hiding this comment.
Is this a whole new button? If so, can we see it in a screenshot?
| ); | ||
| }); | ||
|
|
||
| test('refreshes other tab data after deleting a chart', async () => { |
|
No, this isn't a whole new button. The change modifies the existing delete option in the chart card menu to optionally use a custom onDelete handler for direct deletion, instead of the default confirmation dialog. I can't provide screenshots as I'm a text-based assistant. |
| }, | ||
| ]} | ||
| /> | ||
| {chartToDelete && ( |
There was a problem hiding this comment.
Just checking here, do the other delete buttons have the same kind of modal?If so, is there any code we can reuse here to keep things simple?
There was a problem hiding this comment.
Pull request overview
Fixes the Superset homepage chart list not refreshing after deleting a chart from non-“Mine” tabs when the list is bootstrapped from initialData (so useListViewResource.refreshData() is a no-op without an explicit fetch config).
Changes:
- Adds an optional
refreshDataConfigargument tohandleChartDeleteso callers can force a refresh even whenlastFetchDataConfigwas never initialized. - Updates the homepage
ChartTableto drive chart deletion via aDeleteModaland pass an explicit fetch config tohandleChartDelete. - Adds a regression test covering deletion + refresh behavior on the “All” (other) tab.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/src/views/CRUD/utils.tsx | Extends handleChartDelete to optionally refresh with an explicit FetchDataConfig. |
| superset-frontend/src/features/home/ChartTable.tsx | Adds delete modal flow and passes a concrete fetch config to ensure refresh works with initialData. |
| superset-frontend/src/features/home/ChartTable.test.tsx | Adds a regression test verifying delete triggers a refetch and removes the deleted chart from the UI. |
| superset-frontend/src/features/charts/ChartCard.tsx | Adds an onDelete hook to let parents (homepage) control deletion UX and refresh behavior. |
Comments suppressed due to low confidence (1)
superset-frontend/src/views/CRUD/utils.tsx:347
- In
handleChartDelete, the fallback refresh config for the "Mine" tab builds a filter oncreated_by(rel_o_m). For charts on the homepage, "Mine" filtering is done viaowners(rel_m_m) ingetFilterValues, so this branch can refresh the list with a different set of charts than the tab is showing. Consider aligning this filter with the homepage/tab logic (useowners+rel_m_m, or reuse the same filter builder) so a delete always refreshes to the correct dataset whenrefreshDataConfigisn’t provided.
const filters = {
pageIndex: 0,
pageSize: PAGE_SIZE,
sortBy: [
{
id: 'changed_on_delta_humanized',
desc: true,
},
],
filters: [
{
id: 'created_by',
operator: 'rel_o_m',
value: `${userId}`,
},
],
| label: onDelete ? ( | ||
| <div | ||
| data-test="chart-list-delete-option" | ||
| role="button" | ||
| tabIndex={0} | ||
| className="action-button" | ||
| onClick={() => onDelete(chart)} | ||
| > | ||
| <Icons.DeleteOutlined | ||
| iconSize="l" | ||
| css={css` | ||
| vertical-align: text-top; | ||
| `} | ||
| />{' '} | ||
| {t('Delete')} | ||
| </div> |
There was a problem hiding this comment.
The new onDelete menu label is a focusable <div role="button" tabIndex={0}> with only an onClick handler. This is not keyboard-activatable (Enter/Space) and may not fire when antd Menu items are activated via keyboard navigation. Prefer wiring deletion through the MenuItem’s onClick (or render a <Button>/add onKeyDown handling) so the action works for keyboard users as well.
Code Review Agent Run #f39f4cActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
According to #39428, this PR fixes a UI refresh issue when deleting charts from the homepage.
Unexpected Behavior:
When deleting a chart under tabs such as "All", the homepage does not update, and the deleted chart remains visible until a manual refresh.
Expected Behavior:
After deleting a chart, the homepage should immediately reflect the updated chart list across all tabs.
Root Cause:
On the homepage, chart data may be initialized using
initialData, which bypasses the fetch lifecycle inuseListViewResource. As a result,lastFetchDataConfigis not initialized. WhenrefreshData()is called without arguments, it becomes a no-op and does not trigger a data refresh.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After deletion, the home page immediately reflect the changes.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION