fix: reset form fields in delete dialog on reopen #2546
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. |
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several components, including Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
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: 3
🧹 Outside diff range and nitpick comments (4)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (3)
Line range hint
134-138: Use handleDialogOpenChange in Cancel buttonFor consistency, consider using the new handler in the Cancel button instead of directly manipulating the open state.
<Button type="button" disabled={deleteRole.isLoading} - onClick={() => setOpen(!open)} + onClick={() => handleDialogOpenChange(false)} variant="secondary" >
86-87: Consider enhancing the warning messageWhile the current message is clear, consider being more specific about the impact on existing permissions.
- This role will be deleted, keys with this role will be disconnected - from all permissions granted by this role. + This role will be permanently deleted. All API keys associated with this role + will immediately lose access to the permissions granted by this role.
97-99: Consider enhancing the warning messageWhile the current warning is clear, consider adding more context about the implications.
- This action is not reversible. Please be certain. + This action is not reversible and will immediately affect API access. + Please be certain before proceeding.apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (1)
89-92: Consider adding aria-label for better accessibilityWhile the form structure is good, adding an aria-label would improve accessibility.
<form className="flex flex-col space-y-8" onSubmit={form.handleSubmit(onSubmit)} + aria-label="Delete permission confirmation form" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (4 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (8)
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (4)
46-48: LGTM! Improved validation formattingThe reformatted validation logic enhances readability while maintaining the same functionality.
74-77: LGTM! Properly implements form resetThe new handler correctly implements the form reset functionality, addressing the issue of persisting values.
91-94: LGTM! Well-structured form implementationThe form structure follows React Hook Form best practices with proper event handling.
110-113: LGTM! Improved label formattingThe label formatting changes enhance readability and maintain consistent styling.
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx (4)
73-76: Well-implemented form reset functionalityThe
handleDialogOpenChangefunction effectively addresses the issue by resetting the form state when the dialog closes, which aligns perfectly with the PR objectives.
79-79: Clean dialog state managementThe Dialog component correctly uses the new handler for managing open state and form reset.
108-111: Good handling of long permission namesThe use of
break-allclass and proper spacing ensures that long permission names are displayed correctly without breaking the layout.
73-76: Verify consistent implementation across other dialog componentsLet's ensure this pattern is consistently implemented across other similar dialogs in the codebase.
✅ Verification successful
Let me adjust the search patterns and try again with correct syntax.
The previous script had issues with file type flags. Let's search more broadly for dialog components with
onOpenChangehandlers and form reset patterns across all files. This will help us verify if the form reset implementation is consistent with other similar dialogs in the codebase.
Let me try one more search to specifically look for dialog open state handling patterns:
This will help us verify if the pattern of combining form reset with dialog state changes is consistently implemented across the codebase.
Based on the search results, I can now provide a final response:
Form reset implementation is consistent with similar dialog components
The implementation of
handleDialogOpenChangein the delete-permission component follows the established pattern across the codebase. Specifically:
- The same pattern is used in other deletion dialogs:
apps/dashboard/app/(app)/apis/[apiId]/settings/delete-api.tsxapps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx- Both implement form reset when dialog state changes
The current implementation aligns with the codebase's best practices for handling form state in dialog components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dialog components that might need similar reset functionality echo "Searching for other dialog components with form implementations..." rg -l "Dialog.*onOpenChange" --type tsx # Look for potential inconsistencies in form reset patterns echo "Checking form reset patterns..." ast-grep --pattern 'const [_]+ = useForm<' --type tsxLength of output: 521
Script:
#!/bin/bash # Search for dialog components with form implementations echo "Searching for dialog components with form reset patterns..." rg -A 5 "onOpenChange.*=.*{" # Look for form reset patterns echo "Checking form reset patterns..." rg -A 3 "form\.reset\(\)"Length of output: 2022
Script:
#!/bin/bash # Search for dialog open state handling patterns echo "Searching for dialog open state handling..." rg -A 5 "setOpen\(" # Look for combined patterns of form reset and dialog state echo "Checking combined form reset and dialog state patterns..." rg -C 5 "setOpen.*form\.reset|form\.reset.*setOpen"Length of output: 20621
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/authorization/permissions/[permissionId]/delete-permission.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (1)
78-81: Consider extracting dialog reset logic into a custom hook.Since this form reset pattern is being implemented across multiple dialogs (create and delete), consider extracting it into a reusable custom hook like
useDialogFormResetto maintain consistency and reduce code duplication.Example implementation:
function useDialogFormReset(form: UseFormReturn) { const [open, setOpen] = useState(false); const handleDialogOpenChange = useCallback((newState: boolean) => { setOpen(newState); if (!newState) { form.reset(); } }, [form]); return { open, handleDialogOpenChange }; }apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/delete-namespace.tsx (2)
Line range hint
108-113: Use handleDialogOpenChange consistently.The onClick handler still uses the old pattern while the Dialog uses the new handleDialogOpenChange.
- <Button type="button" onClick={() => setOpen(!open)} variant="alert"> + <Button type="button" onClick={() => handleDialogOpenChange(true)} variant="alert">
86-89: Consider adding debounce to prevent rapid delete requests.For destructive operations, it's recommended to prevent multiple rapid submissions that could occur from double-clicking or similar user actions.
Example implementation using the
useMemohook:const debouncedOnSubmit = useMemo( () => debounce(async (values: z.infer<typeof formSchema>) => { deleteNamespace.mutate({ namespaceId: namespace.id }); }, 1000), [deleteNamespace, namespace.id] );apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (2)
95-98: LGTM! Consider adding error handling.The new
handleDialogOpenChangefunction correctly implements the form reset functionality, addressing the PR objective.Consider adding error handling for the form reset:
function handleDialogOpenChange(newState: boolean) { setOpen(newState); - form.reset(); + try { + form.reset(); + } catch (error) { + console.error('Failed to reset form:', error); + toast.error('Failed to reset form fields'); + } }
192-196: LGTM! Consider adding aria-label for accessibility.The loading state implementation is clean and effective.
Consider adding an aria-label to improve accessibility:
-<Button type="submit"> +<Button type="submit" aria-label={createRole.isLoading ? "Creating role..." : "Create role"}> {createRole.isLoading ? ( <Loading className="w-4 h-4" /> ) : ( "Create" )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (3 hunks)
- apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (6 hunks)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/delete-namespace.tsx (7 hunks)
- apps/dashboard/app/(app)/semantic-cache/[gatewayId]/settings/delete-gateway.tsx (7 hunks)
🔇 Additional comments (11)
apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (3)
95-98: LGTM! Form structure improvements.The added className and explicit form structure improve readability and layout consistency.
89-91: LGTM! Improved text formatting and documentation.The text formatting changes enhance readability, and the proper use of code tags for technical terms improves clarity.
Also applies to: 109-112
78-81: Verify if this change aligns with PR objectives.While the form reset implementation is correct, this appears to be in a "Create Permission" dialog rather than a "Delete" dialog as specified in the PR objectives. The change might be extending beyond the original scope.
Also applies to: 84-84
apps/dashboard/app/(app)/semantic-cache/[gatewayId]/settings/delete-gateway.tsx (4)
55-60: LGTM! Improved readability of validation rules.The reformatting enhances code readability while maintaining the same validation logic.
95-98: Well-implemented form reset functionality!The
handleDialogOpenChangefunction effectively addresses the PR objective by ensuring form fields are reset when the dialog is closed. This prevents stale data from persisting between dialog sessions.
125-128: LGTM! Improved form structure formatting.The form structure has been reformatted for better readability while maintaining the same functionality.
116-116: Verify consistent implementation across other dialogs.The Dialog implementation looks good, using the new handler correctly.
✅ Verification successful
Let me try a different approach to gather information about Dialog implementations.
These scripts should help us:
- Find Dialog component usage patterns
- Understand where Dialog is imported from
- Locate similar delete-related files that might contain dialogs
Based on the gathered information, I can now provide a final response:
Dialog implementation is consistently handled across the codebase
The verification shows that Dialog components with
openandonOpenChangeprops are consistently implemented across the codebase:
- All delete-related dialogs (delete-gateway, delete-namespace, delete-api, delete-permission, delete-role, delete-key) use the
handleDialogOpenChangepattern- Other dialogs also properly manage their open state through similar patterns using either
handleDialogOpenChangeor direct state setters- The Dialog component is properly imported from Radix UI (
@radix-ui/react-dialog) across all implementations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other delete dialogs implement similar reset functionality # Test: Look for similar dialog implementations ast-grep --pattern 'Dialog open={$_} onOpenChange={$_}'Length of output: 57
Script:
#!/bin/bash # Search for Dialog components with open and onOpenChange props using ripgrep # Using more flexible pattern matching rg -U "<Dialog.*open=.*onOpenChange.*>" -A 2Length of output: 4921
Script:
#!/bin/bash # Also search for potential Dialog imports to understand usage rg "import.*Dialog.*from"Length of output: 2612
Script:
#!/bin/bash # Look for files containing "delete" in their name to find similar delete dialogs fd "delete" --type fLength of output: 3071
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/delete-namespace.tsx (1)
91-94: LGTM! Form reset implementation addresses the issue.The implementation correctly resets the form when the dialog state changes, addressing the issue of persisting values.
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (3)
101-101: LGTM! Clean implementation of dialog state management.The Dialog component correctly uses the new
handleDialogOpenChangefunction, ensuring form fields are reset when the dialog is reopened.
178-181: LGTM! Clean implementation of form value updates.The MultiSelect callback is well-structured and properly handles form value updates.
95-98: Verify form reset behavior in error scenarios.While the implementation looks good, we should ensure the form reset works correctly in all scenarios.
Also applies to: 101-101
✅ Verification successful
Let me try different commands to gather the necessary information.
The previous commands failed due to file type flags, so I've simplified them to search across all files. Additionally, I've added an AST-based search to find similar dialog handling patterns. This should help us verify the form reset behavior and consistency across the codebase.
Form reset implementation is consistent with established patterns
The verification results show that the form reset behavior in
create-new-role.tsxfollows a well-established pattern across the codebase:
- The same
handleDialogOpenChangepattern withform.reset()is consistently implemented in multiple dialog components, including delete operations for permissions, APIs, and gateways- The form reset is properly tied to dialog state changes, which handles both open and close scenarios
- Error scenarios are implicitly handled since the form reset occurs on dialog state change, regardless of the previous form state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dialog components that might need similar reset behavior rg -l "onOpenChange.*setOpen" --type typescript --type typescriptreact # Search for potential form reset patterns that should be consistent rg "form\.reset" --type typescript --type typescriptreactLength of output: 202
Script:
#!/bin/bash # Search for dialog components with open state management rg "onOpenChange.*setOpen" # Search for form reset patterns rg "form\.reset" # Look for similar dialog components ast-grep --pattern 'function $_(newState: boolean) { setOpen(newState); $$$ }'Length of output: 6548
apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/semantic-cache/[gatewayId]/settings/delete-gateway.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/delete-namespace.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/delete-namespace.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx
Outdated
Show resolved
Hide resolved
|
@perkinsjr / @mcstepp / @chronark Could you please take a look at this PR |
|
Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619 |
What does this PR do?
This PR addresses the issue where input field values in the delete dialogs for roles and permissions do not reset after closing and reopening the dialogs. This fix improves user experience by ensuring that previously entered values are cleared, preventing confusion.
Fixes #2521
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Refactor