-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat/additional frontend features #72
Conversation
packages/chat-component/src/main.ts
Outdated
@@ -119,6 +133,7 @@ export class ChatComponent extends LitElement { | |||
// non-streamed response | |||
const processedText = processText(message, [citations, followingSteps, followupQuestions]); | |||
message = processedText.replacedText; | |||
console.log(message, '####MESSAGE####'); |
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.
should be removed I guess?
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.
Ooops. We should catch them with the pre-commit hook. We are almost shipping one with every PR. :)
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.
We should add the https://eslint.org/docs/latest/rules/no-console eslint hook then!
@@ -161,6 +176,12 @@ export class ChatComponent extends LitElement { | |||
this.isResetInput = false; | |||
|
|||
const response = this.apiResponse as BotResponse; | |||
// adds thought process support when streaming is disabled |
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.
Why only when it's disabled? It's also available when streaming is enabled on the API (it comes in the first chunk)
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.
The logic is different because when streaming is enabled, data_points and thoughts are part of a different chunk of the streamed response. See line 98 for streamed enabled support.
|
||
for await (const chunk of chunks) { | ||
// eslint-disable-next-line no-prototype-builtins |
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.
we should probably disable this rule, makes no sense to me
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.
Agreed.
@@ -110,8 +117,7 @@ export async function parseStreamedMessages({ | |||
|
|||
visit(); | |||
} | |||
|
|||
return streamedMessageRaw.join(''); | |||
return streamedMessageRaw.join(''), thoughtProcess; |
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.
I'm not what's the intent here? I you want to return both it should be in an array [streamedMessageRaw.join(''), thoughtProcess]
, as currently only thoughtProcess
is returned?
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.
Yep, good catch. Initially I was going to wrap both in an object, but streamedMessageRaw is not used, so I was going to delete it all together. I left it for later to decide, and I simply forgot :)
Pending fix merge conflict. |
Closes #66