[native] Fix server list in Dev tools menu
ClosedPublic
Actions

Authored by bartek on Jan 14 2023, 9:12 AM.

Details

Reviewers

ashoat
atul

Commits

rCOMM91ee903bac9d: [native] Fix server list in Dev tools menu

Summary

On physical devices, the Profile -> Developer tools -> Server list was displaying the localhost as one of default options.

In url-utils.js, the server list was out of sync with rest of the code. It duplicated part of getDevServerHostname() logic, but it used the platform-specific localhost only.

Replaced the duplicated logic with getDevServerHostname() call.

Depends on D6268

Test Plan

Opened the developer options on both simulator and physical device. Ensured that second option from the list is as follows:

On simulator it sticks to localhost / 10.0.2.2(Android)
On physical device it uses the autodetected IP address

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Jan 14 2023, 9:12 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptJan 14 2023, 9:12 AM

Harbormaster completed remote builds in B15375: Diff 20965.Jan 14 2023, 9:24 AM

bartek published this revision for review.Jan 14 2023, 9:28 AM

It looks like this might be done on purpose - the "Custom server" input seems to default to the server with the natDevHostname address.

I might abandon this change

Nice catch!! Not sure why we did this wrong initially

This revision is now accepted and ready to land.Jan 14 2023, 5:19 PM

ashoat mentioned this in D6249: [docs] Fix keyserver port numer, add instructions for connecting from physical device.Jan 16 2023, 7:59 PM

Closed by commit rCOMM91ee903bac9d: [native] Fix server list in Dev tools menu (authored by bartek). · Explain WhyJan 19 2023, 12:03 AM

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMM91ee903bac9d: [native] Fix server list in Dev tools menu.

This diff had three effects:

Intended effect: dev builds on physical devices now use the IP-detecting logic for the options they display in nodeServerOptions
Unintended effect: release builds now crash because they call getDevNodeServerURL
Unintended effect: release builds (for those in staff list that can access "Developer tools") now display the default "192.168.1.1" as an option instead of "localhost". It was previously being shown as the third "custom" option, so now there are two results that seem "duplicated" (even though the custom one can be customized)

I just put up D6332 to address unintended effect #2. As for #3, it's probably fine... either way there's probably no reason to show options like localhost and 10.0.2.2 for physical devices. If we wanted to avoid the duplicate in this case, we could update the code to avoid including getDevNodeServerURL() when !devIsEmulator.