From c55843f8a2ca525b590fe143cf9a1ab0d0a3058d Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 25 Sep 2026 09:04:29 -0500 Subject: [PATCH 1/3] tray: review changes in its own window, files collapsed by default Review opens a resizable window (one, retargeted per row) instead of a sheet capped by the Worktrees window. Each file is a disclosure header on the card colour over a neutral diff body; all start collapsed, and option-click opens or closes them all. Co-Authored-By: Claude Opus 5.5 (1M context) --- rt-tray/Sources/WorktreePanelView.swift | 8 +- rt-tray/Sources/WorktreeReviewSheet.swift | 99 ++++++++++++++----- rt-tray/Sources/WorktreeReviewWindow.swift | 45 +++++++++ rt-tray/Sources/WorktreeSnapshot.swift | 8 +- rt-tray/Sources/WorktreeTokens.swift | 3 +- .../Tests/fixtures/worktree-triage-diff.json | 20 +++- 6 files changed, 147 insertions(+), 36 deletions(-) create mode 100644 rt-tray/Sources/WorktreeReviewWindow.swift diff --git a/rt-tray/Sources/WorktreePanelView.swift b/rt-tray/Sources/WorktreePanelView.swift index 88a9f63d5..83e6ca370 100644 --- a/rt-tray/Sources/WorktreePanelView.swift +++ b/rt-tray/Sources/WorktreePanelView.swift @@ -120,7 +120,6 @@ enum TriageLabels { struct WorktreePanelView: View { @StateObject private var controller: WorktreePanelController - @State private var reviewing: TriageRow? @State private var confirmingDisposeAnyway: TriageRow? @State private var keptOpen: Bool /// The verb each busy row is running, so its button can say what it is doing. @@ -147,10 +146,7 @@ struct WorktreePanelView: View { .background(WT.window) .environment(\.triageSnapshot, isSnapshot) .onAppear { controller.startPolling() } - .onDisappear { controller.stopPolling() } - .sheet(item: $reviewing) { row in - WorktreeReviewSheet(row: row, controller: controller) { inFlight[row.id] = $0 } - } + .onDisappear { controller.stopPolling(); WorktreeReviewWindow.shared.close() } .alert(confirmingDisposeAnyway.map(TriageConfirm.disposeAnywayTitle) ?? "", isPresented: Binding(get: { confirmingDisposeAnyway != nil }, set: { if !$0 { confirmingDisposeAnyway = nil } }), @@ -285,7 +281,7 @@ struct WorktreePanelView: View { switch action { case "dispose": inFlight[r.id] = "dispose"; controller.dispose(r) case "dispose-anyway": confirmingDisposeAnyway = r - case "review": reviewing = r + case "review": WorktreeReviewWindow.shared.show(r, controller: controller) { inFlight[r.id] = $0 } case "push-branch": inFlight[r.id] = "push-branch"; controller.pushBranch(r) case "keep": inFlight[r.id] = "keep"; controller.keep(r) case "unkeep": inFlight[r.id] = "unkeep"; controller.unkeep(r) diff --git a/rt-tray/Sources/WorktreeReviewSheet.swift b/rt-tray/Sources/WorktreeReviewSheet.swift index 918548671..b94e795a5 100644 --- a/rt-tray/Sources/WorktreeReviewSheet.swift +++ b/rt-tray/Sources/WorktreeReviewSheet.swift @@ -5,16 +5,19 @@ struct WorktreeReviewSheet: View { let row: TriageRow @ObservedObject var controller: WorktreePanelController let onStart: (String) -> Void - @Environment(\.dismiss) private var dismiss + let onClose: () -> Void @Environment(\.triageSnapshot) private var isSnapshot @State private var load: DiffLoadState + @State private var expanded: Set init(row: TriageRow, controller: WorktreePanelController, initialLoad: TriageDiffLoad? = nil, - onStart: @escaping (String) -> Void = { _ in }) { + expanded: Set = [], onStart: @escaping (String) -> Void = { _ in }, onClose: @escaping () -> Void = {}) { self.row = row self.controller = controller self.onStart = onStart + self.onClose = onClose _load = State(initialValue: initialLoad.map(DiffLoadState.init) ?? .loading) + _expanded = State(initialValue: expanded) } private var subtitle: String { @@ -43,25 +46,25 @@ struct WorktreeReviewSheet: View { .frame(maxWidth: .infinity, alignment: .leading) .padding(.horizontal, 20).padding(.top, 20).padding(.bottom, 14) SheetRule() - content + content.frame(maxHeight: .infinity, alignment: .top) SheetRule() HStack(spacing: 8) { Text("Discarded files stay in the trash for 14 days.") .font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) .lineLimit(1).layoutPriority(-1) Spacer(minLength: 8) - Button("Keep") { onStart("keep"); controller.keep(row); dismiss() } + Button("Keep") { onStart("keep"); controller.keep(row); onClose() } .buttonStyle(TriageButtonStyle()) - Button("Commit and push") { onStart("push-branch"); controller.pushBranch(row, commitDirty: true); dismiss() } + Button("Commit and push") { onStart("push-branch"); controller.pushBranch(row, commitDirty: true); onClose() } .buttonStyle(TriageButtonStyle()) .disabled(!loaded) - Button("Discard and dispose") { onStart("dispose"); controller.dispose(row, discard: "all"); dismiss() } + Button("Discard and dispose") { onStart("dispose"); controller.dispose(row, discard: "all"); onClose() } .buttonStyle(TriageButtonStyle(primary: true)) .disabled(!loaded) } .padding(.horizontal, 20).padding(.vertical, 12) } - .frame(width: 680) + .frame(minWidth: 560, maxWidth: .infinity, minHeight: 360, maxHeight: .infinity, alignment: .top) .background(WT.card) .task { if case .loading = load { await reload() } @@ -93,7 +96,6 @@ struct WorktreeReviewSheet: View { fileList(files, truncatedFiles: truncatedFiles, lazy: false) } else { ScrollView { fileList(files, truncatedFiles: truncatedFiles, lazy: true) } - .frame(minHeight: 280, maxHeight: 520) } } } @@ -104,35 +106,45 @@ struct WorktreeReviewSheet: View { } /// Flat rows, so the live `LazyVStack` only builds the lines on screen. + /// Only a file's diff body sits on the neutral fill; headers stay on the card. private func fileList(_ files: [ParsedDiffFile], truncatedFiles: Bool, lazy: Bool) -> some View { DiffStack(lazy: lazy) { ForEach(files) { f in - HStack(spacing: 8) { - Image(systemName: f.file.status == "untracked" ? "doc.badge.plus" : "doc.text") - .foregroundStyle(WT.textSecondary) - Text(Self.shortPath(f.file.path)).font(.system(size: 12.5, design: .monospaced)).foregroundStyle(WT.text) - .lineLimit(1).truncationMode(.middle) - Spacer(minLength: 12) - Text(Self.stat(f.file)).font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) + let open = expanded.contains(f.id) + DiffFileHeader(file: f.file, open: open) { toggle(f.id, in: files) } + if open { + SheetRule() + WT.neutralFill.frame(height: 12) + ForEach(f.lines) { DiffLineRow(line: $0) } + if let more = f.moreLines { + Text(more).font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) + .padding(.horizontal, 20).padding(.top, 6) + .frame(maxWidth: .infinity, alignment: .leading) + .background(WT.neutralFill) + } + WT.neutralFill.frame(height: 12) } - .padding(.horizontal, 20).padding(.vertical, 9) SheetRule() - Color.clear.frame(height: 12) - ForEach(f.lines) { DiffLineRow(line: $0) } - if let more = f.moreLines { - Text(more).font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) - .padding(.horizontal, 20).padding(.top, 6) - } - Color.clear.frame(height: 12) } if truncatedFiles { - SheetRule() Text("More files not shown.").font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) .padding(.horizontal, 20).padding(.vertical, 10) } } + .padding(.bottom, 16) .frame(maxWidth: .infinity, alignment: .leading) - .background(WT.neutralFill) + } + + /// Option-click opens or closes every file, as a Finder disclosure does. + private func toggle(_ id: String, in files: [ParsedDiffFile]) { + let opening = !expanded.contains(id) + if NSEvent.modifierFlags.contains(.option) { + expanded = opening ? Set(files.map(\.id)) : [] + } else if opening { + expanded.insert(id) + } else { + expanded.remove(id) + } } static func shortPath(_ path: String) -> String { @@ -242,6 +254,38 @@ struct DiffLine: Identifiable { } } +private struct DiffFileHeader: View { + let file: TriageDiffFile + let open: Bool + let toggle: () -> Void + @State private var hovered = false + + var body: some View { + Button(action: toggle) { + HStack(spacing: 8) { + Image(systemName: "chevron.right") + .font(.system(size: 10, weight: .semibold)) + .rotationEffect(.degrees(open ? 90 : 0)) + .animation(.easeOut(duration: 0.15), value: open) + .foregroundStyle(WT.textTertiary) + .frame(width: 12) + Image(systemName: file.status == "untracked" ? "doc.badge.plus" : "doc.text") + .foregroundStyle(WT.textSecondary) + Text(WorktreeReviewSheet.shortPath(file.path)).font(.system(size: 12.5, design: .monospaced)).foregroundStyle(WT.text) + .lineLimit(1).truncationMode(.middle) + Spacer(minLength: 12) + Text(WorktreeReviewSheet.stat(file)).font(.system(size: 12.5)).foregroundStyle(WT.textTertiary) + } + .padding(.leading, 14).padding(.trailing, 20).padding(.vertical, 9) + .frame(maxWidth: .infinity, alignment: .leading) + .background(hovered ? WT.cardHover : WT.card) + .contentShape(Rectangle()) + } + .buttonStyle(.plain) + .onHover { hovered = $0 } + } +} + private struct DiffLineRow: View { let line: DiffLine var body: some View { @@ -249,6 +293,8 @@ private struct DiffLineRow: View { Text(line.text) .font(.system(size: 12, design: .monospaced)).foregroundStyle(WT.textTertiary) .padding(.leading, 20).padding(.vertical, 3) + .frame(maxWidth: .infinity, alignment: .leading) + .background(WT.neutralFill) } else { HStack(alignment: .firstTextBaseline, spacing: 0) { Text(line.number.map(String.init) ?? "") @@ -263,7 +309,8 @@ private struct DiffLineRow: View { } .font(.system(size: 12.5, design: .monospaced)) .padding(.leading, 18).padding(.trailing, 20) - .frame(height: 18) + .frame(maxWidth: .infinity, minHeight: 18, maxHeight: 18, alignment: .leading) + .background(WT.neutralFill) } } } diff --git a/rt-tray/Sources/WorktreeReviewWindow.swift b/rt-tray/Sources/WorktreeReviewWindow.swift new file mode 100644 index 000000000..404beefe8 --- /dev/null +++ b/rt-tray/Sources/WorktreeReviewWindow.swift @@ -0,0 +1,45 @@ +import AppKit +import SwiftUI +import MattstackCore + +/// Review opens in its own resizable window rather than a sheet, so a long +/// diff is not capped by the Worktrees window's size. One window serves every +/// row: reviewing another row retargets it and keeps the user's frame. +@MainActor +final class WorktreeReviewWindow { + static let shared = WorktreeReviewWindow() + private var window: NSWindow? + private var host: NSHostingController? + + func show(_ row: TriageRow, controller: WorktreePanelController, onStart: @escaping (String) -> Void) { + // `.id` resets the review's loaded diff when the same window is retargeted. + let root = AnyView(WorktreeReviewSheet(row: row, controller: controller, onStart: onStart, + onClose: { [weak self] in self?.close() }).id(row.id)) + let w = window ?? make() + host?.rootView = root + w.title = "Review \(row.tree)" + w.makeKeyAndOrderFront(nil) + NSApp.activate(ignoringOtherApps: true) + } + + func close() { window?.close() } + + private func make() -> NSWindow { + let w = NSWindow(contentRect: NSRect(x: 0, y: 0, width: 960, height: 760), + styleMask: [.titled, .closable, .resizable, .miniaturizable], backing: .buffered, defer: false) + w.backgroundColor = WT.cardNS + w.titlebarAppearsTransparent = true + w.titleVisibility = .hidden + let host = NSHostingController(rootView: AnyView(EmptyView())) + host.sizingOptions = [.minSize] + w.contentViewController = host + // Same shrink-to-fitting-size trap as `detachProcessPanel`. + w.setContentSize(NSSize(width: 960, height: 760)) + w.center() + w.setFrameAutosaveName("rt-worktree-review") + w.isReleasedWhenClosed = false + window = w + self.host = host + return w + } +} diff --git a/rt-tray/Sources/WorktreeSnapshot.swift b/rt-tray/Sources/WorktreeSnapshot.swift index 758d5eb6d..8151a75c9 100644 --- a/rt-tray/Sources/WorktreeSnapshot.swift +++ b/rt-tray/Sources/WorktreeSnapshot.swift @@ -3,7 +3,7 @@ import SwiftUI import MattstackCore /// `rt-tray --render-worktree-snapshots ` renders the -/// Worktrees panel, the Review sheet and the interaction states from fixture +/// Worktrees panel, the Review window (collapsed and expanded) and the interaction states from fixture /// JSON, light and dark, then returns true so the caller exits before any /// window, status item, socket or daemon work exists. DEBUG builds only. enum WorktreeSnapshot { @@ -39,10 +39,14 @@ enum WorktreeSnapshot { render(WorktreeReviewSheet(row: look, controller: WorktreePanelController(fixture: catalog), initialLoad: TriageDiffLoad(files: diff, truncatedFiles: false)), width: 680, scheme, out.appendingPathComponent("review-sheet-\(tag).png")) + render(WorktreeReviewSheet(row: look, controller: WorktreePanelController(fixture: catalog), + initialLoad: TriageDiffLoad(files: diff, truncatedFiles: false), + expanded: [diff[1].path]), + width: 680, scheme, out.appendingPathComponent("review-sheet-expanded-\(tag).png")) render(InteractionStatesSnapshot(row: voldemort), width: 1180, scheme, out.appendingPathComponent("interaction-states-\(tag).png")) } - print("wrote 8 snapshots to \(out.path)") + print("wrote 10 snapshots to \(out.path)") return true #else return false diff --git a/rt-tray/Sources/WorktreeTokens.swift b/rt-tray/Sources/WorktreeTokens.swift index 81f89f18b..34b81f117 100644 --- a/rt-tray/Sources/WorktreeTokens.swift +++ b/rt-tray/Sources/WorktreeTokens.swift @@ -17,7 +17,8 @@ enum WT { static let windowNS = ns(0xf6f6f4, 0x1c1c1e) static let window = Color(nsColor: windowNS) - static let card = c(0xffffff, 0x252528) + static let cardNS = ns(0xffffff, 0x252528) + static let card = Color(nsColor: cardNS) static let cardHover = c(0xf7f7f5, 0x2c2c30) static let border = c(0xe2e2de, 0x34343a) static let borderStrong = c(0xcfcfca, 0x45454c) diff --git a/rt-tray/Tests/fixtures/worktree-triage-diff.json b/rt-tray/Tests/fixtures/worktree-triage-diff.json index 7438013e4..32bf993f1 100644 --- a/rt-tray/Tests/fixtures/worktree-triage-diff.json +++ b/rt-tray/Tests/fixtures/worktree-triage-diff.json @@ -2,10 +2,28 @@ { "path": "apps/backend/src/platform/auth/__tests__/Auth0User.evidence.test.ts", "status": "untracked", - "diff": "import { execute, parse } from 'graphql16';\nimport { graphqlSchema } from '~/src/graphql-schema/schema';\nimport { getUser } from '~/src/platform/auth/auth0';\n\njest.mock('~/src/platform/auth/auth0', () => ({\n getUser: jest.fn(),\n}));\n\ndescribe('Auth0User per-field lookups', () => {\n it('shares one lookup across parity cases', async () => {\n …", + "diff": "import { execute, parse } from 'graphql16';\nimport { graphqlSchema } from '~/src/graphql-schema/schema';\nimport { getUser } from '~/src/platform/auth/auth0';\n\njest.mock('~/src/platform/auth/auth0', () => ({\n getUser: jest.fn(),\n}));\n\ndescribe('Auth0User per-field lookups', () => {\n it('shares one lookup across parity cases', async () => {\n \u2026", "truncated": true, "added": 97, "removed": 0, "totalLines": 97 + }, + { + "path": "apps/backend/src/platform/auth/auth0.ts", + "status": "modified", + "diff": "diff --git a/apps/backend/src/platform/auth/auth0.ts b/apps/backend/src/platform/auth/auth0.ts\nindex 3f2a1b7..9c4d2e0 100644\n--- a/apps/backend/src/platform/auth/auth0.ts\n+++ b/apps/backend/src/platform/auth/auth0.ts\n@@ -41,7 +41,9 @@ export async function getUser(id: string) {\n const client = managementClient();\n- const user = await client.users.get({ id });\n+ const user = await client.users.get({ id }).catch((err) => {\n+ throw new UserLookupError(id, err);\n+ });\n if (!user) {\n return null;\n }\n", + "truncated": false, + "added": 3, + "removed": 1, + "totalLines": null + }, + { + "path": ".scratch/notes.md", + "status": "untracked", + "diff": "Auth0 per-field lookups\n\n- one getUser call per request\n- fields resolve from the cached user\n", + "truncated": false, + "added": 4, + "removed": 0, + "totalLines": 4 } ] From abd0c146771cc639d9ffe872ae8bce6c375c672f Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 25 Sep 2026 09:04:36 -0500 Subject: [PATCH 2/3] tray: keep a worktree row busy until the list refresh lands A finished action cleared the row's busy state on the daemon's reply, then waited seconds for the triage query, so a disposed row sat at rest looking untouched. Rows now stay busy until a query started after the action applies (or fails), via TriageSettleLedger. Clean up N safe does the same and skips rows already busy. Co-Authored-By: Claude Opus 5.5 (1M context) --- rt-tray/Sources-core/Worktree/Triage.swift | 18 ++++++++ rt-tray/Sources/WorktreePanelController.swift | 41 ++++++++++++++----- .../MattstackCoreChecks/TriageChecks.swift | 10 +++++ 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/rt-tray/Sources-core/Worktree/Triage.swift b/rt-tray/Sources-core/Worktree/Triage.swift index 72f3bf781..43ea79965 100644 --- a/rt-tray/Sources-core/Worktree/Triage.swift +++ b/rt-tray/Sources-core/Worktree/Triage.swift @@ -236,6 +236,24 @@ public struct TriageQueryGate: Sendable { } } +/// Work held until a query started after it lands. Applied rows answer every +/// earlier waiter too, since they are at least as new; a failed query answers +/// only its own, so nothing waits on a reply that will never come. +public struct TriageSettleLedger { + private var waiting: [(ticket: Int, value: Value)] = [] + + public init() {} + + public mutating func wait(_ ticket: Int, _ value: Value) { waiting.append((ticket, value)) } + + public mutating func settle(_ ticket: Int, applied: Bool) -> [Value] { + let due = { (w: (ticket: Int, value: Value)) in applied ? w.ticket <= ticket : w.ticket == ticket } + let out = waiting.filter(due).map(\.value) + waiting.removeAll(where: due) + return out + } +} + /// Daemon refusal codes as the tail of a ": ..." status line. Codes /// may carry a ":" suffix; anything unrecognised is shown verbatim. public enum TriageRefusal { diff --git a/rt-tray/Sources/WorktreePanelController.swift b/rt-tray/Sources/WorktreePanelController.swift index 51d8e291f..121447d0c 100644 --- a/rt-tray/Sources/WorktreePanelController.swift +++ b/rt-tray/Sources/WorktreePanelController.swift @@ -61,6 +61,10 @@ final class WorktreePanelController: ObservableObject { private var statusGeneration = 0 private var hasLoaded = false private var queryGate = TriageQueryGate() + /// A finished action keeps its row busy until a query started after it + /// lands; clearing on the daemon's reply leaves a disposed row sitting at + /// rest for the seconds the triage query takes. + private var settling = TriageSettleLedger<() -> Void>() private let fixture: TriageData? init(fixture: TriageData? = nil) { @@ -83,12 +87,16 @@ final class WorktreePanelController: ObservableObject { /// A failed background poll keeps the last rows and says nothing, so it /// never overwrites the footer an action just set. `force` starts a query /// even while a poll is in flight, so a finished action's rows are never - /// left to a poll that began before it. - func refresh(userInitiated: Bool = false, force: Bool = false) { - guard fixture == nil, let ticket = queryGate.begin(force: userInitiated || force) else { return } + /// left to a poll that began before it. `settled` runs in the same update + /// that applies this query's rows (or a newer query's), or once it fails. + func refresh(userInitiated: Bool = false, force: Bool = false, settled: (() -> Void)? = nil) { + guard fixture == nil, let ticket = queryGate.begin(force: userInitiated || force) else { settled?(); return } + if let settled { settling.wait(ticket, settled) } Task { let p = await client.queryTriage() - guard queryGate.finish(ticket, succeeded: p?.data != nil) else { return } + let current = queryGate.finish(ticket, succeeded: p?.data != nil) + defer { settling.settle(ticket, applied: current && p?.data != nil).forEach { $0() } } + guard current else { return } isLoading = false guard let data = p?.data else { if userInitiated || !hasLoaded { @@ -125,10 +133,14 @@ final class WorktreePanelController: ObservableObject { busy.insert(row.id) Task { let (outcome, trashPath) = await performReply(verb, row, payload) - busy.remove(row.id) let line = TriageStatusLine.action(tree: row.tree, outcome: outcome, done: done(trashPath)) setStatus(line.text, isError: line.isError) - refresh(force: true) + if outcome == .done { + refresh(force: true) { [weak self] in self?.busy.remove(row.id) } + } else { + busy.remove(row.id) + refresh(force: true) + } } } @@ -154,22 +166,29 @@ final class WorktreePanelController: ObservableObject { /// One at a time, so the footer and the button can report "1 of 2" and a /// refusal on one row doesn't hide behind the others. func cleanUpSafe() { - let safe = rows.filter { $0.group == "safe" } + let safe = rows.filter { $0.group == "safe" && !busy.contains($0.id) } guard !safe.isEmpty, bulkProgress == nil else { return } bulkProgress = (0, safe.count) Task { var failures: [(tree: String, outcome: TriageActionOutcome)] = [] + var disposed: [String] = [] for (i, row) in safe.enumerated() { busy.insert(row.id) bulkProgress = (i, safe.count) let outcome = await perform("worktree:triage-dispose", row, ["fingerprint": row.fingerprint.jsonObject, "discard": "classified"]) - busy.remove(row.id) - if outcome != .done { failures.append((tree: row.tree, outcome: outcome)) } + if outcome == .done { + disposed.append(row.id) + } else { + busy.remove(row.id) + failures.append((tree: row.tree, outcome: outcome)) + } } - bulkProgress = nil let line = TriageStatusLine.bulk(total: safe.count, failures: failures) setStatus(line.text, isError: line.isError) - refresh(force: true) + refresh(force: true) { [weak self] in + self?.busy.subtract(disposed) + self?.bulkProgress = nil + } } } diff --git a/rt-tray/Tests/MattstackCoreChecks/TriageChecks.swift b/rt-tray/Tests/MattstackCoreChecks/TriageChecks.swift index 5403d27c1..018ba47d6 100644 --- a/rt-tray/Tests/MattstackCoreChecks/TriageChecks.swift +++ b/rt-tray/Tests/MattstackCoreChecks/TriageChecks.swift @@ -96,6 +96,16 @@ let triageChecks: [Check] = [ c.expectEqual(gate.finish(3, succeeded: false), true) c.expectEqual(gate.begin(force: false), 4) }, + Check("a finished action stays busy until a later query lands; applied rows release earlier waiters, a failure only its own") { c in + var ledger = TriageSettleLedger() + ledger.wait(2, "neville") + ledger.wait(3, "olive") + ledger.wait(4, "smaug") + c.expectEqual(ledger.settle(1, applied: true), []) + c.expectEqual(ledger.settle(4, applied: false), ["smaug"]) + c.expectEqual(ledger.settle(3, applied: true), ["neville", "olive"]) + c.expectEqual(ledger.settle(2, applied: false), []) + }, Check("dispose anyway asks first, naming the tree and its unpushed commits") { c in let rows = try JSONDecoder().decode(TriagePayload.self, from: Data(json.utf8)).data!.rows let neville = rows[0] From 83be7a8aa9abe8fcac85955ed31b2586dbc4e23c Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 25 Sep 2026 09:45:56 -0500 Subject: [PATCH 3/3] tray: address review of the review window and settle hold - a fresh identity per Review show, and teardown on close, so the same row reviewed again reloads its diff instead of acting on the old one - close the review window from the panel's willClose (onDisappear never fires for the reused panel window) - disable the review buttons while the row is busy or a bulk clean runs - let loading/failed states shrink with the window - a settle waiter always forces its query; bulk skips busy rows in the view too; catalog fixture lists charlie's three files Co-Authored-By: Claude Opus 5.5 (1M context) --- rt-tray/Sources/AppDelegate.swift | 4 ++++ rt-tray/Sources/WorktreePanelController.swift | 6 ++---- rt-tray/Sources/WorktreePanelView.swift | 6 ++++-- rt-tray/Sources/WorktreeReviewSheet.swift | 11 ++++++----- rt-tray/Sources/WorktreeReviewWindow.swift | 17 +++++++++++------ rt-tray/Sources/WorktreeSnapshot.swift | 2 +- .../Tests/fixtures/worktree-triage-catalog.json | 6 ++++-- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/rt-tray/Sources/AppDelegate.swift b/rt-tray/Sources/AppDelegate.swift index 1738557ab..eeda281d8 100644 --- a/rt-tray/Sources/AppDelegate.swift +++ b/rt-tray/Sources/AppDelegate.swift @@ -1467,6 +1467,10 @@ class AppDelegate: NSObject, NSApplicationDelegate, @unchecked Sendable { w.center() w.setFrameAutosaveName("rt-worktree-panel") w.isReleasedWhenClosed = false + // The panel's hosting view outlives the window, so its onDisappear never fires on close. + NotificationCenter.default.addObserver(forName: NSWindow.willCloseNotification, object: w, queue: .main) { _ in + MainActor.assumeIsolated { WorktreeReviewWindow.shared.close() } + } worktreeWindow = w w.makeKeyAndOrderFront(nil) NSApp.activate(ignoringOtherApps: true) diff --git a/rt-tray/Sources/WorktreePanelController.swift b/rt-tray/Sources/WorktreePanelController.swift index 121447d0c..b3dc12dcf 100644 --- a/rt-tray/Sources/WorktreePanelController.swift +++ b/rt-tray/Sources/WorktreePanelController.swift @@ -61,9 +61,7 @@ final class WorktreePanelController: ObservableObject { private var statusGeneration = 0 private var hasLoaded = false private var queryGate = TriageQueryGate() - /// A finished action keeps its row busy until a query started after it - /// lands; clearing on the daemon's reply leaves a disposed row sitting at - /// rest for the seconds the triage query takes. + /// A finished action keeps its row busy until a query started after it lands. private var settling = TriageSettleLedger<() -> Void>() private let fixture: TriageData? @@ -90,7 +88,7 @@ final class WorktreePanelController: ObservableObject { /// left to a poll that began before it. `settled` runs in the same update /// that applies this query's rows (or a newer query's), or once it fails. func refresh(userInitiated: Bool = false, force: Bool = false, settled: (() -> Void)? = nil) { - guard fixture == nil, let ticket = queryGate.begin(force: userInitiated || force) else { settled?(); return } + guard fixture == nil, let ticket = queryGate.begin(force: userInitiated || force || settled != nil) else { settled?(); return } if let settled { settling.wait(ticket, settled) } Task { let p = await client.queryTriage() diff --git a/rt-tray/Sources/WorktreePanelView.swift b/rt-tray/Sources/WorktreePanelView.swift index 83e6ca370..d4aec69c5 100644 --- a/rt-tray/Sources/WorktreePanelView.swift +++ b/rt-tray/Sources/WorktreePanelView.swift @@ -146,7 +146,7 @@ struct WorktreePanelView: View { .background(WT.window) .environment(\.triageSnapshot, isSnapshot) .onAppear { controller.startPolling() } - .onDisappear { controller.stopPolling(); WorktreeReviewWindow.shared.close() } + .onDisappear { controller.stopPolling() } .alert(confirmingDisposeAnyway.map(TriageConfirm.disposeAnywayTitle) ?? "", isPresented: Binding(get: { confirmingDisposeAnyway != nil }, set: { if !$0 { confirmingDisposeAnyway = nil } }), @@ -199,7 +199,9 @@ struct WorktreePanelView: View { TriageBulkButton(safe: progress.1, progress: progress) {} } else if let safe = controller.counts?.safe, safe > 0 { TriageBulkButton(safe: safe, progress: nil) { - for row in controller.rows where row.group == "safe" { inFlight[row.id] = "dispose" } + for row in controller.rows where row.group == "safe" && !controller.busy.contains(row.id) { + inFlight[row.id] = "dispose" + } controller.cleanUpSafe() } } diff --git a/rt-tray/Sources/WorktreeReviewSheet.swift b/rt-tray/Sources/WorktreeReviewSheet.swift index b94e795a5..74bdfe370 100644 --- a/rt-tray/Sources/WorktreeReviewSheet.swift +++ b/rt-tray/Sources/WorktreeReviewSheet.swift @@ -36,6 +36,7 @@ struct WorktreeReviewSheet: View { private var loaded: Bool { if case .loaded = load { return true } else { return false } } + private var rowBusy: Bool { controller.busy.contains(row.id) || controller.bulkProgress != nil } var body: some View { VStack(alignment: .leading, spacing: 0) { @@ -55,12 +56,13 @@ struct WorktreeReviewSheet: View { Spacer(minLength: 8) Button("Keep") { onStart("keep"); controller.keep(row); onClose() } .buttonStyle(TriageButtonStyle()) + .disabled(rowBusy) Button("Commit and push") { onStart("push-branch"); controller.pushBranch(row, commitDirty: true); onClose() } .buttonStyle(TriageButtonStyle()) - .disabled(!loaded) + .disabled(!loaded || rowBusy) Button("Discard and dispose") { onStart("dispose"); controller.dispose(row, discard: "all"); onClose() } .buttonStyle(TriageButtonStyle(primary: true)) - .disabled(!loaded) + .disabled(!loaded || rowBusy) } .padding(.horizontal, 20).padding(.vertical, 12) } @@ -77,7 +79,7 @@ struct WorktreeReviewSheet: View { Text("Loading changes…") .font(.system(size: 13)).foregroundStyle(WT.textTertiary) .padding(20) - .frame(maxWidth: .infinity, minHeight: 280, alignment: .topLeading) + .frame(maxWidth: .infinity, alignment: .topLeading) case .failed: HStack(spacing: 12) { Text("Couldn't load the changes.").font(.system(size: 13)).foregroundStyle(WT.textSecondary) @@ -85,7 +87,7 @@ struct WorktreeReviewSheet: View { Spacer(minLength: 0) } .padding(20) - .frame(maxWidth: .infinity, minHeight: 280, alignment: .topLeading) + .frame(maxWidth: .infinity, alignment: .topLeading) case .loaded(let files, _) where files.isEmpty: Text("No uncommitted changes left to show.") .font(.system(size: 13)).foregroundStyle(WT.textSecondary) @@ -106,7 +108,6 @@ struct WorktreeReviewSheet: View { } /// Flat rows, so the live `LazyVStack` only builds the lines on screen. - /// Only a file's diff body sits on the neutral fill; headers stay on the card. private func fileList(_ files: [ParsedDiffFile], truncatedFiles: Bool, lazy: Bool) -> some View { DiffStack(lazy: lazy) { ForEach(files) { f in diff --git a/rt-tray/Sources/WorktreeReviewWindow.swift b/rt-tray/Sources/WorktreeReviewWindow.swift index 404beefe8..8b7c759a6 100644 --- a/rt-tray/Sources/WorktreeReviewWindow.swift +++ b/rt-tray/Sources/WorktreeReviewWindow.swift @@ -2,19 +2,19 @@ import AppKit import SwiftUI import MattstackCore -/// Review opens in its own resizable window rather than a sheet, so a long -/// diff is not capped by the Worktrees window's size. One window serves every -/// row: reviewing another row retargets it and keeps the user's frame. +/// One window serves every row: reviewing another row retargets it and keeps +/// the user's frame. @MainActor -final class WorktreeReviewWindow { +final class WorktreeReviewWindow: NSObject, NSWindowDelegate { static let shared = WorktreeReviewWindow() private var window: NSWindow? private var host: NSHostingController? func show(_ row: TriageRow, controller: WorktreePanelController, onStart: @escaping (String) -> Void) { - // `.id` resets the review's loaded diff when the same window is retargeted. + // A fresh identity per show, never `row.id`: reviewing the same row + // again must reload its diff, or Discard would act on files it never showed. let root = AnyView(WorktreeReviewSheet(row: row, controller: controller, onStart: onStart, - onClose: { [weak self] in self?.close() }).id(row.id)) + onClose: { [weak self] in self?.close() }).id(UUID())) let w = window ?? make() host?.rootView = root w.title = "Review \(row.tree)" @@ -24,6 +24,10 @@ final class WorktreeReviewWindow { func close() { window?.close() } + func windowWillClose(_ notification: Notification) { + host?.rootView = AnyView(EmptyView()) + } + private func make() -> NSWindow { let w = NSWindow(contentRect: NSRect(x: 0, y: 0, width: 960, height: 760), styleMask: [.titled, .closable, .resizable, .miniaturizable], backing: .buffered, defer: false) @@ -38,6 +42,7 @@ final class WorktreeReviewWindow { w.center() w.setFrameAutosaveName("rt-worktree-review") w.isReleasedWhenClosed = false + w.delegate = self window = w self.host = host return w diff --git a/rt-tray/Sources/WorktreeSnapshot.swift b/rt-tray/Sources/WorktreeSnapshot.swift index 8151a75c9..130b6b4c8 100644 --- a/rt-tray/Sources/WorktreeSnapshot.swift +++ b/rt-tray/Sources/WorktreeSnapshot.swift @@ -3,7 +3,7 @@ import SwiftUI import MattstackCore /// `rt-tray --render-worktree-snapshots ` renders the -/// Worktrees panel, the Review window (collapsed and expanded) and the interaction states from fixture +/// Worktrees panel, the Review window (collapsed, expanded) and the interaction states from fixture /// JSON, light and dark, then returns true so the caller exits before any /// window, status item, socket or daemon work exists. DEBUG builds only. enum WorktreeSnapshot { diff --git a/rt-tray/Tests/fixtures/worktree-triage-catalog.json b/rt-tray/Tests/fixtures/worktree-triage-catalog.json index ba0594f44..3e76b4537 100644 --- a/rt-tray/Tests/fixtures/worktree-triage-catalog.json +++ b/rt-tray/Tests/fixtures/worktree-triage-catalog.json @@ -215,11 +215,13 @@ "dirt": { "kind": "real", "files": [ - "apps/backend/src/platform/auth/__tests__/Auth0User.evidence.test.ts" + "apps/backend/src/platform/auth/__tests__/Auth0User.evidence.test.ts", + "apps/backend/src/platform/auth/auth0.ts", + ".scratch/notes.md" ] }, "group": "look", - "verdict": "One uncommitted file: Auth0User.evidence.test.ts. Review it before disposing.", + "verdict": "3 uncommitted files: Auth0User.evidence.test.ts, auth0.ts. Review before disposing.", "actions": [ "review", "keep",