refactor: use options object in sendMessage function signature#38772
refactor: use options object in sendMessage function signature#38772kodiakhq[bot] merged 5 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38772 +/- ##
===========================================
- Coverage 70.48% 70.46% -0.03%
===========================================
Files 3176 3176
Lines 111476 111476
Branches 20157 20149 -8
===========================================
- Hits 78578 78550 -28
- Misses 30854 30881 +27
- Partials 2044 2045 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
0307947 to
6622b09
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
23d831a
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
17-20: Consider exportingSendMessageOptionsfor callers that want explicit type annotations.The type is currently unexported. Callers relying on structural typing work correctly today, but code outside this module that wants to declare a typed intermediate variable (e.g.,
const opts: SendMessageOptions = { upsert: true }) cannot import it. Given the PR explicitly introduces this type to be extended (e.g.,filesToConfirm), exporting it now avoids a follow-up churn when new fields are added.♻️ Proposed change
-type SendMessageOptions = { +export type SendMessageOptions = { upsert?: boolean; previewUrls?: string[]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/sendMessage.ts` around lines 17 - 20, The SendMessageOptions type is currently internal; export it so external callers can import and annotate variables (e.g., const opts: SendMessageOptions = {...}) and to make future extensions (like filesToConfirm) non-breaking; modify the declaration of SendMessageOptions to be exported (export type SendMessageOptions = {...}) in sendMessage.ts and keep the same fields (upsert, previewUrls) so consumers can import the symbol and use it in signatures and intermediate variables.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/sendMessage.ts
🧰 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/lib/server/functions/sendMessage.ts
🧠 Learnings (1)
📚 Learning: 2025-11-17T14:30:36.271Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.271Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.
Applied to files:
apps/meteor/app/lib/server/functions/sendMessage.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
sendMessage(135-174)apps/meteor/server/services/messages/service.ts (1)
sendMessage(86-88)
⏰ 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 comments (1)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
225-226: LGTM — destructuring correctly preserves prior defaults.
upsert = falseand the optionalpreviewUrlsmatch the old positional-argument semantics. TypeScript will reject any still-unconverted call site passing a rawbooleanas the 4th argument at compile time. No unconverted call sites exist;executeSendMessagein the methods file is a different import and already callssendMessagewith the new destructured options signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/functions/sendMessage.ts`:
- Around line 17-20: The SendMessageOptions type is currently internal; export
it so external callers can import and annotate variables (e.g., const opts:
SendMessageOptions = {...}) and to make future extensions (like filesToConfirm)
non-breaking; modify the declaration of SendMessageOptions to be exported
(export type SendMessageOptions = {...}) in sendMessage.ts and keep the same
fields (upsert, previewUrls) so consumers can import the symbol and use it in
signatures and intermediate variables.
Proposed changes (including videos or screenshots)
This PR refactors the internal
sendMessagefunction signature to use an options object instead of positional parameters.Before:
After:
Benefits:
Issue(s)
Related to #32703 - This refactoring was suggested during code review to improve the function signature before adding additional parameters.
Further comments
This is a pure internal refactoring with no changes to external APIs (Meteor methods, REST endpoints). The Meteor method
sendMessagesignature remains unchanged - only the internal function used by server-side code was updated.This change prepares the codebase for #32703 which needs to add a
filesToConfirmparameter. With this refactoring in place, that PR can simply extend theSendMessageOptionstype instead of adding yet another positional parameter.Summary by CodeRabbit