Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
6 changes: 5 additions & 1 deletion apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 })),
),
),
);
},
Expand Down
8 changes: 2 additions & 6 deletions apps/web/src/components/chat/ThreadDetailsPrRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
buildFixFindingsHandoff,
buildResolveConflictsPrompt,
classifyPullRequestChecks,
resolvePullRequestDetailChecks,
describePullRequestChecks,
isPullRequestConflicting,
resolvePullRequestMergeMethod,
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
checkoutFailureDescription,
readPullRequestDetailSnapshot,
resolveDisplayedPullRequestDetail,
resolvePullRequestDetailChecks,
resolvePullRequestReferenceHost,
resolvePullRequestPrimaryControl,
allowsSinglePullRequestMerge,
Expand Down Expand Up @@ -1245,6 +1246,51 @@ 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);
});

const makeStorage = () => {
const held = new Map<string, string>();
return {
Expand Down
14 changes: 14 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestDetail.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
type PullRequestActor,
type PullRequestBaseComparison,
type PullRequestCheck,
type PullRequestChecks,
type PullRequestChecksState as ContractPullRequestChecksState,
type PullRequestComment,
type PullRequestCommit,
Expand Down Expand Up @@ -1419,3 +1420,16 @@ 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: checks.state, checks: checks.checks } : detail;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
35 changes: 31 additions & 4 deletions apps/web/src/components/pullRequest/usePullRequestDetail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -18,6 +20,7 @@ import {
import {
readPullRequestDetailSnapshot,
resolveDisplayedPullRequestDetail,
resolvePullRequestDetailChecks,
writePullRequestDetailSnapshot,
} from "./pullRequestDetail.logic";
import { pullRequestChecksState } from "./pullRequestPresentation";
Expand All @@ -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,
Expand Down Expand Up @@ -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,
});
Expand Down
29 changes: 29 additions & 0 deletions packages/client-runtime/src/state/pullRequests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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* () {
Expand Down
55 changes: 31 additions & 24 deletions packages/client-runtime/src/state/pullRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,26 @@ export class EnvironmentHttpConnectionNotReadyError extends Data.TaggedError(

const LINKED_PULL_REQUEST_IDLE_TTL_MS = 5_000;

function normalizePullRequestQuery<A>(
query: (target: { readonly environmentId: EnvironmentId; readonly input: PullRequestRef }) => A,
) {
return ({
environmentId,
input: { projectId, host, repository, number, allowStale, expectedAccountId },
}: Parameters<typeof query>[0]) =>
query({
environmentId,
input: {
projectId,
...(host === undefined ? {} : { host }),
repository,
number,
...(allowStale === undefined ? {} : { allowStale }),
...(expectedAccountId === undefined ? {} : { expectedAccountId }),
},
});
}

function writableQueryFamily<A, E>(
family: (target: {
readonly environmentId: EnvironmentId;
Expand All @@ -77,22 +97,7 @@ function writableQueryFamily<A, E>(
(refresh) => refresh(source),
).pipe(Atom.setIdleTTL(5 * 60_000)),
);
return ({
environmentId,
input: { projectId, host, repository, number, allowStale },
}: Parameters<typeof family>[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. */
Expand Down Expand Up @@ -319,14 +324,16 @@ export function createPullRequestEnvironmentAtoms<R, E>(
}),
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",
Expand Down
1 change: 1 addition & 0 deletions packages/contracts/src/pullRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ export interface PullRequestProviderApi {

readonly getChangeRequestChecks?: (
input: ProviderRepositoryRef & { readonly number: number },
) => Effect.Effect<PullRequestChecks, PullRequestProviderError>;
) => Effect.Effect<Pick<PullRequestChecks, "state" | "checks">, PullRequestProviderError>;

readonly getChangeRequest: (
input: ProviderRepositoryRef & { readonly number: number },
Expand Down
Loading