[Payment due @mkhutornyi] Fire OpenApp without chained promise - #94121
Conversation
|
@madmax330 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef81897a44
ℹ️ 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".
| function getPolicyParamsForOpenOrReconnect(): PolicyParamsForOpenOrReconnect { | ||
| return {policyIDList: getNonOptimisticPolicyIDs(allPolicies)}; |
There was a problem hiding this comment.
When openApp()/reconnectApp() runs before this module-level policy subscription has delivered its first collection callback (for example during cold start, sign-in, or after clearing Onyx while switching accounts), allPolicies is still undefined, so this returns policyIDList: [] and the queued OpenApp/ReconnectApp request is never corrected after policies hydrate. The previous per-call connectWithoutView(...waitForCollectionCallback...) waited for the current policy collection before building the request, so existing workspace policy IDs were sent reliably; without them the server cannot include/update data scoped to those cached policies on the initial sync.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this is a real problem. If we have not loaded the policies in the module level by the time we call this then we probably have nothing to load. I think this is mostly needed for ReconnectApp and it does look to work fine on page refresh.
| function triggerFullReconnect(cutoff: string) { | ||
| Onyx.merge(ONYXKEYS.LAST_FULL_RECONNECT_TIME, getLastFullReconnectTimeToRecord(cutoff)); | ||
| reconnectApp(); | ||
| Onyx.merge(ONYXKEYS.LAST_FULL_RECONNECT_TIME, getLastFullReconnectTimeToRecord(cutoff)).then(() => reconnectApp()); |
There was a problem hiding this comment.
This is needed because before the changes in this PR, reconnectApp already had a promise that it was waiting for and by the time it resolves the onyx value would have been updated already.
Now, since this reconnectApp call happens instantly we need to explicitly wait here before reconnect app
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@mkhutornyi When testing this, can you also please test #18115 in main and in this branch and confirm if we have a different behavior. Ideally, you should land on the room report but if you landed on Home on both branches that may be okay (as long as we are not causing a regression here) |
|
@codex review |
|
@MelvinBot Review |
ReviewThe approach is sound and correctly targets the root cause. Firing Two things worth confirming before merge: 1. Removing the 2. Module-level The Minor nit
Nothing here blocks merge from a code-correctness standpoint — the implementation is clean. The decision hinges on the #18115 QA confirmation in point 1. |
|
@adhorodyski Could you please review as well since you had been working in this area recently so it will be helpful to get your view as well. Thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1a8ce2d1c
ℹ️ 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".
|
My first feedback is that this looks like a big correctness update so 👍🏼 I don't see any pain points and things that could break with it, it does not really touch the SQ or any other ordering mechanisms. |
| if (isLoadingApp) { | ||
| Log.info('[Reconnect] App is still loading, calling openApp'); | ||
| openApp(); |
There was a problem hiding this comment.
This is not correct. If we are loading app (OpenApp in progress) then we should not queue another OpenApp call.
If the app is not loaded, reconnectApp will actually call OpenApp (already handled in the call itself), otherwise we will call reconnectApp normally
|
Should be fixed now. @mkhutornyi Can you please confirm the same |
| test('calls openApp when isLoadingApp is true', async () => { | ||
| await Onyx.merge(ONYXKEYS.SESSION, {accountID: 1234, email: 'test@test.com'}); | ||
| await Onyx.merge(ONYXKEYS.IS_LOADING_APP, true); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| reconnect(); | ||
|
|
||
| expect(jest.mocked(openApp)).toHaveBeenCalledTimes(1); | ||
| expect(jest.mocked(reconnectApp)).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('calls reconnectApp when isLoadingApp is false', async () => { | ||
| await Onyx.merge(ONYXKEYS.SESSION, {accountID: 1234, email: 'test@test.com'}); | ||
| await Onyx.merge(ONYXKEYS.IS_LOADING_APP, false); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| reconnect(); | ||
|
|
||
| expect(jest.mocked(reconnectApp)).toHaveBeenCalledTimes(1); | ||
| expect(jest.mocked(openApp)).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
There are no longer relevant. Whether the app is loading or not, we will always call reconnectApp, we will never queue OpenApp on top of another existing OpenApp.
|
Still same. This is log after fresh install and creating new account: 
|
|
@s77rt @mkhutornyi we merged this PR #94210 yesterday, can you check if it does not fix your issue by any chance? |
|
@mkhutornyi I was not able to reproduce. Can you please check again and verify that you synced the branch Screen.Recording.2026-06-26.at.2.35.46.PM.movBefore testing, please apply this diff to see which openApp is firing the requests Detailsdiff --git a/src/DelegateAccessHandler.tsx b/src/DelegateAccessHandler.tsx
index 62005f45c73..d1d48d9a023 100644
--- a/src/DelegateAccessHandler.tsx
+++ b/src/DelegateAccessHandler.tsx
@@ -68,6 +68,7 @@ function DelegateAccessHandler() {
sessionAccountID,
hasLoadedApp: !!hasLoadedApp,
});
+ console.log('openApp A');
openApp();
}, [hasLoadedApp, isLoadingApp, isOffline, sessionAccountID, isLoadingAppMetadata]);
diff --git a/src/components/OpenAppFailureModal/index.native.tsx b/src/components/OpenAppFailureModal/index.native.tsx
index 3e81764cce7..e53d63a27d5 100644
--- a/src/components/OpenAppFailureModal/index.native.tsx
+++ b/src/components/OpenAppFailureModal/index.native.tsx
@@ -7,6 +7,7 @@ import BaseOpenAppFailureModal from './BaseOpenAppFailureModal';
/** Triggers OpenApp reconnection */
const retryOpenApp = () => {
setIsOpenAppFailureModalOpen(false);
+ console.log('openApp B');
openApp();
};
diff --git a/src/libs/Navigation/AppNavigator/AuthScreensInitHandler.tsx b/src/libs/Navigation/AppNavigator/AuthScreensInitHandler.tsx
index 7eb752147fe..c82399d6838 100644
--- a/src/libs/Navigation/AppNavigator/AuthScreensInitHandler.tsx
+++ b/src/libs/Navigation/AppNavigator/AuthScreensInitHandler.tsx
@@ -125,6 +125,7 @@ function AuthScreensInitHandler() {
// Don't want to call `openReport` again when logging out and then logging in
setIsAuthenticatedAtStartup(true);
}
+ console.log('openApp M');
App.openApp();
} else {
Log.info('[AuthScreens] Sending ReconnectApp');
diff --git a/src/libs/actions/App.ts b/src/libs/actions/App.ts
index da6befbc02e..a10fd8d2c14 100644
--- a/src/libs/actions/App.ts
+++ b/src/libs/actions/App.ts
@@ -481,6 +481,7 @@ function reconnectApp(updateIDFrom: OnyxEntry<number> = 0) {
if (bootsplashSpan) {
endSpan(CONST.TELEMETRY.SPAN_NAVIGATION.APP_OPEN);
}
+ console.log('openApp C');
openApp();
return;
}
diff --git a/src/libs/actions/Delegate.ts b/src/libs/actions/Delegate.ts
index 6f785504214..0fe2112846b 100644
--- a/src/libs/actions/Delegate.ts
+++ b/src/libs/actions/Delegate.ts
@@ -218,6 +218,7 @@ function connect({email, delegatedAccess, credentials, session, activePolicyID,
return clearOnyxForDelegateTransition();
})
.then(() => {
+ console.log('openApp D');
return openApp().then(() => {
if (!CONFIG.IS_HYBRID_APP || !policyID) {
return true;
@@ -322,6 +323,7 @@ function disconnect({stashedCredentials, stashedSession}: DisconnectParams) {
});
Onyx.set(ONYXKEYS.STASHED_CREDENTIALS, {});
Onyx.set(ONYXKEYS.STASHED_SESSION, {});
+ console.log('openApp E');
openApp().then(() => {
if (!CONFIG.IS_HYBRID_APP) {
return;
@@ -760,6 +762,7 @@ function restoreDelegateSession<TKey extends OnyxKey>(authenticateResponse: Resp
NetworkStore.setAuthToken(authenticateResponse.authToken ?? null);
NetworkStore.setIsAuthenticating(false);
+ console.log('openApp F');
openApp();
});
}
diff --git a/src/libs/actions/Session/index.ts b/src/libs/actions/Session/index.ts
index acf9a58748b..6edca69d706 100644
--- a/src/libs/actions/Session/index.ts
+++ b/src/libs/actions/Session/index.ts
@@ -466,6 +466,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess
Onyx.set(ONYXKEYS.STASHED_CREDENTIALS, {});
Onyx.set(ONYXKEYS.STASHED_SESSION, {});
+ console.log('openApp H');
openApp();
if (CONFIG.IS_HYBRID_APP && hasSwitchedAccountInHybridMode) {
@@ -483,6 +484,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess
Onyx.multiSet(onyxSetParams);
if (hasSwitchedAccountInHybridMode) {
+ console.log('openApp I');
openApp();
}
});
@@ -713,6 +715,7 @@ function setupNewDotAfterTransitionFromOldDot(hybridAppSettings: HybridAppSettin
// Just update the session and credentials without clearing
if (isUsingImportedState) {
Log.info('[HybridApp] Skipping Onyx clear because using imported state. Updating session only.');
+ console.log('openApp J');
return Onyx.multiSet({
...stashedData,
[ONYXKEYS.SESSION]: {
@@ -731,6 +734,7 @@ function setupNewDotAfterTransitionFromOldDot(hybridAppSettings: HybridAppSettin
}
Log.info('[HybridApp] User switched account on OldDot side. Clearing onyx and applying delegate data');
+ console.log('openApp K');
return clearOnyxForDelegateTransition()
.then(() =>
Onyx.multiSet({
@@ -1293,6 +1297,7 @@ function updateAuthTokenAndOpenApp(authToken?: string, encryptedAuthToken?: stri
// enough to do the updateSessionAuthTokens() call above.
NetworkStore.setAuthToken(authToken ?? null);
+ console.log('openApp L');
openApp();
}
|
|
@mountiny I merged main yesterday and that change is included |
|
@s77rt here's full video with logs: Screen.Recording.2026-06-26.at.4.12.12.PM.mov |
|
Actually, it behaves the same in production ( |
|
🎯 @mkhutornyi, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f640ba258
ℹ️ 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".
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run 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. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.22-1 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.24-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.24-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #94954 |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.24-0 🚀
|
Explanation of Change
On login, we call
openApp()fromAuthScreensInitHandler'suseEffectsince it's the first mounted screen, then we may callsearch()fromYourSpendSection'suseYourSpendDatawhen we load the home page. Although this seems correct, the actual requests order may not beOpenApp->Searchbecause theopenApp()function does not initiate the request instantly instead it does so after a promise is resolved and since that's done asynchronously, it will often get called aftersearch()making the orderSearch->OpenApp. This is a problem becauseOpenAppmay overwrite data that was returned bySearch.In this PR I have removed the chained promise and now
openApp()will initiate the http call instantly and that would preserve the expected orderOpenApp->Search.Also: I have removed the
isReadyToOpenApppromise since it's a promise and conflicts with the changes here. That promise was added here #18114 to $ #18115 and I think it is not doing anything at this point as after singing in I got redirected to Home (with and without the promise).Fixed Issues
https://github.com/Expensify/Expensify/issues/643382
PROPOSAL:
Tests
NetworktabOpenApprequest before anySearchrequestsScreen.Recording.2026-06-20.at.7.10.32.PM.mov
Screen.Recording.2026-06-20.at.7.21.39.PM.mov
Offline tests
n/a
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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