Add ability to delete messages + start new chat session

[michaelchia]

Description

1. Ability to delete a specific message exchange (human message and its replies).
2. Ability to start a new chat. Same as '/clear' with LLM memory reset

Fixes #616 (comment requesting for the "New Chat" button)

Demo

Delete message

Screen.Recording.2024-09-04.at.9.34.45.AM.mov

Start new chat

Screen.Recording.2024-08-13.at.12.30.05.AM.mov

Implemenation summary

Delete message

• ChatMessageDelete component created for each human message. Similar to ChatMessageMenu for agent messages.
  • ChatHandler object added as a prop to ChatMessages as it needs to be propagated to ChatMessageHeader then to the ChatMessageDelete component.
  • Consequently PendingMessages now also has to add the ChatHandler prop since it required by the ChatMessageHeader component even though it is unused.
• Add an optional 'target' parameter to ClearMessage, which is the human message ID to delete at.
• Create a new request type (in addition of ChatRequest) called ClearRequest which is exactly same schema as
  ClearMessage.
  • This reuses/overloads the ChatHandler.sendMessage to send a new type of message.
  • RootChatHandler.on_message now has to detect and process the new ClearRequest type
    • will broadcast a ClearMessage with the same 'target' as the request.
    • RootChatHandler.broadcast_message now responsible for clearing chat_history and llm_chat_history on receiving a ClearMessage.
    • /clear no longer responsible for clearing of the memory, only will broadcast the ClearMessage and let RootChatHandler.broadcast_message handle the rest.
    • I think RootChatHandler should be responsible for clearing the history as part of overall history management as it is already responsible for adding to the chat_history.
  • tbh ClearRequest looks a bit awkward as is more like the existing 'Message' (has a type parameter) than a 'Request'. Let me know if there is better design for ClearRequest.
• Add reply_to to PendingMessage to allow for clearing of pending message/s attached to the deleted human message.

New Chat

• '+' button on top that sends a ClearRequest without any 'target' parameter.
• RootChatHandler.broadcast_message responsible for calling BaseChatHandler.send_help_message when handling ClearMessage
  • Now ClearChatHandler is only responsible for broadcasting a ClearMessage.

[michaelchia]

Might be failing the screenshot test because of the addition of the '+' in the UI. I am not sure how to verify.

[srdas]

@michaelchia Appreciate the PR and the tool tips '+', 'x', are neat! I saw one unit test is failing and I will also try and chase it down. Seems to be related to galata and playwright which were showing up as erroring in my dev version for the file AIHelper.ts

and it cleared the error when I installed them:

jlpm add -D @jupyterlab/galata
jlpm playwright install

then run ./scripts/install.sh to recompile everything just to make sure.

I am not sure this will fix things but try it and push a fresh commit and see if this works.

I'd be happy to help with the documentation once this is ready.

[michaelchia]

@srdas seems like more stuff started breaking. should i revert those changes?

[srdas]

@srdas seems like more stuff started breaking. should i revert those changes?

Yes, I think that would be best. I'll try and also locate where the issue is after the reversion.

[michaelchia]

From the error, it seems like the screenshot test is failing because of the change in UI.

https://github.com/jupyterlab/jupyter-ai/blob/main/packages/jupyter-ai/ui-tests/tests/jupyter-ai.spec.ts-snapshots/chat-welcome-message-linux.png

This no longer matches because of the new + at the top. I have no idea how to generate the new screenshot as it seems like it has to be pixel perfect. Could you send a new version of the screenshot with the + so that I can replace it?

[dlqqq]

@michaelchia Thank you for opening this PR! The below GitHub comment triggers a workflow that updates the E2E test snapshots. I'm a bit busy wrapping up another PR at the moment. I hope to have this PR reviewed by Friday afternoon PT.

EDIT: Apparently the PR author has to be listed as a maintainer on the repository to run this workflow. I've sent you an invite via email; can you accept it and post the below comment again to update the snapshots?

[dlqqq]

please update snapshots

[srdas]

@michaelchia @dlq Fixed the snapshot issue by adding in the new snapshot manually and now all checks have passed.

1. Tested the 'x' tooltip to make sure it erases chats as intended.
2. Tested the '+' tooltip to see if it removes all chats and starts a new chat.
3. Removing partial chats works in the chat window.
4. After removing partial or full chats the chat history from /export correctly reflects the new state of the chat and does not export erased subchats.
5. Inserted temporary statements in the code to log memory status after all the above actions and the chat memory is updated correctly.

Note: Documentation needs updating. Several screenshots of the chat window in the docs may need updating as the new tooltips are to be updated in all chat images. @dlqqq -- do we change all of them or only add new documentation for this new feature here under Additional chat commands?

[andrii-i]

Note update-snapshot runs: workflow was not successful in updating snapshots, all recent runs just say “This job was skipped”. Nothing critical but worth looking at some point in order not to update snapshots manually which takes longer https://github.com/jupyterlab/jupyter-ai/actions/workflows/update-snapshots.yml

[krassowski]

Note update-snapshot runs: workflow was not successful in updating snapshots, all recent runs just say “This job was skipped”. Nothing critical but worth looking at some point in order not to update snapshots manually which takes longer https://github.com/jupyterlab/jupyter-ai/actions/workflows/update-snapshots.yml

Also saw this one recently here: jupyterlab/jupyterlab#16565 (comment) - where Andrew was a "Member" so it should have passed. I see that @michaelchia is a collaborator so It should have passed too. On the other hand just yesterday it worked well for me here: jupyterlab/jupyterlab#16721 (comment).

For Andrew's PR I think it is because it was open before we updated workflows on July 16th due to GHSA-rc7c-v5qg-g3vw.

Here? No idea. It would be good to create a dummy workflow and print out values for github.event.issue.author_association, github.event.issue.pull_request, and github.event.comment.body just to make sure. Or maybe we should move the association guards from if section which causes skips into the body of the workflow and then print out a useful warning (i.e. useful for debugging).

As per docs the enums we have are fine: https://docs.github.com/en/webhooks/webhook-events-and-payloads#issue_comment

[dlqqq]

@michaelchia Great work! I like how nicely the "+" button for human messages fits in with the "..." button for AI messages. I left some feedback below.

[michaelchia]

please update snapshots

[michaelchia]

reimplemented to delete specific exchanges. updated top comment to reflect that. had to add a reply_to field to PendingMessage to clear the pending messages associated with the deleted message.

[dlqqq]

@michaelchia Just one callout on the BoundedChatHistory implementation, along with a few comments that need updating. This looks almost good-to-go! 👍

[krassowski]

I feel slightly uncomfortable with "+" button being a destructive action (unless I misunderstood). I would prefer to user a different icon for "clear and begin a new chat".

Similarly, I think users would often want to edit a message rather than delete and draft it again. I am not requesting the ability to edit to be added in this PR but just wanted to mention that it might be good to think about where an alternative "edit" button could be placed (in addition to the proposed "delete all below" button). Maybe both of these should bu under vertical three dots in the future?

[michaelchia]

I feel slightly uncomfortable with "+" button being a destructive action (unless I misunderstood). I would prefer to user a different icon for "clear and begin a new chat".

The reason I chose the "+" icon is that it seems to be pretty much standard. It is in quite a few code assistants I've seen (e.g. Github Copilot, Codeium, Cursor).

Good point on it being destructive. However, it would only be destructive until we implement convo/history management #813.

That being said, if you would suggest an alternative icon, I would gladly change it.

Similarly, I think users would often want to edit a message rather than delete and draft it again. I am not requesting the ability to edit to be added in this PR but just wanted to mention that it might be good to think about where an alternative "edit" button could be placed (in addition to the proposed "delete all below" button). Maybe both of these should bu under vertical three dots in the future?

Good suggestion as a future enhancement.

[srdas]

Similarly, I think users would often want to edit a message rather than delete and draft it again. I am not requesting the ability to edit to be added in this PR but just wanted to mention that it might be good to think about where an alternative "edit" button could be placed (in addition to the proposed "delete all below" button). Maybe both of these should bu under vertical three dots in the future?

Good suggestion as a future enhancement.

Agree, this suggestion may also be bundled with the ability to use the up arrow to get previous queries and edit them.

[michaelchia]

I've refactored backend to support "delete all below" (and consequently "edit", which is "delete all below" + submit). I did not implement it in the UI. I'll leave that for someone else to implement as I am not sure about the design for that. This just lays the groundwork to easily do so.

[dlqqq]

@michaelchia Awesome work! 🎉

As Mike pointed out, we may want to change the icon & messaging associated with the "New chat" button in the future. However, I don't think that needs to be a blocker for this PR, which you have worked hard on.