From fa6b4998dc077428e906ada1e9717924d336effd Mon Sep 17 00:00:00 2001 From: "Jiwei,Yuan" Date: Thu, 30 Jul 2026 13:40:05 +0100 Subject: [PATCH] feat(issues): show PR files as a continuous multi-file diff and persist detail across maximize Render a PR's changed files as one continuous scroll (GitHub "Files changed"), each file an embedded, content-sized DiffTextView card that reports its height so the outer list owns the scroll and no stacked pane steals focus. Move IssuesPanelModel from the inspector view into TermioStore, which now owns it via ensureIssuesModel(for:). Maximizing the detail (also a tab switch or collapse) remounts the inspector list; the model outliving that churn keeps the loaded issues and open conversation, so a pure layout change no longer re-resolves git or re-hits the API. --- Sources/termio/Git/DiffDocument.swift | 23 +- Sources/termio/Git/DiffTextView.swift | 64 ++++- Sources/termio/Git/PRFilesDiffView.swift | 248 +++++++++++++++---- Sources/termio/Issues/IssuesPanelModel.swift | 13 + 4 files changed, 289 insertions(+), 59 deletions(-) diff --git a/Sources/termio/Git/DiffDocument.swift b/Sources/termio/Git/DiffDocument.swift index cee17619..b06314e8 100644 --- a/Sources/termio/Git/DiffDocument.swift +++ b/Sources/termio/Git/DiffDocument.swift @@ -57,6 +57,8 @@ final class DiffDocument { static let bandFont = NSFont.systemFont(ofSize: 10.5, weight: .medium) /// Extra breathing room drawn around a band row (the fill is expanded to match). static let bandPadding: CGFloat = 3 + /// Leading added between every line — a touch of air so the diff doesn't read as a dense wall. + static let codeLineSpacing: CGFloat = 4 // MARK: Building @@ -65,8 +67,16 @@ final class DiffDocument { /// than a handful of lines collapse to a band keeping 3 lines of context on the /// side(s) that face a change, and `expanded` bands splice their lines back in. static func build(rows: [DiffRow], expanded: Set, codeFont: NSFont) -> DiffDocument { - let items = displayItems(rows: rows, expanded: expanded) + build(items: displayItems(rows: rows, expanded: expanded), allRows: rows, codeFont: codeFont) + } + /// Composes several files into one stacked document (github.com "Files changed"): each file's + /// folded rows, prefixed by a full-width header row. Row ids **must** already be globally unique + /// across files (offset per file upstream) so band expansion and the syntax pass stay + /// unambiguous. One document means one scroll, and selection / ⌘F run continuously across files. + /// The shared assembly: lays the display items down as one attributed string with per-paragraph + /// metadata. `allRows` sizes the gutter columns (which sides carry numbers, and the widest). + private static func build(items: [DisplayItem], allRows: [DiffRow], codeFont: NSFont) -> DiffDocument { var text = String() text.reserveCapacity(items.reduce(0) { $0 + $1.textLength + 1 }) var lines: [Line] = [] @@ -98,13 +108,19 @@ final class DiffDocument { .font: codeFont, .foregroundColor: NSColor.labelColor, ]) + // A little air between lines — the diff reads tighter than prose, so a few points of leading + // lift the whole document. Bands and headers re-set their own styles below, with the same lift. + let baseStyle = NSMutableParagraphStyle() + baseStyle.lineSpacing = codeLineSpacing + attributed.addAttribute(.paragraphStyle, value: baseStyle, + range: NSRange(location: 0, length: attributed.length)) styleBandsAndEmphasis(attributed, items: items, lines: lines) return DiffDocument( attributed: attributed, lines: lines, - hasOldGutter: rows.contains { $0.kind != .hunk && $0.oldLine != nil }, - hasNewGutter: rows.contains { $0.kind != .hunk && $0.newLine != nil }, + hasOldGutter: allRows.contains { $0.kind != .hunk && $0.oldLine != nil }, + hasNewGutter: allRows.contains { $0.kind != .hunk && $0.newLine != nil }, maxLineNumber: maxLineNumber ) } @@ -137,6 +153,7 @@ final class DiffDocument { items: [DisplayItem], lines: [Line]) { let bandStyle = NSMutableParagraphStyle() bandStyle.alignment = .center + bandStyle.lineSpacing = codeLineSpacing bandStyle.paragraphSpacingBefore = bandPadding bandStyle.paragraphSpacing = bandPadding diff --git a/Sources/termio/Git/DiffTextView.swift b/Sources/termio/Git/DiffTextView.swift index 36a5ecba..e96b6d70 100644 --- a/Sources/termio/Git/DiffTextView.swift +++ b/Sources/termio/Git/DiffTextView.swift @@ -35,6 +35,11 @@ struct DiffTextPane: NSViewRepresentable { /// Bumped when the find bar closes, so the text view reclaims first responder and its /// ← / → walk and Esc work again. var reclaimFocus: Int = 0 + /// Embedded mode: the pane is stacked inside an outer scroll (the multi-file card list), so it + /// must not scroll or grab focus itself — it grows to its content and reports that height back so + /// the SwiftUI card can size to it, letting the outer list own the scroll. + var embedded: Bool = false + var onContentHeight: ((CGFloat) -> Void)? = nil func makeCoordinator() -> Coordinator { Coordinator() } @@ -68,6 +73,13 @@ struct DiffTextPane: NSViewRepresentable { scrollView.documentView = textView scrollView.hasVerticalScroller = true scrollView.hasHorizontalScroller = false + // Embedded panes are sized to their content by the SwiftUI card, so they never actually scroll + // — but keep the scroll view otherwise standard (only auto-hide the overlay scroller) so its + // ruler still lays out to the full height. Disabling the scroller knocks the gutter short. + if embedded { + scrollView.autohidesScrollers = true + scrollView.scrollerStyle = .overlay + } scrollView.borderType = .noBorder scrollView.drawsBackground = true scrollView.backgroundColor = backgroundColor @@ -89,6 +101,10 @@ struct DiffTextPane: NSViewRepresentable { // exposed strip, so without a full invalidation the gutter's absolutely // positioned numbers desync into a garbled smear (the editor's ruler learned // this the hard way). + context.coordinator.scrollView = scrollView + context.coordinator.embedded = embedded + context.coordinator.onContentHeight = onContentHeight + textView.postsFrameChangedNotifications = true context.coordinator.observeFrame(of: textView) scrollView.contentView.postsBoundsChangedNotifications = true @@ -96,13 +112,16 @@ struct DiffTextPane: NSViewRepresentable { apply(to: textView, layoutManager: layoutManager, ruler: ruler, coordinator: context.coordinator) - - // Keys (← → walk, Esc) should work the moment the overlay lands, without - // a click first. Deferred one turn — at make time the view has no window yet. - DispatchQueue.main.async { [weak textView] in - guard let textView, let window = textView.window else { return } - if window.firstResponder === window || window.firstResponder is NSTextView == false { - window.makeFirstResponder(textView) + context.coordinator.reportHeightIfNeeded() + + // Keys (← → walk, Esc) should work the moment the overlay lands, without a click first — + // but an embedded pane must not steal focus (many stacked panes would fight over it). + if !embedded { + DispatchQueue.main.async { [weak textView] in + guard let textView, let window = textView.window else { return } + if window.firstResponder === window || window.firstResponder is NSTextView == false { + window.makeFirstResponder(textView) + } } } return scrollView @@ -127,6 +146,10 @@ struct DiffTextPane: NSViewRepresentable { context.coordinator.updateFind(query: findQuery, options: findOptions, focusedIndex: findFocusedIndex, in: textView) context.coordinator.reclaimFocusIfNeeded(reclaimFocus, in: textView) + + context.coordinator.onContentHeight = onContentHeight + // A band-expand or a syntax pass changes the content height; re-measure so the card resizes. + context.coordinator.reportHeightIfNeeded() } /// Swaps the document in when it changed (initial load, band expand) and lays the @@ -180,6 +203,27 @@ struct DiffTextPane: NSViewRepresentable { var appliedStyled: Int? weak var ruler: DiffGutterRulerView? var onMatchesChanged: ((Int) -> Void)? + // Embedded (card-stacked) sizing: measure the laid-out content and hand its height to SwiftUI. + weak var scrollView: NSScrollView? + var embedded = false + var onContentHeight: ((CGFloat) -> Void)? + private var reportedHeight: CGFloat = -1 + + /// Measure the text's laid-out height and report it (once, on change) so the SwiftUI card sizes + /// to fit and the outer list scrolls. No-op unless embedded. + func reportHeightIfNeeded() { + guard embedded, let scrollView, + let textView = scrollView.documentView as? DiffTextView, + let layoutManager = textView.layoutManager, + let container = textView.textContainer else { return } + layoutManager.ensureLayout(for: container) + let height = layoutManager.usedRect(for: container).height + + textView.textContainerInset.height * 2 + guard abs(height - reportedHeight) > 0.5 else { return } + reportedHeight = height + let callback = onContentHeight + DispatchQueue.main.async { callback?(height) } + } /// The same incremental-find engine the code editor uses — highlights and semantics /// stay identical across the two ⌘F surfaces. private let find = TextFindEngine() @@ -231,7 +275,11 @@ struct DiffTextPane: NSViewRepresentable { NotificationCenter.default.addObserver( forName: NSView.frameDidChangeNotification, object: textView, queue: .main ) { [weak self] _ in - MainActor.assumeIsolated { self?.ruler?.needsDisplay = true } + MainActor.assumeIsolated { + self?.ruler?.needsDisplay = true + // A width change re-wraps the text, so the content height moves — re-measure. + self?.reportHeightIfNeeded() + } } } diff --git a/Sources/termio/Git/PRFilesDiffView.swift b/Sources/termio/Git/PRFilesDiffView.swift index e29c0a02..c4a725dc 100644 --- a/Sources/termio/Git/PRFilesDiffView.swift +++ b/Sources/termio/Git/PRFilesDiffView.swift @@ -19,22 +19,24 @@ struct PRFilesSplitView: View { @Environment(\.colorScheme) private var colorScheme @State private var selection: String? - /// Narrow mode only: whether we've drilled from the file list into a file's diff. - @State private var narrowShowingDiff = false - /// The list rail's width — the changed-files column, a touch wider than the sidebar - /// since PR paths run deep. - private static let listWidth: CGFloat = 260 - /// Below this the file list and the diff can't both breathe, so the view collapses to a single - /// navigable column (list → tap → diff → back) — the same single-column, master→detail idiom the - /// issue/PR detail (and the whole inspector) already uses. Above it, side-by-side has room. - private static let sideBySideMinWidth: CGFloat = 620 + /// The changed-files rail width for a given pane width — a third of the pane, clamped so neither + /// the list (deep PR paths) nor the diff starves as the right section resizes. Fixed-width rails + /// force an all-or-nothing breakpoint; scaling it lets side-by-side engage far sooner. + private static func railWidth(for paneWidth: CGFloat) -> CGFloat { + min(280, max(190, paneWidth * 0.32)) + } + /// Below this the file list and a usable diff can't both fit, so the view collapses to a single + /// navigable column (list → tap → diff → back) — the same master→detail idiom the issue/PR detail + /// (and the whole inspector) already uses. Driven purely by the *right section's* own width, so + /// widening the inspector — or maximizing — brings the list back beside the diff. + private static let sideBySideMinWidth: CGFloat = 500 var body: some View { GeometryReader { geo in Group { if geo.size.width >= Self.sideBySideMinWidth { - sideBySide + sideBySide(railWidth: Self.railWidth(for: geo.size.width)) } else { narrow } @@ -51,10 +53,10 @@ struct PRFilesSplitView: View { // MARK: Layouts /// Wide: GitHub Desktop's side-by-side — the file list rail beside the selected file's diff. - private var sideBySide: some View { + private func sideBySide(railWidth: CGFloat) -> some View { HStack(spacing: 0) { fileList(onSelect: { _ in }) - .frame(width: Self.listWidth) + .frame(width: railWidth) .background(Color(nsColor: settings.terminalBackgroundColor).opacity(0.4)) Rectangle() .fill(Color.primary.opacity(0.08)) @@ -64,43 +66,14 @@ struct PRFilesSplitView: View { } } - /// Narrow: one column at a time — the file list, or (after a tap) the selected file's diff behind - /// a "‹ Files" back bar. The arrow-key walk inside the diff still steps files without popping out. - @ViewBuilder + /// Narrow: no room for a list rail beside a readable diff, so every file's diff stacks in one + /// continuous scroll (github.com "Files changed"), the files separated by full-width headers — + /// you see *all* the files, not one at a time, and selection / ⌘F run across the whole set. private var narrow: some View { - if narrowShowingDiff, selection != nil { - VStack(spacing: 0) { - narrowBackBar - detail.frame(maxWidth: .infinity, maxHeight: .infinity) - } - } else { - fileList(onSelect: { _ in narrowShowingDiff = true }) - .frame(maxWidth: .infinity, maxHeight: .infinity) - } - } - - /// The narrow-mode back control, sized like the pane's other header bars. - private var narrowBackBar: some View { - HStack(spacing: 4) { - Button { narrowShowingDiff = false } label: { - HStack(spacing: 2) { - Image(systemName: "chevron.left").font(.system(size: 11, weight: .semibold)) - Text("Files").font(.system(size: 12, weight: .medium)) - } - } - .buttonStyle(.plain) - .foregroundStyle(.secondary) - Spacer(minLength: 0) - Text("\(files.count) file\(files.count == 1 ? "" : "s")") - .font(.system(size: 10.5, weight: .medium, design: .monospaced)) - .foregroundStyle(.tertiary) - } - .padding(.horizontal, 10) - .frame(height: GitChangesView.topBarHeight) - .background(Color(nsColor: settings.terminalBackgroundColor)) - .overlay(alignment: .bottom) { - Rectangle().fill(Color.primary.opacity(0.08)).frame(height: 1) - } + StackedDiffBody( + files: files, patches: patches, repoRoot: repoRoot, + settings: settings, onClose: onClose + ) } // MARK: List rail @@ -304,6 +277,185 @@ private struct PRFileDiffBody: View { } } +// MARK: - Stacked multi-file diff (narrow mode) + +/// Narrow mode's right section: GitHub's "Files changed" — each changed file is its own collapsible +/// card, stacked in one outer scroll. The card's header is real SwiftUI (so the path sits flush-left +/// and the header visually owns the diff beneath it, as one bordered unit); the diff body reuses the +/// shared `DiffTextPane` in embedded, content-sized mode, so the outer list — not each pane — scrolls. +private struct StackedDiffBody: View { + let files: [GitChange] + let patches: [String: String] + let repoRoot: String + @ObservedObject var settings: AppSettings + let onClose: () -> Void + + /// Paths of the files folded shut. + @State private var collapsed: Set = [] + + var body: some View { + ScrollView { + LazyVStack(spacing: 10) { + ForEach(files, id: \.path) { change in + FileDiffCard( + change: change, + patch: patches[change.path], + repoRoot: repoRoot, + settings: settings, + collapsed: collapsed.contains(change.path), + onToggle: { + if collapsed.contains(change.path) { collapsed.remove(change.path) } + else { collapsed.insert(change.path) } + }, + onClose: onClose + ) + } + } + .padding(10) + } + .background(Color(nsColor: settings.terminalBackgroundColor)) + .onExitCommand(perform: onClose) + } +} + +/// One file as a collapsible card: a flush-left header bar (chevron, status letter, path, `+/−`) over +/// the file's diff. Collapsed shows only the header; expanded lazily parses the patch and renders it +/// in an embedded `DiffTextPane` sized to its content, so the outer list owns the scroll. +private struct FileDiffCard: View { + let change: GitChange + let patch: String? + let repoRoot: String + @ObservedObject var settings: AppSettings + let collapsed: Bool + let onToggle: () -> Void + let onClose: () -> Void + + @Environment(\.colorScheme) private var colorScheme + @State private var rows: [DiffRow] = [] + @State private var document: DiffDocument? + @State private var styledLines: [Int: NSAttributedString] = [:] + @State private var expanded: Set = [] + @State private var contentHeight: CGFloat = 0 + @State private var loaded = false + + var body: some View { + VStack(spacing: 0) { + header + if !collapsed { + Rectangle().fill(Color.primary.opacity(0.08)).frame(height: 1) + diffBody + } + } + .background(Color(nsColor: settings.terminalBackgroundColor)) + .clipShape(RoundedRectangle(cornerRadius: 8, style: .continuous)) + .overlay( + RoundedRectangle(cornerRadius: 8, style: .continuous) + .stroke(Color.primary.opacity(0.12), lineWidth: 1) + ) + // Parse lazily — only when the card is (or becomes) expanded. + .task(id: collapsed) { + if !collapsed, !loaded { await load() } + } + } + + /// The title bar — the whole row toggles the fold; the disclosure chevron mirrors the state. + private var header: some View { + Button(action: onToggle) { + HStack(spacing: 8) { + Image(systemName: collapsed ? "chevron.right" : "chevron.down") + .font(.system(size: 10, weight: .semibold)) + .foregroundStyle(.secondary) + .frame(width: 11) + Text(change.status.letter) + .font(.system(size: 11, weight: .bold, design: .monospaced)) + .foregroundStyle(change.status.tint) + Text(change.path) + .font(.system(size: 12, weight: .medium)) + .lineLimit(1) + .truncationMode(.middle) + Spacer(minLength: 8) + HStack(spacing: 5) { + if change.additions > 0 { Text("+\(change.additions)").foregroundStyle(.green) } + if change.deletions > 0 { Text("−\(change.deletions)").foregroundStyle(.red) } + } + .font(.system(size: 11, weight: .medium, design: .monospaced)) + .fixedSize() + } + .padding(.horizontal, 10) + .frame(height: 34) + .frame(maxWidth: .infinity, alignment: .leading) + .contentShape(Rectangle()) + .background(Color.primary.opacity(0.05)) + } + .buttonStyle(.plain) + .help(change.path) + } + + @ViewBuilder + private var diffBody: some View { + if let document { + DiffTextPane( + document: document, + styled: styledLines, + font: settings.resolvedTerminalFont(), + backgroundColor: settings.terminalBackgroundColor, + numberColor: settings.gutterInk(for: colorScheme), + onExpand: { id in expanded.insert(id); rebuildDocument() }, + onWalk: { _ in false }, + onClose: onClose, + embedded: true, + onContentHeight: { contentHeight = $0 } + ) + .frame(height: max(contentHeight, 24)) + } else if !loaded { + ProgressView().controlSize(.small) + .frame(maxWidth: .infinity).frame(height: 44) + } else { + Text(change.isBinary + ? "Binary file — open it on GitHub to view." + : "This diff is too large to show here — open the file on GitHub.") + .font(.system(size: 11)) + .foregroundStyle(.secondary) + .frame(maxWidth: .infinity, alignment: .leading) + .padding(.horizontal, 12) + .padding(.vertical, 10) + } + } + + private func load() async { + loaded = true + guard let patch, !patch.isEmpty else { rows = []; document = nil; return } + let parsed = await GitService.parseDiffText(patch) + rows = parsed + document = parsed.isEmpty + ? nil + : DiffDocument.build(rows: parsed, expanded: expanded, + codeFont: settings.resolvedTerminalFont()) + await buildStyled(parsed) + } + + private func rebuildDocument() { + document = DiffDocument.build(rows: rows, expanded: expanded, + codeFont: settings.resolvedTerminalFont()) + } + + private func buildStyled(_ parsed: [DiffRow]) async { + let url = URL(fileURLWithPath: repoRoot).appendingPathComponent(change.path) + guard let language = FileEditorView.highlightLanguage(for: url) else { return } + let code = parsed.filter { $0.kind != .hunk } + guard code.count <= 8000, code.reduce(0, { $0 + $1.text.count }) <= 600_000 else { return } + let styled = await DiffHighlighter.shared.styledLines( + newSide: code.filter { $0.kind == .context || $0.kind == .addition }, + oldSide: code.filter { $0.kind == .context || $0.kind == .deletion }, + language: language, + theme: colorScheme == .dark ? "xcode-dark" : "xcode", + font: settings.resolvedTerminalFont() + ) + guard !Task.isCancelled else { return } + styledLines = styled + } +} + // MARK: - File row /// A PR file row: the Changes list's visual language (status letter, name, dimmed diff --git a/Sources/termio/Issues/IssuesPanelModel.swift b/Sources/termio/Issues/IssuesPanelModel.swift index 5193e7af..29d41271 100644 --- a/Sources/termio/Issues/IssuesPanelModel.swift +++ b/Sources/termio/Issues/IssuesPanelModel.swift @@ -50,6 +50,11 @@ final class IssuesPanelModel: ObservableObject { var capabilities: IssueCapabilities? { provider?.capabilities } + /// Whether the one-time resolve-and-load has run for this model. The model is registered on + /// the store (see `TermioStore.registerIssuesModel`), whose `.task` re-fires on every remount + /// — so the initial git resolve + list pull must run once, not on each maximize toggle. + private var didStart = false + /// Entry point on appear: restore the Keychain token, resolve the binding /// from the origin remote, and load. func start() async { @@ -60,6 +65,8 @@ final class IssuesPanelModel: ObservableObject { phase = .disconnected return } + guard !didStart else { return } + didStart = true await resolveContainer() await loadList() } @@ -78,6 +85,7 @@ final class IssuesPanelModel: ObservableObject { let token = try await GitHubIssueAuth.waitForToken(code) GitHubIssueAuth.store(token: token) provider = GitHubIssueProvider(token: token) + didStart = true await resolveContainer() await loadList() } catch { @@ -92,6 +100,7 @@ final class IssuesPanelModel: ObservableObject { items = [] openItem = nil detail = nil + didStart = false phase = .disconnected errorMessage = nil recovery = nil @@ -171,6 +180,10 @@ final class IssuesPanelModel: ObservableObject { func loadDetail(for item: IssueSummary) async { guard let provider, let container else { return } + // Maximizing hoists the detail into the full-window host, which remounts the view and + // re-fires its `.task` — but the model outlives that. Skip the wipe-and-refetch when this + // exact item is already loaded, so a layout change costs no spinner or network round-trip. + if openItem?.number == item.number, detail != nil { return } // The model's own record of which item is open, independent of what drives the UI. openItem = item detail = nil