Skip to content

Render images#3699

Open
rumpl wants to merge 3 commits into
docker:mainfrom
rumpl:feat/tui-tool-images
Open

Render images#3699
rumpl wants to merge 3 commits into
docker:mainfrom
rumpl:feat/tui-tool-images

Conversation

@rumpl

@rumpl rumpl commented Jul 16, 2026

Copy link
Copy Markdown
Member

Can be disabled in the settings

In a terminal that knows about the kitty image protocol

Screenshot 2026-07-16 at 23 56 08

And in a terminal that doesn't know that terminals can render images

Screenshot 2026-07-17 at 00 03 41

@rumpl
rumpl requested a review from a team as a code owner July 16, 2026 22:04
@rumpl
rumpl requested a review from docker-agent July 16, 2026 22:13

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread pkg/tui/image/writer.go Outdated
Comment thread pkg/tui/image/image.go Outdated
}
cols, rows := cellSize(img, width)
id := imageID(img.PNGData)
inlineRegistry.Store(id, img)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@rumpl
rumpl force-pushed the feat/tui-tool-images branch from 9901eb5 to dde193f Compare July 16, 2026 22:18

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 No issues found — LGTM! View logs.

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread pkg/tui/image/writer.go Outdated
b.WriteString("\x1b7")
if w.dirty {
b.WriteString("\x1b_Ga=d,d=a,q=2\x1b\\")
w.dirty = false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread pkg/tui/image/image.go
rumpl added 3 commits July 17, 2026 00:23
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>
@rumpl
rumpl force-pushed the feat/tui-tool-images branch from dde193f to 76d810d Compare July 16, 2026 22:23
@aheritier aheritier added area/tui For features/issues/fixes related to the TUI area/docs Documentation changes area/config For configuration parsing, YAML, environment variables area/cli CLI commands, flags, output formatting kind/feat PR adds a new feature (maps to feat:). Use on PRs only. labels Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli CLI commands, flags, output formatting area/config For configuration parsing, YAML, environment variables area/docs Documentation changes area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants