fix: add await to mutateAsync function calls #2232
Conversation
geraldmaboshe
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@geraldmaboshe is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request modifies several components within the dashboard application to ensure that the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
🕹️ 150 points
Bug
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1)
51-51: Approved: Await added to mutateAsync call.This change correctly implements the PR objective by adding the
awaitkeyword to themutateAsynccall. It ensures that the mutation completes before the function proceeds, improving the control flow.Consider adding error handling to manage potential failures in the mutation:
async function onSubmit(values: z.infer<typeof formSchema>) { try { await updateName.mutateAsync(values); } catch (error) { console.error('Failed to update workspace name:', error); toast.error('Failed to update workspace name. Please try again.'); } }This addition will provide better user feedback and logging in case of errors.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (2)
55-55: Approved: Correct implementation ofawaitformutateAsyncThe addition of
awaittoupdateName.mutateAsync(values)is correct and aligns with the PR objective. This change ensures that the function waits for the mutation to complete before proceeding, which is a best practice for handling asynchronous operations.Consider adding error handling to catch potential errors from the mutation:
- await updateName.mutateAsync(values); + try { + await updateName.mutateAsync(values); + } catch (error) { + toast.error("Failed to update namespace name. Please try again."); + console.error("Error updating namespace name:", error); + }This addition would provide better user feedback and logging in case of errors.
Line range hint
39-56: Consider consistent error handling approachThe addition of
awaittoupdateName.mutateAsync(values)improves the reliability of the form submission process. However, for consistency and better user experience, consider handling errors explicitly in theonSubmitfunction, similar to how success is handled in the mutation'sonSuccesscallback.You could modify the
trpc.ratelimit.namespace.update.name.useMutationcall to include anonErrorcallback:const updateName = trpc.ratelimit.namespace.update.name.useMutation({ onSuccess() { toast.success("Your namespace name has been renamed!"); router.refresh(); }, onError(error) { toast.error(`Failed to update namespace name: ${error.message}`); }, });This approach would provide consistent error handling across all mutation outcomes.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1)
Line range hint
44-46: Consider removing console.error in productionWhile error logging is useful during development, it's generally not recommended to log errors to the console in a production environment. Consider using a more robust error tracking solution or removing the
console.errorcall in production builds.You could modify the error handling as follows:
onError(err) { if (process.env.NODE_ENV !== 'production') { console.error(err); } toast.error(err.message); }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1)
65-65: Approved: Proper use of await for mutateAsyncThe addition of
awaitto theupdateOwnerId.mutateAsync(values)call is correct and aligns with the PR objectives. This change ensures that the mutation completes before the function proceeds, which is a best practice for handling asynchronous operations.Consider adding a try-catch block to handle any potential errors that might occur during the mutation:
async function onSubmit(values: z.infer<typeof formSchema>) { try { await updateOwnerId.mutateAsync(values); } catch (error) { console.error('Error updating owner ID:', error); toast.error('Failed to update owner ID. Please try again.'); } }This addition would provide more robust error handling and improve the user experience by giving more specific feedback in case of failures.
apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (2)
Line range hint
55-59: Removeconsole.errorin production codeThe
console.errorin theonErrorcallback oftrpc.api.updateIpWhitelist.useMutationshould be removed or wrapped in a development-only check for production builds. Logging sensitive information to the console in production can pose security risks.Consider replacing it with a more production-friendly error handling approach:
onError(err) { // Remove or wrap in development-only check if (process.env.NODE_ENV !== 'production') { console.error(err); } toast.error(err.message); }
Line range hint
1-124: Improve accessibility of the formWhile the form structure is good, there are opportunities to enhance its accessibility:
- Add
aria-labelto the form element to provide a descriptive label for screen readers.- Use
aria-describedbyto associate the card description with the form fields.- Add
aria-liveregion for form submission status updates.Here are some suggested improvements:
<form onSubmit={form.handleSubmit(onSubmit)} aria-label="Update IP Whitelist"> <Card> <CardHeader className={cn({ "opacity-40": !isEnabled })}> <CardTitle id="ip-whitelist-title">IP Whitelist</CardTitle> <CardDescription id="ip-whitelist-description"> Protect your keys from being verified by unauthorized sources. Enter your IP addresses either comma or newline separated. </CardDescription> </CardHeader> <CardContent> {workspace.plan === "enterprise" ? ( <div className="flex flex-col space-y-2"> <input type="hidden" name="workspaceId" value={api.workspaceId} /> <input type="hidden" name="apiId" value={api.id} /> <FormField control={form.control} name="ipWhitelist" render={({ field }) => ( <Textarea className="max-w-sm" {...field} autoComplete="off" placeholder={`127.0.0.1\n1.1.1.1`} aria-describedby="ip-whitelist-description" /> )} /> </div> ) : ( // ... (Alert component remains unchanged) )} </CardContent> <CardFooter className={cn("justify-end", { "opacity-30 ": !isEnabled })}> <Button variant={ form.formState.isValid && !form.formState.isSubmitting ? "primary" : "disabled" } disabled={!form.formState.isValid || form.formState.isSubmitting} type="submit" aria-live="polite" > {form.formState.isSubmitting ? <Loading /> : "Save"} </Button> </CardFooter> </Card> </form>These changes will improve the form's accessibility, making it more usable for people using assistive technologies.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-expiration.tsx (3)
Line range hint
61-65: Consider enhancing error handling in the mutation callback.While the current error handling logs the error and displays a toast message, logging to the console in production might not be ideal. Consider removing the
console.error(err)line or replacing it with a more appropriate error tracking method for production environments.
Line range hint
38-45: Potential improvement for the convertDate function.The
convertDatefunction could benefit from additional error handling or validation. Consider adding a try-catch block to handle potential errors when parsing invalid dates.Here's a suggested improvement:
function convertDate(date: Date | null): string { if (!date) { return ""; } try { return format(date, "yyyy-MM-dd'T'HH:mm:ss"); } catch (error) { console.error("Error formatting date:", error); return ""; } }
Line range hint
47-56: Simplify form's default values.The default values for the form could be simplified. Instead of using a ternary operator for
keyId, you can directly assign the value. Also, you can use the nullish coalescing operator forenableExpiration.Here's a suggested improvement:
defaultValues: { keyId: apiKey.id, enableExpiration: apiKey.expires != null, },apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx (2)
120-120: LGTM! Consider enhancing error handling.The addition of
awaitbeforeupdateRemaining.mutateAsync(values)is correct and aligns with the PR objectives. This change ensures that the asynchronous operation completes before proceeding, which is a best practice for handlingmutateAsynccalls.Consider wrapping the mutation call in a try-catch block for more granular error handling:
try { await updateRemaining.mutateAsync(values); } catch (error) { console.error('Error updating remaining uses:', error); toast.error('Failed to update remaining uses. Please try again.'); }This approach would allow you to provide more specific error messages to the user and log detailed error information for debugging purposes.
Line range hint
1-238: Consider extracting form schema and default values.The overall component structure and logic are well-implemented. To further improve code readability and maintainability, consider extracting the form schema and default values into separate constants or functions.
Here's an example of how you could refactor this:
const DEFAULT_FORM_VALUES = (apiKey: Props['apiKey']) => ({ keyId: apiKey.id, limitEnabled: apiKey.remaining ? true : false, remaining: apiKey.remaining ?? undefined, refill: { interval: apiKey.refillInterval ?? "none", amount: apiKey.refillAmount ?? undefined, }, }); // Place this outside the component const formSchema = z.object({ // ... (your existing schema) }); export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => { const form = useForm<z.infer<typeof formSchema>>({ resolver: zodResolver(formSchema), mode: "all", shouldFocusError: true, delayError: 100, defaultValues: DEFAULT_FORM_VALUES(apiKey), }); // ... (rest of your component) }This refactoring would make the component more modular and easier to maintain, especially if you need to reuse the form schema or default values elsewhere in your application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-enabled.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-expiration.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-owner-id.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-ratelimit.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (1 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/root-keys/[keyId]/update-root-key-name.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1)
Line range hint
1-93: Overall impact: Positive change with no adverse effects.The addition of
awaitto themutateAsynccall in theonSubmitfunction aligns well with the PR objectives and improves the handling of asynchronous operations. This change doesn't introduce any new dependencies or alter the component's structure, maintaining its overall integrity.To ensure consistency across the codebase, let's verify if similar changes have been applied to other
mutateAsynccalls:This script will help identify any remaining instances of
mutateAsynccalls that might need similar treatment, ensuring consistency with this change across the codebase.✅ Verification successful
Verification Successful: All
mutateAsynccalls are properly awaited.The changes in this PR are consistent with the rest of the codebase, and no unawaited
mutateAsynccalls were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unhandled mutateAsync calls in the codebase # Test 1: Search for mutateAsync calls without await echo "Searching for potentially unhandled mutateAsync calls:" rg --type typescript --type tsx 'mutateAsync\(' -C 3 | rg -v 'await.*mutateAsync' # Test 2: Verify the changes in this PR echo "Verifying changes in this PR:" git diff origin/main | rg 'await.*mutateAsync'Length of output: 806
Script:
#!/bin/bash # Description: Check for unhandled mutateAsync calls in the codebase # Test 1: Search for mutateAsync calls without await echo "Searching for potentially unhandled mutateAsync calls:" rg --type ts --type tsx 'mutateAsync\(' -C 3 | rg -v 'await\s+mutateAsync' # Test 2: Verify the changes in this PR echo "Verifying changes in this PR:" git diff origin/main | rg 'await\s+mutateAsync'Length of output: 342
Script:
#!/bin/bash # Description: Check for unhandled mutateAsync calls in the codebase # Test 1: Search for mutateAsync calls without await echo "Searching for potentially unhandled mutateAsync calls:" rg 'mutateAsync\(' --glob '*.{ts,tsx}' -C 3 | rg -v 'await\s+mutateAsync' # Test 2: Verify the changes in this PR echo "Verifying changes in this PR:" git diff origin/main | rg 'await\s+mutateAsync'Length of output: 11189
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (1)
Line range hint
1-91: Overall assessment: Changes successfully address the PR objectiveThe modification to add
awaitto themutateAsynccall in theonSubmitfunction successfully addresses the main objective of the PR. This change improves the reliability of the asynchronous operation handling in the component.Key points:
- The implementation is correct and follows best practices for handling asynchronous operations.
- The change is minimal and focused, affecting only the necessary part of the code.
- The overall structure and functionality of the component remain intact.
While the primary goal has been achieved, consider the suggested improvements for error handling to further enhance the robustness and user experience of the component.
apps/dashboard/app/(app)/settings/root-keys/[keyId]/update-root-key-name.tsx (1)
62-62: Correct implementation ofawaitformutateAsync.The addition of
awaittoupdateName.mutateAsync(values)is correct and aligns with the PR objective. This ensures that the mutation completes before proceeding with any subsequent code execution.To ensure consistency, let's verify if there are any other instances of
mutateAsyncin this file that might need similar treatment:If the script returns any results, please review those instances and consider adding
awaitwhere appropriate.✅ Verification successful
All
mutateAsynccalls are properly awaited.No instances of
mutateAsyncwithoutawaitwere found inapps/dashboard/app/(app)/settings/root-keys/[keyId]/update-root-key-name.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other instances of mutateAsync in the file # Test: Search for mutateAsync calls without await rg --type typescript 'mutateAsync(?!.*await)' 'apps/dashboard/app/(app)/settings/root-keys/[keyId]/update-root-key-name.tsx'Length of output: 363
Script:
#!/bin/bash # Description: Check for mutateAsync calls without await # Find lines with 'mutateAsync(' not preceded by 'await' grep 'mutateAsync(' 'apps/dashboard/app/(app)/settings/root-keys/[keyId]/update-root-key-name.tsx' | grep -v 'await'Length of output: 116
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-name.tsx (1)
64-64: Excellent addition ofawaittomutateAsyncThe addition of the
awaitkeyword beforeupdateName.mutateAsync(values)is a crucial improvement. This change ensures that the function waits for the mutation to complete before proceeding, which can prevent potential race conditions or unexpected behavior in the application flow.This modification aligns perfectly with the PR objectives and addresses the issue of unawaited
mutateAsynccalls. Great job on enhancing the reliability of the asynchronous operations in this component!apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-enabled.tsx (1)
59-59: Excellent addition ofawaitto the mutation call.This change correctly addresses the issue of not awaiting
mutateAsynccalls. By addingawait, we ensure that the mutation completes before the function continues execution, which is crucial for proper asynchronous flow control. This improvement aligns with best practices for handling asynchronous operations and enhances the reliability of the component.apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (1)
68-68: LGTM: Correct usage ofawaitwithmutateAsyncThe addition of
awaittoupdateIps.mutateAsync(values)is correct and aligns with the PR objectives. This change ensures that the function waits for the mutation to complete before proceeding, which is a best practice for handling asynchronous operations.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-expiration.tsx (2)
82-82: Excellent addition ofawaitto the mutateAsync call.This change correctly addresses the issue of not awaiting the
mutateAsyncfunction call. By addingawait, we ensure that the function waits for the mutation to complete before proceeding, which is crucial for proper asynchronous operation handling. This improvement aligns with best practices and enhances the reliability of the code.
Line range hint
1-145: Overall, good job addressing the main issue.The primary objective of adding
awaitto themutateAsynccall has been successfully implemented. This change improves the handling of asynchronous operations in the component. A few minor suggestions have been provided to further enhance the code quality, including improving error handling, refining the date conversion function, and simplifying form default values. These suggestions are not critical but could contribute to making the code more robust and maintainable.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-ratelimit.tsx (1)
97-97: Excellent addition ofawaittomutateAsynccall.The addition of
awaittoupdateRatelimit.mutateAsync(values)is a positive change that aligns with best practices for handling asynchronous operations. This modification ensures that:
- The mutation completes before the function continues execution.
- Any errors during the mutation are properly caught and handled.
- The success and error handlers defined in the mutation setup are triggered at the appropriate time.
This change enhances the reliability of the rate limit update process and prevents potential race conditions or unhandled promise rejections.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx (1)
Line range hint
1-238: Overall, the changes look good and meet the PR objectives.The addition of
awaitto themutateAsynccall addresses the main goal of the PR. The component is well-structured, uses appropriate form validation, and follows React best practices. The suggested improvements (enhanced error handling and code organization) are minor and aimed at further enhancing code quality and maintainability.
|
Awarding geraldmaboshe: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/geraldmaboshe |
What does this PR do?
Fixes #2205
This PR adds await to mutateAsync function calls
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
onSubmitfunctions asynchronous across various components, ensuring proper execution order and error handling.