fix(mail): de-duplicate event mails #1128
Conversation
junaid-shirur
requested review from
devesh-juspay,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
Summary of ChangesHello @junaid-shirur, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where calendar event invitation emails were leading to incorrect or duplicated permission assignments. By implementing a conditional check based on the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses the de-duplication of event emails by modifying the logic for determining email permissions. Specifically, it introduces a conditional check based on whether the mailId starts with calendar-, and if so, sets the permissions to only include the userEmail. This change aims to prevent duplicate indexing of calendar event invitation emails where each attendee receives a unique email with their name in the subject line. The review focuses on the correctness and potential implications of this change.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughparseMail now treats mails with IDs starting with "calendar-" as calendar invitations and sets their permissions to a single-element array containing the current user email; other mails continue to collect unique From/To/Cc/Bcc addresses. A new migration script updates stored calendar-mail permissions accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Gmail as Gmail parseMail
participant Parser as parseMail logic
participant Store as Document store
Gmail->>Parser: provide mail data (mailId, headers, userMap)
alt mailId starts with "calendar-"
Parser->>Parser: compute permissions = [currentUserEmail]
else other mail
Parser->>Parser: collect unique From/To/Cc/Bcc (lowercased)
end
Parser->>Store: attach computed permissions to mail record
sequenceDiagram
participant Runner as fix-calendar-mail-permissions
participant Vespa as Vespa query/update API
participant Logger as Logger
Runner->>Vespa: queryCalendarEmails(offset, limit)
Vespa-->>Runner: return documents + totalCount
loop per document
Runner->>Runner: determine expectedPermissions ([userEmail])
alt permissions differ
Runner->>Vespa: UpdateDocument("mail", docId, {permissions: expectedPermissions})
Vespa-->>Runner: update result
Runner->>Logger: log success/failure
else already correct
Runner->>Logger: increment alreadyCorrect
end
end
Runner->>Logger: log totals (totalFixed, alreadyCorrect)
Runner->>Runner: process exit (0 on success, 1 on failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/integrations/google/gmail/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: naSim087
PR: xynehq/xyne#484
File: server/integrations/google/gmail-worker.ts:293-294
Timestamp: 2025-05-28T10:55:46.701Z
Learning: There are two separate `parseMail` functions in the codebase: one in `server/integrations/google/gmail-worker.ts` with signature `(email, gmail, client, userEmail)` returning `{ mailData, insertedPdfCount, exist }`, and another in `server/integrations/google/gmail/index.ts` with signature `(email, gmail, userEmail, client, tracker?)` returning `{ mailData, exist }`. Each file calls its own local version correctly.
📚 Learning: 2025-05-28T10:55:46.701Z
Learnt from: naSim087
PR: xynehq/xyne#484
File: server/integrations/google/gmail-worker.ts:293-294
Timestamp: 2025-05-28T10:55:46.701Z
Learning: There are two separate `parseMail` functions in the codebase: one in `server/integrations/google/gmail-worker.ts` with signature `(email, gmail, client, userEmail)` returning `{ mailData, insertedPdfCount, exist }`, and another in `server/integrations/google/gmail/index.ts` with signature `(email, gmail, userEmail, client, tracker?)` returning `{ mailData, exist }`. Each file calls its own local version correctly.
Applied to files:
server/integrations/google/gmail/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/scripts/fix-calendar-mail-permissions.ts (1)
16-19: Remove redundanthitsparameter.The
hitsparameter is redundant since thelimitis already specified in the YQL query on line 14.Apply this diff to remove the redundant parameter:
const params = new URLSearchParams({ yql, - hits: limit.toString(), })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/scripts/fix-calendar-mail-permissions.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/scripts/fix-calendar-mail-permissions.ts (2)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)server/search/vespa.ts (1)
UpdateDocument(24-24)
🔇 Additional comments (4)
server/scripts/fix-calendar-mail-permissions.ts (4)
1-10: LGTM!The shebang, imports, and logger initialization are correctly structured.
58-98: Permission extraction and update logic is sound.The logic correctly:
- Extracts the user email from userMap keys
- Validates that a user exists before proceeding
- Normalizes email to lowercase for consistent comparison
- Skips documents that already have correct permissions (good optimization)
- Handles individual update failures gracefully without aborting the entire migration
112-120: LGTM!The script execution follows best practices with proper exit codes and error handling.
35-35: Review comment is outdated and does not reflect actual code.The review comment shows
data.root?.fields?.totalCount || 0, but the actual code at line 35 usesdata.root?.fields?.n || 0. The current implementation follows the established codebase pattern where Vespa responses contain the total count in thenfield, nottotalCount. This is consistently applied across all Vespa query scripts (userConnectionData.ts, clear-vespa-data.ts, evaluateSearchQuality.ts, etc.) and validated by test fixtures. The reviewer's suggested alternative using.root.coverage.documentsdoes not exist in the codebase.Likely an incorrect or invalid review comment.
1 participant
Description
https://juspay.atlassian.net/browse/XYNE-225?atlOrigin=eyJpIjoiNTIzNzAxMmE0NjViNDg1YmEyZGY2MmJiMDQ4YzI4ZWQiLCJwIjoiaiJ9
Testing
Additional Notes
Summary by CodeRabbit
Bug Fixes
Chores