-
Notifications
You must be signed in to change notification settings - Fork 13k
test: remove data-qa-setting-id locators
#38002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR removes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38002 +/- ##
===========================================
- Coverage 70.64% 70.61% -0.04%
===========================================
Files 3143 3143
Lines 108684 108661 -23
Branches 19556 19547 -9
===========================================
- Hits 76781 76727 -54
- Misses 29902 29932 +30
- Partials 2001 2002 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fff5d26 to
90c0ced
Compare
edd8ebc to
9dbdbeb
Compare
… and `pressSequentially`
9dbdbeb to
d1b84b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts">
<violation number="1" location="apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts:34">
P1: Race condition: `waitForRequest` is set up after `click()`, so the request may have already been sent before the listener is ready. Set up the promise before triggering the action to avoid flaky tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (4)
apps/meteor/client/views/admin/settings/Setting/inputs/TimespanSettingInput.tsx (1)
91-103: Loss of stable test selectors affects test reliability.Removing
data-qa-reset-setting-idanddata-qa-setting-idattributes forces tests to use less stable selectors (role/label/text-based), which are vulnerable to localization and text changes. This pattern is repeated across multiple setting input components in this PR.Based on learnings, data-qa attributes provide more stable test selectors than localized text.
apps/meteor/client/views/admin/settings/Setting/inputs/ColorSettingInput.tsx (1)
56-95: Data-qa attributes removed from multiple inputs.Removing data-qa attributes from ResetSettingButton, InputBox, TextInput, and Select eliminates stable test hooks across the color setting input component. Tests must now rely on role/label selectors which are less stable.
Based on learnings, data-qa attributes provide better test stability than localized text-based selectors.
apps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsx (1)
35-47: Data-qa attribute removal impacts test selector stability.Removing
data-qa-reset-setting-idanddata-qa-setting-idattributes eliminates stable test hooks. This pattern is consistent across all setting input components in this PR but conflicts with established guidance to prefer data-qa attributes over localized text-based selectors.Based on learnings, data-qa attributes provide better test stability than text/label-based selectors when UI language changes.
apps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsx (1)
33-34: Data-qa attribute removal reduces test stability.Removing
data-qa-reset-setting-idanddata-qa-setting-idattributes forces tests to rely on less stable selectors. Based on learnings, stable data-qa attributes are preferred for test reliability across language changes.
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsx (1)
55-55: Remove implementation comment per coding guidelines.The
aria-labeladdition improves accessibility, but the FIXME comment violates coding guidelines: "Avoid code comments in the implementation."Consider tracking this technical debt in an issue tracker instead.
🔎 Proposed fix
- aria-label={_id} // FIXME: Multiselect (fuselage) should be associating the FieldLabel automatically. This is a workaround for accessibility and test locators. + aria-label={_id}Would you like me to create an issue to track the Fuselage MultiSelect label association improvement?
apps/meteor/tests/e2e/administration-settings.spec.ts (1)
58-58: Verify necessity ofpressSequentiallyoverfill.The test now uses
pressSequentially('any_text')instead offill('any_text'). WhilepressSequentiallyprovides more realistic character-by-character input simulation, it's slower and typically unnecessary for standard input fields unless the component has specific event listeners that require incremental input.Confirm whether this change is necessary for test reliability or if it introduces unnecessary overhead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/__snapshots__/SettingToggle.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/settings/Setting/inputs/__snapshots__/RangeSettingInput.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/ColorSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/LookupSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/PasswordSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RelativeUrlSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/SelectSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/SelectTimezoneSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/TimespanSettingInput.tsxapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/admin-settings.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/settings/Setting/inputs/RelativeUrlSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsxapps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/client/views/admin/settings/Setting/inputs/SelectSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/PasswordSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/ColorSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/SelectTimezoneSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsxapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/TimespanSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsxapps/meteor/tests/e2e/page-objects/admin-settings.tsapps/meteor/client/views/admin/settings/Setting/inputs/LookupSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsx
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize existing page objects pattern from
apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/IntSettingInput.tsxapps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsxapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/page-objects/omnichannel-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.tsapps/meteor/tests/e2e/administration-settings.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.ts
📚 Learning: 2025-12-16T17:29:40.430Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:40.430Z
Learning: In all page-object files under apps/meteor/tests/e2e/page-objects/, import expect from ../../utils/test (Playwright's async expect) instead of from Jest. Jest's expect is synchronous and incompatible with web-first assertions like toBeVisible, which can cause TypeScript errors.
Applied to files:
apps/meteor/tests/e2e/page-objects/omnichannel-settings.tsapps/meteor/tests/e2e/page-objects/admin-settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/e2e/page-objects/admin-settings.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (15)
apps/meteor/client/views/admin/settings/Setting/inputs/RelativeUrlSettingInput.tsx (1)
36-36: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/StringSettingInput.tsx (1)
40-40: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/PasswordSettingInput.tsx (1)
33-33: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/GenericSettingInput.tsx (1)
35-35: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/SelectTimezoneSettingInput.tsx (1)
34-34: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/MultiSelectSettingInput.tsx (1)
42-42: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/RangeSettingInput.tsx (1)
33-33: Attribute removal aligns with PR objective.The removal of
data-qa-reset-setting-idis consistent with the PR's goal. See concerns raised in RoomPickSettingInput.tsx regarding test stability.apps/meteor/client/views/admin/settings/Setting/inputs/RoomPickSettingInput.tsx (1)
39-39: Update admin-settings.ts page object to use semantic locator for reset button.The
btnResetSiteURLselector inadmin-settings.tsline 19 currently uses an XPath expression that depends on DOM structure. Replace the XPath selector with a semantic locator usinggetByRole()to ensure test stability across UI changes and language variations. For example:this.page.getByRole('button', { name: /reset/i }).first()or a similar approach based on accessible attributes.⛔ Skipped due to learnings
Learnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`Learnt from: dougfabris Repo: RocketChat/Rocket.Chat PR: 36987 File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20 Timestamp: 2025-09-23T19:22:59.217Z Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.Learnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuseLearnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright testsLearnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`Learnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright testsLearnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test filesLearnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright testsLearnt from: CR Repo: RocketChat/Rocket.Chat PR: 0 File: .cursor/rules/playwright.mdc:0-0 Timestamp: 2025-11-24T17:08:17.065Z Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright testsLearnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37834 File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22 Timestamp: 2025-12-16T17:29:45.163Z Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.apps/meteor/client/views/admin/settings/Setting/inputs/FontSettingInput.tsx (1)
35-46: [Rewritten review comment]
[Exactly ONE classification tag]apps/meteor/client/views/admin/settings/Setting/inputs/BooleanSettingInput.tsx (1)
33-35: The referenced propmie={8}is not removed and remains present on line 33.The ResetSettingButton includes the
mie={8}margin-inline-end prop in the current code. There is no spacing change to verify because the prop was not removed.Likely an incorrect or invalid review comment.
apps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsx (1)
41-41: LGTM: QA attribute removal is clean.The removal of
data-qa-reset-setting-idsimplifies the component interface without affecting functionality.apps/meteor/client/views/admin/settings/Setting/inputs/ActionSettingInput.tsx (1)
16-16: LGTM: Unused prop removal is appropriate.The
_idprop has been correctly removed from the function signature since it was only used for the now-removeddata-qa-setting-idattribute.apps/meteor/client/views/admin/settings/Setting/inputs/SelectSettingInput.tsx (1)
40-40: LGTM: QA attribute removal is consistent.The removal of
data-qa-reset-setting-idaligns with the PR objective and simplifies the component API.apps/meteor/client/views/admin/settings/Setting/inputs/LookupSettingInput.tsx (1)
49-49: LGTM: Consistent QA attribute cleanup.The removal of
data-qa-reset-setting-idis consistent with the broader refactor across all settings input components.apps/meteor/client/views/admin/settings/Setting/inputs/LanguageSettingInput.tsx (1)
36-36: LGTM: Final QA attribute removal completed.The removal of
data-qa-reset-setting-idcompletes the consistent cleanup across all settings input components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts (1)
38-39: Translated strings in selectors will break when UI language changes.As noted in the previous review, using translated strings like
'Save changes'and'Back'ingetByRoleselectors will cause test failures when the UI language changes. Consider using stabledata-qa-idattributes instead, or implement a Page Object Model with stable selectors.Note: This test is currently skipped, so address this when re-enabling it.
Based on learnings, prefer stable selectors like data-qa-id attributes over localized text in getByRole() calls.
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts (1)
45-45: Reuse the stored locator variable.The locator
page.getByLabel('Hide_System_Messages')is duplicated here instead of reusing thesettingInputvariable defined on line 29. Consider extracting the label locator or adjusting the test to reuse the existing variable.💡 Example refactor
-await expect(page.getByLabel('Hide_System_Messages').getByRole('option', { name: 'User joined' })).toBeVisible(); +const hideSystemMessagesGroup = page.getByLabel('Hide_System_Messages'); +await expect(hideSystemMessagesGroup.getByRole('option', { name: 'User joined' })).toBeVisible();Alternatively, if the structure allows, you could check visibility directly on the
settingInputvariable's parent context.As per coding guidelines, store commonly used locators in variables/constants for reuse.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/settings-persistence-on-ui-navigation.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
CORE-1596
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Accessibility
✏️ Tip: You can customize this high-level summary in your review settings.