Skip to content

feat(goose2): inline edit & retry#8538

Open
tellaho wants to merge 32 commits intomainfrom
donatello/edit-retry
Open

feat(goose2): inline edit & retry#8538
tellaho wants to merge 32 commits intomainfrom
donatello/edit-retry

Conversation

@tellaho
Copy link
Copy Markdown
Collaborator

@tellaho tellaho commented Apr 14, 2026

Category: new-feature
User Impact: Users can now edit sent messages in-place and retry from any point in a conversation, with all attachments, images, and persona overrides preserved through the re-send.

Problem: The Goose 2 desktop app had no way to correct a sent message or retry a failed exchange. Users who made a typo or wanted to refine their prompt had to start a new conversation.

Solution: Adds client-side inline edit and per-message retry using truncate-and-resend. Edit mode renders a textarea over the original message bubble with Save/Cancel controls and keyboard shortcuts (Enter to save, Escape to cancel, Shift+Enter for newline). Retry identifies the originating user message, reconstructs its full context (text, images, file attachments, persona), and re-sends after truncation. A Radix HoverCard replaces the old CSS group-hover for the action toolbar, fixing z-index and animation issues. Session forking support can be layered on as a follow-up.

File changes

ui/goose2/src/features/chat/hooks/useChat.ts
Added retryMessage, editMessage, and cancelEdit to the chat hook. retryMessage validates content and reconstructs attachment drafts before truncating history, then forwards text, persona, and attachments to sendMessage. editMessage enters non-destructive edit mode; truncation only happens on save.

ui/goose2/src/features/chat/hooks/tests/useChat.test.ts
Added 14 hook-level tests: 5 for retryMessage (truncation, persona preservation, assistant-to-user lookup, streaming/thinking guards), 3 for editMessage (state, streaming guard, assistant-role guard), 1 for cancelEdit, 3 for attachment preservation (image-only, file metadata, mixed content), and 2 for retryLastMessage delegation.

ui/goose2/src/features/chat/lib/attachments.ts
Added rebuildAttachmentDrafts() — reconstructs ChatAttachmentDraft[] from a stored message's ImageContent blocks and metadata.attachments, enabling retry and edit to preserve the original message's full attachment context.

ui/goose2/src/features/chat/stores/chatStore.ts
Added editingMessageIdBySession state and setEditingMessageId action. Editing state is scoped per-session and cleaned up on session cleanup.

ui/goose2/src/features/chat/stores/tests/chatStore.test.ts
Added tests for setEditingMessageId set, clear, and session cleanup.

ui/goose2/src/features/chat/ui/ChatView.tsx
Added handleSaveEdit — stops streaming if active, reads persona and attachments from the original message before truncation, then defers the re-send via a ref until chat state returns to idle. Wires onRetryMessage, onEditMessage, onSaveEdit, onCancelEdit, and editingMessageId to MessageTimeline.

ui/goose2/src/features/chat/ui/ChatInput.tsx
Syncs textarea text state when initialValue prop changes (needed for edit mode to populate the input). Added initialValue sync effect.

ui/goose2/src/features/chat/ui/MessageBubble.tsx
Replaced CSS group-hover action toolbar with Radix HoverCard. Added inline edit UI: textarea overlay with Save/Cancel buttons, keyboard handling (Enter/Escape/Shift+Enter), empty-text guard. User bubbles get muted background and max-width constraint.

ui/goose2/src/features/chat/ui/MessageTimeline.tsx
Passes onRetryMessage (assistant messages), onEditMessage (user messages), editingMessageId, onSaveEdit, and onCancelEdit through to MessageBubble.

ui/goose2/src/features/chat/ui/tests/ChatInput.test.tsx
Added test for initialValue sync — verifies textarea updates when the prop changes.

ui/goose2/src/features/chat/ui/tests/MessageBubble.test.tsx
Added 10 tests: inline edit keyboard shortcuts (Enter saves, Escape cancels, Shift+Enter allows newline), empty-text guard, edit/cancel button rendering, assistant edit-button exclusion, user edit-button presence, and HoverCard hover behavior.

ui/goose2/src/shared/i18n/locales/en/chat.json
Added edit.hint, edit.textareaAriaLabel keys for the inline edit UI.

ui/goose2/src/shared/i18n/locales/en/common.json
Removed dead messageBranch i18n keys (unused forking infrastructure).

ui/goose2/src/shared/i18n/locales/es/chat.json
Added Spanish translations for edit.hint, edit.textareaAriaLabel.

