-
Notifications
You must be signed in to change notification settings - Fork 8
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: add chat interaction UI #152
feat: add chat interaction UI #152
Conversation
This is ready for review @sehyod |
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.
Thank you for this PR and for the work to add custom tailwind components!
Very exciting! We're getting close to the MLP. I hit an error when I tried to chat before uploading any references:
I was able to get this working with a PDF I uploaded though, very cool! |
That's the expected behaviour if you didn't uploaded PDFs. I forgot to mention that in the PR description. |
Rather than the unfriendly error message can we say something more useful about needing to upload PDFs to chat? It's not clear to me that's even the right behavior. We should probably just mention to the user they can upload PDFs to improve the chat experience and pass through their chat to get a response. |
That's a good point Jeff. In the frontend we can't distinguish between errors in the backend. You can have this one but also an API Rate limit, or invalid API Key, .. I think that we should add (reply error codes) to the #144 and then improve the UI on top of that. |
Ah, perhaps we should file a broader issue for better sharing of error state between the frontend and backend? |
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.
A few more comments. I got this to work in my local client.
|
||
export async function askForRewrite(selection: string): Promise<string> { | ||
try { | ||
const response = await callSidecar('rewrite', ['--text', String(selection)]); |
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 String()
wrapper here isn't necessary (if the TS types are to be believed!).
// 'Hidden feedback loops in machine learning refer to situations where two systems indirectly influence each other through the world, leading to changes in behavior that may not be immediately visible. These loops may exist between completely disjoint systems and can make analyzing the effect of proposed changes extremely difficult, adding cost to even simple improvements. It is recommended to look carefully for hidden feedback loops and remove them whenever feasible.', | ||
// }, | ||
]); | ||
const [currentChatThreadItem, setCurrentChatThreadItem] = useState<ChatThreadItem | null>(null); |
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.
Do you want to move the thread state into atoms? Presumably the interactions will grow more elaborate in the future and we'll want it to persist even if you close the component. Alternatively the thread state could live in the backend.
]); | ||
const [currentChatThreadItem, setCurrentChatThreadItem] = useState<ChatThreadItem | null>(null); | ||
|
||
function handleKeyDown(evt: React.KeyboardEvent<HTMLTextAreaElement>): void { |
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.
Nit: more concisely written as:
const handleKeydown: React.KeyboardEventHandler = (evt) => {
} | ||
setCurrentChatThreadItem({ id: String(chatThread.length + 1), question: text }); | ||
setText(''); | ||
chatWithAI(question) |
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.
Suggest using async
/ await
.
setText(''); | ||
chatWithAI(question) | ||
.then((reply) => { | ||
setChatThread([...chatThread, { id: String(chatThread.length + 1), question: text, answer: reply[0] }]); |
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.
There may be a stale closure issue here if you send two chats before the first one comes back. Safer to use the function form of setState
:
setChatThread(prevChatThread => [...prevChatThread, { id: String(prevChatThread.length + 1), question: text, answer: reply[0] }]);
(also below)
expect(screen.getByText('Will this test pass?')).toBeInTheDocument(); | ||
expect(screen.getByText('...')).toBeInTheDocument(); | ||
|
||
resolveFn(); |
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 test!
@@ -2,26 +2,26 @@ import { render, screen, userEvent } from '../utils/test-utils'; | |||
import { PrimarySideBar } from './PrimarySideBar'; | |||
|
|||
describe('PrimarySideBar', () => { | |||
it('should display Explorer and References icons', () => { | |||
test('should display Explorer and References icons', () => { |
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 think I left a comment about this elsewhere, but when we write "should" in a test description we should use it
rather than test
to make it read more clearly (I pretty much always use it
) https://stackoverflow.com/questions/45778192/what-is-the-difference-between-it-and-test-in-jest
This PR closes #148 by adding a UI for chat interaction.
Note that currently the backend don't support threads and history (will be fixed by #144) so this PR adds that as a UI state in order to experiment the interaction pattern.