Render images#3699
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR introduces kitty graphics protocol image rendering — a well-structured feature with clear separation between image decoding, marker encoding, and frame-level overlay compositing.
One correctness issue was found in the new Writer type: state is mutated before the overlay write is confirmed, which can leave the writer in a broken state after a (rare) terminal write failure. There is also a minor unbounded memory growth concern in the image registry.
| } | ||
| cols, rows := cellSize(img, width) | ||
| id := imageID(img.PNGData) | ||
| inlineRegistry.Store(id, img) |
There was a problem hiding this comment.
[low] Global inlineRegistry grows without bound — PNG data is never evicted
RenderMarkers stores each image's Inline struct (including its full PNGData byte slice) in a package-level sync.Map keyed by FNV-32a hash. The registry is never cleared or bounded.
extractMarkerOverlays in writer.go looks up PNG data from the registry when resolving marker sequences, so the entries must live at least as long as the current frame. However, there is no eviction path once an image is no longer visible — every distinct tool-result image from the session accumulates indefinitely.
For typical sessions with a small number of images this is benign, but a long-running agent session that returns many distinct images (e.g. repeated screenshot tool calls) will hold all their decompressed PNG bytes in memory until the process exits.
Consider evicting entries whose IDs are no longer referenced by any active overlay, or clearing the registry when Invalidate() is called (the terminal has been reset, so cached data is stale anyway).
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
There was a problem hiding this comment.
Valid. Replaced the unbounded sync.Map with a mutex-protected LRU capped at 128 entries, with a regression test verifying eviction. I did not clear it on Invalidate because cached transcript markers still need to resolve after a terminal reset.
9901eb5 to
dde193f
Compare
docker-agent
left a comment
There was a problem hiding this comment.
🟢 No issues found — LGTM! View logs.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR adds Kitty image protocol rendering to the TUI. The feature logic is well-structured: the marker-based overlay pipeline, capability probe, settings integration, and dual-mode fallback (Kitty vs. plain) all look correct. Two medium/low findings in the new pkg/tui/image/ package are worth addressing before the feature ships widely.
Signed-off-by: Djordje Lukic <[email protected]>
Signed-off-by: Djordje Lukic <[email protected]>
Signed-off-by: Djordje Lukic <[email protected]>
dde193f to
76d810d
Compare
Can be disabled in the settings
In a terminal that knows about the kitty image protocol
And in a terminal that doesn't know that terminals can render images