Fix staging server toggle resetting on cold start in release builds - #98971
Fix staging server toggle resetting on cold start in release builds#98971mikolajpochec wants to merge 11 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Hi @mikolajpochec could you add the test steps and fix checks please, I'll generate builds in the meantime |
|
🚧 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! 🧪🧪
|
Hi @Julesssss, I'm still waiting for our SWM internal review. I'll add test steps soon, and I'll open this draft |
war-in
left a comment
war-in
left a comment
There was a problem hiding this comment.
Good job overall! I'd just review all comments and check if they can be shortened/removed
| // `?? {}` used to stand in for a missing value, but these keys are not all objects — | ||
| // merging `{}` into SHOULD_USE_STAGING_SERVER leaves a truthy non-boolean behind. There is | ||
| // nothing to apply when OldDot sends nothing, so skip the key instead. |
There was a problem hiding this comment.
This sounds like a comment you could add here in GH to explain why the change was made, but it shouldn't land in the code. Once the PR is merged, everyone will wonder what ?? {} was and why this is mentioned here
| // `?? {}` used to stand in for a missing value, but these keys are not all objects — | ||
| // merging `{}` into SHOULD_USE_STAGING_SERVER leaves a truthy non-boolean behind. There is | ||
| // nothing to apply when OldDot sends nothing, so skip the key instead. | ||
| if (value === undefined || value === null) { |
There was a problem hiding this comment.
I wonder if we sometimes would want to send value: null to clear the onyx entry 🤔 if yes, the value won't be applied and we'll waste time debugging
Shouldn't we only check for undefined?
| /** | ||
| * Whether something other than the Play Store put this build on the device. Production builds only ever reach a | ||
| * device through the Play Store, so any other installer means the build was sideloaded, i.e. downloaded from a | ||
| * GitHub release. Unlike the version comparison below, the answer does not change once a production release catches | ||
| * up with the build's version — though Android does rewrite the installer if a different one later updates the app. | ||
| */ |
There was a problem hiding this comment.
I wouldn't compare this function to the other in the description comment. Someone might change the other function and the comment would be outdated
| * GitHub release. Unlike the version comparison below, the answer does not change once a production release catches | ||
| * up with the build's version — though Android does rewrite the installer if a different one later updates the app. | ||
| */ | ||
| function isSideloadedBuild(): boolean { |
There was a problem hiding this comment.
How about naming it isProductionBuild?
| */ | ||
| function isBetaBuild(): IsBetaBuild { | ||
| return new Promise((resolve) => { | ||
| // A sideloaded build is a beta build: testers install those from GitHub prereleases. Answering here also |
There was a problem hiding this comment.
It could land in the function description
| if (!productionVersion) { | ||
|
|
||
| // A rate limited or malformed response carries no usable tag. Production is the safe answer when | ||
| // we cannot tell — the missing `return` here used to fall through into semver.gt(version, undefined), |
There was a problem hiding this comment.
Wasn't that intentional? To fall back to isLastSavedBeta? Also, this comment will get outdated when we merge the PR
3 participants
Explanation of Change
Fixes the staging toggle resetting to production on Android GH-release builds, and being off by
default on a fresh install of a staging build.
Staging and production ship the same binary, so the environment is inferred at runtime. That
inference compared
package.json's version against the latest GitHub release, which decays: onceproduction catches up, the build resolves to PRODUCTION and
ApiUtilsdiscards the storedpreference. It now uses the install source — anything the Play Store didn't install is a build a
tester was handed directly. Decided at install time, no network, cannot decay.
ApiUtilsalso derives the flag on demand instead of caching it, andgetEnvironmentcan nolonger leave its promise unsettled.
OldDot half: https://github.com/Expensify/Mobile-Expensify/pull/14062 — both are needed for HybridApp.
Behavior changes vs main:
com.android.vending(sideload,adb install, null →unknown) resolves STAGINGregardless of version — including a sideloaded production APK.
com.android.vendingis characteristic to apps distributed via Play Store.ONYXKEYS.IS_BETApreserved across sign-out (specific scenario, the old way fix).semver.gt(version, undefined), threw, and landed in the catch. Same verdict, no throw.null/undefinedvalues in the OldDot transition payload are skipped rather than merged as{}, which left a truthy non-boolean inSHOULD_USE_STAGING_SERVER.Fixed Issues
$ #97891
PROPOSAL:
Tests
Offline tests
QA Steps
// 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari