Copilot commented on code in PR #35810:
URL: https://github.com/apache/superset/pull/35810#discussion_r2492173739
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorRTL.test.tsx:
##########
@@ -28,6 +28,21 @@ import {
describe('DatasourceEditor RTL Metrics Tests', () => {
beforeEach(() => {
fetchMock.get(DATASOURCE_ENDPOINT, [], { overwriteRoutes: true });
+ fetchMock.get(
+ url => url.includes('/api/v1/chart/'),
+ { result: [], count: 0, ids: [] },
+ { overwriteRoutes: true },
+ );
+ fetchMock.get(
+ url => url.includes('/api/v1/database/'),
+ { result: [], count: 0, ids: [] },
+ { overwriteRoutes: true },
+ );
+ fetchMock.get(
+ url => url.includes('/api/v1/dataset/related/owners'),
+ { result: [], count: 0 },
+ { overwriteRoutes: true },
+ );
Review Comment:
The same mock setup is duplicated across multiple test files and within test
suites. Consider extracting this into a shared test utility function (e.g.,
`setupDatasourceEditorMocks()`) that can be imported and reused to reduce
duplication and improve maintainability.
```suggestion
// Utility function to set up fetchMock for DatasourceEditor tests
function setupDatasourceEditorMocks() {
fetchMock.get(DATASOURCE_ENDPOINT, [], { overwriteRoutes: true });
fetchMock.get(
url => url.includes('/api/v1/chart/'),
{ result: [], count: 0, ids: [] },
{ overwriteRoutes: true },
);
fetchMock.get(
url => url.includes('/api/v1/database/'),
{ result: [], count: 0, ids: [] },
{ overwriteRoutes: true },
);
fetchMock.get(
url => url.includes('/api/v1/dataset/related/owners'),
{ result: [], count: 0 },
{ overwriteRoutes: true },
);
}
describe('DatasourceEditor RTL Metrics Tests', () => {
beforeEach(() => {
setupDatasourceEditorMocks();
```
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx:
##########
@@ -112,23 +113,38 @@ const DatasetUsageTab = ({
async (page = 1, column = sortColumn, direction = sortDirection) => {
if (!datasourceId) return;
- setLoading(true);
+ if (isMountedRef.current) {
+ setLoading(true);
+ }
Review Comment:
The loading state should be set before checking mount status to ensure the
loading indicator appears immediately when the function is called. The mount
check should only guard state updates that occur after async operations
complete. Move `setLoading(true)` before the mount check.
```suggestion
setLoading(true);
```
--
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]