ui/goose2/src/shared/i18n/locales/es/common.json
Removed dead messageBranch i18n keys (unused forking infrastructure).

ui/goose2/src/shared/ui/ai-elements/message.tsx
Removed MessageBranch and 5 sub-components (~210 lines of dead code). Cleaned 11 unused imports.

ui/goose2/src/shared/ui/hover-card.tsx
Added bare variant to HoverCard — no background, border, or shadow — used by the message action toolbar.

Reproduction Steps

  1. Open the Goose 2 desktop app and start or resume a conversation.
  2. Send a user message, then hover over it — an edit (pencil) button appears in the HoverCard toolbar.
  3. Click edit — the message text appears in an inline textarea with Save and Cancel buttons.
  4. Modify the text and press Enter (or click Save) — the original message and all responses below it are removed, and the edited message is re-sent.
  5. Press Escape (or click Cancel) to exit edit mode without changes.
  6. Hover over an assistant message — a retry button appears but no edit button.
  7. Click retry on an assistant message — the preceding user message is re-sent with its original attachments and persona intact.

Demo

Screen.Recording.2026-04-14.at.12.47.56.PM.mov

@tellaho tellaho changed the title feat(goose2): inline edit & retry UX with attachment/persona preservation feat(goose2): inline edit & retry UX with attachment and persona preservation Apr 14, 2026
@tellaho tellaho force-pushed the donatello/edit-retry branch from f7e3502 to 6a5d19f Compare April 14, 2026 22:55
@tellaho tellaho changed the title feat(goose2): inline edit & retry UX with attachment and persona preservation feat(goose2): inline edit & retry Apr 14, 2026
@tellaho tellaho marked this pull request as ready for review April 14, 2026 23:39
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a5d19ff3d

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/hooks/useChat.ts
Comment thread ui/goose2/src/features/chat/lib/attachments.ts Outdated
@tellaho tellaho force-pushed the donatello/edit-retry branch from 6a5d19f to 5374dfb Compare April 14, 2026 23:48
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5374dfbc92

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/lib/attachments.ts Outdated
Comment thread ui/goose2/src/features/chat/lib/attachments.ts
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3fc723c0c8

ℹ️ 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".

