diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index a438ac780f43..a3cb8ae76d48 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2748,6 +2748,34 @@ it.effect("deduplicates routed PR reports without dropping draft changes", () => }), ); +it.effect("preserves check observation time on cache hits and advances it on refresh", () => + Effect.gen(function* () { + let reads = 0; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + getChangeRequestChecks: () => + Effect.sync(() => { + reads += 1; + return { state: "open" as const, checks: [] }; + }), + }), + ], + }); + yield* TestClock.setTime(1_000); + const first = yield* service.checks(reference); + expect(first).toMatchObject({ observedAt: 1_000 }); + yield* TestClock.adjust("1 second"); + expect(yield* service.checks(reference)).toEqual(first); + expect(reads).toBe(1); + yield* TestClock.adjust("15 seconds"); + expect(yield* service.checks(reference)).toMatchObject({ observedAt: 17_000 }); + expect(reads).toBe(2); + }), +); + it.effect("reuses an observed merged state for strict settlement reads", () => Effect.gen(function* () { const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 3c6c9b333fbf..bc763834f7f6 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2790,7 +2790,11 @@ export const make = Effect.gen(function* () { host: project.host, number: input.number, }) - .pipe(Effect.mapError(toPullRequestError("checks"))), + .pipe( + Effect.mapError(toPullRequestError("checks")), + observeRead, + Effect.map(({ value, observedAt }) => ({ ...value, observedAt })), + ), ), ); }, diff --git a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx index c6741fa035e4..85c65005b9d8 100644 --- a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx +++ b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx @@ -32,6 +32,7 @@ import { buildFixFindingsHandoff, buildResolveConflictsPrompt, classifyPullRequestChecks, + resolvePullRequestDetailChecks, describePullRequestChecks, isPullRequestConflicting, resolvePullRequestMergeMethod, @@ -135,12 +136,7 @@ export function ThreadDetailsPrRow({ const checksSettledEmpty = useEnvironmentQuerySelector(checksAtom, isSettledWithoutChecks); const checksQuery = useEnvironmentQueryRefresh(checksAtom); const checksAreNewer = useEnvironmentQueryIsNewer(checksAtom, detailAtom); - const detail = - detailData === null - ? null - : checksData !== null && checksAreNewer - ? { ...detailData, ...checksData } - : detailData; + const detail = resolvePullRequestDetailChecks(detailData, checksData, checksAreNewer); const open = reference !== null && (detail?.state ?? pr?.state) === "open"; const refreshKey = `${environmentId}:${project?.id}:${reference?.host}:${reference?.repository}:${number}`; useLiveRefresh( diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index ad7e99ecf87e..292d40bfe400 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -46,6 +46,7 @@ import { checkoutFailureDescription, readPullRequestDetailSnapshot, resolveDisplayedPullRequestDetail, + resolvePullRequestDetailChecks, resolvePullRequestReferenceHost, resolvePullRequestPrimaryControl, allowsSinglePullRequestMerge, @@ -1245,6 +1246,59 @@ describe("cached pull request detail", () => { ...overrides, }) as PullRequestDetail; + it("uses fresh checks for detail counts without replacing other PR fields", () => { + const pending = (name: string): PullRequestCheck => ({ + name, + status: "pending", + url: null, + description: null, + }); + const original = detail({ checks: [pending("build"), pending("test")] }); + const checks = [pending("build"), { ...pending("test"), status: "success" as const }]; + expect(resolvePullRequestDetailChecks(original, { state: "closed", checks }, true)).toEqual({ + ...original, + state: "closed", + checks, + }); + expect(resolvePullRequestDetailChecks(original, { state: "closed", checks }, false)).toBe( + original, + ); + expect(resolvePullRequestDetailChecks(original, null, true)).toBe(original); + expect(resolvePullRequestDetailChecks(null, { state: "closed", checks }, true)).toBeNull(); + }); + + it("keeps newer checks when an older cached detail arrives later", () => { + const pending: PullRequestCheck = { + name: "build", + status: "pending", + url: null, + description: null, + }; + const cached = detail({ observedAt: 1_000, checks: [pending] }); + const checks = { + state: "open" as const, + checks: [{ ...pending, status: "success" as const }], + observedAt: 2_000, + }; + const resolved = resolvePullRequestDetailChecks(cached, checks, false); + expect(resolved?.checks).toEqual(checks.checks); + expect(resolved?.observedAt).toBe(1_000); + expect( + resolvePullRequestDetailChecks(cached, { state: checks.state, checks: checks.checks }, true) + ?.checks, + ).toEqual(checks.checks); + const refreshed = detail({ observedAt: 3_000, checks: [pending] }); + expect(resolvePullRequestDetailChecks(refreshed, checks, true)).toBe(refreshed); + }); + + it("keeps merged details settled while applying newer checks", () => { + const merged = detail({ state: "merged", observedAt: 1_000 }); + const checks = [{ name: "build", status: "success" as const, url: null, description: null }]; + expect( + resolvePullRequestDetailChecks(merged, { state: "open", observedAt: 2_000, checks }, true), + ).toEqual({ ...merged, checks }); + }); + const makeStorage = () => { const held = new Map(); return { diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index b4d7a7052163..217a36ee54c7 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -10,6 +10,7 @@ import { type PullRequestActor, type PullRequestBaseComparison, type PullRequestCheck, + type PullRequestChecks, type PullRequestChecksState as ContractPullRequestChecksState, type PullRequestComment, type PullRequestCommit, @@ -1419,3 +1420,22 @@ export function resolveDisplayedPullRequestDetail(input: { return null; } } + +export function resolvePullRequestDetailChecks( + detail: PullRequestDetail | null, + checks: PullRequestChecks | null, + checksAreNewer: boolean, +): PullRequestDetail | null { + if (detail === null || checks === null) return detail; + const newer = + detail.observedAt !== undefined && checks.observedAt !== undefined + ? checks.observedAt >= detail.observedAt + : checksAreNewer; + return newer + ? { + ...detail, + state: detail.state === "merged" ? "merged" : checks.state, + checks: checks.checks, + } + : detail; +} diff --git a/apps/web/src/components/pullRequest/usePullRequestDetail.ts b/apps/web/src/components/pullRequest/usePullRequestDetail.ts index a3228f6b64fa..2ef76be9b159 100644 --- a/apps/web/src/components/pullRequest/usePullRequestDetail.ts +++ b/apps/web/src/components/pullRequest/usePullRequestDetail.ts @@ -7,6 +7,8 @@ import { } from "@t3tools/contracts"; import { useEffect, useEffectEvent, useState } from "react"; +import { usePullRequestChecksRefresh } from "~/hooks/usePullRequestChecksRefresh"; +import { useServerConfigs } from "~/state/entities"; import { useEnvironmentQuery } from "~/state/query"; import { useEnvironmentScope } from "~/state/session"; import { @@ -18,6 +20,7 @@ import { import { readPullRequestDetailSnapshot, resolveDisplayedPullRequestDetail, + resolvePullRequestDetailChecks, writePullRequestDetailSnapshot, } from "./pullRequestDetail.logic"; import { pullRequestChecksState } from "./pullRequestPresentation"; @@ -42,11 +45,35 @@ export function usePullRequestDetail( const scopeKey = `${environmentId}:${pullRequestRefKey(reference)}`; const canWriteSourceControl = useEnvironmentScope(environmentId, AuthSourceControlWriteScope); const detailQuery = useEnvironmentQuery( - pullRequestEnvironment.detail({ environmentId, input: reference }), + pullRequestEnvironment.detail({ environmentId, input: { ...reference, allowStale: false } }), ); const activityQuery = useEnvironmentQuery( pullRequestEnvironment.activity({ environmentId, input: reference }), ); + const serverConfigs = useServerConfigs(); + const supportsChecks = + serverConfigs.get(environmentId)?.environment.capabilities.pullRequestChecks === true; + const checksQuery = useEnvironmentQuery( + supportsChecks && detailQuery.data !== null + ? pullRequestEnvironment.checks({ environmentId, input: reference }) + : null, + ); + const liveDetail = resolvePullRequestDetailChecks( + detailQuery.data, + checksQuery.data, + checksQuery.dataUpdatedAt >= detailQuery.dataUpdatedAt, + ); + usePullRequestChecksRefresh({ + refresh: () => { + if (!checksQuery.isPending && !detailQuery.isPending) checksQuery.refresh(); + }, + enabled: + liveDetail?.state === "open" && + supportsChecks && + !(checksQuery.isSuccess && checksQuery.data === null), + key: `workspace-pr-checks:${environmentId}:${reference.projectId}:${reference.host}:${reference.repository}:${reference.number}`, + checks: liveDetail?.checks ?? [], + }); const [cachedDetail, setCachedDetail] = useState(() => readPullRequestDetailSnapshot( typeof window === "undefined" ? undefined : window.localStorage, @@ -79,16 +106,16 @@ export function usePullRequestDetail( useEffect(() => { if (detailQuery.data !== null) saveDetailSnapshot(detailQuery.data); }, [detailQuery.data]); - const detailSummary = pullRequestDetailSummary(detailQuery.data); + const detailSummary = pullRequestDetailSummary(liveDetail); const observedSummary = useSharedPullRequestSummary( environmentId, reference, detailSummary, - detailQuery.dataUpdatedAt, + Math.max(detailQuery.dataUpdatedAt, checksQuery.dataUpdatedAt), ); const activity = activityQuery.data; const resolvedCoreDetail = resolveDisplayedPullRequestDetail({ - live: detailQuery.data, + live: liveDetail, cached: cachedDetail, reference, }); diff --git a/packages/client-runtime/src/state/pullRequests.test.ts b/packages/client-runtime/src/state/pullRequests.test.ts index a3ca6dc62a15..74493ff95fbc 100644 --- a/packages/client-runtime/src/state/pullRequests.test.ts +++ b/packages/client-runtime/src/state/pullRequests.test.ts @@ -1277,6 +1277,35 @@ it.effect("shares close, reopen, and merge with an untouched client's mounted PR ), ); +it.effect("shares checks reads across reference property order", () => + Effect.scoped( + Effect.gen(function* () { + let reads = 0; + const client = { + [WS_METHODS.pullRequestsSubscribeRefreshes]: () => Stream.never, + [WS_METHODS.pullRequestsChecks]: () => + Effect.sync(() => { + reads += 1; + return { state: "open", checks: [], observedAt: 1_000 }; + }), + } as unknown as WsRpcProtocolClient; + const { atoms, registry } = yield* makeTestRuntime(client); + const projectId = ProjectId.make("project-1"); + const row = atoms.checks({ + environmentId: TARGET.environmentId, + input: { host: "github.com", repository: "acme/web", number: 1, projectId }, + }); + const panel = atoms.checks({ + environmentId: TARGET.environmentId, + input: { projectId, host: "github.com", repository: "acme/web", number: 1 }, + }); + yield* AtomRegistry.getResult(registry, row); + yield* AtomRegistry.getResult(registry, panel); + expect(reads).toBe(1); + }), + ), +); + it.effect("keeps live detail reads separate from reads that allow stale data", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index 757037734479..fa5385c09fad 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -55,6 +55,26 @@ export class EnvironmentHttpConnectionNotReadyError extends Data.TaggedError( const LINKED_PULL_REQUEST_IDLE_TTL_MS = 5_000; +function normalizePullRequestQuery( + query: (target: { readonly environmentId: EnvironmentId; readonly input: PullRequestRef }) => A, +) { + return ({ + environmentId, + input: { projectId, host, repository, number, allowStale, expectedAccountId }, + }: Parameters[0]) => + query({ + environmentId, + input: { + projectId, + ...(host === undefined ? {} : { host }), + repository, + number, + ...(allowStale === undefined ? {} : { allowStale }), + ...(expectedAccountId === undefined ? {} : { expectedAccountId }), + }, + }); +} + function writableQueryFamily( family: (target: { readonly environmentId: EnvironmentId; @@ -77,22 +97,7 @@ function writableQueryFamily( (refresh) => refresh(source), ).pipe(Atom.setIdleTTL(5 * 60_000)), ); - return ({ - environmentId, - input: { projectId, host, repository, number, allowStale }, - }: Parameters[0]) => - writable( - family({ - environmentId, - input: { - projectId, - ...(host === undefined ? {} : { host }), - repository, - number, - ...(allowStale === undefined ? {} : { allowStale }), - }, - }), - ); + return normalizePullRequestQuery((target) => writable(family(target))); } /** Restart pre-mutation reads before patching so they cannot restore stale values. */ @@ -319,14 +324,16 @@ export function createPullRequestEnvironmentAtoms( }), detail, preview, - checks: createEnvironmentRpcQueryAtomFamily(runtime, { - label: "environment-data:pull-requests:checks", - persistence: persistQuery(Schema.NullOr(PullRequestChecks)), - tag: WS_METHODS.pullRequestsChecks, - execute: (input) => routedRequest(WS_METHODS.pullRequestsChecks, input), - staleTimeMs: 45_000, - refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), - }), + checks: normalizePullRequestQuery( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:checks", + persistence: persistQuery(Schema.NullOr(PullRequestChecks)), + tag: WS_METHODS.pullRequestsChecks, + execute: (input) => routedRequest(WS_METHODS.pullRequestsChecks, input), + staleTimeMs: 45_000, + refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + }), + ), activity, threadComments: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:thread-comments", diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index a68f5394b0dd..5f9bc23048dd 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -589,6 +589,7 @@ export const PullRequestDetail = Schema.Struct({ export type PullRequestDetail = typeof PullRequestDetail.Type; export const PullRequestChecks = Schema.Struct({ + observedAt: Schema.optional(Schema.Finite), state: PullRequestState, checks: Schema.Array(PullRequestCheck), }); diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index 3bb7a757f773..daa166e654e2 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -342,7 +342,7 @@ export interface PullRequestProviderApi { readonly getChangeRequestChecks?: ( input: ProviderRepositoryRef & { readonly number: number }, - ) => Effect.Effect; + ) => Effect.Effect, PullRequestProviderError>; readonly getChangeRequest: ( input: ProviderRepositoryRef & { readonly number: number },