Consolidate PopupQuestion and AskYesNo into one question#2194
Open
GeorgeNgMsft wants to merge 4 commits intomainfrom
Open
Consolidate PopupQuestion and AskYesNo into one question#2194GeorgeNgMsft wants to merge 4 commits intomainfrom
GeorgeNgMsft wants to merge 4 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR consolidates the previous askYesNo and popupQuestion interaction flows into a single question interaction that returns a numeric choice index, while preserving higher-level ergonomic wrappers.
Changes:
- Replaces
askYesNo/popupQuestioninClientIOand pending-interaction types with a unifiedquestion(...) => Promise<number>API. - Updates dispatcher/server/client RPC wiring and interaction management to use
question+defaultId. - Updates CLI/Shell implementations and tests to render/handle unified question prompts and responses.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/packages/shell/src/renderer/src/main.ts | Replaces renderer-side askYesNo/popupQuestion stubs with unified question handler. |
| ts/packages/shell/src/main/instance.ts | Main process intercept now implements question via dialog.showMessageBox. |
| ts/packages/dispatcher/types/src/pendingInteraction.ts | Unifies pending interaction union into question + proposeAction. |
| ts/packages/dispatcher/types/src/displayLogEntry.ts | Aligns pending-interaction log entry fields with unified question shape. |
| ts/packages/dispatcher/types/src/clientIO.ts | Replaces askYesNo/popupQuestion with question(requestId?, message, choices, defaultId?, source?). |
| ts/packages/dispatcher/rpc/src/clientIOTypes.ts | Updates RPC invoke surface to question and removes popup/yesno calls. |
| ts/packages/dispatcher/rpc/src/clientIOServer.ts | RPC server delegates question to clientIO.question. |
| ts/packages/dispatcher/rpc/src/clientIOClient.ts | RPC client invokes question instead of askYesNo/popupQuestion. |
| ts/packages/dispatcher/dispatcher/src/context/pendingInteractionManager.ts | Stores defaultId for question cancellation/timeout behavior. |
| ts/packages/dispatcher/dispatcher/src/context/interactiveIO.ts | askYesNoWithContext now calls clientIO.question and maps index↔boolean. |
| ts/packages/dispatcher/dispatcher/src/execute/sessionContext.ts | popupQuestion wrapper now delegates to question. |
| ts/packages/dispatcher/dispatcher/src/displayLog.ts | logPendingInteraction updated to log unified question fields. |
| ts/packages/cli/src/enhancedConsole.ts | CLI prompt parsing/rendering now uses unified question handling and index responses. |
| ts/packages/agentServer/server/src/sharedDispatcher.ts | Server now creates and broadcasts question pending interactions. |
| ts/packages/agentServer/docs/multi-client-interactions.md | Documentation updated to describe the unified question interaction type. |
| Various test files | Tests updated to use question requests/responses and numeric indices. |
Comments suppressed due to low confidence (1)
ts/packages/cli/src/enhancedConsole.ts:1
- The fallback branch formats any non-numeric
entry.responseas a yes/no (truthy -> yes), which will misrepresentproposeActionresponses (typically objects) as "yes". Change the fallback to stringify non-question responses (e.g.,String(entry.response)), and only apply yes/no formatting when you can positively identify a binary question (e.g.,pending.choicesequals[\"Yes\",\"No\"]and response is numeric).
// Copyright (c) Microsoft Corporation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ts/packages/dispatcher/dispatcher/src/execute/sessionContext.ts
Outdated
Show resolved
Hide resolved
ts/packages/dispatcher/dispatcher/src/context/pendingInteractionManager.ts
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify client interaction API: unify askYesNo + popupQuestion into question
Summary
Merges two overlapping client interaction methods — askYesNo (boolean yes/no prompt tied to a request) and popupQuestion (multi-choice popup outside a request) — into a single unified question(requestId, message, choices, defaultId?, source?) method.
question always returns a choice index; callers that need a boolean use thin askYesNo()/popupQuestion() wrappers on SessionContext that convert to/from index.
requestId is now RequestId | undefined, allowing questions to be asked both inside and outside a request context without a separate method.
PendingInteractionType shrinks from three variants (askYesNo, proposeAction, popupQuestion) to two (question, proposeAction), simplifying the discriminated union used across the RPC layer.
Updates all callers: CLI enhanced console, shell renderer, dispatcher command server, shared dispatcher, service worker connection, onboarding test handler, and related tests/docs.
Fixes a regression where the question prompt was not appearing in the shell.