Repository navigation
Refactor Onfido native loader to ActivityIndicator - #96819
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The only failing check is PR Author Checklist / checklist, and it's not a code failure — nothing to push a fix for. It's failing because it's a human-only checklist: the log shows Every actual CI check is green — ( |
|
@MelvinBot add Tests/QA steps |
|
Here are suggested Tests and QA Steps for this change. Since the edit is in Tests
QA StepsSame as Tests (native only). Why native-onlyThe only changed file is
|
|
@MelvinBot please update QA steps and check all items on the author checklist (Do NOT leave any item unchecked). I also verified this across all platforms. |
|
🤖 Done — updated the PR body:
Please add the screenshots/videos to the Screenshots/Videos section before marking it ready for review. |
|
@MelvinBot mark as ready for review |
|
🤖 Done — marked this PR as ready for review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59547174e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <ActivityIndicator | ||
| size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} | ||
| style={styles.flex1} | ||
| reasonAttributes={{context: 'Onfido'}} | ||
| /> |
There was a problem hiding this comment.
Now that this inline ActivityIndicator no longer blocks the header, users can tap Back while OnfidoSDK.start() is still pending, but the effect above has no cleanup/ignore-after-unmount guard. In the native callers I checked, those late callbacks still navigate or mutate wallet/bank-account state (OnfidoStep passes goBack/verifyIdentity, and the verify-identity pages update drafts or call onBackButtonPress), so backing out during initialization can still let a later SDK resolve/reject open the flow, navigate a second time, or submit/show an error after the user has left. Please cancel or ignore the pending promise after unmount before exposing Back during loading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There was a problem hiding this comment.
🤖 Addressed in 1238fa0.
Good catch — with Back now tappable during loading, a late OnfidoSDK.start() resolve/reject could navigate or mutate wallet/bank-account state after the user has already left. I added an isActive guard that the effect's cleanup flips to false on unmount, and routed every callback (onSuccess / onUserExit / onError, including the Android relaunch path and the iOS permission Alert handlers) through guarded wrappers that no-op after unmount. See src/components/Onfido/index.native.tsx:26-47 and the cleanup at src/components/Onfido/index.native.tsx:133-135.
ActivityIndicator cleanly unmounts and stops rendering; the SDK promise chain itself can't be cancelled, but its callbacks are now ignored once the component is gone, so backing out during initialization no longer double-navigates or submits/errors after exit.
|
The only failing check on this PR is PR Reviewer Checklist / checklist (the All actual build/test checks are green (typecheck, ESLint, Bun/Jest tests, Storybook, perf, etc.). To clear it: the assigned reviewer needs to complete the PR Reviewer Checklist. Once that's checked off, this check will pass. |
|
Nothing has changed since my last comment: the only red check is PR Reviewer Checklist / checklist, which fails until a reviewer completes the reviewer checklist on this PR. It's not a code failure and can't be cleared by a push — all build/test checks are green. To clear it: the assigned reviewer needs to complete the PR Reviewer Checklist. |
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@Julesssss 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp96819.moviOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1238fa0cfb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| text: translate('common.cancel'), | ||
| onPress: () => onUserExit(true), | ||
| onPress: () => handleUserExit(true), |
There was a problem hiding this comment.
On iOS with camera or microphone permission denied, backing out while OnfidoSDK.start() is pending can still let the async permission-check branch show Alert.alert after this component has unmounted. The fresh evidence after the follow-up fix is that isActive is only checked inside handleUserExit/handleError (including this alert button callback), so the global alert itself is still created on whatever screen the user returned to; skip the permission-alert branch when !isActive before calling Alert.alert.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes, this is valid. The isActive guard added in the follow-up covers handleSuccess/handleUserExit/handleError, but the iOS permission branch calls Alert.alert(...) with no isActive check in front of it.
Since Back is now exposed while OnfidoSDK.start() is pending, this window is reachable: user denies camera/mic → backs out (component unmounts, isActive = false) → start() rejects → the .catch enters the iOS branch → checkMultiple(...).then(...) resolves and fires Alert.alert. Alert.alert is a global native prompt, not tied to this component, so the stale permission alert pops up on whatever screen the user returned to. Guarding only the button onPress callbacks doesn't stop the alert from appearing — it just no-ops the exit action if a button is tapped.
Fix as suggested: short-circuit when unmounted before presenting the alert, e.g. add if (!isActive) { return; } at the top of the checkMultiple(...).then((statuses) => { ... }) callback (line 88), which also keeps the handleError fallback in that same callback from running after unmount.
There was a problem hiding this comment.
🤖 Addressed in 45f9f21.
Good catch — the isActive guard was only inside handleUserExit/handleError, so the iOS permission branch still built and presented Alert.alert(...) even after unmount. Added an early if (!isActive) return; at the top of the checkMultiple(...).then(...) callback so the stale permission prompt is skipped once the user has backed out during a pending OnfidoSDK.start():
checkMultiple([PERMISSIONS.IOS.MICROPHONE, PERMISSIONS.IOS.CAMERA])
.then((statuses) => {
// The permission check resolves asynchronously, so the user may have already backed out (unmounting this
// component) by the time it settles. Skip the alert to avoid showing a stale permission prompt on the screen they returned to.
if (!isActive) {
return;
}
...npm run fmt and lint-changed are clean. The typecheck failures in this environment are pre-existing ResultMetadata generic errors in unrelated files (card/sidebar hooks) — none in Onfido/index.native.tsx. This is native-only and can't be exercised on the web build.
|
🤖 Code review — LGTM. The loader swap and the unmount guard both look correct. 1. Loader refactor (
2. Unmount guard ( Minor, non-blocking
Testing note: this diff only affects the native code path, so the changed lines can't be exercised on the web build — it needs an iOS/Android native build to verify visually. I don't approve/merge PRs — leaving the formal approval to a human reviewer. |
|
@MelvinBot pull main |
…/App into claude-onfidoActivityIndicator Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
🤖 Merged |
|
Fixing failing check |
|
There's no code fix to push here — every substantive check (tests, typecheck, ESLint, build, perf, etc.) is passing. The only failing check is Check independent approval, which is a review-process gate, not a code problem. It requires an approval from an independent reviewer (separate from the PR author, I can't resolve this by pushing code — and doing so would only invalidate your existing approval and re-trigger CI. This needs a reviewer action (or the check to be dismissed/overridden), not a commit. |
Julesssss
left a comment
Julesssss
left a comment
There was a problem hiding this comment.
Not sure about the isActive approach. It's a critical flow and might we drop errors or success results if component unmounts. We could keep line 91 isActive confition
| // component) by the time it settles. Skip the alert to avoid showing a stale permission prompt on the screen they returned to. | ||
| if (!isActive) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@MelvinBot address #96819 (review).
It's edge case.
There was a problem hiding this comment.
Addressing Julesssss' concern about the isActive approach dropping success/error results in this critical flow.
TL;DR: The guards don't drop a success/error in the normal flow — they only fire when the component unmounts before OnfidoSDK.start() settles. In the happy path that never happens, so I'd lean toward keeping them (agreeing with situchan that the drop scenario is a narrow edge case). Details + an alternative below.
Why normal success/error is never dropped: the native Onfido UI presents on top of this still-mounted component (we only render the spinner underneath). So when start() resolves/rejects, this component is still mounted, isActive === true, and the callback runs — the parent only unmounts us as a result of that callback. isActive is false only when something tears this component down before the SDK settles. In practice that's the user tapping Back on the init spinner (the case these guards were added for), where start() comes back as exit/cancel, not a genuine success. A real success/error is dropped only in the edge case situchan flagged: an external Onyx/navigation change unmounts the flow while the native modal is still up.
Why I'd keep the guards rather than remove them: the callbacks aren't pure "record the result" — several navigate or show global UI, which is exactly what breaks if they run after unmount. So the guards are actively preventing the stale-navigation / stale-toast bugs raised earlier in this review.
Per-caller side effects of each callback (why the guards matter)
onUserExit→Navigation.goBack()(OnfidoStep),onBackButtonPress()+setOnfidoKey(...)React state (requestorVerifyIdentity). Late call → stale double-navigation / setState-after-unmount. Must stay guarded.onError→ globalGrowl.error(...)on all three callers, plusclearOnfidoToken()+onBackButtonPress()on the requestor page. Late call → a stale toast on whatever screen the user returned to, and a stray navigation.onSuccess→ mostly action dispatch (verifyIdentity*+ draft writes), which is safe post-unmount — but the requestor page also callsonSubmit?.(), which advances the step.
Net: removing the guards trades a very-rare dropped verification (the user can retry) for visibly-stale navigation/toasts. Since the drop is only reachable via that external-unmount edge case, keeping the current approach seems right.
If you'd rather bias toward never losing a completed verification (Julesssss' preference), the surgical option is to ungard handleSuccess only — the pure recording path. Caveat: on the requestor page onSuccess also calls onSubmit?.() (navigation), so it isn't 100% side-effect-free. Just say the word and I'll push that variant.
Relevant code: guards, permission-alert guard, unmount cleanup.
|
@MelvinBot pull main and fix conflict |
…Indicator Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com> # Conflicts: # src/components/Onfido/index.native.tsx
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
🤖 Merged latest Conflict: Resolution: Kept this PR's change ( return (
<ActivityIndicator
size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE}
style={styles.flex1}
/>
);Notes
|
|
🚧 Julesssss 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.60-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site files in No help site changes are required. This PR is an internal, native-only refactor of the
There is no change to any user-facing feature, workflow, setting, tab, label, or button. The help articles that touch identity verification (e.g. |
|
It seems like we can’t see these steps because Onfido loads very quickly. Is there anything we can do to make these steps accessible for testing? For example, is there a way to simulate a slow or unstable internet connection? az_recorder_20260825_145808.mp4 |
|
Agree very hard to see loading indicator. Maybe test on very old android device. |
|
@situchan, maybe it would be possible to check this internally on your side? |
Yeah checking off, thank you |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.60-2 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
Refactors the
Onfidonative component's loader fromFullscreenLoadingIndicatortoActivityIndicator, in preparation for making the error/loading page recoverable (enabling the Go Back button by default).Per UI-1,
FullscreenLoadingIndicatorrenders as anabsoluteFilloverlay that covers the visible header/back button. All three consumers of this shared component render a header behind the loader:pages/ReimbursementAccount/USD/Requestor/VerifyIdentity/VerifyIdentity.tsx(InteractiveStepWrapper)pages/EnablePayments/Wallet/VerifyIdentity/VerifyIdentity.tsx(HeaderWithBackButton)pages/EnablePayments/Pay/Onfido/OnfidoStep.tsx(HeaderWithBackButton)So the correct loader here is
ActivityIndicator, which fills the space below the header (style={styles.flex1}) and keeps navigation accessible while loading.This is one of two separate PRs for the linked issue (one per occurrence); the other refactors
pages/settings/Wallet/PersonalCards/steps/BankConnection/index.native.tsx.Fixed Issues
$ #96095
PROPOSAL:
Tests
Offline tests
Onfido identity verification requires a network connection, so there is no offline-specific behavior for this loader change. With no network, the Onfido SDK cannot initialize; the change only affects how the loading spinner is laid out (below the header, with the back button visible/tappable) while it initializes.
QA Steps
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari