Repository navigation
Fix sidebar freeze from spinner terminal titles (#6291) - #6840
austinywang wants to merge 12 commits into
Conversation
CLI tools like pnpm/npm/cargo animate a spinner by rewriting the terminal title every frame with a leading braille glyph. Each raw frame is a distinct string, so every frame mutates the workspace title and posts .workspaceTitleDidChange, thrashing the sidebar's main-thread layout until clicks are starved. This test posts the braille spinner frames of "pnpm install" and asserts they collapse to one stable, spinner-free panel title with a single workspace-title change. It fails on current main (the panel title retains the last spinner glyph and the workspace title changes once per frame). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CLI tools (pnpm, npm, cargo, …) animate a spinner by rewriting the terminal title on every frame with a leading braille glyph (⠋⠙⠹…). cmux forwarded every title through .ghosttyDidSetTitle, and because each raw frame is a distinct string, no existing dedup caught them: each frame mutated the workspace title, posted .workspaceTitleDidChange, and re-rendered the sidebar rows. The volume starved main-thread hit-testing, so sidebar/tab clicks were swallowed until the spinner-emitting command finished. Collapse spinner frames to one stable title at every layer: - TerminalSurface.stableTerminalNotificationTitle strips spinner glyphs (the full Braille Patterns block plus common rotating glyphs) and collapses the whitespace they bordered; titles without a spinner glyph are returned verbatim. - TerminalSurface.publishableTerminalTitle dedupes at the source: the SET_TITLE handler drops a post whose normalized title equals the last one published for that surface, so a spinner storm produces no notifications at all. - TabManager.enqueuePanelTitleUpdate normalizes via the shared algorithm and early-returns when the same title is already queued or the workspace already shows it (Workspace.alreadyReflectsPanelTitleUpdate), so any post that bypasses the source dedup still cannot thrash the sidebar. The toolbar command-text path is already gated to the selected tab via shouldScheduleRawTitleRefresh, so it benefits from the reduced post volume without further change. Adds a pure-algorithm unit test and makes the behavior regression test from the previous commit pass (spinner frames collapse to one stable panel title with a single workspace-title change). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ Finishing Touches🧪 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 SummaryFixes a main-thread starvation issue (#6291) where CLI tools like
Confidence Score: 5/5Safe to merge — the normalization algorithm is correct, dedup logic is layered and well-tested, and no existing title update paths are regressed. The spinner-stripping algorithm correctly handles all described edge cases (embedded Braille preserved, multi-position standalone tokens, multi-token titles, whitespace collapsing). The three-layer dedup is sound and the replacement-surface test directly exercises the trickiest interaction. No correctness or isolation regressions were found. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI Tool
participant GH as GhosttyTerminalView
participant TS as TerminalSurface
participant NC as NotificationCenter
participant TM as TabManager
participant WS as Workspace
CLI->>GH: frame 1 title
GH->>TS: publishableTerminalTitle
TS-->>GH: stable title (publish)
GH->>NC: post .ghosttyDidSetTitle
NC->>TM: enqueuePanelTitleUpdate
TM->>WS: flush panel title
CLI->>GH: frame 2 title
GH->>TS: publishableTerminalTitle
TS-->>GH: nil (same stable title, drop)
Note over GH,WS: Zero additional mutations
%%{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 CLI as CLI Tool
participant GH as GhosttyTerminalView
participant TS as TerminalSurface
participant NC as NotificationCenter
participant TM as TabManager
participant WS as Workspace
CLI->>GH: frame 1 title
GH->>TS: publishableTerminalTitle
TS-->>GH: stable title (publish)
GH->>NC: post .ghosttyDidSetTitle
NC->>TM: enqueuePanelTitleUpdate
TM->>WS: flush panel title
CLI->>GH: frame 2 title
GH->>TS: publishableTerminalTitle
TS-->>GH: nil (same stable title, drop)
Note over GH,WS: Zero additional mutations
Reviews (9): Last reviewed commit: "Reset terminal title dedupe on workspace..." | Re-trigger Greptile |
…6291) Codex review (P3): the previous normalization deleted every spinner scalar anywhere in the title, which would corrupt a legitimate title that merely contains a Braille scalar inside a word (e.g. a path like ~/work/⠋-project). Rework stableTerminalNotificationTitle to drop only whitespace-delimited tokens that are made up ENTIRELY of spinner glyphs (a standalone animation frame at any position); every other token is preserved verbatim. Add unit coverage proving a Braille scalar inside a path component survives even when a leading spinner is stripped. Greptile review: remove the redundant Workspace.alreadyReflectsPanelTitleUpdate predicate (it shadowed the mutation conditions of updatePanelTitle/ applyProcessTitle and was effectively dead in production — the source-level publishableTerminalTitle dedup already prevents the flood, and updatePanelTitle is idempotent). Inline the one-call-site stableTerminalPanelTitle wrapper. The TabManager safety-net keeps the cheap, non-shadowing 'same title already pending' guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…comes-unclickable-when-cli-tools
Regenerated via scripts/swift_file_length_budget.py --write-budget after merging origin/main. Raises budgets for the four files this PR grows (TerminalSurface, TabManager, GhosttyTerminalView, TabManagerTitleUpdateTests) and ratchets down files that shrank on main since the budget was last written. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…comes-unclickable-when-cli-tools # Conflicts: # .github/swift-file-length-budget.tsv # Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swift
…comes-unclickable-when-cli-tools # Conflicts: # .github/swift-file-length-budget.tsv
|
Same story - the SET_TITLE integration point this targets is gone on main (your TabManager hunks still apply, but the load-bearing part conflicts). Details here: #6291 (comment) |
Fixes #6291
Problem
CLI tools like
pnpm,npm, andcargoanimate a progress spinner by rewriting the terminal title on every animation frame with a leading Unicode braille glyph (⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏). cmux forwards every title change through.ghosttyDidSetTitleinto the workspace panel-title coalescer and the toolbar command-text updater.Because every raw frame is a distinct string (only the spinner glyph differs), none of the existing dedup paths caught them: each frame mutated the workspace title, posted
.workspaceTitleDidChange, and re-rendered the sidebar rows. The sheer volume starved main-thread hit-testing, so sidebar/tab clicks were swallowed until the spinner-emitting command finished — the user had to fall back tocmd+1/cmd+2to navigate.Fix
Collapse spinner frames to one stable title at every layer of the title pipeline:
TerminalSurface.stableTerminalNotificationTitle(_:)— strips spinner glyphs (the full Braille Patterns blockU+2800…U+28FFplus common non-braille rotating glyphs) and collapses the whitespace they bordered. Titles that contain no spinner glyph take a fast path and are returned byte-for-byte unchanged, so ordinary titles keep their exact text.TerminalSurface.publishableTerminalTitle(forRawTitle:)— dedupes at the source: theGHOSTTY_ACTION_SET_TITLEhandler drops a post whose normalized title equals the last one published for that surface. A spinner storm now produces zero notifications, so the coalescer, sidebar layout, and toolbar updater are never woken.TabManager.enqueuePanelTitleUpdate— normalizes through the same shared algorithm and early-returns when the same stable title is already queued, or whenWorkspace.alreadyReflectsPanelTitleUpdate(panelId:title:)reports the workspace UI already shows it. This is the safety net for any post that bypasses the source-level dedup. A different pending title still falls through, so last-write-wins correctness is preserved.The toolbar command-text path is already gated to the selected tab via
shouldScheduleRawTitleRefresh, so it benefits from the reduced post volume with no further change.A nice side effect: the sidebar/panel title now shows a stable
pnpm installinstead of a flickering⠋ pnpm install.Tests
Two-commit red/green structure:
spinnerTerminalTitleFramesCollapseToStablePanelTitleposts the 10 braille spinner frames ofpnpm installthrough the real.ghosttyDidSetTitle→TabManagerpath and asserts they collapse to one stable, spinner-free panel title with a single.workspaceTitleDidChange. Fails onmain(panel title retains the last glyph; the workspace title changes once per frame).stableTerminalNotificationTitleStripsSpinnerGlyphs, a pure-algorithm unit test covering leading/embedded/trailing spinner glyphs, whitespace collapse, and the unchanged fast path.Both tests live in the already-wired
cmuxTests/TabManagerTitleUpdateTests.swift.Localization
No user-facing string literals were added — terminal titles are dynamic content from the shell/CLI, not localizable literals. Localization audit: nothing to update.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes sidebar and tab freezes caused by spinner-driven terminal titles by collapsing animation frames to one stable title and deduping updates end-to-end. Keeps the UI responsive during commands like
pnpm install,npm install, andcargo build(fixes #6291).Written for commit e131580. Summary will update on new commits.