chore(compass-assistant): add confirmation step to explain plan entrypoint COMPASS-9836 #7326
Conversation
| callback(); | ||
|
|
||
| if (chat.status === 'streaming') { | ||
| await chat.stop(); |
There was a problem hiding this comment.
this does mean entry points will interupt current stream. imo this is the best way to deal with it. If someone is using an entry point they probably do not care about the current chat anymore.
There was a problem hiding this comment.
Pull Request Overview
Adds a confirmation step to the explain plan entrypoint and refactors message handling to support confirmation metadata in the Compass Assistant feature.
- Introduces confirmation metadata structure to assistant messages with pending/confirmed/rejected states
- Adds UI components for displaying and handling confirmation prompts
- Updates the explain plan prompt to require user confirmation before sending data
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-assistant/src/prompts.ts | Refactors EntryPointMessage type and adds confirmation metadata to explain plan prompt |
| packages/compass-assistant/src/docs-provider-transport.ts | Updates transport to filter out messages with confirmation metadata and use model injection |
| packages/compass-assistant/src/docs-provider-transport.spec.ts | Adds comprehensive tests for message filtering and transport behavior |
| packages/compass-assistant/src/components/confirmation-message.tsx | Creates new UI component for displaying confirmation prompts |
| packages/compass-assistant/src/components/confirmation-message.spec.tsx | Tests for the confirmation message component |
| packages/compass-assistant/src/components/assistant-chat.tsx | Updates chat component to render confirmation messages and handle user interactions |
| packages/compass-assistant/src/components/assistant-chat.spec.tsx | Adds tests for confirmation message handling in the chat interface |
| packages/compass-assistant/src/compass-assistant-provider.tsx | Updates provider to support confirmation metadata and model injection |
| packages/compass-assistant/src/compass-assistant-drawer.tsx | Updates import path for assistant chat component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
gagik
force-pushed
the
gagik/confirmation
branch
from
899e732 to
d5493be
Compare
| }); | ||
| if (newState === 'confirmed') { | ||
| // Force the new message request to be sent | ||
| void ensureOptInAndSend?.(undefined, {}, () => {}); |
There was a problem hiding this comment.
This kinda threw me - the message parameter is optional? And I realise that the message isn't actually new - it just don't have this confirmed prop which means it will now be sent for the first time. Which is a bit nitpicky of me regarding the comment.
I think we should maybe make ensureOptInAndSend's callback parameter optional if we're not going to track anything in some cases like this. But shouldn't we be tracking something? We're missing out on Assistant Prompt Submitted in this path, right? Or am I mistaken?
There was a problem hiding this comment.
we are though I'm unsure if we want to have this, it depends on if we consider an entry point a submitted prompt or if it's just user ones
There was a problem hiding this comment.
I suppose we track the entrypoints used. But should we track the ones that make it past confirmation? I imagine that drop-off would be useful stats to have.
packages/compass-assistant/src/components/confirmation-message.spec.tsx
Outdated
Show resolved
Hide resolved
packages/compass-assistant/src/components/confirmation-message.tsx
Outdated
Show resolved
Hide resolved
2 participants
Adds confirmation step to assistant explain entry point