Skip to content

Conversation

@ricardogarim
Copy link
Contributor

@ricardogarim ricardogarim commented Dec 22, 2025

As per SUP-942, this PR fixes an issue where guest room limits were incorrectly counting DMs. Since the counter was based solely on total subscriptions by userId, guests could hit the maximum rooms limit even when only participating in direct messages.

Proposed changes (including videos or screenshots)

Updated the roomsPerGuest counter to exclude DM subscriptions, ensuring the limit applies only to non-DM rooms, in line with the documented business rule.

Issue(s)

Summary by CodeRabbit

Bug Fixes

  • Guest room limits now exclude Direct Messages from the count, ensuring limits apply only to non-DM rooms.
  • Improved guest room-join validation to check if adding a new room would exceed the configured limit before allowing access.

✏️ Tip: You can customize this high-level summary in your review settings.

@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2025

🦋 Changeset detected

Latest commit: a71c72a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages
Name Type
@rocket.chat/model-typings Patch
@rocket.chat/models Patch
@rocket.chat/meteor Patch
@rocket.chat/apps Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/omnichannel-services Patch
rocketchat-services Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/ui-client Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/media-calls Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/presence Patch
@rocket.chat/network-broker Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Walkthrough

This PR modifies the guest room-per-limit feature to exclude Direct Messages from subscription counts. Changes include introducing a new subscription counter API that filters by type, updating license counter logic to exclude DMs, adjusting room-join validation to check potential overflow, and adding test coverage.

Changes

Cohort / File(s) Summary
Subscription Model API
packages/model-typings/src/models/ISubscriptionsModel.ts, packages/models/src/models/Subscriptions.ts
Replaces countByUserId() method with countByUserIdExceptType(userId, typeException) to enable filtering out a specific subscription type from the count.
License Counter Logic
apps/meteor/ee/app/license/server/startup.ts
Updates the roomsPerGuest license counter to use the new countByUserIdExceptType() method, excluding type 'd' (Direct Messages) from the subscription count.
Room Join Validation
apps/meteor/ee/server/startup/maxRoomsPerGuest.ts
Changes extraCount parameter from 0 to 1 in shouldPreventAction() call, validating potential overflow before adding a user to a room.
Test Coverage
ee/packages/license/src/license.spec.ts
Adds test case validating per-user context behavior for the roomsPerGuest limit.
Changelog
.changeset/strange-ants-impress.md
Documents patch releases and behavioral change: roomsPerGuest now excludes Direct Messages when counting subscriptions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20-25 minutes

  • API signature change: Verify all usages of countByUserId() have been migrated to countByUserIdExceptType()
  • Multi-file logic coordination: Ensure the counter logic, license check, and room-join guard work together correctly with the DM exclusion
  • Type filtering correctness: Confirm that excluding type 'd' (Direct Messages) aligns with the intended behavior across all modified files

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo
  • sampaiodiego
  • tassoevan

Poem

🐰 Direct messages hop away,
Guest room counts just had their day,
No more 'd' types in the tally,
Limits now work properly, rally!
One more room check, clean and tight!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: guest room limit incorrectly counting DMs' directly describes the main change—excluding DMs from the guest room limit counter.
Linked Issues check ✅ Passed The PR implements the core requirement from SUP-942 by modifying the roomsPerGuest counter to exclude DM subscriptions from the guest limit calculation.
Out of Scope Changes check ✅ Passed All changes align with the stated objective: method signature updates, counter logic modifications, and test coverage directly support excluding DMs from guest room limits.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/max-guests

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a587ab3 and 16f9b4f.

📒 Files selected for processing (6)
  • .changeset/strange-ants-impress.md
  • apps/meteor/ee/app/license/server/startup.ts
  • apps/meteor/ee/server/startup/maxRoomsPerGuest.ts
  • ee/packages/license/src/license.spec.ts
  • packages/model-typings/src/models/ISubscriptionsModel.ts
  • packages/models/src/models/Subscriptions.ts
🧰 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:

  • packages/model-typings/src/models/ISubscriptionsModel.ts
  • ee/packages/license/src/license.spec.ts
  • apps/meteor/ee/app/license/server/startup.ts
  • apps/meteor/ee/server/startup/maxRoomsPerGuest.ts
  • packages/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.ts extension for test files (e.g., login.spec.ts)

Files:

  • ee/packages/license/src/license.spec.ts
🧠 Learnings (10)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.

Applied to files:

  • packages/model-typings/src/models/ISubscriptionsModel.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/model-typings/src/models/ISubscriptionsModel.ts
  • apps/meteor/ee/app/license/server/startup.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/model-typings/src/models/ISubscriptionsModel.ts
  • apps/meteor/ee/app/license/server/startup.ts
  • apps/meteor/ee/server/startup/maxRoomsPerGuest.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:

  • ee/packages/license/src/license.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:

  • ee/packages/license/src/license.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.

Applied to files:

  • ee/packages/license/src/license.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:

  • apps/meteor/ee/app/license/server/startup.ts
  • apps/meteor/ee/server/startup/maxRoomsPerGuest.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/ee/server/startup/maxRoomsPerGuest.ts
  • .changeset/strange-ants-impress.md
