feat: add externalIds support for visitor identification#39535
Open
ricardogarim wants to merge 41 commits intodevelopfrom
Open
feat: add externalIds support for visitor identification#39535ricardogarim wants to merge 41 commits intodevelopfrom
ricardogarim wants to merge 41 commits intodevelopfrom
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: 78df16b 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:
WalkthroughAdds external visitor identifier support (IVisitorExternalIdentifier and externalIds) across types, models, bridges, utilities, converters, registration, UI, and tests; implements resolveVisitor lookup (externalId first, phone/email fallback) with progressive enrichment of visitor.externalIds and permission-guarded bridge/API surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant App as External App
participant Bridge as LivechatBridge
participant Resolver as resolveVisitor
participant DB as LivechatVisitors
App->>Bridge: doResolveVisitor(externalId, contactData?)
Bridge->>Bridge: verify livechat-visitor permission
Bridge->>Resolver: resolveVisitor({ source, externalId, contactData })
Resolver->>DB: findOneByExternalId(source, externalId.userId)
alt found by external ID
DB-->>Resolver: visitor
Resolver-->>Bridge: return visitor
else not found & contactData provided
Resolver->>DB: findOneVisitorByPhoneOrEmailAndAddExternalId(contactData, source, externalId)
alt found by contact
DB-->>Resolver: visitor (updated with externalId)
Resolver-->>Bridge: return enriched visitor
else no match
Resolver-->>Bridge: return null
end
else not found & no contactData
Resolver-->>Bridge: return null
end
Bridge-->>App: IVisitor | undefined
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
the
type: feature
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39535 +/- ##
===========================================
- Coverage 70.55% 70.22% -0.33%
===========================================
Files 3271 3280 +9
Lines 116782 116819 +37
Branches 21090 20735 -355
===========================================
- Hits 82393 82039 -354
+ Misses 32338 31502 -836
- Partials 2051 3278 +1227
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/omni-core/src/visitor/create.ts (1)
28-33:⚠️ Potential issue | 🟠 MajorHandle
externalIdsadditively to avoid erasing previously linked identifiers.Line 33 spreads incoming
externalIdsdirectly into the update object passed toupdateOneByIdOrToken(). At line 327 of LivechatVisitors.ts, this method uses{ $set: update }, which fully replaces the field instead of merging. On subsequent visitor registrations from another channel, any existing external IDs will be lost.The codebase already provides
addExternalId()(LivechatVisitors.ts:62–70) which uses$setUnionfor additive writes, and resolveVisitor.ts demonstrates the correct pattern at lines 18–19. Apply the same approach here: either calladdExternalId()separately after confirming the visitor match, or merge against the stored array before the upsert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/omni-core/src/visitor/create.ts` around lines 28 - 33, The current update builds visitorDataToUpdate and spreads externalIds directly which causes updateOneByIdOrToken (which uses {$set: update}) to overwrite the stored externalIds; instead handle externalIds additively by either calling LivechatVisitors.addExternalId(visitorIdOrToken, externalId) for each new externalId after creating/updating the visitor, or merge with the existing visitor.externalIds before calling updateOneByIdOrToken (e.g., compute a union of existing and incoming externalIds and set that union into visitorDataToUpdate); locate and modify the logic around visitorDataToUpdate and the subsequent call to updateOneByIdOrToken to use addExternalId or a union merge to avoid erasing previously linked identifiers.
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/lib/resolveVisitor.ts (1)
18-18: Remove the inline implementation comment.The helper name and
addExternalId()call already explain the enrichment step.As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/livechat/server/lib/resolveVisitor.ts` at line 18, Remove the inline implementation comment "// Enrich existing visitor with external ID (progressive enrichment)" that sits above the call to addExternalId() in resolveVisitor.ts — the helper name and addExternalId() already convey the intent; simply delete the comment (and tidy surrounding whitespace) so the code follows the "avoid code comments in the implementation" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/apps/server/bridges/livechat.ts`:
- Line 239: The deprecated createVisitor() path builds registerData without
copying visitor.externalIds so visitors created there cannot be resolved by
external ID; update createVisitor() to mirror externalIds into the registerData
object (same shape used by createAndReturnVisitor()) or explicitly reject/ignore
externalIds in that deprecated API. Locate createVisitor(), modify the
registerData construction to include ...(visitor.externalIds?.length && {
externalIds: visitor.externalIds }) so externalIds are passed through (or add
validation that throws when externalIds are provided) to ensure consistency with
createAndReturnVisitor().
In `@apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx`:
- Around line 40-46: The shortName is currently built from visitor.username
(from data.visitor.username) which for WhatsApp is the internal livechat
username; change shortName (and the similar occurrence around line 53) to prefer
the external-id username from visitor.externalIds (find externalIds[].username)
when present, falling back to visitor.username or phoneNumber as before; update
the logic that computes shortName in ContactField to mirror
ContactInfoChannelsItem by checking visitor.externalIds for a username first,
then using visitor.username or phone as fallback.
In `@packages/apps-engine/src/server/bridges/LivechatBridge.ts`:
- Around line 98-102: doResolveVisitor currently calls resolveVisitor when the
caller has only livechat-visitor.read, but resolveVisitor can call
LivechatVisitors.addExternalId (a mutating enrichment); change doResolveVisitor
to prevent mutation by either (A) gating the enrichment path behind
livechat-visitor.write—i.e., only call the code path that may call
LivechatVisitors.addExternalId if hasWritePermission('livechat-visitor') is
true—or (B) split the behavior into a pure lookup method and a separate
enrichment method so doResolveVisitor uses the pure lookup when only
hasReadPermission and only invokes the enrichment method (which calls
resolveVisitor/addExternalId) when hasWritePermission; update checks around
doResolveVisitor, resolveVisitor and references to
LivechatVisitors.addExternalId accordingly.
In `@packages/models/src/models/LivechatVisitors.ts`:
- Around line 53-69: The current findOneByExternalId and addExternalId treat
array element matches and deduplication incorrectly; change findOneByExternalId
to query using an $elemMatch on externalIds (e.g. { externalIds: { $elemMatch: {
source: source, userId: externalUserId } } }) so both fields are matched on the
same array element, and change addExternalId to an update pipeline that
deduplicates externalIds by the (source,userId) key rather than by full object
equality—compute a key (e.g. concat source + '|' + userId) for each array
element (including the incoming externalId), use $reduce/$filter to build an
array that keeps only the first element per key (or merges fields if desired),
and $set externalIds to that deduplicated array so duplicates differing only by
extra fields (like username) are collapsed by source+userId.
---
Outside diff comments:
In `@packages/omni-core/src/visitor/create.ts`:
- Around line 28-33: The current update builds visitorDataToUpdate and spreads
externalIds directly which causes updateOneByIdOrToken (which uses {$set:
update}) to overwrite the stored externalIds; instead handle externalIds
additively by either calling LivechatVisitors.addExternalId(visitorIdOrToken,
externalId) for each new externalId after creating/updating the visitor, or
merge with the existing visitor.externalIds before calling updateOneByIdOrToken
(e.g., compute a union of existing and incoming externalIds and set that union
into visitorDataToUpdate); locate and modify the logic around
visitorDataToUpdate and the subsequent call to updateOneByIdOrToken to use
addExternalId or a union merge to avoid erasing previously linked identifiers.
---
Nitpick comments:
In `@apps/meteor/app/livechat/server/lib/resolveVisitor.ts`:
- Line 18: Remove the inline implementation comment "// Enrich existing visitor
with external ID (progressive enrichment)" that sits above the call to
addExternalId() in resolveVisitor.ts — the helper name and addExternalId()
already convey the intent; simply delete the comment (and tidy surrounding
whitespace) so the code follows the "avoid code comments in the implementation"
guideline.
🪄 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: b07474a9-2220-4d0a-aa27-524a46dcc03c
📒 Files selected for processing (16)
apps/meteor/app/apps/server/bridges/livechat.tsapps/meteor/app/apps/server/converters/visitors.jsapps/meteor/app/livechat/server/lib/resolveVisitor.tsapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxapps/meteor/client/views/omnichannel/directory/components/ContactField.tsxapps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.tspackages/apps-engine/src/definition/accessors/ILivechatCreator.tspackages/apps-engine/src/definition/livechat/IVisitor.tspackages/apps-engine/src/definition/livechat/index.tspackages/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.tspackages/models/src/models/LivechatVisitors.tspackages/omni-core/src/visitor/create.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). (1)
- GitHub Check: 📦 Build Packages
🧰 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/apps-engine/src/server/accessors/LivechatCreator.tsapps/meteor/app/apps/server/converters/visitors.jsapps/meteor/client/views/omnichannel/directory/components/ContactField.tsxapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxpackages/apps-engine/src/definition/accessors/ILivechatCreator.tsapps/meteor/app/livechat/server/lib/resolveVisitor.tspackages/core-typings/src/ILivechatVisitor.tspackages/omni-core/src/visitor/create.tspackages/model-typings/src/models/ILivechatVisitorsModel.tspackages/apps-engine/src/definition/livechat/IVisitor.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/models/src/models/LivechatVisitors.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tspackages/apps-engine/src/definition/livechat/index.tsapps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.tsapps/meteor/app/apps/server/bridges/livechat.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/livechat/server/lib/resolveVisitor.spec.ts
🧠 Learnings (21)
📓 Common learnings
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/definition/accessors/ILivechatCreator.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.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/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/definition/accessors/ILivechatCreator.tspackages/apps-engine/src/server/bridges/LivechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.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/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/server/bridges/LivechatBridge.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/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/definition/accessors/ILivechatCreator.tsapps/meteor/app/livechat/server/lib/resolveVisitor.tspackages/core-typings/src/ILivechatVisitor.tspackages/omni-core/src/visitor/create.tspackages/model-typings/src/models/ILivechatVisitorsModel.tspackages/apps-engine/src/definition/livechat/IVisitor.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/models/src/models/LivechatVisitors.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tspackages/apps-engine/src/definition/livechat/index.tsapps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.tsapps/meteor/app/apps/server/bridges/livechat.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/apps-engine/src/server/accessors/LivechatCreator.tspackages/apps-engine/src/definition/accessors/ILivechatCreator.tsapps/meteor/app/livechat/server/lib/resolveVisitor.tspackages/core-typings/src/ILivechatVisitor.tspackages/omni-core/src/visitor/create.tspackages/model-typings/src/models/ILivechatVisitorsModel.tspackages/apps-engine/src/definition/livechat/IVisitor.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/models/src/models/LivechatVisitors.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tspackages/apps-engine/src/definition/livechat/index.tsapps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.tsapps/meteor/app/apps/server/bridges/livechat.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
apps/meteor/client/views/omnichannel/directory/components/ContactField.tsxapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxapps/meteor/app/livechat/server/lib/resolveVisitor.tspackages/apps-engine/src/server/bridges/LivechatBridge.ts
📚 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:
packages/omni-core/src/visitor/create.tspackages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.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/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/unit/app/livechat/server/lib/resolveVisitor.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/unit/app/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.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/livechat/server/lib/resolveVisitor.spec.ts
🔇 Additional comments (9)
apps/meteor/app/apps/server/converters/visitors.js (2)
40-40: LGTM!The mapping addition follows the established pattern used for other visitor fields.
61-61: LGTM!The conditional spread follows the same pattern used for
visitorEmailsanddepartment, maintaining consistency in how optional fields are handled.apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx (1)
33-46: Nice label fallback for channel rows.Preferring
externalIds[].usernamehere keeps the contact-channel view aligned with the new visitor model while still falling back cleanly to the existing source label.Also applies to: 113-113
packages/core-typings/src/ILivechatVisitor.ts (1)
17-21: Type shape matches the new persistence contract.
IVisitorExternalIdentifierplusexternalIdsonILivechatVisitorlines up with thesource/userIdlookup model and gives the UI a typed place to read the external username from.Also applies to: 35-35
packages/apps-engine/src/definition/livechat/index.ts (1)
19-24: Good barrel export update.Re-exporting
IVisitorExternalIdentifierhere keeps the apps-engine public surface consistent with the new visitor contract.packages/apps-engine/src/definition/livechat/IVisitor.ts (1)
4-8: Apps-engine visitor typing stays aligned with core.Adding
IVisitorExternalIdentifierandexternalIdshere gives apps a typed way to participate in the new resolve/enrichment flow without extra translation.Also applies to: 23-23
packages/apps-engine/src/server/accessors/LivechatCreator.ts (1)
6-18: Thin delegation is the right shape here.
resolveVisitor()forwards the external id, optional phone, and app context without adding local policy, which keeps this accessor consistent with the rest of the bridge-backed API.packages/apps-engine/tests/test-data/bridges/livechatBridge.ts (1)
2-2: Test bridge stays in sync with the abstract surface.Adding the
resolveVisitor()stub here avoids test-only bridge drift now that the production bridge exposes the new resolve path.Also applies to: 65-67
packages/omni-core/src/visitor/create.ts (1)
1-20: Good entry-point plumbing forexternalIds.Accepting
externalIdsinRegisterGuestTypeand carrying it into the registration flow is the right place to hook the new visitor identifier data in.
apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx
Outdated
Show resolved
Hide resolved
d-gubert
reviewed
Mar 11, 2026
d-gubert
reviewed
Mar 11, 2026
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps-engine/src/server/bridges/LivechatBridge.ts (1)
204-205: Consider adding JSDoc to clarify the write permission requirement.A brief JSDoc comment would help future maintainers understand why this method requires write permission (due to potential enrichment via
addExternalId), similar to the deprecation notes on other methods.📝 Suggested documentation
+ /** + * Resolves a visitor by external identifier, falling back to phone lookup. + * Requires write permission as it may enrich the visitor record with the external ID. + */ protected abstract resolveVisitor(externalId: IVisitorExternalIdentifier, phone: string | undefined, appId: string): Promise<IVisitor | undefined>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/bridges/LivechatBridge.ts` around lines 204 - 205, Add a brief JSDoc comment above the abstract method resolveVisitor(externalId: IVisitorExternalIdentifier, phone: string | undefined, appId: string): Promise<IVisitor | undefined> explaining that the method requires write permission because implementations may call addExternalId to enrich or persist visitor data; note the intent and any deprecation/permission guidance similar to other methods so future maintainers understand why write access is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/apps-engine/src/server/bridges/LivechatBridge.ts`:
- Around line 204-205: Add a brief JSDoc comment above the abstract method
resolveVisitor(externalId: IVisitorExternalIdentifier, phone: string |
undefined, appId: string): Promise<IVisitor | undefined> explaining that the
method requires write permission because implementations may call addExternalId
to enrich or persist visitor data; note the intent and any
deprecation/permission guidance similar to other methods so future maintainers
understand why write access is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be917816-6ac6-46b5-8e06-bdac3e2145e2
📒 Files selected for processing (11)
apps/meteor/app/apps/server/bridges/livechat.tsapps/meteor/app/livechat/server/lib/resolveVisitor.tsapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxapps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.tspackages/apps-engine/src/definition/livechat/IVisitor.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.tspackages/models/src/models/LivechatVisitors.tspackages/omni-core/src/visitor/create.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.ts
- apps/meteor/app/livechat/server/lib/resolveVisitor.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). (2)
- GitHub Check: 📦 Build Packages
- 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/apps-engine/src/definition/livechat/IVisitor.tsapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxpackages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.ts
🧠 Learnings (23)
📓 Common learnings
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.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx:40-46
Timestamp: 2026-03-11T15:56:13.132Z
Learning: In `apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx`, `shortName` is intentionally built from the visitor's internal `username` and `phone` (not from `externalIds`). `ContactField` shows the visitor's internal system identifier. Per-channel external identifiers (e.g. WhatsApp usernames from `externalIds[].username`) are only displayed in `ContactInfoChannelsItem`, which serves a different UI context.
📚 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:
packages/apps-engine/src/definition/livechat/IVisitor.tsapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxpackages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.ts
📚 Learning: 2026-03-11T15:56:13.132Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx:40-46
Timestamp: 2026-03-11T15:56:13.132Z
Learning: In `apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx`, `shortName` is intentionally built from the visitor's internal `username` and `phone` (not from `externalIds`). `ContactField` shows the visitor's internal system identifier. Per-channel external identifiers (e.g. WhatsApp usernames from `externalIds[].username`) are only displayed in `ContactInfoChannelsItem`, which serves a different UI context.
Applied to files:
packages/apps-engine/src/definition/livechat/IVisitor.tsapps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxpackages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.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/apps-engine/src/definition/livechat/IVisitor.tspackages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.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/apps-engine/src/definition/livechat/IVisitor.tspackages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/apps-engine/tests/test-data/bridges/livechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.tspackages/core-typings/src/ILivechatVisitor.tspackages/model-typings/src/models/ILivechatVisitorsModel.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsxpackages/apps-engine/src/server/bridges/LivechatBridge.ts
📚 Learning: 2026-03-11T22:04:16.136Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:16.136Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
packages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.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/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.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:
packages/models/src/models/LivechatVisitors.ts
📚 Learning: 2026-03-09T21:20:12.687Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.
Applied to files:
packages/models/src/models/LivechatVisitors.ts
📚 Learning: 2026-02-23T17:53:18.785Z
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
packages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tspackages/core-typings/src/ILivechatVisitor.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:
packages/models/src/models/LivechatVisitors.tspackages/apps-engine/src/server/bridges/LivechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
packages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/core-typings/src/ILivechatVisitor.ts
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
packages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.ts
📚 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:
packages/models/src/models/LivechatVisitors.tsapps/meteor/app/apps/server/bridges/livechat.tspackages/omni-core/src/visitor/create.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/apps-engine/src/server/bridges/LivechatBridge.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/apps-engine/src/server/bridges/LivechatBridge.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/apps-engine/src/server/bridges/LivechatBridge.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/apps-engine/tests/test-data/bridges/livechatBridge.tsapps/meteor/app/apps/server/bridges/livechat.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/app/apps/server/bridges/livechat.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/apps/server/bridges/livechat.tspackages/core-typings/src/ILivechatVisitor.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:
packages/core-typings/src/ILivechatVisitor.ts
🔇 Additional comments (14)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx (3)
13-13: LGTM!The
useVisitorInfohook is appropriately imported and used to fetch visitor data for accessingexternalIds. React Query's caching will handle repeated renders efficiently.Also applies to: 33-34
35-44: LGTM!The
channelLabelcomputation correctly:
- Safely looks up the external identifier using
details.idas the key into theexternalIdsRecord- Handles all optional values with proper chaining (
details?.id,visitorData?.externalIds?.[...],externalId?.username)- Combines username and phone when both exist, or falls back to whichever is available
- Has accurate dependency array for
useMemoThis aligns with the retrieved learning that per-channel external identifiers (e.g., WhatsApp usernames) are displayed specifically in
ContactInfoChannelsItem.
111-111: LGTM!Rendering
channelLabelcorrectly displays the combined username and phone (or fallback) in the channel item.packages/apps-engine/src/definition/livechat/IVisitor.ts (1)
4-7: LGTM!The
IVisitorExternalIdentifierinterface is well-defined withuserIdas the required identifier and optionalusername. TheRecord<string, IVisitorExternalIdentifier>structure forexternalIdsis consistent with the core-typings definition and enables keying by source (e.g., appId).Also applies to: 22-22
packages/core-typings/src/ILivechatVisitor.ts (1)
17-20: LGTM!The
IVisitorExternalIdentifierandexternalIdsproperty are correctly defined, matching the apps-engine definition. TheRecord<string, IVisitorExternalIdentifier>structure enforces one external identifier per source, which aligns with the design decision from PR discussions.Also applies to: 34-34
packages/apps-engine/tests/test-data/bridges/livechatBridge.ts (1)
72-74: LGTM!The
resolveVisitorstub follows the established pattern of other methods in the test bridge, correctly implementing the abstract method signature.packages/model-typings/src/models/ILivechatVisitorsModel.ts (1)
53-59: LGTM!The new method signatures are well-defined and align with the model implementation. Return types correctly use
Promise<ILivechatVisitor | null>.apps/meteor/app/apps/server/bridges/livechat.ts (2)
350-364: LGTM!The
resolveVisitorimplementation correctly:
- Logs the operation with
appId- Delegates to the utility function with
appIdas thesource- Converts the result using the established visitor converter pattern
The pattern of passing potentially-null results to
convertVisitoris consistent with other finder methods in this file (e.g.,findVisitorByEmail,findVisitorByToken).
208-211: LGTM!The deprecation notice on
createVisitor()correctly documents that it does not supportexternalIds, guiding callers to usecreateAndReturnVisitor()which properly propagatesexternalIdsat line 249.Also applies to: 249-249
packages/omni-core/src/visitor/create.ts (1)
14-14: LGTM!The
externalIdsparameter flows correctly through the registration process. The spread pattern safely includesexternalIdsonly when provided, and the upsert behavior is appropriate for the visitor creation flow.Also applies to: 19-19, 33-33
packages/apps-engine/src/server/bridges/LivechatBridge.ts (1)
98-102: LGTM!The
doResolveVisitormethod correctly gates behindlivechat-visitor.writepermission, which is appropriate since the underlying resolution can enrich visitor records viaaddExternalId. This was addressed in prior review discussions.packages/models/src/models/LivechatVisitors.ts (3)
34-34: LGTM!The sparse wildcard index on
externalIds.$**correctly covers queries on dynamic nested paths likeexternalIds.${source}.userId. The sparse option ensures visitors withoutexternalIdsdon't bloat the index.
53-69: LGTM!Both methods are correctly implemented:
findOneVisitorByPhoneAndAddExternalId: Atomically finds by phone and sets the external identifier at the source-keyed path, returning the updated document.findOneByExternalId: Queries using the dynamic nested path which is covered by the wildcard index.The
Record<string, IVisitorExternalIdentifier>structure ensures one external ID per source, avoiding the deduplication issues from the previous array-based design.
53-63: External ID overwriting on phone-based lookup is intentional design.The test case "should update existing externalIds when visitor already has some" explicitly validates that unconditional overwriting of
externalIds.${source}during phone-based fallback is expected behavior. This allows the system to handle phone number reassignment across different external user accounts. No guard is needed here—the design trusts that a new external ID provided for an existing phone indicates a legitimate reassignment.
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 @.changeset/forty-dolphins-check.md:
- Around line 2-7: Update the changeset so the public API packages are bumped as
minor: change the entries for '@rocket.chat/core-typings' and
'@rocket.chat/apps-engine' from 'patch' to 'minor' (these expose the new
IVisitorExternalIdentifier and the externalIds on visitor types), while leaving
the internal-only packages ('@rocket.chat/model-typings',
'@rocket.chat/omni-core', '@rocket.chat/models', '@rocket.chat/meteor') as
'patch' if appropriate.
🪄 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: 1a2f5d89-009e-4852-b4c7-92c0c6f2292a
📒 Files selected for processing (1)
.changeset/forty-dolphins-check.md
📜 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🔨 Test Storybook / Test Storybook
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx:40-46
Timestamp: 2026-03-11T15:56:13.132Z
Learning: In `apps/meteor/client/views/omnichannel/directory/components/ContactField.tsx`, `shortName` is intentionally built from the visitor's internal `username` and `phone` (not from `externalIds`). `ContactField` shows the visitor's internal system identifier. Per-channel external identifiers (e.g. WhatsApp usernames from `externalIds[].username`) are only displayed in `ContactInfoChannelsItem`, which serves a different UI context.
📚 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:
.changeset/forty-dolphins-check.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/forty-dolphins-check.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/forty-dolphins-check.md
![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 17 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=".changeset/forty-dolphins-check.md">
<violation number="1" location=".changeset/forty-dolphins-check.md:3">
P2: `@rocket.chat/core-typings` and `@rocket.chat/apps-engine` both add new public API surface (`IVisitorExternalIdentifier`, `externalIds` on visitor types, `resolveVisitor` on `ILivechatCreator` / `LivechatBridge`). Per semver, adding new functionality is a `minor` bump, not `patch`. In particular, the new abstract method on `LivechatBridge` and the new method on `ILivechatCreator` will require downstream implementors to add the methods — marking these as `patch` under-reports the impact for consumers.</violation>
</file>
<file name="packages/omni-core/src/visitor/create.ts">
<violation number="1" location="packages/omni-core/src/visitor/create.ts:33">
P2: Setting `externalIds` as a whole object here overwrites any existing external IDs when updating an existing visitor. Merge with the stored `externalIds` (or set per-source) before the update so you don’t erase other channels’ identifiers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
cardoso
previously approved these changes
Mar 12, 2026
tassoevan
previously approved these changes
Mar 12, 2026
d-gubert
requested changes
Mar 12, 2026
Member
d-gubert
left a comment
d-gubert
left a comment
There was a problem hiding this comment.
Please address the bots' comments on the change set.
Additionally, please add tests for the new Apps-Engine API - you can follow the pattern seen in this PR https://github.com/RocketChat/Rocket.Chat/pull/38374/changes#diff-3edecfa2dd0bd2aec99f5a218abb5d57fcd0c85d434fd7a87136e9f1e3461232
ricardogarim
dismissed stale reviews from gabriellsh, KevLehman, and d-gubert
via
28ba4ed
ricardogarim
force-pushed
the
feat/whatsapp-usernames
branch
from
6dd59f1 to
28ba4ed
Compare
sampaiodiego
requested changes
Apr 7, 2026
apps/meteor/tests/unit/app/livechat/server/lib/resolveVisitor.spec.ts
Outdated
Show resolved
Hide resolved
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
sampaiodiego
reviewed
Apr 8, 2026
sampaiodiego
approved these changes
Apr 13, 2026
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
9 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 adds support for external identifiers (
externalIds) on livechat visitors, enabling WhatsApp BSUID support, username display, and progressive visitor enrichment.Issue(s)
Steps to test or reproduce
livechatCreator.resolveVisitor({ userId: '<BSUID>', username: '@johndoe' }, { phone: '<phone>' })source- the bridge automatically injects the appIdexternalIdspopulatedexternalId)resolveVisitorwith the same phone but newexternalIdexternalIdexternalId)resolveVisitorwith{ email: '<email>' }as contactDataexternalIdwinningPlanusesIXSCANwithindexName: 'externalIds.source_1_externalIds.userId_1'Further comments
externalIdson Visitor, not Contact: external identifiers are channel-specific (e.g., WhatsApp BSUID), so they belong on the visitor. Contacts reference visitors viachannels[].visitor.visitorId, and the UI fetches visitor data to display usernames.externalId, we enrich the existing record rather than creating a duplicate. This handles the migration case where visitors were created before BSUID support.ResolveVisitorContactDataunion type: theresolveVisitor()API accepts{ phone: string } | { email: string }for flexible fallback, making it extensible for future contact data types without breaking changes.externalIdsis an array of{ source, userId, username? }objects using the attribute pattern. A compound multikey index on{ "externalIds.source": 1, "externalIds.userId": 1 }enables efficient$elemMatchqueries. This was chosen over a Record/wildcard index for ~1.5-2× smaller index size and tighter index scans.source- the bridge automatically injects the appId. This ensures each app can only access its own external identifiers. The method signature usesOmit<IVisitorExternalIdentifier, 'source'>to enforce this at the type level.Index creation evidence
mongosh localhost:3001/meteor meteor [direct: primary] meteor> db.rocketchat_livechat_visitor.find({ | externalIds: { | $elemMatch: { | source: "a1b2c3d4-e5f6-4890-8bcd-ef1234567890", | userId: "bsuid-123" | } | } | }).explain("executionStats")Summary by CodeRabbit