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

Fix "stop tracking on browser close" hook #1767

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tcrammond
Copy link
Contributor

@tcrammond tcrammond commented Jun 23, 2020

Please remember the Contributing Guidelines ❤️

🌟 What does this PR do?

Tweaks the code to "detect when browser is closed, and stop the running entry". Removed a counting variable and moved everything into the window callback. Tried to await all the promises that happen so we have the best chance of our code being allowed to complete before the browser is gone.

Note: I couldn't reproduce the issue but it looked pretty likely that something could go wrong with it.

Also removed some dead code that was trying to do the same thing

🐛 Recommendations for testing

All changes should be tested across Chrome and Firefox.

  • Enable the "stop automatically" setting
  • Start a timer
  • Close the browser
  • Open the browser again and see that the entry was stopped ✅

Do you know or can you spot a better way to make this behaviour more reliable?

📝 Links to relevant issues or information

Fixes #1687. (Hopefully)

@tcrammond tcrammond self-assigned this Jun 23, 2020
@lise-toggl
Copy link
Contributor

I could not get this to work (also doesn't work on master) :(

It looks like on close listener doesn't trigger for me at all, could be a 🐧 thing. @shantanuraj can you try it out?

@tcrammond tcrammond added the needs follow up Reviewed PRs that require changes or further action. label Jun 24, 2020
@tcrammond tcrammond removed their assignment Jun 29, 2020
@rylek90 rylek90 added the up for grabs PRs that need a new owner to work on it. label Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs follow up Reviewed PRs that require changes or further action. up for grabs PRs that need a new owner to work on it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Stop Timer Automatically" Feature Not Working
3 participants