Refactor: all dialogs now use <Dialog> headlessui component (and more). #257
Conversation
This makes it more streamline that we have a stack of dialogs during the "Send Assets" UX flow. Showing multiple dialogs at once and pressing Escape will close the latest opened one. Same with closing dialogs. This is preliminary work to fix liftedinit#240.
|
Warning Rate limit exceeded@hansl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 13 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. 📒 Files selected for processing (4)
WalkthroughThis pull request refactors modal implementations across various components by replacing native HTML dialogs with Headless UI’s Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Modal (HeadlessUI Dialog)
participant S as SignModal
participant W as Web3AuthContext
U->>M: Trigger modal open action
M->>S: Render signing interface within modal
U->>M: Interact (approve/reject)
M->>W: Set prompt ID and initiate transaction
W-->>M: Return transaction result
M->>U: Close modal and update UI
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![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
🧹 Nitpick comments (10)
components/bank/modals/sendModal.tsx (4)
35-37: Use optional chaining for cleaner conditional invocation.Instead of "setOpen && setOpen(false);", consider using optional chaining ("setOpen?.(false);") for a more concise and idiomatic approach.
- setOpen && setOpen(false); + setOpen?.(false);🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
49-55: Inline styles may conflict with Tailwind utilities.Consider moving these styles to your Tailwind CSS or a utility class to keep style definition consistent and avoid conflicts between inline styling and Tailwind’s utility classes.
58-69: Consider using Dialog.Overlay for the close button in a more idiomatic way.The form with method="dialog" mimics the native behavior, but Headless UI provides a built-in <Dialog.Overlay> that can handle backdrop interactions in a more declarative manner.
85-99: Use custom overlay element instead of method="dialog" form.A <Dialog.Overlay> element can simplify implementing an outside click. Manually using “method=dialog” can be less intuitive.
components/react/authSignerModal.test.tsx (1)
23-25: Replace 'any' cast with a stronger type for 'data'.While casting to "any" can be convenient for quick testing, consider using a stricter type to ensure test correctness and maintainability.
components/react/authSignerModal.tsx (2)
250-255: Define default props instead of reassigning parameter.Reassigning 'onClose' within the function body is valid but can be clearer if you define a default param directly:
-} : { - visible: boolean; - onClose?: () => void; - ... -}) => { - onClose = onClose || (() => {}); +}: { + visible: boolean; + onClose?: () => void; + ... +} = { + onClose: () => {}, +}) => {
314-314: Prefer <Dialog.Overlay> over form method="dialog".Using "method='dialog'" is a workaround for native , but Headless UI encourages utilizing <Dialog.Overlay> to handle backdrop clicks more cleanly.
components/bank/forms/sendForm.tsx (3)
17-17: Remove unused imports.The
DialogandPortalcomponents are imported but never used in this file.-import { Dialog, Portal } from '@headlessui/react';
40-43: Consider consolidating transaction states.The component maintains two separate states for transaction progress:
isSending(local state)isSigning(fromuseTxhook)Consider using
isSigningfrom theuseTxhook instead of maintaining a separateisSendingstate. This would provide a more accurate representation of the transaction's status.- const [isSending, setIsSending] = useState(false); const [searchTerm, setSearchTerm] = useState(''); const [feeWarning, setFeeWarning] = useState(''); const { tx, isSigning } = useTx(env.chain);Then update the submit button's disabled state and loading indicator:
- disabled={isSending || !isValid || !dirty} + disabled={isSigning || !isValid || !dirty} aria-label="send-btn" > - {isSending ? <span className="loading loading-dots loading-xs"></span> : 'Send'} + {isSigning ? <span className="loading loading-dots loading-xs"></span> : 'Send'}
108-109: Remove redundant state updates.If you adopt the previous suggestion to use
isSigning, these state updates become unnecessary as the signing state will be managed by theuseTxhook.- setIsSending(true); try { // ... existing code ... } catch (error) { console.error('Error during sending:', error); - } finally { - setIsSending(false); - }Also applies to: 153-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/bank/forms/sendForm.tsx(2 hunks)components/bank/modals/sendModal.tsx(2 hunks)components/react/authSignerModal.test.tsx(1 hunks)components/react/authSignerModal.tsx(4 hunks)components/react/modal.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/react/modal.tsx
🧰 Additional context used
📓 Learnings (1)
components/bank/forms/sendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🪛 Biome (1.9.4)
components/bank/modals/sendModal.tsx
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
components/bank/modals/sendModal.tsx (4)
4-4: Great use of the @headlessui/react library.This import statement correctly brings in the necessary Dialog and Portal components.
39-46: Portal + Dialog pairing looks good.Good job leveraging Portal for content outside the root DOM hierarchy. This structure clearly separates your modal content from the main application container.
71-83: SendBox integration is clear.The data props and refetch handlers look consistent with the rest of the code.
100-102: Ending tags confirm correct Portal usage.Portal cleanup and final return structure look correct.
components/react/authSignerModal.test.tsx (1)
30-32: Visibility test logic is accurate.Checking the length of "screen.queryAllByRole('dialog')" is a clear approach to confirm that no dialog is present when "visible" is false.
components/react/authSignerModal.tsx (1)
265-265: Good integration of the Dialog component.Using "open={visible}" and "onClose={onClose}" is aligned with the recommended Headless UI usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/bank/modals/sendModal.tsx (2)
48-48: Use optional chaining for better TypeScript idioms.The current implementation uses the logical AND operator. Consider using optional chaining for a more idiomatic TypeScript approach.
- const handleClose = () => setOpen && setOpen(false); + const handleClose = () => setOpen?.(false);🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-63: Consider extracting styles to CSS modules or Tailwind classes.The modal and backdrop styles are currently mixed between inline styles and classes. Consider:
- Moving inline styles to CSS modules or Tailwind classes for better maintainability.
- Using Tailwind's backdrop utilities instead of a separate form element.
Example refactor for the backdrop:
- <form - method="dialog" - className="modal-backdrop" - style={{ - position: 'fixed', - top: 0, - left: 0, - right: 0, - bottom: 0, - zIndex: -1, - backgroundColor: 'rgba(0, 0, 0, 0.3)', - }} - > - <button onClick={handleClose}>close</button> - </form> + <div + className="fixed inset-0 -z-10 bg-black/30" + onClick={handleClose} + aria-hidden="true" + />Also applies to: 98-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/bank/forms/sendForm.tsx(1 hunks)components/bank/modals/sendModal.tsx(2 hunks)components/react/authSignerModal.test.tsx(1 hunks)components/react/modal.tsx(2 hunks)hooks/useTx.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- hooks/useTx.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- components/react/modal.tsx
- components/react/authSignerModal.test.tsx
- components/bank/forms/sendForm.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/bank/modals/sendModal.tsx
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
components/bank/modals/sendModal.tsx (2)
4-4: LGTM! Good choice of UI library.The addition of @headlessui/react components is a good choice as it provides better accessibility and dialog management out of the box.
57-57: LGTM! Good accessibility implementation.The event handling implementation:
- Uses @headlessui/react's built-in accessibility features
- Properly handles both Escape key and outside clicks
- Maintains keyboard navigation support
Also applies to: 69-76
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
components/bank/modals/sendModal.tsx (1)
48-48: Consider using optional chaining for better readability.The handleClose function can be simplified using optional chaining.
-const handleClose = () => setOpen && setOpen(false); +const handleClose = () => setOpen?.(false);🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/bank/components/tokenList.tsx (1)
46-46: Consider a more consistent state variable name.The suffix 'Hook' in the state variable name is unconventional and doesn't add value.
-const [isSendModalOpen, setIsSendModalOpenHook] = useState(false); +const [isSendModalOpen, setIsSendModalOpenInternal] = useState(false);components/react/authSignerModal.tsx (1)
250-255: Consider improving TypeScript type safety for onClose.While making
onCloseoptional improves flexibility, consider using a required prop with a default value in the component's implementation to maintain type safety and make the contract more explicit.- onClose?: () => void; + onClose: () => void;Then provide a default value in the component's props:
SignModal.defaultProps = { onClose: () => {}, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/bank/components/tokenList.tsx(2 hunks)components/bank/forms/sendForm.tsx(2 hunks)components/bank/modals/sendModal.tsx(2 hunks)components/react/authSignerModal.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/bank/forms/sendForm.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/react/authSignerModal.tsx
[error] 280-280: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
components/bank/modals/sendModal.tsx
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/bank/components/tokenList.tsx
[error] 52-52: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (4)
components/bank/modals/sendModal.tsx (2)
9-27: LGTM! Props interface updated correctly.The
modalIdprop is now optional, which aligns with the @headlessui/react Dialog implementation where id is not required for functionality.
50-97: LGTM! Dialog implementation follows best practices.The implementation:
- Uses Portal for proper stacking context
- Implements proper accessibility attributes
- Handles backdrop clicks and Escape key via @headlessui/react
- Maintains consistent styling with the previous implementation
components/bank/components/tokenList.tsx (1)
236-253: LGTM! SendModal integration is correct.The SendModal component is properly integrated with the new state management.
components/react/authSignerModal.tsx (1)
265-265: 🌐 Web query:What is the recommended z-index scale for modal dialogs in web applications?💡 Result:
The recommended z-index scale for modal dialogs follows Material-UI's established hierarchy system designed to prevent stacking conflicts between components[1][3][5]. Here's the standard implementation: ## Default z-index Values for Components Material-UI components use this standardized scale[1][4][7]: ``` Mobile Stepper: 1000 Floating Action Button (FAB): 1050 App Bar: 1100 Drawer: 1200 Modal/Dialog: 1300 Snackbar: 1400 Tooltip: 1500 ``` ## Key Implementation Details **Modal Dialogs** are assigned: ``` z-index: 1300 ``` This positions them above most components but below transient notifications (snackbars/toast) and tooltips[1][7]. **Best Practices**: 1. **Theme-based Management**: Configure through theme settings rather than inline styles[1][5]: ```javascript const theme = createTheme({ zIndex: { modal: 1300, snackbar: 1400, tooltip: 1500 } }); ``` 2. **Stacking Context Rules**: - Modals should reside in their own stacking context - Parent elements shouldn't create competing contexts with lower z-indexes[3][9] 3. **Customization Guidance**: ```javascript // To elevate specific modals temporarily <Dialog style={{ zIndex: theme.zIndex.modal + 10 }}> ``` Material-UI's system ensures proper layering where: - Dialogs (1300) appear above drawers (1200) and app bars (1100)[1] - Snackbars (1400) and tooltips (1500) overlay active dialogs[1][7] - Mobile interfaces prioritize full-screen dialogs through responsive z-index handling[1][4] For non-Material UI implementations, maintain similar proportional relationships between component layers while keeping modal z-index values between 1000-15000 for compatibility with most third-party libraries[3][5][7]. Citations: - 1: https://www.restack.io/p/material-ui-dialog-z-index-answer - 2: https://dev.to/mimafogeus2/a-better-way-to-manage-z-indexes-1nf - 3: https://css-tricks.com/considerations-styling-modal/ - 4: https://www.syncfusion.com/forums/180564/z-index-dialog - 5: https://www.smashingmagazine.com/2021/02/css-z-index-large-projects/ - 6: https://www.syncfusion.com/forums/139179/modal-dialog-z-index-issues - 7: https://blog.bitsrc.io/top-layer-vs-z-index-for-frontend-dialogs-fcb12217a834?gi=fcd12e824d97 - 8: https://community.developer.atlassian.com/t/atlaskit-modal-dialog-in-server-environment/49705 - 9: https://developer.mozilla.org/en-US/docs/Web/CSS/z-index?v=example - 10: https://www.bomberbot.com/css/z-index-explained-how-to-stack-elements-using-css/
There was a problem hiding this comment.
Thank you for this!
Everything looks good and functions well.
Couple minor issues:
- backdrop on send modal isnt working
- I think all modals need to be pushed right by the width of the sideNav in its extended state. This is especially visible on the mint and burn modals in the tokenfactory denom list.
- @fmorency do we want animations for dialog open and close.
|
Sorry I'm going to put this in draft again, it is not ready. |
Whatever is easiest. I don't really care. |
hansl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
components/admins/modals/__tests__/validatorModal.test.tsx (1)
39-104: 🛠️ Refactor suggestionAdd tests for Dialog-specific behavior.
The PR objectives mention that users can close dialogs using the Escape key or by clicking outside. Add tests to verify this behavior.
test('closes modal when escape key is pressed', async () => { const setOpenValidatorModal = jest.fn(); renderWithProps({ setOpenValidatorModal }); fireEvent.keyDown(document.body, { key: 'Escape' }); await waitFor(() => { expect(setOpenValidatorModal).toHaveBeenCalledWith(false); }); }); test('closes modal when clicking outside', async () => { const setOpenValidatorModal = jest.fn(); renderWithProps({ setOpenValidatorModal }); // Get the backdrop element (exact selector depends on HeadlessUI's implementation) const backdrop = screen.getByRole('dialog').parentElement; fireEvent.click(backdrop); await waitFor(() => { expect(setOpenValidatorModal).toHaveBeenCalledWith(false); }); });components/factory/modals/TransferModal.tsx (1)
220-225: 🛠️ Refactor suggestionRemove portal rendering logic.
The portal rendering is redundant as Headless UI's Dialog already handles modal positioning and rendering.
- // Only render if we're in the browser - if (typeof document !== 'undefined') { - return createPortal(modalContent, document.body); - } - - return null; + return modalContent;components/admins/modals/validatorModal.tsx (1)
136-138:⚠️ Potential issueReplace
<img>with Next.js Image component.To resolve the pipeline warning and improve image optimization:
-{validator?.logo_url && ( - <img className="h-16 w-16 rounded-full" src={validator.logo_url} alt="" /> -)} +{validator?.logo_url && ( + <Image + className="rounded-full" + src={validator.logo_url} + alt={`${validator.description.moniker} logo`} + width={64} + height={64} + /> +)}Don't forget to add the import:
import Image from 'next/image';🧰 Tools
🪛 GitHub Actions: Build
[warning] 137-137: Do not use
<img>element. Use<Image />fromnext/imageinstead.components/admins/modals/__tests__/warningModal.test.tsx (1)
40-55: 🛠️ Refactor suggestionAdd tests for modal state management.
Consider adding tests for:
- Modal open/close behavior
- setOpenWarningModal callback
🧹 Nitpick comments (45)
components/groups/modals/__tests__/voteDetailsModal.test.tsx (2)
39-39: Consider improving mock data and type safety.While the mock functions are properly initialized, consider these improvements:
- Replace
{} as anywith a proper type or interface for the group prop- Use realistic mock data for the group prop to better represent production scenarios
- group: {} as any, + group: { + id: 'test-group-id', + name: 'Test Group', + // Add other required properties based on the actual Group interface + },Also applies to: 47-48
79-82: Consider using more resilient selectors.While the interaction testing is good, using test IDs can be brittle. Consider using more semantic selectors that align with how users interact with the UI.
- fireEvent.click(screen.getByTestId('expand-messages')); + fireEvent.click(screen.getByRole('button', { name: /expand messages/i }));components/factory/modals/updateDenomMetadata.tsx (3)
146-160: Consider moving inline styles to Tailwind classes.The Dialog implementation looks good, but there's an opportunity to improve maintainability by using Tailwind classes instead of inline styles.
Consider replacing the inline styles with Tailwind classes:
- className={`modal modal-open fixed flex p-0 m-0`} - style={{ - height: '100vh', - width: '100vw', - alignItems: 'center', - justifyContent: 'center', - }} + className="modal modal-open fixed flex p-0 m-0 h-screen w-screen items-center justify-center"
139-144: Enhance error handling for better user feedback.The error handling could be improved to provide better feedback to users when transactions fail.
Consider enhancing the error handling:
} catch (error) { - console.error('Error during transaction setup:', error); + console.error('Error during transaction setup:', error); + // Show user-friendly error message + const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred'; + // Assuming you have a toast or notification system + showErrorNotification(errorMessage); } finally { setIsSigning(false); }
249-249: Consider conditional rendering of SignModal.The SignModal component is rendered unconditionally, which might lead to unnecessary renders when not in use.
Consider rendering SignModal only when needed:
- <SignModal /> + {isSigning && <SignModal />}components/admins/modals/__tests__/validatorModal.test.tsx (2)
9-14: Enhance router mock implementation.Consider expanding the router mock to include other commonly used properties and methods for more comprehensive testing. This helps prevent potential test failures if the component starts using additional router features.
mock.module('next/router', () => ({ useRouter: jest.fn().mockReturnValue({ query: {}, push: jest.fn(), + pathname: '/', + asPath: '/', + replace: jest.fn(), + back: jest.fn(), }), }));
24-37: Track modal state changes in tests.Replace the no-op setOpenValidatorModal with a Jest mock function to verify modal interactions.
function renderWithProps(props = {}) { + const setOpenValidatorModal = jest.fn(); return renderWithChainProvider( <ValidatorDetailsModal openValidatorModal={true} - setOpenValidatorModal={() => {}} + setOpenValidatorModal={setOpenValidatorModal} validator={validator} modalId={modalId} admin={admin} totalvp={totalvp} validatorVPArray={validatorVPArray} {...props} /> ); }components/admins/components/validatorList.tsx (2)
54-67: Consider performance optimizations for large datasets.The current implementation uses client-side pagination and filtering. For better performance with large datasets, consider:
- Implementing server-side pagination
- Using virtualization for the table rows
- Expanding search to include validator addresses
133-175: Add error state handling.While loading states are handled well, consider adding explicit error state handling to improve user experience when data fetching fails.
Example implementation:
{isLoading ? ( <table className="table w-full border-separate border-spacing-y-3"> // ... loading skeleton </table> +) : error ? ( + <div className="text-center py-8 text-error"> + <p>Failed to load validators. Please try again.</p> + <button onClick={refetch} className="btn btn-error mt-4"> + Retry + </button> + </div> ) : filteredValidators.length === 0 ? (components/admins/modals/descriptionModal.tsx (2)
17-17: Consider removing the redundantreturn nullcheck.Because Headless UI's
<Dialog>already supports theopenprop, you can rely on it for controlling visibility. If you prefer an explicit early return, this is fine, but it’s somewhat redundant with<Dialog open={open}>(once the literalopenprop is fixed).
19-19: Use optional chaining.For better readability and to address the static analysis hint, rewrite the call to:
- onClose && onClose(); + onClose?.();🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/factory/modals/BurnModal.tsx (1)
40-89: Remove or conditionally apply'modal-open'andmethod="dialog.Headless UI's
<Dialog>handles open/close internally. The class'modal-open'may force the modal to remain styled as open. Also,method="dialog"is typically used with native<dialog>tags and can be safely removed or replaced if you don’t rely on native submission features.Example adjustment:
- <Dialog - open={isOpen} - onClose={onClose} - className={`modal modal-open fixed flex p-0 m-0`} - > - ... - <form method="dialog" onSubmit={onClose}> + <Dialog + open={isOpen} + onClose={onClose} + className="modal fixed flex p-0 m-0" + > + ... + <form onSubmit={onClose}>components/groups/modals/voteModal.tsx (1)
70-70: Consider removing'modal-open'class.Since
<Dialog open={open}>manages modal state,'modal modal-open'is redundant and can inadvertently force the “open” styling even whenopenis false.Example:
- className="modal modal-open fixed flex p-0 m-0 top-0 right-0 z-[9999]" + className="modal fixed flex p-0 m-0 top-0 right-0 z-[9999]"contexts/manifestAppProviders.tsx (2)
39-42: Consider handling null contexts gracefully.The
web3authvariable is retrieved fromWeb3AuthContextwithuseContext, but there is no fallback ifweb3authisnullorundefined. For added robustness, consider handling the scenario where the context may not be available (e.g., if this provider is not wrapped correctly at a higher level).
112-131: Conditionalize React Query Devtools for production build.Including
<ReactQueryDevtools />in production might negatively affect performance. Consider rendering it conditionally (e.g., only in development mode) to optimize production performance.components/groups/modals/groupInfo.tsx (2)
9-12: Remove unused imports ifuseEffectis no longer in use.
useEffectis imported, but you no longer appear to use it. Removing unused imports can keep the code tidy.
175-178: Enhance accessibility of the new<Dialog>structure.Now that the native
<dialog>element is replaced with Headless UI’s<Dialog>, consider adding<Dialog.Title>and<Dialog.Description>elements for improved accessibility. This practice helps screen readers and meets WAI-ARIA guidelines.Also applies to: 186-186, 188-188, 238-238, 249-249
components/groups/modals/memberManagementModal.tsx (1)
394-397: Consider matching theSignModalusage with other files.In
groupInfo.tsxyou use<SignModal id="leave-modal" />while here it’s<SignModal />without an id. Check if an identifier is needed for consistency or tracking.components/groups/modals/voteDetailsModal.tsx (3)
61-97: Centralized message field mappings are well-organized.Defining
importantFieldsanddefaultFieldsfor each message type is a clear approach to controlling which fields are emphasized in the UI. Consider storing these mappings in a dedicated module if the list grows significantly, to keep this file lean and maintainable.
289-310: Consider consolidating repeatedsetIsSigning(false)calls.The
executeWithdrawalfunction resetsisSigningboth in theonSuccesscallback and again after the transaction call. You can simplify to ensure a single call in the success path.Example diff to remove duplication:
const executeWithdrawal = async () => { setIsSigning(true); try { const fee = await estimateFee(address ?? '', [msgWithdraw]); await tx( [msgWithdraw], { fee, - onSuccess: () => { + onSuccess: () => { setIsSigning(false); refetchTally(); refetchVotes(); refetchProposals(); refetchGroupInfo(); refetchDenoms(); }, }, 'withdrawalPrompt' ); - setIsSigning(false); onClose(); } catch (error) { setIsSigning(false); console.error('Failed to execute proposal: ', error); } };
427-455: Conditional logic seems comprehensive but is becoming large.The
getButtonStatelogic covers many proposal states. Extracting it into a separate helper or adding inline comments may improve readability and future maintainability.hooks/useLocalStorage.tsx (1)
18-25: Consider robust fallback for write operations.If
window.localStorageis inaccessible (e.g., in private mode), thesetValuecall could fail. Handling potential exceptions here (or adding a silent catch) can guard against runtime errors.components/admins/modals/__tests__/descriptionModal.test.tsx (1)
26-35: Address the failing test TODO.The commented test needs to be updated to work with HeadlessUI Dialog. Consider using HeadlessUI's testing utilities.
Would you like me to help update this test to work with HeadlessUI Dialog? I can provide an example that uses HeadlessUI's testing utilities.
components/bank/modals/sendModal.tsx (1)
36-36: Optimize handleClose with optional chaining.Simplify the handleClose function using optional chaining for better readability.
-const handleClose = () => setOpen && setOpen(false); +const handleClose = () => setOpen?.(false);🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/factory/modals/MintModal.tsx (2)
64-69: Extract token display logic to a utility function.The token display logic is complex and could be reused across components.
+const getDisplayToken = (display: string) => { + if (!display) return 'UNKNOWN'; + if (display.startsWith('factory')) { + return (display.split('/').pop()?.toUpperCase() ?? + truncateString(display, 12).toUpperCase()); + } + return truncateString(display, 12).toUpperCase(); +}; -{denom.display - ? denom.display.startsWith('factory') - ? (denom.display.split('/').pop()?.toUpperCase() ?? - truncateString(denom.display, 12).toUpperCase()) - : truncateString(denom.display, 12).toUpperCase() - : 'UNKNOWN'} +{getDisplayToken(denom.display)}
56-60: Remove redundant form element.The form element is redundant since the Dialog already handles closing through the onClose prop.
-<form method="dialog" onSubmit={onClose}> <button className="btn btn-sm btn-circle btn-ghost absolute right-2 top-2 text-[#00000099] dark:text-[#FFFFFF99] hover:bg-[#0000000A] dark:hover:bg-[#FFFFFF1A]"> ✕ </button> -</form>contexts/web3AuthContext.tsx (1)
88-98: Memoize promptSign callback.The promptSign callback should be memoized to prevent unnecessary re-renders.
+const promptSign = useCallback( async (_, signData) => new Promise(resolve => setPrompt({ signData, resolve: approved => { setPrompt(undefined); resolve(approved); }, }) ), [] +); makeWeb3AuthWallets({ // ... - promptSign: async (_, signData) => - new Promise(resolve => - setPrompt({ - signData, - resolve: approved => { - setPrompt(undefined); - resolve(approved); - }, - }) - ), + promptSign, })components/groups/components/__tests__/groupInfo.test.tsx (1)
27-28: Add test coverage for modal visibility props.The test suite should include cases for modal visibility state changes.
test('modal visibility can be toggled', () => { const setShowInfoModal = jest.fn(); renderWithProps({ showInfoModal: true, setShowInfoModal }); const closeButton = screen.getByRole('button', { name: /close/i }); fireEvent.click(closeButton); expect(setShowInfoModal).toHaveBeenCalledWith(false); });Also applies to: 101-101
components/factory/modals/denomInfo.tsx (1)
1-2: Remove unused imports.The
useEffecthook andcreatePortalare no longer used after migrating to Headless UI's Dialog component.-import React, { useEffect } from 'react'; -import { createPortal } from 'react-dom'; +import React from 'react';components/admins/modals/cancelUpgradeModal.tsx (1)
1-2: Remove unused imports.The
useEffecthook andcreatePortalare no longer used after migrating to Headless UI's Dialog component.-import React, { useEffect } from 'react'; -import { createPortal } from 'react-dom'; +import React from 'react';hooks/useTx.tsx (1)
61-71: Add type safety for promptId.Consider adding TypeScript types for the promptId parameter to enhance type safety and documentation.
- const tx = async (msgs: Msg[], options: TxOptions, id?: string) => { + const tx = async (msgs: Msg[], options: TxOptions, id?: string | undefined) => {components/factory/modals/TransferModal.tsx (1)
1-5: Remove unused imports and redundant portal.The
useEffecthook is unused, andcreatePortalis redundant when using Headless UI's Dialog.-import React, { useEffect } from 'react'; +import React from 'react'; import { ExtendedMetadataSDKType, truncateString } from '@/utils'; import { useDenomAuthorityMetadata, useFeeEstimation, useTx } from '@/hooks'; import { cosmos, osmosis } from '@liftedinit/manifestjs'; -import { createPortal } from 'react-dom';components/admins/modals/multiMfxBurnModal.tsx (2)
120-136: Improve accessibility by adding ARIA labels.The modal implementation could benefit from additional accessibility attributes.
Apply this diff to enhance accessibility:
<Dialog open onClose={onClose} + aria-labelledby="burn-modal-title" + aria-describedby="burn-modal-description" className="modal modal-open fixed flex p-0 m-0 top-0" style={{ height: '100vh', width: '100vw', alignItems: 'center', justifyContent: 'center', }} > <div className="fixed inset-0 bg-black/30" aria-hidden="true" /> - <Dialog.Panel className="modal-box max-w-4xl mx-auto min-h-[30vh] max-h-[70vh] rounded-[24px] bg-[#F4F4FF] dark:bg-[#1D192D] shadow-lg overflow-y-auto"> + <Dialog.Panel className="modal-box max-w-4xl mx-auto min-h-[30vh] max-h-[70vh] rounded-[24px] bg-[#F4F4FF] dark:bg-[#1D192D] shadow-lg overflow-y-auto" role="dialog">
74-118: Consider adding error recovery mechanism.The error handling in
handleMultiBurncould be improved to provide better user feedback.Apply this diff to enhance error handling:
const handleMultiBurn = async (values: { burnPairs: BurnPair[] }) => { setIsSigning(true); try { const messages = values.burnPairs.map(pair => burnHeldBalance({ authority: admin, burnCoins: [ { denom: denom?.base ?? '', amount: parseNumberToBigInt(pair.amount, denom?.denom_units?.[1].exponent).toString(), }, ], }) ); // ... rest of the code ... } catch (error) { - console.error('Error during multi-burn:', error); + console.error('Error during multi-burn:', error); + // Show error to user + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + // You can use your preferred toast/notification system here + alert(errorMessage); } finally { setIsSigning(false); } };components/react/authSignerModal.tsx (2)
251-252: Use semantic HTML for keyboard navigation.The dropdown implementation could be improved for better keyboard accessibility.
Apply this diff to enhance keyboard navigation:
<label aria-label="version-selector" tabIndex={0} + role="button" + aria-haspopup="listbox" + aria-expanded={isOpen} className="btn btn-md w-full justify-between border border-[#00000033] dark:border-[#FFFFFF33] bg-[#E0E0FF0A] dark:bg-[#E0E0FF0A] hover:bg-transparent" style={{ borderRadius: '12px' }} >Also applies to: 297-298
297-298: Use optional chaining for safer property access.The static analysis hints suggest using optional chaining for safer property access.
Apply this diff to implement optional chaining:
-if (reject) reject(); +reject?.(); -if (approve) approve(); +approve?.();Also applies to: 301-301
🧰 Tools
🪛 Biome (1.9.4)
[error] 297-297: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/admins/modals/multiMfxMintModal.tsx (2)
19-19: Address TODO comment regarding mint amount limits.The TODO comment on line 19 indicates missing validation for maximum mint amount and transaction size limits.
Would you like me to help implement the validation logic for maximum mint amount and transaction size limits? This would involve:
- Adding validation rules to the
MultiMintSchema- Implementing checks in the
handleMultiMintfunction
74-117: Consider implementing transaction batching.The current implementation might hit transaction size limits with many payout pairs.
Consider implementing transaction batching to handle large numbers of payout pairs. This would involve:
- Splitting the payout pairs into smaller batches
- Creating multiple transactions
- Handling the transactions sequentially or in parallel
components/admins/modals/upgradeModal.tsx (2)
30-50: Add error recovery for release parsing.The
parseReleaseBodyfunction could benefit from more robust error handling.Apply this diff to enhance error handling:
const parseReleaseBody = (body: string): UpgradeInfo | null => { try { + if (!body) { + console.warn('Empty release body'); + return null; + } + const nameMatch = body.match(/- \*\*Upgrade Handler Name\*\*: (.*?)(?:\r?\n|$)/); const upgradeableMatch = body.match(/- \*\*Upgradeable\*\*: (.*?)(?:\r?\n|$)/); const commitHashMatch = body.match(/- \*\*Commit Hash\*\*: (.*?)(?:\r?\n|$)/); if (!nameMatch || !upgradeableMatch || !commitHashMatch) { - console.warn('Failed matches:', { nameMatch, upgradeableMatch, commitHashMatch }); + console.warn('Missing required fields in release body:', { + name: !nameMatch, + upgradeable: !upgradeableMatch, + commitHash: !commitHashMatch + }); return null; }
289-294: Consider using React refs instead of DOM queries.Based on the retrieved learnings, while DOM manipulation is acceptable in this project, using React refs would be a cleaner approach.
Apply this diff to use React refs:
+const dropdownRef = useRef<HTMLDivElement>(null); +const labelRef = useRef<HTMLLabelElement>(null); -const dropdown = (e.target as HTMLElement).closest('.dropdown'); -if (dropdown) { - (dropdown as HTMLElement).removeAttribute('open'); - (dropdown.querySelector('label') as HTMLElement)?.focus(); - (dropdown.querySelector('label') as HTMLElement)?.blur(); +if (dropdownRef.current) { + dropdownRef.current.removeAttribute('open'); + labelRef.current?.focus(); + labelRef.current?.blur(); }components/admins/modals/validatorModal.tsx (1)
104-115: Enhance modal accessibility.While the Dialog implementation is good, consider adding these accessibility improvements:
- Add
aria-labelledbyto the Dialog component- Add
aria-describedbyfor the modal content<Dialog open={openValidatorModal} className="modal modal-open fixed flex p-0 m-0" + aria-labelledby="validator-details-title" + aria-describedby="validator-details-content" onClose={() => setOpenValidatorModal(false)} style={{ alignItems: 'center', justifyContent: 'center', }} >components/admins/modals/__tests__/warningModal.test.tsx (2)
1-1: Consider moving router mock to shared test utilities.The router mock could be moved to a shared test utilities file since it's likely needed across multiple test files.
Also applies to: 8-13
22-35: Make modal state configurable in renderWithProps.Consider making the modal state configurable to test both open and closed states:
function renderWithProps(props = {}) { return renderWithChainProvider( <WarningModal - openWarningModal={true} + openWarningModal={props.openWarningModal ?? true} setOpenWarningModal={() => {}} admin={admin}components/admins/modals/warningModal.tsx (2)
79-94: Move styles to Tailwind classes.Consider replacing inline styles with Tailwind classes for better maintainability:
- style={{ - height: '100vh', - width: '100vw', - alignItems: 'center', - justifyContent: 'center', - }} + className="modal modal-open fixed flex p-0 m-0 h-screen w-screen items-center justify-center"
97-137: Improve modal content structure.Consider these improvements:
- The
formelement might be unnecessary with Dialog- Extract button text to constants for better maintainability
- Consider moving SignModal outside Dialog.Panel
- <Dialog.Panel className="modal-box text-black dark:text-white dark:bg-[#1D192D] bg-[#FFFFFF]"> - <form method="dialog"> + <Dialog.Panel className="modal-box text-black dark:text-white dark:bg-[#1D192D] bg-[#FFFFFF]"> <button className="btn btn-sm btn-circle btn-ghost absolute right-2 top-2" onClick={handleClose} > ✕ </button> {/* ... rest of the content ... */} - </form> </Dialog.Panel> + <SignModal />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
components/admins/components/__tests__/validatorList.test.tsx(1 hunks)components/admins/components/validatorList.tsx(1 hunks)components/admins/modals/__tests__/descriptionModal.test.tsx(1 hunks)components/admins/modals/__tests__/validatorModal.test.tsx(2 hunks)components/admins/modals/__tests__/warningModal.test.tsx(2 hunks)components/admins/modals/cancelUpgradeModal.tsx(3 hunks)components/admins/modals/descriptionModal.tsx(1 hunks)components/admins/modals/multiMfxBurnModal.tsx(3 hunks)components/admins/modals/multiMfxMintModal.tsx(3 hunks)components/admins/modals/upgradeModal.tsx(4 hunks)components/admins/modals/validatorModal.tsx(5 hunks)components/admins/modals/warningModal.tsx(2 hunks)components/bank/components/tokenList.tsx(2 hunks)components/bank/forms/sendForm.tsx(2 hunks)components/bank/modals/sendModal.tsx(3 hunks)components/factory/components/DenomList.tsx(0 hunks)components/factory/forms/ConfirmationForm.tsx(2 hunks)components/factory/modals/BurnModal.tsx(3 hunks)components/factory/modals/MintModal.tsx(3 hunks)components/factory/modals/TransferModal.tsx(4 hunks)components/factory/modals/denomInfo.tsx(3 hunks)components/factory/modals/updateDenomMetadata.tsx(4 hunks)components/groups/components/__tests__/groupInfo.test.tsx(2 hunks)components/groups/forms/groups/ConfirmationForm.tsx(2 hunks)components/groups/modals/__tests__/voteDetailsModal.test.tsx(4 hunks)components/groups/modals/groupInfo.tsx(5 hunks)components/groups/modals/index.tsx(1 hunks)components/groups/modals/memberManagementModal.tsx(4 hunks)components/groups/modals/updateGroupModal.tsx(4 hunks)components/groups/modals/voteDetailsModal.tsx(14 hunks)components/groups/modals/voteModal.tsx(3 hunks)components/groups/modals/voting/messagesModal.tsx(1 hunks)components/react/authSignerModal.test.tsx(4 hunks)components/react/authSignerModal.tsx(5 hunks)components/react/index.ts(1 hunks)components/react/modal.tsx(2 hunks)config/env.ts(1 hunks)contexts/manifestAppProviders.tsx(1 hunks)contexts/web3AuthContext.tsx(1 hunks)hooks/index.tsx(1 hunks)hooks/useLocalStorage.tsx(1 hunks)hooks/useTx.tsx(4 hunks)pages/_app.tsx(2 hunks)tests/mock.ts(2 hunks)
💤 Files with no reviewable changes (1)
- components/factory/components/DenomList.tsx
✅ Files skipped from review due to trivial changes (3)
- components/groups/modals/index.tsx
- components/groups/forms/groups/ConfirmationForm.tsx
- components/factory/forms/ConfirmationForm.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- components/react/modal.tsx
- components/bank/forms/sendForm.tsx
- components/react/authSignerModal.test.tsx
- components/bank/components/tokenList.tsx
🧰 Additional context used
🧠 Learnings (6)
components/admins/modals/cancelUpgradeModal.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/admins/modals/cancelUpgradeModal.tsx:62-63
Timestamp: 2024-12-09T17:22:29.162Z
Learning: In the `CancelUpgradeModal` component (`components/admins/modals/cancelUpgradeModal.tsx`), the `address` prop is guaranteed to be defined because the wallet connect layer prevents the page from rendering if the wallet is not connected.
components/admins/modals/upgradeModal.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/admins/modals/upgradeModal.tsx:273-292
Timestamp: 2024-12-09T17:28:45.961Z
Learning: In the Manifest App project, it's acceptable to use direct DOM manipulation methods like `querySelector`, `closest`, `focus`, and `blur` when necessary to override Daisy UI defaults, as seen in `components/admins/modals/upgradeModal.tsx`.
components/groups/modals/memberManagementModal.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/groups/modals/memberManagementModal.tsx:0-0
Timestamp: 2024-12-09T18:36:10.398Z
Learning: We will refactor modals to use a common modal base instead of consistently redefining the same code, and we will use Tailwind CSS rather than defining styles inline. This method was previously used while fixing lots of code.
components/admins/modals/multiMfxBurnModal.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/factory/modals/multiMfxBurnModal.tsx:49-49
Timestamp: 2024-12-09T17:35:33.430Z
Learning: The `MultiBurnModal` component in `components/factory/modals/multiMfxBurnModal.tsx` is always used with a hardcoded `denom` of MFX, so adding a null check for `denom` is unnecessary.
components/factory/modals/BurnModal.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/factory/modals/BurnModal.tsx:0-0
Timestamp: 2024-12-09T17:35:02.534Z
Learning: In the codebase, `denom.base` used in `id` attributes will always contain characters acceptable according to the HTML5 specification, so additional sanitization is unnecessary.
components/admins/components/validatorList.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/admins/components/validatorList.tsx:61-76
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In the `ValidatorList` component, the modal `'warning-modal'` is always mounted when `handleRemove` is called, so additional error handling for its absence is unnecessary.
🪛 GitHub Actions: Build
components/admins/modals/validatorModal.tsx
[warning] 137-137: Do not use <img> element. Use <Image /> from next/image instead.
components/react/authSignerModal.tsx
[warning] 313-313: Do not use <img> element. Use <Image /> from next/image instead.
🪛 Biome (1.9.4)
components/bank/modals/sendModal.tsx
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/admins/modals/descriptionModal.tsx
[error] 19-19: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/react/authSignerModal.tsx
[error] 297-297: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 301-301: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (44)
components/groups/modals/__tests__/voteDetailsModal.test.tsx (1)
100-102: Great improvement in accessibility testing!The change to use aria-labels for button selection while separately verifying text content is a good practice that:
- Improves test reliability
- Ensures accessibility
- Aligns well with HeadlessUI's focus on accessibility
Also applies to: 114-114
components/factory/modals/updateDenomMetadata.tsx (2)
9-9: LGTM: Import changes align with refactor objectives.The addition of Headless UI's Dialog component and SignModal imports aligns with the PR's goal of standardizing dialog implementations.
Also applies to: 14-15
32-255: LGTM: Well-implemented Dialog refactor.The implementation successfully achieves the PR's goal of standardizing dialog components using Headless UI while maintaining existing functionality. The code is well-structured with proper form validation, error handling, and loading states.
components/admins/components/__tests__/validatorList.test.tsx (1)
68-68: LGTM! Improved test selector reliability.The change from text-based selector to
data-testidmakes the test more maintainable and resilient to UI changes.components/admins/components/validatorList.tsx (1)
239-241: LGTM! Improved event handler and test coverage.The changes optimize the onClick handler and add proper test ID for better testability.
components/react/index.ts (1)
6-6: LGTM! Export aligns with dialog refactoring objectives.The addition of
authSignerModalexport makes the component available for use with the new Headless UI Dialog system.Let's verify the component's implementation and its usage across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the authSignerModal component implementation and its usage # Check the component implementation echo "Checking authSignerModal implementation..." ast-grep --pattern 'export const authSignerModal = $_' # Check component usage echo "Checking authSignerModal usage..." rg -A 5 'authSignerModal'Length of output: 1373
LGTM! Export of authSignerModal is correctly integrated.
- The barrel file (
components/react/index.ts) re-exports the module as intended.- Usage in both
components/groups/modals/index.tsxand the test file confirms that the module (now providingSignModalInner) is correctly referenced in the codebase.contexts/manifestAppProviders.tsx (1)
43-55: Double-check environment variable consistency.It's good to see environment-based URLs. However, if
env.rpcUrl,env.apiUrl, etc., are missing or improperly set, this could lead to runtime issues. Consider adding fallback handling or logging to help diagnose configuration errors quickly.components/groups/modals/groupInfo.tsx (1)
145-155: Good practice: Assigning a modal identifier to the tx call.Providing an identifier (
'leave-modal') helps correlate transactions to user actions, which is beneficial for logging or controlling modals. Be sure to confirm that if other transaction logic expects the same identifier, it is consistently used.components/groups/modals/memberManagementModal.tsx (3)
15-16: New imports align with the Headless UI refactor.Importing
DialogandSignModalsets up the new modal logic. Ensure that other references to the old<dialog>approach have been fully removed.
192-194: Early bail-out logic looks good.Returning
nullwhenshowMemberManagementModalis false is a solid approach for screen rendering clarity and performance.
196-210: Confirm consistent use of<Dialog>across all modals.You’ve replaced the native
<dialog>with Headless UI’s<Dialog>. Verify that modal closing logic (e.g., clicking the backdrop, pressing Escape) is consistent with other modals in this app for a uniform UX.components/groups/modals/voteDetailsModal.tsx (10)
1-33: Refactored imports look good.All newly introduced imports (e.g.,
proposalStatusFromJSON,Dialog,SignModal,MessagesModal, icons) align well with the PR’s objective to switch to the Headless UI’s<Dialog>component and to modularize functionality. No issues noted here.
112-113: Status parsing appears correct.Using
proposalStatusFromJSONto derive the typed status fromproposal.statusis consistent and ensures type safety.
143-143: Withdrawal status is properly handled.Mapping
PROPOSAL_STATUS_WITHDRAWNto'withdrawn'helps differentiate it from other states in the UI. This is a good, clear addition.
466-467: Modal states for messages and voting look fine.Declaring
showMessagesandshowVotingPopupas separate state flags is clear and aligns with typical React patterns for modals.
469-493: Transition to Headless UI<Dialog>is smooth.Using
<Dialog open={showVoteModal} onClose={onClose}>is in line with Headless UI best practices, and the overlay plus panel structure is correctly implemented.
550-552: Expand messages button logic is straightforward.The button sets a clear state to open the message details. This is intuitive and easy to follow.
697-699: Good integration for “vote” and “remove” actions.Switching between setting the voting popup and executing withdrawal is a concise approach that keeps the main code path clear.
713-726: Additional removal button logic for proposals is well-handled.Conditionally rendering the withdrawal button when the user is a proposer and hasn’t voted prevents unintended actions. The gating logic is coherent.
731-749: Use of dedicated modals is a neat separation of concerns.
<MessagesModal>and<VotingPopup>are well-structured, reusing props and ensuring each manages its own internal state effectively.
751-753: SignModal integration aligns with the new flow.Adding
<SignModal id="withdrawalPrompt" />centralizes signing steps without adding clutter. Nicely done.hooks/index.tsx (1)
9-9: Export ofuseLocalStoragefosters reusability.Exposing
useLocalStoragein your hooks index ensures a consistent import path across the codebase.hooks/useLocalStorage.tsx (1)
1-17: Local storage initialization is well-protected.Your check for
typeof window !== 'undefined'avoids SSR issues. Defaulting toinitialValueon parsing errors is sensible.config/env.ts (1)
2-2: LGTM! Clean addition of production environment check.The addition of the
productionproperty follows best practices for environment configuration.components/admins/modals/__tests__/descriptionModal.test.tsx (1)
16-16: LGTM! Props updated to match HeadlessUI Dialog pattern.The test cases have been correctly updated to use the new
openprop instead ofmodalId, aligning with the HeadlessUI Dialog component pattern.Also applies to: 22-22
pages/_app.tsx (2)
22-22: LGTM! Improved state persistence with useLocalStorage.Good use of the
useLocalStoragehook to persist drawer visibility state across sessions.
25-43: LGTM! Clean layout structure with proper responsive classes.The layout implementation:
- Uses proper Tailwind classes for responsiveness
- Maintains clean transitions for drawer
- Properly handles mobile navigation
components/groups/modals/voting/messagesModal.tsx (2)
7-15: LGTM! Clean HeadlessUI Dialog implementation.The modal implementation follows HeadlessUI best practices with proper prop types and null handling.
24-24: LGTM! Good accessibility implementation.The modal has proper ARIA labels and semantic HTML structure:
- Modal role with proper z-index
- Dialog panel with descriptive label
- Message headings with proper hierarchy
- Pre-formatted content with descriptive label
Also applies to: 28-28, 43-43, 49-49
components/bank/modals/sendModal.tsx (1)
39-79: LGTM! Dialog implementation follows best practices.The Dialog component implementation:
- Correctly handles modal state and closure
- Implements proper backdrop with accessibility attributes
- Maintains consistent styling with other modals
components/factory/modals/denomInfo.tsx (1)
24-96: LGTM! Dialog implementation looks good.The migration to Headless UI's Dialog component is well-implemented with proper backdrop and panel structure. The component correctly handles modal state and closing behavior.
components/admins/modals/cancelUpgradeModal.tsx (1)
149-150: Verify SignModal integration.The SignModal component has been added but its purpose and interaction with the cancel upgrade flow isn't immediately clear.
✅ Verification successful
🌐 Web query:
What is the purpose of HeadlessUI's Dialog component and how does it handle nested dialogs?💡 Result:
Headless UI's Dialog component provides an accessible foundation for building modal windows with built-in focus management and ARIA compliance[4][8]. Its primary purpose is to handle complex accessibility requirements automatically while giving developers full control over styling and structure[4][7].
Key Features
- Automatic portal rendering ensures proper layering[4][8]
- Focus trapping keeps keyboard navigation within active dialog[3][8]
- Inert attribute application disables background content interaction[1][4]
- Escape key/click-outside dismissal with proper event handling[4][8]
Nested Dialog Handling
Headless UI manages nested dialogs through:
Accessibility layers
- Parent dialogs automatically apply
inertto non-dialog siblings[1][4]- Uses stacking order based on dialog creation time (v2.1+)[2]
// Example of sibling dialogs (v2.1+) <Dialog open={firstOpen}>{/* ... */}</Dialog> <Dialog open={secondOpen}>{/* ... */}</Dialog>Focus management
- Maintains separate focus contexts for each dialog[3]
- Last-opened dialog receives primary focus[2]
- Escape key closes only the top-most dialog[2]
Portal behavior
- Each dialog renders in
headlessui-portal-rootby default[4][8]- Nested portals can conflict with parent dialog's
inertattributes[1]// Potential issue with third-party portal components <Dialog> <MyComponentWithPortal /> {/* Might get inert from parent dialog */} </Dialog>For complex nesting scenarios requiring multiple active layers:
- Use v2.1+ sibling dialog support instead of physical nesting[2]
- Manage z-index values explicitly
- Avoid portal-over-portal scenarios when possible[9]
The component prioritizes accessibility over convenience - while nested dialogs are possible through manual state management[5], the recommended pattern is using sibling dialogs with proper focus ordering[2]. Developers should test thoroughly when combining with third-party portal-based components like dropdowns or datepickers[1][9].
Citations:
- 1: Nested Dialog adds inert prop to every sibling of #headless-ui-portal-root tailwindlabs/headlessui#2413
- 2: https://tailwindcss.com/blog/2024-06-21-headless-ui-v2-1
- 3: https://app.studyraid.com/en/read/11966/381800/focus-management-in-dialogs
- 4: https://headlessui.com/react/dialog
- 5: https://ej2.syncfusion.com/react/documentation/dialog/how-to/create-nested-dialog
- 6: https://headlessui.com/v1/vue/dialog
- 7: https://plainenglish.io/blog/how-to-use-the-dialog-component-with-headless-ui-and-tailwind-css-f8f11edcaf06
- 8: https://headlessui.com/v1/react/dialog
- 9: https://docs.privy.io/guide/troubleshooting/ui/dialog
- 10: https://olamijibadmos.hashnode.dev/how-to-create-modals-in-react-with-headlessuireact
SignModal Integration Verification
- The integration of SignModal within the
Dialog.Panelappears to follow Headless UI’s recommended patterns for nested dialogs—ensuring focus trapping, proper layering via portals, and inert management according to the web guidelines.- However, its exact purpose and how it fits into the cancel upgrade flow isn’t immediately clear from the snippet. It would be beneficial to add inline comments or documentation that clarify the expected behavior and user interaction for the SignModal in this specific context.
hooks/useTx.tsx (1)
169-171: Potential race condition in promptId management.The
setPromptId(undefined)in the finally block might clear a different promptId if multiple transactions are in progress.Consider tracking the initial promptId and only clear if it matches:
+ const initialPromptId = id; try { // ... existing code ... } finally { - setPromptId(undefined); + if (promptId === initialPromptId) { + setPromptId(undefined); + } setIsSigning(false); }components/admins/modals/multiMfxBurnModal.tsx (1)
18-19: LGTM! Dialog and SignModal imports added.The imports align with the PR objectives to refactor all dialogs to use Headless UI's Dialog component.
tests/mock.ts (1)
534-535: LGTM! Improved test data by setting voting period end to tomorrow.The change makes the test data more realistic by ensuring the voting period ends the next day instead of immediately.
Also applies to: 556-556
components/groups/modals/updateGroupModal.tsx (3)
20-21: LGTM! Added imports for Dialog and SignModal components.The imports align with the PR objectives to refactor dialogs using Headless UI's Dialog component.
213-223: LGTM! Added transaction identifier for better tracking.The addition of 'update-group-modal' identifier improves transaction tracking and debugging capabilities.
313-433: LGTM! Refactored modal to use Headless UI's Dialog component.The changes improve accessibility and standardize dialog behavior across the application. The implementation includes:
- Proper backdrop for better visual presentation
- Improved keyboard navigation (Escape key handling)
- Integration with SignModal for consistent signing behavior
components/admins/modals/validatorModal.tsx (3)
17-18: LGTM! Import changes align with refactor objectives.The addition of Headless UI's Dialog component and SignModal imports support the dialog refactoring goals.
45-45: LGTM! State management simplified.The introduction of the
descriptionstate variable streamlines the modal state management, aligning with the PR's goal to reduce complexity.
242-252: Verify SignModal integration and stacking behavior.The integration of DescriptionModal and SignModal supports multiple dialogs, but let's verify the stacking behavior works as expected.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for other SignModal usages to ensure consistent implementation rg -l "SignModal" --type tsxLength of output: 56
Let's try re-running the search using a file glob for TSX files.
🏁 Script executed:
#!/bin/bash # Searching for occurrences of "SignModal" in TSX files using a file glob. rg -l "SignModal" -g "*.tsx" .Length of output: 949
SignModal Integration and Stacking Behavior Verification
The static analysis shows that
SignModalis being rendered consistently across multiple modal components (including incomponents/admins/modals/validatorModal.tsx), which indicates that its integration follows the same pattern as other dialogs. However, since stacking behavior depends on runtime CSS (e.g., z-index management) and internal state handling, please verify in the UI that when both theDescriptionModalandSignModalare active, their layering meets the design requirements.
- Location:
components/admins/modals/validatorModal.tsx- Observation:
DescriptionModalis conditionally rendered (based on state) whileSignModalis always rendered as a sibling. The stacking order is therefore determined by their internal CSS and any portal usage.- Action: Manually confirm in the UI that the dialogs are displayed with the correct stacking order.
components/admins/modals/__tests__/warningModal.test.tsx (1)
57-64: Address the commented-out test for modal closing.The test for modal closing behavior is commented out with a reference to a GitHub issue. Please:
- Check if the referenced issue is still relevant
- Update the test to work with Headless UI's Dialog
- Remove the commented code if it's no longer needed
✅ Verification successful
🌐 Web query:
Is GitHub issue #1184 in haVyppy-dom still open?💡 Result:
GitHub issue #1184 in the Happy DOM repository does not appear in the provided search results. The latest issues listed for Happy DOM range from #1585 to #1718 as of February 2025[4][7]. Given that issue numbers have progressed well beyond #1184, it would have been resolved or closed by now.
The search results show recent activity focused on newer issues like security vulnerabilities (#1585)[4], flaky tests (#1716)[7], and DOM parsing improvements (#1700)[7]. For definitive confirmation, you should check the Happy DOM GitHub Issues page directly[7].
Citations:
- 1: https://nextcloud.com/changelog/
- 2: https://github.com/capricorn86/jest-environment-happy-dom
- 3: Help open-source maintainers stay sane isaacs/github#167
- 4: Security vulnerability that allows for scripts to execute on the server capricorn86/happy-dom#1585
- 5: https://github.com/capricorn86/happy-dom/wiki/Getting-started
- 6: Issue types at creation rather than labels later on dear-github/dear-github#59
- 7: https://github.com/capricorn86/happy-dom/issues
- 8: https://www.npmjs.com/package/happy-dom/v/13.8.0
- 9: https://www.npmjs.com/package/happy-dom
- 10: https://github.com/capricorn86/happy-dom-without-node
Update the modal closing test with the current Headless UI implementation.
Based on the web query, the referenced GitHub issue (#1184) appears to be outdated—its number is well behind the current issues, which suggests the problem was resolved. Please review the modal closing behavior in the context of Headless UI's Dialog. If the test is still relevant, update and uncomment it to correctly assert that the modal closes when the close button is clicked. Otherwise, remove the commented-out code.
components/admins/modals/warningModal.tsx (2)
77-77: LGTM! Simplified modal closing logic.The modal closing logic has been simplified by leveraging Dialog's built-in close handling, reducing the need for custom event listeners.
9-10: Verify SignModal integration.The SignModal component has been added but its purpose and integration need verification:
- Is it properly integrated with the transaction flow?
- Are there any potential state conflicts with the main modal?
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other SignModal usages to understand the integration pattern rg -l "SignModal" --type tsxLength of output: 56
The initial search using the
--type tsxflag failed because “tsx” wasn’t a recognized file type. Please run the following script to search for SignModal usage using a glob pattern (which should cover *.tsx files):If this search still produces no results, you'll need to manually verify the integration:
- Check if
SignModalis actively used in the transaction flow.- Verify that its usage doesn’t cause any state conflicts with the main modal.
- Specifically review its implementation and usage in
components/admins/modals/warningModal.tsx.
🏁 Script executed:
#!/bin/bash # Searching for SignModal usages across the repository in files with .tsx extension rg -l "SignModal" -g "*.tsx"Length of output: 907
SignModal Integration Confirmed
Based on the repository search:
- Multiple modal files (e.g., cancelUpgradeModal, multiMfxBurnModal, upgradeModal, etc.) consistently import and use SignModal, including in warningModal.
- The integration pattern appears to align with transaction flow practices used across similar components, with no immediate evidence of state conflicts with the main modal.
No integration issues were observed from the static analysis.
| clientId: env.web3AuthClientId, | ||
| web3AuthNetwork: env.web3AuthNetwork as WEB3AUTH_NETWORK_TYPE, // Safe to cast since we validate the env vars in config/env.ts |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify environment variables at runtime.
Ensure environment variables are available at runtime to prevent potential runtime errors.
🏁 Script executed:
#!/bin/bash
# Check if environment variables are defined in configuration files
rg -A 2 'web3AuthClientId|web3AuthNetwork' Length of output: 602
Based on our search results:
- The context file imports the environment configuration from
config/env.tsand uses:clientId: env.web3AuthClientId,web3AuthNetwork: env.web3AuthNetwork as WEB3AUTH_NETWORK_TYPE
- In
config/env.ts, the environment variables are set using:web3AuthNetwork: process.env.NEXT_PUBLIC_WEB3AUTH_NETWORK ?? ''web3AuthClientId: process.env.NEXT_PUBLIC_WEB3AUTH_CLIENT_ID ?? ''
- There is no explicit runtime validation (e.g., throw errors if a variable is missing) in the shown snippet; instead, missing variables fall back to empty strings.
- The inline comment in
web3AuthContext.tsxstates that the cast is “Safe to cast since we validate the env vars in config/env.ts”. However, based on the configuration shown, this validation does not actively enforce the presence of runtime environment variables—it only provides default empty string values.
Recommendation:
- Consider adding explicit runtime validation in
config/env.tsto ensure that critical environment variables (likeNEXT_PUBLIC_WEB3AUTH_CLIENT_IDandNEXT_PUBLIC_WEB3AUTH_NETWORK) are correctly defined, rather than defaulting silently to empty strings. This will prevent potential runtime errors when these variables are inadvertently missing.
Action Items:
- Add checks in
config/env.tsto throw an error when critical variables are missing instead of falling back silently.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/messageSyntax.tsx (3)
5-8: Consider improving type safety of the index signature.While the type is now properly exported, consider making the index signature more specific to prevent potential type-related issues.
export type MessageType = { '@type': string; - [key: string]: any; + [key: string]: string | number | boolean | object | null | undefined; };
10-33: Add error handling for missing message fields.The function should handle cases where requested fields don't exist in the message object.
export function messageSyntax(fieldsToShow: string[], message: MessageType, theme: string) { const prettyPrintJSON = (obj: Record<string, any>): string => { return JSON.stringify(obj, null, 2); }; return ( <SyntaxHighlighter language="json" style={theme === 'dark' ? oneDark : oneLight} customStyle={{ backgroundColor: 'transparent', padding: '1rem', borderRadius: '0.5rem', }} > {prettyPrintJSON( fieldsToShow.reduce((acc: Record<string, any>, field: string) => { - acc[field] = message[field]; + if (field in message) { + acc[field] = message[field]; + } return acc; }, {}) )} </SyntaxHighlighter> ); }
35-53: Extract shared prettyPrintJSON function to reduce duplication.The
prettyPrintJSONfunction is duplicated in bothmessageSyntaxandobjectSyntax. Consider extracting it to a shared utility.+const prettyPrintJSON = (obj: Record<string, any>): string => { + return JSON.stringify(obj, null, 2); +}; + export function messageSyntax(fieldsToShow: string[], message: MessageType, theme: string) { - const prettyPrintJSON = (obj: Record<string, any>): string => { - return JSON.stringify(obj, null, 2); - }; // ... rest of messageSyntax function } export function objectSyntax(object: Record<string, any>, theme: string) { - const prettyPrintJSON = (obj: Record<string, any>): string => { - return JSON.stringify(obj, null, 2); - }; // ... rest of objectSyntax function }contexts/manifestAppProviders.tsx (2)
58-80: Optimize performance by memoizing signerOptions.The
signerOptionsobject and its internal registry merging could be memoized to prevent unnecessary recreations on each render.Consider applying this optimization:
- const signerOptions: SignerOptions = { + const signerOptions = useMemo<SignerOptions>(() => ({ signingStargate: (_chain: string | Chain): SigningStargateClientOptions | undefined => { const mergedRegistry = new Registry([ ...cosmosProtoRegistry, ...osmosisProtoRegistry, ...strangeloveVenturesProtoRegistry, ...liftedinitProtoRegistry, ...ibcProtoRegistry, ]); const mergedAminoTypes = new AminoTypes({ ...cosmosAminoConverters, ...liftedinitAminoConverters, ...osmosisAminoConverters, ...ibcAminoConverters, ...strangeloveVenturesAminoConverters, }); return { aminoTypes: mergedAminoTypes, registry: mergedRegistry, }; }, - }; + }), []);
115-116: Conditionally render ReactQueryDevtools based on environment.Consider rendering ReactQueryDevtools only in non-production environments to reduce bundle size in production.
<QueryClientProvider client={client}> - <ReactQueryDevtools /> + {!env.production && <ReactQueryDevtools />} <Web3AuthProvider>components/admins/modals/descriptionModal.tsx (1)
19-21: Add test coverage for the close handler.The
handleClosefunction is not covered by tests. Consider adding test cases for both scenarios: whenonCloseis provided and when it's undefined.Consider using optional chaining for better readability.
- function handleClose() { - onClose && onClose(); - } + function handleClose() { + onClose?.(); + }❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if there are any existing tests for the close handler rg -A 5 "test.*close.*handler|test.*onClose" components/admins/modals/__tests__Length of output: 81
I'll generate a new verification script to search for direct references to
handleClosein tests. This will help further confirm whether any tests cover the close handler. Please verify the output once the script executes.
🏁 Script executed:
#!/bin/bash # Search directly for any mention of 'handleClose' in test files rg "handleClose" components/admins/modals/__tests__Length of output: 51
Test coverage missing for the close handler & optional chaining suggestion applicable
The repository shows no tests covering
handleClose(i.e. no references found in the test files for eitherhandleCloseoronClose). Please add test cases for both scenarios—whenonCloseis provided and when it isn’t. Also, consider simplifying the implementation by using optional chaining (i.e.onClose?.()) for better readability.
- Missing tests: No test cases currently cover
handleClose.- Refactor: Use optional chaining to simplify the function.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
[warning] 19-19: components/admins/modals/descriptionModal.tsx#L19
Added line #L19 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/admins/modals/descriptionModal.tsx(1 hunks)components/groups/modals/voting/messagesModal.tsx(1 hunks)components/messageSyntax.tsx(1 hunks)contexts/manifestAppProviders.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/groups/modals/voting/messagesModal.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/admins/modals/descriptionModal.tsx
[error] 20-20: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
components/admins/modals/descriptionModal.tsx
[warning] 19-19: components/admins/modals/descriptionModal.tsx#L19
Added line #L19 was not covered by tests
🔇 Additional comments (4)
contexts/manifestAppProviders.tsx (2)
94-98: Update placeholder values in WalletConnect metadata.The WalletConnect metadata appears to contain placeholder values that should be updated with actual production values:
- 'Alberto' as the name
- 'alberto.com' as the URL
Please verify and update these values to match your production configuration.
103-104: Remove TypeScript ignore comment by properly typing the TailwindModal component.The
@ts-ignorecomment suppresses type checking. Consider properly typing theTailwindModalcomponent to match the expected type fromChainProvider.components/admins/modals/descriptionModal.tsx (2)
1-9: LGTM! Well-structured interface changes.The updated interface properly defines the props needed for a controlled modal component using Headless UI.
11-17: LGTM! Good use of early return pattern.The component correctly implements the controlled component pattern with proper type safety using
Readonly.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
components/bank/forms/sendForm.tsx (2)
161-163: 🛠️ Refactor suggestionConsider removing redundant error handling.
The
useTxhook already handles errors internally, making this try-catch block redundant. Based on learnings from PR #9, additional error handling inhandleSendis unnecessary.Apply this diff to remove the redundant error handling:
- try { const exponent = values.selectedToken.metadata?.denom_units[1]?.exponent ?? 6; const amountInBaseUnits = parseNumberToBigInt(values.amount, exponent).toString(); const msg = isGroup ? submitProposal({ // ... proposal configuration }) : send({ // ... send configuration }); const fee = await estimateFee(address, [msg]); await tx([msg], { memo: values.memo, fee, onSuccess: () => { refetchBalances(); refetchHistory(); refetchProposals?.(); }, }); - } catch (error) { - console.error('Error during sending:', error); - } finally { setIsSending(false); - }
152-160: 💡 Verification agent🧩 Analysis chain
Add test coverage for transaction handling.
The transaction handling code is not covered by tests. Since this is critical functionality, it should be thoroughly tested.
Let's verify the test coverage:
Would you like me to help create comprehensive test cases for the transaction handling code?
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for SendForm component # Test: Search for test files related to SendForm fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i "sendform"Length of output: 174
I'll verify if the transaction handling code is covered in the test files for the SendForm. Let me search for references to
tx(in the test file to see if the transaction handling is being executed in tests.
🏁 Script executed:
#!/bin/bash # Search for transaction handling references ("tx(") in the sendForm test file. rg "tx\\(" components/bank/forms/__tests__/sendForm.test.tsxLength of output: 61
I'll now check the other test file (
ibcSendForm.test.tsx) for any references totx(to further verify the test coverage.
🏁 Script executed:
#!/bin/bash # Search for transaction handling references ("tx(") in ibcSendForm.test.tsx file. rg "tx\\(" components/bank/forms/__tests__/ibcSendForm.test.tsxLength of output: 64
Transaction handling lacks test coverage.
The search for references to the
tx(call in the SendForm test files (both sendForm.test.tsx and ibcSendForm.test.tsx) produced no results. This indicates that the critical transaction handling code (lines 152–160) in components/bank/forms/sendForm.tsx isn’t explicitly covered by tests.
- Action Required: Add tests to cover scenarios for transaction success (ensuring
refetchBalances,refetchHistory, andrefetchProposalsare called) and potential failure modes.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-152: components/bank/forms/sendForm.tsx#L152
Added line #L152 was not covered by tests
♻️ Duplicate comments (1)
components/bank/forms/sendForm.tsx (1)
152-152: 🛠️ Refactor suggestionRemove unused variable.
The
txResultvariable is declared but never used. Consider removing it if not needed.Apply this diff to fix the issue:
- let txResult = await tx([msg], { + await tx([msg], {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-152: components/bank/forms/sendForm.tsx#L152
Added line #L152 was not covered by tests
🧹 Nitpick comments (2)
components/admins/modals/descriptionModal.tsx (1)
24-54: Remove unnecessary form method attribute.The
method="dialog"attribute is a remnant from the native<dialog>implementation and is not needed with Headless UI's Dialog component.Apply this diff:
- <form method="dialog" className="modal-box "> + <form className="modal-box">Otherwise, the Dialog implementation looks good with proper backdrop and close button handling.
utils/staking.ts (1)
184-195: Consider optimizing the WordArray conversion.While the implementation is correct, it could be optimized for better performance and readability.
Consider this optimized version:
-function wordArrayToUint8Array(wordArray: CryptoJS.lib.WordArray) { - const words = wordArray.words; - const sigBytes = wordArray.sigBytes; - const u8 = new Uint8Array(sigBytes); - - for (let i = 0; i < sigBytes; i++) { - const byte = (words[i >>> 2] >>> (24 - (i % 4) * 8)) & 0xff; - u8[i] = byte; - } - - return u8; -} +function wordArrayToUint8Array(wordArray: CryptoJS.lib.WordArray): Uint8Array { + const { words, sigBytes } = wordArray; + const u8 = new Uint8Array(sigBytes); + + for (let i = 0; i < sigBytes; i++) { + const position = i >>> 2; + const shift = 24 - (i % 4) * 8; + u8[i] = (words[position] >>> shift) & 0xff; + } + + return u8; +}Changes made:
- Added return type annotation for better type safety
- Used object destructuring for cleaner variable assignment
- Split the bit manipulation into more readable steps
- Added proper spacing for better readability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
components/admins/modals/descriptionModal.tsx(1 hunks)components/bank/forms/sendForm.tsx(1 hunks)components/groups/modals/groupDetailsModal.tsx(0 hunks)components/groups/modals/index.tsx(0 hunks)package.json(1 hunks)utils/staking.ts(2 hunks)
💤 Files with no reviewable changes (2)
- components/groups/modals/index.tsx
- components/groups/modals/groupDetailsModal.tsx
🧰 Additional context used
🧠 Learnings (1)
components/bank/forms/sendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🪛 Biome (1.9.4)
components/admins/modals/descriptionModal.tsx
[error] 20-20: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
components/admins/modals/descriptionModal.tsx
[warning] 19-19: components/admins/modals/descriptionModal.tsx#L19
Added line #L19 was not covered by tests
components/bank/forms/sendForm.tsx
[warning] 152-152: components/bank/forms/sendForm.tsx#L152
Added line #L152 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
components/admins/modals/descriptionModal.tsx (2)
1-9: LGTM! Interface changes align with Headless UI's Dialog pattern.The updated interface properly reflects the transition to using Headless UI's Dialog component by replacing the
modalIdwithopenandonCloseprops.
11-18: LGTM! Good use of early return pattern.The early return when
!openis a clean way to handle conditional rendering.utils/staking.ts (1)
2-2: LGTM! Clean import statement.The change to use a named import for
bech32follows modern ES6+ practices and improves code clarity.package.json (1)
58-58: Verify the bech32 package version.Let's ensure we're using the latest stable version of the bech32 package.
✅ Verification successful
🌐 Web query:
What is the latest version of the bech32 npm package?💡 Result:
The latest version of the
bech32npm package is 2.0.0, released on February 15, 2021[3][10]. This version supports both Bech32 and Bech32m encoding/decoding standards used in Bitcoin address formats[3][6]. The package provides ESM and CJS module compatibility and includes TypeScript definitions[1][7].Key details:
- Published to npm 4 years ago (as of February 2025)[3][10]
- MIT licensed[1][6]
- Contains both
bech32(BIP173) andbech32m(BIP350) implementations[3][6]- Includes testing scripts and TypeScript support[1]
No newer versions appear in the search results through February 2025[3][6][10].
Citations:
- 1: https://github.com/bitcoinjs/bech32/blob/master/package.json
- 2: https://www.jsdocs.io/package/bech32
- 3: https://www.npmjs.com/package/bech32
- 4: https://npmjs.com/package/bc-bech32?activeTab=versions
- 5: https://www.npmjs.com/package/bech32-buffer
- 6: https://npm.io/package/bech32
- 7: https://www.npmjs.com/package/@types/bech32
- 8: https://www.npmjs.com/package/@jackallabs/bech32?activeTab=versions
- 9: https://cloudsmith.com/navigator/npm/@types/bech32
- 10: https://www.npmjs.com/package/bech32/v/0.0.1?activeTab=versions
Bech32 Package Version is Up-to-Date
- The current dependency (
"bech32": "^2.0.0") in thepackage.jsonalready reflects the latest stable version as of February 2025.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/admins/modals/descriptionModal.tsx (1)
31-49: Consider removing native dialog attributes.Since we're using Headless UI's Dialog component, the
method="dialog"attribute on the form is unnecessary and might be confusing.- <form method="dialog" className="modal-box "> + <form className="modal-box">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/admins/modals/descriptionModal.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test and Coverage
components/admins/modals/descriptionModal.tsx
[error] 20-20: The above error occurred in the component. Consider adding an error boundary to your tree to customize error handling behavior.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
components/admins/modals/descriptionModal.tsx (2)
1-9: LGTM! Props interface aligns with Headless UI requirements.The interface changes correctly reflect the transition to using Headless UI's Dialog component, with appropriate prop types for modal control.
11-18: LGTM! Good use of early return pattern.The component signature is well-structured with Readonly props, and the early return optimization prevents unnecessary rendering when the modal is closed.
There was a problem hiding this comment.
Thanks! A few comments
-
Pressing the
xbutton in the sign modal does nothing
-
Pressing
Executeon a proposal gives the following error
-
Withdrawing a proposal and rejecting the transaction closes all modals
-
Voting on a proposal and clicking the
xbutton on the "Case your vote" dialog closes the dialog and returns to the proposal. Clicking on theVotebutton again shows the sign modal.
Might not be related to this PR?:
-
I couldn't see my groups after login. I had to refresh + re-login. I'm using
Emailas my login.No group?
After re-login
I also noticed this behavior after creating a new token. I had to refresh + re-login to see it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/react/authSignerModal.tsx (3)
118-164: Consider enhancing error handling and splitting complex functions.The data formatting and decoding logic could be improved for better maintainability and debugging:
- Error messages could include more context about the failure
- The
formatValuefunction handles multiple responsibilities and could be split into smaller, focused functionsConsider applying these improvements:
const decodeBodyBytes = (bodyBytes: Uint8Array) => { try { const decodedBody = TxBody.decode(bodyBytes); return { messages: decodedBody.messages.map(msg => { const base64Value = Buffer.from(msg.value).toString('base64'); try { if (messageDecoders[msg.typeUrl]) { return { typeUrl: msg.typeUrl, value: messageDecoders[msg.typeUrl](msg.value), }; } const decodedValue = Buffer.from(base64Value, 'base64').toString('utf8'); try { return { typeUrl: msg.typeUrl, value: JSON.parse(decodedValue), }; } catch { return { typeUrl: msg.typeUrl, value: decodedValue, }; } } catch (error) { - console.error(`Failed to decode message of type ${msg.typeUrl}:`, error); + console.error( + `Failed to decode message of type ${msg.typeUrl}:`, + error, + '\nValue:', base64Value + ); return { typeUrl: msg.typeUrl, value: base64Value, }; } }), memo: decodedBody.memo, timeoutHeight: decodedBody.timeoutHeight.toString(), extensionOptions: decodedBody.extensionOptions, nonCriticalExtensionOptions: decodedBody.nonCriticalExtensionOptions, }; } catch (error) { - console.error('Failed to decode bodyBytes:', error); + console.error('Failed to decode bodyBytes:', error, '\nValue:', bodyBytes); return 'Failed to decode bodyBytes'; } }; // Split formatValue into smaller functions +const formatUint8Array = (value: Uint8Array) => { + return Buffer.from(value).toString('base64'); +}; + +const formatTxData = (value: any, theme: string) => { + const decodedValue = { + ...value, + bodyBytes: decodeBodyBytes(value.bodyBytes), + authInfoBytes: decodeAuthInfoBytes(value.authInfoBytes), + }; + return objectSyntax( + JSON.parse( + JSON.stringify(decodedValue, (_, v) => (typeof v === 'bigint' ? v.toString() : v)) + ), + theme + ); +}; + const formatValue = (value: any, theme: string) => { if (value instanceof Uint8Array) { - return Buffer.from(value).toString('base64'); + return formatUint8Array(value); } if (typeof value === 'object' && value !== null) { if ('bodyBytes' in value && 'authInfoBytes' in value) { - const decodedValue = { - ...value, - bodyBytes: decodeBodyBytes(value.bodyBytes), - authInfoBytes: decodeAuthInfoBytes(value.authInfoBytes), - }; - return objectSyntax( - JSON.parse( - JSON.stringify(decodedValue, (_, v) => (typeof v === 'bigint' ? v.toString() : v)) - ), - theme - ); + return formatTxData(value, theme); } return objectSyntax( JSON.parse(JSON.stringify(value, (_, v) => (typeof v === 'bigint' ? v.toString() : v))), theme ); } return String(value); };Also applies to: 166-186, 188-212
251-281: Enhance type safety and null checks.The SignModal component could benefit from more explicit type safety and null checks:
Consider these improvements:
+interface SignModalProps { + id?: string; +} + -export const SignModal = ({ id }: { id?: string }) => { +export const SignModal = ({ id }: SignModalProps) => { const { prompt, promptId } = useContext(Web3AuthContext); const [visible, setVisible] = useState(false); const [data, setData] = useState<SignData | undefined>(undefined); useEffect(() => { - if (promptId === id && prompt !== undefined) { + if (promptId === id && prompt?.signData) { setVisible(true); setData(prompt.signData); } else { setVisible(false); + setData(undefined); } }, [promptId, id, prompt]); - if (!prompt || !data || !visible) { + if (!prompt?.signData || !data || !visible) { return null; } - const approve = () => prompt.resolve(true); - const reject = () => prompt.resolve(false); + const approve = () => prompt?.resolve?.(true); + const reject = () => prompt?.resolve?.(false); return ( <SignModalInner visible={visible} onClose={reject} data={data} reject={reject} approve={approve} /> ); };
313-352: Enhance dialog accessibility.The Dialog component could benefit from improved accessibility attributes:
Consider these improvements:
- <Dialog open onClose={onClose} className="modal modal-open top-0 right-0 z-[9999]"> + <Dialog + open + onClose={onClose} + className="modal modal-open top-0 right-0 z-[9999]" + aria-labelledby="dialog-title" + aria-describedby="dialog-description" + > <div className="fixed inset-0 backdrop-blur-sm bg-black/30" aria-hidden="true" /> <Dialog.Panel className="modal-box max-w-lg w-full dark:bg-[#1D192D] bg-[#FFFFFF] rounded-lg shadow-xl"> <div className="flex justify-between items-center pb-4"> <div className="flex items-center gap-3"> <img src={getRealLogo(walletIconString, theme === 'dark')} alt="Wallet type logo" className="w-8 h-8" /> - <h3 className="text-xl font-semibold">Approve transaction?</h3> + <h3 id="dialog-title" className="text-xl font-semibold">Approve transaction?</h3> </div> - <button className="btn btn-sm btn-circle btn-ghost" onClick={onClose}> + <button + className="btn btn-sm btn-circle btn-ghost" + onClick={onClose} + aria-label="Close dialog" + > ✕ </button> </div> - <DisplayDataToSign + <DisplayDataToSign + id="dialog-description" data={data ?? ({} as SignData)} address={address ?? ''} theme={theme} className="space-y-4" addressClassName="p-3 rounded-md text-sm overflow-auto h-12 dark:bg-[#E0E0FF0A] bg-[#E0E0FF0A] dark:border-[#FFFFFF33] border-[#00000033] border" txInfoClassName="p-3 rounded-md text-sm overflow-auto h-[32rem] dark:bg-[#E0E0FF0A] bg-[#E0E0FF0A] dark:border-[#FFFFFF33] border-[#00000033] border" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/react/authSignerModal.test.tsx(4 hunks)components/react/authSignerModal.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/react/authSignerModal.test.tsx
🧰 Additional context used
🧠 Learnings (1)
components/react/authSignerModal.tsx (1)
Learnt from: fmorency
PR: liftedinit/manifest-app#257
File: components/react/authSignerModal.tsx:313-313
Timestamp: 2025-02-12T14:01:23.163Z
Learning: The `SignModal` component in `components/react/authSignerModal.tsx` should use native `<img>` tags instead of `next/image` to maintain framework agnosticism and reusability across different React applications.
🪛 Biome (1.9.4)
components/react/authSignerModal.tsx
[error] 303-303: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
components/react/authSignerModal.tsx (1)
319-323: Maintain framework agnosticism with native<img>tag.Based on the retrieved learnings, the native
<img>tag is preferred here to maintain framework agnosticism and reusability across different React applications.
| function handleReject() { | ||
| reject && reject(); | ||
| onClose(); | ||
| } | ||
| function handleApprove() { | ||
| approve && approve(); | ||
| onClose(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional chaining for handler functions.
The handler functions should use optional chaining as suggested by static analysis:
Apply this diff:
function handleReject() {
- reject && reject();
+ reject?.();
- onClose();
+ onClose?.();
}
function handleApprove() {
- approve && approve();
+ approve?.();
- onClose();
+ onClose?.();
}📝 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.
| function handleReject() { | |
| reject && reject(); | |
| onClose(); | |
| } | |
| function handleApprove() { | |
| approve && approve(); | |
| onClose(); | |
| } | |
| function handleReject() { | |
| reject?.(); | |
| onClose?.(); | |
| } | |
| function handleApprove() { | |
| approve?.(); | |
| onClose?.(); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 303-303: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
This can use any human readable string for defining the voting period. By default it will be 30m (same as before this PR). Fixed liftedinit#268
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pages/_app.tsx (1)
36-38: Consider adding ARIA landmarks for better accessibility.The main content area could benefit from additional ARIA roles and landmarks.
- <main className="p-6 relative z-10"> + <main + className="p-6 relative z-10" + role="main" + aria-label="Main content" + >components/groups/modals/voteDetailsModal.tsx (1)
320-322: Remove unused function.The
executeVotefunction is empty and appears to be unused. Consider removing it to maintain code cleanliness.- const executeVote = async () => { - setIsSigning(true); - };components/groups/modals/voteModal.tsx (3)
24-24: Remove unnecessary null check for required prop.The
proposalprop is marked as required in the type definition, but there's a null check in the component. This is inconsistent and suggests the prop should be optional or the check should be removed.- proposal: ProposalSDKType; + proposal?: ProposalSDKType;Or:
- if (!proposal) return null;Also applies to: 70-70
68-68: Consider inlining the closeModal function.The
closeModalfunction is a simple one-liner that's only used in two places. Consider inlining it to reduce code verbosity.- const closeModal = () => onClose();And update the usages to directly call
onClose.
86-104: Improve loading state UI to maintain layout consistency.The current implementation switches between loading dots and buttons, which can cause layout shifts. Consider keeping the button layout and showing loading state within each button.
- {isSigning ? ( - <div className="loading loading-dots loading-sm" /> - ) : ( - <> <button onClick={() => handleVote(1)} className="btn btn-success"> - Yes + {isSigning ? ( + <div className="loading loading-dots loading-sm" /> + ) : ( + 'Yes' + )} </button> <button onClick={() => handleVote(3)} className="btn btn-error"> - No + {isSigning ? ( + <div className="loading loading-dots loading-sm" /> + ) : ( + 'No' + )} </button> <button onClick={() => handleVote(4)} className="btn btn-warning"> - No With Veto + {isSigning ? ( + <div className="loading loading-dots loading-sm" /> + ) : ( + 'No With Veto' + )} </button> <button onClick={() => handleVote(2)} className="btn btn-info"> - Abstain + {isSigning ? ( + <div className="loading loading-dots loading-sm" /> + ) : ( + 'Abstain' + )} </button> - </> - )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.env.test(1 hunks)components/groups/forms/groups/GroupPolicyForm.tsx(2 hunks)components/groups/modals/updateGroupModal.tsx(5 hunks)components/groups/modals/voteDetailsModal.tsx(14 hunks)components/groups/modals/voteModal.tsx(2 hunks)config/env.ts(2 hunks)package.json(1 hunks)pages/_app.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.test
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (1)
components/groups/forms/groups/GroupPolicyForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#161
File: components/groups/forms/groups/GroupPolicyForm.tsx:213-215
Timestamp: 2025-01-07T01:21:52.501Z
Learning: In the GroupPolicyForm component, the voting period validation only needs to handle the minimum 30-minute period error at the object level, which produces a string error message. Individual field validations for days, hours, minutes, and seconds don't produce error messages.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
pages/_app.tsx (3)
9-10: LGTM! Clean import organization.The consolidation of Web3Auth and wallet management into
ManifestAppProvidersimproves code organization and separation of concerns.
20-20: LGTM! Improved state management.Using
useLocalStoragehook for drawer visibility is a good improvement as it:
- Simplifies state management
- Persists user preference across sessions
23-41: LGTM! Clean component structure with proper responsiveness.The layout structure is well-organized with:
- Proper mobile-first approach using Tailwind
- Smooth transitions for drawer toggle
- Correct z-index management
components/groups/modals/voteDetailsModal.tsx (3)
61-93: Well-structured message field mapping!The
importantFieldsmapping provides a comprehensive and maintainable way to handle different message types, with clear field specifications for each type.
112-112: Good use of type-safe status handling!Using
proposalStatusFromJSONimproves type safety and reduces potential runtime errors.
477-764: Excellent dialog implementation with proper accessibility!The refactored dialog implementation:
- Uses Headless UI's Dialog component correctly
- Includes proper ARIA attributes
- Supports modal stacking with SignModal and MessagesModal
- Maintains consistent styling with the design system
This aligns perfectly with the PR's objective to standardize dialog usage.
components/groups/modals/voteModal.tsx (2)
8-10: LGTM! Imports align with refactor goals.The new imports support the transition to Headless UI's Dialog component and the integration with SignModal, as outlined in the PR objectives.
28-28: LGTM! Improved signing state management.The addition of signing callbacks and the use of
isSigningfromuseTxhook provides better control and feedback during the signing process.Also applies to: 35-36, 54-55, 63-64
config/env.ts (2)
3-10: LGTM! Clean and focused implementation.The
parseDurationfunction is well-implemented with proper null handling and clear conversion to seconds.
50-54:❓ Verification inconclusive
Verify the minimum voting period configuration.
The minimum voting period is configurable via environment variable with a sensible default of 30 minutes.
Let's verify the environment variable usage:
🏁 Script executed:
#!/bin/bash # Description: Check if NEXT_PUBLIC_MINIMUM_VOTING_PERIOD is set in environment files. # Look for environment variable in common environment file locations fd -t f "^\.env" --exec grep -l "NEXT_PUBLIC_MINIMUM_VOTING_PERIOD"Length of output: 275
Verify that the environment variable is sourced from the intended hidden configuration files.
The current configuration uses a default of 30 minutes (1800 seconds), which is sensible. However, our initial automated search didn’t capture any files because it did not include hidden files (i.e. files starting with a dot). Please ensure that the environment variable
NEXT_PUBLIC_MINIMUM_VOTING_PERIODis indeed defined in your hidden.env*files. You might verify this manually or use an enhanced search command like:fd -t f -H "^\.env" --exec grep -l "NEXT_PUBLIC_MINIMUM_VOTING_PERIOD"If the variable is set correctly in your environment configuration, no further changes to the code are needed.
components/groups/forms/groups/GroupPolicyForm.tsx (2)
1-1: LGTM! Clean imports.The imports are properly organized and necessary for the updated validation logic.
Also applies to: 8-8
19-28: LGTM! Validation logic aligns with requirements.The validation schema correctly uses the configurable minimum voting period and provides a human-readable error message, aligning with the retrieved learning about object-level validation.
components/groups/modals/updateGroupModal.tsx (4)
317-330: LGTM! Clean Dialog component integration.The Headless UI Dialog component is properly integrated with backdrop and event handling.
265-280: LGTM! Validation logic handles edge cases.The validation schema correctly:
- Uses configurable minimum voting period
- Skips validation when all values are zero (no update)
- Provides human-readable error message
213-223: LGTM! Transaction handling improved.The tx call now includes an identifier that matches the SignModal, improving traceability.
435-435: LGTM! SignModal properly integrated.The SignModal is correctly integrated with a matching identifier.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
components/groups/modals/voteModal.tsx (2)
8-10: Remove unused import.The
SignModalcomponent is imported but never used in this file.import { Dialog } from '@headlessui/react'; -import { SignModal } from '@/components/react'; import { ProposalSDKType } from '@liftedinit/manifestjs/dist/codegen/cosmos/group/v1/types';
28-55: Enhance error handling and add type safety for vote options.Consider the following improvements:
- Provide more specific error messages based on the error type.
- Use an enum for vote options to improve type safety and maintainability.
enum VoteOption { YES = 1, ABSTAIN = 2, NO = 3, NO_WITH_VETO = 4 }Example usage:
-const handleVote = async (option: number) => { +const handleVote = async (option: VoteOption) => { // ... rest of the code }components/groups/modals/voteDetailsModal.tsx (2)
268-274: Consider using a single refetch function.The
refetchfunction calls multiple individual refetch functions. Consider combining these into a single function in a higher-level component to reduce the number of network requests and improve performance.-function refetch() { - refetchVotes(); - refetchTally(); - refetchProposals(); - refetchGroupInfo(); - refetchDenoms(); -} +// Move this to a higher-level component +async function refetchAll() { + await Promise.all([ + refetchVotes(), + refetchTally(), + refetchProposals(), + refetchGroupInfo(), + refetchDenoms() + ]); +}
776-783: Consider adding loading state to VotingPopup.The
VotingPopupcomponent should reflect the loading state during vote submission to prevent multiple submissions.<VotingPopup open={showVotingPopup} + isLoading={isSigning} onClose={vote => { if (vote) { handleVote(vote); } setShowVotingPopup(false); }} proposal={proposal} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/groups/modals/voteDetailsModal.tsx(15 hunks)components/groups/modals/voteModal.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
components/groups/modals/voteModal.tsx (2)
12-23: Well-structured prop types and hook usage!The refactoring to use the open/onClose pattern with the Dialog component improves state management and follows React best practices. The TypeScript types are well-defined and the useTx hook integration is clean.
64-106: Great modal implementation with accessibility in mind!The Dialog component implementation:
- Uses proper ARIA attributes
- Handles loading states effectively
- Provides semantic color classes for vote options
- Supports keyboard navigation and click-outside behavior
This aligns well with the PR objectives to standardize dialog implementations.
components/groups/modals/voteDetailsModal.tsx (2)
61-93: LGTM! Well-structured message type mapping.The
importantFieldsconstant provides a comprehensive mapping of message types to their essential fields, improving code maintainability and readability.
507-789: LGTM! Well-implemented Dialog component integration.The migration to Headless UI's Dialog component is well-executed with proper accessibility attributes and state management. The component hierarchy is clean and the modal stack is properly managed.
|
@fmorency PTAL.
Fixed
Didn't get that error but didn't get a signing modal either. Fixed that.
Should be fixed now.
Re-did the flow here; now the popup closes and the signing happens outside of it. It fixes a few bugs I had with that dialog. The right way to fix would be in general have a Flow-like state management, but that's a refactor I'm not willing to do. This is fine. Fixed.
This might be fixed by my last commit which calls refetch more often on voting. Maybe not. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
components/groups/modals/voteDetailsModal.tsx (1)
503-784: 💡 Verification agent🧩 Analysis chain
Verify accessibility attributes in Dialog component.
The Dialog implementation should be verified for proper accessibility support.
🌐 Web query:
What are the required ARIA attributes for HeadlessUI Dialog component?💡 Result:
The Headless UI Dialog component requires specific ARIA attributes to ensure accessibility compliance while leveraging built-in functionality:
Core Required Attributes
role="dialog"
Defines the element as a dialog container[4][6]. For alert-style dialogs requiring immediate attention, userole="alertdialog"instead[6].
aria-labelledby
Must reference an element containing the dialog title (typically via<Dialog.Title>)[3][4].
Example implementation:<Dialog open={isOpen} onClose={close}> <Dialog.Title id="title">Payment Confirmation</Dialog.Title> <Dialog.Panel> {/* Content */} </Dialog.Panel> </Dialog>
aria-describedby
Should reference descriptive content (typically via<Dialog.Description>)[3][4]:<Dialog.Description id="desc"> Your payment of $29.99 was processed successfully. </Dialog.Description>Automatically Managed Attributes
Headless UI handles these internally when using its component structure:
aria-modal="true"(sets modal context)[7]aria-hiddenon background elements[5][7]- Focus management with
tabIndexattributes[4]aria-expandedstate for trigger buttons[1]Implementation Example
import { Dialog } from '@headlessui/react' function PaymentDialog() { return ( <Dialog open={isOpen} onClose={close} role="dialog" className="fixed inset-0 z-10" > <div className="flex items-center justify-center min-h-screen"> <Dialog.Panel className="bg-white p-6 rounded-lg" aria-labelledby="payment-title" aria-describedby="payment-desc" > <Dialog.Title id="payment-title" className="text-xl font-bold"> Transaction Complete </Dialog.Title> <Dialog.Description id="payment-desc" className="mt-2"> Funds transferred successfully to account ending in 4567. </Dialog.Description> {/* Interactive elements */} </Dialog.Panel> </div> </Dialog> ) }Accessibility Features
Headless UI automatically provides:
- Focus trapping within dialog[2][4]
- Scroll lock on background content[2][4]
- Escape key closing functionality[4]
- Screen reader announcements via live regions[1][7]
For full accessibility compliance, combine these ARIA patterns with semantic HTML structure and proper keyboard navigation handling as shown in code examples[3][9]. The component handles complex accessibility requirements while allowing complete styling control through Tailwind CSS or custom CSS[8][10].
Citations:
- 1: https://app.studyraid.com/en/read/11966/381798/aria-roles-and-attributes-for-menus
- 2: Reason for no ability to control scroll lock on dialog tailwindlabs/headlessui#510
- 3: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role
- 4: https://headlessui.com/v1/react/dialog
- 5: Nested Dialog adds inert prop to every sibling of #headless-ui-portal-root tailwindlabs/headlessui#2413
- 6: Dialog role="alertdialog" attribute tailwindlabs/headlessui#617
- 7: https://a11y-dialog.netlify.app/further-reading/implementations/
- 8: https://forum.rescript-lang.org/t/help-with-headlessui-bindings/2771
- 9: https://olamijibadmos.hashnode.dev/how-to-create-modals-in-react-with-headlessuireact
- 10: https://headlessui.com
Enhance Accessibility for Modal Dialog
After verifying the Headless UI Dialog accessibility guidelines, please ensure that the modal incomponents/groups/modals/voteDetailsModal.tsxexplicitly provides accessible labeling. While Headless UI automatically sets attributes such asrole="dialog"andaria-modal="true", it is recommended to:
- Wrap the modal's title in a
<Dialog.Title>component and have the dialog container reference it viaaria-labelledby.- Consider adding a
<Dialog.Description>for any additional explanatory text and reference it witharia-describedby.For instance, if the proposal title is meant to be the dialog’s title, replace or wrap the existing
<h1>as follows:- <h1 className="text-2xl font-bold max-w-[20ch] truncate text-primary-content"> - {proposal?.title} - </h1> + <Dialog.Title id="dialog-title" className="text-2xl font-bold max-w-[20ch] truncate text-primary-content"> + {proposal?.title} + </Dialog.Title>Then, update the
<Dialog.Panel>to include the proper ARIA attribute if it isn’t managed automatically:<Dialog.Panel aria-labelledby="dialog-title" ...> {/* Modal content */} </Dialog.Panel>Implementing these changes will help ensure that screen readers correctly announce the modal dialog and its content.
🧹 Nitpick comments (4)
components/groups/modals/voteModal.tsx (3)
37-37: Avoid magic numbers in z-index.Consider defining z-index values in a configuration file or using Tailwind's built-in z-index scale instead of magic numbers like
z-[9999].- className="modal modal-open fixed flex p-0 m-0 top-0 right-0 z-[9999]" + className="modal modal-open fixed flex p-0 m-0 top-0 right-0 z-50"
46-48: Enhance loading state accessibility.Add appropriate ARIA attributes to the loading indicator to improve accessibility.
- <div className="loading loading-dots loading-sm" /> + <div + className="loading loading-dots loading-sm" + role="status" + aria-label="Signing transaction..." + />
45-64: Consider using semantic HTML form for vote options.Enhance accessibility and form handling by using a proper form structure with radio buttons.
- <div className="grid w-full grid-cols-2 gap-4"> + <form + className="grid w-full grid-cols-2 gap-4" + onSubmit={(e) => { + e.preventDefault(); + const formData = new FormData(e.currentTarget); + closeModal(Number(formData.get('vote'))); + }} + > {isSigning ? ( <div className="loading loading-dots loading-sm" role="status" aria-label="Signing transaction..." /> ) : ( <> - <button onClick={() => closeModal(1)} className="btn btn-success"> - Yes - </button> - <button onClick={() => closeModal(3)} className="btn btn-error"> - No - </button> - <button onClick={() => closeModal(4)} className="btn btn-warning"> - No With Veto - </button> - <button onClick={() => closeModal(2)} className="btn btn-info"> - Abstain - </button> + {[ + { value: 1, label: 'Yes', className: 'btn-success' }, + { value: 3, label: 'No', className: 'btn-error' }, + { value: 4, label: 'No With Veto', className: 'btn-warning' }, + { value: 2, label: 'Abstain', className: 'btn-info' } + ].map(({ value, label, className }) => ( + <label key={value} className="flex items-center"> + <input + type="radio" + name="vote" + value={value} + className="hidden" + /> + <span className={`btn w-full ${className}`}> + {label} + </span> + </label> + ))} + <button type="submit" className="hidden" /> </> )} - </div> + </form>components/groups/modals/voteDetailsModal.tsx (1)
61-93: Consider moving field mappings to a separate configuration file.The
importantFieldsmapping is quite extensive and could grow further. Consider moving it to a dedicated configuration file to improve maintainability and reusability.Create a new file
config/fieldMappings.ts:export const importantFields: { [key: string]: string[] } = { '/cosmos.bank.v1beta1.MsgSend': ['from_address', 'to_address', 'amount'], // ... rest of the mappings }; export const defaultFields = ['@type'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/groups/modals/voteDetailsModal.tsx(15 hunks)components/groups/modals/voteModal.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
components/groups/modals/voteModal.tsx (1)
12-31: Well-structured component interface!The component follows React best practices with:
- Clear prop types using TypeScript
- Controlled component pattern with open/onClose
- Good defensive programming with proposal null check
components/groups/modals/voteDetailsModal.tsx (2)
276-303: Add error message toast for failed votes.The error handling in
handleVoteonly logs to console. Consider adding user feedback for failed transactions.
461-489: LGTM! Button state logic is well-structured.The
getButtonStatefunction effectively handles all possible proposal states and user interactions. The use of descriptive variable names and clear conditions makes the code easy to understand.
This PR took a turn and now I have fixed all the dialogs that I could find. This removes a lot of code and
useEffectin favor of simpler state management and dialog.This makes it more streamline when we have a stack of dialogs. Showing multiple dialogs at once and pressing Escape will close the latest opened one. Same with closing dialogs using buttons or actions. This change should have no visible change (outside of going up the stack once when pressing Escape or clicking outside the modal).
The pattern is more or less the same through the app: get a current dialog, either move it to its own component or refactor it into a headlessui
<Dialog>, and reduce the usage of styles and backdrop to simpler classes.Also, moved the
SignModalto aWeb3Authcontext and provider, and create a new component where anyone who needs touseTxalso include the<SignModal>in the right DOM location to show it in their stack.Without actual e2e tests this is hard to test. I fixed the unit tests we already had. I did manually test all dialogs as I went.
If you find a bug that relates to dialogs or UX flow related to signing, let me know.
Summary by CodeRabbit
SignModalcomponents across various modals for signing transactions.