From c5f4d6d6987e50665c3125e32f8c18165ab76fca Mon Sep 17 00:00:00 2001 From: systemblue Date: Mon, 15 Jun 2026 13:58:45 -0400 Subject: [PATCH] refactor: extract PullRequestDetailViewModel from the PR detail view Second slice of the incremental MVVM migration proposed in #41, and the first applied to one of the project's two largest views. PullRequestDetailView drops from 612 lines to a 174-line binding layer plus a 244-line sections extension. All state, derived properties, and async work move into a @MainActor @Observable view model behind PullRequestServiceProtocol and RepositoryServiceProtocol seams, so the view becomes a thin binding layer. Behavior is unchanged, and the view model is covered by unit tests against a fake service. Co-Authored-By: Claude Opus 4.8 --- .../PullRequestDetailServiceProtocols.swift | 47 ++ .../PullRequestDetailViewModel.swift | 283 +++++++++ .../PullRequestDetailView+Sections.swift | 244 ++++++++ Forji/Forji/Views/PullRequestDetailView.swift | 582 ++---------------- .../PullRequestDetailViewModelTests.swift | 227 +++++++ 5 files changed, 868 insertions(+), 515 deletions(-) create mode 100644 Forji/Forji/Services/PullRequestDetailServiceProtocols.swift create mode 100644 Forji/Forji/ViewModels/PullRequestDetailViewModel.swift create mode 100644 Forji/Forji/Views/PullRequestDetailView+Sections.swift create mode 100644 Forji/ForjiTests/PullRequestDetailViewModelTests.swift diff --git a/Forji/Forji/Services/PullRequestDetailServiceProtocols.swift b/Forji/Forji/Services/PullRequestDetailServiceProtocols.swift new file mode 100644 index 0000000..118e701 --- /dev/null +++ b/Forji/Forji/Services/PullRequestDetailServiceProtocols.swift @@ -0,0 +1,47 @@ +import ForgejoKit +import Foundation + +/// The pull-request operations the pull-request detail feature depends on, as a +/// seam so the view model can be exercised against a fake service in unit tests. +/// Part of the incremental MVVM migration (#41). +protocol PullRequestServiceProtocol: Sendable { + func fetchPullRequest(owner: String, repo: String, index: Int) async throws -> PullRequest + func fetchComments(owner: String, repo: String, index: Int) async throws -> [IssueComment] + func fetchReviews(owner: String, repo: String, index: Int) async throws -> [PullRequestReview] + func fetchReviewComments( + owner: String, repo: String, index: Int, reviewId: Int, + ) async throws -> [ReviewComment] + func fetchDiff(owner: String, repo: String, index: Int) async throws -> String + func setState(owner: String, repo: String, index: Int, state: String) async throws -> PullRequest + func createComment(owner: String, repo: String, index: Int, body: String) async throws -> IssueComment + func editComment(owner: String, repo: String, commentId: Int, body: String) async throws -> IssueComment +} + +/// The repository reads the pull-request detail feature depends on, exposed as a +/// seam alongside `PullRequestServiceProtocol` for the same testing reason. +protocol RepositoryServiceProtocol: Sendable { + func fetchRepository(owner: String, repo: String) async throws -> Repository + func fetchAssignees(owner: String, repo: String) async throws -> [User] +} + +/// The attachment upload the pull-request detail feature reuses from the issue +/// service, exposed as a seam alongside the others for the same testing reason. +protocol IssueServiceProtocol: Sendable { + // swiftlint:disable:next function_parameter_count + func uploadIssueAttachment( + owner: String, repo: String, index: Int, fileData: Data, fileName: String, mimeType: String, + ) async throws -> Attachment +} + +extension PullRequestService: PullRequestServiceProtocol { + /// Focused seam over `editPullRequest` for the only edit the detail view makes: + /// opening or closing the pull request. + func setState(owner: String, repo: String, index: Int, state: String) async throws -> PullRequest { + try await editPullRequest( + owner: owner, repo: repo, index: index, title: nil, body: nil, state: state, + ) + } +} + +extension RepositoryService: RepositoryServiceProtocol {} +extension IssueService: IssueServiceProtocol {} diff --git a/Forji/Forji/ViewModels/PullRequestDetailViewModel.swift b/Forji/Forji/ViewModels/PullRequestDetailViewModel.swift new file mode 100644 index 0000000..f6161b6 --- /dev/null +++ b/Forji/Forji/ViewModels/PullRequestDetailViewModel.swift @@ -0,0 +1,283 @@ +import ForgejoKit +import SwiftUI + +/// Owns the pull-request detail state and async work so the view is a thin +/// binding layer. Part of the incremental MVVM migration (#41). +@MainActor +@Observable +final class PullRequestDetailViewModel { + let prNumber: Int + + var repository: Repository + var pullRequest: PullRequest? + var comments: [IssueComment] = [] + var reviews: [PullRequestReview] = [] + var reviewComments: [Int: [ReviewComment]] = [:] + var assignees: [User] = [] + var parsedDiff: ParsedDiff? + var inlineCommentContext: InlineCommentContext? + + var isLoading = true + var isLoadingDiff = false + var isDiffExpanded = false + var isTogglingState = false + var errorMessage: String? + var showError = false + + private var diffText: String? + private let currentUserLogin: String? + private let prService: (any PullRequestServiceProtocol)? + private let repositoryService: (any RepositoryServiceProtocol)? + private let issueService: (any IssueServiceProtocol)? + + init( + repository: Repository, + prNumber: Int, + prService: (any PullRequestServiceProtocol)?, + repositoryService: (any RepositoryServiceProtocol)?, + issueService: (any IssueServiceProtocol)?, + currentUserLogin: String?, + ) { + self.repository = repository + self.prNumber = prNumber + self.prService = prService + self.repositoryService = repositoryService + self.issueService = issueService + self.currentUserLogin = currentUserLogin + } + + // MARK: - Derived State + + var owner: String { + repository.owner + } + + var repo: String { + repository.repoName + } + + var isMerged: Bool { + pullRequest?.merged == true + } + + var hasPushPermission: Bool { + repository.permissions?.push == true || repository.permissions?.admin == true + } + + var isOwnPR: Bool { + guard let currentUserLogin, let prAuthor = pullRequest?.user.login else { return false } + return currentUserLogin == prAuthor + } + + var canEditOrClose: Bool { + hasPushPermission || isOwnPR + } + + var canMerge: Bool { + hasPushPermission + } + + var status: PRStatusStyle { + PRStatusStyle( + state: pullRequest?.stateValue ?? .closed, + merged: pullRequest?.merged, + draft: pullRequest?.draft, + ) + } + + var allReviewComments: [ReviewComment] { + reviewComments.values.flatMap(\.self) + } + + // MARK: - Data Loading + + func loadData() async { + guard let prService, let repositoryService else { return } + isLoading = true + do { + async let fetchedPR = prService.fetchPullRequest(owner: owner, repo: repo, index: prNumber) + async let fetchedComments = prService.fetchComments(owner: owner, repo: repo, index: prNumber) + async let fetchedReviews = prService.fetchReviews(owner: owner, repo: repo, index: prNumber) + async let fetchedAssignees = repositoryService.fetchAssignees(owner: owner, repo: repo) + async let fetchedRepo = repositoryService.fetchRepository(owner: owner, repo: repo) + + let loadedPR = try await fetchedPR + let loadedComments = try await fetchedComments + let loadedReviews = try await fetchedReviews + let loadedAssignees = await (try? fetchedAssignees) ?? [] + + try Task.checkCancellation() + pullRequest = loadedPR + comments = loadedComments + reviews = loadedReviews + assignees = loadedAssignees + if let fullRepo = try? await fetchedRepo { + repository = fullRepo + } + + reviewComments = await fetchAllReviewComments(for: reviews) + + if !allReviewComments.isEmpty { + await loadDiff() + isDiffExpanded = true + } + } catch is CancellationError { + // Ignore cancellation + } catch { + errorMessage = error.localizedDescription + showError = true + } + isLoading = false + } + + func loadReviews() async { + guard let prService else { return } + do { + reviews = try await prService.fetchReviews(owner: owner, repo: repo, index: prNumber) + reviewComments = await fetchAllReviewComments(for: reviews) + + if !allReviewComments.isEmpty { + if diffText == nil { + await loadDiff() + } + isDiffExpanded = true + } + } catch { + errorMessage = error.localizedDescription + showError = true + } + } + + private func fetchAllReviewComments(for reviews: [PullRequestReview]) async -> [Int: [ReviewComment]] { + guard let prService else { return [:] } + let owner = owner + let repo = repo + let prNumber = prNumber + return await withTaskGroup(of: (Int, [ReviewComment])?.self) { group in + for review in reviews { + group.addTask { + guard let comments = try? await prService.fetchReviewComments( + owner: owner, repo: repo, index: prNumber, reviewId: review.id, + ), !comments.isEmpty else { + return nil + } + return (review.id, comments) + } + } + var commentsMap: [Int: [ReviewComment]] = [:] + for await result in group { + if let (reviewId, comments) = result { + commentsMap[reviewId] = comments + } + } + return commentsMap + } + } + + func loadDiff() async { + guard let prService else { return } + isLoadingDiff = true + do { + let raw = try await prService.fetchDiff(owner: owner, repo: repo, index: prNumber) + try Task.checkCancellation() + diffText = raw + parsedDiff = DiffParser.parse(raw) + } catch is CancellationError { + // Ignore cancellation + } catch { + errorMessage = error.localizedDescription + showError = true + } + isLoadingDiff = false + } + + func togglePRState() async { + guard let prService, let currentPR = pullRequest else { return } + isTogglingState = true + + do { + let newState = currentPR.stateValue == .open + ? PullRequestState.closed.rawValue : PullRequestState.open.rawValue + let updated = try await prService.setState( + owner: owner, + repo: repo, + index: prNumber, + state: newState, + ) + pullRequest = updated + NotificationCenter.default.post(name: .pullRequestsDidChange, object: nil) + } catch { + errorMessage = error.localizedDescription + showError = true + } + + isTogglingState = false + } + + @discardableResult + func editComment(commentId: Int, body: String) async -> Bool { + guard let prService else { return false } + do { + let updated = try await prService.editComment( + owner: owner, + repo: repo, + commentId: commentId, + body: body, + ) + if let index = comments.firstIndex(where: { $0.id == commentId }) { + comments[index] = updated + } + return true + } catch { + errorMessage = error.localizedDescription + showError = true + return false + } + } + + func addComment(body: String) async throws { + guard let prService else { throw URLError(.userAuthenticationRequired) } + let comment = try await prService.createComment( + owner: owner, + repo: repo, + index: prNumber, + body: body, + ) + comments.append(comment) + } + + func uploadImage(data: Data, fileName: String, mimeType: String) async throws -> String { + guard let issueService else { throw URLError(.userAuthenticationRequired) } + let attachment = try await issueService.uploadIssueAttachment( + owner: owner, + repo: repo, + index: prNumber, + fileData: data, + fileName: fileName, + mimeType: mimeType, + ) + return attachmentMarkdown(for: attachment) + } + + #if DEBUG + init( + preview _: Void, + repository: Repository, + prNumber: Int, + pullRequest: PullRequest, + comments: [IssueComment] = [], + reviews: [PullRequestReview] = [], + ) { + self.repository = repository + self.prNumber = prNumber + prService = nil + repositoryService = nil + issueService = nil + currentUserLogin = nil + self.pullRequest = pullRequest + self.comments = comments + self.reviews = reviews + isLoading = false + } + #endif +} diff --git a/Forji/Forji/Views/PullRequestDetailView+Sections.swift b/Forji/Forji/Views/PullRequestDetailView+Sections.swift new file mode 100644 index 0000000..f510387 --- /dev/null +++ b/Forji/Forji/Views/PullRequestDetailView+Sections.swift @@ -0,0 +1,244 @@ +import ForgejoKit +import SwiftUI + +extension PullRequestDetailView { + // MARK: - View Sections + + // swiftlint:disable:next function_body_length + func headerSection(_ pullReq: PullRequest) -> some View { + Section { + VStack(alignment: .leading, spacing: 10) { + Text(pullReq.title) + .font(.title3) + .fontWeight(.bold) + .accessibilityIdentifier("pr-detail-title") + + HStack(spacing: 8) { + HStack(spacing: 4) { + Image(systemName: viewModel.status.icon) + Text(viewModel.status.text) + } + .font(.caption) + .fontWeight(.medium) + .padding(.horizontal, 8) + .padding(.vertical, 4) + .foregroundStyle(.white) + .glassEffect(.regular.tint(viewModel.status.color)) + + Text("#\(pullReq.number)") + .font(.subheadline) + .foregroundStyle(.secondary) + } + + Text("\(pullReq.user.login) opened \(formatRelativeDate(pullReq.createdAt))") + .font(.subheadline) + .foregroundStyle(.secondary) + + HStack(spacing: 4) { + Label(pullReq.head.ref, systemImage: "arrow.branch") + .font(.caption) + .padding(.horizontal, 6) + .padding(.vertical, 2) + .glassEffect(.regular) + + Image(systemName: "arrow.right") + .font(.caption2) + .foregroundStyle(.secondary) + + Text(pullReq.base.ref) + .font(.caption) + .padding(.horizontal, 6) + .padding(.vertical, 2) + .glassEffect(.regular) + + Spacer() + + if !viewModel.isMerged { + mergeableIndicator(pullReq) + } + } + + if !pullReq.labels.isEmpty { + FlowLayout(spacing: 4) { + ForEach(pullReq.labels) { label in + IssueLabelView(label: label) + } + } + } + } + .padding(.vertical, 4) + } + .listRowBackground(viewModel.status.color.opacity(0.08)) + } + + @ViewBuilder + func mergeableIndicator(_ pullReq: PullRequest) -> some View { + if pullReq.mergeable == true { + Image(systemName: "checkmark.circle.fill") + .foregroundStyle(.green) + .font(.caption) + } else if pullReq.mergeable == false { + Image(systemName: "xmark.circle.fill") + .foregroundStyle(.red) + .font(.caption) + } else { + Image(systemName: "questionmark.circle") + .foregroundStyle(.secondary) + .font(.caption) + } + } + + @ViewBuilder + func requestedReviewersSection(_ pullReq: PullRequest) -> some View { + if let reviewers = pullReq.requestedReviewers, !reviewers.isEmpty { + Section("Requested Reviewers") { + ForEach(reviewers) { user in + HStack(spacing: 8) { + Image(systemName: "eye") + .foregroundStyle(.orange) + Text(user.fullName ?? user.login) + .font(.subheadline) + if user.fullName != nil { + Text("@\(user.login)") + .font(.caption) + .foregroundStyle(.secondary) + } + } + } + } + } + } + + @ViewBuilder + var reviewsSection: some View { + if !viewModel.reviews.isEmpty { + Section("Reviews (\(viewModel.reviews.count))") { + ForEach(viewModel.reviews) { review in + ReviewSummaryView( + review: review, + comments: viewModel.reviewComments[review.id] ?? [], + ) + } + } + } + } + + var changesSection: some View { + @Bindable var viewModel = viewModel + return Section { + DisclosureGroup("Changes", isExpanded: $viewModel.isDiffExpanded) { + if viewModel.isLoadingDiff { + HStack { + Spacer() + ProgressView() + Spacer() + } + } else if let parsed = viewModel.parsedDiff { + DiffView( + diff: parsed, + reviewComments: viewModel.allReviewComments, + ) { line, path in + viewModel.inlineCommentContext = InlineCommentContext(line: line, path: path) + } + .listRowInsets(EdgeInsets()) + } + } + .onChange(of: viewModel.isDiffExpanded) { + if viewModel.isDiffExpanded, viewModel.parsedDiff == nil, !viewModel.isLoadingDiff { + Task { await viewModel.loadDiff() } + } + } + } + } + + @ViewBuilder + func conflictSection(_ pullReq: PullRequest) -> some View { + if pullReq.stateValue == .open, pullReq.mergeable == false, !viewModel.isMerged { + Section { + HStack(spacing: 8) { + Image(systemName: "exclamationmark.triangle.fill") + .foregroundStyle(.orange) + Text("This pull request has conflicts that must be resolved via git or the web interface.") + .font(.subheadline) + .foregroundStyle(.secondary) + } + } + } + } + + @ViewBuilder + var commentsSection: some View { + if !viewModel.comments.isEmpty { + Section("Comments (\(viewModel.comments.count))") { + ForEach(viewModel.comments) { comment in + CommentView( + comment: comment, + currentUsername: authService.currentUser?.login, + ) { newBody in + await viewModel.editComment(commentId: comment.id, body: newBody) + } + } + } + } + } + + // swiftlint:disable:next function_body_length + func actionMenu(_ pullReq: PullRequest) -> some View { + ExpandableActionMenu(isExpanded: $showActionsExpanded) { + Button { showCommentSheet = true } label: { + Label("Comment", systemImage: "text.bubble") + } + .buttonStyle(.glassProminent) + .transition(.move(edge: .bottom).combined(with: .opacity)) + .accessibilityIdentifier("pr-comment-button") + + if viewModel.canEditOrClose { + Button { showEditSheet = true } label: { + Label("Edit", systemImage: "pencil") + } + .buttonStyle(.glassProminent) + .transition(.move(edge: .bottom).combined(with: .opacity)) + .accessibilityIdentifier("pr-edit-button") + } + + if pullReq.stateValue == .open { + Button { showSubmitReviewSheet = true } label: { + Label("Review", systemImage: "checkmark.message.fill") + } + .buttonStyle(.glassProminent) + .tint(.blue) + .transition(.move(edge: .bottom).combined(with: .opacity)) + .accessibilityIdentifier("pr-submit-review") + } + + if viewModel.canMerge, pullReq.stateValue == .open, pullReq.draft != true, pullReq.mergeable != false { + Button { showMergeSheet = true } label: { + Label("Merge", systemImage: "arrow.triangle.merge") + } + .buttonStyle(.glassProminent) + .tint(.purple) + .transition(.move(edge: .bottom).combined(with: .opacity)) + .accessibilityIdentifier("pr-merge-button") + } + + if !viewModel.isMerged, viewModel.canEditOrClose { + Button { Task { await viewModel.togglePRState() } } label: { + if viewModel.isTogglingState { + ProgressView() + .controlSize(.small) + } else { + Label( + pullReq.stateValue == .open ? "Close" : "Reopen", + systemImage: pullReq.stateValue == .open ? "xmark.circle" : "arrow.uturn.left.circle", + ) + } + } + .buttonStyle(.glassProminent) + .tint(pullReq.stateValue == .open ? .red : .green) + .disabled(viewModel.isTogglingState) + .transition(.move(edge: .bottom).combined(with: .opacity)) + .accessibilityIdentifier("pr-toggle-state") + } + } + } +} diff --git a/Forji/Forji/Views/PullRequestDetailView.swift b/Forji/Forji/Views/PullRequestDetailView.swift index fa8150f..7884f9d 100644 --- a/Forji/Forji/Views/PullRequestDetailView.swift +++ b/Forji/Forji/Views/PullRequestDetailView.swift @@ -1,95 +1,43 @@ import ForgejoKit - -// swiftlint:disable file_length import SwiftUI -// swiftlint:disable:next type_body_length struct PullRequestDetailView: View { - @State private var repository: Repository let prNumber: Int - @State private var authService: AuthenticationService - @State private var pullRequest: PullRequest? - @State private var comments: [IssueComment] = [] - @State private var isLoading = true - @State private var errorMessage: String? - @State private var showError = false - @State private var showEditSheet = false - @State private var showMergeSheet = false - @State private var isTogglingState = false - @State private var showCommentSheet = false - @State private var diffText: String? - @State private var isDiffExpanded = false - @State private var isLoadingDiff = false + @State var authService: AuthenticationService + @State var viewModel: PullRequestDetailViewModel - // Reviews - @State private var reviews: [PullRequestReview] = [] - @State private var reviewComments: [Int: [ReviewComment]] = [:] - @State private var parsedDiff: ParsedDiff? - @State private var assignees: [User] = [] - @State private var showSubmitReviewSheet = false - @State private var inlineCommentContext: InlineCommentContext? - @State private var showActionsExpanded = false + @State var showEditSheet = false + @State var showMergeSheet = false + @State var showCommentSheet = false + @State var showSubmitReviewSheet = false + @State var showActionsExpanded = false - private let prService: PullRequestService? - private let issueService: IssueService? - private let repositoryService: RepositoryService? + let prService: PullRequestService? init(repository: Repository, prNumber: Int, authService: AuthenticationService) { - self.repository = repository self.prNumber = prNumber self.authService = authService - prService = authService.client.map { PullRequestService(client: $0) } - issueService = authService.client.map { IssueService(client: $0) } - repositoryService = authService.client.map { RepositoryService(client: $0) } - } - - private var owner: String { - repository.owner - } - - private var repo: String { - repository.repoName - } - - private var isMerged: Bool { - pullRequest?.merged == true - } - - private var hasPushPermission: Bool { - repository.permissions?.push == true || repository.permissions?.admin == true - } - - private var isOwnPR: Bool { - guard let currentUser = authService.currentUser?.login, - let prAuthor = pullRequest?.user.login - else { - return false - } - return currentUser == prAuthor - } - - private var canEditOrClose: Bool { - hasPushPermission || isOwnPR - } - - private var canMerge: Bool { - hasPushPermission - } - - private var status: PRStatusStyle { - PRStatusStyle( - state: pullRequest?.stateValue ?? .closed, - merged: pullRequest?.merged, - draft: pullRequest?.draft, - ) + let prService = authService.client.map { PullRequestService(client: $0) } + let repositoryService = authService.client.map { RepositoryService(client: $0) } + let issueService = authService.client.map { IssueService(client: $0) } + self.prService = prService + _viewModel = State(initialValue: PullRequestDetailViewModel( + repository: repository, + prNumber: prNumber, + prService: prService, + repositoryService: repositoryService, + issueService: issueService, + currentUserLogin: authService.currentUser?.login, + )) } var body: some View { + @Bindable var viewModel = viewModel ZStack(alignment: .bottomTrailing) { Group { - if isLoading, pullRequest == nil { + if viewModel.isLoading, viewModel.pullRequest == nil { ProgressView() - } else if let activePR = pullRequest { + } else if let activePR = viewModel.pullRequest { List { headerSection(activePR) @@ -130,492 +78,96 @@ struct PullRequestDetailView: View { } } - if let activePR = pullRequest { + if let activePR = viewModel.pullRequest { actionMenu(activePR) } } .navigationTitle("#\(prNumber)") .navigationBarTitleDisplayMode(.inline) .sheet(isPresented: $showEditSheet) { - if let editingPR = pullRequest { + if let editingPR = viewModel.pullRequest { PullRequestEditView( - repository: repository, + repository: viewModel.repository, pullRequest: editingPR, authService: authService, - onUploadImage: uploadPRImage, + onUploadImage: viewModel.uploadImage, ) { _ in - Task { await loadData() } + Task { await viewModel.loadData() } } } } .sheet(isPresented: $showMergeSheet) { - if let mergingPR = pullRequest { + if let mergingPR = viewModel.pullRequest { PullRequestMergeView( - repository: repository, + repository: viewModel.repository, pullRequest: mergingPR, authService: authService, ) { - Task { await loadData() } + Task { await viewModel.loadData() } } } } .sheet(isPresented: $showSubmitReviewSheet) { PullRequestReviewSheet( - repository: repository, + repository: viewModel.repository, prNumber: prNumber, authService: authService, - isOwnPR: isOwnPR, + isOwnPR: viewModel.isOwnPR, ) { - Task { await loadData() } + Task { await viewModel.loadData() } } } - .sheet(item: $inlineCommentContext) { context in + .sheet(item: $viewModel.inlineCommentContext) { context in if let prService { InlineCommentSheet( context: context, prService: prService, - owner: owner, - repo: repo, + owner: viewModel.owner, + repo: viewModel.repo, prNumber: prNumber, - isOwnPR: isOwnPR, + isOwnPR: viewModel.isOwnPR, ) { - Task { await loadReviews() } + Task { await viewModel.loadReviews() } } } } .sheet(isPresented: $showCommentSheet) { - CommentSheet(users: assignees, onUploadImage: uploadPRImage) { body in - guard let prService else { throw URLError(.userAuthenticationRequired) } - let comment = try await prService.createComment( - owner: owner, - repo: repo, - index: prNumber, - body: body, - ) - comments.append(comment) + CommentSheet(users: viewModel.assignees, onUploadImage: viewModel.uploadImage) { body in + try await viewModel.addComment(body: body) } } .task { - await loadData() + await viewModel.loadData() } - .errorAlert(message: $errorMessage, isPresented: $showError) + .errorAlert(message: $viewModel.errorMessage, isPresented: $viewModel.showError) } - - // MARK: - View Sections - - // swiftlint:disable:next function_body_length - private func headerSection(_ pullReq: PullRequest) -> some View { - Section { - VStack(alignment: .leading, spacing: 10) { - Text(pullReq.title) - .font(.title3) - .fontWeight(.bold) - .accessibilityIdentifier("pr-detail-title") - - HStack(spacing: 8) { - HStack(spacing: 4) { - Image(systemName: status.icon) - Text(status.text) - } - .font(.caption) - .fontWeight(.medium) - .padding(.horizontal, 8) - .padding(.vertical, 4) - .foregroundStyle(.white) - .glassEffect(.regular.tint(status.color)) - - Text("#\(pullReq.number)") - .font(.subheadline) - .foregroundStyle(.secondary) - } - - Text("\(pullReq.user.login) opened \(formatRelativeDate(pullReq.createdAt))") - .font(.subheadline) - .foregroundStyle(.secondary) - - HStack(spacing: 4) { - Label(pullReq.head.ref, systemImage: "arrow.branch") - .font(.caption) - .padding(.horizontal, 6) - .padding(.vertical, 2) - .glassEffect(.regular) - - Image(systemName: "arrow.right") - .font(.caption2) - .foregroundStyle(.secondary) - - Text(pullReq.base.ref) - .font(.caption) - .padding(.horizontal, 6) - .padding(.vertical, 2) - .glassEffect(.regular) - - Spacer() - - if !isMerged { - mergeableIndicator(pullReq) - } - } - - if !pullReq.labels.isEmpty { - FlowLayout(spacing: 4) { - ForEach(pullReq.labels) { label in - IssueLabelView(label: label) - } - } - } - } - .padding(.vertical, 4) - } - .listRowBackground(status.color.opacity(0.08)) - } - - @ViewBuilder - private func mergeableIndicator(_ pullReq: PullRequest) -> some View { - if pullReq.mergeable == true { - Image(systemName: "checkmark.circle.fill") - .foregroundStyle(.green) - .font(.caption) - } else if pullReq.mergeable == false { - Image(systemName: "xmark.circle.fill") - .foregroundStyle(.red) - .font(.caption) - } else { - Image(systemName: "questionmark.circle") - .foregroundStyle(.secondary) - .font(.caption) - } - } - - @ViewBuilder - private func requestedReviewersSection(_ pullReq: PullRequest) -> some View { - if let reviewers = pullReq.requestedReviewers, !reviewers.isEmpty { - Section("Requested Reviewers") { - ForEach(reviewers) { user in - HStack(spacing: 8) { - Image(systemName: "eye") - .foregroundStyle(.orange) - Text(user.fullName ?? user.login) - .font(.subheadline) - if user.fullName != nil { - Text("@\(user.login)") - .font(.caption) - .foregroundStyle(.secondary) - } - } - } - } - } - } - - @ViewBuilder - private var reviewsSection: some View { - if !reviews.isEmpty { - Section("Reviews (\(reviews.count))") { - ForEach(reviews) { review in - ReviewSummaryView( - review: review, - comments: reviewComments[review.id] ?? [], - ) - } - } - } - } - - private var changesSection: some View { - Section { - DisclosureGroup("Changes", isExpanded: $isDiffExpanded) { - if isLoadingDiff { - HStack { - Spacer() - ProgressView() - Spacer() - } - } else if let parsed = parsedDiff { - DiffView( - diff: parsed, - reviewComments: allReviewComments, - ) { line, path in - inlineCommentContext = InlineCommentContext(line: line, path: path) - } - .listRowInsets(EdgeInsets()) - } - } - .onChange(of: isDiffExpanded) { - if isDiffExpanded, diffText == nil, !isLoadingDiff { - Task { await loadDiff() } - } - } - } - } - - @ViewBuilder - private func conflictSection(_ pullReq: PullRequest) -> some View { - if pullReq.stateValue == .open, pullReq.mergeable == false, !isMerged { - Section { - HStack(spacing: 8) { - Image(systemName: "exclamationmark.triangle.fill") - .foregroundStyle(.orange) - Text("This pull request has conflicts that must be resolved via git or the web interface.") - .font(.subheadline) - .foregroundStyle(.secondary) - } - } - } - } - - @ViewBuilder - private var commentsSection: some View { - if !comments.isEmpty { - Section("Comments (\(comments.count))") { - ForEach(comments) { comment in - CommentView( - comment: comment, - currentUsername: authService.currentUser?.login, - ) { newBody in - await editComment(commentId: comment.id, body: newBody) - } - } - } - } - } - - // swiftlint:disable:next function_body_length - private func actionMenu(_ pullReq: PullRequest) -> some View { - ExpandableActionMenu(isExpanded: $showActionsExpanded) { - Button { showCommentSheet = true } label: { - Label("Comment", systemImage: "text.bubble") - } - .buttonStyle(.glassProminent) - .transition(.move(edge: .bottom).combined(with: .opacity)) - .accessibilityIdentifier("pr-comment-button") - - if canEditOrClose { - Button { showEditSheet = true } label: { - Label("Edit", systemImage: "pencil") - } - .buttonStyle(.glassProminent) - .transition(.move(edge: .bottom).combined(with: .opacity)) - .accessibilityIdentifier("pr-edit-button") - } - - if pullReq.stateValue == .open { - Button { showSubmitReviewSheet = true } label: { - Label("Review", systemImage: "checkmark.message.fill") - } - .buttonStyle(.glassProminent) - .tint(.blue) - .transition(.move(edge: .bottom).combined(with: .opacity)) - .accessibilityIdentifier("pr-submit-review") - } - - if canMerge, pullReq.stateValue == .open, pullReq.draft != true, pullReq.mergeable != false { - Button { showMergeSheet = true } label: { - Label("Merge", systemImage: "arrow.triangle.merge") - } - .buttonStyle(.glassProminent) - .tint(.purple) - .transition(.move(edge: .bottom).combined(with: .opacity)) - .accessibilityIdentifier("pr-merge-button") - } - - if !isMerged, canEditOrClose { - Button { Task { await togglePRState() } } label: { - if isTogglingState { - ProgressView() - .controlSize(.small) - } else { - Label( - pullReq.stateValue == .open ? "Close" : "Reopen", - systemImage: pullReq.stateValue == .open ? "xmark.circle" : "arrow.uturn.left.circle", - ) - } - } - .buttonStyle(.glassProminent) - .tint(pullReq.stateValue == .open ? .red : .green) - .disabled(isTogglingState) - .transition(.move(edge: .bottom).combined(with: .opacity)) - .accessibilityIdentifier("pr-toggle-state") - } - } - } - - // MARK: - Data Loading - - private var allReviewComments: [ReviewComment] { - reviewComments.values.flatMap(\.self) - } - - private func uploadPRImage(data: Data, fileName: String, mimeType: String) async throws -> String { - guard let issueService else { throw URLError(.userAuthenticationRequired) } - let attachment = try await issueService.uploadIssueAttachment( - owner: owner, repo: repo, index: prNumber, - fileData: data, fileName: fileName, mimeType: mimeType, - ) - return attachmentMarkdown(for: attachment) - } - - private func loadData() async { - guard let prService, let repositoryService else { return } - isLoading = true - do { - async let fetchedPR = prService.fetchPullRequest(owner: owner, repo: repo, index: prNumber) - async let fetchedComments = prService.fetchComments(owner: owner, repo: repo, index: prNumber) - async let fetchedReviews = prService.fetchReviews(owner: owner, repo: repo, index: prNumber) - async let fetchedAssignees = repositoryService.fetchAssignees(owner: owner, repo: repo) - async let fetchedRepo = repositoryService.fetchRepository(owner: owner, repo: repo) - - let loadedPR = try await fetchedPR - let loadedComments = try await fetchedComments - let loadedReviews = try await fetchedReviews - let loadedAssignees = await (try? fetchedAssignees) ?? [] - - try Task.checkCancellation() - pullRequest = loadedPR - comments = loadedComments - reviews = loadedReviews - assignees = loadedAssignees - if let fullRepo = try? await fetchedRepo { - repository = fullRepo - } - - reviewComments = await fetchAllReviewComments(for: reviews) - - if !allReviewComments.isEmpty { - await loadDiff() - isDiffExpanded = true - } - } catch is CancellationError { - // Ignore cancellation - } catch { - errorMessage = error.localizedDescription - showError = true - } - isLoading = false - } - - private func loadReviews() async { - guard let prService else { return } - do { - reviews = try await prService.fetchReviews(owner: owner, repo: repo, index: prNumber) - reviewComments = await fetchAllReviewComments(for: reviews) - - if !allReviewComments.isEmpty { - if diffText == nil { - await loadDiff() - } - isDiffExpanded = true - } - } catch { - errorMessage = error.localizedDescription - showError = true - } - } - - private func fetchAllReviewComments(for reviews: [PullRequestReview]) async -> [Int: [ReviewComment]] { - guard let prService else { return [:] } - return await withTaskGroup(of: (Int, [ReviewComment])?.self) { group in - for review in reviews { - group.addTask { - guard let comments = try? await prService.fetchReviewComments( - owner: owner, repo: repo, index: prNumber, reviewId: review.id, - ), !comments.isEmpty else { - return nil - } - return (review.id, comments) - } - } - var commentsMap: [Int: [ReviewComment]] = [:] - for await result in group { - if let (reviewId, comments) = result { - commentsMap[reviewId] = comments - } - } - return commentsMap - } - } - - private func loadDiff() async { - guard let prService else { return } - isLoadingDiff = true - do { - let raw = try await prService.fetchDiff(owner: owner, repo: repo, index: prNumber) - try Task.checkCancellation() - diffText = raw - parsedDiff = DiffParser.parse(raw) - } catch is CancellationError { - // Ignore cancellation - } catch { - errorMessage = error.localizedDescription - showError = true - } - isLoadingDiff = false - } - - private func togglePRState() async { - guard let prService, let currentPR = pullRequest else { return } - isTogglingState = true - - do { - let newState = currentPR.stateValue == .open - ? PullRequestState.closed.rawValue : PullRequestState.open.rawValue - let updated = try await prService.editPullRequest( - owner: owner, - repo: repo, - index: prNumber, - title: nil, - body: nil, - state: newState, - ) - pullRequest = updated - NotificationCenter.default.post(name: .pullRequestsDidChange, object: nil) - } catch { - errorMessage = error.localizedDescription - showError = true - } - - isTogglingState = false - } - - private func editComment(commentId: Int, body: String) async -> Bool { - guard let prService else { return false } - do { - let updated = try await prService.editComment( - owner: owner, - repo: repo, - commentId: commentId, - body: body, - ) - if let index = comments.firstIndex(where: { $0.id == commentId }) { - comments[index] = updated - } - return true - } catch { - errorMessage = error.localizedDescription - showError = true - return false - } - } - - #if DEBUG - init(preview _: Void, repository: Repository, prNumber: Int, authService: AuthenticationService, - pullRequest: PullRequest, comments: [IssueComment] = [], reviews: [PullRequestReview] = []) - { - self.repository = repository - self.prNumber = prNumber - self.authService = authService - prService = nil - issueService = nil - repositoryService = nil - _pullRequest = State(initialValue: pullRequest) - _comments = State(initialValue: comments) - _reviews = State(initialValue: reviews) - _isLoading = State(initialValue: false) - } - #endif } #if DEBUG + extension PullRequestDetailView { + init( + preview _: Void, + repository: Repository, + prNumber: Int, + authService: AuthenticationService, + pullRequest: PullRequest, + comments: [IssueComment] = [], + reviews: [PullRequestReview] = [], + ) { + self.prNumber = prNumber + self.authService = authService + prService = nil + _viewModel = State(initialValue: PullRequestDetailViewModel( + preview: (), + repository: repository, + prNumber: prNumber, + pullRequest: pullRequest, + comments: comments, + reviews: reviews, + )) + } + } + #Preview { NavigationStack { PullRequestDetailView( diff --git a/Forji/ForjiTests/PullRequestDetailViewModelTests.swift b/Forji/ForjiTests/PullRequestDetailViewModelTests.swift new file mode 100644 index 0000000..ab09871 --- /dev/null +++ b/Forji/ForjiTests/PullRequestDetailViewModelTests.swift @@ -0,0 +1,227 @@ +import ForgejoKit +@testable import Forji +import Foundation +import Testing + +@MainActor +private final class FakePullRequestDetailService: + PullRequestServiceProtocol, RepositoryServiceProtocol, IssueServiceProtocol +{ + var pullRequestToReturn: PullRequest = .preview + var commentsToReturn: [IssueComment] = [] + var reviewsToReturn: [PullRequestReview] = [] + var reviewCommentsToReturn: [ReviewComment] = [] + var assigneesToReturn: [User] = [] + var repositoryToReturn: Repository = .preview + var diffToReturn = "" + var editedPullRequestToReturn: PullRequest = .preview + var editedCommentToReturn: IssueComment = .preview + var createdCommentToReturn: IssueComment = .preview + var attachmentToReturn: ForgejoKit.Attachment = .previewImage + + var fetchError: Error? + var editError: Error? + var editCommentError: Error? + + private(set) var editedStates: [String] = [] + + func fetchPullRequest(owner _: String, repo _: String, index _: Int) async throws -> PullRequest { + if let fetchError { throw fetchError } + return pullRequestToReturn + } + + func fetchComments(owner _: String, repo _: String, index _: Int) async throws -> [IssueComment] { + commentsToReturn + } + + func fetchReviews(owner _: String, repo _: String, index _: Int) async throws -> [PullRequestReview] { + reviewsToReturn + } + + func fetchReviewComments( + owner _: String, repo _: String, index _: Int, reviewId _: Int, + ) async throws -> [ReviewComment] { + reviewCommentsToReturn + } + + func fetchDiff(owner _: String, repo _: String, index _: Int) async throws -> String { + diffToReturn + } + + func setState(owner _: String, repo _: String, index _: Int, state: String) async throws -> PullRequest { + editedStates.append(state) + if let editError { throw editError } + return editedPullRequestToReturn + } + + func createComment(owner _: String, repo _: String, index _: Int, body _: String) async throws -> IssueComment { + createdCommentToReturn + } + + func editComment( + owner _: String, repo _: String, commentId _: Int, body _: String, + ) async throws -> IssueComment { + if let editCommentError { throw editCommentError } + return editedCommentToReturn + } + + func fetchRepository(owner _: String, repo _: String) async throws -> Repository { + repositoryToReturn + } + + func fetchAssignees(owner _: String, repo _: String) async throws -> [User] { + assigneesToReturn + } + + // swiftlint:disable:next function_parameter_count + func uploadIssueAttachment( + owner _: String, repo _: String, index _: Int, fileData _: Data, fileName _: String, mimeType _: String, + ) async throws -> ForgejoKit.Attachment { + attachmentToReturn + } +} + +private struct FetchFailed: LocalizedError { + var errorDescription: String? { + "fetch failed" + } +} + +@MainActor +struct PullRequestDetailViewModelTests { + private func makeViewModel( + service: FakePullRequestDetailService, + currentUserLogin: String? = "testuser", + repository: Repository = .preview, + ) -> PullRequestDetailViewModel { + PullRequestDetailViewModel( + repository: repository, + prNumber: 4, + prService: service, + repositoryService: service, + issueService: service, + currentUserLogin: currentUserLogin, + ) + } + + @Test("load populates the detail from the services") func loadPopulatesDetail() async { + let service = FakePullRequestDetailService() + service.commentsToReturn = [.preview] + service.assigneesToReturn = [.preview] + service.reviewsToReturn = [.preview] + let viewModel = makeViewModel(service: service) + + await viewModel.loadData() + + #expect(viewModel.pullRequest?.number == 4) + #expect(viewModel.comments.count == 1) + #expect(viewModel.assignees.count == 1) + #expect(viewModel.reviews.count == 1) + #expect(!viewModel.isLoading) + } + + @Test("load failure surfaces the error and leaves no pull request") func loadFailureSurfacesError() async { + let service = FakePullRequestDetailService() + service.fetchError = FetchFailed() + let viewModel = makeViewModel(service: service) + + await viewModel.loadData() + + #expect(viewModel.pullRequest == nil) + #expect(viewModel.showError) + #expect(viewModel.errorMessage == "fetch failed") + } + + @Test("toggling an open pull request requests the closed state") func togglesToClosed() async { + let service = FakePullRequestDetailService() + let viewModel = makeViewModel(service: service) + await viewModel.loadData() + + await viewModel.togglePRState() + + #expect(service.editedStates == [PullRequestState.closed.rawValue]) + #expect(!viewModel.isTogglingState) + } + + @Test("toggle failure surfaces the error") func toggleFailureSurfacesError() async { + let service = FakePullRequestDetailService() + let viewModel = makeViewModel(service: service) + await viewModel.loadData() + service.editError = FetchFailed() + + await viewModel.togglePRState() + + #expect(viewModel.showError) + #expect(viewModel.errorMessage == "fetch failed") + } + + @Test("editing a comment replaces it in place") func editCommentUpdatesInPlace() async { + let service = FakePullRequestDetailService() + service.editedCommentToReturn = IssueComment( + id: 1, body: "edited body", user: .preview, createdAt: Date(), updatedAt: Date(), + ) + let viewModel = makeViewModel(service: service) + viewModel.comments = [.preview] + + let succeeded = await viewModel.editComment(commentId: 1, body: "edited body") + + #expect(succeeded) + #expect(viewModel.comments.first?.body == "edited body") + } + + @Test("edit failure returns false and surfaces the error") func editCommentFailure() async { + let service = FakePullRequestDetailService() + service.editCommentError = FetchFailed() + let viewModel = makeViewModel(service: service) + viewModel.comments = [.preview] + + let succeeded = await viewModel.editComment(commentId: 1, body: "x") + + #expect(!succeeded) + #expect(viewModel.showError) + } + + @Test("adding a comment appends the created comment") func addCommentAppends() async throws { + let service = FakePullRequestDetailService() + service.createdCommentToReturn = IssueComment( + id: 2, body: "a new comment", user: .preview, createdAt: Date(), updatedAt: Date(), + ) + let viewModel = makeViewModel(service: service) + + try await viewModel.addComment(body: "a new comment") + + #expect(viewModel.comments.map(\.id).contains(2)) + } + + @Test("isOwnPR matches the current user against the pull request author") func isOwnPRMatchesAuthor() async { + let service = FakePullRequestDetailService() + let ownViewModel = makeViewModel(service: service, currentUserLogin: "testuser") + await ownViewModel.loadData() + #expect(ownViewModel.isOwnPR) + #expect(ownViewModel.canEditOrClose) + + let otherViewModel = makeViewModel(service: service, currentUserLogin: "someone-else") + await otherViewModel.loadData() + #expect(!otherViewModel.isOwnPR) + } + + @Test("loading the diff parses it") func loadDiffParses() async { + let service = FakePullRequestDetailService() + service.diffToReturn = "diff --git a/README.md b/README.md\n@@ -1 +1,2 @@\n # Repo\n+New line" + let viewModel = makeViewModel(service: service) + + await viewModel.loadDiff() + + #expect(viewModel.parsedDiff != nil) + #expect(!viewModel.isLoadingDiff) + } + + @Test("uploading an image returns attachment markdown") func uploadImageReturnsMarkdown() async throws { + let service = FakePullRequestDetailService() + let viewModel = makeViewModel(service: service) + + let markdown = try await viewModel.uploadImage(data: Data(), fileName: "image.png", mimeType: "image/png") + + #expect(markdown == attachmentMarkdown(for: .previewImage)) + } +}