Skip to content
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

Chat UI Streaming Support #1525

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

TamiTakamiya
Copy link
Contributor

@TamiTakamiya TamiTakamiya commented Feb 7, 2025

Jira Issue: https://issues.redhat.com/browse/AAP-39043

Description

Add streaming support to the Chat UI. Note that this PR contains changes on the UI part only and it requires to have AAP-39044 completed to fully enable the chat streaming on our STAGE/PROD deployments.

Testing

Steps to test

  1. Pull down the PR
  2. Run unit tests

Scenarios tested

Unit tests and local test by connecting UI directly to ansible-chatbot-service.

Production deployment

  • This code change is ready for production on its own
  • This code change requires the following considerations before going to production:

@TamiTakamiya TamiTakamiya force-pushed the TamiTakamiya/AAP-39043/chatbot-streaming-ui branch 3 times, most recently from 2f74307 to 0cfd2ee Compare February 8, 2025 21:26
@TamiTakamiya TamiTakamiya force-pushed the TamiTakamiya/AAP-39043/chatbot-streaming-ui branch from 0cfd2ee to 6fdc215 Compare February 9, 2025 01:28
"@patternfly/chatbot": "^2.2.0-prerelease.17",
"@testing-library/jest-dom": "^5.17.0",
"@testing-library/react": "^13.4.0",
"@testing-library/user-event": "^13.5.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed jest-dom, which is not used, and moved other two test libraries from dependencies to devDependencies.

@@ -23,6 +23,7 @@
</div>
<div id="user_name" hidden>{{user_name}}</div>
<div id="bot_name" hidden>{{bot_name}}</div>
<div id="stream" hidden>{{stream}}</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for telling whether the new streaming chat endpoint, which is not implemented, is to be called.

@@ -374,12 +377,17 @@ export const AnsibleChatbot: React.FunctionComponent = () => {
) : (
<></>
)}
{isStreamingSupported() && (
<div key={`scroll_div_9999`} ref={messagesEndRef} />
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the streaming mode is used, UI will automatically scrolls to the bottom of the last chatbot message as it is being updated. When non-streaming mode is used, UI will show at the top of the last chatbot message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where this key value scroll_div_9999 comes from? Does it have to follow any pattern, matters? Why considering 9999 the latest counter value? Maybe just a quick comment would help, if makes any sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be any number. I just picked up a number, which should be bigger than the number of messages on the UI. On lines 344-370, the code renders messages from the list and index is used to point to each message. I assumed the total # of messages should be less than 9999 and used 9999 to indicate the end of messages.

@@ -152,6 +246,16 @@ test("Basic chatbot interaction", async () => {
.toBeVisible();
await expect.element(view.getByText("Create variables")).toBeVisible();

const copyIcon = await screen.findByRole("button", {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is not related to the streaming support, test cases for the copy button support was added for better code coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @TamiTakamiya

@@ -0,0 +1,2 @@
// https://stackoverflow.com/questions/63329331/mocking-the-navigator-object
export const setClipboard = (s: string) => navigator.clipboard?.writeText(s);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for mocking the clipboard function easier.


export const CHAT_HISTORY_HEADER = "Chat History";

export const INITIAL_NOTICE: AlertMessage = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this constant from useChatbot to here.

@@ -53,6 +59,15 @@ export const inDebugMode = () => {
return import.meta.env.PROD ? debug === "true" : debug !== "false";
};

export const isStreamingSupported = () => {
// For making streaming mode debug easier.
if (!import.meta.env.PROD && import.meta.env.MODE.includes("stream")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for debugging. When the vite server is started with the stream mode, the UI stream support is enabled.

@TamiTakamiya TamiTakamiya marked this pull request as ready for review February 10, 2025 13:40
Copy link
Contributor

@justjais justjais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes LGTM!

@@ -152,6 +246,16 @@ test("Basic chatbot interaction", async () => {
.toBeVisible();
await expect.element(view.getByText("Create variables")).toBeVisible();

const copyIcon = await screen.findByRole("button", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @TamiTakamiya

Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @TamiTakamiya ,

Looks great, thanks for this! Just added an small comment, but not a blocker.

Thanks!

@@ -374,12 +377,17 @@ export const AnsibleChatbot: React.FunctionComponent = () => {
) : (
<></>
)}
{isStreamingSupported() && (
<div key={`scroll_div_9999`} ref={messagesEndRef} />
)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where this key value scroll_div_9999 comes from? Does it have to follow any pattern, matters? Why considering 9999 the latest counter value? Maybe just a quick comment would help, if makes any sense.

@TamiTakamiya TamiTakamiya merged commit dbe41cc into main Feb 10, 2025
11 checks passed
@TamiTakamiya TamiTakamiya deleted the TamiTakamiya/AAP-39043/chatbot-streaming-ui branch February 10, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants