-
Notifications
You must be signed in to change notification settings - Fork 116
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(assistant): assistant improvements #4214
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
View your CI Pipeline Execution ↗ for commit eb848b2.
☁️ Nx Cloud last updated this comment at |
Size Change: +5.48 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eb848b2:
|
Paste Run #9327
Run Properties:
|
Project |
Paste
|
Branch Review |
chore-remoce-slack-integration-local
|
Run status |
Passed #9327
|
Run duration | 01m 58s |
Commit |
eb848b216e: fix(assistant): paragraph font size inherit
|
Committer | Kristian Antrobus |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
67
|
View all changes introduced in this branch ↗︎ |
@@ -3,7 +3,7 @@ import React from "react"; | |||
|
|||
const Window: React.FC<React.PropsWithChildren> = ({ children }) => { | |||
return ( | |||
<Box display="grid" gridTemplateColumns="400px 1fr" height="100svh" width="100%"> | |||
<Box display="grid" gridTemplateColumns="300px 1fr" height="100svh" width="100%"> |
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.
Found this was taking too much space on medium displays
<AIChatMessageBody {...props}> | ||
{threadMessage.content.length > 0 && | ||
threadMessage.content[0]?.type === "text" && | ||
compiler(threadMessage.content[0].text.value, assistantMarkdownOptions)} |
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.
Had to use compiler because the animated children was causing the component in the library to throw errors. I also added some information to our docs for how to overcome this.
@@ -15,7 +24,7 @@ export const UserMessage: React.FC<{ threadMessage: Message }> = ({ threadMessag | |||
> | |||
You | |||
</AIChatMessageAuthor> | |||
<AIChatMessageBody> | |||
<AIChatMessageBody {...props}> |
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.
Changed so we can pass in props for the body, for this one the size="fullScreen"
@@ -24,7 +24,7 @@ const Assistant: NextPage = () => { | |||
/> | |||
</Head> | |||
<AssistantPage /> | |||
<ReactQueryDevtools initialIsOpen={false} /> | |||
<ReactQueryDevtools initialIsOpen={false} buttonPosition="bottom-left" /> |
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 developer floating button (the palm trees button when running locally) was covering the submit button on chat composer so moved the positioning
)} | ||
<AIChatMessageBody {...props}> | ||
{threadMessage.content.length > 0 && | ||
threadMessage.content[0]?.type === "text" && |
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 Vercel issue where it would sometimes throw the 500 Sarah was talking about was due to deleting a thread the threadMessage.content[0]
was undefined and threw a 500 that type could not be found.
lgtm 🚀 |
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.
@@ -49,21 +55,25 @@ export const AssistantComposer: React.FC<{ onMessageCreation: (message: string, | |||
throw error; | |||
}, | |||
}} | |||
disabled={isLoading} |
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.
question: not necessarily a change for this PR, but do you remember why we didn't add cursor: not-allowed
to the whole Chat Composer Container?
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.
Uh no I don't remember discussing that sorry. We can start a new thread for it in Slack though if you want.
setIsAnimating(true); | ||
}; | ||
|
||
const userScrolled = (): void => setUserInteractedScroll(true); |
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.
nice touch 👏
key={threadMessage.id} | ||
threadMessage={threadMessage} | ||
// Only animate the last message recieved from AI and must be most recent run to avoid reanimating | ||
animated={index === messages.length - 1 && lastActiveRun?.id === threadMessage.run_id} |
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.
question: if the user sends another prompt before the currently animating response is finished animating, sometimes 2 responses end up animating at the same time. probably wouldn't happen super regularly, but in that case, would we want to stop animating the first response, display the whole thing, and only animate one (most recent) response at a time?
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.
Yea, we need the stop animating functionality on the component. Right now we just have animating hooks. @roxanagomez will be including it in the V2 and we should add that logic in.
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.
gotcha, didn't realize we had also talked about stop animating in the case when 2 messages were animating at the same time. cool!
Added various improvements to the Paste AI Assistant.