izaguirrejoe
commented
2 years ago @joemasilotti Could this be at all related to #48? Is that problem resolved?
Closed izaguirrejoe closed 2 years ago
izaguirrejoe
commented
2 years ago @joemasilotti Could this be at all related to #48? Is that problem resolved?
joemasilotti
commented
2 years ago I believe this is bug in turbo.js, but I can't say for sure because I can't set breakpoints in the javascript code in Xcode. 😢 I need to do more research into how Turbo itself works.
If you open Safari on your desktop you can connect to the simulator's web view via the Debug menu item. You might have to enable Developer Mode in settings for it to appear. Then you should be able to set a break point in the JavaScript.
@joemasilotti Could this be at all related to https://github.com/hotwired/turbo-ios/issues/48? Is that problem resolved?
This might be the root cause, but I can't say for sure! I've adopted to only doing forms in modals to work around the issue.
Very curious to hear what you find or what the maintainers think about this! I haven't heard anything on my issue since I opened it in October.
ghiculescu
commented
2 years ago Sounds similarish to https://github.com/hotwired/turbo-ios/issues/12
Are you calling session.visit(visitable, options: options) ?
izaguirrejoe
commented
2 years ago Sorry for late response, guys. @joemasilotti thanks for the tip! @ghiculescu yes, I'm calling session.visit with the options, which contains the response.
I haven't had time yet to do a deep dive on this yet. I'm starting to think it has more to do with the restoration identifier, I'll keep you guys posted.
joemasilotti
commented
2 years ago @jayohms, any updates or ideas on this one? My workaround no longer works and I'm running into this issue for any submitted form.
Edit: I updated turbo.js via turbo-rails on the Rails side and the issue remains.
joemasilotti
commented
2 years ago Digging in some more, I think I've narrowed down the lines of code that might be causing the issue.
Visit.loadResponse() calls into render() if a response is already present. In desktop Safari, the async callback fires and renderPage() is called. However, in the iOS demo the async callback never fires.
On desktop the following lines of Visit are hit in order:
However, on iOS the order is:
Which makes me think that the first call to render() is being cancelled for some reason. Perhaps this is causing a race condition so the second one never finishes? Or the original caller is losing a reference to the second call to render()?
I'll admit I'm in a bit over my head with this JavaScript. But hopefully this helps us get closer to the root cause and fixing this issue!
ghiculescu
commented
2 years ago I can replicate the original issue here. I haven't fully dug into why yet but I did note that this event gets fired after you tap "submit"
And there is a cached snapshot:
Presumably that comes from here https://github.com/hotwired/turbo/blob/98cdc4037788f078a741c7fb50412db0101bcb97/src/core/drive/visit.ts#L240
ghiculescu
commented
2 years ago Not sure if it's related, but the order of operations in visitStarted in a browser is different to the native order.
I don't see why the snapshot is loaded first in browser, but last in native.
Edit: Android has the same order of calls but doesn't have the issue... probably not this.
jayohms
commented
2 years ago I'll be taking a look at this tomorrow. I don't have my mind wrapped around all the details yet, so I'll spend some dedicated time on it. @joemasilotti or @ghiculescu do you know if the turbo-android library/demo have any issues in this area? I'm not aware of anything, so I presume this might be isolated to the turbo-ios library.
ghiculescu
commented
2 years ago I just tried the turbo-android demo and couldn't replicate.
This log line seems relevant:
2022-06-28 16:33:30.480 32670-302/dev.hotwire.turbo.demo D/TurboLog: visitStarted ...................... [session: main, location: https://turbo-native-demo.glitch.me/success, visitIdentifier: 1ad47178-c464-4636-868d-cd3ba447061e, visitHasCachedSnapshot: false]
Whereas on the iOS demo, hasCachedSnapshot is true on the 2nd and subsequent visits.
joemasilotti
commented
2 years ago Excellent, thanks @jayohms! If it helps, I'm open to pair with you.
izaguirrejoe
commented
2 years ago Sorry guys, I haven't had the time to take a deep dive! I'll see if I can get to it this week.
For what it's worth: the response html is rendered when it's passed to the same session (in my app, I have it push a new view controller). The response html seems to be ignored when it is passed to a different session (like a modal session).
jayohms
commented
2 years ago An update: I've been digging into this. The core issue is the fact that the iOS demo app has separate default and modal sessions. This isn't the case on desktop web or Android.
Whenever a form submission is completed, the core turbo.js library clears all cached snapshots as a fairly blunt approach so that (potentially) stale snapshots are not reused. Since clearing snapshots across multiple sessions is not supported, the default session retains its snapshot cache. Even if this particular bug didn't exist, we still would want the default session snapshot cache cleared, since you'd want to see fresh data as you navigate back through the backstack.
I'm working on an approach to make it possible to clear snapshots across sessions after form submissions, but I'm not entirely confident how successful it'll be yet. I'm hoping to get a PR ready for tomorrow — and if not I'll provide an update.
joemasilotti
commented
2 years ago Amazing! Thanks for the update.
Clearing snapshots after a form submission sounds great to me. It could also make navigating "back" a few pages after submitting a form not render stale data!
jayohms
commented
2 years ago I've got a spike here to manually clear the default Session's snapshot cache when the modal session completes a form submission: https://github.com/hotwired/turbo-ios/pull/84
However, unfortunately it hasn't solved the issue here yet — but it's possible I'm missing something with the implementation. There might also be a race condition with clearing the snapshot and starting the next visit.
It also appears that there's might be some flawed logic here and the navigation is completed early: https://github.com/hotwired/turbo-ios/blob/main/Source/Session/Session.swift#L210-L219
I don't have a solution yet, but I'll keep digging. Feedback welcome @joemasilotti, @ghiculescu or @izaguirrejoe!
joemasilotti
commented
2 years ago With one small change your PR is working great, @jayohms! I commented on the PR with the diff.
jayohms
commented
2 years ago This fix is now available in the new release: https://github.com/hotwired/turbo-ios/releases/tag/7.0.0-rc.7
There's a potential bug in Turbo-iOS when it comes to submitting forms and rendering the response HTML.
I notice the following flow when submitting a form in Turbo iOS:
1) The form is submitted. 2) The server issues a redirect upon successful form response. 3) The redirect is followed, and the response is returned. 4) The WebView bridge is alerted to a "Visit Proposed" event containing the response HTML. 5) You're free to route the proposal as desired, usually ended with calling the "window.turboNative.visitLocationWithOptionsAndRestorationIdentifier" with the response HTML.
The problem is, calling "window.turboNative.visitLocationWithOptionsAndRestorationIdentifier" containing response HTML returns the exact same page, completely ignoring the response HTML.
If you try this in the demo app, it will work the first time and the first time only. Subsequent form submissions will continue to return the same page over and over again. This is because of a "Back Navigation" bug, where submitting the form the first time, the app thinks you hit the back button, and issues another get request.
Walking through step by step in the Demo app, using Proxyman to see requests/responses to/from the server:
1) Run the app 2) Tap "Load a web page modally" 3) Submit the form. Proxyman shows two get requests, the first from the redirect, the second from the above mentioned "Back Navigation" bug: The rendered screen is "correct" only because of the second get request.
4) Hit the back button.
5) Tap "Load a web page modally" again. 6) Submit the form. Proxyman now shows one get request, as expected. However, the response HTML was ignored. This can be seen by comparing the time stamps of the response HTML with the time stamp rendered on screen:
Walking through the "Back Navigation" bug:
1) Run the app 2) Tap "Load a web page modally" 3) Place breakpoints on line 207 of "Session.swift" and line 47 of "Visit.swift". 4) Submit the form. 5) Breakpoint: The first time and first time only, a "visit did complete" message comes from the ScriptMessageHandler, setting the state of the Visit to complete. I have no idea why this event is sent, but it's the reason for the Back Navigation bug. Hit continue. 6) Breakpoint Session's visitableViewWillAppear activates, and ends up in the "Navigating Backward" path, issuing another visit (GET request 2). It is this second GET request that gives the appearance of "correct" behavior when submitting the form the first time.
Subsequent form submissions skip step 5, so on visitableViewWillAppear, it ends up in the "Navigating Forward" path, issuing only one GET request as it should. But because the response HTML is ignored by Turbo, it re-renders the same screen again and again. Refreshing the page renders the correct response.
I believe this is bug in turbo.js, but I can't say for sure because I can't set breakpoints in the javascript code in Xcode. 😢 I need to do more research into how Turbo itself works.
My current workaround is to set the response HTML to nil before passing the proposal to "window.turboNative.visitLocationWithOptionsAndRestorationIdentifier". This seems to trick Turbo into not knowing that the request came a redirected form submission, so it issues another GET request. Submitting two GET requests per form submission is not ideal, though.