Use more precise error and action for the ConnectionError - #7094
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the app’s “frontend connection error” model and UI so error subtypes are more precise (SSL vs auth revoked vs timeouts, etc.) and the error screen can offer context-appropriate recovery actions (refresh, settings, remove server, clear keychain, wait), aligning behavior with the legacy WebViewActivity.
Changes:
- Reworked
FrontendConnectionErrorinto more specific subtypes (e.g.,AuthRevoked,SslError,Timeout,ExternalBusTimeout, TLS client-cert cases) and updated all call sites/tests accordingly. - Introduced an action model (
ErrorActionIntent+errorActions) and hooked error-screen buttons intoFrontendViewModelviaonErrorAction, including a newFrontendEvent.Relaunchflow. - Ported the legacy “security version warning” snackbar behavior into
FrontendViewModel.
Reviewed changes
Copilot reviewed 23 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/main/res/values/strings.xml | Adds new button labels for error recovery actions (“Clear credentials”, “Remove server”) |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/error/FrontendConnectionError.kt | Redefines connection error types with icons/messages suited to each subtype |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/error/ErrorAction.kt | Adds ErrorAction model + errorActions() mapping from error subtype → ordered UI actions |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/error/ErrorActionIntent.kt | Adds intent-only representation of error-screen actions for ViewModel handling |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/error/FrontendConnectionErrorScreen.kt | Uses per-error icons and introduces a reusable ErrorActions button stack |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendScreen.kt | Replaces retry/settings buttons with ErrorActions driven by errorActions() + ViewModel intent handling |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendViewState.kt | Extends Error state with isInternalConnection for refresh label selection |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModel.kt | Adds error-action handling, resolves internal/external connection label, ports security warning snackbar, updates timeout error mapping |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/navigation/FrontendEvent.kt | Adds FrontendEvent.Relaunch for destructive recovery flows |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/navigation/FrontendNavigation.kt | Handles Relaunch by starting LaunchActivity and finishing the current activity |
| app/src/main/kotlin/io/homeassistant/companion/android/util/HAWebViewClient.kt | Maps WebView error callbacks to the new, more precise FrontendConnectionError subtypes |
| app/src/main/kotlin/io/homeassistant/companion/android/onboarding/connection/ConnectionViewModel.kt | Updates onboarding error emission to new error subtype names/constructors |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/session/ServerSessionManager.kt | Refines external auth failure classification (SSL vs timeout vs auth revoked) and adds SSL detection helper |
| app/src/test/kotlin/io/homeassistant/companion/android/util/HAWebViewClientTest.kt | Updates tests to assert new error subtype mapping from WebView errors |
| app/src/test/kotlin/io/homeassistant/companion/android/onboarding/connection/ConnectionViewModelTest.kt | Updates tests for renamed/reshaped error types |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/session/ServerSessionManagerTest.kt | Adds tests for SSL handshake + socket timeout mapping in external auth retrieval |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/handler/FrontendMessageHandlerTest.kt | Updates handler test to use AuthRevoked instead of old auth error type |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModelTest.kt | Updates error-type expectations and adds tests for error actions + security warning snackbar |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenTest.kt | Updates UI tests to validate action dispatch from error screen instead of “Retry” |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/error/FrontendConnectionErrorScreenTest.kt | Updates screen tests for new error types and action rendering/dispatch |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/error/ErrorActionsTest.kt | Adds unit coverage for errorActions() mapping and internal/external refresh label behavior |
| app/src/screenshotTest/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenScreenshotTest.kt | Updates screenshot setup for renamed error types |
| app/src/screenshotTest/kotlin/io/homeassistant/companion/android/frontend/error/FrontendConnectionErrorScreenshotTest.kt | Updates screenshots for new error types and adds per-subtype action previews |
|
Did you consider the actions from the WebViewActivity before re-implementing them? Now is our chance with more room for UI and custom logic. Looking at the screenshots what I notice is:
|
I did I think made a 1:1 mapping between the existing actions and the new one for each kind of error. Except the override to local that I think we should drop.
What is your suggestion just "refresh"?
I was hoping to discuss this in the PR I doesn't really like the design. If you have suggestions.
I'm going to look at how we can better look at this kind of issue, maybe we can reproduce the pattern we have in BlockInsecure with a small card with text and action. |
|
@jpelgrom I've added more actions on specific errors. |
jpelgrom
left a comment
There was a problem hiding this comment.
So many scenarios... I think I tested all but good to do a beta cycle or two in case something was missed after switching the implementation.
…d/navigation/FrontendEvent.kt Co-authored-by: Joris Pelgröm <jpelgrom@users.noreply.github.com>
| <string name="fail_to_navigate_to_uri">No app available to open %s</string> | ||
| <string name="security_settings">the device security settings</string> | ||
| <string name="system_webview">the system WebView</string> |
There was a problem hiding this comment.
Asking for translations, because grammar is much more complicated in German, e.g.:
These strings are combined into a single error message, I assume.
Any additional strings that are used for the second half of the sentence?
There was a problem hiding this comment.
It goes with the above string
<string name="fail_to_navigate_to_uri">No app available to open %s</string>
So something like
No app available to open the device security settings
No app available to open the system WebView
There was a problem hiding this comment.
Thanks for the quick reply. So no other strings or names that are used to create additional messages?
I'm asking because the first part of the sentence was already present in the strings.
There was a problem hiding this comment.
Not at the moment (sorry for the late response).
Summary
This PR is all about making the error screen replicate the old actions we had in the WebViewActivity. I've added screenshot for all the variants.
Checklist
Any other notes
I decided to not add the isInternalOverride since it is quite fragile and that the new error screen should help diagnostic better. Also we are always offering a way to reach the settings.