fix: reset input fields in API delete dialog after reopening #2316
Conversation
Vardhaman619
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Vardhaman619 is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the addition of a Changes
Assessment against linked issues
Possibly related PRs
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 (
|
|
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: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (1)
87-92: LGTM: Form reset functionality implemented correctly.The added useEffect hook successfully addresses the issue of form fields not resetting when the dialog is reopened. It correctly triggers the form reset when the dialog is closed, which aligns with the PR objectives.
Consider optimizing the dependency array by using
useCallbackforform.reset:const resetForm = useCallback(() => form.reset(), [form]); useEffect(() => { if (!open) { resetForm(); } }, [open, resetForm]);This optimization ensures that the
resetFormfunction reference remains stable across renders, potentially reducing unnecessary effect triggers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (2)
4-4: LGTM: Import statement updated correctly.The addition of
useEffectto the import statement is appropriate for implementing the form reset functionality, which aligns with the PR objectives.
Line range hint
1-193: Summary: Changes successfully address the PR objectives.The implemented changes effectively solve the issue of form fields not resetting when the API delete dialog is reopened. The addition of the useEffect hook to reset the form when the dialog closes directly addresses the problem described in the PR objectives and the linked issue.
The changes are minimal, focused, and don't introduce any unrelated modifications to the component. This approach maintains the existing functionality while improving the user experience by ensuring that the form is always in a clean state when opened.
To ensure that these changes don't affect other parts of the codebase, please run the following verification script:
✅ Verification successful
Verification Successful
The verification confirms that the
DeleteApicomponent is used appropriately within the codebase. No other components utilize theopenstate in a manner that requires similar updates. The changes made effectively address the initial issue without introducing any unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the DeleteApi component is only used in the expected locations and that no other components are using the 'open' state in a similar manner that might need updating. # Test 1: Check for usage of DeleteApi component echo "Checking for usage of DeleteApi component:" rg --type typescript --type typescriptreact "import.*DeleteApi" -g '!delete-api.tsx' # Test 2: Check for similar patterns that might need updating echo "Checking for similar patterns that might need updating:" rg --type typescript --type typescriptreact "useEffect\(.+open.+form\.reset" -g '!delete-api.tsx' # Note: If these searches return results, they may indicate areas that need similar updates or potential inconsistencies in the codebase.Length of output: 472
Script:
#!/bin/bash # Description: Verify that the DeleteApi component is only used in the expected locations and that no other components are using the 'open' state in a similar manner that might need updating. # Test 1: Check for usage of DeleteApi component excluding delete-api.tsx echo "Checking for usage of DeleteApi component:" rg "import.*DeleteApi" -g '*.ts' -g '*.tsx' -g '!delete-api.tsx' # Test 2: Check for similar useEffect patterns that reset the form based on 'open' state excluding delete-api.tsx echo "Checking for similar useEffect patterns that might need updating:" rg "useEffect\(.+open.+form\.reset" -g '*.ts' -g '*.tsx' -g '!delete-api.tsx' # Note: If these searches return results, they may indicate areas that need similar updates or potential inconsistencies in the codebase.Length of output: 1463
|
/assign |
|
Assigned to @ankurjaiswalofficial! Please open a draft PR linking this issue within 48h |
|
@Vardhaman619 I think useEffect is overkill and makes it more complicated than it needs to be the modal itself already gives us a way to hook into the close event Let's use that instead |
…nge function for dialog
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (1)
124-124: Approve Dialog component change and suggest cleanupThe
onOpenChangeprop is correctly set tohandleDialogOpenChange, which effectively implements the form reset functionality when the dialog state changes. This change addresses the PR objective and aligns with the suggestion to use the modal's built-in close event hook.Given that this implementation doesn't use
useEffect, please remove any unuseduseEffecthooks in the component if present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (2)
Line range hint
1-192: Summary of changes and effectivenessThe implemented changes effectively address the PR objective of resetting input fields in the API delete dialog after reopening. The solution uses a direct approach by leveraging the Dialog component's
onOpenChangeprop, which is simpler and more maintainable than using auseEffecthook.Key points:
- The
handleDialogOpenChangefunction combines setting the dialog state and resetting the form.- The Dialog component correctly uses this function in its
onOpenChangeprop.These changes should resolve the issue of persisting input values when reopening the dialog, improving the user experience as intended.
To ensure completeness:
- Verify that there are no unused
useEffecthooks in the component.- Consider adding a brief comment explaining the purpose of
handleDialogOpenChangefor future maintainability.
87-90: Approve new function and verify its usageThe new
handleDialogOpenChangefunction effectively addresses the PR objective by resetting the form when the dialog state changes. This is a good approach to ensure that input fields are cleared when the dialog is reopened.Please ensure that this function is being used correctly. Run the following script to verify its usage:
✅ Verification successful
Update Shell Script to Verify Usage of
handleDialogOpenChangePlease run the following updated script to verify the usage of the
handleDialogOpenChangefunction:
Update Shell Script to Search
.tsand.tsxFiles Without--typeFlagsPlease run the following updated script to verify the usage of the
handleDialogOpenChangefunction:
Function
handleDialogOpenChangeusage verifiedThe
handleDialogOpenChangefunction is correctly implemented and utilized withindelete-api.tsx. It effectively resets the form when the dialog state changes, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of handleDialogOpenChange function # Test: Search for handleDialogOpenChange usage rg --type typescript 'handleDialogOpenChange' apps/dashboard/app/Length of output: 100
Script:
#!/bin/bash # Description: Verify the usage of handleDialogOpenChange function # Test: Search for handleDialogOpenChange usage rg 'handleDialogOpenChange' --type ts --type tsx apps/dashboard/app/Length of output: 96
Script:
#!/bin/bash # Description: Verify the usage of handleDialogOpenChange function # Test: Search for handleDialogOpenChange usage in .ts and .tsx files rg 'handleDialogOpenChange' --glob '*.ts' --glob '*.tsx' apps/dashboard/app/Length of output: 318
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (1)
22-25: LGTM with suggestion: Import statements reorganizedThe separation of import statements improves readability and follows best practices. However, the
useEffectimport is missing, which might be needed based on the AI summary.Consider adding the
useEffectimport if it's used in the component:-import { useState } from "react"; +import { useEffect, useState } from "react";Also applies to: 28-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsx (4)
4-5: LGTM: Import statements separatedThe separation of import statements for
ButtonandCardcomponents improves readability and follows best practices.
84-88: LGTM: Implemented form reset on dialog state changeThe new
handleDialogOpenChangefunction effectively addresses the PR objective by resetting the form when the dialog is closed. This implementation aligns with the feedback to use the modal's close event, providing a simple and effective solution.
121-121: LGTM: Dialog component updated to use new handlerThe
Dialogcomponent has been correctly updated to use the newhandleDialogOpenChangefunction for itsonOpenChangeprop. This change ensures that the form reset occurs when the dialog state changes, aligning with the PR objectives.
Line range hint
1-191: Great job: Implemented solution without unnecessary complexityThe changes successfully address the PR objectives without using
useEffect, aligning with the feedback provided by chronark. The implementation is simpler and more direct, which is preferable.To ensure there are no leftover artifacts from previous implementations, please run the following script to check for any unused imports:
If the script returns any results, consider removing the unused imports.
✅ Verification successful
Action Required: Update Shell Script with Correct File Type
The previous shell script failed due to an unrecognized file type
typescript. Please update the script to use the correct file typetsxfor TypeScript React files.
Action Required: Update Shell Script to Use Glob Pattern for .tsx Files
The
--type tsxflag is not recognized by ripgrep. Instead, use a glob pattern to target.tsxfiles directly.
Verification Complete: No Unused Imports Found
The
delete-api.tsxfile does not include any unused imports, specificallyuseEffect. The implementation is clean and aligns with the review objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Search for unused imports, particularly useEffect rg --type typescript 'import.*useEffect' apps/dashboard/app/\(app\)/apis/\[apiId\]/settings/delete-api.tsxLength of output: 141
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Search for unused imports, particularly useEffect rg --type tsx 'import.*useEffect' apps/dashboard/app/\(app\)/apis/\[apiId\]/settings/delete-api.tsxLength of output: 127
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Search for unused imports, particularly useEffect rg --glob '*.tsx' 'import.*useEffect' apps/dashboard/app/\(app\)/apis/\[apiId\]/settings/delete-api.tsxLength of output: 103
|
Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619 |
…#2316) * fix: reset input fields in API delete dialog after reopening * refactor: remove unnecessary useEffect and utilize existing onOpenChange function for dialog * refactor: removed unused imports
…#2316) * fix: reset input fields in API delete dialog after reopening * refactor: remove unnecessary useEffect and utilize existing onOpenChange function for dialog * refactor: removed unused imports
* fix:breadcrum-cache-issue * chore:rename-revalidateMytag * refactor: move-revalidateTag * refactor: extract tags into reusable utility * feat: add template * Update 3_follow_the_unkey_x_account.md (#2323) * fix: capture uniqueness error and return it correctly (#2337) * docs: getVerifications instead of get (#2329) * Update 3_follow_the_unkey_x_account.md (#2324) Followed unkey on Twitter Co-authored-by: Andreas Thomas <dev@chronark.com> * fix: transaction ...: in use: for query (#2278) * fix: transaction ...: in use: for query - Fixes #2197 - The error is caused by the cache revalidation happening in the background, so we have a racecondition with the other queries we are running in the insertGenericAuditLogs function * docs: explain for future self * fix: another racecondition also bulk insert auditLogs and auditLogTargets * [autofix.ci] apply automated fixes --------- Co-authored-by: chronark <dev@chronark.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * fix: handle undefined ratelimits * Update 3_follow_the_unkey_x_account.md (#2338) * fix: handle case when blogs array is empty * [oss.gg] side quest : added another framework; hono on cloudflare workers. (#2345) * fix: weird spacing in changelog by removing the extra gap (#2340) * fix: weird spacing in changelog * feat: formatted * oss.gg: create a template for ratelimiting a oak server in deno with unkey (#2308) Co-authored-by: Andreas Thomas <dev@chronark.com> * follow unkey on X #2252 (#2315) Co-authored-by: Your Name <you@example.com> * Follow the Unkey X account: @unkeydev Complete! (#2332) * Update 3_follow_the_unkey_x_account.md * Update 3_follow_the_unkey_x_account.md --------- Co-authored-by: Andreas Thomas <dev@chronark.com> * fix(www): analytics bento code snippet is not readable (#2311) - add hover effect to analytics bento removing gradient background - make copy code button sticky to top-right corner - impl independent vertical scrolls for language switcher and code editor * Update 7_create_a_template.md * Update 7_create_a_template.md * chore(deps-dev): bump @content-collections/core from 0.6.2 to 0.7.2 (#2255) Bumps @content-collections/core from 0.6.2 to 0.7.2. --- updated-dependencies: - dependency-name: "@content-collections/core" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: order audit logs by time, show latest on top (#2295) * feat: add template * feat: Unkey FastAPI boilerplate (#2307) * feat: add template * feat: follow unkey on X (#2357) * Update 6_record_onboarding.md (#2301) * fix: reset input fields in API delete dialog after reopening (#2316) * fix: reset input fields in API delete dialog after reopening * refactor: remove unnecessary useEffect and utilize existing onOpenChange function for dialog * refactor: removed unused imports * Update 3_follow_the_unkey_x_account.md (#2364) Added name to the followed list. Co-authored-by: Andreas Thomas <dev@chronark.com> * ci: add label * feat: add hover to input fields * Added gaps and width for md (#2371) * docs: update overview.mdx (#2384) avaliable -> available * oss.gg side quest 3_follow_the_unkey_x_account.md (#2399) Followed the unkey x account * feat: Following unkey acc on twitter #2407 (#2408) * Feat: Followed Unkey on X * Feat: Followed Unkey on Twitter --------- Co-authored-by: Andreas Thomas <dev@chronark.com> * resolve merge conflicts * resolve merge conflicts --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: chronark <dev@chronark.com> Co-authored-by: Emily Marie Ahtúnan <86572370+Emmarie-Ahtunan@users.noreply.github.com> Co-authored-by: Harsh Shrikant Bhat <90265455+harshsbhat@users.noreply.github.com> Co-authored-by: Aritra Sadhukhan <60315087+aritradevelops@users.noreply.github.com> Co-authored-by: Flo <53355483+Flo4604@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Devang Rakholiya <116096508+Devang0907@users.noreply.github.com> Co-authored-by: Prabin <42871240+prabincankod@users.noreply.github.com> Co-authored-by: ZiaCodes <72739794+Khaan25@users.noreply.github.com> Co-authored-by: Devansh Baghel <77718741+Devansh-Baghel@users.noreply.github.com> Co-authored-by: Shreenarayan jaiswal <127131074+shreenarayan123@users.noreply.github.com> Co-authored-by: Your Name <you@example.com> Co-authored-by: Chirag Arora <chirag8023@gmail.com> Co-authored-by: Nazar Poshtarenko <32395926+unrenamed@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Saketh Pavan Goti <44170829+Sakethpavan@users.noreply.github.com> Co-authored-by: Teddy ASSIH <110066080+Ionfinisher@users.noreply.github.com> Co-authored-by: Vardhaman Bhandari <97441447+Vardhaman619@users.noreply.github.com> Co-authored-by: Atharva_404 <72994819+Atharva-3000@users.noreply.github.com> Co-authored-by: Rishi <rishipatel0826@gmail.com> Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com> Co-authored-by: Ayan Mondal <ayanmondal1805@gmail.com> Co-authored-by: Kunal Arya <105645118+kunalarya873@users.noreply.github.com>
What does this PR do?
This PR fixes the issue where form fields in the "API Delete" dialog do not reset upon reopening. Previously, input values persisted after the dialog was closed and reopened. Now, the form fields are properly reset to their default state when the dialog is opened again.
Fixes #2296
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes