Display LocalFirstScreen only when necessary #5933

TimoPtr · Oct 14, 2025

Summary

We want to avoid displaying the LocalFirstScreen when it doesn't make sense, like when we are sure that the URL used is publicly accessible. This PR make sure of that by displaying the screen only when it makes sense.

It address the feedback from @dshokouhi #5909 (comment)

Checklist

New or updated tests have been added to cover the changes following the testing guidelines.
The code follows the project's code style and best_practices.
The changes have been thoroughly tested, and edge cases have been considered.
Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Any other notes

It needs to be merged after #5912 to avoid conflicts.

bgoncal

LGTM

jpelgrom

Not sure if you wanted to do this in another PR, but to fully address the feedback, the messaging should also be changed for plaintext + publicly accessible (like http://home.example.com, or simple port forwarding without SSL).

jpelgrom · Oct 14, 2025

common/src/main/kotlin/io/homeassistant/companion/android/util/UrlUtil.kt

+        val addresses = withContext(Dispatchers.IO) {
+            InetAddress.getAllByName(fqdn)
+        }
+        addresses.none { it.isPrivateOrLocal() }


What about people using a public FQDN, but running an internal DNS server so it resolves to the internal IP when at home (with a short TTL so it is refreshed frequently)? I've seen that more than once. Maybe a simple check based on TLD is enough.

Well I think it makes sense when the call is actually made, for such setup ppl "know" what they are doing and most probably they have HTTPS.

But some ppl have static entry in their local DNS for a domain that is not registered externally.

were probably still going to have cases where users who feel shouldnt see this screen still do

common/src/test/kotlin/io/homeassistant/companion/android/util/UrlUtilTest.kt

...tlin/io/homeassistant/companion/android/onboarding/nameyourdevice/NameYourDeviceViewModel.kt

TimoPtr · Oct 15, 2025

Not sure if you wanted to do this in another PR, but to fully address the feedback, the messaging should also be changed for plaintext + publicly accessible (like http://home.example.com, or simple port forwarding without SSL).

@bgoncal suggested to drop the last sentence in this screen

If the users agrees to share his location in any case we are going to choose Most secure for him and if he doesn't we are going to show the LocationForSecureScreen where we explicitly say that the connection is unencrypted.

I'm not sure we need to add a mention on this screen wdyt? (both of you @bgoncal and @jpelgrom)

bgoncal · Oct 15, 2025

I don't think it's needed. Keep it simple

bgoncal · Oct 15, 2025

I created a PR for iOS similar to this but I will leave DNS resolution out of the equation until I get feedback that it's needed home-assistant/iOS#3900

jpelgrom · Oct 15, 2025

Not sure if you wanted to do this in another PR, but to fully address the feedback, the messaging should also be changed for plaintext + publicly accessible (like http://home.example.com, or simple port forwarding without SSL).

@bgoncal suggested to drop the last sentence in this screen

But it still mentions "only accessible on your home network", "set up remoe access later", while it is remotely accessible. This is not going to be fixed by removing/adding one sentence.

@dshokouhi since it is based on your feedback, this is what I find confusing but maybe you have more/other comments

bgoncal · Oct 15, 2025

That will be "handled" by the check that @TimoPtr made to detect if it's internal or external URL.

The endgame is to always receive from core what is the internal and what is the remote for that server, but we don't have that yet.

jpelgrom · Oct 15, 2025

That will be "handled" by the check that @TimoPtr made to detect if it's internal or external URL.

Which check are you referring to? I don't recognize anything like that in the code changes or are able to get it to behave differently, and this PR does not depend on another branch.

bgoncal · Oct 15, 2025

Perhaps I misunderstood, Timo can confirm tomorrow but in my mind the check for if the URL is publicly available was to determine during onboarding if we are onboarding a local or remote URL.

In iOS specifically this means that we put the URL in the correct "place" in the configuration, as you know in iOS we call internal/external URL.

Given the above we could determine if we need to display this disclaimer screen or not.

jpelgrom · Oct 15, 2025

Perhaps I misunderstood, Timo can confirm tomorrow but in my mind the check for if the URL is publicly available was to determine during onboarding if we are onboarding a local or remote URL.

In iOS specifically this means that we put the URL in the correct "place" in the configuration, as you know in iOS we call internal/external URL.

That may be the goal, but is not what the PR is doing. There is also no such thing as only having an "internal" URL in the Android app architecture so that would require bigger changes which are definitely out of scope of this PR.

Given the above we could determine if we need to display this disclaimer screen or not.

... but we're not there yet, the reason mentioned is a specific comment from @dshokouhi and I agree that it is somewhat confusing to see this screen when using something like http(s)://home.example.com :)

dshokouhi · Oct 15, 2025

Had a chance to test the PR, at least for my setup I no longer see the screen which is what I would expect. Generally speaking I dont think we will be handle to every check to ensure this doesnt show up for every user and every configuration. Handling the majority probably makes the most sense.

bgoncal · Oct 15, 2025

@jpelgrom In that case I'll leave for @TimoPtr to take over the discussion, if it can't be determined if remote access exist (in a best effort manner), then this screen shouldn't be shown at all in Android.

But the way I see it, a quick fix is:

Onboarding coming from mdns discovery? No cloud URL fetch from core? Show the screen.
Onboarding coming from manual entry URL and detected that is a remote URL? (doesn't need to be 100% accurate, covering most cases is good enough) Do not show the screen

Not saying this PR does that, that's just my opinion on what should happen.

TimoPtr · Oct 16, 2025

I think all this confusion can be lift off by simplifying the condition. Today we show this screen if it plaintext OR if it is publicly accessible. We do have another screen later to notify the user if the connection is not encrypted (LocationForSecureConnection and later the "blocking" screen). I propose to simply the condition to show this screen to only when we know it is a non public address.

Yes it means that ppl with a complex setup like changing the DNS entry using a short TTL are going to see this screen. But technically this screen is true the user is onboarding on a private address, I don't think it is going to bother the user to see this screen. Mostly because he knows his setup. It is going to be an issue for a secondary user, but I would assume that in that case the secondary user is going to ask the admin to help him setup remote access.

WDYT @bgoncal @jpelgrom @dshokouhi ?

bgoncal · Oct 16, 2025

Looks good to me, my main concern is the average user.

jpelgrom · Oct 16, 2025

Agreed we should focus on average. Simplifying it to only show if it's a non-public address works for me to as a solution to avoid confusion about "only accessible on your home network" 👍

jpelgrom · Oct 16, 2025

common/src/test/kotlin/io/homeassistant/companion/android/util/UrlUtilTest.kt

+
+    @Test
+    fun `Given unresolved TLD when checking isPubliclyAccessible then returns false`() = runTest {
+        val url = URL("http://this-domain-definitely-does-not-exist-12345.com:8123")


If someone wants to troll us they'll buy the domain

home-assistant bot added cla-signed Hacktoberfest labels Oct 14, 2025

TimoPtr requested review from bgoncal and jpelgrom October 14, 2025 14:05

bgoncal approved these changes Oct 14, 2025

View reviewed changes

jpelgrom reviewed Oct 14, 2025

View reviewed changes

Base automatically changed from fix/onboarding-back-after-name-device to main October 15, 2025 07:29

TimoPtr added 3 commits October 15, 2025 09:30

Introduce usesPublicFqdn

bc683e2

Skip LocalFirstScreen when server uses publicly url using encryption

3f692d4

Apply feedbacks

9d9eb73

TimoPtr force-pushed the feature/limit-display-of-local-first branch from def75b1 to 9d9eb73 Compare October 15, 2025 07:37

TimoPtr requested a review from jpelgrom October 15, 2025 07:41

Remove plaintext condition to show local first screen

7de2103

jpelgrom approved these changes Oct 16, 2025

View reviewed changes

jpelgrom reviewed Oct 16, 2025

View reviewed changes

TimoPtr merged commit d90e19c into main Oct 16, 2025
35 of 38 checks passed

TimoPtr deleted the feature/limit-display-of-local-first branch October 16, 2025 19:55

Search code, repositories, users, issues, pull requests...

Uh oh!

Display LocalFirstScreen only when necessary #5933

Display LocalFirstScreen only when necessary #5933

Uh oh!

Conversation

TimoPtr commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Any other notes

Uh oh!

bgoncal left a comment

Choose a reason for hiding this comment

Uh oh!

jpelgrom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpelgrom Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

TimoPtr Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

dshokouhi Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimoPtr commented Oct 15, 2025

Uh oh!

bgoncal commented Oct 15, 2025

Uh oh!

bgoncal commented Oct 15, 2025

Uh oh!

jpelgrom commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgoncal commented Oct 15, 2025

Uh oh!

jpelgrom commented Oct 15, 2025

Uh oh!

bgoncal commented Oct 15, 2025

Uh oh!

jpelgrom commented Oct 15, 2025

Uh oh!

dshokouhi commented Oct 15, 2025

Uh oh!

bgoncal commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimoPtr commented Oct 16, 2025

Uh oh!

bgoncal commented Oct 16, 2025

Uh oh!

jpelgrom commented Oct 16, 2025

Uh oh!

jpelgrom Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TimoPtr commented Oct 14, 2025 •

edited

Loading

jpelgrom left a comment •

edited

Loading

jpelgrom commented Oct 15, 2025 •

edited

Loading

bgoncal commented Oct 15, 2025 •

edited

Loading