Show error messages for users for things like invalid AI API key#239
Show error messages for users for things like invalid AI API key#239
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new React component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ErrorMessages
participant AlertError
participant Database
User->>ErrorMessages: Request error messages
ErrorMessages->>Database: Fetch user error messages
Database-->>ErrorMessages: Return error messages
ErrorMessages->>AlertError: Display error messages
User->>AlertError: Click to clear messages
AlertError->>ErrorMessages: Trigger clearUserErrorMessagesAction
ErrorMessages->>Database: Clear user error messages
Database-->>ErrorMessages: Confirm messages cleared
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
apps/web/prisma/migrations/20241008234839_error_messages/migration.sql (1)
1-2: LGTM! Consider adding a default value or NOT NULL constraint.The addition of the
errorMessagescolumn as JSONB type is a good choice for storing flexible error message structures. This aligns well with the PR objective of showing error messages for users.A few considerations for the team:
- The column allows NULL values. Depending on your requirements, you might want to add a NOT NULL constraint.
- Consider adding a default value (e.g., an empty JSON object '{}') to ensure consistency for existing and new rows.
- Ensure that the application code handles potential NULL values or empty objects when reading from this column.
apps/web/utils/actions/error-messages.ts (2)
1-6: Consider usingimport typefor type-only imports.To optimize the bundle size and ensure type information is removed during transpilation, consider using
import typefor theServerActionResponseimport.Apply this change:
-import { ServerActionResponse } from "@/utils/error"; +import type { ServerActionResponse } from "@/utils/error";🧰 Tools
🪛 Biome
[error] 4-5: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
8-13: Approve implementation with a minor suggestion.The
clearUserErrorMessagesActionfunction is well-implemented with proper error handling and session validation. However, consider adding a success response for better consistency and to allow the client to confirm the operation's success.Consider modifying the function to return a success response:
export async function clearUserErrorMessagesAction(): Promise<ServerActionResponse> { const session = await auth(); if (!session?.user) return { error: "Not logged in" }; await clearUserErrorMessages(session.user.id); revalidatePath("/(app)", "layout"); + return { success: true }; }apps/web/app/(app)/ErrorMessages.tsx (3)
7-13: LGTM: Good error handling and performance considerations.The component efficiently handles cases where there's no user session or no error messages. For improved type safety, consider adding a type annotation for the
errorMessagesvariable.Consider adding a type annotation:
const errorMessages: Record<string, { message: string }> | null = await getUserErrorMessages(session.user.id);
15-36: LGTM with suggestions: Clean JSX structure with room for minor improvements.The component's structure is well-organized and follows React best practices. Consider the following suggestions:
- Extract the
onClickhandler for the "Clear" button to improve readability.- Use a more reliable
keyfor mapped elements, as error messages might not be unique.Here's a suggested refactor:
const handleClearErrors = () => { clearUserErrorMessagesAction(); }; return ( <div className="p-2"> <AlertError title="Error" description={ <> {Object.entries(errorMessages).map(([key, error]) => ( <div key={key}>{error.message}</div> ))} <Button onClick={handleClearErrors}> Clear </Button> </> } /> </div> );
1-37: LGTM: Well-designed and reusable component.The
ErrorMessagescomponent is well-structured, follows React best practices, and effectively handles its responsibilities. It integrates seamlessly with the authentication system and error message utilities.For future improvement, consider adding a prop to customize the error message display or the clear button text. This would enhance the component's flexibility for different use cases.
Example of a more flexible component signature:
interface ErrorMessagesProps { customTitle?: string; clearButtonText?: string; } export async function ErrorMessages({ customTitle = "Error", clearButtonText = "Clear" }: ErrorMessagesProps) { // ... existing code ... }apps/web/app/(app)/layout.tsx (1)
43-46: LGTM: ErrorMessages integration looks good. Consider adding a comment for clarity.The
ErrorMessagescomponent is correctly placed within theSideNavWithTopNav, ensuring error messages appear above the main content. This is a good approach for user experience.Consider adding a brief comment explaining the purpose of the
ErrorMessagescomponent for better code readability:<SideNavWithTopNav> + {/* Display user error messages above main content */} <ErrorMessages /> {children} </SideNavWithTopNav>apps/web/components/Alert.tsx (1)
50-53: LGTM! Consider adding JSDoc for clarity.The change from
stringtoReact.ReactNodefor thedescriptionprop is a good improvement. It alignsAlertErrorwith other Alert components and allows for more flexible content in the description.Consider adding a JSDoc comment to clarify the usage of
React.ReactNodefor future developers:/** * @param props.title - The title of the error alert * @param props.description - The description of the error. Can be a string or a React node. */ export function AlertError(props: { title: string; description: React.ReactNode; }) { // ... (rest of the component) }apps/web/prisma/schema.prisma (1)
62-62: Approved with suggestions:errorMessagesfield inUsermodelThe addition of the
errorMessagesfield to theUsermodel is a good way to store user-specific error information. TheJson?type provides flexibility for various error message structures.Suggestions for improvement:
- Enhance the comment to provide more context about the field's purpose and potential uses beyond the AI API key example.
- Consider using a more structured approach for error messages. For example:
errorMessages Json? // Structure: { "aiApiKey": ["Invalid key", "Expired"], "other": ["Error message"] }This structure would make it easier to query and manage specific types of errors.
- If you frequently need to query for specific error types, consider creating a separate
UserErrormodel for better performance and querying capabilities.Would you like me to propose a more detailed structure for the
errorMessagesfield or a separateUserErrormodel?apps/web/utils/error-messages/index.ts (2)
33-33: Remove unnecessary optional chaining foruser.errorMessagesSince you've already confirmed that
userexists, you don't need to use optional chaining when accessinguser.errorMessages. You can safely access it directly.Apply this change:
-const currentErrorMessages = (user?.errorMessages as ErrorMessages) || {}; +const currentErrorMessages = (user.errorMessages as ErrorMessages) || {};
49-54: Ensure consistency in error message handling inclearUserErrorMessagesWhile
clearUserErrorMessagesclears theerrorMessagesfield, consider adding error handling similar toaddUserErrorMessageto manage cases where the user might not be found.You could add a check for the user's existence:
export async function clearUserErrorMessages(userId: string): Promise<void> { const user = await prisma.user.findUnique({ where: { id: userId } }); if (!user) { console.warn(`User with ID ${userId} not found`); return; } await prisma.user.update({ where: { id: userId }, data: { errorMessages: {} }, }); }apps/web/utils/llms/index.ts (1)
Line range hint
105-157: Consider adding error handling tochatCompletionStreamfor consistencyUnlike the other functions,
chatCompletionStreamdoes not wrap its logic in atry-catchblock to handle potential errors usinghandleError. For consistency and to ensure that all errors are handled uniformly, consider adding error handling tochatCompletionStream.Apply this diff to add error handling:
export async function chatCompletionStream({ userAi, prompt, system, userEmail, usageLabel: label, onFinish, }: { userAi: UserAIFields; prompt: string; system?: string; userEmail: string; usageLabel: string; onFinish?: (text: string) => Promise<void>; }) { + try { const { provider, model, llmModel } = getModel(userAi); const result = await streamText({ model: llmModel, prompt, system, onFinish: async ({ usage, text }) => { await saveAiUsage({ email: userEmail, provider, model, usage, label, }); if (onFinish) await onFinish(text); }, }); return result; + } catch (error) { + await handleError(error, userEmail); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- apps/web/app/(app)/ErrorMessages.tsx (1 hunks)
- apps/web/app/(app)/layout.tsx (2 hunks)
- apps/web/components/Alert.tsx (1 hunks)
- apps/web/prisma/migrations/20241008234839_error_messages/migration.sql (1 hunks)
- apps/web/prisma/schema.prisma (1 hunks)
- apps/web/utils/actions/error-messages.ts (1 hunks)
- apps/web/utils/error-messages/index.ts (1 hunks)
- apps/web/utils/llms/index.ts (3 hunks)
🧰 Additional context used
🪛 Biome
apps/web/utils/actions/error-messages.ts
[error] 4-5: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
🔇 Additional comments (9)
apps/web/app/(app)/ErrorMessages.tsx (1)
1-5: LGTM: Imports are well-organized and relevant.The imports are logically structured and include all necessary modules for the component's functionality.
apps/web/app/(app)/layout.tsx (2)
15-15: LGTM: ErrorMessages import looks good.The import statement for the
ErrorMessagescomponent is correctly implemented, following React and Next.js conventions.
15-15: Verify error handling mechanism for ErrorMessages component.The integration of the
ErrorMessagescomponent aligns well with the PR objective. However, it's not immediately clear how error states are passed to or managed by this component.To ensure proper error handling:
- Verify that the
ErrorMessagescomponent can access and display relevant error states.- Check if there's a centralized error state management (e.g., using Context API or Redux) that this component utilizes.
Consider running the following script to check for error-related props or context usage:
This will help ensure that the error handling mechanism is properly implemented and connected to the newly added
ErrorMessagescomponent.Also applies to: 43-46
✅ Verification successful
Error handling mechanism for
ErrorMessagescomponent is properly implemented.The
ErrorMessagescomponent correctly imports necessary error handling utilities and manages error states by fetching and clearing error messages. This ensures that error messages are effectively displayed to users and appropriately managed within the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for error-related props or context usage in ErrorMessages component # Test: Search for the ErrorMessages component implementation echo "Searching for ErrorMessages component implementation:" ast-grep --lang typescript --pattern 'export $_ function ErrorMessages($_) { $$$ }' # Test: Check for error-related imports or context usage echo "Checking for error-related imports or context usage:" rg --type typescript -i '(error|context)' "$(fd -t f ErrorMessages)"Length of output: 462
Script:
#!/bin/bash # Description: Check for error-related props or context usage in ErrorMessages component # Test: Search for the ErrorMessages component implementation echo "Searching for ErrorMessages component implementation:" ast-grep --lang typescript --pattern 'export $_ function ErrorMessages($_) { $$$ }' # Test: Check for error-related imports or context usage echo "Checking for error-related imports or context usage:" rg -i '(error|context)' --glob '*.tsx' 'apps/web/app/(app)/ErrorMessages.tsx'Length of output: 985
Script:
#!/bin/bash # Description: Check for error-related props or context usage in ErrorMessages component # Test: Search for the async ErrorMessages component implementation echo "Searching for async ErrorMessages component implementation:" ast-grep --lang typescript --pattern 'export async function ErrorMessages($_) { $$$ }' # Test: Check for error-related imports or context usage echo "Checking for error-related imports or context usage:" rg -i '(error|context)' --glob '*.tsx' 'apps/web/app/(app)/ErrorMessages.tsx'Length of output: 1000
apps/web/prisma/schema.prisma (2)
Line range hint
1-391: Overall assessment: Positive changes with minor improvement suggestionsThe changes to the Prisma schema file are generally positive:
- The
@uniqueconstraint onuserIdin theAccountmodel improves data integrity.- The new
errorMessagesfield in theUsermodel allows for flexible error tracking.These changes align well with the PR objective of showing error messages for users. The schema modifications provide the necessary structure to store and manage these error messages effectively.
To further improve the schema:
- Consider adding more detailed comments or documentation for new fields and constraints.
- Evaluate the possibility of using more structured approaches for error messages if specific error types need to be frequently queried.
Line range hint
19-19: Approved with considerations:@uniqueconstraint onuserIdThe addition of the
@uniqueconstraint to theuserIdfield in theAccountmodel is a significant change that ensures each user can only have one associated account. This is generally a good practice for maintaining data integrity.However, please consider the following:
- Ensure that this change doesn't conflict with existing data or application logic that might expect multiple accounts per user.
- The comment "May remove this in the future" suggests uncertainty about this change. It would be helpful to clarify the reasons for potentially removing it and under what circumstances.
To verify the impact of this change, please run the following script:
This script will help identify any existing users with multiple accounts and locate any application code that might be affected by this change.
apps/web/utils/error-messages/index.ts (1)
15-19: Handle the case when the user is not found ingetUserErrorMessagesIf the user is not found (
userisnull), the function currently returnsnull. Alternatively, you might want to consider throwing an error or returning an empty object{}for consistency.Run the following script to check how often
getUserErrorMessagesis called and hownullvalues are handled:✅ Verification successful
Null handling in
getUserErrorMessagesis appropriately implemented.The function
getUserErrorMessagesis consistently handled with null checks wherever it's called. ReturningnullwhenerrorMessagesare absent maintains the current application behavior and consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `getUserErrorMessages` and check for null handling. # Search for calls to `getUserErrorMessages` rg --type javascript --type typescript 'getUserErrorMessages\(' -A 2 # Look for null checks after function calls rg --type javascript --type typescript 'const .* = await getUserErrorMessages\(.*\);' -A 2Length of output: 233
Script:
#!/bin/bash # Description: Find all calls to `getUserErrorMessages` and check for null handling. # Search for calls to `getUserErrorMessages` rg --type js --type ts 'getUserErrorMessages\(' -A 2 # Look for null checks after function calls rg --type js --type ts 'const .* = await getUserErrorMessages\(.*\);' -A 2Length of output: 859
apps/web/utils/llms/index.ts (3)
80-103: Error handling inchatCompletionObjectis implemented correctlyWrapping the
generateObjectcall within atry-catchblock and usinghandleErrorensures that errors are properly managed and user error messages are added when necessary. The usage data is saved appropriately when available.
159-183: Error handling inchatCompletionToolsis implemented correctlyThe addition of the
try-catchblock inchatCompletionToolsmirrors the structure inchatCompletionObject, ensuring consistent error handling across functions. The function correctly saves usage data when available.
187-196: 🛠️ Refactor suggestion
⚠️ Potential issueImprove error detection by using structured error properties instead of string matching
In the
handleErrorfunction, the code checks for specific error messages using string matching:if (error.message.includes("Incorrect API key provided")) { ... }This approach can be fragile, as error messages might change or be localized. Consider using structured properties like error codes or types provided by
APICallErrorfor more reliable error detection.If
APICallErrorprovides an error code or type, modify the condition as follows:async function handleError(error: unknown, userEmail: string) { if (APICallError.isInstance(error)) { - if (error.message.includes("Incorrect API key provided")) { + if (error.code === 'INVALID_API_KEY') { await addUserErrorMessage( userEmail, ErrorType.INCORRECT_OPENAI_API_KEY, error.message, ); } } }Run the following script to check if
APICallErrorincludes an error code or type property:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/utils/error.ts (1)
71-76: LGTM with a suggestion: Consider case-insensitive error message check.The new function
isIncorrectOpenAIAPIKeyErroris well-implemented and serves its purpose of identifying incorrect API key errors. However, to make it more robust against potential changes in the API's error message, consider using a case-insensitive check.Here's a suggested improvement:
export function isIncorrectOpenAIAPIKeyError(error: unknown): boolean { return ( APICallError.isInstance(error) && - error.message.includes("Incorrect API key provided") + error.message.toLowerCase().includes("incorrect api key provided") ); }This change ensures that the function will still work even if the API's error message capitalization changes in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/utils/error.ts (2 hunks)
- apps/web/utils/llms/index.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/llms/index.ts
🧰 Additional context used
🔇 Additional comments (3)
apps/web/utils/error.ts (3)
5-5: LGTM: New import statement added correctly.The import of
APICallErrorfrom the "ai" module is appropriately placed with other imports at the top of the file. This import is necessary for the new error checking function added below.
84-85: LGTM: Correct integration of new error check.The modification to the
isKnownApiErrorfunction correctly incorporates the newisIncorrectOpenAIAPIKeyErrorcheck. This ensures that incorrect OpenAI API key errors are now properly handled as known errors, preventing unnecessary logging in Sentry.
Line range hint
1-87: Overall assessment: Well-implemented error handling enhancements.The changes to this file effectively improve error handling for OpenAI API key issues. The new function and its integration into the existing error checking system are well-implemented and consistent with the file's overall structure and purpose. The only suggestion for improvement is to consider using a case-insensitive check in the
isIncorrectOpenAIAPIKeyErrorfunction for increased robustness.🧰 Tools
🪛 Biome
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores