chore: Improving notifyOnEmailInboxChanged type safety #38860
chore: Improving notifyOnEmailInboxChanged type safety #38860Pratheek555 wants to merge 3 commits into
Conversation
|
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 |
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
211-212: Consider using the already-importedAtLeastutility for consistency.
Pick<IEmailInbox, '_id'> & Partial<IEmailInbox>is semantically identical toAtLeast<IEmailInbox, '_id'>, which is already imported (line 16) and used elsewhere in this file for the same pattern (e.g.,notifyOnIntegrationHistoryChanged). Using the utility type directly improves consistency and readability.♻️ Proposed refactor
-export const notifyOnEmailInboxChanged = async ( - data: Pick<IEmailInbox, '_id'> & Partial<IEmailInbox>, +export const notifyOnEmailInboxChanged = async ( + data: AtLeast<IEmailInbox, '_id'>, clientAction: ClientAction = 'updated', ): Promise<void> => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyListener.ts` around lines 211 - 212, Replace the ad-hoc type annotation in the notifyOnEmailInboxChanged signature (currently "Pick<IEmailInbox, '_id'> & Partial<IEmailInbox>") with the already-imported utility type AtLeast<IEmailInbox, '_id'> for consistency; update the function declaration for notifyOnEmailInboxChanged to use AtLeast<IEmailInbox, '_id'> and ensure no other code changes are needed since AtLeast is already imported and used elsewhere (e.g., notifyOnIntegrationHistoryChanged).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyListener.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/lib/notifyListener.ts
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
apps/meteor/app/lib/server/lib/notifyListener.ts
⏰ 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: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🤖 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/lib/notifyListener.ts`:
- Around line 211-212: Replace the ad-hoc type annotation in the
notifyOnEmailInboxChanged signature (currently "Pick<IEmailInbox, '_id'> &
Partial<IEmailInbox>") with the already-imported utility type
AtLeast<IEmailInbox, '_id'> for consistency; update the function declaration for
notifyOnEmailInboxChanged to use AtLeast<IEmailInbox, '_id'> and ensure no other
code changes are needed since AtLeast is already imported and used elsewhere
(e.g., notifyOnIntegrationHistoryChanged).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38860 +/- ##
===========================================
- Coverage 70.65% 70.64% -0.01%
===========================================
Files 3190 3190
Lines 112732 112732
Branches 20431 20382 -49
===========================================
- Hits 79649 79642 -7
- Misses 31035 31047 +12
+ Partials 2048 2043 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Proposed changes (including videos or screenshots)
This PR focuses on helping with the
//TODO : improve typingremark in apps/meteor/app/lib/server/lib/notifyListener.ts. Upon understanding the places where notifyOnEmailInboxChanged is called and noticed that _id is compulsory to be passed while everything else is optional in the IEmailInbox interface.Related Issue #38856
Changes Made
_idrequired while accepting partial updates.Summary by CodeRabbit