From 21f9ea12969e00578da500482cd2806ddba647a2 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 11:48:53 +0700 Subject: [PATCH 1/7] Threads that are settled fold away, builds that keep themselves current MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things a review keeps asking the window for, and never got. **A settled thread folds away.** Resolution is a thing both hosts record and neither used to say here, so a long review read as a wall of comments with no way to tell the answered ones from the live ones. A resolved thread is now one row — who opened it, a green **Resolved** chip, the file, and its first line — and clicking it reads the rest. The same row in the conversation and inline in the diff, where the file is dropped because the line it hangs under is already the answer. Where it comes from differs by host and cost one design decision. GitHub's REST endpoint, which is where the inline comments come from, says nothing about resolution: it exists only in GraphQL and only per thread, so it is a second call over the same comments. That call is allowed to fail — a token without the scope, an older enterprise schema, no `gh` at all — and every thread then reads open, which is what the app showed before. Bitbucket Cloud hangs a `resolution` object off the thread's first comment; Data Center names it in `state`. The flag is left where each host puts it, on the root, and read back off the node, so the tree builder never has to learn what a thread is. **And it can be settled from here.** Resolve sits next to Reply on the root, and only where the host handed over something to resolve with — the same rule `canReply` already follows. GitHub takes the GraphQL thread node id the conversation was loaded with, Cloud its own endpoint where the method is the whole message, Data Center a write of the comment's `state` with the version it was last read at. Afterwards the conversation is read again rather than guessed at locally. **The builds keep themselves current.** They were a list fetched once, so a green tick could sit there through a run that had since gone red. The panel now asks again every ten seconds. This is the one place the app polls, and it is deliberate: a build finishing is not an event either host reports, so there is nothing to listen for. It asks narrowly — the loop is a `.task` on the pull request being read, so it dies when that pull request is closed or another takes its place, and a tick behind a window nobody is in front of is skipped. It asks quietly too: no spinner every ten seconds, the list assigned only when it has actually changed, and a `gh` that stumbles once leaves the last good list where it is rather than emptying the panel under someone reading it. It lives with the detail view rather than with the panel that draws it, because the badge in the summary bar reads the same runs on the tabs where the panel is not. **One button reads the whole thing again.** Three menu entries used to reload the diff, the commits and the builds separately, which is three trips for the one thing anybody wants. They are now a Refresh in the summary bar: the request itself — its title, state, draft flag, approvals and description all live on that one object — the diff, the conversation, who is reviewing, the runs and how far the branch has drifted, all read together so the wait is the slowest host call rather than the sum of them. The commits come along only if the tab that shows them was ever opened. It has a flag of its own rather than borrowing the one a merge uses: a refresh changes nothing on the host, and two spinners should not turn at once. And the button beside an open pull request's title opens it on the host instead of copying its link. The link was for pasting into a ticket; going there is what the button next to the title is actually reached for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0147HXrW1borgmWJ41gTK8yo --- README.md | 8 +- .../Workspace/Models/PullRequestComment.swift | 245 ++++++++++++++++- Sources/Workspace/Models/ViewerItem.swift | 5 + Sources/Workspace/Models/WorkspaceStore.swift | 124 +++++++++ Sources/Workspace/Views/DiffView.swift | 8 +- .../Workspace/Views/InlineCommentViews.swift | 86 +++++- .../Views/PullRequestDetailView.swift | 257 +++++++++++++----- .../Workspace/Views/PullRequestSidebar.swift | 12 +- Sources/Workspace/Views/ViewerView.swift | 14 +- 9 files changed, 669 insertions(+), 90 deletions(-) diff --git a/README.md b/README.md index fcf4d09..b58711d 100644 --- a/README.md +++ b/README.md @@ -89,12 +89,14 @@ missing ones, so you never have to work out which command it wanted. | --- | --- | | The board | Open PRs as a table: title, age, activity, reviewers, CI — one row each. | | 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. | -| Write back | Comment, reply, edit the description, approve, request changes, merge, reject. | +| 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. | | 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. | +| 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. | | Update from main | One button when the branch has fallen behind. | +| Read it again | One button in the summary bar re-reads the whole request from the host — title, state, diff, conversation, reviewers and builds. | +| Off to the host | The button beside the title opens the pull request in your browser. | ### Code and diffs diff --git a/Sources/Workspace/Models/PullRequestComment.swift b/Sources/Workspace/Models/PullRequestComment.swift index 09931af..96f79ef 100644 --- a/Sources/Workspace/Models/PullRequestComment.swift +++ b/Sources/Workspace/Models/PullRequestComment.swift @@ -44,9 +44,29 @@ struct PullRequestComment: Identifiable, Sendable, Hashable { /// The host's own identifier for this comment, set only when the host can /// attach a reply to it. `nil` means "reply is not possible here". var replyToken: String? + /// Whether the thread this comment belongs to has been marked resolved. + /// + /// Resolution belongs to the thread rather than to any one comment, and + /// every host records it on the thread's first comment, so the flag is left + /// where the host put it and read back off the root — see + /// `PullRequestCommentNode.isResolved`. Copying it onto every reply would + /// mean the tree builder had to know what a thread is, which it does not. + var isResolved: Bool = false + /// Who resolved it, when the host says. + var resolvedBy: String? + /// The handle the host wants when the thread is resolved or opened again. + /// GitHub resolves a review *thread* by its GraphQL node id, Bitbucket the + /// comment the thread hangs off; `nil` means this thread cannot be resolved + /// from here and the button is not drawn. + var resolveToken: String? + /// Bitbucket Data Center refuses to write a comment back without the + /// version it last read, so it is carried along with the handle. + var resolveVersion: Int? var canReply: Bool { replyToken != nil } + var canResolve: Bool { resolveToken != nil } + /// Where in the diff this comment belongs, when it belongs anywhere. var anchor: DiffLineAnchor? { guard let path, let line else { return nil } @@ -67,6 +87,23 @@ struct PullRequestCommentNode: Identifiable, Sendable, Hashable { var replies: [PullRequestCommentNode] = [] var id: String { comment.id } + + /// Whether the whole thread is settled. The root carries the host's answer, + /// so a reply never has to be consulted. + var isResolved: Bool { comment.isResolved } + + /// Everything said under this comment, however deep it is nested. + var totalReplies: Int { + replies.reduce(replies.count) { $0 + $1.totalReplies } + } + + /// The first line with anything in it, for a thread shown as one row. + var preview: String { + comment.body + .split(separator: "\n", omittingEmptySubsequences: false) + .map { $0.trimmingCharacters(in: .whitespaces) } + .first { !$0.isEmpty } ?? "" + } } extension PullRequestComment { @@ -202,11 +239,112 @@ extension PullRequestService { ) } - items.append(contentsOf: await gitHubReviewComments(for: pr, in: directory)) + var inline = await gitHubReviewComments(for: pr, in: directory) + await applyGitHubThreads(to: &inline, for: pr, in: directory) + items.append(contentsOf: inline) 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. + /// + /// 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( + to comments: inout [PullRequestComment], + for pr: PullRequest, + in directory: URL + ) async { + guard !comments.isEmpty, + let repository = await gitHubRepository(for: pr, in: directory) + else { return } + + // `gh api graphql` does not expand {owner}/{repo} the way `gh api` does, + // 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}}}}}}}" + let result = await GitHubCLI.run( + ["api", "graphql", + "-f", "query=\(query)", + "-f", "owner=\(repository.owner)", + "-f", "repo=\(repository.name)", + "-F", "number=\(pr.number)"], + in: directory, + timeout: 60 + ) + guard result.isSuccess else { return } + + struct Response: Decodable { + struct Root: Decodable { let repository: Repository? } + struct Repository: Decodable { let pullRequest: PullRequestNode? } + struct PullRequestNode: Decodable { let reviewThreads: Threads? } + struct Threads: Decodable { let nodes: [Thread]? } + struct Thread: Decodable { + struct Comments: Decodable { let nodes: [Comment]? } + struct Comment: Decodable { let databaseId: Int? } + let id: String? + let isResolved: Bool? + let comments: Comments? + } + let data: Root? + } + + guard let response = try? JSONDecoder().decode(Response.self, from: Data(result.stdout.utf8)), + let threads = response.data?.repository?.pullRequest?.reviewThreads?.nodes + else { return } + + var byCommentID: [String: (token: String, isResolved: Bool)] = [:] + for thread in threads { + 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) + } + } + + for index in comments.indices { + guard let thread = byCommentID[comments[index].id] else { continue } + comments[index].resolveToken = thread.token + comments[index].isResolved = thread.isResolved + } + } + + /// The owner and name GraphQL needs, from the pull request when the remote + /// named them and from `gh` itself otherwise. + private static func gitHubRepository( + for pr: PullRequest, + in directory: URL + ) async -> (owner: String, name: String)? { + if !pr.repositoryOwner.isEmpty, !pr.repositorySlug.isEmpty { + return (pr.repositoryOwner, pr.repositorySlug) + } + let result = await GitHubCLI.run( + ["repo", "view", "--json", "owner,name"], + in: directory, + timeout: 30 + ) + guard result.isSuccess else { return nil } + + struct Repository: Decodable { + struct Owner: Decodable { let login: String? } + let owner: Owner? + let name: String? + } + + guard let decoded = try? JSONDecoder().decode(Repository.self, from: Data(result.stdout.utf8)), + let owner = decoded.owner?.login, let name = decoded.name, + !owner.isEmpty, !name.isEmpty + else { return nil } + return (owner, name) + } + /// Inline review comments. `gh pr view` does not expose them, and they are /// the only GitHub comments that actually form threads (`in_reply_to_id`). private static func gitHubReviewComments( @@ -365,6 +503,11 @@ extension PullRequestService { let toLine = inline?["to"] as? Int let fromLine = inline?["from"] as? Int + // Cloud hangs a `resolution` object off the thread's first comment + // and leaves it out entirely while the thread is open, so its mere + // presence is the answer. + let resolution = item["resolution"] as? [String: Any] + return PullRequestComment( id: identifier, parentID: parent.map { "\($0)" }, @@ -376,7 +519,10 @@ extension PullRequestService { path: inline?["path"] as? String, line: toLine ?? fromLine, side: toLine != nil ? .new : .old, - replyToken: identifier + replyToken: identifier, + isResolved: resolution != nil, + resolvedBy: BitbucketUser.name(from: resolution?["user"] as? [String: Any]), + resolveToken: identifier ) } .sorted { ($0.createdAt ?? .distantPast) < ($1.createdAt ?? .distantPast) } @@ -432,6 +578,15 @@ extension PullRequestService { // Cloud-style parenting; Data Center nests instead, handled below. let parent = (dictionary["parent"] as? [String: Any])?["id"] + // Data Center names the thread's standing in `state`; Cloud, which + // can reach this decoder through `bkt pr view`, still says it with + // a `resolution` object. + let resolution = dictionary["resolution"] as? [String: Any] + let isResolved = (dictionary["state"] as? String)?.uppercased() == "RESOLVED" + || resolution != nil + let resolver = (dictionary["resolver"] as? [String: Any]) + ?? (resolution?["user"] as? [String: Any]) + return PullRequestComment( id: identifier, parentID: parent.map { "\($0)" }, @@ -443,7 +598,11 @@ extension PullRequestService { path: path, line: toLine ?? fromLine ?? anchorLine, side: isOldSide ? .old : .new, - replyToken: dictionary["id"].map { "\($0)" } + replyToken: dictionary["id"].map { "\($0)" }, + isResolved: isResolved, + resolvedBy: BitbucketUser.name(from: resolver), + resolveToken: dictionary["id"].map { "\($0)" }, + resolveVersion: dictionary["version"] as? Int ) } @@ -702,6 +861,86 @@ extension PullRequestService { } } + /// Settles a thread, or opens it again. `comment` is the thread's root, the + /// only comment either host records resolution on. + static func setResolved( + _ resolved: Bool, + for comment: PullRequestComment, + on pr: PullRequest, + in directory: URL + ) async throws { + guard let token = comment.resolveToken else { + throw PullRequestError.commandFailed( + "This host does not let that thread be resolved from here." + ) + } + + switch pr.host { + case .github: + // Resolution is a GraphQL-only idea on GitHub, and it names the + // review thread rather than any comment in it — which is why the + // conversation is loaded with that node id already in hand. + let mutation = "mutation($id:ID!){" + + (resolved ? "resolveReviewThread" : "unresolveReviewThread") + + "(input:{threadId:$id}){thread{id isResolved}}}" + let result = await GitHubCLI.run( + ["api", "graphql", "-f", "query=\(mutation)", "-f", "id=\(token)"], + in: directory, + timeout: 60 + ) + guard result.isSuccess else { + throw PullRequestError.commandFailed(result.failureMessage) + } + + case .bitbucket: + guard !pr.repositoryOwner.isEmpty, !pr.repositorySlug.isEmpty else { + throw PullRequestError.commandFailed( + "This repository's workspace and slug are unknown, so the thread cannot be resolved." + ) + } + + // Cloud has an endpoint of its own for it, where the method is the + // whole message: PUT settles the thread, DELETE opens it again. + let cloud = await Shell.run( + ["bkt", "api", + "/2.0/repositories/\(pr.repositoryOwner)/\(pr.repositorySlug)" + + "/pullrequests/\(pr.number)/comments/\(token)/resolve", + "--method", resolved ? "PUT" : "DELETE"], + in: directory, + timeout: 60 + ) + if cloud.isSuccess { return } + + // Data Center instead writes the comment back with a new `state`, + // and refuses the write unless it is told which version is being + // replaced. Without one there is nothing honest to send, so Cloud's + // refusal is what the user is shown. + guard let version = comment.resolveVersion, + let data = try? JSONSerialization.data(withJSONObject: [ + "state": resolved ? "RESOLVED" : "OPEN", + "version": version, + ]) + else { + throw PullRequestError.commandFailed(cloud.failureMessage) + } + let dataCenter = await Shell.run( + ["bkt", "api", + "/rest/api/1.0/projects/\(pr.repositoryOwner)/repos/\(pr.repositorySlug)" + + "/pull-requests/\(pr.number)/comments/\(token)", + "--method", "PUT", + "--input", String(decoding: data, as: UTF8.self)], + in: directory, + timeout: 60 + ) + guard dataCenter.isSuccess else { + throw PullRequestError.commandFailed(dataCenter.failureMessage) + } + + case .unknown: + throw PullRequestError.unsupportedHost + } + } + static func parseTimestamp(_ string: String) -> Date? { let withFraction = ISO8601DateFormatter() withFraction.formatOptions = [.withInternetDateTime, .withFractionalSeconds] diff --git a/Sources/Workspace/Models/ViewerItem.swift b/Sources/Workspace/Models/ViewerItem.swift index e4a8d65..a7dddd7 100644 --- a/Sources/Workspace/Models/ViewerItem.swift +++ b/Sources/Workspace/Models/ViewerItem.swift @@ -138,6 +138,11 @@ final class ViewerItem: Identifiable { var syncState: PullRequestSyncState? var isCheckingSync = false var isRunningPullRequestAction = false + /// Whether everything about the pull request is being read again. It is not + /// `isRunningPullRequestAction`: that one means something is being *changed* + /// on the host and greys out the whole action bar, and a refresh changes + /// nothing. Two flags also keep two spinners from turning at once. + var isRefreshingPullRequest = false var isLoading = false var errorMessage: String? diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 4325c6d..bdf4165 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -1109,6 +1109,78 @@ final class WorkspaceStore { } } + /// Reads everything this pull request is made of again, from the host. + /// + /// The same things ``openPullRequest(_:project:)`` fetches the first time + /// round, minus the shortcuts it takes: the request itself — its title, + /// state, draft flag, approvals and description all live on that one object + /// — the diff, the conversation, who is reviewing, the CI runs and how far + /// the branch has drifted. The commits come along only if the tab that + /// shows them has already been opened; a review that never went there + /// should not start paying for the call now. + /// + /// They are independent reads, so they go out together the way the + /// dashboard's do: the wait is the slowest host call rather than the sum of + /// them, and each writes what it owns as it lands. + func refreshPullRequest(_ item: ViewerItem, project: Project, pr: PullRequest) async { + // Not while a merge, a rejection or a sync is on its way: what comes + // back would be the state from before it landed. One refresh at a time, + // too — the button is easy to lean on. + guard !item.isRefreshingPullRequest, !item.isRunningPullRequestAction else { return } + item.isRefreshingPullRequest = true + defer { item.isRefreshingPullRequest = false } + + async let request: Void = reloadPullRequest(item, project: project, pr: pr) + async let diff: Void = loadPullRequestDiff(item, project: project, pr: pr) + async let comments: Void = loadComments(item, project: project, pr: pr) + async let builds: Void = loadBuilds(item, project: project, pr: pr) + async let commits: Void = reloadCommitsIfShown(item, project: project, pr: pr) + // Fetching first, because the branch may well have moved on the host + // since it was last counted here. + async let sync: Void = refreshSyncState(item, project: project, pr: pr, fetching: true) + _ = await (request, diff, comments, builds, commits, sync) + + showStatus("#\(pr.number) refreshed") + } + + /// The pull request object itself, re-read from the host. It is what the + /// summary bar and the header are drawn from, so a refresh that left it + /// alone would reload the diff under a title, a state and an approvals + /// count from whenever the request was opened. + private func reloadPullRequest(_ item: ViewerItem, project: Project, pr: PullRequest) async { + guard let remote = project.remote, remote.kind != .unknown else { return } + var latest = pr + do { + latest = try await PullRequestService.load( + number: pr.number, + for: remote, + in: project.url + ) + item.pullRequest = latest + // The header names the pull request by its title, and a title is + // one of the things that can have changed on the host. + item.title = latest.title + } catch { + // Whatever else lands is still worth showing, so this says what + // went wrong and leaves the rest of the refresh to finish. + showError(error.localizedDescription) + } + // Both hosts hand the reviewers over with the request, so this usually + // costs nothing beyond the call just made. + await loadReviewers(item, project: project, pr: latest) + if let named = await PullRequestService.namedMentions(in: latest, directory: project.url) { + item.pullRequest?.body = named + } + } + + /// The commits, but only for a Commits tab that has already been opened — + /// see ``loadCommits(_:project:pr:)`` for why they are not fetched with + /// everything else in the first place. + private func reloadCommitsIfShown(_ item: ViewerItem, project: Project, pr: PullRequest) async { + guard !item.commits.isEmpty else { return } + await loadCommits(item, project: project, pr: pr) + } + /// Opens a pull request known only by its number — a `#123` written in a /// commit message. The list the navigator holds is of open requests only, /// and a commit usually names one that has already merged, so anything not @@ -1285,6 +1357,11 @@ final class WorkspaceStore { item.isLoadingCommits = false } + /// The CI runs, read out loud: the spinner turns while it is in flight, and + /// a refusal replaces the list with what the host said. This is the first + /// look at the panel and the reload button — both are somebody waiting for + /// an answer, so both are allowed to say they are working and to fail + /// visibly. func loadBuilds(_ item: ViewerItem, project: Project, pr: PullRequest) async { item.isLoadingBuilds = true item.buildsError = nil @@ -1297,6 +1374,23 @@ final class WorkspaceStore { item.isLoadingBuilds = false } + /// The same runs, read quietly, for the panel's ticker. + /// + /// Nobody asked for this one, so it says nothing: no spinner to flash every + /// ten seconds, and a `gh` or `bkt` that stumbles once leaves the last good + /// list where it is rather than emptying the panel under someone reading it. + /// The list is only assigned when it has actually changed, so a tick that + /// finds the same jobs redraws nothing. + func refreshBuilds(_ item: ViewerItem, project: Project, pr: PullRequest) async { + guard let builds = try? await PullRequestService.builds(for: pr, in: project.url) else { + return + } + if item.builds != builds { item.builds = builds } + // The host is answering again, so whatever the last failure left on + // screen is out of date. + if item.buildsError != nil { item.buildsError = nil } + } + /// Opens one commit inside the Commits tab and fetches its patch. func showCommit( _ commit: PullRequestCommit, @@ -1504,6 +1598,36 @@ final class WorkspaceStore { 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 + /// saying so. + func setCommentResolved( + _ resolved: Bool, + for comment: PullRequestComment, + on item: ViewerItem, + project: Project, + pr: PullRequest + ) async { + item.isPostingComment = true + item.commentError = nil + do { + try await PullRequestService.setResolved( + resolved, + for: comment, + on: pr, + in: project.url + ) + showStatus(resolved + ? "Thread resolved on #\(pr.number)" + : "Thread reopened on #\(pr.number)") + await loadComments(item, project: project, pr: pr) + } catch { + item.commentError = error.localizedDescription + } + item.isPostingComment = false + } + /// A comment with its mentions in the form the host wants. /// /// The composer writes the **name**, because that is what the person typing diff --git a/Sources/Workspace/Views/DiffView.swift b/Sources/Workspace/Views/DiffView.swift index 2a6fb56..05c8376 100644 --- a/Sources/Workspace/Views/DiffView.swift +++ b/Sources/Workspace/Views/DiffView.swift @@ -307,7 +307,10 @@ struct DiffView: View { isPosting: comments?.isPosting ?? false, replyingTo: $replyingTo, mentions: comments?.mentions ?? .none, - onReply: { parent, body in await comments?.reply(parent, body) } + 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 ) .frame(width: width, alignment: .leading) case .composer: @@ -361,6 +364,9 @@ struct DiffComments { var add: (DiffLineAnchor, String) async -> Void /// Replies to a comment in an existing thread. var reply: (PullRequestComment, String) async -> Void + /// 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)? } extension DiffRow { diff --git a/Sources/Workspace/Views/InlineCommentViews.swift b/Sources/Workspace/Views/InlineCommentViews.swift index 62d61cb..b6a17cd 100644 --- a/Sources/Workspace/Views/InlineCommentViews.swift +++ b/Sources/Workspace/Views/InlineCommentViews.swift @@ -37,6 +37,7 @@ struct DiffCommentThreads: View { @Binding var replyingTo: PullRequestComment? var mentions: MentionSource = .none let onReply: (PullRequestComment, String) async -> Void + var onResolve: ((PullRequestComment, Bool) async -> Void)? var body: some View { VStack(alignment: .leading, spacing: 8) { @@ -47,7 +48,10 @@ struct DiffCommentThreads: View { replyingTo: $replyingTo, isPosting: isPosting, mentions: mentions, - onReply: onReply + // The line it hangs under already says which file this is. + isInline: true, + onReply: onReply, + onResolve: onResolve ) } } @@ -64,6 +68,86 @@ struct DiffCommentThreads: View { } } +/// The green mark a settled thread wears, drawn like the review states beside +/// it: a word in a tinted capsule. +struct ResolvedChip: View { + var resolvedBy: String? + + var body: some View { + Label("Resolved", systemImage: "checkmark.circle.fill") + .font(.caption2) + .padding(.horizontal, 5) + .padding(.vertical, 1) + .background(.green.opacity(0.16), in: Capsule()) + .foregroundStyle(.green) + .help(resolvedBy.map { "Resolved by \($0)" } ?? "Resolved") + } +} + +/// The single row a settled thread is shown as until someone asks for the rest +/// of it. +/// +/// A resolved thread is answered business, and a long review leaves a page of +/// them between the parts still worth reading — so it keeps only what says +/// whose it was, that it is closed, and how much is behind the row. +struct ResolvedThreadRow: View { + let node: PullRequestCommentNode + let isExpanded: Bool + /// Inline in the diff the path is dropped: the line it is drawn under is + /// the file, and there is no room for it. + var showsPath = true + let onTap: () -> Void + + var body: some View { + Button(action: onTap) { + HStack(spacing: 6) { + Image(systemName: isExpanded ? "chevron.down" : "chevron.right") + .font(.caption2) + .foregroundStyle(.tertiary) + AuthorAvatar(name: node.comment.author, url: node.comment.avatarURL, size: 18) + Text(node.comment.author) + .font(.caption.weight(.semibold)) + .foregroundStyle(node.comment.authorColor) + ResolvedChip(resolvedBy: node.comment.resolvedBy) + + if showsPath, let path = node.comment.path { + Text(path) + .font(.system(.caption2, design: .monospaced)) + .foregroundStyle(.secondary) + .lineLimit(1) + .truncationMode(.head) + // The preview is what the row is read for; the path + // gives up its width first. + .layoutPriority(-1) + } + + if !isExpanded, !node.preview.isEmpty { + Text(node.preview) + .font(.caption) + .foregroundStyle(.secondary) + .lineLimit(1) + } + + Spacer(minLength: 4) + + if node.totalReplies > 0 { + Label("\(node.totalReplies)", systemImage: "arrowshape.turn.up.left") + .font(.caption2) + .foregroundStyle(.secondary) + } + } + .padding(.horizontal, 9) + .padding(.vertical, 6) + .frame(maxWidth: .infinity, alignment: .leading) + .background(.quaternary.opacity(0.16), in: RoundedRectangle(cornerRadius: 8)) + .contentShape(RoundedRectangle(cornerRadius: 8)) + } + .buttonStyle(.plain) + .pointerCursor() + .help(isExpanded ? "Hide this resolved thread" : "Show this resolved thread") + } +} + /// The box for a brand new comment on a line of the diff. struct DiffCommentComposer: View { let anchor: DiffLineAnchor diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index c68d665..45fe99e 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -93,6 +93,7 @@ struct PullRequestDetailView: View { descriptionDraft = nil isFetchingDescription = false } + .task(id: item.id) { await watchBuilds() } // Every button in the action bar ends up here first: one sheet, one // shape, and nothing sent to the host until it is confirmed. .sheet(item: $pendingAction) { action in @@ -132,6 +133,38 @@ struct PullRequestDetailView: View { } } + /// Reads the CI runs, and goes on reading them for as long as this pull + /// request is the one on screen. + /// + /// The app watches for things rather than polling for them, and this is the + /// deliberate exception: a build finishing is not an event either host + /// tells us about, so the window asks. It asks narrowly — SwiftUI cancels + /// this task when the pull request is closed or another takes its place, so + /// the loop dies with what it belongs to, and a window nobody is in front + /// of spends nothing on the host. + /// + /// It lives here rather than in the panel that draws the runs because the + /// badge in the summary bar reads them too, and that bar is on screen on + /// the Diff and Commits tabs, where the panel is not. + private func watchBuilds() async { + if item.builds.isEmpty, !item.isLoadingBuilds, item.buildsError == nil { + await store.loadBuilds(item, project: project, pr: pr) + } + + while !Task.isCancelled { + do { + try await Task.sleep(for: .seconds(10)) + } catch { + return + } + // Nothing behind a hidden window, and nothing on top of a reload + // already on its way with a spinner of its own. Either way the next + // tick tries again. + guard NSApplication.shared.isActive, !item.isLoadingBuilds else { continue } + await store.refreshBuilds(item, project: project, pr: pr) + } + } + /// Runs what the sheet confirmed. private func perform(_ action: PullRequestAction, comment: String) { Task { @@ -211,6 +244,7 @@ struct PullRequestDetailView: View { .foregroundStyle(.secondary) } tabPicker + refreshButton actionsMenu } .font(.caption.monospaced()) @@ -502,6 +536,30 @@ struct PullRequestDetailView: View { .pointerCursor() } + /// Everything about the pull request, read again from the host. It is the + /// one thing here that is wanted often enough to be worth a click rather + /// than a trip through the menu, so it sits in the bar as well. + @ViewBuilder + private var refreshButton: some View { + // Same treatment the action bar gives a merge or a sync: while it is in + // flight the bar says so, and there is nothing to press. + if item.isRefreshingPullRequest { + ProgressView().controlSize(.small) + } else { + barButton( + "arrow.clockwise", + help: "Reload #\(pr.number) from \(pr.host.displayName)" + ) { + refresh() + } + .disabled(item.isRunningPullRequestAction) + } + } + + private func refresh() { + Task { await store.refreshPullRequest(item, project: project, pr: pr) } + } + /// What is left once copy and check out have their own buttons, folded into a /// menu so the bar stays one line tall. private var actionsMenu: some View { @@ -537,19 +595,9 @@ struct PullRequestDetailView: View { } Divider() Button { - Task { await store.loadPullRequestDiff(item, project: project, pr: pr) } + refresh() } label: { - Label("Reload Diff", systemImage: "arrow.clockwise") - } - Button { - Task { await store.loadCommits(item, project: project, pr: pr) } - } label: { - Label("Reload Commits", systemImage: "arrow.clockwise") - } - Button { - Task { await store.loadBuilds(item, project: project, pr: pr) } - } label: { - Label("Reload Builds", systemImage: "arrow.clockwise") + Label("Refresh", systemImage: "arrow.clockwise") } } label: { Image(systemName: "ellipsis.circle") @@ -751,6 +799,15 @@ struct PullRequestDetailView: View { pr: pr, replyingTo: parent ) + }, + resolve: { comment, resolved in + await store.setCommentResolved( + resolved, + for: comment, + on: item, + project: project, + pr: pr + ) } ) ) @@ -1063,6 +1120,15 @@ struct ConversationView: View { mentions: mentions, onReply: { parent, body in await post(body, replyingTo: parent) + }, + onResolve: { comment, resolved in + await store.setCommentResolved( + resolved, + for: comment, + on: item, + project: project, + pr: pr + ) } ) } @@ -1154,66 +1220,99 @@ struct CommentThread: View { @Binding var replyingTo: PullRequestComment? let isPosting: Bool var mentions: MentionSource = .none + /// Drawn in the diff rather than in the conversation, where there is less + /// room and the file is already known. + var isInline = false 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)? + + /// 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 + /// tells it, which is with the answered parts out of the way. + @State private var isExpanded = false /// Stop indenting past this depth so deep threads stay readable. private static let maxIndentedDepth = 4 private static let indent: CGFloat = 18 + /// Only a root stands for a thread, and only a thread can be resolved — a + /// reply under it is part of what was settled, not a thing of its own. + private var isSettled: Bool { depth == 0 && node.isResolved } + var body: some View { VStack(alignment: .leading, spacing: 8) { - CommentBubble( - comment: node.comment, - replyCount: depth == 0 ? totalReplies : 0, - isReplying: replyingTo == node.comment, - onReplyTapped: { - replyingTo = replyingTo == node.comment ? nil : node.comment - } - ) - - if replyingTo == node.comment { - CommentComposer( - prompt: "Reply to \(node.comment.author)… @ to name someone", - sendTitle: "Reply", - isPosting: isPosting, - mentions: mentions, - onCancel: { replyingTo = nil }, - onSend: { body in await onReply(node.comment, body) } + if isSettled { + ResolvedThreadRow( + node: node, + isExpanded: isExpanded, + showsPath: !isInline, + onTap: { isExpanded.toggle() } ) - .padding(.leading, Self.indent) } - if !node.replies.isEmpty { - VStack(alignment: .leading, spacing: 8) { - ForEach(node.replies) { reply in - CommentThread( - node: reply, - depth: depth + 1, - replyingTo: $replyingTo, - isPosting: isPosting, - mentions: mentions, - onReply: onReply - ) - } - } - .padding(.leading, depth < Self.maxIndentedDepth ? Self.indent : 0) - .overlay(alignment: .leading) { - // The rule that ties a reply back to what it answers. - Rectangle() - .fill(.quaternary) - .frame(width: 1) - .padding(.vertical, 2) - } + if !isSettled || isExpanded { + thread } } } - /// Every descendant, not just the direct children. - private var totalReplies: Int { - node.replies.reduce(node.replies.count) { total, reply in - total + reply.replies.reduce(0) { $0 + 1 + $1.replies.count } + @ViewBuilder + private var thread: some View { + CommentBubble( + comment: node.comment, + replyCount: depth == 0 ? node.totalReplies : 0, + isReplying: replyingTo == node.comment, + isBusy: isPosting, + onReplyTapped: { + replyingTo = replyingTo == node.comment ? nil : node.comment + }, + onResolveTapped: resolveAction + ) + + if replyingTo == node.comment { + CommentComposer( + prompt: "Reply to \(node.comment.author)… @ to name someone", + sendTitle: "Reply", + isPosting: isPosting, + mentions: mentions, + onCancel: { replyingTo = nil }, + onSend: { body in await onReply(node.comment, body) } + ) + .padding(.leading, Self.indent) + } + + if !node.replies.isEmpty { + VStack(alignment: .leading, spacing: 8) { + ForEach(node.replies) { reply in + CommentThread( + node: reply, + depth: depth + 1, + replyingTo: $replyingTo, + isPosting: isPosting, + mentions: mentions, + isInline: isInline, + onReply: onReply, + onResolve: onResolve + ) + } + } + .padding(.leading, depth < Self.maxIndentedDepth ? Self.indent : 0) + .overlay(alignment: .leading) { + // The rule that ties a reply back to what it answers. + Rectangle() + .fill(.quaternary) + .frame(width: 1) + .padding(.vertical, 2) + } } } + + /// What the Resolve button does, or nil where it is not drawn at all. + private var resolveAction: (() -> Void)? { + guard depth == 0, node.comment.canResolve, let onResolve else { return nil } + return { Task { await onResolve(node.comment, !node.isResolved) } } + } } // MARK: - Reviewers @@ -1857,7 +1956,12 @@ struct CommentBubble: View { /// Shown on a thread root so a collapsed-looking thread still reads as one. var replyCount: Int = 0 var isReplying = false + /// A write is already on its way to the host, so nothing else may be asked. + var isBusy = false 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)? var body: some View { VStack(alignment: .leading, spacing: 5) { @@ -1909,17 +2013,40 @@ struct CommentBubble: View { .font(.callout) } - if comment.canReply, let onReplyTapped { - Button(action: onReplyTapped) { - Label( - isReplying ? "Cancel reply" : "Reply", - systemImage: "arrowshape.turn.up.left" - ) - .font(.caption) + if (comment.canReply && onReplyTapped != nil) || onResolveTapped != nil { + HStack(spacing: 12) { + if comment.canReply, let onReplyTapped { + Button(action: onReplyTapped) { + Label( + isReplying ? "Cancel reply" : "Reply", + systemImage: "arrowshape.turn.up.left" + ) + .font(.caption) + } + .buttonStyle(.plain) + .foregroundStyle(.secondary) + .pointerCursor() + } + + if let onResolveTapped { + Button(action: onResolveTapped) { + Label( + comment.isResolved ? "Unresolve" : "Resolve", + systemImage: comment.isResolved + ? "arrow.uturn.backward.circle" + : "checkmark.circle" + ) + .font(.caption) + } + .buttonStyle(.plain) + .foregroundStyle(comment.isResolved ? Color.secondary : Color.green) + .disabled(isBusy) + .pointerCursor(!isBusy) + .help(comment.isResolved + ? "Mark this thread unresolved" + : "Mark this thread resolved") + } } - .buttonStyle(.plain) - .foregroundStyle(.secondary) - .pointerCursor() .padding(.top, 1) } } diff --git a/Sources/Workspace/Views/PullRequestSidebar.swift b/Sources/Workspace/Views/PullRequestSidebar.swift index bd889f5..848a06f 100644 --- a/Sources/Workspace/Views/PullRequestSidebar.swift +++ b/Sources/Workspace/Views/PullRequestSidebar.swift @@ -34,15 +34,9 @@ struct PullRequestSidebar: View { .frame(maxWidth: .infinity, alignment: .leading) } .background(Color(nsColor: AppColors.viewerBackground)) - // The CI runs cost a call to the host, so they are read the first time - // the panel is on screen rather than with the pull request. A failure is - // not retried on its own — that is what the reload button is for. - .task { - guard item.builds.isEmpty, !item.isLoadingBuilds, item.buildsError == nil else { - return - } - await store.loadBuilds(item, project: project, pr: pr) - } + // Reading the runs, and going on reading them, belongs to the view + // above this one: the badge in its summary bar shows them on the tabs + // where this panel is not drawn at all. See `watchBuilds`. } // MARK: - Reviewers diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index aafe6ec..62559d5 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -224,20 +224,18 @@ struct ViewerView: View { if item.isDirty { Circle().fill(.orange).frame(width: 6, height: 6) } - // Only a pull request has a page to link to, and this is where its - // number and title are — the link is for pasting into a chat or a - // ticket, so it belongs next to what it points at. + // Only a pull request has a page of its own, and this is where its + // number and title are — so the way over to it belongs next to what + // it points at. No toast: the browser coming forward says it. if let url = item.pullRequest?.url { Button { - NSPasteboard.general.clearContents() - NSPasteboard.general.setString(url.absoluteString, forType: .string) - store.showStatus("Pull request link copied") + NSWorkspace.shared.open(url) } label: { - Image(systemName: "link") + Image(systemName: "safari") .font(.caption) } .foregroundStyle(.secondary) - .help("Copy the link to \(item.title)") + .help("Open \(item.title) on the host in your browser") .pointerCursor() } // A commit gets the same treatment, for the same reason: the hash is From 33a57cddf403a15045f254e6a14f75a70b10202c Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 12:20:18 +0700 Subject: [PATCH 2/7] The resolve Bitbucket will take, and a screen that stops redrawing itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Resolving on Bitbucket went through the wrong door.** The Cloud endpoint `/pullrequests/{n}/comments/{id}/resolve` answered a `bkt api` call with 403 and "this endpoint does not support token-based authentication" — the credential `bkt api` presents is not one that endpoint accepts, whatever the account is signed in with. `bkt` has the verb itself, `pr comments resolve` and `reopen`, and its own help says both work on Cloud and on Data Center. Going through the CLI is what the rest of this app already does with these tools, and it is the thing that knows how to authenticate. The hand-rolled Data Center write goes with it, and `resolveVersion` with that — it existed only to feed it. Worth recording, because it contradicts what the last commit assumed: `bkt`'s help states the Data Center API does not expose resolution status when reading comments. A settled thread there will most likely read as open. Resolving and reopening still work; it is the display that cannot be trusted on that host. **And the pull request screen stops doing its work over again.** Measured over a 120-comment review — 40 threads of a root and two replies — one pass of the conversation's `body` was spending 7.4 ms re-parsing Markdown that had not changed and 0.5 ms rebuilding a comment tree out of the same comments, against a 16.7 ms frame. It was paying that on every keystroke in the comment box, on every ten-second build tick, and on every frame of a drag on the side panel's seam. Nothing about that work was needed twice. The thread tree is built where the comments land — `ViewerItem.setComments` keeps the flat list, the nested one and the by-line index in step — instead of in the `body` of every view that draws it. A node counts its replies as it is built. Inline Markdown is cached the way the block split already was; both caches now set the live half aside rather than emptying, because a total flush threw away exactly what the overflowing pass was about to ask for again. The rest is about who owns what. The description editor and the comment composer own the text being typed into them, so a letter no longer redraws the page it sits on. The builds badge is the view that reads the runs, so the ticker's write cannot reach the conversation. The side panel's seam owns its own width. The reviewer sheet reads its list once a pass instead of rebuilding a set per row. And the cache of commits' patches is bounded at twelve — each one is parsed and syntax-highlighted, and stepping through fifty of them kept all fifty. Two real things were left alone on purpose. Tree-sitter highlighting runs on the main actor, so picking a file in a large diff's index blocks it; moving it off means auditing theme and cache isolation, which is a change of its own rather than a corner of this one. And `DiffComments` is rebuilt each pass, which is true but cheap now that it carries the stable index: the diff's rows are lazy and its flattening only rebuilds when the anchored lines change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CSpbKpa8inAaacg6rWETSS --- .../Workspace/Models/PullRequestComment.swift | 102 ++-- Sources/Workspace/Models/ViewerItem.swift | 55 +- Sources/Workspace/Models/WorkspaceStore.swift | 9 +- Sources/Workspace/Views/MarkdownPreview.swift | 85 ++- .../Views/PullRequestDetailView.swift | 521 +++++++++++------- .../Workspace/Views/PullRequestSidebar.swift | 11 +- 6 files changed, 507 insertions(+), 276 deletions(-) diff --git a/Sources/Workspace/Models/PullRequestComment.swift b/Sources/Workspace/Models/PullRequestComment.swift index 96f79ef..a5c1f4c 100644 --- a/Sources/Workspace/Models/PullRequestComment.swift +++ b/Sources/Workspace/Models/PullRequestComment.swift @@ -59,9 +59,6 @@ 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? - /// Bitbucket Data Center refuses to write a comment back without the - /// version it last read, so it is carried along with the handle. - var resolveVersion: Int? var canReply: Bool { replyToken != nil } @@ -83,8 +80,23 @@ struct DiffLineAnchor: Sendable, Hashable { /// One comment with its replies, ready to render. struct PullRequestCommentNode: Identifiable, Sendable, Hashable { - var comment: PullRequestComment - var replies: [PullRequestCommentNode] = [] + let comment: PullRequestComment + let replies: [PullRequestCommentNode] + + /// Everything said under this comment, however deep it is nested. + /// + /// Counted as the tree is built rather than each time it is read: the + /// number rides in a bubble's header and in every resolved thread's row, + /// both of which are drawn again for reasons that have nothing to do with + /// the conversation, and walking the whole subtree on each of those passes + /// is a cost that grows with the review. + let totalReplies: Int + + init(comment: PullRequestComment, replies: [PullRequestCommentNode] = []) { + self.comment = comment + self.replies = replies + totalReplies = replies.reduce(replies.count) { $0 + $1.totalReplies } + } var id: String { comment.id } @@ -92,11 +104,6 @@ struct PullRequestCommentNode: Identifiable, Sendable, Hashable { /// so a reply never has to be consulted. var isResolved: Bool { comment.isResolved } - /// Everything said under this comment, however deep it is nested. - var totalReplies: Int { - replies.reduce(replies.count) { $0 + $1.totalReplies } - } - /// The first line with anything in it, for a thread shown as one row. var preview: String { comment.body @@ -150,6 +157,20 @@ extension PullRequestComment { return nodes } + + /// The thread roots that hang off a line of the diff, keyed by that line. + /// A thread whose line is no longer in the diff simply does not appear; it + /// is still listed in the conversation, so nothing is lost. + static func inlineThreads( + in nodes: [PullRequestCommentNode] + ) -> [DiffLineAnchor: [PullRequestCommentNode]] { + var grouped: [DiffLineAnchor: [PullRequestCommentNode]] = [:] + for node in nodes { + guard let anchor = node.comment.anchor else { continue } + grouped[anchor, default: []].append(node) + } + return grouped + } } extension PullRequestService { @@ -601,8 +622,7 @@ extension PullRequestService { replyToken: dictionary["id"].map { "\($0)" }, isResolved: isResolved, resolvedBy: BitbucketUser.name(from: resolver), - resolveToken: dictionary["id"].map { "\($0)" }, - resolveVersion: dictionary["version"] as? Int + resolveToken: dictionary["id"].map { "\($0)" } ) } @@ -893,47 +913,25 @@ extension PullRequestService { } case .bitbucket: - guard !pr.repositoryOwner.isEmpty, !pr.repositorySlug.isEmpty else { - throw PullRequestError.commandFailed( - "This repository's workspace and slug are unknown, so the thread cannot be resolved." - ) + // `bkt` has the verb itself, and it knows both flavours: Cloud's + // resolve endpoint and Data Center's comment state are one command + // here. Going to the REST path directly instead is what a 403 was + // answering — Cloud's `/resolve` refuses the credential `bkt api` + // presents, and `bkt` is the thing that knows how to ask properly. + // It wants the thread's own comment, which is the only one this is + // ever called with. + var arguments = ["bkt", "pr", "comments", + resolved ? "resolve" : "reopen", + "\(pr.number)", token] + // The checkout usually says which repository this is, but bkt reads + // that from its own context; naming it keeps a context pointing + // somewhere else from settling the wrong thread. + if !pr.repositorySlug.isEmpty { + arguments += ["--repo", pr.repositorySlug] } - - // Cloud has an endpoint of its own for it, where the method is the - // whole message: PUT settles the thread, DELETE opens it again. - let cloud = await Shell.run( - ["bkt", "api", - "/2.0/repositories/\(pr.repositoryOwner)/\(pr.repositorySlug)" - + "/pullrequests/\(pr.number)/comments/\(token)/resolve", - "--method", resolved ? "PUT" : "DELETE"], - in: directory, - timeout: 60 - ) - if cloud.isSuccess { return } - - // Data Center instead writes the comment back with a new `state`, - // and refuses the write unless it is told which version is being - // replaced. Without one there is nothing honest to send, so Cloud's - // refusal is what the user is shown. - guard let version = comment.resolveVersion, - let data = try? JSONSerialization.data(withJSONObject: [ - "state": resolved ? "RESOLVED" : "OPEN", - "version": version, - ]) - else { - throw PullRequestError.commandFailed(cloud.failureMessage) - } - let dataCenter = await Shell.run( - ["bkt", "api", - "/rest/api/1.0/projects/\(pr.repositoryOwner)/repos/\(pr.repositorySlug)" - + "/pull-requests/\(pr.number)/comments/\(token)", - "--method", "PUT", - "--input", String(decoding: data, as: UTF8.self)], - in: directory, - timeout: 60 - ) - guard dataCenter.isSuccess else { - throw PullRequestError.commandFailed(dataCenter.failureMessage) + let result = await Shell.run(arguments, in: directory, timeout: 60) + guard result.isSuccess else { + throw PullRequestError.commandFailed(result.failureMessage) } case .unknown: diff --git a/Sources/Workspace/Models/ViewerItem.swift b/Sources/Workspace/Models/ViewerItem.swift index a7dddd7..158f6c6 100644 --- a/Sources/Workspace/Models/ViewerItem.swift +++ b/Sources/Workspace/Models/ViewerItem.swift @@ -89,8 +89,21 @@ final class ViewerItem: Identifiable { terminals.first { $0.id == selectedTerminalID } ?? terminals.last } - // Pull request conversation. - var comments: [PullRequestComment] = [] + // Pull request conversation. Assigned through `setComments(_:)`, which is + // what keeps the two shapes below in step with the flat list. + private(set) var comments: [PullRequestComment] = [] + /// The conversation as threads, nested and sorted. + /// + /// Built once, where the comments land, rather than in the views that draw + /// them: every one of those is a `body`, and a `body` runs again for + /// reasons that have nothing to do with the conversation — a key pressed in + /// the box at the foot of the page, the ten-second build tick, a drag on + /// the side panel's seam. Rebuilding the tree on each of those passes is + /// the same answer bought again, at a price that grows with the review. + private(set) var commentThreads: [PullRequestCommentNode] = [] + /// The same threads, keyed by the line of the diff they hang off — see + /// ``PullRequestComment/inlineThreads(in:)``. + private(set) var inlineCommentThreads: [DiffLineAnchor: [PullRequestCommentNode]] = [:] var isLoadingComments = false var isPostingComment = false var commentError: String? @@ -128,8 +141,12 @@ final class ViewerItem: Identifiable { var isLoadingBuilds = false var buildsError: String? /// Patches already fetched, keyed by hash: a commit does not change, so - /// stepping back and forth through the list costs one load each. - var commitDiffs: [String: Diff] = [:] + /// stepping back and forth through the list costs one load each. Written + /// through ``cacheCommitDiff(_:for:)``, which is what keeps the pile of + /// them from growing for as long as the pull request stays open. + private(set) var commitDiffs: [String: Diff] = [:] + /// The hashes in `commitDiffs`, oldest first — see `cacheCommitDiff`. + private var commitDiffOrder: [String] = [] var isLoadingCommitDiff = false var commitDiffError: String? @@ -222,6 +239,36 @@ final class ViewerItem: Identifiable { /// session away. nonisolated var survivesClosing: Bool { isTerminal } + /// Keeps a commit's patch for the next time it is opened, and lets the + /// oldest ones go. + /// + /// A patch here is not a string: it is parsed *and* syntax-coloured, an + /// `AttributedString` for every side of every row. Reading down a hundred + /// commits of a long pull request would otherwise hold every one of them + /// for as long as the tab stays open. A dozen is enough for stepping back + /// and forth, which is what the cache is for. + func cacheCommitDiff(_ diff: Diff, for sha: String) { + if commitDiffs[sha] == nil { commitDiffOrder.append(sha) } + commitDiffs[sha] = diff + while commitDiffOrder.count > 12 { + let oldest = commitDiffOrder.removeFirst() + // Never the one being read: a reader who steps back to it would + // find it gone and pay for it again. + if oldest == selectedCommit?.sha { + commitDiffOrder.append(oldest) + continue + } + commitDiffs[oldest] = nil + } + } + + /// The conversation, and everything drawn from it, in one write. + func setComments(_ comments: [PullRequestComment]) { + self.comments = comments + commentThreads = PullRequestComment.tree(from: comments) + inlineCommentThreads = PullRequestComment.inlineThreads(in: commentThreads) + } + init(kind: Kind, title: String, subtitle: String? = nil) { self.kind = kind self.title = title diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index bdf4165..406d55c 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -1234,9 +1234,9 @@ final class WorkspaceStore { item.isLoadingComments = true item.commentError = nil do { - item.comments = try await PullRequestService.comments(for: pr, in: project.url) + item.setComments(try await PullRequestService.comments(for: pr, in: project.url)) } catch { - item.comments = [] + item.setComments([]) item.commentError = error.localizedDescription } item.isLoadingComments = false @@ -1407,7 +1407,10 @@ final class WorkspaceStore { // The tab may have gone back to the list, or on to another commit, // while this was in flight — the patch is still worth keeping. if let text { - item.commitDiffs[commit.sha] = DiffHighlighter.highlight(await DiffParser.parseInBackground(text)) + item.cacheCommitDiff( + DiffHighlighter.highlight(await DiffParser.parseInBackground(text)), + for: commit.sha + ) } else if item.selectedCommit?.sha == commit.sha { item.commitDiffError = "Could not load the changes in \(commit.shortSHA)." } diff --git a/Sources/Workspace/Views/MarkdownPreview.swift b/Sources/Workspace/Views/MarkdownPreview.swift index 9eb5360..c1d039d 100644 --- a/Sources/Workspace/Views/MarkdownPreview.swift +++ b/Sources/Workspace/Views/MarkdownPreview.swift @@ -74,6 +74,41 @@ private struct CodeChipBackdrop: ViewModifier { } } +/// A cache that forgets in halves rather than all at once. +/// +/// The obvious cap — empty the whole thing once it grows past a limit — throws +/// away what is on screen along with what is not, and the pass that overflowed +/// it is usually the pass that then has to make all of it again: a pull request +/// with three hundred comments does exactly that. Keeping the generation before +/// the current one means anything still being read is at worst one lookup away, +/// and finding it there moves it forward, so what is on screen settles into the +/// live half and what has been scrolled past falls out of the old one. +private struct GenerationCache { + /// How much is held before the live half is set aside. Twice this is the + /// most that is ever kept. + let limit: Int + + private var live: [Key: Value] = [:] + private var previous: [Key: Value] = [:] + + init(limit: Int) { self.limit = limit } + + mutating func value(for key: Key) -> Value? { + if let value = live[key] { return value } + guard let value = previous[key] else { return nil } + live[key] = value + return value + } + + mutating func insert(_ value: Value, for key: Key) { + if live.count >= limit { + previous = live + live = [:] + } + live[key] = value + } +} + /// Full-page Markdown view for `.md` files: `MarkdownText` in a scroll view. struct MarkdownPreview: View { let text: String @@ -347,14 +382,43 @@ struct MarkdownText: View { } /// One line of styled text, with the chip backdrop layered behind it. + @MainActor fileprivate static func inline(_ source: String, baseSize: CGFloat = MarkdownText.bodySize) -> some View { - let text = inlineText(source, baseSize: baseSize) + let text = cachedInlineText(source, baseSize: baseSize) // Both layers are the same `Text`, and `.font`/`.foregroundStyle` set by // the caller reach the backdrop through the environment, so the two // always agree on how the text is laid out. return text.modifier(CodeChipBackdrop(text: text)) } + /// The styled line, remembered. + /// + /// Reading the Markdown in a line is the expensive half of drawing a + /// comment, and the blocks being cached does not save it: the parse of a + /// *line* happens here, on the way to a `Text`. A page of comments runs + /// through this on every pass of a body that has nothing to do with their + /// text — a build tick, a drag on the panel's seam — so the answer is kept, + /// the way ``cachedBlocks(in:relativeTo:)`` keeps the blocks above it. The + /// size is part of the key because a code span is set relative to whatever + /// block it sits in. + @MainActor + private static func cachedInlineText(_ source: String, baseSize: CGFloat) -> Text { + let key = InlineKey(text: source, size: baseSize) + if let cached = inlineCache.value(for: key) { return cached } + let text = inlineText(source, baseSize: baseSize) + inlineCache.insert(text, for: key) + return text + } + + private struct InlineKey: Hashable { + let text: String + let size: CGFloat + } + + /// Every line of every block, so it holds more than the block cache does. + @MainActor + private static var inlineCache = GenerationCache(limit: 600) + /// `baseSize` is the point size of the block the text sits in, so a code /// span can be set one point below whatever surrounds it — including inside /// a heading, where the run would otherwise be left at body size. @@ -446,22 +510,27 @@ struct MarkdownText: View { /// scroll, a hover, a window resize — and a page of PR comments is a stack /// of these. Parsing the same Markdown on each of those passes is work /// nobody sees, so the answer is kept, the way `MarkdownCodeHighlighter` - /// keeps its colours. The cap only stops a long reading session from - /// growing this without end. + /// keeps its colours. @MainActor private static func cachedBlocks(in text: String, relativeTo base: URL?) -> [Block] { // The folder is part of the key, not just the text: the same // `![](assets/logo.png)` is a different picture in another repository. - let key = "\(base?.path ?? "")\n\(text)" - if let cached = cache[key] { return cached } + // Two fields rather than one joined string, so a hit costs a hash of + // what is already there instead of a fresh copy of the whole document. + let key = BlockKey(text: text, base: base?.path ?? "") + if let cached = cache.value(for: key) { return cached } let parsed = blocks(in: text, relativeTo: base) - if cache.count > 200 { cache.removeAll() } - cache[key] = parsed + cache.insert(parsed, for: key) return parsed } + private struct BlockKey: Hashable { + let text: String + let base: String + } + @MainActor - private static var cache: [String: [Block]] = [:] + private static var cache = GenerationCache(limit: 200) static func blocks(in text: String, relativeTo base: URL? = nil) -> [Block] { // The bookkeeping a bot hides in `` goes first, so nothing diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 45fe99e..677686c 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -13,11 +13,6 @@ struct PullRequestDetailView: View { let pr: PullRequest let project: Project - /// How wide the side panel is, in this window. Unlike whether it is open — - /// which each pull request remembers for itself — a width is a habit of the - /// window rather than of the thing being read. - @State private var sidebarWidth: CGFloat = 268 - /// The button that was pressed, waiting to be confirmed. Nothing in the /// action bar reaches the host until this has been through the sheet — and /// one piece of state for all of them keeps it that way, where four separate @@ -26,25 +21,17 @@ struct PullRequestDetailView: View { /// Kept out here so the sheet reopens on the way that was picked last. @State private var mergeStrategy: PullRequestMergeStrategy = .squash - /// The description being written, as Markdown — `nil` while it is only being - /// read, which is what tells the two apart. It starts from what the host - /// stores rather than from what is on screen, and `isFetchingDescription` - /// covers the moment in between. + /// The description the editor was opened on, as Markdown — `nil` while it is + /// only being read, which is what tells the two apart. It starts from what + /// the host stores rather than from what is on screen, and + /// `isFetchingDescription` covers the moment in between. + /// + /// What is *typed* lives in the editor's own state rather than here: a + /// keystroke landing on this view would redraw the conversation under it, + /// comment by comment, on every letter. @State private var descriptionDraft: String? @State private var isFetchingDescription = false - /// Thread roots that hang off a line of the diff, keyed by that line. A - /// thread whose line is no longer in the diff simply does not appear here; - /// it is still listed in the conversation, so nothing is lost. - private var inlineThreads: [DiffLineAnchor: [PullRequestCommentNode]] { - var grouped: [DiffLineAnchor: [PullRequestCommentNode]] = [:] - for node in PullRequestComment.tree(from: item.comments) { - guard let anchor = node.comment.anchor else { continue } - grouped[anchor, default: []].append(node) - } - return grouped - } - var body: some View { VStack(spacing: 0) { summaryBar @@ -74,16 +61,13 @@ struct PullRequestDetailView: View { .frame(maxWidth: .infinity, maxHeight: .infinity) if item.pullRequestTab == .details { - PaneResizer(width: $sidebarWidth, range: 210...420, growsLeftwards: true) - PullRequestSidebar( + PullRequestSidebarPane( item: item, pr: pr, project: project, isOpen: isOpen, onAddReviewers: { pendingAction = .addReviewers } ) - .frame(width: sidebarWidth) - .frame(maxHeight: .infinity) } } } @@ -231,7 +215,10 @@ struct PullRequestDetailView: View { badge(review, color: review == "Approved" ? .green : .orange) } approvalsBadge - buildsBadge + // Its own view, and it is the view that reads the runs: the ticker + // behind them writes every ten seconds, and this bar is the top of + // the page the conversation hangs under. + PullRequestBuildsBadge(item: item) { item.pullRequestTab = .details } Spacer(minLength: 8) @@ -324,52 +311,6 @@ struct PullRequestDetailView: View { return .secondary } - /// How CI stands, in one glyph beside the approvals. It is the reason the - /// Builds tab could go: the news is in the bar whichever tab is open, and - /// clicking it goes to the list — back to Details, where the panel is. - /// Nothing is drawn until something has run: an empty badge would only be - /// one more thing to read. - @ViewBuilder - private var buildsBadge: some View { - if !item.builds.isEmpty { - let state = worstBuildState - Button { - item.pullRequestTab = .details - } label: { - HStack(spacing: 3) { - Image(systemName: state.symbol) - Text("\(item.builds.count)") - } - .font(.caption.weight(.medium)) - .padding(.horizontal, 6) - .padding(.vertical, 2) - .background(state.color.opacity(0.18), in: Capsule()) - .foregroundStyle(state.color) - } - .buttonStyle(.plain) - .help(buildsHelp) - .pointerCursor() - } - } - - /// The worst news among the runs — a single failure decides the badge, the - /// same way it decides whether the pull request can be merged. - private var worstBuildState: PullRequestBuild.State { - for state in [PullRequestBuild.State.failed, .running, .pending] where - item.builds.contains(where: { $0.state == state }) { - return state - } - return item.builds.contains { $0.state == .passed } ? .passed : .unknown - } - - private var buildsHelp: String { - let names = item.builds - .prefix(8) - .map { "\($0.name) — \($0.state.title.lowercased())" } - .joined(separator: "\n") - return "\(item.builds.count) builds on the head commit\n\(names)" - } - private var approvalsHelp: String { guard !item.reviewers.isEmpty else { return isOpen @@ -653,7 +594,13 @@ struct PullRequestDetailView: View { // down the right now, where it is on screen for the diff too. if let draft = descriptionDraft { - descriptionEditor(draft) + PullRequestDescriptionEditor( + initialText: draft, + host: pr.host, + isSaving: item.isRunningPullRequestAction, + onCancel: { descriptionDraft = nil }, + onSave: { text in await saveDescription(text) } + ) } else if !pr.body.isEmpty { MarkdownText(text: pr.body) .font(.callout) @@ -693,60 +640,6 @@ struct PullRequestDetailView: View { .pointerCursor(!isFetchingDescription) } - /// The description as its Markdown source: what is typed here is what the - /// host stores, and the drawn version comes back once it is saved. - private func descriptionEditor(_ draft: String) -> some View { - VStack(alignment: .leading, spacing: 7) { - TextEditor(text: Binding( - get: { descriptionDraft ?? "" }, - set: { descriptionDraft = $0 } - )) - .font(.system(.callout, design: .monospaced)) - .frame(height: editorHeight(for: draft)) - .scrollContentBackground(.hidden) - .padding(6) - .background(.quaternary.opacity(0.25), in: RoundedRectangle(cornerRadius: 7)) - .overlay(alignment: .topLeading) { - if draft.isEmpty { - Text("Describe the change, in Markdown…") - .foregroundStyle(.tertiary) - .padding(.horizontal, 11) - .padding(.vertical, 12) - .allowsHitTesting(false) - } - } - - HStack(spacing: 8) { - Text(pr.host == .github ? "Markdown, saved with gh" : "Markdown, saved with bkt") - .font(.caption2) - .foregroundStyle(.tertiary) - Spacer() - Button("Cancel") { descriptionDraft = nil } - .controlSize(.small) - .keyboardShortcut(.cancelAction) - .disabled(item.isRunningPullRequestAction) - .pointerCursor(!item.isRunningPullRequestAction) - Button { - Task { await saveDescription(draft) } - } label: { - if item.isRunningPullRequestAction { - ProgressView().controlSize(.small) - } else { - Label("Save", systemImage: "checkmark") - } - } - .buttonStyle(.borderedProminent) - .controlSize(.small) - .disabled(item.isRunningPullRequestAction) - // Post belongs to the comment box further down the same page, so - // saving takes the app's Save key instead. - .shortcut(.save) - .pointerCursor(!item.isRunningPullRequestAction) - .shortcutHelp("Save the description", .save) - } - } - } - /// Sends the description to the host, and closes the editor once it is /// there. A refusal leaves the box open with the text still in it. private func saveDescription(_ draft: String) async { @@ -754,14 +647,6 @@ struct PullRequestDetailView: View { if saved { descriptionDraft = nil } } - /// How tall the box stands: as tall as what is in it, within reason. A box - /// that scrolls inside a page that scrolls is two scrolls fighting for the - /// same wheel, so a description of any usual length gets to be one page. - private func editorHeight(for draft: String) -> CGFloat { - let lines = draft.reduce(1) { $1 == "\n" ? $0 + 1 : $0 } - return min(560, max(140, CGFloat(lines) * 17 + 20)) - } - @ViewBuilder private var diffTab: some View { if item.isLoading { @@ -773,7 +658,7 @@ struct PullRequestDetailView: View { layout: Binding(get: { item.diffLayout }, set: { item.diffLayout = $0 }), selectedFile: Binding(get: { item.diffFile }, set: { item.diffFile = $0 }), comments: DiffComments( - threads: inlineThreads, + threads: item.inlineCommentThreads, isPosting: item.isPostingComment, mentions: MentionSource( people: item.reviewerCandidates, @@ -1046,6 +931,190 @@ struct PullRequestDetailView: View { } } +// MARK: - The bar's own pieces + +/// How CI stands, in one glyph beside the approvals. It is the reason the +/// Builds tab could go: the news is in the bar whichever tab is open, and +/// clicking it goes to the list — back to Details, where the panel is. Nothing +/// is drawn until something has run: an empty badge would only be one more +/// thing to read. +/// +/// A view of its own so that **this** is what reads `builds`. The list is +/// re-read every ten seconds for as long as the pull request is open, and while +/// the badge lived in the bar's own body every one of those ticks that found a +/// job had moved on redrew the page of comments below it. +struct PullRequestBuildsBadge: View { + let item: ViewerItem + let onTap: () -> Void + + @ViewBuilder + var body: some View { + if !item.builds.isEmpty { + let state = worstState + Button(action: onTap) { + HStack(spacing: 3) { + Image(systemName: state.symbol) + Text("\(item.builds.count)") + } + .font(.caption.weight(.medium)) + .padding(.horizontal, 6) + .padding(.vertical, 2) + .background(state.color.opacity(0.18), in: Capsule()) + .foregroundStyle(state.color) + } + .buttonStyle(.plain) + .help(help) + .pointerCursor() + } + } + + /// The worst news among the runs — a single failure decides the badge, the + /// same way it decides whether the pull request can be merged. + private var worstState: PullRequestBuild.State { + for state in [PullRequestBuild.State.failed, .running, .pending] where + item.builds.contains(where: { $0.state == state }) { + return state + } + return item.builds.contains { $0.state == .passed } ? .passed : .unknown + } + + private var help: String { + let names = item.builds + .prefix(8) + .map { "\($0.name) — \($0.state.title.lowercased())" } + .joined(separator: "\n") + return "\(item.builds.count) builds on the head commit\n\(names)" + } +} + +/// The side panel, and the seam that sizes it. +/// +/// One view for the two of them because of what the seam does: a drag writes a +/// width many times a second, and every one of those writes redraws whatever +/// view holds it. Held a level up, beside the tab, that was the conversation — +/// each frame of the drag rebuilding every comment on the pull request. Here +/// the width reaches nothing but the panel it is the width of. +struct PullRequestSidebarPane: View { + let item: ViewerItem + let pr: PullRequest + let project: Project + let isOpen: Bool + let onAddReviewers: () -> Void + + /// How wide the panel is, in this window. Unlike whether it is open — which + /// each pull request remembers for itself — a width is a habit of the window + /// rather than of the thing being read. + @State private var width: CGFloat = 268 + + var body: some View { + HStack(spacing: 0) { + PaneResizer(width: $width, range: 210...420, growsLeftwards: true) + PullRequestSidebar( + item: item, + pr: pr, + project: project, + isOpen: isOpen, + onAddReviewers: onAddReviewers + ) + .frame(width: width) + .frame(maxHeight: .infinity) + } + } +} + +// MARK: - The description + +/// The description as its Markdown source: what is typed here is what the host +/// stores, and the drawn version comes back once it is saved. +/// +/// A view of its own so that the text being written lives beside the box it is +/// written in. Held a level up, in the whole pull request's view, every letter +/// redrew the summary bar, the side panel and every comment on the page. +struct PullRequestDescriptionEditor: View { + /// What the host has, which is what the box opens on — see + /// `PullRequestService.editableDescription`. + let initialText: String + let host: GitHostKind + /// A save is already on its way, so nothing else may be asked. + let isSaving: Bool + let onCancel: () -> Void + let onSave: (String) async -> Void + + @State private var draft: String + + init( + initialText: String, + host: GitHostKind, + isSaving: Bool, + onCancel: @escaping () -> Void, + onSave: @escaping (String) async -> Void + ) { + self.initialText = initialText + self.host = host + self.isSaving = isSaving + self.onCancel = onCancel + self.onSave = onSave + _draft = State(initialValue: initialText) + } + + var body: some View { + VStack(alignment: .leading, spacing: 7) { + TextEditor(text: $draft) + .font(.system(.callout, design: .monospaced)) + .frame(height: editorHeight) + .scrollContentBackground(.hidden) + .padding(6) + .background(.quaternary.opacity(0.25), in: RoundedRectangle(cornerRadius: 7)) + .overlay(alignment: .topLeading) { + if draft.isEmpty { + Text("Describe the change, in Markdown…") + .foregroundStyle(.tertiary) + .padding(.horizontal, 11) + .padding(.vertical, 12) + .allowsHitTesting(false) + } + } + + HStack(spacing: 8) { + Text(host == .github ? "Markdown, saved with gh" : "Markdown, saved with bkt") + .font(.caption2) + .foregroundStyle(.tertiary) + Spacer() + Button("Cancel", action: onCancel) + .controlSize(.small) + .keyboardShortcut(.cancelAction) + .disabled(isSaving) + .pointerCursor(!isSaving) + Button { + Task { await onSave(draft) } + } label: { + if isSaving { + ProgressView().controlSize(.small) + } else { + Label("Save", systemImage: "checkmark") + } + } + .buttonStyle(.borderedProminent) + .controlSize(.small) + .disabled(isSaving) + // Post belongs to the comment box further down the same page, so + // saving takes the app's Save key instead. + .shortcut(.save) + .pointerCursor(!isSaving) + .shortcutHelp("Save the description", .save) + } + } + } + + /// How tall the box stands: as tall as what is in it, within reason. A box + /// that scrolls inside a page that scrolls is two scrolls fighting for the + /// same wheel, so a description of any usual length gets to be one page. + private var editorHeight: CGFloat { + let lines = draft.reduce(1) { $1 == "\n" ? $0 + 1 : $0 } + return min(560, max(140, CGFloat(lines) * 17 + 20)) + } +} + // MARK: - Conversation /// Existing comments, plus a box to add one. @@ -1071,14 +1140,9 @@ struct ConversationView: View { self.header = header } - @State private var draft = "" /// The comment whose inline reply box is open, if any. @State private var replyingTo: PullRequestComment? - private var threads: [PullRequestCommentNode] { - PullRequestComment.tree(from: item.comments) - } - /// Who an `@` names here, in the composer and in every reply box under it. private var mentions: MentionSource { MentionSource( @@ -1111,7 +1175,9 @@ struct ConversationView: View { .padding(.top, 8) } - ForEach(threads) { thread in + // Threaded once, where the comments landed — see + // `ViewerItem.commentThreads`. + ForEach(item.commentThreads) { thread in CommentThread( node: thread, depth: 0, @@ -1138,13 +1204,62 @@ struct ConversationView: View { } Divider() - composer + // Its own view, so that what is being typed stays in the box: this + // one holds every comment on the pull request, and a letter landing + // here would draw all of them again. + ConversationComposer( + host: pr.host, + // Only once there is something above it to have failed against; + // an empty conversation says so where the comments would be. + error: item.comments.isEmpty ? nil : item.commentError, + isPosting: item.isPostingComment, + mentions: mentions, + onPost: post + ) + } + } + + /// Posts what was written, and says whether it landed — which is what + /// empties the box. + private func post(_ body: String) async -> Bool { + await store.postComment(body, on: item, project: project, pr: pr) + return item.commentError == nil + } + + /// Posts a reply and closes the inline box once it lands. + private func post(_ body: String, replyingTo parent: PullRequestComment) async { + await store.postComment(body, on: item, project: project, pr: pr, replyingTo: parent) + if item.commentError == nil { + replyingTo = nil } } +} + +/// The box at the foot of the conversation, and the button that sends it. +/// +/// It owns the text being written. The conversation above it is a page of +/// comments, each of them a rendered piece of Markdown, and a draft held up +/// there meant redrawing all of it on every keystroke. +struct ConversationComposer: View { + let host: GitHostKind + /// What the host said about the last attempt, when there is a conversation + /// above for it to have failed against. + let error: String? + let isPosting: Bool + var mentions: MentionSource = .none + /// Sends the comment, and says whether it landed. It empties the box only + /// when it did — a refusal leaves what was written where it is. + let onPost: (String) async -> Bool + + @State private var draft = "" + + private var isEmpty: Bool { + draft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + } - private var composer: some View { + var body: some View { VStack(alignment: .leading, spacing: 7) { - if let error = item.commentError, !item.comments.isEmpty { + if let error { Text(error) .font(.caption) .foregroundStyle(.red) @@ -1163,14 +1278,14 @@ struct ConversationView: View { ) HStack { - Text(pr.host == .github ? "Posted with gh" : "Posted with bkt") + Text(host == .github ? "Posted with gh" : "Posted with bkt") .font(.caption2) .foregroundStyle(.tertiary) Spacer() Button { Task { await post() } } label: { - if item.isPostingComment { + if isPosting { ProgressView().controlSize(.small) } else { Label("Comment", systemImage: "paperplane") @@ -1178,15 +1293,9 @@ struct ConversationView: View { } .buttonStyle(.borderedProminent) .controlSize(.small) - .disabled( - item.isPostingComment || - draft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - ) + .disabled(isPosting || isEmpty) .shortcut(.submit) - .pointerCursor( - !item.isPostingComment && - !draft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - ) + .pointerCursor(!isPosting && !isEmpty) .shortcutHelp("Post the comment", .submit) } } @@ -1195,19 +1304,7 @@ struct ConversationView: View { } private func post() async { - let body = draft - await store.postComment(body, on: item, project: project, pr: pr) - if item.commentError == nil { - draft = "" - } - } - - /// Posts a reply and closes the inline box once it lands. - private func post(_ body: String, replyingTo parent: PullRequestComment) async { - await store.postComment(body, on: item, project: project, pr: pr, replyingTo: parent) - if item.commentError == nil { - replyingTo = nil - } + if await onPost(draft) { draft = "" } } } @@ -1384,40 +1481,51 @@ struct PullRequestReviewerSheet: View { @State private var picked: [String] = [] @FocusState private var isSearching: Bool - /// Whoever is already on the pull request, keyed the way the picker's rows - /// are, so those rows can say so instead of offering them again. - private var alreadyAsked: Set { - Set(item.reviewers.map { $0.id.lowercased() }) - } - - /// Everybody the host offered, minus the author: nobody reviews their own - /// pull request. The list itself keeps them, because an `@` in a comment - /// names the author more often than it names anyone else. - private var candidates: [ReviewerCandidate] { - item.reviewerCandidates.filter { + /// Everything the rows are drawn from, worked out once for the pass that + /// draws them. + /// + /// Each of these was a computed property before, and the body read them + /// several times over — `alreadyAsked` once per row, which built the set of + /// who is on the pull request as many times as there are people to offer. + /// A keystroke in the search box redraws all of this, so it is worked out + /// once and handed down. + private struct Offer { + /// Everybody the host offered, minus the author: nobody reviews their + /// own pull request. The list itself keeps them, because an `@` in a + /// comment names the author more often than it names anyone else. + var candidates: [ReviewerCandidate] = [] + var matches: [ReviewerCandidate] = [] + /// The typed handle itself, offered as a row when it is not in the list + /// — which is the only way in on a host that answered with nobody. + var typedHandle: String? + /// Whoever is already on the pull request, keyed the way the rows are, + /// so a row can say so instead of offering them again. + var alreadyAsked: Set = [] + } + + private func readOffer() -> Offer { + var offer = Offer() + offer.candidates = item.reviewerCandidates.filter { $0.name.caseInsensitiveCompare(pr.author) != .orderedSame && $0.handle.caseInsensitiveCompare(pr.author) != .orderedSame } - } + offer.matches = offer.candidates.filter { $0.matches(query) } + offer.alreadyAsked = Set(item.reviewers.map { $0.id.lowercased() }) - private var matches: [ReviewerCandidate] { - candidates.filter { $0.matches(query) } - } - - /// The typed handle itself, offered as a row when it is not in the list — - /// which is the only way in on a host that answered with nobody. - private var typedHandle: String? { let trimmed = query.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty, trimmed.count > 1 else { return nil } - let isKnown = candidates.contains { - $0.handle.caseInsensitiveCompare(trimmed) == .orderedSame - || $0.name.caseInsensitiveCompare(trimmed) == .orderedSame + if !trimmed.isEmpty, trimmed.count > 1 { + let isKnown = offer.candidates.contains { + $0.handle.caseInsensitiveCompare(trimmed) == .orderedSame + || $0.name.caseInsensitiveCompare(trimmed) == .orderedSame + } + offer.typedHandle = isKnown ? nil : trimmed } - return isKnown ? nil : trimmed + return offer } var body: some View { - VStack(alignment: .leading, spacing: 12) { + let offer = readOffer() + return VStack(alignment: .leading, spacing: 12) { header if !item.reviewers.isEmpty { @@ -1435,15 +1543,15 @@ struct PullRequestReviewerSheet: View { // Says what the rows below are: with the box empty they are a // suggestion, and once something is typed they are what matched. - if !candidates.isEmpty { + if !offer.candidates.isEmpty { Text(query.trimmingCharacters(in: .whitespaces).isEmpty - ? "Suggested — \(candidates.count) people who can review" - : "\(matches.count) of \(candidates.count) match “\(query)”") + ? "Suggested — \(offer.candidates.count) people who can review" + : "\(offer.matches.count) of \(offer.candidates.count) match “\(query)”") .font(.caption) .foregroundStyle(.secondary) } - candidateList + candidateList(offer) if !picked.isEmpty { Text(picked.joined(separator: ", ")) @@ -1509,7 +1617,7 @@ struct PullRequestReviewerSheet: View { } @ViewBuilder - private var candidateList: some View { + private func candidateList(_ offer: Offer) -> some View { if item.isLoadingReviewerCandidates { HStack(spacing: 7) { ProgressView().controlSize(.small) @@ -1518,8 +1626,8 @@ struct PullRequestReviewerSheet: View { .foregroundStyle(.secondary) } .frame(height: 150, alignment: .top) - } else if matches.isEmpty && typedHandle == nil { - Text(candidates.isEmpty + } else if offer.matches.isEmpty && offer.typedHandle == nil { + Text(offer.candidates.isEmpty ? "\(pr.host.displayName) did not say who can review this. Type a handle above and it is sent as it is." : "Nobody here matches “\(query)”.") .font(.callout) @@ -1529,20 +1637,22 @@ struct PullRequestReviewerSheet: View { } else { ScrollView { VStack(spacing: 4) { - if let typedHandle { + if let typedHandle = offer.typedHandle { row( handle: typedHandle, name: "Ask “\(typedHandle)”", detail: "Sent to \(pr.host.displayName) as typed", - avatarURL: nil + avatarURL: nil, + alreadyAsked: offer.alreadyAsked ) } - ForEach(matches) { candidate in + ForEach(offer.matches) { candidate in row( handle: candidate.handle, name: candidate.name, detail: candidate.detail, - avatarURL: candidate.avatarURL + avatarURL: candidate.avatarURL, + alreadyAsked: offer.alreadyAsked ) } } @@ -1557,7 +1667,8 @@ struct PullRequestReviewerSheet: View { handle: String, name: String, detail: String?, - avatarURL: URL? + avatarURL: URL?, + alreadyAsked: Set ) -> some View { let isAsked = alreadyAsked.contains(handle.lowercased()) let isPicked = picked.contains { $0.caseInsensitiveCompare(handle) == .orderedSame } diff --git a/Sources/Workspace/Views/PullRequestSidebar.swift b/Sources/Workspace/Views/PullRequestSidebar.swift index 848a06f..cba37fb 100644 --- a/Sources/Workspace/Views/PullRequestSidebar.swift +++ b/Sources/Workspace/Views/PullRequestSidebar.swift @@ -42,23 +42,26 @@ struct PullRequestSidebar: View { // MARK: - Reviewers private var reviewers: some View { - VStack(alignment: .leading, spacing: 8) { + // Sorted once for the pass: the header's summary and the rows under it + // are the same list, and `byStanding` sorts each time it is asked. + let ordered = item.reviewers.byStanding + return VStack(alignment: .leading, spacing: 8) { sectionHeader( "Reviewers", symbol: "person.2", - detail: item.reviewers.isEmpty ? nil : item.reviewers.approvalSummary, + detail: ordered.isEmpty ? nil : item.reviewers.approvalSummary, isLoading: item.isLoadingReviewers ) { EmptyView() } - if item.reviewers.isEmpty { + if ordered.isEmpty { emptyNote(item.reviewersError ?? (item.isLoadingReviewers ? "Reading who is reviewing…" : "Nobody is reviewing #\(pr.number) yet.")) } else { - ForEach(item.reviewers.byStanding) { reviewer in + ForEach(ordered) { reviewer in reviewerRow(reviewer) } } From 153d6878d111ec2861ed6db23216247f9bfab743 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 12:38:43 +0700 Subject: [PATCH 3/7] The keys follow the conversation you are in, and a screen arrives rather than cuts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Clicking a conversation puts the cursor in it, and clicking away takes it out.** A `GhosttySurfaceView` claims the keyboard in its own `mouseDown`, so a click landing in the terminal was always answered. Either side of that, nothing was. A click on a panel's title bar, on its edges, on the gap beside its buttons means "this is the one I am reading" just as plainly, and none of them took the keys. Worse in the other direction: a click on the dashboard, a repository or a file left them where they were, because a SwiftUI row claims no focus of its own — so the window went on typing into a conversation the reader had walked away from. Both halves are one click and one hit test, so they are decided in one place. Two monitors reaching for first responder in the same turn of the runloop is a coin toss, and what loses it is whatever the user just clicked. `TerminalFocus` holds the moves; the window's click monitor holds the policy, and it now answers for the whole window rather than only for the overlay it hangs off — which is what lets it release the centre pane's terminal too. The release goes to nobody, the way a window that has just opened starts. The monitor sees a mouse-down before it is dispatched, so whatever was actually clicked takes focus for itself a moment later if it wants any; naming a successor here would be guessing on its behalf. And nothing happens at all unless a terminal is what holds the keys — a filter box, a rename field or the editor is somebody's sentence in progress. Two things that were already wrong are right now. Folding a panel to the dock, or closing it, used to leave the keyboard going into a conversation no longer on screen. And `EscapeKey`'s own comment claimed that clicking the navigator gave ⎋ back to the app; that was not true until this. **And a screen arrives instead of cutting.** Opening a file, opening a pull request, opening a diff and moving between a pull request's tabs were all instant swaps. They fade now, and rise six points while they do. The timing is in one place — `ViewerMotion` — because four transitions that are nearly the same read worse than four that are exactly the same. A tab is deliberately lighter than an item: shorter, and no rise, because the summary bar, the action bar and the side panel do not move. The page did not arrive; its middle changed. What is *not* animated took as much deciding. A terminal never fades, in or out: the shell is the one thing in the window that is mid-sentence. The status bar stays outside the animated scope, because its appearing changes the editor's height and an animated height relays the text view every frame. A spinner becoming content stays a cut, because the working tree's diff sets `isLoading` on every watcher-driven reload and a fade there would flash the diff on every save. Reduce Motion drops the rise and skips the diff's fade altogether. None of it is allowed to cost what the last commit bought: the animations are scoped to the container that is changing, so the conversation and the diff's row list are never inside one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CSpbKpa8inAaacg6rWETSS --- Docs/Development.md | 3 + Sources/Workspace/Models/WorkspaceStore.swift | 23 ++++- Sources/Workspace/Support/TerminalFocus.swift | 91 +++++++++++++++++++ Sources/Workspace/Support/ViewerMotion.swift | 75 +++++++++++++++ .../Workspace/Views/ChatPanelOverlay.swift | 52 +++++++++-- Sources/Workspace/Views/DiffView.swift | 25 ++++- .../Views/PullRequestDetailView.swift | 13 +++ Sources/Workspace/Views/ViewerView.swift | 62 ++++++++++--- 8 files changed, 321 insertions(+), 23 deletions(-) create mode 100644 Sources/Workspace/Support/TerminalFocus.swift create mode 100644 Sources/Workspace/Support/ViewerMotion.swift diff --git a/Docs/Development.md b/Docs/Development.md index 51d0ec2..3e6108d 100644 --- a/Docs/Development.md +++ b/Docs/Development.md @@ -157,6 +157,9 @@ Sources/Workspace/ Support/AuthorAvatar.swift author pictures: where to find one, cache, disc view Support/WindowKeyMonitor.swift keys the terminal would otherwise eat (⌃⇥, ⎋) Support/EscapeKey.swift ⎋ = close, and who keeps ⎋ for themselves + Support/InitialFocus.swift a window that opens with the keyboard in nobody's field + Support/TerminalFocus.swift the keys into a conversation you click, and out of one you leave + Support/ViewerMotion.swift how long a screen takes to arrive, in one place Support/PullRequestReference.swift finding a `#123` written in a commit message Support/FileOperations.swift what the Files tab does to disk: create, copy, move, rename, trash Support/MarkdownPDF.swift the preview as a PDF: measure, render, cut into sheets diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 406d55c..92fdc40 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -2408,6 +2408,10 @@ final class WorkspaceStore { /// Ends one panel's conversation — the ✕, and nothing else. Being replaced /// on screen is not this: see ``chats``. func closeChatPanel(_ panel: ChatPanel) { + // Same reason as folding, one step further: the surface is about to be + // torn down, and a first responder that has been closed is a window with + // nowhere for the keys to go. + TerminalFocus.relinquish(panel.session) panel.remember() panel.session.terminate() withAnimation(Self.chatPanelMotion) { @@ -2421,7 +2425,7 @@ final class WorkspaceStore { /// **Nothing happens at all when it is already in front**, and that guard /// has to be out here rather than inside ``raise(_:)``. /// - /// `ChatPanelClickMonitor` calls this on *every* left mouse-down in the + /// `WindowClickMonitor` calls this on *every* left mouse-down in the /// window, which is what lets a click anywhere in a panel bring it forward. /// With the check inside `raise`, the mutation was correctly skipped but /// `withAnimation` had already opened a transaction — so every click on the @@ -2442,6 +2446,15 @@ final class WorkspaceStore { /// The same, unfolded: what the Claude list, a banner and a resumed /// conversation all want, since each of them is a request to *read* it. + /// + /// And to type in it, which is why the keyboard comes with it. Every way + /// into this — the bar on the dock, the Claude list, a banner — is somebody + /// asking for a conversation by name, and landing them in it with the cursor + /// still somewhere else would make the next thing they do a click they + /// should not have had to make. + /// + /// The panel's terminal never left the window (see ``chats``), so there is + /// nothing to wait for: the surface is mounted and can take the keys now. func unfoldChatPanel(_ panel: ChatPanel) { withAnimation(Self.chatPanelMotion) { panel.isCollapsed = false @@ -2453,6 +2466,7 @@ final class WorkspaceStore { panel.remember() raise(panel) } + TerminalFocus.give(to: panel.session) } private func raise(_ panel: ChatPanel) { @@ -2492,6 +2506,13 @@ final class WorkspaceStore { /// buy by dropping the movement, and a panel that vanishes rather than /// folding gives the reader nothing to follow to the dock. func collapseChatPanel(_ panel: ChatPanel) { + // Before it goes: the chevron is inside the panel, so the mouse-down + // that reached it has just put the keyboard in this very terminal — and + // the mouse-up folds the terminal away. Left alone, the conversation + // would keep receiving everything typed at a window that no longer shows + // it. Only ever this panel's own focus, so folding one while reading + // another takes nothing from the one being read. + TerminalFocus.relinquish(panel.session) withAnimation(Self.chatPanelMotion) { panel.isCollapsed = true panel.remember() diff --git a/Sources/Workspace/Support/TerminalFocus.swift b/Sources/Workspace/Support/TerminalFocus.swift new file mode 100644 index 0000000..f9a370b --- /dev/null +++ b/Sources/Workspace/Support/TerminalFocus.swift @@ -0,0 +1,91 @@ +import AppKit + +/// Everything the app knows about whether the keyboard belongs to a terminal. +/// +/// A `GhosttySurfaceView` claims first responder in its own `mouseDown`, so a +/// click landing *in* a conversation already puts the cursor at its prompt. What +/// no view answers for is the two halves either side of that. A click on a +/// panel's title bar, on its edges or on the gaps between its buttons means "I +/// am reading this one" just as surely, and nothing there was taking the keys. +/// And a click on the dashboard, a repository or a file left them where they +/// were, because a SwiftUI row claims no focus of its own — so the window went +/// on typing into a conversation the reader had walked away from. +/// +/// These are the moves, not the policy: the one place that decides which of them +/// a click means is ``WindowClickMonitor``, and it is deliberately the only +/// place. Two mechanisms setting first responder in the same turn of the runloop +/// is a coin toss, and what loses it is whatever the user just clicked. +@MainActor +enum TerminalFocus { + /// Puts the keyboard in this conversation. + /// + /// Not while `claude` is still being typed at it: what is behind the cover + /// then is a shell prompt the app is about to run a command from, and a + /// keystroke landing there goes into that command rather than into the + /// conversation. ``TerminalPaneView`` holds the same line for the first + /// focus a surface ever gets, and for the same reason. + static func give(to session: TerminalSession) { + guard !session.isStartingClaude else { return } + let view = session.view + guard let window = view.window, window.firstResponder !== view else { return } + window.makeFirstResponder(view) + } + + /// Takes the keyboard out of a conversation that is on its way off screen — + /// folded down to the dock, or ended. Neither is somewhere the reader can + /// still see what they are typing, and a hidden terminal quietly receiving + /// keystrokes is worse than no focus at all. + static func relinquish(_ session: TerminalSession) { + let view = session.view + guard let window = view.window, window.firstResponder === view else { return } + window.makeFirstResponder(nil) + } + + /// Hands the keyboard back to nobody, the way ``withoutInitialTextFocus`` + /// leaves a window that has just opened. + /// + /// **To nobody, and not to some other view.** This runs from a mouse-down + /// monitor, which sees the event before it is dispatched — so whatever the + /// click is actually aimed at claims focus for itself a moment later if it + /// wants any, and a text field is still a text field to click into. Naming a + /// successor here would be guessing at that on its behalf. + /// + /// Nothing happens unless a terminal is what holds focus. Typing in a filter + /// box, a rename field or the editor is somebody's sentence in progress. + static func release(in window: NSWindow) { + guard isHeldByTerminal(in: window) else { return } + window.makeFirstResponder(nil) + } + + /// Whether the keys are going into a terminal right now. + static func isHeldByTerminal(in window: NSWindow) -> Bool { + surface(enclosing: window.firstResponder as? NSView) != nil + } + + /// The terminal a point in the window lands in, if it lands in one. + /// + /// Hit-tested rather than measured against the panels' rectangles. Panels + /// overlap each other, the dock's bars ride over the centre pane, and a + /// folded conversation is still mounted at its full size behind a crop — a + /// rectangle knows none of that, and would answer for whichever of the + /// things drawn in one place it happened to be asked about. The hit test is + /// the same one AppKit is about to dispatch the click with, so it answers + /// with what will really get it. + static func surface(under point: NSPoint, in window: NSWindow) -> GhosttySurfaceView? { + // `hitTest` takes a point in the receiver's *superview*, which for the + // content view is the window's own coordinate space. + surface(enclosing: window.contentView?.hitTest(point)) + } + + /// The terminal a view is part of: itself, or the one it is drawn inside. A + /// surface has no subviews today, but the walk costs nothing and keeps this + /// true if it ever grows one. + private static func surface(enclosing view: NSView?) -> GhosttySurfaceView? { + var candidate = view + while let current = candidate { + if let surface = current as? GhosttySurfaceView { return surface } + candidate = current.superview + } + return nil + } +} diff --git a/Sources/Workspace/Support/ViewerMotion.swift b/Sources/Workspace/Support/ViewerMotion.swift new file mode 100644 index 0000000..a403dac --- /dev/null +++ b/Sources/Workspace/Support/ViewerMotion.swift @@ -0,0 +1,75 @@ +import AppKit +import SwiftUI + +/// How the centre pane changes what it is showing. +/// +/// Opening a file, opening a pull request, opening a diff and moving between a +/// pull request's tabs are four journeys through the same one pane, so they are +/// written here together and arrive alike. Nothing here decides what is on +/// screen or where it sits — only how it gets there. +/// +/// The register is deliberately small. This is a window on a Mac, not a phone: +/// a change of screen should read as the pane settling rather than as something +/// sliding across it, so the whole vocabulary is a fade, a sixth of a second, +/// and at most a few points of rise. `WorkspaceStore.chatPanelMotion` is the +/// same idea for the panels that float over the window; this is the centre. +@MainActor +enum ViewerMotion { + /// The one duration. Everything below is this or a fraction of it, so a + /// change here retimes all four places at once. + static let duration: TimeInterval = 0.16 + + /// Whether the person at the keyboard has asked for less movement. + /// + /// Read each time rather than kept: the setting can be turned on while the + /// app is running, and the next thing drawn should already obey it. + static var isReduced: Bool { + NSWorkspace.shared.accessibilityDisplayShouldReduceMotion + } + + /// Replacing the whole item in the viewer — a file for a diff, a diff for a + /// pull request. Ease-out rather than ease-in-out: the click already + /// happened, so the movement should be quickest at the start and then come + /// to rest. + static var itemChange: Animation { .easeOut(duration: duration) } + + /// A change *within* what is already on screen — a pull request's tabs, a + /// file picked out of a diff's index. Quicker, and with no rise to it: the + /// page did not arrive, its middle changed, and the bars around it never + /// moved to say otherwise. + static var contentChange: Animation { .easeOut(duration: duration * 0.75) } + + /// How a new viewer item comes in, and how the one it replaces goes. + /// + /// The two are not the same. What arrives fades up from a few points below, + /// which is the whole of the "it came from somewhere" reading; what leaves + /// only fades, because a thing moving *and* a thing moving away is two + /// motions where the eye wanted one. Under Reduce Motion nothing travels at + /// all and the two cross-fade. + /// + /// A terminal gets none of it. `GhosttySurfaceView` is a live grid that + /// reflows on every size it is handed and redraws whatever `claude` has + /// written into it; the offset here would not resize it, but the shell is + /// the one thing in the window that is mid-sentence, and the honest + /// treatment of it is to leave it alone. + static func itemArrival(isTerminal: Bool) -> AnyTransition { + if isTerminal { return .identity } + if isReduced { return .opacity } + return .asymmetric( + insertion: .opacity.combined(with: .offset(y: 6)), + removal: .opacity + ) + } + + /// How a tab or a picked file comes in, where the frame around it stays + /// put. + /// + /// The outgoing side is deliberately instant. These live in a row beside + /// other things rather than stacked over them, so two of them alive at once + /// would be two of them laid out at once — the width of everything beside + /// them would jump for the length of the fade. One at a time, fading up + /// from the pane's own colour, costs nothing and moves nothing. + static var contentArrival: AnyTransition { + .asymmetric(insertion: .opacity, removal: .identity) + } +} diff --git a/Sources/Workspace/Views/ChatPanelOverlay.swift b/Sources/Workspace/Views/ChatPanelOverlay.swift index cf2fb39..d5bdee7 100644 --- a/Sources/Workspace/Views/ChatPanelOverlay.swift +++ b/Sources/Workspace/Views/ChatPanelOverlay.swift @@ -59,11 +59,12 @@ struct ChatPanelOverlay: View { } .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading) .background( - ChatPanelClickMonitor( + WindowClickMonitor( // Open panels only. A folded one has no rectangle to name — // its bar is on a strip that scrolls — and it needs none: // nothing there swallows a click the way a live terminal - // does, so the strip's own gestures are enough. + // does, so the strip's own gestures are enough, and the tap + // that brings one back is what focuses it. panels: panels .filter { !$0.isCollapsed } .map { ($0, store.chatPanelFrame(of: $0).onScreenRect) } @@ -306,7 +307,8 @@ private struct ChatDockBar: View { } } -/// Raises the panel that was clicked in, whatever was clicked. +/// What a click in this window means: which conversation comes to the front, +/// and where the keyboard goes. /// /// A click on the title bar is a gesture this app owns and could act on /// directly; a click **in the terminal** is not — `GhosttySurfaceView` answers @@ -315,7 +317,16 @@ private struct ChatDockBar: View { /// way the panel underneath can be brought forward by clicking the part of it /// you can see. Nothing is ever swallowed: the event carries on to the terminal /// exactly as it would have. -private struct ChatPanelClickMonitor: NSViewRepresentable { +/// +/// **Focus is settled here too, and in one place on purpose.** Raising a panel +/// and putting the cursor in it are the same click and the same hit test, and +/// two monitors both reaching for first responder in the same turn of the +/// runloop would be a race whose loser is whatever the user just clicked. So +/// this one answers for the whole window rather than only for the overlay it is +/// attached to: it is what releases the keys from the **centre pane's** terminal +/// as well, when a click lands on the dashboard or a file or a repository. +/// ``TerminalFocus`` has the moves; this has the policy. +private struct WindowClickMonitor: NSViewRepresentable { /// Each panel with the rectangle it is drawn in — which is its own frame, or /// its bar on the dock when it is folded away. let panels: [(panel: ChatPanel, rect: CGRect)] @@ -370,7 +381,7 @@ private struct ChatPanelClickMonitor: NSViewRepresentable { private func startWatching() { guard monitor == nil else { return } monitor = NSEvent.addLocalMonitorForEvents(matching: .leftMouseDown) { [weak self] event in - self?.raisePanel(under: event) + self?.settle(event) return event } } @@ -381,18 +392,39 @@ private struct ChatPanelClickMonitor: NSViewRepresentable { self.monitor = nil } - private func raisePanel(under event: NSEvent) { + /// One click, three answers. + /// + /// There is no `panels.count > 1` short cut out here any more, and there + /// cannot be: releasing the keyboard has to work with one conversation + /// open and with none at all. Raising is still free in that case — + /// ``WorkspaceStore/raiseChatPanel(_:)`` is a no-op for a panel already + /// in front, which is the guard that comment is about. + private func settle(_ event: NSEvent) { // One monitor sees every window's clicks; Settings and any sheet are // somebody else's business. - guard let window, event.window === window, panels.count > 1 else { return } + guard let window, event.window === window else { return } + let terminal = TerminalFocus.surface(under: event.locationInWindow, in: window) let point = convert(event.locationInWindow, from: nil) // Front to back, so a click where two panels overlap raises the one // that was actually clicked rather than the one behind it. - guard let hit = panels + let hit = panels .sorted(by: { $0.panel.depth > $1.panel.depth }) .first(where: { $0.rect.contains(point) }) - else { return } - raise(hit.panel) + + if let hit { + raise(hit.panel) + // The conversation itself needs nothing from us — the surface + // takes the keyboard in its own `mouseDown`. The rest of the + // panel is what had no answer: its title bar, its edges, the gap + // beside its buttons. Clicking any of those is still a person + // saying which conversation they are in. + if terminal == nil { TerminalFocus.give(to: hit.panel.session) } + } else if terminal == nil { + // Nowhere near a terminal, and something else is about to be + // clicked. Whether that leaves the keys anywhere is its own + // business; what matters is that they stop going to Claude. + TerminalFocus.release(in: window) + } } } } diff --git a/Sources/Workspace/Views/DiffView.swift b/Sources/Workspace/Views/DiffView.swift index 05c8376..fe53b3f 100644 --- a/Sources/Workspace/Views/DiffView.swift +++ b/Sources/Workspace/Views/DiffView.swift @@ -39,6 +39,12 @@ struct DiffView: View { @State private var colouredFiles: [DiffFile.ID: DiffFile] = [:] /// The parse `colouredFiles` was built from; anything older is thrown away. @State private var colouredRevision: UUID? + /// Set the moment a different file is picked out of the index, and cleared + /// on the pass after — which is what gives the file arriving something to + /// fade up from. It is one opacity on the whole scrolling body: the rows + /// are never animated, because a large diff has thousands of them and the + /// thing that changed is the pane, not the lines. + @State private var isSwappingFile = false /// A flattened diff, together with the parse it was built from. /// @@ -94,7 +100,12 @@ struct DiffView: View { .onChange(of: diff.revision, initial: true) { rebuild() } .onChange(of: collapsedFiles) { rebuild() } .onChange(of: composing) { rebuild() } - .onChange(of: currentFile) { rebuild() } + .onChange(of: currentFile) { + rebuild() + // The fade is the whole of the motion here, so Reduce Motion means + // no fade rather than a shorter one — the file simply changes. + if !ViewerMotion.isReduced { isSwappingFile = true } + } // The lines that carry a thread, not the threads themselves: flattening // only asks which lines to leave room under, and comparing the comment // trees instead walked every reply on this pull request each time the @@ -154,6 +165,18 @@ struct DiffView: View { } .background(Color(nsColor: AppColors.viewerBackground)) } + // Picking a file replaces every row under an offset that is about + // to be thrown to the top anyway, which read as a flicker. Two + // steps, because there is no transition to hang this on: the body + // is drawn at nothing on the pass that carries the new file, and + // the pass after brings it up. The index beside it and the bar + // above are outside this, so the only thing that moves is the part + // that changed. + .opacity(isSwappingFile ? 0 : 1) + .task(id: isSwappingFile) { + guard isSwappingFile else { return } + withAnimation(ViewerMotion.contentChange) { isSwappingFile = false } + } } } diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 677686c..a6b380f 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -58,6 +58,19 @@ struct PullRequestDetailView: View { case .commits: commitsTab } } + // A tab is a lighter thing than opening a pull request: the two + // bars above and the panel beside stay exactly where they are, + // so the arriving tab only fades up rather than rising the way + // a whole new item does. + // + // The animation is scoped to this view rather than put on the + // row on purpose. The panel comes and goes with Details, and an + // animated row would drag the width of whatever is open through + // every frame of that — a diff re-laying a thousand rows to end + // up where it already was. Here the row snaps to its new widths + // at once and only the contents fade. + .transition(ViewerMotion.contentArrival) + .animation(ViewerMotion.contentChange, value: item.pullRequestTab) .frame(maxWidth: .infinity, maxHeight: .infinity) if item.pullRequestTab == .details { diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index 62559d5..7147199 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -29,22 +29,62 @@ struct ViewerView: View { } } + /// What the pane is showing, as one value. The transition below fires on + /// any change to it — a different item, or the dashboard taking the pane + /// back — and on nothing else, so a diff reloading or a comment landing + /// still redraws in place. + private var openItemKey: String { + guard let item = store.current, !store.showsDashboard else { return "dashboard" } + return item.id + } + /// The open item, or the dashboard when there is none. It carries the /// pane's colour itself — the window's own background sits below it. private var mainContent: some View { VStack(spacing: 0) { - if let item = store.current, !store.showsDashboard { - content(item) - .frame(maxWidth: .infinity, maxHeight: .infinity) - // Only an editor has a status bar. For everything else the row - // had nothing to say but the repository's name, which the - // sidebar and the breadcrumb both already show. - if let document = item.document, case .text = document.content { - Divider() - StatusBar(document: document) + // Stacked rather than laid out one after another: what leaves and + // what arrives share the pane for the length of the fade, and in a + // column they would briefly be two panes instead of one. Both fill + // it, so nothing is resized on the way through. + ZStack { + if let item = store.current, !store.showsDashboard { + content(item) + .frame(maxWidth: .infinity, maxHeight: .infinity) + // Two files are two screens, not one screen with + // different text in it — without this, opening a file + // over a file changes no identity, and there is nothing + // for a transition to happen to. + // + // The key is the item's, which is its path, hash or + // number: it does not turn over when a diff reloads + // under the reader or a comment lands, so nothing that + // deliberately keeps its place loses it. What it does + // rebuild is a second pull request or a second diff, + // where the view used to be handed the new item + // instead. Both are keyed to the item they draw and + // build from state cached on it, so that is the same + // work either way — and ``CodeEditorView`` already + // keyed itself on the file's address for its own + // reasons, which is the one case that had to stay + // exactly as it was. + .id(item.id) + .transition(ViewerMotion.itemArrival(isTerminal: item.isTerminal)) + } else { + WelcomeView() + .transition(ViewerMotion.itemArrival(isTerminal: false)) } - } else { - WelcomeView() + } + .animation(ViewerMotion.itemChange, value: openItemKey) + + // Only an editor has a status bar. For everything else the row had + // nothing to say but the repository's name, which the sidebar and + // the breadcrumb both already show. It is outside the animation on + // purpose: the row appearing shortens the pane above it, and a + // height that moves is a text view relaid every frame. + if !store.showsDashboard, let document = store.current?.document, + case .text = document.content { + Divider() + StatusBar(document: document) } } .background(Color(nsColor: AppColors.viewerBackground)) From 6f186bde6fb5d6d83dd8b1af49549f823850965c Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 12:42:52 +0700 Subject: [PATCH 4/7] The tabs fade too, on both sets of them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pull request's tabs were meant to fade in the last commit and did not. The transition was hung on the `Group` around the switch, on the reading that a group hands its modifiers to its children — true of layout, not of this. What arrives and leaves is the branch, so that is where a transition has to sit, and the group around it was always there and never transitioned at all. The navigator's own five tabs — Files, Changes, Terminals, Claude, Info — were simply missed. Same treatment, same timing: they are the middle of a pane changing under a picker that stays put, which is exactly what a pull request's tabs are. Nothing there holds a terminal, so nothing there is mid-sentence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CSpbKpa8inAaacg6rWETSS --- Sources/Workspace/Views/NavigatorView.swift | 45 ++++++++++++++----- .../Views/PullRequestDetailView.swift | 11 +++-- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/Sources/Workspace/Views/NavigatorView.swift b/Sources/Workspace/Views/NavigatorView.swift index bc9bcaa..64f5550 100644 --- a/Sources/Workspace/Views/NavigatorView.swift +++ b/Sources/Workspace/Views/NavigatorView.swift @@ -83,20 +83,43 @@ struct NavigatorView: View { } } + /// The five lists, one at a time. + /// + /// They arrive the way a pull request's tabs do, and for the same reason: + /// the picker above them does not move, so what changed is the middle of a + /// pane rather than the pane itself. + /// + /// The transition sits on each list rather than on the `Group` holding + /// them. The group is always here; what arrives and leaves is the branch, + /// and a transition only runs on the view actually being inserted or + /// removed. + /// + /// The outgoing list leaves at once rather than fading out. This pane is + /// the narrow one, and two lists alive together would be two of them laid + /// out together — every row measuring against a width neither of them ends + /// up with, for the length of the fade. @ViewBuilder private func content(_ project: Project) -> some View { - switch store.navigatorTab { - case .files: - FileListView(project: project) - case .changes: - ChangeListView(project: project) - case .terminals: - TerminalListView(project: project) - case .claude: - ClaudeSessionListView(project: project) - case .info: - InfoPanelView(project: project) + Group { + switch store.navigatorTab { + case .files: + FileListView(project: project) + .transition(ViewerMotion.contentArrival) + case .changes: + ChangeListView(project: project) + .transition(ViewerMotion.contentArrival) + case .terminals: + TerminalListView(project: project) + .transition(ViewerMotion.contentArrival) + case .claude: + ClaudeSessionListView(project: project) + .transition(ViewerMotion.contentArrival) + case .info: + InfoPanelView(project: project) + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.contentChange, value: store.navigatorTab) } } diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index a6b380f..4642ae9 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -51,11 +51,15 @@ struct PullRequestDetailView: View { // width, so a second column of anything there is a column taken // from the code. HStack(spacing: 0) { + // The transition rides on each tab rather than on the `Group` + // around them. The group is always here; what arrives and + // leaves is the branch, and a transition only runs on the view + // actually being inserted or removed. Group { switch item.pullRequestTab { - case .details: detailsTab - case .diff: diffTab - case .commits: commitsTab + case .details: detailsTab.transition(ViewerMotion.contentArrival) + case .diff: diffTab.transition(ViewerMotion.contentArrival) + case .commits: commitsTab.transition(ViewerMotion.contentArrival) } } // A tab is a lighter thing than opening a pull request: the two @@ -69,7 +73,6 @@ struct PullRequestDetailView: View { // every frame of that — a diff re-laying a thousand rows to end // up where it already was. Here the row snaps to its new widths // at once and only the contents fade. - .transition(ViewerMotion.contentArrival) .animation(ViewerMotion.contentChange, value: item.pullRequestTab) .frame(maxWidth: .infinity, maxHeight: .infinity) From cb24c11e2671853d8a7ced85d4fb31d35cfb6994 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 12:49:33 +0700 Subject: [PATCH 5/7] Lists that build the rows you are looking at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three lists inside a scroll view were plain stacks, so every row in them was built and laid out whether it was anywhere near the window or not. The dashboard's commit history is the one that shows. "Show more" hands it every commit the repository has, and each row parses its message looking for a `#123` and reaches for a face — all of it, on the way to a screen that holds a dozen. Its day grouping was also asked for inside the `ForEach`, which walked the whole history again on every pass; it is worked out once now. The board's pull requests are the same shape: the hosts are asked for fifty, and a row carries a face, its reviewers and a build summary. And the shortcut settings, which list every key the app binds and ask each row what it clashes with. None of this arrived with the transitions — the stacks predate all of it. What changed is that the app was being read through a debug build, where nothing is optimised, and that is loud enough to make a latent cost audible. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CSpbKpa8inAaacg6rWETSS --- .../Workspace/Views/PullRequestTable.swift | 19 ++++++++---- .../Workspace/Views/ShortcutSettings.swift | 4 ++- Sources/Workspace/Views/ViewerView.swift | 30 +++++++++++++------ 3 files changed, 37 insertions(+), 16 deletions(-) diff --git a/Sources/Workspace/Views/PullRequestTable.swift b/Sources/Workspace/Views/PullRequestTable.swift index 6821b81..eb57ff3 100644 --- a/Sources/Workspace/Views/PullRequestTable.swift +++ b/Sources/Workspace/Views/PullRequestTable.swift @@ -103,12 +103,19 @@ struct PullRequestTable: View { VStack(spacing: 3) { header Divider().padding(.bottom, 2) - ForEach(Array(project.pullRequests.enumerated()), id: \.element.id) { index, pr in - PullRequestTableRow( - pr: pr, - isAlternate: index.isMultiple(of: 2) == false, - open: { store.openPullRequest(pr, project: project) } - ) + // Lazy, for the same reason the commit history below it is: the + // hosts are asked for fifty, and every row here carries a face, a + // row of reviewers and a build summary. A repository with a full + // board was building all of that on the way to a dashboard where + // three of them are on screen. + LazyVStack(spacing: 3) { + ForEach(Array(project.pullRequests.enumerated()), id: \.element.id) { index, pr in + PullRequestTableRow( + pr: pr, + isAlternate: index.isMultiple(of: 2) == false, + open: { store.openPullRequest(pr, project: project) } + ) + } } } .padding(.horizontal, Column.rowInset) diff --git a/Sources/Workspace/Views/ShortcutSettings.swift b/Sources/Workspace/Views/ShortcutSettings.swift index 5e6524d..8535507 100644 --- a/Sources/Workspace/Views/ShortcutSettings.swift +++ b/Sources/Workspace/Views/ShortcutSettings.swift @@ -25,7 +25,9 @@ struct ShortcutSettings: View { Divider() ScrollView { - VStack(spacing: 0) { + // Every key the app binds, which is a list that only grows, and + // each row asks what it is bound to and what it clashes with. + LazyVStack(spacing: 0) { ForEach(ShortcutGroup.allCases) { group in GroupHeader(title: group.title) ForEach(group.actions) { action in diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index 7147199..9ef849e 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -728,6 +728,10 @@ struct WelcomeView: View { ? project.recentCommits : Array(project.recentCommits.prefix(collapsedCommitCount)) + // Grouped once for the pass. Asked for inside the `ForEach`, the + // whole history was walked again every time this view was drawn. + let days = CommitDay.group(shown) + VStack(alignment: .leading, spacing: 10) { HStack(alignment: .firstTextBaseline, spacing: 6) { Text("Recent commits") @@ -741,15 +745,23 @@ struct WelcomeView: View { } } - ForEach(CommitDay.group(shown)) { day in - VStack(alignment: .leading, spacing: 6) { - CommitDayHeading(title: day.title, count: day.commits.count) - ForEach(day.commits) { commit in - RepositoryCommitRow( - commit: commit, - open: { store.openCommit(commit, project: project) }, - openPullRequest: { store.openPullRequest(number: $0, project: project) } - ) + // **Lazy**, and it is the length of the history that decides it. + // "Show more" hands this every commit the repository has, and a + // plain stack builds all of them at once — every row parsing its + // message for a `#123` and reaching for a face — whether they are + // anywhere near the window or not. The rows below the fold are + // the ones nobody is looking at yet. + LazyVStack(alignment: .leading, spacing: 10) { + ForEach(days) { day in + VStack(alignment: .leading, spacing: 6) { + CommitDayHeading(title: day.title, count: day.commits.count) + ForEach(day.commits) { commit in + RepositoryCommitRow( + commit: commit, + open: { store.openCommit(commit, project: project) }, + openPullRequest: { store.openPullRequest(number: $0, project: project) } + ) + } } } } From b1f17d32212746301f106133a34d9ae2a24a98ea Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 14:23:07 +0700 Subject: [PATCH 6/7] The tree says what git says, in the colours the gutter uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file that has changed since HEAD now shows it in the file tree: green for a new one, yellow for an edit, red for a conflict, with the letter every editor puts at the end of such a row. A folder wears the strongest verdict of anything inside it, so a change deep in a collapsed branch still reaches a row that can be seen. Where the verdict comes from is `GitStatus.Change.kind`, read off the porcelain letter rather than off the wording beside it, so a phrase changing in the change list cannot quietly repaint the tree. The tree asks `Project.changeKind(for:)` once per visible row on every redraw, so the whole status is folded into one map — file and every folder above it — and rebuilt only when `gitRevision` moves. Reading that revision first is also what makes a row observe the next status read; a commit moves every verdict without touching a byte of any file. The colours are one enum now. Three places make the same claim about the same repository — the tree, the gutter, the change list — and they were three separate sets of literals, with the tree agreeing with neither of the others. And the gutter markers had never once drawn. `addFloatingSubview` does not add the view to the scroll view: AppKit lifts it into a private container of its own, so scanning the direct subviews found a clip view and that container, and the coordinator installed nothing. It searches breadth-first now, skipping the clip view, which is the one subtree the document makes large. Changed lines are yellow rather than blue as a result of the shared colours, which the README and the feature table now say. The rest is motion. The timings are named in `ViewerMotion` instead of written out at each call site: a pane folding, rows arriving in a list, a twisty, a count changing. Toggling a pane carries its animation on the store's property rather than at the three call sites that used to disagree about it, and the update banner and the status toast transition at all now — an `if` written straight into `body` had nothing above it to hang a transaction on. The navigator's tab fade drops `.identity` on the way out, which meant *drawn unchanged* rather than gone, so the outgoing list sat at full strength under the arriving one for the length of the fade. Two loose ends, both deliberate: `Support/Skeleton.swift` is written but nothing reaches for it yet, and `TODO.md` is the branch's own checklist. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NiNZvYyFWYe6AbQ2H7RWgn --- Docs/Development.md | 3 +- README.md | 3 +- .../Workspace/Editor/GutterDiffMarkers.swift | 87 ++- Sources/Workspace/Models/GitStatus.swift | 60 ++ Sources/Workspace/Models/Project.swift | 109 ++++ Sources/Workspace/Models/WorkspaceStore.swift | 27 +- Sources/Workspace/Support/AuthorAvatar.swift | 10 + .../Workspace/Support/GitChangeColors.swift | 46 ++ Sources/Workspace/Support/RemoteImage.swift | 12 + Sources/Workspace/Support/Skeleton.swift | 139 +++++ Sources/Workspace/Support/ViewerMotion.swift | 46 +- .../Workspace/Views/ChatPanelOverlay.swift | 12 +- .../Views/ClaudeSessionListView.swift | 16 +- Sources/Workspace/Views/ContentView.swift | 17 +- Sources/Workspace/Views/DiffView.swift | 60 +- .../Workspace/Views/FileFinderOverlay.swift | 14 + Sources/Workspace/Views/InfoPanelView.swift | 9 + .../Workspace/Views/InlineCommentViews.swift | 15 +- Sources/Workspace/Views/NavigatorView.swift | 137 +++- Sources/Workspace/Views/ProjectsSidebar.swift | 37 +- .../Views/PullRequestDetailView.swift | 586 ++++++++++++------ .../Workspace/Views/PullRequestSidebar.swift | 69 ++- .../Workspace/Views/PullRequestTable.swift | 24 +- Sources/Workspace/Views/UpdateSettings.swift | 76 ++- Sources/Workspace/Views/ViewerView.swift | 124 +++- Sources/Workspace/WorkspaceApp.swift | 2 +- TODO.md | 4 + 27 files changed, 1344 insertions(+), 400 deletions(-) create mode 100644 Sources/Workspace/Support/GitChangeColors.swift create mode 100644 Sources/Workspace/Support/Skeleton.swift create mode 100644 TODO.md diff --git a/Docs/Development.md b/Docs/Development.md index 3e6108d..6761d14 100644 --- a/Docs/Development.md +++ b/Docs/Development.md @@ -152,6 +152,7 @@ Sources/Workspace/ Support/Shell.swift runs git/gh/bkt through the login shell Support/GitDirectoryWatcher.swift git run outside the app: watches `.git` for a checkout Support/WorkingTreeWatcher.swift files written outside the app: FSEvents over the whole tree + Support/GitChangeColors.swift what added, changed and gone look like — tree, gutter and change list Support/BrandMark.swift real logos (TypeScript, Docker, GitHub…) from SVG Support/BrandPath.swift the path data behind them — generated, don't edit Support/AuthorAvatar.swift author pictures: where to find one, cache, disc view @@ -181,7 +182,7 @@ Sources/Workspace/ PullRequestReviewer.swift who is reviewing, their verdicts, and asking more people PullRequestBuild.swift the CI runs on the head commit, normalised across hosts GitHubAccounts.swift per-repo gh account choice + GH_TOKEN injection - GitStatus.swift porcelain status + per-file diff + GitStatus.swift porcelain status + per-file diff, and the verdict a row is coloured by DefaultBranch.swift which branch the host calls default: gh/bkt, then origin/HEAD RepositoryCommit.swift the branch's own git log, grouped by day DiffModel.swift unified diff → side-by-side rows diff --git a/README.md b/README.md index b58711d..36351b0 100644 --- a/README.md +++ b/README.md @@ -114,11 +114,12 @@ missing ones, so you never have to work out which command it wanted. | Feature | What it does | | --- | --- | | Real editing | Tree-sitter colours, folding, multiple cursors, find and replace (⌘F), auto-indent. | -| Change markers | What has changed since HEAD, down the edge of the gutter — green where lines were added, blue where they were changed, and a wedge where they were deleted. | +| Change markers | What has changed since HEAD, down the edge of the gutter — green where lines were added, yellow where they were changed, and a red wedge where they were deleted. | | Language servers | Diagnostics, completions (⌃Space) and ⌘-click to definition for 20+ languages. | | Go to file (⌘P) | `item controller` finds `app/controllers/item_controller.rb`. Punctuation optional. | | Search in files | ripgrep, grouped by file, every hit marked in the file you open. | | A file tree you can use | Drag in and out, copy and paste with Finder, rename in place, duplicate, multi-select, new file or folder. | +| Changes in the tree | A new file is green, a changed one yellow, a conflict red — same colours as the gutter beside it. Folders wear a dot for whatever is inside them. | ### Previews diff --git a/Sources/Workspace/Editor/GutterDiffMarkers.swift b/Sources/Workspace/Editor/GutterDiffMarkers.swift index 2ae9a13..5032007 100644 --- a/Sources/Workspace/Editor/GutterDiffMarkers.swift +++ b/Sources/Workspace/Editor/GutterDiffMarkers.swift @@ -1,10 +1,9 @@ import AppKit import CodeEditSourceEditor import CodeEditTextView -import SwiftUI /// VS Code's coloured stripe down the line numbers: green where lines were added, -/// blue where they were changed, and a wedge where lines were deleted. +/// yellow where they were changed, and a red wedge where lines were deleted. /// /// **The gutter is the package's, and it offers nothing to decorate it with.** /// `TextViewController.gutterView` is internal, `GutterViewDelegate` has one @@ -17,9 +16,10 @@ import SwiftUI /// What makes it hold together rather than drift: /// /// - **The gutter is findable without the private property.** `GutterView` is a -/// `public` class and `loadView` attaches it with `addFloatingSubview`, so it is -/// a direct subview of the `public` scroll view and can be picked out by type. -/// That is a type check, not a name or an index, and it fails to nothing. +/// `public` class hanging somewhere under the `public` scroll view, so it can be +/// picked out by type — see ``gutterView(in:)`` for where AppKit actually keeps +/// it, which is not where `loadView` appears to put it. That is a type check, +/// not a name or an index, and it fails to nothing. /// - **Its coordinates are the document's.** The gutter is as tall as the whole /// file and the package keeps `origin.y` at `textView.frame.origin.y`, so a /// y inside the gutter *is* a y in the text. Which is why `GutterView` fills its @@ -113,7 +113,7 @@ final class GutterDiffMarkers: TextViewCoordinator, @unchecked Sendable { @MainActor private func install(in controller: TextViewController) { guard let scrollView = controller.scrollView, - let gutter = scrollView.subviews.lazy.compactMap({ $0 as? GutterView }).first + let gutter = Self.gutterView(in: scrollView) else { return } self.gutter = gutter @@ -149,6 +149,36 @@ final class GutterDiffMarkers: TextViewCoordinator, @unchecked Sendable { } } + /// The gutter, wherever AppKit has put it. + /// + /// **Not `scrollView.subviews`, which is where it looks like it should be.** + /// `loadView` attaches it with `addFloatingSubview`, and that does not add + /// the view to the scroll view: AppKit lifts every floating subview into a + /// private `_NSScrollViewFloatingSubviewsContainerView` of its own and adds + /// *that*. So a scan of the direct subviews finds an `NSClipView` and a + /// container, no `GutterView`, and this whole coordinator quietly installed + /// nothing — which is exactly how the markers came to be written, shipped, + /// and never once drawn. + /// + /// Searching breadth-first from the scroll view instead, which is immune to + /// where the container is in the order and to it being renamed or dropped in + /// a later macOS. The clip view is skipped because the document is the text, + /// and the gutter is never inside it — that is the one subtree worth being + /// large. + @MainActor + private static func gutterView(in scrollView: NSScrollView) -> GutterView? { + var queue = scrollView.subviews + var index = 0 + while index < queue.count { + let view = queue[index] + index += 1 + if let gutter = view as? GutterView { return gutter } + if view is NSClipView { continue } + queue.append(contentsOf: view.subviews) + } + return nil + } + /// Set outright rather than left to an autoresizing mask. The gutter's frame /// is assigned in several places for several reasons — width for the line /// count, height for the document, origin for the content insets — and taking @@ -245,46 +275,15 @@ private final class GutterDiffMarkerView: NSView { } } + /// ``GitChangeColors``, the same three the file tree paints a row in. A + /// stripe beside line 40 and the colour on that file's name in the tree are + /// one claim about one repository, so they are one colour — read the tree, + /// open the file, and the marks carry on saying the same thing. private func color(for change: GitLineChange) -> NSColor { switch change { - case .added: GutterDiffColors.added - case .modified: GutterDiffColors.modified - case .deleted: GutterDiffColors.deleted + case .added: GitChangeColors.addedNS + case .modified: GitChangeColors.modifiedNS + case .deleted: GitChangeColors.deletedNS } } } - -/// The three stripe colours. -/// -/// Added and deleted are the diff viewer's own hues — ``DiffColors`` — rather than -/// a second green and a second red, so the two places in the app that say "this -/// line changed" say it in the same colour. They are lifted out of the dark end of -/// their range on the way, because those values are backgrounds for a whole row of -/// text and a three-point stripe on a dark gutter needs to be the thing that is -/// seen rather than the thing behind it. -/// -/// Modified has no such ancestor: the diff viewer never draws a changed line as -/// one thing, it draws the removal beside the addition, so there is no third -/// colour to borrow. Blue is named outright here, as it is in every editor with -/// this feature. -private enum GutterDiffColors { - static let added = stripe(from: DiffColors.addedWord) - static let deleted = stripe(from: DiffColors.removedWord) - static let modified = NSColor(rgb: 0x2E_7A_C6) - - /// Same hue, taken up to the saturation and brightness a thin mark needs. - private static func stripe(from color: Color) -> NSColor { - guard let source = NSColor(color).usingColorSpace(.sRGB) else { return NSColor(color) } - var hue: CGFloat = 0 - var saturation: CGFloat = 0 - var brightness: CGFloat = 0 - var alpha: CGFloat = 0 - source.getHue(&hue, saturation: &saturation, brightness: &brightness, alpha: &alpha) - return NSColor( - hue: hue, - saturation: max(saturation, 0.6), - brightness: max(brightness, 0.62), - alpha: 1 - ) - } -} diff --git a/Sources/Workspace/Models/GitStatus.swift b/Sources/Workspace/Models/GitStatus.swift index e19072e..2d181f0 100644 --- a/Sources/Workspace/Models/GitStatus.swift +++ b/Sources/Workspace/Models/GitStatus.swift @@ -1,5 +1,48 @@ import Foundation +/// What happened to one path, reduced to the handful of verdicts everything +/// outside the change list draws from: the file tree colours a row by this, and +/// the letter the row wears comes from it. +/// +/// **The order is the point.** A folder in the tree wears the strongest verdict +/// of anything inside it, and `Comparable` — synthesised from the order these +/// are declared in — is how "strongest" is decided. A folder holding one +/// conflict and twenty edits is a folder with a conflict in it. +enum GitChangeKind: Sendable, Hashable, Comparable { + /// Staged as new, or not in git at all yet. One idea at two stages, and the + /// tree has no room to say which — the change list beside it does. + case added + case renamed + case modified + case deleted + case conflicted + + /// The one letter the tree puts at the end of a file's row, the way every + /// editor with this feature does. A conflict is the exception: it is the + /// one that wants noticing rather than reading. + var letter: String { + switch self { + case .added: "A" + case .renamed: "R" + case .modified: "M" + case .deleted: "D" + case .conflicted: "!" + } + } + + /// What the row's tooltip says. Plain words, since the letter is the short + /// form and this is the long one. + var label: String { + switch self { + case .added: "Added" + case .renamed: "Renamed" + case .modified: "Modified" + case .deleted: "Deleted" + case .conflicted: "Conflicted" + } + } +} + /// A snapshot of `git status` for a repository. struct GitStatus: Sendable, Hashable { struct Change: Sendable, Hashable, Identifiable { @@ -67,6 +110,23 @@ struct GitStatus: Sendable, Hashable { } } + /// The same verdict as ``label``, in the form the tree and the gutter + /// colour from. Read off the porcelain letter rather than off `label`, + /// so a wording change up there cannot quietly repaint the tree. + /// + /// A copy counts as an addition: what the tree is showing is a file + /// that was not there before. A type change counts as a modification, + /// which is what it is — the path stayed, its contents did not. + var kind: GitChangeKind { + if isConflicted { return .conflicted } + switch statusLetter { + case "A", "?", "C": return .added + case "R": return .renamed + case "D": return .deleted + default: return .modified + } + } + var symbol: String { if isConflicted { return "exclamationmark.triangle.fill" } switch label { diff --git a/Sources/Workspace/Models/Project.swift b/Sources/Workspace/Models/Project.swift index 4ebb6db..0c0e2f4 100644 --- a/Sources/Workspace/Models/Project.swift +++ b/Sources/Workspace/Models/Project.swift @@ -154,6 +154,108 @@ final class Project: Identifiable { return false } + // MARK: - What git says about one row of the tree + + /// Where the repository actually starts, which is not always the folder that + /// was added — see ``GitStatus/root(of:)``. Every path `git status` reports + /// is relative to it, so it is what turns one of those back into a file on + /// disk. Read once and kept: a checkout does not move. + @ObservationIgnored private var gitRoot: URL? + + /// Absolute path → verdict, for every changed file **and every folder above + /// one**, so a change deep in a collapsed tree still shows on the folder the + /// user can see. Built from the whole status in one pass rather than + /// searched per row: the tree asks this once per visible row on every redraw, + /// and a repository mid-rebase has hundreds of changes. + @ObservationIgnored private var changeKindsCache: [String: GitChangeKind] = [:] + /// Which ``gitRevision`` the cache above was built from. `-1` is "never". + @ObservationIgnored private var changeKindsRevision = -1 + + /// What git says about one file or folder, or nil when it says nothing. + /// + /// Reads ``gitRevision`` first and on purpose: that is what makes a SwiftUI + /// row calling this observe the next status read, and it is also what tells + /// the cache it has gone stale. A commit moves every verdict in the tree + /// without touching a byte of any file, so watching the files is not enough. + func changeKind(for fileURL: URL) -> GitChangeKind? { + let revision = gitRevision + if changeKindsRevision != revision { + changeKindsCache = Self.changeKinds(in: gitStatus, folder: url, gitRoot: gitRoot) + changeKindsRevision = revision + } + return changeKindsCache[fileURL.standardizedFileURL.path] + } + + /// One status → the map above. + /// + /// Each change is written against its own path and then walked up, keeping + /// the strongest verdict at every level. The walk stops as soon as it meets + /// an ancestor that already holds something at least as strong, because + /// everything above that one does too — which is what keeps this linear in + /// practice rather than changes × depth. + /// + /// A rename contributes both of its names: the old one so a folder that only + /// lost a file still says so, the new one so the file now on disk does. + /// + /// **Keys are built from the added folder, not from the repository root**, + /// even though the paths git reports are relative to the root. The tree's + /// rows are URLs made by walking down from `folder`, and a dictionary + /// lookup is an exact string match — so a checkout reached through a + /// symlink (`/tmp/…` against git's `/private/tmp/…`) would agree about the + /// file and match on nothing at all. Resolving each row instead would be a + /// filesystem call per visible row per redraw. This resolves twice, here, + /// and only to work out how deep the opened folder sits. + private static func changeKinds( + in status: GitStatus?, + folder: URL, + gitRoot: URL? + ) -> [String: GitChangeKind] { + guard let status, !status.changes.isEmpty else { return [:] } + let base = folder.standardizedFileURL + let basePath = base.path + let prefix = subpath(of: base, under: gitRoot) + var result: [String: GitChangeKind] = [:] + + for change in status.changes { + let kind = change.kind + for path in change.gitPaths { + // A change elsewhere in the repository — a sibling package of + // the monorepo — has no row here to colour. + guard path.hasPrefix(prefix) else { continue } + let relative = String(path.dropFirst(prefix.count)) + guard !relative.isEmpty else { continue } + + var current = base.appendingPathComponent(relative).standardizedFileURL + // Strictly inside the folder: it is not a row in the tree + // itself, and this is also what stops the walk. + while current.path.count > basePath.count { + if let existing = result[current.path], existing >= kind { break } + result[current.path] = kind + current = current.deletingLastPathComponent() + } + } + } + return result + } + + /// How deep the added folder sits inside its repository, as the path prefix + /// git puts in front of everything under it — `"apps/web/"`, or `""` when + /// the folder *is* the repository, which is the ordinary case. + private static func subpath(of folder: URL, under gitRoot: URL?) -> String { + guard let gitRoot else { return "" } + let rootPath = gitRoot.standardizedFileURL.resolvingSymlinksInPath().path + let folderPath = folder.resolvingSymlinksInPath().path + guard folderPath.hasPrefix(rootPath), folderPath.count > rootPath.count else { return "" } + let suffix = folderPath.dropFirst(rootPath.count).drop { $0 == "/" } + return suffix.isEmpty ? "" : suffix + "/" + } + + /// Asks git for the top level, unless it already answered. + private func resolveGitRootIfNeeded() async { + guard gitRoot == nil else { return } + gitRoot = await GitStatus.root(of: url) + } + init(url: URL) { self.url = url self.root = FileNode(url: url, isDirectory: true) @@ -172,8 +274,13 @@ final class Project: Identifiable { async let remoteTask = RemoteInfo.load(for: url) async let statusTask = GitStatus.load(for: url) async let commitTask = RepositoryCommit.load(in: url, limit: commitLimit) + // Alongside the rest rather than before them: it is one process, it is + // only ever run once, and nothing here needs its answer until the + // revision below moves and the tree asks what changed. + async let rootTask: Void = resolveGitRootIfNeeded() remote = await remoteTask gitStatus = await statusTask + await rootTask gitRevision += 1 apply(await commitTask) startWatchingGitIfNeeded() @@ -184,7 +291,9 @@ final class Project: Identifiable { } func refreshGitStatus() async { + async let rootTask: Void = resolveGitRootIfNeeded() gitStatus = await GitStatus.load(for: url) + await rootTask gitRevision += 1 await refreshCommits() startWatchingGitIfNeeded() diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 92fdc40..72f6598 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -714,8 +714,20 @@ final class WorkspaceStore { /// Shows or folds the navigator by hand — the button in the viewer's header /// and the View menu. Nothing else moves the pane: where it is left is where /// it stays, whatever the viewer goes on to show. + /// + /// The fold is animated here rather than at the call sites, because there + /// are three of them and they used to disagree: the header button eased, + /// ⌥⌘0 snapped, and a dashboard tile snapped. A pane is one thing whichever + /// way it is asked for, so the transaction belongs with the property. func toggleNavigator() { - showsNavigator.toggle() + withAnimation(ViewerMotion.paneChange) { showsNavigator.toggle() } + } + + /// The same for the repositories pane, which the header button and ⌘0 both + /// reach for. It exists only to carry the animation — without it the two + /// call sites write the flag themselves and one of them forgets. + func toggleProjects() { + withAnimation(ViewerMotion.paneChange) { showsProjects.toggle() } } /// The document the viewer is actually showing. The open item survives a @@ -1813,8 +1825,10 @@ final class WorkspaceStore { /// Everything that sends the user there goes through this: setting the tab /// alone does nothing visible while the pane is hidden. func showNavigator(_ tab: NavigatorTab) { + // The tab is set outside the transaction: which list is shown is the + // navigator's own business, and only the pane arriving is a fold. navigatorTab = tab - showsNavigator = true + withAnimation(ViewerMotion.paneChange) { showsNavigator = true } } /// Switching the navigator by hand — the tab bar, or the View menu. @@ -3268,8 +3282,15 @@ struct ChatPanelFrame: Equatable, Codable { /// The line the strip's bars sit on, in the overlay's coordinates. + /// + /// Flush with the bottom of the pane, and not held off it by ``margin`` the + /// way a floating panel is. A panel is an object sitting *in* the window and + /// wants air around it; the dock is a rail along the bottom edge, and a rail + /// with a gap under it reads as neither one thing nor the other. What the + /// gap cost was the only thing down there worth covering — the last line of + /// the pane — and covering it is what a dock does. static func dockLine(in region: CGRect) -> CGFloat { - max(region.maxY - margin - titleBarHeight, 0) + max(region.maxY - titleBarHeight, 0) } /// Where a panel goes as it folds: down to the right-hand end of the dock, diff --git a/Sources/Workspace/Support/AuthorAvatar.swift b/Sources/Workspace/Support/AuthorAvatar.swift index d64f22c..20b7223 100644 --- a/Sources/Workspace/Support/AuthorAvatar.swift +++ b/Sources/Workspace/Support/AuthorAvatar.swift @@ -194,14 +194,24 @@ struct AuthorAvatar: View { .resizable() .interpolation(.high) .aspectRatio(contentMode: .fill) + .transition(.opacity) } else { Text(initials) .font(.system(size: size * 0.5, weight: .semibold)) .foregroundStyle(tint) .frame(width: size, height: size) .background(tint.opacity(0.22)) + .transition(.opacity) } } + // The initials give way to the face rather than being swapped for it. + // A pull request opened for the first time asks for every picture on it + // at once and they land one at a time, so without this the whole page + // speckles. Opacity and nothing else: fifty discs growing into place is + // a page that cannot be read while it settles, and the two sides are the + // same disc at the same size anyway. It only ever runs on first sight — + // the initialiser primes a face the cache already has. + .animation(ViewerMotion.isReduced ? nil : .easeOut(duration: 0.2), value: image != nil) .frame(width: size, height: size) .clipShape(Circle()) .help(name) diff --git a/Sources/Workspace/Support/GitChangeColors.swift b/Sources/Workspace/Support/GitChangeColors.swift new file mode 100644 index 0000000..f855ead --- /dev/null +++ b/Sources/Workspace/Support/GitChangeColors.swift @@ -0,0 +1,46 @@ +import AppKit +import SwiftUI + +/// The one place the app decides what "added", "changed" and "gone" look like. +/// +/// Three places say it — the file tree colours a row, the editor's gutter draws a +/// stripe beside a line, and the sidebar's change list dots each entry — and they +/// are the same claim about the same repository, so they are the same colours. +/// Before this they were three separate literals and the tree agreed with neither +/// of the others. +/// +/// The values are the ones VS Code settled on rather than the system's `.green` +/// and `.red`, which are tuned for buttons: they are muted enough to sit under a +/// file name for a whole session without shouting, and light enough to carry a +/// three-point stripe on a dark gutter. Fixed rather than theme-derived, for the +/// same reason the syntax themes are checked in — a changed file should look the +/// same on every Mac. +enum GitChangeColors { + static let added = Color(nsColor: addedNS) + static let renamed = Color(nsColor: renamedNS) + static let modified = Color(nsColor: modifiedNS) + static let deleted = Color(nsColor: deletedNS) + static let conflicted = Color(nsColor: conflictedNS) + + /// The gutter is AppKit and draws into a `CGContext`, so it wants these + /// rather than the SwiftUI wrappers above. + static let addedNS = NSColor(rgb: 0x73_C9_91) + static let renamedNS = NSColor(rgb: 0x6C_9E_F8) + static let modifiedNS = NSColor(rgb: 0xE2_C0_8D) + static let deletedNS = NSColor(rgb: 0xC7_4E_39) + static let conflictedNS = NSColor(rgb: 0xE4_67_6B) + + static func color(for kind: GitChangeKind) -> Color { + switch kind { + case .added: added + case .renamed: renamed + case .modified: modified + case .deleted: deleted + case .conflicted: conflicted + } + } +} + +extension GitChangeKind { + var color: Color { GitChangeColors.color(for: self) } +} diff --git a/Sources/Workspace/Support/RemoteImage.swift b/Sources/Workspace/Support/RemoteImage.swift index 0a4d1ee..be96ee3 100644 --- a/Sources/Workspace/Support/RemoteImage.swift +++ b/Sources/Workspace/Support/RemoteImage.swift @@ -122,6 +122,15 @@ struct MarkdownImage: View { var body: some View { content + // The picture comes up through the placeholder it replaces rather + // than taking its place between two frames. A comment with several + // screenshots in it is otherwise read while it is still being hit + // by them. The line of text moves as the picture takes its real + // height, and that is not worth animating away: the reader is at + // the top of a comment that is still arriving, and a height easing + // open under them is worse than one that simply is. + .animation(ViewerMotion.isReduced ? nil : .easeOut(duration: 0.2), value: image != nil) + .animation(ViewerMotion.isReduced ? nil : .easeOut(duration: 0.2), value: isLoading) .task { await load() } } @@ -147,12 +156,14 @@ struct MarkdownImage: View { .onTapGesture { open() } .pointerCursor() .help(url.isFileURL ? "Open this file" : "Open in browser") + .transition(.opacity) } else if isLoading { placeholder { ProgressView().controlSize(.small) Text(caption) .foregroundStyle(.secondary) } + .transition(.opacity) } else { placeholder { Image(systemName: "photo") @@ -173,6 +184,7 @@ struct MarkdownImage: View { .buttonStyle(.link) .pointerCursor() } + .transition(.opacity) } } diff --git a/Sources/Workspace/Support/Skeleton.swift b/Sources/Workspace/Support/Skeleton.swift new file mode 100644 index 0000000..8c409ec --- /dev/null +++ b/Sources/Workspace/Support/Skeleton.swift @@ -0,0 +1,139 @@ +import AppKit +import SwiftUI + +/// The shape of something that has not arrived yet. +/// +/// A spinner says only that the app is busy. A skeleton says what is coming and +/// where it will be, so the page does not jump when it lands — the rows are +/// already the right height and the columns already the right width, and what +/// happens on arrival is text replacing grey rather than a layout being built. +/// +/// **It is for a first load only.** Anything being *re*-read already has its +/// last answer on screen, and that answer is better than a grey box: the builds +/// panel keeps its list through a failed tick for exactly this reason. A +/// skeleton where a reload happens is a screen that throws away what it knows +/// every few seconds. +/// +/// The pulse is one animation for a whole group rather than one per block — +/// see ``SkeletonGroup``. Twenty blocks each running their own repeating +/// animation is twenty timers, and a placeholder must not cost more than the +/// thing it stands in for. +struct Skeleton: View { + /// How wide, as a fraction of what is offered. Rows of text are ragged, and + /// a column of identical bars reads as a table rather than as prose. + var width: CGFloat? + var height: CGFloat = 12 + var cornerRadius: CGFloat = 4 + + @Environment(\.skeletonPulse) private var pulse + + var body: some View { + RoundedRectangle(cornerRadius: cornerRadius, style: .continuous) + .fill(.quaternary) + .opacity(pulse) + .frame(width: width, height: height) + .frame(maxWidth: width == nil ? .infinity : nil, alignment: .leading) + // It stands for content, and content is read. Nothing here is worth + // a stop on the way through with the keyboard. + .accessibilityHidden(true) + } +} + +/// A circle, for where a face goes. +struct SkeletonCircle: View { + var size: CGFloat = 20 + + @Environment(\.skeletonPulse) private var pulse + + var body: some View { + Circle() + .fill(.quaternary) + .opacity(pulse) + .frame(width: size, height: size) + .accessibilityHidden(true) + } +} + +/// Holds the breath every skeleton inside it takes. +/// +/// One repeating animation, published down the tree, rather than one per block. +/// Under Reduce Motion it does not breathe at all — the blocks are still the +/// shape of what is coming, which is the half of this that carries the meaning. +struct SkeletonGroup: View { + @ViewBuilder var content: Content + + @State private var isDim = false + + var body: some View { + content + .environment(\.skeletonPulse, isDim ? 0.45 : 0.85) + .onAppear { + guard !ViewerMotion.isReduced else { return } + withAnimation(.easeInOut(duration: 0.9).repeatForever(autoreverses: true)) { + isDim = true + } + } + } +} + +private struct SkeletonPulseKey: EnvironmentKey { + /// What a block outside any group is drawn at: still there, still grey, + /// simply not breathing. + static let defaultValue: Double = 0.7 +} + +extension EnvironmentValues { + var skeletonPulse: Double { + get { self[SkeletonPulseKey.self] } + set { self[SkeletonPulseKey.self] = newValue } + } +} + +// MARK: - The shapes this app waits for + +/// A row of the kind the lists in the side panes are made of: a face, a line of +/// text, and a shorter line under it. +struct SkeletonRow: View { + var hasAvatar = true + /// Which of the ragged widths this row takes, so a column of them does not + /// come out as a rectangle. + var seed: Int = 0 + + private static let widths: [CGFloat] = [0.82, 0.64, 0.91, 0.73, 0.58] + + var body: some View { + HStack(spacing: 8) { + if hasAvatar { + SkeletonCircle(size: 20) + } + VStack(alignment: .leading, spacing: 5) { + GeometryReader { geometry in + VStack(alignment: .leading, spacing: 5) { + Skeleton(width: geometry.size.width * Self.widths[seed % Self.widths.count]) + Skeleton(width: geometry.size.width * 0.35, height: 9) + } + } + .frame(height: 26) + } + } + .padding(.horizontal, 8) + .padding(.vertical, 6) + } +} + +/// A column of them, for a list that has not come back yet. +struct SkeletonList: View { + var rows = 5 + var hasAvatar = true + + var body: some View { + SkeletonGroup { + VStack(alignment: .leading, spacing: 4) { + ForEach(0.. some View { Group { switch store.navigatorTab { case .files: - FileListView(project: project) - .transition(ViewerMotion.contentArrival) + tab { FileListView(project: project) } case .changes: - ChangeListView(project: project) - .transition(ViewerMotion.contentArrival) + tab { ChangeListView(project: project) } case .terminals: - TerminalListView(project: project) - .transition(ViewerMotion.contentArrival) + tab { TerminalListView(project: project) } case .claude: - ClaudeSessionListView(project: project) - .transition(ViewerMotion.contentArrival) + tab { ClaudeSessionListView(project: project) } case .info: - InfoPanelView(project: project) - .transition(ViewerMotion.contentArrival) + tab { InfoPanelView(project: project) } } } .animation(ViewerMotion.contentChange, value: store.navigatorTab) } + + /// One tab's list, carrying the pane's own colour. + /// + /// The colour is here rather than only on the stack behind the whole pane, + /// because for the length of the swap both lists are on screen and a + /// background *behind the pair* separates neither of them — the list being + /// replaced shows through the one replacing it, and two sets of rows read as + /// one smeared set. Painted inside the transition, the arriving list simply + /// covers what it is replacing. None of the five paints a background of its + /// own, so this is the only opaque layer between them. + private func tab(@ViewBuilder _ list: () -> Content) -> some View { + list() + .background(Color(nsColor: .controlBackgroundColor)) + .transition(ViewerMotion.contentArrival) + } } // MARK: - Files @@ -391,6 +396,12 @@ struct FileListView: View { .font(.callout) .foregroundStyle(.secondary) .padding(.top, 12) + // "Searching…" becoming "No results" is one line saying + // something else, so the words change rather than the + // line being replaced. This is the only thing here that + // is allowed to fade — see the note under the stack. + .contentTransition(.opacity) + .animation(ViewerMotion.contentChange, value: isSearching) } else { ForEach(searchResults) { file in SearchResultFileRow( @@ -407,6 +418,17 @@ struct FileListView: View { } } } + // Deliberately without an animation on the tree-or-search branch. + // The three arms are one slot in the stack, so a transition between + // them draws both at once — a list of file names coming up through + // a list of matching lines, which is two readings of the pane laid + // over each other rather than one page replacing another. Neither + // arm can be given a backing to hide the other behind: the tree is + // a bare `ForEach` inside a lazy stack, and wrapping it in anything + // that could carry a colour is what stops it being lazy. The rows + // themselves must stay still for their own reasons — they land on a + // 120ms debounce and are rebuilt on every letter, and the tree's + // array is exactly the one D8 warns about. .padding(.bottom, 3) .padding(.horizontal, 6) } @@ -550,6 +572,11 @@ struct SearchResultFileRow: View { .foregroundStyle(.secondary) .padding(.horizontal, 4) .background(Capsule().fill(.quaternary)) + // A file that keeps its place while the query grows a letter + // gets a new count; rolling it is what says the number moved + // rather than the row being a different row. + .contentTransition(.numericText()) + .animation(ViewerMotion.badgeChange, value: file.matches.count) } .padding(.horizontal, 4) .frame(height: 19) @@ -642,6 +669,16 @@ struct CompactFileRow: View { private var isRenaming: Bool { store.renamingFile == node.url } + /// What git says about this row: the file's own verdict, or for a folder the + /// strongest one held by anything inside it. + /// + /// A deleted file is worth saying out loud: it is gone from disk, so it has + /// no row of its own here and never will. Red in the tree is therefore + /// always a folder saying something under it went missing — the file itself + /// is listed in the changes pane below, which is the only place it still + /// exists. + private var change: GitChangeKind? { project.changeKind(for: node.url) } + /// The rows this row's menu, keys and drags act on: the whole selection when /// it is part of it, itself alone when it is not. private var targets: [URL] { store.fileActionTargets(node.url) } @@ -662,6 +699,11 @@ struct CompactFileRow: View { .font(.system(size: 9, weight: .semibold)) .foregroundStyle(.secondary) .rotationEffect(.degrees(node.isExpanded ? 90 : 0)) + // The twisty turns and nothing else does. Opening a folder + // rebuilds the whole visible-row array behind a lazy stack, + // and animating that is the work the lazy stack is for + // avoiding — the rows appear, the chevron says why. + .animation(ViewerMotion.disclosure, value: node.isExpanded) .frame(width: 12) } else { Color.clear.frame(width: 12, height: 1) @@ -694,10 +736,14 @@ struct CompactFileRow: View { .font(.system(size: 13)) .lineLimit(1) .truncationMode(.middle) - .foregroundStyle(isSelected ? AnyShapeStyle(.white) : AnyShapeStyle(.primary)) + .foregroundStyle(nameStyle) } Spacer(minLength: 0) + + if let change { + changeBadge(change) + } } .padding(.horizontal, 4) .frame(height: 22) @@ -788,6 +834,39 @@ struct CompactFileRow: View { return node.url } + /// The name carries the verdict, which is the whole point of the colour: + /// it is the widest thing in the row and the thing the eye lands on. + /// + /// The selected row is the exception. It is filled with the accent colour, + /// and a muted green or tan on top of that is neither readable nor still + /// recognisably green — so the name goes white, the way the icon beside it + /// already does, and the badge at the end goes on saying what changed. + private var nameStyle: AnyShapeStyle { + if isSelected { return AnyShapeStyle(.white) } + if let change { return AnyShapeStyle(change.color) } + return AnyShapeStyle(.primary) + } + + /// The mark at the end of the row: the letter for a file, a dot for a + /// folder. A folder is standing in for whatever is inside it, and a letter + /// there would read as a claim about the folder itself. + private func changeBadge(_ change: GitChangeKind) -> some View { + let style = isSelected ? AnyShapeStyle(.white) : AnyShapeStyle(change.color) + return Group { + if node.isDirectory { + Circle() + .fill(style) + .frame(width: 5, height: 5) + } else { + Text(change.letter) + .font(.system(size: 10, weight: .semibold)) + .foregroundStyle(style) + } + } + .frame(width: 13) + .help(node.isDirectory ? "Contains changes · \(change.label)" : change.label) + } + /// Selected wins over open, and both over the hover: a row can be all three /// at once, and the selection is what the next key or menu item acts on. private var background: AnyShapeStyle { @@ -1167,6 +1246,11 @@ struct ChangeListView: View { .lineLimit(4) .textSelection(.enabled) .frame(maxWidth: .infinity, alignment: .leading) + // It arrives a moment after Commit or Push was pressed and + // pushes the whole box down as it does. Coming down into + // its own space ties it to the press; appearing between two + // frames reads as the box having always been that tall. + .transition(.opacity.combined(with: .offset(y: -4))) } // The Commit button is simply dead while a merge is half-done, and @@ -1239,6 +1323,9 @@ struct ChangeListView: View { } .controlSize(.small) } + // Only the error line above; nothing else in this box is allowed to + // move, least of all the message being typed into it. + .animation(ViewerMotion.isReduced ? nil : .easeOut(duration: 0.18), value: project.gitError) .padding(.horizontal, 12) .padding(.vertical, 8) .background(.bar) @@ -1301,16 +1388,11 @@ struct ChangeListView: View { return "Push \(ahead)" } + /// The same colours the file tree above paints a row in, from the same + /// verdict — one list and one tree describing one repository should not + /// disagree about what green means. See ``GitChangeColors``. private func color(for change: GitStatus.Change) -> Color { - if change.isConflicted { return .red } - switch change.label { - case "Added": return .green - case "Deleted": return .red - case "Renamed": return .blue - // Green like a staged add, because that is what it is about to be. - case "Untracked": return .green - default: return .orange - } + change.kind.color } } @@ -1463,6 +1545,11 @@ struct TerminalListView: View { .padding(.top, 10) } } + // ⌘T and closing a shell happen all day, so the card should be seen + // to arrive or go rather than the list simply being different. + // Keyed on the identities alone: a card whose title its prompt just + // renamed is the same card, and the list should not stir for it. + .animation(ViewerMotion.listChange, value: terminals.map(\.id)) .padding(10) } .frame(maxWidth: .infinity, maxHeight: .infinity) diff --git a/Sources/Workspace/Views/ProjectsSidebar.swift b/Sources/Workspace/Views/ProjectsSidebar.swift index 69016ac..3611095 100644 --- a/Sources/Workspace/Views/ProjectsSidebar.swift +++ b/Sources/Workspace/Views/ProjectsSidebar.swift @@ -74,7 +74,10 @@ struct ProjectsSidebar: View { // in half. .padding(.top, ClaudeWorkingBadge.height / 2 + 2) .padding(.bottom, 10) - .animation(.easeInOut(duration: 0.15), value: store.projects.map(\.id)) + // The shared token rather than a duration of its own, because + // the folded rail lists the same repositories and has to arrive + // at them the same way. + .animation(ViewerMotion.listChange, value: store.projects.map(\.id)) } } } @@ -317,10 +320,17 @@ struct HomeTerminalButton: View { .frame(minWidth: 12, minHeight: 12) .background(Capsule().fill(.tint)) .offset(x: 8, y: -6) + .contentTransition(.numericText()) + .transition(.opacity) } } // Room for the badge, so it never runs past the pane's edge. - .padding(.trailing, count > 0 ? 8 : 0) + // Kept whether or not one is drawn: this button sits at the end + // of a header row, and taking the room back when the last home + // shell closes slid the whole button sideways — a badge going + // away should not move the thing it was on. + .padding(.trailing, 8) + .animation(ViewerMotion.badgeChange, value: count) } .help(count == 0 ? "Open a terminal in your home folder (⇧⌘T)" @@ -363,6 +373,11 @@ struct CollapsedProjectsRail: View { // reason: the top square's would otherwise be clipped. .padding(.top, ClaudeWorkingBadge.height / 2 + 2) .padding(.bottom, 8) + // And the same arrival: adding a repository eased the cards in + // on the open pane and dropped one into the rail between two + // frames, which read as two different lists rather than one + // list at two widths. + .animation(ViewerMotion.listChange, value: store.projects.map(\.id)) } // The pane's footer, down to the two controls that belong to no @@ -472,6 +487,10 @@ struct ProjectCard: View { .foregroundStyle(.tertiary) } + // Every one of these is flipped by a refresh nobody asked for — the + // five-minute sweep, or a file saved somewhere behind the app — so + // the digit rolls rather than being replaced. It is the only sign a + // card that is otherwise identical has been read again. HStack(spacing: 5) { Pill( text: "\(project.pullRequests.count) PR", @@ -485,6 +504,10 @@ struct ProjectCard: View { Pill(text: "\(project.ports.count) port", color: .green) } } + .contentTransition(.numericText()) + .animation(ViewerMotion.badgeChange, value: project.pullRequests.count) + .animation(ViewerMotion.badgeChange, value: project.changeCount) + .animation(ViewerMotion.badgeChange, value: project.ports.count) } .padding(10) .frame(maxWidth: .infinity, alignment: .leading) @@ -538,6 +561,11 @@ extension View { // also what clears a 10-point inset, so the two wants are one number. ClaudeWorkingBadge(count: count) .offset(x: 6, y: -ClaudeWorkingBadge.height / 2) + // Here rather than inside the badge: what has to be animated is + // the badge coming and going, and from inside its own body it + // is not there to say so. Kept off the tile it rides, so a card + // whose count changed does not relayout with it. + .animation(ViewerMotion.badgeChange, value: count > 0) } } } @@ -589,6 +617,11 @@ struct ClaudeWorkingBadge: View { .opacity(isDim ? 0.35 : 1) .animation(.easeInOut(duration: 0.9).repeatForever(autoreverses: true), value: isDim) .onAppear { isDim = true } + // The breath was the only thing about this badge that moved: it + // popped onto the corner already breathing. It grows out of the + // corner instead — small, and away from anything being read, which + // is the one place a scale is worth having. + .transition(.scale.combined(with: .opacity)) .help(count == 1 ? "A Claude conversation is working" : "\(count) Claude conversations are working") diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 4642ae9..9be8a28 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -43,7 +43,9 @@ struct PullRequestDetailView: View { if isOpen { actionBar .background(.bar) + .transition(.opacity) Divider() + .transition(.opacity) } // The tab fills the window, with the panel beside it on Details @@ -87,6 +89,12 @@ struct PullRequestDetailView: View { } } } + // Merging or rejecting takes a whole bar out from under everything + // below it, and the page used to jump the height of it. Keyed to + // `isOpen` and nothing else: the only update that ever runs inside this + // transaction is the one that ended the pull request, so a build tick, + // a keystroke or a comment landing still redraws with nothing animated. + .animation(ViewerMotion.contentChange, value: isOpen) // A half-written description belongs to the pull request it was opened // on, and this one view is reused as the viewer moves between them. .onChange(of: item.id) { @@ -218,17 +226,24 @@ struct PullRequestDetailView: View { Image(systemName: "arrow.right").foregroundStyle(.secondary) branchChip(pr.targetBranch) if let additions = pr.additions, let deletions = pr.deletions { - Text("+\(additions)").foregroundStyle(.green) - Text("−\(deletions)").foregroundStyle(.red) + Text("+\(additions)") + .foregroundStyle(.green) + .transition(ViewerMotion.contentArrival) + Text("−\(deletions)") + .foregroundStyle(.red) + .transition(ViewerMotion.contentArrival) } if let state = pr.state.badge { badge(state, color: pr.state == .merged ? .purple : .red) + .transition(ViewerMotion.contentArrival) } if pr.isDraft { badge("Draft", color: .secondary) + .transition(ViewerMotion.contentArrival) } if let review = pr.reviewLabel { badge(review, color: review == "Approved" ? .green : .orange) + .transition(ViewerMotion.contentArrival) } approvalsBadge // Its own view, and it is the view that reads the runs: the ticker @@ -251,12 +266,32 @@ struct PullRequestDetailView: View { actionsMenu } .font(.caption.monospaced()) + // The badges arrive one after another as the host answers, and each of + // them widens the row. What fades is the badge, never the row: the + // transition is on each of them, and the transaction here is keyed to + // which badges there are — so the ten-second build tick and every + // keystroke in the page below still land with nothing animated. + .animation(ViewerMotion.badgeChange, value: badgeSignature) // Same inset as the header above it, so the tab picker at this end of // the row sits directly under the navigator's. .padding(.horizontal, AppMetrics.barHorizontalPadding) .padding(.vertical, 7) } + /// Which badges the bar is showing, as one value to key the fade on. + /// + /// Deliberately not the pull request itself: a refresh rewrites every field + /// of it, and animating on the whole thing would put a fade on a title that + /// came back the same. `item.builds` is not in here either — reading it + /// would make this view one that the ticker redraws, which is the whole + /// reason ``PullRequestBuildsBadge`` is a view of its own. + private var badgeSignature: String { + """ + \(pr.state.badge ?? "")|\(pr.isDraft)|\(pr.reviewLabel ?? "")\ + |\(pr.additions ?? -1)|\(pr.deletions ?? -1)|\(item.reviewers.isEmpty) + """ + } + /// The branch the pull request comes from, followed by the two things one /// actually does with it. The target branch stays a plain chip: there is /// nothing to check out there. @@ -294,14 +329,18 @@ struct PullRequestDetailView: View { .buttonStyle(.plain) .help(approvalsHelp) .pointerCursor() + .transition(ViewerMotion.contentArrival) } else { - approvalsChip.help(approvalsHelp) + approvalsChip + .help(approvalsHelp) + .transition(ViewerMotion.contentArrival) } } else if isOpen, !item.isLoadingReviewers, item.reviewersError == nil { Button { pendingAction = .addReviewers } label: { approvalsChip } .buttonStyle(.plain) .help(approvalsHelp) .pointerCursor() + .transition(ViewerMotion.contentArrival) } } @@ -309,7 +348,10 @@ struct PullRequestDetailView: View { HStack(spacing: 3) { Image(systemName: item.reviewers.isEmpty ? "person.badge.plus" : "checkmark.seal") if !item.reviewers.isEmpty { + // An approval landing is the one number in this bar worth + // watching change, so the digits roll rather than swap. Text("\(item.reviewers.approvedCount)/\(item.reviewers.count)") + .contentTransition(.numericText()) } } .font(.caption.weight(.medium)) @@ -317,6 +359,11 @@ struct PullRequestDetailView: View { .padding(.vertical, 2) .background(approvalsColor.opacity(0.18), in: Capsule()) .foregroundStyle(approvalsColor) + // Scoped to the chip, and keyed to the two things it draws: the count + // it says and the colour it says it in. Nothing else in the bar is + // inside this, so nothing else is ever animated by it. + .animation(ViewerMotion.badgeChange, value: item.reviewers.approvalSummary) + .animation(ViewerMotion.badgeChange, value: approvalsColor) } /// Green once everybody asked has approved, orange while anyone wants @@ -496,21 +543,30 @@ struct PullRequestDetailView: View { /// Everything about the pull request, read again from the host. It is the /// one thing here that is wanted often enough to be worth a click rather /// than a trip through the menu, so it sits in the bar as well. - @ViewBuilder private var refreshButton: some View { - // Same treatment the action bar gives a merge or a sync: while it is in - // flight the bar says so, and there is nothing to press. - if item.isRefreshingPullRequest { - ProgressView().controlSize(.small) - } else { - barButton( - "arrow.clockwise", - help: "Reload #\(pr.number) from \(pr.host.displayName)" - ) { - refresh() + // Stacked rather than swapped in the row. Both are alive for the length + // of the fade, and side by side in the bar they would push the menu + // beside them along and back again; in one slot the bar is as wide + // either way, and at rest there is only ever one of them in it. + ZStack { + // Same treatment the action bar gives a merge or a sync: while it is + // in flight the bar says so, and there is nothing to press. + if item.isRefreshingPullRequest { + ProgressView() + .controlSize(.small) + .transition(ViewerMotion.contentArrival) + } else { + barButton( + "arrow.clockwise", + help: "Reload #\(pr.number) from \(pr.host.displayName)" + ) { + refresh() + } + .disabled(item.isRunningPullRequestAction) + .transition(ViewerMotion.contentArrival) } - .disabled(item.isRunningPullRequestAction) } + .animation(ViewerMotion.badgeChange, value: item.isRefreshingPullRequest) } private func refresh() { @@ -609,18 +665,28 @@ struct PullRequestDetailView: View { // Who is reviewing used to be a row here; it lives in the panel // down the right now, where it is on screen for the diff too. - if let draft = descriptionDraft { - PullRequestDescriptionEditor( - initialText: draft, - host: pr.host, - isSaving: item.isRunningPullRequestAction, - onCancel: { descriptionDraft = nil }, - onSave: { text in await saveDescription(text) } - ) - } else if !pr.body.isEmpty { - MarkdownText(text: pr.body) - .font(.callout) + // The drawn description and the box it is edited in are the same + // words twice over, so the one becomes the other rather than + // being replaced. Keyed to which of them is open and to nothing + // else — the box grows with what is typed into it, and a height + // on a curve is a text view laid out again on every letter. + Group { + if let draft = descriptionDraft { + PullRequestDescriptionEditor( + initialText: draft, + host: pr.host, + isSaving: item.isRunningPullRequestAction, + onCancel: { descriptionDraft = nil }, + onSave: { text in await saveDescription(text) } + ) + .transition(ViewerMotion.contentArrival) + } else if !pr.body.isEmpty { + MarkdownText(text: pr.body) + .font(.callout) + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.contentChange, value: descriptionDraft == nil) Divider() } @@ -663,62 +729,90 @@ struct PullRequestDetailView: View { if saved { descriptionDraft = nil } } - @ViewBuilder private var diffTab: some View { - if item.isLoading { - ProgressView("Loading diff…") - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if let diff = item.diff, !diff.isEmpty { - DiffView( - diff: diff, - layout: Binding(get: { item.diffLayout }, set: { item.diffLayout = $0 }), - selectedFile: Binding(get: { item.diffFile }, set: { item.diffFile = $0 }), - comments: DiffComments( - threads: item.inlineCommentThreads, - isPosting: item.isPostingComment, - mentions: MentionSource( - people: item.reviewerCandidates, - host: pr.host, - load: { - await store.loadReviewerCandidates(item, project: project, pr: pr) + // The spinner and the diff share the pane, so the one gives way to the + // other rather than being cut to it. Keyed to `isLoading` alone: a diff + // already on screen redraws with nothing animated, which is what keeps + // the rows out of it. + Group { + if item.isLoading { + placeholder { + ProgressView("Loading diff…") + } + .transition(ViewerMotion.contentArrival) + } else if let diff = item.diff, !diff.isEmpty { + DiffView( + diff: diff, + layout: Binding(get: { item.diffLayout }, set: { item.diffLayout = $0 }), + selectedFile: Binding(get: { item.diffFile }, set: { item.diffFile = $0 }), + comments: DiffComments( + threads: item.inlineCommentThreads, + isPosting: item.isPostingComment, + mentions: MentionSource( + people: item.reviewerCandidates, + host: pr.host, + load: { + await store.loadReviewerCandidates(item, project: project, pr: pr) + } + ), + add: { anchor, body in + await store.postInlineComment( + body, + at: anchor, + on: item, + project: project, + pr: pr + ) + }, + reply: { parent, body in + await store.postComment( + body, + on: item, + project: project, + pr: pr, + replyingTo: parent + ) + }, + resolve: { comment, resolved in + await store.setCommentResolved( + resolved, + for: comment, + on: item, + project: project, + pr: pr + ) } - ), - add: { anchor, body in - await store.postInlineComment( - body, - at: anchor, - on: item, - project: project, - pr: pr - ) - }, - reply: { parent, body in - await store.postComment( - body, - on: item, - project: project, - pr: pr, - replyingTo: parent - ) - }, - resolve: { comment, resolved in - await store.setCommentResolved( - resolved, - for: comment, - on: item, - project: project, - pr: pr - ) - } + ) ) - ) - } else { - ContentUnavailableView( - "No diff", - systemImage: "plusminus", - description: Text(item.errorMessage ?? "This pull request has no textual changes.") - ) + .transition(ViewerMotion.contentArrival) + } else { + placeholder { + ContentUnavailableView( + "No diff", + systemImage: "plusminus", + description: Text( + item.errorMessage ?? "This pull request has no textual changes." + ) + ) + } + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.contentChange, value: item.isLoading) + } + + /// A tab with nothing in it yet — reading, or nothing to read. + /// + /// It carries the pane's own colour, which is what the diff and the commit + /// list both paint for themselves. A tab arriving has to cover the one it + /// replaces rather than come up through it, and these stand where those two + /// would be. + private func placeholder( + @ViewBuilder content: () -> Content + ) -> some View { + content() + .frame(maxWidth: .infinity, maxHeight: .infinity) + .background(Color(nsColor: AppColors.viewerBackground)) } // MARK: - Commits @@ -745,62 +839,71 @@ struct PullRequestDetailView: View { } } - @ViewBuilder private var commitList: some View { - if item.isLoadingCommits && item.commits.isEmpty { - ProgressView("Loading commits…") - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if item.commits.isEmpty { - ContentUnavailableView { - Label("No commits", systemImage: "clock.arrow.circlepath") - } description: { - Text(item.commitsError ?? "This pull request has no commits.") - } actions: { - Button("Try Again") { - Task { await store.loadCommits(item, project: project, pr: pr) } + Group { + if item.isLoadingCommits && item.commits.isEmpty { + placeholder { + ProgressView("Loading commits…") } - .buttonStyle(.bordered) - .controlSize(.small) - .pointerCursor() - } - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else { - // A day at a time, headed the way the dashboard heads its own - // history: the same list of the same thing, so it reads the same. - ScrollView { - LazyVStack(alignment: .leading, spacing: 10) { - ForEach(CommitDay.group(item.commits)) { day in - VStack(alignment: .leading, spacing: 6) { - CommitDayHeading( - title: day.title, - count: day.commits.count - ) - ForEach(day.commits) { commit in - CommitRow( - commit: commit, - onOpen: { - Task { - await store.showCommit( - commit, - on: item, - project: project, - pr: pr - ) - } - }, - openPullRequest: { - store.openPullRequest(number: $0, project: project) - } + .transition(ViewerMotion.contentArrival) + } else if item.commits.isEmpty { + placeholder { + ContentUnavailableView { + Label("No commits", systemImage: "clock.arrow.circlepath") + } description: { + Text(item.commitsError ?? "This pull request has no commits.") + } actions: { + Button("Try Again") { + Task { await store.loadCommits(item, project: project, pr: pr) } + } + .buttonStyle(.bordered) + .controlSize(.small) + .pointerCursor() + } + } + .transition(ViewerMotion.contentArrival) + } else { + // A day at a time, headed the way the dashboard heads its own + // history: the same list of the same thing, so it reads the same. + ScrollView { + LazyVStack(alignment: .leading, spacing: 10) { + ForEach(CommitDay.group(item.commits)) { day in + VStack(alignment: .leading, spacing: 6) { + CommitDayHeading( + title: day.title, + count: day.commits.count ) + ForEach(day.commits) { commit in + CommitRow( + commit: commit, + onOpen: { + Task { + await store.showCommit( + commit, + on: item, + project: project, + pr: pr + ) + } + }, + openPullRequest: { + store.openPullRequest(number: $0, project: project) + } + ) + } } } } + .padding(12) + .frame(maxWidth: .infinity, alignment: .leading) } - .padding(12) - .frame(maxWidth: .infinity, alignment: .leading) + .background(Color(nsColor: AppColors.viewerBackground)) + .transition(ViewerMotion.contentArrival) } - .background(Color(nsColor: AppColors.viewerBackground)) } + // The list arriving over the spinner, and nothing else: the rows + // themselves are never in a transaction of their own. + .animation(ViewerMotion.contentChange, value: item.isLoadingCommits) } /// One commit: its message and who wrote it, above the patch itself. @@ -808,35 +911,47 @@ struct PullRequestDetailView: View { VStack(spacing: 0) { commitDetailBar(commit) .background(.bar) + // Opaque while it arrives — see `DiffLayoutBar`, which carries + // the same colour under the same material for the same reason. + .background(Color(nsColor: AppColors.viewerBackground)) Divider() - if let diff = item.commitDiffs[commit.sha], !diff.isEmpty { - DiffView( - diff: diff, - layout: Binding(get: { item.diffLayout }, set: { item.diffLayout = $0 }), - selectedFile: Binding( - get: { item.commitDiffFile }, - set: { item.commitDiffFile = $0 } - ), - // One commit of a pull request reads like any other commit: - // the file index waits until it is asked for. - showsFiles: Binding( - get: { item.showsDiffFileList }, - set: { item.showsDiffFileList = $0 } - ) - ) - } else if item.isLoadingCommitDiff { - ProgressView("Loading changes…") - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else { - ContentUnavailableView( - "No changes", - systemImage: "plusminus", - description: Text( - item.commitDiffError ?? "This commit has no textual changes." + Group { + if let diff = item.commitDiffs[commit.sha], !diff.isEmpty { + DiffView( + diff: diff, + layout: Binding(get: { item.diffLayout }, set: { item.diffLayout = $0 }), + selectedFile: Binding( + get: { item.commitDiffFile }, + set: { item.commitDiffFile = $0 } + ), + // One commit of a pull request reads like any other + // commit: the file index waits until it is asked for. + showsFiles: Binding( + get: { item.showsDiffFileList }, + set: { item.showsDiffFileList = $0 } + ) ) - ) + .transition(ViewerMotion.contentArrival) + } else if item.isLoadingCommitDiff { + placeholder { + ProgressView("Loading changes…") + } + .transition(ViewerMotion.contentArrival) + } else { + placeholder { + ContentUnavailableView( + "No changes", + systemImage: "plusminus", + description: Text( + item.commitDiffError ?? "This commit has no textual changes." + ) + ) + } + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.contentChange, value: item.isLoadingCommitDiff) } } @@ -970,13 +1085,22 @@ struct PullRequestBuildsBadge: View { Button(action: onTap) { HStack(spacing: 3) { Image(systemName: state.symbol) + // A job appearing or dropping off is a number changing, and + // it changes while nobody is looking at it — so the digits + // roll rather than being replaced between two frames. Text("\(item.builds.count)") + .contentTransition(.numericText()) } .font(.caption.weight(.medium)) .padding(.horizontal, 6) .padding(.vertical, 2) .background(state.color.opacity(0.18), in: Capsule()) .foregroundStyle(state.color) + // The one place on the page it is safe to animate the ticker: + // this view is the whole of what the ten-second read redraws, + // and the transaction reaches nothing outside it. + .animation(ViewerMotion.badgeChange, value: item.builds.count) + .animation(ViewerMotion.badgeChange, value: state) } .buttonStyle(.plain) .help(help) @@ -1174,22 +1298,33 @@ struct ConversationView: View { LazyVStack(alignment: .leading, spacing: 10) { header() - if item.isLoadingComments { - HStack(spacing: 7) { - ProgressView().controlSize(.small) - Text("Loading conversation…").foregroundStyle(.secondary) - } - .padding(.top, 8) - } else if let error = item.commentError, item.comments.isEmpty { - Label(error, systemImage: "exclamationmark.triangle") - .font(.callout) - .foregroundStyle(.secondary) - .padding(.top, 8) - } else if item.comments.isEmpty { - Text("No comments yet.") - .foregroundStyle(.secondary) + // What the page says while there is nothing to say. The + // transaction is on this group alone and keyed to the read + // itself: the stack around it holds every comment on the + // pull request, and a fade put there would run over all of + // them every time a letter is typed in the box below. + Group { + if item.isLoadingComments { + HStack(spacing: 7) { + ProgressView().controlSize(.small) + Text("Loading conversation…").foregroundStyle(.secondary) + } .padding(.top, 8) + .transition(ViewerMotion.contentArrival) + } else if let error = item.commentError, item.comments.isEmpty { + Label(error, systemImage: "exclamationmark.triangle") + .font(.callout) + .foregroundStyle(.secondary) + .padding(.top, 8) + .transition(ViewerMotion.contentArrival) + } else if item.comments.isEmpty { + Text("No comments yet.") + .foregroundStyle(.secondary) + .padding(.top, 8) + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.contentChange, value: item.isLoadingComments) // Threaded once, where the comments landed — see // `ViewerItem.commentThreads`. @@ -1233,6 +1368,12 @@ struct ConversationView: View { onPost: post ) } + // 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 + // rest — what it buys is a tab arriving over this one covering it, + // instead of a page of comments coming up through a page of code. + .background(Color(nsColor: AppColors.viewerBackground)) } /// Posts what was written, and says whether it landed — which is what @@ -1276,10 +1417,14 @@ struct ConversationComposer: View { var body: some View { VStack(alignment: .leading, spacing: 7) { if let error { + // It lands a moment after Post was pressed, and the eye is on + // the button rather than on the line above it — so it comes up + // from the box instead of simply being there. Text(error) .font(.caption) .foregroundStyle(.red) .lineLimit(2) + .transition(.opacity.combined(with: .move(edge: .bottom))) } // A line and a bit at rest, growing with what is written. Most @@ -1317,6 +1462,9 @@ struct ConversationComposer: View { } .padding(12) .background(.bar) + // Keyed to what the host said and nothing else, so the box below it is + // never in a transaction while it is being typed in. + .animation(ViewerMotion.badgeChange, value: error) } private func post() async { @@ -1360,12 +1508,18 @@ struct CommentThread: View { node: node, isExpanded: isExpanded, showsPath: !isInline, - onTap: { isExpanded.toggle() } + // On the toggle, not on the stack around it: this is one + // thread opening, and the page it sits on is every comment + // on the pull request. + onTap: { + withAnimation(ViewerMotion.contentChange) { isExpanded.toggle() } + } ) } if !isSettled || isExpanded { thread + .transition(.opacity) } } } @@ -1377,9 +1531,7 @@ struct CommentThread: View { replyCount: depth == 0 ? node.totalReplies : 0, isReplying: replyingTo == node.comment, isBusy: isPosting, - onReplyTapped: { - replyingTo = replyingTo == node.comment ? nil : node.comment - }, + onReplyTapped: openReply, onResolveTapped: resolveAction ) @@ -1393,6 +1545,7 @@ struct CommentThread: View { onSend: { body in await onReply(node.comment, body) } ) .padding(.leading, Self.indent) + .transition(.opacity) } if !node.replies.isEmpty { @@ -1421,6 +1574,22 @@ struct CommentThread: View { } } + /// Opens the reply box under this comment, or closes the one that is open. + /// + /// It fades in in the conversation and appears at once in the diff, which + /// is not an oversight. Inline, `replyingTo` belongs to the view that holds + /// the flattened diff — an animated transaction on it is one that reaches + /// every row of the file being read, and a ten-thousand-line diff is what + /// that stack is flat for. + private func openReply() { + let next = replyingTo == node.comment ? nil : node.comment + if isInline { + replyingTo = next + } else { + withAnimation(ViewerMotion.contentChange) { replyingTo = next } + } + } + /// What the Resolve button does, or nil where it is not drawn at all. private var resolveAction: (() -> Void)? { guard depth == 0, node.comment.canResolve, let onResolve else { return nil } @@ -1632,49 +1801,58 @@ struct PullRequestReviewerSheet: View { .background(.quaternary.opacity(0.2), in: RoundedRectangle(cornerRadius: 8)) } - @ViewBuilder private func candidateList(_ offer: Offer) -> some View { - if item.isLoadingReviewerCandidates { - HStack(spacing: 7) { - ProgressView().controlSize(.small) - Text("Reading who can review…") + Group { + if item.isLoadingReviewerCandidates { + HStack(spacing: 7) { + ProgressView().controlSize(.small) + Text("Reading who can review…") + .font(.callout) + .foregroundStyle(.secondary) + } + .frame(height: 150, alignment: .top) + .transition(ViewerMotion.contentArrival) + } else if offer.matches.isEmpty && offer.typedHandle == nil { + Text(offer.candidates.isEmpty + ? "\(pr.host.displayName) did not say who can review this. Type a handle above and it is sent as it is." + : "Nobody here matches “\(query)”.") .font(.callout) .foregroundStyle(.secondary) - } - .frame(height: 150, alignment: .top) - } else if offer.matches.isEmpty && offer.typedHandle == nil { - Text(offer.candidates.isEmpty - ? "\(pr.host.displayName) did not say who can review this. Type a handle above and it is sent as it is." - : "Nobody here matches “\(query)”.") - .font(.callout) - .foregroundStyle(.secondary) - .fixedSize(horizontal: false, vertical: true) - .frame(height: 150, alignment: .top) - } else { - ScrollView { - VStack(spacing: 4) { - if let typedHandle = offer.typedHandle { - row( - handle: typedHandle, - name: "Ask “\(typedHandle)”", - detail: "Sent to \(pr.host.displayName) as typed", - avatarURL: nil, - alreadyAsked: offer.alreadyAsked - ) - } - ForEach(offer.matches) { candidate in - row( - handle: candidate.handle, - name: candidate.name, - detail: candidate.detail, - avatarURL: candidate.avatarURL, - alreadyAsked: offer.alreadyAsked - ) + .fixedSize(horizontal: false, vertical: true) + .frame(height: 150, alignment: .top) + .transition(ViewerMotion.contentArrival) + } else { + ScrollView { + VStack(spacing: 4) { + if let typedHandle = offer.typedHandle { + row( + handle: typedHandle, + name: "Ask “\(typedHandle)”", + detail: "Sent to \(pr.host.displayName) as typed", + avatarURL: nil, + alreadyAsked: offer.alreadyAsked + ) + } + ForEach(offer.matches) { candidate in + row( + handle: candidate.handle, + name: candidate.name, + detail: candidate.detail, + avatarURL: candidate.avatarURL, + alreadyAsked: offer.alreadyAsked + ) + } } } + .frame(height: 150) + .transition(ViewerMotion.contentArrival) } - .frame(height: 150) } + // The list coming up over the spinner, and only that. Keyed to the read + // rather than to what is in the box: the rows below are rebuilt on + // every letter typed into the search field, and none of that is + // anything to animate. + .animation(ViewerMotion.contentChange, value: item.isLoadingReviewerCandidates) } /// One person the picker offers. Someone already on the pull request stays diff --git a/Sources/Workspace/Views/PullRequestSidebar.swift b/Sources/Workspace/Views/PullRequestSidebar.swift index cba37fb..4260df5 100644 --- a/Sources/Workspace/Views/PullRequestSidebar.swift +++ b/Sources/Workspace/Views/PullRequestSidebar.swift @@ -55,16 +55,25 @@ struct PullRequestSidebar: View { EmptyView() } - if ordered.isEmpty { - emptyNote(item.reviewersError - ?? (item.isLoadingReviewers - ? "Reading who is reviewing…" - : "Nobody is reviewing #\(pr.number) yet.")) - } else { - ForEach(ordered) { reviewer in - reviewerRow(reviewer) + // Who is reviewing changes when somebody reviews or the sheet asks + // more people — a handful of times an hour at the very most, and + // never on a timer. So the list is one of the few here worth + // animating, keyed to who is on it rather than to what they said. + Group { + if ordered.isEmpty { + emptyNote(item.reviewersError + ?? (item.isLoadingReviewers + ? "Reading who is reviewing…" + : "Nobody is reviewing #\(pr.number) yet.")) + .transition(ViewerMotion.contentArrival) + } else { + ForEach(ordered) { reviewer in + reviewerRow(reviewer) + .transition(.opacity) + } } } + .animation(ViewerMotion.listChange, value: item.reviewers.map(\.id)) // Nothing to ask of a pull request that has already ended. The // button sits under the list rather than in the header: it is the @@ -115,25 +124,37 @@ struct PullRequestSidebar: View { .disabled(item.isLoadingBuilds) } - if item.builds.isEmpty { - emptyNote(item.buildsError - ?? (item.isLoadingBuilds - ? "Reading builds…" - : "Nothing has run against this pull request's head commit on \(pr.host.displayName).")) - if item.buildsError != nil, let url = pr.url { - Button { - NSWorkspace.shared.open(url) - } label: { - Label("Open in Browser", systemImage: "safari") - .font(.caption) + // The note gives way to the first runs, and that is the whole of the + // motion here. The **rows** are deliberately left out of it: the + // list is read again every ten seconds and the loader sorts + // failures to the top, so they genuinely reorder on a timer — an + // animated list would have this panel shuffling by itself while it + // is being read. A running job already says it is moving, in the + // pulse on its own glyph. + Group { + if item.builds.isEmpty { + emptyNote(item.buildsError + ?? (item.isLoadingBuilds + ? "Reading builds…" + : "Nothing has run against this pull request's head commit on \(pr.host.displayName).")) + .transition(ViewerMotion.contentArrival) + if item.buildsError != nil, let url = pr.url { + Button { + NSWorkspace.shared.open(url) + } label: { + Label("Open in Browser", systemImage: "safari") + .font(.caption) + } + .buttonStyle(.plain) + .foregroundStyle(.secondary) + .pointerCursor() + .transition(ViewerMotion.contentArrival) } - .buttonStyle(.plain) - .foregroundStyle(.secondary) - .pointerCursor() + } else { + ForEach(item.builds) { BuildRow(build: $0) } } - } else { - ForEach(item.builds) { BuildRow(build: $0) } } + .animation(ViewerMotion.contentChange, value: item.builds.isEmpty) } } diff --git a/Sources/Workspace/Views/PullRequestTable.swift b/Sources/Workspace/Views/PullRequestTable.swift index eb57ff3..5ccfd53 100644 --- a/Sources/Workspace/Views/PullRequestTable.swift +++ b/Sources/Workspace/Views/PullRequestTable.swift @@ -34,11 +34,24 @@ struct PullRequestTable: View { var body: some View { VStack(alignment: .leading, spacing: 10) { title - if project.pullRequests.isEmpty { - emptyState - } else { - table + // Two transactions, and neither of them the requests themselves. A + // sweep every five minutes rewrites all fifty of these, so keying + // the fade to the values would put one on every row of a board that + // came back exactly as it went out; keyed to the identities, it + // runs only when a request has actually opened, merged or moved. + // The other is the read, which is what carries the note away and + // brings the table up in its place. + Group { + if project.pullRequests.isEmpty { + emptyState + .transition(ViewerMotion.contentArrival) + } else { + table + .transition(ViewerMotion.contentArrival) + } } + .animation(ViewerMotion.listChange, value: project.pullRequests.map(\.id)) + .animation(ViewerMotion.contentChange, value: project.isLoadingPullRequests) } } @@ -75,6 +88,7 @@ struct PullRequestTable: View { private var emptyState: some View { if project.isLoadingPullRequests { note("Reading pull requests…") + .transition(ViewerMotion.contentArrival) } else if let error = project.pullRequestError { VStack(alignment: .leading, spacing: 6) { note(error) @@ -82,8 +96,10 @@ struct PullRequestTable: View { .controlSize(.small) .pointerCursor() } + .transition(ViewerMotion.contentArrival) } else { note("Nothing is open — everything is merged.") + .transition(ViewerMotion.contentArrival) } } diff --git a/Sources/Workspace/Views/UpdateSettings.swift b/Sources/Workspace/Views/UpdateSettings.swift index a59097c..f82bded 100644 --- a/Sources/Workspace/Views/UpdateSettings.swift +++ b/Sources/Workspace/Views/UpdateSettings.swift @@ -220,38 +220,56 @@ struct UpdateBanner: View { /// update is never actually lost. @State private var dismissedVersion: String? + /// The release this banner is for, or nothing at all. Pulled out of the body + /// because the transition below needs a transaction, and an `if` written + /// straight into `body` has nothing above it for one to sit on — which is + /// why the notice used to appear and go without playing either. + private var announced: AppRelease? { + guard case .ready(let release) = updater.stage, + dismissedVersion != release.version.text + else { return nil } + return release + } + var body: some View { - if case .ready(let release) = updater.stage, dismissedVersion != release.version.text { - HStack(spacing: 10) { - Image(systemName: "arrow.down.circle.fill") - .foregroundStyle(.blue) - VStack(alignment: .leading, spacing: 1) { - Text("Workspace \(release.version.text) is ready") - .font(.callout.weight(.medium)) - Text("Restarting takes a moment.") - .font(.caption) - .foregroundStyle(.secondary) - } - Button("Relaunch") { updater.installAndRelaunch() } - .buttonStyle(.borderedProminent) - .controlSize(.small) - .pointerCursor() - Button { - dismissedVersion = release.version.text - } label: { - Image(systemName: "xmark") - .foregroundStyle(.tertiary) - } - .buttonStyle(.plain) - .help("Later — the gear in the sidebar keeps it") + Group { + if let release = announced { + banner(release) + } + } + .animation(ViewerMotion.listChange, value: announced?.version.text) + } + + private func banner(_ release: AppRelease) -> some View { + HStack(spacing: 10) { + Image(systemName: "arrow.down.circle.fill") + .foregroundStyle(.blue) + VStack(alignment: .leading, spacing: 1) { + Text("Workspace \(release.version.text) is ready") + .font(.callout.weight(.medium)) + Text("Restarting takes a moment.") + .font(.caption) + .foregroundStyle(.secondary) + } + Button("Relaunch") { updater.installAndRelaunch() } + .buttonStyle(.borderedProminent) + .controlSize(.small) .pointerCursor() + Button { + dismissedVersion = release.version.text + } label: { + Image(systemName: "xmark") + .foregroundStyle(.tertiary) } - .padding(.horizontal, 14) - .padding(.vertical, 10) - .background(.regularMaterial, in: .rect(cornerRadius: 10, style: .continuous)) - .shadow(radius: 6, y: 2) - .padding(16) - .transition(.move(edge: .bottom).combined(with: .opacity)) + .buttonStyle(.plain) + .help("Later — the gear in the sidebar keeps it") + .pointerCursor() } + .padding(.horizontal, 14) + .padding(.vertical, 10) + .background(.regularMaterial, in: .rect(cornerRadius: 10, style: .continuous)) + .shadow(radius: 6, y: 2) + .padding(16) + .transition(.move(edge: .bottom).combined(with: .opacity)) } } diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index 9ef849e..fff2cbc 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -38,6 +38,16 @@ struct ViewerView: View { return item.id } + /// Whether the row under the pane has anything to say: an editor is open, + /// and it is the editor rather than a rendered page or the board. Named + /// because the fade below has to watch it, and a `case let` written into the + /// `if` is not something a transaction can be given. + private var showsStatusBar: Bool { + guard !store.showsDashboard, let document = store.current?.document else { return false } + if case .text = document.content { return true } + return false + } + /// The open item, or the dashboard when there is none. It carries the /// pane's colour itself — the window's own background sits below it. private var mainContent: some View { @@ -81,11 +91,22 @@ struct ViewerView: View { // the breadcrumb both already show. It is outside the animation on // purpose: the row appearing shortens the pane above it, and a // height that moves is a text view relaid every frame. - if !store.showsDashboard, let document = store.current?.document, - case .text = document.content { - Divider() - StatusBar(document: document) + // + // What it does get is a fade, and only a fade. A row that is going + // keeps its full height until the fade is over, so the editor above + // is relaid once on the way in and once on the way out rather than + // on every frame between. The transaction is on this group alone — + // above it sits the editor, whose geometry must never be in one. + Group { + if showsStatusBar, let document = store.current?.document { + VStack(spacing: 0) { + Divider() + StatusBar(document: document) + } + .transition(.opacity) + } } + .animation(ViewerMotion.contentChange, value: showsStatusBar) } .background(Color(nsColor: AppColors.viewerBackground)) } @@ -102,7 +123,7 @@ struct ViewerView: View { // and the rail is wide enough to hold them — see // ``CollapsedProjectsRail``. Button { - withAnimation { store.showsProjects.toggle() } + store.toggleProjects() } label: { Image(systemName: "sidebar.leading") } @@ -181,7 +202,7 @@ struct ViewerView: View { // control of the same width and padding, so that bar's segmented // picker keeps its right edge whatever glyph this button uses. Button { - withAnimation { store.toggleNavigator() } + store.toggleNavigator() } label: { Image(systemName: "sidebar.trailing") } @@ -571,29 +592,42 @@ struct WelcomeView: View { // than called "the default": which branch that is differs per // repository — `main` here, `develop` there — and the host is // what said so, so the button says it too. - if let target = project.defaultBranchToSwitchTo { - BranchActionButton( - title: "Switch to \(target)", - symbol: "arrow.uturn.backward", - help: "Check out “\(target)”, this repository's default branch", - isRunning: project.isRunningGitCommand - ) { - switchToDefaultBranch(project, branch: target) + // + // Both come and go as the repository's status is read — a + // checkout in the terminal is enough — and either one arriving + // shifts everything after it along the line. They fade rather + // than blink in, which is what makes the shift read as one + // movement instead of two. + Group { + if let target = project.defaultBranchToSwitchTo { + BranchActionButton( + title: "Switch to \(target)", + symbol: "arrow.uturn.backward", + help: "Check out “\(target)”, this repository's default branch", + isRunning: project.isRunningGitCommand + ) { + switchToDefaultBranch(project, branch: target) + } } - } - // And next to that, the same branch brought up to date. Drawn - // only for a repository that has a remote to pull from: without - // an `origin` the command has nowhere to go. - if project.gitStatus != nil, project.remote != nil { - BranchActionButton( - title: "Pull", - symbol: "arrow.down", - help: "Pull the current branch from its remote", - isRunning: project.isRunningGitCommand - ) { - pull(project) + // And next to that, the same branch brought up to date. + // Drawn only for a repository that has a remote to pull + // from: without an `origin` the command has nowhere to go. + if project.gitStatus != nil, project.remote != nil { + BranchActionButton( + title: "Pull", + symbol: "arrow.down", + help: "Pull the current branch from its remote", + isRunning: project.isRunningGitCommand + ) { + pull(project) + } } } + .animation(ViewerMotion.contentChange, value: project.defaultBranchToSwitchTo) + .animation( + ViewerMotion.contentChange, + value: project.gitStatus != nil && project.remote != nil + ) Spacer(minLength: 12) AskClaudeButton { store.openClaude(in: project) } } @@ -651,16 +685,32 @@ struct WelcomeView: View { if project.host != .unknown { sectionDivider PullRequestTable(project: project) + // The board lands before its reads come back, so the table + // fills in a moment after the tiles above it. Watched on the + // count rather than the list itself: a pull request whose + // title or review state was re-read is the same row, and it + // should redraw where it stands. + .animation(ViewerMotion.contentChange, value: project.pullRequests.count) .id(Self.pullRequestAnchor) } // The history draws nothing at all for a folder with no commits yet, // so its rule is asked for on the same condition — a divider with // nothing under it would read as the board being cut short. - if project.isLoadingCommits || !project.recentCommits.isEmpty { - sectionDivider + // + // Rule and list share the one transaction because they arrive + // together: the spinner giving way to the first page, and another + // page landing under "Load older", are the two changes it is for. + // On the sections rather than on the column above, so the tiles and + // the table are not dragged into a redraw the history caused. + Group { + if project.isLoadingCommits || !project.recentCommits.isEmpty { + sectionDivider + } + commitHistory(project) } - commitHistory(project) + .animation(ViewerMotion.contentChange, value: project.recentCommits.count) + .animation(ViewerMotion.contentChange, value: project.isLoadingCommits) } .padding(28) // The board takes the whole pane rather than stopping at a reading @@ -781,7 +831,7 @@ struct WelcomeView: View { HStack(spacing: 12) { if unread > 0 || showsEveryCommit { Button { - withAnimation(.easeInOut(duration: 0.16)) { showsEveryCommit.toggle() } + withAnimation(ViewerMotion.contentChange) { showsEveryCommit.toggle() } } label: { Label( showsEveryCommit ? "Show fewer" : "Show \(unread) more", @@ -792,10 +842,13 @@ struct WelcomeView: View { } // Loading a page and then hiding it behind "Show more" would be a - // button that seems to do nothing, so this unfolds the list too. + // button that seems to do nothing, so this unfolds the list too — + // in the same transaction as the button beside it, since the two + // sit under one list and both make it longer. What the page itself + // does when it lands is the history section's own fade. if project.hasMoreCommits { Button { - showsEveryCommit = true + withAnimation(ViewerMotion.contentChange) { showsEveryCommit = true } Task { await project.loadMoreCommits() } } label: { Label( @@ -1031,6 +1084,13 @@ struct StatTile: View { Text(value) .font(.system(size: 21, weight: .semibold, design: .rounded)) .foregroundStyle(tint) + // Every one of these is a count, and each of them lands a + // moment after the board does — the tile is drawn at zero + // and the real number arrives with the read. Rolling the + // digits is what says a number changed rather than that a + // different number was always there. + .contentTransition(.numericText()) + .animation(ViewerMotion.badgeChange, value: value) } .padding(.horizontal, 11) .padding(.vertical, 10) diff --git a/Sources/Workspace/WorkspaceApp.swift b/Sources/Workspace/WorkspaceApp.swift index 622e6d7..fe5db53 100644 --- a/Sources/Workspace/WorkspaceApp.swift +++ b/Sources/Workspace/WorkspaceApp.swift @@ -71,7 +71,7 @@ struct WorkspaceApp: App { CommandGroup(after: .sidebar) { Button(store.showsProjects ? "Hide Repositories" : "Show Repositories") { - store.showsProjects.toggle() + store.toggleProjects() } .shortcut(.toggleRepositories) diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..b44cefa --- /dev/null +++ b/TODO.md @@ -0,0 +1,4 @@ +[x] Resolved comments should be collapse and show resolved status +[x] Resolve / unresolve button on a comment thread in PR detail +[x] When PR detail is opened, build status should be refresh in every 10s +[x] Full refresh button in PR details. and instead of copy PR link in tile, replace with open in browser. From dbfbf57b2b082db8a96e4d74fd34e35a8ca78c8a Mon Sep 17 00:00:00 2001 From: necessarylion Date: Wed, 29 Jul 2026 19:53:41 +0700 Subject: [PATCH 7/7] The cap counts promotions, the box empties, the pulse hears the setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three from review, none of them big: A generation cache promoted from the old half into the live one without counting it, so a pass that read entirely out of the old half could leave both halves holding half again as much as the cap allows. Promotion now rotates the same way an insert does. The conversation composer holds its own draft, and the pull request view around it is reused as the viewer moves between pull requests, so what was typed for one was still in the box on the next. The composer is keyed to the item now, and the open reply box is put down with it. And the skeleton pulse only asked about Reduce Motion once, on appear. Turning it on mid-load left the animation running. It reads SwiftUI's own environment value now and stops or starts when that moves. Checked: Scripts/bundle.sh — build succeeded, no warnings. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VHGiqzgAyPEGRVekVg1fK9 --- Sources/Workspace/Support/Skeleton.swift | 18 ++++++++++++------ Sources/Workspace/Views/MarkdownPreview.swift | 17 +++++++++++++---- .../Views/PullRequestDetailView.swift | 12 +++++++++++- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/Sources/Workspace/Support/Skeleton.swift b/Sources/Workspace/Support/Skeleton.swift index 8c409ec..e880a16 100644 --- a/Sources/Workspace/Support/Skeleton.swift +++ b/Sources/Workspace/Support/Skeleton.swift @@ -62,17 +62,23 @@ struct SkeletonCircle: View { struct SkeletonGroup: View { @ViewBuilder var content: Content + /// SwiftUI's own reading of the setting rather than ``ViewerMotion``'s. + /// Both ask the same system for the same answer, but this one is watched: + /// turning Reduce Motion on with skeletons on screen has to stop a pulse + /// that is already running, and turning it off has to start one. + @Environment(\.accessibilityReduceMotion) private var isReduced + @State private var isDim = false var body: some View { content .environment(\.skeletonPulse, isDim ? 0.45 : 0.85) - .onAppear { - guard !ViewerMotion.isReduced else { return } - withAnimation(.easeInOut(duration: 0.9).repeatForever(autoreverses: true)) { - isDim = true - } - } + // The animation is chosen at the moment the value moves, so the + // reduced case is a plain assignment: nothing repeating is left + // running behind the blocks, which is what stopping it means here. + .animation(isReduced ? nil : .easeInOut(duration: 0.9).repeatForever(autoreverses: true), value: isDim) + .onAppear { isDim = !isReduced } + .onChange(of: isReduced) { isDim = !isReduced } } } diff --git a/Sources/Workspace/Views/MarkdownPreview.swift b/Sources/Workspace/Views/MarkdownPreview.swift index c1d039d..20e9a4b 100644 --- a/Sources/Workspace/Views/MarkdownPreview.swift +++ b/Sources/Workspace/Views/MarkdownPreview.swift @@ -96,17 +96,26 @@ private struct GenerationCache { mutating func value(for key: Key) -> Value? { if let value = live[key] { return value } guard let value = previous[key] else { return nil } + // A promotion fills the live half exactly as an insert does, and a pass + // that finds everything it wants in the old half is all promotions: this + // has to count against the cap too, or the two halves grow past it. + rotateIfFull() live[key] = value return value } mutating func insert(_ value: Value, for key: Key) { - if live.count >= limit { - previous = live - live = [:] - } + rotateIfFull() live[key] = value } + + /// Sets the live half aside once it is full, which is the only thing that + /// ever drops anything: what was in the old half at that moment is gone. + private mutating func rotateIfFull() { + guard live.count >= limit else { return } + previous = live + live = [:] + } } /// Full-page Markdown view for `.md` files: `MarkdownText` in a scroll view. diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 9be8a28..3641fdb 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -1367,7 +1367,17 @@ struct ConversationView: View { mentions: mentions, onPost: post ) - } + // The draft lives inside the composer, so this is the only way to + // end it: a half-written comment belongs to the pull request it was + // written on, and this whole view is reused as the viewer moves + // between them. Without it the text typed for one is still in the + // box on the next, and the button posts it there. + .id(item.id) + } + // 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 } // 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