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.

Sign up for GitHub

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

Fix snapshot cache issue #173

Closed
wants to merge 2 commits into from
Closed

Fix snapshot cache issue #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
845a266
Cherry-pick skip-visit-proposal
joemasilotti Dec 18, 2023
96d92ad
Fix issue where the snapshot cache isn't cleared
joemasilotti Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Source/Session/Session.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public class Session: NSObject {
topmostVisit = currentVisit
}

public func clearSnapshotCache() {
@objc public func clearSnapshotCache() {
bridge.clearSnapshotCache()
}

Expand Down Expand Up @@ -221,7 +221,7 @@ extension Session: VisitableDelegate {
} else if visitable === currentVisit.visitable && currentVisit.state == .started {
// Navigating forward - complete navigation early
completeNavigationForCurrentVisit()
} else if visitable !== topmostVisit.visitable {
} else if visitable !== topmostVisit.visitable && !visitable.visitableViewController.isMovingToParent {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this preventing a visit in the modal session? Was a visit happening both in the modal session and the default session?

Could this break apps that don't have a two-session setup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, a visit was happening in the modal session and then again in the default session.

I disabled the modal path configuration and everything seems to work as expected when navigating back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, why would visitableViewWillAppear() be called in the modal Session if it's being dismissed? Seems like the root issue is at a higher layer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not able to reproduce this, but the additional code is needed for the following reason.

When a new visit is proposed via advance, a new view controller is created. The new view controller is in the process of being added to the navigation controller, thus isMovingToParent is true.

// Navigating backward
visit(visitable, action: .restore)
}
Expand Down
2 changes: 1 addition & 1 deletion Source/Turbo Navigator/TurboNavigator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ extension TurboNavigator: SessionDelegate {

public func sessionDidFinishFormSubmission(_ session: Session) {
if session == modalSession {
self.session.clearSnapshotCache()
self.session.perform(#selector(Session.clearSnapshotCache), with: nil, afterDelay: 1)
Comment on lines 115 to +116
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you moved this logic to sessionDidStartFormSubmission() instead without the delay, do you see the expected result? That might be an ok approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that doesn't seem to do the trick. From what I can gather, we need to delay the call to clearSnapshotCache(), not move it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not able to nail down why Turbo.session.clearCache() seems to silently fail in this scenario but I'm guessing it has something to do with the view not being 100% "visible". As in, the modal might be the active web view so this JavaScript call might not fully execute.

clearCache() is indeed called before the modal is dismissed. If your thesis holds true, then we need to somehow delay clearing the cache until after the modal view has been dismissed. I'll think about it.

Copy link
Contributor

@svara svara Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tested the demo app thoroughly and only once hit a javascript error when calling window.turboNative.clearSnapshotCache. I thought I'd hit it again, but no. I didn't copy the error, but it was along the lines of "couldn't execute in this document."

I was able to find a solution though. Whether it's elegant is up for debate :)
The idea is to set a flag on Session that a cache clear is needed and check the flag on visitableViewWillAppear. The flag is set in TurboNavigator in sessionDidFinishFormSubmission.

public class Session: NSObject {
    ...
    var needsCacheClearing: Bool = false
    ...
}

 public func visitableViewWillAppear(_ visitable: Visitable) {
        if needsCacheClearing {
            clearSnapshotCache()
            needsCacheClearing = false
        }
        ...
}

public func sessionDidFinishFormSubmission(_ session: Session) {
	if session == modalSession {
	    self.session.needsCacheClearing = true
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This does look like a better solution to me and removes the timing implications. @joemasilotti if you can merge up the latest from main to get the changes from #178 and address a fix, that'd be great. Maybe something like snapshotCacheIsStale is a better flag name? It'd also be nice to write a test around this approach if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, I can take a look this week.

While only slightly tangential, it's starting to feel like #150 (and subsequently #174) will make sense to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

This works great! @svara, I opened PR #182 with your changes. Can you please confirm I put the if needsCacheClearing... in the right spot?

Copy link
Contributor

Choose a reason for hiding this comment

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

@joemasilotti The PR looks great. @jayohms The current flag name is still needsCacheClearing. I don't feel strongly about the name, but I agree that snapshotCacheIsStale may be a better name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@svara have you seen this? #181

It's a fairly similar issue - but the opposite problem. Knowing when to take a snapshot cache instead of deleting it. We should use consistent language and implementation if possible. Can you take a look at that one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jayohms I've started reviewing the PR. The solution looks solid, but I need to spend some more time familiarizing myself with caching in general.

}
if let url = session.topmostVisitable?.visitableURL {
delegate.formSubmissionDidFinish(at: url)
Expand Down
3 changes: 3 additions & 0 deletions Source/WebView/turbo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@
if (Turbo.navigator.locationWithActionIsSamePage(location, options.action)) {
Turbo.navigator.view.scrollToAnchorFromLocation(location)
return
} else if (this.currentVisit?.location?.href === location.href) {
this.visitLocationWithOptionsAndRestorationIdentifier(location, options, Turbo.navigator.restorationIdentifier)
return
Comment on lines +106 to +108
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this also fixes this for Turbo 8, which is great: #160

I'd preferably like to see something close to the Android implementation, where we call back to the native session, so we can add proper logging around it, like this: hotwired/turbo-android#292

Copy link
Member Author

Choose a reason for hiding this comment

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

I cherry-picked that commit directly from the PR!

}
}

Expand Down