Fix: BridgeComponents and sync iOS URL after frame navigations #230
Conversation
| if controller.visitableURL != newUrl { | ||
| controller.visitableURL = newUrl | ||
| } |
There was a problem hiding this comment.
- Shouldn't we notify the JS side that the url has been changed by Turbo? I guess this is fine when you do the
replaceaction, but what about theadvance? Don't know if this is even possible with Turbo. - This feels hakish -> in this case there should be a small comment explaining this or a link to a GH issue.
There was a problem hiding this comment.
Shouldn't we notify the JS side that the url has been changed by Turbo? I guess this is fine when you do the
replaceaction, but what about theadvance? Don't know if this is even possible with Turbo.
We could investigate if it'd make sense to trigger the onLoad prop on the JS-side.
The change here with KVO ensures visitableURL on the iOS is kept in-sync, so that pull-to-refresh will refresh the URL correctly after history.push(..) (eg. via Turbo Frame JS navigations on-site that do not go through the normal Turbo advance or replace flow)
On Android, the pull-to-refresh works without similar change. If we trigger onLoad on history changes on iOS, we probably want similar behavior on Android. I haven't been able to figure out how we could get similar behavior on Android though without resorting to some exotic solutions. One solution on Android is to monkey-patch history.push by injecting some JS into the WebView, but this feels very hackish.
This` feels hakish -> in this case there should be a small comment explaining this or a link to a GH issue.
I think the solution is actually quite elegant, but agree that we probably need to add some comments as to what this does and why.
There was a problem hiding this comment.
I’ll add a clarifying comment explaining the motivation behind it.
Good point on triggering onLoad from the native side to notify JS — that could help keep things better in sync. Do you think it makes sense to explore that in this PR, or should we track it as a follow-up so we can keep this focused on fixing the pull-to-refresh issue first?
There was a problem hiding this comment.
Can be tracked as a follow-up.
| if controller.visitableURL != newUrl { | ||
| controller.visitableURL = newUrl | ||
| } |
There was a problem hiding this comment.
Can be tracked as a follow-up.
Summary
This PR provides a comprehensive fix for issues with Strada Bridge Components that occur after Turbo Frame navigations or redirects. It ensures that components remain active and that native iOS state remains synchronized, correcting functionality like pull-to-refresh.
This PR addresses two problems - #228 and iOS Pull-to-Refresh regression. To fix the iOS regression, this PR introduces an approach similar to what was mentioned in #227 - it has been resolved by adding a Key-Value Observer to the WKWebView's url property in RNVisitableView.swift. This restores pull-to-refresh and ensures native state consistency.
Test plan
We can test both Bridge components and pull-to-refresh functionalities work as expected by using the specific branch of the demo server.
This branch of the demo web app includes Strada Components flow that can be used to test them.
This can be checked out and the demo app pointed to the local running version of the test server.
Demo
Functionalities tested on this version of demo web app.
demo.mov