Skip to content

feat(refresh): reset interval on manual refresh #1014

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

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented Apr 16, 2024

This PR further helps reduce the chance of rate-limiting conditions by

  • resetting the automated refresh timer upon a manual refresh (default: 1 minute) to prevent manual+auto fetching being close together
  • disabling the manual refresh button, if clicked, until the next internal (default: 1 minute) to prevent repeated clicks removed based on PR feedback

I have also taken the opportunity to refactor the hardcoded fetch interval into our Constants file

@adufr
Copy link
Contributor

adufr commented Apr 16, 2024

Just sharing my opinion about this: first point looks good to me but I don't agree with the second, IMO the user should be free to refresh whenever he wants, and if he gets rate-limited because he clicked the refresh button too many times, too bad for him but that's his problem
I often use this button and this change would badly affect my experience 🙁

@adufr
Copy link
Contributor

adufr commented Apr 16, 2024

However, having a configurable interval could probably also help with the rate-limiting issue?
This way user would have more control over this issue, and if they report problems we could just instruct them to increase their refresh interval

@setchy
Copy link
Member Author

setchy commented Apr 16, 2024

Just sharing my opinion about this: first point looks good to me but I don't agree with the second, IMO the user should be free to refresh whenever he wants, and if he gets rate-limited because he clicked the refresh button too many times, too bad for him but that's his problem I often use this button and this change would badly affect my experience 🙁

You have a need to manually refresh multiple times within the minute? 🤔

@setchy setchy marked this pull request as ready for review April 16, 2024 14:44
@setchy
Copy link
Member Author

setchy commented Apr 16, 2024

@afonsojramos @brandonweiss - what is your take on item 2 above. Should we pull it or keep it?

@setchy
Copy link
Member Author

setchy commented Apr 16, 2024

However, having a configurable interval could probably also help with the rate-limiting issue?

Raised this as a new FR #1027 1027

@setchy setchy mentioned this pull request Apr 16, 2024
1 task
@afonsojramos
Copy link
Member

Agree with @adufr here. Refreshing manually may increase usage, but it was manual, so that's the user's responsibility. We should not take that "power" away imo.

@setchy
Copy link
Member Author

setchy commented Apr 16, 2024

Agree with @adufr here. Refreshing manually may increase usage, but it was manual, so that's the user's responsibility. We should not take that "power" away imo.

OK then....

@afonsojramos
Copy link
Member

Reverting the change in e62aa37, we can iterate in the future if needed. Let's get 5.3.0 out!

@afonsojramos afonsojramos merged commit b7513ef into main Apr 16, 2024
6 checks passed
@afonsojramos afonsojramos deleted the refactor/interval-magic branch April 16, 2024 18:27
@setchy setchy changed the title feat(refresh): reset interval and disable manual refresh feat(refresh): reset interval on manual refresh Apr 16, 2024
@setchy setchy added the enhancement New feature or enhancement to existing functionality label Jul 13, 2024
@setchy setchy added this to the Release 5.3.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants