fix: detect in-place terminal activity reliably - #185
Conversation
Use Ghostty's IO-path output heartbeat to track raw-mode and occluded work without relying on renderer activity. Preserve foreground/progress authority, require committed input evidence for ambiguous same-PID agent commands, and quiet-settle activity with a non-coalesced deadline. Expose the additive execution state through the CLI, show active-tab status without special masking, and reject GhosttyKit artifacts missing the required ABI.
github-actions
Bot
added
area:ui
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe PR adds Ghostty output-activity heartbeats and command-submission evidence, expands terminal execution-state tracking, removes occlusion-specific settling, updates sidebar indicators, and exposes pane state through control responses and CLI output. Terminal activity and input handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GhosttyCallbacks
participant GhosttyTerminalNSView
participant TerminalPane
participant Pane
participant AppState
GhosttyCallbacks->>GhosttyTerminalNSView: forward output-activity heartbeat
GhosttyTerminalNSView->>TerminalPane: emit output or command-submission event
TerminalPane->>Pane: update execution activity
Pane->>AppState: post quiet-settle deadline
AppState->>Pane: poll and settle eligible activity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Window-state benchmark
Reported value is the median of 3×10s windows per state (splitting the window and taking the median keeps one co-scheduled spike from skewing a state); CPU % is the process CPU-time delta over a window. Runs land on different shared runners, so treat small deltas as noise — 🔺/🔻 marks changes ≥25% that also clear the metric's absolute noise floor (CPU % ≥0.5, Memory (RSS MB) ≥25, CPU ms/s ≥5, Wakeups/s ≥50); CPU deltas off a noise-dominated baseline aren't flagged (CPU % baseline ≥1.5, CPU ms/s baseline ≥15). The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MactermTests/Control/ControlHandlerTests.swift (1)
204-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrive execution state through
AppStatein this test.Lines 211 and 215 mutate
Pane.executionStatedirectly, bypassing the production mutation/notification path. Use the AppState transition API or an injected test seam so this test validates the real state flow.As per coding guidelines, all state mutations must go through AppState.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MactermTests/Control/ControlHandlerTests.swift` around lines 204 - 217, Update pane_list_reports_running_and_done_states to change execution state through AppState’s transition API or an injected test seam instead of assigning pane.executionState directly. Preserve the existing assertions for both running and done states while exercising the production mutation and notification path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@MactermTests/Control/ControlHandlerTests.swift`:
- Around line 204-217: Update pane_list_reports_running_and_done_states to
change execution state through AppState’s transition API or an injected test
seam instead of assigning pane.executionState directly. Preserve the existing
assertions for both running and done states while exercising the production
mutation and notification path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 652b2616-5ec7-4a90-a9f3-f652dadac11d
📒 Files selected for processing (20)
AGENTS.mdCLI/Output.swiftMacterm/App/AppState.swiftMacterm/App/Notifications.swiftMacterm/Control/ControlHandler.swiftMacterm/Control/ControlProtocol.swiftMacterm/Ghostty/GhosttyCallbacks.swiftMacterm/Model/SplitNode.swiftMacterm/Views/Sidebar.swiftMacterm/Views/Terminal/GhosttyTerminalNSView.swiftMacterm/Views/Terminal/TerminalCommandSubmission.swiftMacterm/Views/TerminalPane.swiftMactermTests/App/AppStateTests.swiftMactermTests/Control/ControlHandlerTests.swiftMactermTests/Control/ControlProtocolTests.swiftMactermTests/Model/PaneTests.swiftMactermTests/Model/TerminalExecutionTrackerTests.swiftMactermTests/Views/Terminal/TerminalCommandSubmissionTests.swiftscripts/setup.shwebsite/docs/pages/80-cli.md
github-actions
Bot
added
the
benchmark:improvement
The OUTPUT_ACTIVITY heartbeat is now required (setup.sh rejects a GhosttyKit without the ABI) and fires from the pty IO path regardless of occlusion, so a quiet activity run's silence is meaningful on or off screen. That makes the whole pre-heartbeat occlusion workaround dead weight: drop the parked-renderer fallback (settleIfVisible, paneIsOccluded, previouslyOccludedPanes, refreshActivityWindow, hasOcclusionIndependentHeartbeat) so the poll settles occluded and visible panes identically. Also route activity solely through the heartbeat: the render-path scrollbar no longer feeds activity detection (it only drove the overlay scrollbar and was a redundant, visible-only echo of the heartbeat), which incidentally removes the double foreground re-resolve on visible panes. The TerminalExecutionTracker state machine is untouched.
github-actions
Bot
removed
the
benchmark:improvement
| need_xcframework=false | ||
| else | ||
| echo "Existing GhosttyKit lacks GHOSTTY_ACTION_OUTPUT_ACTIVITY; refreshing it" | ||
| rm -rf "$XCFRAMEWORK_DIR" |
There was a problem hiding this comment.
This rm -rf runs before the replacement is known-good. If the latest release turns out to lack the ABI, the exit 1 down at line 55 fires after the working (if older) framework is already gone — so a tree that could previously build is left with no framework at all, and re-running setup hits the same failure. Worth downloading and validating into a temp location, then swapping, so a bad release is a no-op rather than a regression.
Separately, GHOSTTY_HEADER hardcodes the macos-arm64_x86_64 slice name. If the fork ever ships a differently-named slice, has_output_activity_action silently returns false and you get a re-download every single setup run with no indication why. A glob over the slice dirs would be more robust.
There was a problem hiding this comment.
Both fixed. Download+validate now happens in a mktemp -d, and the old framework is only removed once the replacement passes, so a bad release is a no-op. Probe takes the root as an arg and globs */Headers/ghostty.h instead of naming the slice.
| if ! has_output_activity_action; then | ||
| echo "Error: GhosttyKit from $LATEST_TAG lacks GHOSTTY_ACTION_OUTPUT_ACTIVITY" >&2 | ||
| echo "The thdxg/ghostty output-activity downstream patch must be released first." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Worth calling out that this makes the app unbuildable against any GhosttyKit release predating the fork patch — including when bisecting across this commit. That's a defensible call given the feature genuinely requires the ABI, but the error message could say so directly ("checkouts before need an older GhosttyKit; see AGENTS.md") to save the next person the archaeology.
There was a problem hiding this comment.
Good call. The error now says the ABI has been required since this change, that a pre-change checkout needs a GhosttyKit from a contemporary release, and that the existing one was left alone. Expanded the AGENTS.md paragraph it points at.
| shell: String? = nil, | ||
| env: [String: String]? = nil | ||
| env: [String: String]? = nil, | ||
| activityQuietPollDelay: Duration = .seconds(3) |
There was a problem hiding this comment.
activityQuietPollDelay defaults to 3s and settleTerminalActivityIfQuiet defaults quietInterval to 3s (line 591). So the wake fires at almost exactly the threshold it needs to have crossed: the settle requires now - lastActivityAt >= 3, and the sleep targets 3.0s after the last heartbeat.
Today this works because Task.sleep jitter is positive. But it's load-bearing on that assumption, and the failure mode is bad: if the wake ever lands marginally early the settle no-ops, and with every window occluded there's no timer left to retry — the run stays .running indefinitely, which is precisely what this machinery exists to prevent.
Suggest making the margin explicit, e.g. delay = quietInterval + 0.25s, ideally deriving both from one constant so they can't drift apart.
There was a problem hiding this comment.
Yep, that was relying on jitter sign. Added TerminalActivityTiming with quietInterval = 3 / quietPollMargin = 0.25 / quietPollDelay = interval + margin; the wake and the settle default both derive from it, and a test pins the margin so it cannot collapse.
|
|
||
| func refreshTerminalActivityWindow(now: Date = Date()) { | ||
| executionTracker.refreshActivityWindow(now: now) | ||
| private func scheduleActivityQuietPollIfNeeded() { |
There was a problem hiding this comment.
Minor: this cancels and recreates a Task on every markOutputActivity, i.e. ~2 Hz per pane with live output. Bounded by the throttle so it's not a real cost, but DispatchWorkItem would be lighter and matches the idiom already used a few files over for commandSubmissionEvidenceReset.
There was a problem hiding this comment.
Switched to DispatchWorkItem + asyncAfter. activityQuietPollDelay is a TimeInterval now.
| // Same liveness signal a keystroke sends (execution tracking + poll | ||
| // resume), so an injected command updates the tab title promptly. | ||
| onInteraction?() | ||
| recordCommandInput(text) |
There was a problem hiding this comment.
recordCommandInput(text) records the same string that's checked for a newline two lines down, so hasContent here is derived from the caller's own payload rather than from prior typing. Fine for pane run "sleep 5".
The issue is that Evidence.recordText only ORs in — it never clears — combined with preserveProgrammaticCommandInput's 2s window. So a pane run followed within 2s by a genuinely blank Return will report hasContent == true for that blank Return, which is the exact false positive the blank-submission suppression exists to prevent. Narrow, but reachable from a script that injects a command and then sends a bare newline.
There was a problem hiding this comment.
Reachable, yeah. The view cannot tell the two cases apart (it does not know the payload already executed), so I scoped the carry rather than tried to detect it: it is gated on Pane.allowsInPlaceOutputStart now, which is the same agent-TUI foreground the in-place heuristic needs and the only place a bracketed paste can eat the newline. pane run into a plain shell no longer opens the window.
| /// moves away, so a settled program that reported progress doesn't flip back | ||
| /// to running on its own render output. `pendingProgressQuiesce` covers the | ||
| /// race where progress started and cleared before any foreground poll. | ||
| /// moves away. `pendingProgressQuiesce` covers progress that starts and |
There was a problem hiding this comment.
recordUserInteraction now clears pendingOutputStart, which makes the arming call sites order-dependent: keyDown, sendText, and sendKey all fire onInteraction?() before onCommandSubmitted?(), so it's clear-then-arm and works. Reorder either call and the Pi path silently stops arming, with no test to catch it.
I tried to construct a failing case and couldn't, so this isn't a bug report — just asking for a line of comment here stating that callers must arm after interaction, since the constraint is currently invisible at both ends.
There was a problem hiding this comment.
Documented at both ends: an ordering note on recordUserInteraction, one on onCommandSubmitted, and one at the TerminalPane wiring.
| hasCommand: Bool | ||
| ) -> Bool { | ||
| if keyCode == 53 || keyCode == 51 || keyCode == 117 { return true } // Escape / delete | ||
| if hasControl, [4, 8, 13, 32, 40].contains(keyCode) { return true } // H/C/W/U/K |
There was a problem hiding this comment.
The keycodes check out (4=h, 8=c, 13=w, 32=u, 40=k), but bare literals with a // H/C/W/U/K gloss are easy to get wrong on edit. HotkeyRegistry.parseShortcut already carries a keycode vocabulary the CLI leans on — reusing it, or at minimum naming these, would be more maintainable.
Also line 54: // Escape / delete is two labels for three codes (53 escape, 51 delete, 117 forward-delete).
There was a problem hiding this comment.
Named them, and split the escape/delete gloss into three cases. Cannot read HotkeyRegistry at runtime here since it is @MainActor and this helper is isolation-free, so I added keyCode(forToken:) and a test that drives the codes from that vocabulary through the public API.
| defer { NotificationCenter.default.removeObserver(token) } | ||
|
|
||
| p.markOutputActivity(totalRows: 20) | ||
| try? await Task.sleep(for: .milliseconds(100)) |
There was a problem hiding this comment.
Fixed 100ms sleep against a 20ms delay — this is the shape that's flaked on loaded CI runners before (cf. #181). Better to poll until the confirmation fires with a sleep in the loop, or widen the margin substantially, so a co-scheduled spike doesn't fail the run.
There was a problem hiding this comment.
Fixed. Polls in 25ms ticks with a 5s ceiling, so a healthy run exits on the first tick and a spike cannot fail it.
| // non-null on the Zig side; a request completing during surface | ||
| // teardown (nil `surface`) is UB, not a graceful no-op. Guard it. | ||
| guard let surface = surface(from: ud) else { return false } | ||
| guard let ud else { return false } |
There was a problem hiding this comment.
Functionally identical to the surface(from:) helper this replaces, so the teardown guard the comment describes is preserved — but it open-codes Unmanaged.fromOpaque at a second call site, and the comment above now refers to a guard spread across three lines. A surfaceView(from ud:) overload would keep the pointer-casting in one place.
Also worth a note in the comment: surfaceDidPasteText is dispatched async before the synchronous complete_clipboard_request, so the evidence lands after the paste reaches the surface but before any subsequent Return. That ordering is correct and load-bearing, and currently unremarked.
There was a problem hiding this comment.
Added a surfaceView(from ud:) overload with surface(from:) delegating to it, so the cast lives in one place. Also noted why the paste record is dispatched before the synchronous completion.
…g one The ABI refresh deleted the existing xcframework up front and only checked the downloaded replacement afterwards, so a release that also lacked GHOSTTY_ACTION_OUTPUT_ACTIVITY left the tree with no framework at all — a checkout that could previously build, broken, with every re-run repeating the failure. Download and validate into a scratch dir and swap only on success, so a bad release is a no-op instead of a regression. The probe also named the macos-arm64_x86_64 slice. A renamed slice would have reported "missing ABI" forever, re-downloading on every setup run with no indication why; glob the slice dirs instead. Requiring the ABI is a bisect hazard, so say so where someone hits it: the failure message now explains that pre-change checkouts need a GhosttyKit from a contemporary release, and points at AGENTS.md.
… one place Bare keycode literals behind a `// H/C/W/U/K` gloss are easy to transpose on edit, and nothing downstream notices when they change which keys count as a submission or wipe the evidence — the escape/delete line was already two labels for three codes. Name them, and pin them to HotkeyRegistry's shared vocabulary in a test: TerminalCommandSubmission stays isolation-free so it can't read that @mainactor map at runtime, but it can be held to it. readClipboard open-coded Unmanaged.fromOpaque at a second call site; give surfaceView an overload taking the userdata pointer so the teardown guard and the cast stay together. Also record why the paste evidence is dispatched before the synchronous clipboard completion — that ordering is load-bearing and was unremarked.
The scheduled wake fired at 3s and the settle it triggers requires now - lastActivityAt >= 3s, so it only worked while timer jitter ran positive. An early wake would no-op the settle, and with every window occluded there is no timer left to retry — the run would sit at .running forever, which is exactly what this machinery exists to prevent. Derive both from one constant with an explicit margin so they can't drift apart or meet. preserveProgrammaticCommandInput carried a payload's content evidence for 2s, and the evidence only ORs in — so a `pane run "cmd"` followed by a genuinely blank Return inside that window reported content it didn't have. The carry is only needed where a bracketed paste can swallow the newline, which is the same agent-TUI foreground the in-place heuristic requires, so gate it on that and keep the ambiguous window out of the shell case. Also: reschedule the wake with a work item rather than cancelling and respawning a Task at ~2 Hz per live pane, matching the idiom the view already uses; state the interaction-before-submission ordering contract at both ends, since recordUserInteraction clears the arming that recordCommandSubmission sets; and wait for the wake by polling instead of a fixed 100ms sleep against a 20ms timer (the shape that flaked in thdxg#181).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@onliner10 Thanks for your work! |
2 participants
What
Makes the tab activity indicator follow real terminal work reliably, including in-place redraws from same-PID Pi
!commands and output from fully occluded tabs. Active tabs now render their actual execution state without special masking.Also exposes the underlying
idle/running/donestate additively inmacterm pane list.Why
The existing renderer/scrollback signals stop when a surface is occluded and miss raw-mode TUIs that redraw in place. Process polling cannot identify nested Pi shell commands because Pi keeps the same foreground PID throughout the command.
How
GHOSTTY_ACTION_OUTPUT_ACTIVITYsignal from the PTY IO path.sendText/sendKeypaths, with destructive edits and cancellation clearing stale evidence.Verified
mise run format,mise run lint, andmise run testall passmise run run) and confirmed the behaviorValidated against the official
thdxg/ghosttybuild-2026-07-24universal framework, built fromc459b2aafter thdxg/ghostty#5:! sleep 6transitionedrunning → donewhile the window was minimized, with the foreground Pi PID unchanged.sendText/sendKey, start and finish activity correctly.Notes for reviewers
The Ghostty dependency is already merged and released: thdxg/ghostty#5. The output heartbeat is emitted from the PTY IO path rather than the renderer, so it remains available while the terminal surface is occluded.
The local Xcode installation prints its existing stale-CoreSimulator warning, but macOS tests and the release archive complete successfully.
Summary by CodeRabbit
idle,running,done(including when tracking is enabled).pane listoutput documentation to include execution state.