fix: livechat tags.save rejected by response validation on edit (+ e2e deflakes and Playwright GitHub annotations) - #41504
Conversation
Saving via the contextual bar closes it asynchronously once the request resolves. Tests that clicked Save and immediately reopened the panel could have it detached by the delayed close, causing timeouts (e.g. 'OC - Manage Agents [CE]- Edit and Remove'). Use the FlexTab save() helper (click + waitForDismissal) where available and assert form dismissal for the business hours page.
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnd-to-end tests now synchronize omnichannel saves, encrypted-room readiness, and asynchronous subscription updates before continuing with assertions. Playwright also enables GitHub reporting in GitHub Actions. ChangesE2E test synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41504 +/- ##
===========================================
- Coverage 68.69% 68.67% -0.03%
===========================================
Files 4134 4134
Lines 160417 160417
Branches 29312 29213 -99
===========================================
- Hits 110205 110169 -36
- Misses 45104 45136 +32
- Partials 5108 5112 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated inline implementation comments.
The save-helper changes are correct, but both comments violate the repository rule to avoid code comments in TypeScript/JavaScript implementation. Move the rationale into the relevant step names or page-object documentation.
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts#L84-L85: remove the comment while retainingeditAgent.save().apps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts#L100-L101: remove the comment while retainingeditTag.save().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts` around lines 84 - 85, Remove the inline implementation comment before editAgent.save() in apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts:84-85 while retaining the save call. Also remove the corresponding comment before editTag.save() in apps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts:100-101 while retaining that call; preserve the rationale through existing step names or page-object documentation if needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts`:
- Around line 84-85: Remove the inline implementation comment before
editAgent.save() in
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.ts:84-85 while
retaining the save call. Also remove the corresponding comment before
editTag.save() in
apps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts:100-101 while
retaining that call; preserve the rationale through existing step names or
page-object documentation if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c029825e-ef54-4eb9-a700-9169e8a03d7e
📒 Files selected for processing (3)
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (19)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test API Livechat (FIPS) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test API (FIPS) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test API Apps (node-runtime - EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test API Livechat (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test Federation Matrix
🧰 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/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.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/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.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/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.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/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-agents.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts
🔇 Additional comments (2)
apps/meteor/tests/e2e/omnichannel/omnichannel-tags.spec.ts (1)
117-117: LGTM!apps/meteor/tests/e2e/omnichannel/omnichannel-business-hours.spec.ts (1)
97-98: LGTM!Also applies to: 113-113
- rooms API: renaming a user updates DM subscription names asynchronously; poll subscriptions.getOne instead of a fixed 300ms sleep, which is not enough on slower runners (FIPS). - e2ee file encryption: wait for the room key to be ready before sending, otherwise the message goes out unencrypted and the key icon assertion fails (same guard already used by the encryption-decryption spec).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation comments from the changed TypeScript tests.
As per coding guidelines, TypeScript/Playwright test implementations should avoid code comments.
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts#L54-L54: remove the synchronization comment.apps/meteor/tests/end-to-end/api/rooms.ts#L2767-L2767: remove the polling rationale comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts` at line 54, Remove the implementation comment near the room-key synchronization in apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts:54-54 and remove the polling rationale comment in apps/meteor/tests/end-to-end/api/rooms.ts:2767-2767; leave the surrounding test logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts`:
- Line 55: Update the readiness wait around
EncryptedRoomPage.encryptionNotReadyIndicator so the test first waits for the
indicator to attach during key initialization, then waits for it to become not
visible before uploading. Do not rely solely on not.toBeVisible(), which can
pass before the indicator mounts.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts`:
- Line 54: Remove the implementation comment near the room-key synchronization
in apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts:54-54 and
remove the polling rationale comment in
apps/meteor/tests/end-to-end/api/rooms.ts:2767-2767; leave the surrounding test
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3de0be3d-735a-4d60-83b0-730e04cde026
📒 Files selected for processing (2)
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.tsapps/meteor/tests/end-to-end/api/rooms.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (5)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 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/e2e-encryption/e2ee-file-encryption.spec.tsapps/meteor/tests/end-to-end/api/rooms.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/e2e-encryption/e2ee-file-encryption.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/e2e-encryption/e2ee-file-encryption.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/e2e-encryption/e2ee-file-encryption.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.tsapps/meteor/tests/end-to-end/api/rooms.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.tsapps/meteor/tests/end-to-end/api/rooms.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.tsapps/meteor/tests/end-to-end/api/rooms.ts
🔇 Additional comments (2)
apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts (1)
3-3: LGTM!apps/meteor/tests/end-to-end/api/rooms.ts (1)
2768-2778: LGTM!Also applies to: 2791-2791, 2814-2814
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The unencrypted-message hint only mounts after the E2EE room state hook fires, so asserting it is not visible can pass before the room key exists. Poll the client E2ERoom state for READY instead. Also drop implementation comments from tests per coding guidelines.
TagEdit submits through useFormSubmitWithDirtyCheck: when the department selection is not registered the form is not dirty, so saving is a no-op and the contextual bar stays open. Assert the selected-department chip before saving so a lost selection fails at the right spot instead of timing out on dismissal.
Failed tests become check-run annotations (visible in the PR diff and via the check-runs API) instead of requiring log archaeology.
Opening the thread contextual bar fetches the thread messages; on coverage-instrumented runners this can exceed the default 5s expect timeout while the panel still shows its loading state.
BaseRaw updateOne/insertOne mutate the update object via setUpdatedAt, so createOrUpdateTag returned the record with an injected _updatedAt. The tags.save response schema declares additionalProperties: false (intentionally omitting _updatedAt), so every tag edit failed with error-invalid-body 400 while the write itself succeeded — the UI kept the contextual bar open with an error toast. Rebuild the declared return shape instead of returning the mutated record.
ggazzo
changed the title
|
/jira ARCH |
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Fixes flaky patterns spotted in recent CI runs.
1. Omnichannel UI: save → reopen contextual bar race
Saving via the contextual bar closes it asynchronously once the save request resolves. Tests that clicked
btnSaveand immediately searched/reopened the panel could have the freshly reopened panel detached by the delayed close, making the next click retry until the test times out.Example failure (CI run):
omnichannel-agents.spec.ts: useeditAgent.save()(click +waitForDismissal) instead of rawbtnSave.click()before reopening the agent info panel — same race already documented and worked around in the "Manage departments" test in this file.omnichannel-tags.spec.ts: same, viaeditTag.save().omnichannel-business-hours.spec.ts: no FlexTab here, so assertbtnSaveis gone after saving — same guard already used by the "Toggle BH active status" test in this file.All remaining
btnSave.click()call sites were audited: they are followed by assertions that implicitly wait for the save to take effect, so no race window.2. Rooms API: username rename propagation (failure on this PR's run)
update group dms nametests waited a fixedsleep(300)for the rename to propagate to DM subscription names, which is not enough on slower runners (failed on FIPS). Replaced with a poll onsubscriptions.getOne(up to ~5s).3. E2EE file encryption: message sent before room key is ready (failure on this PR's run)
With
E2E_Allow_Unencrypted_Messageson, sending right after opening a freshly created encrypted room can go out unencrypted if the room key is still being set up — the key icon assertion then fails. Added the sameencryptionNotReadyIndicatorguard the encryption-decryption spec already uses to the file-encryption spec'sbeforeEach.4. Threads: first assertion after opening the thread panel (failure on another run)
Opening the thread contextual bar fetches the thread messages; on coverage-instrumented runners this can exceed the default 5s expect timeout while the panel still shows its loading state (confirmed via the run's trace screenshot). Extended the first content assertion to 15s in both tests of the
hideFlexTabdescribe.5. CI: Playwright
githubreporterEnabled conditionally (
GITHUB_ACTIONS === 'true'), so failed tests become check-run annotations — visible in the PR diff and queryable viagh api repos/.../check-runs/<job_id>/annotations— instead of requiring log archaeology.6. Product bug:
livechat/tags.savealways failed response validation on edit (failure on this PR's run)The dismissal wait added in item 1 exposed a real bug:
BaseRaw.updateOne/insertOnemutate the update object viasetUpdatedAt, soLivechatTag.createOrUpdateTagreturned its local record with an injected_updatedAt. Thetags.saveresponse schema declaresadditionalProperties: false(its type intentionally omits_updatedAt), so every tag edit returnederror-invalid-body400 while the write itself succeeded — the UI kept the contextual bar open with an error toast, and the old test flow passed without ever saving anything (trace evidence: request payload correct, response 400 with the saved tag embedded in the error body).Fixed by rebuilding the declared return shape in
createOrUpdateTaginstead of returning the mutated record. Changeset added.Issue(s)
Steps to test or reproduce
Run the affected specs; the races are timing-dependent (reproduce under CI load):
Further comments
Summary by CodeRabbit
Task: ARCH-2291