Skip to content

fix(graphics): coalesce terminal kitty placements - #3152

Closed
kataokatsuki wants to merge 2 commits into
herdrdev:masterfrom
kataokatsuki:fix/coalesce-terminal-kitty-placements
Closed

fix(graphics): coalesce terminal kitty placements#3152
kataokatsuki wants to merge 2 commits into
herdrdev:masterfrom
kataokatsuki:fix/coalesce-terminal-kitty-placements

Conversation

@kataokatsuki

@kataokatsuki kataokatsuki commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Problem

Since 624dfd4, a terminal image rendered through Unicode placeholders
is re-displayed one placement per frame. kitty_virtual_image_placements
yields one placement per viewport row the image covers, so a 23-row
image takes 23 frames (16 ms apart, 368 ms) to repaint on every redraw,
and each of those frames re-sends the whole cell grid. In Neovim
(snacks.nvim) images visibly fill in row by row. Not in any release: no
tag contains 624dfd4.

Cause

encode_graphics_update_incremental returns incomplete after the
first transaction, whether that transaction is a re-display (a=p,
~70 bytes) or a pixel upload.

Fix

Pure re-display transactions (image already uploaded and the source
already bound to it, so encode_placement_update emits neither an
upload nor a superseded-image delete) now coalesce into the transaction
being assembled, within the caller's transaction budget. Pixel uploads
and deletes keep a transaction of their own, and nothing joins after
them. Only the terminal-only budgeted path coalesces: with pane-layer
graphics active, terminal images keep the one-per-frame cadence, and
stale-placement deletes stay one per frame.

Verification

  • cargo fmt --check and cargo clippy --all-targets --locked -- -D warnings are clean.
  • cargo nextest run --locked with LIBGHOSTTY_VT_OPTIMIZE=ReleaseFast,
    excluding four tests that also fail on unmodified master
    (pane_spawn_cwd_fallback_in_server,
    live_handoff_keeps_unmanaged_agent_name_bound_to_saved_session,
    live_handoff_keeps_agent_started_pane_after_agent_exits,
    multi_client_client_crash_sigkill_does_not_affect_server):
    3,354 passed, 5 skipped.
  • New tests budgeted_image_rows_redisplay_in_one_transaction and
    budgeted_pixel_uploads_do_not_coalesce fail on unmodified master;
    budgeted_redisplay_coalescing_respects_budget pins the budget cutoff;
    budgeted_upload_keeps_other_redisplays_out and
    budgeted_superseded_image_delete_does_not_coalesce pin that uploads
    and superseded-image deletes stay in a transaction of their own.

A Unicode-placeholder image arrives as one placement per viewport row it
covers, and after 624dfd4 the budgeted encoder emitted one placement per
frame, so every redraw painted images one row per frame.
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review Change Stack

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: bc7b0331-3571-4048-900b-915aa9c90159

📥 Commits

Reviewing files that changed from the base of the PR and between e14041c and 2ed2fa6.

📒 Files selected for processing (1)
  • src/kitty_graphics.rs

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


📝 Walkthrough

Walkthrough

The graphics encoder now accepts a coalescing flag. It coalesces only pure redisplays that fit the transaction budget. Uploads and superseded-image deletes stop further coalescing. Tests cover these transaction boundaries.

Changes

Graphics transaction coalescing

Layer / File(s) Summary
Encoder contract and call sites
src/kitty_graphics.rs
The incremental encoder accepts a coalescing flag. Pane updates disable coalescing, while terminal incremental updates enable it. Existing tests use the expanded signature.
Budgeted redisplay transaction logic
src/kitty_graphics.rs
A placement joins the current transaction only when its image is cached and its source is already bound to that image. After an upload or superseded-image delete, later placements do not join the transaction.
Transaction behavior tests
src/kitty_graphics.rs
Tests cover multi-row redisplays, transaction budgets, fresh-upload separation, and superseded-image delete separation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2ed2f

This change coalesces terminal image redisplays so images repaint in one transaction instead of filling in row by row. No actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant GraphicsUpdate
  participant IncrementalEncoder
  participant ImageCache
  GraphicsUpdate->>IncrementalEncoder: encode incremental update with coalescing flag
  IncrementalEncoder->>ImageCache: check cached image and source binding
  ImageCache-->>IncrementalEncoder: pure redisplay or upload/delete required
  IncrementalEncoder->>IncrementalEncoder: coalesce pure redisplays within budget
  IncrementalEncoder-->>GraphicsUpdate: emit transactions
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description check ✅ Passed The description clearly explains the Kitty image redraw problem, its cause, the fix, and verification results.
Title check ✅ Passed The title clearly identifies the main change: fixing coalescing for terminal Kitty graphics placements.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@kangal-bot kangal-bot added the ai-review Trigger automated AI reviews for pull requests admitted by the PR gate label Aug 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f996534-308e-4c22-bf64-01f9a4d3088e

📥 Commits

Reviewing files that changed from the base of the PR and between d6dae88 and e14041c.

📒 Files selected for processing (1)
  • src/kitty_graphics.rs

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

Comment thread src/kitty_graphics.rs Outdated
Keep pixel uploads and superseded-image deletes in a transaction of
their own: a placement joins the coalesced transaction only when its
image is uploaded and its source already maps to that image, and
nothing joins after an upload or a delete.
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Greptile Summary

The PR updates budgeted terminal-only Kitty graphics rendering to batch cached placement replays while retaining one upload or cleanup operation per transaction.

  • Adds explicit detection of placement-only redisplays.
  • Coalesces redisplays only while the caller-provided transaction budget permits.
  • Keeps pane-layer rendering on the existing one-transaction cadence.
  • Adds regression coverage for multi-row replay, budget cutoffs, uploads, and deletes.

Confidence Score: 5/5

The PR appears safe to merge, with cached placement replay batching remaining bounded and isolated from uploads and cleanup operations.

The changed predicate restricts coalescing to already-uploaded, correctly bound images; the incremental loop still visits every placement and preserves replay progress across deferred frames.

Important Files Changed

Filename Overview
src/kitty_graphics.rs Adds budget-aware coalescing for pure terminal-image placement replays and comprehensive regression tests without changing upload or cleanup transaction isolation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Begin budgeted graphics update] --> B{Cleanup already emitted?}
  B -->|Yes| C[Return cleanup transaction]
  B -->|No| D[Inspect next placement]
  D --> E{Cached image and source binding match?}
  E -->|No| F[Emit one upload or update transaction]
  E -->|Yes| G[Encode placement replay]
  G --> H{Combined bytes fit budget?}
  H -->|Yes| I[Append replay and inspect next placement]
  H -->|No| J[Return transaction as incomplete]
  I --> D
  F --> J
  D -->|All visited| K[Clear replay state and return complete]
Loading

Reviews (1): Last reviewed commit: "fix(graphics): coalesce only pure kitty ..." | Re-trigger Greptile

@ogulcancelik

Copy link
Copy Markdown
Collaborator

thanks for finding this and putting together the first fix. while testing it in yazi, i found the same row splitting also affects image upload and cleanup, which causes black bands during preview changes. #3166 preserves your two commits and extends the fix across the full image lifecycle, so i'm superseding this one with that pr.

@kangal-bot kangal-bot removed the ai-review Trigger automated AI reviews for pull requests admitted by the PR gate label Aug 24, 2026
@kataokatsuki

Copy link
Copy Markdown
Contributor Author

makes sense — thanks for extending it.

@kataokatsuki
kataokatsuki deleted the fix/coalesce-terminal-kitty-placements branch August 24, 2026 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants