codeant-ai-for-open-source[bot] commented on code in PR #37529:
URL: https://github.com/apache/superset/pull/37529#discussion_r2738213292
##########
superset-frontend/playwright/pages/DashboardPage.ts:
##########
@@ -63,6 +64,34 @@ export class DashboardPage {
});
}
+ /**
+ * Wait for all charts on the dashboard to finish loading.
+ * Waits until no loading indicators are visible.
+ */
+ async waitForChartsToLoad(options?: { timeout?: number }): Promise<void> {
+ const timeout = options?.timeout ?? TIMEOUT.PAGE_LOAD;
+ const loadingIndicators = this.page.locator('[aria-label="Loading"]');
+
+ // Wait until all loading indicators are gone (count reaches 0)
+ await loadingIndicators
+ .first()
+ .waitFor({ state: 'hidden', timeout })
+ .catch(() => {
+ // No loading indicators found - charts already loaded
+ });
+
+ // Double-check no loading indicators remain
+ await this.page
+ .waitForFunction(
+ selector => document.querySelectorAll(selector).length === 0,
+ '[aria-label="Loading"]',
+ { timeout },
+ )
+ .catch(() => {
+ // Timeout is acceptable - proceed if indicators are gone or minimal
+ });
Review Comment:
**Suggestion:** In `waitForChartsToLoad`, calling
`loadingIndicators.first().waitFor({ state: 'hidden', timeout })` and catching
all errors means that when there are no `[aria-label="Loading"]` elements (or
they never appear), Playwright will still wait the full timeout before throwing
and being caught, turning a no-op into a long delay and potentially causing
unnecessary timeouts in tests that don't show loading spinners; additionally,
swallowing timeouts from the second `waitForFunction` hides situations where
charts never finish loading, so callers proceed while the UI is still loading.
A more robust approach is to first check if any loading indicators exist and,
if so, wait once for the count to reach zero, letting that timeout propagate
instead of being silently ignored. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Playwright dashboard export tests experience long unnecessary delays.
- ⚠️ Test flakiness hidden when charts never finish loading.
- ⚠️ CI job timeouts and longer test runs.
- ⚠️ Affects all tests using waitForChartsToLoad helper.
```
</details>
```suggestion
// If there are no loading indicators, return immediately.
if ((await loadingIndicators.count()) === 0) {
return;
}
// Wait until all loading indicators are gone (count reaches 0)
await this.page.waitForFunction(
selector => document.querySelectorAll(selector).length === 0,
'[aria-label="Loading"]',
{ timeout },
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Playwright test that navigates to a dashboard (tests call
DashboardPage.gotoBySlug / gotoById and then
DashboardPage.waitForChartsToLoad). The
DashboardPage implementation is at
superset-frontend/playwright/pages/DashboardPage.ts
lines 45-126; the waitForChartsToLoad method starts at line 71 (see file
read).
2. For dashboards that render immediately without any spinner elements, the
page contains
zero elements matching '[aria-label="Loading"]'. In this state, the code
executes
loadingIndicators.first().waitFor({ state: 'hidden', timeout }) at lines
75-79. Because
.first().waitFor waits for the locator to become hidden, Playwright will
wait up to the
provided timeout before resolving or rejecting.
3. The code catches the rejection (.catch(() => { /* No loading indicators
found - charts
already loaded */ })) at lines 79-81, swallowing the timeout. This means the
test run will
incur the full timeout delay (TIMEOUT.PAGE_LOAD) before proceeding even
though no spinner
exists.
4. After that, the second check uses page.waitForFunction(...) at lines
84-90 which is
also .catch()-ed. If charts actually never finish loading (spinner stuck or
long-running
loads), the timeouts are swallowed and the test proceeds silently without
failing,
potentially causing later flakiness or assertions to fail unexpectedly.
5. The proposed improved_code (check count() then waitForFunction without
swallowing)
avoids the initial unnecessary wait when count() === 0 and lets the
waitForFunction
timeout propagate if loading indicators are present but never clear. This
reproduces
locally by running the Dashboard Export Playwright tests (per PR testing
instructions)
against a dashboard with no spinners and observing the extra
TIMEOUT.PAGE_LOAD delay
before tests continue.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/pages/DashboardPage.ts
**Line:** 79:92
**Comment:**
*Performance: In `waitForChartsToLoad`, calling
`loadingIndicators.first().waitFor({ state: 'hidden', timeout })` and catching
all errors means that when there are no `[aria-label="Loading"]` elements (or
they never appear), Playwright will still wait the full timeout before throwing
and being caught, turning a no-op into a long delay and potentially causing
unnecessary timeouts in tests that don't show loading spinners; additionally,
swallowing timeouts from the second `waitForFunction` hides situations where
charts never finish loading, so callers proceed while the UI is still loading.
A more robust approach is to first check if any loading indicators exist and,
if so, wait once for the count to reach zero, letting that timeout propagate
instead of being silently ignored.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</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: [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]
