test(playwright): select existing dashboards without creating duplicates

[aminghadersohi]

SUMMARY

The cross-referenced dashboards test types into the Save modal before existing dashboards load. That can select the create-new option and leave behind an empty duplicate that cleanup does not track. The mobile chart-loading test then fails when that duplicate is the first dashboard card.

Type a strict title prefix in the shared SaveChartModal.selectExistingDashboard helper and select only an exact existing-title match before saving. Assert that the PUT includes the intended dashboard id (the exactly-one-dashboard count checks came from #44829). Use the World Bank's Data sample dashboard for mobile interaction tests instead of relying on list order, retry transient navigation failures, and require HTTP 200 with a missing-fixture message.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; test-only changes.

TESTING INSTRUCTIONS

With the example dashboards loaded and MOBILE_CONSUMPTION_MODE enabled, run:

cd superset-frontend
npx playwright test playwright/tests/explore/cross-referenced-dashboards.spec.ts playwright/tests/explore/explore-save.spec.ts
INCLUDE_MOBILE=true npx playwright test mobile/

Confirm the chart is added to the existing dashboards without creating duplicates, and mobile charts load even when an empty dashboard sorts first. Repeat with the dashboard-list response delayed.

Local validation, using CI's server setup (gunicorn, tests.integration_tests.superset_test_config, Postgres, load_examples) and CI's Playwright invocation (CI=true npx playwright test <path> --output=playwright-results):

• explore/cross-referenced-dashboards.spec.ts and explore/explore-save.spec.ts with --retries=0 --repeat-each=4: 12 passed.
• mobile/ with MOBILE_CONSUMPTION_MODE and INCLUDE_MOBILE=true: 14 passed, 2 skipped (the fixture has no native filters).

ADDITIONAL INFORMATION

• Has associated issue: follow-up for the product-side AsyncSelect duplicate option: Save chart modal can create a duplicate dashboard when the typed title matches an existing one #44873
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

[bito-code-review]

Code Review Agent Run #bf9709

Actionable Suggestions - 1

• superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts - 1

  • Missing existence guard · Line 40-40

Additional Suggestions - 2

• superset-frontend/playwright/tests/explore/cross-referenced-dashboards.spec.ts - 1

  • Duplicated select option logic · Line 66-70
    Lines 66-70 re-implement `Select.clickOption` inline, but more fragilely: no `.last()` on the dropdown (strict-mode violation if multiple dropdowns are visible) and no partial-match fallback. Since `Select` is already imported and `open()`/`type()` are used, call `dashboardSelect.clickOption(dashboardName)` instead of duplicating the option targeting.
• superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts - 1

  • Magic string duplicated · Line 40-40
    `'dashboard/world_health/'` is a hardcoded magic string duplicating the identical literal in `getMobileFilterButton` (line 54). The file otherwise routes through the `URL` constants (e.g. `URL.DASHBOARD_LIST`). If the sample dashboard slug changes, both sites must be updated independently. Extract a shared named constant.

Review Details

• Files reviewed - 2 · Commit Range: 19e67f2..19e67f2

  • superset-frontend/playwright/tests/explore/cross-referenced-dashboards.spec.ts
  • superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts
• Files skipped - 0
• Tools

  • Eslint (Linter) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.71%. Comparing base (8ab7a85) to head (a0fb1a4).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #44856   +/-   ##
=======================================
  Coverage   81.71%   81.71%           
=======================================
  Files        2978     2978           
  Lines      181612   181610    -2     
  Branches    41982    41982           
=======================================
+ Hits       148397   148399    +2     
+ Misses      30493    30489    -4     
  Partials     2722     2722           

Flag	Coverage Δ
javascript	77.13% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #b8b2a3

Actionable Suggestions - 0

Additional Suggestions - 1

• superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts - 1

  • Skip guard misses non-404 failures · Line 41-44
    `test.skip(response?.status() === 404, ...)` only skips on 404, but `superset/views/core.py` `dashboard()` also `abort(404)`s on `SupersetSecurityException` and redirects unauthenticated users to login (302). On those paths the test proceeds on an error page instead of skipping. Checking `response.ok()` (any non-2xx) covers 404, 403 and redirects. ([CWE-425](https://cwe.mitre.org/data/definitions/425.html))

Review Details

• Files reviewed - 1 · Commit Range: 19e67f2..cf55144

  • superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts
• Files skipped - 0
• Tools

  • Eslint (Linter) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Replaced the world_health 404 skip with an OK-response assertion because CI runs full load_examples, so the dashboard is required.
A missing route should fail the mobile test rather than hide a routing regression.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	a0fb1a4
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abecc51af29490008332f22
😎 Deploy Preview	https://deploy-preview-44856--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[aminghadersohi]

AsyncSelect filters locally when its initial page contains all dashboards; the spec now waits for the title-filtered Rison search GET only when pagination requires it, armed before typing, while retaining the exact-title click.

[bito-code-review]

Code Review Agent Run #127f1a

Actionable Suggestions - 0

Additional Suggestions - 1

• superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts - 1

  • skip-to-fail regression · Line 41-41
    This converts a graceful `test.skip` on 404 into a hard failure. `getMobileFilterButton` -> `openExampleDashboard` is used by the 'Mobile Filter Drawer' tests (lines 249-268), which have no skip guard for a missing dashboard, unlike the sibling 'Mobile Dashboard Interaction' block's `beforeAll` (line 153) that skips when no dashboards exist. In environments without the sample fixture, these tests now fail instead of skip, contradicting the repo's skip-when-fixture-absent pattern.

Review Details

• Files reviewed - 2 · Commit Range: cf55144..1ec6f38

  • superset-frontend/playwright/tests/explore/cross-referenced-dashboards.spec.ts
  • superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts
• Files skipped - 0
• Tools

  • Eslint (Linter) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[rebenitez1802]

Request changes: this fixes the real flake. The recurring master failures in the Mobile step come from the dashboard leaked by this test, and both Playwright legs pass at 1ec6f38. But the new pagination branch can't work, nothing would catch a recurrence, and it overlaps approved #44829.

🟡 Medium — The count > result.length branch recreates the bug it guards against
In overwriteToDashboard (cross-referenced-dashboards.spec.ts:69-83), suppose the target dashboard isn't on the first page. 250ms after fill(), handleOnSearch adds an allowNewOptions "create new" entry labelled with the typed title. The page-0 search merge keeps that entry because it only removes duplicates by value (AsyncSelect.tsx:477-484), and a string title never equals a numeric id. Both rows show the same text, so lines 84-89 either click "create new" (if the search result hasn't rendered yet) or fail strict mode. CI never takes this branch: there is one value:'' list GET per modal open and no search GET. Fix: drop the count/rison/waitForResponse logic and type a strict prefix, await dashboardSelect.type(dashboardName.slice(0, -1)), then click getByText(dashboardName, { exact: true }) in the visible dropdown. The "create new" entry is then labelled with the prefix and can never be an exact match. Playwright's auto-wait covers both local filtering and server search. Alternatively, keep the GET wait and assert result.length === count as an explicit precondition.

🟡 Medium — Nothing detects a duplicate dashboard, and the mobile change removes the only signal
The post-click check at :90-92 also passes for the "create new" entry, since it has the same label. The "Added to N dashboards" and submenu checks also match by name, and apiPutChart at :172 quietly detaches the duplicate. In the failing master runs, this test reported a pass while POSTing the leaked dashboard; only the mobile "first card" test caught it, and that test now opens world_health. Fix: after the PUT, add expect((await getDashboardsByName(page, dashboardName)).count).toBe(1);, as explore-save.spec.ts:122 and #44829 already do. For a definitive check, pass the dashboard id in and assert (await updated).request().postDataJSON().dashboards contains it.

🟢 Low — Overlaps approved #44829 and #44581
#44829 (approved, mergeable) rewrites this same line to a new SaveChartModal.selectExistingDashboard() and adds the count assertion, so whichever lands second gets a conflict in overwriteToDashboard. #44581, also yours, adds a third wait inside SaveChartModal.selectDashboard. It's worth settling on one shared helper. #44829's getVisibleOptionTexts() wait appears to return early on the second overwrite, because the preselected last-used dashboard is already visible. That makes this PR's wait on the network response (or prefix typing) the stronger basis. Putting it in a SaveChartModal helper would also cover explore-save.spec.ts:114, which still uses the unguarded selectDashboard path. Keep the exact-only click there rather than Select.clickOption, which falls back to a partial match.

🟢 Low — Stale beforeAll in "Mobile Dashboard Interaction"
mobile-dashboard.spec.ts:153-174 still loads the dashboard list and skips the whole suite when there are no cards, but none of its three tests use the list anymore. The result is inconsistent: an empty list skips this suite, while a missing world_health fails the Filter Drawer suite. Fix: delete it, since openExampleDashboard already enforces the real precondition.

🟢 Low — openExampleDashboard uses raw goto, a hardcoded slug, and an assertion with no message
page.goto doesn't retry transient connection resets the way gotoWithRetry does (helpers/navigation.ts:37, which returns the Response, so the check can stay). ok() would also pass on a 200 login-page redirect, and a failure only reports "expected true". The world_health slug is also hardcoded separately in the export and embedded specs. Fix: const response = await gotoWithRetry(page, 'dashboard/world_health/'); expect(response?.status(), 'world_health missing; run superset load_examples').toBe(200);

🟢 Low — Comments and PR body have drifted
The PR body still says the test will "verify it contains the intended existing dashboard", but that assertion was removed in 1ec6f38. The spec comments at :32, :46 and :51 call the fixture "World Bank's Health"; its real title is "World Bank's Data". That wording predates this PR, but the PR touched those blocks.

🟢 Low — Product root cause is worth an issue
AsyncSelect never removes a "create new" (isNewOption) entry when a fetched option has the same label (mergeData and the page-0 search merge at AsyncSelect.tsx:477-484). A real user who types an existing title quickly, or has more than 100 dashboards, can see two identical rows, one of which silently creates an empty dashboard. That's out of scope here, but a follow-up issue would help.

[aminghadersohi]

@rebenitez1802 addressed:

1. Removed the pagination branch → 47bdb0a; strict-prefix, exact-only selection in the shared SaveChartModal helper, used by both Explore specs → a0fb1a4.
2. Post-PUT duplicate-count guard → 47bdb0a; target dashboard-id assertions on PUT bodies → a0fb1a4.
3. Deleted the stale mobile interaction beforeAll → a0fb1a4.
4. gotoWithRetry + HTTP 200 + missing-fixture message → a0fb1a4.
5. Corrected “World Bank's Data” comments → a0fb1a4; corrected the PR summary, preserving the rest of the body.
6. Consolidated on master's existing shared helper → 47bdb0a + a0fb1a4; no other PR changed.
7. AsyncSelect remains untouched and out of scope → a0fb1a4.
   Master merged and conflict cleared → 47bdb0a.
   Validation: ESLint, formatting, oxlint, custom rules, stylelint and whitespace passed; isolated helper/mobile typecheck passed; all 11 tests collected. Full typecheck/pre-commit type hook blocked by missing frontend dependencies/unbuilt packages (no playwright tsconfig). E2E not run: no local Superset. No environment started.

[rebenitez1802]

Approve: both round-1 blockers are fixed. Prefix typing plus the exact-only click makes the "create new" pick impossible by construction, the PUT-body check in cross-referenced-dashboards catches a wrong pick, and CI is green on both legs. Everything below is an optional nit; the first four have one-click suggestions inline.

🟢 Low — The explore-save PUT check can never fail
explore-save.spec.ts:121-123 asserts dashboards contains dashboardId, but the save-as already put the chart on dashboardId, and SaveModal only appends to the chart's existing dashboards (SaveModal.tsx:508-532). A wrong "create new" pick would send [dashboardId, newId] and still pass. The count check at :126 can't see it either, because that duplicate would be titled with the typed prefix. Fix: toEqual([dashboardId]) (inline suggestion).

🟢 Low — The cross-referenced-dashboards comment now describes the wrong check
cross-referenced-dashboards.spec.ts:59-60 says the count check catches a "create new" pick. With prefix typing, that duplicate would carry the prefix title, so count stays 1; the new toContain(dashboardId) is what catches it. Fix: reword it (inline suggestion).

🟢 Low — status() === 200 still passes on a login redirect
That gap was in my round-1 suggestion, sorry. @has_access redirects a lost session to /login/?next=…/dashboard/world_health/, which returns 200, so this check doesn't catch it. A full-URL regex doesn't catch it either, because the target appears unencoded in next. Fix: also check the final path (inline suggestion). The suggestion uses page.url().split('?')[0] rather than new URL(...) because this file imports a URL constant that shadows the global.

🟢 Low — A stale comment remains in getMobileFilterButton
mobile-dashboard.spec.ts:55-58 says filters "depend on the fixture" and "callers skip themselves". In fact world_health ships with native_filter_configuration: [], and the helper itself skips. Fix: inline suggestion.

🟢 Low — The exact-only click copies Select's private selectors
SaveChartModal.ts:107-112 hardcodes the dropdown/option selectors because Select.clickOption falls back to a partial match. Fix: add an exact mode to Select (clickOption(text, { exact: true }), which auto-waits with no fallback) and call it from the helper.

🟢 Low — The refresh test's skip guards can hide regressions
This test now runs on world_health (it was skipped on master), but mobile-dashboard.spec.ts:198-211 still test.skips when the menu or the "Refresh dashboard" item is missing. The second guard runs count() right after the click with no auto-wait. This predates the PR, but the PR changed which dashboard the test opens. Fix: replace both with await expect(...).toBeVisible() before clicking.

🟢 Low — The PR body still lags the code
The body presents the count === 1 checks as this PR's change, but they came from #44829. explore-save.spec.ts is modified but missing from TESTING INSTRUCTIONS. The validation paragraph says tsc was blocked, while your reply says the isolated typecheck passed.

🟢 Low — #44581 still carries the old Save-modal wait (outside this PR)
#44581 still changes SaveChartModal.selectDashboard and Select.selectOption to wait for the list GET. Its comment says this prevents duplicating an existing title, but that method now only handles brand-new titles. Worth dropping those two Playwright files from the MCP PR.

[aminghadersohi]

Follow-ups on a0fb1a4:

• Root cause: filed Save chart modal can create a duplicate dashboard when the typed title matches an existing one #44873 for the AsyncSelect duplicate "create new" row.
• Playwright run locally with CI's server setup and command: explore xref + explore-save at --retries=0 --repeat-each=4, 12/12 passed; mobile/, 14 passed and 2 skipped (no native filters in the fixture).

[rusackas]

LGTM. The rewritten selectExistingDashboard (strict prefix + exact-match click) is a real fix for the race, not just a different way of hiding it, and the new assertions (PUT payload contains the dashboard id, exactly one dashboard with that title afterward) actually would have caught the original leak. Nice catch replacing the mobile suite's list-based skip with a hard status check too.

Heads up for later: #44829 touches this same overwriteToDashboard line, so whichever lands second will need a small conflict resolution.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

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.
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