Skip to content

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch#3710

Merged
rasbhat merged 3 commits intot/team404/sfp-on-pwafrom
rvishwanathbhat/stripe-caching-for-display-spm-fix-tests
Mar 3, 2026
Merged

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch#3710
rasbhat merged 3 commits intot/team404/sfp-on-pwafrom
rvishwanathbhat/stripe-caching-for-display-spm-fix-tests

Conversation

@rasbhat
Copy link

@rasbhat rasbhat commented Mar 3, 2026

Fix for Display SPMs at checkout due to caching issues.
Before fix:
SPMs displayed at checkout were stale
After fix:
Refetches fresh customer SPM data at checkout

Fixed failing tests for sf-payments-sheet

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@rasbhat rasbhat requested a review from a team as a code owner March 3, 2026 20:38
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Mar 3, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[customer, paymentConfig]
)

const [paymentStepReached, setPaymentStepReached] = useState(false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use refs. Is there a reason why state is the right choice this time?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed a re-render on change in step. Update to refs do not cause a re-render of component. So used State. Seems like refs are stable objects in React.

const updateCall = mockUpdatePaymentInstrument.mock.calls[0]
const requestBody = updateCall[0].body

expect(requestBody.paymentReferenceRequest.gatewayProperties).toBeUndefined()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this failing because gatewayProperties was there, and we actually just wanted setupFutureUsage to not be there? I ask because I think setupFutureUsage is the only property we ever send for Stripe. Is it that the SCAPI requires us to send gatewayProperties even if it's empty?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think with the setup_future_usage bug fix, I'd added the gatewayProperties for stripe even when it wasn't mandated by SCAPI for consistency. But removed and tested it. Including it even for guest and non-SPM probably would be overkill and not required. Updated.


useEffect(() => {
// Mount SFP only when all required data and DOM are ready; otherwise skip or wait for a later run.
if (!paymentStepReached) return // Only run after user has reached payment step
Copy link
Collaborator

@nayanavishwa nayanavishwa Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!paymentStepReached) return // Only run after user has reached payment step
if (step !== STEPS.PAYMENT) return // Only run after user has reached payment step

returns early with if (step !== STEPS.PAYMENT) return so SFP only runs on the payment step.

You might not need two useEffect.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason for directly not using step was that if we reach the payment step, but we change back to any other step like shipping on the checkout page, we wouldn't want a re-render of checkout again.

// Omit savedPaymentMethodsKey: init once with current SPM; re-initing when SPM list changes
// causes Stripe/Adyen to complain.
}, [
paymentStepReached,
Copy link
Collaborator

@nayanavishwa nayanavishwa Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
paymentStepReached,
step

and you might not need STEPS in dependency array since it is recreated on every render and this might make the useEffect run on every render.

Copy link
Contributor

@amittapalli amittapalli Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with paymentStepReached in the deps, the state change triggers the effect to re-run, it passes the guard, and SFP initializes correctly. If its step, then when user changes to shipping, this effect will run and can execute the destroy logic unmounting the SDK. But it can't remount again due to the other effect which exits early when in payment step

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we might not need the paymentStepReached state variable

Suggested change
paymentStepReached,
step === STEPS.PAYMENT,

this should work. The effect runs only when you land on or leave the payment step

data: customer,
isLoading: customerLoading,
isFetching: customerFetching
} = useCurrentCustomer(isRegistered ? ['paymentmethodreferences'] : undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought useCurrentCustomer only accepts 1 param, which is Array. It seems like you introduced a new param (refetchOnMount). The first argument ['paymentmethodreferences'] is fine since it's an array of strings matching the expected expand parameter. But the second argument { refetchOnMount: 'always' } has no corresponding parameter in the function signature to receive it. Code won't throw an error for extra arguments but I would think that its silently ignored. So what purpose will refetchOnMount serve then?

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Too many changes on my working local. Missed pushing it. Let me clear up everything else and re-verify all the changes including failOrder. I've made changes to use-current-customer.js to accept optional query params, wehre we could pass React's own refetch query param

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@rasbhat rasbhat merged commit 02d6566 into t/team404/sfp-on-pwa Mar 3, 2026
14 of 16 checks passed
@rasbhat rasbhat deleted the rvishwanathbhat/stripe-caching-for-display-spm-fix-tests branch March 3, 2026 22:38
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/stripe-caching-for-display-spm-fix-tests

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/stripe-caching-for-display-spm-fix-tests

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/stripe-caching-for-display-spm-fix-tests

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants