fix(perf): eliminate redundant terminal wake work

[ogulcancelik]

Summary

• coalesce hidden-pane PTY notifications while preserving immediate visible and direct-observer wakes
• replace aggregate terminal input-state reads with narrow scalar queries, including a tracked libghostty-vt modifyOtherKeys getter
• add architecture enforcement and a fast stable-vs-candidate release performance smoke
• fold the unpublished v0.8.1 notes back into the v0.8.2 unreleased changelog

Performance

• Linux hidden50: -3.8%; visible30: -33.3%
• macOS hidden50: -2.6%; visible30: +5.0% (+0.26 CPU points)
• hidden-pane loop wake rate restored from about 2,800/s to about 120/s

Validation

• just check
• just pre-release-check
• Linux and macOS just bench-release-smoke
• Windows ghostty_modify_other_keys_mode_one_preserves_shift_enter runtime test

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f34191df-7c7c-4040-9ebd-aa47bce4b5ee

📥 Commits

Reviewing files that changed from the base of the PR and between d78bf69 and 33addf6.

📒 Files selected for processing (2)

• scripts/test_ui_hot_path_architecture.py
• src/server/headless.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The change replaces aggregate terminal-state reads with direct capability queries, adjusts PTY wakeup tracking for hidden and visible panes, adds architecture checks, and introduces a cross-platform release CPU smoke benchmark.

Changes

Runtime behavior

Layer / File(s)	Summary
Terminal capability contracts src/ghostty/*, src/input/*, src/pane/*, src/terminal/runtime.rs, vendor/*	Terminal APIs expose direct input-mode and modifyOtherKeys queries. Aggregate input state is restricted to Unix and test builds.
Direct capability consumers src/app/*, src/pane.rs, src/server/*	Input, paste, mouse, scrollback, and terminal-attach paths use narrow runtime accessors.
Visibility-aware PTY rendering src/app/mod.rs, src/app/state.rs, src/render_signal.rs, src/server/headless.rs	Render signaling tracks immediate PTY sources and coalesces hidden-pane output wakeups.
Architecture guardrails scripts/test_ui_hot_path_architecture.py	Architecture checks scan app and server Rust sources for aggregate terminal-state access.
Behavior and release notes CHANGELOG.md, docs/next/CHANGELOG.md	Changelogs describe hidden-pane wakeup and input-mode synchronization fixes.

Release performance validation

Layer / File(s)	Summary
Benchmark harness scripts/release_perf_case.sh, scripts/release_perf_producer.pl	New scripts create terminal scenarios, generate output, sample CPU usage, and validate benchmark artifacts.
Baseline comparison and thresholds scripts/release_perf_smoke.sh	The smoke test compares baseline and candidate binaries across hidden and visible scenarios and enforces CPU thresholds.
Release workflow integration justfile, AGENTS.md	The smoke test runs during pre-release checks. Release guidance requires a 60-second rerun when results move materially or when validating performance work.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to 33add

The PR changes terminal wake scheduling and adds release performance gating, but the current benchmark scripts can report success for an invalid zero-CPU comparison or an interrupted run, weakening the release check; duplicated pane-visibility logic can also drift and cause missed or extra wakeups. Merge readiness needs explicit owner follow-up on these bounded risks.

Sequence Diagram(s)

sequenceDiagram
  participant AppState
  participant RenderSignal
  participant HeadlessObserver
  AppState->>RenderSignal: Set immediate application-surface pane IDs
  HeadlessObserver->>RenderSignal: Request observed and hidden PTY sources
  RenderSignal-->>HeadlessObserver: Report immediate presentation work

Loading

Possibly related PRs

• herdrdev/herdr#2892: Both changes modify hidden-pane PTY wakeups and render scheduling.
• herdrdev/herdr#2578: Both changes modify terminal input-state handling and modifyOtherKeys.
• herdrdev/herdr#2554: Both changes replace aggregate input_state() reads with targeted terminal accessors.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change: removing redundant terminal wake work.
Description check	✅ Passed	The description directly covers the wake coalescing, terminal-state changes, benchmarks, architecture checks, and changelog updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/terminal-wake-performance

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (11)

scripts/release_perf_case.sh (4)

66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Validate root_pane and workspace_id like the loop validates pane_id.

jq -r prints the string null when the field is missing. Line 74 checks for that case, but lines 66 and 67 do not. A malformed API response makes the failure surface later as a confusing pane run or tab create error.

♻️ Proposed change

 root_pane=$(printf '%s\n' "$panes_json" | jq -r '.result.panes[0].pane_id')
 workspace_id=$("${control_env[@]}" "$bin" workspace list | jq -r '.result.workspaces[0].workspace_id')
+[[ -n "$root_pane" && "$root_pane" != null ]] || { echo "session did not report a root pane" >&2; exit 1; }
+[[ -n "$workspace_id" && "$workspace_id" != null ]] || { echo "session did not report a workspace" >&2; exit 1; }

---

101-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silence the two shellcheck findings with explicit directives.

Both hints are false positives here. Line 102 relies on deliberate word splitting of $all_pids, and the comma on line 105 belongs to the -stats value of top. Add directives so the lint output stays clean and the intent is documented.

♻️ Proposed change

 if [[ $platform == linux ]]; then
+  # shellcheck disable=SC2086 # intentional word splitting of the pid list
   pid_csv=$(printf '%s\n' $all_pids | paste -sd, -)
   LC_ALL=C pidstat -h -u -p "$pid_csv" 1 "$seconds" > "$raw"
 else
+  # shellcheck disable=SC2054 # the comma is part of the top -stats value
   top_args=(top -l $((seconds + 1)) -s 1 -stats pid,cpu,time -n 2)

Source: Linters/SAST tools

---

110-117: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Match the pid in the PID column instead of anywhere in the row.

mean_linux scans every field for target. Another numeric column, for example UID or the CPU-core index, can equal the pid value and cause a wrong row to be attributed. With pidstat -h, the pid is always the third field.

The sample-count assertion on line 131 catches most of these cases, so this is robustness only.

♻️ Proposed change

   awk -v target="$2" '
     /^Linux/ || /^`#/` || NF < 5 { next }
-    { found=0; for (i=1; i<=NF; i++) if ($i == target) { found=1; break }
-      if (found && $(NF-2) ~ /^[0-9]+([.][0-9]+)?$/) { sum += $(NF-2); count++ } }
+    $3 == target && $(NF-2) ~ /^[0-9]+([.][0-9]+)?$/ { sum += $(NF-2); count++ }
     END { if (!count) exit 1; printf "%.6f,%d", sum/count, count }

---

41-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Interrupted runs exit with status 0 in both benchmark scripts. Both scripts bind one cleanup function to EXIT INT TERM. On INT or TERM the handler returns after rm -rf, and the script exits with the status of the last command in cleanup, which is normally 0. An interrupted benchmark then looks like a successful one to the caller and to just pre-release-check.

• scripts/release_perf_case.sh#L41-L53: keep trap cleanup EXIT and add trap 'cleanup; exit 130' INT and trap 'cleanup; exit 143' TERM.
• scripts/release_perf_smoke.sh#L27-L28: apply the same split so an interrupted smoke run reports failure instead of printing no verdict and exiting 0.

scripts/release_perf_producer.pl (2)

11-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Clamp the schedule when the producer falls behind.

$next advances by exactly $period on every iteration. If the process is descheduled or the write blocks, $remaining becomes negative and the loop emits lines with no sleep until it catches up. The producer then competes for CPU with the process under measurement and distorts the benchmark result, which is the value the release gate compares.

Reset the schedule when the deficit exceeds one period.

♻️ Proposed change

 while (1) {
     $sequence++;
     printf "\rbench-output-%08d-%s", $sequence, $label;
     $next += $period;
-    my $remaining = $next - clock_gettime(CLOCK_MONOTONIC);
-    sleep $remaining if $remaining > 0;
+    my $now = clock_gettime(CLOCK_MONOTONIC);
+    my $remaining = $next - $now;
+    if ($remaining > 0) {
+        sleep $remaining;
+    } elsif ($remaining < -$period) {
+        $next = $now + $period;
+    }
 }

---

6-9: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Validate that $rate is a positive number.

Line 11 computes 1 / $rate. A non-numeric or zero $rate produces a division-by-zero fatal error or a warning-laden period. The current guard only rejects falsy values.

♻️ Proposed change

 my ($rate, $gate, $label) = `@ARGV`;
-die "usage: $0 <rate-hz> <gate-file> <label>\n" unless $rate && $gate && $label;
+die "usage: $0 <rate-hz> <gate-file> <label>\n"
+    unless defined $rate && defined $gate && defined $label && length $label;
+die "rate must be a positive number\n" unless $rate =~ /^[0-9]*\.?[0-9]+$/ && $rate > 0;

scripts/release_perf_smoke.sh (2)

31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Resolve website/latest.json relative to the repository, not the current directory.

Line 33 reads website/latest.json from the current working directory. just bench-release-smoke runs from the repository root, so that path works. A direct invocation from any other directory fails with a jq error that does not name the missing file. script_dir is already available in this script; compute it before this block and derive the path from it.

♻️ Proposed change

+script_dir=$(cd "$(dirname "$0")" && pwd)
+repo_root=$(cd "$script_dir/.." && pwd)
+
 baseline=${HERDR_PERF_BASELINE_BIN:-}
 if [[ -z "$baseline" ]]; then
-  baseline_version=$(jq -er '.version' website/latest.json)
+  baseline_version=$(jq -er '.version' "$repo_root/website/latest.json")

Then drop the duplicate script_dir assignment on line 44.

---

11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Check for curl only when the script downloads the baseline.

Line 11 requires curl on every run. When HERDR_PERF_BASELINE_BIN points at a local binary, the script never calls curl. This blocks offline runs with a local baseline for no reason.

justfile (1)

142-142: 📐 Maintainability & Code Quality | 🔵 Trivial

Note the added wall-clock cost and the network dependency of pre-release-check.

bench-release-smoke runs 8 benchmark cases. Each case creates up to 50 panes, waits HERDR_PERF_WARMUP_SECONDS, and samples for HERDR_PERF_SAMPLE_SECONDS. It also downloads the current stable binary from GitHub unless HERDR_PERF_BASELINE_BIN is set. pre-release-check therefore now needs network access and takes several minutes longer.

Consider documenting the expected duration and the HERDR_PERF_BASELINE_BIN escape hatch next to this target.

AGENTS.md (1)

61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Name the "longer release matrix" so the instruction is actionable.

The new text tells the reader when to use "the longer release matrix" but does not say what it is. The surrounding section names concrete commands, for example just bench-render-scale. Reference the specific command or document for the longer matrix.

src/server/headless.rs (1)

4090-4136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the two independent pane-visibility implementations.

sync_immediate_pty_sources rebuilds pane visibility from app_surface_pane_ids() plus a manual scan of every workspace/tab for direct-terminal-target matches. pty_source_visible_to_render_targets (used later at line 751 to classify the render plan) computes the same "is this pane visible to a connected render target" rule per pane, using terminal_id_for_pane and app_surface_contains_pane.

Both derive from pty_render_targets(), but they express the visibility rule in two separate places. They agree today, but a future change to one rule (for example, a new client mode, or a change to zoom/popup semantics) can silently desynchronize the other, causing either missed wakes for genuinely visible panes or unnecessary wakes for hidden ones.

Build the immediate_pty_sources set by iterating candidate panes and calling the existing per-pane pty_source_visible_to_render_targets check (or extract a single shared helper that both call sites use), instead of maintaining two separate reconstructions of the same rule.

♻️ Illustrative direction (not a drop-in patch)

 fn sync_immediate_pty_sources(&self) {
     let (has_app_target, direct_terminal_targets) = self.pty_render_targets();
-    let mut pane_ids = if has_app_target {
-        self.app.state.app_surface_pane_ids()
-    } else {
-        HashSet::new()
-    };
-    if !direct_terminal_targets.is_empty() {
-        for workspace in &self.app.state.workspaces {
-            for tab in &workspace.tabs {
-                pane_ids.extend(tab.panes.iter().filter_map(|(&pane_id, pane)| {
-                    direct_terminal_targets
-                        .contains(pane.attached_terminal_id.as_str())
-                        .then_some(pane_id)
-                }));
-            }
-        }
-        if let Some(popup) = &self.app.state.popup_pane {
-            if direct_terminal_targets.contains(popup.terminal_id.as_str()) {
-                pane_ids.insert(popup.pane_id);
-            }
-        }
-    }
+    let pane_ids = self
+        .all_known_pane_ids()
+        .filter(|&pane_id| {
+            self.pty_source_visible_to_render_targets(pane_id, has_app_target, &direct_terminal_targets)
+        })
+        .collect();
     self.app.render_dirty.set_immediate_pty_sources(pane_ids);
 }

Also consider adding a test that connects both an App client and a TerminalAttach/TerminalObserve client at once, to exercise the combined has_app_target && !direct_terminal_targets.is_empty() branch, which no current test covers.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: af4bebeb-5035-44e9-8ca4-130f76448178

📥 Commits

Reviewing files that changed from the base of the PR and between 3667151 and 4e78166.

📒 Files selected for processing (28)

• AGENTS.md
• CHANGELOG.md
• docs/next/CHANGELOG.md
• justfile
• scripts/release_perf_case.sh
• scripts/release_perf_producer.pl
• scripts/release_perf_smoke.sh
• scripts/test_ui_hot_path_architecture.py
• src/app/actions.rs
• src/app/api_helpers.rs
• src/app/input/mouse.rs
• src/app/input/terminal.rs
• src/app/mod.rs
• src/app/state.rs
• src/ghostty/bindings.rs
• src/ghostty/mod.rs
• src/input/mod.rs
• src/input/model.rs
• src/pane.rs
• src/pane/terminal.rs
• src/render_signal.rs
• src/server/headless.rs
• src/server/terminal_attach.rs
• src/terminal/runtime.rs
• vendor/libghostty-vt.patches.md
• vendor/libghostty-vt/include/ghostty/vt/terminal.h
• vendor/libghostty-vt/src/terminal/c/terminal.zig
• vendor/patches/libghostty-vt/0002-expose-modify-other-keys-mode.patch

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[ogulcancelik]

Addressed the benchmark review in d78bf69: interrupt failures, API ID validation, stable PID parsing, producer rate/catch-up validation, local-baseline/off-root support, benchmark sanity checks, and concrete release guidance. I also added coverage for the combined app + direct terminal observer path.

I kept the two visibility paths separate intentionally: synchronization materializes known immediate pane sources using the active-surface fast path, while request classification must fail open for stale/unknown pane IDs. Iterating all panes through request classification on every synchronization would widen a pane-scaled hot path.

[greptile-apps]

Greptile Summary

The PR reduces redundant terminal wake work and replaces aggregate terminal input-state reads with narrow mode queries.

• Coalesces hidden-pane PTY notifications while retaining immediate presentation work for visible panes and direct observers.
• Adds scalar Ghostty terminal-mode accessors, including a vendored modifyOtherKeys query.
• Adds architecture checks and stable-versus-candidate release performance smoke tooling.
• Consolidates the unpublished release notes into the current unreleased changelog.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/render_signal.rs	Introduces visibility-aware PTY wake coalescing while preserving generic and terminal-title presentation work.
src/server/headless.rs	Integrates immediate PTY-source tracking with app clients and direct terminal observers in the headless render loop.
src/app/state.rs	Computes the popup and active-tab pane set currently presented by the app surface.
src/pane/terminal.rs	Adds narrow terminal-mode queries and replaces aggregate input-state reads on hot input paths.
src/ghostty/mod.rs	Exposes the vendored modifyOtherKeys state through the Rust Ghostty wrapper.
vendor/libghostty-vt/src/terminal/c/terminal.zig	Adds the underlying terminal-data query for modifyOtherKeys mode.
scripts/release_perf_smoke.sh	Orchestrates stable-versus-candidate CPU comparisons for visible and hidden output scenarios.
scripts/release_perf_case.sh	Creates isolated benchmark sessions, gathers process CPU samples, and cleans up benchmark resources.
scripts/test_ui_hot_path_architecture.py	Extends deterministic enforcement of narrow terminal-state access on multiplicative UI paths.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  PTY[PTY output] --> RS[RenderSignal coalescing]
  RS -->|visible pane or direct observer| IW[Immediate wake]
  RS -->|hidden pane only| CD[Cadence-deferred work]
  IW --> LOOP[App or headless server loop]
  CD --> LOOP
  LOOP --> FRAME[Render and stream frame]
  TERM[Ghostty terminal state] --> SQ[Scalar mode queries]
  SQ --> INPUT[Keyboard, paste, focus, mouse routing]

Loading

_{Reviews (2): Last reviewed commit: "test(perf): strengthen performance guard..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (2)

scripts/test_ui_hot_path_architecture.py (1)

19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add direct coverage for each aggregate-state rule.

The scanner tests use positional FORBIDDEN_CALLS indexes. They do not directly test keyboard_state_ansi or kitty_keyboard_state_ansi. A rule reorder or regex regression could leave these new guardrails untested. Add fixtures for each new pattern, or expose named rule constants instead of relying on tuple indexes.

src/server/headless.rs (1)

9787-9817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert immediate presentation work for a direct terminal observer.

The test proves that the observed pane is visible to a render target. It does not prove that sync_immediate_pty_sources() made the PTY request immediate. A regression in set_immediate_pty_sources() could therefore pass this test.

Add an assertion after requesting background_pane:

Proposed test strengthening

         let hidden_pane = server.app.state.workspaces[0].tabs[0].root_pane;
         server.sync_immediate_pty_sources();
-        assert!(server.app.render_dirty.request_pty(hidden_pane));
         assert!(server.app.render_dirty.request_pty(background_pane));
+        assert!(server.has_pending_presentation_work(false, false));
+        assert!(server.app.render_dirty.request_pty(hidden_pane));

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 290586d3-026a-4a61-b9bc-f960a146a3ff

📥 Commits

Reviewing files that changed from the base of the PR and between 4e78166 and d78bf69.

📒 Files selected for processing (7)

• AGENTS.md
• justfile
• scripts/release_perf_case.sh
• scripts/release_perf_producer.pl
• scripts/release_perf_smoke.sh
• scripts/test_ui_hot_path_architecture.py
• src/server/headless.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• justfile

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

[ogulcancelik]

Addressed the final CodeRabbit test suggestions in 33addf6: every aggregate-state scanner rule now has direct coverage, and the mixed app + terminal observer test proves that the observed background source itself creates immediate presentation work.