Repository navigation
Collapse spinner-frame terminal titles to fix sidebar freeze (#6291) - #6319
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesSpinner Title Deduplication
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a sidebar freeze (#6291) caused by CLI spinners (pnpm, cargo, codex, ora-based tools) flooding the main thread by posting a new
Confidence Score: 4/5Safe to merge; the churn fix is well-scoped and the dedup logic is correct. The one open edge case — The surface-replacement gap in Sources/GhosttyTerminalView.swift — specifically the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GIO as Ghostty I/O Thread
participant MAIN as Main Thread (DispatchQueue.main)
participant GNV as GhosttyNSView<br/>(lastPublishedTerminalTitle)
participant NC as NotificationCenter
participant TM as TabManager<br/>(pendingPanelTitleUpdates)
participant UI as Sidebar / Toolbar
GIO->>MAIN: GHOSTTY_ACTION_SET_TITLE ("⠋ pnpm install")
MAIN->>MAIN: "stable = stableTerminalPanelTitle → "pnpm install""
MAIN->>GNV: "lastPublishedTerminalTitle != stable?"
GNV-->>MAIN: """ != "pnpm install" → YES"
MAIN->>GNV: "lastPublishedTerminalTitle = "pnpm install""
MAIN->>NC: post .ghosttyDidSetTitle (title: "pnpm install")
NC->>TM: enqueuePanelTitleUpdate(title: "pnpm install")
TM->>TM: "pendingPanelTitleUpdates[key] != "pnpm install" → enqueue + signal coalescer"
TM->>UI: flushPendingPanelTitleUpdates → updatePanelTitle
GIO->>MAIN: GHOSTTY_ACTION_SET_TITLE ("⠙ pnpm install")
MAIN->>MAIN: "stable = "pnpm install""
MAIN->>GNV: "lastPublishedTerminalTitle != stable?"
GNV-->>MAIN: ""pnpm install" == "pnpm install" → NO (dropped here)"
Note over GIO,UI: Frame N+1…N+119 all dropped at source — coalescer and toolbar never see them
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant GIO as Ghostty I/O Thread
participant MAIN as Main Thread (DispatchQueue.main)
participant GNV as GhosttyNSView<br/>(lastPublishedTerminalTitle)
participant NC as NotificationCenter
participant TM as TabManager<br/>(pendingPanelTitleUpdates)
participant UI as Sidebar / Toolbar
GIO->>MAIN: GHOSTTY_ACTION_SET_TITLE ("⠋ pnpm install")
MAIN->>MAIN: "stable = stableTerminalPanelTitle → "pnpm install""
MAIN->>GNV: "lastPublishedTerminalTitle != stable?"
GNV-->>MAIN: """ != "pnpm install" → YES"
MAIN->>GNV: "lastPublishedTerminalTitle = "pnpm install""
MAIN->>NC: post .ghosttyDidSetTitle (title: "pnpm install")
NC->>TM: enqueuePanelTitleUpdate(title: "pnpm install")
TM->>TM: "pendingPanelTitleUpdates[key] != "pnpm install" → enqueue + signal coalescer"
TM->>UI: flushPendingPanelTitleUpdates → updatePanelTitle
GIO->>MAIN: GHOSTTY_ACTION_SET_TITLE ("⠙ pnpm install")
MAIN->>MAIN: "stable = "pnpm install""
MAIN->>GNV: "lastPublishedTerminalTitle != stable?"
GNV-->>MAIN: ""pnpm install" == "pnpm install" → NO (dropped here)"
Note over GIO,UI: Frame N+1…N+119 all dropped at source — coalescer and toolbar never see them
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// the workspace panel-title coalescer or the toolbar command-text updater | ||
| /// (issue #6291). Main-thread only (mutated from the title action handler's | ||
| /// main-queue hop). | ||
| var lastPublishedTerminalTitle: String = "" |
There was a problem hiding this comment.
lastPublishedTerminalTitle on surface replacement
lastPublishedTerminalTitle is initialised to "" and is never cleared when terminalSurface is replaced (e.g. when the terminal process exits and a new shell spawns in the same panel). The property is keyed on the GhosttyNSView instance, not on the (tabId, surfaceId) pair, so if the view is reused with a fresh TerminalSurface the dedup guard will silently suppress the new session's first title notification whenever its stable form matches the previous session's last published title. Clearing lastPublishedTerminalTitle to "" when terminalSurface is set to nil (in the detach path near line 7058) would close the window.
| /// flood the panel-title coalescer and toolbar command-text updater and starve | ||
| /// sidebar hit-testing. The fix collapses such titles to a stable form so the | ||
| /// redundant frames dedupe instead of re-driving the main thread (issue #6291). | ||
| @Suite struct TabManagerWorkspaceOwnershipTests { |
There was a problem hiding this comment.
TabManagerWorkspaceOwnershipTests describes workspace ownership semantics, but every test in the file exercises stableTerminalPanelTitle / spinner collapsing. A future reader looking for workspace-ownership coverage would find spinner tests here, and spinner tests are hard to discover by name.
| @Suite struct TabManagerWorkspaceOwnershipTests { | |
| @Suite struct TabManagerSpinnerTitleCollapseTests { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TabManagerWorkspaceOwnershipTests.swift`:
- Line 16: The test class name TabManagerWorkspaceOwnershipTests does not
accurately reflect the test content, which focuses on spinner-glyph
deduplication and stableTerminalPanelTitle functionality rather than workspace
ownership. Rename the `@Suite` struct TabManagerWorkspaceOwnershipTests to a name
that better describes its actual purpose, such as
TabManagerTerminalTitleDeduplicationTests, to improve code clarity and
maintainability.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2886-2889: The `lastPublishedTerminalTitle` property used for
deduplication in the GhosttyNSView survives when the view is rebound to a
different surface, causing the dedupe guard check to incorrectly drop title
updates if the first title of the new surface matches the previous surface's
last published title. Locate where GhosttyNSView is rebound to a different
surface and reset `lastPublishedTerminalTitle` to nil at that point to clear the
stale dedupe state. Apply the same fix to the related code section that also
handles similar title change logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c572cc34-bdf0-4552-b400-9a043032950e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/TabManagerWorkspaceOwnershipTests.swift
| /// flood the panel-title coalescer and toolbar command-text updater and starve | ||
| /// sidebar hit-testing. The fix collapses such titles to a stable form so the | ||
| /// redundant frames dedupe instead of re-driving the main thread (issue #6291). | ||
| @Suite struct TabManagerWorkspaceOwnershipTests { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Verify test class name aligns with test content.
The class TabManagerWorkspaceOwnershipTests suggests workspace-ownership testing, but the suite actually tests spinner-glyph deduplication via stableTerminalPanelTitle. Consider renaming to TabManagerTerminalTitleDeduplicationTests or similar for clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/TabManagerWorkspaceOwnershipTests.swift` at line 16, The test class
name TabManagerWorkspaceOwnershipTests does not accurately reflect the test
content, which focuses on spinner-glyph deduplication and
stableTerminalPanelTitle functionality rather than workspace ownership. Rename
the `@Suite` struct TabManagerWorkspaceOwnershipTests to a name that better
describes its actual purpose, such as TabManagerTerminalTitleDeduplicationTests,
to improve code clarity and maintainability.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
2880-2889:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset title dedupe state when the view rebinds to a different surface.
lastPublishedTerminalTitleis stored onGhosttyNSView, but dedupe semantics are surface-scoped. When a reused view attaches a new surface, the first title publish can be dropped if it matches the previous surface’s stable title (seeattachSurface(_:)around Line 3799), leaving stale downstream title state.💡 Proposed fix
func attachSurface(_ surface: TerminalSurface) { let isSameSurface = terminalSurface === surface let isAlreadyAttached = surface.isAttached(to: self) if !isSameSurface { appliedColorScheme = nil + // Dedupe state is surface-scoped; clear when rebinding this view. + lastPublishedTerminalTitle = "" } terminalSurface = surface tabId = surface.tabIdAlso applies to: 3425-3431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/GhosttyTerminalView.swift` around lines 2880 - 2889, The lastPublishedTerminalTitle property is used for deduplication of terminal title updates, but when a reused GhosttyNSView rebinds to a different surface in the attachSurface method (around line 3799), the old title state from the previous surface persists. This causes the deduplication guard in the title update logic to incorrectly skip the first title of the new surface if it matches the previous surface's stable title. Reset lastPublishedTerminalTitle to nil in the attachSurface method when binding to a new surface to ensure clean dedupe state for each surface-view binding. Apply the same reset fix to other relevant locations mentioned (around lines 3425-3431).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2880-2889: The lastPublishedTerminalTitle property is used for
deduplication of terminal title updates, but when a reused GhosttyNSView rebinds
to a different surface in the attachSurface method (around line 3799), the old
title state from the previous surface persists. This causes the deduplication
guard in the title update logic to incorrectly skip the first title of the new
surface if it matches the previous surface's stable title. Reset
lastPublishedTerminalTitle to nil in the attachSurface method when binding to a
new surface to ensure clean dedupe state for each surface-view binding. Apply
the same reset fix to other relevant locations mentioned (around lines
3425-3431).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a69bff2-99bd-495e-bfd7-60ebb9f8c664
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/TabManagerTerminalTitleCollapseTests.swift
# Conflicts: # .github/swift-file-length-budget.tsv
|
Heads up - the |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
4 participants
Closes #6291
Root cause
CLI tools like
pnpm,npm,cargo, and Node/ora-based spinners update theterminal title on every animation frame with a leading Unicode braille
spinner glyph (
⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏). Ghostty forwards each title change throughGHOSTTY_ACTION_SET_TITLE→.ghosttyDidSetTitle. Every post droveTabManager.enqueuePanelTitleUpdate(panel-title coalescer) andWindowToolbarController.scheduleFocusedCommandTextUpdate. Frame N and frameN+1 differ only by the leading glyph, so each was treated as a distinct title —
the redundant churn flooded the main thread enough to starve sidebar
hit-testing, leaving the sidebar unclickable until the spinner finished.
Fix
Collapse spinner-frame titles to a stable form and dedupe at the source so the
redundant frames never reach the coalescer or the toolbar updater:
TabManager.stableTerminalPanelTitle(_:)(nonisolated, pure) strips anyrun of leading braille spinner glyphs + following whitespace, then trims.
Frame N and N+1 with the same trailing text map to one string. Limited to
braille glyphs, which never legitimately lead a real title.
GhosttyTerminalViewposts the stable title and drops the post entirelywhen it equals the surface's
lastPublishedTerminalTitle— killing the churnat the source (before the coalescer and the toolbar command-text updater).
TabManager.enqueuePanelTitleUpdateearly-returns when the same stabletitle is already queued for that panel (belt-and-suspenders dedupe).
Verification
./scripts/reload.sh --tag issue-6291— clean Debug build.cmuxTests/TabManagerWorkspaceOwnershipTests(wired intoproject.pbxproj): spinner frames collapse to one stable title, non-spinnertitles are only trimmed, and distinct trailing text stays distinct.
Scope / tradeoffs
Narrowly the sidebar-freeze regression. The source-level dedupe (#2) makes the
toolbar-updater and coalescer guards from the issue's proposed fix list
unnecessary, so I did not add them. Title growth in the two large files is
covered by a
.github/swift-file-length-budget.tsvrefresh (splitting thosefiles is out of scope for this fix).
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the sidebar freeze by collapsing spinner-frame terminal titles into a stable form and deduping updates at the source. Stops rapid title churn from
pnpm,npm,cargo,codex, andoraspinners from flooding the UI (fixes #6291).TabManager.stableTerminalPanelTitleto strip leading spinner glyphs (braille/block) and following whitespace, then trim.GhosttyTerminalViewnow posts the stable title only when it changes; trackslastPublishedTerminalTitleto drop duplicates.TabManageruses the stable title when enqueuing and skips if the same value is already pending; addedcmuxTests/TabManagerTerminalTitleCollapseTests.swiftto cover collapse, non-spinner preservation, distinct trailing text, and effective dedup.Written for commit 84670fd. Summary will update on new commits.
Summary by CodeRabbit