Skip to content
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

Cody: Improve chat history #52904

Merged
merged 14 commits into from
Jun 12, 2023

Conversation

deepak2431
Copy link
Contributor

@deepak2431 deepak2431 commented Jun 5, 2023

This PR closes sourcegraph/cody#339, #52848, #52889 and #52890.

The issues mostly overlapped, so I handled it in this PR.

Changes made:

  1. Add history to the history list on each new conversation started.
  2. Display the last human message in history instead of the assistant message.
  3. Add a delete button to delete individual chats from history.
  4. Load the most recent chat, if available, from the history.
  5. Fix the loading of context files.
  6. Load the history messages to the end of the scroll position.

Test plan

All E2E tests have passed.
https://www.loom.com/share/3dca98dd0db94c298df38ab79c5e72d1

@cla-bot cla-bot bot added the cla-signed label Jun 5, 2023
@deepak2431
Copy link
Contributor Author

@abeatrix @toolmantim Requesting review for this PR.

@abeatrix
Copy link
Contributor

abeatrix commented Jun 5, 2023

@deepak2431 Thanks for the ping! I have tagged my team for review and we will get to this asap, thank you so much for the PR ❤️

@deepak2431
Copy link
Contributor Author

@deepak2431 Thanks for the ping! I have tagged my team for review and we will get to this asap, thank you so much for the PR ❤️

Thanks a lot, @abeatrix. I was looking to add more E2E tests for this. Just a question, how to run the extension in browser mode so I can look over the proper selectors to pick and attach events to it?
Or is there a different way to debug extensions for getting the selectors to attach test events?

@deepak2431 deepak2431 changed the title Cody: Chat History Cody: Improve chat history Jun 6, 2023
@abeatrix
Copy link
Contributor

abeatrix commented Jun 6, 2023

@deepak2431 if you start the e2e test in debug mode and then hit record in the pop up console, it will show you the name of each selector you hover or click on (all thanks to @philipp-spiess 🙏 )

@deepak2431
Copy link
Contributor Author

deepak2431 commented Jun 6, 2023

all thanks to @philipp-spiess 🙏 )

@abeatrix Thanks, Got that. This works fantastic as it captures most of it during the record.

@deepak2431
Copy link
Contributor Author

@abeatrix Added the tests. It was fun to write tests by recording the events in sequential order to get the selectors.

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments inline. The new behavior makes a lot more sense to me so thank you so much for working on this 🙇
In addition, the Clear History button is still clickable if you remove the history item one by one. I tried updating it to disabled={!userHistory || !Object.keys(userHistory).length} and it fixed it for me :)

client/cody/webviews/UserHistory.tsx Outdated Show resolved Hide resolved
client/cody/webviews/UserHistory.tsx Show resolved Hide resolved
client/cody/webviews/UserHistory.module.css Outdated Show resolved Hide resolved
client/cody/webviews/UserHistory.module.css Outdated Show resolved Hide resolved
client/cody/src/chat/ChatViewProvider.ts Show resolved Hide resolved
client/cody/webviews/UserHistory.tsx Outdated Show resolved Hide resolved
@deepak2431
Copy link
Contributor Author

Left some comments inline. The new behavior makes a lot more sense to me so thank you so much for working on this 🙇 In addition, the Clear History button is still clickable if you remove the history item one by one. I tried updating it to disabled={!userHistory || !Object.keys(userHistory).length} and it fixed it for me :)

Thanks a lot for the review @abeatrix, and for thoroughly testing it. I will make all the requested changes here soon.

@deepak2431 deepak2431 requested a review from abeatrix June 7, 2023 11:10
@abeatrix
Copy link
Contributor

abeatrix commented Jun 7, 2023

Approved! Once the conflict is resolved, we can merge this 🎉 thank you so much for the great work!

@deepak2431
Copy link
Contributor Author

Approved! Once the conflict is resolved, we can merge this 🎉 thank you so much for the great work!

Thanks a lot, @abeatrix, for the complete review. I have resolved the conflicts so all good to be merged now!

@abeatrix
Copy link
Contributor

abeatrix commented Jun 8, 2023

@deepak2431 i tried to merge but your PR is not passing the e2e tests 🤔

@deepak2431
Copy link
Contributor Author

@abeatrix Hmm, not sure, as it's passing all of them here 👀
iScreen Shoter - Code - 230608131158

@toolmantim
Copy link
Contributor

@deepak2431 I think #53224 should fix the build errors. Can you merge main into this branch, so we can see? Thanks!

@deepak2431
Copy link
Contributor Author

@deepak2431 I think #53224 should fix the build errors. Can you merge main into this branch, so we can see? Thanks!

@toolmantim I have merged the main and updated the integration test, which I saw now was breaking because we can now load the last chat. So, we need to clear the history after each test gets completed.

All the E2E tests are passing locally.
All integration tests have passed, too, except the Local file finder one.

I hope now the build will succeed as in the logs it shows E2E tests failing, but I'm not sure why!

@deepak2431
Copy link
Contributor Author

@philipp-spiess @abeatrix @toolmantim Thanks a lot. Finally, I can see that the required failing build is successful in this PR!

@philipp-spiess
Copy link
Contributor

@deepak2431 🎉

Yeah it seems like there's something flaky with the inline assist tests but it passed after the retry. 🤔 (cc @abeatrix) we should try and find out why that's happening (it seems to only be running into this issue with the inline assist tests)

This is unrelated to your changes though so let's land these 🚀

@philipp-spiess philipp-spiess merged commit b17c0d6 into sourcegraph:main Jun 12, 2023
22 of 23 checks passed
ErikaRS pushed a commit that referenced this pull request Jun 22, 2023
This PR closes #52544,  #52848, #52889 and #52890.

The issues mostly overlapped, so I handled it in this PR.

Changes made:
1. Add history to the history list on each new conversation started.
2. Display the last human message in history instead of the assistant
message.
3. Add a delete button to delete individual chats from history.
4. Load the most recent chat, if available, from the history.
5. Fix the loading of context files.
6. Load the history messages to the end of the scroll position.

## Test plan

All E2E tests have passed.
https://www.loom.com/share/3dca98dd0db94c298df38ab79c5e72d1

---------

Co-authored-by: Philipp Spiess <hello@philippspiess.com>
@deepak2431 deepak2431 deleted the deepak/chat-history branch June 27, 2023 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cody: VSCode history should list the current chat
4 participants