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