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

GitHub Rate Limiting Modal on Load #20

Closed
0x4007 opened this issue Mar 3, 2024 · 17 comments · Fixed by #22
Closed

GitHub Rate Limiting Modal on Load #20

0x4007 opened this issue Mar 3, 2024 · 17 comments · Fixed by #22

Comments

@0x4007
Copy link
Member

0x4007 commented Mar 3, 2024

There's a bug that shows the rate limit modal even after logging in.

Steps to Reproduce

  1. get rate limited as not logged in user.
  2. press the login with github button
  3. page automatically reloads with you signed in, but the rate limit modal still appears

at this step we would just refresh and it works fine, but I fear that this may not be intuitive for new users.

Fix

Please make sure that the page waits to see if you're logged in before displaying the rate limit modal.

Notice: you can get rate limited while logged in so also make sure that this is handled.

Copy link

ubiquibot bot commented Mar 3, 2024

! action has an uncaught error

@Keyrxng
Copy link
Contributor

Keyrxng commented Mar 3, 2024

/start

Copy link

ubiquibot bot commented Mar 3, 2024

DeadlineSun, Mar 3, 9:26 PM UTC
Registered Wallet 0xAe5D1F192013db889b1e2115A370aB133f359765
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@Keyrxng
Copy link
Contributor

Keyrxng commented Mar 3, 2024

it's 5k request an hour to get rate limited while logged in, how I can QA that lmao

You can use a personal access token to make API requests. Additionally, you can authorize a GitHub App or OAuth app, which can then make API requests on your behalf.

All of these requests count towards your personal rate limit of 5,000 requests per hour

@EresDev
Copy link
Contributor

EresDev commented Mar 21, 2024

it's 5k request an hour to get rate limited while logged in, how I can QA that lmao

I have used k6 for load testing. Works nice.

@el-buku
Copy link

el-buku commented Mar 21, 2024

Is this still available?

@0x4007
Copy link
Member Author

0x4007 commented Mar 21, 2024

Is this still available?

@FernandVEYRIER can you take over the pull review and merge if this is completed correctly?

@gentlementlegen
Copy link
Member

Is this still available?

@FernandVEYRIER can you take over the pull review and merge if this is completed correctly?

@pavlovcik Will do. Seems comments are maybe not all resolved yet in the pull request linked to this task.

Copy link

ubiquibot bot commented May 5, 2024

! action has an uncaught error

@ubiquity ubiquity deleted a comment from ubiquity-os-main bot May 5, 2024
@ubiquibot ubiquibot bot unassigned Keyrxng May 5, 2024
Copy link

ubiquibot bot commented May 5, 2024

! action has an uncaught error

@ubiquity ubiquity deleted a comment from ubiquity-os-main bot May 5, 2024
Copy link

ubiquibot bot commented May 5, 2024

! action has an uncaught error

Copy link

[ 0 WXDAI ]

@Keyrxng
Contributions Overview
View Contribution Count Reward
Conversation Incentives
Comment Formatting Relevance Reward

[ 0.4 WXDAI ]

@EresDev
Contributions Overview
View Contribution Count Reward
Issue Comment 1 0.4
Conversation Incentives
Comment Formatting Relevance Reward
I have used [k6](https://k6.io/) for load testing. Works nice.
1
p:
  count: 9
  score: 1
a:
  count: 1
  score: 1
0.4 0.4

[ 0.013 WXDAI ]

@el-buku
Contributions Overview
View Contribution Count Reward
Issue Comment 1 0.013
Conversation Incentives
Comment Formatting Relevance Reward
Is this still available?
0.1
p:
  count: 4
  score: 1
0.13 0.013

[ 15.682 WXDAI ]

@0x4007
Contributions Overview
View Contribution Count Reward
Issue Specification 1 1.853
Issue Comment 1 1.32
Review Comment 15 12.509
Conversation Incentives
Comment Formatting Relevance Reward
There's a bug that shows the rate limit modal even after logging…
10.9
p:
  count: 109
  score: 1
0.17 1.853
@FernandVEYRIER can you take over the pull review and merge if t…
3
p:
  count: 15
  score: 1
0.44 1.32
What is this? There should always be documentation around enviro…
4.3
p:
  count: 43
  score: 1
0.29 1.247
Is it for testing? Can you rename the property name to make this…
1.4
p:
  count: 14
  score: 1
0.47 0.658
I think it makes more sense to directly pass in the full string …
2.8
p:
  count: 27
  score: 1
code:
  count: 1
  score: 1
0.42 1.176
Can you use normal if/else? Ternary is less readable.
0.9
p:
  count: 9
  score: 1
0.2 0.18
What exactly are the consequences of this? It's a bad idea to ma…
2
p:
  count: 20
  score: 1
0.33 0.66
I think we should still show the modal even if only some of the …
1.7
p:
  count: 17
  score: 1
0.29 0.493
This defaults to null or undefined? I suspect undefined.
0.9
p:
  count: 9
  score: 1
0.25 0.225
This shouldn't happen, nor should this be permissible with a con…
2.5
p:
  count: 25
  score: 1
0.26 0.65
Same here.
0.2
p:
  count: 2
  score: 1
0.28 0.056
@FernandVEYRIER needs to fix the test then. - To clarify, there…
5.6
p:
  count: 56
  score: 1
0.36 2.016
It uses local time. This is relevant for the user, and in my tes…
4
p:
  count: 40
  score: 1
0.42 1.68
Is this the best method to use to get the user? Doesn't octokit …
1.7
p:
  count: 17
  score: 1
0.44 0.748
```suggestion await handleRateLimit(providerToken ?? octok…
1.6
p:
  count: 10
  score: 1
code:
  count: 6
  score: 1
0.35 0.56
You just need to refresh ~80 times within an hour assuming 30 ta…
4.9
p:
  count: 49
  score: 1
0.27 1.323
It must have been from before I implemented cache then. If it's …
2.7
p:
  count: 27
  score: 1
0.31 0.837

[ 24.406 WXDAI ]

@gentlementlegen
Contributions Overview
View Contribution Count Reward
Issue Comment 1 0.437
Review Comment 18 23.969
Conversation Incentives
Comment Formatting Relevance Reward
@pavlovcik Will do. Seems comments are maybe not all resolved ye…
1.9
p:
  count: 19
  score: 1
0.23 0.437
Good to go when @pavlovcik is
0.6
p:
  count: 6
  score: 1
0.22 0.132
Can't this be wrapped within an `expect toThrow` check?
1.1
p:
  count: 9
  score: 1
code:
  count: 2
  score: 1
0.1 0.11
I believe the mock just needs to be updated with the proper ID. …
3.8
p:
  count: 38
  score: 1
0.66 2.508
I can give a look later. But if that is the only way, make sure …
3.2
p:
  count: 32
  score: 1
0.47 1.504
I think this should be removed, as well as the `throw` inside th…
3.7
p:
  count: 34
  score: 1
code:
  count: 3
  score: 1
0.53 1.961
I believe it would be this one https://github.com/ubiquity/work.…
2.3
p:
  count: 23
  score: 1
0.19 0.437
Is there no better check that this? If one letter in the text ch…
2.8
p:
  count: 26
  score: 1
code:
  count: 2
  score: 1
0.32 0.896
I mean that the only check here should be `code === '500'`. `err…
5.1
p:
  count: 44
  score: 1
code:
  count: 7
  score: 1
0.5 2.55
```suggestion displayPopupMessage(`GitHub Login Provider`,…
3.8
p:
  count: 19
  score: 1
code:
  count: 19
  score: 1
0.69 2.622
Shouldn't `(!user)` be enough for this check?
0.8
p:
  count: 7
  score: 1
code:
  count: 1
  score: 1
0.21 0.168
Instead of checking the error message string, I think it might b…
3.7
p:
  count: 37
  score: 1
0.24 0.888
Any reason for this export? I do not see it called anywhere in t…
1.9
p:
  count: 19
  score: 1
0.31 0.589
Does this take the UTC into account when displayed?
0.9
p:
  count: 9
  score: 1
0.5 0.45
This displays a TypeScript error as `error` is `unknown`
1.1
p:
  count: 9
  score: 1
code:
  count: 2
  score: 1
0.55 0.605
While I am not against the idea, I think it is better to check t…
3.7
p:
  count: 34
  score: 1
code:
  count: 1
  score: 1
a:
  count: 2
  score: 1
0.41 1.517
It does but afaik it only works with username, not the user id h…
2.2
p:
  count: 22
  score: 1
0.51 1.122
The rate limited cases can be very easily simulated within Cypre…
4.6
p:
  count: 46
  score: 1
0.69 3.174
There are JSON example responses in the docs, they don't include…
3.8
p:
  count: 38
  score: 1
0.72 2.736

@0x4007 0x4007 reopened this May 5, 2024
@0x4007 0x4007 closed this as completed May 5, 2024
Copy link

ubiquibot bot commented May 5, 2024

+ Evaluating results. Please wait...

Copy link

ubiquibot bot commented May 5, 2024

[ 45 WXDAI ]

@0x4007
Contributions Overview
ViewContributionCountReward
IssueSpecification126.2
IssueComment13
ReviewComment215.8
Conversation Incentives
CommentFormattingRelevanceReward
There's a bug that shows the rate limit modal even after logging...
26.2
h3:
  count: 2
  score: "2"
  words: 4
li:
  count: 3
  score: "3"
  words: 28
126.2
> Is this still available?

@FernandVEYRIER can you take over ...

30.443
> I couldn't QA for being limited while auth'd for obv reasons b...
9.80.89.8
It must have been from before I implemented cache then. If it's ...
60.756

[ 10.4 WXDAI ]

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueComment11.9
ReviewComment28.5
Conversation Incentives
CommentFormattingRelevanceReward
> > Is this still available? > > @FernandVEYRIER can you take...
1.90.411.9
The rate limited cases can be very easily simulated within Cypre...
4.60.7054.6
> > The rate limited cases can be very easily simulated within C...
3.90.7753.9

[ 1.9 WXDAI ]

@EresDev
Contributions Overview
ViewContributionCountReward
IssueComment11.9
Conversation Incentives
CommentFormattingRelevanceReward
> it's 5k request an hour to get rate limited while logged in, h...
1.9
a:
  count: 1
  score: "1"
  words: 1
0.671.9

[ 33 WXDAI ]

@Keyrxng
Contributions Overview
ViewContributionCountReward
IssueComment11.9
ReviewComment531.1
Conversation Incentives
CommentFormattingRelevanceReward
it's 5k request an hour to get rate limited while logged in, how...
1.90.291.9
Me trying to get rate limited spamming refresh and calling ``fet...
6.55
code:
  count: 1
  score: "0.25"
  words: 3
0.686.55
Found two scenarios
  1. The auth provider can't be reached and...
12.85
li:
  count: 2
  score: "0.5"
  words: 41
code:
  count: 3
  score: "0.75"
  words: 25
0.7112.85
> The rate limited cases can be very easily simulated within Cyp...
10.30.7210.3
Not a problem, will get on this today ...
0.80.740.8
Can this be merged? @0x4007 @gentlementlegen ...
0.60.7350.6

@rndquu rndquu reopened this May 10, 2024
Copy link

ubiquibot bot commented May 10, 2024

@Keyrxng the deadline is at 2024-05-10T15:20:51.906Z

@rndquu rndquu closed this as completed May 10, 2024
Copy link

ubiquibot bot commented May 10, 2024

+ Evaluating results. Please wait...

Copy link

ubiquibot bot commented May 10, 2024

[ 45 WXDAI ]

@0x4007
Contributions Overview
ViewContributionCountReward
IssueSpecification126.2
IssueComment13
ReviewComment215.8
Conversation Incentives
CommentFormattingRelevanceReward
There's a bug that shows the rate limit modal even after logging...
26.2
h3:
  count: 2
  score: "2"
  words: 4
li:
  count: 3
  score: "3"
  words: 28
126.2
> Is this still available?

@FernandVEYRIER can you take over ...

30.363
> I couldn't QA for being limited while auth'd for obv reasons b...
9.80.789.8
It must have been from before I implemented cache then. If it's ...
60.666

[ 10.4 WXDAI ]

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueComment11.9
ReviewComment28.5
Conversation Incentives
CommentFormattingRelevanceReward
> > Is this still available? > > @FernandVEYRIER can you take...
1.90.361.9
The rate limited cases can be very easily simulated within Cypre...
4.60.7254.6
> > The rate limited cases can be very easily simulated within C...
3.90.823.9

[ 1.9 WXDAI ]

@EresDev
Contributions Overview
ViewContributionCountReward
IssueComment11.9
Conversation Incentives
CommentFormattingRelevanceReward
> it's 5k request an hour to get rate limited while logged in, h...
1.9
a:
  count: 1
  score: "1"
  words: 1
0.681.9

[ 335.6 WXDAI ]

@Keyrxng
Contributions Overview
ViewContributionCountReward
IssueTask1300
IssueComment10
ReviewComment535.6
Conversation Incentives
CommentFormattingRelevanceReward
it's 5k request an hour to get rate limited while logged in, how...
-0.26-
Me trying to get rate limited spamming refresh and calling ``fet...
7.3
code:
  count: 1
  score: "1"
  words: 3
0.687.3
Found two scenarios
  1. The auth provider can't be reached and...
16.6
li:
  count: 2
  score: "2"
  words: 41
code:
  count: 3
  score: "3"
  words: 25
0.7416.6
> The rate limited cases can be very easily simulated within Cyp...
10.30.710.3
Not a problem, will get on this today ...
0.80.7650.8
Can this be merged? @0x4007 @gentlementlegen ...
0.60.770.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants