chalabi/modal fixes #87
Conversation
WalkthroughThe pull request introduces several changes across multiple components, primarily focusing on the management of modal dialogs and the formatting of transaction amounts. A new function, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 58.85% 59.41% +0.56%
==========================================
Files 145 145
Lines 14040 14040
==========================================
+ Hits 8263 8342 +79
+ Misses 5777 5698 -79 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
components/factory/modals/denomInfo.tsx (4)
16-18: Consider accessibility improvementsWhile the modal has basic ARIA attributes, consider adding:
role="dialog"for better screen reader support- Focus trap for keyboard navigation
- Auto-focus on the close button
Line range hint
48-56: Improve error handling for BASE value decodingThe current implementation silently falls back to the encoded value on error. Consider:
- Adding user-friendly error messaging
- Logging the specific decoding error details
try { return decodeURIComponent(denom?.base); } catch (e) { - console.error('Failed to decode BASE value:', e); - return denom.base; + const error = e instanceof Error ? e.message : 'Unknown error'; + console.error('Failed to decode BASE value:', error); + return `Unable to decode: ${denom.base}`; }
63-66: Consider consolidating modal close handlersThe component has two
formelements withmethod="dialog". Consider consolidating these into a single close handler for better maintainability.- <form method="dialog" className="modal-backdrop"> - <button>close</button> - </form> + <div className="modal-backdrop" onClick={() => { + document.getElementById(modalId)?.close(); + }}> + </div>
Line range hint
89-93: Update block explorer URL handlingPer Issue #31, the transaction links should point to the appropriate block explorer based on the selected endpoint. Currently, it's hardcoded to use
NEXT_PUBLIC_TESTNET_EXPLORER_URL.Consider:
- Accepting the explorer URL as a prop
- Using a context provider for network configuration
- Implementing a service to manage network-specific URLs
components/bank/components/tokenList.tsx (1)
111-116: Consider memoizing the selected denomination metadataThe
findoperation onfilteredBalancesis performed on every render. For better performance, consider memoizing this value.+ const selectedDenomMetadata = useMemo( + () => filteredBalances.find(b => b.denom === selectedDenom)?.metadata ?? null, + [filteredBalances, selectedDenom] + ); {selectedDenom && ( <DenomInfoModal - denom={filteredBalances.find(b => b.denom === selectedDenom)?.metadata ?? null} + denom={selectedDenomMetadata} modalId="denom-info-modal" /> )}components/bank/components/historyBox.tsx (1)
Line range hint
1-276: Consider implementing a reusable modal management systemTo better address the modal-related issues (#28) and improve maintainability, consider:
- Creating a custom hook for modal management
- Implementing a consistent pattern for handling modals across the application
- Adding proper keyboard event handling (Esc key)
Example implementation:
// hooks/useModal.ts export function useModal(modalId: string) { const showModal = useCallback(() => { const modal = document.getElementById(modalId); if (modal instanceof HTMLDialogElement) { modal.showModal(); const handleEsc = (e: KeyboardEvent) => { if (e.key === 'Escape') { modal.close(); document.removeEventListener('keydown', handleEsc); } }; document.addEventListener('keydown', handleEsc); } }, [modalId]); const hideModal = useCallback(() => { const modal = document.getElementById(modalId); if (modal instanceof HTMLDialogElement) { modal.close(); } }, [modalId]); return { showModal, hideModal }; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: components/bank/components/historyBox.tsx#L33
Added line #L33 was not covered by tests
[warning] 35-35: components/bank/components/historyBox.tsx#L35
Added line #L35 was not covered by tests
[warning] 37-37: components/bank/components/historyBox.tsx#L37
Added line #L37 was not covered by tests
[warning] 39-39: components/bank/components/historyBox.tsx#L39
Added line #L39 was not covered by tests
[warning] 41-41: components/bank/components/historyBox.tsx#L41
Added line #L41 was not covered by testscomponents/groups/modals/memberManagementModal.tsx (2)
Line range hint
41-43: Consider making member weight configurableThe weight for new members is hardcoded to '1'. Consider making this configurable to provide more flexibility in member management.
const [members, setMembers] = useState<ExtendedMember[]>(() => initialMembers.map(member => ({ ...member, isNew: false, markedForDeletion: false, - weight: member.weight || '1', + weight: member.weight || defaultWeight, })) );Also applies to: 52-60
Line range hint
301-309: Add user feedback for copy actionThe copy button should provide visual feedback when the address is copied.
<button onClick={e => { e.preventDefault(); - navigator.clipboard.writeText(member.address); + navigator.clipboard.writeText(member.address) + .then(() => { + // Show toast or tooltip feedback + toast.success('Address copied to clipboard'); + }); }} className="btn btn-ghost hover:bg-transparent btn-sm ml-2" >components/bank/modals/txInfo.tsx (1)
54-54: Increase test coverage for the transaction amount displayThe rendering of transaction amounts starting at line 54 is not covered by tests. To ensure reliability and prevent future regressions, consider adding unit tests for this functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: components/bank/modals/txInfo.tsx#L54
Added line #L54 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/bank/components/historyBox.tsx(4 hunks)components/bank/components/tokenList.tsx(3 hunks)components/bank/modals/txInfo.tsx(5 hunks)components/factory/modals/denomInfo.tsx(4 hunks)components/groups/modals/memberManagementModal.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/bank/components/historyBox.tsx
[warning] 33-33: components/bank/components/historyBox.tsx#L33
Added line #L33 was not covered by tests
[warning] 35-35: components/bank/components/historyBox.tsx#L35
Added line #L35 was not covered by tests
[warning] 37-37: components/bank/components/historyBox.tsx#L37
Added line #L37 was not covered by tests
[warning] 39-39: components/bank/components/historyBox.tsx#L39
Added line #L39 was not covered by tests
[warning] 41-41: components/bank/components/historyBox.tsx#L41
Added line #L41 was not covered by tests
[warning] 200-203: components/bank/components/historyBox.tsx#L200-L203
Added lines #L200 - L203 were not covered by tests
components/bank/components/tokenList.tsx
[warning] 58-61: components/bank/components/tokenList.tsx#L58-L61
Added lines #L58 - L61 were not covered by tests
components/bank/modals/txInfo.tsx
[warning] 54-54: components/bank/modals/txInfo.tsx#L54
Added line #L54 was not covered by tests
🔇 Additional comments (9)
components/factory/modals/denomInfo.tsx (2)
11-14: LGTM: Improved null safety with optional chaining
The addition of optional chaining in denom?.name?.startsWith() provides better null safety compared to the previous implementation.
9-9: Verify Esc key functionality for modal closure
The PR aims to make modals closable with Esc key (Issue #28), but the native dialog element might need additional configuration to ensure this behavior works consistently across browsers.
Consider implementing a shared modal wrapper component that handles Esc key functionality consistently across all modals.
✅ Verification successful
Native dialog element handles Esc key automatically
The native HTML <dialog> element used in the modal implementation automatically handles the Esc key for closing modals without requiring additional JavaScript event handlers. This is a built-in accessibility feature of the <dialog> element across modern browsers.
- The modal uses
<dialog>element with propermethod="dialog"forms for both content and backdrop - The backdrop form ensures clicking outside the modal also closes it
- No additional keyboard event handlers are needed as Esc key handling is provided natively
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any ESC key handling implementations
rg -i "escape|esc" --type ts --type tsx
Length of output: 69
Script:
#!/bin/bash
# Search for any ESC key handling implementations with correct file type
rg -i "escape|esc" --type typescript
# Also check for dialog element usage and event handlers
rg -i "dialog.*onclose|onclose.*dialog" --type typescript
# Look for modal-related components and their implementations
fd -e ts -e tsx | xargs rg -l "modal"
Length of output: 2472
Script:
#!/bin/bash
# Let's check the dialog element implementation in the codebase
rg -l "<dialog" .
# Check for any keyboard event handlers in modal components
rg -i "keydown|keyup|keypress" components/factory/modals/
# Look for any shared modal components or hooks
fd -e ts -e tsx | grep -i "modal" | xargs cat
Length of output: 132036
Script:
#!/bin/bash
# Search for dialog element and its keyboard event handlers
rg -A 5 -B 5 "<dialog.*onKeyDown|onKeyPress|onKeyUp" .
# Look for any keyboard event handlers in the codebase
rg -i "keydown.*esc|escape.*keydown" .
# Check for any shared modal components or hooks that might handle keyboard events
fd -e ts -e tsx | grep -i "modal" | xargs rg -l "keydown|keyup|keypress"
Length of output: 2097
components/bank/components/tokenList.tsx (1)
58-63: 🛠️ Refactor suggestion
Consider refactoring modal management for better maintainability and testability
The current implementation has several concerns:
- Direct DOM manipulation via
document.getElementByIdis not idiomatic React - Modal opening logic is duplicated (here and in button click handler)
- Missing test coverage for this critical UI interaction
- No explicit handling of Esc key (from PR objectives)
Consider refactoring to use React state for modal management:
- onClick={() => {
- setSelectedDenom(balance?.denom);
- (
- document?.getElementById(`denom-info-modal`) as HTMLDialogElement
- )?.showModal();
- }}
+ const [isModalOpen, setIsModalOpen] = useState(false);
+
+ const handleModalOpen = (denom: string) => {
+ setSelectedDenom(denom);
+ setIsModalOpen(true);
+ };
+
+ onClick={() => handleModalOpen(balance.denom)}Let's verify the modal implementation across the codebase:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-61: components/bank/components/tokenList.tsx#L58-L61
Added lines #L58 - L61 were not covered by tests
components/bank/components/historyBox.tsx (1)
259-261: LGTM: Transaction amount formatting looks good
The implementation correctly:
- Handles decimal places using metadata
- Formats large numbers for better readability
- Maintains proper denomination display
components/groups/modals/memberManagementModal.tsx (1)
226-226: Consider improving modal interaction pattern
While the HTML5 <dialog> element provides native Esc key support (addressing Issue #28), the interaction between the main modal and contacts modal needs attention. Currently, the main modal is closed when opening contacts, which could lead to state inconsistency if the user presses Esc.
Let's verify the modal interaction pattern:
Consider these alternatives:
- Stack the modals using z-index management instead of closing the main modal
- Use a modal manager to handle modal state transitions
Also applies to: 391-394
components/bank/modals/txInfo.tsx (4)
9-13: Update modal control flow using modalId
Replacing isOpen and onClose with modalId in the TxInfoModal component streamlines modal management. This change should enhance consistency across the application’s modals.
27-33: Ensure modal opens and closes correctly with the new structure
With the shift to using the <dialog> element and removing the onClick handler from the close button, please verify that the modal opens and closes as expected, including when using the Esc key and the close button.
42-47: Good use of optional chaining to enhance error handling
Adding optional chaining (?.) when accessing properties of tx and tx.data improves robustness by preventing runtime errors if these objects are undefined.
91-91: Verify correctness of URL construction in external links
Please confirm that the href attribute constructs the correct URL for both transactions and accounts. Double-check the conditional logic to ensure it accurately reflects the intended paths for different labels.
partially fixes #28 (pending token factory modal logic rework)
fixes #64
fixes #31
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
User Interface Improvements
Refactor