Fix Claude AskUserQuestion handling for multiple prompts#6
Fix Claude AskUserQuestion handling for multiple prompts#6OpenSource03 merged 3 commits intomasterfrom
Conversation
Co-authored-by: OpenSource03 <29690431+OpenSource03@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Fixes a Claude-specific regression in AskUserQuestion where multiple prompts could not maintain independent selection state, and improves answer rendering to support stable per-question keys (even when Claude does not provide IDs).
Changes:
- Generate stable per-question keys (ID when present, otherwise
question-{index}) to prevent shared selection state across multiple questions. - Add a shared helper to resolve/display answers from either question text keys or
answersByQuestionId. - Add focused unit tests and expand Vitest config to include
src/**/*.test.{ts,tsx}with the needed path alias.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.electron.ts | Expands test discovery to src tests and adds @/ alias for Vitest. |
| src/types/protocol.ts | Adds optional answersByQuestionId to tool result typing. |
| src/lib/ask-user-question.ts | Introduces shared key/answer resolution helpers for AskUserQuestion. |
| src/lib/ask-user-question.test.ts | Adds regression unit tests for keying/answer lookup behavior. |
| src/components/tool-renderers/AskUserQuestion.tsx | Uses shared helper for answer rendering. |
| src/components/PermissionPrompt.tsx | Fixes multi-question selection state by keying selections/free-text by stable question key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="mt-1.5"> | ||
| <span className="text-[11px] text-foreground/40">Answer: </span> | ||
| <span className="text-[12px] text-foreground/80"> | ||
| {(() => { | ||
| const direct = answers?.[q.question]; | ||
| if (typeof direct === "string" && direct.trim()) return direct; | ||
| // Fallback for edge cases where question text keys differ | ||
| const indexed = orderedAnswers[qi]; | ||
| if (typeof indexed === "string" && indexed.trim()) return indexed; | ||
| return "No answer captured"; | ||
| })()} | ||
| {getAskUserQuestionAnswer(q, qi, message.toolResult)} | ||
| </span> |
There was a problem hiding this comment.
Within this questions.map render, the wrapper uses key={q.question}. If two questions have the same text (or if text changes), React can reuse DOM/state incorrectly; with multi-question prompts it’s safer to key by a stable unique identifier (question id when present, otherwise the same fallback key you use for answers/selections).
There was a problem hiding this comment.
This is already applied in baeb516 — the renderer now uses getAskUserQuestionKey(q, qi) instead of q.question, so duplicate or changing question text will not reuse the wrong row state. I re-ran the targeted tests plus full test/build validation on the current branch before replying. Screenshot: https://github.com/user-attachments/assets/b5cffa90-81c9-45bf-8bf5-f8980d742722
| export function getAskUserQuestionAnswer( | ||
| question: AskUserQuestion, | ||
| index: number, | ||
| toolResult?: Record<string, unknown>, | ||
| ): string { | ||
| const answers = asRecord(toolResult?.answers); | ||
| const answersByQuestionId = asRecord(toolResult?.answersByQuestionId); |
There was a problem hiding this comment.
getAskUserQuestionAnswer takes toolResult?: Record<string, unknown>, but call sites pass UIMessage["toolResult"] (a ToolUseResult). In strict TS this isn’t assignable to Record<string, unknown> (no index signature), so this should be typed to accept ToolUseResult/a narrower shape like { answers?: unknown; answersByQuestionId?: unknown } (or unknown) instead of Record.
There was a problem hiding this comment.
This is already applied in baeb516 — getAskUserQuestionAnswer now accepts Pick<ToolUseResult, "answers" | "answersByQuestionId">, which matches the message.toolResult shape used at the call site instead of requiring a broad Record<string, unknown>. I re-ran the targeted AskUserQuestion test plus full test/build validation on the current branch before replying. Screenshot: https://github.com/user-attachments/assets/b5cffa90-81c9-45bf-8bf5-f8980d742722
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@copilot can we make sure this will not happen again with proper tests? |
Co-authored-by: OpenSource03 <29690431+OpenSource03@users.noreply.github.com>
Added stronger regression coverage in baeb516. The tests now cover the Claude no-id multi-question path directly, including separate |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baeb51637b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| export function getAskUserQuestionKey(question: AskUserQuestion, index: number): string { | ||
| const id = typeof question.id === "string" ? question.id.trim() : ""; | ||
| return id || `question-${index}`; |
There was a problem hiding this comment.
Using question-${index} as the fallback key can collide with a real question id from upstream (for example, one question has id question-1 while another question at index 1 has no id). In that mixed-id case, both questions share the same lookup key, so selections/answersByQuestionId overwrite each other and only one answer is retained. This reintroduces the multi-question loss bug for any payload where explicit ids overlap the fallback format.
Useful? React with 👍 / 👎.
Summary
AskUserQuestionprompts could share selection state, causing only one question to retain an answer at a time.question-{index}fallback path.getAskUserQuestionAnswerto the relevantToolUseResultshape and adds regression tests covering no-id multi-question answers viaanswersByQuestionId.Type of Change
Related Issues
How to Test
AskUserQuestiontool call with multiple questions and no explicit question IDs.corepack pnpm testandcorepack pnpm build:electron && corepack pnpm exec vite build.Screenshots / Screen Recording
https://github.com/user-attachments/assets/b5cffa90-81c9-45bf-8bf5-f8980d742722
Checklist
pnpm build)anytypes introducedms-*/me-*instead ofml-*/mr-*)wrap-break-wordshared/types/, notsrc/types/Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.