Fix potential race condition bug in pwless login#2758
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
| variant="outline" | ||
| borderColor="gray.500" | ||
| type="submit" | ||
| type="button" |
There was a problem hiding this comment.
We know that pwless does not require the same flow as other flows, we can safely use type=button to avoid it go through submission and handle the flow right in the onPasswordlessLoginClick handler
| pathname: '/checkout' | ||
| const mockLocation = jest.spyOn(window, 'location', 'get').mockReturnValue({ | ||
| pathname: '/checkout', | ||
| origin: 'https://webhook.site' |
There was a problem hiding this comment.
Fix error during tests
console.error
TypeError: Invalid URL: undefined//mobify/proxy/api
....
packages/template-retail-react-app/app/pages/checkout/partials/contact-info.jsx
Show resolved
Hide resolved
|
Thanks @alexvuong for looking into this and finding a fix!
|
| // since the pwless is no longer to through form submission flow, | ||
| // we need to manual validate the email value here | ||
| const email = form.getValues().email | ||
| if (!email) { | ||
| form.setError('email', { | ||
| type: 'manual', | ||
| message: formatMessage({ | ||
| defaultMessage: 'Please enter your email address.', | ||
| id: 'contact_info.error.email_required' | ||
| }) | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ | ||
| if (!emailRegex.test(email)) { | ||
| form.setError('email', { | ||
| type: 'manual', | ||
| message: formatMessage({ | ||
| defaultMessage: 'Please enter a valid email address.', | ||
| id: 'contact_info.error.email_invalid' | ||
| }) | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
This would be the first time we introduce a regex to validate email in the app. The regex doesn't cover all valid email scenarios (e.g., internationalized domains, plus signs, etc.).
Should we stick with react-hook-form's native validation like we do in other places?
Example:
const validateEmail = async () => {
const isValid = await form.trigger('email')
return isValid
}
There was a problem hiding this comment.
good call, I will work on that
|
@hajinsuha1 @adamraya Thanks for reviewing. I've addressed your comments. I've fixed the same flow for login and use-auth-modal. |
# Conflicts: # packages/template-retail-react-app/CHANGELOG.md
alexvuong
changed the title
|
@hajinsuha1 Good catch, I've changed the code to make pwless to be aware on Enter key. Please have another review when you have time I've deployed the bundle to this env if you want to test it on MRT https://runtime.commercecloud.com/salesforce-systems/scaffold-pwa/test-env-3 |
| isSocialEnabled = false, | ||
| idps = [], | ||
| setLoginType | ||
| setLoginType = noop |
There was a problem hiding this comment.
we don't need this prop in the logic but we can't remove it now since it will be a breaking change.
| } | ||
| //TODO: potentially remove this prop in the next major release since | ||
| // we don't need to use this props anymore | ||
| handlePasswordlessLoginClick={noop} |
There was a problem hiding this comment.
we don't need this prop in the logic but we can't remove it now since it will be a breaking change.
…t' into fix-pwless-login-contact-checkout
hajinsuha1
left a comment
hajinsuha1
left a comment
There was a problem hiding this comment.
Thanks for taking the time to fix this! Retested and everything looks good.
adamraya
left a comment
adamraya
left a comment
There was a problem hiding this comment.
I tested the changes on my machine and LGTM
* fix pwless login race conditions
5 participants
Description
There is a potential bug in pwless logic in check out contract info form when users click "Secure Link" to submit pwless flow.
We are trying to setState during submission and expecting the state to update immediately. This is a wrong and bug-prone because React setSate is asynchronous. See explanation here.
This will result in a race condition that we are not anticipating in very slow devices
This bug shows up during unit tests, but we are cheating by clicking the "Secure Link" button twice which forces a a re-rendering the component where it is not supposed in reality. The users on the browser never has to click twice on "Secure Link" to see the modal pop up.
Why It Works in Browser (Single Click)
In the browser, there are natural micro-delays between:
User click → Event handler execution
State update → Form submission
DOM updates → Re-rendering
These small delays give React time to process the state update before the form submission logic runs.
The browser's event loop processes things differently than the test environment:
Browser: Asynchronous, with natural pauses for repainting, user interaction, etc.
Test: Synchronous, everything happens in the same tick
React batches state updates differently in:
Browser: More time between batches, allowing updates to complete
Test: Immediate batching, exposing race conditions
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization