Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components in turbo-frame are being ignored due to url mismatch #19

Closed
mike-northsail opened this issue Oct 11, 2023 · 16 comments
Closed

Comments

@mike-northsail
Copy link

We have "tabs" in our app. Basic links that swap the contents of a turbo-frame. We have bridge components included in the content that gets dynamically loaded into the frame based on the tab you click. They're being ignored because the bridge's location doesn't match the metadata's url in bridgeDidReceiveMessage https://github.com/hotwired/strada-ios/blob/main/Source/BridgeDelegate.swift#L118 which appears to be the url from the link initiating the request, whereas the bridge location is still based on the initial page visit.

We tried this with the turbo-native-demo and experienced the same results with the menu controller.

What's the expectation here? Is there a way around this for the time being, or have we implemented something incorrectly? We don't really see anything in the documents around this. Just a comment in the strada-web library.

// Include the url with each message, so the native app can
// ensure messages are delivered to the correct destination

Thanks!

@joshmcarthur
Copy link

joshmcarthur commented Oct 11, 2023

I've also run into this with a response that redirects to a different URL - for example, in my application, the entrypoint URL for the Turbo webview is /dashboard, and that redirects to /users/log-in if there is not an active user session.

I've observed the same behaviour with components being ignored due to a URL mismatch after this redirect, since the URL has changed from /dashboard to /users-log-in without the bridge delegate location being updated.

I've been looking around at how I can update the location myself, but because of how Turbo navigation visits work, I don't think there's a direct WKWebView delegate method I can use, since the URL could be modified by Turbo via pushState, rather than an actual navigation. There are some call sites where I could update the BridgeDelegate.location property (e.g. in my TurboNavigationController#visit method, however the location is a let, not a var, so it can't be updated once set.

@joshmcarthur
Copy link

I've worked around this by forking and removing the assertion that the BridgeDelegate location matches the message metadata URL property. I then removed the package dependency and re-added it using my own repository. I originally tried extending or overriding BridgeDelegate, but the class being final prevents all the overrides I attempted.

I would like to arrive at a point where I can submit a PR for this problem, but I'm unsure of what the purpose of the guard is. I'm guessing it's to prevent messages being passed to a component that is part of the view hierarchy, but concealed by something else?

Keeping the location more up-to-date in the BridgeDelegate would be a solution, however there are several ways the location could change - it could be redirected, be navigated outside the webview (like using window.location or pushState), or like @mike-northsail ran into, could be a frame update. I think to track the location would require intercepting Turbo events in hotwired/strada-web, and passing this through to the BridgeDelegate somehow.

If it is important that the URL gets matched against, it is still possible to access this within each BridgeComponent's onReceive function - so if necessary, each component could guard or otherwise check that the component is running on the URLs it expects to be run on.

@jayohms
Copy link
Collaborator

jayohms commented Oct 18, 2023

They're being ignored because the bridge's location doesn't match the metadata's url in bridgeDidReceiveMessage https://github.com/hotwired/strada-ios/blob/main/Source/BridgeDelegate.swift#L118 which appears to be the url from the link initiating the request, whereas the bridge location is still based on the initial page visit.

This is surprising behavior. We heavily use turbo-frame elements and have not experienced this issue directly. You can see the metadata url is populated from window.location.href: https://github.com/hotwired/strada-web/blob/main/src/bridge_component.js#L48C12-L48C32

Is the document url changing after loading new turbo-frame content?

@mike-northsail
Copy link
Author

The strada-web side is fine, it's the stale url that the native bridge delegate is using to validate whether it should ignore the component or not. When we're updating the contents of a frame on an existing page, the bridge is never made aware of the url change.

@jayohms
Copy link
Collaborator

jayohms commented Oct 25, 2023

Can you enable debug logging in the strada-ios and turbo-ios libraries and paste a log of events? I'm curious to see where things are breaking down.

@joshmcarthur
Copy link

joshmcarthur commented Oct 26, 2023

I can, but I'll note I have a slightly different cause of this issue - for me, it's that turbo-ios doesn't seem to be aware of the current webview's updated URL after navigation.

Here's what I get from turbo-ios:

