Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. WalkthroughThreads a Logger through AI action handlers, label management, and reply-tracker utilities; replaces internal scoped logger creation with injected logger, adds safer error-inspection helpers, updates Gmail 404 detection, and bumps version to v2.20.20. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant AI as AI action / rule runner
participant LabelSrv as label.server::labelMessageAndSync
Note right of AI `#D5E8D4`: Logger now threaded\nas parameter
Caller->>AI: trigger LABEL / matched rule (includes logger)
AI->>LabelSrv: labelMessageAndSync(..., logger)
LabelSrv-->>AI: result / errors (uses injected logger)
AI-->>Caller: continues flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/web/utils/reply-tracker/label-helpers.test.ts (1)
⏰ 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)
🔇 Additional comments (2)
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| err.error?.status ?? | ||
| err.error?.code; | ||
|
|
||
| return statusCode === 404; |
There was a problem hiding this comment.
statusCode can be a string, so strict === 404 may miss not-found cases. Consider normalizing the value to a number before comparing so getDraft returns null consistently.
- return statusCode === 404;
+ const code = typeof statusCode === "string" ? Number(statusCode) : statusCode;
+ return code === 404;🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
![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
🧹 Nitpick comments (3)
apps/web/utils/gmail/draft.ts (1)
30-47: Consider the function naming overlap.The
isNotFoundErrorfunction is well-implemented and handles multiple Gmail error shapes correctly. However, note that a function with the same name exists inapps/web/utils/prisma-helpers.ts(lines 13-18) for Prisma errors. While they're in different files and handle different error types, consider whether this could cause confusion in the future.The current implementation is acceptable since:
- They handle different error domains (Gmail API vs Prisma)
- They're in separate modules with clear contexts
- The Gmail-specific checks are appropriate here
apps/web/utils/logger.ts (1)
148-154: Optional: Remove redundanthasOwncheck.The
Object.keys(error)on line 149 already returns only own enumerable properties, making theObject.hasOwn(error, key)check on line 150 redundant (though harmless). You could simplify to:if (isRecord(error)) { for (const key of Object.keys(error)) { - if (Object.hasOwn(error, key)) { - serialized[key] = error[key]; - } + serialized[key] = error[key]; } }However, the defensive check doesn't hurt and may prevent issues if the code evolves.
apps/web/utils/label.server.ts (1)
19-19: Dependency injection implemented correctly.The logger parameter is properly threaded through the function signature and used to create a scoped logger with the appropriate context. The implementation maintains the existing logging behavior while enabling consistent context propagation.
The parameter aliasing pattern (
logger: log→const logger = log.with()) is functional but could be clearer. Consider either:
- Using consistent naming:
loggerparameter →const scopedLogger = logger.with(...)- Or more descriptive names:
baseLogger→const logger = baseLogger.with(...)However, if this pattern is intentional and consistent across the codebase (per the PR summary), maintaining consistency may be preferable.
Also applies to: 26-26, 28-28
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/utils/ai/actions.ts(1 hunks)apps/web/utils/ai/choose-rule/run-rules.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)apps/web/utils/gmail/draft.ts(1 hunks)apps/web/utils/label.server.ts(2 hunks)apps/web/utils/logger.ts(3 hunks)apps/web/utils/reply-tracker/label-helpers.ts(5 hunks)apps/web/utils/reply-tracker/outbound.ts(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/utils/gmail/draft.ts (2)
apps/web/utils/prisma-helpers.ts (1)
isNotFoundError(14-19)apps/web/utils/error.ts (1)
isGmailError(25-34)
apps/web/utils/reply-tracker/label-helpers.ts (3)
apps/web/utils/reply-tracker/conversation-status-config.ts (1)
ConversationStatus(10-14)apps/web/utils/email/types.ts (2)
EmailProvider(45-250)EmailLabel(14-25)apps/web/utils/logger.ts (1)
Logger(5-5)
apps/web/utils/label.server.ts (2)
apps/web/utils/email/types.ts (1)
EmailProvider(45-250)apps/web/utils/logger.ts (1)
Logger(5-5)
🪛 GitHub Actions: Run Tests
apps/web/utils/reply-tracker/label-helpers.ts
[error] 141-141: Cannot read properties of undefined (reading 'error') in catch block while applying thread status label. Failed to log error details: 'Failed to apply thread status label'.
⏰ 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: Review for correctness
🔇 Additional comments (13)
version.txt (1)
1-1: LGTM!Version bump is appropriate for this refactoring PR.
apps/web/utils/ai/actions.ts (1)
126-126: LGTM!Logger propagation to
labelMessageAndSyncis correct and consistent with the PR's logging refactor.apps/web/utils/gmail/draft.ts (1)
22-25: LGTM!The refactored error handling with
isNotFoundErrorimproves clarity and logging.apps/web/utils/ai/choose-rule/run-rules.ts (1)
331-331: LGTM!Logger propagation to
removeConflictingThreadStatusLabelsis correct and improves observability of the conversation status tracking flow.apps/web/utils/assistant/process-assistant-email.ts (1)
273-273: LGTM!Logger propagation to
labelMessageAndSyncis correct and maintains consistency with the PR's logging refactor.apps/web/utils/logger.ts (2)
129-129: LGTM!Using
getSimpleErrorMessagecentralizes error message extraction and improves consistency.
182-220: LGTM!The new error handling helpers are well-designed and provide safer type guards for error message extraction. The logic correctly handles multiple error shapes (string, Error instance, objects with message, nested errors).
apps/web/utils/reply-tracker/outbound.ts (1)
85-85: LGTM!Logger propagation to
applyThreadStatusLabelis correct and enables consistent logging in the outbound reply tracking flow.apps/web/utils/reply-tracker/label-helpers.ts (4)
2-2: LGTM!The transition from internal logger creation to dependency injection is correct. The logger parameter is properly typed and will be propagated through all function calls.
Also applies to: 27-27, 35-35
65-79: LGTM!Logger usage in
removeConflictingThreadStatusLabelsis correct, with appropriate info and error logging.
94-94: LGTM!Logger propagation through
applyThreadStatusLabeland its internal calls is correct. The logger is properly threaded throughlabelMessageAndSyncandremoveConflictingThreadStatusLabels.Regarding the pipeline failure about "Cannot read properties of undefined (reading 'error')": The code in this file appears correct. The
loggerparameter is required and properly typed, andtargetLabelis always an object (fromdbLabels). The failure might indicate an issue with how this function is being called elsewhere, but the implementation here is sound.Also applies to: 101-101, 139-139, 157-157, 162-162
126-131: Error logging is correct.Both error logging locations properly use the logger parameter and include relevant context. The
targetLabelvariable is guaranteed to be an object at these points (sourced fromdbLabels), so accessing its properties is safe.Also applies to: 140-146
apps/web/utils/label.server.ts (1)
2-2: LGTM!Good use of type-only import for the Logger type, which aids tree-shaking and clarifies that this is used only for type checking.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Adjust logging by passing scoped
Loggerthroughapps/web/utils/label.server.ts::labelMessageAndSyncand related label helpers, and broaden 404 detection inapps/web/utils/gmail/draft.ts::getDraftwithisNotFoundErrorand info loggingPropagate a provided
Loggerinto label operations and rule handlers, remove internal logger creation, and update Gmail draft retrieval to useisNotFoundErrorfor 404-shaped errors with aninfolog when returningnull. Error formatting utilities are refactored to usegetSimpleErrorMessageandObject.keys-based serialization. Version bumps to v2.20.20.📍Where to Start
Start with the updated logger signature in
labelMessageAndSyncin apps/web/utils/label.server.ts, then follow its call sites in apps/web/utils/ai/actions.ts, apps/web/utils/reply-tracker/label-helpers.ts, and the Gmail draft changes in apps/web/utils/gmail/draft.ts.Macroscope summarized 8bbdd2c.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.