Use Ghostty's queued text path for deferred terminal sends - #3124
lawrencecchen wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces delayed Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as AppDelegate
participant Panel as TerminalPanel
participant Surface as TerminalSurface
Caller->>Panel: sendTextToTerminalSurface(text, preferredPanelId?, beforeSend?)
Panel->>Panel: resolve target panel (by id or active)
alt panel found
Panel->>Surface: terminalPanel.sendText(text, onDispatch: beforeSend)
alt surface ready
Surface->>Surface: writeTextData(...)
Surface->>Panel: invoke onDispatch()
Panel-->>Caller: return true
else surface not ready
Surface->>Surface: enqueue PendingSocketInput(text, onDispatch)
Panel-->>Caller: return false
end
else no panel found
Panel-->>Caller: return false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 replaces the complex Confidence Score: 5/5Safe to merge — clean simplification with no P0/P1 issues and regression coverage for the deferred-surface path All findings are at P2 or below. The refactor correctly delegates deferred sends to the existing No files require special attention Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (AppDelegate)
participant STTS as sendTextToTerminalSurface (static)
participant TP as TerminalPanel
participant TS as TerminalSurface
participant Q as pendingSocketInputQueue
Caller->>STTS: sendTextToTerminalSurface(text, in: workspace)
STTS->>STTS: resolveTerminalPanelForTextSend()
alt No terminal panel found
STTS-->>Caller: return false
else Terminal panel found
STTS->>STTS: beforeSend?()
STTS->>TP: sendText(text)
TP->>TS: surface.sendText(text)
alt surface != nil (live)
TS->>TS: writeTextData(data, to: surface)
else surface == nil (not ready)
TS->>Q: enqueuePendingSocketInput(.text(data))
TS->>TS: requestBackgroundSurfaceStartIfNeeded()
Note over Q,TS: Queue flushed via flushPendingSocketInputIfNeeded() when surface attaches
end
STTS-->>Caller: return true
end
Reviews (1): Last reviewed commit: "Use TerminalSurface queue for deferred t..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/ShortcutAndCommandPaletteTests.swift (1)
237-253: Use a payload above the large-text threshold.This verifies missing-surface queueing, but the 21-byte payload does not cover the large text path this PR is targeting. Use a payload over 256 bytes so the regression protects deferred large inserts too. Based on learnings, prefer
ghostty_surface_textfor large socket text payloads; threshold ispasteTextThreshold = 256, while short text goes throughsendTextEvent.Suggested test adjustment
var beforeSendCalled = false - let payload = "<button>Save</button>" + let payload = String(repeating: "<button>Save</button>", count: 20) + XCTAssertGreaterThan(payload.utf8.count, 256) XCTAssertTrue( AppDelegate.sendTextToTerminalSurface( payload,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/ShortcutAndCommandPaletteTests.swift` around lines 237 - 253, The test uses a 21-byte payload so it never exercises the large-text path; change the payload used in the AppDelegate.sendTextToTerminalSurface call to be larger than pasteTextThreshold (256 bytes) and use a large-text style string such as repeating "ghostty_surface_text" (or repeat "A") until payload.utf8.count > 256 so the code path that queues large socket text is exercised; keep the rest of the assertions (beforeSendCalled, terminalPanel.surface.debugPendingSocketInputSnapshot checks) but update expected snapshot.bytes to match the new payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/ShortcutAndCommandPaletteTests.swift`:
- Around line 237-253: The test uses a 21-byte payload so it never exercises the
large-text path; change the payload used in the
AppDelegate.sendTextToTerminalSurface call to be larger than pasteTextThreshold
(256 bytes) and use a large-text style string such as repeating
"ghostty_surface_text" (or repeat "A") until payload.utf8.count > 256 so the
code path that queues large socket text is exercised; keep the rest of the
assertions (beforeSendCalled,
terminalPanel.surface.debugPendingSocketInputSnapshot checks) but update
expected snapshot.bytes to match the new payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b7ea453-6e8e-4c8d-8a59-73c8e8748ef9
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b6be78420
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/ReactGrab.swift`:
- Around line 87-267: The pasteback extractor can carry C0/C1 control scalars
into notifications; update the Swift-side sanitizer (the filtered(_:) routine
used by BrowserPickerMessageHandler and any pasteback handling) to strip all
control characters except \n and \t and also remove known
dangerousScalars/zero-width/BiDi characters (mirror TerminalController's
filtering logic), and ensure ReactGrabPastebackContentExtractor's fallback
content is passed through that sanitizer before notifying (references:
ReactGrabPastebackContentExtractor, invocationScript, installerSource,
filtered(_:), and TerminalController.dangerousScalars).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f17fd54-4dc7-4be2-b7b2-ce7cf7953924
📒 Files selected for processing (6)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/Panels/ReactGrab.swiftSources/Panels/TerminalPanel.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/GhosttyTerminalView.swift
- Sources/AppDelegate.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7291ea2df3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/ReactGrab.swift">
<violation number="1" location="Sources/Panels/ReactGrab.swift:181">
P2: Consecutive identical blocks are being deduplicated, which can silently drop valid repeated content from pasteback output.</violation>
<violation number="2" location="Sources/Panels/ReactGrab.swift:200">
P1: Avoid passing PRE/CODE output through the whitespace normalizer; it collapses indentation and can corrupt pasted code snippets.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/ReactGrab.swift`:
- Around line 188-192: The code currently falls back to element.href when
getAttribute('href') is falsy, causing empty href attributes to resolve to the
document URL; change the logic in the inline link handling (the block using
hasBlockChild, renderInline, textFromNode) to check element.hasAttribute('href')
first: if hasAttribute('href') use the raw getAttribute('href') value (even if
empty) and if not present then fall back to element.href, then treat an empty
string href as "no link" and return linkText instead of resolving to the
document URL.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 658dd5a6-0b68-43c2-b820-8a030a5e02e3
📒 Files selected for processing (2)
Sources/Panels/ReactGrab.swiftcmuxTests/BrowserPanelTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/BrowserPanelTests.swift (1)
392-501: Add one fallback-path regression for the extractor.These runtime tests cover the formatted-content cases well, but none forces the
fallbackContentLiteralbranch when no readable blocks are produced. A small empty-elements case would protect the safe fallback behavior called out by this PR.🧪 Optional fallback coverage
+ func testPastebackExtractorFallsBackWhenNoReadableElementsAreFound() async throws { + let panel = BrowserPanel(workspaceId: UUID()) + let fallbackLiteral = try XCTUnwrap(cmuxJavaScriptStringLiteral("fallback\ntext")) + + let result = try await panel.evaluateJavaScript( + """ + \(ReactGrabPastebackContentExtractor.invocationScript( + elementsExpression: "[]", + fallbackContentLiteral: fallbackLiteral + )) + """ + ) as? String + + XCTAssertEqual(result, "fallback\ntext") + }Based on learnings, tests in
**/*Test*.swiftshould verify observable runtime behavior through executable paths rather than source-code shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserPanelTests.swift` around lines 392 - 501, Add a regression test that forces the extractor's fallback path by creating an element that produces no readable blocks and asserting the returned string equals the provided fallback; specifically add a new async test (e.g., testPastebackExtractorUsesFallbackWhenNoReadableBlocks) in BrowserPanelTests.swift that constructs an empty target element HTML, obtains htmlLiteral and a fallbackLiteral via cmuxJavaScriptStringLiteral, invokes ReactGrabPastebackContentExtractor.invocationScript(elementsExpression: "[document.getElementById('target')]", fallbackContentLiteral: fallbackLiteral) through panel.evaluateJavaScript, and XCTAssertEqual the result to the fallbackLiteral's unescaped value (the expected fallback string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/BrowserPanelTests.swift`:
- Around line 392-501: Add a regression test that forces the extractor's
fallback path by creating an element that produces no readable blocks and
asserting the returned string equals the provided fallback; specifically add a
new async test (e.g., testPastebackExtractorUsesFallbackWhenNoReadableBlocks) in
BrowserPanelTests.swift that constructs an empty target element HTML, obtains
htmlLiteral and a fallbackLiteral via cmuxJavaScriptStringLiteral, invokes
ReactGrabPastebackContentExtractor.invocationScript(elementsExpression:
"[document.getElementById('target')]", fallbackContentLiteral: fallbackLiteral)
through panel.evaluateJavaScript, and XCTAssertEqual the result to the
fallbackLiteral's unescaped value (the expected fallback string).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 657f61a2-eba9-48eb-96d0-c7cf6f30908f
📒 Files selected for processing (2)
Sources/Panels/ReactGrab.swiftcmuxTests/BrowserPanelTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/ReactGrab.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
AppDelegate.sendTextWhenReadywith directTerminalSurface.sendTextroutingghostty_surface_textpath and deferred queue instead of app-level observers and a 3 second timeoutTesting
./scripts/setup.sh./scripts/reload.sh --tag txtapiIssues
Summary by cubic
Route all terminal text inserts through Ghostty’s queued text API to remove app-level deferral logic and avoid 3s timeouts. Adds a more robust ReactGrab pasteback extractor, defers
beforeSenduntil text dispatch, and treats empty links as plain text. Addresses the Linear task to use the right Ghostty API for large text inserts and remove thesendTextWhenReadycode smell.New Features
Bug Fixes
beforeSendruns only when text is sent to a live surface; queued sends defer the callback. Added tests for extractor formatting (headings/links/code, indentation) and queued-callback behavior.hrefvalues are ignored during pasteback and rendered as plain text.Written for commit 004a2c5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores