QOL: Keep expired AskUserQuestion visible and send answers as messages

[jjjrmy]

Summary

• When the AI asks questions via AskUserQuestion and the 60-second backend timeout expires, the question dialog now stays visible instead of disappearing
• If the user answers after the timeout, their response is sent as a normal user message (since the tool approval has already resolved)
• Dismissing or pressing ESC on an expired question simply clears the UI without making a backend call

How it works

• Introduced expiredUserQuestionsAtom — when a question times out, it moves from pendingUserQuestionsAtom to this new atom instead of being discarded
• The UI renders from displayQuestions = pending ?? expired, so expired questions remain visible
• Answer/skip handlers branch on whether the question is live or expired:
  • Live: existing respondToolApproval path
  • Expired: formats answers as text and sends via sendMessage
• Expired questions are cleared when the user answers, dismisses, sends a new message, or a new question arrives

Test plan

• Trigger AskUserQuestion, wait 60s — question stays visible
• Answer after timeout — response sent as a user message, AI processes it
• Press ESC or "Skip All" after timeout — dialog clears without errors
• Type custom text + answer expired question — combined response sent as message
• New question replaces an expired one correctly
• Normal question flow (answer within 60s) still works as before

Generated with Claude Code

[Danverr]

@1code review this pr

[1code-async]

Task completed.

Now I have a complete picture of the changes. Let me write up my review.

---

PR Review: QOL: Keep expired AskUserQuestion visible and send answers as messages

Summary

This PR improves UX by keeping the AskUserQuestion dialog visible after the 60-second backend timeout expires, allowing users to still answer and have their responses sent as regular user messages.

Architecture Assessment ✅

The approach is sound:

1. New atom (expiredUserQuestionsAtom) — Clean separation between live/pending questions and expired ones
2. Transport layer handles state transitions — Timeout moves question from pending → expired instead of deleting
3. UI derives display state — displayQuestions = pending ?? expired is elegant
4. Branching logic — Handlers correctly branch on isQuestionExpired to use tool approval vs. user message

Code Quality

Strengths:

• Clear variable naming (isQuestionExpired, displayQuestions)
• Good defensive coding (checking existence before map operations)
• Appropriate comments explaining the "why" (e.g., ipc-chat-transport.ts:316-319)

Issues:

1. Unused prop isExpired in agent-user-question.tsx:27

   The prop is added to the interface and destructured but never used in the component. This should either:

   • Be used to show visual feedback (e.g., "Question timed out" badge, different styling)
   • Be removed if no visual distinction is planned

   isExpired = false,  // Destructured but never referenced

2. Duplicated code in handleSubmitWithQuestionAnswer (active-chat.tsx:2726-2771)

   The expired and live branches share significant duplicate logic (clearing input, sending message). Consider extracting shared logic:

   // Both branches do this:
   editorRef.current?.clear()
   if (parentChatId) {
     clearSubChatDraft(parentChatId, subChatId)
   }
   shouldAutoScrollRef.current = true
   await sendMessageRef.current({ role: "user", parts: [{ type: "text", text: ... }] })

3. Minor: Inconsistent answer formatting

   In handleQuestionsAnswer for expired questions (active-chat.tsx:2659-2662):

   const answerLines = Object.entries(answers)
     .map(([question, answer]) => `${question}: ${answer}`)
     .join("\n")

   But in handleSubmitWithQuestionAnswer (active-chat.tsx:2746-2748), the same format is used. If this format matters for AI parsing, consider extracting to a helper function for consistency.

4. Dependency array correctness (active-chat.tsx:2773)

   The handleSubmitWithQuestionAnswer callback has pendingQuestions.toolUseId in its dependency array (from the original code), but the function now uses displayQuestions.toolUseId. This dependency reference should be updated but it's inherited from the original code. Worth verifying it's correct.

Edge Cases to Consider

1. Race condition: If a new question arrives exactly as the user clicks "Submit" on an expired question, the clearPendingQuestionCallback clears both. This seems intentional but worth verifying.

2. Multiple expired questions: The design supports only one expired question per subChat (Map keyed by subChatId). If two questions time out in sequence, only the latest persists. This is probably fine but should be documented.

Test Plan Validation

The test plan in the PR description covers the key scenarios. I'd add:

• Verify no console errors when dismissing expired questions
• Verify the AI properly processes the formatted answer text

Verdict

Approve with minor changes:

1. Either use isExpired prop to show visual feedback (recommended for better UX) or remove it
2. Consider extracting duplicated code in handleSubmitWithQuestionAnswer

The core logic is correct and the feature will meaningfully improve user experience.

---

View full conversation

Powered by 1Code

[jjjrmy]

Addressed in latest commit (refactor: extract shared helpers for expired question handlers):

1. Unused isExpired prop — Removed entirely. The user explicitly requested no visual distinction between live and expired questions (the dialog should look identical), so the prop had no remaining purpose. The isQuestionExpired flag still exists in active-chat.tsx where it's needed to branch handler logic between tool-approval and send-as-message paths.

2. Duplicated code in handleSubmitWithQuestionAnswer — Extracted three shared helpers used across both handleQuestionsAnswer and handleSubmitWithQuestionAnswer:

• formatAnswersAsText — converts answer records to "Question: Answer\n..." text format (was inlined in two places)
• clearInputAndDraft — clears editor + localStorage draft (was duplicated across both branches)
• sendUserMessage — sets auto-scroll flag + sends user message (was duplicated in three places)

3. Dependency array correctness — Verified that handleSubmitWithQuestionAnswer's dependency array already correctly references displayQuestions (not the old pendingQuestions). Updated the array to include the new extracted helpers.

[jjjrmy]

@1code review this pr again

[1code-async]

@jjjrmy Sorry, only collaborators with write access can trigger @1code.

[Danverr]

@1code review this PR again

[1code-async]

Task completed.

Now I have a good understanding of the existing code and the PR changes. Let me write my review.

PR Review: QOL: Keep expired AskUserQuestion visible and send answers as messages

Summary

This PR improves UX by keeping the question dialog visible after the 60-second backend timeout expires, allowing users to still respond (as a normal message) rather than having the dialog disappear unexpectedly.

What's Good ✓

1. Clean state management: The introduction of expiredUserQuestionsAtom is a sensible approach - it keeps expired questions separate from live ones, making the logic clear.

2. Proper state transitions: When a new question arrives, expired questions are correctly cleared (lines 234-240 in ipc-chat-transport.ts). This prevents stale UI.

3. Good extraction of helpers: The refactored helpers (formatAnswersAsText, clearInputAndDraft, sendUserMessage) reduce duplication and improve readability.

4. Correct handling of edge cases:

   • ESC/Skip on expired questions just clears UI (no failed backend calls)
   • New user messages clear expired questions
   • New questions replace expired ones

5. Comment explaining non-obvious behavior (lines 316-319 in ipc-chat-transport.ts): Good documentation about why expired questions aren't cleared when the agent continues emitting chunks.

Issues to Address

1. Dependency array has unnecessary subChatId

In handleSubmitWithQuestionAnswer (around line 2782), the dependency array includes subChatId:

[displayQuestions, isQuestionExpired, clearPendingQuestionCallback, clearInputAndDraft, sendUserMessage, formatAnswersAsText, subChatId]

But subChatId isn't used directly in the callback body - it's only used indirectly through clearInputAndDraft (which already has it in its own deps) and for agentChatStore.setManuallyAborted. The latter is only in the "live" branch where streaming might be active.

This isn't a bug, but the subChatId dep is already captured by clearInputAndDraft. Consider if it's intentional to also track it separately.

2. Potential race condition consideration

If the user submits an answer at nearly the same instant the timeout fires, there could be a brief window where:

1. User clicks "Answer" → handleQuestionsAnswer starts with isQuestionExpired = false
2. Timeout fires → question moves to expired
3. respondToolApproval is called but the backend has already timed out

This is an edge case the existing code already had, and the PR doesn't make it worse. The backend should handle stale tool approvals gracefully. Just worth noting.

3. Minor: clearInputAndDraft missing from handleSubmit dependencies

Looking at the handleSubmit callback (line 3561), setExpiredQuestionsMap was added to deps but the function also calls similar clearing logic inline. This is fine since handleSubmit doesn't use the helper, but could be further unified in a future refactor.

Code Quality

• No leftover isExpired prop: Confirmed removed as mentioned in the PR discussion ✓
• Shared helpers are pure: formatAnswersAsText is a pure function with stable reference via useCallback([], []) ✓
• Correct use of displayQuestions: All references to pendingQuestions in the render logic have been updated to displayQuestions ✓

Testing Suggestions

The test plan in the PR description is solid. Additionally:

• Test rapid timeout + answer scenario (submit answer as timeout is about to fire)
• Test multiple sub-chats: ensure expired questions in one sub-chat don't affect another

Verdict

Approve with minor suggestions. The implementation is clean and handles the UX improvement well. The code correctly distinguishes between live and expired states and routes them appropriately.

---

View full conversation

Powered by 1Code

[jjjrmy]

included in v0.0.50