madhushreeag opened a new pull request, #38851:
URL: https://github.com/apache/superset/pull/38851
### SUMMARY
When navigating from a dashboard to the chart explore page, dashboard
filters (native filters, color scheme, data mask, etc.) are supposed to be
inherited by the chart. This works reliably in Chrome and Firefox, but
intermittently fails
in Safari.
The root cause is a race condition in src/pages/Chart/index.tsx.
getDashboardContextFormData was a module-level function that read URL params by
calling getUrlParam(...), which internally does:
new URLSearchParams(window.location.search).get(name)
In Safari, window.location.search is not guaranteed to be synchronously
updated after a history.push navigation. When the explore page's useEffect
fires, window.location.search can still reflect the old dashboard URL — so
dashboard_page_id is not found, the localStorage context is never looked
up, and no dashboard filters are applied to the chart.
Fix
Two changes:
1. src/utils/urlUtils.ts — Added an optional search?: string parameter to
getUrlParam (and all its overloads). When provided, it parses from that string
instead of window.location.search. Existing callers are unaffected.
2. src/pages/Chart/index.tsx — getDashboardContextFormData now accepts a
search: string parameter and passes it through to both getUrlParam calls. At
the call site inside useEffect, location.search from React Router's
useLocation() is
passed in. React Router's location is always in sync with the current
render cycle, so it correctly reflects the new explore URL even in Safari.
Test
A unit test was added to urlUtils.test.ts that directly simulates the race
condition: window.location.search is set to empty (stale), and the test asserts
that:
- getUrlParam without an override returns null (demonstrating the bug)
- getUrlParam with the correct search string passed in returns the
expected value (demonstrating the fix)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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: [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]
