search

discordx-ts / discordx

🤖 Create a discord bot with TypeScript and Decorators!
https://discordx.js.org
Apache License 2.0
607 stars 50 forks source link

refactor(pagination): add check for unbound state #886

Closed sum117 closed 1 year ago

sum117 commented 1 year ago

Please describe the changes this PR makes and why it should be merged: This is a simple fix for an issue caused by spam clicking buttons. The application will crash if this is not implemented all the times if the clicks have a short interval between them. The best scenario to describe this is multiple users clicking the button at the same time (which happened on my server).

Package

VictoriqueMoe commented 1 year ago

Why does spam pressing the button cause this?

sum117 commented 1 year ago

This is caused because the API is updating the value of the page number before the application can handle the disable event on the previous/forward button.

This check below Get Page is what causes this. It was missing the guard for negative/exceeded page numbers from the very start.

VictoriqueMoe commented 1 year ago

This is caused because the API is updating the value of the page number before the application can handle the disable event on the previous/forward button.

This check below Get Page is what causes this. It was missing the guard for negative/exceeded page numbers from the very start.

I do not feel that currentPage should ever be less than 0, if this is not coming from the api, then the guard of protecting that i do not feel is the solution, the solution is figuring out WHY the framework can not keep up with the updating from the API and prevent the issue from the source.

this guard is fine, but now we have a sitauton where this.currentPage can be <0 and this doesn't feel right

sum117 commented 1 year ago

I can't imagine a situation where you'd want the index to be -1. But yeah, I've seen this issue before in other pagination systems and it's always been like that. If there's multiple people clicking it over and over the application can't handle it.

samarmeena commented 1 year ago

I can't imagine a situation where you'd want the index to be -1. But yeah, I've seen this issue before in other pagination systems and it's always been like that. If there's multiple people clicking it over and over the application can't handle it.

that concern is valid, but then there should be some methods to tackle this. I have following suggestions.

  1. restrict another execution, while processing a pagination, so further calls will be ignored and only first one is taken and processed.
  2. do not run execution for unbound calls, and perhaps provide a unbound handler.
sum117 commented 1 year ago

Changes have been made.

sum117 commented 1 year ago

Removed function, added two if statements

samarmeena commented 1 year ago

@sum117 rebase with signed commit.