Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIntroduces a helper Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as createFilter / updateFilter
participant Builder as buildFilterActions
participant Labels as getLabelById
note right of Caller `#D3E4CD`: Caller supplies addLabelIds/removeLabelIds
Caller->>Builder: buildFilterActions(addLabelIds, removeLabelIds)
activate Builder
alt addLabelIds present
loop for each addLabelId
Builder->>Labels: getLabelById(id)
Labels-->>Builder: categoryName or error/fallback id
end
Builder->>Builder: accumulate assignCategories
end
alt removeLabelIds present
Builder->>Builder: set moveToFolder from removeLabelIds
end
alt no actions resolved
Builder->>Builder: set markAsRead = true (fallback)
end
Builder-->>Caller: actions object (moveToFolder, markAsRead, assignCategories)
deactivate Builder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 1
🧹 Nitpick comments (2)
apps/web/utils/outlook/filter.ts (2)
243-268: Parallelize the API calls for better performance.The function makes sequential API calls in a loop, which can be slow when resolving multiple label IDs. Consider using
Promise.all()orPromise.allSettled()to parallelize these calls.Apply this diff to parallelize the API calls:
async function resolveCategoryNames( client: OutlookClient, labelIds: string[], ): Promise<string[]> { - const categoryNames: string[] = []; - - for (const labelId of labelIds) { - try { - const category = await getLabelById({ client, id: labelId }); - if (category?.displayName) { - categoryNames.push(category.displayName); - } else { - logger.warn("Category not found by ID, using ID as name", { labelId }); - categoryNames.push(labelId); - } - } catch (error) { - logger.warn("Failed to resolve category ID, using ID as name", { - labelId, - error, - }); - categoryNames.push(labelId); - } - } - - return categoryNames; + const results = await Promise.allSettled( + labelIds.map((labelId) => + getLabelById({ client, id: labelId }).then((category) => ({ + labelId, + category, + })) + ) + ); + + return results.map((result, index) => { + const labelId = labelIds[index]; + if (result.status === "fulfilled" && result.value.category?.displayName) { + return result.value.category.displayName; + } else { + const error = result.status === "rejected" ? result.reason : undefined; + logger.warn( + result.status === "rejected" + ? "Failed to resolve category ID, using ID as name" + : "Category not found by ID, using ID as name", + { labelId, error } + ); + return labelId; + } + }); }
57-94: Consider usingbuildFilterActionsfor consistency.The
createAutoArchiveFilterfunction has hardcoded action construction that could be refactored to use the newbuildFilterActionshelper for consistency. While this function has a specific purpose, using the shared helper would reduce code duplication and improve maintainability.You could refactor it like this:
export async function createAutoArchiveFilter({ client, from, labelName, }: { client: OutlookClient; from: string; labelName?: string; }) { try { const actions = await buildFilterActions({ client, addLabelIds: labelName ? [labelName] : undefined, removeLabelIds: ["INBOX", "UNREAD"], context: { from, labelName }, }); const rule: MessageRule = { displayName: `Auto-archive filter for ${from}`, sequence: 1, isEnabled: true, conditions: { senderContains: [from], }, actions, }; const response: MessageRule = await withOutlookRetry(() => client.getClient().api("/me/mailFolders/inbox/messageRules").post(rule), ); return { status: 201, data: response }; } catch (error) { if (isAlreadyExistsError(error)) { logger.warn("Auto-archive filter already exists", { from }); return { status: 200 }; } throw error; } }Note: This assumes
labelNameis a label ID rather than a display name. If it's already a display name, you may need to adjust the approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/outlook/filter.ts(5 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/outlook/filter.ts (2)
apps/web/utils/outlook/client.ts (1)
OutlookClient(19-80)apps/web/utils/outlook/label.ts (1)
getLabelById(68-78)
⏰ 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 (6)
version.txt (1)
1-1: LGTM: Version bump reflects the API changes.The version bump from v2.19.6 to v2.19.7 is appropriate for the filter refactoring changes introduced in this PR.
apps/web/utils/outlook/filter.ts (5)
9-9: LGTM: Import added for label resolution.The
getLabelByIdimport is correctly added to support the new category name resolution functionality.
23-41: LGTM: Refactored to use the new helper function.The refactoring to use
buildFilterActionsimproves code maintainability and consistency with theupdateFilterfunction.
192-221: LGTM: Consistent refactoring with dynamic action construction.The
updateFilterfunction now acceptsaddLabelIdsand usesbuildFilterActionsfor dynamic action construction, maintaining consistency withcreateFilter.
274-320: LGTM: Well-structured helper with good documentation and fallback behavior.The
buildFilterActionsfunction is well-implemented with:
- Proper async handling for category name resolution
- Clear handling of special labels ("INBOX" → archive, "UNREAD" → markAsRead)
- Sensible fallback to
markAsReadwhen no actions are specified- Good logging for debugging
300-306: Label identifiers are correct and consistent with codebase design.The "INBOX" and "UNREAD" identifiers used in the filter code are properly defined in
apps/web/utils/outlook/label.tsand align with the intentional design pattern. The codebase comment explicitly states these are mapped for Outlook "using same format as Gmail for consistency." The identifiers are used consistently throughout the Outlook integration code, including in related filter and stats logic.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/utils/outlook/filter.ts (1)
57-94: Refactor to use the new helper functions for consistency.The
createAutoArchiveFilterfunction was not updated to use the newbuildFilterActionsandresolveCategoryNameshelpers. It still directly assignslabelNametoassignCategorieswithout validation, which could fail iflabelNameis not a valid category display name frommasterCategories.Apply this refactor to align with the approach used in
createFilterandupdateFilter:export async function createAutoArchiveFilter({ client, from, labelName, }: { client: OutlookClient; from: string; labelName?: string; }) { try { - // For Outlook, we'll create a rule that moves messages to archive + const actions = await buildFilterActions({ + client, + addLabelIds: labelName ? [labelName] : undefined, + removeLabelIds: ["INBOX", "UNREAD"], + context: { from, autoArchive: true }, + }); + const rule: MessageRule = { displayName: `Auto-archive filter for ${from}`, sequence: 1, isEnabled: true, conditions: { senderContains: [from], }, - actions: { - moveToFolder: "archive", - markAsRead: true, - ...(labelName && { assignCategories: [labelName] }), - }, + actions, }; const response: MessageRule = await withOutlookRetry(() => client.getClient().api("/me/mailFolders/inbox/messageRules").post(rule), ); return { status: 201, data: response }; } catch (error) { if (isAlreadyExistsError(error)) { logger.warn("Auto-archive filter already exists", { from }); return { status: 200 }; } throw error; } }Note: If
labelNameis expected to be a category ID rather than a display name, this refactor ensures proper resolution. If it's already a display name, you'll need to adjust the approach or add a parameter to distinguish between IDs and names.
🧹 Nitpick comments (1)
apps/web/utils/outlook/filter.ts (1)
243-266: Good fix for the previous review concern, but consider handling partial failures more explicitly.The code correctly addresses the past review comment by not falling back to raw
labelIdvalues as category names. However, when category resolution fails for some or all IDs, they're silently skipped (with error logs). This could lead to filters that don't behave as users expect.Consider one of these approaches:
- Return resolution metadata to let callers know about failures:
-): Promise<string[]> { +): Promise<{ categoryNames: string[]; failedIds: string[] }> { const categoryNames: string[] = []; + const failedIds: string[] = []; for (const labelId of labelIds) { try { const category = await getLabelById({ client, id: labelId }); if (category?.displayName) { categoryNames.push(category.displayName); } else { logger.error("Category not found by ID", { labelId }); + failedIds.push(labelId); } } catch (error) { logger.error("Failed to resolve category ID", { labelId, error, }); + failedIds.push(labelId); } } - return categoryNames; + return { categoryNames, failedIds }; }
- Throw an error if all resolutions fail, ensuring at least some categories are resolved when the user explicitly provides them.
Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/outlook/filter.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/outlook/filter.ts (2)
apps/web/utils/outlook/client.ts (1)
OutlookClient(19-80)apps/web/utils/outlook/label.ts (1)
getLabelById(68-78)
⏰ 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 (4)
apps/web/utils/outlook/filter.ts (4)
9-9: LGTM!The import is necessary for the new category resolution functionality.
23-31: LGTM!Good refactoring to extract action building logic into a reusable helper. This improves maintainability and reduces code duplication.
196-211: LGTM!Consistent refactoring that mirrors the
createFilterchanges, properly adding support foraddLabelIdsparameter.
268-318: No issues found — implementation correctly handles Microsoft Graph API requirements.The web search confirms that the Microsoft Graph messageRule API requires at least one action and rejects empty actions objects with HTTP 400. Your code's fallback to
markAsReadwhen no actions are generated is necessary to satisfy this API requirement. The implementation is correct: it prevents API failures, includes appropriate warning logging when the fallback is triggered, and ensures the service will always receive a valid request. No changes needed.
1 participant
Summary by CodeRabbit
New Features
Chores