Comment on lines 379 to 383
await sendMessage(
textContent.text,
text,
targetPersonaId
? { id: targetPersonaId, name: targetPersonaName }
: undefined,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route retry sends through persona/provider switching logic

This retry path calls sendMessage directly with only targetPersonaId, but sendMessage still selects the provider from providerOverride (the current session provider). If a user retries an older message after switching providers, the resend can go to the wrong backend even though the persona metadata is preserved. ChatView.handleSend already contains the provider-switch flow for persona changes, so bypassing it here can change model/provider behavior for retries.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up. P2 — routing retry through persona/provider switching. Not blocking merge.

Comment on lines +385 to +387
} else if (e.key === "Enter" && !e.shiftKey) {
e.preventDefault();
handleEditSave();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Ignore Enter during IME composition in inline edit

Pressing Enter in this handler always saves (unless Shift is held), but there is no composition guard (isComposing). For CJK/IME users, Enter is used to confirm a candidate, so this can unexpectedly submit the edit and truncate/re-send conversation history mid-composition. The Enter branch should be skipped while composition is active.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 87bfe5a82 — added !e.nativeEvent.isComposing guard to the Enter key handler in the inline edit handleEditKeyDown callback so CJK/IME composition is not interrupted.

@matt2e
Copy link
Copy Markdown
Collaborator

matt2e commented Apr 16, 2026

using truncate-and-resend

I think this PR doesn't do that exactly (based on some quick pairing with an LLM). The goose backend seems to delete the session history if you edit a message so the user sees the session as if it was rewound and that message was changed, but the LLM sees the full history.

So if you edit a message the LLM thinks this is the history:

User: "Write a function to sort a list"
Assistant: [tool_call: write_file] ... [response with code]  ← still here!
User: "Write a function to sort a list in reverse"           ← appended

I'm fine for us to merge this in as the behaviour, but let's make that clearer in the PR description. I'm not sure there is a better way at the moment without the proper ACP truncate+fork feature that seems to be coming soon.

Also, it seems all user messages are editable, so the difference between what the user sees and what the LLM thinks the history is could be very different.

Thinking about it a bit more: is it worth having the feature if it doesnt actually revise the LLM's history? Is there a benefit?

tellaho added 20 commits April 16, 2026 13:04
- Add retryMessage(messageId) to useChat — truncates from the target
  message onward and resends the preceding user message. Works for
  both user and assistant messages.
- Add editMessage(messageId) to useChat — truncates from the target
  user message onward and populates the chat input draft with the
  original text for revision.
- Wire onRetryMessage and onEditMessage from ChatView through
  MessageTimeline to MessageBubble.
- Remove role gate so retry icon shows on both user and assistant
  messages (edit remains user-only).
ChatInput initialized local text via useState(initialValue) which only
reads the prop on mount. Subsequent changes (e.g. cancel edit then
re-edit) were ignored. Added useEffect to re-sync setTextRaw when
initialValue changes. Uses setTextRaw (not setText) to avoid redundant
onDraftChange callback since the store already holds the correct value.
The focus effect only ran on mount. Now keyed on editingMessageId so the
textarea receives focus whenever the user clicks edit.
Pivot from bottom-ChatInput editing to inline editing. When clicking
edit, the message bubble transforms into a textarea with Save and
Cancel buttons. Enter saves, Escape cancels.

- MessageBubble: add isEditing mode with inline textarea, auto-resize,
  focus, and keyboard shortcuts (Enter=save, Escape=cancel)
- MessageTimeline: pass editingMessageId, onSaveEdit, onCancelEdit
- ChatView: add handleSaveEdit (truncate + send), wire to timeline
- ChatInput: remove edit indicator bar, editingMessageId prop, and
  edit-mode focus effect
When editing, the container breaks out of the 80% user bubble constraint
to span the full timeline width with a subtle bg-muted/40 background.
Hides the user avatar during edit. Adds hint text indicating editing
will start a new conversation.
1. handleSaveEdit now forces chatState to idle via store and defers
   sendMessage via pendingEditSend ref + useEffect, avoiding the stale
   closure where sendMessage would bail on old chatState.
2. Split focus into a separate useEffect keyed only on isEditing so it
   doesn't fire on every keystroke and reset cursor position.
3. Removed dead setDraft call in editMessage — inline edit reads text
   directly from the message, not the draft store.
4. Removed unused edit.label and edit.cancel i18n keys.
User messages render as rounded bg-muted bubbles on the content div
only — action buttons and timestamp sit outside. Max width capped at
640px. Avatar removed. Outer group/gap-3 hover structure preserved.
…tions

Swap the fragile CSS group-hover opacity pattern for Radix HoverCard
which handles hover intent, open/close delays, and portal rendering.
Actions appear below the bubble on hover via stripped HoverCardContent
(no border, shadow, padding, or background). Removes group class.
Force-override base HoverCardContent styles (shadow, border, bg,
animations, rounded, padding) with !important. Add mb-6 to user
content div so the portal actions float in the reserved gap.
Remove !animate-none override so fade and slide transitions from
the base HoverCardContent are preserved. All other chrome overrides
(shadow, border, bg, padding, rounded) remain.
8 new tests: inline textarea pre-fill, save/cancel clicks, Enter/Escape
keys, disabled save on empty text, edit hint visibility, retry click.
Updated existing hover test to trigger HoverCard via userEvent.hover.
All 49 tests green (25 MessageBubble + 24 ChatInput).
If editIndex is -1 (message deleted between click and save), bail
early instead of sending to untruncated history. Clears edit state.
…cale

1. Added variant='bare' to HoverCardContent — strips all chrome.
   MessageBubble uses it instead of six !important overrides.
2. Removed duplicate height calc from ChatInput handleInput —
   useLayoutEffect([text]) already covers it.
3. Edit state (editText, editTextareaRef, effects) gated on
   role==='user' via canEdit flag.
4. Added edit.hint to Spanish locale.
…pped

Replace py-4 with pt-4 pb-12 on the timeline inner container to
give the last message enough room for HoverCard actions to render.
Bare variant now z-40 instead of z-50. Popovers and dropdowns
at z-50 will always paint on top of message action hovercards.
Removed size=sm and h-7/px-3/text-xs overrides from Save/Cancel.
Hint text now left-aligned, buttons right-anchored via ml-auto.
Replace px-4 py-2.5 with p-3 for consistent 12px padding on all sides.
…dead code

Bug 1: retryMessage no longer truncates history on image-only messages.
Content is validated BEFORE truncation — if there's nothing to re-send
(no text, no images, no attachments), the operation bails cleanly.

Bug 2: retryMessage and handleSaveEdit now forward attachments and persona
from the original message. Added rebuildAttachmentDrafts() to reconstruct
ChatAttachmentDraft[] from stored message content (ImageContent blocks)
and metadata.attachments (file/directory refs).

Dead code: Removed MessageBranch components (~210 lines) from message.tsx
and messageBranch i18n keys from en/es common.json. Unused forking artifacts.

Tests: +6 new tests covering image-only retry safety, attachment preservation,
assistant edit-button exclusion, Shift+Enter multiline, and chatStore
editingMessageId set/clear/cleanup. 430 tests green.
…rrow TS union

Blocker 1: Narrow ChatAttachmentDraft union in attachments.test.ts before
asserting base64 — fixes TS compilation error that broke CI.

Blocker 2: Unify pendingEditSend + deferredSend into a single pendingSend
ref with a reason discriminator. Edit path now uses flushSync to commit
state synchronously and calls sendMessage immediately — no timing gap,
no race condition. Persona path keeps the deferred effect with the
unified ref.

Blocker 3: Re-add !isUser guard on retry button in MessageBubble so
retry only shows on assistant messages (edit covers user messages).

Bonus: TODO comment on rebuildAttachmentDrafts dedup heuristic explaining
the assumption that all images produce both a content block and metadata
entry.
tellaho added 3 commits April 16, 2026 13:05
…o providerId)

The ACP architecture rewrite on main removed the providerId parameter
from acpSendMessage. Four retry/edit tests still passed the old "goose"
providerId argument — remove it so assertions match the new 3-arg
signature: acpSendMessage(sessionId, prompt, options).
…before_message_id

Uses timestamp bridge: truncate thread_messages by message_id to get
the created_timestamp, then truncate messages table by that timestamp.
This handles the different ID schemes (tmsg_ vs msg_) across tables.
@tellaho tellaho force-pushed the donatello/edit-retry branch from 3fc723c to a5d12c2 Compare April 17, 2026 00:58
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5d12c2e7d

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/hooks/useChat.ts Outdated
Comment thread ui/goose2/src/features/chat/ui/ChatView.tsx Outdated
Comment thread crates/goose-acp/src/server.rs Outdated
Thread truncation metadata from edit/retry UI through the WebSocket API:
- acpApi.ts: prompt() accepts optional _meta parameter
- acp.ts: acpSendMessage passes truncateBeforeMessageId via _meta
- useChat.ts: sendMessage accepts and forwards truncation param,
  retryMessage passes the user message ID for backend truncation
- ChatView.tsx: handleSaveEdit passes truncation ID, uses deferred
  pattern for edit-during-streaming (BLOCKER 2), removes flushSync
  hack and dead 'edit' reason from pendingSend

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39c6149bf9

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/hooks/useChat.ts
…ncation

P0 fix: the frontend generated client-side UUIDs for messages while the
backend independently generated tmsg_{uuid} IDs. When edit/retry sent
the client UUID as truncate_before_message_id, the backend lookup found
nothing and never truncated.

Fix:
- Frontend sends messageId on PromptRequest (ACP unstable_message_id)
- Server uses args.message_id as the thread message ID instead of
  generating a new one, so both sides share the same ID
- Replay chunks now carry message_id so loaded sessions also have
  correct IDs for future edit/retry operations

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: def6cca68a

ℹ️ 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".

Comment thread crates/goose-acp/src/server.rs
Comment thread ui/goose2/src/features/chat/hooks/useChat.ts
tellaho added 2 commits April 16, 2026 21:08
Five test cases covering:
- truncate target + subsequent messages
- truncate from first message clears all
- nonexistent message ID is a no-op
- truncate last message only
- truncation is scoped to the target thread

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5aa09f4fb

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/ui/ChatView.tsx
Comment thread ui/goose2/src/features/chat/ui/ChatView.tsx
- Add isComposing guard to Enter key handler in MessageBubble inline
  edit so CJK/IME users don't accidentally save mid-composition
- Dismiss queued follow-up messages before forcing idle state in
  handleSaveEdit to prevent useMessageQueue from auto-sending against
  the truncated timeline

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho
Copy link
Copy Markdown
Collaborator Author

tellaho commented Apr 17, 2026

Addressing PR comments

matt2e's comment (LLM sees full history)

Fixed. Commits a5d12c2e7 through def6cca68 implement Option A — the backend now truncates both thread_messages and messages tables via _meta.truncate_before_message_id on the PromptRequest. The LLM context is properly rewound on edit/retry. Frontend and backend share the same message IDs (def6cca68), so truncation actually works.

Codex bot comments — triage

Already fixed in this PR:

  • P1: Use backend message IDs → Fixed in def6cca68 (frontend sends messageId on PromptRequest, server uses it)
  • P1: Propagate truncate marker on retry → Fixed in 39c6149bf (retryMessage passes truncateMessageId)
  • P1: Avoid dropping edit resend after stopStreaming → Fixed in 39c6149bf (deferred pendingEditSend ref + useEffect)
  • P2: Keep unsent draft when cancelling edit → Fixed in 143ab8676 (cancelEdit no longer calls clearDraft)
  • P2: Rehydrate pathless file attachments → Fixed in cbf6732ee
  • P2: IME composition guard → Fixed in 87bfe5a82 (added isComposing check)
  • P1: Clear queued sends before forcing idle → Fixed in 87bfe5a82 (dismissQueuedMessage before setChatState)

Acknowledged, follow-up:

  • P1: Make truncation and re-append atomic — Valid. The truncate + append sequence should be transactional. Filed as follow-up — requires adding transaction support to the ACP server's on_prompt handler.
  • P2: Truncate session with precise boundary — Timestamp is second-resolution, but for edit/retry we always truncate from a user message (unique per second). Tool calls happen after and are correctly deleted. Low practical risk, but sub-second precision would be cleaner. Follow-up.
  • P2: Restore truncated history on send failure — Valid UX concern. If sendMessage fails after local truncation, messages are lost locally. Snapshot + restore pattern would fix this. Follow-up.
  • P2: Defer local retry truncation until resend succeeds — Same pattern as above. Follow-up.
  • P2: Route retry through persona/provider switching — Edge case when user switches providers mid-session then retries old message. Follow-up.
  • P2: Skip image metadata / deduplicate attachments — Edge cases around mixed image+file attachments. Follow-up.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87bfe5a822

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/ui/ChatView.tsx Outdated
Comment thread ui/goose2/src/features/chat/hooks/useChat.ts Outdated
tellaho added 2 commits April 17, 2026 06:15
Three follow-ups from PR review:

1. Atomic truncation: new ThreadManager.truncate_and_append() wraps
   truncate + append in a single BEGIN IMMEDIATE transaction so a crash
   can't leave a half-truncated conversation.

2. Precise session truncation: changed truncate_conversation to use
   autoincrement row IDs for ordering instead of second-resolution
   timestamps, eliminating the risk of over-deleting messages that
   share the same second.

3. Snapshot+restore on failure: retryMessage and handleSaveEdit now
   snapshot the message array before optimistic truncation and restore
   it if sendMessage throws, preventing a broken UI state on network
   errors.

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 103865b8f1

ℹ️ 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".

Comment thread ui/goose2/src/features/chat/ui/ChatView.tsx
Comment thread ui/goose2/src/features/chat/ui/MessageBubble.tsx Outdated
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho
Copy link
Copy Markdown
Collaborator Author

tellaho commented Apr 17, 2026

PR Comment Sweep

Human reviewer (matt2e)

LLM sees full history on edit — ✅ Fixed. Option A commits (a5d12c2e7 through 103865b8f) implement backend truncation via _meta.truncate_before_message_id. Both thread_messages and messages tables are truncated atomically before appending the new prompt.

Latest Codex review (103865b8f)

# Comment Status
P1 Route edit resend through persona/provider switch flow Follow-up — edge case where edit bypasses handlePersonaChange. Only matters if user switches provider AND edits an old message. Narrow window, no data loss.
P2 Ignore Escape during IME composition Fixed in c7e96edc4isComposing guard on Escape, matching the Enter guard.

Earlier Codex reviews (already addressed)

