fix: toasts-not-dismissings #2304
Conversation
AkshayBandi027
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
@AkshayBandi027 is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request involve modifications to the Changes
Assessment against linked issues
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! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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 (6)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx (3)
17-17: LGTM: loadingToastId ref added correctly.The
useRefhook is appropriately used to manage the loading toast ID across re-renders. This change aligns well with the PR objective of fixing toast dismissal issues.Consider adding a brief comment explaining the purpose of this ref, e.g.:
// Ref to store the ID of the current loading toast for proper dismissal const loadingToastId = useRef<string | number | null>(null);
37-38: LGTM: Toast dismissal logic implemented correctly.The toast dismissal logic has been properly implemented in both the
onSuccessandonErrorcallbacks. This directly addresses the toast dismissal issue mentioned in the PR objective.Consider extracting the dismissal logic into a separate function to reduce code duplication:
const dismissLoadingToast = () => { toast.dismiss(loadingToastId.current!); loadingToastId.current = null; }; // Then use it in both callbacks: onSuccess: () => { // ... existing success logic dismissLoadingToast(); }, onError: (err) => { // ... existing error logic dismissLoadingToast(); },Also applies to: 43-44
66-67: LGTM: Consistent toast dismissal logic in disconnect mutation.The toast dismissal logic in the
disconnectmutation'sonSuccessandonErrorcallbacks is consistent with theconnectmutation, completing the fix for both operations.As suggested earlier, consider extracting the dismissal logic into a shared function to reduce code duplication across both mutations:
const dismissLoadingToast = () => { toast.dismiss(loadingToastId.current!); loadingToastId.current = null; }; // Use in all four callbacks (connect.onSuccess, connect.onError, disconnect.onSuccess, disconnect.onError)This refactoring would improve code maintainability and reduce the likelihood of inconsistencies in future updates.
Also applies to: 72-73
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1)
63-64: LGTM with a small suggestion: UpdateonSuccessandonErrorcallbacks.The changes in both callbacks correctly dismiss the loading toast using the stored ID and reset the ref. This ensures proper toast lifecycle management in both success and error scenarios.
However, I suggest a small improvement for safer null checking:
// In both onSuccess and onError callbacks if (loadingToastId.current !== null) { toast.dismiss(loadingToastId.current); loadingToastId.current = null; }This change will prevent potential runtime errors if
loadingToastId.currentis unexpectedly null.Also applies to: 69-70
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (2)
47-47: LGTM:loadingToastIdref added correctly.The use of
useRefforloadingToastIdis appropriate for persisting the toast ID across re-renders. The type annotation is comprehensive, covering all possible return types from the toast library.Consider adding a comment explaining the purpose of this ref, e.g.:
// Ref to store the ID of the loading toast for proper dismissal const loadingToastId = useRef<string | number | null>(null);This would enhance code readability and maintainability.
64-65: LGTM: Toast dismissal logic improved, with a minor suggestion.The loading toast is now properly dismissed using the stored ID, and the ref is correctly reset to null afterwards. This ensures proper toast management regardless of the mutation outcome.
Consider adding a null check before dismissing the toast to make the code more robust:
if (loadingToastId.current !== null) { toast.dismiss(loadingToastId.current); loadingToastId.current = null; }This change would prevent potential runtime errors if
loadingToastId.currentis unexpectedly null.Also applies to: 71-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx (5 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (3 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (11)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx (4)
8-8: LGTM: Import statement updated correctly.The import statement has been properly updated to include
useRef, which is necessary for the changes made in the component.
23-25: LGTM: Toast ID properly stored in ref.The loading toast ID is now correctly stored in the
loadingToastIdref. This change is crucial for proper toast dismissal later, addressing the main objective of the PR.
53-54: LGTM: Consistent implementation in disconnect mutation.The changes in the
disconnectmutation'sonMutatecallback are consistent with those in theconnectmutation, ensuring proper toast management for both operations.
Line range hint
1-94: Overall: Great implementation addressing toast dismissal issues.The changes in this file effectively solve the problem of toasts not being dismissed properly. The implementation is consistent across both connect and disconnect operations, using a
useRefhook to manage the loading toast ID. This approach ensures that toasts are correctly dismissed in both success and error scenarios.Key points:
- The
loadingToastIdref is correctly implemented and used.- Toast creation and dismissal logic is consistently applied in all relevant callbacks.
- The changes align well with the PR objective of fixing toast dismissal issues.
Suggestions for improvement:
- Consider adding a brief comment explaining the purpose of the
loadingToastIdref.- Extract the toast dismissal logic into a shared function to reduce code duplication and improve maintainability.
Overall, this is a solid implementation that effectively addresses the issue at hand.
apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (4)
28-28: LGTM: Import statement updated correctly.The
useRefhook has been appropriately added to the import statement from 'react'. This change is consistent with its usage in the component and follows the convention of grouping related imports.
42-42: LGTM:loadingToastIdref added correctly.The
loadingToastIdref is well-implemented usinguseRef. The type annotation<string | number | null>appropriately covers all possible return types from the toast library. Initializing withnullis a good practice for refs that may not have an initial value.
58-59: LGTM:onMutatecallback updated to store toast ID.The
onMutatecallback has been correctly updated to store the loading toast ID in theloadingToastIdref. This change is crucial for properly managing the toast lifecycle, allowing the ID to persist across re-renders and be accessed in other callbacks.
Line range hint
28-70: Overall: Great improvements in toast notification handling!The changes in this file consistently implement a new approach to managing toast notifications using a ref. This improves the handling of loading states and ensures proper lifecycle management of toast notifications. The implementation aligns well with React best practices and enhances the overall reliability of the component.
A minor suggestion was made for safer null checking in the
onSuccessandonErrorcallbacks, but overall, these changes represent a significant improvement in the component's functionality.apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (3)
30-30: LGTM: Import statement updated correctly.The addition of
useRefto the import statement is consistent with its usage in the component logic for managing theloadingToastId.
59-60: LGTM: Loading toast ID properly stored.The loading toast ID is correctly stored in
loadingToastId.current, allowing for proper management of the toast throughout the component lifecycle.
Line range hint
1-153: Summary: PR successfully addresses toast dismissal issue.This PR effectively resolves the issue of toasts not being dismissed as expected. The implementation of
useRefto manage the loading toast ID ensures proper lifecycle management of the toast notifications. These changes align well with the PR objective of fixing the "toasts-not-dismissing" issue.To fully meet the PR objectives:
- Ensure that you've linked this PR to the corresponding issue for tracking purposes.
- Confirm that you've tested the changes by navigating to the dashboard and updating a role at the specified URL path:
dashboard/authorization/roles/[roleId].To verify the changes, please run the following script:
This script will help ensure that the toast dismissal logic has been implemented consistently across all relevant files in the project.
✅ Verification successful
Verified: The toast dismissal logic has been consistently implemented across all relevant TypeScript files.
The verification scripts confirmed the presence of
loadingToastIdusage,toast.dismiss(loadingToastId.current!)calls, and the resetting ofloadingToastId.current = nullin the following files:
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsxapps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsxapps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsxThese changes effectively resolve the issue of toasts not being dismissed as expected, aligning with the PR's objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the toast dismissal logic is implemented consistently across all relevant files. # Test: Search for loadingToastId usage echo "Checking for loadingToastId usage:" rg --type typescript 'const loadingToastId = useRef<string \| number \| null>\(null\)' # Test: Search for toast.dismiss usage with loadingToastId echo "Checking for toast dismissal logic:" rg --type typescript 'toast\.dismiss\(loadingToastId\.current' # Test: Search for loadingToastId reset to null echo "Checking for loadingToastId reset:" rg --type typescript 'loadingToastId\.current = null'Length of output: 553
Script:
#!/bin/bash # Description: Verify that the toast dismissal logic is implemented consistently across all relevant TypeScript files. # Test: Search for loadingToastId usage in .ts and .tsx files echo "Checking for loadingToastId usage in TypeScript files:" rg 'const loadingToastId = useRef<string \| number \| null>\(null\)' --type-add 'typescript:*.ts,*.tsx' --type typescript # Test: Search for toast.dismiss usage with loadingToastId in .ts and .tsx files echo "Checking for toast dismissal logic in TypeScript files:" rg 'toast\.dismiss\(loadingToastId\.current' --type-add 'typescript:*.ts,*.tsx' --type typescript # Test: Search for loadingToastId reset to null in .ts and .tsx files echo "Checking for loadingToastId reset in TypeScript files:" rg 'loadingToastId\.current = null' --type-add 'typescript:*.ts,*.tsx' --type typescriptLength of output: 1120
Script:
#!/bin/bash # Description: Verify that the toast dismissal logic is implemented consistently across all relevant TypeScript files. # Test: Search for loadingToastId usage in .ts and .tsx files echo "Checking for loadingToastId usage in .ts and .tsx files:" rg 'const loadingToastId = useRef<string \| number \| null>\(null\)' --glob '*.ts' --glob '*.tsx' # Test: Search for toast.dismiss usage with loadingToastId in .ts and .tsx files echo "Checking for toast dismissal logic in .ts and .tsx files:" rg 'toast\.dismiss\(loadingToastId\.current' --glob '*.ts' --glob '*.tsx' # Test: Search for loadingToastId reset to null in .ts and .tsx files echo "Checking for loadingToastId reset in .ts and .tsx files:" rg 'loadingToastId\.current = null' --glob '*.ts' --glob '*.tsx'Length of output: 2867
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx (5)
19-19: LGTM: useRef for toast ID management.The addition of
loadingToastIdusinguseRefis a good approach for managing the toast ID across re-renders. The type annotation is comprehensive, covering all possible return types from the toast library.Consider using a more specific name like
loadingToastIdRefto clearly indicate that this is a ref, following common React naming conventions for refs.
36-37: LGTM with suggestion: Toast dismissal in onSuccess.The changes correctly dismiss the loading toast and reset the ref. This addresses the issue of toasts not dismissing after the action completes.
Consider adding a null check before dismissing the toast to handle potential edge cases:
if (loadingToastId.current !== null) { toast.dismiss(loadingToastId.current); loadingToastId.current = null; }This approach would be safer and prevent potential runtime errors if the ID is unexpectedly null.
43-44: LGTM: Consistent toast dismissal in onError.The changes in the
onErrorcallback mirror those inonSuccess, ensuring consistent behavior regardless of the mutation outcome. This contributes to resolving the issue of toasts not dismissing properly.Apply the same null check suggestion as mentioned in the
onSuccesscallback to handle potential edge cases safely.
65-66: LGTM: Consistent toast management in disconnect onSuccess.The changes in the
disconnectmutation'sonSuccesscallback are consistent with theconnectmutation, ensuring uniform behavior across different actions.Apply the same null check suggestion as mentioned in the previous callbacks to handle potential edge cases safely.
71-72: LGTM with minor syntax note: Consistent toast management in disconnect onError.The changes in the
disconnectmutation'sonErrorcallback maintain consistency with other callbacks, contributing to the overall fix for the toast dismissal issue.Apply the same null check suggestion as mentioned in the previous callbacks to handle potential edge cases safely.
Add a semicolon at the end of line 71 for consistency with the coding style in the rest of the file:
toast.dismiss(loadingToastId.current!);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx (3)
8-8: LGTM: Import statement updated correctly.The addition of
useRefto the import statement is appropriate for the new functionality introduced in the component.
23-24: LGTM: Proper toast ID management in onMutate.The changes correctly store the loading toast ID in the ref, allowing for its retrieval and dismissal in other lifecycle methods. This implementation aligns with the PR objective of fixing the issue with toasts not dismissing properly.
Line range hint
1-93: Summary: Effective implementation of toast dismissal with minor improvement suggestions.The changes in this file successfully address the issue of toasts not dismissing properly, as outlined in the PR objectives. The implementation is consistent across different scenarios (success and error cases for both connect and disconnect mutations), ensuring that loading toasts are properly managed throughout the component's lifecycle.
Key points:
- The use of
useReffor managing toast IDs is appropriate and effective.- Toast dismissal is consistently implemented in all relevant callbacks.
- The changes align well with the PR's goal of fixing the issue described in [🕹️] Some toasts keep showing as loading even when they finish #2194.
Suggestions for further improvement:
- Consider using a more specific naming convention for the ref (e.g.,
loadingToastIdRef).- Implement null checks before dismissing toasts to handle potential edge cases safely.
- Ensure consistent use of semicolons at the end of statements.
Overall, these changes significantly improve the user experience by ensuring that toast notifications are properly dismissed after actions are completed.
|
I think the proper way to do this would be using a promise, like the author of the library intended, can we try that? |
Sure !! |
* fixed bucket caching issue for multiple keys on audit log. * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* removed the wrong commits pushed with this PR * adding roadmap after other info --------- Co-authored-by: Meg Stepp <mcstepp@users.noreply.github.com>
Followed unkey on Twitter Co-authored-by: Andreas Thomas <dev@chronark.com>
* fix: transaction ...: in use: for query - Fixes unkeyed#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>
* Update 3_follow_the_unkey_x_account.md * Update 3_follow_the_unkey_x_account.md --------- Co-authored-by: Andreas Thomas <dev@chronark.com>
- 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
…nkeyed#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>
…#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
Added name to the followed list. Co-authored-by: Andreas Thomas <dev@chronark.com>
avaliable -> available
Followed the unkey x account
* Feat: Followed Unkey on X * Feat: Followed Unkey on Twitter --------- Co-authored-by: Andreas Thomas <dev@chronark.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx (1)
93-100: UseonChangeinstead ofonClickfor theCheckboxcomponentThe
Checkboxcomponent should use theonChangeevent handler to ensure proper accessibility and handling of user interactions, especially for keyboard navigation and assistive technologies.Apply this change:
<Checkbox checked={optimisticChecked} - onClick={() => { + onChange={() => { if (optimisticChecked) { handleDisconnect(roleId, permissionId); } else { handleConnect(roleId, permissionId); } }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx (3 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx (3 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/delete-role.tsx
- apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx
[error] 44-47: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 72-75: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
apps/dashboard/app/(app)/authorization/roles/[roleId]/permission-toggle.tsx
[error] 47-50: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 75-78: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/role-toggle.tsx (2)
6-6: Imports added for necessary iconsThe
CircleCheckandLoader2icons are imported correctly fromlucide-react.
93-95: Toggle action logic is properly implementedThe
onClickhandler correctly callshandleDisconnectorhandleConnectbased on theoptimisticCheckedstate.
| const handleConnect = async (roleId: string, permissionId: string) => { | ||
| setOptimisticChecked(true); | ||
| toast.promise(connect.mutateAsync({ roleId, permissionId }), { |
There was a problem hiding this comment.
Revert optimistic UI update if the mutation fails
In handleConnect, you're setting optimisticChecked to true before the mutation completes. If the mutation fails, the checkbox remains checked, which doesn't reflect the actual state.
Consider reverting optimisticChecked in the error handler to ensure UI consistency.
Apply this change:
const handleConnect = async (roleId: string, permissionId: string) => {
setOptimisticChecked(true);
toast.promise(connect.mutateAsync({ roleId, permissionId }), {
loading: "Adding Permission ...",
success: () => (
// success handling
),
error: (error) => {
+ setOptimisticChecked(false);
`${error.message || "An error occurred while adding the permission."}`
},
});
};Similarly, in handleDisconnect, ensure the state is reverted on error.
const handleDisconnect = async (roleId: string, permissionId: string) => {
setOptimisticChecked(false);
toast.promise(disconnect.mutateAsync({ roleId, permissionId }), {
loading: "Removing Permission ...",
success: () => (
// success handling
),
error: (error) => {
+ setOptimisticChecked(true);
`${error.message || "An error occurred while removing the permission."}`
},
});
};Also applies to: 57-58
There was a problem hiding this comment.
@chronark, glad the suggestion makes sense! Let me know if you have any questions or need further assistance.
(_/)
(•ᴥ•)
/ ❤️
|
|
||
| const handleDisconnect = async (roleId: string, permissionId: string) => { | ||
| setOptimisticChecked(false); | ||
| toast.error("My error toast"); |
There was a problem hiding this comment.
Remove placeholder error toast message
At line 62, there's a call to toast.error("My error toast"); which appears to be a placeholder or debug message. This may confuse users if left in production.
Consider removing or replacing it with a meaningful error message.
Apply this diff to fix the issue:
- toast.error("My error toast");📝 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.
| toast.error("My error toast"); |
There was a problem hiding this comment.
please check this @AkshayBandi027
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <button | ||
| onClick={() => connect.mutate({ roleId, permissionId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > |
There was a problem hiding this comment.
Add explicit type="button" to the <button> element to prevent unintended form submissions
Similar to the previous instance, ensure the type attribute is set to "button" to avoid unintended form behavior.
Apply this diff to fix the issue:
<button
+ type="button"
onClick={() => connect.mutate({ roleId, permissionId })}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => connect.mutate({ roleId, permissionId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| <button | |
| type="button" | |
| onClick={() => connect.mutate({ roleId, permissionId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > |
🧰 Tools
🪛 Biome
[error] 75-78: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| onClick={() => disconnect.mutate({ roleId, permissionId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > |
There was a problem hiding this comment.
Add explicit type="button" to the <button> element to prevent unintended form submissions
The button element defaults to type="submit" if no type is specified, which can cause unintended form submissions when the button is inside a form. To prevent this, explicitly set the type attribute to "button".
Apply this diff to fix the issue:
<button
+ type="button"
onClick={() => disconnect.mutate({ roleId, permissionId })}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => disconnect.mutate({ roleId, permissionId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| <button | |
| type="button" | |
| onClick={() => disconnect.mutate({ roleId, permissionId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > |
🧰 Tools
🪛 Biome
[error] 47-50: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| </div> | ||
| </div> | ||
| ), | ||
| error: (error) => `${error.message || "An error occurred while adding the Role ."}`, |
There was a problem hiding this comment.
Correct error message formatting
Remove the extra space before the period and use consistent capitalization in the error message.
Apply this diff to fix the error message:
- error: (error) => `${error.message || "An error occurred while adding the Role ."}`,
+ error: (error) => `${error.message || "An error occurred while adding the role."}`,📝 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.
| error: (error) => `${error.message || "An error occurred while adding the Role ."}`, | |
| error: (error) => `${error.message || "An error occurred while adding the role."}`, |
| <button | ||
| onClick={() => connect.mutate({ roleId, keyId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > |
There was a problem hiding this comment.
Add explicit type attribute to the <button> element
Similarly, the <button> element here should have an explicit type attribute to prevent default form submission behavior. Add type="button".
Apply this diff to fix the issue:
<button
+ type="button"
onClick={() => connect.mutate({ roleId, keyId })}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => connect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| <button | |
| type="button" | |
| onClick={() => connect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > |
🧰 Tools
🪛 Biome
[error] 72-75: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| onClick={() => disconnect.mutate({ roleId, keyId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > |
There was a problem hiding this comment.
Add explicit type attribute to the <button> element
The <button> element should have an explicit type attribute to prevent unintended form submissions when used inside a <form>. Add type="button" to specify the button's behavior.
Apply this diff to fix the issue:
<button
+ type="button"
onClick={() => disconnect.mutate({ roleId, keyId })}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => disconnect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| <button | |
| type="button" | |
| onClick={() => disconnect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > |
🧰 Tools
🪛 Biome
[error] 44-47: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| onClick={() => connect.mutate({ roleId, keyId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > | ||
| Undo | ||
| </button> |
There was a problem hiding this comment.
Update state when 'Undo' is clicked to maintain UI consistency
In this instance, clicking the 'Undo' button does not update the optimisticChecked state. To keep the UI in sync, update optimisticChecked after the mutation.
Apply this diff to update the state:
<button
type="button"
- onClick={() => connect.mutate({ roleId, keyId })}
+ onClick={() => {
+ connect.mutate({ roleId, keyId });
+ setOptimisticChecked(true);
+ }}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => connect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| Undo | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => { | |
| connect.mutate({ roleId, keyId }); | |
| setOptimisticChecked(true); | |
| }} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| Undo | |
| </button> |
🧰 Tools
🪛 Biome
[error] 72-75: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| onClick={() => disconnect.mutate({ roleId, keyId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | ||
| > | ||
| Undo | ||
| </button> |
There was a problem hiding this comment.
Update state when 'Undo' is clicked to maintain UI consistency
When the 'Undo' button is clicked, the optimisticChecked state is not updated. This may cause the checkbox to display an incorrect state. Update optimisticChecked to ensure the UI reflects the actual state.
Apply this diff to update the state:
<button
type="button"
- onClick={() => disconnect.mutate({ roleId, keyId })}
+ onClick={() => {
+ disconnect.mutate({ roleId, keyId });
+ setOptimisticChecked(false);
+ }}
className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors"
>📝 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.
| <button | |
| onClick={() => disconnect.mutate({ roleId, keyId })} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| Undo | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => { | |
| disconnect.mutate({ roleId, keyId }); | |
| setOptimisticChecked(false); | |
| }} | |
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" | |
| > | |
| Undo | |
| </button> |
🧰 Tools
🪛 Biome
[error] 44-47: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| setOptimisticChecked(true); | ||
| toast.promise(connect.mutateAsync({ roleId, keyId }), { | ||
| loading: "Adding Role ...", | ||
| success: () => ( |
There was a problem hiding this comment.
why are we doing custom components here?
the toast already has a way to display an action
https://sonner.emilkowal.ski/toast#action
| <div> | ||
| <button | ||
| onClick={() => disconnect.mutate({ roleId, keyId })} | ||
| className="bg-[#2c2c2c] text-white px-[4px] py-[2px] rounded hover:bg-[#3c3c3c] transition-colors" |
There was a problem hiding this comment.
don't hardcode colors please
chronark
added
the
Changes Requested
What does this PR do?
Fixes #2194
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
https://www.loom.com/share/8a064dcf1094468399521502734993bd?sid=174329a2-6981-4d11-93c9-b1706f0b370a
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes