From 7d83224ce4d9905013a01993a2fe4e921cdffee7 Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Wed, 20 Nov 2024 13:11:02 +0000 Subject: [PATCH] Cache prs and check runs for fast load - adds a LocalCache helper for lscache --- .../lib/forge/github/githubChecksMonitor.ts | 84 +++++++------------ .../src/lib/forge/github/githubPrMonitor.ts | 8 ++ .../src/lib/forge/github/githubPrService.ts | 30 ++++++- apps/desktop/src/lib/forge/github/types.ts | 53 +++++++++++- .../src/lib/forge/interface/forgePrService.ts | 2 +- packages/shared/package.json | 6 +- packages/shared/src/lib/localCache.ts | 35 ++++++++ 7 files changed, 157 insertions(+), 61 deletions(-) create mode 100644 packages/shared/src/lib/localCache.ts diff --git a/apps/desktop/src/lib/forge/github/githubChecksMonitor.ts b/apps/desktop/src/lib/forge/github/githubChecksMonitor.ts index bcad860897..4db54a4335 100644 --- a/apps/desktop/src/lib/forge/github/githubChecksMonitor.ts +++ b/apps/desktop/src/lib/forge/github/githubChecksMonitor.ts @@ -1,8 +1,9 @@ import { scurveBackoff } from '$lib/backoff/scurve'; import { DEFAULT_HEADERS } from '$lib/forge/github/headers'; -import { parseGitHubCheckSuites } from '$lib/forge/github/types'; +import { parseGitHubCheckRuns, parseGitHubCheckSuites } from '$lib/forge/github/types'; import { sleep } from '$lib/utils/sleep'; -import { Octokit, type RestEndpointMethodTypes } from '@octokit/rest'; +import { LocalCache } from '@gitbutler/shared/localCache'; +import { Octokit } from '@octokit/rest'; import { writable } from 'svelte/store'; import type { CheckSuites, ChecksStatus } from '$lib/forge/interface/types'; import type { RepoInfo } from '$lib/url/gitUrl'; @@ -27,6 +28,9 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { private timeout: any; private hasCheckSuites: boolean | undefined; + // Stores previously fetched checks results by pr number. + private cache = new LocalCache({ keyPrefix: 'pr-checks', expiry: 1440 }); + constructor( private octokit: Octokit, private repo: RepoInfo, @@ -34,6 +38,7 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { ) {} async start() { + this.loadFromCache(); this.update(); } @@ -42,15 +47,25 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { delete this.timeout; } + loadFromCache() { + const cachedValue = this.cache.get(this.sourceBranch); + try { + const status = parseGitHubCheckRuns(cachedValue); + this._status = status; + this.status.set(status); + } catch { + this.cache.remove(this.sourceBranch); + } + } + async update() { this.error.set(undefined); this.loading.set(true); try { const checks = await this.fetchChecksWithRetries(this.sourceBranch, 5, 2000); - const status = parseChecks(checks); - this.status.set(status); - this._status = status; + this.status.set(checks); + this._status = checks; } catch (e: any) { console.error(e); this.error.set(e.message); @@ -84,9 +99,13 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { return scurveBackoff(ageMs, 10000, 600000); } - private async fetchChecksWithRetries(ref: string, retries: number, delayMs: number) { + private async fetchChecksWithRetries( + ref: string, + retries: number, + delayMs: number + ): Promise { let checks = await this.fetchChecks(ref); - if (checks.total_count > 0) { + if (checks && checks?.totalCount > 0) { this.hasCheckSuites = true; return checks; } @@ -99,7 +118,7 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { if (!this.hasCheckSuites) return checks; let attempts = 0; - while (checks.total_count === 0 && attempts < retries) { + while (checks && checks.totalCount === 0 && attempts < retries) { attempts++; await sleep(delayMs); checks = await this.fetchChecks(ref); @@ -117,57 +136,14 @@ export class GitHubChecksMonitor implements ForgeChecksMonitor { return { count: resp.data.total_count, items: parseGitHubCheckSuites(resp.data) }; } - private async fetchChecks(ref: string) { + private async fetchChecks(ref: string): Promise { const resp = await this.octokit.checks.listForRef({ headers: DEFAULT_HEADERS, owner: this.repo.owner, repo: this.repo.name, ref: ref }); - return resp.data; + this.cache.set(ref, resp.data); + return parseGitHubCheckRuns(resp.data); } } - -function parseChecks( - data: RestEndpointMethodTypes['checks']['listForRef']['response']['data'] -): ChecksStatus | null { - // Fetch with retries since checks might not be available _right_ after - // the pull request has been created. - - // If there are no checks then there is no status to report - const checkRuns = data.check_runs; - if (checkRuns.length === 0) return null; - - // Establish when the first check started running, useful for showing - // how long something has been running. - const starts = checkRuns - .map((run) => run.started_at) - .filter((startedAt) => startedAt !== null) as string[]; - const startTimes = starts.map((startedAt) => new Date(startedAt)); - - const queued = checkRuns.filter((c) => c.status === 'queued').length; - const failed = checkRuns.filter((c) => c.conclusion === 'failure').length; - const skipped = checkRuns.filter((c) => c.conclusion === 'skipped').length; - const succeeded = checkRuns.filter((c) => c.conclusion === 'success').length; - - const firstStart = new Date(Math.min(...startTimes.map((date) => date.getTime()))); - const completed = checkRuns.every((check) => !!check.completed_at); - const totalCount = data.total_count; - - const success = queued === 0 && failed === 0 && skipped + succeeded === totalCount; - const finished = checkRuns.filter( - (c) => c.conclusion && ['failure', 'success'].includes(c.conclusion) - ).length; - - return { - startedAt: firstStart, - hasChecks: !!totalCount, - success, - failed, - completed, - queued, - totalCount, - skipped, - finished - }; -} diff --git a/apps/desktop/src/lib/forge/github/githubPrMonitor.ts b/apps/desktop/src/lib/forge/github/githubPrMonitor.ts index aaf73497ca..1d4ae29058 100644 --- a/apps/desktop/src/lib/forge/github/githubPrMonitor.ts +++ b/apps/desktop/src/lib/forge/github/githubPrMonitor.ts @@ -27,7 +27,15 @@ export class GitHubPrMonitor implements ForgePrMonitor { private prNumber: number ) {} + private loadFromCache() { + const cachedValue = this.prService.getCached(this.prNumber); + if (cachedValue) { + this.pr.set(cachedValue); + } + } + private start() { + this.loadFromCache(); this.fetch(); this.intervalId = setInterval(() => { this.fetch(); diff --git a/apps/desktop/src/lib/forge/github/githubPrService.ts b/apps/desktop/src/lib/forge/github/githubPrService.ts index dc54550c69..14c77756c5 100644 --- a/apps/desktop/src/lib/forge/github/githubPrService.ts +++ b/apps/desktop/src/lib/forge/github/githubPrService.ts @@ -1,7 +1,8 @@ import { GitHubPrMonitor } from './githubPrMonitor'; import { DEFAULT_HEADERS } from './headers'; -import { ghResponseToInstance, parseGitHubDetailedPullRequest } from './types'; +import { ghResponseToInstance, parseGitHubDetailedPullRequest, parseGitHubReview } from './types'; import { sleep } from '$lib/utils/sleep'; +import { LocalCache } from '@gitbutler/shared/localCache'; import posthog from 'posthog-js'; import { writable } from 'svelte/store'; import type { ForgePrService } from '$lib/forge/interface/forgePrService'; @@ -17,6 +18,9 @@ import type { Octokit } from '@octokit/rest'; export class GitHubPrService implements ForgePrService { loading = writable(false); + // Stores previously fetched pull requests by their number. + private cache = new LocalCache({ keyPrefix: 'pr', expiry: 1440 }); + constructor( private octokit: Octokit, private repo: RepoInfo @@ -72,9 +76,33 @@ export class GitHubPrService implements ForgePrService { repo: this.repo.name, pull_number: prNumber }); + this.cache.set(prNumber, resp.data); return parseGitHubDetailedPullRequest(resp.data); } + getCached(prNumber: number): DetailedPullRequest | undefined { + const cachedValue = this.cache.get(prNumber); + try { + return cachedValue ? parseGitHubDetailedPullRequest(cachedValue) : undefined; + } catch { + this.cache.remove(prNumber); + } + } + + private cacheKey(prNumber: number) { + return 'pr-cache-' + prNumber; + } + + async getReviewStatus(prNumber: number): Promise { + const resp = await this.octokit.pulls.listReviews({ + headers: DEFAULT_HEADERS, + owner: this.repo.owner, + repo: this.repo.name, + pull_number: prNumber + }); + return parseGitHubReview(resp.data); + } + async merge(method: MergeMethod, prNumber: number) { await this.octokit.pulls.merge({ owner: this.repo.owner, diff --git a/apps/desktop/src/lib/forge/github/types.ts b/apps/desktop/src/lib/forge/github/types.ts index 31ce1a93a3..9da8eac954 100644 --- a/apps/desktop/src/lib/forge/github/types.ts +++ b/apps/desktop/src/lib/forge/github/types.ts @@ -1,5 +1,11 @@ import { parseRemoteUrl } from '$lib/url/gitUrl'; -import type { CheckSuite, DetailedPullRequest, Label, PullRequest } from '../interface/types'; +import type { + ChecksStatus, + CheckSuite, + DetailedPullRequest, + Label, + PullRequest +} from '../interface/types'; import type { RestEndpointMethodTypes } from '@octokit/rest'; export type DetailedGitHubPullRequest = RestEndpointMethodTypes['pulls']['get']['response']['data']; @@ -82,3 +88,48 @@ export function parseGitHubCheckSuites(data: GitHubListCheckSuitesResp): CheckSu })); return result; } + +export type GitHubListChecksResp = + RestEndpointMethodTypes['checks']['listForRef']['response']['data']; + +export function parseGitHubCheckRuns(data: GitHubListChecksResp): ChecksStatus | null { + // Fetch with retries since checks might not be available _right_ after + // the pull request has been created. + + // If there are no checks then there is no status to report + const checkRuns = data.check_runs; + if (checkRuns.length === 0) return null; + + // Establish when the first check started running, useful for showing + // how long something has been running. + const starts = checkRuns + .map((run) => run.started_at) + .filter((startedAt) => startedAt !== null) as string[]; + const startTimes = starts.map((startedAt) => new Date(startedAt)); + + const queued = checkRuns.filter((c) => c.status === 'queued').length; + const failed = checkRuns.filter((c) => c.conclusion === 'failure').length; + const skipped = checkRuns.filter((c) => c.conclusion === 'skipped').length; + const succeeded = checkRuns.filter((c) => c.conclusion === 'success').length; + + const firstStart = new Date(Math.min(...startTimes.map((date) => date.getTime()))); + const completed = checkRuns.every((check) => !!check.completed_at); + const totalCount = data.total_count; + + const success = queued === 0 && failed === 0 && skipped + succeeded === totalCount; + const finished = checkRuns.filter( + (c) => c.conclusion && ['failure', 'success'].includes(c.conclusion) + ).length; + + return { + startedAt: firstStart, + hasChecks: !!totalCount, + success, + failed, + completed, + queued, + totalCount, + skipped, + finished + }; +} diff --git a/apps/desktop/src/lib/forge/interface/forgePrService.ts b/apps/desktop/src/lib/forge/interface/forgePrService.ts index 92904a273f..5580950ca4 100644 --- a/apps/desktop/src/lib/forge/interface/forgePrService.ts +++ b/apps/desktop/src/lib/forge/interface/forgePrService.ts @@ -9,7 +9,7 @@ export const [getForgePrService, createForgePrServiceStore] = buildContextStore< export interface ForgePrService { loading: Writable; - get(prNumber: number): Promise; + get(prNumber: number): Promise; createPr({ title, body, diff --git a/packages/shared/package.json b/packages/shared/package.json index 7b66370ee1..72e169909e 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -52,6 +52,7 @@ "autoprefixer": "^10.4.19", "cpy-cli": "^5.0.0", "dayjs": "^1.11.13", + "lscache": "^1.3.4", "postcss": "^8.4.38", "postcss-cli": "^11.0.0", "postcss-minify": "^1.1.0", @@ -63,8 +64,5 @@ "vite": "catalog:", "vitest": "^2.0.5" }, - "type": "module", - "dependencies": { - "lscache": "^1.3.2" - } + "type": "module" } diff --git a/packages/shared/src/lib/localCache.ts b/packages/shared/src/lib/localCache.ts new file mode 100644 index 0000000000..8bd5ba61a7 --- /dev/null +++ b/packages/shared/src/lib/localCache.ts @@ -0,0 +1,35 @@ +import lscache from 'lscache'; + +const DEFAULT_PREFIX = 'cache'; + +export interface CacheOptions { + expiry: number; + keyPrefix: string; +} +/** + * Helper for getting/setting values with lscache. + */ +export class LocalCache { + expiry: number; + keyPrefix: string | undefined; + constructor(options: CacheOptions) { + this.expiry = options.expiry; + this.keyPrefix = options.keyPrefix; + } + + get(key: string | number) { + return lscache.get(this.transformKey(key)); + } + + set(key: string | number, value: string | number | object) { + lscache.set(this.transformKey(key), value, this.expiry); + } + + remove(key: string | number) { + lscache.remove(this.transformKey(key)); + } + + private transformKey(key: string | number) { + return DEFAULT_PREFIX + ':' + this.keyPrefix + ':' + key; + } +}