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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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=
2 changes: 1 addition & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@
{ "selector": "variable", "types": ["boolean"], "format": ["PascalCase"], "prefix": ["is", "has", "should", "can", "did", "will"] }
]
}
}
}
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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"open-source"
],
"dependencies": {
"@octokit/request-error": "^6.1.0",
"@octokit/rest": "^20.0.2",
"@supabase/supabase-js": "^2.39.0",
"dotenv": "^16.3.1",
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
64 changes: 46 additions & 18 deletions src/home/fetch-github/fetch-issues-preview.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
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";
import { RequestError } from "@octokit/request-error";

async function checkPrivateRepoAccess(): Promise<boolean> {
const octokit = new Octokit({ auth: await getGitHubAccessToken() });
Expand All @@ -23,7 +24,7 @@ async function checkPrivateRepoAccess(): Promise<boolean> {
}
return false;
} catch (error) {
if (error.status === 404) {
if (error instanceof RequestError && error.status === 404) {
// If the status is 404, it means the user is not a collaborator, hence no access
return false;
} else {
Expand All @@ -39,7 +40,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 +77,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 instanceof RequestError && 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 +93,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?: RequestError) {
const rate: RateLimit = {
reset: null,
user: false,
};

if (error?.response?.headers["x-ratelimit-reset"]) {
rate.reset = parseInt(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}.`);
}
}
27 changes: 17 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,13 @@ 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";
import { RequestError } from "@octokit/request-error";
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 +28,22 @@ 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) {
if (error instanceof RequestError && error.status === 403) {
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 😂

}
}
return null;
}
8 changes: 0 additions & 8 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
{
"compilerOptions": {
/* Visit https://aka.ms/tsconfig to read more about this file */

/* Projects */
// "incremental": true, /* Save .tsbuildinfo files to allow for incremental compilation of projects. */
// "composite": true, /* Enable constraints that allow a TypeScript project to be used with project references. */
// "tsBuildInfoFile": "./.tsbuildinfo", /* Specify the path to .tsbuildinfo incremental compilation file. */
// "disableSourceOfProjectReferenceRedirect": true, /* Disable preferring source files instead of declaration files when referencing composite projects. */
// "disableSolutionSearching": true, /* Opt a project out of multi-project reference checking when editing. */
// "disableReferencedProjectLoad": true, /* Reduce the number of projects loaded automatically by TypeScript. */

/* Language and Environment */
"target": "es2016" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
"lib": ["DOM", "ESNext"] /* Specify a set of bundled library declaration files that describe the target runtime environment. */,
Expand All @@ -23,7 +21,6 @@
// "noLib": true, /* Disable including any library files, including the default lib.d.ts. */
// "useDefineForClassFields": true, /* Emit ECMAScript-standard-compliant class fields. */
// "moduleDetection": "auto", /* Control what method is used to detect module-format JS files. */

/* Modules */
"module": "commonjs" /* Specify what module code is generated. */,
// "rootDir": "./", /* Specify the root folder within your source files. */
Expand All @@ -42,12 +39,10 @@
"resolveJsonModule": true /* Enable importing .json files. */,
// "allowArbitraryExtensions": true, /* Enable importing files with any extension, provided a declaration file is present. */
// "noResolve": true, /* Disallow 'import's, 'require's or '<reference>'s from expanding the number of files TypeScript should add to a project. */

/* JavaScript Support */
// "allowJs": true, /* Allow JavaScript files to be a part of your program. Use the 'checkJS' option to get errors from these files. */
// "checkJs": true, /* Enable error reporting in type-checked JavaScript files. */
// "maxNodeModuleJsDepth": 1, /* Specify the maximum folder depth used for checking JavaScript files from 'node_modules'. Only applicable with 'allowJs'. */

/* Emit */
// "declaration": true, /* Generate .d.ts files from TypeScript and JavaScript files in your project. */
// "declarationMap": true, /* Create sourcemaps for d.ts files. */
Expand All @@ -72,15 +67,13 @@
// "preserveConstEnums": true, /* Disable erasing 'const enum' declarations in generated code. */
// "declarationDir": "./", /* Specify the output directory for generated declaration files. */
// "preserveValueImports": true, /* Preserve unused imported values in the JavaScript output that would otherwise be removed. */

/* Interop Constraints */
// "isolatedModules": true, /* Ensure that each file can be safely transpiled without relying on other imports. */
// "verbatimModuleSyntax": true, /* Do not transform or elide any imports or exports not marked as type-only, ensuring they are written in the output file's format based on the 'module' setting. */
// "allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */
"esModuleInterop": true /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */,
// "preserveSymlinks": true, /* Disable resolving symlinks to their realpath. This correlates to the same flag in node. */
"forceConsistentCasingInFileNames": true /* Ensure that casing is correct in imports. */,

/* Type Checking */
"strict": true /* Enable all strict type-checking options. */,
// "noImplicitAny": true, /* Enable error reporting for expressions and declarations with an implied 'any' type. */
Expand All @@ -101,7 +94,6 @@
// "noPropertyAccessFromIndexSignature": true, /* Enforces using indexed accessors for keys declared using an indexed type. */
// "allowUnusedLabels": true, /* Disable error reporting for unused labels. */
// "allowUnreachableCode": true, /* Disable error reporting for unreachable code. */

/* Completeness */
// "skipDefaultLibCheck": true, /* Skip type checking .d.ts files that are included with TypeScript. */
"skipLibCheck": true /* Skip type checking all .d.ts files. */
Expand Down
Loading