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

chore: rate limit modal and auto login #22

Merged
merged 15 commits into from
May 5, 2024
Merged
2 changes: 1 addition & 1 deletion .env.example
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
SUPABASE_URL=
SUPABASE_ANON_KEY=
SUPABASE_ANON_KEY=
80 changes: 73 additions & 7 deletions cypress/e2e/devpool.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,80 @@ describe("DevPool", () => {
cy.get('div[id="issues-container"]').children().should("have.length", 2);
});

it("Display a message on rate limited", () => {
cy.intercept("https://api.github.com/repos/*/*/issues**", (req) => {
req.reply({
statusCode: 403,
describe("Display message on rate limited", () => {
const HHMMSS_REGEX = /([01]?[0-9]|2[0-3]):([0-5]?[0-9]):([0-5]?[0-9])/;
const PLEASE_LOG_IN = "Please log in to GitHub to increase your GitHub API limits, otherwise you can try again at";
const RATE_LIMITED = "You have been rate limited. Please try again at";

beforeEach(() => {
cy.intercept("https://api.github.com/rate_limit", {
statusCode: 200,
body: {
resources: {
core: {
limit: 5000,
used: 5000,
remaining: 0,
reset: 1617700000,
},
},
},
});
}).as("getIssues");
cy.visit("/");
cy.get(".preview-header").should("exist");
cy.intercept("https://api.github.com/user", (req) => {
req.reply({
statusCode: 403,
body: {},
headers: { "x-ratelimit-reset": "1617700000" },
});
}).as("getUser");
cy.intercept("https://api.github.com/repos/*/*/issues**", (req) => {
req.reply({
statusCode: 403,
headers: { "x-ratelimit-reset": "1617700000" },
});
}).as("getIssues");
});

it("Should display retry timeframe and login request with no tasks and no user", () => {
cy.visit("/");
cy.get(".preview-header").should("exist");
cy.get(".preview-body-inner").should(($body) => {
const text = $body.text();
expect(text).to.include(PLEASE_LOG_IN);
expect(HHMMSS_REGEX.test(text)).to.be.true;
});
});

it("Should display retry timeframe with no tasks loaded and a logged in user", () => {
cy.intercept("https://api.github.com/user", {
statusCode: 200,
body: { login: "mockUser" },
}).as("getUser");

cy.visit("/");
cy.get(".preview-header").should("exist");
cy.get(".preview-body-inner").should(($body) => {
const text = $body.text();
expect(text).to.include(RATE_LIMITED);
expect(HHMMSS_REGEX.test(text)).to.be.true;
});
});

it("Should log an error if the auth provider fails", () => {
cy.on("window:before:load", (win) => {
cy.stub(win.console, "error").as("consoleError");
});

const urlParams = `#error=server_error&error_code=500&error_description=Error getting user profile from external provider`;
cy.visit(`/${urlParams}`);
cy.get(".preview-header").should("exist");
cy.get(".preview-body-inner").should(($body) => {
const text = $body.text();
expect(text).to.include(PLEASE_LOG_IN);
expect(HHMMSS_REGEX.test(text)).to.be.true;
});
cy.get("@consoleError").should("be.calledWith", "GitHub login provider: Error getting user profile from external provider");
});
});

it("Items can be sorted", () => {
Expand Down
2 changes: 1 addition & 1 deletion src/home/fetch-github/fetch-and-display-previews.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export async function fetchAvatars() {
const urlPattern = /https:\/\/github\.com\/(?<org>[^/]+)\/(?<repo>[^/]+)\/issues\/(?<issue_number>\d+)/;

const avatarPromises = cachedTasks.map(async (task) => {
const match = task.preview.body.match(urlPattern);
const match = task.preview.body?.match(urlPattern);
const orgName = match?.groups?.org;
if (orgName) {
return fetchAvatar(orgName);
Expand Down
61 changes: 44 additions & 17 deletions src/home/fetch-github/fetch-issues-preview.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Octokit } from "@octokit/rest";
import { getGitHubAccessToken, getGitHubUserName } from "../getters/get-github-access-token";
import { GitHubIssue } from "../github-types";
import { taskManager } from "../home";
import { displayPopupMessage } from "../rendering/display-popup-modal";
import { TaskNoFull } from "./preview-to-full-mapping";
import { getGitHubUser } from "../getters/get-github-user";

async function checkPrivateRepoAccess(): Promise<boolean> {
const octokit = new Octokit({ auth: await getGitHubAccessToken() });
Expand Down Expand Up @@ -39,7 +39,6 @@ async function checkPrivateRepoAccess(): Promise<boolean> {

export async function fetchIssuePreviews(): Promise<TaskNoFull[]> {
const octokit = new Octokit({ auth: await getGitHubAccessToken() });

let freshIssues: GitHubIssue[] = [];
let hasPrivateRepoAccess = false; // Flag to track access to the private repository

Expand Down Expand Up @@ -77,14 +76,10 @@ export async function fetchIssuePreviews(): Promise<TaskNoFull[]> {
freshIssues = publicIssues;
}
} catch (error) {
if (403 === error.status) {
console.error(`GitHub API rate limit exceeded.`);
if (taskManager.getTasks().length == 0) {
// automatically login if there are no issues loaded
automaticLogin(error);
}
if (error.status === 403) {
await handleRateLimit(octokit, error);
} else {
console.error(`Failed to fetch issue previews: ${error}`);
console.error("Error fetching issue previews:", error);
}
}

Expand All @@ -97,12 +92,44 @@ export async function fetchIssuePreviews(): Promise<TaskNoFull[]> {

return tasks;
}
function automaticLogin(error: unknown) {
const resetTime = error.response.headers["x-ratelimit-reset"];
const resetParsed = new Date(resetTime * 1000).toLocaleTimeString();

displayPopupMessage(
`GitHub API rate limit exceeded.`,
`You have been rate limited. Please log in to GitHub to increase your GitHub API limits, otherwise you can try again at ${resetParsed}.`
);

function rateLimitModal(message: string) {
displayPopupMessage(`GitHub API rate limit exceeded.`, message);
}

type RateLimit = {
reset: number | null;
user: boolean;
};

export async function handleRateLimit(octokit?: Octokit, error?: unknown) {
const rate: RateLimit = {
reset: null,
user: false,
};

if (error) {
rate.reset = error.response.headers["x-ratelimit-reset"];
}

if (octokit) {
try {
const core = await octokit.rest.rateLimit.get();
const remaining = core.data.resources.core.remaining;
const reset = core.data.resources.core.reset;

rate.reset = !rate.reset && remaining === 0 ? reset : rate.reset;
rate.user = (await getGitHubUser()) ? true : false;
} catch (err) {
console.error("Error handling GitHub rate limit", err);
}
}

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}.`);
Copy link
Member

Choose a reason for hiding this comment

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

Does this take the UTC into account when displayed?

Copy link
Member

@0x4007 0x4007 Apr 8, 2024

Choose a reason for hiding this comment

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

.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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} else {
rateLimitModal(`You have been rate limited. Please try again at ${resetParsed}.`);
}
}
29 changes: 19 additions & 10 deletions src/home/getters/get-github-user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,12 @@ import { Octokit } from "@octokit/rest";
import { GitHubUser, GitHubUserResponse } from "../github-types";
import { OAuthToken } from "./get-github-access-token";
import { getLocalStore } from "./get-local-store";
import { handleRateLimit } from "../fetch-github/fetch-issues-preview";
declare const SUPABASE_STORAGE_KEY: string; // @DEV: passed in at build time check build/esbuild-build.ts

export async function getGitHubUser(): Promise<GitHubUser | null> {
const activeSessionToken = await getSessionToken();
if (activeSessionToken) {
return getNewGitHubUser(activeSessionToken);
} else {
return null;
}
return getNewGitHubUser(activeSessionToken);
}

async function getSessionToken(): Promise<string | null> {
Expand All @@ -30,13 +27,25 @@ async function getNewSessionToken(): Promise<string | null> {
const params = new URLSearchParams(hash.substr(1)); // remove the '#' and parse
const providerToken = params.get("provider_token");
if (!providerToken) {
return null;
const error = params.get("error_description");
// supabase auth provider has failed for some reason
console.error(`GitHub login provider: ${error}`);
}
return providerToken;
return providerToken || null;
}

async function getNewGitHubUser(providerToken: string): Promise<GitHubUser> {
async function getNewGitHubUser(providerToken: string | null): Promise<GitHubUser | null> {
const octokit = new Octokit({ auth: providerToken });
const response = (await octokit.request("GET /user")) as GitHubUserResponse;
return response.data;

try {
const response = (await octokit.request("GET /user")) as GitHubUserResponse;
0x4007 marked this conversation as resolved.
Show resolved Hide resolved
return response.data;
} catch (error: unknown) {
if (error.status === 403) {
Keyrxng marked this conversation as resolved.
Show resolved Hide resolved
await handleRateLimit(providerToken ? octokit : undefined, error);
Copy link
Member

Choose a reason for hiding this comment

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

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

Is this more concise?

Copy link
Contributor Author

@Keyrxng Keyrxng Apr 9, 2024

Choose a reason for hiding this comment

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

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 😂

} else {
console.error("Error getting new GitHub user:", error);
}
}
return null;
}