feat: add inline question prompt flow for tool calls#44
feat: add inline question prompt flow for tool calls#44tanishqkancharla wants to merge 6 commits into
Conversation
|
Claude finished @tanishqkancharla's task in 2m 35s —— View job PR Review
📋 Detailed ReviewSpec AnalysisBased on
Changed Files
Simplicity AssessmentThe implementation is well-structured with appropriate separation of concerns:
One minor observation: In Correctness Assessment
SummaryThis is a well-implemented feature that adds comprehensive question prompt support to the webview client. The code follows existing patterns for permissions, maintains good separation of concerns between state management, SDK integration, and UI components, and includes thorough test coverage. The accordion UX with auto-advance behavior matches the spec requirements. Required ActionsNone - the implementation is solid and ready for merge. Suggestions
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return { | ||
| ...state, | ||
| answers, | ||
| expandedIndex: shouldAutoAdvance(state.expandedIndex, questionIndex, questionCount), | ||
| }; |
There was a problem hiding this comment.
🔴 Multi-select auto-advance collapses panel after first toggle, preventing multiple selections
toggleMultiAnswer calls shouldAutoAdvance on every toggle, which collapses the current question's panel and advances to the next question after the user selects their first option. For multi-select questions, the user needs to select multiple options before advancing, but the panel auto-closes after the very first toggle. They must re-open the accordion header each time to add another selection.
Code path that causes the issue
At src/webview/components/QuestionPrompt.tsx:87, toggleMultiAnswer passes through to shouldAutoAdvance(state.expandedIndex, questionIndex, questionCount) at line 31, which returns nextIndex (or null for the last question) whenever expandedIndex === answerIndex. This means the panel collapses on the first toggle. For single-select (setSingleAnswer) this is correct, but for multi-select it should keep the panel open until the user explicitly moves on.
| return { | |
| ...state, | |
| answers, | |
| expandedIndex: shouldAutoAdvance(state.expandedIndex, questionIndex, questionCount), | |
| }; | |
| return { | |
| ...state, | |
| answers, | |
| expandedIndex: state.expandedIndex, | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| createEffect(() => { | ||
| const inflight = inFlightMessage(); | ||
| if (!inflight) return; | ||
|
|
||
| for (const [, question] of pendingQuestions().entries()) { | ||
| if (question.sessionID === inflight.sessionId) { | ||
| setInFlightMessage(null); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Queued messages never drain after question-interrupted in-flight session
When a question arrives for an in-flight session, the createEffect at src/webview/App.tsx:474-483 clears inFlightMessage to null. After the question is answered and the backend resumes, a session.idle event eventually fires. However, the onSessionIdle callback at src/webview/App.tsx:455-468 checks inflight?.sessionId !== sessionId — since inflight is already null, this check is true and the callback returns early without calling processNextQueuedMessage(). The queued messages remain stuck indefinitely.
Scenario
- User sends a message →
inFlightMessageset for this session - Backend asks a question →
createEffectclearsinFlightMessageto null - User answers the question → question removed from pending
- Backend resumes, session eventually goes idle →
session.idleevent fires onSessionIdlecallback seesinFlightMessage()is null, returns earlyprocessNextQueuedMessage()is never called → queue is permanently stuck
Prompt for agents
In src/webview/App.tsx, the createEffect at lines 474-483 clears inFlightMessage when a question arrives, but does not arrange for the message queue to drain after the question is resolved. The onSessionIdle callback at lines 455-468 only triggers processNextQueuedMessage when it matches an in-flight message, which is already null by that point.
Fix: When clearing inFlightMessage due to a question arrival, store the sessionId so the onSessionIdle callback can still match it. Alternatively, add a separate createEffect that watches hasPendingQuestions() transitioning from true to false and calls processNextQueuedMessage() at that point. For example:
createEffect(() => {
if (!hasPendingQuestions() && messageQueue().length > 0 && !inFlightMessage()) {
queueMicrotask(() => void processNextQueuedMessage());
}
});
This would need to be placed after the processNextQueuedMessage definition (around line 693) and should be careful not to create infinite loops.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Testing