fix: read receipts not turning blue when all active users have read#39246
Open
abhinavkrin wants to merge 4 commits intodevelopfrom
Open
fix: read receipts not turning blue when all active users have read#39246abhinavkrin wants to merge 4 commits intodevelopfrom
abhinavkrin wants to merge 4 commits intodevelopfrom
Conversation
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 6b40a46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThis patch release fixes read receipts not showing as read when all active users have read messages. The fix filters subscription counts to exclude archived subscriptions when determining if a message should be marked as read. User deactivation now archives their subscriptions, and reactivation unarchives them appropriately. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts (2)
22-29: Restore prior setting values instead of hardcodingfalsein teardown.Forcing both settings to
falsecan leak suite state when the initial environment value is different. Capture pre-test values in setup and restore those exact values in teardown.As per coding guidelines "Ensure clean state for each test execution in Playwright tests" and "Maintain test isolation between test cases in Playwright tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts` around lines 22 - 29, The teardown currently hardcodes false for 'Message_Read_Receipt_Enabled' and 'Message_Read_Receipt_Store_Users'; instead capture the original values before mutating (e.g., in test.beforeAll use getSettingValueById for 'Message_Read_Receipt_Enabled' and 'Message_Read_Receipt_Store_Users' and store them in outer-scope variables like originalReadReceiptEnabled and originalStoreUsers) and then in test.afterAll restore those exact stored values by calling setSettingValueById with the saved variables; ensure the getSettingValueById calls happen before you change settings and that you await the restore calls in afterAll.
74-103: Replacepage.locator()with semantic selectors and extract repeated read-receipts patterns.Lines 74, 89, and 102 use
page.locator('role=menuitem[name="Read receipts"]')instead of the semanticgetByRole()API. Additionally, the read-receipts menu interaction (opening menu, clicking "Read receipts", verifying count, closing dialog) is repeated identically across all three test steps and should be extracted into a shared constant or helper function.♻️ Suggested refactor
+ const readReceiptsMenuItem = page.getByRole('menuitem', { name: 'Read receipts' }); + const readReceiptsDialogItems = page.getByRole('dialog').getByRole('listitem'); + const closeReadReceiptsDialog = page.getByRole('button', { name: 'Close' }); + + const assertReadReceiptsCount = async (count: number): Promise<void> => { + await poHomeChannel.content.openLastMessageMenu(); + await readReceiptsMenuItem.click(); + await expect(readReceiptsDialogItems).toHaveCount(count); + await closeReadReceiptsDialog.click(); + }; + await test.step('when all users are active', async () => { await poHomeChannel.content.sendMessage('Message 1: All three users active'); @@ - await poHomeChannel.content.openLastMessageMenu(); - await page.locator('role=menuitem[name="Read receipts"]').click(); - await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(3); - await page.getByRole('button', { name: 'Close' }).click(); + await assertReadReceiptsCount(3); }); @@ - await poHomeChannel.content.openLastMessageMenu(); - await page.locator('role=menuitem[name="Read receipts"]').click(); - await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(2); - await page.getByRole('button', { name: 'Close' }).click(); + await assertReadReceiptsCount(2); }); @@ - await poHomeChannel.content.openLastMessageMenu(); - await page.locator('role=menuitem[name="Read receipts"]').click(); - await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(1); - await page.getByRole('button', { name: 'Close' }).click(); + await assertReadReceiptsCount(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts` around lines 74 - 103, Replace the non-semantic page.locator('role=menuitem[name="Read receipts"]') calls with the semantic getByRole API (e.g., page.getByRole('menuitem', { name: 'Read receipts' })) and factor the repeated read-receipts flow into a helper used by the three test steps: create a small utility (e.g., openReadReceiptsAndAssertCount) that calls poHomeChannel.content.openLastMessageMenu(), clicks the "Read receipts" menu via page.getByRole, asserts dialog listitem count, and closes the dialog; then replace the three inline blocks in the test (the steps around poHomeChannel.content.openLastMessageMenu / page.locator / expect(...toHaveCount) / Close button) with calls to that helper, passing the expected count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/models/src/models/Subscriptions.ts`:
- Around line 1315-1357: The method unarchiveByRoomId currently returns true if
any archived subscription exists in the room even if none are actually
unarchived (e.g., all belong to deactivated users); change it to first run the
aggregation (use the same match + lookup + project stages but REMOVE the $merge)
to produce an array of subscription _id values to unarchive, return false
immediately if that array is empty, otherwise call updateMany({ _id: { $in: ids
} }, { $set: { archived: false, open: true, alert: false } }) on this.col and
return whether the update modified any documents (modifiedCount > 0); keep
references to the existing function name unarchiveByRoomId, this.col,
Users.getCollectionName(), and this.getCollectionName() to locate the code.
---
Nitpick comments:
In `@apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts`:
- Around line 22-29: The teardown currently hardcodes false for
'Message_Read_Receipt_Enabled' and 'Message_Read_Receipt_Store_Users'; instead
capture the original values before mutating (e.g., in test.beforeAll use
getSettingValueById for 'Message_Read_Receipt_Enabled' and
'Message_Read_Receipt_Store_Users' and store them in outer-scope variables like
originalReadReceiptEnabled and originalStoreUsers) and then in test.afterAll
restore those exact stored values by calling setSettingValueById with the saved
variables; ensure the getSettingValueById calls happen before you change
settings and that you await the restore calls in afterAll.
- Around line 74-103: Replace the non-semantic
page.locator('role=menuitem[name="Read receipts"]') calls with the semantic
getByRole API (e.g., page.getByRole('menuitem', { name: 'Read receipts' })) and
factor the repeated read-receipts flow into a helper used by the three test
steps: create a small utility (e.g., openReadReceiptsAndAssertCount) that calls
poHomeChannel.content.openLastMessageMenu(), clicks the "Read receipts" menu via
page.getByRole, asserts dialog listitem count, and closes the dialog; then
replace the three inline blocks in the test (the steps around
poHomeChannel.content.openLastMessageMenu / page.locator /
expect(...toHaveCount) / Close button) with calls to that helper, passing the
expected count.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/hot-lies-divide.mdapps/meteor/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/app/lib/server/functions/unarchiveRoom.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.spec.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/IRoomsModel.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Rooms.tspackages/models/src/models/Subscriptions.ts
📜 Review details
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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:
packages/models/src/models/Rooms.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/models/src/models/Subscriptions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/app/lib/server/functions/unarchiveRoom.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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/read-receipts-deactivated-users.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/read-receipts-deactivated-users.spec.ts
🧠 Learnings (29)
📚 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:
packages/models/src/models/Rooms.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/models/src/models/Subscriptions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/app/lib/server/functions/unarchiveRoom.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:
packages/models/src/models/Rooms.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/models/src/models/Subscriptions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/app/lib/server/functions/unarchiveRoom.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/hot-lies-divide.md
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/hot-lies-divide.md
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/hot-lies-divide.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.changeset/hot-lies-divide.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/hot-lies-divide.md
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/model-typings/src/models/IRoomsModel.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/models/src/models/Subscriptions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/app/lib/server/functions/unarchiveRoom.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts
📚 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/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/e2e/read-receipts-deactivated-users.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} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/read-receipts-deactivated-users.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/read-receipts-deactivated-users.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/read-receipts-deactivated-users.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/read-receipts-deactivated-users.spec.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/tests/e2e/read-receipts-deactivated-users.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/read-receipts-deactivated-users.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/models/src/models/Subscriptions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/app/lib/server/functions/unarchiveRoom.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
packages/model-typings/src/models/ISubscriptionsModel.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/lib/server/functions/unarchiveRoom.ts
🧬 Code graph analysis (6)
packages/models/src/models/Rooms.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(12-85)packages/apps-engine/src/definition/rooms/IRoom.ts (1)
IRoom(4-28)packages/ui-contexts/src/index.ts (1)
FindOptions(110-110)
packages/model-typings/src/models/IRoomsModel.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(12-85)packages/apps-engine/src/definition/rooms/IRoom.ts (1)
IRoom(4-28)packages/ui-contexts/src/index.ts (1)
FindOptions(110-110)
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts (1)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
setUserActiveStatus(59-179)
apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts (1)
apps/meteor/tests/e2e/page-objects/home-channel.ts (1)
HomeChannel(7-141)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
packages/models/src/index.ts (1)
Subscriptions(189-189)
apps/meteor/app/lib/server/functions/unarchiveRoom.ts (1)
packages/models/src/index.ts (1)
Subscriptions(189-189)
🔇 Additional comments (11)
packages/model-typings/src/models/ISubscriptionsModel.ts (1)
41-42: Type-level API updates are coherent with the new archived-aware flow.These signature changes are consistent with the updated model contracts and callers.
Also applies to: 245-247
packages/models/src/models/Subscriptions.ts (1)
70-70: The archived-aware query updates are solid.These changes correctly align indexing, counting, and user-scoped archive/unarchive behavior with the read-receipt fix.
Also applies to: 169-178, 1287-1288, 1359-1373, 1785-1788
packages/model-typings/src/models/IRoomsModel.ts (1)
264-264: IRoomsModel addition is clear and correctly scoped.The new method signature is appropriate for archived-room filtering in activation/reactivation flows.
.changeset/hot-lies-divide.md (1)
1-7: Changeset entry is accurate and appropriately scoped.Version bumps and summary text match the fix intent.
apps/meteor/app/lib/server/functions/unarchiveRoom.ts (1)
10-14: Good notification gating inunarchiveRoom.Conditionally emitting subscription-change events here is the right control-flow improvement.
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
75-75: Correct method switch for read-receipt eligibility checks.Using the unarchived-only counter here aligns with the intended “active users only” behavior.
packages/models/src/models/Rooms.ts (1)
1600-1607:findArchivedByRoomIdsimplementation looks good.The query shape is clean and appropriate for the reactivation exclusion flow.
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts (1)
5-239: Test coverage for activation/deactivation subscription behavior is strong.The scenarios cover both positive and no-op branches, plus key error cases, and align well with the code-path changes.
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (2)
22-27: Good reactivation helper for preserving archived-room intent.This helper cleanly separates fetching user room ids from filtering admin-archived rooms before unarchiving subscriptions.
122-134:modifiedCount-guarded subscription notifications look correct.Only notifying on actual subscription mutations avoids unnecessary subscription-changed broadcasts during activate/deactivate transitions.
apps/meteor/tests/e2e/read-receipts-deactivated-users.spec.ts (1)
65-105: Nice use oftest.step()to structure state transitions.The three-step progression (all active → some deactivated → sender alone) is clear and directly maps to the read-receipt behavior being validated.
As per coding guidelines "Use
test.step()for complex test scenarios to improve organization in Playwright tests".
Comment on lines
+1315
to
+1357
| async unarchiveByRoomId(roomId: string): Promise<boolean> { | ||
| const hasArchived = await this.col.countDocuments({ rid: roomId, archived: true }, { limit: 1 }); | ||
| if (!hasArchived) { | ||
| return false; | ||
| } | ||
|
|
||
| await this.col | ||
| .aggregate( | ||
| [ | ||
| { $match: { rid: roomId, archived: true } }, | ||
| { $project: { '_id': 1, 'u._id': 1 } }, | ||
| { | ||
| $lookup: { | ||
| from: Users.getCollectionName(), | ||
| localField: 'u._id', | ||
| foreignField: '_id', | ||
| as: '_user', | ||
| pipeline: [{ $project: { active: 1 } }], | ||
| }, | ||
| }, | ||
| { $match: { '_user.active': true } }, | ||
| { | ||
| $project: { | ||
| _id: 1, | ||
| archived: { $literal: false }, | ||
| open: { $literal: true }, | ||
| alert: { $literal: false }, | ||
| }, | ||
| }, | ||
| { | ||
| $merge: { | ||
| into: this.getCollectionName(), | ||
| whenMatched: 'merge', | ||
| whenNotMatched: 'discard', | ||
| }, | ||
| }, | ||
| ], | ||
| { allowDiskUse: true }, | ||
| ) | ||
| .toArray(); | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Return value should reflect actual unarchive updates, not just archived presence.
Line [1316] checks only for any archived subscription in the room. If all archived subscriptions belong to deactivated users, no document is unarchived but this method still returns true, which can trigger downstream notifications with no state change.
🔧 Proposed fix
async unarchiveByRoomId(roomId: string): Promise<boolean> {
- const hasArchived = await this.col.countDocuments({ rid: roomId, archived: true }, { limit: 1 });
- if (!hasArchived) {
+ const hasArchivedForActiveUsers = await this.col
+ .aggregate([
+ { $match: { rid: roomId, archived: true } },
+ { $project: { 'u._id': 1 } },
+ {
+ $lookup: {
+ from: Users.getCollectionName(),
+ localField: 'u._id',
+ foreignField: '_id',
+ as: '_user',
+ pipeline: [{ $project: { active: 1 } }],
+ },
+ },
+ { $match: { '_user.active': true } },
+ { $limit: 1 },
+ ])
+ .hasNext();
+
+ if (!hasArchivedForActiveUsers) {
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async unarchiveByRoomId(roomId: string): Promise<boolean> { | |
| const hasArchived = await this.col.countDocuments({ rid: roomId, archived: true }, { limit: 1 }); | |
| if (!hasArchived) { | |
| return false; | |
| } | |
| await this.col | |
| .aggregate( | |
| [ | |
| { $match: { rid: roomId, archived: true } }, | |
| { $project: { '_id': 1, 'u._id': 1 } }, | |
| { | |
| $lookup: { | |
| from: Users.getCollectionName(), | |
| localField: 'u._id', | |
| foreignField: '_id', | |
| as: '_user', | |
| pipeline: [{ $project: { active: 1 } }], | |
| }, | |
| }, | |
| { $match: { '_user.active': true } }, | |
| { | |
| $project: { | |
| _id: 1, | |
| archived: { $literal: false }, | |
| open: { $literal: true }, | |
| alert: { $literal: false }, | |
| }, | |
| }, | |
| { | |
| $merge: { | |
| into: this.getCollectionName(), | |
| whenMatched: 'merge', | |
| whenNotMatched: 'discard', | |
| }, | |
| }, | |
| ], | |
| { allowDiskUse: true }, | |
| ) | |
| .toArray(); | |
| return true; | |
| } | |
| async unarchiveByRoomId(roomId: string): Promise<boolean> { | |
| const hasArchivedForActiveUsers = await this.col | |
| .aggregate([ | |
| { $match: { rid: roomId, archived: true } }, | |
| { $project: { 'u._id': 1 } }, | |
| { | |
| $lookup: { | |
| from: Users.getCollectionName(), | |
| localField: 'u._id', | |
| foreignField: '_id', | |
| as: '_user', | |
| pipeline: [{ $project: { active: 1 } }], | |
| }, | |
| }, | |
| { $match: { '_user.active': true } }, | |
| { $limit: 1 }, | |
| ]) | |
| .hasNext(); | |
| if (!hasArchivedForActiveUsers) { | |
| return false; | |
| } | |
| await this.col | |
| .aggregate( | |
| [ | |
| { $match: { rid: roomId, archived: true } }, | |
| { $project: { '_id': 1, 'u._id': 1 } }, | |
| { | |
| $lookup: { | |
| from: Users.getCollectionName(), | |
| localField: 'u._id', | |
| foreignField: '_id', | |
| as: '_user', | |
| pipeline: [{ $project: { active: 1 } }], | |
| }, | |
| }, | |
| { $match: { '_user.active': true } }, | |
| { | |
| $project: { | |
| _id: 1, | |
| archived: { $literal: false }, | |
| open: { $literal: true }, | |
| alert: { $literal: false }, | |
| }, | |
| }, | |
| { | |
| $merge: { | |
| into: this.getCollectionName(), | |
| whenMatched: 'merge', | |
| whenNotMatched: 'discard', | |
| }, | |
| }, | |
| ], | |
| { allowDiskUse: true }, | |
| ) | |
| .toArray(); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/models/Subscriptions.ts` around lines 1315 - 1357, The
method unarchiveByRoomId currently returns true if any archived subscription
exists in the room even if none are actually unarchived (e.g., all belong to
deactivated users); change it to first run the aggregation (use the same match +
lookup + project stages but REMOVE the $merge) to produce an array of
subscription _id values to unarchive, return false immediately if that array is
empty, otherwise call updateMany({ _id: { $in: ids } }, { $set: { archived:
false, open: true, alert: false } }) on this.col and return whether the update
modified any documents (modifiedCount > 0); keep references to the existing
function name unarchiveByRoomId, this.col, Users.getCollectionName(), and
this.getCollectionName() to locate the code.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39246 +/- ##
===========================================
+ Coverage 70.65% 70.74% +0.09%
===========================================
Files 3189 3193 +4
Lines 112715 113166 +451
Branches 20420 20539 +119
===========================================
+ Hits 79640 80062 +422
- Misses 31034 31055 +21
- Partials 2041 2049 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
This PR fixes a bug where the "Message viewed" indicator (double blue ticks) would not appear even after all active users in a channel had read a message, if any deactivated users remained as members.
Three issues combined:
getMinimumLastSeenByRoomIdincluded archived (deactivated) subscriptions in the min-ls calculation. A deactivated user'slsnever advances, so the "fully read" state was permanently blocked.setArchivedByUsernameon deactivation only archived DM subscriptions — channels and groups were left unarchived, so fix 1 had no effect there.countByRoomIdAndNotUserId(used inmarkMessageAsReadBySender) didn't exclude archived subscriptions, so a sender alone with only deactivated members never got their message auto-marked as read.Changes
getMinimumLastSeenByRoomId: addedarchived: { $ne: true }filter + supporting compound index{ rid, archived, ls }setArchivedByUsername: changed query from{ t: 'd', name }→{ 'u.username' }to archive all room types on deactivationsetUserActiveStatus: reactivation now unarchives subscriptions only for non-archived rooms (preserves admin-archived room state)countByRoomIdAndNotUserId→countUnarchivedByRoomIdAndNotUserId: excludes deactivated users from the "is sender alone" checkfindArchivedByRoomIds(Rooms),unarchiveByUsernameExcludingRoomIds(Subscriptions)unarchiveByRoomId(Subscriptions): when a room is unarchived, deactivated users' subscriptions are now left archived. Uses a server-side aggregation pipeline —$lookupto join with users, filter toactive: true, then$mergeto write back only the matched subscriptions. An earlycountDocumentswithlimit: 1short-circuits if there are no archived subscriptions. The aggregation uses early$projectto minimize document size in-flight andallowDiskUse: trueas a safety net for very large rooms.unarchiveRoom: delegates entirely to the model method and conditionally fires the subscription change notification based on the return value.Issue(s)
Steps to test or reproduce
Further comments
Why archive subscriptions instead of filtering by user active status?
The read receipt query (
getMinimumLastSeenByRoomId) finds the earliestlsacross all room members to determine if everyone has read a message. The natural alternative fix would be to join against the users collection and filter out inactive users at query time — but subscriptions don't embeduser.active, so that would require either a$lookupaggregation (expensive on every message read event) or denormalizingactiveonto each subscription (a larger, riskier change).The subscription
archivedfield already exists and is indexed. Archiving a user's subscriptions on deactivation is also semantically correct — a deactivated user is effectively removed from active participation in rooms. This keeps the read receipt query a simple single-collection find with a covered index.Why exclude archived rooms during reactivation?
A naive reactivation would unarchive all the user's subscriptions. But if an admin has explicitly archived a room, that room's subscriptions should stay archived regardless of user reactivation — otherwise reactivating a user would silently undo an admin action. The fix fetches the current archive state of each room and excludes those room IDs from the unarchive operation, so only subscriptions to actively open rooms are restored.
CORE-1207
Summary by CodeRabbit