fix(graphics): isolate oversized kitty images - #3035
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughKitty graphics encoding now quarantines oversized image sources within a bounded cache. Budgeted terminal updates continue with smaller images, deduplicate data requests, preserve quarantine state during pane cleanup, and retain legacy behavior without a transaction budget. The changelog also records a Windows pane fix. ChangesKitty graphics rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The change prevents oversized Kitty images from starving later images, but pane-source cleanup can still combine with a budget-sized terminal image transaction and exceed the graphics budget, potentially dropping an image batch. The PR is mergeable with explicit owner awareness or follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kitty_graphics.rs (1)
227-255: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep pane-layer cleanup inside the transaction budget.
Line 227 can generate delete-image commands. Lines 251-253 prepend those commands to a terminal image update that can already consume
HEADLESS_GRAPHICS_TRANSACTION_BUDGET.image_transaction_fitsdoes not include the cleanup bytes.Process pane-source deletes through the incremental budgeted path. Return
incompletebefore an upload when cleanup consumes the transaction. Add a regression test with cached pane sources and a terminal image at the budget boundary.Proposed direction
- let mut bytes = cache.clear_pane_sources(); - let mut encoded = - encode_terminal_graphics_update(cache, &placements, view_changed, transaction_budget); - bytes.extend(encoded.bytes); + if transaction_budget.is_some() { + cache.request_placement_replay(); + return encode_graphics_update_incremental( + cache, + &placements, + &HashSet::new(), + transaction_budget, + ); + } + + let mut bytes = cache.clear_pane_sources(); + let mut encoded = + encode_terminal_graphics_update(cache, &placements, view_changed, transaction_budget); + bytes.extend(encoded.bytes);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7283773d-844d-48cd-9c42-2b18d934b0f6
📒 Files selected for processing (2)
docs/next/CHANGELOG.mdsrc/kitty_graphics.rs
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
|
agreed, fixed in cd7c0d0. |
Greptile SummaryThe PR applies the existing graphics transaction budget to terminal-only attached clients so oversized Kitty uploads are quarantined without blocking later images.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/kitty_graphics.rs | Adds budget-aware terminal-only graphics encoding, bounded oversized-image quarantine, request deduplication, and focused regression tests. |
| docs/next/CHANGELOG.md | Records that oversized Kitty images no longer prevent later smaller images from rendering. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Collect visible Kitty placements] --> B{Image already uploaded?}
B -->|Yes| P[Emit or replay placement]
B -->|No| C{Signature quarantined?}
C -->|Yes| D[Skip oversized image data]
C -->|No| E{Upload fits transaction budget?}
E -->|Yes| F[Upload image]
F --> P
E -->|No| G[Quarantine source and signature]
G --> H[Continue processing later bounded images]
Reviews (2): Last reviewed commit: "fix(graphics): budget pane image cleanup" | Re-trigger Greptile
cd7c0d0 to
e7815b7
Compare
kangal-bot
removed
the
ai-review
3 participants
Issue
Showing a large Kitty image leaves blank space. Smaller images shown later in the same pane also stay blank.
Problem
The decoded large image produced an upload larger than the client graphics frame limit. Terminal-only rendering bundled that upload with later images, dropped the whole frame, and retried it on every render.
How did we fix it?
Herdr now applies the existing per-image transaction budget to terminal-only attached clients. It quarantines an oversized upload and continues with later bounded images. The quarantine is bounded and avoids copying unchanged pixels again. Local rendering keeps its existing uncapped behavior.
Verification
Before the change, the large image produced a 41,267,682-byte dropped payload and the later small image produced a 41,379,271-byte dropped payload. Afterward, the same pane transmitted and placed the 158×132 image without retrying the 3456×2234 upload. Kitty graphics tests and render scaling passed.
just checkpassed 3,490 of 3,491 tests; an unrelated live-handoff PID-discovery test still fails although its log shows the replacement server started.refs #3033