🐛 fix: consolidate Settings title testid to fix Playwright strict mode violations#19229
Conversation
…e violations Both desktop and mobile Settings page titles now use the same data-testid to prevent strict mode violations when tests use CSS selectors that match multiple elements. Visibility is still managed via CSS classes (hidden lg:block for desktop sidebar, lg:hidden for mobile section). Fixes #19208 - Playwright Cross-Browser (Nightly) test failures in Firefox, WebKit, and Mobile browsers due to h1:has-text('Settings') selector matching multiple elements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>"
Now that both desktop and mobile Settings titles share the same testid,
update the test to use .filter({visible: true}) to select the appropriate
element based on viewport.
Fixes #19208
…e violations
Both desktop and mobile Settings page titles now use the same data-testid
('settings-title') to prevent strict mode violations when tests use CSS
selectors that match multiple elements. Visibility is still properly
managed via CSS classes (hidden lg:block for desktop sidebar, lg:hidden
for mobile section).
Fixes #19208 - Playwright Cross-Browser (Nightly) test failures in Firefox,
WebKit, and Mobile browsers.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
✅ Test Coverage CheckAll new source files in this PR have corresponding test files. Checked |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix Playwright strict-mode selector ambiguity on the Settings page title by standardizing how the title is targeted in the UI and referenced in E2E tests (important for cross-browser + mobile projects).
Changes:
- Updated
Settings.tsxso both desktop and mobile<h1>titles usedata-testid="settings-title". - Simplified E2E selectors by removing fallback references to
settings-title-mobilein two Playwright specs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
web/src/components/settings/Settings.tsx |
Changes Settings title test IDs; also includes additional prop/interface changes to settings sections that currently don’t match the exported component APIs. |
web/e2e/user-flows/settings-configuration.spec.ts |
Removes fallback locator for settings-title-mobile and relies on the shared test id. |
web/e2e/deep-links-and-data-flow.spec.ts |
Removes fallback locator for settings-title-mobile and updates the title-locator comment. |
| <ProfileSection | ||
| initialEmail={user?.email || ''} | ||
| initialSlackId={user?.slack_id || ''} | ||
| githubLogin={user?.github_login} | ||
| user={user} | ||
| refreshUser={refreshUser} | ||
| isLoading={isUserLoading} | ||
| /> |
| <SettingsBackupSection | ||
| syncStatus={syncStatus} | ||
| lastSaved={lastSaved} | ||
| filePath={filePath} | ||
| onExport={exportSettings} | ||
| onImport={importSettings} | ||
| exportSettings={exportSettings} | ||
| importSettings={importSettings} | ||
| /> |
| @@ -505,8 +506,9 @@ | |||
| <div className="lg:hidden mb-6"> | |||
| <div className="flex items-center justify-between"> | |||
| <h1 | |||
| data-testid="settings-title-mobile" | |||
| data-testid="settings-title" | |||
| className="text-2xl font-bold text-foreground" | |||
| data-qa="settings-header-mobile" | |||
| > | |||
| {t('settings.title')} | |||
| </h1> | |||
| const settingsTitle = page.getByTestId('settings-title') | ||
| .or(page.getByTestId('settings-title-mobile')) | ||
| .or(page.getByRole('heading', { name: /settings/i })) | ||
|
|
||
| await expect(settingsTitle.first()).toBeVisible({ timeout: ELEMENT_VISIBLE_TIMEOUT_MS }) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: GitHub Copilot <copilot@kubestellar.io>
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
❌ Post-Merge Verification: failedCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Fixes #19208
Problem
Playwright Cross-Browser (Nightly) tests are failing across Firefox, WebKit, and Mobile browsers (Chromium/Safari) due to Playwright strict mode violations when selecting the Settings page title.
Root Cause: The Settings component has two h1 elements with identical text ("Settings"):
data-testid="settings-title"for desktop (hidden withhidden lg:block)data-testid="settings-title-mobile"for mobile (shown withlg:hidden)When tests use CSS selectors like
h1:has-text("Settings"), Playwright strict mode rejects the ambiguous selector because it matches both elements in the DOM, even though only one is visible.Solution
Consolidate both title elements to use the same
data-testid="settings-title", removing the need for duplicate elements:Settings.tsx: Both desktop and mobile titles now use
data-testid="settings-title"deep-links-and-data-flow.spec.ts: Removed fallback to
settings-title-mobilesettings-configuration.spec.ts: Removed fallback to
settings-title-mobileTest Coverage
getByTestId('settings-title')✓Verification
settings-title-mobileremain in codebase