feat: presence sync engine#40274
Open
ricardogarim wants to merge 12 commits into
Open
Conversation
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: 21fc0a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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:
WalkthroughThis pull request implements a claim-based unified presence engine: adds typed presence sources and previous-state fields, implements claim resolution (processPresence) with priority/expiration/previousState logic, refactors Presence to claim APIs with cron-driven expiration processing, updates models for atomic presence updates, rewires API/methods, adds tests, and updates build deps. ChangesUnified Presence Engine with Priority-Based Claims
Sequence DiagramsequenceDiagram
participant Client as Client/API
participant API as REST Endpoint<br>(users.setStatus)
participant Method as SetUserStatus<br>Method
participant Presence as Presence Service
participant Engine as Presence Engine<br>(processPresence)
participant Models as Users Model
participant Broadcast as Broadcast
Client->>API: setStatus(user, status, message)
activate API
API->>API: Validate status against allowed values
API->>API: Check invisibility policy
alt status + message provided
API->>Presence: setActiveState(userId, {statusDefault, statusSource: 'manual', statusText})
else only status provided
API->>Presence: clearActiveState(userId)
else only message provided
API->>API: setStatusText(user, message)
end
API->>Models: Calendar.cancelUpcomingStatusChanges(user._id)
deactivate API
Method->>Method: Validate statusText policy
activate Method
alt status=online + no statusText
Method->>Presence: clearActiveState(userId)
else other status
Method->>Presence: setActiveState(userId, {statusDefault, statusSource: 'manual', statusText?})
end
deactivate Method
activate Presence
Presence->>Models: Load user (with status fields)
Presence->>Models: Load user sessions
Presence->>Engine: processPresence(user, sessions, claimUpdate)
activate Engine
Engine->>Engine: Resolve claim priority (internal > manual > external)
Engine->>Engine: Check expiration & previousState
Engine->>Engine: Compute display status from claim/sessions
Engine-->>Presence: {values: {...}, clear?: [...]}
deactivate Engine
Presence->>Models: updatePresenceAndStatus(userId, values, clear)
Presence->>Broadcast: broadcast(updatedUser, oldStatus)
deactivate Presence
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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40274 +/- ##
===========================================
- Coverage 69.99% 69.58% -0.42%
===========================================
Files 3309 3318 +9
Lines 120956 122127 +1171
Branches 21683 21857 +174
===========================================
+ Hits 84666 84981 +315
- Misses 32990 33818 +828
- Partials 3300 3328 +28
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
feat/presence-sync-engine
branch
6 times, most recently
from
75e7504 to
319fbf9
Compare
ricardogarim
force-pushed
the
feat/presence-sync-engine
branch
4 times, most recently
from
98b5b4e to
c809370
Compare
ricardogarim
force-pushed
the
feat/presence-sync-engine
branch
3 times, most recently
from
7f841b5 to
d25d179
Compare
ricardogarim
force-pushed
the
feat/presence-sync-engine
branch
from
5a44f65 to
f15837d
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
the
type: feature
![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: 3
🧹 Nitpick comments (2)
ee/packages/presence/src/Presence.ts (1)
108-120: ⚡ Quick winReplace recursive
processExpiredStatuseswith an iterative loop to avoid stack growth.Each recursive call retains the previous async frame (JS has no tail-call optimisation for
asyncfunctions). For a large number of expired statuses — e.g. after a calendar import creates thousands of timed claims — the recursion depth grows asceil(count / batchSize), risking a stack overflow before all batches complete.♻️ Proposed fix
- private async processExpiredStatuses(batchSize = 100): Promise<void> { - const expiredUsers = await Users.findExpiredStatuses(batchSize).toArray(); - - if (expiredUsers.length === 0) { - return; - } - - await Promise.allSettled(expiredUsers.map(({ _id }) => this.endActiveState(_id))); - - if (expiredUsers.length === batchSize) { - return this.processExpiredStatuses(batchSize); - } - } + private async processExpiredStatuses(batchSize = 100): Promise<void> { + let processed: number; + do { + const expiredUsers = await Users.findExpiredStatuses(batchSize).toArray(); + processed = expiredUsers.length; + if (processed === 0) return; + await Promise.allSettled(expiredUsers.map(({ _id }) => this.endActiveState(_id))); + } while (processed === batchSize); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/presence/src/Presence.ts` around lines 108 - 120, The recursive processExpiredStatuses implementation should be converted to an iterative loop to avoid growing the async call stack: in Presence.processExpiredStatuses replace the self-call with a loop that repeatedly fetches Users.findExpiredStatuses(batchSize).toArray(), returns immediately if none, awaits Promise.allSettled over expiredUsers.map(({_id}) => this.endActiveState(_id)) for each batch, and breaks when the returned array length is less than batchSize; keep using the same batchSize parameter and the same calls to Users.findExpiredStatuses and this.endActiveState to preserve behavior.ee/packages/presence/src/lib/presenceEngine.ts (1)
4-7: ⚡ Quick win
newStatetype inClaimUpdateshould enforce required fields.
Pick<IUser, 'statusDefault' | 'statusSource' | ...>inherits optionality fromIUser, meaning bothstatusDefaultandstatusSourcecan beundefined. However, the priority resolution system (PRIORITY[newState.statusSource]) silently degrades toNO_PRIORITYwhenstatusSourceis absent, andstatusDefaultfalls back through two levels. Callers that omit these fields would create ambiguous claims with no compile-time error.Introduce a dedicated claim state type with required fields:
♻️ Proposed fix
+type ActiveClaimState = { + statusDefault: UserStatus; + statusSource: 'manual' | 'external' | 'internal'; + statusText?: string; + statusEmoji?: string; + statusExpiresAt?: Date; +}; + export type ClaimUpdate = - | { type: 'setActive'; newState: Pick<IUser, 'statusDefault' | 'statusSource' | 'statusText' | 'statusEmoji' | 'statusExpiresAt'> } + | { type: 'setActive'; newState: ActiveClaimState } | { type: 'endActive' } | { type: 'clearActive' };Update the
setActiveStateparameter type inPresence.tsto useActiveClaimStateas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/presence/src/lib/presenceEngine.ts` around lines 4 - 7, The ClaimUpdate variant with type 'setActive' currently uses Pick<IUser,...> for newState which inherits optional properties from IUser and allows statusDefault/statusSource to be undefined, leading to ambiguous priority resolution (PRIORITY[newState.statusSource]) at runtime; define a new ActiveClaimState type that makes statusDefault, statusSource, statusText, statusEmoji, and statusExpiresAt required (non-optional) and replace the Pick usage in ClaimUpdate with ActiveClaimState, then update any callers and the setActiveState parameter in Presence.ts to accept ActiveClaimState so callers get compile-time errors when required fields are omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/packages/presence/src/lib/presenceEngine.spec.ts`:
- Around line 150-166: The test "should discard lower priority claim when
previousState has higher priority" uses expect(result.values).toMatchObject({}),
which is vacuously true; update the assertion in presenceEngine.spec.ts to use
expect(result.values).toStrictEqual({}) to assert the values object is truly
empty, and make the same replacement for the other failing test case that also
uses toMatchObject({}) (the other occurrence near the other priority-rejection
test) so both assertions use toStrictEqual; locate the assertions in the tests
referencing processPresence and replace toMatchObject with toStrictEqual.
In `@packages/models/src/models/Users.ts`:
- Around line 1100-1102: The findExpiredStatuses method returns expired user IDs
without a deterministic order, causing nondeterministic batching when expired
documents exceed the limit; update the query options in findExpiredStatuses
(function name: findExpiredStatuses in Users.ts) to include an explicit sort
key—e.g., add sort: { statusExpiresAt: 1 } alongside projection and limit—so
results are consistently ordered by expiration time.
- Around line 1104-1107: The updatePresenceAndStatus method currently calls the
BaseRaw wrapper this.findOneAndUpdate which triggers setUpdatedAt; change it to
call this.col.findOneAndUpdate directly to avoid mutating _updatedAt on hot
presence writes. Keep the same query ({ _id: userId }), the same update document
({ $set: values, ...($unset && { $unset }) }) where $unset is built from clear,
and preserve the option { returnDocument: 'after' }; ensure no wrapper helpers
that call setUpdatedAt are used so presence updates won't change _updatedAt.
---
Nitpick comments:
In `@ee/packages/presence/src/lib/presenceEngine.ts`:
- Around line 4-7: The ClaimUpdate variant with type 'setActive' currently uses
Pick<IUser,...> for newState which inherits optional properties from IUser and
allows statusDefault/statusSource to be undefined, leading to ambiguous priority
resolution (PRIORITY[newState.statusSource]) at runtime; define a new
ActiveClaimState type that makes statusDefault, statusSource, statusText,
statusEmoji, and statusExpiresAt required (non-optional) and replace the Pick
usage in ClaimUpdate with ActiveClaimState, then update any callers and the
setActiveState parameter in Presence.ts to accept ActiveClaimState so callers
get compile-time errors when required fields are omitted.
In `@ee/packages/presence/src/Presence.ts`:
- Around line 108-120: The recursive processExpiredStatuses implementation
should be converted to an iterative loop to avoid growing the async call stack:
in Presence.processExpiredStatuses replace the self-call with a loop that
repeatedly fetches Users.findExpiredStatuses(batchSize).toArray(), returns
immediately if none, awaits Promise.allSettled over expiredUsers.map(({_id}) =>
this.endActiveState(_id)) for each batch, and breaks when the returned array
length is less than batchSize; keep using the same batchSize parameter and the
same calls to Users.findExpiredStatuses and this.endActiveState to preserve
behavior.
🪄 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: 8da52d69-b065-403f-92b8-e681295bbf6d
📒 Files selected for processing (18)
.changeset/full-results-share.mdapps/meteor/app/api/server/v1/users.tsapps/meteor/app/user-status/server/methods/setUserStatus.tsapps/meteor/tests/end-to-end/apps/update-status-text.tsee/apps/presence-service/Dockerfileee/apps/presence-service/package.jsonee/packages/presence/package.jsonee/packages/presence/src/Presence.spec.tsee/packages/presence/src/Presence.tsee/packages/presence/src/lib/presenceEngine.spec.tsee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/lib/processConnectionStatus.tsee/packages/presence/tests/lib/processConnectionStatus.test.tspackages/core-services/src/types/IPresence.tspackages/core-typings/src/IUser.tspackages/core-typings/src/UserStatus.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.ts
💤 Files with no reviewable changes (2)
- ee/packages/presence/src/lib/processConnectionStatus.ts
- ee/packages/presence/tests/lib/processConnectionStatus.test.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). (5)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build (coverage)
🧰 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/IUsersModel.tsapps/meteor/tests/end-to-end/apps/update-status-text.tspackages/core-typings/src/IUser.tsapps/meteor/app/user-status/server/methods/setUserStatus.tsapps/meteor/app/api/server/v1/users.tspackages/core-services/src/types/IPresence.tsee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.tspackages/models/src/models/Users.tsee/packages/presence/src/Presence.tspackages/core-typings/src/UserStatus.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:
ee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.ts
🧠 Learnings (13)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/full-results-share.md
📚 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/model-typings/src/models/IUsersModel.tsapps/meteor/tests/end-to-end/apps/update-status-text.tspackages/core-typings/src/IUser.tsapps/meteor/app/user-status/server/methods/setUserStatus.tsapps/meteor/app/api/server/v1/users.tspackages/core-services/src/types/IPresence.tsee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.tspackages/models/src/models/Users.tsee/packages/presence/src/Presence.tspackages/core-typings/src/UserStatus.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/model-typings/src/models/IUsersModel.tsapps/meteor/tests/end-to-end/apps/update-status-text.tspackages/core-typings/src/IUser.tsapps/meteor/app/user-status/server/methods/setUserStatus.tsapps/meteor/app/api/server/v1/users.tspackages/core-services/src/types/IPresence.tsee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.tspackages/models/src/models/Users.tsee/packages/presence/src/Presence.tspackages/core-typings/src/UserStatus.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/model-typings/src/models/IUsersModel.tsapps/meteor/tests/end-to-end/apps/update-status-text.tspackages/core-typings/src/IUser.tsapps/meteor/app/user-status/server/methods/setUserStatus.tsapps/meteor/app/api/server/v1/users.tspackages/core-services/src/types/IPresence.tsee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.tspackages/models/src/models/Users.tsee/packages/presence/src/Presence.tspackages/core-typings/src/UserStatus.ts
📚 Learning: 2026-02-04T12:08:56.950Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:08:56.950Z
Learning: In the E2E tests under apps/meteor/tests/end-to-end/apps, prefer using a fixed sleep (e.g., 1s) instead of implementing polling or retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
apps/meteor/tests/end-to-end/apps/update-status-text.ts
📚 Learning: 2026-03-16T11:50:11.087Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:11.087Z
Learning: In end-to-end tests under apps/meteor/tests/end-to-end/apps, there is an established pattern: call createAgent() and makeAgentAvailable() immediately after updateSetting('Livechat_enabled', true) with no intermediate delay. Do not flag this sequence as a race condition or require a sleep/delay. This pattern is used across 10+ tests in the codebase.
Applied to files:
apps/meteor/tests/end-to-end/apps/update-status-text.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-15T14:31:25.380Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:25.380Z
Learning: Do not flag this type/schema misalignment in the OpenAPI/migration review for apps/meteor/app/api/server/v1/users.ts. The UserCreateParamsPOST type intentionally uses non-optional fields: fields: string and settings?: IUserSettings without an AJV schema entry, carried over from the original rest-typings (PR `#39647`). Treat this as a known pre-existing divergence and document it as a separate follow-up fix; do not block or mark it as a review issue during the migration.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-16T23:33:11.443Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:11.443Z
Learning: In rockets OpenAPI/AJV migration reviews for RocketChat/Rocket.Chat, when reviewing migrations that involve apps/meteor/app/api/server/v1/users.ts, do not require or flag a missing query AJV schema for the fields consumed by parseJsonQuery() (i.e., fields, sort, query) as part of this endpoint's migration PR. The addition of global query-param schemas for parseJsonQuery() usage is a cross-cutting concern and out of scope for individual endpoint migrations. Only flag violations related to the specific scope of the migration, not the absence of a query schema for parseJsonQuery() in this file.
Applied to files:
apps/meteor/app/api/server/v1/users.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/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.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:
ee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.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:
ee/packages/presence/src/Presence.spec.tsee/packages/presence/src/lib/presenceEngine.spec.ts
🔇 Additional comments (12)
.changeset/full-results-share.md (1)
2-10: Changeset content looks good and aligned with the PR scope.Version entries and summary text are clear, and the described behavior matches the presence-engine foundation introduced in this PR.
packages/core-typings/src/UserStatus.ts (1)
9-9: Looks good —PresenceSourceis clear and appropriately scoped.packages/core-typings/src/IUser.ts (1)
190-199: Presence typing extension is consistent with the new engine contract.packages/model-typings/src/models/IUsersModel.ts (1)
175-177: Interface additions are coherent with the new expiration/update flow.packages/core-services/src/types/IPresence.ts (1)
18-25: API contract update is clear and migration-friendly.ee/packages/presence/package.json (1)
21-21: Dependency addition aligns with the new expiration scheduler.ee/apps/presence-service/package.json (1)
24-24: Presence service dependency graph update looks correct.ee/apps/presence-service/Dockerfile (1)
16-17: Docker build context update correctly includes the new cron package artifacts.apps/meteor/tests/end-to-end/apps/update-status-text.ts (1)
70-82: LGTM — test expectation correctly reflects the new engine behavior.The comment accurately clarifies the intent (test app defaults
statusTextto''when absent).apps/meteor/app/user-status/server/methods/setUserStatus.ts (1)
30-39: LGTM — logic mirrors the REST endpoint behavior consistently.apps/meteor/app/api/server/v1/users.ts (1)
1978-2007: LGTM — presence API migration and calendar cancellation are correct.ee/packages/presence/src/Presence.spec.ts (1)
1-241: LGTM — test coverage is comprehensive and assertions are correct.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/packages/presence/src/lib/presenceEngine.ts (1)
9-10: ⚡ Quick winInline implementation comments conflict with the project's "no code comments" rule.
Most of the bodies in this file (e.g., lines 9–10, 81, 84, 92, 109, 117, 136, 149, 168–169) carry narration-style comments that the coding guidelines explicitly discourage in implementation files. Consider letting the code carry the intent — for example, by extracting the priority-comparison branches into named helpers (
hasHigherPriority,hasSamePriority,canQueueAsPrevious) and naming the early-return paths (e.g.,OFFLINE_CLAIM_REJECTED) so the comments become unnecessary. The JSDoc on the two exported entry points (processPresence,ClaimUpdate) is fine to keep as API documentation.As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 81-81, 84-84, 92-92, 109-109, 117-117, 136-136, 149-149, 168-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/packages/presence/src/lib/presenceEngine.ts` around lines 9 - 10, Replace the narration-style inline comments in presenceEngine.ts by extracting intentful branches into small named helpers and named early-return reasons: implement functions like hasHigherPriority(existingClaim, incomingClaim), hasSamePriority(existingClaim, incomingClaim), and canQueueAsPrevious(existingClaim, incomingClaim) and replace comment-led condition blocks in processPresence and ClaimUpdate with calls to these helpers; also replace comment-based early returns with well-named constants/enum values (e.g., OFFLINE_CLAIM_REJECTED, DUPLICATE_CLAIM_IGNORED) to make the code self-documenting and remove the discouraged implementation comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ee/packages/presence/src/lib/presenceEngine.ts`:
- Around line 9-10: Replace the narration-style inline comments in
presenceEngine.ts by extracting intentful branches into small named helpers and
named early-return reasons: implement functions like
hasHigherPriority(existingClaim, incomingClaim), hasSamePriority(existingClaim,
incomingClaim), and canQueueAsPrevious(existingClaim, incomingClaim) and replace
comment-led condition blocks in processPresence and ClaimUpdate with calls to
these helpers; also replace comment-based early returns with well-named
constants/enum values (e.g., OFFLINE_CLAIM_REJECTED, DUPLICATE_CLAIM_IGNORED) to
make the code self-documenting and remove the discouraged implementation
comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46b31428-4b9f-4cb4-ac48-c84666bcc684
📒 Files selected for processing (6)
ee/packages/presence/src/Presence.spec.tsee/packages/presence/src/Presence.tsee/packages/presence/src/lib/presenceEngine.spec.tsee/packages/presence/src/lib/presenceEngine.tspackages/core-services/src/types/IPresence.tspackages/core-typings/src/IUser.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core-typings/src/IUser.ts
- packages/core-services/src/types/IPresence.ts
- ee/packages/presence/src/lib/presenceEngine.spec.ts
- ee/packages/presence/src/Presence.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:
ee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/presence/src/Presence.spec.ts
🧠 Learnings (6)
📚 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:
ee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
ee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
ee/packages/presence/src/lib/presenceEngine.tsee/packages/presence/src/Presence.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/presence/src/Presence.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:
ee/packages/presence/src/Presence.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:
ee/packages/presence/src/Presence.spec.ts
🔇 Additional comments (1)
ee/packages/presence/src/Presence.spec.ts (1)
41-236: LGTM — solid coverage of the new claim API.Tests are appropriately scoped: setActive happy/rejected/no-sessions/expiresAt paths, endActive restore, clearActive reset, and the deprecated
setStatusedge cases (ONLINE+no text, ONLINE+empty string, ONLINE+text, explicit''vsundefinedstatusText, missing user). Assertions onupdatePresenceMock(set payload + clear array) match the{ values, clear }contract fromprocessPresence.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 18 files
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="ee/packages/presence/src/Presence.ts">
<violation number="1" location="ee/packages/presence/src/Presence.ts:115">
P1: The recursive expiration loop can run indefinitely when updates fail, because failures are ignored but recursion continues whenever the batch is full.</violation>
</file>
<file name="ee/packages/presence/src/lib/presenceEngine.ts">
<violation number="1" location="ee/packages/presence/src/lib/presenceEngine.ts:171">
P1: When processing a claim update with no sessions, this sets `status` to `statusDefault` and can mark offline users as online (for example after claim expiration). No-session updates should still resolve display status with offline connection precedence.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Introduces a unified Presence Engine - a single
processPresence()function that decides the user's final display status by combining status claims (manual, calendar, voice) with DDP connection state. Manual status writers now go through this function instead of writing directly to the User document. Calendar and voice integrations will be migrated in follow-up PRs.Changes
presenceEngine.ts): replaces the previous split acrosspresenceEngine.ts,processConnectionStatus.ts, and merge logic inPresence.tswith a singleprocessPresence(user, sessions, claimUpdate?)functionstatusSource,statusExpiresAt,previousStatestatusExpiresAthas passed, and callsendActiveStateto restore their previous status or reset to system-managedusers.setStatusendpoint and the MeteorsetUserStatusmethod now usesetActiveState/clearActiveStatedirectly instead of writing to the DBNew public methods
These replace
Presence.setStatus()(now@deprecated):setActiveState(userId, newState)- apply a status claim with a source (manual, external, internal)endActiveState(userId)- end current claim, restorepreviousStateif valid, otherwise resetclearActiveState(userId)- full reset, remove all claimsPriority system
internalmanualexternalWhen a higher-priority source takes over, the current state is saved as
previousStateand restored when it ends.Write flow
flowchart LR subgraph Writers["Current writers"] A[Meteor method\nsetUserStatus] B[REST API\nusers.setStatus] end subgraph Future["Future writers"] C[Calendar\napplyStatusChange] D[Voice / Pexip] end A -->|"source: manual"| E B -->|"source: manual"| E C -.->|"source: external"| E D -.->|"source: internal"| E E["processPresence\nresolveIntent + connection reality"] E --> F["updateUserPresence\n(atomic findOneAndUpdate)"] F --> G[broadcast]Decision logic -
setActiveflowchart TD START(["setActive(user, newState)"]) --> OFFLINE{"User offline?"} OFFLINE -->|"Yes + non-manual"| REJECT["Reject claim"] OFFLINE -->|"No / manual"| COMPARE COMPARE{"Compare priorities"} COMPARE -->|"Higher priority"| SAVE["Save current as previousState\nApply new"] COMPARE -->|"Same priority"| OVERWRITE["Overwrite active\nKeep previousState"] COMPARE -->|"Lower priority"| SLOT{"previousState\nslot available?"} SLOT -->|"Empty or expired"| QUEUE["Save as previousState\n(not visible)"] SLOT -->|"Occupied + higher"| DISCARD["Discard silently"] SLOT -->|"Occupied + same/lower"| REPLACE["Replace previousState"]endActive: restorespreviousStateif it exists and isn't expired, otherwise resets to Online and clears all claim fields.clearActive: always resets to Online and clears all claim fields.Breaking changes
None.
updateUserPresencewas removed from theIPresenceinterface andprocessConnectionStatus.tswas deleted, but neither had external callers.Issue(s)
Steps to test or reproduce
See the use case matrix for the full set of scenarios.
Further comments
Pending for follow-up PRs:
applyStatusChangefrom direct DB writes tosetActiveState/endActiveStatesetActiveState/endActiveStatewithstatusSource: 'internal'statusSource,statusExpiresAtto the event payload when the UI is ready to consume themusers.setStatusto acceptexpiresAtparam, updateusers.getStatusto return new fieldssetStatustosetActiveStatewhenIUserBridgesupports the new fieldsstatusSource,statusExpiresAt) in sidebar, avatar, and profile viewsstatusTextwithout re-asserting a claim (currently handled by standalonesetStatusText)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores