Terminal usability: clipboard, scrollback, mouse, modifier keys - #4
Merged
Conversation
Wires libghostty-vt's key + mouse + paste + formatter encoders into the GTK4 surface so the terminal pane behaves like users expect: copy/paste to the system clipboard, scroll the scrollback, click-drag selection, mouse pass-through to TTY apps, and proper Shift+Tab / Shift+Enter handling for Claude Code. User-visible: - Cmd+V / Alt+V / Ctrl+Shift+V paste the system clipboard, sanitized via ghostty_paste_encode and bracketed-paste-wrapped when DECSET 2004 is on. 4 MiB cap, 64 KiB chunked PTY writes. - Cmd+C / Alt+C / Ctrl+Shift+C copy the current selection via the formatter API (preserves interior whitespace, unwraps soft-wrap). PRIMARY clipboard populated on Linux. Bare Ctrl+C stays as SIGINT. - Mouse wheel / two-finger trackpad scrolls the scrollback. Smooth scroll on macOS scaled by cell height. Any input-producing key snaps to the bottom. - Click-drag selects cells with a translucent accent overlay. Selection clears on PTY output and on resize. - Mouse-tracking apps (vim with :set mouse=a, htop, tmux) get encoded click/drag/release/wheel events. Shift bypasses tracking for local selection (xterm convention). - Alt-screen wheel is translated to ArrowUp/Down keystrokes so vim, less, jed, etc. respond to the trackpad without :set mouse=a. - handleKey now flows through libghostty-vt's key encoder, so Shift+Tab (CSI Z), Shift+Enter (xterm modifyOtherKeys / Kitty CSI-u), application-cursor mode, and macOS Option-as-Unicode-compose all work correctly. Implementation: - Six new internal/ghostty cgo files: paste.go, key.go, mouse.go, formatter.go, plus tests. internal/ghostty/terminal.go grows ScrollViewportDelta/ToBottom, BracketedPasteEnabled, KittyKeyboardFlags, MouseTrackingActive, AltScreenActive. - New cmd/roost/keymap.go is pure Go (no cgo leak) — maps GDK keyvals to the package's Go-typed Key/Mods enums. - New cmd/roost/selection.go owns the drag model + ribbon-rect math. - DA's EventControllerKey moved to PhaseCapture so GTK's focus chain doesn't eat Tab / Shift+Tab. - activeSession() uses C-pointer comparison (coreglib.InternObject( page).Native()) instead of *adw.TabPage Go-pointer map lookup. Same Go-pointer-key bug exists in ~7 other call sites in app.go; full sweep tracked in a follow-up plan. Out of scope (explicit defers): IME / dead-key composition, OSC 52 clipboard, double/triple-click word/line selection, paste-confirmation dialog for large multi-line pastes. Docs updated: keybindings (full Clipboard + Mouse sections, expanded Terminal-keys table for Shift+Tab/Shift+Enter/app-cursor/Option), first-run (scroll/select/paste behavior), architecture (new files + encoder threading rules), spec (Phase 2 keybindings paragraph), README (broadened keybindings link). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds libghostty key/mouse/paste encoders, system clipboard copy/paste (bracketed paste, size cap), viewport-relative mouse selection with translucent overlay, smooth fractional scroll quantization, and GTK gesture/shortcut wiring to encode and route input to PTYs and update render/selection state. Changes
Sequence DiagramssequenceDiagram
participant User
participant GTK as GTK Events
participant App
participant Clipboard as System Clipboard
participant Ghost as libghostty
participant PTY
User->>GTK: Press paste shortcut
GTK->>App: Shortcut callback
App->>App: Resolve active session / focus check
App->>Clipboard: Read clipboard (async)
Clipboard-->>App: Clipboard bytes
App->>Ghost: BracketedPasteEnabled?
alt bracketed
App->>Ghost: EncodePaste(bytes, true)
else not bracketed
App->>Ghost: EncodePaste(bytes, false)
end
App->>App: Chunk encoded bytes (<=4MiB cap enforced)
loop per chunk
App->>PTY: QueueWrite chunk via GTK idle
end
PTY-->>User: Injected input
sequenceDiagram
participant User
participant GTK as GTK Gestures
participant Session
participant Ghost as libghostty
participant PTY
participant Render as Renderer
User->>GTK: Mouse press/drag/release
GTK->>Session: Gesture events
alt Mouse-tracking active (app requests mouse)
Session->>Ghost: MouseEncoder.Encode(press/motion/release)
Ghost->>PTY: Return encoded bytes
Session->>PTY: QueueWrite bytes
else Local selection mode
Session->>Session: selection.start/update/clear
Session->>Render: invalidate -> compute ribbonRects
Render->>Render: Draw translucent selection overlay
alt on release
Session->>Ghost: CopyViewportSelection -> text
Session->>Clipboard: Write selection text
end
end
sequenceDiagram
participant User
participant GTK as GTK Scroll
participant Session
participant Ghost as libghostty
participant KeyEnc as KeyEncoder
participant PTY
participant Viewport
User->>GTK: Scroll wheel / two-finger scroll
GTK->>Session: handleScroll(deltaY)
Session->>Session: quantizeScroll(accum, deltaY)
alt Mouse-tracking active
Session->>Ghost: MouseEncoder.Encode(wheel up/down sequences)
Session->>PTY: QueueWrite wheel events
else Alt-screen active
loop per dispatched row
Session->>KeyEnc: Encode(ArrowUp/Down)
KeyEnc-->>Session: bytes
Session->>PTY: QueueWrite bytes
end
else Normal scrollback
Session->>Viewport: ScrollViewportDelta(rows)
end
alt Any input key pressed while scrolled back
Session->>Viewport: ScrollViewportToBottom()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
- gofmt three files (column-alignment + tab/space). - Replace deprecated coreglib.InternObject with coreglib.BaseObject (drop-in alias kept by gotk4 for back-compat, but staticcheck flags the old name). - Apply De Morgans law to selection.touches and convert a single-case switch in selection.ribbonRects to if/else (QF1001/QF1002). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
internal/ghostty/paste.go (1)
9-12: Include the Ghostty status code in returned paste errors.Both failure paths currently collapse distinct libghostty statuses into generic strings, which will make clipboard regressions harder to triage from logs.
Suggested diff
import ( - "errors" + "fmt" "unsafe" ) @@ rc := C.ghostty_paste_encode(dataPtr, dataLen, C.bool(bracketed), nil, 0, &needed) if rc != C.GHOSTTY_SUCCESS && rc != C.GHOSTTY_OUT_OF_SPACE { - return nil, errors.New("ghostty_paste_encode size query failed") + return nil, fmt.Errorf("ghostty_paste_encode size query failed: %d", int(rc)) } @@ ) if rc != C.GHOSTTY_SUCCESS { - return nil, errors.New("ghostty_paste_encode failed") + return nil, fmt.Errorf("ghostty_paste_encode failed: %d", int(rc)) }Also applies to: 35-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/paste.go` around lines 9 - 12, The paste error paths in internal/ghostty/paste.go currently return generic errors; update them to include the Ghostty status code by formatting the error with fmt (e.g., fmt.Errorf("paste failed: status=%d: %w", status, errOrMessage)) instead of errors.New; add "fmt" to the imports and replace the two failure returns referenced (the two error paths around the libghostty call at lines ~35-49) so they embed the numeric status (or status string) and original message/err for both branches where the libghostty status is used.cmd/roost/selection.go (2)
61-68: Simplify condition and remove unused variable.The staticcheck QF1001 warning can be addressed by applying De Morgan's law, which also eliminates the unused
sColvariable:func (s *selection) touches(minRow, maxRow int) bool { if !s.active { return false } - sCol, sRow, _, eRow := s.normalized() - _ = sCol - return !(eRow < minRow || sRow > maxRow) + _, sRow, _, eRow := s.normalized() + return eRow >= minRow && sRow <= maxRow }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/selection.go` around lines 61 - 68, The touches method contains an unused variable sCol and a negated OR condition; remove the unused assignment to sCol and replace the return !(eRow < minRow || sRow > maxRow) with the simplified positive condition eRow >= minRow && sRow <= maxRow in the selection.touches function (use the row values returned by selection.normalized()).
97-132: Consider tagged switch (optional).staticcheck QF1002 suggests using a tagged switch on
sRow == eRow. This is a minor style preference and the current form is readable.♻️ Optional refactor
- switch { - case sRow == eRow: + switch sRow == eRow { + case true: return []ribbonRect{{However, the current
switch { case sRow == eRow: ... default: }idiom is common in Go and arguably clearer here since the condition is a comparison rather than a value match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/selection.go` around lines 97 - 132, The switch block that checks sRow == eRow should be converted from an untagged switch to a tagged switch (or a simple if/else) for clarity and to satisfy staticcheck QF1002: replace the current "switch { case sRow == eRow: ... default: ... }" with "switch sRow == eRow { case true: ... case false: ... }" (or "if sRow == eRow { ... } else { ... }") while keeping the same logic that builds the ribbonRect slices using padX, padY, cw, ch, cols, sCol, eCol, sRow and eRow so ribbonRect construction and the out slice behavior are unchanged.internal/ghostty/formatter_test.go (1)
21-26: Assert the exact copied payload here, not just substrings.
strings.Containswill still pass if the formatter inserts extra blank lines, duplicates text, or regresses trim/unwrap behavior. For clipboard formatting tests, the exact output is the contract.Also applies to: 41-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ghostty/formatter_test.go` around lines 21 - 26, The test currently uses strings.Contains which allows extra/duplicated/trimmed content; change the assertions in internal/ghostty/formatter_test.go to compare the full returned payload exactly: build the expected string (including newlines and any trailing newline/whitespace the formatter should produce) and replace the substring checks after CopyViewportSelection with a strict equality check (e.g. if got != expected { t.Fatalf("CopyViewportSelection = %q, want %q", got, expected) }); apply the same exact-equality replacement for the second occurrence around lines 41-46 so both clipboard-formatting tests assert the exact contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/roost/app.go`:
- Around line 1103-1121: The current writeChunked ignores the returned byte
count from sess.pty.Write and does synchronous writes on the GTK thread, which
can drop unwritten bytes and stall the UI; fix by introducing a per-Session
buffered paste channel and a dedicated writer goroutine that performs all PTY
write operations and handles short writes by advancing the offset by the actual
bytes written (off + n) and re-queueing any remaining tail, and change
writeChunked to push paste byte slices onto that per-tab channel instead of
calling sess.pty.Write directly or recursing via coreglib.IdleAdd; ensure the
goroutine uses glib.IdleAdd only to marshal UI work if needed but performs PTY
writes itself so writes are serialized and backpressure is respected (refer to
Session, writeChunked, sess.pty.Write, pasteChunkBytes, and coreglib.IdleAdd).
- Around line 943-946: The global shortcut registrations (the add calls that
bind clipboardMod+"v", "<Control><Shift>v", clipboardMod+"c", and
"<Control><Shift>c" to pasteIntoActive and copyFromActive) are intercepting
copy/paste before focused editable widgets can handle them; update the handlers
to first inspect the current focus and only perform the global action when the
terminal (or your terminal widget) is focused, otherwise return and let the
focused GtkEditable/GtkEntry handle the event. Concretely, wrap the
pasteIntoActive and copyFromActive invocation in a guard that gets the window's
focused widget (e.g., window.GetFocus()/GetDefaultWidget), checks its type/role
(is-a GtkEditable/GtkEntry → return without handling; is-a Terminal widget or
a.isTerminalFocused() → proceed), and only then execute the existing logic; keep
the same add(...) registrations but move this focus-check into the handler
closures around pasteIntoActive and copyFromActive.
- Around line 993-995: The identity comparison in activeSession() is
over-wrapping objects; replace the InternObject(...) calls with direct .Native()
calls: compute selectedPtr from page.Native() (instead of
coreglib.InternObject(page).Native()) and compare against p.Native() (instead of
coreglib.InternObject(p).Native()); follow the same pattern used in
renameActiveTab() to perform pointer-equality.
In `@cmd/roost/selection.go`:
- Around line 16-17: The struct field alignment for anchorCol, anchorRow,
currentCol, and currentRow in selection.go is misformatted causing gofmt CI
failure; run `gofmt -w` (e.g., `gofmt -w cmd/roost/selection.go`) or reformat
the struct fields so they are properly aligned (ensure the declarations for
anchorCol, anchorRow, currentCol, currentRow follow gofmt style) and commit the
formatted file.
In `@cmd/roost/session.go`:
- Around line 533-543: The GTK input handlers are writing encoded key/mouse
bytes directly to the PTY via s.keys.Encode(...) and s.pty.Write(...), which can
block the main loop; instead push the encoded byte slices onto the session's
per-tab buffered writer channel (the same channel drained by the per-tab writer
goroutine) rather than writing to s.pty directly. Modify the places that call
s.keys.SyncFromTerminal(s.term) and use s.keys.Encode(ghostty.KeyEvent{...}) to
create the output, then send that output (or a copy) into the existing per-tab
buffered channel (the one consumed by the writer goroutine via glib.IdleAdd) so
the writer goroutine performs the actual PTY.Write calls. Ensure both the key
press loop and the mouse/alt-scroll handlers use this channel path and do not
call s.pty.Write directly.
In `@docs/getting-started/keybindings.md`:
- Line 105: Docs claim selection clears only when PTY output touches selected
rows, but the implementation in cmd/roost/session.go clears any active selection
on every PTY write; either update the text to match current behavior or make the
writer row-aware. Change the sentence on line 105 to state "Selection clears
automatically on any PTY output, on resize, and on a new click" (or similar), or
if you prefer to change code, modify the PTY write handler in
cmd/roost/session.go (the function that processes PTY writes and currently
clears selections) to check whether the write's affected row range intersects
the current selection before clearing it. Ensure you reference and update the
exact handler that performs the clear so docs and implementation remain
consistent.
In `@internal/ghostty/key.go`:
- Around line 181-182: Remove the runtime.SetFinalizer usage that calls
e.Close() for KeyEncoder (and the analogous finalizers on Terminal, RenderState,
MouseEncoder); finalizers run on the GC thread and can cause thread-affinity
violations when calling ghostty_key_event_free/ghostty_key_encoder_free. Instead
rely on explicit Close methods and the existing Session.Close + glib.IdleAdd
cleanup path; delete the SetFinalizer line(s) (the runtime.SetFinalizer(e,
func(e *KeyEncoder) { e.Close() }) invocation and its equivalents) so no
automatic finalizer will call Close from the GC goroutine.
In `@internal/ghostty/mouse.go`:
- Around line 46-53: Run gofmt on internal/ghostty/mouse.go to fix formatting
issues in the MouseButton constant block; specifically reformat the const block
containing MouseButtonNone, MouseButtonLeft, MouseButtonRight,
MouseButtonMiddle, MouseButtonWheelUp and MouseButtonWheelDown (e.g., use gofmt
or goimports) so the spacing and alignment match standard gofmt output and CI
will pass.
---
Nitpick comments:
In `@cmd/roost/selection.go`:
- Around line 61-68: The touches method contains an unused variable sCol and a
negated OR condition; remove the unused assignment to sCol and replace the
return !(eRow < minRow || sRow > maxRow) with the simplified positive condition
eRow >= minRow && sRow <= maxRow in the selection.touches function (use the row
values returned by selection.normalized()).
- Around line 97-132: The switch block that checks sRow == eRow should be
converted from an untagged switch to a tagged switch (or a simple if/else) for
clarity and to satisfy staticcheck QF1002: replace the current "switch { case
sRow == eRow: ... default: ... }" with "switch sRow == eRow { case true: ...
case false: ... }" (or "if sRow == eRow { ... } else { ... }") while keeping the
same logic that builds the ribbonRect slices using padX, padY, cw, ch, cols,
sCol, eCol, sRow and eRow so ribbonRect construction and the out slice behavior
are unchanged.
In `@internal/ghostty/formatter_test.go`:
- Around line 21-26: The test currently uses strings.Contains which allows
extra/duplicated/trimmed content; change the assertions in
internal/ghostty/formatter_test.go to compare the full returned payload exactly:
build the expected string (including newlines and any trailing
newline/whitespace the formatter should produce) and replace the substring
checks after CopyViewportSelection with a strict equality check (e.g. if got !=
expected { t.Fatalf("CopyViewportSelection = %q, want %q", got, expected) });
apply the same exact-equality replacement for the second occurrence around lines
41-46 so both clipboard-formatting tests assert the exact contract.
In `@internal/ghostty/paste.go`:
- Around line 9-12: The paste error paths in internal/ghostty/paste.go currently
return generic errors; update them to include the Ghostty status code by
formatting the error with fmt (e.g., fmt.Errorf("paste failed: status=%d: %w",
status, errOrMessage)) instead of errors.New; add "fmt" to the imports and
replace the two failure returns referenced (the two error paths around the
libghostty call at lines ~35-49) so they embed the numeric status (or status
string) and original message/err for both branches where the libghostty status
is used.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0e8df46-aee3-4ccf-a2f4-3a69ff70f258
📒 Files selected for processing (22)
README.mdcmd/roost/app.gocmd/roost/input.gocmd/roost/keymap.gocmd/roost/keymap_test.gocmd/roost/render.gocmd/roost/scroll_test.gocmd/roost/selection.gocmd/roost/selection_test.gocmd/roost/session.godocs/development/spec.mddocs/getting-started/first-run.mddocs/getting-started/keybindings.mddocs/reference/architecture.mdinternal/ghostty/formatter.gointernal/ghostty/formatter_test.gointernal/ghostty/key.gointernal/ghostty/key_test.gointernal/ghostty/mouse.gointernal/ghostty/paste.gointernal/ghostty/paste_test.gointernal/ghostty/terminal.go
- internal/ghostty/paste.go: include the libghostty status code in
EncodePaste error messages so failures are diagnosable from logs.
- internal/ghostty/{key,mouse}.go: drop runtime.SetFinalizer. Finalizers
run on the GC goroutine; libghostty key/mouse encoder cleanup must
run on the GTK main thread (same constraint as Terminal). Session.
Close already schedules cleanup on the main thread via glib.IdleAdd
-- thats the only correct path.
- cmd/roost/app.go writeChunked: honor short PTY writes by advancing
the offset by the actual bytes accepted, not the full chunk size.
Previously a partial write would silently drop the tail.
- docs/getting-started/keybindings.md: clarify selection-clear policy
matches implementation (clear on any PTY output, not only when the
write touches selected rows).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/roost/app.go (2)
943-946:⚠️ Potential issue | 🟠 MajorDon't steal copy/paste from focused edit fields.
These bindings are still registered on a capture-phase global controller and always consume the event, so inline rename
GtkEntrys and otherGtkEditables never get normal copy/paste. Please make the handler focus-aware and returnfalsewhen an editable owns focus so GTK can deliver the shortcut to that widget instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 943 - 946, The global keyboard bindings registered via add(...) are stealing copy/paste from focused editables; wrap the handlers (a.pasteIntoActive and a.copyFromActive) with a focus-aware check that returns false when a GtkEditable (e.g., GtkEntry, any widget implementing Gtk.Editable) currently has focus so GTK can handle the shortcut. Concretely, replace the direct registrations add(clipboardMod+"v", a.pasteIntoActive) and add(...c, a.copyFromActive) with small wrapper functions that inspect the currently focused widget (via the toplevel/application/focus API), detect if it is an editable, and if so return false; otherwise invoke the existing method (pasteIntoActive/copyFromActive) and return true.
1103-1121:⚠️ Potential issue | 🔴 CriticalMove paste writes onto the session's PTY writer path.
writeChunkedstill performssess.pty.Writefrom the GTK/main-loop path, so a slow PTY can stall the UI here. It also discards the returned byte count, so any partial progress before an error loses the unwritten tail. Queue encoded paste bytes onto the per-tab writer goroutine instead of writing fromIdleAddcallbacks. Based on learnings:PTY read/write operations must run in per-tab goroutines; push raw bytes onto a per-tab buffered channel and drain via glib.IdleAdd on the main thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 1103 - 1121, writeChunked currently writes directly to sess.pty from IdleAdd (GTK/main) and drops partial writes; instead enqueue the paste bytes onto the session's per-tab writer channel and let the per-tab writer goroutine perform the actual PTY writes and handle partial-write retries/errors. Change writeChunked to slice/copy the data chunk and send it to a buffered channel on Session (e.g. sess.pasteCh) rather than calling sess.pty.Write or coreglib.IdleAdd; implement/ensure a sessionPTYWriter (or startPTYWriter) goroutine reads from sess.pasteCh and performs the writes to sess.pty, handling the returned byte count (requeue or continue with the unwritten tail on error) and using glib.IdleAdd only if PTY writes must run on the main loop. Keep references: writeChunked, Session.pasteCh (new), sessionPTYWriter/startPTYWriter, sess.pty.
🧹 Nitpick comments (1)
cmd/roost/selection_test.go (1)
84-106: Add onetouches()case for inactive selections.You already cover overlap semantics well; adding an inactive-selection assertion would lock down the early-return path too.
➕ Suggested test addition
func TestSelection_TouchesRowRange(t *testing.T) { var s selection s.start(0, 5) s.update(10, 8) @@ for _, c := range cases { if got := s.touches(c.min, c.max); got != c.want { t.Errorf("touches(%d,%d): got %v, want %v", c.min, c.max, got, c.want) } } + + var inactive selection + if got := inactive.touches(0, 100); got { + t.Errorf("inactive selection: touches(0,100) = %v, want false", got) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/selection_test.go` around lines 84 - 106, Add a test case that verifies touches() returns false for an inactive selection: after creating the selection with selection.start(0,5) and selection.update(10,8) explicitly mark the selection inactive (set s.active = false) and assert s.touches(anyRange) == false (e.g. use {0,100,false}); this checks the early-return path in touches() using the selection type and its methods start, update and the active field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ghostty/mouse.go`:
- Around line 139-179: The Encode method on MouseEncoder currently returns nil,
nil when no output is generated; update MouseEncoder.Encode so that when written
== 0 it returns an actual empty byte slice (e.g. []byte{}), not nil, to honor
the function contract; locate the check in Encode (the written == 0 branch) and
change the return value accordingly while leaving error handling and other
return paths unchanged.
---
Duplicate comments:
In `@cmd/roost/app.go`:
- Around line 943-946: The global keyboard bindings registered via add(...) are
stealing copy/paste from focused editables; wrap the handlers (a.pasteIntoActive
and a.copyFromActive) with a focus-aware check that returns false when a
GtkEditable (e.g., GtkEntry, any widget implementing Gtk.Editable) currently has
focus so GTK can handle the shortcut. Concretely, replace the direct
registrations add(clipboardMod+"v", a.pasteIntoActive) and add(...c,
a.copyFromActive) with small wrapper functions that inspect the currently
focused widget (via the toplevel/application/focus API), detect if it is an
editable, and if so return false; otherwise invoke the existing method
(pasteIntoActive/copyFromActive) and return true.
- Around line 1103-1121: writeChunked currently writes directly to sess.pty from
IdleAdd (GTK/main) and drops partial writes; instead enqueue the paste bytes
onto the session's per-tab writer channel and let the per-tab writer goroutine
perform the actual PTY writes and handle partial-write retries/errors. Change
writeChunked to slice/copy the data chunk and send it to a buffered channel on
Session (e.g. sess.pasteCh) rather than calling sess.pty.Write or
coreglib.IdleAdd; implement/ensure a sessionPTYWriter (or startPTYWriter)
goroutine reads from sess.pasteCh and performs the writes to sess.pty, handling
the returned byte count (requeue or continue with the unwritten tail on error)
and using glib.IdleAdd only if PTY writes must run on the main loop. Keep
references: writeChunked, Session.pasteCh (new),
sessionPTYWriter/startPTYWriter, sess.pty.
---
Nitpick comments:
In `@cmd/roost/selection_test.go`:
- Around line 84-106: Add a test case that verifies touches() returns false for
an inactive selection: after creating the selection with selection.start(0,5)
and selection.update(10,8) explicitly mark the selection inactive (set s.active
= false) and assert s.touches(anyRange) == false (e.g. use {0,100,false}); this
checks the early-return path in touches() using the selection type and its
methods start, update and the active field.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56e109b9-8501-44c5-83a5-9858c198abae
📒 Files selected for processing (4)
cmd/roost/app.gocmd/roost/selection.gocmd/roost/selection_test.gointernal/ghostty/mouse.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/roost/selection.go
Comment on lines
+139
to
+179
| // Encode produces the terminal escape sequence for ev. Returns an | ||
| // empty slice when the event doesn't generate output (e.g. motion | ||
| // outside the active tracking mode). | ||
| func (e *MouseEncoder) Encode(ev MouseEvent) ([]byte, error) { | ||
| if e.c == nil || e.event == nil { | ||
| return nil, errors.New("ghostty: mouse encoder closed") | ||
| } | ||
|
|
||
| C.ghostty_mouse_event_set_action(e.event, C.GhosttyMouseAction(ev.Action)) | ||
| if ev.Button == MouseButtonNone { | ||
| C.ghostty_mouse_event_clear_button(e.event) | ||
| } else { | ||
| C.ghostty_mouse_event_set_button(e.event, C.GhosttyMouseButton(ev.Button)) | ||
| } | ||
| C.ghostty_mouse_event_set_mods(e.event, C.GhosttyMods(ev.Mods)) | ||
| pos := C.GhosttyMousePosition{x: C.float(ev.X), y: C.float(ev.Y)} | ||
| C.ghostty_mouse_event_set_position(e.event, pos) | ||
|
|
||
| buf := make([]byte, 64) | ||
| var written C.size_t | ||
| rc := C.ghostty_mouse_encoder_encode( | ||
| e.c, e.event, | ||
| (*C.char)(unsafe.Pointer(&buf[0])), | ||
| C.size_t(len(buf)), | ||
| &written, | ||
| ) | ||
| if rc == C.GHOSTTY_OUT_OF_SPACE { | ||
| buf = make([]byte, int(written)) | ||
| rc = C.ghostty_mouse_encoder_encode( | ||
| e.c, e.event, | ||
| (*C.char)(unsafe.Pointer(&buf[0])), | ||
| C.size_t(len(buf)), | ||
| &written, | ||
| ) | ||
| } | ||
| if rc != C.GHOSTTY_SUCCESS { | ||
| return nil, fmt.Errorf("ghostty_mouse_encoder_encode: %d", int(rc)) | ||
| } | ||
| if written == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Return an actual empty slice to match the function contract.
At Line 178, Encode returns nil, nil, but the comment says it returns an empty slice when no output is generated. This contract mismatch can cause subtle caller behavior differences.
Proposed fix
if written == 0 {
- return nil, nil
+ return []byte{}, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ghostty/mouse.go` around lines 139 - 179, The Encode method on
MouseEncoder currently returns nil, nil when no output is generated; update
MouseEncoder.Encode so that when written == 0 it returns an actual empty byte
slice (e.g. []byte{}), not nil, to honor the function contract; locate the check
in Encode (the written == 0 branch) and change the return value accordingly
while leaving error handling and other return paths unchanged.
- cmd/roost/app.go: gate Cmd+C / Cmd+V on whether a GtkEditable owns focus. When the focus is in the sidebar rename GtkEntry (or any future text-input widget), the shortcut steps aside so GTKs native copy/paste handles the keystroke. Type-asserts against *gtk.EditableTextWidget and walks one parent up to cover wrapper widgets like AdwEntryRow that nest the editable inside. - cmd/roost/selection_test.go: add a TestSelection_TouchesRowRange case for inactive selections, locking down the early-return path. Skipped from the re-review: per-tab writer goroutine for paste + encoded-key/mouse bytes. PTY writes are kernel-buffered and havent been observed stalling the UI; substantive refactor better suited to a follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/roost/app.go (2)
1100-1128:⚠️ Potential issue | 🔴 CriticalMove paste writes onto the session PTY writer goroutine.
writeChunked()still callssess.pty.Writesynchronously from the GTK thread. On a slow consumer that can stall the UI, and theIdleAddhop between chunks still allows later input to interleave into the paste stream. Queue the encoded bytes onto the tab’s write goroutine instead of writing here.As per coding guidelines, "PTY read/write operations must run in per-tab goroutines; push raw bytes onto a per-tab buffered channel and drain via
glib.IdleAddon the main thread".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 1100 - 1128, writeChunked currently performs sess.pty.Write on the GTK thread, which can block and allow input interleaving; instead push the slice (or a copy) onto the session's per-tab write channel and let the existing per-tab writer goroutine perform sess.pty.Write; update writeChunked to enqueue data to Session's buffered write channel (use the Session write goroutine/handler name and channel field, e.g., Session.writeCh or similar) and use coreglib.IdleAdd only to schedule feeding the UI if needed, ensuring the per-tab goroutine drains the channel and performs all PTY writes serially.
910-919:⚠️ Potential issue | 🟠 MajorLet focused editables keep their copy/paste shortcuts.
These bindings are still routed through a capture-phase controller whose callback always returns handled, so a focused
GtkEntry/editable never getsCmd/Alt/Ctrl-Shift + C/V. That still breaks the inline rename flows in this file. Make the clipboard shortcuts conditional on terminal focus and returnfalsewhen focus is on an editable widget.Also applies to: 943-946
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 910 - 919, The current shortcut action created via gtk.NewShortcutTriggerParseString and gtk.NewCallbackAction always returns true, preventing focused editables from receiving clipboard shortcuts; update the callback created in the add closure (and the duplicate at the other occurrence) to first inspect the currently focused widget and terminal focus state, and if the focused widget implements/editable (e.g. GtkEntry/GtkEditable) or focus is not on the terminal, return false so the widget receives the event; only call fn() and return true when focus is on the terminal (or the focused widget is not editable), keeping the rest of the shortcut wiring intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/roost/app.go`:
- Around line 1100-1128: writeChunked currently performs sess.pty.Write on the
GTK thread, which can block and allow input interleaving; instead push the slice
(or a copy) onto the session's per-tab write channel and let the existing
per-tab writer goroutine perform sess.pty.Write; update writeChunked to enqueue
data to Session's buffered write channel (use the Session write
goroutine/handler name and channel field, e.g., Session.writeCh or similar) and
use coreglib.IdleAdd only to schedule feeding the UI if needed, ensuring the
per-tab goroutine drains the channel and performs all PTY writes serially.
- Around line 910-919: The current shortcut action created via
gtk.NewShortcutTriggerParseString and gtk.NewCallbackAction always returns true,
preventing focused editables from receiving clipboard shortcuts; update the
callback created in the add closure (and the duplicate at the other occurrence)
to first inspect the currently focused widget and terminal focus state, and if
the focused widget implements/editable (e.g. GtkEntry/GtkEditable) or focus is
not on the terminal, return false so the widget receives the event; only call
fn() and return true when focus is on the terminal (or the focused widget is not
editable), keeping the rest of the shortcut wiring intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c35072c-a7d8-4854-9655-6ead8fd76d9d
📒 Files selected for processing (5)
cmd/roost/app.godocs/getting-started/keybindings.mdinternal/ghostty/key.gointernal/ghostty/mouse.gointernal/ghostty/paste.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/ghostty/paste.go
- internal/ghostty/mouse.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/roost/app.go (2)
943-946:⚠️ Potential issue | 🟠 MajorReturn
falsefor clipboard shortcuts when an editable owns focus.The new guards in
pasteIntoActive()/copyFromActive()only skip the terminal action. These bindings still run in capture phase, andadd()always reports the shortcut as handled, so focusedGtkEditablewidgets still never receive their native copy/paste keystrokes.Minimal shape for the fix
+ addMaybe := func(spec string, fn func() bool) { + t := gtk.NewShortcutTriggerParseString(spec) + if t == nil { + slog.Warn("shortcut: trigger parse failed", "spec", spec) + return + } + action := gtk.NewCallbackAction(func(_ gtk.Widgetter, _ *glib.Variant) bool { + return fn() + }) + ctrl.AddShortcut(gtk.NewShortcut(t, action)) + } + - add(clipboardMod+"v", a.pasteIntoActive) - add("<Control><Shift>v", a.pasteIntoActive) - add(clipboardMod+"c", a.copyFromActive) - add("<Control><Shift>c", a.copyFromActive) + addMaybe(clipboardMod+"v", func() bool { + if a.editableHasFocus() { + return false + } + a.pasteIntoActive() + return true + }) + addMaybe("<Control><Shift>v", func() bool { + if a.editableHasFocus() { + return false + } + a.pasteIntoActive() + return true + }) + addMaybe(clipboardMod+"c", func() bool { + if a.editableHasFocus() { + return false + } + a.copyFromActive() + return true + }) + addMaybe("<Control><Shift>c", func() bool { + if a.editableHasFocus() { + return false + } + a.copyFromActive() + return true + })In GTK4 / gotk4, when a ShortcutController runs in PhaseCapture, does a CallbackAction that returns false allow the focused GtkEditable widget to continue handling the shortcut?Also applies to: 1054-1057, 1098-1100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 943 - 946, The current shortcut bindings added with add(clipboardMod+"v", ...), add("<Control><Shift>v", ...), add(clipboardMod+"c", ...), and add("<Control><Shift>c", ...) always report the shortcut as handled, preventing focused GtkEditable widgets from receiving native copy/paste; modify the CallbackAction handlers pasteIntoActive and copyFromActive to return false when an editable owns focus (so the ShortcutController capture phase will not consume the event) and ensure the add(...) registrations expect and propagate that boolean return (i.e., only return true when the action actually handled the shortcut for the terminal).
1139-1158:⚠️ Potential issue | 🟠 MajorKeep PTY writes off the GTK main thread.
writeChunked()now handles short writes correctly, butsess.pty.Write(...)still runs on the GTK thread andIdleAddonly schedules the next chunk. A slow PTY can still stall the UI here. Please enqueue paste bytes onto the session’s per-tab writer goroutine instead of writing directly from this callback path. As per coding guidelines,PTY read/write operations must run in per-tab goroutines; push raw bytes onto a per-tab buffered channel and drain via glib.IdleAdd on the main thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 1139 - 1158, writeChunked currently performs sess.pty.Write(...) on the GTK main thread which can stall the UI; instead create/use a per-tab buffered channel on Session (e.g., session.writeCh) and enqueue the raw bytes (or the remaining slice) to that channel from writeChunked/IdleAdd; implement/ensure a dedicated per-tab writer goroutine (spawned when Session is created) that reads from session.writeCh and does the actual sess.pty.Write calls and handles short writes/retries, while IdleAdd is only used by that goroutine to marshal UI interactions if needed. Ensure writeChunked only sends to session.writeCh and returns immediately so no PTY I/O happens on the GTK thread.
🧹 Nitpick comments (1)
cmd/roost/selection_test.go (1)
101-105: Optional: use subtests for each table case.Using
t.Runhere improves failure isolation and rerun ergonomics (-runby case name).Suggested refactor
- for _, c := range cases { - if got := s.touches(c.min, c.max); got != c.want { - t.Errorf("touches(%d,%d): got %v, want %v", c.min, c.max, got, c.want) - } - } + for _, c := range cases { + c := c + t.Run( + fmt.Sprintf("min=%d_max=%d", c.min, c.max), + func(t *testing.T) { + if got := s.touches(c.min, c.max); got != c.want { + t.Errorf("touches(%d,%d): got %v, want %v", c.min, c.max, got, c.want) + } + }, + ) + }// add import import "fmt"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/selection_test.go` around lines 101 - 105, Convert the table-driven loop into subtests by wrapping each iteration in t.Run so failures are isolated and can be rerun by name: for each element of the cases slice, call t.Run(fmt.Sprintf("case-%d", i) or use a case.name if present) and move the existing assertion into the subtest body (keeping the call to s.touches(c.min, c.max) and the t.Errorf). Also add the import "fmt" at the top of the test file if you use fmt.Sprintf for the subtest name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/roost/app.go`:
- Around line 943-946: The current shortcut bindings added with
add(clipboardMod+"v", ...), add("<Control><Shift>v", ...), add(clipboardMod+"c",
...), and add("<Control><Shift>c", ...) always report the shortcut as handled,
preventing focused GtkEditable widgets from receiving native copy/paste; modify
the CallbackAction handlers pasteIntoActive and copyFromActive to return false
when an editable owns focus (so the ShortcutController capture phase will not
consume the event) and ensure the add(...) registrations expect and propagate
that boolean return (i.e., only return true when the action actually handled the
shortcut for the terminal).
- Around line 1139-1158: writeChunked currently performs sess.pty.Write(...) on
the GTK main thread which can stall the UI; instead create/use a per-tab
buffered channel on Session (e.g., session.writeCh) and enqueue the raw bytes
(or the remaining slice) to that channel from writeChunked/IdleAdd;
implement/ensure a dedicated per-tab writer goroutine (spawned when Session is
created) that reads from session.writeCh and does the actual sess.pty.Write
calls and handles short writes/retries, while IdleAdd is only used by that
goroutine to marshal UI interactions if needed. Ensure writeChunked only sends
to session.writeCh and returns immediately so no PTY I/O happens on the GTK
thread.
---
Nitpick comments:
In `@cmd/roost/selection_test.go`:
- Around line 101-105: Convert the table-driven loop into subtests by wrapping
each iteration in t.Run so failures are isolated and can be rerun by name: for
each element of the cases slice, call t.Run(fmt.Sprintf("case-%d", i) or use a
case.name if present) and move the existing assertion into the subtest body
(keeping the call to s.touches(c.min, c.max) and the t.Errorf). Also add the
import "fmt" at the top of the test file if you use fmt.Sprintf for the subtest
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b86bdc50-0a30-4171-8701-5f8289934240
📒 Files selected for processing (2)
cmd/roost/app.gocmd/roost/selection_test.go
… shortcuts
Address two CodeRabbit re-review concerns that turned out to be real:
1. Clipboard shortcut callbacks now return false when a GtkEditable
owns focus, so a focused sidebar rename GtkEntry (or any future
text-input widget) gets its native copy/paste handling. The earlier
editableHasFocus guard inside pasteIntoActive/copyFromActive was a
no-op because the action callback still reported the event as
handled. New addCond helper lets the action signal "not consumed."
2. New Session.QueueWrite serializes background PTY writes via a
per-session writeMu and a goroutine, matching the CLAUDE.md
threading contract (Goroutine per tab for PTY read/write). Every
PTY write site now goes through QueueWrite instead of synchronous
pty.Write on the GTK main thread:
- input.go handleKey
- session.go handleScroll alt-screen arrow translation
- session.go sendMouseEvent
- app.go pasteIntoActive (writeChunked removed; the goroutine
handles the full encoded paste in one go, kernel buffers
handle backpressure)
Order is preserved: paste-then-keystroke arrives at the shell in
that order because both goroutines must acquire writeMu before
writing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/roost/app.go (1)
1023-1025: Consider using.Native()directly for consistency.Lines 1023 and 1025 wrap with
coreglib.BaseObject()before calling.Native(). TherenameActiveTab()function (line 825) shows a simpler pattern: callingpage.Native()directly. Both solve the gotk4 pointer-equality issue, but the direct approach is more concise.♻️ Suggested simplification
- selectedPtr := coreglib.BaseObject(page).Native() + selectedPtr := page.Native() for tabID, p := range a.tabPages { - if coreglib.BaseObject(p).Native() == selectedPtr { + if p.Native() == selectedPtr { return a.sessions[tabID]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/app.go` around lines 1023 - 1025, The code uses coreglib.BaseObject(...).Native() to get native pointers for comparison in selectedPtr and inside the loop over a.tabPages; change these to call .Native() directly on the page and p values (e.g., use page.Native() and p.Native()) for consistency with renameActiveTab() and to simplify the pointer-equality checks involving selectedPtr and a.tabPages entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/roost/app.go`:
- Around line 1023-1025: The code uses coreglib.BaseObject(...).Native() to get
native pointers for comparison in selectedPtr and inside the loop over
a.tabPages; change these to call .Native() directly on the page and p values
(e.g., use page.Native() and p.Native()) for consistency with renameActiveTab()
and to simplify the pointer-equality checks involving selectedPtr and a.tabPages
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52d976c8-3ff9-4d4c-8391-c9aefb87997d
📒 Files selected for processing (3)
cmd/roost/app.gocmd/roost/input.gocmd/roost/session.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/roost/input.go
Drop the coreglib.BaseObject(page).Native() wrapper in favor of page.Native() — *adw.TabPage embeds the base Object directly so the result is the same C pointer, and the direct form matches the pattern already used in renameActiveTab. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
charliek
added a commit
that referenced
this pull request
May 23, 2026
…PC envelope strict + partial-write loop Batch 2 of the Phase-8-prep polish. Four CR findings: **1. App.swift `.tabOpened` auto-attach** (CR App.swift:1003 + M0–M9 sub-agent #4): the arm was logging + dropping cross-client `tab.opened` events, so `roostctl tab open` succeeded in the shared workspace while the Mac UI never showed the new tab. This also meant headless-opened tabs got no OSC scanning (the user noted custom OSC titles weren't appearing for these). Materialize a TabSession from the event payload and attach to the existing workspace tab + supervisor PTY: * New `RoostClient.attachShellSession(socketPath:, tabID:, cols:, rows:, keystrokes:, onOutput:)` — mirrors `runShellSession` for everything *after* the open, with a subscriber that filters to the supplied tabID + a keystroke pump. * New `TabSession.attach(socketPath:, tabID:, closeOwnedByExternal:)` — counterpart to `start()` that skips the `openTab` call. Sets `skipCloseRPC = true` by default so quitting the UI doesn't reap an externally- spawned shell out from under `roostctl`. * App.swift's `.tabOpened` arm constructs a TabSession with the project ID + theme/font + initial size; dedupes against `tabs[]` (UI-driven `openNewTab` always inserts the session before this event fires, so the dedupe wins for self-opened tabs); rebuilds the strip + sidebar so the new tab appears immediately. * `.active` split into its own arm so we can drop it explicitly (UI's active state is authoritative within the UI). **2. IPCMessages.swift envelope deny-unknown-fields** (CR IPCMessages.swift:190): `IPCRequest.init(from:)` was using a keyed container without checking `allKeys` against the allowed set, so a client could pass on macOS and fail on the Rust side (which uses `#[serde(deny_unknown_fields)]` on `RawRequest`). Added an `allKeys`-vs-`{"id","op","params"}` check that throws `DecodingError.dataCorrupted` on overflow. The op-specific params already get the same treatment via `IPCHandlerImpl.decodeParams(expected:)`. **3. SIGPIPE handler** (CR IPCServer.swift:263): writing to a Unix-domain socket whose peer has closed its end raises SIGPIPE and terminates the process on macOS by default. The IPC server's `writeAll` already checks for EPIPE on a negative Darwin.write return, so installing `signal(SIGPIPE, SIG_IGN)` once at startup lets all error handling live in user-space. Hooked into `RoostBackend.start` via a `nonisolated(unsafe)`-guarded `ignoreSigpipe()` so it fires exactly once. **4. PtySupervisor.write partial-write loop** (CR PtySupervisor.swift:284 / 330): the prior single-write call could drop tail bytes (a kernel pty buffer can short-write the user) and didn't retry on EINTR. Loop until the full buffer lands; retry on EINTR; throw on any other negative return; treat n==0 as a writer disconnect. 132/132 Swift tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 23, 2026
* fix(linux): Linux IPC robustness sweep (closes #80) Address the deferred CR items from the inline-core refactor (PR #78). Reconciled against the merged code: #1 (parse-error reply), #10 (pending HashSet), #13 (smoke-test ordering) were already done; the rest are fixed here. Project is early-stage, so the bar is long-term correctness. PTY supervisor (crates/roost-linux/src/daemon/pty.rs, tab_session.rs): - #11: wait task now removes the session on child exit (sessions behind Arc<Mutex>), so writes to a dead PTY return NotFound instead of silently succeeding. - #12: close() escalates SIGHUP -> SIGKILL after a grace period. The cloned portable-pty ChildKiller only sends SIGHUP, so a shell that traps/ignores it used to outlive close(); a reaped flag + pid watchdog force-kill it, mirroring the Mac teardown. - #8: send_input/send_resize feed one per-tab serial channel drained by a single task, so keystrokes can't reorder (the old per-call tokio::spawn could under the multi-thread runtime). Workspace state (crates/roost-linux/src/daemon/state.rs): - #4: new project/tab position uses max(position)+1, not len()/count(), so a delete-then-create can't collide. - #5: every mutator now publishes its event while still holding `inner`, so broadcast order matches commit order; persist runs after the drop. The events-resync Resync path is preserved. Server framing (crates/roost-ipc/src/server.rs): - #2: on an envelope decode failure, peel `id` from the raw JSON (string- or number-encoded) so the parse-error reply lands at the client's id, not 0. Lifecycle (crates/roost-linux/src/main.rs, ipc.rs, app.rs, messages.rs): - #7: IPC bind is now a synchronous startup requirement; bind failure aborts startup instead of leaving the UI socket-less. - #6: a second launch dials the new `app.activate` op; the handler forwards to the GTK thread which raises the running window. - #9: events.subscribe returns not-implemented (Linux + Mac) instead of a false {} ACK, so clients don't wait forever. Real streaming is deferred to its first consumer (roostctl watch). Tests: write-after-exit -> NotFound, SIGHUP-ignoring child force-killed, send_input ordering, position max+1 after delete, id-preserving parse error, bind-failure surfaced, events.subscribe not-implemented. Rust workspace + Mac build/clippy/tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(mac): IPC handler dispatch harness (#80 follow-up) The Rust handler has tests/ipc_dispatch.rs; the Mac IPCHandlerImpl had no equivalent, leaving its hand-written cross-cutting logic untested — strict unknown-field rejection (decodeParams), ipcDim u16 validation, mapWorkspace/mapPty error-code mapping, the not-implemented/unknown-op paths, and result encoding. The two handlers must stay behaviorally convergent over the shared wire contract, so this guards that. Calls IPCHandlerImpl.handle(op:params:) directly — no socket. Exercises only non-PTY-spawning ops to avoid the forkpty/swift-testing SIGTRAP that disables the PTY paths elsewhere (tab.open stays on the manual pass; the error-mapping ops reach the supervisor only on the lookup-fails path). Seeds 8 tests: events.subscribe→not-implemented (the #9 path), unknown-op, unknown-field strictness, project.rename→not-found, tab.resize out-of-range→invalid-param, tab.resize missing→not-found, identify profile echo, project.create→tab.list round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): address CodeRabbit review on #80 sweep Resolves 4 of CodeRabbit's 6 findings on PR #86 (the persistence-ordering finding is pre-existing/orthogonal and tracked separately). pty.rs: - Major: spawn() acquired the master reader/writer AFTER spawn_command, so a try_clone_reader/take_writer failure returned with a live child and no wait task — an orphaned PTY. Acquire reader+writer BEFORE spawning, making spawn_command the last fallible step (no orphan). - Major: the exit wait task removed the session by tab_id alone, which could evict a newer session that reused the same tab_id (close() frees the slot synchronously; pty_smoke reuses id 42). Gate the remove on Arc::ptr_eq of the per-spawn `reaped` identity so only the owning waiter deletes. (My prior "monotonic ids" comment over-assumed caller discipline; the supervisor can't rely on it.) - Major: input and resize were unified at TabSession but re-split into the supervisor's two channels (select!), so mixed input/resize wasn't FIFO end-to-end. Merge them into one WriterCmd channel drained by a single ordered loop — genuine submission-order delivery. ipc.rs + messages.rs: - Minor: app.activate ACK'd any payload. Add an empty strict AppActivateParams and decode it so the op validates its envelope (rejects unknown fields) like every other op. IPCHandlerTests.swift: - Minor: replace `as? T != nil` (SwiftLint prefer_type_checking) with a meaningful empty-array assertion on a fresh project's tabs. Rust clippy --all-targets clean; roost-ipc + roost-linux tests green (A3 ordering + dup-spawn still pass under the new channel/guard). Mac swift build + 140 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): order state.json persistence by commit seq (CR #3) Resolves the last CodeRabbit finding on PR #86. persist_async did synchronous file I/O on the caller thread after dropping `inner`, so two concurrent mutators could write state.json out of commit order — a slow earlier commit could clobber a newer one, regressing restart durability. Keep persistence synchronous (state_persist.rs and restart-reload depend on it; an async writer thread would break that), but make it ordered: - Inner.persist_seq: a monotonic commit counter bumped under the lock when a snapshot is taken, so the seq reflects commit order. - snapshot_for_persist now returns (SnapshotFile, seq). - persist(seq, snapshot) serializes on a new persist_guard mutex and drops any snapshot whose seq is <= the highest already written, so the newest committed snapshot always wins regardless of which thread races first. Newest-wins verified by persist_drops_stale_out_of_order_writes. Rust clippy --all-targets clean; roost-ipc + roost-linux suites green (incl. state_persist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt ipc.rs import block The AppActivateParams import addition left the use block un-reflowed; CI's rust-lint (cargo fmt --all -- --check) caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 31, 2026
…198) Document what PR #194 added/changed so the new knobs are discoverable: - test-automation.md §5.6 (new): make targets (e2e-{gtk,mac}-ci), the env/flag table (ROOST_TEST_MODE, --roost-fresh/ROOST_TEST_FRESH, ROOST_STATE_DIR, ROOST_DEFAULTS_SUITE, ROOST_TEST_TIMEOUT_SCALE, ROOST_CONFIG), the retirement of ROOST_TEST_RESET_STATE, and the skip policy (precondition / skip_on_ci / cwd_reaches + the SKIPS: N summary). - §8/§9 + Open-decision #4 updated: temp-workspace isolation is now implemented (throwaway ROOST_STATE_DIR), not an open question. - tools/roosttest/README.md: the *-ci targets, a "Hermetic / fresh mode" section, a "Skip policy" section, and corrected layout-table rows (util.py, the fresh fixture, the test_terminal split, the seed precondition). Docs only. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bindings added
Bare Ctrl+C is left as SIGINT.
Out of scope (explicit defers)
Test plan
Unit tests added — all green:
Manual smoke tests done on macOS:
Manual tests still recommended on Linux:
Notes for reviewers
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests