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 issue where dismiss read FAB action would not work consistently #743

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

hjiangsu
Copy link
Member

Pull Request Description

This PR fixes an issue where the dismiss read FAB action would not work consistently. This removes the throttle duration for the dismiss read events so they can be triggered at any time without any limits.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM!

I wonder how many of these throttles we actually need. Sometimes I refresh or do other actions in quick succession and they get throttled.

lib/community/bloc/community_bloc.dart Show resolved Hide resolved
@hjiangsu
Copy link
Member Author

I wonder how many of these throttles we actually need. Sometimes I refresh or do other actions in quick succession and they get throttled.

I added the throttling in initially to prevent spamming requests to the instance servers. Since then, I've just been adding them out of habit. I think we can evaluate these things on a case-by-case basis.

For example, you may want to mark a lot of messages as read from your inbox, so it makes sense to remove throttling there. Similarly, you may want to upvote or save a lot of posts within a short timeframe. However, refreshing posts and fetching for more content doesn't need to happen very quickly!

@hjiangsu hjiangsu merged commit 6b37595 into develop Sep 15, 2023
1 check passed
@hjiangsu hjiangsu deleted the fix/dismiss-read-regression branch September 15, 2023 18:09
@micahmo
Copy link
Member

micahmo commented Sep 15, 2023

I added the throttling in initially to prevent spamming requests to the instance servers.

That makes total sense. However I think it would be hard for a user to overload an instance via manual actions from a client app. I think servers get overloaded when there are automated API requests in the thousands+ per second. If I can press the refresh button on my phone so fast that the server starts having trouble, then it's really gonna be in trouble when it has lots of users. 😆 But fair enough, no need to overburden them more than necessary. Maybe we can look at eliminating some and lowering others.

Also you mentioned refresh, which ironically is what I often do that gets throttled by the app. 😆

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