Fix snapshot cache issue

[joemasilotti]

This PR is an attempt to fix an issue where the snapshot cache doesn't get cleared.

To recreate:

1. Present a modal that submits a form - this calls `clearSnapshotCache()
2. Navigate back

Expected behavior:

The flash message appears on the redirected to page. Navigating back reloads the page from the server.

Actual behavior:

The flash message is "eaten" by Rails because turbo-ios makes two requests to the show page. Navigating back shows the stale content because the snapshot cache was never cleared.

---

I think we have a race condition. 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.

This PR makes two changes. The changes to Session fix the flash message by no longer performing the visit twice. I'd like a second set of eyes on this to make sure I'm not overlooking something.

The second change, in TurboNavigator, is a hack. I think that delaying the clearing of the cache by 1 second fixes the issue because the non-modal web view has become "visible" again. Obviously, hard-coding a one second delay is ripe for future issues. Does anyone have any ideas on how we could handle this better?

[jayohms]

@joemasilotti I'm not quite clear, with this PR do you see the expected behavior?

[joemasilotti]

Sorry, I wrote this more like an issue than a PR!

This PR fixes both issues.

[jayohms]

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

[joemasilotti]

I cherry-picked that commit directly from the PR!

[jayohms]

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?

[joemasilotti]

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.

[jayohms]

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?

[svara]

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.

[jayohms]

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

[joemasilotti]

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.

[svara]

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.

[svara]

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
	}
}

[jayohms]

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.

[joemasilotti]

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.

[joemasilotti]

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

[svara]

@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.

[jayohms]

@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?

[svara]

@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.

[svara]

@joemasilotti I've started reviewing and debugging this today. I'll provide the feedback tomorrow.

[joemasilotti]

Closing this out in favor of #182.