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

Embeddable sandbox: do not force refresh on updates #7875

Closed
habovh opened this issue Apr 26, 2024 · 8 comments
Closed

Embeddable sandbox: do not force refresh on updates #7875

habovh opened this issue Apr 26, 2024 · 8 comments

Comments

@habovh
Copy link

habovh commented Apr 26, 2024

I'm using the embedded sandbox in my development environment extensively. It's great, and works offline as long as it's been accessed once in the browser.

The issue

However, there's nothing more frustrating that opening my browser, navigating to my development server URL where the embedded sandbox is served, typing in my query/mutation/params/headers and the sandbox decides it's time to update, refreshes the entire sandbox and I'm good to start all over again.
On slow internet connections, this may happen very late, to the point where I could have a document almost ready to copy to my app but then it's completely lost.

This is really putting me off. I even considered going back to my previous dev tools such as Playground and GraphiQL for this single reason, even though Apollo Studio is better feature-wise in my opinion.

Suggestions

Instead of forcefully installing and refreshing the entire Studio, would it be possible to have some kind of "update" icon showing up in the sidebar if/when there's an update available, allowing the developer to install it at a convenient time?
Another alternative would be to completely block the UI with a loading screen until Apollo Studio has determined there's no pending update. That way, when the actual Studio UI shows up we can confidently start using it instead of being left here wondering: "did it have time to check updates? Am I good to go or will I lost yet another document as soon as I'll start typing?".

The main issue here is lost time & effort due to having to start over. If you have other ideas that would improve the DX in this direction please share away!

@mayakoneval
Copy link
Contributor

Hello! Thank you for this bug report. We will look into and update when we find a resolution.

@cheapsteak
Copy link
Member

cheapsteak commented Apr 29, 2024

Hi there, thanks so much for the bug report

Could you please help confirm a few things so we can make sure we're looking at the right thing? 🙏

Is it the case that the page is refreshing by itself, with no user interaction?
The codepaths that trigger a refresh are currently all in button click handlers, I can't imagine how that could be possible 🤔
It is the case that if you have multiple tabs open, clicking on "update" in one tab will trigger updates across all tabs, might that be what you experienced?

Wrt losing time and effort, we use localStorage to persist state, refreshing shouldn't cause lost work.
Was sandbox being opened in your browser's incognito mode?
Does refreshing a tab normally cause contents to be lost, or only if it was from an update? (how do you know the page refreshed from an update?)

@habovh
Copy link
Author

habovh commented May 3, 2024

Hi @cheapsteak,

To answer your question yes, the page is refreshing itself with no user interaction, that is, the user didn't request the page refresh. I have never seen any "update" button you're referring to, but having one and installing updates only when clicked or on manual page refresh would very much fix this issue.

As shown by this screen capture, you can see it takes roughly 10 seconds from the moment I access the page for the first time and the moment it reloads. I even used Incognito mode to make sure no extension or any other tab is interfering.

I have time to start building my document, and the page refreshes before showing again in its default empty state.

Untitled.2.mov

Now, I'm aware I can configure the embed to keep its state between refreshes with the persistExplorerState option, but in my opinion it's not a valid solution since it's still disruptive, and I'd like to avoid storing authorisation headers in the local storage.

@cheapsteak
Copy link
Member

thanks so much for the thoughtful steps in the screencapture 🙏

We're able to reliably reproduce the issue, have identified the problem and have a PR awaiting review

@cheapsteak
Copy link
Member

Hi there, we just released a fix for this, you might need to clear out service worker cache to test
Please let me know if the issue still persists 🙏

@habovh
Copy link
Author

habovh commented May 6, 2024

@cheapsteak thanks for the followup! Out of curiosity, is the PR publicly accessible? I'd very much like to understand what was causing the issue.

@cheapsteak
Copy link
Member

@habovh The issue was on the Studio side of the code (contents of the embed) and is not a public repo

I can describe the issue though

Turns out I was wrong about "The codepaths that trigger a refresh are currently all in button click handlers". There is one additional path where window.location.reload() is directly called -- when the controlling event is received (context: we use workbox to manager service workers)

The flow that we originally envisioned was: User is running on existing service worker, app detects it is out of date, prompt user to update, user clicks update, app tells waiting service worker to skip waiting, new service worker takes control, all open tabs reload

The problem was that 1) on app's initial load, there is no existing running service worker, so the service worker takes control as soon as all resources are downloaded (what you captured in your screenshare in incognito); 2) when all the app's tabs are closed, the next time one tab is opened, the stale service worker no longer blocks the upcoming worker and the previously waiting service worker will automatically take control and the page will reload

The fix right now is to remove the reload that triggers when the "controlling" event is sent, but rather have user-triggered updates only reload the current tab

The tradeoff is that background tabs won't be reloaded now when the user updates the app in one tab, and it's conceivable that the now out-of-date bundle might try to request a file that is no longer in cache, and that would be a problem if the user is also offline, but that should be rather rare (if someone is only using explorer it'd never happen) and would be solved by manually reloading the page

@habovh
Copy link
Author

habovh commented May 7, 2024

@cheapsteak thanks for the explanation! If I understand it right, you could address the latest point by still subscribing to the controlling event, but instead of reloading the page, check whether the bundle is current and conditionally display a notification prompting the user to reload manually because their current tab is working with an outdated bundle. That'd provide an opportunity for the user to "backup" their work before reloading this specific tab while avoiding "surprises" of trying to access resources that do not exist anymore.

Just my two cents anyway, again, thanks for sharing the info!

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

No branches or pull requests

3 participants