test(ui): suppress landing welcome banner at source, remove dismiss helpers - #31853
test(ui): suppress landing welcome banner at source, remove dismiss helpers#31853karanh37 wants to merge 4 commits into
Conversation
The Playwright removeLandingBanner helper waited up to 5000ms for the welcome-screen close button. For storageState sessions the banner never renders (auth.setup records the user in the `loggedInUsers` localStorage key that gates it), so that call always ran out the full timeout — 5s of dead wait per invocation across many specs. Only specs that log in a freshly created user on a new context actually show the banner, and it paints shortly after currentUser resolves, so a bounded wait is still needed. Reduce the timeout from 5000ms to 2000ms, which comfortably covers the fresh-login case while cutting the wasted wait everywhere else. No test asserts on the banner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Follow-up to the timeout reduction: eliminate the welcome-banner dismiss dance entirely instead of waiting on it. The landing page renders the welcome banner only when the logged-in user's `name` is absent from the `loggedInUsers` localStorage key (MyDataPage.component.tsx). storageState sessions already have it seeded (auth.setup logs each user in, which records them), which is why the old removeLandingBanner waitFor always ran out its full timeout — it waited on an element that never appears. Only specs that log in a freshly created UserClass on a new context actually rendered the banner. Seed `loggedInUsers` in UserClass.login() via addInitScript, before the first navigation (mirroring disableEtagConditionalReads). The seeded name equals the app's currentUser.name — responseData.name for a created user, the email local-part for a pure login such as admin — so the banner never renders for any session. With the banner suppressed at the source, the reactive dismissals are dead code and are removed: - delete removeLandingBanner (common.ts) and all ~27 call sites - delete closeWelcomeScreenIfVisible and its inline blocks (searchRBAC.ts) - delete the inline dismiss block in entity.ts Tour.spec.ts keeps its own conditional guards: that suite drives the welcome/tour flow directly and the guards are harmless no-ops now. No test asserts on the welcome banner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Playwright Results — workflow succeededValidated commit ✅ 664 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 47m 37s ⏱️ Max setup 4m 40s · max shard execution 14m 28s · max shard-job elapsed before upload 18m 4s · reporting 5s 🌐 221.63 requests/attempt · 2.89 app boots/UI scenario · 25.35% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
The welcome-banner suppression seeds `loggedInUsers` in UserClass.login,
which hid the banner for every session. But the "Tour should work from
welcome screen" test enters the tour by clicking the banner's own CTA
("Take a product tour to get started!", rendered only in
WelcomeScreen.component.tsx), so suppressing the banner broke it.
Add a `suppressWelcomeScreen` login option (default true, so every other
spec keeps the suppression) and forward it through AdminClass. Tour.spec
logs in with `suppressWelcomeScreen: false` so the banner renders; its
other tests keep their existing "dismiss if visible" guards.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ ApprovedPlaywright test suite refactor that suppresses the landing welcome banner at the source via localStorage seeding and removes all obsolete dismissal helpers and call sites. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Description
The Playwright
removeLandingBannerhelper waited up to 5000ms for the welcome-screen close button on many specs. That wait was almost always wasted: forstorageStatesessions the welcome banner never renders —auth.setup.tslogs each user in, which lands on/my-dataand records the user in theloggedInUserslocalStorage key that gates the banner (MyDataPage.component.tsx), and that state is captured into every.auth/*.json. So thewaitForran out its full timeout waiting on an element that never appears.Only specs that log in a freshly created
UserClasson a new context (e.g.DomainDataProductsWidgets,CustomizeLandingPage,CustomizeWidgets,CuratedAssets) actually render the banner.Fix: suppress at the source
Seed
loggedInUsersinUserClass.login()viaaddInitScript, before the first navigation (mirroring the existingdisableEtagConditionalReadsopt-out). The seeded value equals the app'scurrentUser.name:responseData.namefor a created user (verified: both derive from the same server-side Username),admin).With the banner suppressed for every session, the reactive dismissals are dead code and are removed:
removeLandingBanner(common.ts) + all ~27 call sitescloseWelcomeScreenIfVisible+ its inline blocks (searchRBAC.ts)entity.tsTour.spec.tskeeps its own conditional guards — that suite drives the welcome/tour flow directly and the guards are harmless no-ops now. No test asserts on the welcome banner.Type of change
Tests
Playwright-only; no product code changed. Static checks on all changed files: 0 ESLint errors, 0 new
tscerrors (two pre-existingTS6133inentity.tsare unrelated, just line-shifted), prettier clean.Behavioral validation must come from the Playwright CI run. Watch the fresh-login specs specifically — they exercise the new source suppression:
DomainDataProductsWidgets,CustomizeLandingPage,CustomizeWidgets,CuratedAssets, plus the search-RBAC and activity-feed suites.🤖 Generated with Claude Code
Greptile Summary
The PR suppresses the Playwright landing-page welcome banner before navigation and removes redundant reactive dismissal waits.
loggedInUsersfromUserClass.login, with an explicit opt-out for tour coverage.removeLandingBannerand equivalent inline dismissal logic from affected specs and utilities.AdminClass.loginto forward welcome-screen suppression options.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Login["UserClass.login"] --> Option{"Suppress welcome screen?"} Option -->|Yes| Seed["Seed loggedInUsers via addInitScript"] Option -->|No| Preserve["Leave localStorage unchanged"] Seed --> SignIn["Navigate to /signin and authenticate"] Preserve --> SignIn SignIn --> Landing["Load /my-data"] Landing --> Gate{"Current user is in loggedInUsers?"} Gate -->|Yes| Hidden["Welcome banner remains hidden"] Gate -->|No| Visible["Welcome banner renders for tour coverage"]Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile