You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2026/04/21 05:12:43 UTC
[PR] fix(explore): skip re-fetch when navigating away from /explore [superset]
mistercrunch opened a new pull request, #39506:
URL: https://github.com/apache/superset/pull/39506
### SUMMARY
Fixes the "Save & go to dashboard" flow from the chart save modal, which was failing to land cleanly on the destination dashboard and often surfacing a spurious "Failed to load chart data" toast.
`superset-frontend/src/pages/Chart/index.tsx` subscribes to `history.listen` and re-runs `loadExploreData` on every `PUSH`/`POP`. Because it did not check the destination `pathname`, it also re-fetched when navigation left the `/explore` route entirely (e.g. `history.push(dashboard.url)` → `/superset/dashboard/:id/` from `SaveModal`'s "Save & go to dashboard" branch). That kicks off an `/api/v1/explore/` request using the destination dashboard's URL params while `ExplorePage` is about to unmount — it either races the unmount's `AbortController` or completes and dispatches `addDangerToast` on the destination page.
The regression landed with #39065 (Apr 14), which introduced the `AbortController` + `history.listen` pattern on the same day the bug was reported.
**Fix:** guard the listener to only re-fetch when the destination is one of the routes where `ChartPage` is actually mounted (`/explore/*` or `/superset/explore/p`). Post-save `REPLACE` with a `saveAction` in `location.state` continues to trigger a re-fetch without unmount, as before.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — pure behavioral fix; no UI changes.
### TESTING INSTRUCTIONS
1. Open a chart at `/explore/?slice_id=<id>`, make an edit.
2. In the Save modal, pick an existing dashboard (or leave the chart already on one) and choose **"Save & go to dashboard"**.
3. Expect: lands on `/superset/dashboard/:id/` with the updated chart rendered, no error toast.
4. Regression unit test: `superset-frontend/src/pages/Chart/Chart.test.tsx` — `does not re-fetch explore data when navigating off the /explore route (...)`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-104553 (internal)
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] 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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-5377681483
<!-- Bito Reply -->
The current implementation uses `loc.pathname.startsWith('/explore')` and `loc.pathname.startsWith('/superset/explore')` to determine if the navigation stays within the Explore route. If you want to align this more strictly with the router configuration, you could define a constant or a helper function that encapsulates the exact paths where the Chart page is mounted (e.g., `['/explore/', '/explore/p']`). Using a regex or a path-matching utility would also be a more robust alternative to `startsWith` if the route structure becomes more complex.
**superset-frontend/src/pages/Chart/index.tsx**
```
const EXPLORE_ROUTES = ['/explore/', '/explore/p'];
const isExploreRoute = EXPLORE_ROUTES.some(route => loc.pathname.startsWith(route));
```
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-4286202560
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39506?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 64.44%. Comparing base ([`151d7d7`](https://app.codecov.io/gh/apache/superset/commit/151d7d76da2855a7c6c6c83e670e0f367f62a55e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1b570c1`](https://app.codecov.io/gh/apache/superset/commit/1b570c1d9a0e40307ef7c04d135ef0bb8bedcc6d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39506 +/- ##
=======================================
Coverage 64.44% 64.44%
=======================================
Files 2560 2560
Lines 133574 133578 +4
Branches 31017 31020 +3
=======================================
+ Hits 86082 86086 +4
Misses 45999 45999
Partials 1493 1493
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39506/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/39506/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.44% <100.00%> (+<0.01%)` | :arrow_up: |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/39506?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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-5416011280
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-5414449303
Rebased onto current master and addressed the feedback/CI failure: navigation is gated through an helper using the canonical route, so no legacy literal is introduced. The regression test covers dashboard navigation, and in-route navigation tests use .
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #39506:
URL: https://github.com/apache/superset/pull/39506
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #39506:
URL: https://github.com/apache/superset/pull/39506#discussion_r3834990047
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -312,8 +312,18 @@ export default function ExplorePage() {
// Other REPLACE: ignored (URL sync from updateHistory).
// Entries holding a chart state of the loaded chart are skipped: Explore
// pushed them itself, and ExploreViewContainer restores a popped one in place.
+ // Navigations that leave the /explore route (e.g. "Save & go to dashboard"
+ // pushes /superset/dashboard/:id/) must not trigger a re-fetch here: the
+ // Explore page is about to unmount, and fetching /api/v1/explore/ with the
+ // destination's params starts a request that either races the unmount abort
+ // or surfaces a spurious error toast on the destination page.
useEffect(() => {
const unlisten = history.listen((loc: Location, action: Action) => {
+ // ChartPage is mounted at /explore/ and /superset/explore/p in
+ // routes.tsx; only re-fetch for navigations that stay inside those.
+ const isExploreRoute =
+ loc.pathname.startsWith('/explore') ||
+ loc.pathname.startsWith('/superset/explore');
Review Comment:
This still treats `/explore-anything` and stale `/superset/explore...` paths as Explore even though the router only mounts Chart at `/explore/` and `/explore/p`. Navigating to one of those non-Chart paths will reintroduce the unwanted request during unmount; could this use the route constants or matcher instead?
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-4286072530
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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(explore): skip re-fetch when navigating away from /explore [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39506:
URL: https://github.com/apache/superset/pull/39506#issuecomment-5041367642
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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