fix(desktop): allow removing images when editing user messages - #9979
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8da09e619
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
b8da09e to
4454c37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7a3af2bbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Address Codex review on aaif-goose#9979: - UserMessage: compute keepImages before the empty-text check so image-only messages (or removing some of several images) are no longer rejected; reject only when both text and kept images are empty - BaseChat: pass the fork payload whenever editedMessage is defined (not just truthy) so empty-text image-only forks still submit images Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82bea07303
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Address Codex review on aaif-goose#9979: extracting kept images as bare {data, mimeType} dropped the _meta and annotations the ACP adapter preserves on image content blocks, mutating the message replayed to the agent even when the user only edited text. Extend ImageData to carry optional _meta/annotations, add a shared imageDataFromMessage extractor that preserves them, propagate them through createUserMessage, and replace the duplicated stripping mappings across UserMessage, chatSessionController, and useChatStream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e838446e77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { textContent, imagePaths } = getTextAndImageContent(message); | ||
| const timestamp = formatMessageTimestamp(message.created); | ||
|
|
||
| // Extract image data from message content for edit mode management |
There was a problem hiding this comment.
Remove self-evident edit-image comments
The root /workspace/goose/AGENTS.md says “Never add comments that restate what code does”; this new comment only repeats the immediately following imageDataFromMessage(message) assignment, and the same pattern appears in the newly added edit-image state/keepImages comments. Please remove these comments or keep only non-obvious rationale.
Useful? React with 👍 / 👎.
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for the PR, and for the thoughtful responses to the earlier codex feedback. Two things to clean up before this lands:
- The self-evident comments codex flagged (P1) are still in
UserMessage.tsx. They restate what the code does and should be removed — the names already make the intent clear. - The inline
import('../types/message').ImageData[]type annotations across the callback chain should be normal top-level imports for readability/consistency.
No blocker on the feature itself — just these. See the inline comments.
| const { textContent, imagePaths } = getTextAndImageContent(message); | ||
| const timestamp = formatMessageTimestamp(message.created); | ||
|
|
||
| // Extract image data from message content for edit mode management |
There was a problem hiding this comment.
These comments just restate what the code does (imageDataFromMessage / removedImageIndices already say it). Per the project's code-quality guidance, prefer self-documenting code and drop comments that don't explain why. Same applies to // Handle removing an image during editing, // Build list of images to keep (exclude removed ones), and // Images in edit mode below — please remove these. This is the unaddressed P1 from codex.
There was a problem hiding this comment.
Addressed in fc4c9750a. Removed the self-evident image-edit comments in UserMessage.tsx and left the code self-documenting.
| messageId: string, | ||
| newContent: string, | ||
| editType: 'fork' | 'edit' | undefined, | ||
| keepImages: import('../types/message').ImageData[] | undefined, |
There was a problem hiding this comment.
Inline import('../types/message').ImageData[] is hard to read and repeated in several files. Please add a normal top-level import { ImageData } from '../types/message'; (matching UserMessage.tsx) and use ImageData[] here and in the other spots (BaseChat.tsx, ProgressiveMessageList.tsx, useAcpChatSession.ts, useChatSessionTypes.ts, useChatStream.ts).
There was a problem hiding this comment.
Addressed in fc4c9750a. Replaced the inline ImageData annotations with top-level type imports across chatSessionController, BaseChat, ProgressiveMessageList, useAcpChatSession, useChatSessionTypes, and useChatStream.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4c9750a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6689cdc9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for working on this. One important bit of context: the desktop app has moved to the ACP+ flow, so we should no longer expand/maintain the legacy non-ACP chat-stream path for this behavior. Could you please clean up the diff so this fits the new paradigm? In practice, please focus the implementation on the ACP path: truncate/fork the ACP session, then submit the edited message normally with the retained images. Any changes to the old I am going to snooze this for a few days to give you time to adjust. |
5e09d39 to
15eab78
Compare
When editing a user message that contains images, the edit UI previously showed only a textarea with no visibility or control over attached images. Images were silently preserved on save with no way to remove them. This change: - Shows attached images in the edit mode UI with hover-to-reveal X buttons - Tracks removed images and only preserves kept images on save - Propagates keepImages through the full callback chain (UserMessage → ProgressiveMessageList → useChatStream / useAcpChatSession → chatSessionController) - Fixes fork session path to also carry editedImages so forked sessions preserve user-selected images
f95ffda to
958bc98
Compare
# Conflicts: # ui/desktop/src/acp/chatSessionController.ts # ui/desktop/src/components/BaseChat.tsx # ui/desktop/src/types/message.ts
DOsinga
left a comment
There was a problem hiding this comment.
thanks and sorry for the long journey!
* main: fix(desktop): allow removing images when editing user messages (#9979) Fetch cursor-agent models from CLI (#10367) fix(cerebras): preserve thinking inline for models that reject reasoning_content (#10774) perf: make CLI streaming render incremental instead of O(n^2) (#10487) Moving to issues as the new PRs (#10818) ci: defer privileged recipe scans for forks (#10825) docs: adopt issue-first contribution workflow (#10819) Add Azure AI Foundry multi-LLM provider (#10622) fix(together): parse model list response (#10576) docs: remove retired site resources (#10817) feat: streaming shell output while commands run (#10808)
* main: (224 commits) fix(desktop): allow removing images when editing user messages (aaif-goose#9979) Fetch cursor-agent models from CLI (aaif-goose#10367) fix(cerebras): preserve thinking inline for models that reject reasoning_content (aaif-goose#10774) perf: make CLI streaming render incremental instead of O(n^2) (aaif-goose#10487) Moving to issues as the new PRs (aaif-goose#10818) ci: defer privileged recipe scans for forks (aaif-goose#10825) docs: adopt issue-first contribution workflow (aaif-goose#10819) Add Azure AI Foundry multi-LLM provider (aaif-goose#10622) fix(together): parse model list response (aaif-goose#10576) docs: remove retired site resources (aaif-goose#10817) feat: streaming shell output while commands run (aaif-goose#10808) feat(acp): allow session/new to request a Hidden session via _meta.hidden (aaif-goose#10811) chore: removed stale text pnpm workspace (aaif-goose#10809) feat(otel): emit GenAI semantic convention attributes (aaif-goose#10700) fix: avoid double loading full conversation on session open (aaif-goose#10794) feat(providers): forward images and MCP embedded-resource blobs in Anthropic and Google formats (aaif-goose#10340) Deprecate and remove ui/text TUI (aaif-goose#10799) Upgrade to rmcp 3.0 (aaif-goose#10789) fix(release): signing environment (aaif-goose#10797) feat(tools): collapse const-union enums in tool schemas (aaif-goose#10577) ...
Problem
When editing a user message that contains images in the Goose Desktop UI, the edit mode only shows a textarea — attached images are invisible and cannot be removed. Images are silently preserved on save with no way for the user to control which ones to keep.
Fix
UserMessage.tsx(core)ImageData[]from message content for edit mode managementremovedImageIndicesstatekeepImagesparameter toonMessageUpdatecallbackCallback chain (5 files)
ProgressiveMessageList.tsx,useChatStream.ts,useAcpChatSession.ts,chatSessionController.ts,useChatSessionTypes.tsonMessageUpdate/updateMessagesignatures updated with optionalkeepImages?: ImageData[]parameterkeepImagesis provided, use it; otherwise fall back to preserving all images (backward compatible)Fork path fix (
BaseChat.tsx)editedImagesalongsideeditedMessageFiles changed
UserMessage.tsxProgressiveMessageList.tsxuseChatStream.tsuseAcpChatSession.tschatSessionController.tsuseChatSessionTypes.tsBaseChat.tsxTesting