-
Notifications
You must be signed in to change notification settings - Fork 445
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(writing assistant): auto-scroll and follow-up #447
feat(writing assistant): auto-scroll and follow-up #447
Conversation
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.
PR Summary
This pull request enhances the Writing Assistant component with follow-up functionality and auto-scrolling, improving user interaction and conversation flow.
- Introduced new
ConversationHistory
component in/src/components/WritingAssistant/ConversationHistory.tsx
for displaying chat history with auto-scrolling - Added streaming functionality for real-time response generation in
WritingAssistant.tsx
- Implemented follow-up text field and modified
getLLMResponse
function to handle message history inWritingAssistant.tsx
- Enhanced
generatePromptString
function inutils.ts
to support continuous interactions and improved context handling
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
{history.map((message) => ( | ||
<div className={`mb-2 rounded-md p-2 ${message.role === 'user' ? 'bg-blue-500' : 'bg-gray-500'}`}> | ||
<p className="font-bold">{message.role === 'user' ? 'You' : 'Assistant'}:</p> | ||
<ReactMarkdown rehypePlugins={[rehypeRaw]} className="markdown-content break-words"> | ||
{convertMessageToString(message)} | ||
</ReactMarkdown> | ||
</div> |
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.
logic: Add a unique 'key' prop to the outer div of the mapped messages to improve React's rendering performance
useEffect(() => { | ||
bottomRef.current?.scrollIntoView({ behavior: 'smooth' }) | ||
}, [history, streamingMessage]) |
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.
style: Consider using a more efficient scrolling method, such as scrolling only when new content is added, to prevent unnecessary scrolling on every render
return ( | ||
<div className="mt-4 overflow-y-auto" style={{ maxHeight: markdownMaxHeight }}> | ||
{history.map((message) => ( | ||
<div className={`mb-2 rounded-md p-2 ${message.role === 'user' ? 'bg-blue-500' : 'bg-gray-500'}`}> |
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.
style: The background color classes 'bg-blue-500' and 'bg-gray-500' might not provide enough contrast with the text. Ensure the color scheme meets accessibility standards
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.
thanks for working on this. looks mostly good to me.
the only thing i think needs improvement is the conversation history:
- it should be rendered inside a reactmarkdown component
- the user message shows the full prompt, not just what they typed in the input field
- i'm not sure about the chat kind of ui in conversation history that shows messages in chat bubbles because this is writing assistant not chat:
- one idea i had for the ui was that instead of chat history, for each follow up we could render it by itself (without showing old follow ups) and then have back and forth navigation buttons at the top of the writing assistant to navigate between past follow ups
Please look at the latest commit. It is a WIP but is it the UI we want? |
yes i think that's what i had in mind. i'd suggest that no need to show the user messages at all and only show assistant message for V1. |
In the latest commit it only shows the assistant message. If this is good then I'll clean things up and finalize it. |
yes it look good to me :) i'd propose to remove the "Assistant: " thing and move buttons (apart from next and previous btns) to the bottom of the component |
please take a look at the latest commit |
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.
lgtm
#442