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.

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>

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>
             }
           />
         )}

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>

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 ?? []));
   }

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 });

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,
     ],
   );