From 7770bc710610fa28a1c766d68f8f44c9816f9e48 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 21:35:58 +0700 Subject: [PATCH 1/7] fix: Improve branch name sanitization and refresh UI state Sanitize forbidden characters, consecutive dots, and .lock suffixes in branch names. Use structure-aware row updates in file status list to avoid full reloads. Publish refreshed local statuses and poll background sync more frequently. --- macgit/Models/GitBranchNameSanitizer.swift | 24 ++++++++- macgit/Services/SyncState.swift | 20 ++++--- .../FileStatus/FileStatusNativeList.swift | 28 ++++++++-- macgit/Views/FileStatus/FileStatusView.swift | 54 +++++++++---------- .../MainWindow/SidebarView+Loading.swift | 14 ++--- macgit/Views/MainWindow/SidebarView.swift | 7 +-- 6 files changed, 98 insertions(+), 49 deletions(-) diff --git a/macgit/Models/GitBranchNameSanitizer.swift b/macgit/Models/GitBranchNameSanitizer.swift index 2ad4bc0b..57e7dcd9 100644 --- a/macgit/Models/GitBranchNameSanitizer.swift +++ b/macgit/Models/GitBranchNameSanitizer.swift @@ -20,6 +20,28 @@ import Foundation enum GitBranchNameSanitizer { nonisolated static func sanitize(_ input: String) -> String { - input.split(whereSeparator: { $0.isWhitespace }).joined(separator: "-") + let forbidden = CharacterSet(charactersIn: "~^:?*[\\") + .union(.controlCharacters) + var name = input.split(whereSeparator: { $0.isWhitespace }).joined(separator: "-") + name = String(name.unicodeScalars.map { forbidden.contains($0) ? "-" : String($0) }.joined()) + name = name.replacingOccurrences(of: "@{", with: "-") + while name.contains("..") { + name = name.replacingOccurrences(of: "..", with: "-") + } + // Branch names are stricter than refs: an initial dash is forbidden. + while name.hasPrefix("-") || name.hasPrefix("/") { name.removeFirst() } + + // A slash separates ref components; each component has its own rules. + let components = name.split(separator: "/").compactMap { component -> String? in + var value = String(component) + while value.hasPrefix(".") { value.removeFirst() } + while value.hasSuffix(".lock") { value.removeLast(5) } + while value.hasSuffix(".") { value.removeLast() } + return value.isEmpty ? nil : value + } + name = components.joined(separator: "/") + if name.hasPrefix("-") { return sanitize(name) } + if name == "@" || name == "HEAD" { name += "-branch" } + return name } } diff --git a/macgit/Services/SyncState.swift b/macgit/Services/SyncState.swift index eaa3f86a..7c18a3f8 100644 --- a/macgit/Services/SyncState.swift +++ b/macgit/Services/SyncState.swift @@ -123,10 +123,11 @@ class SyncState: ObservableObject { private let refreshCoordinator = SyncRefreshCoordinator() private var refreshGeneration = 0 private var refreshedCurrentBranches: [URL: String] = [:] - private var refreshedLocalStatuses: [URL: GitStatus] = [:] + @Published private(set) var refreshedLocalStatuses: [URL: GitStatus] = [:] private static let backgroundRefreshInterval: Duration = .seconds(10) + private static let backgroundPollingInterval: Duration = .seconds(5) private static let localRefreshCoalescingInterval: TimeInterval = 2 - private static let autoFetchInterval: TimeInterval = 60 + private static let autoFetchInterval: TimeInterval = 15 init() { networkMonitor.start(queue: networkMonitorQueue) @@ -193,8 +194,10 @@ class SyncState: ObservableObject { let previousBranch = refreshedCurrentBranches.updateValue(currentBranch, forKey: repositoryURL) let localStatusDidChange: Bool if let localStatus { - let previousStatus = refreshedLocalStatuses.updateValue(localStatus, forKey: repositoryURL) - localStatusDidChange = previousStatus != localStatus + localStatusDidChange = refreshedLocalStatuses[repositoryURL] != localStatus + if localStatusDidChange { + refreshedLocalStatuses[repositoryURL] = localStatus + } } else { localStatusDidChange = false } @@ -240,8 +243,13 @@ class SyncState: ObservableObject { stopBackgroundSync() let autoFetchEnabled = settings.resolvedAutoFetchEnabled(globalValue: globalAutoFetchEnabled) backgroundTask = Task { + let clock = ContinuousClock() + var nextLocalRefresh = clock.now while !Task.isCancelled { - await refresh(repositoryURL: repositoryURL, force: false) + if clock.now >= nextLocalRefresh { + await refresh(repositoryURL: repositoryURL, force: false) + nextLocalRefresh = clock.now.advanced(by: Self.backgroundRefreshInterval) + } if autoFetchEnabled, networkMonitor.currentPath.status == .satisfied, await performAutomaticFetch( @@ -258,7 +266,7 @@ class SyncState: ObservableObject { ) } } - try? await Task.sleep(for: Self.backgroundRefreshInterval) + try? await Task.sleep(for: Self.backgroundPollingInterval) } } } diff --git a/macgit/Views/FileStatus/FileStatusNativeList.swift b/macgit/Views/FileStatus/FileStatusNativeList.swift index 9529829d..3068a21d 100644 --- a/macgit/Views/FileStatus/FileStatusNativeList.swift +++ b/macgit/Views/FileStatus/FileStatusNativeList.swift @@ -101,16 +101,38 @@ struct FileStatusNativeList: NSViewRepresentable { let oldRows = rows let structureChanged = oldRows.map(\.file.id) != model.rows.map(\.file.id) rows = model.rows - if structureChanged || scaleChanged { + if scaleChanged { let anchor = viewportAnchor(in: oldRows) tableView.reloadData() restoreViewport(anchor) return } + if structureChanged { + let anchor = viewportAnchor(in: oldRows) + let diff = rows.map(\.file.id).difference(from: oldRows.map(\.file.id)) + var removed = IndexSet(), inserted = IndexSet() + for change in diff { + switch change { + case .remove(let index, _, _): removed.insert(index) + case .insert(let index, _, _): inserted.insert(index) + } + } + NSAnimationContext.runAnimationGroup { context in + context.duration = 0 + tableView.beginUpdates() + tableView.removeRows(at: removed, withAnimation: []) + tableView.insertRows(at: inserted, withAnimation: []) + tableView.endUpdates() + } + restoreViewport(anchor) + } + let previousRows = Dictionary(uniqueKeysWithValues: oldRows.map { ($0.file.id, $0) }) var changed = IndexSet() - for index in rows.indices where rows[index] != oldRows[index] { - changed.insert(index) + for index in rows.indices { + if let previous = previousRows[rows[index].file.id], previous != rows[index] { + changed.insert(index) + } } if !changed.isEmpty { tableView.reloadData(forRowIndexes: changed, columnIndexes: IndexSet(integer: 0)) diff --git a/macgit/Views/FileStatus/FileStatusView.swift b/macgit/Views/FileStatus/FileStatusView.swift index 2da14829..4fb27564 100644 --- a/macgit/Views/FileStatus/FileStatusView.swift +++ b/macgit/Views/FileStatus/FileStatusView.swift @@ -53,6 +53,7 @@ struct FileStatusView: View { @ObservedObject private var integrationSettings = IntegrationSettingsStore.shared @State private var gitStatus: GitStatus = GitStatus(staged: [], unstaged: [], untracked: []) + @State private var statusLoadGeneration = 0 @State private var stagedLineCounts: [String: FileLineChangeCount] = [:] @State private var changedLineCounts: [String: FileLineChangeCount] = [:] @State private var stagedLineCountRefreshID = UUID() @@ -368,11 +369,8 @@ struct FileStatusView: View { requestRefresh() } } - .onReceive(NotificationCenter.default.publisher(for: .repositoryLocalStateDidRefresh)) { notification in - guard let url = notification.userInfo?["repositoryURL"] as? URL, - url == repositoryURL, - let status = notification.userInfo?["gitStatus"] as? GitStatus - else { return } + .onReceive(syncState.$refreshedLocalStatuses) { snapshots in + guard let status = snapshots[repositoryURL] else { return } applyBackgroundChangedFiles(from: status) } .onReceive(NotificationCenter.default.publisher(for: .repositoryRemoteRefsDidRefresh)) { notification in @@ -1504,6 +1502,8 @@ struct FileStatusView: View { } private func loadStatus() async { + statusLoadGeneration += 1 + let generation = statusLoadGeneration let shouldShowLoading = !hasChanges && !hasInProgressOperation if shouldShowLoading { isLoading = true @@ -1529,12 +1529,17 @@ struct FileStatusView: View { } let paths = Set((loadedStatus.staged + loadedStatus.unstaged + loadedStatus.untracked).map(\.path)) - lfsPaths = (try? await GitStatusService.shared.lfsPaths(Array(paths), in: repositoryURL)) ?? [] - stagedLFSPaths = (try? await GitStatusService.shared.lfsPaths(loadedStatus.staged.map(\.path), cached: true, in: repositoryURL)) ?? [] + let loadedLFSPaths = (try? await GitStatusService.shared.lfsPaths(Array(paths), in: repositoryURL)) ?? [] + let loadedStagedLFSPaths = (try? await GitStatusService.shared.lfsPaths(loadedStatus.staged.map(\.path), cached: true, in: repositoryURL)) ?? [] + let loadedRecentCommits = await GitStatusService.shared.recentCommits(in: repositoryURL) + guard generation == statusLoadGeneration else { return } + if lfsPaths != loadedLFSPaths { lfsPaths = loadedLFSPaths } + if stagedLFSPaths != loadedStagedLFSPaths { stagedLFSPaths = loadedStagedLFSPaths } if gitStatus != loadedStatus { gitStatus = loadedStatus - changedFiles = loadedStatus.unstaged + loadedStatus.untracked } + let loadedChangedFiles = loadedStatus.unstaged + loadedStatus.untracked + if changedFiles != loadedChangedFiles { changedFiles = loadedChangedFiles } // The same paths/statuses can have different content after an edit. // Refresh counts even when the file-list snapshot is unchanged, and // keep the previous badges visible until their replacement arrives. @@ -1542,7 +1547,7 @@ struct FileStatusView: View { changedLineCountRefreshID = UUID() currentBranch = loadedCurrentBranch currentBranchIntegrationStatus = loadedIntegrationStatus - recentCommits = await GitStatusService.shared.recentCommits(in: repositoryURL) + recentCommits = loadedRecentCommits diffRefreshID = UUID() restoreSelectedFileAfterStatusRefresh() @@ -1556,6 +1561,7 @@ struct FileStatusView: View { } onCustomActionSelectionChanged(actionSelection.selectedFiles.map(\.path)) } catch { + guard generation == statusLoadGeneration else { return } errorMessage = error.localizedDescription showingError = true } @@ -1605,10 +1611,16 @@ struct FileStatusView: View { private func applyBackgroundChangedFiles(from status: GitStatus) { let loadedChangedFiles = status.unstaged + status.untracked - guard changedFiles != loadedChangedFiles else { return } - - changedFiles = loadedChangedFiles - changedLineCountRefreshID = UUID() + guard gitStatus != status || changedFiles != loadedChangedFiles else { return } + statusLoadGeneration += 1 + let stagedChanged = gitStatus.staged != status.staged + if gitStatus != status { gitStatus = status } + if stagedChanged { stagedLineCountRefreshID = UUID() } + if changedFiles != loadedChangedFiles { + changedFiles = loadedChangedFiles + changedLineCountRefreshID = UUID() + } + restoreSelectedFileAfterStatusRefresh() if !selectedActionFileKeys.isEmpty { selectedActionFileKeys = actionSelection.prunedSelection @@ -1618,22 +1630,6 @@ struct FileStatusView: View { self.actionSelectionAnchorKey = selectedActionFileKeys.first } - guard let selectedFileKey, !selectedFileKey.isStaged else { return } - if let matchedFile = loadedChangedFiles.first(where: { - FileStatusSelectionKey(file: $0, isStaged: false) == selectedFileKey - }) { - selectedFile = matchedFile - } else if let firstChangedFile = loadedChangedFiles.first { - selectFileForPreview( - firstChangedFile, - selectionKey: FileStatusSelectionKey(file: firstChangedFile, isStaged: false) - ) - } else { - selectedFile = nil - self.selectedFileKey = nil - diffHunks = [] - isLoadingDiff = false - } } private func reloadRepositoryState() async { diff --git a/macgit/Views/MainWindow/SidebarView+Loading.swift b/macgit/Views/MainWindow/SidebarView+Loading.swift index 3f4231c0..99d37092 100644 --- a/macgit/Views/MainWindow/SidebarView+Loading.swift +++ b/macgit/Views/MainWindow/SidebarView+Loading.swift @@ -197,7 +197,6 @@ extension SidebarView { let expandedFoldersForLoad = hadLoadedBranches ? expandedFolders.intersection(allFolders) : currentBranchFolders - let branchStructureChanged = branchNodes != tree || currentBranch != current await MainActor.run { guard activeBranchSyncLoadID == loadID else { return } @@ -205,14 +204,15 @@ extension SidebarView { if currentBranch != current { currentBranch = current } if !headHash.isEmpty { headHash = "" } let existingBranches = Set(filteredLocals) - branchSyncStatus = branchSyncStatus.filter { existingBranches.contains($0.key) } + let retainedStatuses = branchSyncStatus.filter { existingBranches.contains($0.key) } + if branchSyncStatus != retainedStatuses { branchSyncStatus = retainedStatuses } if currentBranchIntegrationStatus?.branch != current { currentBranchIntegrationStatus = nil } - if branchStructureChanged { - loadedBranchSyncBranches = [] - syncingBranchSyncBranches = [] - } + // Remote tips can change without changing the local branch tree. + // Every reload starts a new generation of ahead/behind reads. + loadedBranchSyncBranches = [] + syncingBranchSyncBranches = [] // Reveal the current branch only on the initial load. Refreshes // preserve the user's tree state while dropping removed folders. if expandedFolders != expandedFoldersForLoad { expandedFolders = expandedFoldersForLoad } @@ -285,7 +285,7 @@ extension SidebarView { updatedStatuses.removeValue(forKey: branch) } } - branchSyncStatus = updatedStatuses + if branchSyncStatus != updatedStatuses { branchSyncStatus = updatedStatuses } loadedBranchSyncBranches.formUnion(branches) syncingBranchSyncBranches.subtract(branches) } diff --git a/macgit/Views/MainWindow/SidebarView.swift b/macgit/Views/MainWindow/SidebarView.swift index 0399a6e2..bbb12166 100644 --- a/macgit/Views/MainWindow/SidebarView.swift +++ b/macgit/Views/MainWindow/SidebarView.swift @@ -732,10 +732,11 @@ struct SidebarView: View { } .onReceive(NotificationCenter.default.publisher(for: .repositoryRemoteRefsDidRefresh)) { notification in if let url = notification.userInfo?["repositoryURL"] as? URL, - url == repositoryURL, - notification.userInfo?["userInitiated"] as? Bool == true { + url == repositoryURL { Task { - await loadRemotes() + if notification.userInfo?["userInitiated"] as? Bool == true { + await loadRemotes() + } await loadBranches(force: true) } } From 9e80fc74e1c05a38a2948fff3b12ec31704308c1 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:05:38 +0700 Subject: [PATCH 2/7] feat: Open pull request details in a dedicated window Add PullRequestDetailWindowController with window reuse and centering, and move detail actions into the header with Open PR and Merge buttons. --- .../PullRequestDetailWindowController.swift | 61 +++ .../PullRequests/PullRequestChangesView.swift | 12 +- .../PullRequestDetailHeader.swift | 45 ++- .../PullRequests/PullRequestListView.swift | 364 +++++++++--------- 4 files changed, 291 insertions(+), 191 deletions(-) create mode 100644 macgit/App/PullRequestDetailWindowController.swift diff --git a/macgit/App/PullRequestDetailWindowController.swift b/macgit/App/PullRequestDetailWindowController.swift new file mode 100644 index 00000000..c803f7ec --- /dev/null +++ b/macgit/App/PullRequestDetailWindowController.swift @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +import AppKit +import SwiftUI + +@MainActor +final class PullRequestDetailWindowController: NSWindowController, NSWindowDelegate { + private var onClose: (() -> Void)? + + init() { + super.init(window: nil) + } + + required init?(coder: NSCoder) { + fatalError("init(coder:) has not been implemented") + } + + func activateIfPresented() -> Bool { + guard let window else { return false } + window.makeKeyAndOrderFront(nil) + return true + } + + func show(title: String, content: Content, onClose: @escaping () -> Void) { + if activateIfPresented() { return } + self.onClose = onClose + let screen = NSApp.keyWindow?.screen ?? NSScreen.main + let visibleFrame = screen?.visibleFrame ?? NSRect(x: 0, y: 0, width: 1200, height: 800) + let size = NSSize(width: min(1200, visibleFrame.width - 40), + height: min(800, visibleFrame.height - 80)) + let window = NSWindow(contentRect: NSRect(origin: .zero, size: size), + styleMask: [.titled, .closable, .resizable], backing: .buffered, defer: false) + window.title = title + window.isReleasedWhenClosed = false + window.tabbingMode = .disallowed + window.collectionBehavior = [.fullScreenNone] + window.standardWindowButton(.miniaturizeButton)?.isHidden = true + window.standardWindowButton(.zoomButton)?.isHidden = true + window.delegate = self + let hostingView = NSHostingView(rootView: GeometryReader { geometry in + content.frame(width: geometry.size.width, height: geometry.size.height) + }) + // Window geometry owns the viewport; diff content must not expand the window. + hostingView.sizingOptions = [] + window.contentView = hostingView + window.contentMinSize = NSSize(width: min(960, size.width), height: min(560, size.height)) + window.setContentSize(size) + window.setFrameOrigin(NSPoint(x: visibleFrame.midX - window.frame.width / 2, + y: visibleFrame.midY - window.frame.height / 2)) + self.window = window + showWindow(nil) + window.makeKeyAndOrderFront(nil) + } + + func windowWillClose(_ notification: Notification) { + let callback = onClose + onClose = nil + window = nil + callback?() + } +} diff --git a/macgit/Views/PullRequests/PullRequestChangesView.swift b/macgit/Views/PullRequests/PullRequestChangesView.swift index 4adbfe36..4d7a4d9e 100644 --- a/macgit/Views/PullRequests/PullRequestChangesView.swift +++ b/macgit/Views/PullRequests/PullRequestChangesView.swift @@ -118,6 +118,7 @@ struct PullRequestChangesView: View { HStack(spacing: 6) { Image(systemName: isExpanded ? "chevron.down" : "chevron.right") Image(systemName: "folder") + .foregroundStyle(.blue) Text(name) .lineLimit(1) } @@ -162,9 +163,14 @@ struct PullRequestChangesView: View { if let additions = file.additions, let deletions = file.deletions { - Text("+\(additions) −\(deletions)") - .font(.system(size: 10, design: .monospaced).scaled(by: textScale)) - .foregroundStyle(.secondary) + HStack(spacing: 4) { + Text("+\(additions)") + .foregroundStyle(.green) + Text("−\(deletions)") + .foregroundStyle(.red) + } + .font(.system(size: 10, design: .monospaced).scaled(by: textScale)) + .fixedSize() } } .padding(.vertical, 2) diff --git a/macgit/Views/PullRequests/PullRequestDetailHeader.swift b/macgit/Views/PullRequests/PullRequestDetailHeader.swift index 132d7e02..89c87e23 100644 --- a/macgit/Views/PullRequests/PullRequestDetailHeader.swift +++ b/macgit/Views/PullRequests/PullRequestDetailHeader.swift @@ -21,7 +21,10 @@ import SwiftUI struct PullRequestDetailHeader: View { @Environment(\.appTextScale) private var textScale let summary: PullRequestSummary - let onClose: () -> Void + let onOpenPullRequest: () -> Void + let onMerge: () -> Void + let isMergeDisabled: Bool + let mergeButtonHelp: String var body: some View { VStack(alignment: .leading, spacing: 12) { @@ -34,10 +37,7 @@ struct PullRequestDetailHeader: View { .font(.title2.scaled(by: textScale)) .foregroundStyle(.secondary) Spacer(minLength: 12) - Button("Close detail", systemImage: "xmark", action: onClose) - .buttonStyle(.borderless) - .labelStyle(.iconOnly) - .help("Close pull request detail") + } HStack(spacing: 8) { @@ -60,17 +60,32 @@ struct PullRequestDetailHeader: View { } HStack(spacing: 7) { - Text(summary.source.ref) - .branchBadge(scale: textScale) - Image(systemName: "arrow.right") - .font(.caption) - .foregroundStyle(.secondary) - .accessibilityLabel("into") - Text(summary.target.ref) - .branchBadge(scale: textScale) + HStack(spacing: 7) { + Text(summary.source.ref) + .branchBadge(scale: textScale) + Image(systemName: "arrow.right") + .font(.caption) + .foregroundStyle(.secondary) + .accessibilityLabel("into") + Text(summary.target.ref) + .branchBadge(scale: textScale) + } + .accessibilityElement(children: .combine) + .accessibilityLabel("From \(summary.source.ref) into \(summary.target.ref)") + + Spacer(minLength: 12) + + HStack(spacing: 8) { + Button("Open PR", systemImage: "safari", action: onOpenPullRequest) + Button("Merge Pull Request", systemImage: "arrow.triangle.merge", action: onMerge) + .buttonStyle(.borderedProminent) + .tint(.green) + .disabled(isMergeDisabled) + .help(mergeButtonHelp) + } + .font(.body.scaled(by: textScale)) + .fixedSize() } - .accessibilityElement(children: .combine) - .accessibilityLabel("From \(summary.source.ref) into \(summary.target.ref)") } .frame(maxWidth: .infinity, alignment: .leading) .padding(.horizontal, 20) diff --git a/macgit/Views/PullRequests/PullRequestListView.swift b/macgit/Views/PullRequests/PullRequestListView.swift index 75d35dae..eda68aac 100644 --- a/macgit/Views/PullRequests/PullRequestListView.swift +++ b/macgit/Views/PullRequests/PullRequestListView.swift @@ -29,6 +29,7 @@ struct PullRequestListView: View { var authorizeAction: () async -> Bool = { true } @State private var pendingCommentPullRequest: PullRequestSummary? @State private var selectedPullRequestID: Int? + @State private var detailWindowController = PullRequestDetailWindowController() var body: some View { VStack(spacing: 0) { @@ -74,23 +75,6 @@ struct PullRequestListView: View { .font(.body.scaled(by: textScale)) .foregroundStyle(.secondary) .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if selectedPullRequestID != nil { - PersistentHSplit( - autosaveName: "PullRequestMainSplit", - left: { - pullRequestListPanel - }, - right: { - detailPanel - .frame( - minWidth: 680, - idealWidth: 820, - maxWidth: .infinity, - maxHeight: .infinity, - alignment: .top - ) - } - ) } else { pullRequestListPanel } @@ -118,7 +102,10 @@ struct PullRequestListView: View { } message: { Text(controller.detailErrorMessage ?? "Could not load pull request details.") } - .onDisappear { controller.releaseVisibleData() } + .onDisappear { + closeDetail() + controller.releaseVisibleData() + } .onReceive(NotificationCenter.default.publisher(for: .advancedClearSessionCaches)) { _ in closeDetail() Task { @@ -167,10 +154,23 @@ struct PullRequestListView: View { ) .contentShape(Rectangle()) .onTapGesture { - selectedPullRequestID = item.id - controller.clearSelectedDetail() Task { guard await authorizeAction() else { return } + if detailWindowController.activateIfPresented() { return } + controller.clearSelectedDetail() + selectedPullRequestID = item.id + detailWindowController.show( + title: "Pull Request #\(item.number) — \(repositoryURL.lastPathComponent)", + content: PullRequestDetailWindowContent( + controller: controller, + selectedPullRequestID: item.id, + authorizeAction: authorizeAction, + onClose: closeDetail + ) + .environment(\.appTextScale, textScale) + .sheetPresentationScope(), + onClose: closeDetail + ) await controller.loadPullRequestDetail(item) } } @@ -183,76 +183,15 @@ struct PullRequestListView: View { .frame(minWidth: 280, idealWidth: 360, maxWidth: .infinity) } - @ViewBuilder - private var detailPanel: some View { - if controller.isLoadingDetail, - controller.selectedDetail?.id != selectedPullRequestID { - ProgressView("Loading pull request…") - .font(.body.scaled(by: textScale)) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if let detail = controller.selectedDetail, - detail.id == selectedPullRequestID { - PullRequestDetailPane( - detail: detail, - onClose: closeDetail, - onOpenPullRequest: { - Task { - guard await authorizeAction() else { return } - controller.openInBrowser(detail.summary) - } - }, - isRefreshingDetail: controller.isLoadingDetail, - onRefreshDetail: { - Task { - guard await authorizeAction() else { return } - await controller.loadPullRequestDetail(detail.summary, forceRefresh: true) - } - }, - isSubmittingComment: controller.isPerformingAction, - onSubmitComment: { body in - Task { - guard await authorizeAction() else { return } - await controller.comment(on: detail.summary, body: body) - } - }, - changes: controller.selectedChanges, - isLoadingChanges: controller.isLoadingChanges, - changesErrorMessage: controller.changesErrorMessage, - onLoadChanges: { - Task { - guard await authorizeAction() else { return } - await controller.loadPullRequestChanges(detail.summary) - } - }, - onRefreshChanges: { - Task { - guard await authorizeAction() else { return } - await controller.loadPullRequestChanges(detail.summary, forceRefresh: true) - } - }, - isMerging: controller.isPerformingAction, - onMerge: { - Task { - guard await authorizeAction() else { return } - await controller.merge(detail.summary) - } - } - ) - } else { - ProgressView("Loading pull request…") - .font(.body.scaled(by: textScale)) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - } - private func closeDetail() { + detailWindowController.close() selectedPullRequestID = nil controller.clearSelectedDetail() } private var detailErrorPresented: Binding { Binding( - get: { controller.detailErrorMessage != nil }, + get: { selectedPullRequestID == nil && controller.detailErrorMessage != nil }, set: { isPresented in if !isPresented { controller.detailErrorMessage = nil @@ -348,6 +287,101 @@ struct PullRequestListView: View { } } +private struct PullRequestDetailWindowContent: View { + @Environment(\.appTextScale) private var textScale + @ObservedObject var controller: PullRequestController + let selectedPullRequestID: Int + let authorizeAction: () async -> Bool + let onClose: () -> Void + + var body: some View { + detailPanel + .alert("Pull Request", isPresented: detailErrorPresented) { + Button("OK", role: .cancel) { + controller.detailErrorMessage = nil + if controller.selectedDetail == nil { onClose() } + } + } message: { + Text(controller.detailErrorMessage ?? "Could not load pull request details.") + } + } + + @ViewBuilder + private var detailPanel: some View { + if controller.isLoadingDetail, + controller.selectedDetail?.id != selectedPullRequestID { + ProgressView("Loading pull request…") + .font(.body.scaled(by: textScale)) + .frame(maxWidth: .infinity, maxHeight: .infinity) + } else if let detail = controller.selectedDetail, + detail.id == selectedPullRequestID { + PullRequestDetailPane( + detail: detail, + onOpenPullRequest: { + Task { + guard await authorizeAction() else { return } + controller.openInBrowser(detail.summary) + } + }, + isRefreshingDetail: controller.isLoadingDetail, + onRefreshDetail: { + Task { + guard await authorizeAction() else { return } + await controller.loadPullRequestDetail(detail.summary, forceRefresh: true) + } + }, + isSubmittingComment: controller.isPerformingAction, + onSubmitComment: { body in + Task { + guard await authorizeAction() else { return } + await controller.comment(on: detail.summary, body: body) + } + }, + changes: controller.selectedChanges, + isLoadingChanges: controller.isLoadingChanges, + changesErrorMessage: controller.changesErrorMessage, + onLoadChanges: { + Task { + guard await authorizeAction() else { return } + await controller.loadPullRequestChanges(detail.summary) + } + }, + onRefreshChanges: { + Task { + guard await authorizeAction() else { return } + await controller.loadPullRequestChanges(detail.summary, forceRefresh: true) + } + }, + isMerging: controller.isPerformingAction, + onMerge: { + Task { + guard await authorizeAction() else { return } + await controller.merge(detail.summary) + } + } + ) + } else if controller.detailErrorMessage != nil { + ContentUnavailableView( + "Could not load pull request", + systemImage: "exclamationmark.triangle", + description: Text("Close this window and select the pull request to try again.") + ) + .frame(maxWidth: .infinity, maxHeight: .infinity) + } else { + ProgressView("Loading pull request…") + .font(.body.scaled(by: textScale)) + .frame(maxWidth: .infinity, maxHeight: .infinity) + } + } + + private var detailErrorPresented: Binding { + Binding( + get: { controller.detailErrorMessage != nil }, + set: { if !$0 { controller.detailErrorMessage = nil } } + ) + } +} + private struct PullRequestRow: View { @Environment(\.appTextScale) private var textScale let summary: PullRequestSummary @@ -502,7 +536,6 @@ private struct PullRequestRow: View { private struct PullRequestDetailPane: View { @Environment(\.appTextScale) private var textScale let detail: PullRequestDetail - let onClose: () -> Void let onOpenPullRequest: () -> Void let isRefreshingDetail: Bool let onRefreshDetail: () -> Void @@ -523,7 +556,13 @@ private struct PullRequestDetailPane: View { var body: some View { VStack(spacing: 0) { - PullRequestDetailHeader(summary: detail.summary, onClose: onClose) + PullRequestDetailHeader( + summary: detail.summary, + onOpenPullRequest: onOpenPullRequest, + onMerge: { isConfirmingMerge = true }, + isMergeDisabled: !canMerge || isMerging, + mergeButtonHelp: mergeButtonHelp + ) PullRequestDetailTabBar(selection: $selectedTab) if selectedTab == .overview { @@ -537,7 +576,6 @@ private struct PullRequestDetailPane: View { ) } - footer } .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .top) .onChange(of: isCommentBarExpanded) { _, isExpanded in @@ -569,104 +607,84 @@ private struct PullRequestDetailPane: View { } private var overviewContent: some View { - HStack(spacing: 0) { - VStack(spacing: 0) { - ScrollView { - VStack(alignment: .leading, spacing: 14) { - PullRequestConversationBlock( - author: detail.summary.author, - date: detail.summary.createdAt, - action: "opened this pull request" - ) { - if detail.body.isEmpty { - Text("No description provided.") - .font(.body.scaled(by: textScale)) - .italic() - .foregroundStyle(.secondary) - } else { - Markdown(detail.body) - .markdownTheme(.gitHub) - .markdownTextStyle { FontSize(.em(textScale)) } - .textSelection(.enabled) - } - } - - ForEach(detail.comments) { comment in + PersistentHSplit( + autosaveName: "PullRequestOverviewSplit", + minimumLeftWidth: 440, + minimumRightWidth: 180, + left: { + VStack(spacing: 0) { + ScrollView { + VStack(alignment: .leading, spacing: 14) { PullRequestConversationBlock( - author: comment.author, - date: comment.createdAt, - action: "commented" + author: detail.summary.author, + date: detail.summary.createdAt, + action: "opened this pull request" ) { - Markdown(comment.body) - .markdownTheme(.gitHub) - .markdownTextStyle { FontSize(.em(textScale)) } - .textSelection(.enabled) + if detail.body.isEmpty { + Text("No description provided.") + .font(.body.scaled(by: textScale)) + .italic() + .foregroundStyle(.secondary) + } else { + Markdown(detail.body) + .markdownTheme(.gitHub) + .markdownTextStyle { FontSize(.em(textScale)) } + .textSelection(.enabled) + } } - } - if detail.comments.isEmpty { - Text("No comments yet") - .font(.subheadline.scaled(by: textScale)) - .foregroundStyle(.secondary) - .frame(maxWidth: .infinity, alignment: .center) - .padding(.vertical, 8) - } + ForEach(detail.comments) { comment in + PullRequestConversationBlock( + author: comment.author, + date: comment.createdAt, + action: "commented" + ) { + Markdown(comment.body) + .markdownTheme(.gitHub) + .markdownTextStyle { FontSize(.em(textScale)) } + .textSelection(.enabled) + } + } + + if detail.comments.isEmpty { + Text("No comments yet") + .font(.subheadline.scaled(by: textScale)) + .foregroundStyle(.secondary) + .frame(maxWidth: .infinity, alignment: .center) + .padding(.vertical, 8) + } - Button(action: onRefreshDetail) { - if isRefreshingDetail { - ProgressView() - .controlSize(.small) - } else { - Label("Refresh comments", systemImage: "arrow.clockwise") + Button(action: onRefreshDetail) { + if isRefreshingDetail { + ProgressView() + .controlSize(.small) + } else { + Label("Refresh comments", systemImage: "arrow.clockwise") + } } + .buttonStyle(.borderless) + .font(.body.scaled(by: textScale)) + .foregroundStyle(.secondary) + .disabled(isRefreshingDetail) + .frame(maxWidth: .infinity, alignment: .center) } - .buttonStyle(.borderless) - .font(.body.scaled(by: textScale)) - .foregroundStyle(.secondary) - .disabled(isRefreshingDetail) - .frame(maxWidth: .infinity, alignment: .center) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(20) } - .frame(maxWidth: .infinity, alignment: .leading) - .padding(20) - } - - commentComposer - } - .frame(minWidth: 440, maxWidth: .infinity, maxHeight: .infinity) - Divider() - - PullRequestMetadataSidebar( - reviewers: detail.reviewers, - assignees: detail.assignees - ) - .frame(width: 220) - .frame(maxHeight: .infinity) - } - } + commentComposer + } + .frame(minWidth: 440, maxWidth: .infinity, maxHeight: .infinity) - private var footer: some View { - HStack { - Button("Open PR", systemImage: "safari", action: onOpenPullRequest) - Button("Merge Pull Request", systemImage: "arrow.triangle.merge") { - isConfirmingMerge = true + }, + right: { + PullRequestMetadataSidebar( + reviewers: detail.reviewers, + assignees: detail.assignees + ) + .frame(minWidth: 180, idealWidth: 220, maxWidth: .infinity, maxHeight: .infinity) } - .buttonStyle(.borderedProminent) - .tint(.green) - .disabled(!canMerge || isMerging) - .help(mergeButtonHelp) - Spacer() - Button("Close", action: onClose) - .keyboardShortcut(.cancelAction) - } - .font(.body.scaled(by: textScale)) - .padding(16) - .background(Color(nsColor: .controlBackgroundColor)) - .overlay(alignment: .top) { - Rectangle() - .fill(.separator) - .frame(height: 0.5) - } + ) } private var canMerge: Bool { From 9d46358eeb89be58b6132164ac8256bc6197d158 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:11:36 +0700 Subject: [PATCH 3/7] fix: Refresh branch badges after local repository state changes --- macgit/Views/MainWindow/SidebarView.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/macgit/Views/MainWindow/SidebarView.swift b/macgit/Views/MainWindow/SidebarView.swift index bbb12166..4f8a5417 100644 --- a/macgit/Views/MainWindow/SidebarView.swift +++ b/macgit/Views/MainWindow/SidebarView.swift @@ -730,6 +730,16 @@ struct SidebarView: View { } } } + .onReceive(NotificationCenter.default.publisher(for: .repositoryLocalStateDidRefresh)) { notification in + guard let url = notification.userInfo?["repositoryURL"] as? URL, + url == repositoryURL else { return } + Task { + // Local commits and tracking changes must refresh branch badges + // even when the subsequent automatic fetch cannot reach a remote. + await GitStatusService.shared.invalidateBranchListCache(in: repositoryURL) + await loadBranches(force: true) + } + } .onReceive(NotificationCenter.default.publisher(for: .repositoryRemoteRefsDidRefresh)) { notification in if let url = notification.userInfo?["repositoryURL"] as? URL, url == repositoryURL { From f9e7003a02fe973b57676273f47ebb448d06b033 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:36:51 +0700 Subject: [PATCH 4/7] perf: Avoid redundant pull request list refreshes Cache entry expiry now drives detail refresh decisions. Retain loaded list state on fetch errors. --- macgit/App/PullRequestController.swift | 52 ++++++++++++++++++-------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/macgit/App/PullRequestController.swift b/macgit/App/PullRequestController.swift index c5b77e99..c83538ec 100644 --- a/macgit/App/PullRequestController.swift +++ b/macgit/App/PullRequestController.swift @@ -67,6 +67,8 @@ private struct GitRepositoryIdentityKey: Hashable { final class PullRequestController: ObservableObject { @Published private(set) var items: [PullRequestSummary] = [] @Published private(set) var isLoading = false + @Published private(set) var hasLoadedList = false + private var displayedListKey: PullRequestListCacheKey? @Published var errorMessage: String? @Published var detailErrorMessage: String? @Published var stateFilter: PullRequestListFilter = .open @@ -389,6 +391,8 @@ final class PullRequestController: ObservableObject { knownGitLabHosts: connectedGitLabHosts, knownHosts: providerAccountController.accounts.map { GitProviderHost(kind: $0.provider, baseURL: $0.hostURL) } ) else { + hasLoadedList = false + displayedListKey = nil items = [] resetPagination() selectedProviderAccountID = nil @@ -409,6 +413,8 @@ final class PullRequestController: ObservableObject { let matchingAccounts = matchingAccounts(for: repository) guard !matchingAccounts.isEmpty else { + hasLoadedList = false + displayedListKey = nil items = [] resetPagination() selectedProviderAccountID = nil @@ -420,6 +426,8 @@ final class PullRequestController: ObservableObject { } guard let apiCredential = apiCredential(for: matchingAccounts) else { + hasLoadedList = false + displayedListKey = nil items = [] resetPagination() selectedProviderAccountID = nil @@ -437,6 +445,8 @@ final class PullRequestController: ObservableObject { let token = apiCredential.token guard let service = services[repository.provider] else { + hasLoadedList = false + displayedListKey = nil items = [] resetPagination() activeRepository = nil @@ -455,13 +465,18 @@ final class PullRequestController: ObservableObject { page: page, perPage: pullRequestPageSize ) - if !forceRefresh, - let cached = await diskCache.value(PullRequestListPage.self, key: cacheKey.diskKey, generation: cacheGeneration) { + hasLoadedList = displayedListKey == cacheKey + var displayedPage: PullRequestListPage? + if displayedListKey == cacheKey { + displayedPage = PullRequestListPage(items: items, page: currentPage, perPage: pullRequestPageSize, hasPreviousPage: hasPreviousPage, hasNextPage: hasNextPage) + hasLoadedList = true + } + if let cached = await diskCache.entry(PullRequestListPage.self, key: cacheKey.diskKey, generation: cacheGeneration) { guard listLoadID == loadID, !Task.isCancelled else { return } - apply(cached) - accountConnectionHost = nil - errorMessage = nil - return + if displayedPage != cached.value { apply(cached.value) } + displayedPage = cached.value + displayedListKey = cacheKey + hasLoadedList = true } guard listLoadID == loadID, !Task.isCancelled else { return } @@ -477,19 +492,25 @@ final class PullRequestController: ObservableObject { await diskCache.save(pageResult, key: cacheKey.diskKey, accountID: cacheKey.accountID, kind: "list", ttl: listCacheTTL, generation: cacheGeneration) guard listLoadID == loadID, !Task.isCancelled else { return } - apply(pageResult) + if displayedPage != pageResult { apply(pageResult) } + displayedListKey = cacheKey + hasLoadedList = true accountConnectionHost = nil errorMessage = nil } catch let error as PullRequestProviderError { guard listLoadID == loadID, !Task.isCancelled else { return } - items = [] - resetPagination() + if !hasLoadedList { + items = [] + resetPagination() + } accountConnectionHost = nil errorMessage = error.localizedDescription } catch { guard listLoadID == loadID, !Task.isCancelled else { return } - items = [] - resetPagination() + if !hasLoadedList { + items = [] + resetPagination() + } accountConnectionHost = nil errorMessage = error.localizedDescription } @@ -531,11 +552,10 @@ final class PullRequestController: ObservableObject { accountID: selectedProviderAccountID ?? "", number: summary.number ) - if !forceRefresh, - let cached = await diskCache.value(PullRequestDetail.self, key: cacheKey.diskKey, generation: cacheGeneration) { + if let cached = await diskCache.entry(PullRequestDetail.self, key: cacheKey.diskKey, generation: cacheGeneration) { guard detailLoadID == loadID, !Task.isCancelled else { return } - selectedDetail = cached - return + selectedDetail = cached.value + if !forceRefresh, cached.expiresAt > .now { return } } guard detailLoadID == loadID, !Task.isCancelled else { return } @@ -1034,6 +1054,8 @@ final class PullRequestController: ObservableObject { } func releaseVisibleData() { + hasLoadedList = false + displayedListKey = nil listLoadID = UUID() isLoading = false items = [] From bd44c7afd9ba67d5b47279bf68864d10a9b09be3 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:39:34 +0700 Subject: [PATCH 5/7] fix: address branch sanitization and refresh review feedback --- macgit/App/PullRequestDetailWindowController.swift | 2 +- macgit/Models/GitBranchNameSanitizer.swift | 10 ++++++++-- macgit/Views/FileStatus/FileStatusView.swift | 2 ++ macgit/Views/PullRequests/PullRequestListView.swift | 4 +++- macgitTests/GitFlowLitePhase1Tests.swift | 7 +++++++ 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/macgit/App/PullRequestDetailWindowController.swift b/macgit/App/PullRequestDetailWindowController.swift index c803f7ec..760bb1c8 100644 --- a/macgit/App/PullRequestDetailWindowController.swift +++ b/macgit/App/PullRequestDetailWindowController.swift @@ -22,7 +22,7 @@ final class PullRequestDetailWindowController: NSWindowController, NSWindowDeleg } func show(title: String, content: Content, onClose: @escaping () -> Void) { - if activateIfPresented() { return } + close() self.onClose = onClose let screen = NSApp.keyWindow?.screen ?? NSScreen.main let visibleFrame = screen?.visibleFrame ?? NSRect(x: 0, y: 0, width: 1200, height: 800) diff --git a/macgit/Models/GitBranchNameSanitizer.swift b/macgit/Models/GitBranchNameSanitizer.swift index 57e7dcd9..e9d07e80 100644 --- a/macgit/Models/GitBranchNameSanitizer.swift +++ b/macgit/Models/GitBranchNameSanitizer.swift @@ -35,8 +35,14 @@ enum GitBranchNameSanitizer { let components = name.split(separator: "/").compactMap { component -> String? in var value = String(component) while value.hasPrefix(".") { value.removeFirst() } - while value.hasSuffix(".lock") { value.removeLast(5) } - while value.hasSuffix(".") { value.removeLast() } + // Removing one suffix can expose another forbidden suffix. + while value.hasSuffix(".") || value.hasSuffix(".lock") { + if value.hasSuffix(".") { + value.removeLast() + } else { + value.removeLast(5) + } + } return value.isEmpty ? nil : value } name = components.joined(separator: "/") diff --git a/macgit/Views/FileStatus/FileStatusView.swift b/macgit/Views/FileStatus/FileStatusView.swift index 4fb27564..c519fe6b 100644 --- a/macgit/Views/FileStatus/FileStatusView.swift +++ b/macgit/Views/FileStatus/FileStatusView.swift @@ -1630,6 +1630,8 @@ struct FileStatusView: View { self.actionSelectionAnchorKey = selectedActionFileKeys.first } + // Replace the invalidated full load so ancillary metadata also catches up. + requestRefresh() } private func reloadRepositoryState() async { diff --git a/macgit/Views/PullRequests/PullRequestListView.swift b/macgit/Views/PullRequests/PullRequestListView.swift index eda68aac..6156e23d 100644 --- a/macgit/Views/PullRequests/PullRequestListView.swift +++ b/macgit/Views/PullRequests/PullRequestListView.swift @@ -156,7 +156,9 @@ struct PullRequestListView: View { .onTapGesture { Task { guard await authorizeAction() else { return } - if detailWindowController.activateIfPresented() { return } + if selectedPullRequestID == item.id, + detailWindowController.activateIfPresented() { return } + detailWindowController.close() controller.clearSelectedDetail() selectedPullRequestID = item.id detailWindowController.show( diff --git a/macgitTests/GitFlowLitePhase1Tests.swift b/macgitTests/GitFlowLitePhase1Tests.swift index c5f04332..f3b179c0 100644 --- a/macgitTests/GitFlowLitePhase1Tests.swift +++ b/macgitTests/GitFlowLitePhase1Tests.swift @@ -38,6 +38,13 @@ final class GitFlowLitePhase1Tests: XCTestCase { XCTAssertEqual(GitBranchNameSanitizer.sanitize("Feature--API"), "Feature--API") } + func testBranchNameSanitizerRemovesExposedForbiddenSuffixes() { + XCTAssertEqual(GitBranchNameSanitizer.sanitize("feature.lock."), "feature") + XCTAssertEqual(GitBranchNameSanitizer.sanitize("feature.lock.lock."), "feature") + XCTAssertEqual(GitBranchNameSanitizer.sanitize("feature.lock.lock"), "feature") + XCTAssertEqual(GitBranchNameSanitizer.sanitize("feature.lock./child.lock."), "feature/child") + } + func testPlannerMapsTopicKindsToExpectedBaseAndPrefix() throws { let configuration = GitFlowConfiguration(isEnabled: true) let planner = GitFlowPlanner() From 4bff1367060cb0ba2309bf1922b6569c78f1e448 Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:40:38 +0700 Subject: [PATCH 6/7] fix: Keep cached PRs visible during revalidation Preserve fetched entries past expiration and surface refresh errors without clearing the loaded list. --- macgit/Services/PullRequestDiskCache.swift | 37 ++++++++++++++----- .../PullRequests/PullRequestListView.swift | 12 +++++- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/macgit/Services/PullRequestDiskCache.swift b/macgit/Services/PullRequestDiskCache.swift index 6684f774..7060c96c 100644 --- a/macgit/Services/PullRequestDiskCache.swift +++ b/macgit/Services/PullRequestDiskCache.swift @@ -26,15 +26,32 @@ actor PullRequestDiskCache { func generation() -> UUID { epoch } + struct Entry: Sendable { + let value: Value + let fetchedAt: Date + let expiresAt: Date + } + func value(_ type: T.Type, key: String, generation: UUID, now: Date = .now) -> T? { + guard let cached = entry(type, key: key, generation: generation, now: now), + cached.expiresAt > now else { return nil } + return cached.value + } + + /// Expiration controls revalidation, not whether local data may be displayed. + func entry(_ type: T.Type, key: String, generation: UUID, + now: Date = .now) -> Entry? { guard generation == epoch else { return nil } return try? database { db in - var result: T? - try query(db, "SELECT payload FROM entries WHERE key = ? AND expires > ?", [key, String(now.timeIntervalSince1970)]) { statement in + var result: Entry? + try query(db, "SELECT payload, fetched, expires FROM entries WHERE key = ?", [key]) { statement in let count = Int(sqlite3_column_bytes(statement, 0)) - guard count <= maximumEntryBytes, let bytes = sqlite3_column_blob(statement, 0) else { return } - result = try? JSONDecoder().decode(type, from: Data(bytes: bytes, count: count)) + guard count <= maximumEntryBytes, let bytes = sqlite3_column_blob(statement, 0), + let value = try? JSONDecoder().decode(type, from: Data(bytes: bytes, count: count)) else { return } + result = Entry(value: value, + fetchedAt: Date(timeIntervalSince1970: sqlite3_column_double(statement, 1)), + expiresAt: Date(timeIntervalSince1970: sqlite3_column_double(statement, 2))) } if result != nil { try query(db, "UPDATE entries SET accessed = ? WHERE key = ?", [String(now.timeIntervalSince1970), key]) @@ -57,10 +74,9 @@ actor PullRequestDiskCache { try? database { db in try query(db, "BEGIN IMMEDIATE") do { - try query(db, "DELETE FROM entries WHERE expires <= ?", [String(now.timeIntervalSince1970)]) - try query(db, "INSERT OR REPLACE INTO entries (key, account, kind, number, expires, accessed, payload) VALUES (?, ?, ?, ?, ?, ?, ?)", + try query(db, "INSERT OR REPLACE INTO entries (key, account, kind, number, expires, accessed, fetched, payload) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", [key, accountID, kind, String(number), String(now.addingTimeInterval(ttl).timeIntervalSince1970), - String(now.timeIntervalSince1970), String(decoding: data, as: UTF8.self)]) + String(now.timeIntervalSince1970), String(now.timeIntervalSince1970), String(decoding: data, as: UTF8.self)]) var size = 0 var count = 0 try query(db, "SELECT COALESCE(SUM(length(CAST(payload AS BLOB))), 0), COUNT(*) FROM entries") { @@ -118,9 +134,12 @@ actor PullRequestDiskCache { try query(db, "PRAGMA max_page_count = 10240") var version = 0 try query(db, "PRAGMA user_version") { version = Int(sqlite3_column_int($0, 0)) } - guard version <= 1 else { throw CocoaError(.fileReadCorruptFile) } + guard version <= 2 else { throw CocoaError(.fileReadCorruptFile) } try query(db, "CREATE TABLE IF NOT EXISTS entries (key TEXT PRIMARY KEY, account TEXT NOT NULL, kind TEXT NOT NULL, number INTEGER NOT NULL, expires REAL NOT NULL, accessed REAL NOT NULL, payload TEXT NOT NULL)") - if version == 0 { try query(db, "PRAGMA user_version = 1") } + if version < 2 { + try query(db, "ALTER TABLE entries ADD COLUMN fetched REAL NOT NULL DEFAULT 0") + try query(db, "PRAGMA user_version = 2") + } return try operation(db) } diff --git a/macgit/Views/PullRequests/PullRequestListView.swift b/macgit/Views/PullRequests/PullRequestListView.swift index eda68aac..c8e5d05a 100644 --- a/macgit/Views/PullRequests/PullRequestListView.swift +++ b/macgit/Views/PullRequests/PullRequestListView.swift @@ -20,6 +20,7 @@ import MarkdownUI import SwiftUI struct PullRequestListView: View { + @EnvironmentObject private var appState: AppState @Environment(\.appTextScale) private var textScale @ObservedObject var controller: PullRequestController let repositoryURL: URL @@ -35,11 +36,17 @@ struct PullRequestListView: View { VStack(spacing: 0) { header Divider() + if controller.hasLoadedList, let error = controller.errorMessage { + Text(error) + .font(.callout.scaled(by: textScale)) + .foregroundStyle(.secondary) + .padding(8) + } - if controller.isLoading { + if controller.isLoading && !controller.hasLoadedList { ProgressView() .frame(maxWidth: .infinity, maxHeight: .infinity) - } else if let errorMessage = controller.errorMessage { + } else if let errorMessage = controller.errorMessage, !controller.hasLoadedList { VStack(spacing: 12) { Text(errorMessage) .font(.headline.scaled(by: textScale)) @@ -167,6 +174,7 @@ struct PullRequestListView: View { authorizeAction: authorizeAction, onClose: closeDetail ) + .environmentObject(appState) .environment(\.appTextScale, textScale) .sheetPresentationScope(), onClose: closeDetail From 8e23b05688ffcd3a856ff491addf2215d27838ab Mon Sep 17 00:00:00 2001 From: Thanh Tran Date: Wed, 14 Oct 2026 22:50:18 +0700 Subject: [PATCH 7/7] refactor: Restructure fetch and pull sheets into consistent layouts Use Grid and GroupBox for options, add descriptive subtitles and clearer labels, and replace custom button styles with standard bordered styles. --- macgit/Views/Common/FetchSheetView.swift | 55 +++++---- macgit/Views/Common/PullSheetView.swift | 144 +++++++++++++---------- 2 files changed, 115 insertions(+), 84 deletions(-) diff --git a/macgit/Views/Common/FetchSheetView.swift b/macgit/Views/Common/FetchSheetView.swift index 62d5a62f..1944e74a 100644 --- a/macgit/Views/Common/FetchSheetView.swift +++ b/macgit/Views/Common/FetchSheetView.swift @@ -40,47 +40,58 @@ struct FetchSheetView: View { } var body: some View { - VStack(spacing: 0) { - VStack(alignment: .leading, spacing: 16) { - VStack(alignment: .leading, spacing: 10) { + VStack(alignment: .leading, spacing: 20) { + VStack(alignment: .leading, spacing: 4) { + Text("Fetch Changes") + .font(.title2) + .bold() + Text("Download remote updates without changing your local branches.") + .foregroundStyle(.secondary) + } + + GroupBox { + VStack(alignment: .leading, spacing: 12) { Toggle("Fetch from all remotes", isOn: $fetchAllRemotes) - .font(.system(size: 13)) - .toggleStyle(.checkbox) - Toggle("Prune tracking branches no longer present on remote(s)", isOn: $pruneBranches) - .font(.system(size: 13)) - .toggleStyle(.checkbox) + VStack(alignment: .leading, spacing: 4) { + Toggle("Prune deleted remote branches", isOn: $pruneBranches) + Text("Remove remote-tracking references that no longer exist on the remote.") + .font(.callout) + .foregroundStyle(.secondary) + .padding(.leading, 20) + } Toggle("Fetch and store all tags locally", isOn: $fetchTags) - .font(.system(size: 13)) - .toggleStyle(.checkbox) } - .padding(12) - .background(.quaternary.opacity(0.3)) - .clipShape(RoundedRectangle(cornerRadius: 8, style: .continuous)) + .toggleStyle(.checkbox) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(8) + } label: { + Text("Options") + .bold() } - .padding(24) - - Spacer() - HStack(spacing: 12) { + HStack(spacing: 8) { Spacer() Button("Cancel", role: .cancel) { dismiss() } .keyboardShortcut(.cancelAction) + .buttonStyle(.bordered) - Button("OK") { + Button("Fetch") { dismiss() onFetch(fetchOptions) } .keyboardShortcut(.defaultAction) - .buttonStyle(GlassProminentButtonStyle(tint: .accentColor, fontSize: 13)) + .buttonStyle(.borderedProminent) } - .padding([.horizontal, .bottom], 24) } - .frame(minWidth: 420, idealWidth: 420, maxWidth: 420) - .frame(minHeight: 200, idealHeight: 220) + .font(.body) + .controlSize(.regular) + .padding(24) + .frame(width: 480) + .fixedSize(horizontal: false, vertical: true) } } diff --git a/macgit/Views/Common/PullSheetView.swift b/macgit/Views/Common/PullSheetView.swift index 61f75ae6..505590cc 100644 --- a/macgit/Views/Common/PullSheetView.swift +++ b/macgit/Views/Common/PullSheetView.swift @@ -58,19 +58,28 @@ struct PullSheetView: View { } var body: some View { - VStack(spacing: 0) { - ScrollView { - VStack(alignment: .leading, spacing: 16) { - // Remote repository - VStack(alignment: .leading, spacing: 4) { - Text("Pull from repository:") - .font(.system(size: 13)) - Picker("", selection: $selectedRemote) { + VStack(alignment: .leading, spacing: 20) { + VStack(alignment: .leading, spacing: 4) { + Text("Pull Changes") + .font(.title2) + .bold() + Text("Integrate changes from a remote branch into your current branch.") + .foregroundStyle(.secondary) + } + + Grid(alignment: .leading, horizontalSpacing: 16, verticalSpacing: 14) { + GridRow(alignment: .firstTextBaseline) { + Text("Remote:") + .gridColumnAlignment(.trailing) + VStack(alignment: .leading, spacing: 6) { + Picker("Remote", selection: $selectedRemote) { ForEach(remotes, id: \.self) { remote in Text(remote).tag(remote) } } .pickerStyle(.menu) + .labelsHidden() + .frame(maxWidth: .infinity, alignment: .leading) .onChange(of: selectedRemote) { _, newValue in guard !isLoading else { return } Task { @@ -81,86 +90,97 @@ struct PullSheetView: View { if !remoteURL.isEmpty { Text(remoteURL) - .font(.system(size: 11)) + .font(.callout) .foregroundStyle(.secondary) .lineLimit(1) + .truncationMode(.middle) + .help(remoteURL) } } + .gridColumnAlignment(.leading) + } - // Remote branch - HStack(spacing: 12) { - VStack(alignment: .leading, spacing: 4) { - Text("Remote branch to pull:") - .font(.system(size: 13)) - HStack(spacing: 8) { - SearchableReferencePicker( - title: "Remote branch to pull", selection: selectedBranch, - options: remoteBranches, searchPrompt: "Search remote branches", - onSelect: { selectedBranch = $0 }) - .pickerStyle(.menu) - - Button("Refresh") { - Task { await loadRemoteBranches(remote: selectedRemote) } - } - .buttonStyle(GlassButtonStyle(tint: .accentColor, fontSize: 11)) - } + GridRow(alignment: .firstTextBaseline) { + Text("Remote branch:") + HStack(spacing: 8) { + SearchableReferencePicker( + title: "Remote branch to pull", selection: selectedBranch, + options: remoteBranches, searchPrompt: "Search remote branches", + onSelect: { selectedBranch = $0 }) + .buttonStyle(.bordered) + .help(selectedBranch) + + Button("Refresh", systemImage: "arrow.clockwise") { + Task { await loadRemoteBranches(remote: selectedRemote) } } + .labelStyle(.iconOnly) + .help("Refresh remote branches") } + } - // Local branch - HStack(spacing: 12) { - Text("Pull into local branch:") - .font(.system(size: 13)) - Text(localBranch) - .font(.system(size: 13, weight: .medium)) - Spacer() - } - - // Options - VStack(alignment: .leading, spacing: 10) { - Text("Options") - .font(.system(size: 13, weight: .semibold)) - .padding(.bottom, 2) - - Toggle("Commit merged changes immediately", isOn: $commitMerged) - .font(.system(size: 12)) - - Toggle("Include messages from commits being merged in merge commit", isOn: $includeMessages) - .font(.system(size: 12)) - - Toggle("Create new commit even if fast-forward merge", isOn: $noFastForward) - .font(.system(size: 12)) + GridRow(alignment: .firstTextBaseline) { + Text("Local branch:") + Text(localBranch.isEmpty ? "—" : localBranch) + .fontWeight(.medium) + .lineLimit(2) + .truncationMode(.middle) + .textSelection(.enabled) + .help(localBranch) + .frame(maxWidth: .infinity, alignment: .leading) + } + } - Toggle("Rebase instead of merge (WARNING: make sure you haven't pushed your changes)", isOn: $rebaseInstead) - .font(.system(size: 12)) + GroupBox { + VStack(alignment: .leading, spacing: 12) { + Toggle("Commit merged changes immediately", isOn: $commitMerged) + Toggle("Include merged commit messages", isOn: $includeMessages) + .help("Include messages from commits being merged in the merge commit") + Toggle("Create a merge commit even when fast-forward is possible", isOn: $noFastForward) + VStack(alignment: .leading, spacing: 4) { + Toggle("Rebase instead of merge", isOn: $rebaseInstead) + Text("Only rebase changes you haven’t pushed.") + .font(.callout) + .foregroundStyle(.secondary) + .padding(.leading, 20) } - .padding(12) - .background(.quaternary.opacity(0.3)) - .clipShape(RoundedRectangle(cornerRadius: 8, style: .continuous)) } - .padding(24) + .toggleStyle(.checkbox) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(8) + } label: { + Text("Options") + .bold() } - // Buttons - HStack(spacing: 12) { + HStack(spacing: 8) { + if isLoading { + ProgressView() + .controlSize(.small) + Text("Loading branches…") + .font(.callout) + .foregroundStyle(.secondary) + } Spacer() Button("Cancel", role: .cancel) { dismiss() } .keyboardShortcut(.cancelAction) + .buttonStyle(.bordered) - Button("OK") { + Button("Pull") { dismiss() onPull(selectedRemote, selectedBranch, pullOptions) } .keyboardShortcut(.defaultAction) - .buttonStyle(GlassProminentButtonStyle(tint: .accentColor, fontSize: 13)) + .buttonStyle(.borderedProminent) .disabled(isLoading || selectedRemote.isEmpty || selectedBranch.isEmpty) } - .padding([.horizontal, .bottom], 24) } - .frame(minWidth: 520, idealWidth: 520, maxWidth: 520) - .frame(minHeight: 420, idealHeight: 460) + .font(.body) + .controlSize(.regular) + .padding(24) + .frame(width: 560) + .fixedSize(horizontal: false, vertical: true) .task { await loadData() }