[ISSUE-252] refactor(cashout): use ValidatedInput for account number #484
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Cashout/Components/Initial.view.tsx (1)
303-326: Consider UX improvements for the ValidatedInput implementation.
- Extract the debounce time as a constant for better maintainability:
+const ACCOUNT_VALIDATION_DEBOUNCE_MS = 750; <ValidatedInput placeholder="IBAN / US account number" label="To" value={accountNumber} - debounceTime={750} + debounceTime={ACCOUNT_VALIDATION_DEBOUNCE_MS}
- Consider showing a loading indicator during validation to provide better user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Cashout/Components/Initial.view.tsx (8 hunks)
🔇 Additional comments (4)
src/components/Cashout/Components/Initial.view.tsx (4)
5-5: LGTM: Clean state management for account validation.The new imports and state variables are well-structured and appropriately support the ValidatedInput implementation for account number validation.
Also applies to: 17-17, 64-66
87-101: LGTM: Comprehensive disable conditions for the proceed button.The disable conditions properly account for validation states, ensuring users can only proceed with valid input.
Line range hint
1-377: Overall implementation looks solid with good attention to UX.The ValidatedInput integration for account number validation is well-implemented with proper:
- State management
- Error handling
- Validation feedback
- Integration with existing bank account selection
The changes align well with the PR objective of enhancing the cashout user experience.
309-325: Verify error handling for all possible bank account formats.The error handling looks good, but we should verify that the validation covers all supported bank account formats.
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setSelectedBankAccount(undefined) | ||
| setAccountNumber('') |
There was a problem hiding this comment.
thought (non-blocking): imo it's better to have this typed as <string | undefined> bc we can run ifs like if(accountNumber) more cleanly. An empty string is falsy, but it just feels cleaner to me.
There was a problem hiding this comment.
In the end inputs need a defined value I prefer to have the state of bankAccountNumber and the value on the input to always be the same (as opposed to the state being undefined and the input be empty string)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Cashout/Components/Initial.view.tsx (3)
64-66: Consider using more descriptive state variable names.The state variables could be more descriptive to better reflect their purpose:
bankAccountNumber→bankAccountIdentifierisValidBankAccountNumber→isBankAccountValidisValidatingBankAccountNumber→isBankAccountValidating
313-324: Enhance error message specificity.The current error message "Invalid bank account. Please make sure your account is supported" could be more specific about what makes an account supported (e.g., IBAN format for EU accounts or routing/account number format for US accounts).
setErrorState({ showError: true, errorMessage: - 'Invalid bank account. Please make sure your account is supported', + 'Invalid bank account format. Please enter a valid IBAN for EU accounts or routing/account number for US accounts', })
Line range hint
283-293: Add aria-label for accessibility.The clear button (✕) should have an aria-label for better accessibility.
<button className="text-lg text-black" + aria-label="Clear bank account selection" onClick={(e) => { e.stopPropagation() setBankAccountNumber('') }} > ✕ </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Cashout/Components/Initial.view.tsx (8 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Cashout/Components/Initial.view.tsx (1)
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#484 File: src/components/Cashout/Components/Initial.view.tsx:273-274 Timestamp: 2024-10-25T11:33:46.776Z Learning: In the `InitialCashoutView` component (`src/components/Cashout/Components/Initial.view.tsx`), linked bank accounts should not generate error states, and the `ValidatedInput` component will clear any error messages if needed. Therefore, it's unnecessary to manually clear the error state when selecting or clearing linked bank accounts.
🔇 Additional comments (2)
src/components/Cashout/Components/Initial.view.tsx (2)
303-326: Implementation looks solid!The ValidatedInput integration successfully improves the bank account validation UX with:
- Proper debouncing of validation
- Clear error messaging
- Consistent state management
- Smooth integration with existing linked accounts functionality
309-325: Verify validation state management.There's a potential race condition in the validation state management. If multiple validation callbacks are triggered in quick succession (despite debouncing), you might get into an inconsistent state. Consider using a ref to track the latest validation request.
Consider implementing a validation queue or using a ref to track the latest validation request:
const latestValidationId = useRef(0); // In onUpdate: const currentId = ++latestValidationId.current; // ... perform validation ... if (currentId === latestValidationId.current) { // Update state only if this is still the latest validation setIsValidBankAccountNumber(isValid); }
| xchainAllowed, | ||
| isBelowMinLimit, | ||
| isExceedingMaxLimit, | ||
| ]) |
There was a problem hiding this comment.
nit: booleans like these variables usually don't need useMemo, you're not getting any performance improvement. Also it includes potential bugs if you're missing a variable in the dependency array the the compute is broken
Added the ValidatedInput component to check if the account number is valid and show feedback to the user before they try to proceed