From 589543a3660bd94bd4a0b1150d156bc6adce7fd6 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 20:32:32 +0700 Subject: [PATCH] The comment you can take back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other thing said on a pull request could be changed here — the title, the description, a review's verdict — except the one thing said most often. A typo in a review comment meant leaving the window for the browser. **Edit sits next to Reply and Resolve**, on a comment in the conversation and on one hanging off a line of the diff alike. It opens the box *in place of* the comment's text rather than under it: what is in the box is the comment, and drawing both reads as two of them. The header stays, so it still says whose it is and when. Cancel and Save are the box's own, so the row of verbs underneath goes away while it is open — the same `⌘↩` posts it and the same `⎋` puts it down, and `@` names people out of the same list. **Who gets the button is the host's answer, not a guess.** GitHub says so itself: every comment in GraphQL carries `viewerCanUpdate`, which is also true for a maintainer on somebody else's comment, exactly as the host's own page behaves. That is not a second call — the query that already reads resolution per thread now reads the conversation's comments and the review summaries beside it, so one request covers all three kinds. It is allowed to fail the way it already was: no `gh`, no scope, an older enterprise schema, and nothing offers an edit. Bitbucket has no such flag on either flavour, so the account has to be looked up and matched by hand. `BitbucketIdentity` asks once per checkout and keeps the answer: Cloud's `/2.0/user` gives an account id, which is what a comment carries too, and Data Center has only the handle, which is what its comments carry. A Cloud token without the `account` scope falls through to `bkt auth status`, whose handle is the *username* — and a Cloud comment is signed with the nickname, so nothing matches and no comment offers an edit. That is the same shape of retreat resolution already makes, and it is why the lookup is not cached when it fails. **Three kinds of GitHub comment need three different calls**, which is the one thing this change could not paper over. `gh pr view` reports a conversation comment and a review by their GraphQL node ids and nothing else, so those go through `updateIssueComment` and `updatePullRequestReview`; an inline review comment already has a REST id in hand and takes a plain `PATCH`. Rather than leave the writer to work out which from the shape of an id, the loader — the only thing that ever knew — records it as a `PullRequestComment.EditTarget`. Bitbucket is one case there because both flavours are one `PUT` tried twice: Cloud takes the Markdown under `content.raw`, Data Center takes it as `text` and refuses an edit that does not name the version it replaces, which is read off the comment when it is loaded. **An edit starts from what the host stores, not from what is on screen.** Bitbucket Cloud writes a mention as an account id and the app puts the person's name in its place for reading; saving that back would post the name as plain words and quietly drop the mention. The raw text is kept beside the rendered one for exactly the comments where the two differ, and the way out goes through the same mention encoding a new comment does. Afterwards the conversation is read again rather than patched locally, for the reason a resolve does the same: the host's copy is the one that counts, and it is what carries whatever "edited" mark the host adds. That reload is also what closes the box — the comment that comes back says something else, so it is no longer the one being edited. An edit the host *refused* leaves the comment as it was, so the box stays open with the text still in it, which is the point of not clearing it by hand. Checked: `Scripts/run.sh` — build succeeded, no warnings, app launches. Both GraphQL mutations were run against the live API with a deliberately bogus node id: the fields resolve and only the id is rejected, and `gh` exits non-zero on a GraphQL error, so a refusal reaches the box. The id spellings were confirmed to match on a real pull request — `gh pr view --json comments` and the GraphQL `comments.nodes.id` return the identical node id, which is what the lookup keys on. Not checked: an edit that actually lands, which means writing to a real comment on a real repository. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WgLkUJ7ZK3rd3zZ1HhhH9t --- Docs/Development.md | 2 +- README.md | 1 + Sources/Workspace/Models/PullRequest.swift | 3 + .../Workspace/Models/PullRequestComment.swift | 374 ++++++++++++++++-- Sources/Workspace/Models/WorkspaceStore.swift | 29 ++ Sources/Workspace/Views/DiffView.swift | 9 +- .../Workspace/Views/InlineCommentViews.swift | 42 +- .../Views/PullRequestDetailView.swift | 122 +++++- 8 files changed, 540 insertions(+), 42 deletions(-) diff --git a/Docs/Development.md b/Docs/Development.md index 6761d14..d00cb06 100644 --- a/Docs/Development.md +++ b/Docs/Development.md @@ -176,7 +176,7 @@ Sources/Workspace/ NewRepository.swift making one instead of finding one: `git init`, `git clone` Project.swift one repository: remote, status, PRs, ports, tree PullRequest.swift unified PR model + gh/bkt loaders - PullRequestComment.swift reading and posting PR comments + PullRequestComment.swift reading, posting and rewriting PR comments PullRequestCommit.swift a PR's commits + the diff of one commit PullRequestActions.swift merge, reject, and drift from the target branch PullRequestReviewer.swift who is reviewing, their verdicts, and asking more people diff --git a/README.md b/README.md index 36351b0..a323c6e 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,7 @@ missing ones, so you never have to work out which command it wanted. | Read it all | Details, Diff and Commits tabs. Markdown, images and mentions rendered properly. | | Threads | Replies nested under what they answer, every author in a colour of their own. A resolved one folds to a single row — author, file and first line, under a green **Resolved** chip — in the conversation and in the diff alike; click it to read the rest. | | Write back | Comment, reply, resolve a thread or open it again, edit the description, approve, request changes, merge, reject. | +| Second thoughts | **Edit** on a comment of your own — in the conversation or on a line of the diff — reopens it in place, wherever the host lets you change it. | | Reviewers | See who approved, add more from a searchable list of the people who can be asked. | | `@` anyone | Type `@` in any comment box and pick a name from the repo. | | Builds beside you | CI runs for the head commit in a side panel, failures first, kept current on their own while the panel is in front of you. | diff --git a/Sources/Workspace/Models/PullRequest.swift b/Sources/Workspace/Models/PullRequest.swift index 792ed86..c8ffc97 100644 --- a/Sources/Workspace/Models/PullRequest.swift +++ b/Sources/Workspace/Models/PullRequest.swift @@ -149,6 +149,7 @@ enum PullRequestError: LocalizedError { case commandFailed(String) case unsupportedHost case replyUnsupported + case editUnsupported var errorDescription: String? { switch self { @@ -160,6 +161,8 @@ enum PullRequestError: LocalizedError { "This repository's remote is neither GitHub nor Bitbucket." case .replyUnsupported: "This host cannot thread a reply onto that comment." + case .editUnsupported: + "That comment cannot be edited from here." } } } diff --git a/Sources/Workspace/Models/PullRequestComment.swift b/Sources/Workspace/Models/PullRequestComment.swift index a5c1f4c..1e449fb 100644 --- a/Sources/Workspace/Models/PullRequestComment.swift +++ b/Sources/Workspace/Models/PullRequestComment.swift @@ -19,6 +19,24 @@ struct PullRequestComment: Identifiable, Sendable, Hashable { } } + /// What the host wants named when a comment's text is replaced. + /// + /// Three kinds of GitHub comment arrive from three different APIs and are + /// edited through three different calls, so the loader — the only thing that + /// knows which one it read — says so here rather than leaving the writer to + /// guess it back out of an id. + enum EditTarget: Sendable, Hashable { + /// A comment on the conversation tab, by its GraphQL node id. + case gitHubIssueComment(String) + /// A review's summary, by its GraphQL node id. + case gitHubReview(String) + /// An inline review comment, by its REST id. + case gitHubReviewComment(String) + /// A Bitbucket comment. Data Center refuses an edit that does not name + /// the version it replaces; Cloud has no such idea and sends nothing. + case bitbucket(id: String, version: Int?) + } + /// Which column of the diff a line number refers to. enum Side: String, Sendable, Hashable { /// The file before the change — a removed or context line. @@ -34,6 +52,14 @@ struct PullRequestComment: Identifiable, Sendable, Hashable { /// The author's picture, when the host tells us where to find one. var avatarURL: URL? var body: String + /// The Markdown the host itself stores, when that is not what `body` says. + /// + /// Bitbucket Cloud writes a mention as an account id and the app puts the + /// person's name in its place for reading — see ``BitbucketMarkup``. Saving + /// that back would post the name as plain words and lose the mention, so an + /// edit starts from what the host actually holds. Nil everywhere else, where + /// `body` is already the original. + var rawBody: String? var createdAt: Date? var kind: Kind /// Set when the comment is anchored to a file in the diff. @@ -59,11 +85,21 @@ struct PullRequestComment: Identifiable, Sendable, Hashable { /// comment the thread hangs off; `nil` means this thread cannot be resolved /// from here and the button is not drawn. var resolveToken: String? + /// The handle the host wants when this comment's text is replaced. `nil` + /// means it cannot be edited from here — someone else wrote it, or the host + /// gave no way to tell whose it is. + var editTarget: EditTarget? var canReply: Bool { replyToken != nil } var canResolve: Bool { resolveToken != nil } + var canEdit: Bool { editTarget != nil } + + /// The text an edit box opens with: what the host stores, which is not + /// always what is on screen — see ``rawBody``. + var editableBody: String { rawBody ?? body } + /// Where in the diff this comment belongs, when it belongs anywhere. var anchor: DiffLineAnchor? { guard let path, let line else { return nil } @@ -173,6 +209,82 @@ extension PullRequestComment { } } +/// Who `bkt` is signed in as, so a comment of one's own can be told from +/// everyone else's. +/// +/// GitHub answers that question itself — every comment in GraphQL carries +/// `viewerCanUpdate` — and Bitbucket has nothing of the kind on either flavour, +/// so the account has to be looked up and matched by hand. Cloud gives an +/// account id, which is what a comment carries too; Data Center has only the +/// handle, which is what its comments carry. +/// +/// Read once per checkout and kept: one answer covers every comment on every +/// pull request in it, and a failed read is not cached, so a `bkt` that was +/// signed out and back in is picked up on the next conversation. +actor BitbucketIdentity { + static let shared = BitbucketIdentity() + + struct Account: Sendable, Hashable { + var accountID: String? + var uuid: String? + var login: String? + + /// Whether a comment's `user` object is this account. The id is the + /// answer wherever there is one; the handle is what Data Center leaves. + func owns(_ user: [String: Any]?) -> Bool { + guard let user else { return false } + if let accountID, !accountID.isEmpty, user["account_id"] as? String == accountID { + return true + } + if let uuid, !uuid.isEmpty, user["uuid"] as? String == uuid { + return true + } + guard let login, !login.isEmpty, + let theirs = BitbucketUser.login(from: user) + else { return false } + return theirs.caseInsensitiveCompare(login) == .orderedSame + } + } + + private var accounts: [String: Account] = [:] + + func current(in directory: URL) async -> Account? { + let key = directory.path + if let known = accounts[key] { return known } + guard let account = await Self.read(in: directory) else { return nil } + accounts[key] = account + return account + } + + private static func read(in directory: URL) async -> Account? { + // Cloud hands over the whole account, id and all. + let cloud = await Shell.run(["bkt", "api", "/2.0/user"], in: directory, timeout: 30) + if cloud.isSuccess, + let object = try? JSONSerialization.jsonObject(with: Data(cloud.stdout.utf8)) as? [String: Any], + object["account_id"] != nil || object["uuid"] != nil { + return Account( + accountID: object["account_id"] as? String, + uuid: object["uuid"] as? String, + login: BitbucketUser.login(from: object) + ) + } + + // Data Center has no such endpoint, but `bkt` knows what it signed in + // with. This is also where a Cloud token without the `account` scope + // lands, and there the handle it gives — the username — is not the + // nickname a Cloud comment is signed with, so nothing matches and no + // comment offers an edit. That is the same shape of retreat resolution + // already makes when GraphQL is out of reach. + let status = await Shell.run(["bkt", "auth", "status", "--json"], in: directory, timeout: 30) + guard status.isSuccess, + let object = try? JSONSerialization.jsonObject(with: Data(status.stdout.utf8)) as? [String: Any], + let hosts = object["hosts"] as? [[String: Any]], + let login = hosts.compactMap({ $0["username"] as? String }).first(where: { !$0.isEmpty }) + else { return nil } + return Account(login: login) + } +} + extension PullRequestService { // MARK: - Reading @@ -260,23 +372,28 @@ extension PullRequestService { ) } - var inline = await gitHubReviewComments(for: pr, in: directory) - await applyGitHubThreads(to: &inline, for: pr, in: directory) - items.append(contentsOf: inline) + items.append(contentsOf: await gitHubReviewComments(for: pr, in: directory)) + await applyGitHubThreadsAndEditRights(to: &items, for: pr, in: directory) return items.sorted { ($0.createdAt ?? .distantPast) < ($1.createdAt ?? .distantPast) } } - /// Marks the review comments whose thread has been resolved, and records the - /// handle each thread is resolved by. + /// Marks the review comments whose thread has been resolved, records the + /// handle each thread is resolved by, and says which comments the signed-in + /// account is allowed to rewrite. /// - /// The REST endpoint the comments themselves come from says nothing about - /// resolution — it exists only in GraphQL, and only per thread — so this is - /// a second call over the same comments. It is allowed to fail: a token - /// without the scope, an enterprise host on an older schema, or no `gh` at - /// all leaves every thread looking open, which is what the app showed - /// before and still reads correctly. - private static func applyGitHubThreads( + /// All three are GraphQL-only. The REST endpoint the inline comments come + /// from says nothing about resolution — it exists per thread, and nowhere + /// else — and neither `gh pr view` nor REST will say whether an edit would + /// be accepted, which `viewerCanUpdate` answers for every kind of comment at + /// once. So one query covers the conversation, the review summaries and the + /// inline comments together. + /// + /// It is allowed to fail: a token without the scope, an enterprise host on + /// an older schema, or no `gh` at all leaves every thread looking open and + /// nothing editable, which is what the app showed before and still reads + /// correctly. + private static func applyGitHubThreadsAndEditRights( to comments: inout [PullRequestComment], for pr: PullRequest, in directory: URL @@ -289,8 +406,11 @@ extension PullRequestService { // so the names are passed as variables. let query = "query($owner:String!,$repo:String!,$number:Int!)" + "{repository(owner:$owner,name:$repo)" - + "{pullRequest(number:$number){reviewThreads(first:100){nodes" - + "{id isResolved comments(first:100){nodes{databaseId}}}}}}}" + + "{pullRequest(number:$number){" + + "comments(first:100){nodes{id viewerCanUpdate}}" + + "reviews(first:100){nodes{id viewerCanUpdate}}" + + "reviewThreads(first:100){nodes" + + "{id isResolved comments(first:100){nodes{databaseId viewerCanUpdate}}}}}}}" let result = await GitHubCLI.run( ["api", "graphql", "-f", "query=\(query)", @@ -305,11 +425,25 @@ extension PullRequestService { struct Response: Decodable { struct Root: Decodable { let repository: Repository? } struct Repository: Decodable { let pullRequest: PullRequestNode? } - struct PullRequestNode: Decodable { let reviewThreads: Threads? } + struct PullRequestNode: Decodable { + let comments: Nodes? + let reviews: Nodes? + let reviewThreads: Threads? + } + /// A conversation comment or a review summary: the node id the + /// mutation names, and whether it would take one. + struct Nodes: Decodable { let nodes: [Node]? } + struct Node: Decodable { + let id: String? + let viewerCanUpdate: Bool? + } struct Threads: Decodable { let nodes: [Thread]? } struct Thread: Decodable { struct Comments: Decodable { let nodes: [Comment]? } - struct Comment: Decodable { let databaseId: Int? } + struct Comment: Decodable { + let databaseId: Int? + let viewerCanUpdate: Bool? + } let id: String? let isResolved: Bool? let comments: Comments? @@ -318,22 +452,40 @@ extension PullRequestService { } guard let response = try? JSONDecoder().decode(Response.self, from: Data(result.stdout.utf8)), - let threads = response.data?.repository?.pullRequest?.reviewThreads?.nodes + let pullRequest = response.data?.repository?.pullRequest else { return } - var byCommentID: [String: (token: String, isResolved: Bool)] = [:] - for thread in threads { + // Keyed the way the loaders above spelled the ids, so each comment finds + // its own row whatever kind it is. + var threadByCommentID: [String: (token: String, isResolved: Bool)] = [:] + var editableByCommentID: [String: PullRequestComment.EditTarget] = [:] + + for node in pullRequest.comments?.nodes ?? [] { + guard let id = node.id, node.viewerCanUpdate == true else { continue } + editableByCommentID["issue-\(id)"] = .gitHubIssueComment(id) + } + for node in pullRequest.reviews?.nodes ?? [] { + guard let id = node.id, node.viewerCanUpdate == true else { continue } + editableByCommentID["review-\(id)"] = .gitHubReview(id) + } + for thread in pullRequest.reviewThreads?.nodes ?? [] { guard let token = thread.id else { continue } for comment in thread.comments?.nodes ?? [] { guard let databaseId = comment.databaseId else { continue } - byCommentID["review-comment-\(databaseId)"] = (token, thread.isResolved ?? false) + let key = "review-comment-\(databaseId)" + threadByCommentID[key] = (token, thread.isResolved ?? false) + if comment.viewerCanUpdate == true { + editableByCommentID[key] = .gitHubReviewComment("\(databaseId)") + } } } for index in comments.indices { - guard let thread = byCommentID[comments[index].id] else { continue } - comments[index].resolveToken = thread.token - comments[index].isResolved = thread.isResolved + if let thread = threadByCommentID[comments[index].id] { + comments[index].resolveToken = thread.token + comments[index].isResolved = thread.isResolved + } + comments[index].editTarget = editableByCommentID[comments[index].id] } } @@ -455,6 +607,10 @@ extension PullRequestService { for pr: PullRequest, in directory: URL ) async throws -> [PullRequestComment] { + // Which of these are the reader's own, and so may be rewritten. Nil + // where `bkt` would not say; then nothing offers an edit. + let account = await BitbucketIdentity.shared.current(in: directory) + // Bitbucket Cloud: `bkt pr view` carries no comments at all, so go to // the REST API directly. if !pr.repositoryOwner.isEmpty, !pr.repositorySlug.isEmpty { @@ -468,7 +624,7 @@ extension PullRequestService { if result.isSuccess, let object = try? JSONSerialization.jsonObject(with: Data(result.stdout.utf8)) as? [String: Any], let values = object["values"] as? [[String: Any]] { - return decodeCloudComments(values) + return decodeCloudComments(values, ownedBy: account) } } @@ -493,13 +649,16 @@ extension PullRequestService { guard let object = try? JSONSerialization.jsonObject(with: Data(result.stdout.utf8)) else { continue } - return decodeBitbucketComments(from: object) + return decodeBitbucketComments(from: object, ownedBy: account) } throw PullRequestError.commandFailed(lastMessage) } /// Decodes the fixed shape of Bitbucket Cloud's `/pullrequests/N/comments`. - private static func decodeCloudComments(_ values: [[String: Any]]) -> [PullRequestComment] { + private static func decodeCloudComments( + _ values: [[String: Any]], + ownedBy account: BitbucketIdentity.Account? + ) -> [PullRequestComment] { values.enumerated().compactMap { index, item in if item["deleted"] as? Bool == true { return nil } if item["pending"] as? Bool == true { return nil } @@ -535,6 +694,9 @@ extension PullRequestService { author: author, avatarURL: AvatarURL.hosted(avatar as? String), body: body, + // Only when the name put in place of an account id made it + // something other than what the host holds. + rawBody: body == raw ? nil : raw, createdAt: (item["created_on"] as? String).flatMap(parseTimestamp), kind: .comment, path: inline?["path"] as? String, @@ -543,7 +705,12 @@ extension PullRequestService { replyToken: identifier, isResolved: resolution != nil, resolvedBy: BitbucketUser.name(from: resolution?["user"] as? [String: Any]), - resolveToken: identifier + resolveToken: identifier, + // Cloud numbers a comment's revisions nowhere; only Data Center + // asks for the version an edit replaces. + editTarget: account?.owns(user) == true + ? .bitbucket(id: identifier, version: nil) + : nil ) } .sorted { ($0.createdAt ?? .distantPast) < ($1.createdAt ?? .distantPast) } @@ -552,7 +719,10 @@ extension PullRequestService { /// Walks the JSON for any comment-shaped object. Data Center nests replies /// in a `comments` array on the parent, so descend into those explicitly /// and carry the parent's id down. - private static func decodeBitbucketComments(from object: Any) -> [PullRequestComment] { + private static func decodeBitbucketComments( + from object: Any, + ownedBy account: BitbucketIdentity.Account? + ) -> [PullRequestComment] { var found: [PullRequestComment] = [] var seen: Set = [] @@ -614,6 +784,7 @@ extension PullRequestService { author: author, avatarURL: AvatarURL.hosted(avatar), body: content, + rawBody: content == raw ? nil : raw, createdAt: timestamp.flatMap(parseTimestamp), kind: .comment, path: path, @@ -622,7 +793,12 @@ extension PullRequestService { replyToken: dictionary["id"].map { "\($0)" }, isResolved: isResolved, resolvedBy: BitbucketUser.name(from: resolver), - resolveToken: dictionary["id"].map { "\($0)" } + resolveToken: dictionary["id"].map { "\($0)" }, + // Data Center takes an edit only against the revision it is + // replacing, and counts them here. + editTarget: account?.owns(userDictionary) == true + ? .bitbucket(id: identifier, version: dictionary["version"] as? Int) + : nil ) } @@ -881,6 +1057,146 @@ extension PullRequestService { } } + /// Replaces what a comment says. + /// + /// `comment` has to be one the conversation was loaded with: the handle each + /// host wants rides on it, and only the loader knew which kind of comment it + /// was reading — see ``PullRequestComment/EditTarget``. + static func updateComment( + _ body: String, + of comment: PullRequestComment, + on pr: PullRequest, + in directory: URL + ) async throws { + let trimmed = body.trimmingCharacters(in: .whitespacesAndNewlines) + guard !trimmed.isEmpty else { + throw PullRequestError.commandFailed( + "A comment cannot be emptied. Delete it on the host instead." + ) + } + guard let target = comment.editTarget else { + throw PullRequestError.editUnsupported + } + + switch target { + case .gitHubIssueComment(let nodeID): + try await updateGitHubNode( + nodeID, + to: trimmed, + mutation: "updateIssueComment", + idField: "id", + in: directory + ) + + case .gitHubReview(let nodeID): + try await updateGitHubNode( + nodeID, + to: trimmed, + mutation: "updatePullRequestReview", + idField: "pullRequestReviewId", + in: directory + ) + + case .gitHubReviewComment(let id): + // The one kind with a REST id in hand, and so the one that needs no + // GraphQL at all. + let result = await GitHubCLI.run( + ["api", "--method", "PATCH", + "repos/{owner}/{repo}/pulls/comments/\(id)", + "-f", "body=\(trimmed)"], + in: directory, + timeout: 60 + ) + guard result.isSuccess else { + throw PullRequestError.commandFailed(result.failureMessage) + } + + case .bitbucket(let id, let version): + try await updateBitbucketComment( + trimmed, + id: id, + version: version, + on: pr, + in: directory + ) + } + } + + /// One of GitHub's two "change the text" mutations, which differ only in + /// what they call the id. Neither has a REST equivalent that takes what is + /// in hand: `gh pr view` reports a conversation comment and a review by + /// their GraphQL node ids, and those are exactly what GraphQL wants. + private static func updateGitHubNode( + _ nodeID: String, + to body: String, + mutation: String, + idField: String, + in directory: URL + ) async throws { + let query = "mutation($id:ID!,$body:String!){" + + "\(mutation)(input:{\(idField):$id,body:$body}){clientMutationId}}" + let result = await GitHubCLI.run( + ["api", "graphql", + "-f", "query=\(query)", + "-f", "id=\(nodeID)", + "-f", "body=\(body)"], + in: directory, + timeout: 60 + ) + guard result.isSuccess else { + throw PullRequestError.commandFailed(result.failureMessage) + } + } + + /// The same edit on both Bitbucket flavours: Cloud takes the new Markdown + /// under `content.raw`, Data Center takes it as `text` and insists on the + /// version it is replacing. Tried in that order, like every other write here. + private static func updateBitbucketComment( + _ body: String, + id: String, + version: Int?, + on pr: PullRequest, + in directory: URL + ) async throws { + guard !pr.repositoryOwner.isEmpty, !pr.repositorySlug.isEmpty else { + throw PullRequestError.commandFailed( + "This repository's workspace and slug are unknown, so the comment cannot be edited." + ) + } + + var dataCenter: [String: Any] = ["text": body] + if let version { dataCenter["version"] = version } + + let attempts: [(path: String, payload: [String: Any])] = [ + ( + "/2.0/repositories/\(pr.repositoryOwner)/\(pr.repositorySlug)" + + "/pullrequests/\(pr.number)/comments/\(id)", + ["content": ["raw": body]] + ), + ( + "/rest/api/1.0/projects/\(pr.repositoryOwner)/repos/\(pr.repositorySlug)" + + "/pull-requests/\(pr.number)/comments/\(id)", + dataCenter + ), + ] + + var lastMessage = "Could not save the comment." + for attempt in attempts { + guard let data = try? JSONSerialization.data(withJSONObject: attempt.payload) else { + continue + } + let result = await Shell.run( + ["bkt", "api", attempt.path, "--method", "PUT", + "--input", String(decoding: data, as: UTF8.self)], + in: directory, + timeout: 60 + ) + if result.isSuccess { return } + lastMessage = result.failureMessage + } + throw PullRequestError.commandFailed(lastMessage) + } + /// Settles a thread, or opens it again. `comment` is the thread's root, the /// only comment either host records resolution on. static func setResolved( diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 72f6598..9b400d7 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -1613,6 +1613,35 @@ final class WorkspaceStore { item.isPostingComment = false } + /// Replaces what an already-posted comment says. + /// + /// The conversation is read back for the same reason a resolve reads it + /// back: the host's copy is the one that counts, and it is what renders the + /// edited comment — including the "edited" mark a host may add. + func updateComment( + _ body: String, + of comment: PullRequestComment, + on item: ViewerItem, + project: Project, + pr: PullRequest + ) async { + item.isPostingComment = true + item.commentError = nil + do { + try await PullRequestService.updateComment( + mentioning(body, on: item, pr: pr), + of: comment, + on: pr, + in: project.url + ) + showStatus("Comment updated on #\(pr.number)") + await loadComments(item, project: project, pr: pr) + } catch { + item.commentError = error.localizedDescription + } + item.isPostingComment = false + } + /// Settles a comment thread, or opens it again. The conversation is read /// back afterwards rather than edited in place: resolution is the host's /// word, and a thread someone else has already touched should come back diff --git a/Sources/Workspace/Views/DiffView.swift b/Sources/Workspace/Views/DiffView.swift index 80ca0f1..3406475 100644 --- a/Sources/Workspace/Views/DiffView.swift +++ b/Sources/Workspace/Views/DiffView.swift @@ -29,6 +29,8 @@ struct DiffView: View { /// The comment whose reply box is open, shared by every thread on screen so /// that opening one closes the last. @State private var replyingTo: PullRequestComment? + /// The comment being rewritten, shared the same way for the same reason. + @State private var editing: PullRequestComment? /// The diff flattened to one entry per visible line — see `DiffElement`. /// Cached rather than recomputed in `body`, which also runs on every frame /// of a window resize. @@ -349,11 +351,13 @@ struct DiffView: View { threads: threads(file: file, row: row), isPosting: comments?.isPosting ?? false, replyingTo: $replyingTo, + editing: $editing, mentions: comments?.mentions ?? .none, onReply: { parent, body in await comments?.reply(parent, body) }, // Flattened: chaining through an optional bundle onto an // optional closure would otherwise nest one inside the other. - onResolve: comments?.resolve ?? nil + onResolve: comments?.resolve ?? nil, + onEdit: comments?.edit ?? nil ) .frame(width: width, alignment: .leading) case .composer: @@ -383,6 +387,7 @@ struct DiffView: View { guard let anchor = row.anchor(in: file, side: side) else { return } composing = composing == anchor ? nil : anchor replyingTo = nil + editing = nil } } @@ -410,6 +415,8 @@ struct DiffComments { /// Settles a thread, or opens it again. Nil for a diff that has no host /// behind it to be told. var resolve: ((PullRequestComment, Bool) async -> Void)? + /// Replaces what a comment says. Nil for the same reason. + var edit: ((PullRequestComment, String) async -> Void)? } extension DiffRow { diff --git a/Sources/Workspace/Views/InlineCommentViews.swift b/Sources/Workspace/Views/InlineCommentViews.swift index bb81697..535151a 100644 --- a/Sources/Workspace/Views/InlineCommentViews.swift +++ b/Sources/Workspace/Views/InlineCommentViews.swift @@ -35,9 +35,12 @@ struct DiffCommentThreads: View { let threads: [PullRequestCommentNode] let isPosting: Bool @Binding var replyingTo: PullRequestComment? + /// The comment whose text is being rewritten, if any. + @Binding var editing: PullRequestComment? var mentions: MentionSource = .none let onReply: (PullRequestComment, String) async -> Void var onResolve: ((PullRequestComment, Bool) async -> Void)? + var onEdit: ((PullRequestComment, String) async -> Void)? var body: some View { VStack(alignment: .leading, spacing: 8) { @@ -46,12 +49,14 @@ struct DiffCommentThreads: View { node: thread, depth: 0, replyingTo: $replyingTo, + editing: $editing, isPosting: isPosting, mentions: mentions, // The line it hangs under already says which file this is. isInline: true, onReply: onReply, - onResolve: onResolve + onResolve: onResolve, + onEdit: onEdit ) } } @@ -202,18 +207,43 @@ struct DiffCommentComposer: View { } } -/// A text box with Cancel and a send button. Used for replies in a thread and -/// for new comments on a line, which differ only in their wording. +/// A text box with Cancel and a send button. Used for replies in a thread, for +/// new comments on a line, and for rewriting a comment already posted — which +/// differ only in their wording and in what the box starts out holding. struct CommentComposer: View { let prompt: String let sendTitle: String - var sendSymbol = "arrowshape.turn.up.left" + let sendSymbol: String let isPosting: Bool - var mentions: MentionSource = .none + let mentions: MentionSource let onCancel: () -> Void let onSend: (String) async -> Void - @State private var draft = "" + @State private var draft: String + + /// `text` is what the box opens with — empty for anything being written for + /// the first time, and the comment as the host stores it for an edit. It is + /// spelled out rather than left to the memberwise initialiser because a + /// `@State` needs its starting value here, before the view is on screen. + init( + prompt: String, + sendTitle: String, + sendSymbol: String = "arrowshape.turn.up.left", + isPosting: Bool, + mentions: MentionSource = .none, + startingFrom text: String = "", + onCancel: @escaping () -> Void, + onSend: @escaping (String) async -> Void + ) { + self.prompt = prompt + self.sendTitle = sendTitle + self.sendSymbol = sendSymbol + self.isPosting = isPosting + self.mentions = mentions + self.onCancel = onCancel + self.onSend = onSend + _draft = State(initialValue: text) + } private var isEmpty: Bool { draft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 3641fdb..28ea1e5 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -781,6 +781,15 @@ struct PullRequestDetailView: View { project: project, pr: pr ) + }, + edit: { comment, body in + await store.updateComment( + body, + of: comment, + on: item, + project: project, + pr: pr + ) } ) ) @@ -1282,6 +1291,8 @@ struct ConversationView: View { /// The comment whose inline reply box is open, if any. @State private var replyingTo: PullRequestComment? + /// The comment being rewritten, if any. + @State private var editing: PullRequestComment? /// Who an `@` names here, in the composer and in every reply box under it. private var mentions: MentionSource { @@ -1333,6 +1344,7 @@ struct ConversationView: View { node: thread, depth: 0, replyingTo: $replyingTo, + editing: $editing, isPosting: item.isPostingComment, mentions: mentions, onReply: { parent, body in @@ -1346,6 +1358,15 @@ struct ConversationView: View { project: project, pr: pr ) + }, + onEdit: { comment, body in + await store.updateComment( + body, + of: comment, + on: item, + project: project, + pr: pr + ) } ) } @@ -1377,7 +1398,10 @@ struct ConversationView: View { // The same argument for the reply box that is open under a comment. It // is held here rather than inside the composer, so it can simply be put // down; the comment it names is not on the pull request that arrived. - .onChange(of: item.id) { replyingTo = nil } + .onChange(of: item.id) { + replyingTo = nil + editing = nil + } // The pane's own colour, which the diff and the commit list each paint // for themselves and the conversation did not. It is the same shade the // viewer already draws behind all three, so nothing looks different at @@ -1489,6 +1513,9 @@ struct CommentThread: View { let node: PullRequestCommentNode let depth: Int @Binding var replyingTo: PullRequestComment? + /// The comment whose text is being rewritten. Shared across the whole + /// conversation like `replyingTo`, so opening one box closes the last. + @Binding var editing: PullRequestComment? let isPosting: Bool var mentions: MentionSource = .none /// Drawn in the diff rather than in the conversation, where there is less @@ -1497,6 +1524,8 @@ struct CommentThread: View { let onReply: (PullRequestComment, String) async -> Void /// Settles the thread, or opens it again. Nil where the host gave no way. var onResolve: ((PullRequestComment, Bool) async -> Void)? + /// Replaces what a comment says. Nil where nothing here may be edited. + var onEdit: ((PullRequestComment, String) async -> Void)? /// Whether a settled thread has been opened up to be read. It is deliberately /// not remembered: a reload brings the conversation back the way the host @@ -1540,9 +1569,16 @@ struct CommentThread: View { comment: node.comment, replyCount: depth == 0 ? node.totalReplies : 0, isReplying: replyingTo == node.comment, + isEditing: isEditing, isBusy: isPosting, + mentions: mentions, onReplyTapped: openReply, - onResolveTapped: resolveAction + onResolveTapped: resolveAction, + onEditTapped: editAction, + onEditCancelled: { setEditing(nil) }, + onEditSubmitted: onEdit.map { edit in + { body in await edit(node.comment, body) } + } ) if replyingTo == node.comment { @@ -1565,11 +1601,13 @@ struct CommentThread: View { node: reply, depth: depth + 1, replyingTo: $replyingTo, + editing: $editing, isPosting: isPosting, mentions: mentions, isInline: isInline, onReply: onReply, - onResolve: onResolve + onResolve: onResolve, + onEdit: onEdit ) } } @@ -1598,6 +1636,7 @@ struct CommentThread: View { } else { withAnimation(ViewerMotion.contentChange) { replyingTo = next } } + if next != nil { setEditing(nil) } } /// What the Resolve button does, or nil where it is not drawn at all. @@ -1605,6 +1644,34 @@ struct CommentThread: View { guard depth == 0, node.comment.canResolve, let onResolve else { return nil } return { Task { await onResolve(node.comment, !node.isResolved) } } } + + private var isEditing: Bool { editing == node.comment } + + /// What the Edit button does, or nil when this comment is not the reader's + /// to change. + private var editAction: (() -> Void)? { + guard node.comment.canEdit, onEdit != nil else { return nil } + return { + setEditing(isEditing ? nil : node.comment) + replyingTo = nil + } + } + + /// Opening the box is the same transaction question as opening a reply box — + /// see ``openReply``. + /// + /// Nothing closes it on the way back: an edit that lands reloads the + /// conversation, and the comment that arrives says something else, so it is + /// no longer the one `editing` names. An edit the host refused leaves the + /// comment exactly as it was, and the box stays open with the text still in + /// it — which is the whole point of not clearing it here. + private func setEditing(_ comment: PullRequestComment?) { + if isInline { + editing = comment + } else { + withAnimation(ViewerMotion.contentChange) { editing = comment } + } + } } // MARK: - Reviewers @@ -2271,12 +2338,22 @@ struct CommentBubble: View { /// Shown on a thread root so a collapsed-looking thread still reads as one. var replyCount: Int = 0 var isReplying = false + /// Whether the box that rewrites this comment is open in place of its text. + var isEditing = false /// A write is already on its way to the host, so nothing else may be asked. var isBusy = false + /// Who an `@` names in that box. + var mentions: MentionSource = .none var onReplyTapped: (() -> Void)? /// Settles the thread this comment heads, or opens it again. Nil where the /// host offers no handle for it, and then no button is drawn. var onResolveTapped: (() -> Void)? + /// Opens the box, or puts it away again. Nil where this comment is not the + /// reader's to change. + var onEditTapped: (() -> Void)? + var onEditCancelled: (() -> Void)? + /// Sends the rewritten text. The box is drawn only when this is given. + var onEditSubmitted: ((String) async -> Void)? var body: some View { VStack(alignment: .leading, spacing: 5) { @@ -2319,7 +2396,21 @@ struct CommentBubble: View { .truncationMode(.head) } - if comment.body.isEmpty { + if isEditing, let onEditSubmitted { + // In place of the text rather than under it: what is in the box + // *is* the comment, and showing both reads as two of them. The + // header above still says whose it is and when it was written. + CommentComposer( + prompt: "Edit this comment… @ to name someone", + sendTitle: "Save", + sendSymbol: "checkmark", + isPosting: isBusy, + mentions: mentions, + startingFrom: comment.editableBody, + onCancel: { onEditCancelled?() }, + onSend: onEditSubmitted + ) + } else if comment.body.isEmpty { Text("(no message)") .font(.callout) .foregroundStyle(.tertiary) @@ -2328,7 +2419,9 @@ struct CommentBubble: View { .font(.callout) } - if (comment.canReply && onReplyTapped != nil) || onResolveTapped != nil { + // While the box is open it carries its own Cancel and Save, and a + // row of other verbs under it is only in the way. + if !isEditing, hasActions { HStack(spacing: 12) { if comment.canReply, let onReplyTapped { Button(action: onReplyTapped) { @@ -2343,6 +2436,18 @@ struct CommentBubble: View { .pointerCursor() } + if comment.canEdit, let onEditTapped { + Button(action: onEditTapped) { + Label("Edit", systemImage: "pencil") + .font(.caption) + } + .buttonStyle(.plain) + .foregroundStyle(.secondary) + .disabled(isBusy) + .pointerCursor(!isBusy) + .help("Change what this comment says") + } + if let onResolveTapped { Button(action: onResolveTapped) { Label( @@ -2370,6 +2475,13 @@ struct CommentBubble: View { .background(.quaternary.opacity(0.22), in: RoundedRectangle(cornerRadius: 9)) } + /// Whether anything at all can be done to this comment from here. + private var hasActions: Bool { + (comment.canReply && onReplyTapped != nil) + || (comment.canEdit && onEditTapped != nil) + || onResolveTapped != nil + } + private var tint: Color { switch comment.kind { case .comment: .secondary