Skip to content

Fix note attachment deletion on note remove#171

Open
WiXSL wants to merge 12 commits intomainfrom
fix-note-attachment-delete
Open

Fix note attachment deletion on note remove#171
WiXSL wants to merge 12 commits intomainfrom
fix-note-attachment-delete

Conversation

@WiXSL
Copy link
Collaborator

@WiXSL WiXSL commented Feb 17, 2026

Problem

When a user deletes a note with attachments, attachments are not deleted

Solution

Use Supabase dataProvider lifecycle callbacks to delete attachment files from Storage when a note is deleted (contact_notes and deal_notes).
Add tests + manual verification to confirm note deletion also removes the attachment file.

How To Test

1- Create a note with one attachment.
2- Open the attachment link and confirm it works.
3- Delete the note and wait for undo timeout.
4- Open/refresh the same attachment link again.

Before Solution:
5- The file still loads.

After Solution:
5- It now fails (object not found).

Additional Checks

  • Added tests
  • The documentation is up to date
    - [ ] Tested with fakerest provider (see related documentation)
  • Tested with Mobile resolution

@WiXSL WiXSL added the RFR label Feb 17, 2026
@WiXSL WiXSL changed the title Fix notes attachments delete Fix note attachment deletion on note remove Feb 17, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a bug where attachments were not being deleted when notes were deleted. It adds lifecycle callbacks to automatically delete attachment files from Supabase Storage when contact_notes or deal_notes are deleted.

Changes:

  • Added deleteNoteAttachments function to delete attachment files from storage before deleting notes
  • Implemented extractAttachmentPaths and extractAttachmentPath helper functions to extract storage paths from attachment objects
  • Added beforeDelete lifecycle callbacks for contact_notes and deal_notes resources
  • Refactored hardcoded "attachments" bucket name to use ATTACHMENTS_BUCKET constant
  • Added comprehensive unit tests for the attachment deletion functionality

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
src/components/atomic-crm/providers/supabase/dataProvider.ts Added attachment deletion logic, helper functions, lifecycle callbacks, and refactored bucket name to constant
src/components/atomic-crm/providers/supabase/dataProvider.spec.ts Added unit tests for note attachment deletion scenarios
registry.json Registered the new test file in the component registry

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +263 to +264
beforeDelete: async (params, dataProvider, resource) =>
deleteNoteAttachments(params, dataProvider, resource),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I found a potential issue: if you delete a Deal (or a Contact I guess), then all the related notes are deleted, but in that case the attachments deletion is not triggered!

I believe that's because this behavior is not using dataProvider lifecycle callbacks, but Postgres CASCADE DELETE feature.

So IMO we have 2 options:

  • Either acknowledge this limitation, keeping in mind that some stale storage items might remain
  • Or move the attachments deletion logic to the DB, using for instance a trigger, which would solve all cases and also solve the transactional problem raised by Copilot. But this solution is a bit more costly of course.

@fzaninotto wdyt?

@WiXSL WiXSL force-pushed the fix-note-attachment-delete branch from 6420daf to 0630338 Compare February 19, 2026 17:23
@WiXSL WiXSL force-pushed the fix-note-attachment-delete branch from 94dea24 to 71e7454 Compare February 19, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments