From a8749d7d263b83b708b40cd80e504464ad03ecd5 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:39:46 +0100 Subject: [PATCH 01/13] feat(pr): add GitHub maintainer edit control --- .../PullRequestMaintainerEdits.tsx | 71 ++++++++++++++ .../pull-requests/PullRequestRouteScreen.tsx | 10 ++ .../pullRequest/PullRequestService.test.ts | 62 ++++++++++++ .../src/pullRequest/PullRequestService.ts | 23 ++++- .../pullRequest/PullRequestDetailPanel.tsx | 1 + ...ullRequestMaintainerEdits.browser.test.tsx | 94 +++++++++++++++++++ .../PullRequestMaintainerEdits.tsx | 82 ++++++++++++++++ .../pullRequest/PullRequestSummaryTab.tsx | 15 +++ packages/contracts/src/pullRequest.ts | 4 + .../src/server/PullRequestProvider.ts | 2 + .../src/server/GitHubPullRequestApi.test.ts | 22 +++++ .../src/server/GitHubPullRequestApi.ts | 4 + .../server/GitHubPullRequestProvider.test.ts | 3 + .../src/server/GitHubPullRequestProvider.ts | 16 +++- .../src/server/gitHubPullRequestJson.test.ts | 23 +++++ .../src/server/gitHubPullRequestJson.ts | 10 ++ .../src/server/pullRequestMutations.graphql | 16 +++- .../server/pullRequestMutations.graphql.ts | 8 +- .../src/server/pullRequestReads.graphql | 1 + .../src/server/pullRequestReads.graphql.ts | 4 +- 20 files changed, 459 insertions(+), 12 deletions(-) create mode 100644 apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx create mode 100644 apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx create mode 100644 apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx diff --git a/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx b/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx new file mode 100644 index 0000000000000..6a39cf25a2fb5 --- /dev/null +++ b/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx @@ -0,0 +1,71 @@ +import { useAtomRefresh } from "@effect/atom-react"; +import type { PullRequestDetail } from "@t3tools/contracts"; +import { AsyncResult } from "effect/reactivity"; +import { useRef, useState } from "react"; +import { View } from "react-native"; + +import { AppText } from "../../components/ui/AppText"; +import { ListToggleRow } from "../../components/ui/ListToggleRow"; +import { composerPullRequests } from "../../state/pull-requests"; +import { useAtomCommand } from "../../state/use-atom-command"; +import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff"; +import { usePullRequestPermissions } from "./usePullRequestPermissions"; + +export function PullRequestMaintainerEdits({ + params, + pr, +}: { + params: PullRequestParams; + pr: PullRequestDetail; +}) { + const target = pullRequestTarget(params); + const permissions = usePullRequestPermissions(params, pr); + const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); + const refresh = useAtomRefresh(composerPullRequests.detail(target)); + const [value, setValue] = useState(pr.maintainerCanModify ?? false); + const [saving, setSaving] = useState(false); + const [error, setError] = useState(false); + const inFlight = useRef(false); + const save = async (next: boolean) => { + if (!permissions.canEditChangeRequest || inFlight.current) return; + const previous = value; + inFlight.current = true; + setSaving(true); + setError(false); + setValue(next); + const result = await update({ + ...target, + input: { ...target.input, maintainerCanModify: next }, + }); + inFlight.current = false; + setSaving(false); + if (AsyncResult.isFailure(result)) { + setValue(previous); + setError(true); + return; + } + refresh(); + }; + + return ( + + void save(next)} + /> + {saving ? ( + + Saving… + + ) : null} + {error ? ( + + Could not save maintainer access. Try again. + + ) : null} + + ); +} diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 164b1dc965c7f..8bde4ddea45b3 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -110,6 +110,7 @@ import { PullRequestReadStatus, } from "./PullRequestPresentation"; import { usePullRequestFilesSection } from "./PullRequestFiles"; +import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; import { PullRequestDescription } from "./PullRequestDescription"; import { PullRequestMediaVisibleContext } from "./PullRequestMedia"; import { PullRequestMarkdownRepository } from "./PullRequestMarkdown"; @@ -325,6 +326,15 @@ function PullRequestOverview(props: { footer={} /> + {pr.capabilities.edit?.maintainerEdits === true && + pr.viewerPermissions.maintainerEdits === true && + pr.maintainerCanModify !== undefined ? ( + + ) : null} ); } diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 4b2d67738c20c..5188145720409 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -116,6 +116,68 @@ function hostedChangeRequest(body: string, additions = 1) { }; } +it.effect("reads and changes maintainer edits through the existing update command", () => + Effect.gen(function* () { + let enabled = true; + const provider = fakePullRequestProvider(SourceControlProviderKind.make("github"), { + getChangeRequest: () => + Effect.succeed({ ...hostedChangeRequest("Body"), maintainerCanModify: enabled }), + updateChangeRequest: (input) => + Effect.sync(() => { + assert.strictEqual(input.title, undefined); + assert.strictEqual(input.body, undefined); + enabled = input.maintainerCanModify!; + }), + }); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + { + ...provider, + capabilities: { + ...provider.capabilities, + edit: { changeRequest: true, comment: true, maintainerEdits: true }, + }, + }, + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + assert.strictEqual((yield* service.detail(ref)).maintainerCanModify, true); + for (const value of [false, true]) { + yield* service.update({ ...ref, maintainerCanModify: value }); + assert.strictEqual((yield* service.detail(ref)).maintainerCanModify, value); + } + }), +); + +it.effect("refuses maintainer edits on hosts that do not support them", () => + Effect.gen(function* () { + let writes = 0; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakePullRequestProvider(SourceControlProviderKind.make("github"), { + updateChangeRequest: () => + Effect.sync(() => { + writes += 1; + }), + }), + ], + }); + const result = yield* service + .update({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + body: "Body", + maintainerCanModify: false, + }) + .pipe(Effect.flip); + assert.strictEqual(result._tag, "PullRequestOperationError"); + assert.strictEqual(writes, 0); + }), +); + it.effect("does not wait for an in-flight detail read to display a preview", () => Effect.gen(function* () { const detailStarted = yield* Deferred.make(); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 6a9c721e3a127..6f272ba5f8154 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -1753,6 +1753,9 @@ export const make = Effect.gen(function* () { ...(changeRequest.autoMergeMethod === undefined ? {} : { autoMergeMethod: changeRequest.autoMergeMethod }), + ...(changeRequest.maintainerCanModify === undefined + ? {} + : { maintainerCanModify: changeRequest.maintainerCanModify }), ...(changeRequest.workflowApprovalsRequired === undefined ? {} : { workflowApprovalsRequired: changeRequest.workflowApprovalsRequired }), @@ -2134,7 +2137,22 @@ export const make = Effect.gen(function* () { }), ); } - if (input.title === undefined && input.body === undefined) { + if ( + input.maintainerCanModify !== undefined && + project.api.capabilities.edit?.maintainerEdits !== true + ) { + return Effect.fail( + new PullRequestOperationError({ + operation: "update", + detail: "This host does not support maintainer edits.", + }), + ); + } + if ( + input.title === undefined && + input.body === undefined && + input.maintainerCanModify === undefined + ) { return Effect.fail( new PullRequestOperationError({ operation: "update", @@ -2147,6 +2165,9 @@ export const make = Effect.gen(function* () { repository: project.repository, host: project.host, number: input.number, + ...(input.maintainerCanModify === undefined + ? {} + : { maintainerCanModify: input.maintainerCanModify }), ...(input.title === undefined ? {} : { title: input.title }), ...(input.body === undefined ? {} : { body: input.body }), }).pipe(Effect.mapError(toPullRequestError("update"))); diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index b9e1633efce11..9f77a2059a47c 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -1736,6 +1736,7 @@ export function PullRequestDetailPanel({ detail={detail} activityPending={activityPending} checksStale={checksStale} + canWriteSourceControl={canWriteSourceControl && environmentConnected} activityError={activityError} pendingFinding={handoff} fixFindingLabel={handoffLabels.fixFinding} diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx new file mode 100644 index 0000000000000..9079145e96174 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx @@ -0,0 +1,94 @@ +import "~/index.css"; +import { EnvironmentId, ProjectId } from "@t3tools/contracts"; +import { expect, it, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + update: vi.fn<(input: unknown) => Promise<{ _tag: "Success" | "Failure" }>>(), +})); +vi.mock( + import("~/state/pullRequests"), + () => ({ pullRequestEnvironment: { update: {} } }) as never, +); +vi.mock( + import("~/state/use-atom-command"), + () => ({ useAtomCommand: () => mocks.update }) as never, +); + +import { render } from "../../../test/render"; +import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; + +const reference = { projectId: ProjectId.make("project"), repository: "owner/repo", number: 7 }; +const environmentId = EnvironmentId.make("environment"); + +function setup() { + mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); + const onSaved = vi.fn(); + const control = (allowed = true) => ( + + ); + return { control, onSaved }; +} + +it("saves both directions without waiting for a refreshed detail", async () => { + const { control, onSaved } = setup(); + await using screen = await render(control()); + const checkbox = screen.getByRole("checkbox", { name: "Allow edits from maintainers" }); + await checkbox.click(); + await expect.element(checkbox).not.toBeChecked(); + await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); + await checkbox.click(); + await expect.element(checkbox).toBeChecked(); + expect(mocks.update.mock.calls.map(([input]) => input)).toEqual([ + { environmentId, input: { ...reference, maintainerCanModify: false } }, + { environmentId, input: { ...reference, maintainerCanModify: true } }, + ]); +}); + +it("locks the control while a save is in flight", async () => { + const { control } = setup(); + let settle!: (value: { _tag: "Success" }) => void; + const pending = new Promise<{ _tag: "Success" }>((resolve) => { + settle = resolve; + }); + mocks.update.mockReturnValue(pending); + await using screen = await render(control()); + const checkbox = screen.getByRole("checkbox"); + await checkbox.click(); + await expect.element(checkbox).toBeDisabled(); + await expect.element(screen.getByRole("status")).toHaveTextContent("Saving…"); + expect(mocks.update).toHaveBeenCalledTimes(1); + settle({ _tag: "Success" }); + await expect.element(checkbox).toBeEnabled(); +}); + +it("restores the last saved value after a failed save and lets the user retry", async () => { + const { control, onSaved } = setup(); + await using screen = await render(control()); + const checkbox = screen.getByRole("checkbox"); + await checkbox.click(); + await expect.element(checkbox).not.toBeChecked(); + await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); + mocks.update.mockResolvedValueOnce({ _tag: "Failure" }); + await checkbox.click(); + await expect + .element(screen.getByRole("alert")) + .toHaveTextContent("Could not save maintainer access. Try again."); + await expect.element(checkbox).not.toBeChecked(); + expect(onSaved).toHaveBeenCalledTimes(1); + await checkbox.click(); + await expect.element(checkbox).toBeChecked(); + await expect.element(screen.getByRole("alert")).not.toBeInTheDocument(); +}); + +it("blocks a session without source control write access", async () => { + const { control } = setup(); + await using screen = await render(control(false)); + await expect.element(screen.getByRole("checkbox")).toBeDisabled(); + expect(mocks.update).not.toHaveBeenCalled(); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx new file mode 100644 index 0000000000000..5e9012f6a672f --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx @@ -0,0 +1,82 @@ +import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; +import { useId, useRef, useState } from "react"; + +import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useAtomCommand } from "~/state/use-atom-command"; +import { Checkbox } from "../ui/checkbox"; + +export function PullRequestMaintainerEdits({ + environmentId, + reference, + enabled, + allowed, + onSaved, +}: { + environmentId: EnvironmentId; + reference: PullRequestRef; + enabled: boolean; + allowed: boolean; + onSaved: () => void; +}) { + const id = useId(); + const update = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false }); + const [value, setValue] = useState(enabled); + const [saving, setSaving] = useState(false); + const [error, setError] = useState(false); + const inFlight = useRef(false); + const save = async (next: boolean) => { + if (!allowed || inFlight.current) return; + const previous = value; + inFlight.current = true; + setSaving(true); + setError(false); + setValue(next); + const result = await update({ + environmentId, + input: { ...reference, maintainerCanModify: next }, + }); + inFlight.current = false; + setSaving(false); + if (result._tag === "Failure") { + setValue(previous); + setError(true); + return; + } + onSaved(); + }; + + return ( +
+
+ + void save(next)} + /> + +
+ + + Maintainers can push to this branch. If your fork has workflows, this can also give + access to its secrets. + +
+
+ {saving ? ( +

+ Saving… +

+ ) : null} + {error ? ( +

+ Could not save maintainer access. Try again. +

+ ) : null} +
+ ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index f8084aa550ad3..422ec2bafd456 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -31,6 +31,7 @@ import { RelativeTime } from "~/components/RelativeTime"; import { Button } from "../ui/button"; import { PullRequestEditButton } from "./PullRequestEditButton"; +import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { Menu, @@ -384,6 +385,7 @@ export function PullRequestSummaryTab({ detail, activityPending, checksStale = false, + canWriteSourceControl = false, activityError, pendingFinding, fixFindingLabel = "Fix in a thread", @@ -399,6 +401,7 @@ export function PullRequestSummaryTab({ detail: PullRequestDetailView; activityPending: boolean; checksStale?: boolean; + canWriteSourceControl?: boolean; activityError: string | null; pendingFinding?: string | null; fixFindingLabel?: string; @@ -789,6 +792,18 @@ export function PullRequestSummaryTab({ )} + {detail.capabilities.edit?.maintainerEdits === true && + detail.viewerPermissions.maintainerEdits === true && + detail.maintainerCanModify !== undefined ? ( + + ) : null}
diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index e38d5fa6cf95a..3b11209f46dae 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -242,6 +242,7 @@ export const PullRequestReviewCapabilities = Schema.Struct({ export type PullRequestReviewCapabilities = typeof PullRequestReviewCapabilities.Type; export const PullRequestEditCapabilities = Schema.Struct({ + maintainerEdits: Schema.optional(Schema.Boolean), changeRequest: Schema.Boolean, comment: Schema.Boolean, }); @@ -295,6 +296,7 @@ export const PullRequestViewerPermissions = Schema.Struct({ updateMethods: Schema.optional(ForwardCompatibleArray(PullRequestUpdateMethod)), labels: Schema.optional(Schema.Boolean), editChangeRequest: Schema.optional(Schema.Boolean), + maintainerEdits: Schema.optional(Schema.Boolean), }); export type PullRequestViewerPermissions = typeof PullRequestViewerPermissions.Type; @@ -589,6 +591,7 @@ export const PullRequestDetail = Schema.Struct({ autoMergeEnabled: Schema.optional(Schema.Boolean), autoMergeMethod: Schema.optional(PullRequestMergeMethod), workflowApprovalsRequired: Schema.optional(NonNegativeInt), + maintainerCanModify: Schema.optional(Schema.Boolean), }); export type PullRequestDetail = typeof PullRequestDetail.Type; @@ -743,6 +746,7 @@ export const PullRequestCommentInput = Schema.Struct({ export type PullRequestCommentInput = typeof PullRequestCommentInput.Type; export const PullRequestUpdateInput = Schema.Struct({ + maintainerCanModify: Schema.optional(Schema.Boolean), ...PullRequestRef.fields, title: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(1024))), body: Schema.optional(Schema.String.check(Schema.isMaxLength(65_536))), diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index ce1a72c1aedf6..fe203f055dd4c 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -182,6 +182,7 @@ export interface ProviderChangeRequestStat { } export interface ProviderChangeRequestDetail extends ProviderChangeRequest { + readonly maintainerCanModify?: boolean; readonly headSha?: string | null; readonly body: string; readonly changedFiles: number; @@ -461,6 +462,7 @@ export interface PullRequestProviderApi { /** Requires edit capability and at least one field. */ readonly updateChangeRequest?: ( input: ProviderRepositoryRef & { + readonly maintainerCanModify?: boolean | undefined; readonly number: number; readonly title?: string | undefined; readonly body?: string | undefined; diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 61005227a168c..d1264f3a2a8e1 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -60,6 +60,7 @@ const coreResponse = (pullRequest: Readonly> = {}) => ({ mergedAt: null, closedAt: null, isCrossRepository: false, + maintainerCanModify: true, headRepositoryOwner: null, headRepository: null, headRef: null, @@ -694,6 +695,27 @@ it.effect( ); layer("GitHubPullRequestApi.layer", (it) => { + it.effect.each([true, false])( + "sets maintainer edits to %s without rewriting PR text", + (enabled) => + Effect.gen(function* () { + mockedExecute.mockImplementation((call) => + Effect.succeed( + output( + encodeJson( + call.kind === "graphql" && matches(call, NODE_ID_QUERY) ? nodeIdAnswer("PR_7") : {}, + ), + ), + ), + ); + const api = yield* GitHubPullRequestApi.GitHubPullRequestApi; + yield* api.updatePullRequest({ ...pullRequestInput, maintainerCanModify: enabled }); + expect(variablesOf("mutation UpdatePullRequest")).toEqual([ + { pullRequestId: "PR_7", maintainerCanModify: enabled }, + ]); + }), + ); + it.effect.each(["mixed", "merges", "unavailable"] as const)( "reads stats only for displayed non-merge commits (%s)", (kind) => diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index a35ff2e2d0a8a..68f9028901ab0 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -725,6 +725,7 @@ export class GitHubPullRequestApi extends Context.Service< }) => Effect.Effect; readonly updatePullRequest: (input: { + readonly maintainerCanModify?: boolean | undefined; readonly cwd: string; readonly repository: string; readonly host: string; @@ -2763,6 +2764,9 @@ export const make = Effect.gen(function* () { client .UpdatePullRequest({ pullRequestId, + ...(input.maintainerCanModify === undefined + ? {} + : { maintainerCanModify: input.maintainerCanModify }), ...(input.title === undefined ? {} : { title: input.title }), ...(input.body === undefined ? {} : { body: input.body }), }) diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index fe9bcaf82c405..39da39474a9ea 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -16,6 +16,8 @@ import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullReque import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts"; const coreFields = { + maintainerCanModify: true, + canEditMaintainerEdits: false, reviewRequestTeams: [], checksTruncated: false, comparison: { behindBy: 0, viewerCanUpdate: true }, @@ -205,6 +207,7 @@ describe("gitHubViewerPermissions", () => { }); expect(detail.viewerPermissions).toEqual({ + maintainerEdits: false, actions: ["ready", "draft", "close", "reopen"], comment: true, resolve: false, diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index 0116657c641ea..deb686d69c7a0 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -49,7 +49,7 @@ const CAPABILITIES: PullRequestCapabilities = { verdicts: ["comment", "approve", "request-changes"], }, reviewers: { request: true, listCandidates: true }, - edit: { changeRequest: true, comment: true }, + edit: { changeRequest: true, comment: true, maintainerEdits: true }, stacks: true, stackActions: true, labels: true, @@ -465,10 +465,13 @@ export const make = Effect.gen(function* () { ...pullRequest.reviewRequestTeams, ], mergeCapabilities: pullRequest.viewerAccess.mergeCapabilities, - viewerPermissions: gitHubViewerPermissions({ - ...pullRequest.viewerAccess, - canUpdateBranch: pullRequest.comparison?.viewerCanUpdate === true, - }), + viewerPermissions: { + ...gitHubViewerPermissions({ + ...pullRequest.viewerAccess, + canUpdateBranch: pullRequest.comparison?.viewerCanUpdate === true, + }), + maintainerEdits: pullRequest.canEditMaintainerEdits, + }, baseComparison: pullRequest.comparison === null || pullRequest.comparison.behindBy === null ? "unknown" @@ -678,6 +681,9 @@ export const make = Effect.gen(function* () { updateChangeRequest: (input) => cli .updatePullRequest({ + ...(input.maintainerCanModify === undefined + ? {} + : { maintainerCanModify: input.maintainerCanModify }), cwd: input.cwd, repository: input.repository, host: input.host, diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index b217d490143ee..4585dc79eec13 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -363,6 +363,7 @@ describe("pull request detail decoding", () => { baseRefName: "main", headRefOid: "abc123", isCrossRepository: false, + maintainerCanModify: true, headRepositoryOwner: null, headRepository: null, headRef: null, @@ -395,6 +396,28 @@ describe("pull request detail decoding", () => { pullRequest?: Record, ) => expectSuccess(decodePullRequestCoreJson(coreJson(rollup, pullRequest))); + it.each([ + [{}, true], + [{ viewerDidAuthor: false }, false], + [{ isCrossRepository: false }, false], + [{ state: "CLOSED" }, false], + [{ headRepositoryOwner: { __typename: "Organization", login: "team" } }, false], + [{ headRepository: null }, false], + [{ headRef: null }, false], + ])("limits maintainer edits to the author's open personal fork: %j", (overrides, allowed) => { + const detail = detailOf([], { + maintainerCanModify: false, + isCrossRepository: true, + viewerDidAuthor: true, + headRepositoryOwner: { __typename: "User", login: "octocat" }, + headRepository: { nameWithOwner: "octocat/t3code", viewerPermission: "ADMIN" }, + headRef: { id: "REF_7" }, + ...overrides, + }); + expect(detail.maintainerCanModify).toBe(false); + expect(detail.canEditMaintainerEdits).toBe(allowed); + }); + const detailJson = JSON.stringify({ comments: [{ id: "c1", body: "second", createdAt: "2026-07-04T00:00:00Z" }], reviews: [ diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index cf4def54bbd03..42e7d41166039 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -1008,6 +1008,8 @@ export function toPullRequestWatchFingerprint( } export interface GitHubPullRequestCore extends GitHubPullRequestDetail { + readonly maintainerCanModify: boolean; + readonly canEditMaintainerEdits: boolean; readonly reviewRequestTeams: ReadonlyArray; readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess; readonly comparison: GitHubBaseComparison | null; @@ -1055,6 +1057,14 @@ export function toPullRequestCore(result: PullRequestCore.Result): GitHubPullReq statusCheckRollup: toCheckContexts(nodesOf(contexts)), }), headRepositoryNameWithOwner: trimmed(pr.headRepository?.nameWithOwner), + maintainerCanModify: pr.maintainerCanModify, + canEditMaintainerEdits: + pr.state === "OPEN" && + pr.isCrossRepository && + pr.viewerDidAuthor && + pr.headRepositoryOwner?.__typename === "User" && + pr.headRepository !== null && + pr.headRef !== null, viewerAccess: { canWrite: toCanWrite(repository.viewerPermission), canDeleteBranch: toCanWrite(pr.headRepository?.viewerPermission) && pr.headRef !== null, diff --git a/packages/source-control-github/src/server/pullRequestMutations.graphql b/packages/source-control-github/src/server/pullRequestMutations.graphql index 4ba7854507af7..9ec83192557c0 100644 --- a/packages/source-control-github/src/server/pullRequestMutations.graphql +++ b/packages/source-control-github/src/server/pullRequestMutations.graphql @@ -44,8 +44,20 @@ mutation UnresolveReviewThread($threadId: ID!) { # Rewrites the pull request's own words. Both are nullable so that one document serves a change to # the title, to the description, or to the two together: a variable the request does not send puts # no entry in the input at all, which leaves that field as it was rather than clearing it. -mutation UpdatePullRequest($pullRequestId: ID!, $title: String, $body: String) { - updatePullRequest(input: { pullRequestId: $pullRequestId, title: $title, body: $body }) { +mutation UpdatePullRequest( + $pullRequestId: ID! + $title: String + $body: String + $maintainerCanModify: Boolean +) { + updatePullRequest( + input: { + pullRequestId: $pullRequestId + title: $title + body: $body + maintainerCanModify: $maintainerCanModify + } + ) { pullRequest { id } diff --git a/packages/source-control-github/src/server/pullRequestMutations.graphql.ts b/packages/source-control-github/src/server/pullRequestMutations.graphql.ts index 05a67b81d4e5c..d11b2b1e69fcf 100644 --- a/packages/source-control-github/src/server/pullRequestMutations.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestMutations.graphql.ts @@ -205,17 +205,19 @@ export const UpdatePullRequest = GraphQL.mutation("UpdatePullRequest", { variables: [ { name: "pullRequestId", body: "ID!" }, { name: "title", body: "String" }, - { name: "body", body: "String" } + { name: "body", body: "String" }, + { name: "maintainerCanModify", body: "Boolean" } ], selections: [ - { name: "updatePullRequest", body: ["(input:{pullRequestId:$", "pullRequestId", " title:$", "title", " body:$", "body", "}){pullRequest{id}}"] } + { name: "updatePullRequest", body: ["(input:{pullRequestId:$", "pullRequestId", " title:$", "title", " body:$", "body", " maintainerCanModify:$", "maintainerCanModify", "}){pullRequest{id}}"] } ], fragments: [] }, variables: { pullRequestId: Shared.ID, title: Schema.optional(Schema.NullOr(Schema.String)), - body: Schema.optional(Schema.NullOr(Schema.String)) + body: Schema.optional(Schema.NullOr(Schema.String)), + maintainerCanModify: Schema.optional(Schema.NullOr(Schema.Boolean)) }, result: Schema.Struct({ /** Update a pull request */ diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index d5a95256fc1a3..78e5efe0e4558 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -87,6 +87,7 @@ query PullRequestCore( baseRefName headRefOid isCrossRepository + maintainerCanModify headRepositoryOwner { login } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index e261ec0240a73..5bc09fa4e2ce4 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -395,6 +395,8 @@ export const PullRequestCoreRepositoryPullRequestPullRequest = Schema.Struct({ headRefOid: Shared.GitObjectID, /** The head and base repositories are different. */ isCrossRepository: Schema.Boolean, + /** Indicates whether maintainers can modify the pull request. */ + maintainerCanModify: Schema.Boolean, /** The owner of the repository associated with this pull request's head Ref. */ headRepositoryOwner: Schema.NullOr(RepositoryOwner), /** The repository associated with this pull request's head Ref. */ @@ -457,7 +459,7 @@ export const PullRequestCore = GraphQL.query("PullRequestCore", { { name: "requestedReviewers", body: "Int!" } ], selections: [ - { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed autoMergeAllowed viewerPermission pullRequest(number:$", "number", "){number title url body state isDraft mergeable mergeStateStatus reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository headRepositoryOwner{__typename login}headRepository{nameWithOwner viewerPermission}headRef{id}author{__typename login avatarUrl...on User{id name}}autoMergeRequest{mergeMethod}viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch baseRef{compare(headRef:$", "headRef", "){behindBy}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name}...on Bot{login}...on Team{slug combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}labels(first:100){nodes{name color}}commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage}}}}}}}}"] } + { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed autoMergeAllowed viewerPermission pullRequest(number:$", "number", "){number title url body state isDraft mergeable mergeStateStatus reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository maintainerCanModify headRepositoryOwner{__typename login}headRepository{nameWithOwner viewerPermission}headRef{id}author{__typename login avatarUrl...on User{id name}}autoMergeRequest{mergeMethod}viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch baseRef{compare(headRef:$", "headRef", "){behindBy}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name}...on Bot{login}...on Team{slug combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}labels(first:100){nodes{name color}}commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage}}}}}}}}"] } ], fragments: [ { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{workflow{name}}}}}"] } From 1ea77382c5644d186b966d685400c9b1f12cd502 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Tue, 27 Oct 2026 04:29:58 +0100 Subject: [PATCH 02/13] refactor(pr): move maintainer edits to menus --- .../src/components/ui/AndroidScreenHeader.tsx | 4 +- .../PullRequestMaintainerEdits.tsx | 71 ----------------- .../pull-requests/PullRequestRouteScreen.tsx | 36 ++++++--- .../usePullRequestMaintainerEdits.ts | 60 ++++++++++++++ .../pullRequest/PullRequestActionsMenu.tsx | 35 ++++++++- .../pullRequest/PullRequestDetailPanel.tsx | 3 +- ...ullRequestMaintainerEdits.browser.test.tsx | 78 ++++++++++++++----- .../PullRequestMaintainerEdits.tsx | 60 +++++++------- .../pullRequest/PullRequestSummaryTab.tsx | 15 ---- 9 files changed, 207 insertions(+), 155 deletions(-) delete mode 100644 apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx create mode 100644 apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts diff --git a/apps/mobile/src/components/ui/AndroidScreenHeader.tsx b/apps/mobile/src/components/ui/AndroidScreenHeader.tsx index 7931717682fa1..3d7605b71b5fb 100644 --- a/apps/mobile/src/components/ui/AndroidScreenHeader.tsx +++ b/apps/mobile/src/components/ui/AndroidScreenHeader.tsx @@ -21,6 +21,7 @@ export interface AndroidHeaderAction { readonly disabled?: boolean; readonly loading?: boolean; readonly selected?: boolean; + readonly subtitle?: string; readonly destructive?: boolean; readonly overflowOnly?: boolean; } @@ -178,10 +179,11 @@ export function AndroidScreenHeader(props: { actions={overflowActions.map((action, index) => ({ id: String(index), title: action.accessibilityLabel, + subtitle: action.subtitle, + state: action.selected ? "on" : undefined, attributes: { disabled: Boolean(action.disabled), destructive: action.destructive, - state: action.selected ? "on" : undefined, }, }))} onPressAction={({ nativeEvent }) => diff --git a/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx b/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx deleted file mode 100644 index 6a39cf25a2fb5..0000000000000 --- a/apps/mobile/src/features/pull-requests/PullRequestMaintainerEdits.tsx +++ /dev/null @@ -1,71 +0,0 @@ -import { useAtomRefresh } from "@effect/atom-react"; -import type { PullRequestDetail } from "@t3tools/contracts"; -import { AsyncResult } from "effect/reactivity"; -import { useRef, useState } from "react"; -import { View } from "react-native"; - -import { AppText } from "../../components/ui/AppText"; -import { ListToggleRow } from "../../components/ui/ListToggleRow"; -import { composerPullRequests } from "../../state/pull-requests"; -import { useAtomCommand } from "../../state/use-atom-command"; -import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff"; -import { usePullRequestPermissions } from "./usePullRequestPermissions"; - -export function PullRequestMaintainerEdits({ - params, - pr, -}: { - params: PullRequestParams; - pr: PullRequestDetail; -}) { - const target = pullRequestTarget(params); - const permissions = usePullRequestPermissions(params, pr); - const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); - const refresh = useAtomRefresh(composerPullRequests.detail(target)); - const [value, setValue] = useState(pr.maintainerCanModify ?? false); - const [saving, setSaving] = useState(false); - const [error, setError] = useState(false); - const inFlight = useRef(false); - const save = async (next: boolean) => { - if (!permissions.canEditChangeRequest || inFlight.current) return; - const previous = value; - inFlight.current = true; - setSaving(true); - setError(false); - setValue(next); - const result = await update({ - ...target, - input: { ...target.input, maintainerCanModify: next }, - }); - inFlight.current = false; - setSaving(false); - if (AsyncResult.isFailure(result)) { - setValue(previous); - setError(true); - return; - } - refresh(); - }; - - return ( - - void save(next)} - /> - {saving ? ( - - Saving… - - ) : null} - {error ? ( - - Could not save maintainer access. Try again. - - ) : null} - - ); -} diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 8bde4ddea45b3..fa13cf10439fb 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -110,7 +110,7 @@ import { PullRequestReadStatus, } from "./PullRequestPresentation"; import { usePullRequestFilesSection } from "./PullRequestFiles"; -import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; +import { usePullRequestMaintainerEdits } from "./usePullRequestMaintainerEdits"; import { PullRequestDescription } from "./PullRequestDescription"; import { PullRequestMediaVisibleContext } from "./PullRequestMedia"; import { PullRequestMarkdownRepository } from "./PullRequestMarkdown"; @@ -326,15 +326,6 @@ function PullRequestOverview(props: { footer={} /> - {pr.capabilities.edit?.maintainerEdits === true && - pr.viewerPermissions.maintainerEdits === true && - pr.maintainerCanModify !== undefined ? ( - - ) : null} ); } @@ -425,6 +416,7 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps pr && @@ -526,6 +518,19 @@ export function PullRequestRouteScreen({ route }: StaticScreenPropsEdit description ) : null} + {maintainerEdits.visible ? ( + + Allow edits from maintainers + + ) : null} {identity ? ( { + if (!visible || !permissions.canEditChangeRequest || inFlight.current) return; + const previous = value; + inFlight.current = true; + setSaving(true); + setSelection({ key, value: next }); + const result = await update({ + ...target, + input: { ...target.input, maintainerCanModify: next }, + }); + inFlight.current = false; + setSaving(false); + if (AsyncResult.isFailure(result)) { + setSelection({ key, value: previous }); + Dialog.alert("Could not save maintainer access", "Try again."); + return; + } + refresh(); + }; + + return { + visible, + value, + disabled: !permissions.canEditChangeRequest || saving, + subtitle: saving + ? "Saving…" + : "Maintainers can push to this branch and access fork workflow secrets.", + toggle: () => void save(!value), + }; +} diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index c920bbd3e19ff..2c02c7968adc3 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -44,6 +44,7 @@ import { } from "./pullRequestDetail.logic"; import { PullRequestGlyph } from "./pullRequestIcons"; import type { PickableEnvironment } from "./pullRequestProjectAssignment.logic"; +import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; import { PullRequestThreadLinks } from "./PullRequestThreadLinks"; import { useInlineActionConfirm } from "./useInlineActionConfirm"; @@ -83,6 +84,8 @@ export function PullRequestActionsMenu({ onSelectMergeMethod, showsDeleteBranch, environmentConnected, + canWriteSourceControl, + onMaintainerEditsSaved, confirmDeleteBranch, pendingAction, perform, @@ -92,7 +95,16 @@ export function PullRequestActionsMenu({ environmentId: EnvironmentId; reference: PullRequestRef; pullRequestKey: string; - detail: Pick; + detail: Pick< + PullRequestDetail, + | "url" + | "state" + | "isDraft" + | "headBranch" + | "capabilities" + | "viewerPermissions" + | "maintainerCanModify" + >; terminology: ChangeRequestTerminology; linkThreadRef: ScopedThreadRef | null; onThreadPickerOpenChange: (open: boolean) => void; @@ -123,6 +135,8 @@ export function PullRequestActionsMenu({ onSelectMergeMethod: (method: PullRequestMergeMethod) => void; showsDeleteBranch: boolean; environmentConnected: boolean; + canWriteSourceControl: boolean; + onMaintainerEditsSaved: () => void; confirmDeleteBranch: () => Promise; pendingAction: PullRequestAction | null; perform: PerformPullRequestAction; @@ -132,6 +146,10 @@ export function PullRequestActionsMenu({ const keybindings = useAtomValue(primaryServerKeybindingsAtom); const inlineConfirm = useInlineActionConfirm(pullRequestKey); const actionPending = pendingAction !== null; + const showsMaintainerEdits = + detail.capabilities.edit?.maintainerEdits === true && + detail.viewerPermissions.maintainerEdits === true && + detail.maintainerCanModify !== undefined; return ( @@ -164,7 +182,7 @@ export function PullRequestActionsMenu({ : `More ${terminology.singular} actions`} - + ) : null} + {showsMaintainerEdits && detail.maintainerCanModify !== undefined ? ( + <> + + + + ) : null} {detail.state === "open" ? ( <> {showsDraftToggle ? ( diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 9f77a2059a47c..5e1406319c3b9 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -1383,6 +1383,8 @@ export function PullRequestDetailPanel({ onSelectMergeMethod={selectMergeMethod} showsDeleteBranch={showsDeleteBranch} environmentConnected={environmentConnected} + canWriteSourceControl={canWriteSourceControl} + onMaintainerEditsSaved={refreshDetail} confirmDeleteBranch={confirmDeleteBranch} pendingAction={pendingAction} perform={perform} @@ -1736,7 +1738,6 @@ export function PullRequestDetailPanel({ detail={detail} activityPending={activityPending} checksStale={checksStale} - canWriteSourceControl={canWriteSourceControl && environmentConnected} activityError={activityError} pendingFinding={handoff} fixFindingLabel={handoffLabels.fixFinding} diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx index 9079145e96174..7cbc182e05016 100644 --- a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx @@ -1,9 +1,11 @@ import "~/index.css"; import { EnvironmentId, ProjectId } from "@t3tools/contracts"; import { expect, it, vi } from "vite-plus/test"; +import { page, userEvent } from "vite-plus/test/browser"; const mocks = vi.hoisted(() => ({ update: vi.fn<(input: unknown) => Promise<{ _tag: "Success" | "Failure" }>>(), + notify: vi.fn(), })); vi.mock( import("~/state/pullRequests"), @@ -14,7 +16,11 @@ vi.mock( () => ({ useAtomCommand: () => mocks.update }) as never, ); +vi.mock(import("../ui/toast"), () => ({ toastManager: { add: mocks.notify } }) as never); + import { render } from "../../../test/render"; +import { Menu, MenuPopup, MenuTrigger } from "../ui/menu"; + import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; const reference = { projectId: ProjectId.make("project"), repository: "owner/repo", number: 7 }; @@ -22,15 +28,21 @@ const environmentId = EnvironmentId.make("environment"); function setup() { mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); + mocks.notify.mockReset(); const onSaved = vi.fn(); const control = (allowed = true) => ( - + + More pull request actions + + + + ); return { control, onSaved }; } @@ -38,7 +50,14 @@ function setup() { it("saves both directions without waiting for a refreshed detail", async () => { const { control, onSaved } = setup(); await using screen = await render(control()); - const checkbox = screen.getByRole("checkbox", { name: "Allow edits from maintainers" }); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + expect(document.querySelector('[role="menuitemcheckbox"]')).not.toBeNull(); + const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + await expect + .element(checkbox) + .toHaveAccessibleDescription( + "Maintainers can push to this branch. If your fork has workflows, this can also give access to its secrets.", + ); await checkbox.click(); await expect.element(checkbox).not.toBeChecked(); await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); @@ -50,7 +69,7 @@ it("saves both directions without waiting for a refreshed detail", async () => { ]); }); -it("locks the control while a save is in flight", async () => { +it("locks the control while a save is in flight, including after reopening the menu", async () => { const { control } = setup(); let settle!: (value: { _tag: "Success" }) => void; const pending = new Promise<{ _tag: "Success" }>((resolve) => { @@ -58,37 +77,54 @@ it("locks the control while a save is in flight", async () => { }); mocks.update.mockReturnValue(pending); await using screen = await render(control()); - const checkbox = screen.getByRole("checkbox"); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox"); await checkbox.click(); - await expect.element(checkbox).toBeDisabled(); - await expect.element(screen.getByRole("status")).toHaveTextContent("Saving…"); + await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); + await expect.element(page.getByRole("status")).toHaveTextContent("Saving…"); + await userEvent.keyboard("{Escape}"); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); expect(mocks.update).toHaveBeenCalledTimes(1); settle({ _tag: "Success" }); - await expect.element(checkbox).toBeEnabled(); + await expect.element(checkbox).not.toHaveAttribute("aria-disabled", "true"); }); -it("restores the last saved value after a failed save and lets the user retry", async () => { +it("reports a failed save after closing the menu and lets the user retry", async () => { const { control, onSaved } = setup(); await using screen = await render(control()); - const checkbox = screen.getByRole("checkbox"); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox"); await checkbox.click(); await expect.element(checkbox).not.toBeChecked(); await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); - mocks.update.mockResolvedValueOnce({ _tag: "Failure" }); + let settle!: (value: { _tag: "Failure" }) => void; + mocks.update.mockReturnValueOnce( + new Promise<{ _tag: "Failure" }>((resolve) => { + settle = resolve; + }), + ); await checkbox.click(); - await expect - .element(screen.getByRole("alert")) - .toHaveTextContent("Could not save maintainer access. Try again."); + await userEvent.keyboard("{Escape}"); + settle({ _tag: "Failure" }); + await vi.waitFor(() => + expect(mocks.notify).toHaveBeenCalledWith({ + type: "error", + title: "Could not save maintainer access", + description: "Try again.", + }), + ); + await screen.getByRole("button", { name: "More pull request actions" }).click(); await expect.element(checkbox).not.toBeChecked(); expect(onSaved).toHaveBeenCalledTimes(1); await checkbox.click(); await expect.element(checkbox).toBeChecked(); - await expect.element(screen.getByRole("alert")).not.toBeInTheDocument(); }); it("blocks a session without source control write access", async () => { const { control } = setup(); await using screen = await render(control(false)); - await expect.element(screen.getByRole("checkbox")).toBeDisabled(); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + await expect.element(page.getByRole("menuitemcheckbox")).toHaveAttribute("aria-disabled", "true"); expect(mocks.update).not.toHaveBeenCalled(); }); diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx index 5e9012f6a672f..00a78bbc24e1f 100644 --- a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx +++ b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx @@ -3,7 +3,8 @@ import { useId, useRef, useState } from "react"; import { pullRequestEnvironment } from "~/state/pullRequests"; import { useAtomCommand } from "~/state/use-atom-command"; -import { Checkbox } from "../ui/checkbox"; +import { MenuCheckboxItem } from "../ui/menu"; +import { toastManager } from "../ui/toast"; export function PullRequestMaintainerEdits({ environmentId, @@ -22,14 +23,12 @@ export function PullRequestMaintainerEdits({ const update = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false }); const [value, setValue] = useState(enabled); const [saving, setSaving] = useState(false); - const [error, setError] = useState(false); const inFlight = useRef(false); const save = async (next: boolean) => { if (!allowed || inFlight.current) return; const previous = value; inFlight.current = true; setSaving(true); - setError(false); setValue(next); const result = await update({ environmentId, @@ -39,44 +38,37 @@ export function PullRequestMaintainerEdits({ setSaving(false); if (result._tag === "Failure") { setValue(previous); - setError(true); + toastManager.add({ + type: "error", + title: "Could not save maintainer access", + description: "Try again.", + }); return; } onSaved(); }; return ( -
-
- - void save(next)} - /> + void save(next)} + > + + Allow edits from maintainers + + Maintainers can push to this branch. If your fork has workflows, this can also give access + to its secrets. -
- - - Maintainers can push to this branch. If your fork has workflows, this can also give - access to its secrets. + {saving ? ( + + Saving… -
-
- {saving ? ( -

- Saving… -

- ) : null} - {error ? ( -

- Could not save maintainer access. Try again. -

- ) : null} -
+ ) : null} + + ); } diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 422ec2bafd456..f8084aa550ad3 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -31,7 +31,6 @@ import { RelativeTime } from "~/components/RelativeTime"; import { Button } from "../ui/button"; import { PullRequestEditButton } from "./PullRequestEditButton"; -import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { Menu, @@ -385,7 +384,6 @@ export function PullRequestSummaryTab({ detail, activityPending, checksStale = false, - canWriteSourceControl = false, activityError, pendingFinding, fixFindingLabel = "Fix in a thread", @@ -401,7 +399,6 @@ export function PullRequestSummaryTab({ detail: PullRequestDetailView; activityPending: boolean; checksStale?: boolean; - canWriteSourceControl?: boolean; activityError: string | null; pendingFinding?: string | null; fixFindingLabel?: string; @@ -792,18 +789,6 @@ export function PullRequestSummaryTab({ )} - {detail.capabilities.edit?.maintainerEdits === true && - detail.viewerPermissions.maintainerEdits === true && - detail.maintainerCanModify !== undefined ? ( - - ) : null}
From 05ea16622ac1a6a4da21f0a5973541a774209f9b Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:03:27 +0100 Subject: [PATCH 03/13] fix(pr): hide maintainer edits on closed requests --- .../usePullRequestMaintainerEdits.ts | 6 ++---- .../pullRequest/PullRequestActionsMenu.tsx | 6 ++---- .../src/pullRequestEditing.test.ts | 16 ++++++++++++++++ .../client-runtime/src/pullRequestEditing.ts | 14 ++++++++++++++ 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index c989978555c07..1d7ff56730c63 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -1,4 +1,5 @@ import { useAtomRefresh } from "@effect/atom-react"; +import { canEditPullRequestMaintainerEdits } from "@t3tools/client-runtime/pull-request-editing"; import type { PullRequestDetail } from "@t3tools/contracts"; import { AsyncResult } from "effect/reactivity"; import { useRef, useState } from "react"; @@ -24,10 +25,7 @@ export function usePullRequestMaintainerEdits( const value = selection.key === key ? selection.value : (pr?.maintainerCanModify ?? false); const [saving, setSaving] = useState(false); const inFlight = useRef(false); - const visible = - pr?.capabilities.edit?.maintainerEdits === true && - pr.viewerPermissions.maintainerEdits === true && - pr.maintainerCanModify !== undefined; + const visible = pr !== null && canEditPullRequestMaintainerEdits(pr); const save = async (next: boolean) => { if (!visible || !permissions.canEditChangeRequest || inFlight.current) return; const previous = value; diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index 2c02c7968adc3..c57a7f941faa3 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -1,4 +1,5 @@ import { useAtomValue } from "@effect/atom-react"; +import { canEditPullRequestMaintainerEdits } from "@t3tools/client-runtime/pull-request-editing"; import type { ChangeRequestTerminology } from "@t3tools/client-runtime/source-control-clients"; import type { EnvironmentId, @@ -146,10 +147,7 @@ export function PullRequestActionsMenu({ const keybindings = useAtomValue(primaryServerKeybindingsAtom); const inlineConfirm = useInlineActionConfirm(pullRequestKey); const actionPending = pendingAction !== null; - const showsMaintainerEdits = - detail.capabilities.edit?.maintainerEdits === true && - detail.viewerPermissions.maintainerEdits === true && - detail.maintainerCanModify !== undefined; + const showsMaintainerEdits = canEditPullRequestMaintainerEdits(detail); return ( diff --git a/packages/client-runtime/src/pullRequestEditing.test.ts b/packages/client-runtime/src/pullRequestEditing.test.ts index 1b473f4657def..e9dde2b1d62ff 100644 --- a/packages/client-runtime/src/pullRequestEditing.test.ts +++ b/packages/client-runtime/src/pullRequestEditing.test.ts @@ -10,6 +10,7 @@ import { describe, expect, it } from "vite-plus/test"; import { canEditPullRequestChangeRequest, canEditPullRequestComment, + canEditPullRequestMaintainerEdits, canResolvePullRequestThread, } from "./pullRequestEditing.ts"; @@ -141,6 +142,21 @@ describe("canEditPullRequestChangeRequest", () => { }); }); +describe("canEditPullRequestMaintainerEdits", () => { + it.each(["open", "closed", "merged"] as const)( + "only allows an open pull request when cached permissions allow edits: %s", + (state) => { + const detail = { + state, + capabilities: capabilities({ changeRequest: true, comment: true, maintainerEdits: true }), + viewerPermissions: permissions({ maintainerEdits: true }), + maintainerCanModify: false, + }; + expect(canEditPullRequestMaintainerEdits(detail)).toBe(state === "open"); + }, + ); +}); + describe("canEditPullRequestComment", () => { it("lets the reader rewrite their own remark on a line", () => { expect(canEditPullRequestComment(subject(), comment({ kind: "review-comment" }))).toBe(true); diff --git a/packages/client-runtime/src/pullRequestEditing.ts b/packages/client-runtime/src/pullRequestEditing.ts index 96b4d5c494f9b..f9534e821eccb 100644 --- a/packages/client-runtime/src/pullRequestEditing.ts +++ b/packages/client-runtime/src/pullRequestEditing.ts @@ -25,6 +25,20 @@ export function canEditPullRequestChangeRequest(detail: EditingSubject): boolean ); } +export function canEditPullRequestMaintainerEdits( + detail: Pick< + PullRequestDetail, + "state" | "capabilities" | "viewerPermissions" | "maintainerCanModify" + >, +): boolean { + return ( + detail.state === "open" && + detail.capabilities.edit?.maintainerEdits === true && + detail.viewerPermissions.maintainerEdits === true && + detail.maintainerCanModify !== undefined + ); +} + /** Hosts do not support rewriting review summaries through comment mutations. */ export function canEditPullRequestComment( detail: EditingSubject, From ff02566f5683bf3e96e06cafda86955ed3a7d8de Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:30:43 +0100 Subject: [PATCH 04/13] fix(mobile): use maintainer edit permission --- .../usePullRequestMaintainerEdits.test.tsx | 168 ++++++++++++++++++ .../usePullRequestMaintainerEdits.ts | 12 +- apps/mobile/vite.config.ts | 1 + 3 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx new file mode 100644 index 0000000000000..615a1de3fa7bf --- /dev/null +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx @@ -0,0 +1,168 @@ +import { RegistryContext } from "@effect/atom-react"; +import { canEditPullRequestChangeRequest } from "@t3tools/client-runtime/pull-request-editing"; +import { + EnvironmentId, + ProjectId, + SourceControlProviderKind, + type PullRequestDetail, +} from "@t3tools/contracts"; +import { Atom, AtomRegistry } from "effect/reactivity"; +import { act, useLayoutEffect } from "react"; +import { createRoot } from "react-dom/client"; +import { expect, it, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + connected: true, + update: vi.fn(), + permission: undefined as unknown as Atom.Writable, + detail: undefined as unknown as Atom.Writable, +})); +vi.mock("../../state/pull-requests", () => { + const command = { permissionAtom: () => mocks.permission }; + return { + composerPullRequests: { + runAction: command, + comment: command, + submitReview: command, + replyToThread: command, + setThreadResolution: command, + update: command, + updateComment: command, + setReaction: command, + detail: () => mocks.detail, + }, + }; +}); +vi.mock("../../state/use-atom-command", () => ({ useAtomCommand: () => mocks.update })); +vi.mock("../../components/ui/Dialog", () => ({ alert: vi.fn() })); +vi.mock("../../state/workspace", () => ({ + useWorkspaceEnvironments: () => [ + { + environmentId: "environment", + connectionState: mocks.connected ? "connected" : "offline", + }, + ], +})); + +import { usePullRequestMaintainerEdits } from "./usePullRequestMaintainerEdits"; + +function setup() { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + mocks.connected = true; + mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); + mocks.permission = Atom.make(true); + mocks.detail = Atom.make(null); + const registry = AtomRegistry.make(); + const detail: PullRequestDetail = { + provider: SourceControlProviderKind.make("github"), + projectId: ProjectId.make("project"), + projectTitle: "Project", + workspaceRoot: "/repo", + repository: "owner/repo", + number: 7, + title: "A change", + body: "", + url: "https://github.com/owner/repo/pull/7", + author: { login: "author", name: null, avatarUrl: null }, + state: "open", + isDraft: false, + mergeability: "unknown", + additions: 0, + deletions: 0, + changedFiles: 0, + headBranch: "feature", + baseBranch: "main", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + mergedAt: null, + closedAt: null, + reviewers: [], + labels: [], + checks: [], + mergeCapabilities: { merge: false, squash: false, rebase: false }, + capabilities: { + diff: true, + comment: true, + search: true, + actions: [], + mergeMethods: [], + review: { inlineComment: false, reply: false, resolve: false, verdicts: [] }, + reviewers: { request: false, listCandidates: false }, + edit: { changeRequest: true, comment: true, maintainerEdits: true }, + }, + viewerPermissions: { + actions: [], + comment: true, + resolve: false, + verdicts: [], + requestReviewers: false, + maintainerEdits: true, + }, + maintainerCanModify: false, + }; + const root = createRoot(document.createElement("div")); + let current!: ReturnType; + function Probe() { + const result = usePullRequestMaintainerEdits( + { + environmentId: "environment", + projectId: "project", + repository: "owner/repo", + number: 7, + }, + detail, + ); + useLayoutEffect(() => { + current = result; + }); + return null; + } + const render = () => + act(() => + root.render( + + + , + ), + ); + render(); + return { + detail, + render, + registry, + get current() { + return current; + }, + [Symbol.dispose]() { + act(() => root.unmount()); + registry.dispose(); + }, + }; +} + +it("saves an explicit maintainer grant without a viewer login, while honoring local access", async () => { + using screen = setup(); + expect(canEditPullRequestChangeRequest(screen.detail)).toBe(false); + expect(screen.current.visible).toBe(true); + expect(screen.current.disabled).toBe(false); + await act(async () => screen.current.toggle()); + expect(screen.current.value).toBe(true); + expect(mocks.update).toHaveBeenCalledExactlyOnceWith({ + environmentId: EnvironmentId.make("environment"), + input: { + projectId: ProjectId.make("project"), + repository: "owner/repo", + number: 7, + maintainerCanModify: true, + }, + }); + act(() => screen.registry.set(mocks.permission, false)); + expect(screen.current.disabled).toBe(true); + await act(async () => screen.current.toggle()); + act(() => screen.registry.set(mocks.permission, true)); + mocks.connected = false; + screen.render(); + expect(screen.current.disabled).toBe(true); + await act(async () => screen.current.toggle()); + expect(mocks.update).toHaveBeenCalledTimes(1); +}); diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index 1d7ff56730c63..e8687b5eb1724 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -1,4 +1,4 @@ -import { useAtomRefresh } from "@effect/atom-react"; +import { useAtomRefresh, useAtomValue } from "@effect/atom-react"; import { canEditPullRequestMaintainerEdits } from "@t3tools/client-runtime/pull-request-editing"; import type { PullRequestDetail } from "@t3tools/contracts"; import { AsyncResult } from "effect/reactivity"; @@ -7,14 +7,16 @@ import * as Dialog from "../../components/ui/Dialog"; import { composerPullRequests } from "../../state/pull-requests"; import { useAtomCommand } from "../../state/use-atom-command"; import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff"; -import { usePullRequestPermissions } from "./usePullRequestPermissions"; +import { usePullRequestConnected } from "./usePullRequestConnected"; export function usePullRequestMaintainerEdits( params: PullRequestParams, pr: PullRequestDetail | null, ) { const target = pullRequestTarget(params); - const permissions = usePullRequestPermissions(params, pr); + const connected = usePullRequestConnected(target.environmentId); + const canUpdate = useAtomValue(composerPullRequests.update.permissionAtom(target.environmentId)); + const allowed = connected && canUpdate; const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); const refresh = useAtomRefresh(composerPullRequests.detail(target)); const key = `${pr?.url}:${pr?.maintainerCanModify}`; @@ -27,7 +29,7 @@ export function usePullRequestMaintainerEdits( const inFlight = useRef(false); const visible = pr !== null && canEditPullRequestMaintainerEdits(pr); const save = async (next: boolean) => { - if (!visible || !permissions.canEditChangeRequest || inFlight.current) return; + if (!visible || !allowed || inFlight.current) return; const previous = value; inFlight.current = true; setSaving(true); @@ -49,7 +51,7 @@ export function usePullRequestMaintainerEdits( return { visible, value, - disabled: !permissions.canEditChangeRequest || saving, + disabled: !allowed || saving, subtitle: saving ? "Saving…" : "Maintainers can push to this branch and access fork workflow secrets.", diff --git a/apps/mobile/vite.config.ts b/apps/mobile/vite.config.ts index 6a92a025778d9..98e1627a6b084 100644 --- a/apps/mobile/vite.config.ts +++ b/apps/mobile/vite.config.ts @@ -13,6 +13,7 @@ const reactTests = [ "src/features/diffs/nativeReviewDiffSurface.test.tsx", "src/features/pull-requests/usePullRequestDiff.test.tsx", "src/components/ProjectFavicon.test.tsx", + "src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx", "src/components/ui/Dialog.android.test.tsx", "src/features/files/ZipArchiveView.test.tsx", "src/features/pull-requests/guide/useReviewGuide.test.tsx", From d65b597b6dbffb9b7662d35b58d69658aed52b55 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Tue, 27 Oct 2026 23:42:50 +0100 Subject: [PATCH 05/13] fix(pr): preserve pending maintainer edits on refresh --- .../usePullRequestMaintainerEdits.test.tsx | 33 ++++- .../usePullRequestMaintainerEdits.ts | 7 +- .../pullRequest/PullRequestActionsMenu.tsx | 2 +- ...ullRequestMaintainerEdits.browser.test.tsx | 123 +++++++++++++++--- .../PullRequestMaintainerEdits.tsx | 5 + 5 files changed, 148 insertions(+), 22 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx index 615a1de3fa7bf..ab4381eb57c6e 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx @@ -53,7 +53,7 @@ function setup() { mocks.permission = Atom.make(true); mocks.detail = Atom.make(null); const registry = AtomRegistry.make(); - const detail: PullRequestDetail = { + let detail: PullRequestDetail = { provider: SourceControlProviderKind.make("github"), projectId: ProjectId.make("project"), projectTitle: "Project", @@ -127,7 +127,13 @@ function setup() { ); render(); return { - detail, + get detail() { + return detail; + }, + refresh(enabled: boolean) { + detail = { ...detail, maintainerCanModify: enabled }; + render(); + }, render, registry, get current() { @@ -166,3 +172,26 @@ it("saves an explicit maintainer grant without a viewer login, while honoring lo await act(async () => screen.current.toggle()); expect(mocks.update).toHaveBeenCalledTimes(1); }); + +it("keeps a newer selection when an earlier save's refresh arrives", async () => { + using screen = setup(); + await act(async () => screen.current.toggle()); + expect(screen.current.value).toBe(true); + let settle!: (value: { _tag: "Success" }) => void; + mocks.update.mockReturnValueOnce( + new Promise<{ _tag: "Success" }>((resolve) => { + settle = resolve; + }), + ); + await act(async () => screen.current.toggle()); + expect(screen.current.value).toBe(false); + screen.refresh(true); + expect(screen.current.disabled).toBe(true); + expect(screen.current.value).toBe(false); + await act(async () => screen.current.toggle()); + expect(mocks.update).toHaveBeenCalledTimes(2); + await act(async () => settle({ _tag: "Success" })); + expect(screen.current.value).toBe(false); + screen.refresh(false); + expect(screen.current.value).toBe(false); +}); diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index e8687b5eb1724..ec78877a55035 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -20,12 +20,13 @@ export function usePullRequestMaintainerEdits( const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); const refresh = useAtomRefresh(composerPullRequests.detail(target)); const key = `${pr?.url}:${pr?.maintainerCanModify}`; + const [saving, setSaving] = useState(false); const [selection, setSelection] = useState({ key, value: pr?.maintainerCanModify ?? false }); + const value = + selection.key === key || saving ? selection.value : (pr?.maintainerCanModify ?? false); if (selection.key !== key) { - setSelection({ key, value: pr?.maintainerCanModify ?? false }); + setSelection({ key, value }); } - const value = selection.key === key ? selection.value : (pr?.maintainerCanModify ?? false); - const [saving, setSaving] = useState(false); const inFlight = useRef(false); const visible = pr !== null && canEditPullRequestMaintainerEdits(pr); const save = async (next: boolean) => { diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index c57a7f941faa3..12240af538624 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -237,7 +237,7 @@ export function PullRequestActionsMenu({ {showsMaintainerEdits && detail.maintainerCanModify !== undefined ? ( <> ({ toastManager: { add: mocks.notify } }) as never); +vi.mock(import("~/state/server"), async () => { + const { Atom } = await import("effect/reactivity"); + return { primaryServerKeybindingsAtom: Atom.make([]) } as never; +}); +vi.mock(import("./PullRequestThreadLinks"), () => ({ PullRequestThreadLinks: () => null })); +vi.mock( + import("./PullRequestDetailActions"), + () => ({ ActOnEnvironmentPicker: () => null }) as never, +); import { render } from "../../../test/render"; -import { Menu, MenuPopup, MenuTrigger } from "../ui/menu"; - -import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; +import { PullRequestActionsMenu } from "./PullRequestActionsMenu"; const reference = { projectId: ProjectId.make("project"), repository: "owner/repo", number: 7 }; const environmentId = EnvironmentId.make("environment"); @@ -30,19 +38,74 @@ function setup() { mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); mocks.notify.mockReset(); const onSaved = vi.fn(); - const control = (allowed = true) => ( - - More pull request actions - - - - + const control = (allowed = true, enabled = true) => ( + {}} + refreshing={false} + refreshFromHost={async () => {}} + handoff={null} + attachesToComposer={false} + askAboutPullRequest={() => {}} + explainPullRequest={() => {}} + canFixFindings={false} + startFixFindings={() => {}} + handoffLabels={{ fixFindings: "Fix review findings" }} + canWriteGuide={false} + guideRunning={false} + guideWritten={false} + writeGuide={() => {}} + pickableEnvironments={[]} + actingEnvironmentId={environmentId} + onActingEnvironmentChange={() => {}} + can={() => false} + autoMergeArmed={false} + showsDraftToggle={false} + showsMergeNow={false} + showsAutoMerge={false} + showsMergeMethods={false} + allowedMergeMethods={[]} + selectedMergeMethod="squash" + onSelectMergeMethod={() => {}} + showsDeleteBranch={false} + environmentConnected={true} + canWriteSourceControl={allowed} + onMaintainerEditsSaved={onSaved} + confirmDeleteBranch={async () => {}} + pendingAction={null} + perform={async () => true} + openOnHostText="Open on GitHub" + copyReference={() => {}} + /> ); return { control, onSaved }; } @@ -128,3 +191,31 @@ it("blocks a session without source control write access", async () => { await expect.element(page.getByRole("menuitemcheckbox")).toHaveAttribute("aria-disabled", "true"); expect(mocks.update).not.toHaveBeenCalled(); }); + +it("keeps a second save locked when the first save's refresh arrives", async () => { + const { control, onSaved } = setup(); + await using screen = await render(control()); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox"); + await checkbox.click(); + await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); + let settle!: (value: { _tag: "Success" }) => void; + mocks.update.mockReturnValueOnce( + new Promise<{ _tag: "Success" }>((resolve) => { + settle = resolve; + }), + ); + await checkbox.click(); + await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); + await screen.rerender(control(true, false)); + await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); + await expect.element(checkbox).toBeChecked(); + await userEvent.keyboard("{Enter}"); + expect(mocks.update).toHaveBeenCalledTimes(2); + settle({ _tag: "Success" }); + await expect.element(checkbox).not.toHaveAttribute("aria-disabled", "true"); + await expect.element(checkbox).toBeChecked(); + await screen.rerender(control(true, true)); + await screen.rerender(control(true, false)); + await expect.element(checkbox).not.toBeChecked(); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx index 00a78bbc24e1f..88ff6e43715d7 100644 --- a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx +++ b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx @@ -23,6 +23,11 @@ export function PullRequestMaintainerEdits({ const update = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false }); const [value, setValue] = useState(enabled); const [saving, setSaving] = useState(false); + const [observedValue, setObservedValue] = useState(enabled); + if (observedValue !== enabled) { + setObservedValue(enabled); + if (!saving) setValue(enabled); + } const inFlight = useRef(false); const save = async (next: boolean) => { if (!allowed || inFlight.current) return; From 718327b037dc22e4ba40a4d2d60625a5b5ca9ff7 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 02:17:13 +0000 Subject: [PATCH 06/13] test(pull-requests): takeover tests for maintainer edits Cut the PR's tests down to what proves the spec: GitHub decoding of maintainerCanModify and who may change it, the UpdatePullRequest mutation sending it alone, the service forwarding it and refusing hosts without the capability, and one actions-menu test for saving/rollback and visibility. Co-Authored-By: Claude Opus 5.5 --- .../usePullRequestMaintainerEdits.test.tsx | 197 ---------------- apps/mobile/vite.config.ts | 1 - .../pullRequest/PullRequestService.test.ts | 11 +- .../PullRequestActionsMenu.browser.test.tsx | 163 +++++++++++++ ...ullRequestMaintainerEdits.browser.test.tsx | 221 ------------------ .../src/pullRequestEditing.test.ts | 16 -- .../src/server/GitHubPullRequestApi.test.ts | 32 ++- .../src/server/gitHubPullRequestJson.test.ts | 30 +-- 8 files changed, 198 insertions(+), 473 deletions(-) delete mode 100644 apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx create mode 100644 apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx delete mode 100644 apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx deleted file mode 100644 index ab4381eb57c6e..0000000000000 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx +++ /dev/null @@ -1,197 +0,0 @@ -import { RegistryContext } from "@effect/atom-react"; -import { canEditPullRequestChangeRequest } from "@t3tools/client-runtime/pull-request-editing"; -import { - EnvironmentId, - ProjectId, - SourceControlProviderKind, - type PullRequestDetail, -} from "@t3tools/contracts"; -import { Atom, AtomRegistry } from "effect/reactivity"; -import { act, useLayoutEffect } from "react"; -import { createRoot } from "react-dom/client"; -import { expect, it, vi } from "vite-plus/test"; - -const mocks = vi.hoisted(() => ({ - connected: true, - update: vi.fn(), - permission: undefined as unknown as Atom.Writable, - detail: undefined as unknown as Atom.Writable, -})); -vi.mock("../../state/pull-requests", () => { - const command = { permissionAtom: () => mocks.permission }; - return { - composerPullRequests: { - runAction: command, - comment: command, - submitReview: command, - replyToThread: command, - setThreadResolution: command, - update: command, - updateComment: command, - setReaction: command, - detail: () => mocks.detail, - }, - }; -}); -vi.mock("../../state/use-atom-command", () => ({ useAtomCommand: () => mocks.update })); -vi.mock("../../components/ui/Dialog", () => ({ alert: vi.fn() })); -vi.mock("../../state/workspace", () => ({ - useWorkspaceEnvironments: () => [ - { - environmentId: "environment", - connectionState: mocks.connected ? "connected" : "offline", - }, - ], -})); - -import { usePullRequestMaintainerEdits } from "./usePullRequestMaintainerEdits"; - -function setup() { - (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; - mocks.connected = true; - mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); - mocks.permission = Atom.make(true); - mocks.detail = Atom.make(null); - const registry = AtomRegistry.make(); - let detail: PullRequestDetail = { - provider: SourceControlProviderKind.make("github"), - projectId: ProjectId.make("project"), - projectTitle: "Project", - workspaceRoot: "/repo", - repository: "owner/repo", - number: 7, - title: "A change", - body: "", - url: "https://github.com/owner/repo/pull/7", - author: { login: "author", name: null, avatarUrl: null }, - state: "open", - isDraft: false, - mergeability: "unknown", - additions: 0, - deletions: 0, - changedFiles: 0, - headBranch: "feature", - baseBranch: "main", - createdAt: "2026-01-01T00:00:00Z", - updatedAt: "2026-01-01T00:00:00Z", - mergedAt: null, - closedAt: null, - reviewers: [], - labels: [], - checks: [], - mergeCapabilities: { merge: false, squash: false, rebase: false }, - capabilities: { - diff: true, - comment: true, - search: true, - actions: [], - mergeMethods: [], - review: { inlineComment: false, reply: false, resolve: false, verdicts: [] }, - reviewers: { request: false, listCandidates: false }, - edit: { changeRequest: true, comment: true, maintainerEdits: true }, - }, - viewerPermissions: { - actions: [], - comment: true, - resolve: false, - verdicts: [], - requestReviewers: false, - maintainerEdits: true, - }, - maintainerCanModify: false, - }; - const root = createRoot(document.createElement("div")); - let current!: ReturnType; - function Probe() { - const result = usePullRequestMaintainerEdits( - { - environmentId: "environment", - projectId: "project", - repository: "owner/repo", - number: 7, - }, - detail, - ); - useLayoutEffect(() => { - current = result; - }); - return null; - } - const render = () => - act(() => - root.render( - - - , - ), - ); - render(); - return { - get detail() { - return detail; - }, - refresh(enabled: boolean) { - detail = { ...detail, maintainerCanModify: enabled }; - render(); - }, - render, - registry, - get current() { - return current; - }, - [Symbol.dispose]() { - act(() => root.unmount()); - registry.dispose(); - }, - }; -} - -it("saves an explicit maintainer grant without a viewer login, while honoring local access", async () => { - using screen = setup(); - expect(canEditPullRequestChangeRequest(screen.detail)).toBe(false); - expect(screen.current.visible).toBe(true); - expect(screen.current.disabled).toBe(false); - await act(async () => screen.current.toggle()); - expect(screen.current.value).toBe(true); - expect(mocks.update).toHaveBeenCalledExactlyOnceWith({ - environmentId: EnvironmentId.make("environment"), - input: { - projectId: ProjectId.make("project"), - repository: "owner/repo", - number: 7, - maintainerCanModify: true, - }, - }); - act(() => screen.registry.set(mocks.permission, false)); - expect(screen.current.disabled).toBe(true); - await act(async () => screen.current.toggle()); - act(() => screen.registry.set(mocks.permission, true)); - mocks.connected = false; - screen.render(); - expect(screen.current.disabled).toBe(true); - await act(async () => screen.current.toggle()); - expect(mocks.update).toHaveBeenCalledTimes(1); -}); - -it("keeps a newer selection when an earlier save's refresh arrives", async () => { - using screen = setup(); - await act(async () => screen.current.toggle()); - expect(screen.current.value).toBe(true); - let settle!: (value: { _tag: "Success" }) => void; - mocks.update.mockReturnValueOnce( - new Promise<{ _tag: "Success" }>((resolve) => { - settle = resolve; - }), - ); - await act(async () => screen.current.toggle()); - expect(screen.current.value).toBe(false); - screen.refresh(true); - expect(screen.current.disabled).toBe(true); - expect(screen.current.value).toBe(false); - await act(async () => screen.current.toggle()); - expect(mocks.update).toHaveBeenCalledTimes(2); - await act(async () => settle({ _tag: "Success" })); - expect(screen.current.value).toBe(false); - screen.refresh(false); - expect(screen.current.value).toBe(false); -}); diff --git a/apps/mobile/vite.config.ts b/apps/mobile/vite.config.ts index 98e1627a6b084..6a92a025778d9 100644 --- a/apps/mobile/vite.config.ts +++ b/apps/mobile/vite.config.ts @@ -13,7 +13,6 @@ const reactTests = [ "src/features/diffs/nativeReviewDiffSurface.test.tsx", "src/features/pull-requests/usePullRequestDiff.test.tsx", "src/components/ProjectFavicon.test.tsx", - "src/features/pull-requests/usePullRequestMaintainerEdits.test.tsx", "src/components/ui/Dialog.android.test.tsx", "src/features/files/ZipArchiveView.test.tsx", "src/features/pull-requests/guide/useReviewGuide.test.tsx", diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 5188145720409..6c6c8215aa315 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -116,7 +116,7 @@ function hostedChangeRequest(body: string, additions = 1) { }; } -it.effect("reads and changes maintainer edits through the existing update command", () => +it.effect("reads maintainer edits and changes them through the update command", () => Effect.gen(function* () { let enabled = true; const provider = fakePullRequestProvider(SourceControlProviderKind.make("github"), { @@ -143,14 +143,12 @@ it.effect("reads and changes maintainer edits through the existing update comman }); const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; assert.strictEqual((yield* service.detail(ref)).maintainerCanModify, true); - for (const value of [false, true]) { - yield* service.update({ ...ref, maintainerCanModify: value }); - assert.strictEqual((yield* service.detail(ref)).maintainerCanModify, value); - } + yield* service.update({ ...ref, maintainerCanModify: false }); + assert.strictEqual((yield* service.detail(ref)).maintainerCanModify, false); }), ); -it.effect("refuses maintainer edits on hosts that do not support them", () => +it.effect("refuses maintainer edits on a host without the capability", () => Effect.gen(function* () { let writes = 0; const service = yield* makeService({ @@ -169,7 +167,6 @@ it.effect("refuses maintainer edits on hosts that do not support them", () => projectId: "p1" as ProjectId, repository: "acme/web", number: 1, - body: "Body", maintainerCanModify: false, }) .pipe(Effect.flip); diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx new file mode 100644 index 0000000000000..0d8b7d22c6682 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -0,0 +1,163 @@ +import "~/index.css"; +import { EnvironmentId, ProjectId } from "@t3tools/contracts"; +import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients"; +import { expect, it, vi } from "vite-plus/test"; +import { page } from "vite-plus/test/browser"; + +const mocks = vi.hoisted(() => ({ + update: vi.fn<(input: unknown) => Promise<{ _tag: "Success" | "Failure" }>>(), + notify: vi.fn(), +})); +vi.mock( + import("~/state/pullRequests"), + () => ({ pullRequestEnvironment: { update: {} } }) as never, +); +vi.mock( + import("~/state/use-atom-command"), + () => ({ useAtomCommand: () => mocks.update }) as never, +); + +vi.mock(import("../ui/toast"), () => ({ toastManager: { add: mocks.notify } }) as never); +vi.mock(import("~/state/server"), async () => { + const { Atom } = await import("effect/reactivity"); + return { primaryServerKeybindingsAtom: Atom.make([]) } as never; +}); +vi.mock(import("./PullRequestThreadLinks"), () => ({ PullRequestThreadLinks: () => null })); +vi.mock( + import("./PullRequestDetailActions"), + () => ({ ActOnEnvironmentPicker: () => null }) as never, +); + +import { render } from "../../../test/render"; +import { PullRequestActionsMenu } from "./PullRequestActionsMenu"; + +const reference = { projectId: ProjectId.make("project"), repository: "owner/repo", number: 7 }; +const environmentId = EnvironmentId.make("environment"); + +type Detail = Parameters[0]["detail"]; + +const openDetail: Detail = { + url: "https://github.com/owner/repo/pull/7", + state: "open", + isDraft: false, + headBranch: "feature", + maintainerCanModify: true, + capabilities: { + diff: true, + comment: true, + actions: [], + mergeMethods: [], + search: true, + review: { inlineComment: true, reply: true, resolve: true, verdicts: [] }, + reviewers: { request: true, listCandidates: true }, + edit: { changeRequest: true, comment: true, maintainerEdits: true }, + }, + viewerPermissions: { + actions: [], + comment: true, + resolve: true, + verdicts: [], + requestReviewers: true, + maintainerEdits: true, + }, +}; + +function setup() { + mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); + mocks.notify.mockReset(); + return (detail: Detail = openDetail) => ( + {}} + refreshing={false} + refreshFromHost={async () => {}} + handoff={null} + attachesToComposer={false} + askAboutPullRequest={() => {}} + explainPullRequest={() => {}} + canFixFindings={false} + startFixFindings={() => {}} + handoffLabels={{ fixFindings: "Fix review findings" }} + canWriteGuide={false} + guideRunning={false} + guideWritten={false} + writeGuide={() => {}} + pickableEnvironments={[]} + actingEnvironmentId={environmentId} + onActingEnvironmentChange={() => {}} + can={() => false} + autoMergeArmed={false} + showsDraftToggle={false} + showsMergeNow={false} + showsAutoMerge={false} + showsMergeMethods={false} + allowedMergeMethods={[]} + selectedMergeMethod="squash" + onSelectMergeMethod={() => {}} + showsDeleteBranch={false} + environmentConnected={true} + canWriteSourceControl={true} + onMaintainerEditsSaved={() => {}} + confirmDeleteBranch={async () => {}} + pendingAction={null} + perform={async () => true} + openOnHostText="Open on GitHub" + copyReference={() => {}} + /> + ); +} + +it("saves maintainer edits, and restores the previous value with an error when saving fails", async () => { + const control = setup(); + let settle!: (value: { _tag: "Failure" }) => void; + mocks.update.mockReturnValueOnce( + new Promise<{ _tag: "Failure" }>((resolve) => { + settle = resolve; + }), + ); + await using screen = await render(control()); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + await expect.element(checkbox).toBeChecked(); + await checkbox.click(); + await expect.element(checkbox).not.toBeChecked(); + await expect.element(page.getByRole("status")).toHaveTextContent("Saving…"); + expect(mocks.update).toHaveBeenCalledExactlyOnceWith({ + environmentId, + input: { ...reference, maintainerCanModify: false }, + }); + settle({ _tag: "Failure" }); + await expect.element(checkbox).toBeChecked(); + expect(mocks.notify).toHaveBeenCalledWith( + expect.objectContaining({ type: "error", title: "Could not save maintainer access" }), + ); +}); + +it.each<[string, Detail]>([ + ["closed", { ...openDetail, state: "closed" }], + [ + "not authored by the viewer", + { + ...openDetail, + viewerPermissions: { ...openDetail.viewerPermissions, maintainerEdits: false }, + }, + ], + [ + "on a host without maintainer edits", + { + ...openDetail, + capabilities: { ...openDetail.capabilities, edit: { changeRequest: true, comment: true } }, + }, + ], +])("hides maintainer edits on a pull request that is %s", async (_, detail) => { + const control = setup(); + await using screen = await render(control(detail)); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + await expect.element(page.getByRole("menuitem", { name: "Open on GitHub" })).toBeVisible(); + expect(document.querySelector('[role="menuitemcheckbox"]')).toBeNull(); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx deleted file mode 100644 index 2b545b406d733..0000000000000 --- a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.browser.test.tsx +++ /dev/null @@ -1,221 +0,0 @@ -import "~/index.css"; -import { EnvironmentId, ProjectId } from "@t3tools/contracts"; -import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients"; -import { expect, it, vi } from "vite-plus/test"; -import { page, userEvent } from "vite-plus/test/browser"; - -const mocks = vi.hoisted(() => ({ - update: vi.fn<(input: unknown) => Promise<{ _tag: "Success" | "Failure" }>>(), - notify: vi.fn(), -})); -vi.mock( - import("~/state/pullRequests"), - () => ({ pullRequestEnvironment: { update: {} } }) as never, -); -vi.mock( - import("~/state/use-atom-command"), - () => ({ useAtomCommand: () => mocks.update }) as never, -); - -vi.mock(import("../ui/toast"), () => ({ toastManager: { add: mocks.notify } }) as never); -vi.mock(import("~/state/server"), async () => { - const { Atom } = await import("effect/reactivity"); - return { primaryServerKeybindingsAtom: Atom.make([]) } as never; -}); -vi.mock(import("./PullRequestThreadLinks"), () => ({ PullRequestThreadLinks: () => null })); -vi.mock( - import("./PullRequestDetailActions"), - () => ({ ActOnEnvironmentPicker: () => null }) as never, -); - -import { render } from "../../../test/render"; -import { PullRequestActionsMenu } from "./PullRequestActionsMenu"; - -const reference = { projectId: ProjectId.make("project"), repository: "owner/repo", number: 7 }; -const environmentId = EnvironmentId.make("environment"); - -function setup() { - mocks.update.mockReset().mockResolvedValue({ _tag: "Success" }); - mocks.notify.mockReset(); - const onSaved = vi.fn(); - const control = (allowed = true, enabled = true) => ( - {}} - refreshing={false} - refreshFromHost={async () => {}} - handoff={null} - attachesToComposer={false} - askAboutPullRequest={() => {}} - explainPullRequest={() => {}} - canFixFindings={false} - startFixFindings={() => {}} - handoffLabels={{ fixFindings: "Fix review findings" }} - canWriteGuide={false} - guideRunning={false} - guideWritten={false} - writeGuide={() => {}} - pickableEnvironments={[]} - actingEnvironmentId={environmentId} - onActingEnvironmentChange={() => {}} - can={() => false} - autoMergeArmed={false} - showsDraftToggle={false} - showsMergeNow={false} - showsAutoMerge={false} - showsMergeMethods={false} - allowedMergeMethods={[]} - selectedMergeMethod="squash" - onSelectMergeMethod={() => {}} - showsDeleteBranch={false} - environmentConnected={true} - canWriteSourceControl={allowed} - onMaintainerEditsSaved={onSaved} - confirmDeleteBranch={async () => {}} - pendingAction={null} - perform={async () => true} - openOnHostText="Open on GitHub" - copyReference={() => {}} - /> - ); - return { control, onSaved }; -} - -it("saves both directions without waiting for a refreshed detail", async () => { - const { control, onSaved } = setup(); - await using screen = await render(control()); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - expect(document.querySelector('[role="menuitemcheckbox"]')).not.toBeNull(); - const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); - await expect - .element(checkbox) - .toHaveAccessibleDescription( - "Maintainers can push to this branch. If your fork has workflows, this can also give access to its secrets.", - ); - await checkbox.click(); - await expect.element(checkbox).not.toBeChecked(); - await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); - await checkbox.click(); - await expect.element(checkbox).toBeChecked(); - expect(mocks.update.mock.calls.map(([input]) => input)).toEqual([ - { environmentId, input: { ...reference, maintainerCanModify: false } }, - { environmentId, input: { ...reference, maintainerCanModify: true } }, - ]); -}); - -it("locks the control while a save is in flight, including after reopening the menu", async () => { - const { control } = setup(); - let settle!: (value: { _tag: "Success" }) => void; - const pending = new Promise<{ _tag: "Success" }>((resolve) => { - settle = resolve; - }); - mocks.update.mockReturnValue(pending); - await using screen = await render(control()); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - const checkbox = page.getByRole("menuitemcheckbox"); - await checkbox.click(); - await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); - await expect.element(page.getByRole("status")).toHaveTextContent("Saving…"); - await userEvent.keyboard("{Escape}"); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); - expect(mocks.update).toHaveBeenCalledTimes(1); - settle({ _tag: "Success" }); - await expect.element(checkbox).not.toHaveAttribute("aria-disabled", "true"); -}); - -it("reports a failed save after closing the menu and lets the user retry", async () => { - const { control, onSaved } = setup(); - await using screen = await render(control()); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - const checkbox = page.getByRole("menuitemcheckbox"); - await checkbox.click(); - await expect.element(checkbox).not.toBeChecked(); - await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); - let settle!: (value: { _tag: "Failure" }) => void; - mocks.update.mockReturnValueOnce( - new Promise<{ _tag: "Failure" }>((resolve) => { - settle = resolve; - }), - ); - await checkbox.click(); - await userEvent.keyboard("{Escape}"); - settle({ _tag: "Failure" }); - await vi.waitFor(() => - expect(mocks.notify).toHaveBeenCalledWith({ - type: "error", - title: "Could not save maintainer access", - description: "Try again.", - }), - ); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - await expect.element(checkbox).not.toBeChecked(); - expect(onSaved).toHaveBeenCalledTimes(1); - await checkbox.click(); - await expect.element(checkbox).toBeChecked(); -}); - -it("blocks a session without source control write access", async () => { - const { control } = setup(); - await using screen = await render(control(false)); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - await expect.element(page.getByRole("menuitemcheckbox")).toHaveAttribute("aria-disabled", "true"); - expect(mocks.update).not.toHaveBeenCalled(); -}); - -it("keeps a second save locked when the first save's refresh arrives", async () => { - const { control, onSaved } = setup(); - await using screen = await render(control()); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - const checkbox = page.getByRole("menuitemcheckbox"); - await checkbox.click(); - await vi.waitFor(() => expect(onSaved).toHaveBeenCalledTimes(1)); - let settle!: (value: { _tag: "Success" }) => void; - mocks.update.mockReturnValueOnce( - new Promise<{ _tag: "Success" }>((resolve) => { - settle = resolve; - }), - ); - await checkbox.click(); - await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); - await screen.rerender(control(true, false)); - await expect.element(checkbox).toHaveAttribute("aria-disabled", "true"); - await expect.element(checkbox).toBeChecked(); - await userEvent.keyboard("{Enter}"); - expect(mocks.update).toHaveBeenCalledTimes(2); - settle({ _tag: "Success" }); - await expect.element(checkbox).not.toHaveAttribute("aria-disabled", "true"); - await expect.element(checkbox).toBeChecked(); - await screen.rerender(control(true, true)); - await screen.rerender(control(true, false)); - await expect.element(checkbox).not.toBeChecked(); -}); diff --git a/packages/client-runtime/src/pullRequestEditing.test.ts b/packages/client-runtime/src/pullRequestEditing.test.ts index e9dde2b1d62ff..1b473f4657def 100644 --- a/packages/client-runtime/src/pullRequestEditing.test.ts +++ b/packages/client-runtime/src/pullRequestEditing.test.ts @@ -10,7 +10,6 @@ import { describe, expect, it } from "vite-plus/test"; import { canEditPullRequestChangeRequest, canEditPullRequestComment, - canEditPullRequestMaintainerEdits, canResolvePullRequestThread, } from "./pullRequestEditing.ts"; @@ -142,21 +141,6 @@ describe("canEditPullRequestChangeRequest", () => { }); }); -describe("canEditPullRequestMaintainerEdits", () => { - it.each(["open", "closed", "merged"] as const)( - "only allows an open pull request when cached permissions allow edits: %s", - (state) => { - const detail = { - state, - capabilities: capabilities({ changeRequest: true, comment: true, maintainerEdits: true }), - viewerPermissions: permissions({ maintainerEdits: true }), - maintainerCanModify: false, - }; - expect(canEditPullRequestMaintainerEdits(detail)).toBe(state === "open"); - }, - ); -}); - describe("canEditPullRequestComment", () => { it("lets the reader rewrite their own remark on a line", () => { expect(canEditPullRequestComment(subject(), comment({ kind: "review-comment" }))).toBe(true); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index d1264f3a2a8e1..f995ab3294865 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -695,25 +695,23 @@ it.effect( ); layer("GitHubPullRequestApi.layer", (it) => { - it.effect.each([true, false])( - "sets maintainer edits to %s without rewriting PR text", - (enabled) => - Effect.gen(function* () { - mockedExecute.mockImplementation((call) => - Effect.succeed( - output( - encodeJson( - call.kind === "graphql" && matches(call, NODE_ID_QUERY) ? nodeIdAnswer("PR_7") : {}, - ), + it.effect("turns maintainer edits off without rewriting the pull request's text", () => + Effect.gen(function* () { + mockedExecute.mockImplementation((call) => + Effect.succeed( + output( + encodeJson( + call.kind === "graphql" && matches(call, NODE_ID_QUERY) ? nodeIdAnswer("PR_7") : {}, ), ), - ); - const api = yield* GitHubPullRequestApi.GitHubPullRequestApi; - yield* api.updatePullRequest({ ...pullRequestInput, maintainerCanModify: enabled }); - expect(variablesOf("mutation UpdatePullRequest")).toEqual([ - { pullRequestId: "PR_7", maintainerCanModify: enabled }, - ]); - }), + ), + ); + const api = yield* GitHubPullRequestApi.GitHubPullRequestApi; + yield* api.updatePullRequest({ ...pullRequestInput, maintainerCanModify: false }); + expect(variablesOf("mutation UpdatePullRequest")).toEqual([ + { pullRequestId: "PR_7", maintainerCanModify: false }, + ]); + }), ); it.effect.each(["mixed", "merges", "unavailable"] as const)( diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index 4585dc79eec13..31d71a89f31f7 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -403,20 +403,22 @@ describe("pull request detail decoding", () => { [{ state: "CLOSED" }, false], [{ headRepositoryOwner: { __typename: "Organization", login: "team" } }, false], [{ headRepository: null }, false], - [{ headRef: null }, false], - ])("limits maintainer edits to the author's open personal fork: %j", (overrides, allowed) => { - const detail = detailOf([], { - maintainerCanModify: false, - isCrossRepository: true, - viewerDidAuthor: true, - headRepositoryOwner: { __typename: "User", login: "octocat" }, - headRepository: { nameWithOwner: "octocat/t3code", viewerPermission: "ADMIN" }, - headRef: { id: "REF_7" }, - ...overrides, - }); - expect(detail.maintainerCanModify).toBe(false); - expect(detail.canEditMaintainerEdits).toBe(allowed); - }); + ])( + "reads maintainer edits and lets only the author of an open personal fork change them: %j", + (overrides, allowed) => { + const detail = detailOf([], { + maintainerCanModify: false, + isCrossRepository: true, + viewerDidAuthor: true, + headRepositoryOwner: { __typename: "User", login: "octocat" }, + headRepository: { nameWithOwner: "octocat/t3code", viewerPermission: "ADMIN" }, + headRef: { id: "REF_7" }, + ...overrides, + }); + expect(detail.maintainerCanModify).toBe(false); + expect(detail.canEditMaintainerEdits).toBe(allowed); + }, + ); const detailJson = JSON.stringify({ comments: [{ id: "c1", body: "second", createdAt: "2026-07-04T00:00:00Z" }], From 8ed48fd5e96c3fe18da8783d437774edf456878f Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 02:42:46 +0000 Subject: [PATCH 07/13] refactor(pull-requests): maintainer edits follow the existing action patterns Web keeps the toggle's pending choice in PullRequestActionsMenu, keyed to the value the host reported when it was made, instead of a separate component that synced state during render and kept the whole menu mounted. Mobile reads visibility from usePullRequestPermissions like canEditChangeRequest and saves in the usePullRequestAction style. Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/PullRequestRouteScreen.tsx | 4 +- .../usePullRequestMaintainerEdits.ts | 61 +++++++------- .../usePullRequestPermissions.ts | 3 + .../pullRequest/PullRequestActionsMenu.tsx | 78 +++++++++++++++--- .../PullRequestMaintainerEdits.tsx | 79 ------------------- packages/contracts/src/pullRequest.ts | 4 +- .../src/server/pullRequestMutations.graphql | 6 +- 7 files changed, 108 insertions(+), 127 deletions(-) delete mode 100644 apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index fa13cf10439fb..051f1b9c3ec91 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -524,7 +524,7 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index ec78877a55035..329fddb796e9f 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -1,5 +1,5 @@ -import { useAtomRefresh, useAtomValue } from "@effect/atom-react"; -import { canEditPullRequestMaintainerEdits } from "@t3tools/client-runtime/pull-request-editing"; +import { useAtomRefresh } from "@effect/atom-react"; +import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { PullRequestDetail } from "@t3tools/contracts"; import { AsyncResult } from "effect/reactivity"; import { useRef, useState } from "react"; @@ -7,55 +7,58 @@ import * as Dialog from "../../components/ui/Dialog"; import { composerPullRequests } from "../../state/pull-requests"; import { useAtomCommand } from "../../state/use-atom-command"; import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff"; -import { usePullRequestConnected } from "./usePullRequestConnected"; +import { usePullRequestPermissions } from "./usePullRequestPermissions"; export function usePullRequestMaintainerEdits( params: PullRequestParams, pr: PullRequestDetail | null, ) { const target = pullRequestTarget(params); - const connected = usePullRequestConnected(target.environmentId); - const canUpdate = useAtomValue(composerPullRequests.update.permissionAtom(target.environmentId)); - const allowed = connected && canUpdate; + const permissions = usePullRequestPermissions(params, pr); const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); - const refresh = useAtomRefresh(composerPullRequests.detail(target)); - const key = `${pr?.url}:${pr?.maintainerCanModify}`; + const refreshDetail = useAtomRefresh(composerPullRequests.detail(target)); + const pending = useRef(false); const [saving, setSaving] = useState(false); - const [selection, setSelection] = useState({ key, value: pr?.maintainerCanModify ?? false }); + // The choice holds only while the host still reports the value it was made against. + const [choice, setChoice] = useState<{ + readonly url: string; + readonly observed: boolean | undefined; + readonly value: boolean; + } | null>(null); + const observed = pr?.maintainerCanModify; const value = - selection.key === key || saving ? selection.value : (pr?.maintainerCanModify ?? false); - if (selection.key !== key) { - setSelection({ key, value }); - } - const inFlight = useRef(false); - const visible = pr !== null && canEditPullRequestMaintainerEdits(pr); + choice !== null && choice.url === pr?.url && choice.observed === observed + ? choice.value + : observed === true; const save = async (next: boolean) => { - if (!visible || !allowed || inFlight.current) return; - const previous = value; - inFlight.current = true; + if (pending.current || pr === null || !permissions.canEditMaintainerEdits) return; + pending.current = true; setSaving(true); - setSelection({ key, value: next }); - const result = await update({ + setChoice({ url: pr.url, observed, value: next }); + const response = await update({ ...target, input: { ...target.input, maintainerCanModify: next }, }); - inFlight.current = false; + pending.current = false; setSaving(false); - if (AsyncResult.isFailure(result)) { - setSelection({ key, value: previous }); - Dialog.alert("Could not save maintainer access", "Try again."); + if (AsyncResult.isFailure(response)) { + setChoice(null); + const cause = squashAtomCommandFailure(response); + Dialog.alert( + "Could not save maintainer access", + cause instanceof Error ? cause.message : "Try again.", + ); return; } - refresh(); + refreshDetail(); }; - return { - visible, + visible: permissions.canEditMaintainerEdits, value, - disabled: !allowed || saving, + saving, subtitle: saving ? "Saving…" - : "Maintainers can push to this branch and access fork workflow secrets.", + : "Maintainers can push to this branch and may get access to fork workflow secrets.", toggle: () => void save(!value), }; } diff --git a/apps/mobile/src/features/pull-requests/usePullRequestPermissions.ts b/apps/mobile/src/features/pull-requests/usePullRequestPermissions.ts index 741b9c2ad0828..28a2bbb04cc97 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestPermissions.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestPermissions.ts @@ -2,6 +2,7 @@ import { useAtomValue } from "@effect/atom-react"; import { canEditPullRequestChangeRequest, canEditPullRequestComment, + canEditPullRequestMaintainerEdits, } from "@t3tools/client-runtime/pull-request-editing"; import type { PullRequestComment, PullRequestDetail } from "@t3tools/contracts"; import { usePullRequestConnected } from "./usePullRequestConnected"; @@ -49,6 +50,8 @@ export function usePullRequestPermissions(params: PullRequestParams, pr: PullReq canWrite: connected && canWrite, canEditChangeRequest: connected && canUpdate && pr !== null && canEditPullRequestChangeRequest(pr), + canEditMaintainerEdits: + connected && canUpdate && pr !== null && canEditPullRequestMaintainerEdits(pr), canEditComment: (comment: Pick) => connected && canUpdateComment && pr !== null && canEditPullRequestComment(pr, comment), canReact: connected && canSetReaction && pr?.capabilities.reactions === true, diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index 12240af538624..1bee23d2f663d 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -21,13 +21,17 @@ import { SparklesIcon, Trash2Icon, } from "lucide-react"; +import { useId, useState } from "react"; import { shortcutLabelForCommand } from "~/keybindings"; import { readLocalApi } from "~/localApi"; +import { pullRequestEnvironment } from "~/state/pullRequests"; import { primaryServerKeybindingsAtom } from "~/state/server"; +import { useAtomCommand } from "~/state/use-atom-command"; import { Button } from "../ui/button"; import { Menu, + MenuCheckboxItem, MenuItem, MenuPopup, MenuRadioGroup, @@ -37,6 +41,7 @@ import { MenuTrigger, } from "../ui/menu"; import { RefreshIcon } from "../ui/refresh-icon"; +import { toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { ActOnEnvironmentPicker, type PerformPullRequestAction } from "./PullRequestDetailActions"; import { @@ -45,7 +50,6 @@ import { } from "./pullRequestDetail.logic"; import { PullRequestGlyph } from "./pullRequestIcons"; import type { PickableEnvironment } from "./pullRequestProjectAssignment.logic"; -import { PullRequestMaintainerEdits } from "./PullRequestMaintainerEdits"; import { PullRequestThreadLinks } from "./PullRequestThreadLinks"; import { useInlineActionConfirm } from "./useInlineActionConfirm"; @@ -147,7 +151,44 @@ export function PullRequestActionsMenu({ const keybindings = useAtomValue(primaryServerKeybindingsAtom); const inlineConfirm = useInlineActionConfirm(pullRequestKey); const actionPending = pendingAction !== null; - const showsMaintainerEdits = canEditPullRequestMaintainerEdits(detail); + const maintainerEditsHintId = useId(); + const updatePullRequest = useAtomCommand(pullRequestEnvironment.update, { + reportFailure: false, + }); + // The choice holds only while the host still reports the value it was made against. + const [maintainerEditsChoice, setMaintainerEditsChoice] = useState<{ + readonly pullRequestKey: string; + readonly observed: boolean | undefined; + readonly value: boolean; + } | null>(null); + const [maintainerEditsSaving, setMaintainerEditsSaving] = useState(false); + const maintainerCanModify = + maintainerEditsChoice?.pullRequestKey === pullRequestKey && + maintainerEditsChoice.observed === detail.maintainerCanModify + ? maintainerEditsChoice.value + : detail.maintainerCanModify; + const canSaveMaintainerEdits = + canWriteSourceControl && environmentConnected && !maintainerEditsSaving; + const saveMaintainerEdits = async (value: boolean) => { + if (!canSaveMaintainerEdits) return; + setMaintainerEditsChoice({ pullRequestKey, observed: detail.maintainerCanModify, value }); + setMaintainerEditsSaving(true); + const result = await updatePullRequest({ + environmentId, + input: { ...reference, maintainerCanModify: value }, + }); + setMaintainerEditsSaving(false); + if (result._tag === "Failure") { + setMaintainerEditsChoice(null); + toastManager.add({ + type: "error", + title: "Could not save maintainer access", + description: "Try again.", + }); + return; + } + onMaintainerEditsSaved(); + }; return ( @@ -180,7 +221,7 @@ export function PullRequestActionsMenu({ : `More ${terminology.singular} actions`} - + ) : null} - {showsMaintainerEdits && detail.maintainerCanModify !== undefined ? ( + {canEditPullRequestMaintainerEdits(detail) ? ( <> - + void saveMaintainerEdits(value)} + > + + Allow edits from maintainers + + Maintainers can push to this branch. If your fork has workflows, this can also + give access to its secrets. + + {maintainerEditsSaving ? ( + + Saving… + + ) : null} + + ) : null} diff --git a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx b/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx deleted file mode 100644 index 88ff6e43715d7..0000000000000 --- a/apps/web/src/components/pullRequest/PullRequestMaintainerEdits.tsx +++ /dev/null @@ -1,79 +0,0 @@ -import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; -import { useId, useRef, useState } from "react"; - -import { pullRequestEnvironment } from "~/state/pullRequests"; -import { useAtomCommand } from "~/state/use-atom-command"; -import { MenuCheckboxItem } from "../ui/menu"; -import { toastManager } from "../ui/toast"; - -export function PullRequestMaintainerEdits({ - environmentId, - reference, - enabled, - allowed, - onSaved, -}: { - environmentId: EnvironmentId; - reference: PullRequestRef; - enabled: boolean; - allowed: boolean; - onSaved: () => void; -}) { - const id = useId(); - const update = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false }); - const [value, setValue] = useState(enabled); - const [saving, setSaving] = useState(false); - const [observedValue, setObservedValue] = useState(enabled); - if (observedValue !== enabled) { - setObservedValue(enabled); - if (!saving) setValue(enabled); - } - const inFlight = useRef(false); - const save = async (next: boolean) => { - if (!allowed || inFlight.current) return; - const previous = value; - inFlight.current = true; - setSaving(true); - setValue(next); - const result = await update({ - environmentId, - input: { ...reference, maintainerCanModify: next }, - }); - inFlight.current = false; - setSaving(false); - if (result._tag === "Failure") { - setValue(previous); - toastManager.add({ - type: "error", - title: "Could not save maintainer access", - description: "Try again.", - }); - return; - } - onSaved(); - }; - - return ( - void save(next)} - > - - Allow edits from maintainers - - Maintainers can push to this branch. If your fork has workflows, this can also give access - to its secrets. - - {saving ? ( - - Saving… - - ) : null} - - - ); -} diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 3b11209f46dae..64111ab4742b1 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -242,9 +242,9 @@ export const PullRequestReviewCapabilities = Schema.Struct({ export type PullRequestReviewCapabilities = typeof PullRequestReviewCapabilities.Type; export const PullRequestEditCapabilities = Schema.Struct({ - maintainerEdits: Schema.optional(Schema.Boolean), changeRequest: Schema.Boolean, comment: Schema.Boolean, + maintainerEdits: Schema.optional(Schema.Boolean), }); export type PullRequestEditCapabilities = typeof PullRequestEditCapabilities.Type; @@ -746,10 +746,10 @@ export const PullRequestCommentInput = Schema.Struct({ export type PullRequestCommentInput = typeof PullRequestCommentInput.Type; export const PullRequestUpdateInput = Schema.Struct({ - maintainerCanModify: Schema.optional(Schema.Boolean), ...PullRequestRef.fields, title: Schema.optional(TrimmedNonEmptyString.check(Schema.isMaxLength(1024))), body: Schema.optional(Schema.String.check(Schema.isMaxLength(65_536))), + maintainerCanModify: Schema.optional(Schema.Boolean), }); export type PullRequestUpdateInput = typeof PullRequestUpdateInput.Type; diff --git a/packages/source-control-github/src/server/pullRequestMutations.graphql b/packages/source-control-github/src/server/pullRequestMutations.graphql index 9ec83192557c0..3e2d5b3133ad5 100644 --- a/packages/source-control-github/src/server/pullRequestMutations.graphql +++ b/packages/source-control-github/src/server/pullRequestMutations.graphql @@ -41,9 +41,9 @@ mutation UnresolveReviewThread($threadId: ID!) { } } -# Rewrites the pull request's own words. Both are nullable so that one document serves a change to -# the title, to the description, or to the two together: a variable the request does not send puts -# no entry in the input at all, which leaves that field as it was rather than clearing it. +# Rewrites the pull request's own words or its maintainer-edit setting. Every field is nullable so +# that one document serves any combination: a variable the request does not send puts no entry in +# the input at all, which leaves that field as it was rather than clearing it. mutation UpdatePullRequest( $pullRequestId: ID! $title: String From 05d1a23647770c206883bd4faadbd9aadc9ebd92 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 04:33:02 +0000 Subject: [PATCH 08/13] fix(pull-requests): a saved maintainer-edits choice no longer masks a later host change Co-Authored-By: Claude Opus 5.5 --- .../usePullRequestMaintainerEdits.ts | 11 ++++++----- .../PullRequestActionsMenu.browser.test.tsx | 15 +++++++++++++++ .../pullRequest/PullRequestActionsMenu.tsx | 14 +++++++++++--- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index 329fddb796e9f..d0bfa1bb16192 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -19,17 +19,18 @@ export function usePullRequestMaintainerEdits( const refreshDetail = useAtomRefresh(composerPullRequests.detail(target)); const pending = useRef(false); const [saving, setSaving] = useState(false); - // The choice holds only while the host still reports the value it was made against. const [choice, setChoice] = useState<{ readonly url: string; readonly observed: boolean | undefined; readonly value: boolean; } | null>(null); const observed = pr?.maintainerCanModify; - const value = - choice !== null && choice.url === pr?.url && choice.observed === observed - ? choice.value - : observed === true; + // A choice ends at the first host value that differs from the one it was made against, so a + // later change made elsewhere is never masked by it. + if (choice !== null && !saving && (choice.url !== pr?.url || choice.observed !== observed)) { + setChoice(null); + } + const value = choice !== null && choice.url === pr?.url ? choice.value : observed === true; const save = async (next: boolean) => { if (pending.current || pr === null || !permissions.canEditMaintainerEdits) return; pending.current = true; diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx index 0d8b7d22c6682..b86c87976ec80 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -138,6 +138,21 @@ it("saves maintainer edits, and restores the previous value with an error when s ); }); +it("keeps a saved choice through a stale refresh, then follows the host", async () => { + const control = setup(); + await using screen = await render(control()); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + await checkbox.click(); + await expect.element(page.getByRole("status")).not.toBeInTheDocument(); + await screen.rerender(control({ ...openDetail })); + await expect.element(checkbox).not.toBeChecked(); + await screen.rerender(control({ ...openDetail, maintainerCanModify: false })); + await expect.element(checkbox).not.toBeChecked(); + await screen.rerender(control({ ...openDetail, maintainerCanModify: true })); + await expect.element(checkbox).toBeChecked(); +}); + it.each<[string, Detail]>([ ["closed", { ...openDetail, state: "closed" }], [ diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index 1bee23d2f663d..90d003204870c 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -155,16 +155,24 @@ export function PullRequestActionsMenu({ const updatePullRequest = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false, }); - // The choice holds only while the host still reports the value it was made against. const [maintainerEditsChoice, setMaintainerEditsChoice] = useState<{ readonly pullRequestKey: string; readonly observed: boolean | undefined; readonly value: boolean; } | null>(null); const [maintainerEditsSaving, setMaintainerEditsSaving] = useState(false); + // A choice ends at the first host value that differs from the one it was made against, so a + // later change made elsewhere is never masked by it. + if ( + maintainerEditsChoice !== null && + !maintainerEditsSaving && + (maintainerEditsChoice.pullRequestKey !== pullRequestKey || + maintainerEditsChoice.observed !== detail.maintainerCanModify) + ) { + setMaintainerEditsChoice(null); + } const maintainerCanModify = - maintainerEditsChoice?.pullRequestKey === pullRequestKey && - maintainerEditsChoice.observed === detail.maintainerCanModify + maintainerEditsChoice?.pullRequestKey === pullRequestKey ? maintainerEditsChoice.value : detail.maintainerCanModify; const canSaveMaintainerEdits = From c7eaa64680836e56a5549ed6ebe25563e8cb384c Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 05:29:01 +0000 Subject: [PATCH 09/13] fix(pull-requests): a failed maintainer-edits save restores the previous choice Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/usePullRequestMaintainerEdits.ts | 3 ++- .../PullRequestActionsMenu.browser.test.tsx | 14 ++++++++++++++ .../pullRequest/PullRequestActionsMenu.tsx | 3 ++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index d0bfa1bb16192..199a47a01662e 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -34,6 +34,7 @@ export function usePullRequestMaintainerEdits( const save = async (next: boolean) => { if (pending.current || pr === null || !permissions.canEditMaintainerEdits) return; pending.current = true; + const previousChoice = choice; setSaving(true); setChoice({ url: pr.url, observed, value: next }); const response = await update({ @@ -43,7 +44,7 @@ export function usePullRequestMaintainerEdits( pending.current = false; setSaving(false); if (AsyncResult.isFailure(response)) { - setChoice(null); + setChoice(previousChoice); const cause = squashAtomCommandFailure(response); Dialog.alert( "Could not save maintainer access", diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx index b86c87976ec80..56a55a7e6cdc4 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -138,6 +138,20 @@ it("saves maintainer edits, and restores the previous value with an error when s ); }); +it("keeps the saved value when a follow-up save fails before the host refreshes", async () => { + const control = setup(); + await using screen = await render(control()); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + await checkbox.click(); + await expect.element(page.getByRole("status")).not.toBeInTheDocument(); + mocks.update.mockResolvedValueOnce({ _tag: "Failure" }); + await checkbox.click(); + await expect.element(page.getByRole("status")).not.toBeInTheDocument(); + expect(mocks.notify).toHaveBeenCalledOnce(); + await expect.element(checkbox).not.toBeChecked(); +}); + it("keeps a saved choice through a stale refresh, then follows the host", async () => { const control = setup(); await using screen = await render(control()); diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index 90d003204870c..a2373110d0239 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -179,6 +179,7 @@ export function PullRequestActionsMenu({ canWriteSourceControl && environmentConnected && !maintainerEditsSaving; const saveMaintainerEdits = async (value: boolean) => { if (!canSaveMaintainerEdits) return; + const previousChoice = maintainerEditsChoice; setMaintainerEditsChoice({ pullRequestKey, observed: detail.maintainerCanModify, value }); setMaintainerEditsSaving(true); const result = await updatePullRequest({ @@ -187,7 +188,7 @@ export function PullRequestActionsMenu({ }); setMaintainerEditsSaving(false); if (result._tag === "Failure") { - setMaintainerEditsChoice(null); + setMaintainerEditsChoice(previousChoice); toastManager.add({ type: "error", title: "Could not save maintainer access", From 03ee95a90d7a22951718c62d9510030961b24771 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 06:12:17 +0000 Subject: [PATCH 10/13] refactor(pull-requests): one pending-choice rule for maintainer edits on web and mobile The rule that keeps a pending "Allow edits from maintainers" choice through a stale refresh lived in both clients. It is now maintainerEditsChoiceInForce in client-runtime, tested once. Web's failure toast keeps the host's reason, mobile uses the web wording, and the mobile hook reuses the screen's permissions. Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/PullRequestRouteScreen.tsx | 6 ++- .../usePullRequestMaintainerEdits.ts | 30 +++++++------ .../PullRequestActionsMenu.browser.test.tsx | 21 +++++++--- .../pullRequest/PullRequestActionsMenu.tsx | 42 +++++++++---------- .../src/pullRequestEditing.test.ts | 15 +++++++ .../client-runtime/src/pullRequestEditing.ts | 17 ++++++++ 6 files changed, 87 insertions(+), 44 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 06f040fab9677..80391ef4060eb 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -417,7 +417,11 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps pr && diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index 199a47a01662e..4f1e227d049ca 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -1,4 +1,8 @@ import { useAtomRefresh } from "@effect/atom-react"; +import { + maintainerEditsChoiceInForce, + type MaintainerEditsChoice, +} from "@t3tools/client-runtime/pull-request-editing"; import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { PullRequestDetail } from "@t3tools/contracts"; import { AsyncResult } from "effect/reactivity"; @@ -7,36 +11,30 @@ import * as Dialog from "../../components/ui/Dialog"; import { composerPullRequests } from "../../state/pull-requests"; import { useAtomCommand } from "../../state/use-atom-command"; import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff"; -import { usePullRequestPermissions } from "./usePullRequestPermissions"; export function usePullRequestMaintainerEdits( params: PullRequestParams, pr: PullRequestDetail | null, + canEdit: boolean, ) { const target = pullRequestTarget(params); - const permissions = usePullRequestPermissions(params, pr); const update = useAtomCommand(composerPullRequests.update, { reportFailure: false }); const refreshDetail = useAtomRefresh(composerPullRequests.detail(target)); const pending = useRef(false); const [saving, setSaving] = useState(false); - const [choice, setChoice] = useState<{ - readonly url: string; - readonly observed: boolean | undefined; - readonly value: boolean; - } | null>(null); + const [choice, setChoice] = useState(null); const observed = pr?.maintainerCanModify; - // A choice ends at the first host value that differs from the one it was made against, so a - // later change made elsewhere is never masked by it. - if (choice !== null && !saving && (choice.url !== pr?.url || choice.observed !== observed)) { - setChoice(null); + const kept = maintainerEditsChoiceInForce(choice, pr?.url ?? "", observed, saving); + if (kept !== choice) { + setChoice(kept); } - const value = choice !== null && choice.url === pr?.url ? choice.value : observed === true; + const value = kept?.value ?? observed === true; const save = async (next: boolean) => { - if (pending.current || pr === null || !permissions.canEditMaintainerEdits) return; + if (pending.current || pr === null || !canEdit) return; pending.current = true; const previousChoice = choice; setSaving(true); - setChoice({ url: pr.url, observed, value: next }); + setChoice({ key: pr.url, observed, value: next }); const response = await update({ ...target, input: { ...target.input, maintainerCanModify: next }, @@ -55,12 +53,12 @@ export function usePullRequestMaintainerEdits( refreshDetail(); }; return { - visible: permissions.canEditMaintainerEdits, + visible: canEdit, value, saving, subtitle: saving ? "Saving…" - : "Maintainers can push to this branch and may get access to fork workflow secrets.", + : "Maintainers can push to this branch. If your fork has workflows, this can also give access to its secrets.", toggle: () => void save(!value), }; } diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx index 56a55a7e6cdc4..a226a14e26c25 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -1,11 +1,13 @@ import "~/index.css"; import { EnvironmentId, ProjectId } from "@t3tools/contracts"; import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients"; +import { Cause } from "effect"; import { expect, it, vi } from "vite-plus/test"; import { page } from "vite-plus/test/browser"; const mocks = vi.hoisted(() => ({ - update: vi.fn<(input: unknown) => Promise<{ _tag: "Success" | "Failure" }>>(), + update: + vi.fn<(input: unknown) => Promise<{ _tag: "Success" } | { _tag: "Failure"; cause: unknown }>>(), notify: vi.fn(), })); vi.mock( @@ -114,9 +116,9 @@ function setup() { it("saves maintainer edits, and restores the previous value with an error when saving fails", async () => { const control = setup(); - let settle!: (value: { _tag: "Failure" }) => void; + let settle!: (value: { _tag: "Failure"; cause: unknown }) => void; mocks.update.mockReturnValueOnce( - new Promise<{ _tag: "Failure" }>((resolve) => { + new Promise((resolve) => { settle = resolve; }), ); @@ -131,10 +133,17 @@ it("saves maintainer edits, and restores the previous value with an error when s environmentId, input: { ...reference, maintainerCanModify: false }, }); - settle({ _tag: "Failure" }); + settle({ + _tag: "Failure", + cause: Cause.fail(new Error("Resource not accessible by integration")), + }); await expect.element(checkbox).toBeChecked(); expect(mocks.notify).toHaveBeenCalledWith( - expect.objectContaining({ type: "error", title: "Could not save maintainer access" }), + expect.objectContaining({ + type: "error", + title: "Could not save maintainer access", + description: "Resource not accessible by integration", + }), ); }); @@ -145,7 +154,7 @@ it("keeps the saved value when a follow-up save fails before the host refreshes" const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); await checkbox.click(); await expect.element(page.getByRole("status")).not.toBeInTheDocument(); - mocks.update.mockResolvedValueOnce({ _tag: "Failure" }); + mocks.update.mockResolvedValueOnce({ _tag: "Failure", cause: Cause.fail("refused") }); await checkbox.click(); await expect.element(page.getByRole("status")).not.toBeInTheDocument(); expect(mocks.notify).toHaveBeenCalledOnce(); diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index a2373110d0239..e4835c229a2c1 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -1,6 +1,11 @@ import { useAtomValue } from "@effect/atom-react"; -import { canEditPullRequestMaintainerEdits } from "@t3tools/client-runtime/pull-request-editing"; +import { + canEditPullRequestMaintainerEdits, + maintainerEditsChoiceInForce, + type MaintainerEditsChoice, +} from "@t3tools/client-runtime/pull-request-editing"; import type { ChangeRequestTerminology } from "@t3tools/client-runtime/source-control-clients"; +import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestAction, @@ -47,6 +52,7 @@ import { ActOnEnvironmentPicker, type PerformPullRequestAction } from "./PullReq import { PULL_REQUEST_MERGE_METHOD_LABELS, pullRequestActionMenuHasGroup, + readableFailure, } from "./pullRequestDetail.logic"; import { PullRequestGlyph } from "./pullRequestIcons"; import type { PickableEnvironment } from "./pullRequestProjectAssignment.logic"; @@ -155,32 +161,26 @@ export function PullRequestActionsMenu({ const updatePullRequest = useAtomCommand(pullRequestEnvironment.update, { reportFailure: false, }); - const [maintainerEditsChoice, setMaintainerEditsChoice] = useState<{ - readonly pullRequestKey: string; - readonly observed: boolean | undefined; - readonly value: boolean; - } | null>(null); + const [maintainerEditsChoice, setMaintainerEditsChoice] = useState( + null, + ); const [maintainerEditsSaving, setMaintainerEditsSaving] = useState(false); - // A choice ends at the first host value that differs from the one it was made against, so a - // later change made elsewhere is never masked by it. - if ( - maintainerEditsChoice !== null && - !maintainerEditsSaving && - (maintainerEditsChoice.pullRequestKey !== pullRequestKey || - maintainerEditsChoice.observed !== detail.maintainerCanModify) - ) { - setMaintainerEditsChoice(null); + const maintainerEditsChoiceKept = maintainerEditsChoiceInForce( + maintainerEditsChoice, + pullRequestKey, + detail.maintainerCanModify, + maintainerEditsSaving, + ); + if (maintainerEditsChoiceKept !== maintainerEditsChoice) { + setMaintainerEditsChoice(maintainerEditsChoiceKept); } - const maintainerCanModify = - maintainerEditsChoice?.pullRequestKey === pullRequestKey - ? maintainerEditsChoice.value - : detail.maintainerCanModify; + const maintainerCanModify = maintainerEditsChoiceKept?.value ?? detail.maintainerCanModify; const canSaveMaintainerEdits = canWriteSourceControl && environmentConnected && !maintainerEditsSaving; const saveMaintainerEdits = async (value: boolean) => { if (!canSaveMaintainerEdits) return; const previousChoice = maintainerEditsChoice; - setMaintainerEditsChoice({ pullRequestKey, observed: detail.maintainerCanModify, value }); + setMaintainerEditsChoice({ key: pullRequestKey, observed: detail.maintainerCanModify, value }); setMaintainerEditsSaving(true); const result = await updatePullRequest({ environmentId, @@ -192,7 +192,7 @@ export function PullRequestActionsMenu({ toastManager.add({ type: "error", title: "Could not save maintainer access", - description: "Try again.", + description: readableFailure(squashAtomCommandFailure(result), "Try again."), }); return; } diff --git a/packages/client-runtime/src/pullRequestEditing.test.ts b/packages/client-runtime/src/pullRequestEditing.test.ts index 1b473f4657def..ca6207a5a2f33 100644 --- a/packages/client-runtime/src/pullRequestEditing.test.ts +++ b/packages/client-runtime/src/pullRequestEditing.test.ts @@ -11,6 +11,7 @@ import { canEditPullRequestChangeRequest, canEditPullRequestComment, canResolvePullRequestThread, + maintainerEditsChoiceInForce, } from "./pullRequestEditing.ts"; type Subject = Pick; @@ -202,3 +203,17 @@ describe("canResolvePullRequestThread", () => { ).toBe(false); }); }); + +describe("maintainerEditsChoiceInForce", () => { + const choice = { key: "pr-7", observed: true, value: false }; + + it.each([ + ["keeps it through a refresh that still shows the old host value", "pr-7", true, false, choice], + ["drops it once the host shows a different value", "pr-7", false, false, null], + ["drops it on another pull request", "pr-8", true, false, null], + ["drops it on another pull request while saving", "pr-8", true, true, null], + ["keeps it while a save is in flight whatever the host shows", "pr-7", false, true, choice], + ] as const)("%s", (_, key, observed, saving, expected) => { + expect(maintainerEditsChoiceInForce(choice, key, observed, saving)).toBe(expected); + }); +}); diff --git a/packages/client-runtime/src/pullRequestEditing.ts b/packages/client-runtime/src/pullRequestEditing.ts index f9534e821eccb..3d48a98991a99 100644 --- a/packages/client-runtime/src/pullRequestEditing.ts +++ b/packages/client-runtime/src/pullRequestEditing.ts @@ -39,6 +39,23 @@ export function canEditPullRequestMaintainerEdits( ); } +export interface MaintainerEditsChoice { + readonly key: string; + readonly observed: boolean | undefined; + readonly value: boolean; +} + +/** A choice ends at the first host value that differs from the one it was made against. */ +export function maintainerEditsChoiceInForce( + choice: MaintainerEditsChoice | null, + key: string, + observed: boolean | undefined, + saving: boolean, +): MaintainerEditsChoice | null { + if (choice === null || choice.key !== key) return null; + return saving || choice.observed === observed ? choice : null; +} + /** Hosts do not support rewriting review summaries through comment mutations. */ export function canEditPullRequestComment( detail: EditingSubject, From affe7e2abb3ccd58bcc7d6c79f163d58c038d2eb Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 09:48:16 +0000 Subject: [PATCH 11/13] test(pull-requests): cut the maintainer-edit takeover's tests to the minimum Co-Authored-By: Claude Opus 5.5 --- .../PullRequestActionsMenu.browser.test.tsx | 40 +++++++------------ .../src/pullRequestEditing.test.ts | 15 ------- 2 files changed, 15 insertions(+), 40 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx index a226a14e26c25..241972108a663 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -114,31 +114,35 @@ function setup() { ); } -it("saves maintainer edits, and restores the previous value with an error when saving fails", async () => { +it("saves maintainer edits, locks while saving, and restores the saved value when a save fails", async () => { const control = setup(); - let settle!: (value: { _tag: "Failure"; cause: unknown }) => void; - mocks.update.mockReturnValueOnce( - new Promise((resolve) => { - settle = resolve; - }), - ); await using screen = await render(control()); await screen.getByRole("button", { name: "More pull request actions" }).click(); const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); await expect.element(checkbox).toBeChecked(); await checkbox.click(); - await expect.element(checkbox).not.toBeChecked(); - await expect.element(page.getByRole("status")).toHaveTextContent("Saving…"); + await expect.element(page.getByRole("status")).not.toBeInTheDocument(); expect(mocks.update).toHaveBeenCalledExactlyOnceWith({ environmentId, input: { ...reference, maintainerCanModify: false }, }); + + let settle!: (value: { _tag: "Failure"; cause: unknown }) => void; + mocks.update.mockReturnValueOnce( + new Promise((resolve) => { + settle = resolve; + }), + ); + await checkbox.click(); + await expect.element(checkbox).toBeChecked(); + await expect.element(checkbox).toBeDisabled(); + await expect.element(page.getByRole("status")).toHaveTextContent("Saving…"); settle({ _tag: "Failure", cause: Cause.fail(new Error("Resource not accessible by integration")), }); - await expect.element(checkbox).toBeChecked(); - expect(mocks.notify).toHaveBeenCalledWith( + await expect.element(checkbox).not.toBeChecked(); + expect(mocks.notify).toHaveBeenCalledExactlyOnceWith( expect.objectContaining({ type: "error", title: "Could not save maintainer access", @@ -147,20 +151,6 @@ it("saves maintainer edits, and restores the previous value with an error when s ); }); -it("keeps the saved value when a follow-up save fails before the host refreshes", async () => { - const control = setup(); - await using screen = await render(control()); - await screen.getByRole("button", { name: "More pull request actions" }).click(); - const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); - await checkbox.click(); - await expect.element(page.getByRole("status")).not.toBeInTheDocument(); - mocks.update.mockResolvedValueOnce({ _tag: "Failure", cause: Cause.fail("refused") }); - await checkbox.click(); - await expect.element(page.getByRole("status")).not.toBeInTheDocument(); - expect(mocks.notify).toHaveBeenCalledOnce(); - await expect.element(checkbox).not.toBeChecked(); -}); - it("keeps a saved choice through a stale refresh, then follows the host", async () => { const control = setup(); await using screen = await render(control()); diff --git a/packages/client-runtime/src/pullRequestEditing.test.ts b/packages/client-runtime/src/pullRequestEditing.test.ts index ca6207a5a2f33..1b473f4657def 100644 --- a/packages/client-runtime/src/pullRequestEditing.test.ts +++ b/packages/client-runtime/src/pullRequestEditing.test.ts @@ -11,7 +11,6 @@ import { canEditPullRequestChangeRequest, canEditPullRequestComment, canResolvePullRequestThread, - maintainerEditsChoiceInForce, } from "./pullRequestEditing.ts"; type Subject = Pick; @@ -203,17 +202,3 @@ describe("canResolvePullRequestThread", () => { ).toBe(false); }); }); - -describe("maintainerEditsChoiceInForce", () => { - const choice = { key: "pr-7", observed: true, value: false }; - - it.each([ - ["keeps it through a refresh that still shows the old host value", "pr-7", true, false, choice], - ["drops it once the host shows a different value", "pr-7", false, false, null], - ["drops it on another pull request", "pr-8", true, false, null], - ["drops it on another pull request while saving", "pr-8", true, true, null], - ["keeps it while a save is in flight whatever the host shows", "pr-7", false, true, choice], - ] as const)("%s", (_, key, observed, saving, expected) => { - expect(maintainerEditsChoiceInForce(choice, key, observed, saving)).toBe(expected); - }); -}); From b4aaaee3646e4a146af4f907551403654415cf85 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 28 Oct 2026 10:47:11 +0000 Subject: [PATCH 12/13] fix(pull-requests): a saved maintainer-edits choice yields to the host's next reading The saved choice stayed for as long as the host value matched the value it was made against. If maintainer edits were turned back on elsewhere before T3 read the disabled state, every later read matched, and the menu showed "off" until remount. The choice now ends at the first detail with a different observedAt after the save, then follows the host. While saving it tracks the latest reading, so a read that was already in flight and lands during the save still counts as stale. The post-save refresh rebuilds the detail atom, which interrupts any read still in flight. The server keys detail reads by a per-PR epoch that update bumps, so the next reading started after the write. Co-Authored-By: Claude Opus 5.5 --- .../usePullRequestMaintainerEdits.ts | 7 +++-- .../PullRequestActionsMenu.browser.test.tsx | 27 ++++++++++++++++--- .../pullRequest/PullRequestActionsMenu.tsx | 5 ++-- .../client-runtime/src/pullRequestEditing.ts | 12 ++++++--- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts index 4f1e227d049ca..d474b8e5d5462 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestMaintainerEdits.ts @@ -23,18 +23,17 @@ export function usePullRequestMaintainerEdits( const pending = useRef(false); const [saving, setSaving] = useState(false); const [choice, setChoice] = useState(null); - const observed = pr?.maintainerCanModify; - const kept = maintainerEditsChoiceInForce(choice, pr?.url ?? "", observed, saving); + const kept = maintainerEditsChoiceInForce(choice, pr?.url ?? "", pr?.observedAt, saving); if (kept !== choice) { setChoice(kept); } - const value = kept?.value ?? observed === true; + const value = kept?.value ?? pr?.maintainerCanModify === true; const save = async (next: boolean) => { if (pending.current || pr === null || !canEdit) return; pending.current = true; const previousChoice = choice; setSaving(true); - setChoice({ key: pr.url, observed, value: next }); + setChoice({ key: pr.url, observedAt: pr.observedAt, value: next }); const response = await update({ ...target, input: { ...target.input, maintainerCanModify: next }, diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx index 241972108a663..521d9c47f316e 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.browser.test.tsx @@ -44,6 +44,7 @@ const openDetail: Detail = { isDraft: false, headBranch: "feature", maintainerCanModify: true, + observedAt: 1, capabilities: { diff: true, comment: true, @@ -156,13 +157,33 @@ it("keeps a saved choice through a stale refresh, then follows the host", async await using screen = await render(control()); await screen.getByRole("button", { name: "More pull request actions" }).click(); const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + let settle!: (value: { _tag: "Success" }) => void; + mocks.update.mockReturnValueOnce( + new Promise((resolve) => { + settle = resolve; + }), + ); await checkbox.click(); + await screen.rerender(control({ ...openDetail, observedAt: 2 })); + settle({ _tag: "Success" }); await expect.element(page.getByRole("status")).not.toBeInTheDocument(); - await screen.rerender(control({ ...openDetail })); + await screen.rerender(control({ ...openDetail, observedAt: 2 })); + await expect.element(checkbox).not.toBeChecked(); + await screen.rerender(control({ ...openDetail, maintainerCanModify: false, observedAt: 3 })); await expect.element(checkbox).not.toBeChecked(); - await screen.rerender(control({ ...openDetail, maintainerCanModify: false })); + await screen.rerender(control({ ...openDetail, maintainerCanModify: true, observedAt: 4 })); + await expect.element(checkbox).toBeChecked(); +}); + +it("follows the host's first reading after a save, even when it matches the old value", async () => { + const control = setup(); + await using screen = await render(control()); + await screen.getByRole("button", { name: "More pull request actions" }).click(); + const checkbox = page.getByRole("menuitemcheckbox", { name: "Allow edits from maintainers" }); + await checkbox.click(); + await expect.element(page.getByRole("status")).not.toBeInTheDocument(); await expect.element(checkbox).not.toBeChecked(); - await screen.rerender(control({ ...openDetail, maintainerCanModify: true })); + await screen.rerender(control({ ...openDetail, observedAt: 2 })); await expect.element(checkbox).toBeChecked(); }); diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index e4835c229a2c1..4ab50135dc7da 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -115,6 +115,7 @@ export function PullRequestActionsMenu({ | "capabilities" | "viewerPermissions" | "maintainerCanModify" + | "observedAt" >; terminology: ChangeRequestTerminology; linkThreadRef: ScopedThreadRef | null; @@ -168,7 +169,7 @@ export function PullRequestActionsMenu({ const maintainerEditsChoiceKept = maintainerEditsChoiceInForce( maintainerEditsChoice, pullRequestKey, - detail.maintainerCanModify, + detail.observedAt, maintainerEditsSaving, ); if (maintainerEditsChoiceKept !== maintainerEditsChoice) { @@ -180,7 +181,7 @@ export function PullRequestActionsMenu({ const saveMaintainerEdits = async (value: boolean) => { if (!canSaveMaintainerEdits) return; const previousChoice = maintainerEditsChoice; - setMaintainerEditsChoice({ key: pullRequestKey, observed: detail.maintainerCanModify, value }); + setMaintainerEditsChoice({ key: pullRequestKey, observedAt: detail.observedAt, value }); setMaintainerEditsSaving(true); const result = await updatePullRequest({ environmentId, diff --git a/packages/client-runtime/src/pullRequestEditing.ts b/packages/client-runtime/src/pullRequestEditing.ts index 3d48a98991a99..e65c19009e653 100644 --- a/packages/client-runtime/src/pullRequestEditing.ts +++ b/packages/client-runtime/src/pullRequestEditing.ts @@ -41,19 +41,23 @@ export function canEditPullRequestMaintainerEdits( export interface MaintainerEditsChoice { readonly key: string; - readonly observed: boolean | undefined; + readonly observedAt: number | undefined; readonly value: boolean; } -/** A choice ends at the first host value that differs from the one it was made against. */ +/** + * A saved choice ends at the first host reading delivered after the save. Readers refresh once it + * succeeds, which drops reads still in flight, so that reading started after the write. + */ export function maintainerEditsChoiceInForce( choice: MaintainerEditsChoice | null, key: string, - observed: boolean | undefined, + observedAt: number | undefined, saving: boolean, ): MaintainerEditsChoice | null { if (choice === null || choice.key !== key) return null; - return saving || choice.observed === observed ? choice : null; + if (saving) return choice.observedAt === observedAt ? choice : { ...choice, observedAt }; + return observedAt !== undefined && choice.observedAt === observedAt ? choice : null; } /** Hosts do not support rewriting review summaries through comment mutations. */ From cf247b97d12c22dd737746f8258475fb4d220566 Mon Sep 17 00:00:00 2001 From: Bil0000 <62337003+Bil0000@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:01:54 +0100 Subject: [PATCH 13/13] fix(web): align maintainer edits with PR menu actions --- .../src/components/pullRequest/PullRequestActionsMenu.tsx | 1 + apps/web/src/components/ui/menu.tsx | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx index 4ab50135dc7da..df402185ce350 100644 --- a/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx +++ b/apps/web/src/components/pullRequest/PullRequestActionsMenu.tsx @@ -288,6 +288,7 @@ export function PullRequestActionsMenu({ {canEditPullRequestMaintainerEdits(detail) ? ( <> ) : ( <> - +