From 164430e37f5806d09d9482db61eef011007f4281 Mon Sep 17 00:00:00 2001
From: Marco Beretta <81851188+berry-13@users.noreply.github.com>
Date: Fri, 28 Aug 2026 19:59:55 +0200
Subject: [PATCH] fix(client): address second round of Codex review findings

Reuse the shared Button primitive for the three question move controls (the
chat card's chevron and both popover chevrons), which each repeated the same
hover, focus-ring, color and rounding recipe locally. Only the icon geometry
stays local.

Drop provider image results that carry neither thumbnailUrl nor imageUrl.
Both are optional in the ImageResult contract, and an unusable entry still
made images.length nonzero, so the card rendered an anchor and img with
undefined destinations instead of omitting the result.

Gate the memory deletion confirmation on a completed phase. The panel opens as
soon as the key streams in and the phase can still settle as cancelled, so
"Memory deleted" was claiming a removal that never happened while the header
correctly read Cancelled.

Key pending answer drafts by action id instead of a single shared slot. With
Save drafts disabled, editing a collapsed question in one conversation and
then editing another paused conversation's question overwrote the only slot,
and the action-scoped reader then showed the first card an empty box with its
unsent answer unrecoverable. Entries are dropped once answered.
---
 .../Chat/Input/AskUserQuestionPopover.tsx     | 18 +++----
 .../Chat/Messages/Content/AskUserQuestion.tsx |  9 ++--
 .../Messages/Content/Parts/MemoryCall.tsx     |  7 ++-
 .../Chat/Messages/Content/verticals.tsx       | 10 +++-
 .../src/hooks/Input/useAskAnswerMode.spec.ts  | 47 +++++++++++++------
 client/src/hooks/Input/useAskAnswerMode.ts    | 41 ++++++++++------
 6 files changed, 88 insertions(+), 44 deletions(-)

diff --git a/client/src/components/Chat/Input/AskUserQuestionPopover.tsx b/client/src/components/Chat/Input/AskUserQuestionPopover.tsx
index 4565ebd753..bf9ceb6b8c 100644
--- a/client/src/components/Chat/Input/AskUserQuestionPopover.tsx
+++ b/client/src/components/Chat/Input/AskUserQuestionPopover.tsx
@@ -63,14 +63,15 @@ function AskUserQuestionsPopoverPanel({ ask }: { ask: ReturnType<typeof useAskAn
             description={localize('com_ui_ask_move_to_chat')}
             side="top"
             render={
-              <button
-                type="button"
+              <Button
+                variant="ghost"
+                size="icon"
                 aria-label={localize('com_ui_ask_move_to_chat')}
-                className="rounded-md p-1 text-text-secondary transition-colors hover:bg-surface-hover hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy"
+                className="size-auto rounded-md p-1 text-text-secondary"
                 onClick={collapse}
               >
                 <ChevronDown className="size-4" aria-hidden="true" />
-              </button>
+              </Button>
             }
           />
         </div>
@@ -171,14 +172,15 @@ function AskUserQuestionPopoverPanel({
             description={localize('com_ui_ask_move_to_chat')}
             side="top"
             render={
-              <button
-                type="button"
+              <Button
+                variant="ghost"
+                size="icon"
                 aria-label={localize('com_ui_ask_move_to_chat')}
-                className="rounded-md p-1 text-text-secondary transition-colors hover:bg-surface-hover hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy"
+                className="size-auto rounded-md p-1 text-text-secondary"
                 onClick={collapse}
               >
                 <ChevronDown className="size-4" aria-hidden="true" />
-              </button>
+              </Button>
             }
           />
         </div>
diff --git a/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx b/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx
index 6087302a00..016872666c 100644
--- a/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx
+++ b/client/src/components/Chat/Messages/Content/AskUserQuestion.tsx
@@ -191,14 +191,15 @@ function AskUserQuestionSingle({
             description={localize('com_ui_ask_move_to_composer')}
             side="top"
             render={
-              <button
-                type="button"
+              <Button
+                variant="ghost"
+                size="icon"
                 aria-label={localize('com_ui_ask_move_to_composer')}
-                className="rounded-md p-1 text-text-secondary transition-colors hover:bg-surface-hover hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy"
+                className="size-auto rounded-md p-1 text-text-secondary"
                 onClick={expand}
               >
                 <ChevronUp className="size-4" aria-hidden="true" />
-              </button>
+              </Button>
             }
           />
         )}
diff --git a/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx b/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx
index 35acd69e17..8c41160315 100644
--- a/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx
+++ b/client/src/components/Chat/Messages/Content/Parts/MemoryCall.tsx
@@ -121,7 +121,12 @@ export default function MemoryCall({
                       {memoryValue}
                     </div>
                   )}
-                  {!isSave && (
+                  {/* Only a completed delete actually removed anything. The
+                      panel opens as soon as the key streams in, and the phase
+                      can still land on cancelled, so an ungated confirmation
+                      claimed the memory was gone while the header said
+                      Cancelled. The key above already shows what was tried. */}
+                  {!isSave && phase === 'completed' && (
                     <div className="text-sm italic text-text-secondary">
                       {localize('com_ui_memory_deleted')}
                     </div>
diff --git a/client/src/components/Chat/Messages/Content/verticals.tsx b/client/src/components/Chat/Messages/Content/verticals.tsx
index 4d04f4d067..1845a901ba 100644
--- a/client/src/components/Chat/Messages/Content/verticals.tsx
+++ b/client/src/components/Chat/Messages/Content/verticals.tsx
@@ -50,7 +50,15 @@ export function collectSearchVerticals(attachments?: TAttachment[]): SearchVerti
     if (!data) {
       continue;
     }
-    images.push(...(data.images ?? []));
+    /** Both image URLs are optional in the provider contract and `ImageStrip`
+     *  needs one for the `<img>` src. Keeping an entry with neither still
+     *  makes `images.length` nonzero, so the card renders a broken tile
+     *  instead of omitting the unusable result. */
+    for (const image of data.images ?? []) {
+      if (image.thumbnailUrl || image.imageUrl) {
+        images.push(image);
+      }
+    }
     shopping.push(...(data.shopping ?? []));
     places.push(...(data.places ?? []));
   }
diff --git a/client/src/hooks/Input/useAskAnswerMode.spec.ts b/client/src/hooks/Input/useAskAnswerMode.spec.ts
index 7f0df2014f..6f59bf0c1e 100644
--- a/client/src/hooks/Input/useAskAnswerMode.spec.ts
+++ b/client/src/hooks/Input/useAskAnswerMode.spec.ts
@@ -5,11 +5,18 @@ const mockSetComposerText = jest.fn();
 const mockSetCollapsedIds = jest.fn();
 const mockSetSelected = jest.fn();
 const mockSetChecked = jest.fn();
-const mockSetAnswerDraft = jest.fn();
+let mockAnswerDrafts: Record<string, string> = {};
+/** Applies updater functions so the tests can assert the resulting drafts
+ *  rather than the setter's call shape. */
+const mockSetAnswerDrafts = jest.fn((update: unknown) => {
+  mockAnswerDrafts =
+    typeof update === 'function'
+      ? (update as (current: Record<string, string>) => Record<string, string>)(mockAnswerDrafts)
+      : (update as Record<string, string>);
+});
 const mockSetDraft = jest.fn();
 let mockSaveDrafts = false;
 let mockCollapsedIds: string[] = [];
-let mockAnswerDraft = { actionId: null as string | null, text: '' };
 
 jest.mock('~/data-provider', () => ({ useGetMessagesByConvoId: jest.fn() }));
 jest.mock('~/components/Chat/Messages/Content/ApprovalContext', () => ({
@@ -41,7 +48,7 @@ jest.mock('recoil', () => ({
       return [[], mockSetChecked];
     }
     if (state.key === 'askAnswerModeText') {
-      return [mockAnswerDraft, mockSetAnswerDraft];
+      return [mockAnswerDrafts, mockSetAnswerDrafts];
     }
     return [[], jest.fn()];
   },
@@ -74,7 +81,7 @@ describe('useAskAnswerMode', () => {
     jest.clearAllMocks();
     mockSaveDrafts = false;
     mockCollapsedIds = [];
-    mockAnswerDraft = { actionId: null, text: '' };
+    mockAnswerDrafts = {};
     mockGetComposerText.mockReturnValue('answer from A');
   });
 
@@ -126,13 +133,13 @@ describe('useAskAnswerMode', () => {
 
     act(() => result.current.collapse());
 
-    expect(mockSetAnswerDraft).not.toHaveBeenCalled();
+    expect(mockSetAnswerDrafts).not.toHaveBeenCalled();
     expect(mockResetComposer).not.toHaveBeenCalled();
   });
 
   it('does not overwrite normal composer text when expanding a batch', () => {
     mockCollapsedIds = ['a1'];
-    mockAnswerDraft = { actionId: 'a1', text: 'stale batch handoff' };
+    mockAnswerDrafts = { a1: 'stale batch handoff' };
     mockUseGetMessages.mockReturnValue({ data: batchAsk });
     const { result } = renderHook(() => useAskAnswerMode('conversation-1'));
 
@@ -168,10 +175,7 @@ describe('useAskAnswerMode', () => {
 
     act(() => result.current.collapse());
 
-    expect(mockSetAnswerDraft).toHaveBeenCalledWith({
-      actionId: 'a1',
-      text: 'answer from A',
-    });
+    expect(mockAnswerDrafts).toEqual({ a1: 'answer from A' });
     expect(mockResetComposer).toHaveBeenCalledTimes(1);
   });
 
@@ -187,7 +191,7 @@ describe('useAskAnswerMode', () => {
 
   it('restores the card answer into the composer when drafts are disabled', () => {
     mockCollapsedIds = ['a1'];
-    mockAnswerDraft = { actionId: 'a1', text: 'answer edited in the card' };
+    mockAnswerDrafts = { a1: 'answer edited in the card' };
     mockUseGetMessages.mockReturnValue({ data: liveAsk });
     const { result } = renderHook(() => useAskAnswerMode('conversation-1'));
 
@@ -203,16 +207,29 @@ describe('useAskAnswerMode', () => {
 
     act(() => result.current.setAnswerText('answer edited in the card'));
 
-    expect(mockSetAnswerDraft).toHaveBeenCalledWith({
-      actionId: 'a1',
-      text: 'answer edited in the card',
-    });
+    expect(mockAnswerDrafts).toEqual({ a1: 'answer edited in the card' });
     expect(mockSetDraft).toHaveBeenCalledWith({
       id: 'draft-a1',
       value: 'answer edited in the card',
     });
   });
 
+  it("preserves another paused question's answer when a second one is edited", () => {
+    mockAnswerDrafts = { a1: 'answer from A' };
+    const otherAsk = {
+      actionId: 'a2',
+      question: { question: 'Pick one', options: [], multiSelect: false },
+    } as unknown as typeof liveAsk;
+    mockUseGetMessages.mockReturnValue({ data: otherAsk });
+    const { result } = renderHook(() => useAskAnswerMode('conversation-2'));
+
+    act(() => result.current.setAnswerText('answer from B'));
+
+    /** A single shared slot dropped A's unsent answer the moment B claimed
+     *  it, and the action-scoped reader then showed A an empty box. */
+    expect(mockAnswerDrafts).toEqual({ a1: 'answer from A', a2: 'answer from B' });
+  });
+
   it('does not let a delayed answer success clear the composer or selection after navigation', () => {
     let finishAnswer: (() => void) | undefined;
     mockUseGetMessages.mockReturnValue({ data: liveAsk });
diff --git a/client/src/hooks/Input/useAskAnswerMode.ts b/client/src/hooks/Input/useAskAnswerMode.ts
index 230703c431..a090aa8402 100644
--- a/client/src/hooks/Input/useAskAnswerMode.ts
+++ b/client/src/hooks/Input/useAskAnswerMode.ts
@@ -40,10 +40,16 @@ const askAnswerCheckedAtom = atom<number[]>({
   default: [],
 });
 
-/** Free-form answer handed between the composer and the in-message card. */
-const askAnswerTextAtom = atom<{ actionId: string | null; text: string }>({
+/**
+ * Free-form answers handed between the composer and the in-message card,
+ * keyed by pending action id. A single `{ actionId, text }` slot lost an
+ * unsent answer as soon as a second paused conversation's question claimed
+ * it: the reader is action-scoped, so returning to the first card showed an
+ * empty box with no route back to the text. Entries are dropped on submit.
+ */
+const askAnswerTextAtom = atom<Record<string, string>>({
   key: 'askAnswerModeText',
-  default: { actionId: null, text: '' },
+  default: {},
 });
 
 /**
@@ -79,7 +85,7 @@ export default function useAskAnswerMode(conversationId?: string | null) {
   const [collapsedIds, setCollapsedIds] = useRecoilState(collapsedAskActionsAtom);
   const [selected, setSelected] = useRecoilState(askAnswerSelectionAtom);
   const [checked, setChecked] = useRecoilState(askAnswerCheckedAtom);
-  const [answerDraft, setAnswerDraft] = useRecoilState(askAnswerTextAtom);
+  const [answerDrafts, setAnswerDrafts] = useRecoilState(askAnswerTextAtom);
   const saveDrafts = useRecoilValue<boolean>(store.saveDrafts);
   const { submitAskAnswer } = useResumeSubmit();
   /** Recoil-backed so the lock/status works from the composer, which renders
@@ -154,11 +160,11 @@ export default function useAskAnswerMode(conversationId?: string | null) {
     () => splitOtherOption(batchMode ? undefined : liveAsk?.question.options),
     [batchMode, liveAsk],
   );
-  const answerText = answerDraft.actionId === liveAsk?.actionId ? answerDraft.text : '';
+  const answerText = liveAsk != null ? (answerDrafts[liveAsk.actionId] ?? '') : '';
   const setAnswerText = useCallback(
     (text: string) => {
       if (liveAsk && !batchMode) {
-        setAnswerDraft({ actionId: liveAsk.actionId, text });
+        setAnswerDrafts((current) => ({ ...current, [liveAsk.actionId]: text }));
         /** While the card owns the answer, `useAutoSave` is tracking the
          *  conversation draft instead. Keep the dormant ask draft current so
          *  expanding can restore this edit without clobbering that message. */
@@ -167,7 +173,7 @@ export default function useAskAnswerMode(conversationId?: string | null) {
         }
       }
     },
-    [batchMode, liveAsk, saveDrafts, setAnswerDraft],
+    [batchMode, liveAsk, saveDrafts, setAnswerDrafts],
   );
 
   /** Selection state is per-question: a new pause must never inherit a stale
@@ -187,7 +193,7 @@ export default function useAskAnswerMode(conversationId?: string | null) {
       const composerAnswer = !batchMode ? (formContext?.getValues('text') ?? answerText) : '';
       morphTransition(() => {
         if (!batchMode) {
-          setAnswerDraft({ actionId: liveAsk.actionId, text: composerAnswer });
+          setAnswerDrafts((current) => ({ ...current, [liveAsk.actionId]: composerAnswer }));
           if (!saveDrafts) {
             formContext?.reset();
           }
@@ -197,7 +203,7 @@ export default function useAskAnswerMode(conversationId?: string | null) {
         );
       });
     }
-  }, [liveAsk, batchMode, formContext, answerText, saveDrafts, setAnswerDraft, setCollapsedIds]);
+  }, [liveAsk, batchMode, formContext, answerText, saveDrafts, setAnswerDrafts, setCollapsedIds]);
 
   const expand = useCallback(() => {
     if (liveAsk) {
@@ -265,11 +271,16 @@ export default function useAskAnswerMode(conversationId?: string | null) {
           }
           setSelected(null);
           setChecked([]);
-          setAnswerDraft((current) =>
-            current.actionId === submittedActionId
-              ? { actionId: submittedActionId, text: '' }
-              : current,
-          );
+          /** Drop only the answered question's entry, so a draft belonging to
+           *  another paused conversation survives and the map stays bounded. */
+          setAnswerDrafts((current) => {
+            if (current[submittedActionId] == null) {
+              return current;
+            }
+            const next = { ...current };
+            delete next[submittedActionId];
+            return next;
+          });
           if (
             (consumedComposerText || (wasActive && saveDrafts)) &&
             currentScope.formContext?.getValues('text') === submittedComposerText
@@ -290,7 +301,7 @@ export default function useAskAnswerMode(conversationId?: string | null) {
       submitAskAnswer,
       setSelected,
       setChecked,
-      setAnswerDraft,
+      setAnswerDrafts,
     ],
   );