fix: preserve wallet settings on accordion expand/collapse#4933
fix: preserve wallet settings on accordion expand/collapse#4933itsaryanchauhan wants to merge 3 commits into
Conversation
- Change pmtName to item-specific values to prevent unmounting. - Remove onItemCollapseClick handlers to prevent settings reset/disabling on collapse.
Changed Files
|
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Classification: Bugfix — Wallet Settings Persistence on Accordion State Change
Risk Level: Medium (state management change affecting V1 and V2 payment method flows)
Files Changed: 2 files (connector payment method configuration UI)
Verification Against Control Center Checklist
Tier 1 — High Signal:
✅ Naming Consistency — Function rename from handleBankDebitCheckboxClick to handleCheckboxClick is clearer and matches generalized usage
✅ No feature flag gating needed — This is a bugfix that corrects unintended behavior
✅ Blast radius considered — Changes affect both V1 (PaymentMethod.res) and V2 (OtherPaymentMethod.res) flows consistently
Findings
✅ Core fix is sound
The PR addresses the root cause correctly:
- Problem: Using shared
selectedWalletstate forpmtNamecaused component remounting when accordion state changed - Solution: Using loop-local
pmtData.payment_method_subtype/value.payment_method_typepreserves component identity
✅ Removing collapse callback is correct
The removal of onItemCollapseClick handlers that called resetValues() / removeSelectedWallet() aligns with the intended UX:
- Collapsing = visual toggle only
- Cancel button = explicit reset
- Checkbox = explicit enable/disable
Suggestions
💡 Consider backward compatibility edge case
If a wallet was previously configured and the user opens it again, verify that:
- The form pre-populates with existing values correctly
- The checkbox state reflects the "already enabled" status
The PR description confirms manual testing covered this scenario.
💡 Test coverage recommendation
Since this fixes a regression-prone area (accordion state + form persistence), consider adding a Playwright/Cypress test that:
- Opens wallet A, enters config, clicks Proceed
- Opens wallet B
- Asserts wallet A's form values persist
Positive Notes
- Clean, minimal fix that addresses the exact bug described
- Both V1 and V2 flows updated consistently
- Handler naming improved (
handleCheckboxClickvshandleBankDebitCheckboxClick) - Manual testing documented thoroughly in PR description
Verdict: ✅ Approve — Clean bugfix with correct approach. The state management change from shared selectedWallet to loop-local values correctly prevents unwanted component remounting while preserving React Final Form state.
Review SummaryClassification: Bugfix — Wallet Form State Persistence OverviewThis PR fixes a legitimate bug where wallet configuration form values were being reset when expanding/collapsing accordions. The fix changes from shared parent state ( Findings
Inline FindingsFile:
|
|
Update: Fixed Playwright test failures My change keeps the mounted in the DOM when collapsed (which is necessary to prevent resetting the React Final Form state). This caused Playwright's broad .getByText() selectors to break in two ways:
|
…le Pay and Google Pay
5d709c3 to
8b7c42a
Compare
Untitled.2.mov
Type of Change
Description
This PR fixes a bug where entering configurations for a wallet (e.g., Google Pay), clicking "Proceed" to collapse/save, and then expanding another wallet accordion (e.g., Apple Pay) would reset/clear the entered credentials for the first wallet.
Changes:
OtherPaymentMethod.res(V2) andPaymentMethod.res(V1) to pass the loop-item-specific value (pmtData.payment_method_subtype/value.payment_method_type) to thepmtNameprop of<AdditionalDetailsSidebar>instead of using the shared parent component state (selectedWallet.payment_method_subtype/selectedWallet.payment_method_type).onItemCollapseClickcallback fromAccordionAdapteritems in both files. Collapsing the accordion programmatically (upon clicking Proceed) or manually should not trigger a reset of form values or disable the payment method (relying instead on the Cancel button to reset, and the Checkbox to disable).PaymentMethod.restohandleCheckboxClick(removing theBankDebitcheck) so that wallets can be explicitly checked/unchecked directly. This prevents the previous issue where expanding a new wallet undersingleOpen=truewould disable the previously open wallet.Motivation and Context
Fixes #4212.
When configuring multiple wallets (like Google Pay and Apple Pay), the user should be able to configure one, close it, and proceed to the next without having their previously entered configuration details wiped out by shared state changes or collapse callbacks.
How did you test it?
Tested manually on local development server against sandbox:
Where to test it?
Backend Dependency
Backend PR URL: N/A
Feature Flag
Feature flag name(s): N/A
Checklist
npm run re:build