Fix focus reporting leak during pane creation (#2446)#2511
Conversation
… config Pass desiredFocusState through surfaceConfig.focused so background panes start unfocused at the Ghostty level. This prevents CSI I/O focus sequences from leaking into shells during pane creation when DECSET 1004 is enabled. Also extracts GhosttyKit build/cache logic into ensure-ghosttykit.sh for reuse by both setup.sh and reload.sh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughInitial surface focus is now explicitly seeded and propagated through Ghostty at creation (surfaces start unfocused by default). GhosttyKit xcframework caching/build logic was extracted to a new lock-backed script; setup and reload scripts now invoke it. The ghostty submodule reference was updated. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Host App (cmux)
participant Swift as GhosttyTerminalView / TerminalSurface
participant C as Ghostty C API
participant Renderer as Renderer/TermIO / IO Thread
App->>Swift: create TerminalSurface (desiredFocusState = false)
Swift->>C: prepare surfaceConfig (focused = desiredFocusState)
C->>C: ghostty_surface_new(surfaceConfig)
C-->>Swift: createdSurface
Swift->>C: ghostty_surface_set_focus(createdSurface, desiredFocusState)
Swift->>Renderer: seed renderer/termio focus bookkeeping (focused = desiredFocusState)
Note over Renderer,Swift: Converge/re-apply focus during init before IO thread runs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/reload.sh (1)
282-283: Consider using$SCRIPT_DIRfor consistency withsetup.sh.The script invocation works correctly since
ensure-ghosttykit.shresolves its own directory at startup. However,setup.shuses"$SCRIPT_DIR/ensure-ghosttykit.sh"while this uses"$PWD/scripts/ensure-ghosttykit.sh". For consistency and resilience whenPWDdiffers from repo root:Suggested change
Add
SCRIPT_DIRresolution at the top ofreload.sh(if not already present) and use:-"$PWD/scripts/ensure-ghosttykit.sh" +"$(dirname "${BASH_SOURCE[0]}")/ensure-ghosttykit.sh"Or, since
reload.shalready operates in$PWD, this is acceptable as-is givenensure-ghosttykit.sh's self-resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` around lines 282 - 283, The invocation in reload.sh uses "$PWD/scripts/ensure-ghosttykit.sh" which is inconsistent with setup.sh; update reload.sh to resolve and use SCRIPT_DIR like setup.sh by adding the same SCRIPT_DIR resolution block at the top of reload.sh (the code that sets SCRIPT_DIR based on the script's directory) and replace the "$PWD/..." invocation with "$SCRIPT_DIR/ensure-ghosttykit.sh" so ensure-ghosttykit.sh is invoked relative to the script location; reference SCRIPT_DIR, reload.sh, setup.sh, and ensure-ghosttykit.sh when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ghostty-fork.md`:
- Around line 118-129: The docs currently state "Status: working tree change
(not yet committed)" for the Ghostty fork; fix this by committing and pushing
the changes in the ghostty submodule (the changes that touch include/ghostty.h,
macos/Sources/Ghostty/Surface View/SurfaceView.swift, src/Surface.zig,
src/apprt/embedded.zig, src/termio/stream_handler.zig) to the
manaflow-ai/ghostty fork, note the resulting commit SHA, then update the
docs/ghostty-fork.md entry to replace the status line with the actual commit
hash (e.g. "Status: commit <SHA>") and optionally add the commit link or SHA(s)
in the summary so the parent repo can pin and reproduce the behavior.
---
Nitpick comments:
In `@scripts/reload.sh`:
- Around line 282-283: The invocation in reload.sh uses
"$PWD/scripts/ensure-ghosttykit.sh" which is inconsistent with setup.sh; update
reload.sh to resolve and use SCRIPT_DIR like setup.sh by adding the same
SCRIPT_DIR resolution block at the top of reload.sh (the code that sets
SCRIPT_DIR based on the script's directory) and replace the "$PWD/..."
invocation with "$SCRIPT_DIR/ensure-ghosttykit.sh" so ensure-ghosttykit.sh is
invoked relative to the script location; reference SCRIPT_DIR, reload.sh,
setup.sh, and ensure-ghosttykit.sh when making the change.
🪄 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: 60c7689c-34e2-4154-8aaf-10576b811d0d
📒 Files selected for processing (6)
Sources/GhosttyTerminalView.swiftdocs/ghostty-fork.mdghostty.hscripts/ensure-ghosttykit.shscripts/reload.shscripts/setup.sh
Greptile SummaryThis PR fixes spurious Key points:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant WS as Workspace/AppKit Focus Path
participant TS as TerminalSurface (Swift)
participant GC as Ghostty C Surface
Note over TS: desiredFocusState = false (new default)
TS->>GC: createSurface(surfaceConfig.focused = false)
Note over GC: Surface starts unfocused<br/>(no CSI I emitted)
TS->>GC: ghostty_surface_set_focus(surface, false)
Note over TS,GC: Re-sync guard — converges any<br/>focus change during init
alt Pane is the focused pane
WS->>TS: setFocus(true)
TS->>TS: desiredFocusState = true
TS->>GC: ghostty_surface_set_focus(surface, true)
Note over GC: CSI I emitted (legitimate focus-in)
else Pane is a background pane
Note over TS,GC: desiredFocusState stays false<br/>No CSI I/O sequences emitted
end
Reviews (1): Last reviewed commit: "Fix focus reporting leak: seed initial f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| exit 1 | ||
| fi | ||
|
|
||
| "$PWD/scripts/ensure-ghosttykit.sh" |
There was a problem hiding this comment.
$PWD-based path vs $SCRIPT_DIR
reload.sh uses $PWD/scripts/ensure-ghosttykit.sh, but setup.sh uses the more robust $SCRIPT_DIR/ensure-ghosttykit.sh. If reload.sh is invoked with an absolute path from a directory other than the project root (e.g., /abs/path/to/scripts/reload.sh --tag foo from /tmp), $PWD will not be the project root and the path to ensure-ghosttykit.sh won't resolve correctly.
Consider deriving SCRIPT_DIR at the top of reload.sh (the same way setup.sh and ensure-ghosttykit.sh do) and using it here:
| "$PWD/scripts/ensure-ghosttykit.sh" | |
| "$SCRIPT_DIR/scripts/ensure-ghosttykit.sh" |
Or, since reload.sh itself lives in scripts/, simply compute SCRIPT_DIR at the top of reload.sh:
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"and then call:
"$SCRIPT_DIR/ensure-ghosttykit.sh"| extract_surface_config_block() { | ||
| local path="$1" | ||
| python3 - "$path" <<'PY' | ||
| from pathlib import Path | ||
| import sys | ||
|
|
||
| text = Path(sys.argv[1]).read_text() | ||
| start = text.index("typedef struct {\n ghostty_platform_e platform_tag;") | ||
| end = text.index("} ghostty_surface_config_s;") + len("} ghostty_surface_config_s;") | ||
| print(text[start:end], end="") | ||
| PY | ||
| } |
There was a problem hiding this comment.
str.index() raises ValueError on a missing substring
Path.index() in Python raises ValueError (not a clean exit) when the target string is not found. Because this function is called from a $(...) subshell under set -euo pipefail, the outer script will abort on failure — but the developer/user will see a raw Python traceback rather than the informative "out of sync" message. Consider using a try/except block for a cleaner error:
try:
start = text.index("typedef struct {\n ghostty_platform_e platform_tag;")
end = text.index("} ghostty_surface_config_s;") + len("} ghostty_surface_config_s;")
except ValueError as e:
print(f"error: ghostty_surface_config_s block not found in {sys.argv[1]}: {e}", file=sys.stderr)
sys.exit(1)| LOCK_START=$SECONDS | ||
| while ! mkdir "$LOCK_DIR" 2>/dev/null; do | ||
| if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then | ||
| echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..." | ||
| rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR" | ||
| continue | ||
| fi | ||
| echo "==> Waiting for GhosttyKit cache lock for $GHOSTTY_KEY..." | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
LOCK_START, causing tight spin after first timeout
After the lock is forcibly removed at line 96 (rmdir/rm -rf) and continue jumps to the top of the loop, LOCK_START is never reset. If another process immediately re-creates the lock directory (active contention), the condition (( SECONDS - LOCK_START > LOCK_TIMEOUT )) will be true on the very next check, so the loop will spin without the sleep 1 delay — repeatedly force-removing the lock in a tight loop rather than once and then falling back to polite polling.
Reset LOCK_START after clearing the stale lock so the full 300-second window applies before the next forced eviction:
| LOCK_START=$SECONDS | |
| while ! mkdir "$LOCK_DIR" 2>/dev/null; do | |
| if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then | |
| echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..." | |
| rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR" | |
| continue | |
| fi | |
| echo "==> Waiting for GhosttyKit cache lock for $GHOSTTY_KEY..." | |
| sleep 1 | |
| done | |
| if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then | |
| echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..." | |
| rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR" | |
| LOCK_START=$SECONDS | |
| continue | |
| fi |
| @@ -450,6 +458,9 @@ typedef struct { | |||
| const char* initial_input; | |||
| bool wait_after_command; | |||
| ghostty_surface_context_e context; | |||
| ghostty_surface_io_mode_e io_mode; | |||
| ghostty_io_write_cb io_write_cb; | |||
| void* io_write_userdata; | |||
| } ghostty_surface_config_s; | |||
There was a problem hiding this comment.
ghostty.h is ahead of the submodule — clean checkouts will fail the sync guard
ghostty.h now declares focused, ghostty_surface_io_mode_e, ghostty_io_write_cb, io_mode, io_write_cb, and io_write_userdata, but the PR description explicitly states the corresponding Zig changes in the submodule (Surface.zig, embedded.zig, stream_handler.zig, and ghostty/include/ghostty.h) are not yet committed.
The sync check introduced in ensure-ghosttykit.sh (lines 50–56) compares ghostty_surface_config_s in the root ghostty.h against ghostty/include/ghostty.h inside the submodule. For any developer or CI job that checks out this branch without the uncommitted submodule working-tree changes present, that check will fail immediately with:
error: ghostty_surface_config_s is out of sync between ghostty.h and ghostty/include/ghostty.h.
This also means the core fix (surfaceConfig.focused = desiredFocusState) won't actually seed the initial focus state in the C surface until the Zig implementation that reads that field is shipped and the submodule pointer is updated. Without the committed Zig side, the desiredFocusState = false change still prevents the post-creation ghostty_surface_set_focus(surface, false) call from being a no-op (it will still send a focus-loss transition to an already-running surface), partially defeating the goal.
Before merging, please:
- Commit the Zig changes to the
manaflow-ai/ghosttyfork. - Update the submodule pointer so
ghostty/include/ghostty.hmatches the rootghostty.h. - Verify CI passes end-to-end on a clean clone.
![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 6 files
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="scripts/ensure-ghosttykit.sh">
<violation number="1" location="scripts/ensure-ghosttykit.sh:94">
P1: Do not delete the cache lock solely because this process waited >300s; it can remove an active lock and allow concurrent GhosttyKit builds/copies.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| LOCK_TIMEOUT=300 | ||
| LOCK_START=$SECONDS | ||
| while ! mkdir "$LOCK_DIR" 2>/dev/null; do | ||
| if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then |
There was a problem hiding this comment.
P1: Do not delete the cache lock solely because this process waited >300s; it can remove an active lock and allow concurrent GhosttyKit builds/copies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/ensure-ghosttykit.sh, line 94:
<comment>Do not delete the cache lock solely because this process waited >300s; it can remove an active lock and allow concurrent GhosttyKit builds/copies.</comment>
<file context>
@@ -0,0 +1,141 @@
+LOCK_TIMEOUT=300
+LOCK_START=$SECONDS
+while ! mkdir "$LOCK_DIR" 2>/dev/null; do
+ if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then
+ echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..."
+ rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR"
</file context>
…rting-leak # Conflicts: # ghostty # ghostty.h
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 `@scripts/ensure-ghosttykit.sh`:
- Around line 96-100: The current mkdir-based lock loop (using LOCK_DIR,
LOCK_START, LOCK_TIMEOUT and SECONDS) force-removes the lock directory after
timeout which can delete a live lock; instead, implement a safe-stale-check:
when the timeout is reached, inspect a pid/owner file inside "$LOCK_DIR" (e.g.,
"$LOCK_DIR/pid"), read the PID, and use kill -0 PID to determine if the locking
process is still alive; only remove rmdir/"rm -rf $LOCK_DIR" if the owner
process is gone (or pid file missing/outdated), otherwise continue waiting (or
renew timeout); apply the same change to the later removal logic around the
other timeout branch as well.
🪄 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: 6dd8d00f-e1d7-4da7-95f2-6e53283f159e
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftdocs/ghostty-fork.mdghosttyscripts/ensure-ghosttykit.sh
✅ Files skipped from review due to trivial changes (2)
- ghostty
- Sources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ghostty-fork.md
| while ! mkdir "$LOCK_DIR" 2>/dev/null; do | ||
| if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then | ||
| echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..." | ||
| rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR" | ||
| continue |
There was a problem hiding this comment.
Avoid deleting potentially live locks after timeout.
At Line 98-100, a waiting process force-removes the lock directory after 300s. A legitimate long build can exceed that and get its live lock deleted, allowing concurrent cache writes for the same key.
Suggested fix
LOCK_TIMEOUT=300
LOCK_START=$SECONDS
+LOCK_OWNER_PID_FILE="$LOCK_DIR/pid"
while ! mkdir "$LOCK_DIR" 2>/dev/null; do
+ if [[ -f "$LOCK_OWNER_PID_FILE" ]]; then
+ read -r lock_pid < "$LOCK_OWNER_PID_FILE" || lock_pid=""
+ if [[ -n "$lock_pid" ]] && ! kill -0 "$lock_pid" 2>/dev/null; then
+ echo "==> Removing orphaned GhosttyKit cache lock (pid $lock_pid)..."
+ rm -rf "$LOCK_DIR"
+ continue
+ fi
+ fi
if (( SECONDS - LOCK_START > LOCK_TIMEOUT )); then
- echo "==> Lock stale (>${LOCK_TIMEOUT}s), removing and retrying..."
- rmdir "$LOCK_DIR" 2>/dev/null || rm -rf "$LOCK_DIR"
- continue
+ echo "error: timed out waiting for GhosttyKit cache lock for $GHOSTTY_KEY" >&2
+ exit 1
fi
echo "==> Waiting for GhosttyKit cache lock for $GHOSTTY_KEY..."
sleep 1
done
-trap 'rmdir "$LOCK_DIR" >/dev/null 2>&1 || true' EXIT
+echo "$$" > "$LOCK_OWNER_PID_FILE"
+trap 'rm -rf "$LOCK_DIR" >/dev/null 2>&1 || true' EXITAlso applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ensure-ghosttykit.sh` around lines 96 - 100, The current mkdir-based
lock loop (using LOCK_DIR, LOCK_START, LOCK_TIMEOUT and SECONDS) force-removes
the lock directory after timeout which can delete a live lock; instead,
implement a safe-stale-check: when the timeout is reached, inspect a pid/owner
file inside "$LOCK_DIR" (e.g., "$LOCK_DIR/pid"), read the PID, and use kill -0
PID to determine if the locking process is still alive; only remove rmdir/"rm
-rf $LOCK_DIR" if the owner process is gone (or pid file missing/outdated),
otherwise continue waiting (or renew timeout); apply the same change to the
later removal logic around the other timeout branch as well.
1 participant
Summary
desiredFocusState = falseso new surfaces opt into focus only when the workspace/AppKit focus path explicitly requests it, preventing CSI I/O focus sequences from leaking into shells during pane creation.desiredFocusStatethroughsurfaceConfig.focusedso Ghostty surfaces can be seeded as unfocused at the C level, eliminating the window where a background pane briefly appears focused.scripts/ensure-ghosttykit.shfor reuse by bothsetup.shandreload.sh.Note: Ghostty submodule changes (initial focus seeding in
Surface.zig/embedded.zig, DECSET 1004 side-effect-free enablement instream_handler.zig) are in the working tree but not yet committed to the fork. Theghostty.hheader and fork docs are updated in this PR to reflect the intended API.Closes #2446
Test plan
CSI Ion creation🤖 Generated with Claude Code
Summary by cubic
Start new Ghostty surfaces unfocused and emit focus sequences only on real focus changes. This stops stray CSI I/O with DECSET 1004 enabled and prevents background panes from briefly appearing focused.
Bug Fixes
desiredFocusState = false, pass through assurfaceConfig.focused, and re-apply post‑creation so runtime focus matches any changes during init.ghosttysubmodule andghostty.hto seed initial focus and keep DECSET 1004 enablement side‑effect free.Refactors
scripts/ensure-ghosttykit.shand invoke it fromsetup.shandreload.sh.GhosttyKit.xcframework, pinned checksum for the mergedghosttySHA, and updated fork docs.Written for commit 7a89bec. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores