Repository navigation
feat(chat): show a tool's pictures as small thumbnails on its work line - #1952
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Image extraction and display rules src/lib/workImages.ts, src/lib/workImages.test.ts |
New helpers extract ordered tool images, apply display limits, count hidden images, and position previews. Tests cover ordering, limits, and preview placement. |
Thumbnail display and interactions src/components/ChatView.tsx, src/components/ChatView.css, src/lib/i18n/chat.*.ts, scripts/claude-image-browser-qa.ts, scripts/pi-image-browser-qa.ts, DESIGN.md, changelog.d/chat-work-line-images.md |
Chat shows thumbnails on folded work and row lines. Hover and focus show previews; selecting a thumbnail or overflow tile opens the viewer. Styling, translations, documentation, and browser checks cover the behavior. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature · Severity of issue fixed: Medium
Possibly related PRs
- devswha/ovis#1935: Adds Claude tool-image references and retrieval consumed by this change.
- devswha/ovis#1945: Adds pi prompt and tool-result image references consumed by this change.
Suggested reviewers: devswha
Merge Risk: 
ee5ea
-
No confirmed issue blocks the Chat thumbnail change. The remaining Claude browser check can be run as normal validation.
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check PR #1951requires pictures on each opened row's line while retaining the opened-row gallery.WorkRowrendersLineImagesonly under!open, so the thumbnails disappear when the row opens. PR#1951…Render the row's LineImageswhile the row is open and keepToolImagesas the larger gallery. Add client-side per-reference caching or an equivalent mechanism sono-storerefs do not fetch again when the thumbnail, preview, and viewer …-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: displaying tool-returned pictures as small thumbnails on work lines. Out of Scope Changes check The changes stay within PR #1951. The Chat components and CSS implement the requested line thumbnails and previews. The pure helpers, unit tests, browser QA updates, translations, design entry, and ch…Docstring Coverage Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 8 files. (3 skipped: 3 … Description check The description includes the required Change and Validation sections. It explains the problem, resulting behavior, linked issue, scope, edge cases, tests, manual checks, and known environment-specific… Full details: Out of Scope Changes check
-
Explanation
The changes stay within PR
#1951. The Chat components and CSS implement the requested line thumbnails and previews. The pure helpers, unit tests, browser QA updates, translations, design entry, and changelog support the same UI behavior. No unrelated server, endpoint, wire, or non-Chat feature changes are shown.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 8 files. (3 skipped: 3 unsupported.)
Full details: Description check
-
Explanation
The description includes the required Change and Validation sections. It explains the problem, resulting behavior, linked issue, scope, edge cases, tests, manual checks, and known environment-specific failures.
-
Full details: Linked Issues check
-
Explanation
PR
#1951requires pictures on each opened row's line while retaining the opened-row gallery.WorkRowrendersLineImagesonly under!open, so the thumbnails disappear when the row opens. PR#1951also requires each picture to fetch once per browser. The changed code has no client-side fetch cache, and the PR description states that Codex and OpenCode refs areno-store.Resolution
Render the row's
LineImageswhile the row is open and keepToolImagesas the larger gallery. Add client-side per-reference caching or an equivalent mechanism sono-storerefs do not fetch again when the thumbnail, preview, and viewer use the same picture.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Owner
|
Thanks for this, @mattmarcin. The maintainer is happy to take tool-picture thumbnails on folded work rows, on one condition:
With that change, the merge pass will review the PR and merge it. Until then it stays open. |
Pictures a tool returned (a Read of a screenshot, a browser screenshot) were drawn only inside an opened work row, and a settled turn's block starts folded, so what the agent saw was two clicks away. Since devswha#1935 the conversation carries image refs only and Claude refs are served immutable, so the load-on-open guard from devswha#262 no longer has to hold for a few small thumbnails. Following the Claude app's folded "Read 2 files" line: - a folded block's line ends in small thumbnails of every picture its tools returned; opened, each row's line carries its own, and an opened row keeps its larger gallery - hover or keyboard focus shows a larger preview with the tool's id and "n of m", placed above, below or on the roomier side inside the viewport, and following the thumbnail as the chat scrolls; a touch opens the viewer at once - a click opens the existing image viewer on all of the line's pictures at that one - the latest four show (three on a coarse pointer, where each is a --touch-target square), the rest are one "+N" tile, so the line never wraps - a picture that fails to load leaves the line, its count and the viewer Client only: no endpoint, wire or server change. The pi and Claude image browser QA now expect the thumbnails on the folded line instead of nothing before the row opens. Refs devswha#1951, devswha#992
A folded work line fetched every picture its tools returned, including Codex and OpenCode ones the server sends no-store, so each redraw of the line fetched them again. Per the maintainer's condition on devswha#1952, a line now carries only refs the browser keeps (Claude's and pi's, served immutable); any other agent's pictures load only when their row is opened, as on main. The allowlist mirrors the cache rule in server/pane-routes.ts, so an agent added later stays on load-on-open until its refs are known to be cacheable.
mattmarcin
force-pushed
the
feat/chat-work-image-thumbs
branch
from
October 26, 2026 04:10
ee5ea7d to
ca0bea0
Compare
Contributor
Author
|
Thanks. Done in ca0bea0, with the branch rebased onto main:
The PR description, |
Owner
|
Merge pass review report. Overall: PASSED. This is an outside contribution that changes client behaviour. The maintainer agreed to take it on one condition, which it meets. Review: an Astra gate (omo-native-gate-reviewer, gpt-6-astra) approved ca0bea0. The condition holds.
No regressions:
Resolved during review: Astra first flagged the decoding wait in
|
Merged
devswha
added a commit
that referenced
this pull request
Oct 26, 2026
## Release 0.4.18 The release PR, made by the steps in [docs/development.md#releasing](docs/development.md#releasing): - **Version:** `version` is 0.4.18 in `package.json` and `herdr-plugin.toml`. - **Changelog:** `bun scripts/changelog-fold.ts 0.4.18 2026-10-26` moved the 6 `changelog.d` fragments into CHANGELOG.md, with PR links, and deleted them. The outside contributors are credited: #1706 @jpcarranza94, #1952 @mattmarcin, #1956 @YuuuXie. - **Release summaries:** `release-summaries.json` has the 0.4.18 entry in en, ko, ja and zh, with the same lists in each: 1 new, 1 improved, 3 fixed. Every line is under the limit. - **Remote bundle:** `REMOTE_BUNDLE_VERSION` goes from 36 to 37 in `shared/machines.ts`, because the remote bundle's inputs changed since `remote-v36`. Once this merges, `remote-v37` is pushed on the merge commit before the Release run. `bun scripts/release-guards.ts` reports: "Release 0.4.18: remote bundle, telemetry Worker and release metadata are in order." No telemetry Worker deploy is needed. `bun run check fast` passes locally. The maintainer approved this release. CI runs on GitHub-hosted runners (`CI_SELF_HOSTED_LABELS=ubuntu-24.04`). Co-authored-by: devswha <devswha@users.noreply.github.com>
devswha
added a commit
that referenced
this pull request
Oct 26, 2026
… a Codex tool returns (#1998) Part of #992, the omp and Codex part. It follows #1935 (Claude Code) and #1945 (pi), with the same picture check and the same endpoint rules. ## Change In an omp pane, Chat dropped every picture: a screenshot sent with a prompt, a prompt that was only a picture, and the picture a `read` returned. omp moves a picture's bytes out of the transcript into its blob store and writes `blob:sha256:<digest>` in their place, so there was nothing to decode. In a Codex pane, a picture a tool returned (`view_image`, an image generation, a screenshot a command read) was dropped too: only prompt pictures were read. What it does now: - **omp prompt pictures** are named as pi's are, `pi-prompt:<entry id>:<place>`, and a prompt that is only a picture is a turn of its own and opens one for paging (`turnStarts`). - **omp tool pictures** are named `pi:<call id>:<place>` on the tool's row. omp's call ids hold a `|` (`call_…|fc_…`), which the ref shape now takes. - **omp's blob store:** omp 18.8.7 keeps a session store at `<data>/sessions` and its blobs at `<data>/blobs` (`getBlobsDir`, the same `data` base), so `ompBlobStore` names the store beside the transcript's real session store (the default agent directory, a profile's, `PI_CODING_AGENT_DIR`'s or the XDG one), by layout alone, whether omp has made it yet or not. A blob is read only by omp's own 64-hex digest and only when its bytes hash to that digest. The image endpoint takes the store's identity from the objects that authorized the pane's transcript (`ompAuthorizedTranscript`): the ownership check returns the store's real root and the dev:ino of the transcript file it opened, and the agent data directory is then opened once, without following a link and held to the path named, and `sessions`, the path down to that same transcript file (each component without following a link) and `blobs` are all reached from that one held descriptor. It reads a blob only from that very directory: the store is opened as a directory without following a link, its descriptor must have that identity (`fstat` dev:ino) and be the directory the path names (`/proc/self/fd`), and the blob is opened through that descriptor (`O_NOFOLLOW`). So no directory or link swapped in afterwards, at the store or above it, can stand in for it. A store that is missing, a link, or not a directory answers nothing. **Platform limit:** Node has no descriptor-relative open on macOS or Windows (checked on macOS), and a blob opened by name there follows whatever a directory above it is swapped to for that moment, so there no omp blob is read: blob-backed omp pictures are not shown on those PCs, while omp's inline pictures and every Codex picture are (follow-up #2004). A session in a `--session-dir` of its own names no data directory: its blob pictures stay unnamed, and inline ones still show. - **Codex tool pictures:** the `input_image` data URLs in a `custom_tool_call_output` or `function_call_output` are named `codex-tool:<call id>:<place>` on the tool's row, for any tool. The `ImageView` and image-generation events that repeat them are not read, so each picture shows once. They are served `no-store` like Codex's other refs, so they load only when their row is opened (as #1952's work line requires). - **One check, both sides:** a blob or a data URL is a picture only as PNG, JPEG, GIF or WebP by declared type and leading bytes, at most 8 MiB, and the parser and the endpoint ask the same question (`inlineImageType`, `ompBlobPicture`), so the page never names a picture the endpoint refuses. - **Same history as the page:** the endpoint reads an omp picture after the last clear (also one JSON writes with escapes), a tool's picture only from the first result answering a call the page draws, a prompt's only from a prompt it draws; a Codex tool's picture only from the first output answering a call the page draws, in the rollouts the page reads (`codexHistorySegments`, without the bytes a backtrack left). Ids are matched however JSON writes them. Like Claude's, it searches a window at a time and reads a record's shape a window at a time (`recordShape`, which Claude's reader now shares), holding only the served picture whole; a picture only counted on the way to the served one is checked by a streaming scan that answers exactly as the whole-string check. The omp and Codex readers run in steps a 1 MiB window apart, and every 32 records, and give the event loop its turn between them; the scan for clears runs in steps too, on its own copy of what it keeps. So a lookup never holds the bridge for more than a window's work (Claude's reader and page reads run the same steps straight through, unchanged). **HTTP contract:** `/api/pane/conversation/image` accepts one more ref shape (`codex-tool:`) and answers omp's `pi:`/`pi-prompt:` refs. No new endpoint, no new wire field (`tool.images` and image parts already exist). **Remote bundle:** the server files are in the remote PC bundle. `REMOTE_BUNDLE_VERSION` is not raised here; the release does that (0.4.18 raised it to 37 and its Release run has finished). **Design:** no new UI: the prompt thumbnails, the work line, the row gallery and the viewer that Claude and pi pictures already use. **Not in this PR:** omo and gjc (no recorded sessions with pictures here; they name none, as before), omp's `xd://generate_image` pictures that live only in `details`, and local Markdown images in answers. ## Validation - **Recorded fixtures** from real CLIs on this PC, picture bytes kept as recorded: - `server/fixtures/omp-pictures-18.8.7-session.jsonl` and its blob `omp-pictures-18.8.7-blob.webp`: omp 18.8.7 (`omp -p --model openai-codex/gpt-6-luna:medium`) with `@shot.png` and a question, a `read` of shot.png (call id `call_…|fc_…`), and `@shot.png` alone. omp turned the PNG into a WebP and moved it to its blob store. - `server/fixtures/codex-pictures-0.163.0-rollout.jsonl`: codex-cli 0.163.0, `codex exec -i shot.png` then `codex exec resume` calling `view_image` through the exec tool. Only the records the chat reads are kept (no account ids, instructions, environment or encrypted reasoning). - **Unit tests** (`server/omp-images.test.ts`, new; `server/codex-images.test.ts`), each shown failing with its fix undone (21 mutations, every one fails its test): - the recorded sessions name their pictures by ref only (no base64, no blob ref in the page) and the endpoint serves their exact bytes; - refused, at the parser and the endpoint alike: an svg, non-raster bytes under a raster type, a picture past 8 MiB (exactly 8 MiB passes), a remote URL, a blob ref that is no digest omp writes, a missing blob, bytes that do not hash to their name, a blob that is a link, malformed refs; - a blob that goes or changes while the bridge runs keeps its place: its ref is a 404 and the next picture keeps its own ref; - a tool's picture only from the result or output the page draws: an orphan, a second answer, a call made again after an answer with no picture, a `function_call` output as well as a custom tool's; - a picture from before a clear (also an escaped `context_clear`) is out of the chat and out of reach; - call ids and entry ids written with JSON escapes (omp's `|` among them) are named and served as the parser reads them; - a picture-only omp prompt is its own turn and opens a page there (`turnStarts`); a growing session names the same refs as a cold read; - memory: a call and a result each padded with 20 MiB, two 3 MiB blobs, and a 2 MiB escaped inline picture before the served one: no allocation past a 1 MiB window except the served picture; - the streaming checks answer exactly as `inlineImageType` and the data URL check do, for every two-piece and many three-piece cuts, including a cut inside the padding at exactly 8 MiB. - `server/pi.test.ts`: the omp case of the picture-only `turnStarts` test now expects a turn (omp draws its prompt pictures now). - **HTTP contract** (`server/omp.contract.test.ts`, `server/codex.contract.test.ts`, on an isolated herdr via `bun run check run`): the recorded omp session's three refs answer `image/webp` with the blob's exact bytes and `private, max-age=86400, immutable`; the Codex `codex-tool:` ref answers `image/png` with the viewed PNG's bytes and `no-store`, and 404 once the rollout no longer holds it; unnamed and malformed refs are 404. 8/8 pass. - **Real transcripts on this PC** (shapes only, nothing printed): 36 omp sessions with 652 blob pictures name 650 refs (1 prompt, 649 tool) and the endpoint serves all 650; 7 Codex rollouts name 23 tool-picture refs and the endpoint serves all 23. No page carries base64 or a blob ref. - **Browser** (`scripts/omp-codex-image-browser-qa.ts`, real Chrome, added to the CI browser lane), served by the endpoint's own readers from the recorded files: omp's three prompt pictures decode from the blob store (200x200 WebP), the picture-only prompt is its own turn with no empty bubble, the `read` picture shows on its folded work line, is fetched by its `|` call id and opens the viewer; a Codex tool's picture stays off the folded line with no element requesting it, and decodes (32x32) once its row is opened, beside the prompt's own attachment; at 390 px nothing spills past the screen. Desktop and phone screenshots checked. - **Merge pass gate** (Astra), two rounds, every blocker fixed and reviewed again (see the gate follow-up comments): - On ae860b0: C1, a symlinked blobs directory served another store's picture; C5, the readers blocked the event loop (225 ms on a warm 64 MiB lookup). - On 5c1bc56: B1, an agent directory swapped for a link after the ownership check still moved the read (the store was found again from `realpath`); B2, the macOS/Windows by-name open is racy on an intermediate path; C5, 30,000 short records still held the loop 211 ms, and the scan for clears was synchronous. - On 64c8527: C1a, a directory swapped between the ownership check and the anchor lent another store (the anchor walked the path again); then, raised by Codex, the store's sessions reparented under another agent directory. Fixed by the one-held-descriptor anchor above; tests swap the agent directory, move another real sessions in, and reparent, through a hook right after the data directory's open. On a947a19, round 4: moves before the data directory's open adopt another data directory. Decided trust boundary (a): write access to the user's own omp data directory; the directory at that path when the anchor opens it is trusted, and the transcript and blob store always come from that one held descriptor (tests before and after the open, 1ef6bf8). - Fixes, as described above: the descriptor-relative open held to the identity taken at authorization; no blob read off Linux; yields every 32 records and per clear-scan window. Tests: through a test seam that acts at each open, a link or another real store swapped in for the store, or for the agent directory above it, at the store's or the blob's open; the agent directory swapped after authorization; the non-Linux branch forced; the longest stretch between loop turns bounded in buffers taken (no clocks) over 30,000 records and 40 MiB, cold. Each defence is shown failing its test when removed. - Measured: 30,000 short records over 30.5 MB, longest event-loop stall 3.9 ms warm (omp), 33.5 ms cold (one 16 MiB window of the clear scan), Codex 2.1 ms; a warm 64 MiB lookup 2.1 ms. - On the way, the code reviewer found the store named null before omp made it, so a live page missed pictures a cold read names (fixed: named by layout), and Codex found a link, then another real directory, swapped in at the open, and the anchor taken only after the async resolve (all fixed as above). Both approve the final commits. One gap left as a known one: the scan for clears working on its own copy has no test of its own (without it, a page read in the middle of an image read redoes the scan). - **Reviews:** - `omo-native-code-reviewer`: APPROVE on 4a9a574 (then 7005452d, before the rebase), with one MEDIUM (a blob deleted while the bridge runs could move the next picture into its ref) and four LOWs (refusals re-read every poll, the blob cache surviving a reset, weak spots in the browser QA, earlier inline pictures read whole to count them). - Codex (Sol 6.1): REQUEST CHANGES on 4a9a574 (then 7005452d, before the rebase): the same MEDIUM, a MEDIUM that a picture only counted on the way to the served one was read whole (a blob hashed whole to name it, a data URL rebuilt to count it), and a LOW that the browser QA could pass on an undecoded picture. - All fixed in ae860b0 (fcc0187a before the rebase) with tests: the place model, a per-file blob cache that keeps refusals, streaming checks, the QA's decode and error checks. Both reviewed the delta again: APPROVE, every finding FIXED, no new finding from Codex. The code reviewer's one new LOW is left as is: the served inline picture is read from the file twice (once checked, once held), up to about 11 MB of extra reading and no extra memory; holding it during the check would mean holding a picture before it passes, which the memory rule forbids for any picture not served. - `bun run check fast` on 1ef6bf8: workflow syntax, generated types and themes, typecheck and build pass, and 7265 of 7266 unit tests pass. The one failure is the known slot-admission flake in `scripts/check.test.ts` (`slots > gives its place up when it stops waiting`: ticket ports taken on a busy host), in a file this PR does not touch, which passes 66/0 alone. The run before it had only an untouched load-timing failure (`scripts/install-node-fallback.test.ts`, hung-host kill, 10/0 alone). Counted as passed per the lane's rule for known slot-admission flakes; GitHub CI runs the full suite. --------- Co-authored-by: mp <mp@local>
This was referenced Oct 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
Closes #1951 (a UI follow-up to #992). #1951 has no maintainer reply yet; I'm happy to change the approach or close this if the direction isn't wanted.
Pictures a tool returned (a
Readof a screenshot, a browser screenshot) were drawn only inside an opened work row, and a settled turn's work block starts folded, so what the agent saw was two clicks away.Since #1935 the conversation carries image refs only, and Claude refs are served
private, max-age=86400, immutable, so the load-on-open guard from #262 (keeping base64 out of every poll) no longer has to hold for a few small thumbnails.This follows the Claude desktop app's folded
Read 2 filesline, in three steps:no-store, so they stay off the line and load only when their row is opened, as on main. Opened, each row's line carries its own, and an opened row keeps its existing larger gallery. The thumbnails sit beside the header button, not inside it.4 of 6), above the thumbnail where it fits, else below, else on the roomier side, always inside the viewport. It follows the thumbnail when the chat scrolls under it. A touch has no hover: a tap opens the viewer.Edge cases:
--touch-targetsquare), and the rest are one+Ntile that opens the viewer at the first, so the line never wraps.loading="lazy"and only rows the virtualized timeline mounts draw them.Client only: no endpoint, wire or server change. The pure parts (which pictures a line shows, where the preview goes) are in
src/lib/workImages.ts.Behaviour change to note: a folded block now fetches up to 4 Claude or pi pictures without a click. Each is fetched at full size, since there is no server-side thumbnail, and only once per browser, since those refs are served
private, max-age=86400, immutable. Which refs a line may fetch is an allowlist (cachedImageRefinsrc/lib/workImages.ts, mirroring the cache rule inserver/pane-routes.ts): Codex and OpenCode refs (no-store) and any agent not on it load only when their row is opened. A downscaled variant would be a contract change, so I left it out.The
DESIGN.mdentry is under Work block, and the changelog fragment ischangelog.d/chat-work-line-images.md.Validation
bun run typecheck,bun run build,bun run generate:types --checkandbun run generate:themes --checkpass.src/lib/workImages.test.ts(11 tests: picture order, which refs a line may fetch with one sample ref per agent, the line's shown and hidden pictures, preview placement including a viewport too short for either side) pass, as doessrc/lib/i18n.test.tswith the two new strings in ko, ja and zh.scripts/pi-image-browser-qa.ts, updated to the new behaviour, passes in Chrome. It covers: six pictures giving four thumbnails and+2with only those four fetched, the hover preview and its label, a click and the tile opening the viewer at the right picture, an opened block and row handing their pictures down, a picture whose fetch fails leaving the line, the phone line (three 44px squares, a tap opening the viewer with no preview) and a keyboard Enter then Escape.scripts/claude-image-browser-qa.tsis updated to expect the thumbnail on the folded line, but I could not run it: it needs a Unix process table and I worked on Windows. It still needs a run in the browser lane.bun run check faston the branch rebased onto main, in a Linuxoven/bun:1.4.2container: workflow syntax, generated types and themes, typecheck, build and 7,020 unit tests pass. 61 tests in 6 files (server/codex-processes,server/copilot,server/shell-agent,scripts/self-hosted-runner,scripts/ci-runner-route,scripts/install-node-fallback) fail there with ENOENT or "Executable not found in $PATH". The same 61 fail identically on unchanged main in that container, so they come from the bare image, not this PR; CI's Fast checks run them on the real runner.