fix: add create button to auth keys error state #1
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe AuthKeysView was refactored to use a consistent header (back button, title, "Create Auth Key") and to render error and empty states as styled in-layout panels; key list rendering and the create-key modal remain functional and integrated into the new layout. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @adityakhare86, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical usability issue within the authentication key management interface. Previously, if no authentication keys were present, the UI would display an error without offering a way to create new keys, effectively blocking user progression. The changes introduce a 'Create Auth Key' button directly into this error state, along with improved styling and navigation, ensuring that users always have the necessary tools to manage their authentication keys. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where users were blocked from creating their first authentication key if the API returned an error for an empty list. The change updates the error state UI to include a 'Create Auth Key' button, which is a great improvement for the user experience. My main feedback is regarding code duplication. The new error view duplicates a significant portion of the page layout and header from the main success view. This will make the component harder to maintain in the long run. I've left a detailed comment with a suggestion to refactor the component to share the layout and conditionally render the content, which would eliminate the duplication and improve maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (1)
280-319: Consider extracting the duplicated header component.The header section (back button, title, and "Create Auth Key" button) is duplicated between the error state and success state (lines 357-396). Extract this into a reusable component to improve maintainability.
🔎 Suggested refactor
Create a new component to eliminate duplication:
const AuthKeysHeader: React.FC<{ onCreateClick: () => void }> = ({ onCreateClick }) => ( <Flex justifyContent="space-between" alignItems="center" mb={4}> <Flex alignItems="center"> <Box as="button" aria-label="Back to dashboard" mr={3} sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center', height: '36px', width: '36px', border: '1px solid #e0e0e0', borderRadius: '4px', bg: 'transparent', cursor: 'pointer', '&:hover': { bg: '#f7f7f7' } }} onClick={() => navigate('/dashboard')} > <FaArrowLeft size={16} /> </Box> <Text fontSize="28px" fontWeight="bold"> Auth Keys </Text> </Flex> <Button onClick={onCreateClick} sx={{ display: 'flex', alignItems: 'center', }} > <Box mr={1}> <FaPlus size={14} /> </Box> Create Auth Key </Button> </Flex> );Then use
<AuthKeysHeader onCreateClick={() => setShowModal(true)} />in both error and success states.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx
🔇 Additional comments (3)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (3)
269-279: LGTM: Consistent error state container.The error state container styling matches the loading and success states, providing a consistent user experience.
307-318: Verify that key creation works when initial fetch fails.The error state provides a "Create Auth Key" button, which is the intended fix. However, ensure that the
createAuthKeyfunction can succeed even when the initial fetch of auth keys fails. If both operations share the same API endpoint or authentication mechanism, the create operation might also fail due to the same underlying issue (network, auth, server error).Consider testing scenarios where:
- The database is empty (no keys exist) - this should now work with your fix
- The API is unreachable or returns 5xx errors - key creation might also fail
- Authentication/authorization issues - create might also be blocked
If key creation can fail for the same reason as the fetch, consider adding error handling feedback to guide users (e.g., "Please check your connection" or "Contact support if this persists").
336-340: LGTM: Modal correctly enables key creation in error state.The CreateAuthKeyModal is appropriately rendered in the error state, allowing users to create their first auth key even when the initial fetch fails. This directly addresses the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (1)
10-111: Add ARIA attributes and focus management to modal.The modal lacks proper accessibility attributes. Add
role="dialog",aria-modal="true", andaria-labelledbyto the modal container. Consider implementing keyboard trap and focus management to ensure the modal is accessible to keyboard and screen reader users.🔎 Proposed accessibility enhancements
+ const modalTitleId = 'create-auth-key-modal-title'; + if (!isOpen) return null; return ( <Box sx={{ position: 'fixed', top: 0, left: 0, right: 0, bottom: 0, backgroundColor: 'rgba(0, 0, 0, 0.5)', display: 'flex', alignItems: 'center', justifyContent: 'center', zIndex: 1000, }} onClick={onClose} > <Box + role="dialog" + aria-modal="true" + aria-labelledby={modalTitleId} sx={{ backgroundColor: 'white', borderRadius: '8px', padding: '24px', width: '400px', maxWidth: '90vw', }} onClick={(e) => e.stopPropagation()} > - <Text fontSize="20px" fontWeight="bold" mb={3}> + <Text id={modalTitleId} fontSize="20px" fontWeight="bold" mb={3}> Create New Auth Key </Text>Additionally, consider using a library like
react-focus-lockor implementing custom focus trap logic to prevent keyboard users from tabbing out of the modal.
♻️ Duplicate comments (2)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (2)
282-302: Add accessible label to back button.The back button uses only a visual icon without an accessible label for screen readers.
🔎 Proposed fix
<Box as="button" + aria-label="Back to dashboard" mr={3} sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center', height: '36px', width: '36px', border: '1px solid #e0e0e0', borderRadius: '4px', bg: 'transparent', cursor: 'pointer', '&:hover': { bg: '#f7f7f7' } }} onClick={() => navigate('/dashboard')} > <FaArrowLeft size={16} /> </Box>
332-334: Avoid exposing raw API error messages to users.Displaying the raw error message from the API may expose sensitive implementation details or technical information. Consider providing a user-friendly error message instead.
🔎 Proposed fix
-<Text fontSize="18px" fontWeight="bold" mb={2} color="#d32f2f"> - Error: {error} -</Text> +<Text fontSize="18px" fontWeight="bold" mb={2} color="#d32f2f"> + Unable to load auth keys +</Text> <Text fontSize="16px" color="#666" mb={3}> There was an error loading auth keys. You can still create a new auth key. </Text>Consider logging the actual error for debugging:
console.error('Failed to load auth keys:', error);
🧹 Nitpick comments (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (1)
429-473: Add aria-label attributes to icon-only buttons.While the
titleattributes provide tooltips on hover, addingaria-labelattributes to the visibility toggle and copy buttons will improve screen reader accessibility.🔎 Proposed enhancement
<Box as="button" + aria-label={showKeys[authKey.key] ? 'Hide key' : 'Show key'} mr={1} sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center', height: '32px', width: '32px', border: '1px solid #e0e0e0', borderRadius: '4px', bg: 'transparent', cursor: 'pointer', '&:hover': { bg: '#f0f0f0' } }} onClick={() => toggleKeyVisibility(authKey.key)} title={showKeys[authKey.key] ? 'Hide key' : 'Show key'} > {showKeys[authKey.key] ? <FaEyeSlash size={14} /> : <FaEye size={14} />} </Box> {showKeys[authKey.key] && ( <Box as="button" + aria-label="Copy to clipboard" sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center', height: '32px', width: '32px', border: '1px solid #e0e0e0', borderRadius: '4px', bg: 'transparent', cursor: 'pointer', '&:hover': { bg: '#f0f0f0' } }} onClick={() => copyToClipboard(authKey.key)} title="Copy to clipboard" > <FaCopy size={14} /> </Box> )}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx
🔇 Additional comments (2)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (2)
279-319: Excellent refactoring to eliminate code duplication.The introduction of a common header successfully addresses the code duplication concern raised in previous reviews. The layout, header, and modal are no longer duplicated across different states, making the code more maintainable.
322-346: PR objective successfully achieved.The error state now includes a "Create Auth Key" button, addressing the PR's objective of allowing users to create their first auth key even when the initial fetch fails. The layout is consistent with the empty state styling.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (1)
41-109: Add ARIA attributes and focus management to modal.The modal is missing critical accessibility features that prevent proper usage with assistive technologies:
- No
role="dialog"andaria-modal="true"to announce the modal to screen readers- No
aria-labelledbyconnecting to the modal title- No focus trap to prevent keyboard users from tabbing to background content
- No focus restoration when the modal closes
🔎 Recommended accessibility improvements
return ( <Box sx={{ position: 'fixed', top: 0, left: 0, right: 0, bottom: 0, backgroundColor: 'rgba(0, 0, 0, 0.5)', display: 'flex', alignItems: 'center', justifyContent: 'center', zIndex: 1000, }} onClick={onClose} + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" > <Box sx={{ backgroundColor: 'white', borderRadius: '8px', padding: '24px', width: '400px', maxWidth: '90vw', }} onClick={(e) => e.stopPropagation()} > - <Text fontSize="20px" fontWeight="bold" mb={3}> + <Text id="modal-title" fontSize="20px" fontWeight="bold" mb={3}> Create New Auth Key </Text>Additionally, consider:
- Adding ESC key handler:
useEffect(() => { const handleEsc = (e) => { if (e.key === 'Escape') onClose(); }; if (isOpen) document.addEventListener('keydown', handleEsc); return () => document.removeEventListener('keydown', handleEsc); }, [isOpen, onClose]);- Implementing a focus trap library like
focus-trap-reactorreact-focus-lock- Storing and restoring focus:
const previousFocus = useRef(); useEffect(() => { if (isOpen) previousFocus.current = document.activeElement; else previousFocus.current?.focus(); }, [isOpen]);
🧹 Nitpick comments (2)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (2)
132-135: Consider adding error handling for clipboard operations.The
navigator.clipboard.writeText()method can fail in certain contexts (non-HTTPS, permissions denied, etc.) and returns a Promise that may reject. Adding error handling would improve robustness.🔎 Proposed error handling
const copyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); - toast.success('Auth key copied to clipboard!'); + navigator.clipboard.writeText(text) + .then(() => { + toast.success('Auth key copied to clipboard!'); + }) + .catch(() => { + toast.error('Failed to copy auth key to clipboard'); + }); };
137-145: Consider parameterizing the locale for better internationalization.The
formatDatefunction hard-codes the'en-US'locale, which may not match all users' preferences. If your application supports multiple locales, consider accepting a locale parameter or using the browser's default locale.🔎 Proposed parameterization
-const formatDate = (dateString: string) => { - return new Date(dateString).toLocaleDateString('en-US', { +const formatDate = (dateString: string, locale = navigator.language) => { + return new Date(dateString).toLocaleDateString(locale, { year: 'numeric', month: 'short', day: 'numeric', hour: '2-digit', minute: '2-digit', }); };Alternatively, use
undefinedas the locale to let the browser choose:.toLocaleDateString(undefined, { ... })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx
🔇 Additional comments (1)
web/src/views/dashboard/authKeys/AuthKeysView.tsx (1)
279-319: Unified header successfully eliminates duplication.The refactor to extract a common header that applies to all states (error, empty, list) successfully addresses the code duplication concern raised in previous reviews. The Create Auth Key button is now always accessible, ensuring users can create their first key even when the API returns an error.
1 participant
When no auth keys exist in the database, the API returns an error which caused the error state UI to render. This error state only showed an error message with no way to create a new auth key, effectively blocking users from creating their first key via the UI.
Fix: Updated the error state to include a button to create auth keys.
This ensures users can always create auth keys through the UI, even when the initial fetch fails.
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.