Skip to content

FrontendScreen filter out messages not coming from the frontend external bus#6687

Merged
TimoPtr merged 10 commits intomainfrom
feature/frontend_screen_only_allow_main_frame
Apr 13, 2026
Merged

FrontendScreen filter out messages not coming from the frontend external bus#6687
TimoPtr merged 10 commits intomainfrom
feature/frontend_screen_only_allow_main_frame

Conversation

@TimoPtr
Copy link
Copy Markdown
Member

@TimoPtr TimoPtr commented Apr 8, 2026

Summary

Same as #6680 but on the FrontendScreen, it also add proper testing. I had to rework the bridge pattern to make it more coherent with the switch between the two version.

  • I've changed how FailFast.failWhen to avoid duplication of code
  • I've extended the Json UnknownJsonContent to contains the discriminator
  • I've shared some const with the WebViewActivity to avoid duplication
  • I've moved some logic out of the repository into the bridge and do a first step of JSON parsing in the bridge

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

Based on #6680

I found out a small bug in the FrontendViewModel.urlFlow that if we call .value on it without any subscriber even if the state contains a URL it returns null, I'm going to address this into another PR.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the “only react to messages coming from the frontend external bus” work to FrontendScreen by introducing a revised JS bridge (V1 + V2) that parses/validates messages earlier, adds origin/iframe filtering for V2, and updates related plumbing + tests.

Changes:

  • Introduces a new frontend/js/FrontendJsBridge with V1 (addJavascriptInterface) + V2 (WebMessageListener) support and shared parsing/dispatch logic
  • Switches external bus handling to pass JsonElement (instead of raw JSON strings) and enriches unknown-polymorphic payloads with a discriminator for better diagnostics
  • Updates FailFast.failWhen to return the checked condition, enabling guard-style usage, and adjusts tests accordingly

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
common/src/test/kotlin/io/homeassistant/companion/android/common/util/FailFastTest.kt Updates unit tests for new FailFast.failWhen boolean return value
common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt Fixes serializer module naming and extends UnknownJsonContent with discriminator
common/src/main/kotlin/io/homeassistant/companion/android/common/util/FailFast.kt Makes failWhen return the evaluated condition for inline-guard usage
common/src/main/kotlin/io/homeassistant/companion/android/common/data/websocket/impl/entities/SocketResponse.kt Renames serializer module and propagates discriminator into unknown socket responses
app/src/test/kotlin/io/homeassistant/companion/android/util/HAWebViewClientTest.kt Updates test after removing JS callback reattachment from HAWebViewClient
app/src/test/kotlin/io/homeassistant/companion/android/onboarding/connection/ConnectionViewModelTest.kt Updates mocks/signatures after HAWebViewClientFactory.create API change
app/src/test/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsBridgeTest.kt Adds comprehensive tests for V1/V2 bridge attach + dispatch + validation
app/src/test/kotlin/io/homeassistant/companion/android/frontend/handler/FrontendMessageHandlerTest.kt Updates tests for typed payloads and JsonElement external bus input
app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModelTest.kt Updates ViewModel construction to inject FrontendJsBridgeFactory
app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenTest.kt Updates imports for new JS bridge location
app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendJsBridgeTest.kt Removes old bridge test that targeted the pre-refactor bridge API
app/src/test/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepositoryImplTest.kt Updates external bus repository tests to use JsonElement inputs and adds new edge-case coverage
app/src/screenshotTest/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenScreenshotTest.kt Updates previews/tests to use FrontendJsBridge.noOp
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt Reuses bridge constants/logic and uses FailFast.failWhen guard pattern for callback validation
app/src/main/kotlin/io/homeassistant/companion/android/util/HAWebViewClient.kt Removes frontendJsCallback parameter and reattachment behavior on render-process crash
app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsHandler.kt Adds a typed handler interface for bridge callbacks
app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsCallback.kt Adds an attach-only callback interface for WebView bridge registration
app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsBridge.kt Adds new V1/V2 bridge implementation with origin/iframe filtering and early JSON parsing
app/src/main/kotlin/io/homeassistant/companion/android/frontend/handler/FrontendMessageHandler.kt Switches to typed auth payload + JsonElement bus messages; moves parsing responsibility to the bridge
app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModel.kt Creates the bridge via assisted factory and passes bridge state (serverId/url)
app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendScreen.kt Attaches the JS bridge in WebViewEffects right before loading the URL
app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendJsBridge.kt Removes the old single-protocol bridge implementation
app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/incoming/IncomingExternalBusMessage.kt Adds discriminator propagation for unknown incoming external bus message types
app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepositoryImpl.kt Changes repository input to JsonElement and decodes via decodeFromJsonElement
app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepository.kt Updates repository API to accept JsonElement instead of raw strings

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

Comment on lines 203 to 209

override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean {
Timber.e("onRenderProcessGone: webView crashed")
view?.let { webView ->
frontendJsCallback?.attachToWebView(webView)
onCrash?.invoke()
}
onCrash?.invoke()
return true
}

Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onRenderProcessGone indicates the WebView renderer process has died and the existing WebView instance is no longer safe to keep using. Currently the code only invokes onCrash and returns true, but does not destroy the WebView (or otherwise ensure it gets recreated), which can lead to repeated failures if the UI continues calling loadUrl on the same instance.

Consider explicitly destroying the view here and/or changing the crash callback contract so the UI can drop/recreate the WebView instance after a renderer crash.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpelgrom I don't want to change this here since I've replicated the existing behavior but maybe that's something to consider? I have no idea how to properly test this scenario.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We previously had a discussion about this in #3787 and I'm still not sure what the best path is here. Reloading seems to work despite the documentation suggesting otherwise. You can test with chrome://crash.

@TimoPtr TimoPtr force-pushed the feature/frontend_screen_only_allow_main_frame branch from 07390e1 to 87be04c Compare April 8, 2026 12:39
@TimoPtr TimoPtr marked this pull request as ready for review April 9, 2026 12:21
Base automatically changed from feature/only_allow_main_frame to main April 11, 2026 20:31
@jpelgrom
Copy link
Copy Markdown
Member

I think I made a Git mistake by merging the other one first, and now it says there is a merge conflict even though it was clean when stacked on top? (Thankfully this is used in debug only)

Copy link
Copy Markdown
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and reviewed. Good to merge after resolving the merge conflict.

@TimoPtr
Copy link
Copy Markdown
Member Author

TimoPtr commented Apr 11, 2026

I think I made a Git mistake by merging the other one first, and now it says there is a merge conflict even though it was clean when stacked on top? (Thankfully this is used in debug only)

It's fine it's just a rebase to do since it was based on the other branch. I'll handle it Monday

@TimoPtr TimoPtr force-pushed the feature/frontend_screen_only_allow_main_frame branch from 87be04c to c6279f3 Compare April 13, 2026 08:57
@TimoPtr TimoPtr enabled auto-merge (squash) April 13, 2026 09:00
@TimoPtr TimoPtr disabled auto-merge April 13, 2026 09:35
@TimoPtr TimoPtr merged commit 9b0b469 into main Apr 13, 2026
22 of 23 checks passed
@TimoPtr TimoPtr deleted the feature/frontend_screen_only_allow_main_frame branch April 13, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants