Fix: physical-address error persists after selecting a valid address#94528
Conversation
…oes not reappear Co-authored-by: Justin Persaud <justinpersaud@users.noreply.github.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@thelullabyy @justinpersaud One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@marufsharifi this is a regression from #92289 I had Melvin fix it here if you can help test cc @samranahm |
|
🚧 justinpersaud has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-06-27.at.1.32.12.AM.movAndroid: mWeb ChromeScreen.Recording.2026-06-27.at.1.41.51.AM.moviOS: HybridAppScreen.Recording.2026-07-17.at.11.06.35.AM.moviOS: mWeb SafariScreen.Recording.2026-06-27.at.1.57.03.AM.movMacOS: Chrome / SafariScreen.Recording.2026-06-27.at.12.27.25.AM.mov |
|
@mitarachim can you please retest the regression reported in #94519 over here using https://94528.pr-testing.expensify.com/ It works for me |
|
bump @mitarachim @marufsharifi |
|
@justinpersaud, should this PR cover this? RF-5.mp4 |
|
@marufsharifi I can still reproduce the reported bug with these PR changes. Kapture.2026-06-30.at.05.58.32.mp4 |
|
The issue doesn't seem to be fixed. I will check this tomorrow. |
|
@MelvinBot, the original issue is still reproducible. Could you please find the root cause? |
Issue AnalysisRoot cause: This fix can't reliably work because it's racing an asynchronous network call. Selecting a Google Places suggestion doesn't write the clean street synchronously — with This is why it reproduces intermittently — Recommended fix: Stop relying on the blur timer reading the "latest" value (it can't, because the selection commit is async). Instead make the selection authoritative:
Option 1 is the most targeted — it removes the racing re-validation entirely rather than trying to out-time the network. Evidence & timelineThe async gate (library): Timeline (slow fetch > 200 ms):
Fast fetch (< 200 ms): clean street commits first, blur at 200 ms reads it → stays cleared. This is the "works for me" path — and exactly what the current fix improves, which is why it looks fixed in some environments. Confirmed not the cause: the inputID key mapping is correct (typed and selected values share
Next Steps: Reply with |
|
@MelvinBot, based on your root cause, what is the best appraoch to fix this, please don't implement, just explain your approach to me |
Recommended approachDon't try to out-time the network — stop the blur timer from validating the address input at all when a suggestion is being selected, and let the selection be the single source of truth. The core constraint from the root cause is that The blur re-validation is redundant here: selecting a suggestion already runs Primary — suppress the blur validation when focus moves into the suggestion list. Fallback (if Complementary — guarantee the async commit is the last validation. I'd implement the primary alone first — it's the smallest change that removes the race — and keep the current PR's "validate against latest committed values" edit too, since it's still correct for the fast-fetch ordering. Testing note: reproduce the slow path deterministically by throttling the network (DevTools → Slow 3G, or add latency to the Next Steps: Reply with |
|
@MelvinBot, could you validate my RCA for this issue Root Cause On the first interaction: The form starts with This only happens the first time because later renders capture the updated error state, causing deepEqual to detect the change and clear the error. |
|
@justinpersaud, the reviewer checklist was already done by me. I used Melvin to complete the PR author checklist. |
…ressErrorPersists Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com> # Conflicts: # src/components/Form/FormProvider.tsx
|
🤖 Merged latest There was one merge conflict, in
I resolved it to match exactly the reactive values the merged callback body references: [shouldTrimValues, formID, validate, translate, allowHTML, shouldUseStrictHtmlTagValidation, hasServerError, hasServerErrorFields]i.e.
|
|
@justinpersaud, it's done from my side, all yours. Thanks. |
|
Going to need a new reviewer on this one |
|
@marufsharifi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@justinpersaud Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@marufsharifi We're still getting the same error here. Kapture.2026-07-18.at.00.11.24.mp4Should I post my root cause analysis and core solution to let the melvin implement that or I can open a PR real quick. cc: @justinpersaud |
@samranahm, this is expected, and the error disappears after selection. |
|
@justinpersaud, could you please take a look at this when you get a chance. thanks. |
|
I tested it out and it works for me but we need another reviewer here |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 justinpersaud has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/justinpersaud in version: 9.4.41-0 🚀
|
|
🤖 Docs review: no help site changes required. I reviewed the changes in this PR against the articles under What this PR changes: a bug fix in shared form machinery ( Why no docs update is needed:
Since the article already reflects the current, correct behavior, there's nothing to add or amend. No draft docs PR was created. @justinpersaud, please confirm you agree no help site changes are required here. If you'd like me to document anything additional (e.g. add a note to the address step), reply with |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.41-4 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On the bank account address steps, typing
po box 123into the Street field and then selecting a valid (non-PO-box)address from the suggestion list left the
A physical address is required. PO boxes and mail drops are not accepted. error showing, even though the field now held a valid address.Root cause: When a suggestion is tapped, two things happen almost at once:
FormProvider'sonBlurschedules a delayed validation (VALIDATE_DELAY= 200ms) that calledonValidate(inputValues, …)using theinputValuescaptured in the closure at blur time — which still held"po box 123".Because that timeout fires ~200ms after the selection already cleared the error — and it also marks the field touched — it re-ran validation against the stale "po box 123" value and re-applied the error. The error therefore briefly cleared and then came back.
This lives in shared form machinery (FormProvider), not in the address validation added in #92289 — the PO box check already existed before that PR (under the older "Please enter a valid street address" copy), so the same persistence affects the emoji "valid street address" error too.
Fix: Make the delayed blur validation read the latest committed form values instead of the closure-captured ones, using the same functional-state-read pattern already used by onInputChange in this file. Now if a suggestion is selected after the blur is scheduled, the validation runs against the selected (valid) value and the error stays cleared.
Fixed Issues
$ #94519
PROPOSAL: #94519 (comment)
Tests
po box 123and wait for the physical-address error to appear.A physical address is required…error disappears and does not reappear.Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari