Conversation
…es it doesn't return
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the TypeScript codebase, primarily focusing on server action handling. A new action, Changes
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 (
|
|
Could potentially use |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (19)
apps/web/utils/actions/error-messages.ts (3)
8-10: LGTM: Function refactored to use withActionInstrumentation.The refactoring improves server action handling as per the PR objective. The function name is correctly passed as a string to
withActionInstrumentation.Consider using a constant for the action name to avoid potential typos and improve maintainability:
const ACTION_NAME = "clearUserErrorMessages"; export const clearUserErrorMessagesAction = withActionInstrumentation( ACTION_NAME, async () => { // ... } );
11-15: LGTM: Function body logic remains correct.The function body correctly handles session checking, error message clearing, and path revalidation.
Consider enhancing error handling by returning a more specific error message:
if (!session?.user) return { error: "User not authenticated" };Also, consider adding a try-catch block to handle potential errors from
clearUserErrorMessagesandrevalidatePath:try { await clearUserErrorMessages(session.user.id); revalidatePath("/(app)", "layout"); return { success: true }; } catch (error) { console.error("Error clearing user error messages:", error); return { error: "Failed to clear error messages" }; }
Line range hint
1-16: Summary: Improved server action handling for clearUserErrorMessagesActionThe refactoring of
clearUserErrorMessagesActionto usewithActionInstrumentationaligns well with the PR objective of improving server action handling. This change likely enhances logging and error tracking capabilities while maintaining the original functionality.Key improvements:
- Added instrumentation to the action
- Maintained existing logic for clearing user error messages
- Preserved error handling and path revalidation
The changes are well-implemented and improve the overall quality of the codebase. Consider the minor suggestions provided in previous comments for further enhancements.
As you continue to refactor other actions to use
withActionInstrumentation, ensure consistency across the codebase. This pattern will improve observability and error handling throughout the application.apps/web/utils/actions/unsubscriber.ts (1)
17-30: Improved formatting and Prisma usageThe restructuring of the
prisma.newsletter.upsertcall enhances readability without altering the core functionality. The use ofconnectin thecreateobject is a more idiomatic approach to establishing relationships in Prisma.For consistency, consider applying the same formatting to the
updateobject:update: { status: options.status },This minor change would make the structure of all objects in the upsert call uniform.
apps/web/utils/actions/admin.ts (1)
8-36: Good use of instrumentation and improved parameter handling.The refactoring of
adminProcessHistoryActionto usewithActionInstrumentationis a positive change. This likely enhances monitoring and logging capabilities for this server action. The use of object destructuring for parameters is also a good practice, making the function more flexible and easier to extend in the future.Consider improving the error handling to align with the PR objective of "Handle server action failure better". Instead of returning simple objects for errors, you could create a custom error type or use a more structured error response. For example:
type AdminActionError = { code: 'NOT_LOGGED_IN' | 'NOT_ADMIN'; message: string; }; // In the function: if (!userId) return { error: { code: 'NOT_LOGGED_IN', message: "Not logged in" } as AdminActionError }; if (!isAdmin(session.user.email)) return { error: { code: 'NOT_ADMIN', message: "Not admin" } as AdminActionError };This would provide more type safety and allow for more specific error handling on the client side.
apps/web/app/(app)/ErrorMessages.tsx (1)
33-33: Approve the button text change with a minor suggestion.The change from "Clear" to "I've fixed them" is a good improvement in terms of user experience. It makes the action more personal and implies user responsibility. However, to be more accurate and avoid potential confusion, consider a slight modification.
Consider changing the button text to "I've addressed these issues" or "Mark as resolved". This maintains the personal touch while being more precise about the action being taken.
- I've fixed them + I've addressed these issuesapps/web/utils/actions/categorize.ts (1)
60-60: Simplification of return value improves function focus.The change to return only
{ category: res?.category }instead of the entireresobject is a good improvement. It makes the function's purpose clearer and reduces the potential for misuse of other data that might have been present in the full response.However, there are a couple of points to consider:
- Error handling: Ensure that calling code can still distinguish between successful categorization and errors.
- Nullish coalescing: Consider using nullish coalescing to provide a default value if
res?.categoryis undefined.Consider updating the return statement to handle potential undefined values:
- return { category: res?.category }; + return { category: res?.category ?? 'uncategorized' };This ensures that the
categoryproperty always has a value, which can help prevent issues in the calling code.apps/web/utils/actions/user.ts (3)
19-30: LGTM: Refactoring improves instrumentation.The
saveAboutActionfunction has been successfully refactored to usewithActionInstrumentation, which enhances monitoring capabilities. The core logic remains intact, ensuring that the functionality is preserved.Consider adding a return statement at the end of the function to explicitly indicate a successful operation:
export const saveAboutAction = withActionInstrumentation( "saveAbout", async (options: SaveAboutBody) => { const session = await auth(); if (!session?.user.email) return { error: "Not logged in" }; await prisma.user.update({ where: { email: session.user.email }, data: { about: options.about }, }); + return { success: true }; }, );This change would make the function's success case more explicit and consistent with error handling.
32-56: LGTM: Refactoring enhances monitoring while preserving functionality.The
deleteAccountActionfunction has been successfully refactored to usewithActionInstrumentation, improving monitoring capabilities. The core logic, including error handling and multiple asynchronous operations, remains intact.Consider adding a return statement at the end of the function to explicitly indicate a successful operation:
export const deleteAccountAction = withActionInstrumentation( "deleteAccount", async () => { const session = await auth(); if (!session?.user.email) return { error: "Not logged in" }; try { await Promise.allSettled([ // ... existing operations ... ]); } catch (error) { console.error("Error while deleting account: ", error); captureException(error, undefined, session.user.email); } await prisma.user.delete({ where: { email: session.user.email } }); + return { success: true }; }, );This change would make the function's success case more explicit and consistent with error handling.
58-69: LGTM: Refactoring enhances monitoring capabilities.The
completedOnboardingActionfunction has been successfully refactored to usewithActionInstrumentation, improving monitoring capabilities. The core logic remains intact, ensuring that the functionality is preserved.Consider adding a return statement at the end of the function to explicitly indicate a successful operation:
export const completedOnboardingAction = withActionInstrumentation( "completedOnboarding", async () => { const session = await auth(); if (!session?.user.id) return { error: "Not logged in" }; await prisma.user.update({ where: { id: session.user.id }, data: { completedOnboarding: true }, }); + return { success: true }; }, );This change would make the function's success case more explicit and consistent with error handling.
apps/web/utils/error.ts (1)
34-35: Approve changes with suggestions for improvementThe updates to
ActionErrorandServerActionResponsetype definitions enhance flexibility and type safety. However, there's room for further improvement:
- For
ActionError<E = {}>, consider using a more specific default type or constraint forE.- For
ServerActionResponse<T, E = {}>, similarly, use a more specific default type or constraint forE.Consider refactoring the type definitions as follows:
export type ActionError<E extends Record<string, unknown> = Record<string, never>> = { error: string } & E; export type ServerActionResponse<T, E extends Record<string, unknown> = Record<string, never>> = ActionError<E> | T;This change ensures that
Eis always an object type, providing better type safety while maintaining flexibility.🧰 Tools
🪛 Biome
[error] 34-34: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 35-35: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
apps/web/app/(app)/automation/RulesPrompt.tsx (1)
99-129: Improved action handling and success message construction.The refactoring of the
onSubmitfunction brings several improvements:
- Use of
handleActionCallfor better action management.- Consistent setting of
isSubmittingstate.- More robust success message construction.
However, consider simplifying the error handling:
Consider flattening the promise chain to simplify error handling:
const onSubmit = useCallback( async (data: SaveRulesPromptBody) => { setIsSubmitting(true); toast.promise( async () => { const result = await handleActionCall("saveRulesPromptAction", () => saveRulesPromptAction(data) ); if (isActionError(result)) { throw new Error(result.error); } router.push("/automation?tab=rules"); mutate(); return result; }, { loading: "Saving rules... This may take a while to process...", success: (result) => { setIsSubmitting(false); const { createdRules, editedRules, removedRules } = result || {}; const message = [ createdRules ? `${createdRules} rules created.` : "", editedRules ? `${editedRules} rules edited.` : "", removedRules ? `${removedRules} rules removed.` : "", ] .filter(Boolean) .join(" "); return `Rules saved successfully! ${message}`; }, error: (err) => { setIsSubmitting(false); return `Error saving rules: ${err.message}`; }, } ); }, [router, mutate] );This approach ensures that
setIsSubmitting(false)is called in both success and error cases, and simplifies the promise chain.apps/web/components/email-list/EmailList.tsx (1)
Line range hint
233-238: Approve new error checks and suggest minor improvementThe new error checks improve the robustness of the
onAiCategorizefunction. They handle both action errors and incomplete requests, enhancing the overall error handling.Consider removing the unnecessary
elseclause as both error cases will throw and exit the function. This will make the code more concise:if (isActionError(result)) { setIsCategorizing((s) => ({ ...s, [thread.id]: false })); throw new Error(`There was an error categorizing the email.`); } else if (!result) { throw new Error("The request did not complete"); -} else { - // setCategory(res); - refetch(); } +// setCategory(res); +refetch();🧰 Tools
🪛 Biome
[error] 232-232: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/actions/middleware.ts (1)
2-2: Import statements should follow project conventions.Ensure that the import order and grouping follow the project's coding standards. Typically, third-party imports come before internal imports.
If your project uses a tool like ESLint or Prettier with import ordering rules, consider reordering the imports:
import { withServerActionInstrumentation } from "@sentry/nextjs"; +import { ActionError, type ServerActionResponse } from "@/utils/error";🧰 Tools
🪛 Biome
[error] 1-2: 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)
apps/web/utils/actions/mail.ts (1)
23-23: Consider removing obsolete commentThe comment
// do not return functions to the client or we'll get an errormay no longer be relevant since the current code does not appear to return functions to the client. Removing unnecessary comments helps keep the codebase clean.apps/web/utils/actions/group.ts (1)
31-38: Log errors for better debuggingIn the
catchblock, the caught error is not being logged or handled beyond returning a generic message. Logging the error can aid in debugging issues in production.Consider logging the error:
} catch (error) { + console.error('Error creating group:', error); return { error: "Error creating group" }; }🧰 Tools
🪛 Biome
[error] 35-35: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/actions/premium.ts (3)
Line range hint
4-4: Remove Unused ImportwithServerActionInstrumentationThe import
withServerActionInstrumentationfrom"@sentry/nextjs"on line 4 is no longer used since you've switched to usingwithActionInstrumentation. Removing unused imports helps keep the codebase clean.Apply this diff to remove the unused import:
-import { withServerActionInstrumentation } from "@sentry/nextjs";
18-18: Import Grouping: Organize Imports for Better ReadabilityConsider grouping imports from the same module together or ordering them alphabetically to improve readability.
You could rearrange the imports as follows:
import uniq from "lodash/uniq"; +import { withActionInstrumentation } from "@/utils/actions/middleware"; import { auth } from "@/app/api/auth/[...nextauth]/auth"; import prisma from "@/utils/prisma"; import { env } from "@/env"; -import { withActionInstrumentation } from "@/utils/actions/middleware";
42-42: Provide Feedback When Premium User Skips Credit DecrementAt line 42, when a premium user attempts to decrement unsubscribe credits, the function returns silently. Consider providing feedback to inform the user that premium users have unlimited unsubscribe credits.
You could modify the code to return a message:
if (isUserPremium) - return; + return { message: "Premium users have unlimited unsubscribe credits." };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- .cursorrules (1 hunks)
- apps/web/app/(app)/ErrorMessages.tsx (1 hunks)
- apps/web/app/(app)/admin/AdminUserControls.tsx (1 hunks)
- apps/web/app/(app)/automation/RulesPrompt.tsx (2 hunks)
- apps/web/app/(landing)/components/page.tsx (1 hunks)
- apps/web/components/email-list/EmailList.tsx (1 hunks)
- apps/web/utils/actions/admin.ts (1 hunks)
- apps/web/utils/actions/categorize.ts (1 hunks)
- apps/web/utils/actions/error-messages.ts (1 hunks)
- apps/web/utils/actions/group.ts (1 hunks)
- apps/web/utils/actions/helpers.ts (0 hunks)
- apps/web/utils/actions/mail.ts (1 hunks)
- apps/web/utils/actions/middleware.ts (1 hunks)
- apps/web/utils/actions/premium.ts (2 hunks)
- apps/web/utils/actions/unsubscriber.ts (1 hunks)
- apps/web/utils/actions/user.ts (1 hunks)
- apps/web/utils/error.ts (1 hunks)
- apps/web/utils/gmail/filter.ts (2 hunks)
- apps/web/utils/server-action.ts (2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/utils/actions/helpers.ts
🧰 Additional context used
🪛 Biome
apps/web/components/email-list/EmailList.tsx
[error] 233-238: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
apps/web/utils/actions/group.ts
[error] 35-35: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 47-49: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 86-86: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 118-118: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 156-156: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 192-192: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 204-204: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 223-223: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 237-237: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/actions/middleware.ts
[error] 16-16: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 18-18: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 29-29: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 48-48: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
apps/web/utils/actions/premium.ts
[error] 262-262: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
apps/web/utils/actions/user.ts
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
apps/web/utils/error.ts
[error] 34-34: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 35-35: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
apps/web/utils/gmail/filter.ts
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
apps/web/utils/server-action.ts
[error] 3-9: Some named imports are only used as types.
This import is only used as a type.
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)
[error] 22-22: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (23)
apps/web/utils/actions/error-messages.ts (1)
6-6: LGTM: Import statement for withActionInstrumentation added.The import statement for
withActionInstrumentationis correctly added and necessary for the refactored function.apps/web/utils/actions/unsubscriber.ts (2)
6-13: Excellent use of middleware for improved action tracking!The addition of
withActionInstrumentationmiddleware enhances the monitoring capabilities of thesetNewsletterStatusAction. This change aligns well with the PR objective of better handling server actions. The function signature remains unchanged, ensuring backward compatibility.
1-32: Summary: Solid improvements with minor suggestions for enhancementOverall, the changes to
setNewsletterStatusActionare well-implemented and align with the PR objectives. The use ofwithActionInstrumentationmiddleware enhances action tracking, which is a significant improvement. The refactoring of the Prisma query improves readability without altering functionality.While the core changes are solid, there are opportunities for further enhancement:
- Improving error handling and type safety as suggested earlier.
- Minor formatting adjustments for consistency in the Prisma upsert call.
These suggestions are minor and don't detract from the overall quality of the implemented changes. Great work on improving the server action handling!
apps/web/utils/actions/categorize.ts (2)
60-60: Update TypeScript types and consuming components.The change in the return value of
categorizeActionmay require updates in other parts of the codebase.Please ensure the following:
- Update any TypeScript types that define the return type of
categorizeAction.- Modify components or functions that consume
categorizeActionto handle the new return format.Run the following script to find TypeScript types related to
categorizeAction:#!/bin/bash # Description: Find TypeScript types related to categorizeAction # Search for type definitions that mention categorizeAction rg --type typescript 'type.*categorizeAction'Review the results and update any relevant type definitions to match the new return format.
60-60: Verify error handling with the new return format.While simplifying the return value improves the function's focus, it's crucial to ensure that error cases are still properly handled. The function has multiple points where it returns error objects, but the new return statement doesn't explicitly handle errors.
To ensure that error handling hasn't been compromised, please verify the following:
- Check all call sites of
categorizeActionto confirm they can still distinguish between successful categorization and errors.- Verify that the calling code doesn't rely on any properties from the
resobject that are no longer being returned.Run the following script to find all usages of
categorizeAction:Review the results to ensure that error handling is still appropriate with the new return format.
apps/web/app/(app)/admin/AdminUserControls.tsx (1)
44-46: LGTM! Verify consistency ofadminProcessHistoryActionusage.The change to
adminProcessHistoryActionlooks good. The function now receives an object with anemailAddressproperty instead of a plain email string, which can improve type safety and make the function more extensible.To ensure consistency across the codebase, let's verify the usage of
adminProcessHistoryAction:✅ Verification successful
Consistency Verified for
adminProcessHistoryActionUsageAll instances of
adminProcessHistoryActionnow use the{ emailAddress: email }format, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct email string usage with adminProcessHistoryAction # Test: Search for adminProcessHistoryAction usage # Expect: All occurrences should use the new { emailAddress: email } format rg --type typescript 'adminProcessHistoryAction\s*\(' -A 3Length of output: 95
Script:
#!/bin/bash # Description: Check for any remaining direct email string usage with adminProcessHistoryAction # Test: Search for adminProcessHistoryAction usage in .ts and .tsx files # Expect: All occurrences should use the new { emailAddress: email } format rg 'adminProcessHistoryAction\s*\(' --glob '*.ts' --glob '*.tsx' -A 3Length of output: 445
apps/web/utils/actions/user.ts (1)
13-14: LGTM: Import statements updated correctly.The import statements have been updated to reflect the changes in the implementation. The removal of
ServerActionResponseand the addition ofwithActionInstrumentationare consistent with the refactoring of the action functions.apps/web/app/(landing)/components/page.tsx (1)
146-149: Improved layout for error and action test buttons.The changes enhance the visual arrangement of the
TestErrorButtonandTestActionButtoncomponents by wrapping them in a flex container with a gap. This improvement aligns well with the PR objective of "Handle server action failure better" by making it easier to test and interact with these components.The modification is clean, consistent with the existing code style, and improves the overall user experience when working with error handling and server actions.
apps/web/app/(app)/automation/RulesPrompt.tsx (2)
32-32: LGTM: New utility import for improved action handling.The addition of
handleActionCallfrom@/utils/server-actionis a good practice for centralizing action handling logic.
Line range hint
1-265: Overall improvement in error handling and user feedback.The changes in this file align well with the PR objective of handling server action failure better. The introduction of
handleActionCalland the refactoring of theonSubmitfunction contribute to more robust error handling and potentially improved instrumentation.The modifications to the success message construction enhance the user experience by providing clearer and more concise feedback about the results of the action.
These changes maintain the core functionality of the component while improving its reliability and user interaction.
apps/web/components/email-list/EmailList.tsx (1)
Line range hint
233-238: Summary of changes: Improved error handling in email categorizationThe changes in this file focus on enhancing the error handling within the
onAiCategorizefunction. These improvements make the email categorization feature more robust by explicitly handling action errors and incomplete requests. The overall structure and functionality of theEmailListcomponent remain unchanged, maintaining its core purpose while improving its reliability.🧰 Tools
🪛 Biome
[error] 232-232: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
apps/web/utils/gmail/filter.ts (3)
12-12: Added 'await' to handle the promise correctlyAdding
awaitto thegmail.users.settings.filters.createcall ensures that the function waits for the promise to resolve before proceeding. This prevents potential issues with unhandled promise rejections.
24-28: Destructured parameters increateAutoArchiveFilterfor clarityDestructuring the parameters directly in the function signature improves readability and makes it explicit which arguments are required.
33-45: Gracefully handle "Filter already exists" errorThe addition of the
try-catchblock ensures that if the filter already exists, the function handles the error gracefully by returning a successful status. This improves the user experience by preventing unnecessary errors.🧰 Tools
🪛 Biome
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
apps/web/utils/actions/middleware.ts (1)
55-59: Ensure consistent error handling and typing in the catch block.In the catch block, the returned object is being cast to
ActionError<Err & { success: false }>, but the structure may not match the expectedServerActionResponsetype. Ensure that the error response includes all necessary fields and types.Consider adjusting the error response to match the expected type:
} catch (error) { // error is already captured by Sentry in `withServerActionInstrumentation` return { error: "An error occurred", success: false, + ...(error instanceof Error ? { message: error.message } : {}), } as ActionError<Err & { success: false }>; }Do you need assistance in refining the error handling to ensure it aligns with your application's conventions?
apps/web/utils/actions/group.ts (4)
36-36: Remove unnecessary template literalLine 36 contains a template literal without interpolation.
This issue has been addressed in a previous comment regarding unnecessary template literals.
202-202:⚠️ Potential issueUse
deleteManywith composite filtersIn Prisma, the
deletemethod requires unique fields. Since we're filtering by bothidanduserId, consider usingdeleteManyinstead.Apply this diff to fix the issue:
- await prisma.group.delete({ where: { id, userId: session.user.id } }); + await prisma.group.deleteMany({ where: { id, userId: session.user.id } });Ensure that the group deletion works as expected when using
deleteMany.
130-135:⚠️ Potential issueCheck for group ownership by including user ID in the query
Similar to previous comment, including
userIdin thewhereclause enhances security and avoids unnecessary checks.Apply this diff to fix the issue:
const existingGroup = await prisma.group.findUnique({ - where: { id: groupId }, + where: { id: groupId, userId: session.user.id }, select: { items: { select: { id: true, type: true, value: true } } }, }); if (!existingGroup) return { error: "Group not found" }; -Likely invalid or redundant comment.
166-171:⚠️ Potential issueInclude user ID in group retrieval for security
Ensure that only the user's own groups are accessed by adding
userIdto the query.Apply this diff to fix the issue:
const existingGroup = await prisma.group.findUnique({ - where: { id: groupId }, + where: { id: groupId, userId: session.user.id }, select: { items: { select: { id: true, type: true, value: true } } }, }); if (!existingGroup) return { error: "Group not found" };Likely invalid or redundant comment.
apps/web/utils/actions/premium.ts (4)
49-50: Handle Potential Null Values When Creating Premium for UserAt lines 49-50, when calling
createPremiumForUser(session.user.id), ensure that the function handles cases where the premium creation might fail and returns a valid premium object.
211-225: Check for Undefined Values inupgradeToPremiumCallIn lines 211-225, when calling
upgradeToPremium, ensure all required parameters are provided and handle any potential undefined values, especially those fromlemonSqueezyLicense.data?.meta.
273-292: Confirm Email Accounts Access Quantity Matches Tier LimitationsAt lines 273-292, when setting
emailAccountsAccess, ensure that the value aligns with the limitations of the selectedPremiumTier. This prevents users from exceeding their allowed email accounts.
Line range hint
20-326: Consistent Application of Action InstrumentationWrapping actions with
withActionInstrumentationenhances monitoring. Ensure that all server actions in the codebase are consistently wrapped to maintain uniform instrumentation and observability.🧰 Tools
🪛 Biome
[error] 262-262: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| const session = await auth(); | ||
| if (!session?.user.email) return { error: "Not logged in" }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and type safety
While the authentication check is maintained, which is good for security, we could further improve the error handling and type safety. Consider the following suggestions:
- Use a more specific error type instead of a generic string.
- Implement a custom error class for authentication failures.
- Use TypeScript's discriminated unions for better type checking of the return value.
Here's an example of how you could implement these improvements:
type AuthError = { kind: 'AuthError'; message: string };
type Success = { kind: 'Success'; data: any };
type ActionResult = AuthError | Success;
// ... (in the function body)
if (!session?.user.email) {
return { kind: 'AuthError', message: 'Not logged in' };
}
// ... (at the end of the function)
return { kind: 'Success', data: result };This approach would provide more robust error handling and improve type safety throughout the application.
| export const saveOnboardingAnswersAction = withActionInstrumentation( | ||
| "saveOnboardingAnswers", | ||
| async (onboardingAnswers: { | ||
| surveyId?: string; | ||
| questions: any; | ||
| answers: Record<string, string>; | ||
| }) => { | ||
| const session = await auth(); | ||
| if (!session?.user.id) return { error: "Not logged in" }; | ||
|
|
||
| await prisma.user.update({ | ||
| where: { id: session.user.id }, | ||
| data: { onboardingAnswers }, | ||
| }); | ||
| } | ||
| await prisma.user.update({ | ||
| where: { id: session.user.id }, | ||
| data: { onboardingAnswers }, | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
LGTM with suggestions: Refactoring improves monitoring, but type safety can be enhanced.
The saveOnboardingAnswersAction function has been successfully refactored to use withActionInstrumentation, improving monitoring capabilities. The core logic remains intact, ensuring that the functionality is preserved.
The questions parameter is typed as any, which reduces type safety. Consider defining a more specific type for this parameter to improve code quality and catch potential errors at compile-time.
Consider the following improvements:
- Define a more specific type for the
questionsparameter:
type OnboardingQuestion = {
id: string;
text: string;
// Add other relevant properties
};
export const saveOnboardingAnswersAction = withActionInstrumentation(
"saveOnboardingAnswers",
async (onboardingAnswers: {
surveyId?: string;
questions: OnboardingQuestion[];
answers: Record<string, string>;
}) => {
// ... existing code ...
},
);- Add a return statement to explicitly indicate a successful operation:
export const saveOnboardingAnswersAction = withActionInstrumentation(
"saveOnboardingAnswers",
async (onboardingAnswers: {
surveyId?: string;
questions: OnboardingQuestion[];
answers: Record<string, string>;
}) => {
const session = await auth();
if (!session?.user.id) return { error: "Not logged in" };
await prisma.user.update({
where: { id: session.user.id },
data: { onboardingAnswers },
});
+ return { success: true };
},
);These changes would improve type safety and make the function's success case more explicit.
🧰 Tools
🪛 Biome
[error] 75-75: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| result = await actionFn(); | ||
| } catch (error) { | ||
| captureException(error, { extra: { actionName } }); | ||
| return { error: String(error) } as ActionError<E>; |
There was a problem hiding this comment.
Use error.message instead of String(error) for clearer error messages
Using String(error) might include unnecessary information like the error name and stack trace. Using error.message provides a cleaner and more user-friendly error description.
Apply this diff to improve the error message:
-return { error: String(error) } as ActionError<E>;
+return { error: error instanceof Error ? error.message : String(error) } as ActionError<E>;📝 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.
| return { error: String(error) } as ActionError<E>; | |
| return { error: error instanceof Error ? error.message : String(error) } as ActionError<E>; |
| } | ||
| } | ||
|
|
||
| export async function handleActionCall<T, E = {}>( |
There was a problem hiding this comment.
Avoid using {} as a type; specify a more precise default type
Using {} as a type means "any non-nullable value," which is discouraged. It can lead to unintended type matches and obscure type errors. Consider specifying a more precise default type for E, such as Record<string, unknown> or another appropriate interface.
Apply this diff to fix the issue:
-export async function handleActionCall<T, E = {}>(
+export async function handleActionCall<T, E = Record<string, unknown>>(📝 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.
| export async function handleActionCall<T, E = {}>( | |
| export async function handleActionCall<T, E = Record<string, unknown>>( |
🧰 Tools
🪛 Biome
[error] 22-22: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| import { | ||
| ActionError, | ||
| type ServerActionResponse, | ||
| captureException, | ||
| isActionError, | ||
| } from "@/utils/error"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize type-only imports by using import type
Some of the imported entities ActionError and ServerActionResponse are only used as types. To improve build performance and ensure that these imports are erased during transpilation, consider using import type for type-only imports.
Apply this diff to adjust the imports:
-import {
- ActionError,
- type ServerActionResponse,
- captureException,
- isActionError,
-} from "@/utils/error";
+import { captureException, isActionError } from "@/utils/error";
+import type { ActionError, ServerActionResponse } from "@/utils/error";📝 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.
| import { | |
| ActionError, | |
| type ServerActionResponse, | |
| captureException, | |
| isActionError, | |
| } from "@/utils/error"; | |
| import { captureException, isActionError } from "@/utils/error"; | |
| import type { ActionError, ServerActionResponse } from "@/utils/error"; |
🧰 Tools
🪛 Biome
[error] 3-9: Some named imports are only used as types.
This import is only used as a type.
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)
| ); | ||
| if (!subscription) return { error: "Subscription not found" }; | ||
| lemonSqueezySubscriptionId = Number.parseInt(subscription.id); | ||
| const attributes = subscription.attributes as any; |
There was a problem hiding this comment.
Avoid Using any Type for subscription.attributes
Using as any on line 262 disables type checking and can introduce bugs. Define a proper type for subscription.attributes to ensure type safety.
Consider defining an interface for SubscriptionAttributes and using it:
interface SubscriptionAttributes {
order_id: string;
product_id: string;
variant_id: string;
first_subscription_item: { id?: string };
// Include other relevant fields
}
const attributes = subscription.attributes as SubscriptionAttributes;🧰 Tools
🪛 Biome
[error] 262-262: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| export const decrementUnsubscribeCreditAction = withActionInstrumentation( | ||
| "decrementUnsubscribeCredit", | ||
| async () => { | ||
| const session = await auth(); | ||
| if (!session?.user.email) return { error: "Not logged in" }; | ||
|
|
||
| const user = await prisma.user.findUnique({ | ||
| where: { email: session.user.email }, | ||
| select: { | ||
| premium: { | ||
| select: { | ||
| id: true, | ||
| unsubscribeCredits: true, | ||
| unsubscribeMonth: true, | ||
| lemonSqueezyRenewsAt: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| if (!user) return { error: "User not found" }; | ||
| if (!user) return { error: "User not found" }; | ||
|
|
||
| const isUserPremium = isPremium(user.premium?.lemonSqueezyRenewsAt || null); | ||
| if (isUserPremium) return; | ||
| const isUserPremium = isPremium(user.premium?.lemonSqueezyRenewsAt || null); | ||
| if (isUserPremium) return; | ||
|
|
||
| const currentMonth = new Date().getMonth() + 1; | ||
| const currentMonth = new Date().getMonth() + 1; | ||
|
|
||
| // create premium row for user if it doesn't already exist | ||
| const premium = user.premium || (await createPremiumForUser(session.user.id)); | ||
| // create premium row for user if it doesn't already exist | ||
| const premium = | ||
| user.premium || (await createPremiumForUser(session.user.id)); | ||
|
|
||
| if ( | ||
| !premium?.unsubscribeMonth || | ||
| premium?.unsubscribeMonth !== currentMonth | ||
| ) { | ||
| // reset the monthly credits | ||
| await prisma.premium.update({ | ||
| where: { id: premium.id }, | ||
| data: { | ||
| // reset and use a credit | ||
| unsubscribeCredits: env.NEXT_PUBLIC_FREE_UNSUBSCRIBE_CREDITS - 1, | ||
| unsubscribeMonth: currentMonth, | ||
| }, | ||
| }); | ||
| } else { | ||
| if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0) return; | ||
| if ( | ||
| !premium?.unsubscribeMonth || | ||
| premium?.unsubscribeMonth !== currentMonth | ||
| ) { | ||
| // reset the monthly credits | ||
| await prisma.premium.update({ | ||
| where: { id: premium.id }, | ||
| data: { | ||
| // reset and use a credit | ||
| unsubscribeCredits: env.NEXT_PUBLIC_FREE_UNSUBSCRIBE_CREDITS - 1, | ||
| unsubscribeMonth: currentMonth, | ||
| }, | ||
| }); | ||
| } else { | ||
| if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0) | ||
| return; | ||
|
|
||
| // decrement the monthly credits | ||
| await prisma.premium.update({ | ||
| where: { id: premium.id }, | ||
| data: { unsubscribeCredits: { decrement: 1 } }, | ||
| }); | ||
| } | ||
| } | ||
| // decrement the monthly credits | ||
| await prisma.premium.update({ | ||
| where: { id: premium.id }, | ||
| data: { unsubscribeCredits: { decrement: 1 } }, | ||
| }); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Ensure Consistent Error Handling in decrementUnsubscribeCreditAction
While wrapping the decrementUnsubscribeCreditAction with withActionInstrumentation, ensure that all code paths return a consistent response type. Currently, some paths return an object with an error key, while others return nothing.
Consider updating the function to always return a standardized response, such as:
type ServerActionResponse = { error?: string; message?: string };
...
if (!session?.user.email) return { error: "Not logged in" };
...
return { message: "Credit decremented successfully." };| export const changePremiumStatusAction = withActionInstrumentation( | ||
| "changePremiumStatus", | ||
| async (options: ChangePremiumStatusOptions) => { | ||
| const session = await auth(); | ||
| if (!session?.user.email) return { error: "Not logged in" }; | ||
| if (!isAdmin(session.user.email)) return { error: "Not admin" }; | ||
|
|
||
| const userToUpgrade = await prisma.user.findUnique({ | ||
| where: { email: options.email }, | ||
| select: { id: true, premiumId: true }, | ||
| }); | ||
| } else if (userToUpgrade) { | ||
| if (userToUpgrade.premiumId) { | ||
| await cancelPremium({ | ||
| premiumId: userToUpgrade.premiumId, | ||
| lemonSqueezyEndsAt: new Date(), | ||
| }); | ||
| } else { | ||
| return { error: "User not premium." }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export async function claimPremiumAdminAction(): Promise<ServerActionResponse> { | ||
| const session = await auth(); | ||
| if (!session?.user.id) return { error: "Not logged in" }; | ||
| if (!userToUpgrade) return { error: "User not found" }; | ||
|
|
||
| const ONE_MONTH = 1000 * 60 * 60 * 24 * 30; | ||
|
|
||
| let lemonSqueezySubscriptionId: number | null = null; | ||
| let lemonSqueezySubscriptionItemId: number | null = null; | ||
| let lemonSqueezyOrderId: number | null = null; | ||
| let lemonSqueezyProductId: number | null = null; | ||
| let lemonSqueezyVariantId: number | null = null; | ||
|
|
||
| if (options.upgrade) { | ||
| if (options.lemonSqueezyCustomerId) { | ||
| const lemonCustomer = await getLemonCustomer( | ||
| options.lemonSqueezyCustomerId.toString(), | ||
| ); | ||
| if (!lemonCustomer.data) return { error: "Lemon customer not found" }; | ||
| const subscription = lemonCustomer.data.included?.find( | ||
| (i) => i.type === "subscriptions", | ||
| ); | ||
| if (!subscription) return { error: "Subscription not found" }; | ||
| lemonSqueezySubscriptionId = Number.parseInt(subscription.id); | ||
| const attributes = subscription.attributes as any; | ||
| lemonSqueezyOrderId = Number.parseInt(attributes.order_id); | ||
| lemonSqueezyProductId = Number.parseInt(attributes.product_id); | ||
| lemonSqueezyVariantId = Number.parseInt(attributes.variant_id); | ||
| lemonSqueezySubscriptionItemId = attributes.first_subscription_item.id | ||
| ? Number.parseInt(attributes.first_subscription_item.id) | ||
| : null; | ||
| } | ||
|
|
||
| const user = await prisma.user.findUnique({ | ||
| where: { id: session.user.id }, | ||
| select: { premium: { select: { id: true, admins: true } } }, | ||
| }); | ||
| await upgradeToPremium({ | ||
| userId: userToUpgrade.id, | ||
| tier: options.period, | ||
| lemonSqueezyCustomerId: options.lemonSqueezyCustomerId || null, | ||
| lemonSqueezySubscriptionId, | ||
| lemonSqueezySubscriptionItemId, | ||
| lemonSqueezyOrderId, | ||
| lemonSqueezyProductId, | ||
| lemonSqueezyVariantId, | ||
| lemonSqueezyRenewsAt: | ||
| options.period === PremiumTier.PRO_ANNUALLY || | ||
| options.period === PremiumTier.BUSINESS_ANNUALLY || | ||
| options.period === PremiumTier.BASIC_ANNUALLY | ||
| ? new Date(+new Date() + ONE_MONTH * 12) | ||
| : options.period === PremiumTier.PRO_MONTHLY || | ||
| options.period === PremiumTier.BUSINESS_MONTHLY || | ||
| options.period === PremiumTier.BASIC_MONTHLY || | ||
| options.period === PremiumTier.COPILOT_MONTHLY | ||
| ? new Date(+new Date() + ONE_MONTH) | ||
| : null, | ||
| emailAccountsAccess: options.emailAccountsAccess, | ||
| }); | ||
| } else if (userToUpgrade) { | ||
| if (userToUpgrade.premiumId) { | ||
| await cancelPremium({ | ||
| premiumId: userToUpgrade.premiumId, | ||
| lemonSqueezyEndsAt: new Date(), | ||
| }); | ||
| } else { | ||
| return { error: "User not premium." }; | ||
| } | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Validate options.period Before Using in Date Calculations
When calculating lemonSqueezyRenewsAt based on options.period, ensure that options.period is a valid PremiumTier. Invalid values could lead to incorrect renewal dates.
Consider adding validation for options.period:
if (!Object.values(PremiumTier).includes(options.period)) {
return { error: "Invalid premium tier specified." };
}🧰 Tools
🪛 Biome
[error] 262-262: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| if (options.lemonSqueezyCustomerId) { | ||
| const lemonCustomer = await getLemonCustomer( | ||
| options.lemonSqueezyCustomerId.toString(), | ||
| ); | ||
| if (!lemonCustomer.data) return { error: "Lemon customer not found" }; | ||
| const subscription = lemonCustomer.data.included?.find( | ||
| (i) => i.type === "subscriptions", | ||
| ); | ||
| if (!subscription) return { error: "Subscription not found" }; | ||
| lemonSqueezySubscriptionId = Number.parseInt(subscription.id); | ||
| const attributes = subscription.attributes as any; | ||
| lemonSqueezyOrderId = Number.parseInt(attributes.order_id); | ||
| lemonSqueezyProductId = Number.parseInt(attributes.product_id); | ||
| lemonSqueezyVariantId = Number.parseInt(attributes.variant_id); | ||
| lemonSqueezySubscriptionItemId = attributes.first_subscription_item.id | ||
| ? Number.parseInt(attributes.first_subscription_item.id) | ||
| : null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strongly Type Lemon Squeezy API Responses
In lines 252-269, avoid using loosely typed or any-typed variables when handling API responses. Define interfaces for the Lemon Squeezy API responses to improve type safety and code clarity.
Define interfaces for the expected response structures and update the code accordingly.
🧰 Tools
🪛 Biome
[error] 262-262: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0) | ||
| return; |
There was a problem hiding this comment.
Handle Case When No Unsubscribe Credits Remain
At lines 65-66, if the user has no unsubscribe credits left, the function returns without any feedback. It's better to inform the user that they've exhausted their credits.
Modify the code to provide an error message:
if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0)
- return;
+ return { error: "No unsubscribe credits remaining for this month." };📝 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.
| if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0) | |
| return; | |
| if (!premium?.unsubscribeCredits || premium.unsubscribeCredits <= 0) | |
| return { error: "No unsubscribe credits remaining for this month." }; |
1 participant
Summary by CodeRabbit
Release Notes
New Features
User Interface Changes
Bug Fixes
Refactor
Chores