fix: forward pane terminal bells - #2498
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds pane BEL counting and forwarding. PTY readers publish bell events, the server sends bell counts to the foreground client, and clients write BEL characters to the outer terminal. The wire protocol and related tests advance to version 20. ChangesTerminal bell forwarding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PaneTerminal
participant AppEventQueue
participant HeadlessServer
participant Client
participant OuterTerminal
PaneTerminal->>AppEventQueue: publish TerminalBell count
AppEventQueue->>HeadlessServer: deliver AppEvent::TerminalBell
HeadlessServer->>Client: send ServerMessage::TerminalBell count
Client->>OuterTerminal: write terminal bells
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/terminal_effects.rs (1)
5-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClamp terminal bell counts before forwarding.
bell_countcan reachu16::MAX, andwrite_terminal_bellsemits one BEL byte per count. Clamp the count before creatingAppEvent::TerminalBellto prevent a single PTY read from sending 65,535 BEL bytes to the outer terminal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b40ced45-9dce-4e9a-aa8f-8e30b5871ef3
📒 Files selected for processing (16)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsonsrc/app/actions.rssrc/app/api.rssrc/client/mod.rssrc/events.rssrc/ghostty/mod.rssrc/main.rssrc/pane.rssrc/pane/terminal.rssrc/protocol/wire.rssrc/server/headless.rssrc/terminal_effects.rstests/api_ping.rstests/cli/sessions.rstests/support/mod.rs
5b60dd2 to
392fc8a
Compare
Greptile SummaryThe PR adds end-to-end forwarding of libghostty BEL callbacks from pane PTY parsing to the monolithic host terminal or the headless server's foreground client, and bumps the client/server protocol to 20.
Confidence Score: 4/5The PR should not merge until terminal bells are retained or retried when the bounded application-event queue is temporarily full. Both PTY reader paths route bells through try_send, so queue pressure can discard the newly promised terminal side effect even though parsing and downstream forwarding are otherwise correctly wired. Files Needing Attention: src/pane.rs
|
| Filename | Overview |
|---|---|
| src/ghostty/mod.rs | Registers a saturating BEL callback counter and exposes a take-and-reset accessor. |
| src/pane/terminal.rs | Drains stale callback state before live PTY parsing and returns the resulting live bell count. |
| src/pane.rs | Publishes parsed bell counts from both PTY reader paths, but drops them whenever the bounded event queue is full. |
| src/server/headless.rs | Intercepts terminal-bell events and forwards their counts only to the foreground client. |
| src/client/mod.rs | Emits received bell counts to the attached client's stdout serially with other server messages. |
| src/protocol/wire.rs | Appends the TerminalBell server message and bumps the incompatible wire protocol to version 20. |
| src/app/api.rs | Handles monolithic bell events as non-rendering host-terminal side effects. |
| src/terminal_effects.rs | Writes and flushes the exact requested number of BEL bytes in bounded chunks. |
Sequence Diagram
sequenceDiagram
participant PTY as Pane PTY
participant Ghostty as libghostty
participant Runtime as PaneRuntime
participant Server as App / HeadlessServer
participant Client as Foreground client
participant Host as Outer terminal
PTY->>Ghostty: Output containing BEL
Ghostty->>Runtime: Parsed bell callback count
Runtime->>Server: AppEvent::TerminalBell
alt Monolithic mode
Server->>Host: Write count × BEL
else Headless mode
Server->>Client: ServerMessage::TerminalBell
Client->>Host: Write count × BEL
end
Reviews (1): Last reviewed commit: "fix: forward pane terminal bells" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if count == 0 { | ||
| return; | ||
| } | ||
| if let Err(err) = events.try_send(AppEvent::TerminalBell { pane_id, count }) { |
There was a problem hiding this comment.
When the bounded application-event channel is full, try_send discards the parsed terminal bell event, causing the outer terminal to miss the pane's audible or visual bell.
Knowledge Base Used:
There was a problem hiding this comment.
I’m keeping this best-effort on the existing bounded application-event channel. Blocking the PTY reader when the UI loop is under pressure can stall terminal output or deadlock against pane work; adding a reliable side-effect queue would be a broader transport-policy change than BEL forwarding.
There was a problem hiding this comment.
That's a sound tradeoff. PTY reader stalls would degrade the core terminal experience in a much more visible way than an occasional missed bell — and bells are inherently best-effort signals. The existing channel back-pressure semantics already encode that priority correctly.
The one low-cost improvement worth considering: log the drop so it's observable without adding any blocking:
| if let Err(err) = events.try_send(AppEvent::TerminalBell { pane_id, count }) { | |
| if let Err(err) = events.try_send(AppEvent::TerminalBell { pane_id, count }) { | |
| tracing::debug!(pane_id = ?pane_id, count, ?err, "bell event dropped (channel full)"); |
That way the behavior stays non-blocking but the drop isn't completely silent during debugging. Otherwise the approach is justified.
|
On CodeRabbit review 4886460553: I’m keeping the exact count. Production PTY reads are bounded to 8 KiB, so one parsed read cannot reach |
kangal-bot
removed
the
ai-review
3 participants
Summary
Checks
printf '\a'interaction emitted 0 BEL bytes before and exactly 1 afterjust ci 'all() - test(live_server_holds_one_pty_master_fd_per_pane)'(3254 tests passed),just windows-lint, and 97 maintenance testsjust checkreached an environment-only existing harness limitation: the isolated externalCARGO_TARGET_DIRprevents that one handoff test from recognizing its successfully started replacement process under checkouttarget/debugrefs #2453