From 94bce6b205f4e12bc5bf85777ebd03faa0c76214 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:03:38 +0200 Subject: [PATCH 1/9] perf(pr): reuse unchanged GitHub checks with conditional requests --- .../src/pullRequest/GitHubPullRequestCli.ts | 5 + .../GitHubPullRequestProvider.test.ts | 19 +- .../pullRequest/GitHubPullRequestProvider.ts | 8 +- .../pullRequest/PullRequestService.test.ts | 8 +- .../gitHubConditionalChecks.test.ts | 198 ++++++++++++++++++ .../pullRequest/gitHubConditionalChecks.ts | 150 +++++++++++++ .../src/sourceControl/GitHubCli.test.ts | 55 +++++ apps/server/src/sourceControl/GitHubCli.ts | 38 +++- packages/contracts/src/pullRequest.ts | 1 + 9 files changed, 474 insertions(+), 8 deletions(-) create mode 100644 apps/server/src/pullRequest/gitHubConditionalChecks.test.ts create mode 100644 apps/server/src/pullRequest/gitHubConditionalChecks.ts diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 723fcfc4d9b9..d295d500f69a 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -1,3 +1,4 @@ +import { makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; import { runGitHubStackAction, type GitHubStackActionError } from "./githubStackActions.ts"; import * as Context from "effect/Context"; import * as Clock from "effect/Clock"; @@ -517,6 +518,8 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; }) => Effect.Effect; + readonly revalidateChecks: Effect.Success>; + readonly getPullRequestDetail: (input: { readonly cwd: string; readonly repository: string; @@ -1065,6 +1068,7 @@ function actionArgs( export const make = Effect.gen(function* () { const github = yield* GitHubCli.GitHubCli; const graphQlBudget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + const revalidateChecks = yield* makeChecksRevalidator(github); const routingIdentities = new Map< string, { @@ -1729,6 +1733,7 @@ export const make = Effect.gen(function* () { return GitHubPullRequestCli.of({ withVerifiedCredential, + revalidateChecks, getRoutingIdentity, getViewerLogin: (input) => getRoutingIdentity(input).pipe(Effect.map((identity) => identity.viewer)), diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 6c57c3b6aea6..0970686cc90f 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -30,6 +30,7 @@ it.effect("maps credential verification failures without relabeling operation fa const provider = yield* make.pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, withVerifiedCredential: (_input, use) => verificationFails ? Effect.fail( @@ -74,6 +75,7 @@ it.effect("refreshes checks without permissions or comparison reads", () => const provider = yield* make.pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.sync(() => { reads++; @@ -116,6 +118,7 @@ it.effect("uses one narrow read for a linked pull request summary", () => const provider = yield* make.pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestSummary: () => Effect.sync(() => { summaryReads += 1; @@ -165,6 +168,7 @@ it.effect("declares host-native stacks and passes the one the CLI reads through" const provider = yield* make.pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestStack: (input) => Effect.succeed(input.number === 7 ? stack : null), }), ), @@ -184,6 +188,7 @@ it.effect("reports a failed stack read against its own operation", () => const provider = yield* make.pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestStack: () => Effect.fail( new GitHubPullRequestCli.GitHubPullRequestReadError({ @@ -337,6 +342,7 @@ describe("gitHubViewerPermissions", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed({ ...coreFields, @@ -405,7 +411,7 @@ describe("gitHubViewerPermissions", () => { if (readChecks === undefined) return yield* Effect.die("checks read missing"); expect( yield* readChecks({ cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }), - ).toEqual({ state: detail.state, checks: detail.checks }); + ).toEqual({ state: detail.state, checks: detail.checks, headSha: "abc123" }); expect(detail.workflowApprovalsRequired).toBe(1); expect(detail.checks).toEqual([ { @@ -430,6 +436,7 @@ describe("gitHubViewerPermissions", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed({ ...coreFields, @@ -576,6 +583,7 @@ it.effect("does not classify same-repository gates as fork workflow approvals", }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed({ ...openDetail, isCrossRepository: false }), getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), listWorkflowRunsRequiringApproval: () => @@ -615,6 +623,7 @@ it.effect("keeps an unsafe workflow approval scope visible as unknown", () => }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), listWorkflowRunsRequiringApproval: () => @@ -658,6 +667,7 @@ it.effect("propagates workflow discovery rate limits", () => }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), listWorkflowRunsRequiringApproval: () => @@ -700,6 +710,7 @@ describe("getViewerPermissions", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.die("Unexpected detail read"), getPullRequestBaseComparison: () => Effect.die("Unexpected comparison read"), getViewerAccess: () => @@ -725,6 +736,7 @@ describe("getViewerPermissions", () => { }>, ) => Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => comparison, getViewerAccess: () => @@ -771,6 +783,7 @@ describe("getViewerPermissions", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: (input) => Effect.sync(() => { @@ -810,6 +823,7 @@ describe("getViewerPermissions", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => Effect.fail( @@ -879,6 +893,7 @@ describe("getChangeRequest commits", () => { const layerWith = (commits: GitHubReviewThreadComments["commits"]) => Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestActivity: () => Effect.succeed({ author: baseDetail.author, @@ -963,6 +978,7 @@ describe("getChangeRequestActivity dismissed reviews", () => { }; const layerFor = (body: string) => Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, getPullRequestActivity: () => Effect.succeed({ author: null, comments: [dismissedReview(body)], commits: [] }), listReviewThreadComments: () => Effect.succeed(threadComments), @@ -1045,6 +1061,7 @@ describe("editing", () => { }).pipe( Effect.provide( Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + revalidateChecks: (_input, read) => read, updatePullRequest: (input) => Effect.sync(() => void rewrites.push(input)), updateComment: (input) => Effect.sync(() => void rewrites.push(input)), }), diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 73cbcc7eca74..49938c0f2db2 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -363,8 +363,12 @@ export const make = Effect.gen(function* () { cli.getPullRequestPreview(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), getChangeRequestChecks: (input) => - readChecks(input).pipe( - Effect.map(({ state, checks }) => ({ state, checks })), + cli.revalidateChecks(input, readChecks(input)).pipe( + Effect.map(({ state, checks, headSha }) => ({ + state, + checks, + ...(headSha == null ? {} : { headSha }), + })), Effect.mapError(fail("getChangeRequestChecks")), ), diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 5d162fed8e53..005b3293b218 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1844,14 +1844,16 @@ for (const [provider, host] of [ number: 1, allowStale: false, }; - yield* Effect.all([service[read](reference), service[read](reference)], { concurrency: 2 }); + const request: Effect.Effect = + service[read](reference); + yield* Effect.all([request, request], { concurrency: 2 }); assert.strictEqual(calls, 1); yield* TestClock.adjust("45 seconds"); limited = true; - yield* Effect.flip(service[read](reference)); + assert.strictEqual((yield* Effect.exit(request))._tag, "Failure"); assert.strictEqual(calls, 2); yield* TestClock.adjust("45 seconds"); - yield* Effect.flip(service[read](reference)); + assert.strictEqual((yield* Effect.exit(request))._tag, "Failure"); assert.strictEqual(calls, 2); yield* TestClock.adjust("75 seconds"); limited = false; diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts new file mode 100644 index 000000000000..e5590e87f6bf --- /dev/null +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts @@ -0,0 +1,198 @@ +import { expect, it } from "@effect/vitest"; +import * as TestClock from "effect/testing/TestClock"; +import * as Effect from "effect/Effect"; +import * as Schema from "effect/Schema"; +import * as Redacted from "effect/Redacted"; +import { ChildProcessSpawner } from "effect/unstable/process"; +import type { PullRequestCheck } from "@t3tools/contracts"; + +import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import { makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; + +const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); + +const reference = { cwd: "/repo", repository: "acme/web", host: "github.com", number: 1 }; +const credential = { + host: "github.com", + token: Redacted.make("token"), + credentialFingerprint: "one", +}; + +it.effect( + "reuses unchanged checks and catches reruns, later pages, fork approvals, pushes and account changes", + () => + Effect.gen(function* () { + let sha = "a".repeat(40); + let changed = ""; + let reads = 0; + const requests: string[] = []; + const revalidate = yield* makeChecksRevalidator({ + execute: ({ args }) => + Effect.sync(() => { + const endpoint = args[1]!; + requests.push(endpoint); + const head = endpoint.endsWith("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/pulls/1"); + const modified = !args.includes("-H") || (endpoint.includes(changed) && changed !== ""); + const next = endpoint.includes("check-runs") && endpoint.endsWith("page=1"); + return { + exitCode: ChildProcessSpawner.ExitCode(modified ? 0 : 1), + stdout: modified + ? `HTTP/2.0 200 OK\r\nEtag: "${sha}-${changed}"\r\n${next ? 'Link: ; rel="next"\r\n' : ""}\r\n${head ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 2 }) : ""}` + : "HTTP/2.0 304 Not Modified\r\n\r\n", + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + }), + }); + const read = Effect.sync(() => { + reads++; + return { + state: "open" as const, + checks: [ + { name: "build", status: "success", description: null, url: null }, + ] satisfies PullRequestCheck[], + headSha: sha, + workflowApprovalsRequired: 0, + }; + }); + const poll = (identity = credential) => + revalidate(reference, read).pipe( + Effect.provideService(GitHubCli.PinnedGitHubCredential, identity), + ); + yield* poll(); + expect(reads).toBe(1); + requests.length = 0; + yield* poll(); + expect(reads).toBe(1); + expect(requests).toHaveLength(5); + for (const endpoint of [ + "check-runs?filter=all&per_page=100&page=2", + "/status", + "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/actions/runs", + "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/pulls/1", + ]) { + changed = endpoint; + yield* poll(); + changed = ""; + yield* poll(); + } + expect(reads).toBe(5); + sha = "b".repeat(40); + changed = "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/pulls/1"; + requests.length = 0; + expect((yield* poll()).headSha).toBe(sha); + expect( + requests + .filter((endpoint) => endpoint.includes("/commits/")) + .every((endpoint) => endpoint.includes(sha)), + ).toBe(true); + changed = ""; + yield* poll({ ...credential, credentialFingerprint: "two" }); + expect(reads).toBe(7); + yield* TestClock.adjust("5 minutes"); + yield* poll(); + expect(reads).toBe(8); + }), +); + +it.effect("does not retain failed or incomplete reads, and supports hosts without ETags", () => + Effect.gen(function* () { + const sha = "a".repeat(40); + let etags = true; + let unavailable = false; + let fail = true; + let complete = true; + let reads = 0; + const revalidate = yield* makeChecksRevalidator({ + execute: ({ args }) => + unavailable + ? Effect.fail( + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: "/repo", + cause: undefined, + httpStatus: 502, + }), + ) + : Effect.succeed({ + exitCode: ChildProcessSpawner.ExitCode(0), + stdout: + args.includes("-H") && etags + ? "HTTP/2.0 304 Not Modified\n\n" + : `HTTP/2.0 200 OK\n${etags ? 'Etag: "one"\n' : ""}\n${args[1]!.endsWith("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/pulls/1") ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 1 }) : ""}`, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }), + }); + const read = Effect.suspend(() => { + reads++; + return fail + ? Effect.fail( + new GitHubCli.GitHubCliCommandError({ command: "gh", cwd: "/repo", cause: undefined }), + ) + : Effect.succeed({ + state: "open" as const, + checks: [], + headSha: sha, + ...(complete ? { workflowApprovalsRequired: 0 } : {}), + }); + }); + const poll = () => + revalidate(reference, read).pipe( + Effect.provideService(GitHubCli.PinnedGitHubCredential, credential), + ); + yield* poll().pipe(Effect.flip); + fail = false; + complete = false; + yield* poll(); + yield* poll(); + expect(reads).toBe(3); + complete = true; + yield* poll(); + yield* poll(); + expect(reads).toBe(4); + unavailable = true; + yield* poll(); + expect(reads).toBe(5); + unavailable = false; + yield* poll(); + yield* poll(); + expect(reads).toBe(6); + etags = false; + yield* poll(); + yield* poll(); + expect(reads).toBe(8); + }), +); + +it.effect("falls back when REST checks are unavailable without retrying unsupported probes", () => + Effect.gen(function* () { + let probes = 0; + let reads = 0; + const revalidate = yield* makeChecksRevalidator({ + execute: () => { + probes++; + return Effect.fail( + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: "/repo", + cause: undefined, + httpStatus: 404, + }), + ); + }, + }); + const read = Effect.sync(() => { + reads++; + return { state: "open" as const, checks: [] }; + }); + for (let tick = 0; tick < 2; tick++) + yield* revalidate(reference, read).pipe( + Effect.provideService(GitHubCli.PinnedGitHubCredential, credential), + ); + expect(probes).toBe(1); + expect(reads).toBe(2); + }), +); diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.ts new file mode 100644 index 000000000000..098f1d1d3fb4 --- /dev/null +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.ts @@ -0,0 +1,150 @@ +import * as Clock from "effect/Clock"; +import * as Cache from "effect/Cache"; +import * as Effect from "effect/Effect"; +import * as Schema from "effect/Schema"; +import * as Semaphore from "effect/Semaphore"; +import { PositiveInt, type PullRequestChecks } from "@t3tools/contracts"; + +import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import type { GitHubPullRequestDetail } from "./gitHubPullRequestJson.ts"; +import type { GitHubPullRequestCliError } from "./GitHubPullRequestCli.ts"; +import type { ProviderRepositoryRef } from "./PullRequestProvider.ts"; + +const decodeHeadSchema = Schema.Struct({ + head: Schema.Struct({ sha: Schema.String.check(Schema.isPattern(/^[a-f0-9]{40,64}$/)) }), + base: Schema.Struct({ repo: Schema.Struct({ id: PositiveInt }) }), + headRepositoryId: Schema.NullOr(PositiveInt), +}); +const decodeHead = Schema.decodeUnknownEffect(Schema.fromJsonString(decodeHeadSchema)); + +type Validator = { etag: string | undefined; next: boolean }; + +export const makeChecksRevalidator = (github: Pick) => + Effect.gen(function* () { + const entries = yield* Cache.makeWith( + (_key: string) => + Effect.sync(() => ({ + gate: Semaphore.makeUnsafe(1), + validators: new Map(), + head: null as Schema.Schema.Type | null, + value: null as PullRequestChecks | null, + readAt: 0, + supported: true, + })), + { capacity: 128, timeToLive: () => "30 minutes" }, + ); + return ( + input: ProviderRepositoryRef & { readonly number: number }, + read: Effect.Effect< + Pick & { + workflowApprovalsRequired?: number; + }, + GitHubPullRequestCliError + >, + ) => + Effect.gen(function* () { + const credential = yield* GitHubCli.PinnedGitHubCredential; + if (credential === null) return yield* read; + const key = `${credential.credentialFingerprint}\0${input.host}\0${input.repository}\0${input.number}`; + const entry = yield* Cache.get(entries, key); + return yield* entry.gate.withPermit( + Effect.gen(function* () { + if (!entry.supported) return yield* read; + const fail = () => + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: input.cwd, + cause: new Error("GitHub returned an invalid conditional checks response."), + }); + const get = (endpoint: string, head = false) => + Effect.gen(function* () { + const previous = entry.validators.get(endpoint); + const result = yield* github + .execute({ + cwd: input.cwd, + args: [ + "api", + endpoint, + "--hostname", + input.host, + "--include", + ...(head + ? [ + "--jq", + "{head:{sha:.head.sha},base:{repo:{id:.base.repo.id}},headRepositoryId:.head.repo.id}", + ] + : ["--silent"]), + ...(previous?.etag ? ["-H", `If-None-Match: ${previous.etag}`] : []), + ], + acceptNotModified: true, + }) + .pipe( + Effect.catchTag("GitHubCliCommandError", (error) => + Effect.sync(() => { + entry.supported = ![404, 405, 501].includes(error.httpStatus ?? 0); + return null; + }), + ), + ); + if (result === null) { + entry.value = null; + return null; + } + if (result.stdoutTruncated || result.stdoutInvalidUtf8) return yield* fail(); + const split = result.stdout.search(/\r?\n\r?\n/); + const headers = split < 0 ? result.stdout : result.stdout.slice(0, split); + const status = /^HTTP\/\S+ (\d+)/.exec(headers)?.[1]; + if (status === "304" && previous) return previous.next; + if (status !== "200") return yield* fail(); + entry.value = null; + if (head) { + const decoded = yield* decodeHead(result.stdout.slice(split).trim()).pipe( + Effect.mapError(fail), + ); + if (entry.head?.head.sha !== decoded.head.sha) entry.validators.clear(); + entry.head = decoded; + } + const validator = { + etag: /^etag:\s*(.+)$/im.exec(headers)?.[1]?.trim(), + next: /^link:.*rel="next"/im.test(headers), + }; + entry.validators.set(endpoint, validator); + return validator.next; + }); + const root = `repos/${input.repository}`; + if ((yield* get(`${root}/pulls/${input.number}`, true)) === null) return yield* read; + const head = entry.head; + if (head === null) return yield* fail(); + const endpoints = [ + `${root}/commits/${head.head.sha}/check-runs?filter=all`, + `${root}/commits/${head.head.sha}/status`, + ...(head.headRepositoryId !== head.base.repo.id + ? [`${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request`] + : []), + ]; + for (const endpoint of endpoints) { + for (let page = 1; ; page++) { + if (page > 100) return yield* fail(); + const next = yield* get( + `${endpoint}${endpoint.includes("?") ? "&" : "?"}per_page=100&page=${page}`, + ); + if (next === null) return yield* read; + if (!next) break; + } + } + const now = yield* Clock.currentTimeMillis; + if (entry.value !== null && now - entry.readAt < 5 * 60_000) return entry.value; + const fresh = yield* read; + const value = { + state: fresh.state, + checks: fresh.checks, + ...(fresh.headSha ? { headSha: fresh.headSha } : {}), + }; + if (fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha) + entry.value = value; + entry.readAt = now; + return value; + }), + ); + }); + }); diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 5893c21ff772..f23975b0ee9c 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -637,3 +637,58 @@ describe("GitHubCli.layer", () => { }).pipe(Effect.provide(layer)), ); }); + +it.effect("accepts conditional 304 responses and preserves HTTP errors and retry delays", () => + Effect.gen(function* () { + const gh = yield* GitHubCli.GitHubCli; + const request = { + cwd: "/repo", + args: [ + "api", + "repos/acme/web/pulls/1", + "--hostname", + "github.com", + "--include", + "-H", + 'If-None-Match: "one"', + ], + acceptNotModified: true, + }; + const respond = (status: number, headers = "") => + mockRun.mockImplementation(() => + Effect.succeed({ + ...processOutput(`HTTP/2.0 ${status}\r\n${headers}\r\n`), + exitCode: ChildProcessSpawner.ExitCode(1), + }), + ); + respond(304); + expect((yield* gh.execute(request)).stdout).toContain("304"); + expect(mockRun.mock.calls[0]?.[0].allowNonZeroExit).toBe(true); + respond(401); + expect((yield* gh.execute(request).pipe(Effect.flip))._tag).toBe( + "GitHubCliAuthenticationError", + ); + respond(403); + expect((yield* gh.execute(request).pipe(Effect.flip))._tag).toBe("GitHubCliCommandError"); + for (const status of [403, 429]) { + respond(status, "Retry-After: 120\r\n"); + expect(yield* gh.execute(request).pipe(Effect.flip)).toMatchObject({ + _tag: "GitHubCliRateLimitError", + retryAt: (yield* Clock.currentTimeMillis) + 120_000, + }); + } + respond( + 403, + `X-RateLimit-Remaining: 0\r\nX-RateLimit-Reset: ${Math.floor((yield* Clock.currentTimeMillis) / 1_000) + 60}\r\n`, + ); + expect(yield* gh.execute(request).pipe(Effect.flip)).toMatchObject({ + _tag: "GitHubCliRateLimitError", + retryAt: (yield* Clock.currentTimeMillis) + 60_000, + }); + respond(500); + expect(yield* gh.execute(request).pipe(Effect.flip)).toMatchObject({ + _tag: "GitHubCliCommandError", + httpStatus: 500, + }); + }).pipe(Effect.provide(layer)), +); diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index c525740efeae..e1d1627a9318 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -132,7 +132,7 @@ export class GitHubPullRequestNotFoundError extends Schema.TaggedError()( "GitHubCliCommandError", - gitHubCliFailureFields, + { ...gitHubCliFailureFields, httpStatus: Schema.optional(Schema.Int) }, ) { get detail(): string { return "GitHub CLI command failed."; @@ -291,6 +291,7 @@ export class GitHubCli extends Context.Service< readonly maxOutputBytes?: number; readonly rateLimitHost?: string; readonly allowReserve?: boolean; + readonly acceptNotModified?: boolean; }) => Effect.Effect; readonly listOpenPullRequests: (input: { @@ -423,18 +424,51 @@ export const make = Effect.gen(function* () { GITHUB_ENTERPRISE_TOKEN: token, GH_DEBUG: "", }; - return yield* process + const result = yield* process .run({ operation: "GitHubCli.execute", command: "gh", args: input.args, cwd: input.cwd, timeoutMs: input.timeoutMs ?? DEFAULT_TIMEOUT_MS, + ...(input.acceptNotModified ? { allowNonZeroExit: true } : {}), ...(input.stdin !== undefined ? { stdin: input.stdin } : {}), ...(env !== undefined ? { env } : {}), ...(input.maxOutputBytes !== undefined ? { maxOutputBytes: input.maxOutputBytes } : {}), }) .pipe(Effect.mapError((error) => fromVcsError({ command: "gh", cwd: input.cwd }, error))); + if (result.exitCode !== 0 && input.acceptNotModified) { + const status = /^HTTP\/\S+ (\d+)/.exec(result.stdout)?.[1]; + if (status !== "304" || !input.args.includes("--include")) { + const context = { command: "gh" as const, cwd: input.cwd, cause: undefined }; + const headers = result.stdout.split(/\r?\n\r?\n/, 1)[0] ?? ""; + const header = (name: string) => + new RegExp(`^${name}:\\s*(.*)$`, "im").exec(headers)?.[1]?.trim(); + if ( + status === "429" || + (status === "403" && + (header("x-ratelimit-remaining") === "0" || + header("retry-after") !== undefined || + /rate limit/i.test(result.stderr))) + ) { + const now = DateTime.toEpochMillis(yield* DateTime.now); + const reset = Number(header("x-ratelimit-reset")) * 1_000; + const retryAt = + SourceControlRateLimit.retryAtFromHeader(header("retry-after"), now) ?? + (Number.isFinite(reset) && reset > now ? reset : undefined); + return yield* new GitHubCliRateLimitError({ + ...context, + ...(retryAt === undefined ? {} : { retryAt }), + }); + } + if (status === "401") return yield* new GitHubCliAuthenticationError(context); + return yield* new GitHubCliCommandError({ + ...context, + ...(status === undefined ? {} : { httpStatus: Number(status) }), + }); + } + } + return result; }, ); diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 75e9312f92aa..c5e8ad3fd523 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -882,6 +882,7 @@ export const PullRequestDetail = Schema.Struct({ export type PullRequestDetail = typeof PullRequestDetail.Type; export const PullRequestChecks = Schema.Struct({ + headSha: Schema.optional(TrimmedNonEmptyString), state: PullRequestState, checks: Schema.Array(PullRequestCheck), }); From a88b3eb45814f5ad3514641486f1b5747b34fe35 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:04:25 +0200 Subject: [PATCH 2/9] fix(pr): refresh active checks and wake readers after pushes --- .../src/git/linkCreatedPullRequest.test.ts | 44 +++++++++ .../src/git/refreshPushedPullRequests.ts | 36 ++++++++ apps/server/src/ws.ts | 14 +++ apps/web/src/components/GitActionsControl.tsx | 1 + .../components/chat/ThreadDetailsPrRow.tsx | 8 +- apps/web/src/hooks/useLiveRefresh.ts | 29 +++--- .../hooks/usePullRequestChecksRefresh.test.ts | 89 +++++++++++++++++++ .../src/hooks/usePullRequestChecksRefresh.ts | 37 ++++++++ .../src/state/vcsAction.test.ts | 5 +- .../client-runtime/src/state/vcsAction.ts | 3 + packages/contracts/src/git.ts | 9 +- 11 files changed, 260 insertions(+), 15 deletions(-) create mode 100644 apps/server/src/git/refreshPushedPullRequests.ts create mode 100644 apps/web/src/hooks/usePullRequestChecksRefresh.test.ts create mode 100644 apps/web/src/hooks/usePullRequestChecksRefresh.ts diff --git a/apps/server/src/git/linkCreatedPullRequest.test.ts b/apps/server/src/git/linkCreatedPullRequest.test.ts index d0f317622d4d..e2d698f9d4a1 100644 --- a/apps/server/src/git/linkCreatedPullRequest.test.ts +++ b/apps/server/src/git/linkCreatedPullRequest.test.ts @@ -21,6 +21,8 @@ import { } from "../orchestration-v2/Orchestrator.ts"; import { v2PullRequestThread } from "../orchestration-v2/testkit/pullRequestFixtures.ts"; import { ProjectionSnapshotQuery } from "../orchestration/Services/ProjectionSnapshotQuery.ts"; +import { refreshPushedPullRequests } from "./refreshPushedPullRequests.ts"; +import { PullRequestService } from "../pullRequest/PullRequestService.ts"; import { createdPullRequestKey, linkCreatedPullRequest } from "./linkCreatedPullRequest.ts"; const PROJECT_ID = ProjectId.make("project-1"); @@ -229,3 +231,45 @@ describe("linkCreatedPullRequest", () => { }), ); }); + +it.effect( + "refreshes PR readers after a push from a thread or project, but not a local commit", + () => + Effect.gen(function* () { + const refreshed: string[] = []; + const dependencies = Layer.mergeAll( + Layer.mock(OrchestratorV2)({ + getThreadShell: () => Effect.succeed(v2PullRequestThread(thread)), + }), + Layer.mock(ProjectionSnapshotQuery)({ + getProjectShellsWithoutEnrichment: () => Effect.succeed([project]), + }), + Layer.mock(PullRequestService)({ + refreshAfterTurn: (id) => + Effect.sync(() => { + refreshed.push(id); + }), + }), + ); + yield* refreshPushedPullRequests( + { cwd: "/worktree", threadId: THREAD_ID }, + { push: { status: "pushed" } }, + ).pipe(Effect.provide(dependencies)); + yield* refreshPushedPullRequests( + { cwd: project.workspaceRoot }, + { push: { status: "pushed" } }, + ).pipe(Effect.provide(dependencies)); + yield* refreshPushedPullRequests({ cwd: "/unrelated" }, { push: { status: "pushed" } }).pipe( + Effect.provide(dependencies), + ); + yield* refreshPushedPullRequests( + { cwd: project.workspaceRoot, threadId: THREAD_ID }, + { push: { status: "skipped_not_requested" } }, + ).pipe(Effect.provide(dependencies)); + yield* refreshPushedPullRequests( + { cwd: "/draft-worktree", projectId: PROJECT_ID }, + { push: { status: "pushed" } }, + ).pipe(Effect.provide(dependencies)); + expect(refreshed).toEqual([PROJECT_ID, PROJECT_ID, PROJECT_ID]); + }), +); diff --git a/apps/server/src/git/refreshPushedPullRequests.ts b/apps/server/src/git/refreshPushedPullRequests.ts new file mode 100644 index 000000000000..c89e3bf3ec5a --- /dev/null +++ b/apps/server/src/git/refreshPushedPullRequests.ts @@ -0,0 +1,36 @@ +import type { GitRunStackedActionInput, GitRunStackedActionResult } from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; + +import { OrchestratorV2 } from "../orchestration-v2/Orchestrator.ts"; +import { ProjectionSnapshotQuery } from "../orchestration/Services/ProjectionSnapshotQuery.ts"; +import { PullRequestService } from "../pullRequest/PullRequestService.ts"; + +export const refreshPushedPullRequests = Effect.fn("refreshPushedPullRequests")( + function* ( + input: Pick, + result: Pick, + ) { + if (result.push.status !== "pushed") return; + const pullRequests = yield* PullRequestService; + if (input.threadId !== undefined) { + const engine = yield* OrchestratorV2; + const thread = yield* engine.getThreadShell(input.threadId); + if (thread !== null) { + yield* pullRequests.refreshAfterTurn(thread.projectId); + return; + } + } + if (input.projectId !== undefined) { + yield* pullRequests.refreshAfterTurn(input.projectId); + return; + } + const snapshots = yield* ProjectionSnapshotQuery; + const projects = yield* snapshots.getProjectShellsWithoutEnrichment(); + yield* Effect.forEach( + projects.filter((project) => project.workspaceRoot === input.cwd), + (project) => pullRequests.refreshAfterTurn(project.id), + { discard: true }, + ); + }, + Effect.ignore({ log: true }), +); diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 2aa6de65dd79..ad54de4f5c6f 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -190,6 +190,7 @@ import * as WorkspacePaths from "./workspace/WorkspacePaths.ts"; import * as VcsStatusBroadcaster from "./vcs/VcsStatusBroadcaster.ts"; import * as VcsProvisioningService from "./vcs/VcsProvisioningService.ts"; import * as GitWorkflowService from "./git/GitWorkflowService.ts"; +import { refreshPushedPullRequests } from "./git/refreshPushedPullRequests.ts"; import { linkCreatedPullRequest } from "./git/linkCreatedPullRequest.ts"; import * as ReviewService from "./review/ReviewService.ts"; import * as ProjectEnrichmentService from "./project/ProjectEnrichmentService.ts"; @@ -3107,6 +3108,19 @@ const makeWsRpcLayer = ( ), ) ).pipe( + Effect.andThen( + refreshPushedPullRequests(input, result).pipe( + Effect.provideService(OrchestratorV2, orchestrationEngine), + Effect.provideService( + ProjectionSnapshotQuery.ProjectionSnapshotQuery, + projectionSnapshotQuery, + ), + Effect.provideService( + PullRequestService.PullRequestService, + pullRequests, + ), + ), + ), Effect.andThen(refreshGitStatus(input.cwd)), Effect.andThen(Queue.end(queue).pipe(Effect.asVoid)), ), diff --git a/apps/web/src/components/GitActionsControl.tsx b/apps/web/src/components/GitActionsControl.tsx index d06990411900..78c183022bf9 100644 --- a/apps/web/src/components/GitActionsControl.tsx +++ b/apps/web/src/components/GitActionsControl.tsx @@ -1364,6 +1364,7 @@ export default function GitActionsControl({ // A pull request the action opens is linked to the thread it ran beside. Drafts // have no server thread yet, so there is nothing to link to. ...(activeServerThread ? { threadId: activeServerThread.id } : {}), + ...(activeDraftThread ? { projectId: activeDraftThread.projectId } : {}), }); if (result._tag === "Failure") { diff --git a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx index 1fc5cfa9912f..d89772ebaa8f 100644 --- a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx +++ b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx @@ -21,6 +21,7 @@ import { ArrowUpRightIcon, FileDiffIcon, GitBranchIcon, TriangleAlertIcon } from import { useState, type MouseEvent as ReactMouseEvent } from "react"; import { useLiveRefresh } from "~/hooks/useLiveRefresh"; +import { usePullRequestChecksRefresh } from "~/hooks/usePullRequestChecksRefresh"; import { cn } from "~/lib/utils"; import { useServerConfigs } from "~/state/entities"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -138,10 +139,13 @@ export function ThreadDetailsPrRow({ key: `workspace-pr:${refreshKey}`, intervalMs: 10 * 60_000, }); - useLiveRefresh(checksQuery.isPending || detailQuery.isPending ? null : checksQuery.refresh, { + usePullRequestChecksRefresh({ + refresh: checksQuery.isPending || detailQuery.isPending ? null : checksQuery.refresh, enabled: open && supportsChecks && !(checksQuery.isSuccess && checksQuery.data === null), key: `workspace-pr-checks:${refreshKey}`, - intervalMs: 45_000, + checks: detail?.checks ?? [], + headSha: checksQuery.data?.headSha, + updatedAt: checksQuery.dataUpdatedAt, }); const { actionPending, perform } = usePullRequestActionRunner({ diff --git a/apps/web/src/hooks/useLiveRefresh.ts b/apps/web/src/hooks/useLiveRefresh.ts index da6bdd0657af..439eb40a5d8f 100644 --- a/apps/web/src/hooks/useLiveRefresh.ts +++ b/apps/web/src/hooks/useLiveRefresh.ts @@ -86,18 +86,20 @@ const lastRefreshedAtByView = new Map(); /** * When the reader last did anything. Shared rather than per view: a person is present in the - * window, not in one component of it. Only tracked while a live view is mounted, and the handler - * writes a number and nothing else, so a mousemove costs what a mousemove costs. + * window, not in one component of it. Only tracked while a live view is mounted. */ let lastInteractedAt = 0; -let interactionWatchers = 0; +const interactionWatchers = new Set<() => void>(); const INTERACTION_EVENTS = ["pointerdown", "pointermove", "keydown", "wheel"] as const; const noteInteraction = () => { - lastInteractedAt = Date.now(); + const now = Date.now(); + const wasIdle = now - lastInteractedAt >= LIVE_REFRESH_IDLE_AFTER_MS; + lastInteractedAt = now; + if (wasIdle) for (const resume of interactionWatchers) resume(); }; -function watchInteraction(): () => void { - if (interactionWatchers === 0) { +function watchInteraction(resume: () => void): () => void { + if (interactionWatchers.size === 0) { // Arriving is itself the reader doing something, and it is what makes the first interval tick // after a mount count. lastInteractedAt = Date.now(); @@ -105,10 +107,10 @@ function watchInteraction(): () => void { document.addEventListener(event, noteInteraction, { passive: true }); } } - interactionWatchers += 1; + interactionWatchers.add(resume); return () => { - interactionWatchers -= 1; - if (interactionWatchers > 0) return; + interactionWatchers.delete(resume); + if (interactionWatchers.size > 0) return; for (const event of INTERACTION_EVENTS) { document.removeEventListener(event, noteInteraction); } @@ -137,12 +139,14 @@ export function useLiveRefresh( useEffect(() => { if (!enabled) return; const read = (now: number) => { + if (latest.current === null) return; lastRefreshedAtByView.set(viewId, now); - latest.current?.(); + latest.current(); }; const visible = () => document.visibilityState === "visible"; const onArrival = () => { const now = Date.now(); + if (visible()) lastInteractedAt = now; const lastRefreshedAt = lastRefreshedAtByView.get(viewId); if (lastRefreshedAt === undefined) { // Nothing read yet, so nothing to refresh: the mount's own read is what fills this in. @@ -172,7 +176,10 @@ export function useLiveRefresh( syncTimer(); }; - const stopWatchingInteraction = watchInteraction(); + const stopWatchingInteraction = watchInteraction(() => { + onArrival(); + syncTimer(); + }); onArrival(); syncTimer(); window.addEventListener("focus", onArrival); diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts new file mode 100644 index 000000000000..613848c60a11 --- /dev/null +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts @@ -0,0 +1,89 @@ +import type { PullRequestCheck } from "@t3tools/contracts"; +import { act, createElement } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import { expect, it, vi } from "vite-plus/test"; + +import { usePullRequestChecksRefresh } from "./usePullRequestChecksRefresh"; + +it("polls quiet PRs each minute, speeds up for runs and pushes, resumes from idle, and stops for closed PRs", () => { + vi.useFakeTimers(); + vi.setSystemTime(1_000_000); + const document = Object.assign(new EventTarget(), { visibilityState: "visible" }); + vi.stubGlobal("document", document); + vi.stubGlobal("window", new EventTarget()); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const refresh = vi.fn(); + let updatedAt = Date.now(); + let renderer: ReactTestRenderer | undefined; + function Probe({ + status = "success", + headSha = "one", + enabled = true, + busy = false, + }: { + status?: PullRequestCheck["status"] | null; + headSha?: string; + enabled?: boolean; + busy?: boolean; + }) { + usePullRequestChecksRefresh({ + refresh: busy ? null : refresh, + enabled, + key: "test-pr-checks", + headSha, + updatedAt, + checks: status === null ? [] : [{ name: "CI", status, description: null, url: null }], + }); + return null; + } + const update = (props: Parameters[0]) => { + updatedAt = Date.now(); + act(() => renderer?.update(createElement(Probe, props))); + }; + const advance = (ms: number) => act(() => vi.advanceTimersByTime(ms)); + try { + act(() => { + renderer = create(createElement(Probe)); + }); + advance(59_999); + expect(refresh).toHaveBeenCalledTimes(0); + advance(1); + expect(refresh).toHaveBeenCalledTimes(1); + update({ status: "pending" }); + advance(44_999); + expect(refresh).toHaveBeenCalledTimes(1); + advance(1); + expect(refresh).toHaveBeenCalledTimes(2); + update({ status: "failure" }); + advance(60_000); + expect(refresh).toHaveBeenCalledTimes(3); + update({ headSha: "two", status: null }); + for (let tick = 0; tick < 3; tick++) { + advance(45_000); + update({ headSha: "two", status: null }); + } + expect(refresh).toHaveBeenCalledTimes(6); + advance(59_999); + expect(refresh).toHaveBeenCalledTimes(6); + advance(1); + expect(refresh).toHaveBeenCalledTimes(7); + advance(6 * 60_000); + const beforeResume = refresh.mock.calls.length; + act(() => document.dispatchEvent(new Event("pointerdown"))); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); + document.visibilityState = "hidden"; + act(() => document.dispatchEvent(new Event("visibilitychange"))); + advance(60_000); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); + document.visibilityState = "visible"; + act(() => document.dispatchEvent(new Event("visibilitychange"))); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); + update({ enabled: false }); + advance(120_000); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); + } finally { + act(() => renderer?.unmount()); + vi.useRealTimers(); + vi.unstubAllGlobals(); + } +}); diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.ts new file mode 100644 index 000000000000..6e32f0c07537 --- /dev/null +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.ts @@ -0,0 +1,37 @@ +import type { PullRequestCheck } from "@t3tools/contracts"; +import { useState } from "react"; + +import { useLiveRefresh } from "./useLiveRefresh"; + +export function usePullRequestChecksRefresh(input: { + refresh: (() => void) | null; + enabled: boolean; + key: string; + checks: ReadonlyArray; + headSha: string | undefined; + updatedAt: number; +}) { + const [observed, setObserved] = useState(() => ({ + key: input.key, + headSha: input.headSha, + waitingSince: input.checks.length === 0 ? input.updatedAt : null, + })); + if (observed.key !== input.key || observed.headSha !== input.headSha) { + setObserved({ + key: input.key, + headSha: input.headSha, + waitingSince: + input.checks.length === 0 || (observed.key === input.key && observed.headSha !== undefined) + ? input.updatedAt + : null, + }); + } + const waiting = + observed.waitingSince !== null && input.updatedAt - observed.waitingSince < 2 * 60_000; + useLiveRefresh(input.refresh, { + key: input.key, + enabled: input.enabled, + intervalMs: + waiting || input.checks.some((check) => check.status === "pending") ? 45_000 : 60_000, + }); +} diff --git a/packages/client-runtime/src/state/vcsAction.test.ts b/packages/client-runtime/src/state/vcsAction.test.ts index b78401a44e21..37ff6a440d32 100644 --- a/packages/client-runtime/src/state/vcsAction.test.ts +++ b/packages/client-runtime/src/state/vcsAction.test.ts @@ -1,6 +1,7 @@ import { EnvironmentId, ThreadId, + ProjectId, WS_METHODS, type GitActionProgressEvent, type GitRunStackedActionInput, @@ -663,11 +664,13 @@ describe("vcsActionState", () => { expect(registry.get(state).revision).toBe(0); const threadId = ThreadId.make("thread-stacked-action"); + const projectId = ProjectId.make("project-stacked-action"); const successfulResult = yield* Effect.promise(() => manager.runStackedAction(targetKey).run(registry, { actionId: successfulActionId, action, threadId, + projectId, }), ); @@ -677,7 +680,7 @@ describe("vcsActionState", () => { expect(removed).toEqual([`${environmentId}:*`]); // The server links a created pull request to this thread, so the id must ride along. expect(rpcInputs).toEqual([ - { actionId: successfulTransportActionId, cwd, action, threadId }, + { actionId: successfulTransportActionId, cwd, action, threadId, projectId }, ]); const failedResult = yield* Effect.promise(() => diff --git a/packages/client-runtime/src/state/vcsAction.ts b/packages/client-runtime/src/state/vcsAction.ts index 829ec19d0b95..2f871953cbab 100644 --- a/packages/client-runtime/src/state/vcsAction.ts +++ b/packages/client-runtime/src/state/vcsAction.ts @@ -7,6 +7,7 @@ import { type GitRunStackedActionResult, GitStackedAction, type ThreadId, + type ProjectId, WS_METHODS, } from "@t3tools/contracts"; import * as Cause from "effect/Cause"; @@ -79,6 +80,7 @@ export interface RunVcsStackedActionInput { readonly filePaths?: ReadonlyArray; /** The thread the action runs beside; the server links a pull request it creates to it. */ readonly threadId?: ThreadId; + readonly projectId?: ProjectId; readonly onProgress?: (event: GitActionProgressEvent) => void; } @@ -470,6 +472,7 @@ export function createVcsActionManager( ...(input.featureBranch ? { featureBranch: true } : {}), ...(input.filePaths?.length ? { filePaths: [...input.filePaths] } : {}), ...(input.threadId !== undefined ? { threadId: input.threadId } : {}), + ...(input.projectId !== undefined ? { projectId: input.projectId } : {}), }; const clearOwnedState = Effect.sync(() => { const current = registry.get(stateAtom); diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index c5a5825e193e..2efd79f5baa2 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -1,6 +1,12 @@ import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; -import { NonNegativeInt, PositiveInt, ThreadId, TrimmedNonEmptyString } from "./baseSchemas.ts"; +import { + NonNegativeInt, + PositiveInt, + ProjectId, + ThreadId, + TrimmedNonEmptyString, +} from "./baseSchemas.ts"; import { SourceControlProviderError, SourceControlProviderInfo } from "./sourceControl.ts"; import { VcsDriverKind } from "./vcs.ts"; @@ -121,6 +127,7 @@ export const GitRunStackedActionInput = Schema.Struct({ ), /** The thread the action runs beside; a pull request it creates is linked to it. */ threadId: Schema.optional(ThreadId), + projectId: Schema.optional(ProjectId), }); export type GitRunStackedActionInput = typeof GitRunStackedActionInput.Type; From 3ab596f32d18d0e5d720b941d5b7a19bd2fc80f4 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:13:12 +0200 Subject: [PATCH 3/9] fix(web): preserve project scope through stacked git actions --- apps/web/src/state/sourceControlActions.ts | 23 +++------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/apps/web/src/state/sourceControlActions.ts b/apps/web/src/state/sourceControlActions.ts index b3d42ede7c3f..c2c215e3d32c 100644 --- a/apps/web/src/state/sourceControlActions.ts +++ b/apps/web/src/state/sourceControlActions.ts @@ -7,12 +7,11 @@ import type { import { VcsActionUnavailableError, type VcsActionOperation, + type RunVcsStackedActionInput, } from "@t3tools/client-runtime/state/vcs"; import type { EnvironmentId, - GitActionProgressEvent, GitResolvePullRequestResult, - GitStackedAction, SourceControlCloneProtocol, SourceControlRepositoryVisibility, ThreadId, @@ -212,15 +211,7 @@ export function useGitStackedAction(scope: SourceControlActionScope) { ); const action = useCallback( - async (input: { - actionId: string; - action: GitStackedAction; - commitMessage?: string; - featureBranch?: boolean; - filePaths?: string[]; - threadId?: ThreadId; - onProgress?: (event: GitActionProgressEvent) => void; - }) => { + async (input: RunVcsStackedActionInput) => { if (resolveScope(scope) === null) { return AsyncResult.failure( Cause.fail( @@ -232,15 +223,7 @@ export function useGitStackedAction(scope: SourceControlActionScope) { ), ); } - return runStackedAction({ - actionId: input.actionId, - action: input.action, - ...(input.commitMessage ? { commitMessage: input.commitMessage } : {}), - ...(input.featureBranch ? { featureBranch: true } : {}), - ...(input.filePaths?.length ? { filePaths: input.filePaths } : {}), - ...(input.threadId !== undefined ? { threadId: input.threadId } : {}), - ...(input.onProgress ? { onProgress: input.onProgress } : {}), - }); + return runStackedAction(input); }, [runStackedAction, scope], ); From b4039d18d4032fbf8637a3c164f20ded5779f751 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:13:54 +0200 Subject: [PATCH 4/9] refactor(pr): acquire checks dependencies through Effect --- .../src/pullRequest/GitHubPullRequestCli.ts | 4 +- .../gitHubConditionalChecks.test.ts | 120 +++++---- .../pullRequest/gitHubConditionalChecks.ts | 243 +++++++++--------- 3 files changed, 191 insertions(+), 176 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index d295d500f69a..74617116827c 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -518,7 +518,7 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; }) => Effect.Effect; - readonly revalidateChecks: Effect.Success>; + readonly revalidateChecks: Effect.Success; readonly getPullRequestDetail: (input: { readonly cwd: string; @@ -1068,7 +1068,7 @@ function actionArgs( export const make = Effect.gen(function* () { const github = yield* GitHubCli.GitHubCli; const graphQlBudget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; - const revalidateChecks = yield* makeChecksRevalidator(github); + const revalidateChecks = yield* makeChecksRevalidator; const routingIdentities = new Map< string, { diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts index e5590e87f6bf..61a753190801 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts @@ -1,6 +1,7 @@ import { expect, it } from "@effect/vitest"; import * as TestClock from "effect/testing/TestClock"; import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; import * as Schema from "effect/Schema"; import * as Redacted from "effect/Redacted"; import { ChildProcessSpawner } from "effect/unstable/process"; @@ -26,25 +27,30 @@ it.effect( let changed = ""; let reads = 0; const requests: string[] = []; - const revalidate = yield* makeChecksRevalidator({ - execute: ({ args }) => - Effect.sync(() => { - const endpoint = args[1]!; - requests.push(endpoint); - const head = endpoint.endsWith("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/pulls/1"); - const modified = !args.includes("-H") || (endpoint.includes(changed) && changed !== ""); - const next = endpoint.includes("check-runs") && endpoint.endsWith("page=1"); - return { - exitCode: ChildProcessSpawner.ExitCode(modified ? 0 : 1), - stdout: modified - ? `HTTP/2.0 200 OK\r\nEtag: "${sha}-${changed}"\r\n${next ? 'Link: ; rel="next"\r\n' : ""}\r\n${head ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 2 }) : ""}` - : "HTTP/2.0 304 Not Modified\r\n\r\n", - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, - }; + const revalidate = yield* makeChecksRevalidator.pipe( + Effect.provide( + Layer.mock(GitHubCli.GitHubCli)({ + execute: ({ args }) => + Effect.sync(() => { + const endpoint = args[1]!; + requests.push(endpoint); + const head = endpoint.endsWith("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/pulls/1"); + const modified = + !args.includes("-H") || (endpoint.includes(changed) && changed !== ""); + const next = endpoint.includes("check-runs") && endpoint.endsWith("page=1"); + return { + exitCode: ChildProcessSpawner.ExitCode(modified ? 0 : 1), + stdout: modified + ? `HTTP/2.0 200 OK\r\nEtag: "${sha}-${changed}"\r\n${next ? 'Link: ; rel="next"\r\n' : ""}\r\n${head ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 2 }) : ""}` + : "HTTP/2.0 304 Not Modified\r\n\r\n", + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + }), }), - }); + ), + ); const read = Effect.sync(() => { reads++; return { @@ -104,28 +110,32 @@ it.effect("does not retain failed or incomplete reads, and supports hosts withou let fail = true; let complete = true; let reads = 0; - const revalidate = yield* makeChecksRevalidator({ - execute: ({ args }) => - unavailable - ? Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/repo", - cause: undefined, - httpStatus: 502, - }), - ) - : Effect.succeed({ - exitCode: ChildProcessSpawner.ExitCode(0), - stdout: - args.includes("-H") && etags - ? "HTTP/2.0 304 Not Modified\n\n" - : `HTTP/2.0 200 OK\n${etags ? 'Etag: "one"\n' : ""}\n${args[1]!.endsWith("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/pulls/1") ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 1 }) : ""}`, - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, - }), - }); + const revalidate = yield* makeChecksRevalidator.pipe( + Effect.provide( + Layer.mock(GitHubCli.GitHubCli)({ + execute: ({ args }) => + unavailable + ? Effect.fail( + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: "/repo", + cause: undefined, + httpStatus: 502, + }), + ) + : Effect.succeed({ + exitCode: ChildProcessSpawner.ExitCode(0), + stdout: + args.includes("-H") && etags + ? "HTTP/2.0 304 Not Modified\n\n" + : `HTTP/2.0 200 OK\n${etags ? 'Etag: "one"\n' : ""}\n${args[1]!.endsWith("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/pulls/1") ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 1 }) : ""}`, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }), + }), + ), + ); const read = Effect.suspend(() => { reads++; return fail @@ -171,19 +181,23 @@ it.effect("falls back when REST checks are unavailable without retrying unsuppor Effect.gen(function* () { let probes = 0; let reads = 0; - const revalidate = yield* makeChecksRevalidator({ - execute: () => { - probes++; - return Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/repo", - cause: undefined, - httpStatus: 404, - }), - ); - }, - }); + const revalidate = yield* makeChecksRevalidator.pipe( + Effect.provide( + Layer.mock(GitHubCli.GitHubCli)({ + execute: () => { + probes++; + return Effect.fail( + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: "/repo", + cause: undefined, + httpStatus: 404, + }), + ); + }, + }), + ), + ); const read = Effect.sync(() => { reads++; return { state: "open" as const, checks: [] }; diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.ts index 098f1d1d3fb4..723b39f4001c 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.ts @@ -19,132 +19,133 @@ const decodeHead = Schema.decodeUnknownEffect(Schema.fromJsonString(decodeHeadSc type Validator = { etag: string | undefined; next: boolean }; -export const makeChecksRevalidator = (github: Pick) => - Effect.gen(function* () { - const entries = yield* Cache.makeWith( - (_key: string) => - Effect.sync(() => ({ - gate: Semaphore.makeUnsafe(1), - validators: new Map(), - head: null as Schema.Schema.Type | null, - value: null as PullRequestChecks | null, - readAt: 0, - supported: true, - })), - { capacity: 128, timeToLive: () => "30 minutes" }, - ); - return ( - input: ProviderRepositoryRef & { readonly number: number }, - read: Effect.Effect< - Pick & { - workflowApprovalsRequired?: number; - }, - GitHubPullRequestCliError - >, - ) => - Effect.gen(function* () { - const credential = yield* GitHubCli.PinnedGitHubCredential; - if (credential === null) return yield* read; - const key = `${credential.credentialFingerprint}\0${input.host}\0${input.repository}\0${input.number}`; - const entry = yield* Cache.get(entries, key); - return yield* entry.gate.withPermit( - Effect.gen(function* () { - if (!entry.supported) return yield* read; - const fail = () => - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: input.cwd, - cause: new Error("GitHub returned an invalid conditional checks response."), - }); - const get = (endpoint: string, head = false) => - Effect.gen(function* () { - const previous = entry.validators.get(endpoint); - const result = yield* github - .execute({ - cwd: input.cwd, - args: [ - "api", - endpoint, - "--hostname", - input.host, - "--include", - ...(head - ? [ - "--jq", - "{head:{sha:.head.sha},base:{repo:{id:.base.repo.id}},headRepositoryId:.head.repo.id}", - ] - : ["--silent"]), - ...(previous?.etag ? ["-H", `If-None-Match: ${previous.etag}`] : []), - ], - acceptNotModified: true, - }) - .pipe( - Effect.catchTag("GitHubCliCommandError", (error) => +export const makeChecksRevalidator = Effect.gen(function* () { + const github = yield* GitHubCli.GitHubCli; + const entries = yield* Cache.makeWith( + (_key: string) => + Effect.sync(() => ({ + gate: Semaphore.makeUnsafe(1), + validators: new Map(), + head: null as Schema.Schema.Type | null, + value: null as PullRequestChecks | null, + readAt: 0, + supported: true, + })), + { capacity: 128, timeToLive: () => "30 minutes" }, + ); + return ( + input: ProviderRepositoryRef & { readonly number: number }, + read: Effect.Effect< + Pick & { + workflowApprovalsRequired?: number; + }, + GitHubPullRequestCliError + >, + ) => + Effect.gen(function* () { + const credential = yield* GitHubCli.PinnedGitHubCredential; + if (credential === null) return yield* read; + const key = `${credential.credentialFingerprint}\0${input.host}\0${input.repository}\0${input.number}`; + const entry = yield* Cache.get(entries, key); + return yield* entry.gate.withPermit( + Effect.gen(function* () { + if (!entry.supported) return yield* read; + const fail = () => + new GitHubCli.GitHubCliCommandError({ + command: "gh", + cwd: input.cwd, + cause: new Error("GitHub returned an invalid conditional checks response."), + }); + const get = (endpoint: string, head = false) => + Effect.gen(function* () { + const previous = entry.validators.get(endpoint); + const result = yield* github + .execute({ + cwd: input.cwd, + args: [ + "api", + endpoint, + "--hostname", + input.host, + "--include", + ...(head + ? [ + "--jq", + "{head:{sha:.head.sha},base:{repo:{id:.base.repo.id}},headRepositoryId:.head.repo.id}", + ] + : ["--silent"]), + ...(previous?.etag ? ["-H", `If-None-Match: ${previous.etag}`] : []), + ], + acceptNotModified: true, + }) + .pipe( + Effect.catchTags({ + GitHubCliCommandError: (error) => Effect.sync(() => { entry.supported = ![404, 405, 501].includes(error.httpStatus ?? 0); return null; }), - ), - ); - if (result === null) { - entry.value = null; - return null; - } - if (result.stdoutTruncated || result.stdoutInvalidUtf8) return yield* fail(); - const split = result.stdout.search(/\r?\n\r?\n/); - const headers = split < 0 ? result.stdout : result.stdout.slice(0, split); - const status = /^HTTP\/\S+ (\d+)/.exec(headers)?.[1]; - if (status === "304" && previous) return previous.next; - if (status !== "200") return yield* fail(); + }), + ); + if (result === null) { entry.value = null; - if (head) { - const decoded = yield* decodeHead(result.stdout.slice(split).trim()).pipe( - Effect.mapError(fail), - ); - if (entry.head?.head.sha !== decoded.head.sha) entry.validators.clear(); - entry.head = decoded; - } - const validator = { - etag: /^etag:\s*(.+)$/im.exec(headers)?.[1]?.trim(), - next: /^link:.*rel="next"/im.test(headers), - }; - entry.validators.set(endpoint, validator); - return validator.next; - }); - const root = `repos/${input.repository}`; - if ((yield* get(`${root}/pulls/${input.number}`, true)) === null) return yield* read; - const head = entry.head; - if (head === null) return yield* fail(); - const endpoints = [ - `${root}/commits/${head.head.sha}/check-runs?filter=all`, - `${root}/commits/${head.head.sha}/status`, - ...(head.headRepositoryId !== head.base.repo.id - ? [`${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request`] - : []), - ]; - for (const endpoint of endpoints) { - for (let page = 1; ; page++) { - if (page > 100) return yield* fail(); - const next = yield* get( - `${endpoint}${endpoint.includes("?") ? "&" : "?"}per_page=100&page=${page}`, + return null; + } + if (result.stdoutTruncated || result.stdoutInvalidUtf8) return yield* fail(); + const split = result.stdout.search(/\r?\n\r?\n/); + const headers = split < 0 ? result.stdout : result.stdout.slice(0, split); + const status = /^HTTP\/\S+ (\d+)/.exec(headers)?.[1]; + if (status === "304" && previous) return previous.next; + if (status !== "200") return yield* fail(); + entry.value = null; + if (head) { + const decoded = yield* decodeHead(result.stdout.slice(split).trim()).pipe( + Effect.mapError(fail), ); - if (next === null) return yield* read; - if (!next) break; + if (entry.head?.head.sha !== decoded.head.sha) entry.validators.clear(); + entry.head = decoded; } + const validator = { + etag: /^etag:\s*(.+)$/im.exec(headers)?.[1]?.trim(), + next: /^link:.*rel="next"/im.test(headers), + }; + entry.validators.set(endpoint, validator); + return validator.next; + }); + const root = `repos/${input.repository}`; + if ((yield* get(`${root}/pulls/${input.number}`, true)) === null) return yield* read; + const head = entry.head; + if (head === null) return yield* fail(); + const endpoints = [ + `${root}/commits/${head.head.sha}/check-runs?filter=all`, + `${root}/commits/${head.head.sha}/status`, + ...(head.headRepositoryId !== head.base.repo.id + ? [`${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request`] + : []), + ]; + for (const endpoint of endpoints) { + for (let page = 1; ; page++) { + if (page > 100) return yield* fail(); + const next = yield* get( + `${endpoint}${endpoint.includes("?") ? "&" : "?"}per_page=100&page=${page}`, + ); + if (next === null) return yield* read; + if (!next) break; } - const now = yield* Clock.currentTimeMillis; - if (entry.value !== null && now - entry.readAt < 5 * 60_000) return entry.value; - const fresh = yield* read; - const value = { - state: fresh.state, - checks: fresh.checks, - ...(fresh.headSha ? { headSha: fresh.headSha } : {}), - }; - if (fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha) - entry.value = value; - entry.readAt = now; - return value; - }), - ); - }); - }); + } + const now = yield* Clock.currentTimeMillis; + if (entry.value !== null && now - entry.readAt < 5 * 60_000) return entry.value; + const fresh = yield* read; + const value = { + state: fresh.state, + checks: fresh.checks, + ...(fresh.headSha ? { headSha: fresh.headSha } : {}), + }; + if (fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha) + entry.value = value; + entry.readAt = now; + return value; + }), + ); + }); +}); From 5a49d9e4cadf372addbe6766aa944aedf82f9537 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:43:31 +0200 Subject: [PATCH 5/9] fix(web): keep idle PR views paused on focus --- apps/web/src/hooks/useLiveRefresh.test.ts | 38 +++++++++++++++++++++++ apps/web/src/hooks/useLiveRefresh.ts | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/apps/web/src/hooks/useLiveRefresh.test.ts b/apps/web/src/hooks/useLiveRefresh.test.ts index b85c1573fd1f..e8524a456a5e 100644 --- a/apps/web/src/hooks/useLiveRefresh.test.ts +++ b/apps/web/src/hooks/useLiveRefresh.test.ts @@ -68,6 +68,44 @@ describe("live refresh cadence", () => { it("waits five minutes between automatic host reads", () => { expect(LIVE_REFRESH_INTERVAL_MS).toBe(5 * 60_000); }); + + it.each(["focus", "visibilitychange"])( + "keeps an idle view paused after %s until input", + (event) => { + vi.useFakeTimers(); + vi.setSystemTime(1_000_000); + const document = Object.assign(new EventTarget(), { visibilityState: "visible" }); + const window = new EventTarget(); + vi.stubGlobal("document", document); + vi.stubGlobal("window", window); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const refresh = vi.fn(); + let renderer: ReactTestRenderer | undefined; + function Probe() { + useLiveRefresh(refresh, { intervalMs: 45_000 }); + return null; + } + try { + act(() => { + renderer = create(createElement(Probe)); + }); + act(() => vi.advanceTimersByTime(LIVE_REFRESH_IDLE_AFTER_MS)); + refresh.mockClear(); + act(() => (event === "focus" ? window : document).dispatchEvent(new Event(event))); + expect(refresh).not.toHaveBeenCalled(); + act(() => vi.advanceTimersByTime(90_000)); + expect(refresh).not.toHaveBeenCalled(); + act(() => document.dispatchEvent(new Event("pointerdown"))); + expect(refresh).toHaveBeenCalledTimes(1); + act(() => vi.advanceTimersByTime(45_000)); + expect(refresh).toHaveBeenCalledTimes(2); + } finally { + act(() => renderer?.unmount()); + vi.useRealTimers(); + vi.unstubAllGlobals(); + } + }, + ); }); describe("shouldLiveRefresh", () => { diff --git a/apps/web/src/hooks/useLiveRefresh.ts b/apps/web/src/hooks/useLiveRefresh.ts index 439eb40a5d8f..e1555b65b466 100644 --- a/apps/web/src/hooks/useLiveRefresh.ts +++ b/apps/web/src/hooks/useLiveRefresh.ts @@ -146,7 +146,7 @@ export function useLiveRefresh( const visible = () => document.visibilityState === "visible"; const onArrival = () => { const now = Date.now(); - if (visible()) lastInteractedAt = now; + if (now - lastInteractedAt >= LIVE_REFRESH_IDLE_AFTER_MS) return; const lastRefreshedAt = lastRefreshedAtByView.get(viewId); if (lastRefreshedAt === undefined) { // Nothing read yet, so nothing to refresh: the mount's own read is what fills this in. From 3f675d0b145ed507bb41736f3267a8981a413b22 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:45:22 +0200 Subject: [PATCH 6/9] refactor(pr): simplify checks refresh cadence --- .../GitHubPullRequestProvider.test.ts | 2 +- .../pullRequest/GitHubPullRequestProvider.ts | 6 +---- .../gitHubConditionalChecks.test.ts | 3 ++- .../pullRequest/gitHubConditionalChecks.ts | 1 - .../components/chat/ThreadDetailsPrRow.tsx | 2 -- .../hooks/usePullRequestChecksRefresh.test.ts | 14 ++++------- .../src/hooks/usePullRequestChecksRefresh.ts | 24 +++---------------- packages/contracts/src/pullRequest.ts | 1 - 8 files changed, 11 insertions(+), 42 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 0970686cc90f..4b2d618a0768 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -411,7 +411,7 @@ describe("gitHubViewerPermissions", () => { if (readChecks === undefined) return yield* Effect.die("checks read missing"); expect( yield* readChecks({ cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }), - ).toEqual({ state: detail.state, checks: detail.checks, headSha: "abc123" }); + ).toEqual({ state: detail.state, checks: detail.checks }); expect(detail.workflowApprovalsRequired).toBe(1); expect(detail.checks).toEqual([ { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 49938c0f2db2..5e70c2ee1ed4 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -364,11 +364,7 @@ export const make = Effect.gen(function* () { getChangeRequestChecks: (input) => cli.revalidateChecks(input, readChecks(input)).pipe( - Effect.map(({ state, checks, headSha }) => ({ - state, - checks, - ...(headSha == null ? {} : { headSha }), - })), + Effect.map(({ state, checks }) => ({ state, checks })), Effect.mapError(fail("getChangeRequestChecks")), ), diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts index 61a753190801..294b5e265a8d 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts @@ -87,7 +87,8 @@ it.effect( sha = "b".repeat(40); changed = "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/pulls/1"; requests.length = 0; - expect((yield* poll()).headSha).toBe(sha); + yield* poll(); + expect(reads).toBe(6); expect( requests .filter((endpoint) => endpoint.includes("/commits/")) diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.ts index 723b39f4001c..e8c60623c30f 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.ts @@ -139,7 +139,6 @@ export const makeChecksRevalidator = Effect.gen(function* () { const value = { state: fresh.state, checks: fresh.checks, - ...(fresh.headSha ? { headSha: fresh.headSha } : {}), }; if (fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha) entry.value = value; diff --git a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx index d89772ebaa8f..1ad0778b1b5c 100644 --- a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx +++ b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx @@ -144,8 +144,6 @@ export function ThreadDetailsPrRow({ enabled: open && supportsChecks && !(checksQuery.isSuccess && checksQuery.data === null), key: `workspace-pr-checks:${refreshKey}`, checks: detail?.checks ?? [], - headSha: checksQuery.data?.headSha, - updatedAt: checksQuery.dataUpdatedAt, }); const { actionPending, perform } = usePullRequestActionRunner({ diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts index 613848c60a11..7505fc62c470 100644 --- a/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts @@ -5,7 +5,7 @@ import { expect, it, vi } from "vite-plus/test"; import { usePullRequestChecksRefresh } from "./usePullRequestChecksRefresh"; -it("polls quiet PRs each minute, speeds up for runs and pushes, resumes from idle, and stops for closed PRs", () => { +it("polls quiet PRs each minute, speeds up for pending or missing checks, resumes from idle, and stops for closed PRs", () => { vi.useFakeTimers(); vi.setSystemTime(1_000_000); const document = Object.assign(new EventTarget(), { visibilityState: "visible" }); @@ -13,16 +13,13 @@ it("polls quiet PRs each minute, speeds up for runs and pushes, resumes from idl vi.stubGlobal("window", new EventTarget()); vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); const refresh = vi.fn(); - let updatedAt = Date.now(); let renderer: ReactTestRenderer | undefined; function Probe({ status = "success", - headSha = "one", enabled = true, busy = false, }: { status?: PullRequestCheck["status"] | null; - headSha?: string; enabled?: boolean; busy?: boolean; }) { @@ -30,14 +27,11 @@ it("polls quiet PRs each minute, speeds up for runs and pushes, resumes from idl refresh: busy ? null : refresh, enabled, key: "test-pr-checks", - headSha, - updatedAt, checks: status === null ? [] : [{ name: "CI", status, description: null, url: null }], }); return null; } const update = (props: Parameters[0]) => { - updatedAt = Date.now(); act(() => renderer?.update(createElement(Probe, props))); }; const advance = (ms: number) => act(() => vi.advanceTimersByTime(ms)); @@ -57,13 +51,13 @@ it("polls quiet PRs each minute, speeds up for runs and pushes, resumes from idl update({ status: "failure" }); advance(60_000); expect(refresh).toHaveBeenCalledTimes(3); - update({ headSha: "two", status: null }); + update({ status: null }); for (let tick = 0; tick < 3; tick++) { advance(45_000); - update({ headSha: "two", status: null }); + update({ status: null }); } expect(refresh).toHaveBeenCalledTimes(6); - advance(59_999); + advance(44_999); expect(refresh).toHaveBeenCalledTimes(6); advance(1); expect(refresh).toHaveBeenCalledTimes(7); diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.ts index 6e32f0c07537..ca8631f32611 100644 --- a/apps/web/src/hooks/usePullRequestChecksRefresh.ts +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.ts @@ -1,5 +1,4 @@ import type { PullRequestCheck } from "@t3tools/contracts"; -import { useState } from "react"; import { useLiveRefresh } from "./useLiveRefresh"; @@ -8,30 +7,13 @@ export function usePullRequestChecksRefresh(input: { enabled: boolean; key: string; checks: ReadonlyArray; - headSha: string | undefined; - updatedAt: number; }) { - const [observed, setObserved] = useState(() => ({ - key: input.key, - headSha: input.headSha, - waitingSince: input.checks.length === 0 ? input.updatedAt : null, - })); - if (observed.key !== input.key || observed.headSha !== input.headSha) { - setObserved({ - key: input.key, - headSha: input.headSha, - waitingSince: - input.checks.length === 0 || (observed.key === input.key && observed.headSha !== undefined) - ? input.updatedAt - : null, - }); - } - const waiting = - observed.waitingSince !== null && input.updatedAt - observed.waitingSince < 2 * 60_000; useLiveRefresh(input.refresh, { key: input.key, enabled: input.enabled, intervalMs: - waiting || input.checks.some((check) => check.status === "pending") ? 45_000 : 60_000, + input.checks.length === 0 || input.checks.some((check) => check.status === "pending") + ? 45_000 + : 60_000, }); } diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index c5e8ad3fd523..75e9312f92aa 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -882,7 +882,6 @@ export const PullRequestDetail = Schema.Struct({ export type PullRequestDetail = typeof PullRequestDetail.Type; export const PullRequestChecks = Schema.Struct({ - headSha: Schema.optional(TrimmedNonEmptyString), state: PullRequestState, checks: Schema.Array(PullRequestCheck), }); From dfb5190d823eed7db14d27d9c7670d53cdd94f41 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:49:37 +0200 Subject: [PATCH 7/9] fix(pr): discard cached checks after incomplete refreshes --- .../src/pullRequest/gitHubConditionalChecks.test.ts | 12 +++++++++--- .../src/pullRequest/gitHubConditionalChecks.ts | 6 ++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts index 294b5e265a8d..d4772789e133 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts @@ -164,17 +164,23 @@ it.effect("does not retain failed or incomplete reads, and supports hosts withou yield* poll(); yield* poll(); expect(reads).toBe(4); + yield* TestClock.adjust("5 minutes"); + complete = false; + yield* poll(); + yield* poll(); + expect(reads).toBe(6); + complete = true; unavailable = true; yield* poll(); - expect(reads).toBe(5); + expect(reads).toBe(7); unavailable = false; yield* poll(); yield* poll(); - expect(reads).toBe(6); + expect(reads).toBe(8); etags = false; yield* poll(); yield* poll(); - expect(reads).toBe(8); + expect(reads).toBe(10); }), ); diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.ts index e8c60623c30f..04d2f8706c52 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.ts @@ -140,8 +140,10 @@ export const makeChecksRevalidator = Effect.gen(function* () { state: fresh.state, checks: fresh.checks, }; - if (fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha) - entry.value = value; + entry.value = + fresh.workflowApprovalsRequired !== undefined && fresh.headSha === head.head.sha + ? value + : null; entry.readAt = now; return value; }), From 5afcab4cee2324abb6b192720babee91cf249594 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:57:02 +0200 Subject: [PATCH 8/9] fix(pr): poll checks awaiting approval at the active rate --- .../hooks/usePullRequestChecksRefresh.test.ts | 151 +++++++++--------- .../src/hooks/usePullRequestChecksRefresh.ts | 3 +- 2 files changed, 79 insertions(+), 75 deletions(-) diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts index 7505fc62c470..749f04d8c628 100644 --- a/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.test.ts @@ -5,79 +5,82 @@ import { expect, it, vi } from "vite-plus/test"; import { usePullRequestChecksRefresh } from "./usePullRequestChecksRefresh"; -it("polls quiet PRs each minute, speeds up for pending or missing checks, resumes from idle, and stops for closed PRs", () => { - vi.useFakeTimers(); - vi.setSystemTime(1_000_000); - const document = Object.assign(new EventTarget(), { visibilityState: "visible" }); - vi.stubGlobal("document", document); - vi.stubGlobal("window", new EventTarget()); - vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); - const refresh = vi.fn(); - let renderer: ReactTestRenderer | undefined; - function Probe({ - status = "success", - enabled = true, - busy = false, - }: { - status?: PullRequestCheck["status"] | null; - enabled?: boolean; - busy?: boolean; - }) { - usePullRequestChecksRefresh({ - refresh: busy ? null : refresh, - enabled, - key: "test-pr-checks", - checks: status === null ? [] : [{ name: "CI", status, description: null, url: null }], - }); - return null; - } - const update = (props: Parameters[0]) => { - act(() => renderer?.update(createElement(Probe, props))); - }; - const advance = (ms: number) => act(() => vi.advanceTimersByTime(ms)); - try { - act(() => { - renderer = create(createElement(Probe)); - }); - advance(59_999); - expect(refresh).toHaveBeenCalledTimes(0); - advance(1); - expect(refresh).toHaveBeenCalledTimes(1); - update({ status: "pending" }); - advance(44_999); - expect(refresh).toHaveBeenCalledTimes(1); - advance(1); - expect(refresh).toHaveBeenCalledTimes(2); - update({ status: "failure" }); - advance(60_000); - expect(refresh).toHaveBeenCalledTimes(3); - update({ status: null }); - for (let tick = 0; tick < 3; tick++) { - advance(45_000); +it.each(["pending", "action-required"] as const)( + "polls quiet PRs each minute, speeds up for %s or missing checks, resumes from idle, and stops for closed PRs", + (pendingStatus) => { + vi.useFakeTimers(); + vi.setSystemTime(1_000_000); + const document = Object.assign(new EventTarget(), { visibilityState: "visible" }); + vi.stubGlobal("document", document); + vi.stubGlobal("window", new EventTarget()); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const refresh = vi.fn(); + let renderer: ReactTestRenderer | undefined; + function Probe({ + status = "success", + enabled = true, + busy = false, + }: { + status?: PullRequestCheck["status"] | null; + enabled?: boolean; + busy?: boolean; + }) { + usePullRequestChecksRefresh({ + refresh: busy ? null : refresh, + enabled, + key: `test-pr-checks:${pendingStatus}`, + checks: status === null ? [] : [{ name: "CI", status, description: null, url: null }], + }); + return null; + } + const update = (props: Parameters[0]) => { + act(() => renderer?.update(createElement(Probe, props))); + }; + const advance = (ms: number) => act(() => vi.advanceTimersByTime(ms)); + try { + act(() => { + renderer = create(createElement(Probe)); + }); + advance(59_999); + expect(refresh).toHaveBeenCalledTimes(0); + advance(1); + expect(refresh).toHaveBeenCalledTimes(1); + update({ status: pendingStatus }); + advance(44_999); + expect(refresh).toHaveBeenCalledTimes(1); + advance(1); + expect(refresh).toHaveBeenCalledTimes(2); + update({ status: "failure" }); + advance(60_000); + expect(refresh).toHaveBeenCalledTimes(3); update({ status: null }); + for (let tick = 0; tick < 3; tick++) { + advance(45_000); + update({ status: null }); + } + expect(refresh).toHaveBeenCalledTimes(6); + advance(44_999); + expect(refresh).toHaveBeenCalledTimes(6); + advance(1); + expect(refresh).toHaveBeenCalledTimes(7); + advance(6 * 60_000); + const beforeResume = refresh.mock.calls.length; + act(() => document.dispatchEvent(new Event("pointerdown"))); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); + document.visibilityState = "hidden"; + act(() => document.dispatchEvent(new Event("visibilitychange"))); + advance(60_000); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); + document.visibilityState = "visible"; + act(() => document.dispatchEvent(new Event("visibilitychange"))); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); + update({ enabled: false }); + advance(120_000); + expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); + } finally { + act(() => renderer?.unmount()); + vi.useRealTimers(); + vi.unstubAllGlobals(); } - expect(refresh).toHaveBeenCalledTimes(6); - advance(44_999); - expect(refresh).toHaveBeenCalledTimes(6); - advance(1); - expect(refresh).toHaveBeenCalledTimes(7); - advance(6 * 60_000); - const beforeResume = refresh.mock.calls.length; - act(() => document.dispatchEvent(new Event("pointerdown"))); - expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); - document.visibilityState = "hidden"; - act(() => document.dispatchEvent(new Event("visibilitychange"))); - advance(60_000); - expect(refresh).toHaveBeenCalledTimes(beforeResume + 1); - document.visibilityState = "visible"; - act(() => document.dispatchEvent(new Event("visibilitychange"))); - expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); - update({ enabled: false }); - advance(120_000); - expect(refresh).toHaveBeenCalledTimes(beforeResume + 2); - } finally { - act(() => renderer?.unmount()); - vi.useRealTimers(); - vi.unstubAllGlobals(); - } -}); + }, +); diff --git a/apps/web/src/hooks/usePullRequestChecksRefresh.ts b/apps/web/src/hooks/usePullRequestChecksRefresh.ts index ca8631f32611..d83ca968a2dc 100644 --- a/apps/web/src/hooks/usePullRequestChecksRefresh.ts +++ b/apps/web/src/hooks/usePullRequestChecksRefresh.ts @@ -12,7 +12,8 @@ export function usePullRequestChecksRefresh(input: { key: input.key, enabled: input.enabled, intervalMs: - input.checks.length === 0 || input.checks.some((check) => check.status === "pending") + input.checks.length === 0 || + input.checks.some((check) => check.status === "pending" || check.status === "action-required") ? 45_000 : 60_000, }); From 1864d771f55f7c6bea40ebcd6502cbd2456d02cd Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:57:16 +0200 Subject: [PATCH 9/9] refactor(pr): preserve push refresh service namespaces --- apps/server/src/git/refreshPushedPullRequests.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/server/src/git/refreshPushedPullRequests.ts b/apps/server/src/git/refreshPushedPullRequests.ts index c89e3bf3ec5a..4cde0eaff6db 100644 --- a/apps/server/src/git/refreshPushedPullRequests.ts +++ b/apps/server/src/git/refreshPushedPullRequests.ts @@ -1,9 +1,9 @@ import type { GitRunStackedActionInput, GitRunStackedActionResult } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; -import { OrchestratorV2 } from "../orchestration-v2/Orchestrator.ts"; -import { ProjectionSnapshotQuery } from "../orchestration/Services/ProjectionSnapshotQuery.ts"; -import { PullRequestService } from "../pullRequest/PullRequestService.ts"; +import * as OrchestratorV2 from "../orchestration-v2/Orchestrator.ts"; +import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSnapshotQuery.ts"; +import * as PullRequestService from "../pullRequest/PullRequestService.ts"; export const refreshPushedPullRequests = Effect.fn("refreshPushedPullRequests")( function* ( @@ -11,9 +11,9 @@ export const refreshPushedPullRequests = Effect.fn("refreshPushedPullRequests")( result: Pick, ) { if (result.push.status !== "pushed") return; - const pullRequests = yield* PullRequestService; + const pullRequests = yield* PullRequestService.PullRequestService; if (input.threadId !== undefined) { - const engine = yield* OrchestratorV2; + const engine = yield* OrchestratorV2.OrchestratorV2; const thread = yield* engine.getThreadShell(input.threadId); if (thread !== null) { yield* pullRequests.refreshAfterTurn(thread.projectId); @@ -24,7 +24,7 @@ export const refreshPushedPullRequests = Effect.fn("refreshPushedPullRequests")( yield* pullRequests.refreshAfterTurn(input.projectId); return; } - const snapshots = yield* ProjectionSnapshotQuery; + const snapshots = yield* ProjectionSnapshotQuery.ProjectionSnapshotQuery; const projects = yield* snapshots.getProjectShellsWithoutEnrichment(); yield* Effect.forEach( projects.filter((project) => project.workspaceRoot === input.cwd),