fix: read receipts not turning blue when all active users have read#39246
Open
abhinavkrin wants to merge 19 commits intodevelopfrom
Open
fix: read receipts not turning blue when all active users have read#39246abhinavkrin wants to merge 19 commits intodevelopfrom
abhinavkrin wants to merge 19 commits intodevelopfrom
Conversation
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
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: 05c8922 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
|
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:
WalkthroughAdjusts subscription archive/unarchive flows and read-receipt calculations to ignore archived (deactivated) subscriptions; updates Subscriptions model API and indexes; switches notifications to userId-based variants; adds lifecycle hooks, unit and E2E tests, and release metadata. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Admin as Admin/User
participant Server as Server (user-active handlers)
participant Sub as Subscriptions DB
participant Receipt as ReadReceipt Service
participant Notify as Notifier
Admin->>Server: deactivate(userId)
Server->>Sub: setArchivedByUserId(userId, true)
Sub-->>Server: modifiedCount/affected
alt any archived
Server->>Notify: notifyOnSubscriptionChangedByUserId(userId)
end
Admin->>Receipt: message posted / check read state
Receipt->>Sub: countUnarchivedByRoomIdAndNotUserId(rid, userId)
Sub-->>Receipt: count (excludes archived/deactivated)
Receipt->>Receipt: determine read state (all active read?)
Admin->>Server: activate(userId)
Server->>Sub: hasArchivedSubscriptionsInNonArchivedRoomsByUserId(userId)
Sub-->>Server: true/false
alt true
Server->>Sub: unarchiveByUserIdExceptForArchivedRooms(userId)
Sub-->>Server: result
Server->>Notify: notifyOnSubscriptionChangedByUserId(userId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. 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 |
![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".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39246 +/- ##
===========================================
- Coverage 70.95% 70.54% -0.42%
===========================================
Files 3197 3272 +75
Lines 113338 116856 +3518
Branches 20567 21273 +706
===========================================
+ Hits 80423 82439 +2016
- Misses 30871 32366 +1495
- Partials 2044 2051 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
abhinavkrin
force-pushed
the
fix/read-receipts-ignore-deactivated-users
branch
2 times, most recently
from
8f37b52 to
6d56a2b
Compare
Signed-off-by: Abhinav Kumar <[email protected]>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/e2e/utils/user-helpers.ts">
<violation number="1" location="apps/meteor/tests/e2e/utils/user-helpers.ts:77">
P2: loginTestUser always uses DEFAULT_USER_CREDENTIALS.password, so it will fail for users created with a custom password. Consider accepting the password as an argument (defaulting to the default password) or storing the generated password on ITestUser.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pierre-lehnen-rc
requested changes
Mar 10, 2026
Signed-off-by: Abhinav Kumar <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
115-125: Consider guarding the activation notification with a modification check for consistency.The deactivation path (lines 117-120) correctly guards the notification call with
modifiedCount, avoiding unnecessary broadcasts when no subscriptions were modified. However, the activation path (lines 122-123) unconditionally firesnotifyOnSubscriptionChangedByUserIdregardless of whether any subscriptions were actually unarchived.Since
unarchiveByUserIdExceptForArchivedRoomsreturnsPromise<void>(per Context Snippet 1 using$mergeaggregation), the modification count isn't directly available. If feasible, consider updating the model method to return modification info, or alternatively perform a preliminarycountDocumentscheck before the unarchive operation.Potential approach using a preliminary count
} else if (active === true && !user.active) { - await Subscriptions.unarchiveByUserIdExceptForArchivedRooms(userId); - void notifyOnSubscriptionChangedByUserId(userId); + const hadArchivedSubs = await Subscriptions.countDocuments({ 'u._id': userId, archived: true }, { limit: 1 }) > 0; + if (hadArchivedSubs) { + await Subscriptions.unarchiveByUserIdExceptForArchivedRooms(userId); + void notifyOnSubscriptionChangedByUserId(userId); + } }Alternatively, the model method could be enhanced to return the number of modified documents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/setUserActiveStatus.ts` around lines 115 - 125, The activation path in setUserActiveStatus currently always calls notifyOnSubscriptionChangedByUserId after calling Subscriptions.unarchiveByUserIdExceptForArchivedRooms even though that method returns void; update the logic so the notification is only sent when subscriptions were actually modified by either (a) changing Subscriptions.unarchiveByUserIdExceptForArchivedRooms to return modification info (e.g., modifiedCount) and checking that before invoking notifyOnSubscriptionChangedByUserId, or (b) performing a preliminary Subscriptions.countDocuments (or equivalent query) to determine how many archived subscriptions would be unarchived for the given userId and only calling notifyOnSubscriptionChangedByUserId when that count > 0; reference the existing deactivation pattern which uses Subscriptions.setArchivedByUsername and its modifiedCount check to mirror behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/functions/setUserActiveStatus.ts`:
- Around line 115-125: The activation path in setUserActiveStatus currently
always calls notifyOnSubscriptionChangedByUserId after calling
Subscriptions.unarchiveByUserIdExceptForArchivedRooms even though that method
returns void; update the logic so the notification is only sent when
subscriptions were actually modified by either (a) changing
Subscriptions.unarchiveByUserIdExceptForArchivedRooms to return modification
info (e.g., modifiedCount) and checking that before invoking
notifyOnSubscriptionChangedByUserId, or (b) performing a preliminary
Subscriptions.countDocuments (or equivalent query) to determine how many
archived subscriptions would be unarchived for the given userId and only calling
notifyOnSubscriptionChangedByUserId when that count > 0; reference the existing
deactivation pattern which uses Subscriptions.setArchivedByUsername and its
modifiedCount check to mirror behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e48742f-1589-4af2-8bb5-130f4d809f10
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/setUserActiveStatus.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 (1)
**/*.{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/app/lib/server/functions/setUserActiveStatus.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
📚 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:
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: 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.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.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: 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/app/lib/server/functions/setUserActiveStatus.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/app/lib/server/functions/setUserActiveStatus.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
18-20: LGTM on the notification import change.The switch to
notifyOnSubscriptionChangedByUserIdaligns with the userId-based flow introduced by this PR and correctly matches the subscription modification operations below.
Signed-off-by: Abhinav Kumar <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/models/src/models/Subscriptions.ts (2)
1359-1377: Consider adding$projectto limit fields in$lookup.The
$lookupfrom Rooms doesn't restrict projected fields, potentially fetching entire room documents. For rooms with large custom field payloads, this could impact memory.♻️ Proposed optimization
{ $lookup: { from: Rooms.getCollectionName(), localField: 'rid', foreignField: '_id', as: 'room', + pipeline: [{ $project: { archived: 1 } }], }, },🤖 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 1359 - 1377, The lookup pulls full room documents which can be large; in hasArchivedSubscriptionsInNonArchivedRoomsByUserId add a projection stage after the $lookup to retain only the fields needed (e.g., room._id and room.archived) or use a $lookup pipeline that projects those fields from Rooms.getCollectionName(), so the subsequent match on 'room.0.archived' only inspects a small projected object.
1379-1401: Inconsistent field updates compared tounarchiveByRoomId.This method only sets
archived: false, whileunarchiveByRoomId(lines 1336-1343) also setsopen: true, alert: false. When a user is reactivated, their subscriptions may remain closed/hidden, which differs from room-level unarchive behavior.If intentional (preserving user's previous open/alert state), this is fine. Otherwise, consider aligning the update fields.
♻️ Proposed fix for consistency
{ $merge: { on: '_id', into: this.getCollectionName(), - whenMatched: [{ $set: { archived: false } }], + whenMatched: [{ $set: { archived: false, open: true, alert: false } }], whenNotMatched: 'discard', }, },🤖 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 1379 - 1401, The unarchiveByUserIdExceptForArchivedRooms method only sets archived:false, which is inconsistent with unarchiveByRoomId; update the aggregation's whenMatched $set to include the same fields as unarchiveByRoomId (archived: false, open: true, alert: false) so user subscriptions are reopened and alerts cleared when unarchived; modify the $merge stage in unarchiveByUserIdExceptForArchivedRooms to set open: true and alert: false along with archived: false to match the behavior of unarchiveByRoomId.apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts (1)
119-138: Consider adding edge case for already-inactive user deactivation.The current tests don't cover the scenario where
active === falseanduser.active === false(deactivating an already deactivated user). This is a no-op path that might be worth verifying doesn't trigger archival.💡 Suggested additional test
it('should not archive subscriptions when deactivating already inactive user', async () => { stubs.Users.findOneById.resolves({ _id: userId, username, active: false }); await setUserActiveStatus(userId, false); // The code currently doesn't have this guard, so this test would help // ensure no redundant archival operations expect(stubs.Subscriptions.setArchivedByUsername.calledOnce).to.be.true; // or false if guarded });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts` around lines 119 - 138, Add a test covering the no-op path when deactivating an already-inactive user: in the spec for Subscription archiving on deactivation, stub Users.findOneById to return a user object with active: false and then call setUserActiveStatus(userId, false); assert that Subscriptions.setArchivedByUsername is not invoked (or invoked exactly as your intended behavior) and that notifyOnSubscriptionChangedByUserId is not called; this ensures setUserActiveStatus short-circuits when user.active is already false and prevents redundant archival logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts`:
- Around line 119-138: Add a test covering the no-op path when deactivating an
already-inactive user: in the spec for Subscription archiving on deactivation,
stub Users.findOneById to return a user object with active: false and then call
setUserActiveStatus(userId, false); assert that
Subscriptions.setArchivedByUsername is not invoked (or invoked exactly as your
intended behavior) and that notifyOnSubscriptionChangedByUserId is not called;
this ensures setUserActiveStatus short-circuits when user.active is already
false and prevents redundant archival logic.
In `@packages/models/src/models/Subscriptions.ts`:
- Around line 1359-1377: The lookup pulls full room documents which can be
large; in hasArchivedSubscriptionsInNonArchivedRoomsByUserId add a projection
stage after the $lookup to retain only the fields needed (e.g., room._id and
room.archived) or use a $lookup pipeline that projects those fields from
Rooms.getCollectionName(), so the subsequent match on 'room.0.archived' only
inspects a small projected object.
- Around line 1379-1401: The unarchiveByUserIdExceptForArchivedRooms method only
sets archived:false, which is inconsistent with unarchiveByRoomId; update the
aggregation's whenMatched $set to include the same fields as unarchiveByRoomId
(archived: false, open: true, alert: false) so user subscriptions are reopened
and alerts cleared when unarchived; modify the $merge stage in
unarchiveByUserIdExceptForArchivedRooms to set open: true and alert: false along
with archived: false to match the behavior of unarchiveByRoomId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6860b88-e369-44fb-bb05-fd4dac4101d0
📒 Files selected for processing (4)
apps/meteor/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/ISubscriptionsModel.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: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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.ts
🧠 Learnings (19)
📚 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:
apps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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.tspackages/models/src/models/Subscriptions.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/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
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 tests run reliably in parallel without shared state conflicts
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 : All test files must be created in `apps/meteor/tests/e2e/` directory
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.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 : 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-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.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.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: 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.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/unit/app/lib/server/functions/setUserActiveStatus.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: 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/models/src/models/Subscriptions.ts
🔇 Additional comments (9)
packages/models/src/models/Subscriptions.ts (5)
70-70: LGTM!The compound index
{ rid: 1, archived: 1, ls: 1 }correctly supports the updatedgetMinimumLastSeenByRoomIdquery which now filters onarchived: { $ne: true }and sorts byls.
169-178: LGTM!The new method correctly counts unarchived subscriptions for a room excluding the specified user. Query structure aligns with the existing
countUnarchivedByRoomIdpattern.
1283-1298: LGTM!Adding
archived: { $ne: true }correctly excludes deactivated users' subscriptions from the minimum last-seen calculation, which is essential for accurate read receipt determination.
1315-1357: Return value may not reflect actual unarchive updates.The method returns
truebased on whether any archived subscriptions exist (line 1316), not whether any were actually unarchived. If all archived subscriptions belong to inactive users, the aggregation pipeline will update zero documents but the method still returnstrue. This could trigger downstream notifications with no actual state change.
1814-1826: LGTM!Changing the query from
{ t: 'd', name: username }to'u.username': usernamecorrectly expands archival to all room types (not just DMs), which is necessary to exclude deactivated users from read receipt calculations across all rooms.packages/model-typings/src/models/ISubscriptionsModel.ts (2)
41-42: LGTM!Type signature correctly matches the implementation.
245-247: LGTM!The updated return type for
unarchiveByRoomIdand new method signatures align with the implementation changes.apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
116-127: LGTM!The restructured logic correctly:
- Guards deactivation notification with
modifiedCountcheck- Uses
hasArchivedSubscriptionsInNonArchivedRoomsByUserIdas an early check to avoid unnecessary unarchive operations- Follows the suggested grouping pattern from prior review
The shift to
notifyOnSubscriptionChangedByUserIdstill broadcasts all user subscriptions, but the guard conditions minimize unnecessary notifications.apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts (1)
1-207: LGTM!Comprehensive test coverage for the subscription archiving/unarchiving logic. The test structure is clean with proper isolation using proxyquire and sinon.
Signed-off-by: Abhinav Kumar <[email protected]>
KevLehman
requested changes
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 169-178: The query in countUnarchivedByRoomIdAndNotUserId
currently uses 'u._id': { $ne: uid } which will match documents where u._id is
missing; update the query to mirror countUnarchivedByRoomId by adding an $exists
check so the user id must exist and not equal uid (i.e., change the 'u._id'
condition to include $exists: true alongside $ne: uid) to avoid counting
subscriptions without a user id.
🪄 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: 6cd103a3-4bb9-4f87-aa17-f497ae2351e4
📒 Files selected for processing (1)
packages/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 (1)
**/*.{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/Subscriptions.ts
🧠 Learnings (7)
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
packages/models/src/models/Subscriptions.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 by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Subscriptions.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:
packages/models/src/models/Subscriptions.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/models/src/models/Subscriptions.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:
packages/models/src/models/Subscriptions.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:
packages/models/src/models/Subscriptions.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/Subscriptions.ts
🔇 Additional comments (6)
packages/models/src/models/Subscriptions.ts (6)
70-70: LGTM!The compound index
{ rid: 1, archived: 1, ls: 1 }correctly supports the updatedgetMinimumLastSeenByRoomIdquery which now filters byridandarchived, then sorts byls.
1283-1298: LGTM!Adding
archived: { $ne: true }correctly excludes archived subscriptions (from deactivated users) from the minimum last-seen calculation, fixing the read-receipt bug. The new compound index supports this query.
1315-1357: Return value may incorrectly signal updates when none occurred.Line 1316 checks for any archived subscription, but the aggregation only unarchives subscriptions belonging to active users. If all archived subscriptions belong to inactive users, the method returns
truedespite no documents being updated, potentially triggering unnecessary downstream notifications.
1359-1378: LGTM!The aggregation correctly identifies archived subscriptions in non-archived rooms by joining with the Rooms collection and checking the room's archived status. The
{ $ne: true }filter properly handles rooms wherearchivedis eitherfalseor undefined.
1816-1828: LGTM!The query change from
{ t: 'd', name: username }to{ 'u.username': username }correctly archives all subscriptions for a deactivated user across all room types, not just DMs. This aligns with the PR objective to exclude deactivated users from read-receipt calculations.
1380-1404: Inconsistency in archived subscription fields during user reactivation.
unarchiveByRoomIdexplicitly setsarchived: false,open: true, andalert: false, whereasunarchiveByUserIdExceptForArchivedRoomsonly setsarchived: false. Since this method is called during user reactivation, leavingopen: falsemay prevent unarchived subscriptions from appearing in the user's sidebar. Clarify whether this difference is intentional—if rooms should become visible on reactivation, addopen: trueandalert: falseto the$setoperation to match the room unarchive behavior.
Signed-off-by: Abhinav Kumar <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts (1)
1-131: Well-structured test setup with limited coverage.The test file has excellent dependency mocking infrastructure and proper cleanup. The current tests cover critical error paths (user not found, federated users).
Consider expanding coverage to include:
- Activation/deactivation happy paths verifying callbacks are triggered
- Last admin protection logic
- Room ownership handling paths
However, since the subscription archival behavior is tested separately in
afterUserActions.spec.ts, the current scope may be acceptable for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts` around lines 1 - 131, The tests only cover error paths; add unit tests for the happy activation/deactivation flow and edge cases: create tests that call setUserActiveStatus(userId, true/false) and assert Users.setUserActive was called with correct args, callbacks.run was invoked, Rooms.setDmReadOnlyByUserId updated DMs, and notifyOnUserChange/notifyOnRoomChangedByUserDM were called; add a test for last-admin protection by stubbing Users.findOneAdmin and Users.countActiveUsersInRoles to simulate a single remaining admin and assert the function prevents deactivation; add a test for ownership transfer by stubbing shouldRemoveOrChangeOwner to true and ensuring relinquishRoomOwnerships/getUserSingleOwnedRooms/getSubscribedRoomsForUserWithDetails are invoked and handled accordingly; reuse existing stubs (Users, Rooms, callbacks, shouldRemoveOrChangeOwner, relinquishRoomOwnerships, notifyOnUserChange) and reset them in afterEach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts`:
- Around line 1-131: The tests only cover error paths; add unit tests for the
happy activation/deactivation flow and edge cases: create tests that call
setUserActiveStatus(userId, true/false) and assert Users.setUserActive was
called with correct args, callbacks.run was invoked, Rooms.setDmReadOnlyByUserId
updated DMs, and notifyOnUserChange/notifyOnRoomChangedByUserDM were called; add
a test for last-admin protection by stubbing Users.findOneAdmin and
Users.countActiveUsersInRoles to simulate a single remaining admin and assert
the function prevents deactivation; add a test for ownership transfer by
stubbing shouldRemoveOrChangeOwner to true and ensuring
relinquishRoomOwnerships/getUserSingleOwnedRooms/getSubscribedRoomsForUserWithDetails
are invoked and handled accordingly; reuse existing stubs (Users, Rooms,
callbacks, shouldRemoveOrChangeOwner, relinquishRoomOwnerships,
notifyOnUserChange) and reset them in afterEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98052ce7-769f-4c75-9881-bb9842a20742
📒 Files selected for processing (8)
apps/meteor/app/lib/server/functions/setUserActiveStatus.tsapps/meteor/app/lib/server/functions/unarchiveRoom.tsapps/meteor/app/lib/server/hooks/afterUserActions.tsapps/meteor/app/lib/server/index.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tspackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/lib/server/functions/unarchiveRoom.ts
- packages/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 (2)
**/*.{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/app/lib/server/index.tsapps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/meteor/app/lib/server/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.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/hooks/afterUserActions.spec.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
🧠 Learnings (28)
📚 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:
apps/meteor/app/lib/server/index.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/index.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.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/app/lib/server/index.tsapps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/meteor/app/lib/server/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.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/app/lib/server/index.tsapps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/meteor/app/lib/server/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.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/hooks/afterUserActions.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/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/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/hooks/afterUserActions.spec.tsapps/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/hooks/afterUserActions.spec.tsapps/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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/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 : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/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 : 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/hooks/afterUserActions.spec.tsapps/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/hooks/afterUserActions.spec.tsapps/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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.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/hooks/afterUserActions.spec.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.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/unit/app/lib/server/hooks/afterUserActions.spec.tsapps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/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: 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:
apps/meteor/app/lib/server/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/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: 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/hooks/afterUserActions.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/lib/server/hooks/afterUserActions.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:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.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: 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-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2026-03-11T16:46:50.707Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:50.707Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 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:
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
🔇 Additional comments (5)
apps/meteor/app/lib/server/hooks/afterUserActions.ts (1)
1-23: LGTM!The hook implementation is clean and follows the established patterns:
- Correctly uses
user._idwhich is guaranteed to be present per the callback contract (confirmed by context snippets showing callbacks receive fullIUserobjects)- Appropriate use of
voidfor fire-and-forget notifications- The pre-check pattern in
handleActivateUser(usinghasArchivedSubscriptionsInNonArchivedRoomsByUserIdbefore the heavier$mergepipeline) is a sensible optimization as discussed in prior reviewsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
1-16: LGTM!Clean refactoring that removes direct
Subscriptionsdependency from this file. The subscription archival logic is now delegated to theafterUserActionshooks via the callback system at lines 102-108, providing better separation of concerns.packages/model-typings/src/models/ISubscriptionsModel.ts (1)
41-42: LGTM!The interface changes are well-designed:
countUnarchivedByRoomIdAndNotUserIdcorrectly replaces the old method to exclude archived subscriptions from read-receipt calculationsunarchiveByRoomIdreturn type change toPromise<void>accurately reflects the$mergepipeline behavior (confirmed by implementation context)- New methods for archived subscription checks and userId-based operations align with the PR objectives
Also applies to: 245-248, 290-290
apps/meteor/app/lib/server/index.ts (1)
45-45: LGTM!The side-effect import correctly ensures the
afterUserActionshooks are registered during server initialization, following the established pattern of other method/hook imports in this file.apps/meteor/tests/unit/app/lib/server/hooks/afterUserActions.spec.ts (1)
1-88: LGTM!Excellent test coverage for the hook handlers:
- Clever use of
callbacks.add.callsFaketo capture the registered handlers for direct testing- All four key scenarios are covered (deactivate with/without modifications, activate with/without archived subscriptions)
- Properly validates both the method invocations and conditional notification behavior
- Clean isolation with proper stub resets
KevLehman
requested changes
Mar 12, 2026
Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
KevLehman
requested changes
Mar 16, 2026
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
Outdated
Show resolved
Hide resolved
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
Show resolved
Hide resolved
apps/meteor/tests/unit/app/lib/server/functions/setUserActiveStatus.spec.ts
Outdated
Show resolved
Hide resolved
- Refactor setUserActiveStatus.spec.ts to use sinon sandbox and expand test coverage for administration actions - Optimize aggregations in Subscriptions.ts by updating the match pipeline stage
abhinavkrin
commented
Mar 19, 2026
KevLehman
previously approved these changes
Mar 19, 2026
gabriellsh
requested changes
Mar 24, 2026
Member
gabriellsh
left a comment
gabriellsh
left a comment
There was a problem hiding this comment.
Nitpicks more than anything else.
Signed-off-by: Abhinav Kumar <[email protected]>
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
7 participants
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
Bug Fixes
New Features
Tests
Chores