From da273d91cfe6b266f451ba8430ee7b5f66a29e76 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Thu, 30 Jul 2026 15:01:46 +0700 Subject: [PATCH 1/3] An app that does not get slower the longer it is open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five places where the cost of the app grew with how long it had been running, or with how many things were left open in it. None of them is a feature; all of them are the same shape. **Every ghostty callback took a hop to the main actor.** A shell painting a spinner — every `claude` mid-turn — asks to render every frame whether or not anyone has that tab on screen, and wakes the app several times per frame besides. Both are gathered on ghostty's own thread now (`RenderQueue` and `Latch`), and one hop marks everything that arrived while it was waiting its turn. The latch is cleared at the start of the scheduled work, not the end, so a request landing mid-drain schedules the next pass rather than being swallowed by this one. **A tab title was published on every rename.** `claude` paints its spinner into the OSC title, so a running turn was a new name every frame — and each one was a write to an observable property, which redraws the terminals list, the tab bar and the viewer header. `titleArrived` separates what a name means from the text of it: `isWorking` is read at once and written only when it changes, the text lands on a 200 ms tick, and the last name of a burst is always published so a tab never settles showing half a spinner. **Language servers were never stopped.** Nothing let one go short of removing its repository from the sidebar, and a dashboard prewarms up to `prewarmLimit` of them before a file has been opened — a day across eight repositories left every server of all eight resident, a few hundred megabytes each and more for sourcekit-lsp. A server holding no documents for fifteen minutes is now shut down and forgotten. There is no sweep on a clock: a server can only become idle at the moment its last document closes, so the wait is put on then. `shutdown()` also clears the remembered handshake, without which a service stopped for being idle could never start again. **The five-minute sweep started one task per repository.** One read is a handful of processes — `git status`, `git log`, `gh pr list` — so a sidebar of a dozen fired forty-odd of them at once onto a pool with a thread per core, which is a hitch across the window rather than work happening behind it. Three at a time, one in one out, and the sweep in flight is cancelled by the next so a hand refresh landing on the tick no longer reads everything twice. The auto-refresh loop also sleeps for what is left of the interval instead of waking every minute to find there is nothing to do. **A pull request's builds were read every ten seconds for as long as it was open.** A run still going is what the person is sitting there for, so the first minute stays at ten seconds and eases off to thirty and then sixty. Once every run has a verdict there is no news left but a push, and that is what the sidebar's own sweep is paced for — so a settled pull request drops to thirty seconds and then five minutes. A new run appearing restarts the count, which is what `PullRequestBuild.State.isSettled` is for; `unknown` is deliberately not settled, since it is a word this app failed to read rather than a host saying the run is over. **And one thing you can see:** a terminal's header puts the weight on the repository rather than on the shell's own name. Everywhere else the item's title is the thing being read, but a terminal's title is whatever `claude` last renamed the tab to — a spinner frame as often as a word — while the repository is what tells three open shells apart. The two keep their places and trade their emphasis. `Docs/LSP.md` and `Docs/Terminal.md` record the two decisions worth recording: when a server is allowed to go, and why neither ghostty callback nor a tab title travels straight to the main actor. Checked: `Scripts/bundle.sh` — build succeeded. Not measured under a profiler, and not watched over a working day, which is the timescale four of these are about. Co-Authored-By: Claude Opus 5 (1M context) --- Docs/LSP.md | 14 ++- Docs/Terminal.md | 18 ++++ .../LSP/LanguageServerRegistry.swift | 67 ++++++++++++- Sources/Workspace/LSP/LanguageService.swift | 38 +++++++- .../Workspace/Models/PullRequestBuild.swift | 13 +++ .../Workspace/Models/TerminalSession.swift | 73 +++++++++++--- Sources/Workspace/Models/WorkspaceStore.swift | 63 +++++++++++-- .../Workspace/Terminal/GhosttyRuntime.swift | 94 ++++++++++++++++++- .../Views/PullRequestDetailView.swift | 40 +++++++- Sources/Workspace/Views/ViewerView.swift | 17 +++- 10 files changed, 403 insertions(+), 34 deletions(-) diff --git a/Docs/LSP.md b/Docs/LSP.md index 4004d7a..56c17a9 100644 --- a/Docs/LSP.md +++ b/Docs/LSP.md @@ -21,7 +21,8 @@ conversation with the server, and the join between the two. language. Owns the handshake, the open-document set and the version counters. - **`LSP/LanguageServerRegistry.swift`** — language → binary, one server per (root, language), started lazily the first time a matching file is opened and - prewarmed from the dashboard. + prewarmed from the dashboard. It is also what stops one again: see + **Servers do not stay forever** below. - **`LSP/LanguageServerConfiguration.swift`**, **`LanguageServerOptions.swift`**, **`ManagedLanguageServers.swift`** — which servers exist, their flags, and the ones the app can fetch for you. @@ -137,6 +138,17 @@ been fine; they were not why. ## Also worth knowing +- **Servers do not stay forever.** A server holding no documents for fifteen + minutes is shut down and forgotten by the registry — the node ones are a few + hundred megabytes each, sourcekit-lsp rather more, and a dashboard prewarms up + to `prewarmLimit` of them before a single file has been opened. Nothing used + to stop one short of removing its repository from the sidebar, so a day across + eight repositories left every server of all eight resident. `LanguageService` + records `idleSince` when its open-document count reaches zero and tells the + registry; there is no sweep on a clock, because a server can only become idle + at the moment its last document closes. Opening a file in that language starts + a fresh one, which is what already happens the first time — `shutdown()` + clears the remembered handshake so it can. - **Large files are excluded on purpose.** See `OpenDocument.largeFileNote`. `didOpen` sends the whole text, and the answer to a 3.5 MB minified bundle is a diagnostic per line of something nobody is editing. No coordinator is even diff --git a/Docs/Terminal.md b/Docs/Terminal.md index b79c703..bf55477 100644 --- a/Docs/Terminal.md +++ b/Docs/Terminal.md @@ -45,6 +45,24 @@ sixteen ANSI slots a terminal needs. ## Gotchas learned the hard way +- **Ghostty's callbacks arrive at frame rate, and a hop each is too many.** A + shell painting a spinner — every `claude` mid-turn — asks to render every + frame whether or not anyone is looking at that tab, and wakes the app several + times per frame besides. One `Task { @MainActor … }` per request meant the + cost of a window grew with each terminal left running in it. Both are gathered + on ghostty's own thread instead (`RenderQueue`, `Latch` at the foot of + `GhosttyRuntime.swift`), and one hop serves everything that arrived while it + was waiting its turn. The latch is cleared at the *start* of the scheduled + work, so a request landing mid-drain schedules the next pass rather than being + swallowed by this one. +- **A tab title is not published on every rename, for the same reason.** `claude` + paints its spinner into the OSC title, and each name used to be a write to an + observable property — which redraws the terminals list, the tab bar and the + viewer header. `TerminalSession.titleArrived` splits what a name *means* from + the text of it: `isWorking` (what raises the "finished" banner) is read at + once and only written when it changes, while the text lands on a 200 ms tick. + The last name of a burst is always published, so a tab never settles showing + half a spinner. - `ghostty_surface_text` is a **paste** (`completeClipboardPaste` in core). Under bracketed paste, a pasted `\n` never executes a command — that is why `TerminalSession.send` turns a trailing newline into `pressEnter()`, a diff --git a/Sources/Workspace/LSP/LanguageServerRegistry.swift b/Sources/Workspace/LSP/LanguageServerRegistry.swift index 9737d94..96b17de 100644 --- a/Sources/Workspace/LSP/LanguageServerRegistry.swift +++ b/Sources/Workspace/LSP/LanguageServerRegistry.swift @@ -63,7 +63,7 @@ final class LanguageServerRegistry { guard let entry = catalog.entry(forFile: url) ?? catalog.entry(for: language) else { return nil } let key = Self.key(root: root, language: entry.language) if let existing = services[key] { return existing } - let service = LanguageService(definition: Self.definition(for: entry), root: root) + let service = makeService(Self.definition(for: entry), root: root) services[key] = service return service } @@ -140,7 +140,7 @@ final class LanguageServerRegistry { let key = Self.key(root: root, language: entry.language) let service = services[key] - ?? LanguageService(definition: Self.definition(for: entry), root: root) + ?? makeService(Self.definition(for: entry), root: root) services[key] = service // Deliberately not awaited in turn: sourcekit-lsp can spend the // best part of a minute on its handshake, and the servers behind it @@ -163,7 +163,7 @@ final class LanguageServerRegistry { }) else { return nil } let key = Self.key(root: root, language: entry.language) if let existing = services[key] { return existing } - let service = LanguageService(definition: Self.definition(for: entry), root: root) + let service = makeService(Self.definition(for: entry), root: root) services[key] = service return service } @@ -217,6 +217,67 @@ final class LanguageServerRegistry { } } + // MARK: - Letting idle servers go + + /// How long a server may hold no documents before it is stopped. + /// + /// Nothing used to stop one short of removing its repository from the + /// sidebar, and a server is not a cheap thing to leave sitting there: the + /// node ones are a few hundred megabytes each, sourcekit-lsp rather more, + /// and a repository prewarms up to ``prewarmLimit`` of them the first time + /// its dashboard is looked at. A working day across eight repositories left + /// every server of all eight resident, which is the machine slowing down + /// rather than the app — but it is the app that is blamed for it. + /// + /// Long enough that reading two files in a language with a break between + /// them does not pay the handshake twice, and reading one file in a + /// repository visited by accident does not hold a server all afternoon. + private static let idleLimit: TimeInterval = 15 * 60 + + /// The one wait in flight. There is no sweep on a clock: a server can only + /// become idle by having its last document closed, so the wait is put on + /// when that happens and taken off when it is served. + @ObservationIgnored private var idleSweep: Task? + + /// Every service is made here, so every one of them reports going idle. + private func makeService(_ definition: LanguageServerDefinition, root: URL) -> LanguageService { + let service = LanguageService(definition: definition, root: root) + service.onIdle = { [weak self] in self?.scheduleIdleSweep() } + return service + } + + private func scheduleIdleSweep(in delay: TimeInterval = LanguageServerRegistry.idleLimit) { + guard idleSweep == nil else { return } + idleSweep = Task { [weak self] in + try? await Task.sleep(for: .seconds(delay)) + guard !Task.isCancelled, let self else { return } + shutdownIdleServices() + } + } + + /// Stops everything that has been idle long enough, and puts the wait back + /// on for whatever is idle but not yet due. + private func shutdownIdleServices() { + idleSweep = nil + let now = Date() + var soonest: TimeInterval? + + for (key, service) in services { + guard let idleSince = service.idleSince else { continue } + let idle = now.timeIntervalSince(idleSince) + if idle >= Self.idleLimit { + service.shutdown() + services[key] = nil + } else { + soonest = min(soonest ?? .greatestFiniteMagnitude, Self.idleLimit - idle) + } + } + + // A server that went idle while this wait was running has its own left + // to serve; scheduling for the nearest keeps one wait for all of them. + if let soonest { scheduleIdleSweep(in: soonest) } + } + private static func key(root: URL, language: String) -> String { "\(root.path)|\(language)" } diff --git a/Sources/Workspace/LSP/LanguageService.swift b/Sources/Workspace/LSP/LanguageService.swift index 6ca56e9..8765b0f 100644 --- a/Sources/Workspace/LSP/LanguageService.swift +++ b/Sources/Workspace/LSP/LanguageService.swift @@ -39,9 +39,32 @@ final class LanguageService { /// coordinator to decide whether it may send ranges or must send the file. private(set) var syncKind: LSP.SyncKind = .full + /// Since when this server has been holding no documents at all, or nil + /// while it is holding one. + /// + /// A language server is an expensive thing to leave running — a few hundred + /// megabytes each for the node ones, more for sourcekit-lsp — and nothing + /// used to stop one until the repository it belongs to was removed from the + /// sidebar. A day of looking at eight repositories left every server of all + /// eight resident. ``LanguageServerRegistry`` reads this and stops the ones + /// that have been idle long enough; opening a file in that language starts + /// a fresh one, which is what already happens the first time. + private(set) var idleSince: Date? { + didSet { if idleSince != nil { onIdle?() } } + } + + /// Told when this server stops holding documents, so the registry can put + /// a wait on it rather than sweeping every server on a clock. + @ObservationIgnored var onIdle: (() -> Void)? + @ObservationIgnored private let connection: LSPConnection @ObservationIgnored private var versions: [String: Int] = [:] - @ObservationIgnored private var openCount: [String: Int] = [:] + @ObservationIgnored private var openCount: [String: Int] = [:] { + didSet { + guard openCount.isEmpty != oldValue.isEmpty else { return } + idleSince = openCount.isEmpty ? Date() : nil + } + } @ObservationIgnored private var diagnosticObservers: [ObjectIdentifier: (String, [LSP.Diagnostic]) -> Void] = [:] @ObservationIgnored private var startTask: Task? @ObservationIgnored private var settings: LSP.Value? @@ -121,6 +144,9 @@ final class LanguageService { ) } status = .running + // A prewarmed server holds nothing yet, so its idle clock + // starts here. `open` clears it the moment a file arrives. + if openCount.isEmpty, idleSince == nil { idleSince = Date() } return true } catch { status = .failed(error.localizedDescription) @@ -134,8 +160,18 @@ final class LanguageService { } func shutdown() { + // Before the resets below, so emptying the documents does not report a + // server that is going away as one that has just gone idle. + onIdle = nil Task { await connection.stop() } status = .notStarted + // The handshake is what `startIfNeeded` remembers; without this a + // service stopped for being idle could never be started again. + startTask = nil + openCount = [:] + versions = [:] + diagnostics = [:] + idleSince = nil } // MARK: - Document sync diff --git a/Sources/Workspace/Models/PullRequestBuild.swift b/Sources/Workspace/Models/PullRequestBuild.swift index c834a4a..d3b0b67 100644 --- a/Sources/Workspace/Models/PullRequestBuild.swift +++ b/Sources/Workspace/Models/PullRequestBuild.swift @@ -33,6 +33,19 @@ struct PullRequestBuild: Identifiable, Sendable, Hashable { } } + /// Whether this is a verdict rather than a stage on the way to one. + /// A run that has settled will not change again, so there is nothing + /// left to learn by asking the host about it a second time — see + /// `PullRequestDetailView.watchBuilds`. `unknown` is deliberately not + /// settled: it is a word this app failed to read, not a host saying + /// the run is over. + var isSettled: Bool { + switch self { + case .passed, .failed, .cancelled, .skipped: true + case .running, .pending, .unknown: false + } + } + /// Every host spells these differently, and one host spells them /// differently in two places — Cloud reports a pipeline's outcome in /// `state.result.name` but a commit status's in `state`. Matching on diff --git a/Sources/Workspace/Models/TerminalSession.swift b/Sources/Workspace/Models/TerminalSession.swift index d743eb8..b3f95ce 100644 --- a/Sources/Workspace/Models/TerminalSession.swift +++ b/Sources/Workspace/Models/TerminalSession.swift @@ -104,21 +104,7 @@ final class TerminalSession: Identifiable { self.title = title self.view = GhosttySurfaceView() view.onTitleChange = { [weak self] title in - guard let self else { return } - let wasWorking = isWorking - self.title = title - isWorking = Self.readsAsBusy(title) - titleChanges += 1 - // `claude` renames the tab as it starts work on a prompt, which is - // exactly when a conversation that had no transcript may have got - // one — so a rename is the cue to go looking again. - watchForClaudeName() - // The spinner going away is the turn ending: whatever was being - // waited for is done, and the person who set it going is very - // likely somewhere else by now. - if wasWorking, !isWorking { - raiseAttention("Finished — waiting for you") - } + self?.titleArrived(title) } view.onClose = { [weak self] in self?.isWorking = false @@ -135,6 +121,61 @@ final class TerminalSession: Identifiable { } } + /// The newest name the shell has given itself, published or not. + @ObservationIgnored private var latestTitle: String? + /// The wait that will publish it; see ``titleArrived``. + @ObservationIgnored private var titlePublish: Task? + /// How long a burst of renames is allowed to run before the tab catches + /// up. Short enough to read as immediate, long enough that a spinner is + /// several frames rather than several redraws. + private static let titleInterval = Duration.milliseconds(200) + + /// A terminal renames itself far faster than anyone can read. + /// + /// `claude` paints its spinner **into the tab title**, so a running turn is + /// a new name every frame — and every one of those used to be a write to an + /// observable property, which redraws the terminals list, the tab bar and + /// the viewer header. With several conversations going at once that was the + /// whole window redrawing continuously for a character nobody can follow, + /// and it got worse with every tab left running. + /// + /// So the two halves are separated. What a name *means* is read straight + /// away — `isWorking` is what puts a banner up when a turn ends, and it + /// only changes at the two ends of one, so writing it on the change alone + /// costs nothing. The text itself is published on a slow tick, and the last + /// name of a burst is always published, so a tab never settles showing a + /// frame of the spinner. + private func titleArrived(_ title: String) { + // A program re-announcing the name it already has is not a rename. + guard title != latestTitle else { return } + latestTitle = title + + let wasWorking = isWorking + let working = Self.readsAsBusy(title) + if working != wasWorking { isWorking = working } + titleChanges += 1 + // `claude` renames the tab as it starts work on a prompt, which is + // exactly when a conversation that had no transcript may have got one — + // so a rename is the cue to go looking again. + watchForClaudeName() + // The spinner going away is the turn ending: whatever was being waited + // for is done, and the person who set it going is very likely somewhere + // else by now. + if wasWorking, !working { + raiseAttention("Finished — waiting for you") + } + + guard titlePublish == nil else { return } + titlePublish = Task { [weak self] in + try? await Task.sleep(for: Self.titleInterval) + guard let self else { return } + // Cleared before the write, so a rename arriving in the same turn + // starts the next wait rather than being dropped. + titlePublish = nil + if let latestTitle, latestTitle != self.title { self.title = latestTitle } + } + } + /// Whether a tab title reads as "the program in here is busy". /// /// There is no other way to know. The app drives a real terminal, so the @@ -311,6 +352,8 @@ final class TerminalSession: Identifiable { isWorking = false nameWatch?.cancel() nameWatch = nil + titlePublish?.cancel() + titlePublish = nil claudeStartupWatch?.cancel() claudeStartupWatch = nil view.close() diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 6c7c9dc..560a0bc 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -426,13 +426,44 @@ final class WorkspaceStore { /// Reads every repository again. `quietly` is the sweep on a clock: the same /// reads, with no spinner put up for them — see `Project.refresh`. + /// + /// **A few at a time.** One read is a handful of processes — `git status`, + /// `git log`, `gh pr list` — and starting one task per repository meant a + /// sidebar of a dozen fired forty-odd of them at once, every five minutes. + /// They land on the cooperative pool, which has a thread per core, so the + /// sweep was a hitch across the whole window rather than work happening + /// behind it. The numbers are no less fresh for arriving a second later. func refreshAll(quietly: Bool = false) { lastAutoRefresh = Date() - for project in projects { - Task { await project.refresh(quietly: quietly) } + let due = projects + refreshSweep?.cancel() + refreshSweep = Task { + await withTaskGroup(of: Void.self) { group in + var next = due.makeIterator() + var running = 0 + while running < Self.refreshWidth, let project = next.next() { + group.addTask { await project.refresh(quietly: quietly) } + running += 1 + } + // One in, one out: the group never holds more than the width. + while await group.next() != nil { + guard !Task.isCancelled, let project = next.next() else { continue } + group.addTask { await project.refresh(quietly: quietly) } + } + } } } + /// How many repositories are read at once. Three keeps the sweep off the + /// pool's last threads on a four-core machine while still being several + /// times faster than reading them one after another. + private static let refreshWidth = 3 + + /// The sweep in flight, so a second one replaces it rather than doubling + /// up — a hand refresh landing on top of the five-minute tick used to run + /// every repository twice. + @ObservationIgnored private var refreshSweep: Task? + // MARK: - Refreshing on a clock /// How often every repository in the sidebar is read again — the branch it @@ -468,15 +499,31 @@ final class WorkspaceStore { startReloadingOnReturningToFront() autoRefreshTask = Task { [weak self] in - // The loop runs a minute at a time and decides by the clock, so a - // sweep the user asked for by hand pushes the next one out too. + // The loop decides by the clock rather than by its own rhythm, so a + // sweep the user asked for by hand pushes the next one out too — it + // sleeps for whatever is left of the interval rather than waking + // every minute to work out that there is nothing to do yet. while !Task.isCancelled { - try? await Task.sleep(for: .seconds(60)) - guard !Task.isCancelled, let self else { return } - let due = Date().timeIntervalSince(lastAutoRefresh) >= Self.autoRefreshInterval - guard due || missedAutoRefresh else { continue } + guard let self else { return } + let waited = Date().timeIntervalSince(lastAutoRefresh) + // A second short is due: without the slack, a sleep that lands + // a hair early goes round again for a fraction of a second. + let remaining = Self.autoRefreshInterval - waited - 1 + if remaining > 0, !missedAutoRefresh { + try? await Task.sleep(for: .seconds(remaining)) + guard !Task.isCancelled else { return } + // The clock is asked again rather than trusted: a hand + // refresh during the sleep moved the deadline, and the Mac + // waking from sleep moved it the other way. + continue + } guard NSApp.isActive else { + // Behind something else: nothing goes out, and the one tick + // it owes is paid when the app comes back — checked a + // minute at a time, which is cheap next to a sweep. missedAutoRefresh = true + try? await Task.sleep(for: .seconds(60)) + guard !Task.isCancelled else { return } continue } missedAutoRefresh = false diff --git a/Sources/Workspace/Terminal/GhosttyRuntime.swift b/Sources/Workspace/Terminal/GhosttyRuntime.swift index e7aabf5..fe00b6f 100644 --- a/Sources/Workspace/Terminal/GhosttyRuntime.swift +++ b/Sources/Workspace/Terminal/GhosttyRuntime.swift @@ -45,8 +45,17 @@ final class GhosttyRuntime { runtime.userdata = nil runtime.supports_selection_clipboard = false + // Ghostty wakes us whenever it has work, which under a program painting + // at frame rate is many times per frame. One tick answers all of them, + // so only the first wakeup of a batch pays for a hop. runtime.wakeup_cb = { _ in - Task { @MainActor in GhosttyRuntime.shared.tick() } + guard GhosttyRuntime.pendingTick.claim() else { return } + Task { @MainActor in + // Cleared first: a wakeup arriving during the tick is about + // work this tick has not seen, and has to schedule its own. + GhosttyRuntime.pendingTick.clear() + GhosttyRuntime.shared.tick() + } } runtime.action_cb = { _, target, action in @@ -214,8 +223,18 @@ final class GhosttyRuntime { let surfaceBits = Int(bitPattern: surface) switch action.tag { + // Gathered rather than hopped one at a time; see ``pendingRenders``. case GHOSTTY_ACTION_RENDER: - onView(surfaceBits) { view in view.needsDisplay = true } + guard pendingRenders.add(surfaceBits) else { return true } + Task { @MainActor in + for bits in pendingRenders.drain() { + guard let surface = UnsafeMutableRawPointer(bitPattern: bits), + let userdata = ghostty_surface_userdata(surface) else { continue } + Unmanaged.fromOpaque(userdata) + .takeUnretainedValue() + .needsDisplay = true + } + } return true case GHOSTTY_ACTION_SET_TITLE: @@ -273,6 +292,21 @@ final class GhosttyRuntime { } } + // MARK: - Coalescing + + /// The surfaces waiting to be marked for redraw. + /// + /// Ghostty asks to render whenever a surface is dirty, and a shell running + /// a spinner — every `claude` mid-turn — is dirty every frame, whether or + /// not anyone has that tab on screen. A hop per request meant the cost of + /// a window grew with each terminal left running in it, which is the shape + /// of an app that gets slower the longer it is open. The requests are + /// gathered on ghostty's own thread instead, and one hop marks everything + /// that arrived while it was waiting its turn. + private nonisolated static let pendingRenders = RenderQueue() + /// The same for `wakeup_cb`, which has nothing to gather. + private nonisolated static let pendingTick = Latch() + /// Finds the view that owns a surface and runs `body` on the main actor. private nonisolated static func onView( _ surfaceBits: Int, @@ -285,3 +319,59 @@ final class GhosttyRuntime { } } } + +/// A one-shot flag shared across threads: the first caller to ``claim`` owes +/// the main-actor hop, and everyone arriving behind it is already covered by +/// the hop that one is about to make. +private final class Latch: @unchecked Sendable { + private let lock = NSLock() + private var isSet = false + + /// True when this caller is the one that has to schedule the work. + func claim() -> Bool { + lock.lock() + defer { lock.unlock() } + guard !isSet else { return false } + isSet = true + return true + } + + /// Called at the *start* of the scheduled work, so anything arriving while + /// it runs schedules a fresh pass rather than being swallowed by this one. + func clear() { + lock.lock() + isSet = false + lock.unlock() + } +} + +/// The surfaces that have asked to be redrawn since the last drain, and one +/// latch saying whether a drain is already on its way. +private final class RenderQueue: @unchecked Sendable { + private let lock = NSLock() + private var pending: Set = [] + private var isScheduled = false + + /// Records a surface, and returns true only for the request that has to + /// schedule the drain. + func add(_ surfaceBits: Int) -> Bool { + lock.lock() + defer { lock.unlock() } + pending.insert(surfaceBits) + guard !isScheduled else { return false } + isScheduled = true + return true + } + + /// Everything that arrived since the last drain. Taking the batch and + /// releasing the latch is one step, so a request landing mid-drain is + /// either in this batch or schedules the next one — never neither. + func drain() -> Set { + lock.lock() + defer { lock.unlock() } + isScheduled = false + let batch = pending + pending.removeAll(keepingCapacity: true) + return batch + } +} diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 0ecabcf..7bd3652 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -163,9 +163,21 @@ struct PullRequestDetailView: View { await store.loadBuilds(item, project: project, pr: pr) } + // How many reads this loop has made of the runs it is watching now, and + // which those are. A new run appearing — a push, a re-run — starts the + // count again, so it is always watched closely at first. + var rounds = 0 + var watching: Set = [] + while !Task.isCancelled { + let unsettled = Set(item.builds.filter { !$0.state.isSettled }.map(\.id)) + if unsettled != watching { + watching = unsettled + rounds = 0 + } + do { - try await Task.sleep(for: .seconds(10)) + try await Task.sleep(for: Self.buildInterval(round: rounds, unsettled: unsettled.count)) } catch { return } @@ -174,6 +186,32 @@ struct PullRequestDetailView: View { // tick tries again. guard NSApplication.shared.isActive, !item.isLoadingBuilds else { continue } await store.refreshBuilds(item, project: project, pr: pr) + rounds += 1 + } + } + + /// How long to wait before reading the runs again. + /// + /// A run still going is the thing the person is sitting here for, so the + /// first minute is read every ten seconds and it eases off from there: a + /// build that takes half an hour does not need a hundred and eighty reads + /// of it, and each one is a `gh` or `bkt` process. + /// + /// Once every run has its verdict there is no news left but a push, and + /// this loop used to keep asking every ten seconds anyway, for as long as + /// the pull request stayed on screen — a pull request left open for a + /// working day was thousands of calls to the host for an answer that + /// stopped changing in the first three minutes. So a settled pull request + /// drops to the slow tick, which is there to notice a run that appears + /// rather than one that changes. The count restarts the moment one does. + private static func buildInterval(round: Int, unsettled: Int) -> Duration { + // Nothing running: only a push can change this, and that is the kind of + // thing the sidebar's five-minute sweep is paced for. + guard unsettled > 0 else { return .seconds(round < 6 ? 30 : 300) } + switch round { + case ..<6: return .seconds(10) + case ..<12: return .seconds(30) + default: return .seconds(60) } } diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index 23b1c04..dd08287 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -260,6 +260,15 @@ struct ViewerView: View { ? (item.selectedTerminal?.displayTitle ?? item.title) : item.title + // A shell is named twice over: what the program inside it calls itself, + // and the repository it is rooted in. Everywhere else the first of those + // is the thing being read, so it gets the weight — but a terminal's own + // name is whatever `claude` or a prompt last renamed the tab to, which + // is a spinner frame as often as it is a word, while the repository is + // what tells three open shells apart at a glance. So for a terminal the + // two keep their places and trade their emphasis. + let namesRepositoryFirst = item.isTerminal + return HStack(spacing: 7) { // A commit is one person's work, so the face says more than the // glyph every other kind of item gets. @@ -273,14 +282,16 @@ struct ViewerView: View { .font(.caption) } Text(title) - .font(.callout.weight(.medium)) + .font(namesRepositoryFirst ? .caption : .callout.weight(.medium)) + .foregroundStyle(namesRepositoryFirst ? .tertiary : .primary) .lineLimit(1) .truncationMode(.middle) if let subtitle = item.subtitle { Text(subtitle) - .font(.caption) - .foregroundStyle(.tertiary) + .font(namesRepositoryFirst ? .callout.weight(.medium) : .caption) + .foregroundStyle(namesRepositoryFirst ? .primary : .tertiary) .lineLimit(1) + .truncationMode(.middle) } if item.isDirty { Circle().fill(.orange).frame(width: 6, height: 6) From b95bb856365a76e434e2e156140f755a9be2d042 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Thu, 30 Jul 2026 15:23:12 +0700 Subject: [PATCH 2/3] The review's three findings, and all three were real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Replacing the sweep waited for nothing.** Cancellation is cooperative and `Project.refresh` checks it nowhere, so the repositories already in flight ran their `git` and `gh` to the end regardless — the cancel only stopped the sweep adding more. Starting the replacement on top of that put twice the width on the pool and read up to three repositories twice, which is the doubling the task handle was added to stop. The new sweep awaits the old one's value first, and re-checks its own cancellation after, since a third refresh may have arrived while it waited. **A cancelled title publish published anyway.** `try?` swallowed the `CancellationError`, so the task carried straight on to the write — `terminate()` cancelling it still got one last name written over the tab it was closing. `do { … } catch { return }` instead, which is the form the build watcher already uses two files over. **A failed read looked exactly like a settled one.** Both are an empty set of runs still going, and only one of them means there is nothing left to ask. A first `loadBuilds` that failed empties the list, so the loop dropped to the five-minute tick after three minutes and a host that came back was not noticed until the next one — while `refreshBuilds` never repopulates a list it could not read. `buildsError` now keeps the loop on the 10/30/60 backoff, for the same reason `unknown` is not settled: not knowing is not a verdict. Raised by CodeRabbit on #23. Checked: `Scripts/bundle.sh` — build succeeded. The three warnings it prints are all on `main` and none is in a line this branch touches. Co-Authored-By: Claude Opus 5 (1M context) --- .../Workspace/Models/TerminalSession.swift | 5 +++- Sources/Workspace/Models/WorkspaceStore.swift | 15 ++++++++++-- .../Views/PullRequestDetailView.swift | 24 ++++++++++++++++--- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/Sources/Workspace/Models/TerminalSession.swift b/Sources/Workspace/Models/TerminalSession.swift index b3f95ce..d8ca90f 100644 --- a/Sources/Workspace/Models/TerminalSession.swift +++ b/Sources/Workspace/Models/TerminalSession.swift @@ -167,7 +167,10 @@ final class TerminalSession: Identifiable { guard titlePublish == nil else { return } titlePublish = Task { [weak self] in - try? await Task.sleep(for: Self.titleInterval) + // `try?` here would swallow the cancellation and publish anyway, + // which is `terminate()` cancelling this and getting one last name + // written over the tab it is closing. + do { try await Task.sleep(for: Self.titleInterval) } catch { return } guard let self else { return } // Cleared before the write, so a rename arriving in the same turn // starts the next wait rather than being dropped. diff --git a/Sources/Workspace/Models/WorkspaceStore.swift b/Sources/Workspace/Models/WorkspaceStore.swift index 560a0bc..f199c30 100644 --- a/Sources/Workspace/Models/WorkspaceStore.swift +++ b/Sources/Workspace/Models/WorkspaceStore.swift @@ -436,8 +436,19 @@ final class WorkspaceStore { func refreshAll(quietly: Bool = false) { lastAutoRefresh = Date() let due = projects - refreshSweep?.cancel() + // Cancelled *and waited for*. Cancellation here only stops the sweep + // adding more repositories: `Project.refresh` checks nothing, so the + // ones already in flight run their `git` and `gh` to the end whatever + // this says. Starting the replacement on top of them would put twice + // the width on the pool and read those repositories twice, which is + // the doubling this whole task was added to stop. + let previous = refreshSweep + previous?.cancel() refreshSweep = Task { + await previous?.value + // A third sweep may have arrived while this one waited, and it is + // the newer answer. + guard !Task.isCancelled else { return } await withTaskGroup(of: Void.self) { group in var next = due.makeIterator() var running = 0 @@ -461,7 +472,7 @@ final class WorkspaceStore { /// The sweep in flight, so a second one replaces it rather than doubling /// up — a hand refresh landing on top of the five-minute tick used to run - /// every repository twice. + /// every repository twice. Replacing means waiting for it; see `refreshAll`. @ObservationIgnored private var refreshSweep: Task? // MARK: - Refreshing on a clock diff --git a/Sources/Workspace/Views/PullRequestDetailView.swift b/Sources/Workspace/Views/PullRequestDetailView.swift index 7bd3652..c79f082 100644 --- a/Sources/Workspace/Views/PullRequestDetailView.swift +++ b/Sources/Workspace/Views/PullRequestDetailView.swift @@ -177,7 +177,17 @@ struct PullRequestDetailView: View { } do { - try await Task.sleep(for: Self.buildInterval(round: rounds, unsettled: unsettled.count)) + try await Task.sleep(for: Self.buildInterval( + round: rounds, + unsettled: unsettled.count, + // A read that failed is not a verdict either. A tick that + // stumbles leaves the last good list alone, but a first + // load that failed leaves no list at all — and no list + // reads as "nothing is running", which is the slow tick. + // So an error keeps this on the close watch until the host + // answers, for the same reason `unknown` is not settled. + afterFailedRead: item.buildsError != nil + )) } catch { return } @@ -204,10 +214,18 @@ struct PullRequestDetailView: View { /// stopped changing in the first three minutes. So a settled pull request /// drops to the slow tick, which is there to notice a run that appears /// rather than one that changes. The count restarts the moment one does. - private static func buildInterval(round: Int, unsettled: Int) -> Duration { + /// + /// `afterFailedRead` is there because settling and not knowing look the + /// same from here — both are an empty list of runs still going — and only + /// one of them means there is nothing left to ask about. + private static func buildInterval( + round: Int, + unsettled: Int, + afterFailedRead: Bool + ) -> Duration { // Nothing running: only a push can change this, and that is the kind of // thing the sidebar's five-minute sweep is paced for. - guard unsettled > 0 else { return .seconds(round < 6 ? 30 : 300) } + guard unsettled > 0 || afterFailedRead else { return .seconds(round < 6 ? 30 : 300) } switch round { case ..<6: return .seconds(10) case ..<12: return .seconds(30) From 749ce6751e12c76619fad469df67061e63c96eb9 Mon Sep 17 00:00:00 2001 From: necessarylion Date: Thu, 30 Jul 2026 15:45:30 +0700 Subject: [PATCH 3/3] A repository you can find, and a shell that says which one The floating chat bar and the viewer's terminal header both put the project behind its title. The name a conversation or a shell gives itself changes under you; the repository is what tells three of them apart. Now both give the repository the weight and let the title sit behind it. --- Sources/Workspace/Views/ChatPanelView.swift | 25 ++++++++++++--------- Sources/Workspace/Views/ViewerView.swift | 17 +++----------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/Sources/Workspace/Views/ChatPanelView.swift b/Sources/Workspace/Views/ChatPanelView.swift index ccd371d..cbaa134 100644 --- a/Sources/Workspace/Views/ChatPanelView.swift +++ b/Sources/Workspace/Views/ChatPanelView.swift @@ -281,19 +281,23 @@ struct ChatPanelBarContent: View { ChatPanelMark(isWorking: panel.session.isWorking) // One line, repository first: `workspace | fix the scroll bug`. - // Stacked, the two competed for a 30pt bar and the conversation — - // the half that actually changes — got the smaller type. + // Stacked, the two competed for a 30pt bar. // - // **The repository holds its width and the conversation gives way**, - // which is the same argument `dockedMinimumWidth` is written from: - // the dock is shared by every repository, so the project is what a - // folded bar must still be saying when there is no room left. Both - // are capped at one line, so a long conversation title ends in an + // **The repository is the one that is read**, so it carries the + // weight and the conversation sits behind it. The name a + // conversation is given is whatever it happened to be about, and it + // changes under you; which repository a floating bar belongs to is + // the thing being looked for when several are docked side by side. + // + // The same argument decides the width, and it did before the type + // followed it: **the repository holds its width and the + // conversation gives way** — see `dockedMinimumWidth`. Both are + // capped at one line, so a long conversation title ends in an // ellipsis rather than pushing the buttons off the end of the bar. HStack(spacing: 5) { Text(panel.projectName) - .font(.system(size: 12, weight: .medium)) - .foregroundStyle(.secondary) + .font(.system(size: 12, weight: .semibold)) + .foregroundStyle(.primary) .lineLimit(1) .truncationMode(.tail) .layoutPriority(1) @@ -302,7 +306,8 @@ struct ChatPanelBarContent: View { .foregroundStyle(.tertiary) .layoutPriority(1) Text(panel.title) - .font(.system(size: 12, weight: .medium)) + .font(.system(size: 12)) + .foregroundStyle(.secondary) .lineLimit(1) .truncationMode(.tail) } diff --git a/Sources/Workspace/Views/ViewerView.swift b/Sources/Workspace/Views/ViewerView.swift index dd08287..23b1c04 100644 --- a/Sources/Workspace/Views/ViewerView.swift +++ b/Sources/Workspace/Views/ViewerView.swift @@ -260,15 +260,6 @@ struct ViewerView: View { ? (item.selectedTerminal?.displayTitle ?? item.title) : item.title - // A shell is named twice over: what the program inside it calls itself, - // and the repository it is rooted in. Everywhere else the first of those - // is the thing being read, so it gets the weight — but a terminal's own - // name is whatever `claude` or a prompt last renamed the tab to, which - // is a spinner frame as often as it is a word, while the repository is - // what tells three open shells apart at a glance. So for a terminal the - // two keep their places and trade their emphasis. - let namesRepositoryFirst = item.isTerminal - return HStack(spacing: 7) { // A commit is one person's work, so the face says more than the // glyph every other kind of item gets. @@ -282,16 +273,14 @@ struct ViewerView: View { .font(.caption) } Text(title) - .font(namesRepositoryFirst ? .caption : .callout.weight(.medium)) - .foregroundStyle(namesRepositoryFirst ? .tertiary : .primary) + .font(.callout.weight(.medium)) .lineLimit(1) .truncationMode(.middle) if let subtitle = item.subtitle { Text(subtitle) - .font(namesRepositoryFirst ? .callout.weight(.medium) : .caption) - .foregroundStyle(namesRepositoryFirst ? .primary : .tertiary) + .font(.caption) + .foregroundStyle(.tertiary) .lineLimit(1) - .truncationMode(.middle) } if item.isDirty { Circle().fill(.orange).frame(width: 6, height: 6)