Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRefactors logging by removing module-scoped loggers and threading a request/action-scoped Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router as withEmailAccount/<route>
participant Handler as RouteHandler
participant Req as RequestWithLogger
participant Factory as createEmailProvider
participant Provider as EmailProvider
Note right of Router `#D3E4CD`: incoming HTTP request\nincludes request.logger
Client->>Router: HTTP POST/GET
Router->>Handler: invoke handler(request)
Handler->>Req: extract emailAccountId & request.logger
Req-->>Handler: emailAccountId, provider, request.logger
Handler->>Factory: createEmailProvider({ emailAccountId, provider, logger: request.logger })
Factory->>Provider: new Provider(client, logger)
Provider->>Provider: use this.logger for internal logs
alt success
Provider-->>Handler: result
Handler-->>Client: 200/response
else error
Provider-->>Handler: throws
Handler->>Handler: request.logger.error(...)
Handler-->>Client: error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/utils/email/watch-manager.ts (1)
7-7: Module-scoped logger contradicts PR objectives.This file still uses a module-scoped logger, which contradicts the PR's goal to "remove module-scoped loggers and thread request-scoped Logger." Module-scoped loggers lack request context (request IDs, user IDs, etc.) that would be available with request-scoped loggers.
The exported function
ensureEmailAccountsWatchedand internal functions should accept aloggerparameter and thread it through the call chain, similar to how other files in this PR handle it.Apply this approach:
-const logger = createScopedLogger("email/watch-manager"); - export type WatchEmailAccountResult =Update the exported function signature:
export async function ensureEmailAccountsWatched({ userIds, + logger, }: { userIds: string[] | null; + logger: Logger; }): Promise<WatchEmailAccountResult[]> { const emailAccounts = await getEmailAccountsToWatch(userIds); - return await watchEmailAccounts(emailAccounts); + return await watchEmailAccounts(emailAccounts, logger); }Then thread it through
watchEmailAccountsandwatchEmailAccount:async function watchEmailAccounts( emailAccounts: Awaited<ReturnType<typeof getEmailAccountsToWatch>>, logger: Logger, ): Promise<WatchEmailAccountResult[]> async function watchEmailAccount( emailAccount: Awaited<ReturnType<typeof getEmailAccountsToWatch>>[number], logger: Logger, ): Promise<WatchEmailAccountResult | null>apps/web/utils/email/google.ts (1)
677-696: Fix inverted warning condition in blockUnsubscribedEmailIn
blockUnsubscribedEmail, the warning condition is reversed:const unsubscribeLabel = await this.getOrCreateInboxZeroLabel("unsubscribed"); if (unsubscribeLabel?.id) { log.warn("Unsubscribe label not found"); }This logs
"Unsubscribe label not found"whenunsubscribeLabel.idis actually present. It should warn when the label is missing, and you may also want to short‑circuit in that case. For example:- const unsubscribeLabel = - await this.getOrCreateInboxZeroLabel("unsubscribed"); - - if (unsubscribeLabel?.id) { - log.warn("Unsubscribe label not found"); - } + const unsubscribeLabel = + await this.getOrCreateInboxZeroLabel("unsubscribed"); + + if (!unsubscribeLabel?.id) { + log.warn("Unsubscribe label not found"); + }Optionally, you could
returnearly after the warning to avoid callinglabelMessagewith an undefined label id.apps/web/utils/actions/ai-rule.ts (1)
527-574: Add logger threading to maintain consistency.The
createEmailProvidercall (lines 535-538) doesn't include theloggerparameter, making it inconsistent with other actions in this file (runRulesAction,testAiCustomContentAction) and the PR's objective.Apply this diff to thread the logger:
-.action(async ({ ctx: { emailAccountId, provider } }) => { +.action(async ({ ctx: { emailAccountId, provider, logger } }) => { const emailAccount = await getEmailAccountWithAi({ emailAccountId }); if (!emailAccount) throw new SafeError("Email account not found"); const emailProvider = await createEmailProvider({ emailAccountId, provider, + logger, });
🧹 Nitpick comments (3)
apps/web/app/api/scheduled-actions/execute/route.ts (1)
93-97: Consider using request-scoped logger for better tracing.The code passes the module-scoped
loggertocreateEmailProvider. Since this handler is wrapped withwithErrormiddleware (line 21), which attaches a request-scoped logger withrequestIdand other context, consider usingrequest.loggerinstead for better request tracing.Apply this diff to use request-scoped logging:
const provider = await createEmailProvider({ emailAccountId: scheduledAction.emailAccountId, provider: scheduledAction.emailAccount.account.provider, - logger, + logger: request.logger, });apps/web/utils/email/microsoft.ts (1)
105-121: Structured logging across OutlookProvider methods is consistent; consider verbosity on hot pathsUsing
this.logger.info/warn/errorwith structured fields (e.g.threadId,messageId,senderEmail, filter/query parameters, error codes) gives good observability and keeps logs provider-scoped. The patterns ingetThread,getMessage,getSentMessages, label operations, pagination helpers, counting/checking reply helpers, history processing, and archive/signature helpers all look correct.If you find logs too chatty in production, you might consider downgrading some high-frequency
infologs (like those ingetMessagesWithPagination,checkIfReplySent, andcountReceivedMessages) totraceto reduce noise without losing debug detail.Also applies to: 148-159, 222-237, 340-390, 492-505, 507-551, 559-586, 672-697, 719-794, 934-951, 953-983, 1120-1143, 1197-1226, 1253-1261, 1279-1283, 1287-1296, 1318-1334, 1374-1406
apps/web/utils/email/google.ts (1)
246-268: GmailProvider logging is well‑structured; watch log volume in high‑traffic helpersThe new
this.logger.with(...)usage inarchiveMessage, bulk archive/trash helpers,labelMessage,draftEmail, reply/count helpers, and history/folder-related methods adds useful structured context (action name, IDs, sender, thresholds) and keeps Gmail logs scoped to the provider instance.As with Outlook, if production logs become noisy, it may be worth moving some very frequent
infologs (e.g. in bulk operations or count/check helpers) totracewhile keeping warnings/errors as-is.Also applies to: 270-290, 293-373, 375-486, 521-576, 586-615, 969-989, 992-1018, 1182-1203, 1231-1247
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/web/app/api/google/watch/route.ts(1 hunks)apps/web/app/api/google/webhook/process-history-item.ts(1 hunks)apps/web/app/api/outlook/webhook/process-history.ts(1 hunks)apps/web/app/api/resend/digest/route.ts(1 hunks)apps/web/app/api/scheduled-actions/execute/route.ts(1 hunks)apps/web/app/api/user/no-reply/route.ts(2 hunks)apps/web/app/api/user/rules/[id]/example/route.ts(3 hunks)apps/web/app/api/user/stats/day/route.ts(1 hunks)apps/web/utils/actions/ai-rule.ts(7 hunks)apps/web/utils/actions/assess.ts(3 hunks)apps/web/utils/actions/categorize.ts(2 hunks)apps/web/utils/actions/clean.ts(1 hunks)apps/web/utils/actions/cold-email.ts(3 hunks)apps/web/utils/actions/email-account.ts(3 hunks)apps/web/utils/actions/mail-bulk-action.ts(2 hunks)apps/web/utils/actions/mail.ts(10 hunks)apps/web/utils/actions/rule.ts(1 hunks)apps/web/utils/actions/stats.ts(1 hunks)apps/web/utils/actions/whitelist.ts(1 hunks)apps/web/utils/email/google.ts(13 hunks)apps/web/utils/email/microsoft.ts(24 hunks)apps/web/utils/email/provider.ts(1 hunks)apps/web/utils/email/watch-manager.ts(1 hunks)apps/web/utils/middleware.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/utils/actions/categorize.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-08T13:14:07.449Z
Learnt from: elie222
Repo: elie222/inbox-zero PR: 537
File: apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx:30-34
Timestamp: 2025-07-08T13:14:07.449Z
Learning: The clean onboarding page in apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx is intentionally Gmail-specific and should show an error for non-Google email accounts rather than attempting to support multiple providers.
Applied to files:
apps/web/app/api/google/webhook/process-history-item.tsapps/web/utils/email/google.tsapps/web/utils/actions/assess.ts
📚 Learning: 2025-07-17T04:19:57.099Z
Learnt from: edulelis
Repo: elie222/inbox-zero PR: 576
File: packages/resend/emails/digest.tsx:78-83
Timestamp: 2025-07-17T04:19:57.099Z
Learning: In packages/resend/emails/digest.tsx, the DigestEmailProps type uses `[key: string]: DigestItem[] | undefined | string | Date | undefined` instead of intersection types like `& Record<string, DigestItem[] | undefined>` due to implementation constraints. This was the initial implementation approach and cannot be changed to more restrictive typing.
Applied to files:
apps/web/app/api/resend/digest/route.ts
🧬 Code graph analysis (13)
apps/web/app/api/user/rules/[id]/example/route.ts (1)
apps/web/utils/logger.ts (1)
Logger(5-5)
apps/web/utils/email/google.ts (1)
apps/web/utils/logger.ts (2)
Logger(5-5)createScopedLogger(17-81)
apps/web/utils/actions/whitelist.ts (3)
apps/web/env.ts (1)
env(16-244)apps/web/utils/email/provider-types.ts (1)
isGoogleProvider(1-3)apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/utils/email/microsoft.ts (2)
apps/web/utils/logger.ts (2)
Logger(5-5)createScopedLogger(17-81)apps/web/utils/outlook/client.ts (1)
OutlookClient(19-80)
apps/web/utils/actions/cold-email.ts (2)
apps/web/utils/actions/cold-email.validation.ts (2)
markNotColdEmailBody(16-16)coldEmailBlockerBody(3-13)apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/app/api/user/stats/day/route.ts (1)
apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/utils/actions/assess.ts (1)
apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/utils/actions/mail-bulk-action.ts (1)
apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/app/api/user/no-reply/route.ts (2)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/utils/email/provider.ts (3)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/email/google.ts (1)
GmailProvider(76-1258)apps/web/utils/email/microsoft.ts (1)
OutlookProvider(70-1408)
apps/web/utils/actions/ai-rule.ts (5)
apps/web/utils/actions/ai-rule.validation.ts (2)
runRulesBody(8-13)testAiCustomContentBody(3-5)apps/web/utils/user/get.ts (1)
getEmailAccountWithAi(36-64)apps/web/utils/error.ts (1)
SafeError(59-69)apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)apps/web/utils/actions/rule.validation.ts (2)
saveRulesPromptBody(169-169)createRulesBody(172-172)
apps/web/utils/actions/email-account.ts (1)
apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)
apps/web/utils/actions/mail.ts (2)
apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)apps/web/utils/gmail/mail.ts (1)
sendEmailBody(22-37)
⏰ 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 (34)
apps/web/app/api/user/rules/[id]/example/route.ts (1)
7-7: LGTM!Logger is correctly threaded from
request.loggerthroughgetExamplestocreateEmailProvider, enabling request-scoped logging context.Also applies to: 15-15, 20-20, 32-32, 49-54
apps/web/utils/actions/email-account.ts (1)
40-40: LGTM!Logger is correctly extracted from action context and threaded to
createEmailProviderin bothanalyzePersonaActionandfetchSignaturesFromProviderAction.Also applies to: 61-61, 110-110, 114-114
apps/web/app/api/resend/digest/route.ts (1)
126-126: LGTM!Logger is correctly passed to
createEmailProvider, enabling request-scoped logging throughout the email provider operations.apps/web/app/api/outlook/webhook/process-history.ts (1)
52-52: LGTM!Logger is correctly threaded from the function parameter to
createEmailProvider, maintaining request context throughout the webhook processing flow.apps/web/app/api/google/watch/route.ts (1)
30-30: LGTM!Request logger is correctly passed to
createEmailProvider, ensuring request-scoped logging context is available throughout the email watching setup.apps/web/utils/actions/clean.ts (1)
54-54: LGTM!Logger is correctly extracted from action context (line 38) and threaded to
createEmailProvider, enabling proper request-scoped logging.apps/web/utils/actions/whitelist.ts (1)
11-11: LGTM!Logger is correctly extracted from action context and passed to
createEmailProvider, maintaining request-scoped logging context.Also applies to: 18-18
apps/web/utils/actions/rule.ts (1)
674-678: LGTM!The logger propagation through to
createEmailProvideris correct and consistent with the PR's objective of threading request-scoped loggers through the email provider creation flow.apps/web/app/api/user/stats/day/route.ts (1)
104-108: LGTM!Correctly propagates
request.loggerto the email provider creation, enabling request-scoped logging within the provider.apps/web/app/api/google/webhook/process-history-item.ts (1)
28-32: LGTM!The logger is properly forwarded from the function parameter to the email provider, maintaining consistent logging context throughout the webhook processing flow.
apps/web/utils/actions/stats.ts (1)
45-49: LGTM!The logger from the action context is correctly propagated to the email provider, enabling consistent logging throughout the email loading process.
apps/web/utils/middleware.ts (1)
290-299: LGTM! Improved encapsulation of provider logging context.The logger is now correctly forwarded from the email account request to both the provider creation and the resulting request. The removal of provider-specific context addition in the middleware (previously
logger.with({ provider })) is intentional—provider constructors now manage their own logging context, which improves encapsulation.apps/web/utils/actions/mail-bulk-action.ts (1)
15-23: LGTM!Both
bulkArchiveActionandbulkTrashActioncorrectly destructure the logger from the action context and propagate it to the email provider creation, maintaining consistent logging throughout the bulk operations.Also applies to: 41-49
apps/web/utils/email/provider.ts (1)
14-29: LGTM! Core change enabling logger propagation.This is the foundational change that enables request-scoped logging throughout the email provider flow. The optional
loggerparameter maintains backward compatibility while allowing all call sites to progressively adopt structured logging. The logger is correctly forwarded to bothGmailProviderandOutlookProviderconstructors.apps/web/utils/actions/assess.ts (1)
15-20: Logger propagation in assess actions looks correctBoth
assessActionandanalyzeWritingStyleActionnow pullloggerfromctxand pass it tocreateEmailProvider, aligning these actions with the request-scoped logging approach without changing functional behavior.Also applies to: 40-63
apps/web/utils/email/microsoft.ts (1)
37-38: Scoped logger initialization for OutlookProvider is soundThe optional
loggerparameter with fallback tocreateScopedLogger("outlook-provider").with({ provider: "microsoft" })cleanly guarantees a provider-scoped logger while preserving backwards compatibility for existingnew OutlookProvider(client)call sites.Also applies to: 70-80
apps/web/utils/actions/cold-email.ts (2)
19-31: markNotColdEmailAction: schema and logger wiring look goodSwitching to
.inputSchema(markNotColdEmailBody)and using{ ctx: { emailAccountId, provider, logger }, parsedInput: { sender } }is consistent with the safe-action pattern, and passingloggerintocreateEmailProvidercorrectly hooks this action into request-scoped logging.
98-133: testColdEmailAction correctly adopts inputSchema and propagates loggerUsing
.inputSchema(coldEmailBlockerBody)withparsedInputand threadingloggerintocreateEmailProviderintegrates this action with the new logging flow while keeping the cold-email evaluation logic unchanged.apps/web/utils/email/google.ts (1)
72-86: Scoped logger initialization for GmailProvider is correctThe optional
loggerparameter and fallback tocreateScopedLogger("gmail-provider").with({ provider: "google" })provide a consistent, provider-scoped logger without breaking existingnew GmailProvider(client)usages.apps/web/app/api/user/no-reply/route.ts (1)
5-6: Route now correctly threads request.logger into getNoReply/email providerAdding the
logger: Loggerparameter togetNoReply, passing it intocreateEmailProvider, and wiring it fromrequest.loggerin theGEThandler cleanly integrates this endpoint with the request-scoped logging system without altering the returnedNoReplyResponseshape.Also applies to: 13-24, 56-61
apps/web/utils/actions/ai-rule.ts (5)
32-106: LGTM! Logger properly threaded through the action.The migration to
.inputSchema()is correct, and the logger is properly extracted from context (line 37) and passed tocreateEmailProvider(line 50).
108-162: LGTM! Consistent logger threading.The action correctly uses
.inputSchema()and threads the logger from context (line 113) tocreateEmailProvider(line 123).
164-177: LGTM! API migration is correct.The migration to
.inputSchema()is appropriate. This action doesn't need logger threading as it only performs database operations.
193-430: LGTM! API migration is correct.The migration to
.inputSchema()is appropriate. The logger is already available in context (line 198) and used throughout the function for comprehensive logging.
432-517: LGTM! API migration is correct.The migration to
.inputSchema()is appropriate. The logger is already available in context (line 436) and used throughout for detailed logging.apps/web/utils/actions/mail.ts (9)
13-35: LGTM! Logger properly integrated.The migration to
.inputSchema()is correct, and the logger is properly extracted from context (line 20) and passed tocreateEmailProvider(line 26).
37-53: LGTM! Consistent logger threading.The migration to
.inputSchema()is correct, and the logger flows from context (line 42) tocreateEmailProvider(line 48).
55-71: LGTM! Proper logger integration.The API migration and logger threading are correctly implemented (context extraction on line 60, provider call on line 66).
73-99: LGTM! Consistent implementation.The migration to
.inputSchema()and logger threading are correctly implemented (context on line 84, provider on line 90).
101-125: LGTM! Logger properly threaded.The API migration and logger integration are correct (context on line 106, provider on line 112).
127-146: LGTM! Consistent logger threading.The migration to
.inputSchema()and logger integration are correctly implemented (context on line 132, provider on line 138).
148-166: LGTM! Proper logger integration.The API migration and logger threading are correctly implemented (context on line 155, provider on line 161).
168-213: LGTM! API migration is correct.The migration to
.inputSchema()is appropriate. This action doesn't need logger threading as it only performs database operations without creating an email provider.
215-234: LGTM! Logger properly integrated.The migration to
.inputSchema()and logger threading are correctly implemented (context on line 219, provider on line 223).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/web/utils/ai/actions.ts (1)
78-301: Consider adding logging to other action functions.Now that the logger infrastructure is in place, other action functions could benefit from similar logging patterns, especially for:
- Error cases when external API calls fail (archive, mark_spam, mark_read, move_folder)
- Success/failure logging for email operations (draft, reply, send_email, forward)
- Webhook call results (call_webhook)
This would improve observability and debugging capabilities across all action types.
For example, the archive function could be enhanced:
const archive: ActionFunction<Record<string, unknown>> = async ({ client, email, userEmail, + logger, }) => { + logger.info("Archiving thread", { threadId: email.threadId }); await client.archiveThread(email.threadId, userEmail); };apps/web/__tests__/ai-choose-args.test.ts (1)
6-6: Tests correctly inject a logger; consider reusing a single instanceImporting
createScopedLoggerand passingloggerintogetActionItemsWithAiArgsmatches the updated API and keeps tests aligned with production. If you want slightly cleaner tests and consistent context, you could define oneconst logger = createScopedLogger("test")in thedescribescope and reuse it across all invocations.Also applies to: 30-31, 54-55, 85-86, 116-117, 155-156, 198-199
apps/web/utils/webhook/process-history-item.ts (1)
14-24: Logger propagation here looks correct; consider adding a scoped logger for richer contextAdding
loggertoSharedProcessHistoryOptions, using it for all log lines, and passing it through torunRulesmakes this path consistent with the new logging pattern without changing behavior. For consistency with other modules (e.g.,choose-args,execute, scheduled-actions executor) and to avoid repeating identifiers in each log call, you might consider:const log = logger.with({ module: "process-history-item", emailAccountId, messageId, });and then using
log.*instead oflogger.*below.Also applies to: 36-43, 51-52, 55-56, 91-93, 96-98, 107-110, 119-121, 134-135, 160-162, 165-167, 185-196, 208-215
apps/web/utils/scheduled-actions/executor.test.ts (1)
5-5: Tests are aligned with the new logger and schema; minor duplication onlyThe tests now correctly pass a
LoggerintoexecuteScheduledAction, and the updatedexecutedRule.updatemock (addingmatchMetadata) matches the likely Prisma shape without altering behavior. To avoid recreating loggers repeatedly, you could optionally define a singleconst logger = createScopedLogger("test")in thedescribe("executeScheduledAction")block and reuse it across the three calls.Also applies to: 66-69, 102-114, 139-143, 156-160, 218-222, 234-237, 250-254
apps/web/utils/ai/choose-rule/choose-args.ts (1)
16-20: Scoped logger usage here is solid; you might enrich the contextAdding
logger: LoggertogetActionItemsWithAiArgsand derivinglog = logger.with({ module: "choose-args" })is consistent with the rest of the refactor and keeps draft-generation logs well-scoped. If you want even more useful traces, consider including identifiers likeemailAccountId,ruleId, andthreadIdin the.with(...)call so all subsequent logs in this function share that context by default.Also applies to: 21-35, 36-37, 47-51, 58-61, 63-67
apps/web/app/api/scheduled-actions/execute/route.ts (1)
5-5: Good: provider and executor now share the same logger; consider per-request scopingThreading
loggerinto bothcreateEmailProviderandexecuteScheduledActioncorrectly extends logging through the scheduled-action execution path. IfwithErroror other middleware attaches a request-scoped logger, you may later want to derive from that (e.g.,const log = request.logger.with({ module: "scheduled-actions-executor" })) instead of the module-level logger so QStash executions inherit request-level metadata automatically.Also applies to: 12-12, 93-97, 98-102
apps/web/utils/ai/choose-rule/execute.ts (1)
5-5: executeAct logging is well-scoped; consider passingloginto draft update helperInjecting
logger: LoggerintoexecuteAct, deriving a scopedlogwith rule/message context, and passinglogger: logintorunActionFunctiongives you nicely correlated logs for action execution and errors. For consistency, you might also passlograther than the baseloggerintoupdateExecutedActionWithDraftIdso any logs inside that helper carry the sameexecutedRuleId/messageIdcontext:- await updateExecutedActionWithDraftId({ - actionId: action.id, - draftId: actionResult.draftId, - logger, - }); + await updateExecutedActionWithDraftId({ + actionId: action.id, + draftId: actionResult.draftId, + logger: log, + });Also applies to: 10-11, 16-24, 31-39, 43-52, 54-60, 62-68, 71-78
apps/web/utils/ai/choose-rule/match-rules.ts (1)
18-19: Module-scoped logger usage is consistent; consider avoiding nestedwith({ module })The pattern of accepting a request-level
logger, then deriving a module-scoped logger vialogger.with({ module: MODULE })infindMatchingRules,matchesStaticRule, andfilterConversationStatusRulesis consistent and keeps logs well-tagged.One minor refinement: because some callers (e.g.
runRules) already pass in a logger that may have{ module: "ai/choose-rule" }, andmatchesStaticRule/filterConversationStatusRulescallwith({ module: MODULE })again, you can end up nesting or overwriting themodulefield multiple times.This is harmless but could be simplified by:
- deriving a module-scoped
logonce infindMatchingRules, and- passing that
logdown to helpers instead of the baselogger, so they don’t need to call.with({ module: MODULE })again.Also applies to: 35-36, 47-54, 129-143, 215-219, 233-239, 370-377, 380-387, 546-555, 589-612
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/__tests__/ai-choose-args.test.ts(7 hunks)apps/web/app/api/scheduled-actions/execute/route.ts(1 hunks)apps/web/utils/actions/ai-rule.ts(9 hunks)apps/web/utils/ai/actions.ts(5 hunks)apps/web/utils/ai/choose-rule/choose-args.ts(3 hunks)apps/web/utils/ai/choose-rule/execute.ts(5 hunks)apps/web/utils/ai/choose-rule/match-rules.ts(16 hunks)apps/web/utils/ai/choose-rule/run-rules.ts(11 hunks)apps/web/utils/scheduled-actions/executor.test.ts(5 hunks)apps/web/utils/scheduled-actions/executor.ts(13 hunks)apps/web/utils/webhook/process-history-item.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
apps/web/__tests__/ai-choose-args.test.ts (1)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)
apps/web/utils/ai/choose-rule/choose-args.ts (5)
apps/web/utils/types.ts (2)
ParsedMessage(51-73)RuleWithActions(16-18)apps/web/utils/llms/types.ts (1)
EmailAccountWithAI(10-30)apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/llms/model.ts (1)
ModelType(17-17)apps/web/utils/logger.ts (1)
Logger(5-5)
apps/web/utils/ai/choose-rule/run-rules.ts (6)
apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/types.ts (2)
ParsedMessage(51-73)RuleWithActions(16-18)apps/web/utils/llms/types.ts (1)
EmailAccountWithAI(10-30)apps/web/utils/llms/model.ts (1)
ModelType(17-17)apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/ai/choose-rule/choose-args.ts (1)
getActionItemsWithAiArgs(21-86)
apps/web/utils/actions/ai-rule.ts (5)
apps/web/utils/actions/ai-rule.validation.ts (2)
runRulesBody(8-13)testAiCustomContentBody(3-5)apps/web/utils/user/get.ts (1)
getEmailAccountWithAi(36-64)apps/web/utils/error.ts (1)
SafeError(59-69)apps/web/utils/email/provider.ts (1)
createEmailProvider(14-32)apps/web/utils/actions/rule.validation.ts (2)
saveRulesPromptBody(169-169)createRulesBody(172-172)
apps/web/utils/scheduled-actions/executor.test.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/scheduled-actions/executor.ts (1)
executeScheduledAction(18-94)
apps/web/utils/scheduled-actions/executor.ts (3)
apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/ai/types.ts (2)
EmailForAction(4-15)ActionItem(17-30)
apps/web/utils/ai/choose-rule/execute.ts (3)
apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/types.ts (1)
ParsedMessage(51-73)apps/web/utils/logger.ts (1)
Logger(5-5)
apps/web/utils/ai/choose-rule/match-rules.ts (5)
apps/web/utils/types.ts (2)
RuleWithActions(16-18)ParsedMessage(51-73)apps/web/utils/llms/types.ts (1)
EmailAccountWithAI(10-30)apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/llms/model.ts (1)
ModelType(17-17)apps/web/utils/logger.ts (1)
Logger(5-5)
apps/web/app/api/scheduled-actions/execute/route.ts (1)
apps/web/utils/scheduled-actions/executor.ts (1)
executeScheduledAction(18-94)
apps/web/utils/ai/actions.ts (1)
apps/web/utils/logger.ts (1)
Logger(5-5)
🪛 GitHub Actions: Run Tests
apps/web/utils/ai/choose-rule/match-rules.ts
[error] 459-459: TypeError: Cannot read properties of undefined (reading 'with') in logger usage inside matchesStaticRule.
🪛 GitHub Check: test
apps/web/utils/ai/choose-rule/match-rules.ts
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match parentheses in subject
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:150:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match exact Creator Message subject
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:141:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match Creator Message subject pattern
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:128:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match @domain.com
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:117:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match body content with wildcard
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:108:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should return false when no conditions are provided
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:98:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should handle invalid regex patterns gracefully
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:89:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should handle multiple wildcards in pattern
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:80:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should not match when wildcard pattern doesn't match domain
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:71:12
[failure] 459-459: utils/ai/choose-rule/match-rules.test.ts > matchesStaticRule > should match wildcard pattern at start of email
TypeError: Cannot read properties of undefined (reading 'with')
❯ Module.matchesStaticRule utils/ai/choose-rule/match-rules.ts:459:22
❯ utils/ai/choose-rule/match-rules.test.ts:62:12
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (7)
apps/web/utils/ai/actions.ts (4)
2-2: LGTM! Clean logger infrastructure setup.The logger imports and MODULE constant are well-structured for enabling request-scoped logging throughout the action execution flow.
Also applies to: 11-11
13-33: LGTM! Logger threading is properly typed.The addition of
logger: Loggerto both theActionFunctiontype andrunActionFunctionoptions ensures type-safe logger propagation through the entire action execution pipeline.
34-49: LGTM! Excellent scoped logging implementation.The logger is properly scoped with module context and used for both high-level action tracking and detailed tracing. Passing the scoped logger to individual actions maintains consistent context throughout the execution flow.
86-126: LGTM! Good logging practices in the label action.The label function demonstrates excellent use of the logger with appropriate error and info logging for template validation, label lookup, and creation operations.
apps/web/utils/actions/ai-rule.ts (2)
34-35: Schema →inputSchemamigration looks correctUsing the dedicated
runRulesBody,testAiCustomContentBody,saveRulesPromptBody,createRulesBody, and an emptyz.object({})forgenerateRulesPromptActionkeeps validation centralized and consistent with the validation modules. No issues from a typing or runtime perspective.Also applies to: 111-116, 168-168, 197-197, 436-436, 531-531
37-41: Logger propagation through AI rule actions is sound—confirm ctx.logger is always presentThreading the request-scoped logger from
ctxintocreateEmailProviderandrunRules(and addingmessageId/threadIdcontext inrunRulesAction) is a good improvement for observability.Both
runRulesActionandtestAiCustomContentActionnow assumectx.loggeris a concrete logger instance (e.g.ctxLogger.with(...)), so any usage of these actions must ensure the action context always includes a valid logger.Please confirm that your
actionClientsetup always injects a non-undefinedloggerintoctxfor these actions (including tests and any non-HTTP callers). If there are edge cases without a logger, consider either:
- providing a default no-op/logger at context creation, or
- making the logger in ctx required at the type level so misuse is caught at compile time.
Also applies to: 47-51, 95-102, 121-125, 136-140
apps/web/utils/scheduled-actions/executor.ts (1)
7-8: Scheduled action executor logger refactor verified—all callers updated correctlyThe change to require
LoggerinexecuteScheduledActionhas been properly propagated to all callers:
- Test calls (3 × executor.test.ts): Each passes
createScopedLogger("test")as the logger- Production call (route.ts:98): Passes
loggerfrom the route handler's scopeAll four call sites supply the Logger instance as the 3rd parameter, consistent with the function signature and your request-scoped logging pattern. Semantics of status updates and error handling remain unchanged; only logging is improved with module and action context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/ai/choose-rule/run-rules.test.ts (1)
265-265: Consider reusing the module-level logger for consistency.This test creates a new logger inline, while other tests in this file use the existing
loggerconstant defined at line 19. Since both use the same scope, consider using the existing logger for consistency.Apply this diff:
- const result = limitDraftEmailActions(matches, createScopedLogger("test")); + const result = limitDraftEmailActions(matches, logger);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/__tests__/ai-choose-args.test.ts(7 hunks)apps/web/app/api/user/rules/[id]/example/controller.ts(4 hunks)apps/web/app/api/user/rules/[id]/example/route.ts(3 hunks)apps/web/utils/ai/choose-rule/match-rules.test.ts(103 hunks)apps/web/utils/ai/choose-rule/run-rules.test.ts(15 hunks)apps/web/utils/scheduled-actions/executor.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/tests/ai-choose-args.test.ts
- apps/web/utils/scheduled-actions/executor.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/app/api/user/rules/[id]/example/route.ts (2)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/app/api/user/rules/[id]/example/controller.ts (1)
fetchExampleMessages(15-50)
apps/web/utils/ai/choose-rule/run-rules.test.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/ai/choose-rule/run-rules.ts (1)
limitDraftEmailActions(555-613)
apps/web/app/api/user/rules/[id]/example/controller.ts (4)
apps/web/app/(app)/[emailAccountId]/assistant/rule/[ruleId]/examples/types.ts (1)
RuleWithGroup(4-6)apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/ai/choose-rule/match-rules.ts (1)
matchesStaticRule(454-509)
apps/web/utils/ai/choose-rule/match-rules.test.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/ai/choose-rule/match-rules.ts (2)
matchesStaticRule(454-509)evaluateRuleConditions(255-310)
⏰ 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: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
🔇 Additional comments (13)
apps/web/utils/ai/choose-rule/run-rules.test.ts (2)
17-19: LGTM! Logger setup is clean.The import and logger initialization follow best practices for test files. Creating the logger at module scope allows for reuse across all test cases.
65-72: LGTM! Logger correctly threaded through test calls.All invocations of
ensureConversationRuleContinuityconsistently pass the logger, aligning with the updated function signature.apps/web/utils/ai/choose-rule/match-rules.test.ts (2)
31-36: LGTM! Logger setup follows best practices.The logger is properly imported and initialized at module scope for reuse across test cases.
65-2592: LGTM! Comprehensive and consistent logger integration.All test cases have been updated to pass the logger parameter to the refactored functions (
matchesStaticRule,findMatchingRules,evaluateRuleConditions,filterConversationStatusRules). The changes are thorough, consistent, and align with the PR's objective to thread request-scoped loggers throughout the codebase.apps/web/app/api/user/rules/[id]/example/route.ts (5)
7-7: LGTM: Logger type import added correctly.The type-only import is appropriate for the function signature annotations.
11-21: LGTM: Logger parameter added to function signature.The logger parameter is correctly typed and will enable request-scoped logging throughout the example fetching flow.
22-25: Good addition: Prisma include ensures necessary data is loaded.The
includeclause loads the related group and items, which are required byfetchExampleMessageswhen processing group rules (lines 40-44 in controller.ts).
29-39: LGTM: Logger correctly propagated through the call chain.Both
createEmailProviderandfetchExampleMessagesreceive the logger, maintaining consistent logging context throughout the example retrieval flow.
44-62: LGTM: Request logger properly threaded from middleware.The route correctly passes
request.loggerfrom thewithEmailProvidermiddleware intogetExamples, establishing the request-scoped logging context.apps/web/app/api/user/rules/[id]/example/controller.ts (4)
13-13: LGTM: Logger type import added correctly.
15-19: LGTM: Logger parameter added to public API.The
fetchExampleMessagesfunction signature is correctly updated to accept the logger, matching the call site in route.ts.
37-37: LGTM: Logger properly threaded to internal function.The logger is correctly passed from
fetchExampleMessages(line 37) to the internalfetchStaticExampleMessagesfunction, maintaining logging context for static rule processing.Also applies to: 52-56
75-77: LGTM: Logger correctly passed to rule matching logic.The logger is properly passed to
matchesStaticRule, which enables error logging for invalid regex patterns (as implemented in the match-rules utility).
1 participant
Summary by CodeRabbit
Chores
Refactor
Style
Tests