android icon indicating copy to clipboard operation
android copied to clipboard

Refactor connection state management

Open TimoPtr opened this issue 2 months ago • 4 comments

Summary

This PR introduces a major refactoring of how the app manages server connection state, along with the block insecure HTTP connections when not on the home network.

Key changes:

  1. New ServerConnectionStateProvider - A reactive provider that determines the appropriate URL based on current network conditions (internal vs external), manages security checks, and emits updates when network conditions change. This replaces the scattered URL resolution logic previously in ServerConnectionInfo.

  2. Refactored ServerConnectionInfo - Moved URL parsing logic to lazy cached HttpUrl properties and extracted connection state logic to ServerConnectionStateProvider. And it now contains allowInsecureConnection instead of the local storage, it is more relevant and make the logic easier to pick a URL.

  3. Replace all the usage of getUrl with the new flow when it makes sense. Try to uniform how we decide to pick a URL instead of doing differently in different places (even in the webview itself)

Checklist

  • [x] New or updated tests have been added to cover the changes following the testing guidelines.
  • [x] The code follows the project's code style and best_practices.
  • [x] The changes have been thoroughly tested, and edge cases have been considered.
  • [x] 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

Ui is not yet fully ready and tested. Some TODO are still in the code and might need to be address otherwise we need to removed them. This is a major change and might introduce regressions.

I've changed how we open the integration after Improv suggest since I wanted to remove the getUrl and it was easier to simply use NavigateTo and improve the experience of the user.

I've used Claude to help me generate an extensive set of tests since it's a critical piece of the app I want it to be fully tested. It introduce the usage of nested test and it is quite nice when running inside the IDE to create categories. I've on purpose didn't make all the tests Parametrized to try to keep the name meaningful as much as possible. Claude was going to hard on this and making tests unreadable.

Important: This PR is the first one of the refactor. A second one is needed to change the usage of getApiUrls since today it doesn't take in account the allowInsecure flag. Another one can be made later once everything is merged to handle more state while getting the URL to actually handle better when network in unavailable for the WebView and WebSocket for instance.

I don't like the name UrlState if you have a better name please share it.

TimoPtr avatar Dec 05 '25 17:12 TimoPtr

Ui is not yet fully ready and tested

What UI are you referring to? The PR description makes it sound like there are no UI changes.

jpelgrom avatar Dec 05 '25 17:12 jpelgrom

Ui is not yet fully ready and tested

What UI are you referring to? The PR description makes it sound like there are no UI changes.

The base of this PR is the PR of the blocking UI. And it needs some adjustments on top of the fact that I didn't test properly if it works as it should outside of the happy path.

TimoPtr avatar Dec 05 '25 17:12 TimoPtr

Test Results

104 files  +15  104 suites  +15   7m 58s ⏱️ +5s 933 tests +98  933 ✅ +98  0 💤 ±0  0 ❌ ±0  946 runs  +98  946 ✅ +98  0 💤 ±0  0 ❌ ±0 

Results for commit 1d003d29. ± Comparison against base commit 2b7a5a2e.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 05 '25 18:12 github-actions[bot]

https://github.com/user-attachments/assets/11a6586a-79da-4f2e-a18a-548da2e8cdcb

The improv flow using navigate instead of opening the URL again.

TimoPtr avatar Dec 09 '25 13:12 TimoPtr