📚 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/ee/server/startup/maxRoomsPerGuest.ts
  • .changeset/strange-ants-impress.md
📚 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/ee/server/startup/maxRoomsPerGuest.ts
🧬 Code graph analysis (4)
packages/model-typings/src/models/ISubscriptionsModel.ts (1)
packages/core-typings/src/ISubscription.ts (1)
  • ISubscription (11-81)
apps/meteor/ee/app/license/server/startup.ts (1)
packages/core-services/src/index.ts (1)
  • License (155-155)
apps/meteor/ee/server/startup/maxRoomsPerGuest.ts (1)
packages/core-services/src/index.ts (1)
  • License (155-155)
packages/models/src/models/Subscriptions.ts (1)
packages/core-typings/src/ISubscription.ts (1)
  • ISubscription (11-81)
🔇 Additional comments (6)
.changeset/strange-ants-impress.md (1)

1-7: LGTM! Clear changeset documentation.

The changeset accurately describes the behavioral change where roomsPerGuest now excludes Direct Messages when counting subscriptions.

apps/meteor/ee/app/license/server/startup.ts (1)

110-112: LGTM! Correctly excludes DMs from guest room counting.

The counter now uses countByUserIdExceptType with type 'd' (Direct Messages) as the exception, ensuring DMs don't count toward the guest room limit. The fallback to 0 when no userId is present is appropriate.

apps/meteor/ee/server/startup/maxRoomsPerGuest.ts (1)

11-13: LGTM! Correct use of extraCount for prospective validation.

The change to extraCount = 1 properly checks whether adding one more room would exceed the limit, since this hook runs before the user is added to the room. The comment clearly explains this semantic distinction.

ee/packages/license/src/license.spec.ts (1)

275-299: LGTM! Comprehensive test for per-user context.

This test validates the per-user context behavior for roomsPerGuest limits, correctly verifying that:

  • user1 with 2 rooms can add 1 more (2+1=3, at limit) → returns false
  • user2 with 3 rooms cannot add 1 more (3+1=4, exceeds limit) → returns true

The test aligns with the actual usage pattern in maxRoomsPerGuest.ts where extraCount=1 is used.

packages/model-typings/src/models/ISubscriptionsModel.ts (1)

332-332: LGTM! Interface updated to support type exclusion.

The method signature change from countByUserId to countByUserIdExceptType correctly adds the typeException parameter, enabling selective counting by subscription type. The parameter type ISubscription['t'] appropriately references the subscription type field.

packages/models/src/models/Subscriptions.ts (1)

1175-1182: LGTM! Implementation correctly excludes subscription type.

The method implementation properly filters subscriptions by userId while excluding those with the specified type using MongoDB's $ne operator. This enables the license counter to exclude DMs (type 'd') from the roomsPerGuest count.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Dec 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.63%. Comparing base (f29a04e) to head (a71c72a).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37919      +/-   ##
===========================================
- Coverage    70.63%   70.63%   -0.01%     
===========================================
  Files         3143     3143              
  Lines       108693   108693              
  Branches     19583    19592       +9     
===========================================
- Hits         76780    76770      -10     
- Misses       29905    29914       +9     
- Partials      2008     2009       +1     
Flag Coverage Δ
e2e 60.13% <ø> (-0.03%) ⬇️
e2e-api 47.41% <ø> (-0.05%) ⬇️
unit 71.77% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.1GiB 1.1GiB +11MiB
rocketchat 355MiB 345MiB +11MiB
omnichannel-transcript-service 132MiB 132MiB -687B
queue-worker-service 132MiB 132MiB +891B
ddp-streamer-service 126MiB 126MiB +173B
account-service 113MiB 113MiB +769B
authorization-service 111MiB 111MiB -1.6KiB
presence-service 111MiB 111MiB +640B

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 11:55 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Loading

Statistics (last 30 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.1GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37919
  • Baseline: develop
  • Timestamp: 2025-12-26 11:55:16 UTC
  • Historical data points: 30

Updated: Fri, 26 Dec 2025 11:55:17 GMT

@ricardogarim ricardogarim added this to the 8.1.0 milestone Dec 22, 2025
@ricardogarim ricardogarim marked this pull request as ready for review December 22, 2025 20:09
@ricardogarim ricardogarim requested a review from a team as a code owner December 22, 2025 20:09
@ricardogarim ricardogarim added the stat: QA assured Means it has been tested and approved by a company insider label Dec 24, 2025
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Dec 24, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 24, 2025
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Dec 24, 2025
@scuciatto scuciatto modified the milestone: 8.1.0 Dec 26, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 26, 2025
@kodiakhq kodiakhq bot merged commit 5fa1509 into develop Dec 26, 2025
43 checks passed
@kodiakhq kodiakhq bot deleted the fix/max-guests branch December 26, 2025 12:16
gaolin1 pushed a commit to gaolin1/medsense.webchat that referenced this pull request Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants