[Onyx Audit] Migrate keys to RAM-only part 2/2#83450
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@Krishna2323 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] |
melvin-bot
Bot
requested review from
JmillsExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <ValidateCodeModal | ||
| accountID={Number(accountID)} | ||
| code={validateCode} | ||
| /> | ||
| )} | ||
| {(!autoAuthState ? shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.SIGNING_IN) && <FullScreenLoadingIndicator />} | ||
| {(!effectiveAutoAuthState ? shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.SIGNING_IN) && <FullScreenLoadingIndicator />} | ||
| </> |
There was a problem hiding this comment.
❌ UI-1 (docs)
FullScreenLoadingIndicator is rendered here without shouldUseGoBackButton and no navigation component (e.g., HeaderWithBackButton) is visible in the same return statement or conditional branch. If loading hangs, the user has no escape route.
Note: This is a pre-existing issue (only the condition variable name changed from autoAuthState to effectiveAutoAuthState), but worth addressing. Add shouldUseGoBackButton to provide an emergency exit:
{(!effectiveAutoAuthState ? shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.SIGNING_IN) && <FullScreenLoadingIndicator shouldUseGoBackButton />}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This is a pre-existing issue — our change only renamed the condition variable from autoAuthState to effectiveAutoAuthState, the FullScreenLoadingIndicator was already rendered without shouldUseGoBackButton before this PR. Adding it would be out of scope for this migration.
![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: 4a1e83bf41
ℹ️ 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".
| // stored values here. | ||
| initWithStoredValues: false, | ||
| }); | ||
| const [userWallet] = useOnyx(ONYXKEYS.USER_WALLET); |
There was a problem hiding this comment.
Dropping initWithStoredValues: false here makes useOnyx(ONYXKEYS.USER_WALLET) consume cached disk data first, so stale isPendingOnfidoResult/hasFailedOnfido values can trigger the early redirect path before openEnablePaymentsPage() runs. In that case the page navigates away and never refreshes wallet state, which can block affected users from re-entering the enable-payments flow until another screen updates USER_WALLET.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed — the redirect logic is now deferred using a wasLoadingRef that tracks the isLoading: true → false cycle. The effect that calls openEnablePaymentsPage() always fires on mount, and a separate effect only evaluates the redirect after isLoading has transitioned from true back to false, ensuring we act on fresh API data rather than stale cached values.
| @@ -38,6 +41,8 @@ function ValidateLoginPage({ | |||
| const [preferredLocale] = useOnyx(ONYXKEYS.NVP_PREFERRED_LOCALE); | |||
|
|
|||
| useEffect(() => { | |||
| setHasInitialized(true); | |||
There was a problem hiding this comment.
This sets hasInitialized to true before initAutoAuthState() resets session.autoAuthState, so a persisted value from a prior magic-link attempt (for example FAILED) can be read and rendered for one frame depending on update timing. That reintroduces the stale-state flash that the prior initWithStoredValues: false behavior avoided for logged-out users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This isn't a real issue. For logged-out users, hasInitialized starts as false (line 32: useState(isSignedIn)), so effectiveAutoAuthState is undefined on the first render — no stale value is ever exposed. The useEffect then calls setHasInitialized(true) and initAutoAuthState() in the same synchronous block. React batches these updates, so there's no intermediate render where hasInitialized is true but autoAuthState is still stale.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
@JKobrynski Confclits @Krishna2323 can you prioritize this review please? |
|
Yes, I’ll be on it today. The first one looks good now, so I’ll finish that first and then move on to this one. |
…o maintain functionality
|
We need to figure out what to do with the ESLint failures. If they are not caused by this PR, we need someone working on them to get them fixed. If they are caused by this PR, they should be fixed here. |
|
@JKobrynski there are some stale references:
|
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
PR was already approved and tested. The failing check doesn't seem related.
Checklist here: #83450 (review)
…-migrate-keys-to-ram-only-part-2
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Agree that the use of deprecated StyleSheet.absoluteFillObject wasn't introduced in this PR
|
@JKobrynski you have conflicts here |
|
@roryabraham addressing them at this very moment! |
…91-migrate-keys-to-ram-only-part-2
…91-migrate-keys-to-ram-only-part-2
…-migrate-keys-to-ram-only-part-2
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @roryabraham 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/roryabraham in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed all changes in this PR. This is a purely internal technical refactoring that migrates Onyx keys to RAM-only storage (adding No changes are required to the help site files under |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
|
@JKobrynski @roryabraham @tgolen This PR was reverted as per this comment, Can I checkoff this one? |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.53-7 🚀
|
10 participants
Explanation of Change
This PR is part 2/2 of RAM-only migration - it migrates the keys that were not obvious RAM-only candidates. It basically includes two scenarios for these keys that used to be paired with
initWithStoredValues: false:initWithStoredValuesinitWithStoredValuesThis also includes making sure that the baseline component logic is maintained - making the necessary code changes.
Fixed Issues
$ #80091
PROPOSAL: N/A
Tests
RAM-Only Key Migration Part 2 — Testing Steps
Prerequisites
1. Plaid Bank Account Flow (
PLAID_LINK_TOKEN)2. Enable Payments / Wallet (
USER_WALLET)3. Magic Link — ValidateLoginPage (Web only)
4. Issue New Expensify Card (
ISSUE_NEW_EXPENSIFY_CARD)5. Sidebar & App Loading (
IS_SIDEBAR_LOADED,IS_LOADING_APP,IS_LOADING_REPORT_DATA)isLoadingAppdoesn't cause issues (WorkspaceAvatarModalContent)6. Translations Loading (
ARE_TRANSLATIONS_LOADING)7. Mobile Selection Mode (
MOBILE_SELECTION_MODE)8. General Regression Checks
Offline: Toggle airplane mode at various points — verify no crashes, loading states recover when back online
App restart: Force-close and reopen — verify RAM-only keys start fresh (no stale data persisted from previous session)
Console errors: Monitor JS console throughout all tests — verify no new warnings or errors related to Onyx keys
Verify that no errors appear in the JS console
Offline tests
N/A
QA Steps
Same as Tests section above.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web-compressed.mov