Refactor WebViewActivity path handling#6447
Refactor WebViewActivity path handling#6447Gifford47 wants to merge 22 commits intohome-assistant:mainfrom
Conversation
|
@TimoPtr can you please have a look on it? you have additional webview PRs ... maybe there's conflict between the PRs ... |
TimoPtr
left a comment
TimoPtr
left a comment
There was a problem hiding this comment.
Thanks for your work, did you test it in multiple conditions? Like changing quickly the URL multiple times?
Also yes I'm currently working on making a new version of this WebViewActivity, it is going to take some time but you can already look at how it looks #6386
|
Also check what is happening when having arguments in the url like some filtered on the history page. |
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Thanks for the thorough review @TimoPtr! I've addressed your feedback: Duplicate path retrieval: Removed getCurrentWebViewPath() from the Activity — the Presenter already handles this fallback in collectUrlStateChanges(), so the Activity now only passes the intentPath (or null). intent.removeExtra(EXTRA_PATH): Moved before presenter.load() so the extra is consumed immediately. moreInfoEntity assignment: Fixed the behavioral change — removed the unconditional raw assignment. Now only sets moreInfoEntity to the regex-validated entity for older servers (JS dispatch path), and explicitly clears it when using the URL path approach (>= 2025.6). Exception handling in getCurrentWebViewPath(): Removed the broad catch (Exception) — Uri.parse() is lenient and doesn't throw, so the try-catch was unnecessary. Path extraction simplification: Replaced with webView.url?.toUri()?.path?.takeIf { it.length > 1 } as suggested. Comment reference: Updated to use the full GitHub issue URL. Regarding URL arguments (e.g. filters on the history page): Uri.path only returns the path component and excludes query parameters, so filtered views like /history?entity_id=sensor.foo will correctly preserve just /history as the path — the frontend will reload with its default state for that view, which is the expected behavior when switching between internal/external URLs. |
Actually it might be nice to keep the whole URI and just change the host/port imagine you are on It would be nice to only change the host/port |
|
The issue would be that most probably you are going to loose the history and if it is not the case I wonder what happens. Did you try to play with the app to see how it behaves for the history? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Changes
Added getCurrentWebViewRelativeUrl() which extracts the full relative URL (path + query parameters + fragment) from the current WebView URL, stripping the external_auth parameter since the presenter re-adds it on every load.
Added lastBaseUrl tracking in collectUrlStateChanges to detect when the base URL changes (internal ↔ external).
After history is cleared, rapid urlFlow emissions can create stale history entries from the old connection. The back button handler now validates the origin of the previous history entry using WebBackForwardList before navigating back. Tested: WiFi → mobile data switch on Lovelace tabs: URL switches correctly, page preserved |
|
@TimoPtr all tests are successfully done. Do you have any hints? |
Track the last base URL in collectUrlStateChanges so a connection-type switch reuses the current WebView relative URL instead of reloading the root. Tighten resolveBackAction tests to call the pure function directly.
|
Yes, I used the agent to thoroughly analyze the logic and review the code. However, the new logic was developed by me and has been tested by me as well. In fact, I spent the past two weeks carefully re-testing everything to ensure it works reliably: Pure function extraction - Split resolveBackAction into a WebView convenience overload that extracts the previousUrl from the back/forward list, and a pure VisibleForTesting internal function that takes (previousUrl: Uri?, loadedUrl: Uri?). This makes the logic testable without mocking WebView. All tests now call the pure function directly - no more MockK for WebView/WebBackForwardList. Uri.parse() -> .toUri() - Replaced with the Kotlin extension as suggested. Exit -> None - Good call, renamed to BackAction.None. The caller decides what to do - the function just says "I can't handle this". Updated all callsites. clearHistory -> keepHistory - Reverted the rename, now using keepHistory directly to avoid the inverted !clearHistory logic. presenter.load() vs webView.loadUrl() for NavigateToRoot - I intentionally use webView.loadUrl() here because presenter.load() does a full reconnect cycle (resolve URL, auth check, security level). For NavigateToRoot we're staying on the same server and just changing the path to /, so going through the presenter would add unnecessary overhead and could trigger unwanted side effects like the security level dialog. @jpelgrom's question about hasNonRootPath in doUpdateVisitedHistory - Sorry I missed this one. The OnBackPressedCallback needs to be enabled not just when canGoBack() is true, but also when the user is on a sub-path like /history. Without this, pressing back on a sub-path with empty history would trigger the system back handler (exit app) instead of first navigating to root. This also keeps the predictive back animation working correctly on Android 14+ - the system needs to know in advance that we'll handle the gesture. Test naming - Updated all test names to Given/When/Then convention. Duplicate test - Removed the duplicate "Exit when on root path" test (was identical to "no previous url and root loaded url"). Also added a new test for the about:blank edge case - verifying that a non-HTTP previousUrl doesn't trigger GoBack. |
| fun Uri.toRelativeUrl(excludeParams: Set<String> = emptySet()): String? { | ||
| val path = encodedPath?.takeIf { it.length > 1 } ?: return null | ||
|
|
||
| val relativeUrl = Uri.Builder() | ||
| .encodedPath(path) | ||
| .apply { | ||
| queryParameterNames | ||
| .filterNot { it in excludeParams } | ||
| .flatMap { name -> getQueryParameters(name).map { name to it } } | ||
| .forEach { (name, value) -> appendQueryParameter(name, value) } | ||
| } | ||
| .encodedFragment(encodedFragment) | ||
| .build() | ||
| .toString() |
There was a problem hiding this comment.
queryParameterNames is a Set, so iteration order isn’t guaranteed; this can make toRelativeUrl() output nondeterministic (and can cause flaky tests or inconsistent navigation URLs) when multiple query params exist. Consider preserving the original query order by parsing encodedQuery (filtering out excluded param names while keeping ordering/encoding), or at minimum sorting parameter names before appending to ensure deterministic output.
- BackAction: inline previousUrl lookup (TimoPtr suggestion) - WebViewPresenterImpl: simplify keepHistory expression with De Morgan - WebViewPresenterImpl: add unit test for base URL change path preservation - WebViewActivity: expand doUpdateVisitedHistory comment with predictive back rationale - WebViewBackNavigationTest: capitalize Given to match project convention - ktlint: rename WebViewBackNavigation.kt -> BackAction.kt, fix import order
- BackAction: make inner resolveBackAction private, move resolution KDoc to public function so it shows at call sites - WebViewPresenterImpl: gate baseUrlChanged path preservation with !isNewServer (Copilot bug: prevents leaking path across server switches) - WebViewPresenterImpl: simplify baseUrlChanged using lastBaseUrl nullability - WebViewBackNavigationTest: drop Robolectric in favor of mockk-based WebView mocks, exercise logic through the public resolveBackAction overload
- BackAction: guard against currentIndex == 0 in previousUrl lookup - WebViewActivity: drop redundant moreInfoEntity = "" reset in the URL-path branch; moreInfoEntity is never set in this branch, so clearing it is unnecessary
Did you already test the feature? On my side everything works as expected. I tested it for some days in my productive environment. |
I honestly didn't because I don't have a test setup with internal/external URL. I want to test this once it's merged because we have an internal lane on the play store. I focused on the code itself for now. |
|
The switching now mostly works from a technical point of view and preserves the URL when switching between networks on the same server, and doesn't when switching servers. However I have two issues:
Before: shortcut-pre.mp4After (this also shows the animation issue): shortcut-post.mp4(I'm using energy as an example as I'm not that much into dashboards myself, but you can imagine people having shortcuts to specific dashboards and/or views who get this extra step now.) |
…eToRoot guard Require previousUrl != null for BackAction.NavigateToRoot so that pressing back with an empty back-stack (e.g. opened via deeplink to /history) returns None instead of synthesizing a navigation to root. This aligns resolveBackAction with jpelgrom's concern about losing Android's predictive-back peek animation.
|
I've spent some time thinking about a solution for the keepHistory issue. Logic:
Fixes both points:
Touched files: new If the direction works for you I'll prepare the changes. |
I'm not a big fan to try to keep a secondary history. Especially that if you are deep into the app and switch URL each back would reload the app because the origin would be different, so showing the loader every back. I invite you to join the discord discussion with the product people to decide where to go with this. Since it's mostly a UX question not really technical. |
|
Even with the latest changes, predictive back doesn't work at all in my testing. Likely because the history doesn't start with My WebView has decided to start doing native crashes when changing network somewhat regularly, making testing quite inconvenient. It does seem that when it doesn't crash it is never inserting a navigation to the default dashboard anymore though. |
Summary
This PR refactors how paths are handled in the WebView to ensure navigation keeps the current path and exposes it reliably.
Key Changes
WebViewPresenterImpl.kt,WebViewActivity.kt,WebView.kt.getCurrentPath()→ retrieves the active WebView path.Why
Impact
getCurrentPath()can be used wherever current WebView state is needed.Checklist
Screenshots
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#
Developer Documentation: home-assistant/developers.home-assistant#
Any other notes
See issue #4983