Repository navigation
Keep screenshot paste preparation off the main thread - #8838
Conversation
|
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:
📝 WalkthroughWalkthroughThis change adds worker-backed clipboard preparation, transactional pasteboard access, runtime clipboard request sequencing, bounded HTML parsing, and reservation-aware composer pastes with attachment cleanup. ChangesClipboard paste flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning, 1 inconclusive)
✅ Passed checks (17 passed)
✨ Finishing Touches 💡 1📝 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 moves the entire screenshot-paste pipeline — named-pasteboard provider resolution, image decode/downscale/tone-map, temporary-file materialization, and composer thumbnail creation — out of the main process's synchronous AppKit paste responder and into a killable worker subprocess managed by a bounded FIFO actor. The clipboard input sequencer preserves terminal keyboard ordering while the worker runs, and the pending-paste reservation system preserves composer selection, caret position, undo state, and repeat-paste FIFO order throughout the async preparation window.
Confidence Score: 5/5Safe to merge. The change is a large, well-structured rearchitecting of the paste pipeline with comprehensive test coverage; no correctness, concurrency, or security defects were found. After tracing the full job lifecycle in the FIFO actor, the worker-client adoption protocol, the pending-paste reservation commit/rollback paths, the atomic admit/reserve sequencer, and the HTML parser bounds, no functional or concurrency defects were identified. The security hardening is thorough. The Swift 6 strict-concurrency constraints are satisfied throughout. The two comments raised are documentation clarity items, not correctness gaps. Files Needing Attention: No files require special attention. The most complex logic is in TextBoxInputTextView+PendingPasteReservation.swift and TerminalImageTransferPreparationService.swift, both covered by the new test suites in cmuxTests/. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as C Callback (off-main)
participant Seq as TerminalClipboardInputSequencer
participant MA as Main Actor
participant FIFO as TerminalImageTransferPreparationService (actor)
participant W as Worker Subprocess
participant FS as Filesystem (0700 dir)
C->>Seq: reserveRequestAdmission() [nonisolated atomic]
C->>MA: "Task { @MainActor }"
MA->>Seq: beginReservedClipboardRead(id)
MA->>MA: "Launch inner Task { @MainActor }"
Note over MA: keyboard events buffered by Seq
MA->>FIFO: await prepare(request, mode)
FIFO->>FIFO: enqueue job + start deadlineTask (5s budget)
FIFO->>W: spawn worker process (re-exec binary)
W->>FS: read 0600 request.json
W->>W: NSPasteboard(name:) – provider IPC in worker
W->>W: decode/downscale/tone-map/materialize
W->>FS: write 0600 response.json + image files
W-->>FIFO: exit(0)
FIFO->>FS: validate containment/symlink/type/size
FIFO->>FIFO: adoptTemporaryImageFile → move to service dir
FIFO-->>MA: TerminalPastePreparationResult
MA->>Seq: completeRequest(id) → replay buffered keys
MA->>MA: insert text / attach thumbnails
Reviews (10): Last reviewed commit: "fix: resolve paste pipeline CI compilati..." | Re-trigger Greptile |
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/TerminalImageTransferConcurrencyTests.swift`:
- Around line 31-34: Update the concurrency test to call the `@MainActor`
TerminalImageTransfer.prepare wrapper instead of
TerminalImageTransferPlanner.prepare, so it exercises
TerminalImageTransferPreparationService’s off-main-actor provider resolution and
validates the expected backgroundThreadData path.
In `@Sources/TextBoxInput.swift`:
- Around line 3008-3012: Update the preparedAttachmentsByPath construction in
uploadFileAttachments to use the dictionary initializer with a merge rule
instead of uniqueKeysWithValues. Ensure duplicate standardized paths resolve
gracefully by retaining a single prepared attachment, allowing attachment
completion mapping to proceed without trapping.
🪄 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 Plus
Run ID: ddd28e8e-708b-4fe5-b876-34ee440de210
📒 Files selected for processing (16)
Sources/GhosttyTerminalView.swiftSources/TerminalImageTransfer.swiftSources/TerminalImageTransferPreparationService.swiftSources/TerminalImageTransferPreparedContent+Cleanup.swiftSources/TerminalPasteboardReadRequest.swiftSources/TextBoxAttachment+PreparedPaste.swiftSources/TextBoxInput.swiftSources/TextBoxInputContainer+Paste.swiftSources/TextBoxInputTextView+AsyncPastePreparation.swiftSources/TextBoxPastePreparationService.swiftSources/TextBoxPastePreparedContent.swiftSources/TextBoxPreparedAttachment.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/PasteboardThreadSignalingDataProvider.swiftcmuxTests/TerminalImageTransferConcurrencyTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
589-590: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace
MainActor.assumeIsolatedwith an actual actor hop.These callbacks are described as potentially off-main.
assumeIsolatedwill trap rather than schedule UI access when that happens. Dispatch the workspace/UI work throughTask {@mainactorin ... }(or guarantee the callback executor at its source).Also applies to: 615-628
🤖 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 589 - 590, Update the callback handling around callbackContext.terminalSurface?.owningWorkspace() and the corresponding block at the additional location to use an actual Task { `@MainActor` in ... } actor hop instead of MainActor.assumeIsolated. Ensure workspace and UI access occurs inside the main-actor task, preserving the existing callback behavior for potentially off-main callers.
🤖 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
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService`+TextContents.swift:
- Around line 102-112: Add a test in TerminalPasteboardServiceTests.swift for
htmlPlainTextContents that populates the pasteboard with UTF-8 HTML via setData
for the .html type, then verifies the resulting plain text. Keep the existing
setString coverage unchanged and ensure this test exercises the
data(forType:).flatMap UTF-8 fallback branch directly.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 589-590: Update the callback handling around
callbackContext.terminalSurface?.owningWorkspace() and the corresponding block
at the additional location to use an actual Task { `@MainActor` in ... } actor hop
instead of MainActor.assumeIsolated. Ensure workspace and UI access occurs
inside the main-actor task, preserving the existing callback behavior for
potentially off-main callers.
🪄 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 Plus
Run ID: ab44e5be-b90e-43f8-86b0-37b29bce50bc
📒 Files selected for processing (12)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/PasteboardTextFidelity.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService+TextContents.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Pasteboard/HTMLPlainTextParserTests.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swiftSources/GhosttyTerminalView.swiftSources/TextBoxInput.swiftSources/TextBoxInputContainer+Paste.swiftSources/TextBoxInputTextView+AsyncPastePreparation.swiftSources/TextBoxInputTextView+PendingPasteReservation.swiftSources/TextBoxPendingPasteReservation.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swift (2)
48-70: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDecode the complete named-entity set used by HTML pasteboard content.
Only a small subset of named entities is supported; common values such as
é,€, and“remain literal becausedecodedEntityreturnsnil. Since the HTML paste path now uses this parser, localized and non-ASCII content can be corrupted. Use a complete HTML5 entity table or another standards-compliant decoder, with regression coverage.Based on the supplied HTML paste flow, this parser now supplies the plain-text result for
.htmlcontent.Also applies to: 263-276
🤖 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 `@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swift` around lines 48 - 70, Expand HTMLPlainTextParser’s decodedEntity support beyond the limited namedEntities dictionary by using a complete HTML5 entity table or standards-compliant decoder, ensuring entities such as eacute, euro, and ldquo decode correctly in HTML paste content. Add regression coverage for representative localized and non-ASCII named entities, including the examples cited.
78-121: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep malformed-tag handling linear and non-destructive.
endOfTagscans from every<to the next>or end-of-input, while the caller advances only one character when no tag end exists. Inputs with many<characters and no>become O(n²), which can reintroduce paste-time beachballing. The same logic also consumes ordinary text such as2 < 3and can swallow a hidden script’s closing tag when its body contains<, causing all following visible content to be dropped.Use a single-pass tokenizer that validates tag openers, preserves non-tag
<, and handles hidden raw-text elements by locating their matching closing tag. As per path instructions, production code must avoid repeated full-collection scans over scalable input.Also applies to: 147-167
🤖 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 `@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swift` around lines 78 - 121, Replace the per-`<` `endOfTag` scanning in the parser loop and hidden-tag handling with a single-pass tokenizer that validates tag openers before treating `<` as markup, preserves non-tag `<` text such as `2 < 3`, and advances monotonically on malformed input. For hidden raw-text elements, locate and consume only their matching closing tag so `<` characters in the body cannot suppress subsequent visible content; update the related logic around the hidden-tag flow at the referenced symbols without repeated full-input scans.Source: Path instructions
🤖 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
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swift`:
- Around line 124-129: Update the hidden-element handling in HTMLPlainTextParser
so opening hidden block tags always enter hidden mode, regardless of a trailing
slash that may belong to an unquoted attribute value; do not treat that syntax
as self-closing for elements such as script, style, or template. Preserve
closing-tag handling and ensure hidden content remains excluded from pasted
text.
---
Outside diff comments:
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swift`:
- Around line 48-70: Expand HTMLPlainTextParser’s decodedEntity support beyond
the limited namedEntities dictionary by using a complete HTML5 entity table or
standards-compliant decoder, ensuring entities such as eacute, euro, and ldquo
decode correctly in HTML paste content. Add regression coverage for
representative localized and non-ASCII named entities, including the examples
cited.
- Around line 78-121: Replace the per-`<` `endOfTag` scanning in the parser loop
and hidden-tag handling with a single-pass tokenizer that validates tag openers
before treating `<` as markup, preserves non-tag `<` text such as `2 < 3`, and
advances monotonically on malformed input. For hidden raw-text elements, locate
and consume only their matching closing tag so `<` characters in the body cannot
suppress subsequent visible content; update the related logic around the
hidden-tag flow at the referenced symbols without repeated full-input scans.
🪄 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 Plus
Run ID: b81cb1fd-37d3-4843-addb-8f8591326ee8
📒 Files selected for processing (5)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/PasteboardTextFidelity.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService+TextContents.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Pasteboard/HTMLPlainTextParserTests.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalImageTransfer.swift (1)
178-203: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftKeep
prepareSynchronouslyfrom the async planner path and make it non-public.
TerminalImageTransferPreparationService.prepare(_:mode:)awaitsTerminalImageTransferPlanner.preparethen callsprepareSynchronouslyback on the same content, so the async request path still blocks on pasteboard/provider reads and image materialization. Also, the synchronousplan(/prepare)overloads keep the same labels as the async versions, so callers withoutawaitcan still pick the blocking overload. Make synchronous preparation private to controlled legacy/test cases, and route production planner callers through the async request path.🤖 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/TerminalImageTransfer.swift` around lines 178 - 203, Remove the synchronous re-preparation from TerminalImageTransferPreparationService.prepare and ensure the async planner path uses TerminalImageTransferPlanner.prepare directly, preserving its prepared result. Make prepareSynchronously and the synchronous plan/prepare overloads private or otherwise inaccessible to production callers, while retaining them only for controlled legacy/test use; update production call sites to use the async request path.
🤖 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.
Outside diff comments:
In `@Sources/TerminalImageTransfer.swift`:
- Around line 178-203: Remove the synchronous re-preparation from
TerminalImageTransferPreparationService.prepare and ensure the async planner
path uses TerminalImageTransferPlanner.prepare directly, preserving its prepared
result. Make prepareSynchronously and the synchronous plan/prepare overloads
private or otherwise inaccessible to production callers, while retaining them
only for controlled legacy/test use; update production call sites to use the
async request path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4f428893-4c33-4649-bd11-931b4c36c440
📒 Files selected for processing (3)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextParser.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Pasteboard/HTMLPlainTextTag.swiftSources/TerminalImageTransfer.swift
This reverts commit 36f65f6.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c8f69b3. Configure here.
|
#8838 also fixes a 60 s whole-app freeze when pasting plain text, not just screenshots Reporting against SymptomPasting a copied web page into any terminal pane freezes the entire UI for ~63 s. Same with ReproSelect all of https://www.lipsum.com in a browser, copy, paste into a terminal pane. That page happens to satisfy both required conditions:
Path in v0.64.22
Isolated from cmuxSame importer, plain Swift process, same machine, so cmux is not involved in these numbers:
Only a subresource that would actually load hangs. It burns the 60 s document-read timeout and then returns nil. This part is environment-dependent (macOS 26.6.2 with a CrowdStrike Falcon network extension active), so not every user will see a full 60 s, but on the stable build every user pays subresource load latency on the main thread. Why #8838 fixes it
AskIs a stable release carrying #8838 planned? |
Summary
Root cause
The synchronous AppKit paste responder previously ran the complete pasteboard and image pipeline on the main thread. A large Retina or HDR/PQ Screen Share screenshot could trigger lazy provider resolution, TIFF/HDR ColorSync conversion, image decoding, file I/O, and initial thumbnail rasterization without yielding the run loop, beachballing the entire app. No synchronous O(number of panes) paste traversal was found.
Architecture
SIGKILLwithout wedging cmux.0700/0600; response size, file ownership, containment, symlink status, image type, and image size are validated before adoption.XMLDocumentreplaces AppKit's HTML attributed-string importer, disables external entities, strips hidden blocks, and now rejects HTML above 4 MiB before DOM creation while bounding generated text to 4 MiB.MainActor, so a key event cannot overtake a not-yet-admitted paste; request-keyed reservations cover overlapping callbacks and are removed on admission or invalidation.Regression coverage
Testing
swift test --package-path Packages/macOS/CmuxFoundation— 140 tests passedclang -std=c11 -Wall -Wextra -Werror -fsyntax-onlypassed for the atomic C shimscripts/check-pbxproj.shscripts/lint-pbxproj-test-wiring.shpython3 scripts/check-workspace-package-groups.py --checkpython3 scripts/check-package-resolved-policy.pygit diff --checkorigin/mainThe requested
python3 scripts/swift_file_length_budget.pycommand cannot run because that script and.github/swift-file-length-budget.tsvare absent from this checkout and currentorigin/main; direct per-file enforcement above was run instead. Localxcodebuild, reload, and XCUITests were intentionally not run per task instructions.#5330 coordination
PR #8817 is merged and owns composer attachment-chip thumbnail caching/rerasterization. This PR does not duplicate that work: it fixes the earlier paste-ingestion boundary and supplies the already-normalized thumbnail into the cached attachment path.
Demo video
Not applicable: there is no visual UI change. The deterministic provider-thread, process-deadline, and ordering tests exercise the formerly blocking path.
Localization audit
No user-facing strings or localized surfaces changed.
Resources/Localizable.xcstrings,web/messages/en.json, andweb/messages/ja.jsonremain untouched.Closes #8835
Summary by CodeRabbit
New Features
Bug Fixes
Reliability
Note
High Risk
Large clipboard/paste refactor touching synchronous Ghostty callbacks, terminal input ordering, and pasteboard mutation recovery; regressions could cause paste hangs, lost clipboard data, or reordered keyboard input.
Overview
Moves heavy pasteboard work off the main thread and adds process-wide ordered read/write lanes for standard and selection clipboards so Ghostty runtime callbacks, terminal paste, and app writes do not interleave or clear the board without recovery.
Pasteboard service now routes managed pasteboards through
TerminalPasteboardTransactionLanewith bounded queues, read/mutation leases, conditional writes, rollback capture (via an injectable worker snapshot provider), and write-failure recovery. Clipboard writes enqueue representations instead of clearing synchronously; one-shot capture preserves full MIME representations. AddsadoptTemporaryImageFilefor validated worker image adoption and injectableFileManager.HTML handling replaces regex-based “no visible text” checks with a bounded
HTMLPlainTextParser(XMLDocument, template normalizer, size/nesting limits). Image/RTFD paste paths use precomputed HTML outcomes and fall back to plain or RTF when HTML is rejected or whitespace-only.Terminal input ordering defers binding actions, paste, keys, socket input, and mobile gestures while a runtime clipboard read is active (
deferRuntimeInputDuringClipboardReadon the native view). Paste intent is scoped viawithRuntimeClipboardPasteIntent; clipboard requests are invalidated on surface teardown, replacement, and before native free.Small wiring: About titlebar debug copy goes through host
copyTextinstead ofNSPasteboard.generaldirectly.Reviewed by Cursor Bugbot for commit a9408be. Bugbot is set up for automated code reviews on this repo. Configure here.