-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: save users' names to call history #37867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds contact name and username to internal call-history records, saves those fields when creating internal call history entries, and updates call-history documents when a user's name or username changes via a new CallHistory.updateUserReferences method wired into the user-save flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 #37867 +/- ##
===========================================
+ Coverage 67.69% 67.78% +0.08%
===========================================
Files 3474 3474
Lines 113852 113852
Branches 20938 20938
===========================================
+ Hits 77073 77172 +99
+ Misses 34595 34494 -101
- Partials 2184 2186 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/models/src/models/CallHistory.ts (1)
32-48: Consider adding a database index on contactId for performance.The implementation correctly updates call history records when user identity changes. However, the
updateManyquery filters bycontactId, which is not currently indexed (onlyuidandcallIdare indexed per line 13).For users with extensive call history, this could result in slower updates. While this is a one-time operation per user identity change, consider adding an index on
contactIdif performance becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/app/lib/server/functions/saveUserIdentity.ts(2 hunks)apps/meteor/server/services/media-call/service.ts(2 hunks)apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts(5 hunks)packages/core-typings/src/ICallHistoryItem.ts(1 hunks)packages/model-typings/src/models/ICallHistoryModel.ts(2 hunks)packages/models/src/models/CallHistory.ts(2 hunks)
🧰 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/models/src/models/CallHistory.tsapps/meteor/server/services/media-call/service.tsapps/meteor/app/lib/server/functions/saveUserIdentity.tspackages/model-typings/src/models/ICallHistoryModel.tspackages/core-typings/src/ICallHistoryItem.tsapps/meteor/tests/unit/server/users/saveUserIdentity.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:
apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:23.819Z
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.
📚 Learning: 2025-12-18T15:18:23.819Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:23.819Z
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:
packages/models/src/models/CallHistory.tsapps/meteor/server/services/media-call/service.tsapps/meteor/app/lib/server/functions/saveUserIdentity.tspackages/model-typings/src/models/ICallHistoryModel.tspackages/core-typings/src/ICallHistoryItem.tsapps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
packages/models/src/models/CallHistory.tsapps/meteor/server/services/media-call/service.tsapps/meteor/app/lib/server/functions/saveUserIdentity.tspackages/model-typings/src/models/ICallHistoryModel.tspackages/core-typings/src/ICallHistoryItem.tsapps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/lib/server/functions/saveUserIdentity.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/unit/server/users/saveUserIdentity.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/server/users/saveUserIdentity.spec.ts
🧬 Code graph analysis (5)
packages/models/src/models/CallHistory.ts (1)
packages/core-typings/src/IUser.ts (1)
IRegisterUser(261-264)
apps/meteor/server/services/media-call/service.ts (3)
packages/media-signaling/src/lib/Call.ts (1)
contact(81-83)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)packages/core-typings/src/ICallHistoryItem.ts (1)
IInternalMediaCallHistoryItem(37-45)
apps/meteor/app/lib/server/functions/saveUserIdentity.ts (1)
packages/models/src/index.ts (1)
CallHistory(152-152)
packages/model-typings/src/models/ICallHistoryModel.ts (1)
packages/core-typings/src/IUser.ts (1)
IRegisterUser(261-264)
packages/core-typings/src/ICallHistoryItem.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-259)
🔇 Additional comments (8)
apps/meteor/app/lib/server/functions/saveUserIdentity.ts (2)
3-3: LGTM!The CallHistory import follows the established pattern for model imports.
184-186: LGTM!The CallHistory update propagation correctly mirrors the VideoConference pattern and ensures user identity changes are reflected in call history records.
packages/core-typings/src/ICallHistoryItem.ts (1)
40-41: LGTM!The optional fields are appropriately typed and positioned. Making them optional ensures backward compatibility with existing call history records.
apps/meteor/server/services/media-call/service.ts (2)
135-143: LGTM!The helper method appropriately structures contact data for internal call history, making the code more maintainable and ensuring consistent field mapping.
174-174: LGTM!The spread operator usage cleanly integrates contact data into history items, replacing the previous direct
contactIdassignments while adding the new identity fields.Also applies to: 181-181
packages/model-typings/src/models/ICallHistoryModel.ts (1)
1-1: LGTM!The interface correctly defines the
updateUserReferencesmethod contract, with appropriate type references and optional name parameter.Also applies to: 19-19
apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts (2)
13-13: LGTM!The test stub and mock setup correctly mirror the VideoConference pattern, ensuring CallHistory updates are testable.
Also applies to: 36-38
117-117: LGTM!Test assertions appropriately verify that CallHistory updates occur when user identity changes and are skipped when there are no changes.
Also applies to: 129-129, 139-139
Proposed changes (including videos or screenshots)
Issue(s)
VGA-116
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.