fix toolConfirmation icon and completion status #264073
Conversation
* `getIcon` did not account for `ToolConfirmKind.Setting` (e.g. auto-approve) * `completion` promise was being changed to `Canceled` despite it already being resolved. See my [verbose(!) debugging comment](microsoft#264023 (comment)) which walks you through states of `ConfirmationKind` in `getIcon` under different commits. Fixes microsoft#264023. Relates microsoft#263314 (`getIcon` added by @connor4312). Relates microsoft#262982 (`ConfirmationKind.UserSetting` added by @Tyriar). Reviewers: I'm not 100% on what the expected UI iconography experience is, so please verify that the icons align with what y'all have in mind. Suggested follow ups: - [ ] Tests - especially one around `getIcon` to ensure it handles `ConfirmationKind` exhaustively as folks add new kinds.
|
@Tyriar - Hmmm… if I say: …and then click the Cancel button (next to the chat box) it's showing up as a checkmark next to the command. is that expected, or which icon are we expecting there? |
it looks like that's the behavior even with this (and my other commit reverted). |
That sounds wrong. It should only ever get a tick if the command runs to completion |
|
Closing this for now — there's still some edge cases not working (e.g. see above). I'll revert my commit to ensure it's not in Monday's build and then work more on it next week. |
3 participants
getIcondid not account forToolConfirmKind.Setting(e.g. auto-approve)completionpromise was being changed toCanceleddespite it already being resolved.See my verbose(!) debugging comment
which walks you through states of
ConfirmationKindin
getIconunder different commits.Fixes #264023.
Relates #263314 (
getIconadded by @connor4312).Relates #262982 (
ConfirmationKind.UserSettingadded by @Tyriar).Supercedes #264047 (revert of a commit that changed confirmation logic from sync -> async by @rwoll).
Reviewers: I'm not 100% on what the expected UI iconography experience is, so
please verify that the icons align with what y'all have in mind.
Suggested follow ups:
getIconto ensure it handlesConfirmationKindexhaustively as folks add new kinds.