fix: reset form values for delete role and delete permission dialogs#2529
fix: reset form values for delete role and delete permission dialogs#2529ChaseNelson wants to merge 4 commits intounkeyed:mainfrom
Conversation
ChaseNelson
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
@ChaseNelson 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 introduce a new function 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
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: 0
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1)
78-78: Good use of the newhandleDialogOpenChangefunction!The
Dialogcomponent now correctly uses thehandleDialogOpenChangefunction for itsonOpenChangeprop. This change effectively implements the form reset functionality when the dialog state changes.For consistency, consider updating the "Cancel" button's onClick handler to use
handleDialogOpenChangeas well:-onClick={() => setOpen(!open)} +onClick={() => handleDialogOpenChange(false)}This would ensure that the form reset logic is applied consistently across all dialog close actions.
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (2)
77-77: Good update to Dialog component!The
onOpenChangeprop now uses the newhandleDialogOpenChangefunction, which is a clean and maintainable approach. This ensures that the form reset occurs whenever the dialog's state changes.For consistency, consider updating the
onClickhandler of the Cancel button (around line 120) to usehandleDialogOpenChange(!open)instead of directly setting the state. This would centralize all dialog open state changes through the new function.
67-77: Excellent implementation addressing the reset issue!The changes effectively solve the problem of input values not resetting in delete dialogs, as outlined in the PR objectives. The new
handleDialogOpenChangefunction and its usage in the Dialog component create a robust solution that prevents user confusion and potential accidental deletions.One minor suggestion for improvement: Consider enhancing the error handling in the
deletePermissionmutation'sonErrorcallback. Instead of just displaying the error message, you could add more context, like:onError(err) { toast.error(`Failed to delete permission: ${err.message}`); },This would provide users with clearer information about what went wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (2)
68-71: Excellent addition to reset form on dialog state change!The new
handleDialogOpenChangefunction effectively addresses the issue of input values not resetting in delete dialogs. By callingform.reset()whenever the dialog's state changes, it ensures that the form is cleared each time the dialog is opened or closed. This implementation aligns perfectly with the PR objectives and resolves the problem described in issue #2521.
Line range hint
1-138: Overall, excellent implementation addressing the reported issue!The changes in this file effectively solve the problem of input values not resetting in delete dialogs, as reported in issue #2521. The new
handleDialogOpenChangefunction, combined with its usage in theDialogcomponent, ensures that the form is reset whenever the dialog is opened or closed. This implementation:
- Improves user experience by clearing previous inputs.
- Reduces the risk of accidental deletions.
- Maintains code clarity and maintainability.
The solution is well-integrated into the existing component structure and aligns perfectly with the PR objectives. Great job on this fix!
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (2)
67-70: Great addition to reset form on dialog state change!The new
handleDialogOpenChangefunction effectively addresses the issue of input values not resetting in delete dialogs. By resetting the form whenever the dialog's state changes, it ensures a clean slate for each interaction, preventing potential user confusion and accidental deletions.
Line range hint
1-140: Overall, great job on addressing the reset issue!The changes in this file effectively solve the problem described in the PR objectives. The implementation is clean, maintainable, and significantly improves the user experience by preventing potential confusion and errors when deleting permissions.
A few minor suggestions have been made for further improvements, but these are not critical. The core functionality has been successfully implemented and meets the requirements outlined in the PR objectives.
|
looks like those new functions aren't used |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (2)
Line range hint
134-138: Use handleDialogOpenChange in Cancel button.For consistency, use the
handleDialogOpenChangefunction instead of directly manipulating the open state. This ensures the form is properly reset when cancelled.<Button type="button" disabled={deleteRole.isLoading} - onClick={() => setOpen(!open)} + onClick={() => handleDialogOpenChange(false)} variant="secondary" >🧰 Tools
🪛 Biome
[error] 68-68: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
Line range hint
89-99: Consider adding loading state to form fields.While the submit button shows a loading state, consider disabling the form input during submission to prevent multiple submissions and improve UX.
<FormControl> - <Input {...field} autoComplete="off" /> + <Input {...field} autoComplete="off" disabled={deleteRole.isLoading} /> </FormControl>🧰 Tools
🪛 Biome
[error] 68-68: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (2)
Line range hint
60-63: Consider enhancing error handling.While the current error handling works, consider these improvements:
- Use user-friendly error messages instead of raw error messages
- Add error logging for debugging purposes
Here's a suggested implementation:
onError(err) { + console.error('Failed to delete permission:', err); + const userMessage = 'Failed to delete permission. Please try again or contact support.'; - toast.error(err.message); + toast.error(userMessage); },🧰 Tools
🪛 Biome
[error] 67-67: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
Line range hint
124-131: Use handleDialogOpenChange for Cancel button.The Cancel button directly toggles the open state using
setOpen(!open), bypassing the form reset functionality. This should usehandleDialogOpenChangefor consistency.Update the Cancel button's onClick handler:
<Button type="button" disabled={deletePermission.isLoading} - onClick={() => setOpen(!open)} + onClick={() => handleDialogOpenChange(false)} variant="secondary" > Cancel </Button>🧰 Tools
🪛 Biome
[error] 67-67: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx
[error] 67-67: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx
[error] 68-68: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
🔇 Additional comments (2)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1)
Line range hint
101-104: LGTM: Form reset implementation.The implementation of
handleDialogOpenChangecorrectly addresses the PR objective by resetting the form values when the dialog's state changes. This ensures a clean form state each time the dialog is opened.🧰 Tools
🪛 Biome
[error] 68-68: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1)
Line range hint
77-80: LGTM: Form reset implementation addresses the issue.The implementation correctly handles form reset when the dialog state changes, addressing the original issue where input values weren't clearing between dialog sessions.
🧰 Tools
🪛 Biome
[error] 67-67: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
| function handleDialogOpenChange(newState: boolean) { | ||
| setOpen(newState); | ||
| form.reset(); | ||
| } |
There was a problem hiding this comment.
Remove duplicate function declaration.
The handleDialogOpenChange function is declared twice in the component. Remove the first declaration to fix the error.
- function handleDialogOpenChange(newState: boolean) {
- setOpen(newState);
- form.reset();
- }Also applies to: 101-104
🧰 Tools
🪛 Biome
[error] 68-68: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
| function handleDialogOpenChange(newState: boolean) { | ||
| setOpen(newState); | ||
| form.reset(); | ||
| } |
There was a problem hiding this comment.
Remove duplicate function definition.
The handleDialogOpenChange function is defined twice with identical implementation. This appears to be a merge or copy-paste error.
Remove one of the duplicate function definitions:
- function handleDialogOpenChange(newState: boolean) {
- setOpen(newState);
- form.reset();
- }Also applies to: 77-80
🧰 Tools
🪛 Biome
[error] 67-67: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
|
Looks like #2546 fixed this issue |
3 participants
What does this PR do?
Fixes #2521
ENG-1523
https://www.loom.com/share/1cf0dc720fc14f769873080764dff5fb?sid=5be4896c-b02b-498e-80a9-39c550d48dce
https://www.loom.com/share/544a3ab13abb483f949fd0fef7e54f34?sid=cbcc7ef0-b4a6-45b5-bd77-7d888e7b276d
Type of change
How should this be tested?
Test Delete Role
Authorization > Roles > Role > Delete RoleTest Delete Permission
Authorization > Permissions > Permission > Delete PermissionChecklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes