chore: rate limit modal and auto login #22

Keyrxng · 2024-03-03T18:37:17Z

Resolves #20

with tasks and no user we display the login rate limit
With no tasks and no user we display the login rate limit
with no tasks and a user we force another login (logging in required a fresh to fetch the issues, this prevents that manual refresh)
with tasks and a user we display an alternative rate limit message
added a new env var for SUPABASE_REF and a little helper so it's easier for the likes of myself

I tried to debug why the issues wouldn't load first attempt after logging in following being rate limited but couldn't see why. I presume it's to do with token storage as you clearly see the auth token being handled in the URL.

I couldn't QA for being limited while auth'd for obv reasons but all bases are covered

0x4007 · 2024-03-04T07:35:28Z

I couldn't QA for being limited while auth'd for obv reasons but all bases are covered

You just need to refresh ~80 times within an hour assuming 30 tasks. I was able to hit these limits while developing the UI.

Each "task" that is loaded invokes three requests: 1. the preview 2. the full details 3. sometimes, the image (although i have caching for this)

.env.example

cypress/e2e/devpool.cy.ts

src/home/fetch-github/fetch-issues-preview.ts

Keyrxng · 2024-03-04T10:29:07Z

Me trying to get rate limited spamming refresh and calling fetchAndDisplayPreviewsFromCache() & fetchIssuesFull() x30 lmao

I'm seeing no success so I check my rate limit remaining in a separate script, still 5K.

Running it down for QA sake but I'm amazed you were able to hit that locally, I had cache logic disabled and was clearing browser cache every run or two

0x4007 · 2024-03-04T11:33:36Z

It must have been from before I implemented cache then. If it's really not going down due to cache then I'd say don't worry about it anymore.

Keyrxng · 2024-03-04T12:58:10Z

Found two scenarios

The auth provider can't be reached and returns an error instead of the auth token into the url
You get the API rate limit exceeded for user ID 106.... If you reach out to GitHub Support for help, ...

Got the first one maxing it the first time, then number two the second time.

I'd say it's very unlikely to happen but I've added a notice for them anyway.

The getNewSessionToken() error handling isn't very robust but I wasn't sure whether to log any error into the modal or be explicit with them Error getting user profile from external provider could be a number of things I suppose

gentlementlegen · 2024-03-12T06:48:27Z

The rate limited cases can be very easily simulated within Cypress by intercepting routes and sending back the appropriate response. It should help you develop and test these cases without having to really spam the endpoint. Also, would be nice to see these testing cases implemented.

Keyrxng · 2024-03-12T10:31:05Z

The rate limited cases can be very easily simulated within Cypress by intercepting routes and sending back the appropriate response. It should help you develop and test these cases without having to really spam the endpoint. Also, would be nice to see these testing cases implemented.

Makes sense. I'm not as exp with testing as you seem to be and my default is usually: mocking requires pre-knowledge of what's going to happen for a fact, as opposed to simulating a response I never knew existed and even if I do know it exists, mocking it sort of defeats the purpose lmao probs a naïve outlook and why I'm not so great with writing them. Always happy to learn and improve but so

As part of this PR would you like me to add in test cases to cover what I've done here? @FernandVEYRIER

gentlementlegen · 2024-03-12T10:38:47Z

The rate limited cases can be very easily simulated within Cypress by intercepting routes and sending back the appropriate response. It should help you develop and test these cases without having to really spam the endpoint. Also, would be nice to see these testing cases implemented.

Makes sense. I'm not as exp with testing as you seem to be and my default is usually: mocking requires pre-knowledge of what's going to happen for a fact, as opposed to simulating a response I never knew existed and even if I do know it exists, mocking it sort of defeats the purpose lmao probs a naïve outlook and why I'm not so great with writing them. Always happy to learn and improve but so

As part of this PR would you like me to add in test cases to cover what I've done here? @FernandVEYRIER

There are JSON example responses in the docs, they don't include failed cases but for each route you have the return codes, which I believe should suffice. That would be really great to have some test for sure!

Keyrxng · 2024-03-12T10:40:16Z

Not a problem, will get on this today

ubiquity-os-deployer · 2024-03-13T00:17:44Z

448fbe0

b650c13

e5c6669

5b66bd4

dac1eab

9c26568

6465f5c

68a77c0

f676a93

365aafa

2672cbf

src/home/fetch-github/fetch-and-display-previews.ts

src/home/fetch-github/fetch-issues-preview.ts

src/home/getters/get-github-user.ts

src/home/sorting/sorting-manager.ts

cypress/e2e/devpool.cy.ts

src/home/getters/get-github-user.ts

Co-authored-by: Mentlegen <9807008+gentlementlegen@users.noreply.github.com>

src/home/fetch-github/fetch-issues-preview.ts

src/home/getters/get-github-user.ts

src/home/rendering/render-github-login-button.ts

gentlementlegen · 2024-04-08T01:19:08Z

src/home/fetch-github/fetch-issues-preview.ts

+  const resetParsed = rate.reset && new Date(rate.reset * 1000).toLocaleTimeString();
+
+  if (!rate.user) {
+    rateLimitModal(`You have been rate limited. Please log in to GitHub to increase your GitHub API limits, otherwise you can try again at ${resetParsed}.`);


Does this take the UTC into account when displayed?

.toLocaleTimeString();

It uses local time. This is relevant for the user, and in my testing, it is accurate.

Just realized, this pull needs to be rebased/reopened. Pretty sure this is my code from a long time ago that's already in production.

https://github.com/ubiquity/work.ubq.fi/blame/ecb2187ddc2067e99ae806af3430c67bf19a8d9d/src/home/fetch-github/fetch-issues-preview.ts#L100

The code is similar I guess but this pr is up to date with development if that's where prod is running from

src/home/getters/get-github-user.ts

gentlementlegen

Good to go when @pavlovcik is

src/home/getters/get-github-user.ts

0x4007 · 2024-04-09T01:48:08Z

src/home/getters/get-github-user.ts

+    return response.data;
+  } catch (error) {
+    if (error instanceof RequestError && error.status === 403) {
+      await handleRateLimit(providerToken ? octokit : undefined, error);


Suggested change

await handleRateLimit(providerToken ? octokit : undefined, error);

await handleRateLimit(providerToken ?? octokit, error);

Is this more concise?

Although if there is a providerToken and we get an error it'll pass providerToken as an arg instead of octokit which would mean rebuilding octokit within handleRateLimit() while both invocations of handleRateLimit() already have octokit built, one is guaranteed to have a provider token and this one in question is not.

If we make this more concise we litter the rate limit function

I guess if(typeof octokit == "string") would work instead of if(octokit){}else{} but typing octokit as string seemed daft when it first popped into my head 😂

Keyrxng · 2024-04-23T11:42:23Z

Can this be merged? @0x4007 @gentlementlegen

…lt-conf feat: cypress default configuration

0x4007 reviewed Mar 4, 2024

View reviewed changes

.env.example Outdated Show resolved Hide resolved

0x4007 reviewed Mar 4, 2024

View reviewed changes

cypress/e2e/devpool.cy.ts Outdated Show resolved Hide resolved

0x4007 reviewed Mar 4, 2024

View reviewed changes

src/home/fetch-github/fetch-issues-preview.ts Outdated Show resolved Hide resolved

0x4007 requested changes Mar 4, 2024

View reviewed changes

src/home/fetch-github/fetch-issues-preview.ts Outdated Show resolved Hide resolved

Keyrxng force-pushed the development branch from 167310b to 448fbe0 Compare March 13, 2024 00:16

gentlementlegen self-requested a review March 13, 2024 05:55

0x4007 reviewed Mar 13, 2024

View reviewed changes

src/home/fetch-github/fetch-and-display-previews.ts Outdated Show resolved Hide resolved

0x4007 reviewed Mar 13, 2024

View reviewed changes

src/home/fetch-github/fetch-issues-preview.ts Outdated Show resolved Hide resolved

0x4007 reviewed Mar 13, 2024

View reviewed changes

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

0x4007 reviewed Mar 13, 2024

View reviewed changes

src/home/sorting/sorting-manager.ts Outdated Show resolved Hide resolved

0x4007 reviewed Mar 13, 2024

View reviewed changes

src/home/sorting/sorting-manager.ts Outdated Show resolved Hide resolved

gentlementlegen requested changes Mar 13, 2024

View reviewed changes

cypress/e2e/devpool.cy.ts Outdated Show resolved Hide resolved

Keyrxng added 7 commits March 22, 2024 11:29

chore: rate limit modal and auto login

db89905

chore: rate limit scenarios

2f026db

chore: tests and minor fix

473078e

fix: throw errs, remove task len for rate limit

30957fc

chore: update test err handling

00f7a15

fix: dont throw for no token

301729a

fix: remove githubUser

e5c6669

Keyrxng force-pushed the development branch from b650c13 to e5c6669 Compare March 22, 2024 11:45

gentlementlegen requested changes Mar 22, 2024

View reviewed changes

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

Keyrxng added 2 commits March 22, 2024 14:45

fix: remove err and code check

5b66bd4

fix: use code, console.err and show toast regardless

065c84e

gentlementlegen requested changes Mar 25, 2024

View reviewed changes

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

Update src/home/getters/get-github-user.ts

dac1eab

Co-authored-by: Mentlegen <9807008+gentlementlegen@users.noreply.github.com>

gentlementlegen requested changes Mar 26, 2024

View reviewed changes

src/home/fetch-github/fetch-issues-preview.ts Outdated Show resolved Hide resolved

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

src/home/rendering/render-github-login-button.ts Outdated Show resolved Hide resolved

Keyrxng added 2 commits March 28, 2024 08:47

Merge branch 'ubiquity:development' into development

9c26568

fix: handle rate limit

6465f5c

gentlementlegen reviewed Apr 8, 2024

View reviewed changes

gentlementlegen requested changes Apr 8, 2024

View reviewed changes

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

chore: allow catch block arg cast to any

68a77c0

gentlementlegen requested changes Apr 8, 2024

View reviewed changes

src/home/getters/get-github-user.ts Outdated Show resolved Hide resolved

Keyrxng added 2 commits April 8, 2024 23:45

chore: use request error class

f676a93

chore: cast catch as unknown

365aafa

gentlementlegen approved these changes Apr 8, 2024

View reviewed changes

0x4007 reviewed Apr 9, 2024

View reviewed changes

gentlementlegen requested a review from 0x4007 April 25, 2024 03:40

gentlementlegen merged commit 2672cbf into ubiquity:development May 5, 2024
1 check passed

ubiquity-os-main bot mentioned this pull request May 5, 2024

GitHub Rate Limiting Modal on Load #20

Closed

ghost pushed a commit to ariesgun/work.ubq.fi that referenced this pull request Oct 13, 2024

Merge pull request ubiquity#22 from FernandVEYRIER/feat/cypress-defau…

438da50

…lt-conf feat: cypress default configuration

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: rate limit modal and auto login #22

chore: rate limit modal and auto login #22

Keyrxng commented Mar 3, 2024

0x4007 commented Mar 4, 2024

Keyrxng commented Mar 4, 2024 •

edited

Loading

0x4007 commented Mar 4, 2024

Keyrxng commented Mar 4, 2024 •

edited

Loading

gentlementlegen commented Mar 12, 2024

Keyrxng commented Mar 12, 2024

gentlementlegen commented Mar 12, 2024

Keyrxng commented Mar 12, 2024

ubiquity-os-deployer bot commented Mar 13, 2024 •

edited

Loading

gentlementlegen Apr 8, 2024

0x4007 Apr 8, 2024 •

edited

Loading

Keyrxng Apr 8, 2024

gentlementlegen left a comment

0x4007 Apr 9, 2024

Keyrxng Apr 9, 2024 •

edited

Loading

Keyrxng commented Apr 23, 2024

	await handleRateLimit(providerToken ? octokit : undefined, error);
	await handleRateLimit(providerToken ?? octokit, error);

chore: rate limit modal and auto login #22

chore: rate limit modal and auto login #22

Conversation

Keyrxng commented Mar 3, 2024

0x4007 commented Mar 4, 2024

Keyrxng commented Mar 4, 2024 • edited Loading

0x4007 commented Mar 4, 2024

Keyrxng commented Mar 4, 2024 • edited Loading

gentlementlegen commented Mar 12, 2024

Keyrxng commented Mar 12, 2024

gentlementlegen commented Mar 12, 2024

Keyrxng commented Mar 12, 2024

ubiquity-os-deployer bot commented Mar 13, 2024 • edited Loading

gentlementlegen Apr 8, 2024

Choose a reason for hiding this comment

0x4007 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Keyrxng Apr 8, 2024

Choose a reason for hiding this comment

gentlementlegen left a comment

Choose a reason for hiding this comment

0x4007 Apr 9, 2024

Choose a reason for hiding this comment

Keyrxng Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Keyrxng commented Apr 23, 2024

Keyrxng commented Mar 4, 2024 •

edited

Loading

Keyrxng commented Mar 4, 2024 •

edited

Loading

ubiquity-os-deployer bot commented Mar 13, 2024 •

edited

Loading

0x4007 Apr 8, 2024 •

edited

Loading

Keyrxng Apr 9, 2024 •

edited

Loading