feat: add chat interaction UI#152
Conversation
|
This is ready for review @sehyod |
sehyod
left a comment
sehyod
left a comment
There was a problem hiding this comment.
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? |
sergioramos
changed the title
danvk
left a comment
danvk
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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(); |
|
|
||
| describe('PrimarySideBar', () => { | ||
| it('should display Explorer and References icons', () => { | ||
| test('should display Explorer and References icons', () => { |
There was a problem hiding this comment.
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
5 participants
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.