Fix: deletion and name updates of attachments in body reflected in Files #13169

omarNaifer12 · 2025-07-10T22:48:40Z

This PR fixes an issue where the onChange trigger was deleting all attachments by removing the JWT token from their paths (if it existed) and comparing the new body with the old body to identify deleted attachments. It ensures that only attachments actually removed from the body get deleted, preventing unintended deletion of attachments not added directly through the body. It also handles updating attachment names in the body so changes are reflected in Files, with related tests updated accordingly.

ByStatus-Tasks-Personal-MicrosoftEdge2025-07-1022-29-26-ezgif.com-crop-video.mp4

greptile-apps

PR Summary

Fixes critical attachment handling issues in the activity rich text editor by improving path comparison and name update functionality.

Fixed unintended attachment deletion in ActivityRichTextEditor.tsx by properly handling JWT tokens in file paths during onChange events
Implemented attachment name synchronization between rich text editor and Files through new getActivityAttachmentIdsAndNameToUpdate.ts utility
Added path restoration logic in getActivityAttachmentPathsToRestore.ts to prevent accidental deletions when editing content
Consolidated attachment handling by renaming getActivityAttachmentPaths.ts to getActivityAttachmentPathsAndName.ts with enhanced functionality
Extended test coverage with new test files specifically for attachment path handling and name updates

_{13 files reviewed, 19 comments}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-07-10T22:49:11Z

...twenty-front/src/modules/activities/utils/__tests__/getActivityAttachmentIdsToDelete.test.ts

+    const newActivityBody = JSON.stringify([
+      {
+        type: 'file',
+        props: { url: '/files/images/test.txt' },
+      },
+      {
+        type: 'file',
+        props: { url: '/files/images/test2.txt' },
+      },
+    ]);


style: Test data construction could be moved to a shared test helper function since it's duplicated in both test cases.

greptile-apps · 2025-07-10T22:49:12Z

...nty-front/src/modules/activities/utils/__tests__/getActivityAttachmentPathsToRestore.test.ts

    const oldActivityAttachments = [
      {
        id: '1',
-        fullPath: '/files/attachment/test.txt',
+        fullPath: '/files/images/test.txt',
      },
    ] as Attachment[];


style: Consider making test data reusable by moving it to a test fixtures file since it's used in multiple test files

greptile-apps · 2025-07-10T22:49:13Z

packages/twenty-front/src/modules/activities/utils/__tests__/filterAttachmentsToRestore.test.ts

    const attachmentIdsToRestore = filterAttachmentsToRestore(
-      ['attachment/test.txt'],
+      ['images/test.txt'],
      softDeletedAttachments,


logic: Test path is inconsistent with initial fullPath. Using 'images/test.txt' vs '/files/images/test.txt' might cause path matching issues in production.

packages/twenty-front/src/modules/activities/components/ActivityRichTextEditor.tsx

greptile-apps · 2025-07-10T22:49:13Z

packages/twenty-front/src/modules/activities/utils/getActivityAttachmentIdsToDelete.ts


 export const getActivityAttachmentIdsToDelete = (
  newActivityBody: string,
  oldActivityAttachments: Attachment[] = [],
+  oldActivityBody: string,


style: New required parameter 'oldActivityBody' should have a type annotation for consistency

packages/twenty-front/src/modules/activities/utils/getAttachmentPath.ts

packages/twenty-front/src/modules/activities/utils/getActivityAttachmentIdsAndNameToUpdate.ts

greptile-apps · 2025-07-10T22:49:24Z

packages/twenty-front/src/modules/activities/utils/getActivityAttachmentIdsAndNameToUpdate.ts

+  return activityAttchmentsNameAndPaths.reduce(
+    (acc: Partial<Attachment>[], activity: AttachmentInfo) => {
+      const foundActivity = oldActivityAttachments.find(
+        (attchment) => getAttachmentPath(attchment.fullPath) === activity.path,
+      );
+      if (isDefined(foundActivity) && foundActivity.name !== activity.name) {
+        acc.push({ id: foundActivity.id, name: activity.name });
+      }
+      return acc;
+    },
+    [],


style: Consider extracting this reduce operation into a separate utility function to improve readability and maintainability

github-actions · 2025-07-10T23:26:41Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:12190

This environment will automatically shut down when the PR is closed or after 5 hours.

lucasbordeau

LGTM nice work

lucasbordeau

@omarNaifer12 Could you please handle the case of embedded media ? Also please split the utils, because the code would be more easy to maintain with small utils, one for parsing and Attachment fullPath, one for parsing a URL.

…on in Files and update related tests

omarNaifer12 · 2025-07-17T12:55:06Z

Thanks @lucasbordeau for catching this case. I added the compareUrls function to check if two URLs have the same domain and path.

prastoin · 2025-07-21T15:28:39Z

Hey @lucasbordeau could you please re-review this PR when you have some free time please ?

lucasbordeau

LGTM

github-actions · 2025-07-23T12:37:25Z

Thanks @omarNaifer12 for your contribution!
This marks your 12th PR on the repo. You're top 3% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

…les (#13169) resolve #13168 , #8501 This PR fixes an issue where the onChange trigger was deleting all attachments by removing the JWT token from their paths (if it existed) and comparing the new body with the old body to identify deleted attachments. It ensures that only attachments actually removed from the body get deleted, preventing unintended deletion of attachments not added directly through the body. It also handles updating attachment names in the body so changes are reflected in Files, with related tests updated accordingly. https://github.com/user-attachments/assets/8d824a24-b257-4794-942e-3b2dceb9907d --------- Co-authored-by: Lucas Bordeau <bordeau.lucas@gmail.com>

omarNaifer12 · 2025-07-26T00:13:53Z

@lucasbordeau I have a quick question: this PR syncs body changes to files. If we update or remove directly from files, does it also reflect in the body?

lucasbordeau · 2025-07-28T10:21:55Z

@omarNaifer12 Indeed we could also handle that the other way around, if that's not too complex with the existing utils ?

omarNaifer12 · 2025-07-29T13:51:22Z

I did a small investigation, and I think we might only need to use the compareUrls utility and apply filtering to the entire block note body.We also need to extract the record (either task or note) that is opened on the record page or in the side panel.

lucasbordeau · 2025-07-29T14:09:10Z

Ok, from what I know, that seems a bit nice to have for now, you could certainly find something more important to contribute to.

greptile-apps bot reviewed Jul 10, 2025

View reviewed changes

lucasbordeau self-requested a review July 16, 2025 13:53

lucasbordeau approved these changes Jul 16, 2025

View reviewed changes

lucasbordeau enabled auto-merge (squash) July 16, 2025 15:31

lucasbordeau disabled auto-merge July 16, 2025 15:32

lucasbordeau requested changes Jul 16, 2025

View reviewed changes

lucasbordeau self-assigned this Jul 17, 2025

charlesBochet added the -PR: awaiting author label Jul 17, 2025

omarNaifer12 added 8 commits July 17, 2025 10:24

Fix deletion and name updates of attachments in body; ensure reflecti…

2c268e3

…on in Files and update related tests

fix lint

f56891f

fix

aee0050

fix

561b1d3

fix: move loop inside useRecoilCallback

743fb14

fix

b89f8e9

add compareUrls function to handle embedded media case

28b8c35

Update existing tests and add tests for compareUrls function

3b462c6

omarNaifer12 force-pushed the fix/attachment-deletion branch from 37e3e34 to 3b462c6 Compare July 17, 2025 12:48

FelixMalfait added -PR: awaiting review and removed -PR: awaiting author labels Jul 18, 2025

prastoin requested a review from lucasbordeau July 21, 2025 15:28

Merge branch 'main' into fix/attachment-deletion

b62a620

lucasbordeau approved these changes Jul 23, 2025

View reviewed changes

lucasbordeau merged commit dd5ae66 into twentyhq:main Jul 23, 2025
51 checks passed

omarNaifer12 deleted the fix/attachment-deletion branch July 24, 2025 11:39

Fix: deletion and name updates of attachments in body reflected in Files #13169

Fix: deletion and name updates of attachments in body reflected in Files #13169

Uh oh!

Conversation

omarNaifer12 commented Jul 10, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucasbordeau left a comment

Choose a reason for hiding this comment

Uh oh!

lucasbordeau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarNaifer12 commented Jul 17, 2025

Uh oh!

prastoin commented Jul 21, 2025

Uh oh!

lucasbordeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 23, 2025

Uh oh!

omarNaifer12 commented Jul 26, 2025

Uh oh!

lucasbordeau commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omarNaifer12 commented Jul 29, 2025

Uh oh!

lucasbordeau commented Jul 29, 2025

Uh oh!

Uh oh!

github-actions bot commented Jul 10, 2025 •

edited

Loading

lucasbordeau left a comment •

edited

Loading

lucasbordeau commented Jul 28, 2025 •

edited

Loading