Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR refactors Outlook message retrieval by removing folder-ID inference logic and helper functions, shifting folder and draft filtering to downstream processing. The Changes
Sequence DiagramsequenceDiagram
participant Client
participant microsoft.ts as getMessagesWithPagination<br/>(microsoft.ts)
participant message.ts as queryBatchMessages<br/>(message.ts)
participant OutlookAPI
Client->>microsoft.ts: Request messages
Note over microsoft.ts: No folderId inference<br/>from parentFolderId
microsoft.ts->>message.ts: Call queryBatchMessages<br/>(no folderId param)
alt folderId provided
message.ts->>message.ts: Build filters array<br/>with folderFilter
else folderId not provided
message.ts->>message.ts: Build filters array<br/>without folder constraint
end
message.ts->>OutlookAPI: Request with conditional<br/>filter constraints
OutlookAPI-->>message.ts: Return messages<br/>(multiple folders possible)
message.ts->>message.ts: Post-fetch filtering<br/>(downstream filtering:<br/>drafts, folder constraints)
message.ts-->>Client: Filtered messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/utils/outlook/message.ts (1)
232-256: Filter‑path refactor to usefilters[]and conditional.filter()is an improvementBuilding a
filtersarray, composingcombinedFilteronly when non‑empty, and guarding the call to.filter(combinedFilter)avoids sending an empty$filterand keeps the logic straightforward.This is a clean, maintainable approach and aligns with the new optional folder filter behavior.
If you later need to add more conditions (e.g., explicit Junk/Deleted exclusion), extending this
filtersarray pattern will keep the code simple and testable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/utils/email/microsoft.ts(1 hunks)apps/web/utils/outlook/message.test.ts(0 hunks)apps/web/utils/outlook/message.ts(3 hunks)version.txt(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/utils/outlook/message.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/outlook/message.ts (1)
apps/web/utils/outlook/odata-escape.ts (1)
escapeODataString(13-20)
⏰ 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: cubic · AI code reviewer
- GitHub Check: test
🔇 Additional comments (2)
version.txt (1)
1-1: Version bump looks consistentVersion updated to
v2.19.8; no code logic involved here. Looks good.apps/web/utils/outlook/message.ts (1)
186-225: New optional folder filtering semantics look good, but confirm callers expect broader scopeThe change to compute
folderFilteronly whenfolderIdis provided and to rely on post‑processing for search results means:
- If
folderIdis omitted, both search and filter paths now operate over all folders (subject to Graph defaults) instead of implicitly limiting to inbox/archive.- If
folderIdis provided, the behavior remains scoped viaparentFolderId eq '<folderId>'or post‑filtering in the search path.This matches the PR goal of loading more data, but it does change the contract: any consumers that previously assumed “inbox/archive only” from
queryBatchMessageswhen omittingfolderIdwill now see a wider set (e.g., sent, archive, potentially others).Recommend quickly auditing the known call sites (e.g., generic message listing vs. stats) to ensure:
- UI‑facing flows that should stay inbox‑only pass an explicit folderId or use
queryMessagesWithFilters.- Stats/reporting flows intentionally rely on the broader scope.
| // Don't pass folderId - let the API return all folders except Junk/Deleted (auto-excluded) | ||
| // Drafts are filtered out in convertMessages | ||
| const response = await queryBatchMessages(this.client, { |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify drafts behavior and Junk/Deleted assumptions for this path
The new behavior (no folderId passed) makes sense for “load more data for Outlook stats” and matches the new queryBatchMessages semantics.
Two points to double‑check:
-
Drafts filtering:
convertMessagesexplicitly filters outmessage.isDraft.getDrafts()here callsgetMessagesWithPagination({ query: "isDraft eq true", ... }), which ultimately goes throughqueryBatchMessages→convertMessages, so any drafts returned by Graph will be dropped.- If
getDrafts()is used anywhere, it will never return results with the current pipeline. You may want either:- A dedicated drafts path that bypasses
convertMessages, or - A way to opt out of the draft filter when the caller explicitly requests drafts.
- A dedicated drafts path that bypasses
-
“API excludes Junk/Deleted by default” comment:
- If this relies on Microsoft Graph behavior rather than our own filters, it might be worth confirming and/or documenting that guarantee; otherwise, we may need explicit exclusion filters if Junk/Deleted should stay out of stats.
🏁 Script executed:
#!/bin/bash
# Find getDrafts implementation
echo "=== Finding getDrafts implementation ==="
rg -nA 20 'getDrafts\s*\(' --type=ts apps/web/utils/
echo ""
echo "=== Finding convertMessages implementation ==="
rg -nA 30 'export.*convertMessages|function convertMessages' --type=ts apps/web/utils/
echo ""
echo "=== Checking queryBatchMessages and getMessagesWithPagination ==="
rg -nB 5 -A 15 'const queryBatchMessages|function queryBatchMessages' --type=ts apps/web/Length of output: 11592
🌐 Web query:
Microsoft Graph API /me/mailFolders default behavior Junk Deleted excluded
💡 Result:
Short answer: /me/mailFolders lists the mail folders directly under the mailbox root (shallow), and it does not implicitly exclude JunkEmail or DeletedItems — those well‑known folders exist as children and will be returned if they are direct children. (What does get excluded by default is hidden folders; and /me/messages — a different endpoint — does skip some folders like Sent/Outbox/Junk/Deleted when listing "all messages".) [1][2][3]
Notes and quick tips:
- To get a specific folder use the well‑known name, e.g. /me/mailFolders/Inbox or /me/mailFolders/JunkEmail. [2]
- To include hidden folders add ?includeHiddenFolders=true. [1][3]
Sources:
- MailFolders behavior (list, includeHiddenFolders). [1]
- Well‑known folder names and examples (Inbox, JunkEmail, DeletedItems). [2]
- Microsoft Q&A / guidance on traversal (v1.0 shallow vs beta full traversal). [3]
🏁 Script executed:
#!/bin/bash
# Get full queryBatchMessages implementation
echo "=== Full queryBatchMessages implementation ==="
rg -nA 50 'export async function queryBatchMessages' apps/web/utils/outlook/message.ts
echo ""
echo "=== Check for Graph endpoint calls and folder filtering ==="
rg -nB 5 -A 5 'mailFolders|/messages|JunkEmail|DeletedItems' apps/web/utils/outlook/message.ts | head -80Length of output: 3063
🏁 Script executed:
#!/bin/bash
# Find getMessagesWithPagination in microsoft.ts (the method called by getDrafts)
echo "=== getMessagesWithPagination in microsoft.ts ==="
rg -nB 5 -A 40 'async getMessagesWithPagination' apps/web/utils/email/microsoft.ts
echo ""
echo "=== Check if convertMessages is called directly or in queryBatchMessages ==="
rg -nB 3 -A 3 'convertMessages' apps/web/utils/email/microsoft.tsLength of output: 2061
🏁 Script executed:
#!/bin/bash
# Get complete queryBatchMessages implementation (it was cut off)
echo "=== Complete queryBatchMessages implementation ==="
rg -nA 120 'export async function queryBatchMessages' apps/web/utils/outlook/message.ts | grep -A 120 'export async function queryBatchMessages'
echo ""
echo "=== Check where convertMessages is called in queryBatchMessages output ==="
rg -n 'convertMessages' apps/web/utils/outlook/message.tsLength of output: 4856
Fix getDrafts() to bypass draft filtering when explicitly requesting drafts
When getDrafts() calls getMessagesWithPagination({ query: "isDraft eq true", ... }), the results flow through queryBatchMessages → convertMessages, which unconditionally filters out all messages where !message.isDraft (line 370 of apps/web/utils/outlook/message.ts). This means getDrafts() will always return an empty array, breaking the active feature in apps/web/utils/ai/report/fetch.ts:110–128 that relies on it to fetch draft email templates.
Two options to fix:
- Add a bypass flag to
convertMessagesto skip draft filtering when explicitly requested, or - Create a dedicated drafts-only path that skips
convertMessagesfiltering.
The comment about Junk/Deleted being auto-excluded by the API is accurate per Microsoft Graph /me/messages behavior and does not require changes.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 763-765, getDrafts() currently
calls queryBatchMessages → convertMessages which unconditionally filters out
drafts; update convertMessages to accept an options parameter (e.g., {
allowDrafts: boolean } defaulting to false) that, when true, bypasses the
"!message.isDraft" filter, then change queryBatchMessages/getDrafts to pass
allowDrafts: true for the drafts path; ensure all other call sites keep default
behavior and update related types/comments accordingly.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores