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

Refresh client hourly when inactive #4648

Merged

Conversation

StachuDotNet
Copy link
Member

@StachuDotNet StachuDotNet commented Dec 16, 2022

Changelog:

Editor
- refresh the editor when it is outdated and inactive

How this works:

  • appsupport.js listens for changes in whether the page is in focus or not, with windowFocusChange
  • upon change of hidden/visible, it sends a message to a BrowserSubscription in ReScript land, which in turn triggers a PageVisibilityChange Msg
  • that triggers a PageVisibilityChange Msg, which results in an update to a pageVisibility field in our main model
    • new: the 'hidden' variant of PageVisibility now has a since field in it, which is used to track how long it's been since a page became hidden
  • separately, a timer runs every minute, triggering a CheckIfClientIsOutdated Msg; that results in an API call being made to a new endpoint in dark-editor, made possible by Add DarkInternal::serverBuildHash fn that returns the git hash of the server's current deploy #4649:
    image
  • when the result of that API (the hash) is returned, we assess a few variables, and refresh the page if all are true:
    • the client hash and the server hash don't match (i.e. the client is out of date)
    • the page is 'safe to refresh'[1]
    • the page is not currently in focus, and has been 'hidden' for at least an hour

[1] later, we can/should expand this criteria - currently, only the Architecture and FocusedPackageManagerFn pages are considered safely refreshable

I ran into some confusion when trying to tunnel my static assets in prod, so wrote some external docs in darklang/docs, and referenced those in the relevant portion of the Contributor settings.

@StachuDotNet StachuDotNet force-pushed the refresh-outdated-client-when-inactive branch from 8d4b110 to 865ba86 Compare December 16, 2022 17:24
client/src/app/Main.res Outdated Show resolved Hide resolved
client/src/app/Main.res Outdated Show resolved Hide resolved
Copy link
Member

@pbiggar pbiggar left a comment

Choose a reason for hiding this comment

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

I think we should get a little bit better logic in here:

  • ensure we'll only refresh when there's a new client
  • ensure we won't do it if the user is currently active (not hidden)

@StachuDotNet
Copy link
Member Author

StachuDotNet commented Dec 27, 2022

As of #4649 and a tiny HTTP handler, https://dark-editor.builtwithdark.com/latest-backend-build-hash is now available.

Given that, and this conversation, my current plan for this is:

  • (rip out the current commits in favor of this new plan)
  • store a new field in our app state, lastActive, representing when the page was last active/visible
  • add a timer every minute that checks if the tab is active.
    • If active, set lastActive to 'now'.
    • If inactive, check if it's been an hour since lastActive.
      • if it has been at least an hour, fire off a process (event) to:
        • query that new endpoint for the most recent hash
        • if the hashes differ and the page is 'refreshable', refresh

@StachuDotNet StachuDotNet force-pushed the refresh-outdated-client-when-inactive branch from 7632b50 to f610b76 Compare December 29, 2022 20:11
client/src/api/API.res Outdated Show resolved Hide resolved
// beginning of the function, we still exercise the message and request generating
// code locally.
if m.origin == "https://darklang.com" {
// TODO: test this locally.
Copy link
Member Author

Choose a reason for hiding this comment

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

My local testing so far has been to raise the RefreshClientIfOutdated Msg with a known-incorrect build hash. I suppose the next step, to better test, will be to set up a tunnel.

client/src/api/API.res Outdated Show resolved Hide resolved
client/src/app/Main.res Outdated Show resolved Hide resolved
client/src/app/Main.res Outdated Show resolved Hide resolved
@StachuDotNet StachuDotNet marked this pull request as ready for review January 2, 2023 15:29
Copy link
Member

@pbiggar pbiggar left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

However, I think the ordering is wrong, leading it to do the API call every minute instead of every hour.

What it does:

  • each minute check if there's a new client
  • then checks if it's safe to update to it

What it should do:

  • check if we're in a position to update (eg it's been an hour)
  • then check if there is an update

@StachuDotNet
Copy link
Member Author

Updated - do you mean like this?

Copy link
Member

@pbiggar pbiggar left a comment

Choose a reason for hiding this comment

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

Perfect!

@StachuDotNet StachuDotNet merged commit e95c6b6 into darklang:main Jan 2, 2023
@StachuDotNet StachuDotNet deleted the refresh-outdated-client-when-inactive branch June 9, 2023 15:17
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

Successfully merging this pull request may close these issues.

None yet

2 participants