Fix branch name sanitization and improve UI refresh state handling

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 = []

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<Content: View>(title: String, content: Content, onClose: @escaping () -> Void) {
+        close()
+        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?()
+    }
+}

macgit/Models/GitBranchNameSanitizer.swift

@@ -20,6 +20,34 @@ 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() }
+            // 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: "/")
+        if name.hasPrefix("-") { return sanitize(name) }
+        if name == "@" || name == "HEAD" { name += "-branch" }
+        return name
     }
 }

macgit/Services/PullRequestDiskCache.swift

@@ -26,15 +26,32 @@ actor PullRequestDiskCache {
 
     func generation() -> UUID { epoch }
 
+    struct Entry<Value: Sendable>: Sendable {
+        let value: Value
+        let fetchedAt: Date
+        let expiresAt: Date
+    }
+
     func value<T: Decodable & Sendable>(_ 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<T: Decodable & Sendable>(_ type: T.Type, key: String, generation: UUID,
+                                       now: Date = .now) -> Entry<T>? {
         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<T>?
+            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)
     }
 

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)
             }
         }
     }