Skip to content

Add "Delete" button for users to be able to delete files in copilot-chat-app sample - #89

Closed
kenzabenjelloun wants to merge 12 commits into
microsoft:mainfrom
kenzabenjelloun:delete_feature
Closed

Add "Delete" button for users to be able to delete files in copilot-chat-app sample #89
kenzabenjelloun wants to merge 12 commits into
microsoft:mainfrom
kenzabenjelloun:delete_feature

Conversation

@kenzabenjelloun

@kenzabenjelloun kenzabenjelloun commented Aug 2, 2023

Copy link
Copy Markdown

Hello !

I was trying to add a "delete" button in the copilot-chat-app sample to be able to delete files, it seems to work but I don't know if I'm doing it right. I am still a beginner in building webapps, so I was wondering if I could get some help here. (I don't know if a pull request is the best way to do it ? but I thought it might be a nice feature to add to the sample). I made a couple of changes in DocumentsTab and useFile and added DocumentDeleteService and DocumentDeleteController, if someone can help me pinpoint where I might be doing something wrong, I would be really grateful.

Thank you !

@github-actions github-actions Bot added webapp Pull requests that update Typescript code webapi Pull requests that update .net code PR: ready for review labels Aug 2, 2023
@kenzabenjelloun kenzabenjelloun changed the title add delete button Add "Delete" button for users to be able to delete files in copilot-chat-app sample Aug 2, 2023
Comment thread webapi/appsettings.json Outdated
@glahaye

Copy link
Copy Markdown
Contributor

Teresa Hoang (@teresaqhoang) Tao Chen (@TaoChenOSU) I think you may be working on something similar already?

Comment thread webapi/appsettings.json Outdated
@TaoChenOSU

Copy link
Copy Markdown
Contributor

Hello kenzabenjelloun,

Thank you for your PR!

This is a very nice feature to add, and we are happy that you are considering it.

Document handling is an essential part of the chat copilot app. Each imported document will generate data in 3 places:

  1. The memory source repository
  2. The chat message repository
  3. The memory storage (in the form of embeddings)

To completely remove a document, we need to remove the date generated in all these 3 places. This PR only removes the memory sources. We encourage you to add implementation to complete the deletion of documents.

P.s. To delete a resource, you should use the delete request: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/DELETE.

@crickman Chris (crickman) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the document still present in vector database (Document Memory)?

Comment thread webapi/CopilotChat/Controllers/DocumentDeleteController.cs Outdated
Comment thread webapi/CopilotChat/Models/DocumentDeleteForm.cs Outdated
Comment thread webapp/src/libs/services/DocumentDeleteService.ts Outdated
@gitri-ms

Copy link
Copy Markdown
Contributor

Hi kenzabenjelloun - This is a great start! I've added some additional suggestions on top of what my colleagues said earlier.

Are you still interested in pursuing this PR? I just want to check since I haven't seen any updates from you in a couple weeks.

  • If yes - I recommend you review the suggestions and take a look at Tao's comment about how to remove all the document's data
  • If no - no worries! If I don't hear from you within the next couple days, I will go ahead and close this PR. If you want to come back and work on it at a later date, you are welcome to reactivate it.

@gitri-ms Gina Triolo (gitri-ms) added enhancement New feature or request help wanted Extra attention is needed labels Aug 23, 2023
@kenzabenjelloun

Copy link
Copy Markdown
Author

Hi Gina Triolo (@gitri-ms) ! Yes, I would still like to work on it. Can I just get some pointers on when the embeddings are generated in the code ? I'm having a bit of trouble figuring it out (I thought it would be directly after the document is imported but I don't see it in DocumentImportController?)

@crickman

Copy link
Copy Markdown
Contributor

FYI - DocumentImportController is being deleted in this change: #152

At this stage, introducing the feature might be best accomplished after #152 is merged.

Note: Your change and this update may overlap: #193

@gitri-ms

Copy link
Copy Markdown
Contributor

Hi kenzabenjelloun - thank you again for this contribution. However, given the large refactor coming with #152, we are unable to accept this change at this time. For that reason, I am going to close this PR for now.

We may be interested in revisiting it with you once the semantic memory refactor code comes in. However, I do want to note that while we welcome community contributions, we have limited engineering resources to assist with implementation of features that aren't in our project roadmap.

Going forward, for new features like this, we would appreciate if you would first open an issue with us to discuss the feature you want to bring in and its design/prioritization.

Alternatively, you are always welcome to maintain a fork of Chat Copilot and build on it there! I also encourage you to join our Discord community to engage with other developers building on SK and Chat Copilot, as they might have ideas on how to go about implementing new features like this.

Jonny Toombs (jdtoombs) pushed a commit to jdtoombs/chat-copilot that referenced this pull request Apr 10, 2025
…-chat-extension-as-dependency

Reference azure open ai chat extension as dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request help wanted Extra attention is needed webapi Pull requests that update .net code webapp Pull requests that update Typescript code

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

6 participants