Bug fix: first biometric auth after log out/in sees unnecessary soft prompt#86291
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@thesahindia 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] |
|
@codex 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: 676ccffb1d
ℹ️ 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".
| const {accountID} = useCurrentUserPersonalDetails(); | ||
| const [serverHasCredentials, setServerHasCredentials] = useState(false); | ||
| const [deviceBiometricsState] = useOnyx(ONYXKEYS.DEVICE_BIOMETRICS); | ||
| const [deviceBiometricsState] = useOnyx(getDeviceBiometricsOnyxKey(accountID)); |
There was a problem hiding this comment.
This read now only checks deviceBiometrics_${accountID}, but the previous value was stored under the legacy non-collection key, so existing users who already accepted the soft prompt will be treated as first-time until they re-approve. In practice, the first MFA authorization after this upgrade can show an unnecessary soft prompt even without reinstall/logout. Add a migration or fallback read from the legacy key during the transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It is known and acceptable - this is not a commonly used feature, as the main use for it is behind a beta, and the UX cost of re-registration is low. I don't think it's worth the complexity to add a migration.
| useEffect(() => { | ||
| // Non-reactive read of deviceBiometrics. Using Onyx.connectWithoutView (set up in a useEffect | ||
| // inside the provider) instead of useOnyx to avoid triggering process() too many times during | ||
| // the fresh registration flow. | ||
| const connection = Onyx.connectWithoutView({ | ||
| key: getDeviceBiometricsOnyxKey(accountID), | ||
| callback: (data) => { | ||
| deviceBiometricsState = data; | ||
| }, | ||
| }); | ||
| return () => Onyx.disconnect(connection); | ||
| }, [accountID]); | ||
|
|
There was a problem hiding this comment.
Using Onyx.connectWithoutView in components is a massive anti-pattern, and if it’s possible for this code to work with useOnyx, it would definitely be better to use that instead.
In useEffect, the deps array isn’t fully populated anyway, so omitting one additional dependency shouldn’t be a problem.
There was a problem hiding this comment.
I agree. I don’t think useOnyx would cause process() to trigger too frequently. Could you clarify in more detail why we shouldn’t use useOnyx?
There was a problem hiding this comment.
Experimentally, it did though 🙈. The process function relies on being called precisely when it should be and never more, and adding a subscription to this flag caused one extra call when we set the flag.
There was a problem hiding this comment.
I agree it feels icky, but we've already talked about refactoring this file and agreed it's not a short term priority (connectWithoutView was the pattern already established in this file)
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.soft.prompt.works.correctly.mp4iOS: Nativeios.soft.prompt.works.correctly.mp4MacOS: Chrome / Safarichrome.soft.prompt.works.correctly.mp4 |
This comment was marked as outdated.
This comment was marked as outdated.
|
🚧 @chuckdries 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/chuckdries in version: 9.3.49-0 🚀
Bundle Size Analysis (Sentry): |
|
@rafecolton Step 7 fails for Android, is this testable on Android? |
|
@nlemma that behavior is only important on iOS, but it is still surprising to me that it failed. What did you observe? |
|
@chuckdries On step 7, "Log in, go to troubleshoot, confirm you're still registered, and tap Test We were no longer registered (it says "Another device registered"), and were prompted to scan our finger twice. 1774540018321.D3dd8d6d-07F8-4Ec0-96C2-5519C9decfbb-1.mp4 |
|
Interesting. Seems like android gives us different credential IDs when you reinstall the app. This is fine, as the flow the user sees is reasonable and not confusing or broken. @IuliiaHerets @nlemma please mark this as a pass. Thanks! |
|
Very interesting, good to know. Agree with Chuck's assessment, thanks for flagging 👍 |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.3.49-2 🚀
|
7 participants
Explanation of Change
Fixes a bug where a user registers a passkey or mobile native biometrics, logs out, logs in, and sees a soft prompt before authenticating, as opposed to the authorization screen.
This fix also ensures that if the app is entirely deleted from a device and reinstalled, that we DO show the soft prompt again. That is still needed because iOS will show the system prompt in that case.
Fixed Issues
$ #86284
PROPOSAL: N/A
Tests
Web
Testand register a passkey. You should see the "soft prompt" first (it's a page that says"Enable quick, secure..."and has a green button).Now, let's authenticate you...screen with the spinner right away, and you should not have to click any green button to get the browser passkey prompt.iOS
Test. You should go to authentication right away and not see the soft or the system prompt.TestOffline tests
N/A, online only
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
iOS: Native
Screen.Recording.2026-03-24.at.5.16.49.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-24.at.4.59.47.PM.mov