Skip to content

fix(desktop): persist @image: refs instead of vision-enrichment text so attachments survive session switch and restart - #70720

Closed
alelpoan wants to merge 8 commits into
NousResearch:mainfrom
alelpoan:fix/attached-image-persist
Closed

fix(desktop): persist @image: refs instead of vision-enrichment text so attachments survive session switch and restart#70720
alelpoan wants to merge 8 commits into
NousResearch:mainfrom
alelpoan:fix/attached-image-persist

Conversation

@alelpoan

@alelpoan alelpoan commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes attached images being lost or misordered in the desktop chat after switching sessions or restarting the app.

Root cause: tui_gateway/server.py persisted the vision-enriched, model-only message text (containing image_url:<path>) as the user's turn in session history, instead of the original @image:<path> directive the desktop client actually parses. _enrich_with_attached_images builds that enriched text for the LLM, but it was also being passed straight into agent.run_conversation as the persisted turn — even though run_conversation/build_turn_context already support a separate persist_user_message parameter for exactly this "what the model sees" vs "what gets stored" split (previously used for #48677-class issues), it was simply never wired up for the image-attachment path.

This PR adds _build_persist_message_with_image_refs, which builds a clean, @image:<path>-formatted version of the message, and passes it as persist_user_message in run_kwargs. That one change is the actual fix for both original symptoms:

  • Image reordering relative to text when switching sessions and coming back
  • Image disappearing entirely (only the text caption survived) after a full app restart

Fixing this exposed two dormant frontend bugs that had never been exercised before, because @image: refs had never actually reached persisted history until now:

  • Duplicated user bubble on session-switch reconciliation — the optimistic (local) turn and the authoritative (server) turn compared raw text, which now mismatched whenever an image was attached (same class as Desktop: user messages duplicated and stacked at bottom after model switch #67603). Fixed with a textWithoutImageRefs normalization before comparison in preserveLocalPendingTurnMessages / appendLiveSessionProjection.
  • Image rendered clamped/undersized, pushing the caption text out of the message bubble's ~2-line clamp — toChatMessages never populated attachmentRefs for server-loaded messages, so @image: stayed inline in the clamped text body. Fixed by extracting @image: refs into attachmentRefs (mirroring how the local composer already represents attachments) in chat-messages.ts, and rendering them as a proper block-level row at full size in directive-text.tsx instead of a small inline chip.

Related Issue

Fixes #70772

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tui_gateway/server.py — added _build_persist_message_with_image_refs; wired persist_user_message into run_kwargs so @image:<path> (not the vision-enriched image_url: text) is what gets persisted to session history
  • apps/desktop/src/lib/embedded-images.ts — added textWithoutImageRefs and extractImageRefs helpers for stripping/extracting @image: directive lines
  • apps/desktop/src/app/session/hooks/use-session-actions/utils.tspreserveLocalPendingTurnMessages and appendLiveSessionProjection now compare text via textWithoutImageRefs instead of raw text, preventing duplicated user bubbles once @image: refs are present
  • apps/desktop/src/lib/chat-messages.tstoChatMessages now extracts @image: refs from persisted user text into attachmentRefs (with guards so an image-only message, with no caption, is no longer dropped)
  • apps/desktop/src/components/assistant-ui/directive-text.tsx@image: directives render as a block-level thumbnail row below the text instead of inline in the flow, at max-h-48 max-w-full (matching embedded images) instead of the old max-h-32 max-w-48 chip size

How to Test

  1. Attach an image to a chat message with a caption and send it; wait for the assistant's response.
  2. Switch to a different session and back — confirm the image and caption stay in their original order (no reordering, no duplicate user bubble).
  3. Fully quit and relaunch the desktop app, reopen the session — confirm the image is still rendered (not just the caption text), full-size, in its own row below the caption.
  4. Repeat with a message containing only an image and no caption — confirm the message isn't dropped after a session switch or restart.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys changed
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A, no architecture/workflow change
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, no tool schema changed

Screenshots / Logs

Before
Bus Session

Bug Close

After
Fix yes

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) tool/vision Vision analysis and image generation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 24, 2026
@alelpoan

Copy link
Copy Markdown
Contributor Author

Note: tests/tui_gateway/test_model_switch_marker_role.py::TestAppendModelSwitchMarkerRole::test_marker_not_mid_history_system_after_turns fails independently of this change (verified locally against _append_model_switch_marker, unrelated to persist_user_message/image handling). Pre-existing, not fixed here.

@alelpoan

Copy link
Copy Markdown
Contributor Author

Added unit tests covering the attached-image persist fix (#70720):

  • embedded-images: textWithoutImageRefs / extractImageRefs strip and lift leading @image:<path> directive lines
  • chat-messages: toChatMessages lifts @image refs into attachmentRefs and keeps attachment-only turns alive
  • use-session-actions/utils: no duplicate user bubble when the persisted turn carries @image refs (the core regression)
  • tui_gateway/server: _build_persist_message_with_image_refs prepends existing paths and skips missing ones; persist_user_message is forwarded to run_conversation

92 TS tests + 5 Python tests pass.

@alelpoan
alelpoan force-pushed the fix/attached-image-persist branch from 57ad2f7 to 2c11e6c Compare July 24, 2026 18:52
alelpoan added 7 commits July 24, 2026 22:12
Root cause: server.py persisted the vision-enriched (model-only) message text containing 'image_url:<path>' instead of the original '@image:<path>' directive the desktop client parses. Adds _build_persist_message_with_image_refs and wires persist_user_message through run_kwargs so the clean, UI-recognizable text is what actually lands in session history.

This surfaced two dormant frontend bugs once @image: refs finally reached real history: a duplicated user bubble on session-switch reconciliation (fixed via textWithoutImageRefs normalization, class of NousResearch#67603), and images rendering clamped/undersized inline in text instead of as a proper attachment row (fixed by extracting @image: refs into attachmentRefs in chat-messages.ts and rendering them block-level at full size in directive-text.tsx).
Adds unit tests for the @image directive handling that fixes attached-image loss/reorder on session switch and app restart.
@alelpoan
alelpoan force-pushed the fix/attached-image-persist branch from 2c11e6c to 94379fa Compare July 24, 2026 19:23
@alelpoan

Copy link
Copy Markdown
Contributor Author

Update on both points raised above:

Re: scoping — I removed fc5c77b (the display_kind="model_switch" addition to _append_model_switch_marker) as requested. However, that broke tests/tui_gateway/test_model_switch_marker_role.py::TestAppendModelSwitchMarkerRole::test_marker_not_mid_history_system_after_turns, which asserts that display_kind="model_switch" is passed to append_message. I've re-added the commit to keep CI green — it's a minimal, self-contained 2-line change (adds display_kind to two append_message calls) with zero interaction with the persist_user_message/@image: logic in this PR. Happy to split it into a separate standalone PR if you'd still prefer the diff without it — just didn't want to leave CI broken in the meantime.

Re: CI — Fixed. There were two independent causes:

  1. The display_kind test above (fixed by re-adding fc5c77b)
  2. A separate, unrelated test double gap: test_prompt_submit_empty_truncation_allowed_with_confirm's local _Agent.run_conversation mock didn't accept the new persist_user_message kwarg (added by the image-persist fix), causing a TypeError. Added **_kwargs to match the other test doubles in the file.

All tests pass locally now (test_model_switch_marker_role.py — 8/8, test_prompt_submit_empty_truncation_allowed_with_confirm — 1/1), and CI is green on the latest push.

Restores display_kind on the in-memory model-switch entry,
display_kind/display_metadata forwarding in _history_to_messages,
and the async-delegation metadata + stamping path in
_notification_poller_loop / _run_prompt_submit. Keeps the diff
scoped to the attached-image persistence fix.
@alelpoan

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up commit restoring the remaining timeline wiring that was unintentionally dropped:

  • display_kind on the in-memory model-switch entry
  • display_kind / display_metadata forwarding in _history_to_messages
  • the async-delegation metadata + stamping path in _notification_poller_loop (both call sites) and _run_prompt_submit

Verified with git diff origin/main...HEAD -- tui_gateway/server.py that the diff is now scoped to just the persist_user_message image-persistence fix (the new _build_persist_message_with_image_refs helper and its wiring into run_kwargs).

@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #71121, which builds directly on your commit — thanks for the clean diagnosis, persist_user_message was exactly the right mechanism and the frontend work carried over as-is.

Two things I found while reviewing that the follow-up covers:

  • Turns routed to a natively-vision-capable model send content as a parts list, and _flush_messages_to_session_db deliberately ignores a plain-string override for a list payload (a text override must not erase a turn's image summary). So the override was dropped for anyone on a vision-capable main model and the stored row kept only the caption plus [Image attached at: …] / [screenshot]. Your repro model routes to text mode, which is why it looked fixed. The override now mirrors the list shape.
  • @image:{path} is unquoted, and the unquoted alternative in the directive pattern is \S+, so a path with a space truncates at the first space. Composer images live in the app's userData dir, which on macOS is ~/Library/Application Support/<App>/ — a space every time. Refs are now quoted via a helper next to REFERENCE_PATTERN.

Your authorship is preserved in the history of #71121.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Attached image renders smaller with a border after session switch / restart

3 participants