diff --git a/apps/web/src/components/pullRequest/PullRequestLabelPicker.tsx b/apps/web/src/components/pullRequest/PullRequestLabelPicker.tsx index f1e5fc6a1da4..fe9062f83e55 100644 --- a/apps/web/src/components/pullRequest/PullRequestLabelPicker.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLabelPicker.tsx @@ -33,15 +33,12 @@ export function PullRequestLabelPicker({ environmentId, reference, allowed, - onChanged, }: { environmentId: EnvironmentId; reference: PullRequestRef; /** False where the host would refuse this account's change. Disabled with the reason rather * than hidden, like the reviewer control beside it. */ allowed: boolean; - /** The detail carries the labels, so it is re-read once the host has taken the change. */ - onChanged: () => void; }) { const [open, setOpen] = useState(false); const [query, setQuery] = useState(""); @@ -79,8 +76,6 @@ export function PullRequestLabelPicker({ }); return; } - onChanged(); - candidatesQuery.refresh(); }; return ( @@ -94,8 +89,8 @@ export function PullRequestLabelPicker({ query={query} onQueryChange={setQuery} searchLabel="Search labels" - isPending={candidatesQuery.isPending} - error={candidatesQuery.error} + isPending={candidatesQuery.isPending && candidatesQuery.data === null} + error={candidatesQuery.data === null ? candidatesQuery.error : null} candidates={candidates} emptyLabel="This repository has no labels." noMatchLabel="No label matches that." diff --git a/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx b/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx index a4da0d99514d..ac4c83ad714e 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewerPicker.tsx @@ -38,15 +38,12 @@ export function PullRequestReviewerPicker({ environmentId, reference, allowed, - onRequested, }: { environmentId: EnvironmentId; reference: PullRequestRef; /** False where the host would refuse this account's request, which is worth saying rather than * hiding: the control disabled with a reason answers the question its absence would raise. */ allowed: boolean; - /** The detail carries who is requested, so it is re-read once the host has taken the change. */ - onRequested: () => void; }) { const [open, setOpen] = useState(false); const [query, setQuery] = useState(""); @@ -96,8 +93,6 @@ export function PullRequestReviewerPicker({ ? `Review request to ${candidate.login} taken back` : `Review requested from ${candidate.login}`, }); - onRequested(); - candidatesQuery.refresh(); }; return ( @@ -111,8 +106,8 @@ export function PullRequestReviewerPicker({ query={query} onQueryChange={setQuery} searchLabel="Search people with access" - isPending={candidatesQuery.isPending} - error={candidatesQuery.error} + isPending={candidatesQuery.isPending && candidatesQuery.data === null} + error={candidatesQuery.data === null ? candidatesQuery.error : null} candidates={candidates} emptyLabel="Nobody else has access to this repository." noMatchLabel="Nobody with access matches that." diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index fed932d457a5..9501d61a67e2 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -683,7 +683,6 @@ export function PullRequestSummaryTab({ environmentId={environmentId} reference={reference} allowed={detail.viewerPermissions.requestReviewers} - onRequested={onRefresh} /> ) : null} @@ -718,7 +717,6 @@ export function PullRequestSummaryTab({ environmentId={environmentId} reference={reference} allowed={detail.viewerPermissions.labels !== false} - onChanged={onRefresh} /> ) : null} diff --git a/packages/client-runtime/src/state/pullRequests.test.ts b/packages/client-runtime/src/state/pullRequests.test.ts index fdd109d6e8c4..6187cf2e7bd0 100644 --- a/packages/client-runtime/src/state/pullRequests.test.ts +++ b/packages/client-runtime/src/state/pullRequests.test.ts @@ -1,5 +1,6 @@ import { EnvironmentId, ProjectId, WS_METHODS, type PullRequestStack } from "@t3tools/contracts"; import { expect, it } from "@effect/vitest"; +import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; import * as Latch from "effect/Latch"; import * as Layer from "effect/Layer"; @@ -26,6 +27,8 @@ import { import { PullRequestDiffLoader } from "./pullRequestDiffHttp.ts"; import { executeAtomQuery } from "./runtime.ts"; +class MutationRefused extends Data.TaggedError("MutationRefused") {} + const TARGET = new PrimaryConnectionTarget({ environmentId: EnvironmentId.make("environment-1"), label: "Test environment", @@ -216,6 +219,245 @@ it.effect("refreshes pull request activity after a comment is updated", () => ), ); +it.effect("updates cached labels after successful edits without rereading the host", () => + Effect.scoped( + Effect.gen(function* () { + let detailReads = 0; + let candidateReads = 0; + let refuse = false; + let failDetail = false; + const existing = { name: "existing", color: "111111" }; + const addedLabel = { name: "new", color: "abcdef" }; + const detailRefreshStarted = yield* Latch.make(); + const releaseDetailRefresh = yield* Latch.make(); + const client = { + [WS_METHODS.pullRequestsSubscribeRefreshes]: () => Stream.never, + [WS_METHODS.pullRequestsDetail]: () => + Effect.gen(function* () { + detailReads++; + if (failDetail) { + yield* detailRefreshStarted.open; + yield* releaseDetailRefresh.await; + return yield* Effect.fail(new MutationRefused()); + } + return { title: "keep this title", labels: [existing] }; + }), + [WS_METHODS.pullRequestsLabelCandidates]: () => + Effect.sync(() => { + candidateReads++; + return { + candidates: [ + { ...existing, description: null, isApplied: true }, + { ...addedLabel, description: "description", isApplied: false }, + ], + truncated: false, + }; + }), + [WS_METHODS.pullRequestsSetLabels]: () => + refuse ? Effect.fail(new MutationRefused()) : Effect.void, + } as unknown as WsRpcProtocolClient; + const { atoms, registry } = yield* makeTestRuntime(client); + const target = { + environmentId: TARGET.environmentId, + input: { + projectId: ProjectId.make("project-1"), + repository: "acme/web", + number: 1, + host: "github.example.com", + }, + }; + const detail = atoms.detail(target); + const candidates = atoms.labelCandidates(target); + registry.mount(detail); + const unmountCandidates = registry.mount(candidates); + yield* AtomRegistry.getResult(registry, detail, { suspendOnWaiting: true }); + yield* AtomRegistry.getResult(registry, candidates, { suspendOnWaiting: true }); + + const added = yield* Effect.promise(() => + atoms.setLabels.run(registry, { + ...target, + input: { + host: target.input.host, + projectId: target.input.projectId, + repository: target.input.repository, + number: target.input.number, + labels: ["new"], + applied: true, + }, + }), + ); + expect(AsyncResult.isSuccess(added)).toBe(true); + expect(yield* AtomRegistry.getResult(registry, detail)).toEqual({ + title: "keep this title", + labels: [existing, addedLabel], + }); + unmountCandidates(); + registry.mount(atoms.labelCandidates(target)); + expect((yield* AtomRegistry.getResult(registry, candidates)).candidates[1]).toEqual({ + ...addedLabel, + description: "description", + isApplied: true, + }); + + for (const name of ["existing", "new"]) { + refuse = name === "new"; + const result = yield* Effect.promise(() => + atoms.setLabels.run(registry, { + ...target, + input: { ...target.input, labels: [name], applied: false }, + }), + ); + expect(result._tag).toBe(refuse ? "Failure" : "Success"); + expect((yield* AtomRegistry.getResult(registry, detail)).labels).toEqual([addedLabel]); + expect((yield* AtomRegistry.getResult(registry, candidates)).candidates).toMatchObject([ + { name: "existing", isApplied: false }, + { name: "new", isApplied: true }, + ]); + } + expect(detailReads).toBe(1); + expect(candidateReads).toBe(1); + + failDetail = true; + registry.refresh(detail); + yield* detailRefreshStarted.await; + expect(registry.get(detail).waiting).toBe(true); + expect(Option.getOrThrow(AsyncResult.value(registry.get(detail))).labels).toEqual([ + addedLabel, + ]); + yield* releaseDetailRefresh.open; + yield* Effect.exit(AtomRegistry.getResult(registry, detail, { suspendOnWaiting: true })); + expect(AsyncResult.isFailure(registry.get(detail))).toBe(true); + expect(Option.getOrThrow(AsyncResult.value(registry.get(detail))).labels).toEqual([ + addedLabel, + ]); + failDetail = false; + registry.refresh(detail); + expect( + (yield* AtomRegistry.getResult(registry, detail, { suspendOnWaiting: true })).labels, + ).toEqual([existing]); + expect(detailReads).toBe(3); + }), + ), +); + +it.effect("updates reviewer requests and enriched reviewers without rereading the host", () => + Effect.scoped( + Effect.gen(function* () { + let reads = 0; + let refuse = false; + const actor = { login: "reviewer", name: "Reviewer", avatarUrl: null }; + const hostActor = { ...actor, login: "Reviewer" }; + let hostRequested = false; + let reviewed = false; + let pauseActivity = false; + const activityStarted = yield* Latch.make(); + const client = { + [WS_METHODS.pullRequestsSubscribeRefreshes]: () => Stream.never, + [WS_METHODS.pullRequestsDetail]: () => + Effect.sync(() => { + reads++; + return { reviewers: hostRequested ? [hostActor] : [] }; + }), + [WS_METHODS.pullRequestsActivity]: () => + Effect.gen(function* () { + reads++; + if (pauseActivity) { + pauseActivity = false; + yield* activityStarted.open; + return yield* Effect.never; + } + return { + reviewers: hostRequested ? [hostActor] : [], + comments: reviewed ? [{ kind: "review-comment", author: hostActor }] : [], + }; + }), + [WS_METHODS.pullRequestsReviewerCandidates]: (input: { number: number }) => + input.number === 2 + ? Effect.never + : Effect.sync(() => { + reads++; + return { + candidates: [{ ...actor, id: "12", kind: "user", isRequested: false }], + truncated: false, + }; + }), + [WS_METHODS.pullRequestsRequestReviewers]: (input: { requested: boolean }) => + refuse + ? Effect.fail(new MutationRefused()) + : Effect.sync(() => { + hostRequested = input.requested; + }), + } as unknown as WsRpcProtocolClient; + const { atoms, registry } = yield* makeTestRuntime(client); + const target = { + environmentId: TARGET.environmentId, + input: { + projectId: ProjectId.make("project-1"), + repository: "acme/web", + number: 1, + host: "github.example.com", + }, + }; + const detail = atoms.detail(target); + const activity = atoms.activity(target); + const candidates = atoms.reviewerCandidates(target); + registry.mount(detail); + registry.mount(activity); + registry.mount(candidates); + yield* AtomRegistry.getResult(registry, detail, { suspendOnWaiting: true }); + yield* AtomRegistry.getResult(registry, activity, { suspendOnWaiting: true }); + yield* AtomRegistry.getResult(registry, candidates, { suspendOnWaiting: true }); + const request = (requested: boolean, reference = target) => + Effect.promise(() => + atoms.requestReviewers.run(registry, { + ...reference, + input: { ...reference.input, reviewers: [{ id: "12", kind: "user" }], requested }, + }), + ); + for (const operation of ["request", "refuse", "remove"]) { + refuse = operation === "refuse"; + expect((yield* request(operation === "request"))._tag).toBe(refuse ? "Failure" : "Success"); + const expected = operation === "remove" ? [] : [actor]; + expect((yield* AtomRegistry.getResult(registry, detail)).reviewers).toEqual(expected); + expect((yield* AtomRegistry.getResult(registry, activity)).reviewers).toEqual(expected); + expect((yield* AtomRegistry.getResult(registry, candidates)).candidates).toMatchObject([ + { isRequested: operation !== "remove" }, + ]); + } + expect(reads).toBe(3); + // A slow activity read started before the write must not hide the new request. + pauseActivity = true; + registry.refresh(activity); + yield* activityStarted.await; + expect(AsyncResult.isSuccess(yield* request(true))).toBe(true); + expect( + (yield* AtomRegistry.getResult(registry, activity, { suspendOnWaiting: true })).reviewers, + ).toEqual([hostActor]); + expect(reads).toBe(5); + expect(AsyncResult.isSuccess(yield* request(false))).toBe(true); + expect((yield* AtomRegistry.getResult(registry, activity)).reviewers).toEqual([]); + + reviewed = true; + yield* request(true); + registry.refresh(activity); + yield* AtomRegistry.getResult(registry, activity, { suspendOnWaiting: true }); + yield* request(false); + expect((yield* AtomRegistry.getResult(registry, activity)).reviewers).toEqual([hostActor]); + + // A caller without an open picker still needs authoritative reviewer identities. + const otherTarget = { ...target, input: { ...target.input, number: 2 } }; + const otherDetail = atoms.detail(otherTarget); + registry.mount(otherDetail); + yield* AtomRegistry.getResult(registry, otherDetail, { suspendOnWaiting: true }); + yield* request(true, otherTarget); + expect( + (yield* AtomRegistry.getResult(registry, otherDetail, { suspendOnWaiting: true })) + .reviewers, + ).toEqual([hostActor]); + }), + ), +); + it.effect("refreshes stack state after reopening and head SHAs after a turn", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index c9d23d02bf55..26a0c08c13ed 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -1,7 +1,10 @@ import { WS_METHODS, + type EnvironmentId, + type PullRequestActor, type PullRequestDetail, type PullRequestDiffInput, + type PullRequestRef, type PullRequestSummary, type VcsStatusResult, } from "@t3tools/contracts"; @@ -9,7 +12,7 @@ import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; import * as Option from "effect/Option"; import * as SubscriptionRef from "effect/SubscriptionRef"; -import { Atom } from "effect/unstable/reactivity"; +import { AsyncResult, Atom, AtomRegistry } from "effect/unstable/reactivity"; import { createAtomCommandScheduler, @@ -36,6 +39,52 @@ export class EnvironmentHttpConnectionNotReadyError extends Data.TaggedError( const LINKED_PULL_REQUEST_IDLE_TTL_MS = 5_000; +/** Keep confirmed edits on the same cached reference regardless of input property order. */ +function writableQueryFamily( + family: (target: { + readonly environmentId: EnvironmentId; + readonly input: PullRequestRef; + }) => Atom.Atom>, +) { + const writable = Atom.family((source: Atom.Atom>) => + Atom.writable( + (get) => { + const result = get(source); + if (result._tag === "Success" && !result.waiting) return result; + const previous = get.self>(); + const value = Option.flatMap(previous, AsyncResult.value); + if (Option.isNone(value)) return result; + return result._tag === "Failure" + ? AsyncResult.failureWithPrevious(result.cause, { previous, waiting: result.waiting }) + : AsyncResult.success(value.value, result); + }, + (context, value: AsyncResult.AsyncResult) => context.setSelf(value), + (refresh) => refresh(source), + ).pipe(Atom.setIdleTTL(5 * 60_000)), + ); + return ({ + environmentId, + input: { projectId, host, repository, number }, + }: Parameters[0]) => + writable( + family({ + environmentId, + input: { projectId, ...(host === undefined ? {} : { host }), repository, number }, + }), + ); +} + +/** Restart pre-mutation reads before patching so they cannot restore stale values. */ +function updateCached( + registry: AtomRegistry.AtomRegistry, + atom: Atom.Writable>, + update: (value: A) => A, + refresh = false, +) { + if (refresh || registry.get(atom).waiting) registry.refresh(atom); + registry.update(atom, AsyncResult.map(update)); +} + function createPullRequestRefreshAtomFamily( runtime: Atom.AtomRuntime, ) { @@ -92,7 +141,7 @@ export function pullRequestDetailToVcsStatus( /** * Reopening a PR within a minute reuses detail and activity. Explicit refreshes and * turn notifications still revalidate. Mutations run serially per environment: actions on the same - * pull request are order-sensitive, and the detail view refetches after each one. + * pull request are order-sensitive. Confirmed label and reviewer edits update cached state. */ export function createPullRequestEnvironmentAtoms( runtime: Atom.AtomRuntime, @@ -103,12 +152,36 @@ export function createPullRequestEnvironmentAtoms( mode: "serial", key: ({ environmentId }: { readonly environmentId: string }) => environmentId, } as const; - const activity = createEnvironmentRpcQueryAtomFamily(runtime, { - label: "environment-data:pull-requests:activity", - tag: WS_METHODS.pullRequestsActivity, - staleTimeMs: 60_000, - refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), - }); + const activity = writableQueryFamily( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:activity", + tag: WS_METHODS.pullRequestsActivity, + staleTimeMs: 60_000, + refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + }), + ); + const detail = writableQueryFamily( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:detail", + tag: WS_METHODS.pullRequestsDetail, + staleTimeMs: 60_000, + refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + }), + ); + const labelCandidates = writableQueryFamily( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:label-candidates", + tag: WS_METHODS.pullRequestsLabelCandidates, + staleTimeMs: 60_000, + }), + ); + const reviewerCandidates = writableQueryFamily( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:reviewer-candidates", + tag: WS_METHODS.pullRequestsReviewerCandidates, + staleTimeMs: 60_000, + }), + ); return { refreshes, linkedThreads: createEnvironmentRpcQueryAtomFamily(runtime, { @@ -137,12 +210,7 @@ export function createPullRequestEnvironmentAtoms( staleTimeMs: 60_000, refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), }), - detail: createEnvironmentRpcQueryAtomFamily(runtime, { - label: "environment-data:pull-requests:detail", - tag: WS_METHODS.pullRequestsDetail, - staleTimeMs: 60_000, - refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), - }), + detail, activity, threadComments: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:thread-comments", @@ -241,28 +309,117 @@ export function createPullRequestEnvironmentAtoms( * for a minute, because who has access to a repository changes far more slowly than the * change request it is being read for. */ - reviewerCandidates: createEnvironmentRpcQueryAtomFamily(runtime, { - label: "environment-data:pull-requests:reviewer-candidates", - tag: WS_METHODS.pullRequestsReviewerCandidates, - staleTimeMs: 60_000, - }), + reviewerCandidates, requestReviewers: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:request-reviewers", tag: WS_METHODS.pullRequestsRequestReviewers, scheduler: commandScheduler, concurrency: serialPerEnvironment, + onSuccess: (target, registry) => + Effect.sync(() => { + const { reviewers, requested } = target.input; + const candidatesAtom = reviewerCandidates(target); + const candidates = Option.getOrNull(AsyncResult.value(registry.get(candidatesAtom))); + const selected = + candidates?.candidates.filter((candidate) => + reviewers.some( + (reviewer) => reviewer.id === candidate.id && reviewer.kind === candidate.kind, + ), + ) ?? []; + const missingIdentities = selected.length < reviewers.length; + updateCached(registry, candidatesAtom, (value) => ({ + ...value, + candidates: value.candidates.map((candidate) => + selected.includes(candidate) ? { ...candidate, isRequested: requested } : candidate, + ), + })); + const selectedLogins = new Set( + selected.map((candidate) => candidate.login.toLowerCase()), + ); + const updateReviewers = ( + actors: ReadonlyArray, + keep = (_actor: PullRequestActor) => false, + ) => + requested + ? [ + ...actors, + ...selected + .filter( + (candidate) => + !actors.some( + (actor) => actor.login.toLowerCase() === candidate.login.toLowerCase(), + ), + ) + .map(({ login, name, avatarUrl }) => ({ login, name, avatarUrl })), + ] + : actors.filter( + (actor) => !selectedLogins.has(actor.login.toLowerCase()) || keep(actor), + ); + updateCached( + registry, + detail(target), + (value) => ({ + ...value, + reviewers: updateReviewers(value.reviewers), + }), + missingIdentities, + ); + updateCached( + registry, + activity(target), + (value) => ({ + ...value, + reviewers: + value.reviewers === undefined + ? undefined + : updateReviewers(value.reviewers, (actor) => + value.comments.some( + (comment) => + (comment.kind === "review" || comment.kind === "review-comment") && + comment.author?.login.toLowerCase() === actor.login.toLowerCase(), + ), + ), + }), + missingIdentities, + ); + }), }), /** Read when the label menu opens, and kept for a minute, like the reviewer candidates. */ - labelCandidates: createEnvironmentRpcQueryAtomFamily(runtime, { - label: "environment-data:pull-requests:label-candidates", - tag: WS_METHODS.pullRequestsLabelCandidates, - staleTimeMs: 60_000, - }), + labelCandidates, setLabels: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:set-labels", tag: WS_METHODS.pullRequestsSetLabels, scheduler: commandScheduler, concurrency: serialPerEnvironment, + onSuccess: (target, registry) => + Effect.sync(() => { + const { labels, applied } = target.input; + const candidatesAtom = labelCandidates(target); + const candidates = Option.getOrNull(AsyncResult.value(registry.get(candidatesAtom))); + const names = new Set(labels); + updateCached(registry, candidatesAtom, (value) => ({ + ...value, + candidates: value.candidates.map((candidate) => + names.has(candidate.name) ? { ...candidate, isApplied: applied } : candidate, + ), + })); + updateCached(registry, detail(target), (value) => ({ + ...value, + labels: applied + ? [ + ...value.labels, + ...labels + .filter((name) => !value.labels.some((label) => label.name === name)) + .map((name) => ({ + name, + color: + candidates?.candidates.find((candidate) => candidate.name === name) + ?.color ?? null, + })), + ] + : value.labels.filter((label) => !names.has(label.name)), + })); + }), }), setThreadResolution: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:set-thread-resolution",