feat(tui): add compact image paste placeholders - #418
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed the current head 9c7f9ba. The compact placeholders and atomic deletion address a useful TUI problem, but attachment ownership still breaks across submission paths. Please address the two inline findings before merging.
Validation: the 11 image-paste/editor-layer tests pass locally, and all three required CI jobs are green. Additional minimal reproductions using Pi 0.85.1's actual InteractiveMode.flushCompactionQueue and ExtensionRunner.emitInput methods reproduce both findings. These are programmatic lifecycle reproductions, not visual TUI acceptance or live provider calls.
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed at head bc2739f.
Standards
[P1] Image ownership is inferred only from the OS temp directory, a pi-clipboard filename pattern, and statSync(). statSync follows symlinks, and the later read/delete has a check-to-use race. The extension can therefore claim and remove a matching file without runtime-proven ownership. Please use a Pi-provided attachment handle, or copy into an OpenPI-owned directory and validate identity with no-follow, bounded I/O.
[P1] The 481-line editor layer depends on several private lifecycle details at once: onSubmit wrapping, raw Alt+Enter handling, setText cleanup timing, input ordering, and compaction retry behavior. Pi 0.85.1 still exposes no stable attachment/submission seam, so upgrades can silently break ownership. This should be reduced around an explicit Pi-native attachment/submission boundary rather than duplicating InteractiveMode lifecycle assumptions.
[P2] The input handler performs unbounded synchronous readFileSync plus base64 conversion. A large clipboard file can block the TUI and amplify memory use. Add a size limit and bounded asynchronous reading after identity validation.
Spec
[P1] Successful compaction with multiple queued submissions is still unsafe. Every submission enters pending, but after normal compaction only the first queued message goes through prompt/input; later messages use steer/followUp and bypass the input transform. Their images are neither sent nor promptly cleaned, and a later identical text can consume an old FIFO submission. The existing test covers only one willRetry=true submission, not successful compaction with multiple queued submissions or repeated identical text.
The Alt+Enter cleanup race from the previous review is fixed, but attachment ownership across compaction remains incomplete. Please add the missing lifecycle evidence and resolve the current main conflict before requesting re-review.
…ge-paste # Conflicts: # README.md
Submission expands placeholders into real clipboard paths so the model receives byte-identical input to unmodified Pi and the read tool can still open the file. That moved issue openpi-dev#413's long temp paths out of the editor and into the transcript, so the noise the issue reported was relocated rather than removed. Collapse those paths back into compact placeholders at render time through registerMarkdownTransformer, the same seam Pi's built-in mermaid renderer uses. The transform is stateless and scoped to user messages, so replays, forks and reloaded sessions render identically without carrying a mapping through submission, and paths the assistant quotes are left untouched. Match the directory portion as discrete segments that exclude ':' and are lazy rather than greedy. Pasting images back to back leaves adjacent paths with no separator, and a greedy middle absorbed the next path's 'C:' or '/tmp' and rendered several images as a single placeholder.
|
Thanks for the review. I've reworked the approach rather than patching the individual findings, because the structural concern turned out to be the root of the others. What changed The previous version converted placeholders into base64 This version follows Pi's own long-paste marker pattern instead: the buffer shows a compact marker, submission expands it back to the real content. Concretely, the extension no longer reads files, no longer produces base64, and no longer deletes anything. The model receives the real clipboard path, byte-identical to unmodified Pi, and the
How that maps to your findings
The main conflict is also resolved. One thing worth flagging Expanding at submission solved the model side but relocated the problem the issue actually reported: the long temp path moved out of the editor and into the transcript. I've added a rendering-only counterpart via While testing that I found and fixed a real bug: pasting images back to back produces adjacent paths with no separator, and a greedy directory match absorbed the following path's Known limitation Transcript numbering follows the order paths appear in the message, so it can differ from the editor if images were deleted mid-draft. Making it strictly identical would require carrying a mapping through message metadata, which is outside the seams available to an extension. Since transcript numbering only distinguishes images within a single message, I've documented it rather than worked around it. Longer term, folding image paths into Pi's native paste marker registry seems like the better home for this. Happy to open an upstream issue if you agree. 24 targeted tests pass; type check, lint and format are clean. |
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed exact head 52abcea.
The redesign is substantially simpler and fixes the earlier ownership/TOCTOU, unbounded read, private submission-state, and compaction-loss problems. Two P1 acceptance gaps remain:
- [P1] #413 requires submitting the attachment as multimodal ImageContent and cleaning up temporary resources. This implementation expands the token to ordinary filesystem-path text, and cleanup() explicitly preserves the file. The model receives no image payload unless it independently reads the path. Either the accepted spec must change explicitly or this needs a Pi-native attachment seam and lifecycle.
- [P1] Pi queues the result of getExpandedText() for Alt+Enter follow-ups. The pending display therefore shows the full temp path, and Alt+Up restores that path through setText() without rebuilding the attachment mapping. The compact multi-input UX still breaks on the native queued-message lifecycle; current tests do not cover pending display, dequeue, or history recall.
[P2] collapseClipboardPaths() also rewrites any matching basename in any directory, without provenance or Markdown awareness, so an unrelated path can be hidden and a Markdown image target can be corrupted.
Focused tests and hosted CI are green, but the two P1s block merge.
…ll adoption - collapseClipboardPaths: only collapse paths in known temp directories, skip fenced/inline code and Markdown link/image targets - setText: adopt expanded clipboard paths on dequeue, rebuild mapping - onChange: adopt expanded paths on history recall (setTextInternal bypass) - do not override addToHistory; history stores real paths, recall adopts - add 9 regression tests for provenance, Markdown awareness, dequeue, multi-image dequeue, and history recall via onChange Addresses review feedback on openpi-dev#418 (P1 queued-message lifecycle, P2 collapsing provenance). Scope narrowed to openpi-dev#527 (display-only).
|
Thanks for the thorough review. Addressing each point against the new head: [P1] ImageContent and file cleanup — scope change After investigating Making all providers receive I've opened #527 with acceptance criteria scoped to TUI display and editing only — the model receives byte-identical content to an unmodified Pi install. The PR description now targets #527 and the rationale is documented there. #413 has been commented with the split explanation. [P1] Queued messages, dequeue, and history recall — fixed Three changes in this push:
Pending display ( New tests cover: dequeue restoring placeholders and expanding again, multi-image dequeue renumbering, and history recall via the [P2] collapseClipboardPaths provenance and Markdown awareness — fixed Three guards added:
New tests cover: non-temp directory paths preserved, Markdown image targets preserved, fenced and inline code preserved, and prose still collapses when the same message contains a code block. |
Problem
Closes #527. Relates to #413.
Pasting clipboard images in the TUI editor inserts a long temporary file path that occupies most of the line width, makes the prompt unreadable when multiple images are attached, and requires dozens of backspace presses to remove.
Value
[Image #N]tokens while editing.readtool can still open the file.Approach
The extension mirrors Pi's native long-paste marker pattern: the editor buffer shows a compact token, submission expands it back to the real content.
Editing and submission
[Image #N]and tracks the mapping in a draft registry.getExpandedText()is overridden to expand placeholders back to real paths. Pi reads the submitted text through this seam before choosing a delivery path —handleFollowUp()calls it ahead ofprompt(),queueCompactionMessage(), andonSubmit— so all three Alt+Enter branches expand correctly.Transcript rendering
registerMarkdownTransformercollapses clipboard paths back to[Image #N]at render time, the same public seam Pi's built-in mermaid renderer uses.History and queued messages
getExpandedText()results for Alt+Enter follow-ups and stores them in editor history. When these expanded paths reenter the editor throughsetTextorsetTextInternal→onChange, the extension detects them, rebuilds the placeholder mapping, and collapses the display. The next submission expands again correctly.Known limitation
Pending message display (
Follow-up: ...in the queue area) is rendered by Pi'supdatePendingMessagesDisplayusing the queued text directly. The extension cannot intercept that rendering, so the queue area may briefly show the expanded path. This is a cosmetic limitation of the available extension seams and does not affect what the model receives.Validation
getExpandedText()purity, transcript collapsing with provenance and Markdown awareness, dequeue restoration, history recall, and adjacent path separation on both Windows and POSIX.[Image #1][Image #2], transcript shows placeholders, model receives full paths,readtool opens the file, Up-arrow recall shows placeholders and resubmission expands correctly.Impact
Display-only change to TUI interactive image pasting. Does not alter the content sent to the model, does not delete temporary files, and does not affect non-TUI surfaces.