-
Notifications
You must be signed in to change notification settings - Fork 13k
chore!: migrate user names into existing Call History entries #37898
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! 🎉 |
|
WalkthroughThis PR introduces a new database migration (v332) that backfills missing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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: 1
📜 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 (2)
apps/meteor/server/startup/migrations/index.ts(1 hunks)apps/meteor/server/startup/migrations/v332.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/startup/migrations/v332.tsapps/meteor/server/startup/migrations/index.ts
🧠 Learnings (4)
📓 Common learnings
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:
apps/meteor/server/startup/migrations/v332.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:
apps/meteor/server/startup/migrations/v332.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/server/startup/migrations/index.ts
🧬 Code graph analysis (1)
apps/meteor/server/startup/migrations/v332.ts (1)
packages/models/src/index.ts (1)
CallHistory(141-141)
🔇 Additional comments (2)
apps/meteor/server/startup/migrations/v332.ts (1)
19-51: Aggregation pipeline logic looks correct.The
$lookup,$addFields,$project, and$mergestages are correctly structured. The for-await loop to drain the cursor is the correct pattern to ensure the$mergeoperation completes.apps/meteor/server/startup/migrations/index.ts (1)
40-40: LGTM!The new migration import follows the established pattern and is correctly sequenced after v331.
| $match: { | ||
| external: false, | ||
| contactId: { $exists: true }, | ||
| contactName: { $exists: false }, | ||
| contactUssername: { $exists: false }, | ||
| }, |
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.
Critical typo: contactUssername should be contactUsername.
Line 15 has a typo with double 's' in contactUssername. This breaks the filter logic—documents that already have contactUsername populated will still match because contactUssername (the typo) doesn't exist on any document. This will cause the migration to unnecessarily process and update records that should be skipped.
🔎 Proposed fix
{
$match: {
external: false,
contactId: { $exists: true },
contactName: { $exists: false },
- contactUssername: { $exists: false },
+ contactUsername: { $exists: false },
},
},🤖 Prompt for AI Agents
In apps/meteor/server/startup/migrations/v332.ts around lines 11 to 16, the
$match filter contains a typo "contactUssername" (double 's') which prevents
correct filtering; change that key to "contactUsername" so the query checks the
actual field, update any other occurrences of the misspelled field in this file,
and run the migration/dry-run to verify only documents missing contactUsername
are processed.
d14079a to
381143c
Compare
4699eb7 to
c8ae694
Compare
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.