Skip to content

Render images#3699

Merged
dgageot merged 3 commits into
docker:mainfrom
rumpl:feat/tui-tool-images
Jul 17, 2026
Merged

Render images#3699
dgageot merged 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
Comment thread pkg/tui/image/image.go
@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
@dgageot
dgageot merged commit 643a431 into docker:main Jul 17, 2026
14 of 18 checks passed
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.

4 participants