feat: enhance RecipientDetailsForm with NGN account validation#479
Open
sundayonah wants to merge 4 commits into
Open
feat: enhance RecipientDetailsForm with NGN account validation#479sundayonah wants to merge 4 commits into
sundayonah wants to merge 4 commits into
Conversation
- Added logic to cap account number input at 6 or 10 digits based on the selected institution for NGN currency. - Implemented validation for account number length, providing user feedback for incorrect input. - Updated input type to text for better handling of numeric input and added maxLength attribute for NGN accounts. - Improved state management for account identifier registration and manual entry handling.
Contributor
📝 WalkthroughWalkthroughCentralizes NGN account-digit rules and validation, switches account input from numeric to text with numeric inputMode and sanitization/truncation for NGN, adds NGN-specific maxLength, introduces an ChangesAccount Identifier Validation & Input Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/recipient/RecipientDetailsForm.tsx`:
- Around line 431-447: The validation message in accountIdentifierRegister uses
inconsistent capitalization ("account Number"); update the message in the
validate block of accountIdentifierRegister to use "account number" (lowercase
"number") and also make the matching change inside the related useEffect that
sets/uses the same error text so both places (the register validate logic and
the useEffect error handling) use the exact lowercase "account number" phrasing;
locate these by the symbols accountIdentifierRegister, register(...) and the
useEffect that references account number validation for selectedInstitution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 045ada1c-5324-40c8-b302-6031cfd85364
📒 Files selected for processing (1)
app/components/recipient/RecipientDetailsForm.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/recipient/RecipientDetailsForm.tsx (1)
302-333:⚠️ Potential issue | 🟠 MajorClear stale recipient names when the identifier becomes invalid or changes.
These early returns leave the previous
recipientNamein place, so the UI can keep showing a verified name for an empty/invalid account number. The same effect also accepts latefetchAccountNameresults from an older input and can write that stale name back after the user has already edited the field.Suggested fix
useEffect(() => { let timeoutId: NodeJS.Timeout; + let isStale = false; + const getRecipientName = async () => { if (!isManualEntry) return; const isNGN = currency === "NGN"; const digits = String(accountIdentifier ?? "").replace(/\D/g, ""); const requiredLen = selectedInstitution?.code === "SAFAKEPC" ? 6 : 10; if (!institution || !accountIdentifier) { + setIsFetchingRecipientName(false); + setValue("recipientName", ""); setRecipientNameError(""); return; } if (isNGN && digits.length !== requiredLen) { + setIsFetchingRecipientName(false); + setValue("recipientName", ""); if (digits.length > 0) { setRecipientNameError( requiredLen === 10 ? "Please enter a valid 10-digit account number." : "Invalid account number. Please enter a 6-digit account number.", @@ try { const accountName = await fetchAccountName({ institution: institution.toString(), accountIdentifier: accountIdentifier.toString(), }); + if (isStale) return; setValue("recipientName", accountName); setIsFetchingRecipientName(false); } catch (error) { + if (isStale) return; setRecipientNameError("No recipient account found."); setIsFetchingRecipientName(false); } }; @@ return () => { + isStale = true; clearTimeout(timeoutId); }; }, [Based on learnings, this component uses
recipientNameas the signal that verification has completed, so leaving an old value behind breaks that contract.Also applies to: 344-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/recipient/RecipientDetailsForm.tsx` around lines 302 - 333, Early returns leave a stale recipientName in the form and allow out-of-order fetchAccountName results to overwrite current input; clear the recipient name on any early return (call setValue("recipientName", "") where you currently return when !institution || !accountIdentifier and when isNGN digits length is invalid) and guard the async fetchAccountName result with a request token/sequence check (generate a local requestId before calling fetchAccountName and only call setValue("recipientName", accountName) if the requestId still matches) so late responses from previous inputs cannot write stale names; keep existing setRecipientNameError and setIsFetchingRecipientName handling around these changes.
🧹 Nitpick comments (1)
app/components/recipient/RecipientDetailsForm.tsx (1)
91-95: Finish centralizing the NGN account rules into one helper.The required length and invalid-length copy are still repeated across the memo, the verification effect, the field registration, and the inline
maxLengthprop. A small local helper likegetNgnAccountRules(selectedInstitution?.code)would keep the 6/10-digit rule and message aligned in one place.Also applies to: 431-447, 582-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/recipient/RecipientDetailsForm.tsx` around lines 91 - 95, Centralize the NGN account validation by extracting a helper function (e.g., getNgnAccountRules(selectedInstitutionCode)) that returns the max digit count and the associated messages/rules (required length and invalid-length copy), then replace the inline logic and literals currently in ngnAccountMaxDigits (useMemo), the account verification effect, the field registration rules, and the inline maxLength prop with values from that helper; ensure all locations reference getNgnAccountRules(selectedInstitution?.code) so the 6/10 digit rule and messages stay consistent across the component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/components/recipient/RecipientDetailsForm.tsx`:
- Around line 302-333: Early returns leave a stale recipientName in the form and
allow out-of-order fetchAccountName results to overwrite current input; clear
the recipient name on any early return (call setValue("recipientName", "") where
you currently return when !institution || !accountIdentifier and when isNGN
digits length is invalid) and guard the async fetchAccountName result with a
request token/sequence check (generate a local requestId before calling
fetchAccountName and only call setValue("recipientName", accountName) if the
requestId still matches) so late responses from previous inputs cannot write
stale names; keep existing setRecipientNameError and setIsFetchingRecipientName
handling around these changes.
---
Nitpick comments:
In `@app/components/recipient/RecipientDetailsForm.tsx`:
- Around line 91-95: Centralize the NGN account validation by extracting a
helper function (e.g., getNgnAccountRules(selectedInstitutionCode)) that returns
the max digit count and the associated messages/rules (required length and
invalid-length copy), then replace the inline logic and literals currently in
ngnAccountMaxDigits (useMemo), the account verification effect, the field
registration rules, and the inline maxLength prop with values from that helper;
ensure all locations reference getNgnAccountRules(selectedInstitution?.code) so
the 6/10 digit rule and messages stay consistent across the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57c44daa-daf4-4fee-8c75-052c25ce1e67
📒 Files selected for processing (1)
app/components/recipient/RecipientDetailsForm.tsx
chibie
requested changes
Apr 30, 2026
| validate: (value) => { | ||
| if (currency !== "NGN") return true; | ||
| const digits = String(value ?? "").replace(/\D/g, ""); | ||
| const requiredLen = selectedInstitution?.code === "SAFAKEPC" ? 6 : 10; |
Contributor
There was a problem hiding this comment.
why validating SAFAKEPC when currency is NGN?
Collaborator
Author
There was a problem hiding this comment.
not treating SAFAKEPC as a currency. currency === "NGN" gates NUBAN-style digit rules. selectedInstitution?.code === "SAFAKEPC" is the existing institution-level exception in this app
Comment on lines
-296
to
-304
| if ( | ||
| !institution || | ||
| !accountIdentifier || | ||
| accountIdentifier.toString().length < | ||
| (selectedInstitution?.code === "SAFAKEPC" ? 6 : 10) | ||
| ) { | ||
| if (!institution || !accountIdentifier) { | ||
| setRecipientNameError(""); | ||
| } |
Contributor
There was a problem hiding this comment.
why is the minimum length for SAFAKEPC removed here?
Collaborator
Author
There was a problem hiding this comment.
It wasn’t removed for NGN—it was moved out of the old combined guard that also ran for non-NGN.
Comment on lines
+92
to
+94
| const ngnAccountMaxDigits = useMemo(() => { | ||
| if (currency !== "NGN") return null; | ||
| return selectedInstitution?.code === "SAFAKEPC" ? 6 : 10; |
Contributor
There was a problem hiding this comment.
why check for SAFAKEPC when non-NGN is already returning null?
Collaborator
Author
There was a problem hiding this comment.
return null only means “no digit cap when currency isn’t NGN.”
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/recipient/RecipientDetailsForm.tsx (1)
584-590: ⚡ Quick winUse
ngnAccountMaxDigitsformaxLengthto avoid duplication.The
maxLengthattribute recalculates the same logic thatngnAccountMaxDigitsalready computes. This violates DRY and could lead to divergence if the rules change.♻️ Proposed fix
placeholder="Account number" - maxLength={ - currency === "NGN" - ? selectedInstitution?.code === "SAFAKEPC" - ? 6 - : 10 - : undefined - } + maxLength={ngnAccountMaxDigits ?? undefined} {...accountIdentifierRegister}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/recipient/RecipientDetailsForm.tsx` around lines 584 - 590, Replace the inline ternary expression used for the input's maxLength with the existing ngnAccountMaxDigits variable to avoid duplicating the NGN logic; locate the maxLength prop in RecipientDetailsForm (the JSX where currency === "NGN" ? ... is used) and set maxLength={currency === "NGN" ? ngnAccountMaxDigits : undefined} (or simply maxLength={currency === "NGN" && ngnAccountMaxDigits} depending on surrounding code) so the component reuses the computed ngnAccountMaxDigits value instead of recalculating the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/recipient/RecipientDetailsForm.tsx`:
- Around line 584-590: Replace the inline ternary expression used for the
input's maxLength with the existing ngnAccountMaxDigits variable to avoid
duplicating the NGN logic; locate the maxLength prop in RecipientDetailsForm
(the JSX where currency === "NGN" ? ... is used) and set maxLength={currency ===
"NGN" ? ngnAccountMaxDigits : undefined} (or simply maxLength={currency ===
"NGN" && ngnAccountMaxDigits} depending on surrounding code) so the component
reuses the computed ngnAccountMaxDigits value instead of recalculating the same
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 530a55e7-c536-410c-bb27-fadc97c351eb
📒 Files selected for processing (1)
app/components/recipient/RecipientDetailsForm.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
References
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit