Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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:
📝 WalkthroughWalkthroughUpdates docs and ghostty submodule; adds background-blur config parsing; implements a pending Ghostty search activation/resolution workflow; refactors runtime vs loaded chrome/theme composition and titlebar opacity; adds tests and a UI socket stress test; updates checksum mapping. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant AD as AppDelegate
participant TS as TerminalSurface
participant GTV as GhosttyTerminalView
participant GA as GhosttyApp
User->>AD: startOrFocusTerminalSearch()
AD->>TS: requestGhosttySearchActivation(.startSearch)
AD->>TS: performBindingAction("start_search")
alt binding succeeds
TS->>GA: binding succeeded / notify
GA->>GTV: cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded()
GTV->>GTV: create/update searchState (maybe with needle)
GTV->>AD: post focus / invoke notifier
else binding fails
TS->>TS: clearGhosttySearchActivationRequest()
AD->>GTV: fallback initialize searchState
GTV->>AD: post ghosttySearchFocus notification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
lawrencecchen
force-pushed
the
task-sync-ghostty-upstream-main
branch
from
70c6fa2 to
fcdeac5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🧹 Nitpick comments (3)
docs/ghostty-fork.md (1)
97-97: Avoid section-number coupling in HEAD note.“section 6” is brittle if sections are reordered later. Prefer referencing the commit hash directly (or both section title + hash) to keep this line stable over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ghostty-fork.md` at line 97, The HEAD note currently says "section 6 theme-picker hardening commit," which couples the note to a mutable section number; update that line to reference the commit hash (and optionally the section title) instead of the section number so it remains stable — replace "section 6 theme-picker hardening commit" with the commit SHA (e.g., <commit-hash>) or "theme-picker hardening commit (<commit-hash>)" in the HEAD note.Sources/WorkspaceContentView.swift (1)
209-218: Capture runtime opacity once per resolve pass.
defaultBackgroundOpacity()is invoked twice in this branch. Capturing it once makes the resolved/logged values deterministic within the same pass.♻️ Suggested refactor
- let runtimeBackground = defaultBackground() + let runtimeBackground = defaultBackground() + let runtimeBackgroundOpacity = defaultBackgroundOpacity() let resolved = cmuxResolveGhosttyChromeConfig( loadedConfig: loadedConfig, runtimeBackgroundColor: runtimeBackground, - runtimeBackgroundOpacity: defaultBackgroundOpacity() + runtimeBackgroundOpacity: runtimeBackgroundOpacity ) next = resolved runtimeBackgroundHex = runtimeBackground.hexString() - runtimeOpacity = defaultBackgroundOpacity() + runtimeOpacity = runtimeBackgroundOpacity🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 209 - 218, The code calls defaultBackgroundOpacity() twice causing nondeterministic values; fix by calling defaultBackgroundOpacity() once at the start of the pass (store it in a local let, e.g. let runtimeOpacityValue = defaultBackgroundOpacity()), pass that variable into cmuxResolveGhosttyChromeConfig (as runtimeBackgroundOpacity) and use the same runtimeOpacityValue when assigning runtimeOpacity, keeping runtimeBackground and runtimeBackgroundHex logic unchanged and removing the duplicate defaultBackgroundOpacity() call.Sources/ContentView.swift (1)
329-347: Use the clamped runtime opacity when writing back the resolved config.Line 345 currently stores
runtimeBackgroundOpacity(raw) even though Line 334 already computes a clamped value. Writing the clamped value keepsGhosttyConfignormalized and avoids invalid opacity values propagating.♻️ Proposed patch
func cmuxResolveGhosttyChromeConfig( loadedConfig: GhosttyConfig, runtimeBackgroundColor: NSColor, runtimeBackgroundOpacity: Double ) -> GhosttyConfig { let runtimeOpacity = GhosttyBackgroundTheme.clampedOpacity(runtimeBackgroundOpacity) let loadedOpacity = GhosttyBackgroundTheme.clampedOpacity(loadedConfig.backgroundOpacity) @@ var resolved = loadedConfig resolved.backgroundColor = runtimeBackgroundColor - resolved.backgroundOpacity = runtimeBackgroundOpacity + resolved.backgroundOpacity = runtimeOpacity return resolved }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 329 - 347, The function cmuxResolveGhosttyChromeConfig computes a clamped runtimeOpacity but then writes the raw runtimeBackgroundOpacity into resolved.backgroundOpacity; change the assignment in cmuxResolveGhosttyChromeConfig to set resolved.backgroundOpacity = runtimeOpacity (the clamped value) so GhosttyConfig remains normalized and invalid opacity values are not propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghostty`:
- Line 1: The ghostty submodule pointer in the superproject is out of sync:
superproject expects commit 51ba49a4bb74c2f3030b8a2878f4162d2624d4ca but the
submodule is at fcdeac56e3c466fb3fc77e6686c93c1fd3f02b8a; update the ghostty
submodule to the expected commit by checking out or merging the correct commit
in the ghostty repo, push that commit to the manaflow-ai/ghostty fork's main
branch, then in the superproject run git -C ghostty fetch && git -C ghostty
merge origin/main (or checkout the specific commit) and verify with git -C
ghostty merge-base --is-ancestor HEAD origin/main so the submodule HEAD matches
the superproject pointer before merging.
In `@Sources/AppDelegate.swift`:
- Around line 1743-1748: The call to
cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded is being deferred with
DispatchQueue.main.async which creates a timing gap where the function can
return true while searchState is still nil; remove the async deferral and invoke
cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(terminalSurface,
searchFocusNotifier: searchFocusNotifier) synchronously on the current execution
context (or, if it must run on the main thread, perform a main-thread safe
synchronous invocation—i.e. call directly when already on main, otherwise
dispatch synchronously to DispatchQueue.main) so that searchState is initialized
deterministically before any immediate search-active checks or overlay
activation logic runs.
---
Nitpick comments:
In `@docs/ghostty-fork.md`:
- Line 97: The HEAD note currently says "section 6 theme-picker hardening
commit," which couples the note to a mutable section number; update that line to
reference the commit hash (and optionally the section title) instead of the
section number so it remains stable — replace "section 6 theme-picker hardening
commit" with the commit SHA (e.g., <commit-hash>) or "theme-picker hardening
commit (<commit-hash>)" in the HEAD note.
In `@Sources/ContentView.swift`:
- Around line 329-347: The function cmuxResolveGhosttyChromeConfig computes a
clamped runtimeOpacity but then writes the raw runtimeBackgroundOpacity into
resolved.backgroundOpacity; change the assignment in
cmuxResolveGhosttyChromeConfig to set resolved.backgroundOpacity =
runtimeOpacity (the clamped value) so GhosttyConfig remains normalized and
invalid opacity values are not propagated.
In `@Sources/WorkspaceContentView.swift`:
- Around line 209-218: The code calls defaultBackgroundOpacity() twice causing
nondeterministic values; fix by calling defaultBackgroundOpacity() once at the
start of the pass (store it in a local let, e.g. let runtimeOpacityValue =
defaultBackgroundOpacity()), pass that variable into
cmuxResolveGhosttyChromeConfig (as runtimeBackgroundOpacity) and use the same
runtimeOpacityValue when assigning runtimeOpacity, keeping runtimeBackground and
runtimeBackgroundHex logic unchanged and removing the duplicate
defaultBackgroundOpacity() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27761b8b-8e50-4fa3-b4aa-7ce1bbddd310
📒 Files selected for processing (13)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftdocs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ghosttykit-checksums.txt
| @@ -1 +1 @@ | |||
| Subproject commit bc9be90a21997a4e5f06bf15ae2ec0f937c2dc42 | |||
| Subproject commit 51ba49a4bb74c2f3030b8a2878f4162d2624d4ca | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read the submodule SHA recorded by the superproject
expected_sha="$(git ls-tree HEAD ghostty | awk '{print $3}')"
echo "Superproject pointer (ghostty): ${expected_sha}"
# 2) Ensure submodule checkout exists and matches the pointer
if [ ! -d ghostty ]; then
echo "ghostty submodule directory not present. Initialize submodules first."
exit 1
fi
actual_sha="$(git -C ghostty rev-parse HEAD)"
echo "Checked-out ghostty HEAD: ${actual_sha}"
if [ "${expected_sha}" != "${actual_sha}" ]; then
echo "Mismatch: checked-out submodule HEAD does not match superproject pointer."
exit 1
fi
# 3) Verify the commit is on remote main
git -C ghostty fetch origin main --quiet
if git -C ghostty merge-base --is-ancestor HEAD origin/main; then
echo "OK: submodule commit is reachable from origin/main."
else
echo "FAIL: submodule commit is NOT reachable from origin/main."
exit 1
fiRepository: manaflow-ai/cmux
Length of output: 273
Submodule pointer mismatch: superproject and ghostty checkout are out of sync.
The superproject points to commit 51ba49a4bb74c2f3030b8a2878f4162d2624d4ca, but the current ghostty submodule checkout is at fcdeac56e3c466fb3fc77e6686c93c1fd3f02b8a. This inconsistency must be resolved before merging. Ensure the ghostty submodule is updated to match the pointer, that the commit is pushed to the manaflow-ai/ghostty fork's main branch, and verify with git -C ghostty merge-base --is-ancestor HEAD origin/main.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghostty` at line 1, The ghostty submodule pointer in the superproject is out
of sync: superproject expects commit 51ba49a4bb74c2f3030b8a2878f4162d2624d4ca
but the submodule is at fcdeac56e3c466fb3fc77e6686c93c1fd3f02b8a; update the
ghostty submodule to the expected commit by checking out or merging the correct
commit in the ghostty repo, push that commit to the manaflow-ai/ghostty fork's
main branch, then in the superproject run git -C ghostty fetch && git -C ghostty
merge origin/main (or checkout the specific commit) and verify with git -C
ghostty merge-base --is-ancestor HEAD origin/main so the submodule HEAD matches
the superproject pointer before merging.
| DispatchQueue.main.async { | ||
| cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded( | ||
| terminalSurface, | ||
| searchFocusNotifier: searchFocusNotifier | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid run-loop deferral for search fallback state initialization.
At Line 1743, forcing cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(...) onto DispatchQueue.main.async creates a timing gap where this function returns true while searchState can still be nil. That can desync immediate search-active checks and overlay activation paths.
Suggested fix
- if terminalSurface.performBindingAction("start_search") {
- DispatchQueue.main.async {
- cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(
- terminalSurface,
- searchFocusNotifier: searchFocusNotifier
- )
- }
+ if terminalSurface.performBindingAction("start_search") {
+ if Thread.isMainThread {
+ cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(
+ terminalSurface,
+ searchFocusNotifier: searchFocusNotifier
+ )
+ } else {
+ DispatchQueue.main.async {
+ cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(
+ terminalSurface,
+ searchFocusNotifier: searchFocusNotifier
+ )
+ }
+ }
return true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 1743 - 1748, The call to
cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded is being deferred with
DispatchQueue.main.async which creates a timing gap where the function can
return true while searchState is still nil; remove the async deferral and invoke
cmuxApplyPendingGhosttyStartSearchFallbackIfNeeded(terminalSurface,
searchFocusNotifier: searchFocusNotifier) synchronously on the current execution
context (or, if it must run on the main thread, perform a main-thread safe
synchronous invocation—i.e. call directly when already on main, otherwise
dispatch synchronously to DispatchQueue.main) so that searchState is initialized
deterministically before any immediate search-active checks or overlay
activation logic runs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (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="cmuxUITests/AutomationSocketUITests.swift">
<violation number="1" location="cmuxUITests/AutomationSocketUITests.swift:194">
P2: Avoid unconditional netcat retry here: it can resend commands that were already written and duplicate stateful operations (e.g., `surface.send_key`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: d937c39bc8
ℹ️ 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".
| if Double(normalized) != nil { | ||
| return .enabled |
There was a problem hiding this comment.
BackgroundBlur.parse currently maps any numeric string to .enabled, so zero-form values like 00 or 0.0 are treated as blur-on instead of blur-off. This makes cmux’s parsed config diverge from Ghostty semantics for zero blur intensity and can trigger incorrect chrome/theme decisions (for example, blur-change refreshes and style checks) when config writers emit normalized numeric zero values.
Useful? React with 👍 / 👎.
| panel.surface.requestGhosttySearchActivation(.searchSelection) | ||
| if !panel.performBindingAction("search_selection") { | ||
| panel.surface.clearGhosttySearchActivationRequest() |
There was a problem hiding this comment.
searchSelection() sets pendingGhosttySearchActivation(.searchSelection) but only clears it when performBindingAction returns false; unlike the new start_search path, there is no fallback/cleanup when Ghostty acknowledges the binding but never emits GHOSTTY_ACTION_START_SEARCH. In that callback-loss case, Find Selection silently does nothing and the stale pending request can later cause unrelated start-search callbacks to be resolved with the wrong request type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 605fe5c019
ℹ️ 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".
| panel.surface.requestGhosttySearchActivation(.searchSelection) | ||
| if !panel.performBindingAction("search_selection") { | ||
| panel.surface.clearGhosttySearchActivationRequest() | ||
| } |
There was a problem hiding this comment.
search_selection binding is unavailable
searchSelection() now only sets a pending request and calls performBindingAction("search_selection"); when that call returns false, it just clears pending state and exits. This regresses the previous fallback behavior where Find Selection still opened/focused the search UI, so on builds/configs where search_selection is not exposed (or temporarily fails), the command becomes a silent no-op for users.
Useful? React with 👍 / 👎.
| let pendingRequest = terminalSurface.consumeGhosttySearchActivationRequest() | ||
| let shouldDecodeNeedle = | ||
| terminalSurface.searchState != nil || pendingRequest != .startSearch |
There was a problem hiding this comment.
consumeGhosttySearchActivationRequest() is called before hopping to DispatchQueue.main, but the same pending field is written from main-thread UI paths (request.../clear...). Because this callback path is not guaranteed to be on main (the handler explicitly uses main hopping elsewhere), this introduces an unsynchronized cross-thread read/write race that can intermittently drop or misclassify pending search requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (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="cmuxUITests/AutomationSocketUITests.swift">
<violation number="1" location="cmuxUITests/AutomationSocketUITests.swift:38">
P2: Replace the fixed `RunLoop` sleep after `terminate()` with a deterministic wait on app state to reduce UI test flakiness.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| let cleanup = XCUIApplication() | ||
| cleanup.terminate() | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) |
There was a problem hiding this comment.
P2: Replace the fixed RunLoop sleep after terminate() with a deterministic wait on app state to reduce UI test flakiness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/AutomationSocketUITests.swift, line 38:
<comment>Replace the fixed `RunLoop` sleep after `terminate()` with a deterministic wait on app state to reduce UI test flakiness.</comment>
<file context>
@@ -32,11 +32,15 @@ final class AutomationSocketUITests: XCTestCase {
+
+ let cleanup = XCUIApplication()
+ cleanup.terminate()
+ RunLoop.current.run(until: Date().addingTimeInterval(0.5))
}
</file context>
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) | |
| _ = cleanup.wait(for: .notRunning, timeout: 5.0) |
There was a problem hiding this comment.
1 issue found across 1 file (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="cmuxUITests/AutomationSocketUITests.swift">
<violation number="1" location="cmuxUITests/AutomationSocketUITests.swift:665">
P2: The new socket read loop returns at the first newline, which truncates multi-line command responses and breaks parsing of surface/workspace lists.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| accumulator.append(chunk) | ||
| if let newline = accumulator.firstIndex(of: "\n") { | ||
| return String(accumulator[..<newline]) | ||
| } |
There was a problem hiding this comment.
P2: The new socket read loop returns at the first newline, which truncates multi-line command responses and breaks parsing of surface/workspace lists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/AutomationSocketUITests.swift, line 665:
<comment>The new socket read loop returns at the first newline, which truncates multi-line command responses and breaks parsing of surface/workspace lists.</comment>
<file context>
@@ -568,39 +568,108 @@ final class AutomationSocketUITests: XCTestCase {
+ }
+ if count <= 0 { break }
+ if let chunk = String(bytes: buffer[0..<count], encoding: .utf8) {
+ accumulator.append(chunk)
+ if let newline = accumulator.firstIndex(of: "\n") {
+ return String(accumulator[..<newline])
</file context>
| accumulator.append(chunk) | |
| if let newline = accumulator.firstIndex(of: "\n") { | |
| return String(accumulator[..<newline]) | |
| } | |
| accumulator.append(chunk) |
There was a problem hiding this comment.
1 issue found across 1 file (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="cmuxUITests/AutomationSocketUITests.swift">
<violation number="1" location="cmuxUITests/AutomationSocketUITests.swift:211">
P2: `waitForSocketReadyDiagnostics` returns diagnostics even when readiness times out, so callers can proceed without `socketReady == "1"`. Return `nil` on timeout so the guard correctly fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| return diagnostics["socketReady"] == "1" | ||
| } | ||
| return isReady ? lastDiagnostics : loadDiagnostics() |
There was a problem hiding this comment.
P2: waitForSocketReadyDiagnostics returns diagnostics even when readiness times out, so callers can proceed without socketReady == "1". Return nil on timeout so the guard correctly fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/AutomationSocketUITests.swift, line 211:
<comment>`waitForSocketReadyDiagnostics` returns diagnostics even when readiness times out, so callers can proceed without `socketReady == "1"`. Return `nil` on timeout so the guard correctly fails.</comment>
<file context>
@@ -178,16 +186,29 @@ final class AutomationSocketUITests: XCTestCase {
+ }
+ return diagnostics["socketReady"] == "1"
+ }
+ return isReady ? lastDiagnostics : loadDiagnostics()
}
</file context>
| return isReady ? lastDiagnostics : loadDiagnostics() | |
| return isReady ? lastDiagnostics : nil |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06da3d91cf
ℹ️ 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".
| private func socketCommand(_ command: String, responseTimeout: TimeInterval = 2.0) -> String? { | ||
| if let response = ControlSocketClient(path: socketPath, responseTimeout: responseTimeout).sendLine(command) { | ||
| return response | ||
| } | ||
| return socketCommandViaNetcat(command, responseTimeout: responseTimeout) |
There was a problem hiding this comment.
socketCommand falls back to socketCommandViaNetcat whenever ControlSocketClient.sendLine returns nil, but sendLine can return nil after it has already connected and written the command (for example, on read timeout). In that case, mutating commands such as new_workspace or send_key_surface are executed twice, which can create duplicate side effects and make this UI test flaky under slow CI timing. The fallback should only be used for pre-send failures (or retries should be made idempotent).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb008d3a1
ℹ️ 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".
Sources/AppDelegate.swift
Outdated
| if let surfaceId = tabManager.focusedPanelId(for: workspaceId) { | ||
| return (workspaceId: workspaceId, surfaceId: surfaceId) | ||
| } |
There was a problem hiding this comment.
resolvedAutomationSocketStressTarget returns tabManager.focusedPanelId immediately, but that ID can belong to a browser panel. The stress loop then calls send_key_surface <id> enter and expects OK, so any workspace focused on a non-terminal panel will deterministically fail this new UI-test harness even though terminal surfaces are available (the terminal-only fallbacks below are never reached once this early return runs). Restrict this first branch to focused terminal panels (or validate the ID is terminal) before using it as the send-key target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:2906">
P2: Use a terminal panel ID when selecting the stress-test target; `focusedPanelId` can point to non-terminal panels and breaks `send_key_surface`.</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.
1 issue found across 1 file (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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:2823">
P2: Moving the stress loop to a global queue introduces unsynchronized concurrent writes to the shared UI-test diagnostics file, which can drop `automationSocketStress*` fields and make stress-test completion/status flaky.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| let workspaceId = target.workspaceId.uuidString | ||
| let surfaceId = target.surfaceId.uuidString | ||
| DispatchQueue.global(qos: .userInitiated).async { [weak self] in |
There was a problem hiding this comment.
P2: Moving the stress loop to a global queue introduces unsynchronized concurrent writes to the shared UI-test diagnostics file, which can drop automationSocketStress* fields and make stress-test completion/status flaky.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 2823:
<comment>Moving the stress loop to a global queue introduces unsynchronized concurrent writes to the shared UI-test diagnostics file, which can drop `automationSocketStress*` fields and make stress-test completion/status flaky.</comment>
<file context>
@@ -2820,51 +2820,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
-
- trace.append(
- "iteration\(iteration)=pingBefore:\(pingBefore ?? "<nil>"),send:\(sendResponse ?? "<nil>"),pingAfter:\(pingAfter ?? "<nil>"),list:\(listResponse ?? "<nil>")"
+ DispatchQueue.global(qos: .userInitiated).async { [weak self] in
+ self?.performAutomationSocketStressLoop(
+ socketPath: socketPath,
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed9da3e24b
ℹ️ 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".
| let workspaceListResponse = TerminalController.probeSocketCommand( | ||
| "list_workspaces", | ||
| at: socketPath, | ||
| timeout: 1.0 | ||
| ) |
There was a problem hiding this comment.
runAutomationSocketStressUITestAttempt is scheduled on DispatchQueue.main, but it then synchronously probes list_workspaces/list_surfaces over the control socket. Those handlers resolve data via DispatchQueue.main.sync in TerminalController, so this path can block the handler waiting on main while main is blocked waiting for the socket reply (until timeout), causing the new stress harness to stay in waiting/failed despite a healthy listener. Execute these preflight probes from a background queue (or avoid commands that require main-sync) before parsing IDs.
Useful? React with 👍 / 👎.
| var payload = loadUITestDiagnostics(at: path) | ||
| for (key, value) in updates { | ||
| payload[key] = value | ||
| } |
There was a problem hiding this comment.
updateUITestDiagnosticsIfNeeded performs a read-modify-write on the diagnostics JSON without synchronization, and this helper is invoked from the background stress loop while other diagnostics writers on main also rewrite the same file. These concurrent writes can overwrite each other and drop fields like automationSocketStressDone or trace data, making the UI test completion wait flaky. Use a single serial queue/lock for all diagnostics file writes.
Useful? React with 👍 / 👎.
1 participant
Summary
ghosttyfrombc9be90a21997a4e5f06bf15ae2ec0f937c2dc42to51ba49a4bb74c2f3030b8a2878f4162d2624d4cadocs/ghostty-fork.mdfor the refreshed retained patch stack on upstreammainxcframework-51ba49a4bb74c2f3030b8a2878f4162d2624d4caTesting
cd ghostty && zig build -Demit-xcframework=true -Demit-macos-app=false -Dxcframework-target=universal -Doptimize=ReleaseFastGHOSTTY_SHA=51ba49a4bb74c2f3030b8a2878f4162d2624d4ca ./scripts/download-prebuilt-ghosttykit.sh./scripts/setup.sh./scripts/reload.sh --tag task-sync-ghostty-upstreamSummary by cubic
Syncs our
ghosttyfork with upstreammain, pins the matchingGhosttyKitxcframework checksum, and updates fork docs. Keeps the macOS 26 main titlebar opaque, hardens Find/search activation (pending-request tracking, safe fallback, ignore unsolicited opens), and refines chrome theme resolution (loaded-vs-runtime background, composited chrome opacity,background-blur).send_key_surface/list_surfaces; runs underallowAlland uses an in-app stress harness with diagnostics, socket health checks, retries, per-command timeouts, a background-thread loop, direct UNIX client withncfallback, smartercmuxCLI discovery withenv cmuxfallback, forcedCMUX_SOCKET_MODE, and now resolves the target workspace/surface via socket queries.background-blurparsing, titlebar composited opacity, chrome config resolution (runtime-vs-loaded), and search activation decoding and fallbacks.Written for commit ed9da3e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores