feat(graphics): add direct pane frame streaming - #2523
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 layered pane graphics, direct Kitty file transport, bounded retained rendering, BGRA normalization, frame acknowledgements, pixel-aware mouse input, and placement-only resize replay. ChangesPane graphics platform
Pixel mouse input
Documentation and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/headless.rs (1)
3057-3072: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the preserved cell size to
runtime.resize, not the raw event values.Lines 3061-3063 keep the stored
cell_sizewhen the client reports an unknown cell size. Line 3071 then callsruntime.resize(rows, cols, cell_width_px, cell_height_px)with the raw event values. Whencell_width_pxandcell_height_pxare zero, the runtime loses its pixel geometry whileclient.cell_sizestill holds the last known value. The two then disagree.
attach_terminal_clientalready uses the stored value at line 2702:runtime.resize(rows, cols, cell_size.width_px, cell_size.height_px). Apply the same rule here.🐛 Proposed fix: capture and forward the effective cell size
- let direct_terminal_id = if let Some(ClientConnection { + let direct_terminal = if let Some(ClientConnection { mode: ClientConnectionMode::TerminalAttach { terminal_id }, terminal_size, cell_size, render_state, .. }) = self.clients.get_mut(&client_id) { *terminal_size = (cols, rows); let observed = crate::kitty_graphics::HostCellSize { width_px: cell_width_px, height_px: cell_height_px, }; if observed.is_known() { *cell_size = observed; } render_state.request_repaint(); - Some(terminal_id.clone()) + Some((terminal_id.clone(), *cell_size)) } else { None }; - if let Some(terminal_id) = direct_terminal_id { + if let Some((terminal_id, effective_cell_size)) = direct_terminal { if let Some(runtime) = self.runtime_for_terminal_id_string(&terminal_id) { - runtime.resize(rows, cols, cell_width_px, cell_height_px); + runtime.resize( + rows, + cols, + effective_cell_size.width_px, + effective_cell_size.height_px, + ); } return true; }src/client/mod.rs (1)
562-579: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset DEC mode 1016 on Windows when disabling mouse capture.
clear_host_mouse_reportingis a no-op on Windows, soset_mouse_capture(false, ...)emits no\x1b[?1016l. If pixel reporting was enabled, Windows can continue sending pixel coordinates whilehost_sgr_pixels_activeis false.
🧹 Nitpick comments (16)
src/server/headless/pane_graphics.rs (1)
543-555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the oversized-serialization fallback.
Err(_) => return RetainedGraphicsOutcome::Fallbackon Line 553 is the only fallback path in this function without acrate::render_prof::event(...)marker; the paths on Lines 475, 486, 499, 507, 511, 515 and 519 all emit one. The previoustracing::warnfor oversized serialization was also removed. An oversized pane-graphics frame now triggers a full redraw with no signal in either profile events or logs.Add an event marker, and a
warn!if you want the payload size in the logs.♻️ Proposed change
) { Ok(serialized) => Some(serialized), - Err(_) => return RetainedGraphicsOutcome::Fallback, + Err(err) => { + crate::render_prof::event("retained_graphics_fallback.oversized"); + warn!(client_id, err = %err, "pane graphics frame exceeded the graphics frame limit"); + return RetainedGraphicsOutcome::Fallback; + } }As per coding guidelines: "use
tracingfor logging".Source: Coding guidelines
src/server/headless.rs (3)
3133-3147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the now-dead
deferred_rendercomputation.Both arms of the match at lines 3144-3146 return
RenderImpact::Full. The lookup at lines 3134-3140 therefore has no effect on the result. It still runs aHashMaplookup on every server event.♻️ Proposed refactor
fn handle_server_event_with_render_impact(&mut self, ev: ServerEvent) -> RenderImpact { - let deferred_render = match &ev { - ServerEvent::ClientWriterDrained { client_id } => self - .clients - .get(client_id) - .map_or(DeferredRender::None, ClientConnection::deferred_render), - _ => DeferredRender::None, - }; if !self.handle_server_event(ev) { return RenderImpact::None; } - match deferred_render { - DeferredRender::None | DeferredRender::Full => RenderImpact::Full, - } + RenderImpact::Full }Check whether
DeferredRenderand thedeferred_renderaccessor remain used elsewhere before you drop the import.
3767-3800: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the focused-pane lookup and skip it when no client supports pixel mouse.
stream_host_mouse_capture_moderuns on every event-loop iteration. Lines 3772-3794 repeat the active-workspace and focused-pane traversal thatfocused_pane_graphics_demandalready performs at lines 3759-3764, then resolve the runtime and read its input state.
client_sgr_pixelsat line 3800 isfalsefor every client that does not setpixel_mouse. When no attached client sets it, the entiresgr_pixelscomputation is discarded. Compute it lazily.♻️ Proposed refactor
+ fn focused_pane_sgr_pixels_requested(&self) -> bool { + let Some(ws_idx) = self.app.state.active else { + return false; + }; + let Some(pane_id) = self + .app + .state + .workspaces + .get(ws_idx) + .and_then(crate::workspace::Workspace::focused_pane_id) + else { + return false; + }; + if !self.app.pane_graphics.active_for_pane(pane_id) { + return false; + } + self.app + .state + .runtime_for_pane_in_workspace(&self.app.terminal_runtimes, ws_idx, pane_id) + .and_then(crate::terminal::TerminalRuntime::input_state) + .is_some_and(|state| { + state.mouse_protocol_encoding == crate::input::MouseProtocolEncoding::SgrPixels + }) + } + fn stream_host_mouse_capture_mode(&mut self) { let enabled = self .app .state .should_capture_host_mouse_from(&self.app.terminal_runtimes); - let sgr_pixels = self.focused_pane_graphics_demand() - && self - .app - .state - .active - .and_then(|ws_idx| { - ... - }) - .and_then(crate::terminal::TerminalRuntime::input_state) - .is_some_and(|state| { - state.mouse_protocol_encoding == crate::input::MouseProtocolEncoding::SgrPixels - }); + let any_pixel_mouse_client = self + .clients + .values() + .any(|client| client.is_full_app_client() && client.pixel_mouse); + let sgr_pixels = any_pixel_mouse_client && self.focused_pane_sgr_pixels_requested();
5581-5581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a client that negotiates direct graphics.
Every
ClientConnectedfixture in this module setsdirect_graphics: false. No test constructs a client withdirect_graphics: true, and no test exercises theServerEvent::ClientInputPixelsarm at lines 2900-2928.That arm contains the validation chain for pixel reports, including the geometry consistency check flagged separately. Add tests that cover:
- A client with
direct_graphics: truesets bothconnection.direct_graphicsandconnection.pixel_mouse.direct_graphics_available()returnstruefor exactly one such foreground app client, andfalsewhen a second app client attaches.ClientInputPixelsis rejected whenhost_sgr_pixels_activeis notSome(true), whenterminal_sizedisagrees withgeometry, and when the geometry reports zero columns.The
AppStateandWorkspacetest constructors already used in this module support these cases without a PTY.Source: Coding guidelines
src/app/api/pane_graphics.rs (1)
421-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
stream_activeis taken rather than cloned.
Slot::dropstoresfalseintostream_active. Theinserton Line 427 drops the replaced slot.take()on Line 425 moves the handle out first, so the drop cannot deactivate the live stream. Without thattake, every streamed frame would close its own stream.The invariant is not obvious from the code. Add a short comment so a later refactor does not change
take()toclone().♻️ Proposed comment
let (stream_owner, stream_active) = self .pane_graphics .slots .get_mut(&key) + // Move the handle out: `insert` below drops the replaced slot, and + // `Slot::drop` would otherwise deactivate the live stream. .map(|slot| (slot.stream_owner.clone(), slot.stream_active.take())) .unwrap_or_default();src/app/pane_graphics.rs (1)
278-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
attach_stream_activeandretain_live_panes.The test module covers the two capacity bounds only.
attach_stream_activeandretain_live_panescarry the lifecycle rules that decide whether a layer can ever be reclaimed, and neither has a test. A test that opens two layers for one owner and then asserts both slots share the attached handle would have caught the defect flagged on Line 159-171.src/app/state.rs (1)
1496-1497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
host_mouse_pixelsinassert_invariants_for_test.
host_mouse_pixelsis transient state that must not survive a dispatch.assert_invariants_for_testalready asserts that comparable transient fields such asdrag,selection_autoscroll, andright_click_passthroughare absent for an empty state. Add the same assertion so a future change that leaks the provenance window fails a test.The coding guidelines ask for the invariant test helpers to cover state refactors.
♻️ Proposed assertion in the empty-state block
assert!( self.context_menu.is_none(), "empty app state must not keep context menu" ); + assert!( + self.host_mouse_pixels.is_none(), + "empty app state must not keep host mouse pixel provenance" + ); return;Source: Coding guidelines
src/client/input.rs (1)
285-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a log for the dropped pixel mouse report.
If
sgr_pixelsis true andgeometryisNone,classify_unix_inputreturnsNone.send_unix_input_chunksthen drops the chunk at Line 267 without any record. Geometry staysNonewhenHostGeometry::current()never succeeded, so every mouse event disappears silently in that state. Add atracingline to make the condition diagnosable.♻️ Proposed log on the drop path
fn classify_unix_input( data: Vec<u8>, sgr_pixels: bool, geometry: Option<crate::input::mouse::HostGeometry>, ) -> Option<ClientLoopEvent> { if sgr_pixels && crate::input::mouse::parse_report(&data).is_some() { + if geometry.is_none() { + tracing::debug!("dropping SGR pixel mouse report: host geometry unavailable"); + } return geometry.map(|geometry| ClientLoopEvent::PixelMouse(data, geometry)); } Some(ClientLoopEvent::StdinInput(data)) }As per coding guidelines: "use
tracingfor logging".Source: Coding guidelines
src/client/mod.rs (3)
367-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated
set_mouse_capturebranches.Both branches at Line 368-372 and Line 382-386 call
set_mouse_capturewith the same second argument and differ only in the first. Passmouse_capturedirectly.♻️ Proposed simplification
if enable_client_protocols { - if mouse_capture { - set_mouse_capture(true, false)?; - } else { - set_mouse_capture(false, false)?; - } + set_mouse_capture(mouse_capture, false)?; execute!(io::stdout(), EnableBracketedPaste, EnableFocusChange)?; @@ - if mouse_capture { - set_mouse_capture(true, false)?; - } else { - set_mouse_capture(false, false)?; - } + set_mouse_capture(mouse_capture, false)?; }
718-733: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider blocking GNU screen alongside tmux.
direct_graphics_profile_allowedtreatsTMUXas a blocked transport at Line 730. GNU screen is the equivalent multiplexer and setsSTY. Under screen the terminal that renders the frame can differ from the process that wrote the file, which is the same risk that theTMUXcheck addresses. AddSTYto the blocked set.The failure mode is a rejected Kitty file transfer and a fallback, so this is a robustness improvement rather than a correctness break.
♻️ Proposed change
direct_attach || is_remote_client_process() || std::env::var_os("SSH_CONNECTION").is_some() || std::env::var_os("SSH_TTY").is_some() - || std::env::var_os("TMUX").is_some(), + || std::env::var_os("TMUX").is_some() + || std::env::var_os("STY").is_some(),
1825-1838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
next_sgr_pixelstoset_mouse_capture.Line 1829 passes
sgr_pixels, and Line 1838 storesnext_sgr_pixels. The two agree today becauseset_mouse_captureignores the second argument whenenabledis false. Passing the same value in both places keeps the tracked state and the emitted modes aligned if the enable logic changes.♻️ Proposed change
if mouse_mode_changed { - set_mouse_capture(enabled, sgr_pixels) + set_mouse_capture(enabled, next_sgr_pixels) .map_err(ClientError::ConnectionFailed)?;src/kitty_graphics.rs (1)
272-274: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftStatic pane-graphics scenes cost O(N) transactions and O(N) cache clones per frame. Two changes combine into one per-frame cost.
request_placement_replay()runs unconditionally on every incremental encode, which marks every placement as needing a re-display. The incremental encoder then clones the whole cache once per candidate placement while it walks that work list, andpaint_local_pane_graphicsre-enters the encoder once per emitted transaction.
src/kitty_graphics.rs#L272-L274: gaterequest_placement_replay()on a view change or a full-redraw request, matching the legacy path at Line 242-244. If the host text blit overwrites the graphics surface every frame, keep the unconditional call and record that reason in a comment.src/kitty_graphics.rs#L567-L607: remove the speculativecache.clone()at Line 588 by splittingencode_placement_updateinto a decision step and an apply step, so the cache is mutated only when a transaction is committed.src/pane_graphics_files.rs (4)
136-144: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStale generation directories survive an abnormal exit.
Dropremoves the generation root only on a graceful shutdown. If the process receives SIGKILL or aborts, theserver-{pid}-{nonce}directory and its frame files remain under the runtime base. Nothing in this module reaps them, andcreate_generationat Line 181 creates the base without sweeping siblings.Each leaked directory holds raw pane pixel data at 0600. The data is same-user readable only, so this is a disk-growth and stale-data-retention concern rather than an exposure. On
/var/tmpthere is no reboot cleanup, unlikeXDG_RUNTIME_DIR.Add a startup sweep that removes
server-*directories whose pid is no longer alive.♻️ Sketch of a startup sweep
fn create_generation(base: &Path) -> io::Result<Generation> { @@ fs::create_dir_all(base)?; fs::set_permissions(base, fs::Permissions::from_mode(DIRECTORY_MODE))?; validate_directory(base)?; + remove_stale_generations(base); let nonce = std::time::SystemTime::now()#[cfg(unix)] fn remove_stale_generations(base: &Path) { let Ok(entries) = fs::read_dir(base) else { return; }; for entry in entries.flatten() { let Some(pid) = entry .file_name() .to_str() .and_then(|name| name.strip_prefix("server-")) .and_then(|rest| rest.split('-').next()) .and_then(|pid| pid.parse::<i32>().ok()) else { continue; }; // SAFETY: `kill` with signal 0 only probes for process existence. if unsafe { libc::kill(pid, 0) } == 0 { continue; } if let Err(err) = fs::remove_dir_all(entry.path()) { tracing::warn!(path = %entry.path().display(), err = %err, "failed to remove stale pane graphics directory"); } } }Note that pid reuse makes the
killprobe conservative, not exact. It only ever keeps directories, so it cannot delete a live generation.
200-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd SAFETY comments to the
libc::geteuid()blocks.Line 207-209 wraps
libc::geteuid()inunsafewith no justification comment. The same pattern repeats at Line 246 and Line 268.geteuidcannot fail and has no preconditions, so a one-line comment documents that and keeps the unsafe surface auditable.♻️ Proposed comment
- root.join(format!("herdr-pane-graphics-{}", unsafe { - libc::geteuid() - })) + // SAFETY: `geteuid` takes no arguments, cannot fail, and has no preconditions. + let euid = unsafe { libc::geteuid() }; + root.join(format!("herdr-pane-graphics-{euid}"))
334-402: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd direct coverage for
validate_direct_source.The test module covers
lease, which sharesvalidate_metadata,open_no_follow, andvalidate_path_identity. It does not covervalidate_direct_sourceat Line 147.
validate_direct_sourceis the client-side gate that decides whether a server-supplied path may be handed to the terminal. Its path-shape checks at Line 150-160 and its directory-privacy loop at Line 161-163 have no test. Those checks are pure logic and are cheap to exercise.Cover at least these rejections: a relative path, a parent directory not named
source, a generation directory without theserver-prefix, and a path whose grandparent is not the runtime base.Do you want me to generate these tests?
118-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse positional reads in
copy_rgba.
File::try_clone()shares the Unix file offset, so concurrent calls can interfere throughrewind()andread_to_end(). UseFileExt::read_exact_atat offset0, then useread_atfor a one-byte probe atself.inner.len as u64to retain growth detection. Gate the Unix-only API because this module also compiles on Windows. This removes the per-call descriptor clone and rewind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4bbea6a-d4ae-4dd8-ad0c-2f2d08d863b1
📒 Files selected for processing (41)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsondocs/next/website/src/content/docs/socket-api.mdxsrc/api/mod.rssrc/api/schema.rssrc/api/schema/panes.rssrc/api/schema/response.rssrc/api/server.rssrc/api/server/pane_graphics_stream.rssrc/app/actions.rssrc/app/api.rssrc/app/api/pane_graphics.rssrc/app/ids.rssrc/app/input/mod.rssrc/app/input/mouse.rssrc/app/mod.rssrc/app/pane_graphics.rssrc/app/runtime.rssrc/app/state.rssrc/client/direct_graphics.rssrc/client/input.rssrc/client/mod.rssrc/ghostty/mod.rssrc/input/encode.rssrc/input/mod.rssrc/input/model.rssrc/input/mouse.rssrc/kitty_graphics.rssrc/main.rssrc/pane.rssrc/pane/input.rssrc/pane/terminal.rssrc/pane_graphics_files.rssrc/protocol/wire.rssrc/server/alt_screen_read.rssrc/server/client_transport.rssrc/server/clients.rssrc/server/headless.rssrc/server/headless/pane_graphics.rssrc/server/headless/tests/pane_graphics.rssrc/terminal/runtime.rs
💤 Files with no reviewable changes (1)
- src/app/actions.rs
|
Review follow-up in
The Windows DEC 1016 reset note is not applicable to the current path: direct/pixel mouse negotiation is Unix-only, and Windows clients therefore never receive Protocol stays at 20 intentionally: stable and current preview publish protocol 19, and project policy keeps multiple pre-publication incompatible changes on the same next protocol number. |
Greptile SummaryThis PR adds bounded pane-graphics layers and direct regular-file frame streaming for eligible local terminal clients, together with pixel mouse metadata and cleanup/acknowledgement handling.
Confidence Score: 4/5The PR is not yet safe to merge because mixed version-20 clients and servers can accept incompatible binary wire layouts and then misroute launch modes or disconnect. The previously reported protocol compatibility failure remains: the wire format changes Files Needing Attention: src/protocol/wire.rs
|
| Filename | Overview |
|---|---|
| src/protocol/wire.rs | Adds direct-graphics launch and transport messages plus pixel mouse data, but the previously reported unchanged protocol-version incompatibility remains. |
| src/api/server/pane_graphics_stream.rs | Extends dedicated streams with named layers and direct-file headers; downstream validation prevents arbitrary paths from reaching the terminal. |
| src/pane_graphics_files.rs | Introduces private frame generations, strict ownership and metadata validation, identity checks, leases, and cleanup. |
| src/app/api/pane_graphics.rs | Implements layered graphics limits, stream ownership, direct-frame leasing, acknowledgements, and fallback state. |
| src/server/headless/pane_graphics.rs | Coordinates direct frame delivery, per-client capability checks, fallback, and transmission lifecycle. |
| src/client/direct_graphics.rs | Adds client-side direct graphics transmission tracking and acknowledgement behavior. |
| src/kitty_graphics.rs | Expands Kitty graphics encoding and cache behavior for layered, inline, and regular-file frames. |
Sequence Diagram
sequenceDiagram
participant Producer as Pane graphics producer
participant API as Graphics stream API
participant App as App graphics state
participant Server as Headless server
participant Client as Local terminal client
participant Terminal as Kitty-compatible terminal
Producer->>API: Open named layer stream
API->>App: Claim layer
Producer->>API: Submit inline frame or private file path
API->>App: Validate and install frame
App->>Server: Render transaction
alt eligible direct RGBA file
Server->>Client: GraphicsFile
Client->>Client: Revalidate private frame path
Client->>Terminal: Kitty regular-file command
Terminal-->>Client: Transmission result
else owned fallback
Server->>Client: Inline graphics bytes
Client->>Terminal: Kitty inline command
end
Client-->>Server: Frame accepted
Server-->>API: Frame acknowledgement
API-->>Producer: sequence/revision acknowledgement
Reviews (4): Last reviewed commit: "Merge branch 'master' into feat/shared-p..." | Re-trigger Greptile
|
Greptile protocol finding: not changing this. Protocol 20 is already the current source protocol on |
f44ae95 to
1ffc2a2
Compare
kangal-bot
removed
the
ai-review
2 participants
Summary
Validation
just check(3321 Rust tests plus integration, marketplace, Windows clippy, and maintenance checks)origin/master