sadpandajoe opened a new pull request, #35810: URL: https://github.com/apache/superset/pull/35810
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY <!--- Describe the change below, including rationale and design decisions --> The DatasourceEditor component and its DatasetUsageTab child component have several async race conditions that cause React warnings, memory leaks, and flaky tests: 1. setState on unmounted components - Four async methods (fetchUsageData, syncMetadata, onQueryFormat, formatSql) can complete after the component unmounts, triggering React warnings and potential state corruption 2. Memory leaks - DatasetUsageTab's pagination scroll timeout wasn't cleaned up on unmount 3. Flaky tests - Tests were missing API mocks for endpoints called on mount, causing 60+ second hangs before timeout failures **What this PR does:** 1. Adds mount tracking to DatasourceEditor.jsx - Introduces isComponentMounted flag set in componentDidMount/componentWillUnmount - Guards all setState and toast calls in 4 async methods with mount checks 2. Fixes DatasetUsageTab async safety (merged with upstream) - Adds isMountedRef to track component mount state - Guards all setState calls in handleFetchCharts with mount checks - Preserves upstream's performant requestAnimationFrame scroll implementation - Adds prevLoadingRef for scroll-on-load-complete timing 3. Adds missing API mocks to prevent test hangs - Mocks /api/v1/chart/ endpoint (called by fetchUsageData) - Mocks /api/v1/database/ endpoint (called by DatabaseSelector) - Both mocks include ids: [] to mirror production endpoint structure ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> 1. Verify tests pass: npm test -- --testPathPatterns=DatasourceEditor --no-coverage Expected: All 56 tests pass with no "Unmatched GET" warnings or async setState warnings. 2. Manual testing for setState warnings: - Open a dataset in edit mode (navigate to any dataset → Edit) - Trigger async operations: - Click "Sync columns from source" (tests syncMetadata) - Format SQL if in SQL mode (tests onQueryFormat/formatSql) - Switch to "Usage" tab to trigger chart fetch (tests fetchUsageData) - Quickly navigate away or close the modal before operations complete - Check browser console: should see no React warnings about setState on unmounted component 3. Manual testing for memory leaks: - Open dataset editor and navigate to Usage tab - Click through multiple pages rapidly - Close the modal - No timeouts should be left running (verify in browser dev tools) ### 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]
