feat: display Pi image artifacts inline - #36
Conversation
There was a problem hiding this comment.
ℹ️ No critical issues — two minor suggestions and one robustness note inline.
Reviewed changes
- Artifact protocol +
display_imagetool — adds a versionedChatArtifactV1/ChatArtifactEventV1contract and a chat-scopedPiAgentRuntimeExtensionthat exposesdisplay_imageonly to attended foreground workspace chats. - Durable staging + crash recovery —
DisplayImageArtifactStorestages normalized bytes before announcement, recovers interrupted responses into image-only interrupted assistant messages at startup, and blocks sends/copies/exports while recovery is pending. - Inline rendering —
MessageAttachmentsrenders bounded PNG/JPEG/static GIF/WebP/BMP inline with a preview dialog and file-card fallback; live artifacts dedupe against persisted history. - Safety validation — workspace-relative path pinning, descendant-symlink rejection, raster structure/animation/dimension checks, quota enforcement, and versioned IPC parsed before React state.
- Continuity & onboarding — empty assistant content is rewritten to an image-count placeholder for Pi history continuity; onboarding bento copy advertises the feature.
The hand-rolled GIF/PNG/WebP/JPEG/BMP header parsers and the symlink/identity checks in readPickedAttachments (O_NOFOLLOW + lstat walk) hold up under review; the pre-decode dimension checks are defense-in-depth over Chromium's actual decode with a renderer-side card fallback.
ℹ️ Startup image recovery has no failure path
displayImageArtifactStore.initialize() and recover() run inside app.whenReady() with no try/catch, so any throw — a corrupt/unsafe staging file, or recover()'s "would exceed chat limits" guard — aborts the ready handler and Aiden never finishes launching. initialize()'s corrupt-file throw matches the existing fail-fast pattern (piRuntimeEffectStore), but the recover() limit guard is a new, data-state failure mode whose only recovery is hand-editing display-image-artifacts.json.
Technical details
# Startup image recovery has no failure path
## Affected sites
- main/index.ts:1475-1507 — `initialize()` and `recover()` called with no guard; a throw propagates out of `whenReady`.
- main/services/display-image-artifact-store.ts:1003-1064 — `recover()` throws `Recovered image artifacts would exceed chat ... limits.` when persisted + pending usage exceeds per-chat caps.
## Required outcome
- A failed recovery/initialization must not prevent the app from launching. Log and degrade (leave the stage pending and keep the composer blocked for that chat), rather than throwing out of the ready handler.
- Optional: a test asserting startup tolerates a throwing `recover`.
## Open questions for the human
- Is the fail-fast-on-corrupt-store behavior intentional here, or should only the corrupt/unsafe case be fatal while the limit-exceeded case degrades?DeepSeek Pro | 𝕏
|
|
||
| async pending(): Promise<readonly StagedDisplayImageArtifact[]> { | ||
| this.requireInitialized(); | ||
| return structuredClone((await this.data.load()).records); |
There was a problem hiding this comment.
pending() deep-clones every staged artifact — including the base64 image payloads — on each call, and both hasPending() and usageByChat() route through it. hasPending() runs on every chats:get, append, copy, export, and delete, so while a recovery is pending (stages still present) each chat read clones up to megabytes of image bytes just to learn count > 0.
Technical details
# Avoid deep-cloning staged payloads in usage queries
## Affected sites
- main/services/display-image-artifact-store.ts:304 — `structuredClone((await this.data.load()).records)` clones `data` for every caller.
- main/services/display-image-artifact-store.ts:311-320 — `usageByChat` / `hasPending` call `pending()`.
## Required outcome
- A non-cloning usage projection that sums `size`, `count`, and `pixels` from the cached records (no base64 copy), used by `usageByChat`/`hasPending`.
Summary
Safety
Validation
Review