AP-25051: Add cancel button to Agent Chat Widget#19
Conversation
| aria-hidden="true" | ||
| focusable="false" | ||
| /> | ||
| </FunctionButton> |
There was a problem hiding this comment.
We could also separate this into two buttons.
|
|
||
| function finishLoading(shallApplyViewData: boolean) { | ||
| isLoading.value = false; | ||
| isInterrupted.value = false; |
There was a problem hiding this comment.
My strategy was to check where we reset isLoading and to also reset isInterrupted there. But we should double-check if this covers all cases.
AtR1an
force-pushed
the
bug/AP-25110-implement-error-handling-option
branch
2 times, most recently
from
ad00fad to
0ffb5c6
Compare
iusethemouse
requested review from
iusethemouse
and removed request for
a team
ya-hn
force-pushed
the
enh/AP-25051-agent-chat-widget-cancel-button
branch
from
a815361 to
1398cd2
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds cancel functionality to the Agent Chat Widget, allowing users to interrupt agent responses in progress. The implementation introduces a new isInterrupted state to track cancellation and updates the UI to show an abort button when the agent is processing.
Changes:
- Added
cancelAgent()method andisInterruptedstate management to the chat store - Updated MessageInput component to toggle between send and abort buttons based on processing state
- Implemented backend
cancel_agent()method to handle cancellation requests
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/agents/chat_app/src/stores/chat.ts | Added isInterrupted state, cancelAgent() method, and reset logic for cancellation tracking |
| src/agents/chat_app/src/components/chat/MessageInput.vue | Updated UI to conditionally show abort/send button and handle cancellation in submit logic |
| src/agents/_data_service.py | Implemented cancel_agent() method and reset _is_canceled flag in appropriate lifecycle points |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stroke: var(--knime-dove-gray); | ||
|
|
||
| &.send-icon { | ||
| & .send-icon .abort-icon { |
There was a problem hiding this comment.
Invalid CSS selector syntax. The selector should use a comma to target both classes independently: &.send-icon, &.abort-icon
| & .send-icon .abort-icon { | |
| & .send-icon, | |
| & .abort-icon { |
| await jsonDataService.value?.data({ | ||
| method: "cancel_agent", | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Unnecessary explicit return statement in async function. The function already returns implicitly.
| return; |
|
|
||
| const isInputValid = computed(() => input.value?.trim().length > 0); | ||
| const isDisabled = computed(() => !isInputValid.value || chatStore.isLoading); | ||
| const isDisabled = computed(() => (!isInputValid.value && !chatStore.isLoading) || chatStore.isInterrupted); |
There was a problem hiding this comment.
The logic for isDisabled is complex and could be clearer. When isLoading is true, the button should be enabled (to allow cancellation), but when isInterrupted is true, it should be disabled. Consider refactoring to: (chatStore.isInterrupted) || (!chatStore.isLoading && !isInputValid.value)
| const isDisabled = computed(() => (!isInputValid.value && !chatStore.isLoading) || chatStore.isInterrupted); | |
| const isDisabled = computed( | |
| () => chatStore.isInterrupted || (!chatStore.isLoading && !isInputValid.value), | |
| ); |
iusethemouse
left a comment
iusethemouse
left a comment
There was a problem hiding this comment.
Very nice 👍 Besides that one code structure comment in the MessageInput component, looks good.
We also have unit tests, which you can run with pnpm run test:unit (yeah, you have to run them manually 😬 , they're not part of the build process yet). Could you please expand test suites for MessageInput.vue and chat.ts? Opus should be very helpful there
| const isInputValid = computed(() => input.value?.trim().length > 0); | ||
| const isDisabled = computed(() => !isInputValid.value || chatStore.isLoading); | ||
| const isDisabled = computed(() => (!isInputValid.value && !chatStore.isLoading) || chatStore.isInterrupted); | ||
| const isProcessing = computed(() => chatStore.isLoading); |
There was a problem hiding this comment.
Just to tidy things up a bit: we generally use computed when we want to combine a few reactive properties (properties that can have their inner .value change during runtime) into a new singular reactive property. So we want a new Prop A which changes whenever either Prop B or Prop C change - then Prop A is our computed prop.
Here, we basically say const myVar = otherVar 😃 Nothing wrong with that, it will work just fine (and I'm pretty sure Vue would cleverly optimise this), but isLoading is already a reactive property (see that it's a ref inside chat.ts), so we don't need to wrap it with a computed unless we want to combine its reactivity with some other reactive property.
There's a nice function in Pinia called storeToRefs, which allows extracting reactive properties from stores and retain their reactivity, and we could use it here, e.g.:
import { storeToRefs } from "pinia"
const { isLoading, isInterrupted } = storeToRefs(chatStore)
and then use those nice reactive properties elsewhere in this component
| aria-hidden="true" | ||
| focusable="false" | ||
| /> | ||
| </FunctionButton> |
2 participants
Adds a cancel functionality to the send button in the Agent Chat Widget node.