Skip to content

Comments

Staging#702

Merged
MrgSub merged 50 commits intomainfrom
staging
Apr 17, 2025
Merged

Staging#702
MrgSub merged 50 commits intomainfrom
staging

Conversation

@MrgSub
Copy link
Collaborator

@MrgSub MrgSub commented Apr 17, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of unauthorized access across multiple features by introducing a consistent, user-friendly sign-out and redirect flow instead of generic error messages.
    • Enhanced error feedback on the login screen for unauthorized access from third-party providers.
    • Reduced unnecessary error logging and improved error messaging in several areas.
  • Refactor

    • Standardized error handling by delegating unauthorized access responses to a centralized utility function.
    • Minor formatting and style adjustments in various components for consistency.
  • New Features

    • Added a centralized utility to gracefully handle unauthorized access, ensuring users are securely signed out and redirected when necessary.

Muhammad-Owais-Warsi and others added 30 commits April 5, 2025 13:05
- refactor hotkeys
- add hotkey recorder
- added route for hotkeys
- added table for hotkeys
- indexdb for hotkeys (synced with db)
@vercel
Copy link

vercel bot commented Apr 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
0 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2025 8:21pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 17, 2025

Walkthrough

This set of changes standardizes unauthorized access handling across multiple mail app modules by introducing and adopting a centralized utility function, throwUnauthorizedGracefully. Instead of directly throwing errors upon unauthorized access, affected functions now call this utility, which logs the event, signs out the user, and redirects to the login page with an error indicator. Several files update their import statements, error handling logic, and minor formatting. Additionally, the login client UI now displays a toast message for an "unauthorized" error, and some error logging is improved or wrapped in try-catch blocks.

Changes

File(s) Change Summary
apps/mail/app/api/utils.ts Added throwUnauthorizedGracefully function for standardized unauthorized handling. Updated getAuthenticatedUserId to use this utility instead of throwing errors directly.
apps/mail/actions/ai-reply.ts
apps/mail/actions/ai.ts
apps/mail/actions/brain.ts
apps/mail/actions/connections.ts
apps/mail/actions/getSummary.ts
apps/mail/actions/send.ts
apps/mail/actions/utils.ts
apps/mail/app/api/notes/index.ts
Replaced direct error throws on unauthorized access with calls to throwUnauthorizedGracefully. Updated imports and minor formatting.
apps/mail/actions/mail.ts Modified getMails to call throwUnauthorizedGracefully() and return empty array on fatal errors instead of re-throwing.
apps/mail/actions/drafts.ts Updated getDrafts to call throwUnauthorizedGracefully() on error instead of re-throwing. Minor stylistic changes in createDraft and getDraft.
apps/mail/app/(auth)/login/login-client.tsx Added UI toast for "unauthorized" error in login flow.
apps/mail/hooks/use-drafts.ts Reordered imports and commented out some properties in mapped draft objects.
apps/mail/lib/auth.ts Improved error logging and wrapped redirect in try-catch for robustness.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AppFunction
    participant throwUnauthorizedGracefully
    participant AuthAPI
    participant Router

    User->>AppFunction: Make request (unauthorized)
    AppFunction->>throwUnauthorizedGracefully: Call utility
    throwUnauthorizedGracefully->>AuthAPI: Sign out user
    throwUnauthorizedGracefully->>Router: Redirect to /login?error=unauthorized
    Router-->>User: Show login page with error
Loading

Possibly related PRs

  • Mail-0/Zero#564: Also modifies login-client to handle error cases with toast messages.
  • Mail-0/Zero#534: Changes error handling and prompt customization in ai-reply.ts, affecting the same function as this PR.
  • Mail-0/Zero#526: Introduces the initial implementation of ai-reply.ts, which is further refined in this PR.

Poem

In the warren of code, where errors once flew,
A gentle paw guides what users now do.
No more abrupt jumps, just a graceful retreat—
Unauthorized bunnies land softly on their feet.
With a toast and a hop, we redirect the way,
Keeping our burrow safe, every single day.
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
apps/mail/actions/ai-reply.ts (1)

25-25: Simplified content processing.

The variable assignment has been simplified to directly use threadContent without any transformation. Note that the comment on line 24 about "aggressive content reduction" is now misleading since no reduction is happening.

// Use a more aggressive content reduction approach
-const processedContent = threadContent;
+const processedContent = threadContent; // No reduction applied
apps/mail/actions/getSummary.ts (1)

38-38: Consider keeping error logging.

Error logging has been commented out. While reducing console noise is good, completely removing error logging might make debugging more difficult. Consider keeping the logging but at a lower level (like debug) or adding more context about why this error is considered non-critical.

-    // console.error('Error getting summary:', error);
+    console.debug('Non-critical error getting summary:', error);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab4f90d and 45a35d1.

📒 Files selected for processing (12)
  • apps/mail/actions/ai-reply.ts (4 hunks)
  • apps/mail/actions/ai.ts (3 hunks)
  • apps/mail/actions/brain.ts (1 hunks)
  • apps/mail/actions/connections.ts (3 hunks)
  • apps/mail/actions/getSummary.ts (3 hunks)
  • apps/mail/actions/send.ts (4 hunks)
  • apps/mail/actions/utils.ts (1 hunks)
  • apps/mail/app/(auth)/login/login-client.tsx (1 hunks)
  • apps/mail/app/api/notes/index.ts (6 hunks)
  • apps/mail/app/api/utils.ts (2 hunks)
  • apps/mail/hooks/use-drafts.ts (3 hunks)
  • apps/mail/lib/auth.ts (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/mail/actions/ai.ts (1)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
apps/mail/actions/getSummary.ts (1)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
apps/mail/actions/brain.ts (3)
packages/db/src/schema.ts (1)
  • connection (77-94)
apps/mail/actions/utils.ts (1)
  • getActiveConnection (63-78)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
apps/mail/actions/send.ts (4)
packages/db/src/schema.ts (1)
  • connection (77-94)
apps/mail/actions/utils.ts (1)
  • getActiveConnection (63-78)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
apps/mail/app/api/driver/index.ts (1)
  • createDriver (8-20)
apps/mail/actions/utils.ts (5)
packages/db/src/schema.ts (2)
  • session (19-30)
  • connection (77-94)
apps/mail/lib/auth.ts (1)
  • auth (231-234)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
packages/db/src/index.ts (1)
  • db (17-17)
apps/mail/app/api/driver/index.ts (1)
  • createDriver (8-20)
apps/mail/app/api/utils.ts (2)
apps/mail/lib/auth.ts (1)
  • auth (231-234)
packages/db/src/schema.ts (1)
  • session (19-30)
apps/mail/app/api/notes/index.ts (2)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
apps/mail/app/api/notes/types.ts (1)
  • Note (1-11)
🔇 Additional comments (38)
apps/mail/app/api/utils.ts (3)

2-2: Added essential navigation import.

The addition of redirect from 'next/navigation' is necessary for the new unauthorized handling function.


22-32: Well-implemented centralized unauthorized handling.

This new utility function provides a consistent way to handle unauthorized access across the application. It properly:

  1. Logs the unauthorized attempt
  2. Signs out the user with current headers
  3. Redirects to login with an error parameter
  4. Catches and logs any errors in the process

The error handling with try-catch ensures robustness even if the sign-out process fails.


38-40: Improved consistency in authentication handling.

Good refactoring to use the central utility function instead of throwing a generic error. This standardizes the unauthorized access handling across the application.

apps/mail/lib/auth.ts (2)

97-97: Enhanced error logging.

Improved error logging by including the actual error object, which will provide more context for debugging.


99-103: Added robust error handling for redirect.

Properly wrapped the redirect call in a try-catch block to handle and log any exceptions that might occur during the redirection process. This aligns with the application's improved error handling strategy.

apps/mail/app/(auth)/login/login-client.tsx (1)

70-72: Added user-friendly error message for unauthorized access.

This change completes the user experience flow by displaying an appropriate toast message when users are redirected to the login page due to unauthorized access. The message is clear and informative without exposing technical details.

apps/mail/actions/connections.ts (5)

3-3: Updated import to include the new utility function.

Correctly imported both getAuthenticatedUserId and the new throwUnauthorizedGracefully function.


16-18: Standardized unauthorized access handling.

Good replacement of direct error throwing with the centralized throwUnauthorizedGracefully function for consistent error handling.


22-24: Standardized unauthorized access handling for missing user ID.

Properly replaced direct error throwing with the centralized utility for the case when user ID is missing.


48-50: Consistent error handling in putConnection.

Applied the same standardized approach for handling missing sessions in the putConnection function, maintaining consistency.


54-56: Standardized handling for missing user ID in putConnection.

Correctly replaced direct error throwing with the centralized utility function for consistency.

apps/mail/actions/ai.ts (1)

4-4: Good addition of the centralized error handling utility.

Adding the import for throwUnauthorizedGracefully aligns with the goal of standardizing unauthorized access handling across the mail app.

apps/mail/actions/ai-reply.ts (2)

3-4: Good import organization.

Adding the import for throwUnauthorizedGracefully and reordering imports improves code organization.


86-87: Good code style improvement.

Adding a trailing comma after the last property improves code consistency and makes future additions cleaner.

apps/mail/hooks/use-drafts.ts (3)

8-8: Improved import organization.

Reordering imports for better organization is a good practice.


75-75: Good code style improvement.

Adding a trailing comma after the last property improves code consistency and makes future additions cleaner.


84-88:

Details

❌ Incorrect review comment

Verify intentional removal of draft properties.

Several properties have been commented out in the returned draft objects:

  • threadId
  • title
  • subject
  • receivedOn

Ensure this is intentional and won't break any code that relies on these properties.


🏁 Script executed:

#!/bin/bash
# Check for usages of these properties in other files
echo "Checking for usage of threadId, title, subject and receivedOn properties on draft objects:"
grep -r "draft\.threadId" --include="*.ts" --include="*.tsx" apps/mail/
grep -r "draft\.title" --include="*.ts" --include="*.tsx" apps/mail/
grep -r "draft\.subject" --include="*.ts" --include="*.tsx" apps/mail/
grep -r "draft\.receivedOn" --include="*.ts" --include="*.tsx" apps/mail/

Length of output: 953


🏁 Script executed:

#!/bin/bash
# Display the first 200 lines of the hook to review return shape
sed -n '1,200p' apps/mail/hooks/use-drafts.ts

Length of output: 3349


Redundant Commented‑Out Draft Properties
The spread (...draft) at line 87 already includes threadId, title, subject, and receivedOn, so commenting out the explicit overrides does not remove those fields at runtime. All existing code (for example, create-email.tsx’s use of draft.subject) will continue to work.

– If you intended to provide a fallback for receivedOn, you’ll need to reintroduce that override.
– Otherwise, you can safely delete these commented lines for clarity.

Likely an incorrect or invalid review comment.

apps/mail/actions/getSummary.ts (4)

2-2: Good addition of centralized error handling utility.

Adding the import for throwUnauthorizedGracefully aligns with the goal of standardizing unauthorized access handling across the mail app.


23-25: Standardized error handling for unauthorized access.

Using throwUnauthorizedGracefully() here ensures consistent handling of unauthorized access scenarios.


28-30: Changed error handling approach.

Previously this would throw an error, now it returns null. This is a behavior change that callers need to be aware of, but it's consistent with the function's error handling pattern of returning null in the catch block.


14-16:

Details

❌ Incorrect review comment

Verify return type compatibility.

The function now returns the result of throwUnauthorizedGracefully() when there's no session or connection ID, which redirects to the login page. Ensure this return value is compatible with what callers of this function expect.


🏁 Script executed:

#!/bin/bash
# Find where GetSummary is used to see what callers expect
grep -r "GetSummary" --include="*.ts" --include="*.tsx" apps/mail/

Length of output: 306


🏁 Script executed:

#!/bin/bash
# Display the contents of the GetSummary action
sed -n '1,200p' apps/mail/actions/getSummary.ts

# Locate the definition of throwUnauthorizedGracefully
rg --context 3 "throwUnauthorizedGracefully" -n .

# Display the use-summary hook to see how GetSummary’s return value is handled
sed -n '1,200p' apps/mail/hooks/use-summary.ts

Length of output: 12267


🏁 Script executed:

#!/bin/bash
# Show the implementation of throwUnauthorizedGracefully to understand its return type
sed -n '1,200p' apps/mail/app/api/utils.ts

Length of output: 2071


No return type change needed—throwUnauthorizedGracefully triggers a redirect

The throwUnauthorizedGracefully helper never actually returns a value (it always throws a redirect), so when you return throwUnauthorizedGracefully() in GetSummary, the call is aborted by Next.js navigation rather than returning undefined or another type. TypeScript infers GetSummary’s return type as a union of string | null | never, which is compatible with how callers (like your SWR hook) handle it. You can safely leave the code as-is.

Likely an incorrect or invalid review comment.

apps/mail/actions/brain.ts (4)

1-4: Good reorganization of imports.

The imports are well-organized with the new utility function throwUnauthorizedGracefully imported first. This change aligns with the broader standardization of unauthorized access handling.


7-9: Improved error handling for missing environment variables.

Converting from throwing an error to an early return with null is a better pattern for handling missing environment configuration. This allows callers to handle the absence of the brain URL gracefully.


13-15: Standardized unauthorized access handling.

Using the centralized throwUnauthorizedGracefully function improves consistency in error handling across the application. This ensures users are properly signed out and redirected when their authentication is invalid.


17-20: Code remains functionally equivalent.

The axios request implementation remains unchanged, maintaining the core functionality while improving the error handling flow.

apps/mail/app/api/notes/index.ts (3)

1-5: Imports properly reorganized.

The imports now include the centralized throwUnauthorizedGracefully utility, maintaining a clean and organized structure.


13-15: Standardized unauthorized access handling in getCurrentUserId.

Replacing the direct error throw with throwUnauthorizedGracefully() ensures consistent handling of missing user sessions, aligning with the standardized approach across the application.


38-39: Improved code formatting with consistent trailing commas.

The code now consistently uses trailing commas in parameter lists and object literals, which improves maintainability and makes future additions cleaner in version control.

Also applies to: 51-52, 73-74, 82-83

apps/mail/actions/utils.ts (5)

1-8: Well-organized imports with the new utility.

Imports are properly organized, including the new throwUnauthorizedGracefully utility, which is now consistently used throughout the file.


10-11: Minor syntax improvement.

Added a semicolon to the FatalErrors array declaration, improving consistency with the codebase style.


12-29: Standardized error handling in deleteActiveConnection.

The function now properly uses throwUnauthorizedGracefully for unauthorized access scenarios, maintaining the existing error handling for other errors. This ensures consistent behavior across the application when users aren't properly authenticated.


31-61: Improved error handling in getActiveDriver.

The function now uses throwUnauthorizedGracefully at multiple checkpoints:

  1. When session or connectionId is missing
  2. When the connection record isn't found
  3. When access or refresh tokens are missing

This comprehensive approach ensures all unauthorized scenarios are handled consistently.


63-78: Standardized error handling in getActiveConnection.

The function now uses the early return pattern with throwUnauthorizedGracefully when authorization checks fail, making the code more readable while ensuring consistent error handling.

apps/mail/actions/send.ts (5)

1-7: Clean import organization.

Imports are well-organized with the new utility function throwUnauthorizedGracefully imported first, followed by other imports in a logical order.


16-17: Improved function parameter formatting.

Added trailing commas to the threadId parameter in both the function signature and the type definition, which improves consistency and makes future changes cleaner in version control.

Also applies to: 25-26


28-29: Consistent string style.

Changed double quotes to single quotes in the error message, maintaining consistency with the codebase's string style preferences.


33-35: Standardized unauthorized access handling.

Replaced direct error throwing with the centralized throwUnauthorizedGracefully function for missing connection tokens. This ensures consistent behavior for unauthorized scenarios across the application.


37-54: Maintained core functionality with improved formatting.

The driver creation and email sending functionality remain unchanged, while formatting improvements (trailing commas, consistent style) have been applied. This ensures the code remains functionally equivalent while improving readability and maintainability.

Comment on lines 40 to 42
if (!session?.user) {
throw new Error('Unauthorized');
return throwUnauthorizedGracefully();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify return type compatibility with function signature.

The function is declared to return Promise<AIEmailResponse>, but now it can return the result of throwUnauthorizedGracefully() which redirects to the login page. This creates a potential type inconsistency.


🏁 Script executed:

#!/bin/bash
# Check what throwUnauthorizedGracefully actually returns and if it's compatible with AIEmailResponse
grep -r "throwUnauthorizedGracefully" --include="*.ts" apps/mail/app/api/utils.ts

Length of output: 180


🏁 Script executed:

#!/bin/bash
# Show full definition of throwUnauthorizedGracefully for return type inspection
rg -C 5 "export const throwUnauthorizedGracefully" apps/mail/app/api/utils.ts

Length of output: 367


Ensure return type consistency for the unauthorized flow

The ai.ts action is declared to return Promise<AIEmailResponse>, but on an unauthenticated session you’re doing:

if (!session?.user) {
  return throwUnauthorizedGracefully();
}

Here throwUnauthorizedGracefully() is an async helper that ultimately calls redirect() (which throws) and otherwise returns Promise<void>, not Promise<AIEmailResponse>. This mismatch can lead to subtle runtime or type‑checking issues.

To fix this, choose one of the following:

  • Change your action’s signature to allow the redirect branch explicitly, e.g.
    Promise<AIEmailResponse>, or Promise<AIEmailResponse | void>, so TS knows you may not return a payload when redirecting.
  • Remove the return, call it with await throwUnauthorizedGracefully(); (so the action never actually returns), or
  • Modify throwUnauthorizedGracefully() to return or throw a value that satisfies AIEmailResponse (e.g. throw a typed exception or return a dummy response), ensuring all code paths adhere to AIEmailResponse.

Files/locations to review:

  • apps/mail/actions/ai.ts → lines 40–42 (unauthenticated check)
  • apps/mail/app/api/utils.ts → throwUnauthorizedGracefully return type

Comment on lines 16 to 18
if (!session?.user) {
throw new Error('Unauthorized');
return throwUnauthorizedGracefully();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify return type compatibility with function signature.

The function generateAIResponse is declared to return Promise<string>, but now returns the result of throwUnauthorizedGracefully() which redirects to the login page. This creates a potential type inconsistency.


🏁 Script executed:

#!/bin/bash
# Check what throwUnauthorizedGracefully actually returns and if TypeScript throws any errors
grep -r "throwUnauthorizedGracefully" --include="*.ts" apps/mail/app/api/utils.ts

Length of output: 180


Let’s inspect the full implementation of throwUnauthorizedGracefully to confirm its actual return type and behavior:


🏁 Script executed:

#!/bin/bash
rg -n "throwUnauthorizedGracefully" -B5 -A20 apps/mail/app/api/utils.ts

Length of output: 1525


Additional context is needed to confirm the signature of generateAIResponse:


🏁 Script executed:

#!/bin/bash
rg -n "generateAIResponse" -B5 -A15 apps/mail/actions/ai-reply.ts

Length of output: 768


Ensure return-type consistency in the unauthorized branch

We’ve confirmed that throwUnauthorizedGracefully() is an async function returning Promise<void> (it calls Next.js’s redirect(), which itself returns never), so returning it from generateAIResponse(): Promise<string> will cause a TypeScript mismatch. To resolve, pick one of the following strategies:

• apps/mail/actions/ai-reply.ts (lines 16–18)
Current code:

if (!session?.user) {
  return throwUnauthorizedGracefully();
}

Options to fix:

  1. Invoke redirect without returning a non‐string value
    if (!session?.user) {
      await throwUnauthorizedGracefully();
      // redirect() never returns—function won’t continue or return a string
    }
  2. Annotate throwUnauthorizedGracefully as Promise<never>
    export const throwUnauthorizedGracefully = async (): Promise<never> => {
      /* ... */
    };
    This makes Promise<never> assignable to Promise<string> by leveraging covariance.
  3. Convert the unauthorized branch into an explicit error throw
    if (!session?.user) {
      throw new Error('Unauthorized');  // then handle redirect at a higher level
    }

Choose the approach that best aligns with your control‐flow conventions so that generateAIResponse always either resolves to a string or never returns.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45a35d1 and 1ef5490.

📒 Files selected for processing (2)
  • apps/mail/actions/drafts.ts (3 hunks)
  • apps/mail/actions/mail.ts (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/actions/mail.ts (1)
apps/mail/app/api/utils.ts (1)
  • throwUnauthorizedGracefully (22-32)
🔇 Additional comments (5)
apps/mail/actions/mail.ts (2)

3-3: Good addition of centralized error handling utility

Adding the import for the centralized throwUnauthorizedGracefully utility supports better error handling standardization across the application.


30-32: Improved error handling, but inconsistently applied

The change properly handles unauthorized errors by:

  1. Using the centralized utility for graceful handling
  2. Returning an empty array instead of throwing, which prevents UI crashes

However, this pattern is only applied to getMails but not to other similar functions in this file (like getMail, markAsRead, etc.), which still use the direct throw error pattern.

Is this inconsistency intentional? If standardizing unauthorized handling is the goal, consider applying this pattern to other functions in this file as well.

apps/mail/actions/drafts.ts (3)

1-3: Appropriate imports for centralized error handling

Good addition of the throwUnauthorizedGracefully import to support standardized error handling.


32-33: Inconsistent error handling patterns

There's an inconsistency in how errors are handled across functions in this file:

  • getDrafts uses throwUnauthorizedGracefully and doesn't throw errors
  • createDraft logs errors and returns a structured error object
  • getDraft still throws errors directly

If standardizing error handling is the goal, consider applying the throwUnauthorizedGracefully pattern consistently to all functions that might encounter authorization issues.

Also applies to: 48-49


39-39: Minor formatting improvements

The error message and logging statement changes are minor improvements for clarity.

Also applies to: 42-42

Comment on lines +18 to +20
console.error('Error getting threads:', error);
await throwUnauthorizedGracefully();
// throw error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improved error handling but missing return value

The error handling now correctly uses the centralized utility for graceful handling of unauthorized errors. However, unlike in mail.ts where an empty array is returned, here there's no explicit return value after calling throwUnauthorizedGracefully().

This could result in an undefined return value when an error occurs, potentially causing issues in the UI. Consider adding an appropriate return value:

  try {
    const driver = await getActiveDriver();
    return await driver.listDrafts(q, max, pageToken);
  } catch (error) {
    console.error('Error getting threads:', error);
    await throwUnauthorizedGracefully();
    // throw error;
+   return { messages: [], nextPageToken: null };
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.error('Error getting threads:', error);
await throwUnauthorizedGracefully();
// throw error;
console.error('Error getting threads:', error);
await throwUnauthorizedGracefully();
// throw error;
return { messages: [], nextPageToken: null };

@MrgSub MrgSub merged commit 0e9856c into main Apr 17, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants