Refactor WebViewActivity path handling#6447
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? |
- 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. |
|
Pushed three commits addressing predictive-back end-to-end (both code paths):
Tested on Android 16 emulator (API 36.1), gesture navigation, both code paths: Legacy WebViewActivity (USE_FRONTEND_V2 = false): predictive-back works on root, normal webView.goBack() inside the app. |
The OnBackPressedCallback in WebViewActivity was kept enabled whenever the loaded URL had a non-root path, which caused Android to assume the app would handle the back gesture and suppress the predictive-back peek animation — even on the root screen with no browser history. Drop the hasNonRootPath() fallback in doUpdateVisitedHistory so the callback is only enabled when webView.canGoBack() is true. With the earlier resolveBackAction guard requiring previousUrl != null, NavigateToRoot is now produced only when the WebView genuinely has back-stack entries — so the fallback was redundant and only served to keep the callback armed against predictive-back.
jpelgrom
left a comment
jpelgrom
left a comment
There was a problem hiding this comment.
The back animation finally appears to work consistently! Thanks for your patience in trying to fix it every time 🙏
WebView is now crashing (natively) quite consistently for me when switching networks, both on the emulator with v133 and a real device with v147, making this hard to test. That isn't new and also happening with the main app. However, it may be related for:
I see the NavigateToRoot action in code (which I still dislike), but never seem to get it anymore after switching networks. In other words: after switching networks it always acts as if there is no history and closes the app. Fine by me but not your intention? Or if it is why is NavigateToRoot still in the code? (Tested both with manually switching between Wi-Fi and mobile data using the quick setting toggle, and in the emulator with adb shell "svc wifi enable"/adb shell "svc wifi disable".)
| // same page, including filtered views like history with date ranges. | ||
| // Skipped for server switches where the path may not exist and would leak | ||
| // navigation context from the previous server. | ||
| withContext(Dispatchers.Main) { view.getCurrentWebViewRelativeUrl() } |
There was a problem hiding this comment.
Is withContext(Dispatchers.Main) necessary? If yes I'd move it to inside the view (activity) function as this shouldn't be the presenter's concern.
|
Short video showing my test for the issue described above: it keeps the correct path when switching (note the network icon on the emulator, it reloads but keeps the set date!) but back exits the app. Screen.Recording.2026-05-11.at.22.40.54.mov |
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