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.
| b.WriteString("\x1b7") | ||
| if w.dirty { | ||
| b.WriteString("\x1b_Ga=d,d=a,q=2\x1b\\") | ||
| w.dirty = false |
There was a problem hiding this comment.
[medium] Writer.Write() mutates w.dirty and w.uploaded before the overlay write succeeds
w.dirty is set to false (line 136) and w.uploaded[image.id] is set to true (line 142) while building the escape-sequence buffer — before io.WriteString(w.out, b.String()) is called at line 152. If that final write fails (e.g., EPIPE, a short write to a slow terminal), the Writer believes the images are already uploaded and the dirty flag is clear. On the next Write() call it will skip both the Ga=d delete-all command and the Ga=t re-transmission for those images, leaving the terminal with blank/missing image slots. There is no automatic rollback: Invalidate() is the only recovery path, and nothing in the frame-write cycle calls it on a write error.
To fix, move the w.dirty = false assignment and the w.uploaded[image.id] = true assignment to after io.WriteString returns successfully. This way, if the overlay write fails, the next frame will retry the full transmission.
| Confidence | Score |
|---|---|
| 🟡 moderate | 75/100 |
There was a problem hiding this comment.
Valid. Updated the writer to treat overlay emission transactionally: dirty/uploaded/active state is now committed only after the complete overlay buffer is written successfully, and short writes are converted to io.ErrShortWrite. A failed write leaves the prior state intact for retry.
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
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