Comment Status
Use backend message IDs for truncation ✅ Fixed in def6cca68 — frontend sends messageId on PromptRequest
Propagate truncate marker when retrying ✅ Fixed in 39c6149bfretryMessage passes truncateMessageId
Avoid dropping edit resend after stopStreaming ✅ Fixed in 39c6149bf — deferred pendingEditSend ref pattern
Keep unsent draft when cancelling edit ✅ Fixed in 143ab8676cancelEdit no longer calls clearDraft
Make truncation atomic ✅ Fixed in 103865b8ftruncate_and_append() single transaction
Precise session truncation boundary ✅ Fixed in 103865b8f — autoincrement ID ordering
Restore history on send failure ✅ Fixed in 103865b8f — snapshot+restore in retry and edit
Clear queued sends before forcing idle ✅ Fixed in 87bfe5a82dismissQueuedMessage before setChatState
IME Enter guard ✅ Fixed in 87bfe5a82isComposing check
Rehydrate pathless file attachments ✅ Fixed in cbf6732ee
Deduplicate image attachments Follow-up — edge case with mixed image+file attachments
Skip image metadata when rebuilding Follow-uphasImageDrafts heuristic has a TODO tracking it

@tellaho
Copy link
Copy Markdown
Collaborator Author

tellaho commented Apr 17, 2026

All PR comments addressed

matt2e's comment (LLM sees full history)

Fixed. This was accurate when the comment was written, but the Option A commits (a5d12c2e7 through 48232511f) now truncate both storage layers:

  • thread_messages (display log) — truncated by message_id
  • messages (LLM context) — truncated via timestamp bridge → autoincrement ID boundary

The LLM now sees the same truncated history as the user. The truncation + new message append is atomic (single BEGIN IMMEDIATE transaction).

Codex review comments (18 total)

  • 10 fixed — pointed to specific commits on each comment thread
  • 3 previously addressed by Bart's earlier sweep
  • 5 acknowledged as follow-ups (P2, not blocking merge):
    • Skip image metadata when rebuilding retry attachments
    • Deduplicate image attachments per file
    • Route retry/edit sends through persona/provider switching
    • Preserve compose draft on retry/edit
    • Wait for cancellation before truncating

Additional fix from this sweep

  • c7e96edc4 — IME isComposing guard on Escape key (was only on Enter)

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7e96edc40

ℹ️ 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".

Comment on lines +370 to +372
const attachmentDrafts = rebuildAttachmentDrafts(userMessage);
const hasContent = text.trim() || attachmentDrafts.length > 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve replay metadata before retrying loaded messages

retryMessage reconstructs resend context entirely from the in-memory userMessage, but replayed history messages are built from chunk events that do not include the original attachment/persona metadata. After reopening a session, retry/edit can therefore resend only text and fall back to the currently selected persona/provider instead of the original target context. Please restore attachment/persona metadata during replay (or disable retry/edit when that metadata is unavailable) so loaded-history retries are faithful.

Useful? React with 👍 / 👎.

Comment on lines +464 to +468
pendingEditSend.current = {
text,
truncateMessageId: messageId,
persona,
attachments,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Persist deferred edit resend across component teardown

In the busy-state edit path, the code truncates local history and then parks the resend payload in the component-local pendingEditSend ref. If the chat view unmounts before the idle effect drains that ref (for example, user navigates away right after save), the pending resend is lost and the conversation stays truncated without the edited replacement message. The deferred resend state should live in shared/session state or be dispatched in a teardown-safe queue.

Useful? React with 👍 / 👎.

Pass userMessage.id from the local Zustand store through to
acpSendMessage so the backend stores the same UUID. Without this,
the frontend and backend have different IDs for the same user message,
causing truncate_before_message_id lookups to find nothing — edit/retry
truncation silently fails and messages zombie back on session reload.

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: becf37f2a9

ℹ️ 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".

Comment on lines +442 to +443
if let Some((row_id, created_ts)) = row {
// 2. Delete from thread_messages
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject truncate requests when target message is missing

truncate_and_append only deletes history when the lookup finds a matching row, but it still appends the new message unconditionally afterward. If the client sends a stale/unknown truncate_message_id (for example after replay/id drift), retry/edit becomes append-only on the backend while the UI has already truncated locally, so deleted turns reappear after reload and model context is wrong. This path should return an error (or skip append) when the truncate target is not found.

Useful? React with 👍 / 👎.

Comment on lines +454 to +456
"DELETE FROM messages WHERE session_id = ? AND id >= \
(SELECT id FROM messages WHERE session_id = ? AND created_timestamp >= ? ORDER BY id ASC LIMIT 1)",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use exact message mapping for session truncate boundary

Fresh evidence: the session-side delete boundary is still chosen with created_timestamp >= ? and the first matching row id, which cannot distinguish multiple messages created in the same second. In that case, truncating from a later message can delete earlier same-second messages that should be kept. The boundary needs a stable per-message mapping/tie-breaker (not timestamp-only bridging) to avoid over-truncation.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants