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. WalkthroughThis PR updates error logging across 13 TypeScript files and version.txt by replacing formatted error message strings with raw error objects in logger calls. No control flow changes occur; only the logged error payloads are modified to preserve richer debugging information. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
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}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| error: errorMessage, | ||
| error, | ||
| }); | ||
| await cleanupInvalidTokens({ |
There was a problem hiding this comment.
cleanupInvalidTokens(...) is awaited inside the catch. If it throws, watchEmails rejects instead of returning { success: false, error }. Consider wrapping it in its own try/catch (and log) so the function preserves its return contract.
- await cleanupInvalidTokens({
- emailAccountId,
- reason: isInvalidGrant ? "invalid_grant" : "insufficient_permissions",
- logger,
- });
+ try {
+ await cleanupInvalidTokens({
+ emailAccountId,
+ reason: isInvalidGrant ? "invalid_grant" : "insufficient_permissions",
+ logger,
+ });
+ } catch (cleanupError) {
+ logger.error("Failed to cleanup invalid tokens", {
+ emailAccountId,
+ providerName: provider.name,
+ error: cleanupError,
+ });
+ captureException(cleanupError);
+ }🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
1 participant
Adjust logging to record raw error objects across web app services and remove
errorMessagefromapps/web/app/api/user/stats/newsletters/route.ts:getNewsletterCountsReplace string error messages with raw error objects in warnings and errors across API handlers, actions, assistants, and providers, and remove the
errorMessagefield from one structured log; bump version to v2.21.49.📍Where to Start
Start with the logging change in
getNewsletterCountsin route.ts, then scan similar logger updates in action and assistant utilities.📊 Macroscope summarized cef1f01. 13 files reviewed, 25 issues evaluated, 19 issues filtered, 1 comment posted
🗂️ Filtered Issues
apps/web/app/api/user/stats/newsletters/route.ts — 0 comments posted, 1 evaluated, 1 filtered
limitClauseis built via string interpolation without validating thatoptions.limitis non‑negative. A negative value (e.g.,?limit=-1) passes Zod coercion and is truthy, yieldingLIMIT -1, which can cause a runtime SQL error in PostgreSQL (LIMIT must not be negative). Validate and clamp to a non‑negative integer (and optionally cap to a sane maximum) before constructing the clause. [ Out of scope ]apps/web/app/api/watch/controller.ts — 1 comment posted, 3 evaluated, 2 filtered
DatefromcreateManagedOutlookSubscriptionbut does not persist it to the database (e.g.,prisma.emailAccount.update({ watchEmailsExpirationDate })). This creates contract/side‑effect asymmetry with the non‑Microsoft path, which explicitly updateswatchEmailsExpirationDate. IfcreateManagedOutlookSubscriptiondoes not itself persist the expiration (or subscription id), the database remains stale, making downstream logic that relies on DB state (e.g., cleanup/scheduling/unwatch flows) believe the account is not being watched. [ Out of scope ]result.expirationDateis persisted. Ifprovider.watchEmails()also returns a subscription identifier (commonly required to unwatch), it is silently dropped with no explicit rejection or comment. This risks making laterunwatchoperations impossible if they rely on a storedwatchEmailsSubscriptionId, and it violates the requirement to avoid silent data loss at data‑conversion boundaries. [ Out of scope ]apps/web/utils/actions/ai-rule.ts — 0 comments posted, 4 evaluated, 3 filtered
emailAccount.rulesPrompt. This contradicts the documented flow step to update the user's prompt and causes the user's edited prompt text to be discarded. The problematic early return occurs after computing the diff when!diff.addedRules.length && !diff.editedRules.length && !diff.removedRules.length, skipping the DB update ofrulesPrompteven thougholdPromptFile !== rulesPrompt. [ Out of scope ]emailAccount.rulesPromptand reports counts based on intended operations rather than confirmed successes, leading to inconsistent state and misleading results when some operations fail but are caught. Specifically: (1) In the create loop, errors other than duplicates are caught and only logged, yetcreatedRulesis still reported asaddedRules?.lengthand the prompt is updated; (2) In the delete path, ifdeleteRulethrows a non-NotFound error, it is caught and the loop continues,removeRulesCountis incremented, and the prompt is later updated. This can persist a prompt that implies changes that did not actually apply, and overstatecreatedRules/removedRulesin the returned response. [ Low confidence ]errorobject tologger.errorinstead of a stringified/serialized representation. If the logger expects JSON-serializable data, logging anErrorwith circular references or large nested properties can cause serialization failures or drop useful context, potentially throwing during logging or truncating logs. Previously it loggederror instanceof Error ? error.message : String(error), which guaranteed serializable output. [ Code style ]apps/web/utils/actions/report.ts — 0 comments posted, 1 evaluated, 1 filtered
fetchGmailLabels, each user label is processed with an individualgmail.users.labels.getcall inside aPromise.allover the entireuserLabelsset. With many labels, this can create a burst of concurrent requests, causing 429s or quota errors. Although per-label failures are caught and zeroed, this can degrade accuracy (counts all zero) and produce noisy logs. Use bounded concurrency (e.g., a limiter) or batch/sequential processing to keep request rates within API limits. [ Low confidence ]apps/web/utils/ai/assistant/process-user-request.ts — 0 comments posted, 2 evaluated, 1 filtered
messagesis empty: the code unconditionally accessesmessages[messages.length - 1].roleat line 46. Ifmessages.length === 0,messages[messages.length - 1]isundefinedand reading.rolethrows a runtime TypeError. The function signature allows anymessages: { role: "assistant" | "user"; content: string }[](including an empty array). Add a guard for empty arrays before indexing, or enforce non-empty via validation. [ Out of scope ]apps/web/utils/ai/mcp/mcp-tools.ts — 0 comments posted, 3 evaluated, 3 filtered
toolsByIntegrationis keyed byintegration.id, and each iteration callstoolsByIntegration.set(integration.id, ...). If there are multiplemcpConnectionrows for the sameintegration.id, later entries will overwrite earlier ones, dropping tools from prior connections without warning. Consider merging tool sets per integration or guarding against duplicates. [ Out of scope ]errorobject to the logger may cause serialization/logging to throw (e.g., if the logger JSON-serializes the meta and the error contains circular references). This exception would be thrown inside the inner catch block, potentially escaping and triggering the outer catch; combined with the current outer error path, this can lead to leaked clients and lost processing of other integrations. [ Low confidence ]clients, the outercatchreturns a cleanup that is a no-op, so those clients will never be closed. This can happen if any code after client creation throws outside the inner per-integration try/catch (e.g., a logging failure or an unexpected error during merging). Ensure previously created clients are closed on the outer error path. [ Out of scope ]apps/web/utils/ai/report/fetch.ts — 0 comments posted, 2 evaluated, 2 filtered
errorobject vialogger.warn("Failed to process draft:", { error });. If the logger attempts to serialize contextual objects (e.g., JSON.stringify), a non-serializable or circularerrorcan causelogger.warnto throw. Because this call is inside thetryblock for the whole function, a thrown log here would escape the inner catch and abort the entire outertry, falling into the outercatchand potentially returning an empty list — losing any templates accumulated so far. Previously, the code stringifiederrorsafely. Consider logging a safe projection (e.g.,error instanceof Error ? { message: error.message, stack: error.stack } : { error: String(error) }) to avoid logger-induced exceptions. [ Low confidence ]errorobject vialogger.warn("Failed to fetch email templates:", { error });. If the logger serializes the context and theerroris non-serializable or circular,logger.warnmay throw, causing the function to reject instead of returning[]as intended by the catch. Previously, the code logged a stringified/normalized error, avoiding this risk. Use a safe projection of the error to ensure logging cannot throw and the function reliably returns an empty array on failure. [ Low confidence ]apps/web/utils/email/microsoft.ts — 0 comments posted, 2 evaluated, 1 filtered
message.headers.fromwithout verifyingmessage.headersexists. If anymessagelacks aheadersobject,message.headers.fromwill throw. Add a guard (e.g., checkmessage.headersandmessage.headers.fromor use optional chaining) before constructing the return value. [ Out of scope ]apps/web/utils/user/merge-premium.ts — 0 comments posted, 3 evaluated, 2 filtered
isOnHigherTier(targetTier, sourceTier)is called when both users have apremiumId(branch starting atif (sourceUser.premiumId && targetUser.premiumId)), but there is no explicit check thatsourceUser.premiumandtargetUser.premiumare non-null. If apremiumIdexists but the relatedpremiumrecord is not loaded/resolvable,sourceTierortargetTiermay beundefined, potentially causingisOnHigherTierto throw or mis-evaluate. Add explicit null checks (or defaulting) before invokingisOnHigherTier. [ Low confidence ]prisma.premium.update({ data: { admins: { connect: { id: targetUserId }}}})and conditionallyprisma.user.update({ data: { premiumAdminId: sourceUser.premiumAdminId }}). Without a transaction and with parallel execution, failure of either can leave an inconsistent state (e.g.,premiumAdminIdpoints to an admin group the user is not actually connected to as admin). Execute these updates within a singleprisma.$transaction([...])to ensure consistency, and consider ordering or idempotency checks if needed. [ Low confidence ]apps/web/utils/webhook/process-history-item.ts — 0 comments posted, 4 evaluated, 3 filtered
markMessageAsProcessing({ userEmail, messageId })sets a processing marker, but there is no corresponding clear/unmark on any success or early-return path, nor in afinally. If the marker is not TTL-based, any early return (e.g., ignored sender, not-inbox, assistant paths) or thrown error will leave the message permanently marked as processing, causing future attempts to skip processing (isFree === false). Add afinallythat reliably clears the marker on all exit paths, or ensure the helper uses a short TTL. [ Low confidence ]processAssistantEmail(...)is returned withoutawait, so any rejection thrown by that promise will bypass the surroundingtry/catchand skip the provider-specific not-found handling and error logging. This creates inconsistent error handling compared to other paths (e.g.,handleOutboundMessageis awaited) and can lead to unhandled rejection at the call site. Replacereturn processAssistantEmail(...)withawait processAssistantEmail(...)followed byreturn;to keep errors within thetry/catchscope. [ Low confidence ]extractEmailAddress(parsedMessage.headers.from)result is used inprisma.newsletter.findUnique({ where: { email_emailAccountId: { email: sender, emailAccountId } } })without verifyingsenderis a non-empty string. Ifsenderisundefined, Prisma will throw at runtime. Add a guard before querying or skip categorization when the email cannot be extracted. [ Low confidence ]Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.