Repository navigation
Add a server selection page (part 2/5) - #100209
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
7abcae8 to
3b21f59
Compare
…tion # Conflicts: # src/ROUTES.ts # src/components/TestToolMenu.tsx # src/libs/Navigation/linkingConfig/RELATIONS/SETTINGS_TO_RHP.ts
|
@codex review |
|
@MelvinBot review |
|
@MelvinBot find regressions |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 534bfa793b
ℹ️ 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".
|
🤖 Reviewed at 1. The PR doesn't do half of what the title and description sayThe title says "sign out on a QA crossing," and the description's 2. Save doesn't close the page
3.
|
|
🤖 Regression hunt on 1. Save leaves you on the page with no sign anything happened — confirmed on webDrove the branch on web: selecting Staging enables Save, but pressing Save leaves the Server page exactly where it was. The only feedback is the button fading back to disabled.
2. The row reports the wrong server on the first render of every non-dev build
The old 3. Switching servers went from one tap to four, and QA steps assume otherwiseRow → page → option → Save, versus one switch flip. That's a deliberate trade for making the QA crossing confirmable, but the confirmation isn't in this PR (see my earlier review), so right now it's cost without the benefit. Combined with 1, a tester gets no success signal at all. Worth saying explicitly in the QA steps that the change is deliberate. Lower-risk notes, and three things I checked that are not regressionsLower risk
Checked, not regressions
Not covered by the live run: the web harness can't send a Ctrl+D chord, so everything reached through the test tools modal was verified by reading the code rather than by driving it. Next Steps: Reply with |
|
Replying to this comment. Fixed: #1, #2, #3. #2 in CI. Right, The local-web deeplink. Right, and inert rather than harmful. One correction: a stored The two raw Step 5.2 and the #4, the unresolved environment. Half wrong, and the rest is not getting fixed here. The Troubleshoot row cannot paint pinned, because the page renders the whole test tool menu under |
|
Replying to this comment. #1 is fixed in The On #3, the four taps are deliberate, but the QA steps do not assume one: steps 1 through 8 open Troubleshoot, press the row, select a server and press Save, and the old toggle's behaviour is carried as an explicit regression test in the Tests section. |
|
@codex review |
|
@MelvinBot review |
|
@MelvinBot find regressions |
|
🤖 Re-reviewed at Fixed:
Still open:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e77e938da
ℹ️ 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".
|
Replying to this comment. The pinned-flag item is answered here, and the disproof still holds at Step 1 of the optional developer-only test, Test 5 since the description was renumbered after that review, shows the same thing rather than cutting against it. It hard-codes the value inside |
|
🤖 Regression hunt on Regression:
|
| Step | Status |
|---|---|
| ✅ | |
| ✅ | |
| ✅ | |
| ✅ | |
| ✅ | |
Ctrl+D open/dismiss, including from the pushed selectorNot run. The web driver has no primitive for a modifier-key chord and Test Tools has no clickable trigger, so thegetBackToParam and pop changes are unverified at runtime. |
Coverage gap
The Ctrl+D dismissal path is the part of this PR most likely to regress existing behaviour and it's the one part I could not exercise. Please run Tests 5.1–5.4 by hand. Native and HybridApp aren't covered by this run either.
Chased and cleared — not regressions
Navigation.pop(rootKey)replacingNavigation.goBack()inTestTool.ts:36-38. I expected this to strand a user who opened/test-toolsby direct link:StackRouter'sPOPreturnsnullwhen the target index is 0, whereas the oldgoBack()had an explicitresetToAppRoot()escape for exactly that case. It doesn't happen —getAdaptedStatealways prepends a full-screen route when building state from a path, so the root stack has at least two routes and the pop lands. Worth knowing the escape hatch is gone if that ever stops holding.getBackToParam()widening from the focused route to anyTEST_TOOLS_MODAL_NAVIGATORin the root stack. Every caller is gated behindgetActiveRoute().includes('test-tools'), which is only true when the modal is focused, so there's no reachable behaviour difference.- The new
test-tools/serverpath colliding withtest-tools. Nothing else insrc/string-matchestest-tools, and both patterns areexact: true. cidMapretyped toRecord<Environment, string>. Purely additive QA keys; no existing environment's value changed.TestToolMenu's new required prop. Both call sites updated.- Nesting the new pressable inside
TestToolsModalPage's wrapper. That wrapper isaccessible={false}with noonPress— same as the existingSwitchandButtonchildren.
Non-code fallout worth a heads-up
The accessibilityLabel="Use Staging Server" switch is gone. Any QA or Applause automation keyed on that label breaks with this PR — worth a note to whoever owns those scripts.
Next Steps: Reply with @MelvinBot <your feedback> to dig into any of these.
|
Replying to this comment. Not a bug, and it is the third landing of this one: answered here and here. The flag is spuriously pinned on the first committed render, and neither surface can paint it. The Troubleshoot row's only mount path is inside the test tool menu, gated on The one build where the environment is genuinely slow is a TestFlight staging build waiting on the native beta check, and there the synchronous config reports production as well, so the cross-check |
|
@sharabai Is this PR ready for me to review again? Tag me when it’s ready again. Thanks! |
Fixed in |
Fixed in Worth flagging that this one is not from this PR. It reproduces on |
|
@huult thanks for the review! Great catches! I've addressed everything you pointed out. Main is merged into the branch. |
|
Thanks, I’ll review it soon. |
| "../../src/pages/workspace/WorkspacesListPage.tsx" "rulesdir/no-direct-personal-details-list" 1 | ||
| "../../src/polyfills/requestIdleCallback.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 | ||
| "../../src/ROUTES.ts" "@typescript-eslint/no-deprecated/getUrlWithBackToParam" 52 | ||
| "../../src/ROUTES.ts" "@typescript-eslint/no-deprecated/getUrlWithBackToParam" 53 |
There was a problem hiding this comment.
@sharabai I think this change is unnecessary. Could you undo it?
| TEST_TOOLS_SERVER: { | ||
| route: 'test-tools/server', | ||
|
|
||
| getRoute: (backTo?: string) => getUrlWithBackToParam('test-tools/server' as const, backTo), |
There was a problem hiding this comment.
@sharabai We don't use the backTo prop anymore. Could you update this page to use the dynamic route?
It's actually correct, you're really hitting production. |
|
@huult about I'd keep it. The Test Tools modal underneath already uses Doing it the proper way means rewriting the modal as a dynamic route, then stacking the server page on top of it as a second dynamic route. The docs say a single-entry screen like this should just be a static route, but that's off the table once the parent is dynamic. That's a lot of rework for one debug screen, and |
|
@iwiznia all yours! |
|
🚧 iwiznia 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/iwiznia in version: 9.4.96-0 🚀
|
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.4.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.99-2 🚀
Bundle Size Analysis (Sentry): |









Explanation of Change
Settings > Troubleshoot and the test tools modal replace the staging toggle with a row opening
ServerSelector, a list of production and staging that commits on Save and returns to the screen it wasopened from.
setActiveServeris unchanged: production and staging share one database, so the sessionsurvives. QA does not, and both the QA row and the sign-out it needs arrive in part 4 of five, behind
isQAAuthConfigured().A QA bundle pins its own server and ignores whatever is stored, so a screen reading
ACTIVE_SERVERcan name a server the requests do not go to. A production bundle honors a stored staging,since support hands staging to customers on store builds, but refuses a stored QA, since it has no QA host
to reach.
resolveActiveServernow returnsActiveServerState {activeServer, isPinnedByEnvironment, isStagingIgnored, isQASelectable}rather than abare server, and a new
useActiveServerhook calls it during render with whatuseOnyxanduseEnvironmenthand back. Theselector and the test tools menu read through it, and the remaining screens move onto the hook in part 4. On
a pinned build the selector lists its server, disables every row, hides Save and explains itself, and the
test-tools row states the server rather than linking to a page that could not change it. The selector also
reads
isQASelectablefrom the same state, so the QA row is offered only where a stored QA would survivethe next read.
Important
The test tools modal now dismisses from a pushed child screen.
getBackToParamreadsbackTooff themodal's root screen rather than the focused route, so the debug shortcut pressed on the selector closes the
modal instead of popping the selector and leaving the modal behind. An open that carries a
backTotakesgoBack(backTo). An open from a public screen other than sign-in carries none, becausegetAdaptedStateleaves
TAB_NAVIGATORout of the root when every route is a public screen, and theretoggleTestToolsModalpops the root stack's top route.
dismissModal()cannot serve that path: the public root is a plainplatform stack and never handles
DISMISS_MODAL.Fixed Issues
$ #91419
PROPOSAL:
Tests
Preconditions: web, one account, and either no
.envat all, whose defaults satisfy both values below, oran
.envwithUSE_NGROK=falseand anEXPENSIFY_URLthat does not containdev. Restartnpm run webafter editing
.env.Two entry points reach the same
ServerSelector, and both are exercised: Settings > Troubleshoot, and thetest tools modal, opened with Cmd+D on macOS or Ctrl+D elsewhere (
CONST.KEYBOARD_SHORTCUTS.DEBUG) andreachable signed out.
The root check, used below: open DevTools > Network, type
api/in the filter box, open any report tofire a request, click the newest row, and read the request URL in the Headers tab. Staging is
/staging/api/OpenReportwith the web proxy on orhttps://staging.expensify.com/api/OpenReportwith itoff; production is the same path without the
stagingprefix. Equivalently, without opening DevTools,run
performance.getEntriesByType('resource').filter((e) => e.name.includes('/api/')).map((e) => e.name)in the console and read the same URLs.
Test 3 is regression coverage for the behaviour the old toggle had. Tests 1, 2 and 4 exercise what this PR
adds. Test 5 is optional and developer-only.
The row replaces the toggle at both entry points, signed in and signed out
("Production" on a default config) with a right-facing chevron.
Staging, each with a description and no QA row among them, and the current server selected. The
active server sorts first, so the order follows the selection rather than being fixed.
present there and opens the same list inside the modal.
session.
/connection-completein the URL bar, press Cmd+D, press the "Server" row,then press Cmd+D again, and verify that the whole modal closes and the page underneath is showing.
This is test 4.2 on the public navigator, which mounts the modal separately from the authenticated
one, and it runs here to reuse the signed-out session step 6 leaves behind.
Save commits the pick and closes the page, and leaving without Save discards it
behind it has not changed: the pick commits on Save, not on row tap, so leaving by the back caret
discards it.
"Server" row reading "Staging".
and the test tools menu is showing with the row reading "Production".
A production to staging switch moves the API root and signs nobody out
page happens.
earliest
Pingcalls are addressed to the production root and that everything fromReconnectApponward goes to staging. An unread preference is indistinguishable from an unset one, so it stands in
as production until Onyx hands the stored value back, which stops an environment that defaults to
staging from ignoring an opt-out.
root.
The test tools modal closes from the selector
it.
is showing rather than the modal being dismissed.
Optional, developer-only: a pinned build states its server instead of offering one
Only a QA-environment build is pinned, so hard-code the environment for one run in
src/hooks/useActiveServer.ts, and setQA_EXPENSIFY_URLin.envso the QA config is present.The file does not import
CONST, so both lines are needed:Reload and open Settings > Troubleshoot. Verify that the "Server" row reads "QA", has no
chevron, and does nothing when pressed.
Load
/settings/troubleshoot/serverin the URL bar, which is the only way to the page on a pinnedbuild: the test tools modal still shows the row, but renders it without a press handler. Verify that
no row responds to a press, the Save button is absent, and the copy "This build always talks to one
server, so it cannot be changed here." is shown above the list. A pinned QA build lists QA alongside
Production and Staging, since the selector appends the active server when the offered set omits it.
Run
Onyx.set('activeServer', 'staging')in the browser console, and verify that QA is stillthe selected row.
Revert both changes from step 1, the hard-coded environment and the
QA_EXPENSIFY_URLentry,before committing.
The sign-out on a QA crossing has no step here: it needs the QA build target that part 4 supplies, so
nothing on this branch can reach QA.
Offline tests
N/A: this PR changes which host a request is addressed to and adds a settings page, and it touches no queued
write, no optimistic update, and no persisted data beyond the
activeServerkey the selector writes. Ittriggers no sign-out and sends no request of its own.
QA Steps
Server" toggle.
Staging in either order, each with a description below its label.
and you are still signed in.
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
logged.out.switch.mp4
logged.in.switch.mp4