-
Notifications
You must be signed in to change notification settings - Fork 3
Bug/ap 24779 agent chat view doesnt respond warning #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -59,7 +59,7 @@ | |||||||||
| import knime.extension as knext | ||||||||||
| from langchain_core.messages.human import HumanMessage | ||||||||||
|
|
||||||||||
| from typing import TYPE_CHECKING, Optional | ||||||||||
| from typing import TYPE_CHECKING, Optional, Union | ||||||||||
|
|
||||||||||
| if TYPE_CHECKING: | ||||||||||
| from .base import AgentPrompterConversation | ||||||||||
|
|
@@ -87,17 +87,21 @@ def __init__( | |||||||||
| widget_config: AgentChatWidgetConfig, | ||||||||||
| tool_converter: LangchainToolConverter, | ||||||||||
| combined_tools_workflow_info: dict, | ||||||||||
| startup_error: Optional[str] = None, | ||||||||||
| ): | ||||||||||
| self._ctx = ctx | ||||||||||
|
|
||||||||||
| self._chat_model = chat_model | ||||||||||
| self._startup_error = startup_error | ||||||||||
| self._conversation = FrontendConversation( | ||||||||||
| conversation, tool_converter, self._check_canceled | ||||||||||
| ) | ||||||||||
| self._agent_config = agent_config | ||||||||||
| self._agent = Agent( | ||||||||||
| self._conversation, self._chat_model, toolset, self._agent_config | ||||||||||
| ) | ||||||||||
| self._agent = None | ||||||||||
| if self._chat_model is not None: | ||||||||||
| self._agent = Agent( | ||||||||||
| self._conversation, self._chat_model, toolset, self._agent_config | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| self._data_registry = data_registry | ||||||||||
| self._widget_config = widget_config | ||||||||||
|
|
@@ -108,6 +112,9 @@ def __init__( | |||||||||
| self._thread = None | ||||||||||
| self._is_canceled = False | ||||||||||
|
|
||||||||||
| if self._startup_error: | ||||||||||
| self._conversation.append_warning_to_frontend(self._startup_error) | ||||||||||
|
|
||||||||||
| def get_initial_message(self): | ||||||||||
| if self._widget_config.initial_message: | ||||||||||
| return { | ||||||||||
|
|
@@ -179,10 +186,20 @@ def get_view_data(self): | |||||||||
| def _post_user_message(self, user_message: str): | ||||||||||
| from langchain_core.messages import AIMessage | ||||||||||
|
|
||||||||||
| # append user message before checking for start-up error to mirror UI | ||||||||||
| self._conversation.append_messages_to_backend( | ||||||||||
| HumanMessage(content=user_message) | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| if self._agent is None: | ||||||||||
| self._conversation.append_error( | ||||||||||
| Exception( | ||||||||||
| self._startup_error | ||||||||||
| or "The selected model is not available." | ||||||||||
| ) | ||||||||||
| ) | ||||||||||
| return | ||||||||||
|
|
||||||||||
| try: | ||||||||||
| self._agent.run() | ||||||||||
|
|
||||||||||
|
|
@@ -263,10 +280,9 @@ def _append_messages(self, messages): | |||||||||
| """Appends messages to both backend and frontend without checking for cancellation.""" | ||||||||||
| from langchain_core.messages import HumanMessage | ||||||||||
|
|
||||||||||
| # will not raise since backend has no context | ||||||||||
| self._backend_messages.append_messages(messages) | ||||||||||
|
|
||||||||||
| for new_message in messages: | ||||||||||
| self._backend_messages._append(new_message) # _backend_messages.append_messages does validation | ||||||||||
|
|
||||||||||
| if isinstance(new_message, HumanMessage): | ||||||||||
| continue | ||||||||||
|
|
||||||||||
|
|
@@ -276,8 +292,13 @@ def _append_messages(self, messages): | |||||||||
|
|
||||||||||
| def append_messages_to_backend(self, messages): | ||||||||||
| """Appends messages only to the backend conversation.""" | ||||||||||
| # will not raise since backend has no context | ||||||||||
| self._backend_messages.append_messages(messages) | ||||||||||
| from langchain_core.messages import BaseMessage | ||||||||||
|
|
||||||||||
| if isinstance(messages, BaseMessage): | ||||||||||
| messages = [messages] | ||||||||||
|
|
||||||||||
| for msg in messages: | ||||||||||
| self._backend_messages._append(msg) # _backend_messages.append_messages does validation | ||||||||||
|
Comment on lines
+300
to
+301
|
||||||||||
| for msg in messages: | |
| self._backend_messages._append(msg) # _backend_messages.append_messages does validation | |
| # Use the backend's public API for appending messages instead of its private `_append` method. | |
| self._backend_messages.append_messages(messages) |
Copilot
AI
Mar 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append_warning_to_frontend enqueues a warning dict without an id. In the frontend, WarningMessage extends BaseMessage and requires id: string (and several tests/matchers assume an id is present). Consider generating a stable unique id for warnings (e.g., uuid/monotonic counter) and including it in the payload to keep the message shape consistent with other message types.
This file was deleted.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| <script setup lang="ts"> | ||
| import AbortIcon from "@knime/styles/img/icons/close.svg"; | ||
|
|
||
| import type { WarningMessage } from "@/types"; | ||
|
|
||
| defineProps<{ warning: WarningMessage }>(); | ||
|
|
||
| defineEmits<{ (event: "dismiss"): void }>(); | ||
| </script> | ||
|
|
||
| <template> | ||
| <div class="warning-banner"> | ||
| <span class="message">{{ warning.content }}</span> | ||
| <button | ||
| class="dismiss-button" | ||
| type="button" | ||
| aria-label="Dismiss warning" | ||
| @click="$emit('dismiss')" | ||
| > | ||
| <AbortIcon aria-hidden="true" focusable="false" /> | ||
| </button> | ||
| </div> | ||
| </template> | ||
|
|
||
| <style lang="postcss" scoped> | ||
| @import url("@knime/styles/css/mixins"); | ||
|
|
||
| .warning-banner { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| gap: var(--space-12); | ||
| margin: var(--space-8) 0 var(--space-16); | ||
| padding: var(--space-12) var(--space-16); | ||
| border-radius: var(--space-8); | ||
| border: 1px solid var(--knime-yellow); | ||
| background-color: var(--knime-yellow-ultra-light); | ||
| color: var(--knime-masala); | ||
| font-size: 13px; | ||
| } | ||
|
|
||
| .message { | ||
| flex: 1; | ||
| } | ||
|
|
||
| .dismiss-button { | ||
| border: none; | ||
| background: transparent; | ||
| color: inherit; | ||
| cursor: pointer; | ||
| padding: var(--space-4) var(--space-8); | ||
| border-radius: 9999px; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .dismiss-button:hover { | ||
| background-color: var(--knime-yellow); | ||
| } | ||
|
|
||
| .dismiss-button :deep(svg) { | ||
| stroke: var(--knime-masala); | ||
|
|
||
| @mixin svg-icon-size 16; | ||
| } | ||
| </style> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { mount } from "@vue/test-utils"; | ||
|
|
||
| import WarningBanner from "../WarningBanner.vue"; | ||
|
|
||
| const createWrapper = (content = "Warning text") => | ||
| mount(WarningBanner, { | ||
| props: { | ||
| warning: { | ||
| id: "warning-1", | ||
| type: "warning", | ||
| content, | ||
| }, | ||
| }, | ||
| global: { | ||
| stubs: { | ||
| AbortIcon: { template: "<svg />" }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| describe("WarningBanner", () => { | ||
| it("renders warning content", () => { | ||
| const wrapper = createWrapper("Heads up"); | ||
|
|
||
| expect(wrapper.text()).toContain("Heads up"); | ||
| }); | ||
|
|
||
| it("emits dismiss when dismiss button is clicked", async () => { | ||
| const wrapper = createWrapper(); | ||
|
|
||
| await wrapper.find(".dismiss-button").trigger("click"); | ||
|
|
||
| expect(wrapper.emitted("dismiss")).toHaveLength(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FrontendConversation._append_messagescalls the privateAgentPrompterConversation._append(...)to bypass validation. This works today, but it tightly couples the data service to a private method and makes future refactors ofAgentPrompterConversationriskier. Consider adding an explicit public API onAgentPrompterConversationfor appending without validation (or a flag onappend_messages) and use that instead of reaching into_append.