-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: improve exported type for 'call-history.info' endpoint #37905
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! 🎉 |
|
WalkthroughSimplifies the CallHistoryInfo type to a non-mutually-exclusive union and refactors call-history.info to read query parameters into local Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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)
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 (1)
apps/meteor/app/api/server/v1/call-history.ts(2 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/app/api/server/v1/call-history.ts
🧠 Learnings (3)
📓 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/app/api/server/v1/call-history.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/app/api/server/v1/call-history.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/call-history.ts (1)
packages/models/src/index.ts (1)
CallHistory(152-152)
⏰ 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: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/call-history.ts (1)
137-137: Problematic type cast obscures intent; consider clearer alternative.The union type simplification and runtime schema validation are sound approaches. However, line 205's type cast
as Record<string, never> & typeof this.queryParamsis semantically misleading—Record<string, never>literally means all values arenevertype and doesn't accurately reflect the intent to make both properties optional for destructuring. This works because AJV schema validation (lines 139-164) enforces mutual exclusivity at runtime, but the cast choice obscures the actual contract.Consider replacing with a clearer cast:
as Partial<Record<'historyId' | 'callId', string>>- Or simply remove the cast and rely on type narrowing after validation
The simplified type and schema-based enforcement work correctly—existing callers pass either
historyIdorcallIdwithout issue—but the implementation should reflect that clearly.⛔ Skipped due to 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.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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37905 +/- ##
===========================================
- Coverage 67.77% 67.71% -0.07%
===========================================
Files 3475 3475
Lines 113861 113863 +2
Branches 20942 20942
===========================================
- Hits 77171 77098 -73
- Misses 34498 34584 +86
+ Partials 2192 2181 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Old type could not be passed without type casting;
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.