From c3243f05088c3c20e9439f285fc6d7b0bfe1747d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 9 Oct 2026 23:05:38 -0700 Subject: [PATCH] feat(pull-requests): hosts can report edit and resolve permissions per item (#17667) Co-authored-by: Claude Opus 5.5 --- .../pullRequest/PullRequestCodeTab.tsx | 10 ++- .../pullRequestEditing.logic.test.ts | 66 +++++++++++++++++++ .../pullRequest/pullRequestEditing.logic.ts | 30 +++++++-- packages/contracts/src/pullRequest.ts | 23 +++++++ 4 files changed, 121 insertions(+), 8 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 1339b46196b8911cbb667d88dd8a6551d29147b3..d5f054dadab0136d157de8403082dbe17f594be0 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -33,7 +33,7 @@ import { useClientSettings, useUpdateClientSettings } from "~/hooks/useSettings" import { useTheme } from "~/hooks/useTheme"; import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; import { pullRequestFindingKey, type PullRequestFinding } from "./pullRequestDetail.logic"; -import { canEditPullRequestComment } from "./pullRequestEditing.logic"; +import { canEditPullRequestComment, canResolvePullRequestThread } from "./pullRequestEditing.logic"; import { orderDiffFiles } from "./pullRequestFileOrder.logic"; import { buildFileDiffRenderKey, @@ -934,7 +934,7 @@ function PullRequestCodeTab({ thread={thread} workspaceRoot={detail.workspaceRoot} canReply={review.reply} - canResolve={review.resolve} + canResolve={review.resolve && canResolvePullRequestThread(detail, thread)} canReact={detail.capabilities.reactions === true} environmentId={environmentId} reference={reference} @@ -966,7 +966,11 @@ function PullRequestCodeTab({ } // A conversation on a line is made of review comments, whatever the host filed them as. canEditComment={(comment) => - canEditPullRequestComment(detail, { author: comment.author, kind: "review-comment" }) + canEditPullRequestComment(detail, { + author: comment.author, + kind: "review-comment", + canEdit: comment.canEdit, + }) } onEditComment={(commentId, body) => runThreadCommand("The comment could not be saved", () => diff --git a/apps/web/src/components/pullRequest/pullRequestEditing.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestEditing.logic.test.ts index 1b43ff209b23d7b03da1338857bdfbbc5799633d..06ecec5d329592e2745efee3b3c3bf453dfbc526 100644 --- a/apps/web/src/components/pullRequest/pullRequestEditing.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestEditing.logic.test.ts @@ -10,6 +10,7 @@ import { describe, expect, it } from "vite-plus/test"; import { canEditPullRequestChangeRequest, canEditPullRequestComment, + canResolvePullRequestThread, } from "./pullRequestEditing.logic"; type Subject = Pick; @@ -89,6 +90,33 @@ describe("canEditPullRequestChangeRequest", () => { ).toBe(true); }); + it("follows the host's own answer about the reader over authorship and merge access", () => { + expect( + canEditPullRequestChangeRequest( + subject({ + author: actor("someone-else"), + viewerPermissions: permissions({ editChangeRequest: true }), + }), + ), + ).toBe(true); + expect( + canEditPullRequestChangeRequest( + subject({ + viewerPermissions: permissions({ actions: ["merge"], editChangeRequest: false }), + }), + ), + ).toBe(false); + // The host can't rewrite change requests at all, so the reader's answer can't widen that. + expect( + canEditPullRequestChangeRequest( + subject({ + capabilities: capabilities({ changeRequest: false, comment: true }), + viewerPermissions: permissions({ editChangeRequest: true }), + }), + ), + ).toBe(false); + }); + it("refuses a reader who neither wrote it nor may merge it", () => { expect( canEditPullRequestChangeRequest( @@ -146,6 +174,23 @@ describe("canEditPullRequestComment", () => { ); }); + it("follows the host's own answer about a remark over authorship", () => { + expect(canEditPullRequestComment(subject(), comment({ canEdit: false }))).toBe(false); + expect( + canEditPullRequestComment( + subject(), + comment({ author: actor("someone-else"), canEdit: true }), + ), + ).toBe(true); + // The host can't rewrite remarks at all, so its per-remark answer can't widen that. + expect( + canEditPullRequestComment( + subject({ capabilities: capabilities({ changeRequest: true, comment: false }) }), + comment({ canEdit: true }), + ), + ).toBe(false); + }); + it("refuses a remark the host attributes to nobody", () => { expect(canEditPullRequestComment(subject(), comment({ author: null }))).toBe(false); }); @@ -170,3 +215,24 @@ describe("canEditPullRequestComment", () => { expect(canEditPullRequestComment(subject({ viewer: undefined }), comment())).toBe(false); }); }); + +describe("canResolvePullRequestThread", () => { + it("uses the reader's repository-wide permission where the host says nothing per thread", () => { + expect(canResolvePullRequestThread(subject(), {})).toBe(true); + expect( + canResolvePullRequestThread( + subject({ viewerPermissions: permissions({ resolve: false }) }), + {}, + ), + ).toBe(false); + }); + + it("only narrows the reader's permission with the host's answer for one thread", () => { + expect(canResolvePullRequestThread(subject(), { canResolve: false })).toBe(false); + expect( + canResolvePullRequestThread(subject({ viewerPermissions: permissions({ resolve: false }) }), { + canResolve: true, + }), + ).toBe(false); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestEditing.logic.ts b/apps/web/src/components/pullRequest/pullRequestEditing.logic.ts index a7e49f3f635f09dffae6fce40ba245156607301c..520ff7a7ef96529a12ae23a7be5c98ea292de8be 100644 --- a/apps/web/src/components/pullRequest/pullRequestEditing.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestEditing.logic.ts @@ -1,4 +1,8 @@ -import type { PullRequestComment, PullRequestDetail } from "@t3tools/contracts"; +import type { + PullRequestComment, + PullRequestDetail, + PullRequestReviewThread, +} from "@t3tools/contracts"; /** Only the parts of a detail either answer reads, so a caller can pass a whole detail view. */ type EditingSubject = Pick< @@ -14,12 +18,15 @@ function sameLogin(one: string | null | undefined, other: string | null | undefi /** * Whether the title and description can be rewritten from here. Beyond the host being able to at - * all, either the reader wrote the change request or they may merge it — merging is the one action - * every host here grants with write access and withholds without it, so it stands in for the - * permission none of them publishes by name. + * all, a host that reports `editChangeRequest` answers directly. Otherwise either the reader wrote + * the change request or they may merge it: merging is the one action every host here grants with + * write access and withholds without it, so it stands in for the permission they don't publish. */ export function canEditPullRequestChangeRequest(detail: EditingSubject): boolean { if (detail.capabilities.edit?.changeRequest !== true) return false; + if (detail.viewerPermissions.editChangeRequest !== undefined) { + return detail.viewerPermissions.editChangeRequest; + } return ( sameLogin(detail.viewer, detail.author?.login) || detail.viewerPermissions.actions.includes("merge") @@ -33,9 +40,22 @@ export function canEditPullRequestChangeRequest(detail: EditingSubject): boolean */ export function canEditPullRequestComment( detail: EditingSubject, - comment: Pick, + comment: Pick, ): boolean { if (detail.capabilities.edit?.comment !== true) return false; if (comment.kind !== "issue-comment" && comment.kind !== "review-comment") return false; + if (comment.canEdit !== undefined) return comment.canEdit; return sameLogin(detail.viewer, comment.author?.login); } + +/** + * Whether the reader may resolve or reopen one review thread. The host's answer for this thread + * can only narrow the reader's repository-wide `resolve` permission: a thread can't grant what the + * repository withholds. + */ +export function canResolvePullRequestThread( + detail: Pick, + thread: Pick, +): boolean { + return detail.viewerPermissions.resolve && thread.canResolve !== false; +} diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 2cd0e45cc8e034d5d3c1444dc765bf1fec1b9e04..1efbc1808d143beddebc0934d3680fa4870a3a93 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -209,6 +209,12 @@ export const PullRequestComment = Schema.Struct({ reviewState: Schema.NullOr(Schema.String), /** Absent from a host with no reactions at all, which is a different thing from none on this. */ reactions: Schema.optional(Schema.Array(PullRequestReaction)), + /** + * The host's own answer to whether this reader may rewrite this remark. Absent where the host + * doesn't say, which leaves the page to guess from authorship. A remark on a line appears both + * here and in its review thread; a host that sets this sets it on both copies alike. + */ + canEdit: Schema.optional(Schema.Boolean), }); export type PullRequestComment = typeof PullRequestComment.Type; @@ -232,6 +238,12 @@ export const PullRequestThreadComment = Schema.Struct({ editedAt: Schema.optional(Schema.NullOr(IsoDateTime)), url: Schema.NullOr(Schema.String), reactions: Schema.optional(Schema.Array(PullRequestReaction)), + /** + * The host's own answer to whether this reader may rewrite this remark. Absent where the host + * doesn't say, which leaves the page to guess from authorship. A remark on a line appears both + * here and in its review thread; a host that sets this sets it on both copies alike. + */ + canEdit: Schema.optional(Schema.Boolean), }); export type PullRequestThreadComment = typeof PullRequestThreadComment.Type; @@ -257,6 +269,12 @@ export const PullRequestReviewThread = Schema.Struct({ commentCount: Schema.optional(NonNegativeInt), /** Opaque cursor for the next comment page. Absent once this thread is whole. */ nextCommentsCursor: Schema.optional(TrimmedNonEmptyString), + /** + * The host's own answer to whether this reader may resolve this thread. It can only narrow the + * reader's repository-wide `resolve` permission, never widen it. Absent where the host + * doesn't say per thread. + */ + canResolve: Schema.optional(Schema.Boolean), }); export type PullRequestReviewThread = typeof PullRequestReviewThread.Type; @@ -481,6 +499,11 @@ export const PullRequestViewerPermissions = Schema.Struct({ * changed on this host at all. */ labels: Schema.optional(Schema.Boolean), + /** + * This viewer may rewrite the change request's title and description. Absent where the host + * doesn't say, which leaves the page to guess from authorship and merge access. + */ + editChangeRequest: Schema.optional(Schema.Boolean), }); export type PullRequestViewerPermissions = typeof PullRequestViewerPermissions.Type; -- 2.54.0