2023-10-26 00:11:56 +0000 [Session] visit ["options": Turbo.VisitOptions(action: Turbo.VisitAction.replace, response: nil), "reload": false, "location": http://localhost:3000/dashboard]
2023-10-26 00:11:56 +0000 [ColdBootVisit] startVisit http://localhost:3000/dashboard [:]
bridgeDestinationViewDidLoad: http://localhost:3000/dashboard
bridgeDestinationViewWillAppear: http://localhost:3000/dashboard
bridgeDestinationViewDidAppear: http://localhost:3000/dashboard

Then if I tap on the 'Continue with Google' button (which the BridgeComponent is bound to):

bridgeDidIgnoreMessage: Message(id: "1", component: "google-auth", event: "authorize", metadata: Optional(Strada.Message.Metadata(url: "http://localhost:3000/users/log-in")), jsonData: "{\"metadata\":{\"url\":\"http:\\/\\/localhost:3000\\/users\\/log-in\"}}")

If I add a breakpoint in BridgeDelegate#bridgeDidReceiveMessage, location is "http://localhost:3000/dashboard", while the URL in the message metadata (coming from strada-web), is "http://localhost:3000/users/log-in". If I print the BridgeDelegate's bridge's webView's URL, it is:

(lldb) p bridge?.webView?.url
(Foundation.URL?) "http://localhost:3000/users/log-in"

So it's like the location that the bridge is aware of is not following when navigation occurs. I've been patching out that guard completely, but now that I look at it, I wonder if I'd be better to check the webView URL at runtime when that guard check occurs instead.

Here's what my backend request logs look like:

Started GET "/dashboard" for ::1 at 2023-10-26 13:16:30 +1300
Processing by DashboardController#show as HTML
Completed 401 Unauthorized in 2ms (ActiveRecord: 0.0ms | Allocations: 307)


Started GET "/users/log-in" for ::1 at 2023-10-26 13:16:30 +1300
Processing by Users::SessionsController#new as HTML
...etc

This flow is based on the Turbo-iOS demo application, specifically catching the 401 response, and then prompting for authentication.

Just like the demo application, my sign-in route is configured to open with 'modal' presentation, so I have two sessions in my app - one for the main webview, and another for modals. The session initialisation code for both includes initializing the bridge:

public func makeSession() -> Session {
        let configuration = Self.makeWKWebViewConfiguration()
        let session = Self.makeSession(configuration: configuration)
        session.delegate = self
        session.webView.uiDelegate = self
        Bridge.initialize(session.webView)
        
        
        return session
    }

I hope this helps!

@mike-northsail
Copy link
Author

In our case it's as simple as including a turbo-frame in the turbo-native-demo index.ejs and to wrap the strada-menu.ejs in a corresponding turbo frame.

  <!-- index.ejs -->
  <turbo-frame id="demo-frame">frame waiting...</turbo-frame>
  <a class="actions__item" href="/strada-menu" data-turbo-frame="demo-frame" data-turbo-action="advance">
    <div class="actions__icon --modal" aria-hidden="true"></div>
    Menu example
  </a>

  <!-- strada-menu.ejs -->
  <turbo-frame id="demo-frame">
    ...
  </turbo-frame> 

We tried the suggestion from @joshmcarthur and when using the webView.url it allows us to keep the guard in place while still having the bridge delegate work as expected. Though at this time we're also using a version that has completely removed the guard.

@joemasilotti
Copy link
Member

Shot in the dark here… but can you print out the two URLs that are being compared in the guard statement? I'm wondering if a trailing slash is messing things up.

@mike-northsail
Copy link
Author

http://localhost:3000/clients/1 <- holds the turbo-frame and is the original url bound to the bridge delegate's location variable
http://localhost:3000/clients/1/tickets <- new url due to the history.pushState when advancing a turbo-frame

In our case, we have a single turbo frame acting as a dynamic placeholder for our links that give the visual representation of tabs. When you click on a tab, they update the content of the frame and turbo uses the history.pushState to update the url. We need this to allow the pull to refresh function to maintain the tab state. When the new tab html loads into the frame, strada-web uses the current (updated from the pushstate) location when binding the new bridge components, but the bridge delegate associated with the current session/webview is using the stale location variable in the guard as we haven't technically invoked a route proposal.

@joshmcarthur
Copy link

joshmcarthur commented Oct 26, 2023

I'd imagine this is also going to affect someone who is using a lazy-loaded turbo frame (or in fact multiple turbo-frames on a page that navigate to different places), since strada-web is using the URL from the window.location object, not from the frame's src.

I'd still be quite curious to know the purpose of the guard - I've guessed it's to stop components running on the wrong webview?

@tapster
Copy link

tapster commented Oct 30, 2023

Hi there, I hit the same issue with bridgeDidIgnoreMessage here https://github.com/hotwired/strada-ios/blob/main/Source/BridgeDelegate.swift#L120 because location does not match message.metadata?.url

In my case its pretty simple in that when the app launches the WebView visits localhost:3000 which goes to the home#index action where I redirect_to another path if the user is logged out.

In that case the Strada component is not rendered.

Interestingly if I navigate away in the same view then hit the NavigationController "back" button, the component is now rendered fine. Only that first load is not working.

Also, other tabs that do not redirect render the components as expected

@solilin
Copy link

solilin commented Nov 7, 2023

I have the same issues withbridgeDidIgnoreMessageafter redirect and after clicking to link with data-turbo-action="advance"

@alhajrahmoun
Copy link

I had something similar to this and the difference was the trailing slash at the end of the url (url with trailing slash works fine).
Here's the a snapshot of the log testing both cases:

No slash:

2023-11-12 19:14:10 +0000 [Session] visit ["location": http://192.168.0.89:3000, "options": Turbo.VisitOptions(action: Turbo.VisitAction.advance, response: nil), "reload": false]
2023-11-12 19:14:10 +0000 [ColdBootVisit] startVisit http://192.168.0.89:3000 [:]
bridgeDestinationViewDidLoad: http://192.168.0.89:3000
bridgeDestinationViewWillAppear: http://192.168.0.89:3000
bridgeDestinationViewDidAppear: http://192.168.0.89:3000
bridgeDidIgnoreMessage: Message(id: "1", component: "sync", event: "connect", metadata: Optional(Strada.Message.Metadata(url: "http://192.168.0.89:3000/")), jsonData: "{\"metadata\":{\"url\":\"http:\\/\\/192.168.0.89:3000\\/\"},\"syncTitle\":\"Sync data\"}")
2023-11-12 19:14:12 +0000 [Bridge] ← pageLoaded ["timestamp": 1699816452935, "restorationIdentifier": c5689ea0-844d-4e20-8b72-4529521cba73] [:]

With slash:

2023-11-12 19:13:11 +0000 [Session] visit ["options": Turbo.VisitOptions(action: Turbo.VisitAction.advance, response: nil), "reload": false, "location": http://192.168.0.89:3000/]
2023-11-12 19:13:11 +0000 [ColdBootVisit] startVisit http://192.168.0.89:3000/ [:]
bridgeDestinationViewDidLoad: http://192.168.0.89:3000/
bridgeDestinationViewWillAppear: http://192.168.0.89:3000/
bridgeDestinationViewDidAppear: http://192.168.0.89:3000/
bridgeDidReceiveMessage Message(id: "1", component: "sync", event: "connect", metadata: Optional(Strada.Message.Metadata(url: "http://192.168.0.89:3000/")), jsonData: "{\"metadata\":{\"url\":\"http:\\/\\/192.168.0.89:3000\\/\"},\"syncTitle\":\"Sync data\"}")
2023-11-12 19:13:13 +0000 [Bridge] ← pageLoaded ["restorationIdentifier": a2092ce4-70b9-48a6-ae67-cc0dab0b2d5c, "timestamp": 1699816393697] [:]

Shot in the dark here… but can you print out the two URLs that are being compared in the guard statement? I'm wondering if a trailing slash is messing things up.

@adampal
Copy link

adampal commented Nov 26, 2023

There seem to be 2 separate issues in this thread:

  • Stale BridgeDelegate.location causing messages to be ignored
  • Turbo-frames not updating the URL and causing messages to be ignored

Given the title of this issue is for turbo-frames, I created a new issue (#23) with my investigation of the stale location bug. The two issues are related and cause the same behaviour, however I suspect they have different underlying causes.

@joemasilotti
Copy link
Member

Hey folks, can you give PR #24 a try? Thanks to @joshmcarthur for the idea of grabbing the URL at runtime!

@joemasilotti
Copy link
Member

Resolved via #24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants